HarperFast / HarperFast/harper
uWS backend (#1096): silent onUpgrade auth bypass, unconditional loopback IP on delegated routes, unbounded static-file buffering
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
## Summary
Exploratory QA against the new opt-in uWebSockets.js HTTP/WebSocket backend (#1096, `HARPER_UWS_UDS`/`HARPER_UWS_HTTP`, merged `bd1dce0b1`) found three independent, confirmed gaps — two security-shaped, one a resource-exhaustion vector. Both flags are default-off, so there's no live production exposure today, but all three are concrete gaps an operator would hit immediately upon opting in for the throughput win the PR advertises. Grouping them here since they're all on the same feature, found the same day, and likely worth reviewing together before any default flip or staging rollout.
All three reproduced on `bd1dce0b1`, `npm run test:integration` on each linked test file.
---
## 1. `onUpgrade` pre-handshake auth gate is silently inert under uWS
The PR's own text discloses that custom `onUpgrade` (`server.upgrade()`) listeners "aren't mirrored yet" under uWS — confirmed in code: `server/serverHelpers/uwsServer.ts` never touches `onUpgrade`/`upgradeChains` at all. The concrete, exercised consequence is worse than "a feature isn't implemented yet": **it's a silent bypass with zero operator-facing signal.**
Repro: a component registers `server.upgrade((request, socket, head, next) => {...})` rejecting any WS handshake missing a custom `X-App-Token` header (401 + `socket.destroy()`), backed by a real `server.ws(...)` handler.
- **Node backend (control):** header-less client → correctly rejected (401, socket closed). Header-present client → connects.
- **uWS backend (`HARPER_UWS_HTTP=1`):** header-less client → **connects successfully anyway**, receives the `welcome` message the gate should have blocked.
- Checked both startup output and continuous runtime logs across the whole run: **no signal anywhere** that a registered `onUpgrade` listener is incompatible with the active backend.
Root cause: `onWebSocket`'s `uws` branch (`server/http.ts` ~1340-1367) wires the WS handler straight into uWS's native `app.ws()`, bypassing `upgradeChains` entirely.
Any app using `server.upgrade()` for its own pre-handshake authorization (IP allowlisting, custom pre-auth token check, etc. — a documented API, not an edge case) silently loses that protection the moment uWS is enabled.
Test: `integrationTests/qa-scratch/qa531-uws-onupgrade-bypass.test.ts` (drives both backends in one run).
---
## 2. Legacy Fastify-delegated routes see an unconditional `127.0.0.1` client IP under uWS
The PR states an explicit security property for the native path: "reads the real client IP from the TCP peer and only trusts `X-Forwarded-For` on the UDS path, so a direct client can't spoof loopback to satisfy local auth." This holds for the **native** httpChain path — but routes that fall through to the new `injectToFastify()` bridge (added by this same PR, for legacy Fastify-registered custom-function routes) get a mocked request whose IP is **always `127.0.0.1`**, for every real client, with **no forged header required** — this isn't spoofing, it's a permanently wrong value.
Repro: two "local-admin-only" IP-gate routes side by side — one served by the native httpChain, one an unmatched legacy `fastifyRoutes` route (confirmed via a marker that it genuinely hit the delegation bridge, not the native path). Client explicitly bound its outbound socket to a distinct loopback address so a real, non-matching peer IP could be constructed.
- Native path: correct real peer IP on both Node and uWS backends; forged `X-Forwarded-For` never trusted.
- Delegated path under uWS: **unconditionally `127.0.0.1`**, `admitted:true`, even with zero headers forged, for a real peer at a different address.
Root cause: `injectToFastify()` (`server/http.ts` ~1100-1116) forwards headers to `fastify.inject()` but never passes a `remoteAddress` option. Fastify v5's `fastify.inject()` is backed by `light-my-request`, whose `MockSocket` hardcodes `remoteAddress = options.remoteAddress || '127.0.0.1'` when unset; Harper never sets `trustProxy`, so `request.ip` just returns that hardcoded value.
Any app relying on a legacy Fastify-registered route for local-admin/IP-allowlist gating is unconditionally, permanently bypassed the moment uWS is enabled — no attacker action needed.
Suggested fix: `injectToFastify()` should pass `remoteAddress: request.ip` (the real value already resolved on the native request before delegation) through to `fastify.inject()`'s options.
Test: `integrationTests/qa-scratch/qa533-uws-fastify-delegate-ip-trust.test.ts`.
---
## 3. Static-file serving under uWS buffers 2.5-3.4× the file size in peak RSS, uncapped under concurrency
The PR discloses "static responses are buffered, not streamed" as a known tradeoff for typical assets. Measurement shows this understates the severity: it isn't one buffered copy per request, and the per-request cost multiplies with concurrency with no cap anywhere in the path.
Measured (byte-exact transfers throughout, no crashes — a resource curve, not a functional break):
| Test | Node RSS Δ | uWS RSS Δ | Node latency | uWS latency |
|---|---|---|---|---|
| 50MB file | 46MB | 123MB (~2.5×) | 124ms | 215ms |
| 200MB file | 0.2MB (streamed) | 670MB (~3.35×) | 267ms | 1330ms |
| 500MB file | 41MB | 1523MB (~3×) | 580ms | 4611ms |
| 5× concurrent 500MB | 20MB total | 6932MB (2.77× a naive "5 full copies" estimate) | 2.66s wall | 21.8s wall |
| 10× concurrent 500MB | 3.5MB total | 8905MB (peak RSS ~14.3GB) | 5.36s wall | 41.4s wall |
Root cause: `bufferSendStream()` (`server/http.ts`, reached only via `makeUwsHandler`) drains the `send`-package `SendStream` into a `chunks: Buffer[]` and `Buffer.concat()`s it before responding — no size cap, no streaming fallback for large files.
On a typical 2-8GB container, a handful of concurrent requests to one moderately-large **unauthenticated** static asset (no auth required for static serving) is enough to threaten OOM. Memory also didn't fully drain between bursts in testing — sustained moderate load ratchets the high-water mark upward, not just spike-and-recover.
Suggested fix: stream static responses through uWS (the PR's own stated eventual direction) before considering a default flip, or add a size/concurrency-aware cap as a stopgap.
Test: `integrationTests/qa-scratch/qa532-uws-static-buffering.test.ts` (self-generates/self-deletes large fixtures; standalone regen helper `generate-large-fixture.mjs` retained).
---
## Recommendation
Recommend prioritizing #1 (auth bypass) and #2 (IP-trust bypass) over #3 (resource curve) given the security shape, but all three are worth resolving — or at minimum documenting loudly with a startup-time warning — before uWS graduates past default-off or gets exercised on a staging cluster with real traffic.
— KrAIs 🤖 (exploratory QA, on Kris's behalf)
Contributor guide
Research direction
Start with server/http.ts and server/serverHelpers/uwsServer.ts, then run the three named QA tests: qa531-uws-onupgrade-bypass.test.ts, qa533-uws-fastify-delegate-ip-trust.test.ts, and qa532-uws-static-buffering.test.ts. Trace the uWS upgrade, Fastify delegation, and bufferSendStream paths. Done means the regression tests pass with pre-handshake authorization and client IP preserved, while static responses no longer exhibit the reported unbounded buffering behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- backend, performance, security, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100