Skip to content

feat(static)!: security hardenings and general improvements - #252

Merged
pi0 merged 12 commits into
mainfrom
fix/static-sec
Jul 17, 2026
Merged

feat(static)!: security hardenings and general improvements#252
pi0 merged 12 commits into
mainfrom
fix/static-sec

Conversation

@pi0x

@pi0x pi0x commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

serveStatic is a public export (srvx/static) with no test coverage. This adds a suite (111 tests) and fixes what writing it surfaced. Findings are backported from h3's serveStatic and from #233, adapted to srvx's filesystem-backed model.

Each fix has a test that fails without it — 66 of the 111 fail against main's static.ts.

Symlink escape

stat() follows symlinks, but the only containment check was lexical (filePath.startsWith(dir)), which cannot see through a link. Any symlink inside the root served any file on the host:

SERVED  /link.env          -> ../secret/creds.env   "TOPSECRET"
SERVED  /linkdir/creds.env -> ../secret/            "TOPSECRET"

Containment is now re-asserted against the realpath-resolved path, which also covers links in intermediate segments. h3 delegates this to the integrator; srvx is the integrator, so it owns the check.

Two details: both sides are resolved, since a legitimately symlinked dir (/var/www -> /data/www) would otherwise reject every file; and only escaping links are blocked — links resolving inside the root still work, as dist/pnpm trees rely on them and nginx follows symlinks by default.

The lexical startsWith(dir) check stays, but it is a cheap pre-filter, not the boundary: it rejects an obvious escape without a syscall and makes the relative path meaningful for the dotfile check. isContained is what actually holds. Nothing distinguishes the two in tests — removing the pre-filter alone leaves the suite green — so the source says so explicitly.

Dotfiles

Only bare dotfiles were protected, and only by accident: extname(".env") returns "" (POSIX treats a leading dot as a hidden-file marker), so .env was probed as .env.html and missed. Anything with a real extension was served:

path before after
/.env 404 404
/.env.production served 404
/sub/.env.local served 404
/.git/config.txt served 404

dotfiles is now boolean | string[] and denies dot segments by default — with one carve-out. RFC 8615 reserves /.well-known/ for public metadata: ACME HTTP-01 challenges, security.txt, assetlinks.json. Behind an all-or-nothing opt-in, renewing a certificate would require publishing .env and .git/config — the exact files the default exists to hide, and a break that surfaces ~60 days after upgrade, at renewal. So the default is [".well-known"]:

dotfiles /.well-known/acme-challenge/<token> /.env
[".well-known"] (default) served 404
true served served
false / [] 404 404
[".git"] 404 404 — and /.git/config.txt served

Matching is by exact segment, so [".well-known"] exposes neither a sibling sharing its prefix (.well-known-backup) nor a dot segment nested under it (.well-known/.env) — both pinned by tests. Every dot segment in a path must be allow-listed, not just the first.

The check runs on the post-join relative path, so resolved ../. segments aren't mistaken for dotfiles (/sub/../index.html -> index.html, pinned by a test). It also runs after percent-decoding, so /%2Eenv is subject to the same policy as /.env, and /%2Ewell-known/security.txt to the same allow-list as /.well-known/security.txt.

Percent-decoding

The pathname was never decoded: url.pathname keeps the wire encoding (h3 decodes only its own event.url clone, never req._url), and static.ts joined it into the filesystem path verbatim. Any file whose name a client must percent-encode — a space, any non-ASCII character, a literal % — was unreachable:

request before after
/hello%20world.txt 404 serves hello world.txt
/caf%C3%A9.txt 404 serves café.txt
/50%25.txt 404 serves 50%.txt
/sub%2Finside.txt 404 404 — %2F never becomes a separator
/%252e%252e/… 404 404 — no double decode
/foo%, /%ZZ 404 400 — malformed encoding, as nginx/serve-static answer

The path is decoded exactly once with decodeURI — not decodeURIComponent — so %2F/%3F/%23 stay encoded and an encoded separator never becomes a real one. This is the invariant h3's static util documents as its traversal contract, and a single decode from the wire form produces the same net lookup as h3's two-stage decode (event layer + lookup peel).

Compression

createBrotliCompress() ran at BROTLI_DEFAULT_QUALITY = 11 — the maximum — per request, with no cache. An unauthenticated client could amplify cheap requests into heavy CPU work, including via HEAD, whose body Node discards anyway.

The vector is the quality, not the feature. An earlier revision of this PR removed on-the-fly compression outright; it is back, bounded by the two things that actually made q11 dangerous:

Quality 4, not 11 — roughly 12x less CPU for a few percent of size, which is what CDNs encode at dynamically. It matters beyond the request's own latency: node:zlib streams run on the same 4-thread libuv pool as every stat/open in this file, so an over-tuned quality stalls the filesystem work too — the same pool the O_NONBLOCK commit on this branch exists to protect. gzip stays at zlib's default level 6, an order of magnitude cheaper than brotli at any quality. BROTLI_PARAM_SIZE_HINT is passed from the fstat'd size, which is known exactly and never a guess.

Bounds of 1 KiB to 10 MiB — compression inverts what normally makes a large file self-limiting: the response gets smaller while the server burns CPU proportional to the uncompressed size, so a cheap request stops paying for itself in bandwidth. Below 1 KiB the encoded body can come out larger than the input. Above 10 MiB, precompress instead — a build affords a better ratio than a per-request encode can justify.

h3's precompressed model is kept and takes precedence: a .br/.gz on disk costs no CPU, so it wins whenever one exists, and on-the-fly encoding is strictly the fallback. But the disk lookup is now opt-in (encodings, default false): most deployments ship no precompressed files, so probing for one is a stat that always misses on every compressible request — and since serveStatic falls through to the app on every unmatched route, the whole non-static surface paid for it. On-the-fly compression covers the compressible case by default, so the lookup is pure overhead until a build actually produces variants. encodings: true enables it with { br: ".br", gzip: ".gz" }; a map customizes the extensions. compress (default true) governs the fallback and is independent, so encodings: true + compress: false is "disk variants only" and the default is "always encode on the fly".

Verified end-to-end against a real server over node:http rather than fetch — undici transparently decodes br/gzip, which would hide the very bytes under test. Decoding the wire bytes is what proves real brotli was produced rather than plain bytes sent under an encoding header:

request encoding wire bytes decoded
/big.js + br, gzip br 31 32775 compressed here (default: no disk lookup)
/big.js + gzip gzip 102 32775
/variant.js + gzip (default) gzip 102 32775 a .gz on disk is ignored, compressed here
/variant.js + br, gzip (encodings: true) gzip 2065 (marker) opt in and the .gz wins, uncompressed here
/big.js + br;q=0, gzip gzip 102 32775 q=0 refusal honored
/small.js (1023 B) + br none 1023 under the floor
/logo.png + br none 32775 incompressible type
HEAD /big.js + br br 0 headers intact, bytes never produced

Bugs fixed along the way: br;q=0 (an explicit refusal) was honored as brotli; x-gzip/brotli matched as substrings; Content-Length was deleted on encoded responses and is now correct from the variant's size (an on-the-fly body is chunked, since its length is unknowable until the bytes exist).

Compression is gated on compressible types (from #233), so images/video/fonts cost no extra stat calls, are never encoded, and omit Vary. Vary: Accept-Encoding is set on compressible responses including uncompressed ones, since a shared cache must key on the header either way. renderHTML routes are never compressed: the rendered Response is the caller's to encode, and a variant on disk would not match its output.

The identity file gates the variant lookup: variants are only probed once <path> itself is known to exist. Nothing is lost — a client accepting no encoding needs that file regardless, so an orphan .br was never servable in practice — and it is what keeps the miss path cheap (below). Precompressed variants go through the same symlink containment as everything else, so the .br lookup can't become an escape hatch; an escaping variant is skipped and the contained plain file is served.

On pipeline vs pipe. The compressor is wired with stream.pipeline, not stream.pipe(encoded): pipe leaves the source running when the destination is destroyed, so a client disconnecting mid-response strands the fd behind the compressor until GC. This needs a response genuinely still in flight to observe — 9 MiB of random bytes, slow enough to brotli that the abort lands mid-stream. Over 20 aborted requests, pipe strands 6 fds and Node itself warns Closing file descriptor 22 on garbage collection; pipeline strands none.

Filesystem cost

Containment costs a realpath, which walks every segment. So it is only paid for the file actually served: statFile answers existence, isContained is the boundary, and the two are called separately rather than on every probe. Compression adds no syscalls — it runs on the fd already opened to serve the file.

Measured per request by instrumenting node:fs/promises, in steady state (realDir resolves once at startup):

request before after
/nope — falls through to the app 7 3
/app.js + br, default (no lookup) 2 2
/app.js + br, encodings: true, variant on disk 2 3
/app.js + br, encodings: true, no variant 2 2
/index.html + br, no variant 4 4

By default there is no variant lookup, so a compressible request costs no extra stat over main and compression runs on the fd already opened to serve the file. A hit under encodings: true costs one extra stat. The miss path drops 57%, which is the better side of the trade: serveStatic falls through to the app on every unmatched route, so all non-static traffic pays that path — as does anything scanning for .env/.git.

HEAD

Returns headers without reading the file, and without compressing it. To be precise: Node's http layer already discards HEAD bodies at the wire, so nothing leaked — the cost was server-side (a full disk read, and brotli-11, for a 0-byte response). The headers still describe exactly what GET would send, chunked encoding included. renderHTML routes run the render then strip the body, so HEAD reports the same headers GET would rather than the raw file size — and the unused rendered body is cancelled, so a stream-backed response releases its resource instead of waiting for GC.

Path resolution

A request now resolves as <path>, then <path>.html, then <path>/index.html. Probing the literal path first makes extension-less files reachable at their exact name — LICENSE, apple-app-site-association, and ACME challenge tokens, which are extension-less and previously 404'd as <token>.html (from #233). It also makes bare dotfiles reachable when allow-listed, which the extname() quirk prevented.

Adds .wasm, .avif, .mp3, .gz MIME types (from #233).

Breaking changes

  • Brotli encodes at quality 4 rather than 11. Bodies are a few percent larger for ~12x less CPU. Set encodings: true and ship app.js.br to serve a maximum-quality body — a precompressed variant always wins.
  • Only compressible types are compressed, and only between 1 KiB and 10 MiB. Previously every type was brotli'd at q11, images and video included; now those are served as-is, and so is anything outside the size band.
  • The precompressed-variant lookup is opt-in (encodings, default false). A .br/.gz on disk is only served when encodings is set; by default a compressible file is compressed on the fly instead. Any release that served app.js.br off Accept-Encoding now needs encodings: true.
  • Dotfiles now 404 by default, except /.well-known/. Set dotfiles: true for all, or allow-list segments ([".git"]). Note that enabling all by default would be more permissive than any released version, since bare dotfiles like .env were previously unreachable.
  • A precompressed variant with no source file beside it is no longer served. app.js.br without app.js now 404s; it was already unservable to any client that did not accept brotli.
  • Paths are now percent-decoded once. A file literally named with an encoded sequence (hello%20world.txt on disk) is no longer reachable at its verbatim name; request it properly encoded (/hello%2520world.txt).
  • Malformed percent-encoding now answers 400 instead of falling through to next().
  • An extension-less file now takes precedence over its .html sibling.

All are silent from a user's perspective and worth a CHANGELOG line.

Relationship to #233

#233 (closed) covered similar ground. Taken from it: compressible-type gating, literal extension-less probing, the MIME additions, and — in adjusted form — pathname decoding and the .well-known carve-out (as an allow-list defaulting to [".well-known"], rather than a hard-coded exemption, so operators can add or drop segments).

Deliberately not taken:

  • decodeURIComponent(pathname). It turns %2f into a real separator — what h3's docs explicitly warn against. This PR decodes with decodeURI instead, which keeps %2F/%3F/%23 encoded: same reachability for real-world names, none of the separator hazard, and pinned by tests (/sub%2Finside.txt stays 404, %252e%252e never double-decodes).
  • Conditional requests. fix(static): harden srvx/static #233's ETag/304 work is worth having, but it derives the ETag from the identity file and reuses it for the compressed body — distinct representations sharing an ETag violates RFC 9110. The precompressed model here would give per-variant ETags for free. Left for a follow-up.

Where #233 keeps createBrotliCompress() at quality 11 and only gates it by MIME type, this PR additionally drops the quality to 4 and bounds the file size, which is what closes the amplification vector rather than narrowing it.

Notes

  • vitest.config.mjs gains a srvx -> node-adapter alias. static.ts imports FastResponse from "srvx" at runtime (deliberate — FastResponse is per-adapter and resolves via the exports conditions), which resolves through dist/. It's the only runtime self-import in src/, so no existing test hit it. Without the alias, pnpm test fails on a fresh clone (it doesn't build); CI builds first, so it passes either way.
  • Still open: no ETag/Last-Modified/304; MIME hardening (no charset, no nosniff); no redirect from /sub to /sub/sub/index.html is served in place, so relative links inside it resolve against / (left as a TODO in the source; nginx sends 301). Accept-Encoding is negotiated in server-preference order, so a client's relative q ranking is honored only as a refusal (q=0); RFC 9110 §12.5.3 would have the client's preference win. On-the-fly output is not cached, so a repeat request re-encodes; precompressing is the answer, and a variant always wins the lookup.
  • On the traversal posture. Containment rests on join() + startsWith(dir) for dot segments and on realpath for symlinks. FastURL does resolve dot segments before static.ts sees them (_needsNormRE deopts ., .. and their %2e forms to the native parser), but nothing here relies on that: it is another module's invariant, and decoding can surface a dot segment after it has run. An earlier revision of this PR claimed the opposite in the source comment and in this description — the comment now states the actual invariant. Backporting h3's resolveDotSegments would make resolution self-contained; still a reasonable follow-up, not needed for safety.
  • The traversal tests previously asserted nothing. new Request() collapses .. in its constructor, so the middleware never saw one: the old test passed against a serveStatic with both containment checks deleted. They now build _url directly via FastURL's origin-form fast path — the one way an unresolved pathname reaches static.ts — and fail when either check is removed.

Test plan

  • pnpm build && pnpm vitest run --exclude '**/{deno,bun}*.test.ts' test/ — 1078 passed, 32 skipped
  • oxlint / oxfmt --check clean, tsc --noEmit exits 0
  • 66 of the 111 static tests confirmed failing against main's static.ts
  • Containment verified by mutation: removing the realpath boundary fails 4 tests, removing both checks fails 8
  • Compression verified by mutation: defaulting compress off fails 7 tests; removing the 1 KiB floor fails 6; removing the 10 MiB ceiling fails 1; removing the variant-over-on-the-fly guard fails 1; flipping encodings back to on-by-default fails the "off by default" test. The precedence mutation initially passed — the .gz fixture was under the size floor, so the bounds check masked the broken precedence. The fixture is now padded over the floor, so only precedence decides it.
  • fd teardown measured directly (/proc/self/fd) under 20 aborted brotli responses: pipe strands 6, pipeline strands 0
  • Syscall counts in the table above measured by instrumenting node:fs/promises around a single middleware instance
  • Precompressed lookup, on-the-fly encoding, incompressible gating, HEAD, and ACME tokens verified end-to-end through a real serve()

🤖 Generated with Claude Code

@pi0x
pi0x requested a review from pi0 as a code owner July 16, 2026 17:56
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

serveStatic adds configurable dotfile access and precompressed variant negotiation, enforces symlink-safe containment, expands path resolution, handles HTML rendering and HEAD requests, and adds comprehensive routing, security, encoding, and filesystem tests.

Changes

Static serving behavior

Layer / File(s) Summary
Options and safe path resolution
src/static.ts
Adds dotfiles and encodings, resolves extension-less candidates, parses encoding preferences, and validates realpath containment and dot-segment rules.
Variant selection and response handling
src/static.ts, test/static.test.ts
Selects accepted on-disk variants, sets MIME and encoding headers, applies renderHTML to source HTML files, and returns computed headers without a body for HEAD requests.
Behavior coverage and filesystem validation
test/static.test.ts
Covers routing, methods, symlinks, traversal, dotfiles, .well-known, negotiation, decoding, MIME types, rendering, and HEAD responses.
Documentation and test runtime wiring
docs/1.guide/4.middleware.md, vitest.config.mjs
Documents the updated static-serving semantics and configures the test-time node adapter alias.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • h3js/srvx#233: Overlapping static path, dotfile, symlink, encoding, and HEAD handling changes.

Suggested reviewers: pi0

Poem

A rabbit found files beneath the tree,
With br and gz tucked cleverly.
Dotfiles hid, bad links were bound,
HEADs brought headers, no bytes down.
“Hop!” said the hare, “the paths are sound!”

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is relevant to the PR’s main focus on static serving security hardening and related improvements.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/static-sec

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Jul 16, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/srvx@252

commit: 241d754

@pi0 pi0 changed the title fix(static): contain symlinks, deny dotfiles, and drop on-the-fly compression fix(static): security hardenings Jul 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
vitest.config.mjs (1)

10-12: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Use fileURLToPath() for the Vitest alias. .pathname keeps %20 escapes and doesn’t produce a native path, so this can break alias resolution on paths with spaces and Windows installs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@vitest.config.mjs` around lines 10 - 12, Update the Vitest alias
configuration to convert the `new URL("src/adapters/node.ts", import.meta.url)`
result with `fileURLToPath()` instead of using `.pathname`, ensuring the
replacement is a native filesystem path across spaces and Windows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/1.guide/4.middleware.md`:
- Line 105: Update the adjacent admonition blockquotes in the middleware
documentation to separate them with an HTML comment rather than a blank
blockquote line, preserving both admonitions as distinct while satisfying
markdownlint MD028.

In `@src/static.ts`:
- Around line 142-156: Update resolveFile and the surrounding static-file
response flow to open the candidate once, validate the opened handle’s resolved
target remains within getRealDir(), and reuse that handle for metadata,
readFile, and createReadStream operations. Remove subsequent reopening of the
original path so Content-Length and response data come from the same validated
file.
- Around line 95-113: The encoding selection in src/static.ts lines 95-113
currently filters by q-value without ranking; update the flow around the quality
map and Object.entries(encodings) to sort accepted encodings by descending
quality while preserving configured order for ties. In test/static.test.ts lines
208-212, expand the request to competing values such as gzip;q=1 and br;q=0.5
and assert that gzip is selected.

In `@test/static.test.ts`:
- Around line 208-212: Update the “honors an explicit q ranking” test to request
both gzip and Brotli with different quality values, such as gzip at 1 and Brotli
at 0.5, then assert that the response uses gzip and returns the corresponding
gzip fixture content.

---

Nitpick comments:
In `@vitest.config.mjs`:
- Around line 10-12: Update the Vitest alias configuration to convert the `new
URL("src/adapters/node.ts", import.meta.url)` result with `fileURLToPath()`
instead of using `.pathname`, ensuring the replacement is a native filesystem
path across spaces and Windows.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8349432a-136c-47ac-ac2d-7815d7d268f4

📥 Commits

Reviewing files that changed from the base of the PR and between 294d00f and 8264f3a.

📒 Files selected for processing (4)
  • docs/1.guide/4.middleware.md
  • src/static.ts
  • test/static.test.ts
  • vitest.config.mjs

Comment thread docs/1.guide/4.middleware.md
Comment thread src/static.ts Outdated
Comment on lines +95 to +113
const quality = new Map<string, number>();
for (const part of header.split(",")) {
const [token, ...params] = part.split(";");
const name = token!.trim().toLowerCase();
if (!name) {
continue;
}
let q = 1;
for (const param of params) {
const trimmed = param.trim();
if (trimmed.startsWith("q=")) {
// A malformed q (`q=abc`) parses to NaN; treat it as refused.
q = Number.parseFloat(trimmed.slice(2)) || 0;
}
}
quality.set(name, q);
}
const wildcard = quality.get("*");
return Object.entries(encodings).filter(([name]) => (quality.get(name) ?? wildcard ?? 0) > 0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Implement and test actual q-value ranking.

The parser only treats q-values as accepted/refused, and the current test cannot expose that because it supplies one encoding.

  • src/static.ts#L95-L113: sort accepted encodings by descending quality, using configured order only as the tie-breaker.
  • test/static.test.ts#L208-L212: offer competing values such as gzip;q=1, br;q=0.5 and assert gzip is selected.
📍 Affects 2 files
  • src/static.ts#L95-L113 (this comment)
  • test/static.test.ts#L208-L212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/static.ts` around lines 95 - 113, The encoding selection in src/static.ts
lines 95-113 currently filters by q-value without ranking; update the flow
around the quality map and Object.entries(encodings) to sort accepted encodings
by descending quality while preserving configured order for ties. In
test/static.test.ts lines 208-212, expand the request to competing values such
as gzip;q=1 and br;q=0.5 and assert that gzip is selected.

Comment thread src/static.ts Outdated
Comment thread test/static.test.ts Outdated
…pression

`serveStatic` had no test coverage despite being a public export (`srvx/static`).
Adds a suite (44 tests) and fixes what it surfaced.

Symlink escape: `stat()` follows symlinks but the only containment check was
lexical (`filePath.startsWith(dir)`), so a link inside `dir` served any file on
the host. Re-assert containment against the `realpath`-resolved path. Both sides
are resolved, or a legitimately symlinked `dir` (`/var/www` -> `/data/www`) would
reject every file. Links resolving inside `dir` are still served.

Dotfiles: only bare dotfiles were protected, by accident -- `extname(".env")` is
`""`, so `.env` was probed as `.env.html` and missed. Anything with a real
extension was served (`.env.production`, `sub/.env.local`, `.git/config.txt`).
Deny dot segments by default behind a new `dotfiles` option. `.well-known` gets
no exemption; enable `dotfiles` to serve well-known URIs.

Compression: `createBrotliCompress()` ran at quality 11 (the maximum) per
request with no cache, letting an unauthenticated client amplify cheap requests
into heavy CPU work -- including via HEAD, whose body Node discards anyway.
Replaced with a precompressed lookup (`app.js.br`, `app.js.gz`) via a new
`encodings` option, mirroring h3. Nothing is compressed on the fly. This also
fixes `br;q=0` (an explicit refusal) being honored as brotli, `x-gzip`/`brotli`
matching as substrings, and restores `Content-Length` on encoded responses.
Variants are only looked up for compressible types, so an image or font costs no
extra stat calls and omits `Vary`.

HEAD: return headers without reading the file.

Path resolution: probe the literal path before the `.html` route candidates, so
extension-less files (`LICENSE`, `apple-app-site-association`, ACME challenge
tokens) are reachable at their exact name. This also makes bare dotfiles
reachable under `dotfiles: true`, which the `extname()` quirk above prevented.

Adds `.wasm`, `.avif`, `.mp3` and `.gz` MIME types.

BREAKING: assets are no longer compressed on the fly -- precompress at build time
to keep compressed responses. Dotfiles (including `/.well-known/`) now 404 unless
`dotfiles: true`. An extension-less file now takes precedence over its `.html`
sibling.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@pi0x
pi0x force-pushed the fix/static-sec branch from 8264f3a to 6adb025 Compare July 16, 2026 18:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/static.test.ts`:
- Around line 361-366: Update the traversal test in “does not serve traversal
outside the root” by replacing the encoded `%2e%2e%2f` input with a request path
that the adapter actually decodes into `../` segments; otherwise remove that
case if the adapter never performs decoding. Keep the existing 404 assertion for
the remaining traversal input.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ada39dd8-0af7-4731-b9e2-5be5a8f0996e

📥 Commits

Reviewing files that changed from the base of the PR and between 8264f3a and 6adb025.

📒 Files selected for processing (4)
  • docs/1.guide/4.middleware.md
  • src/static.ts
  • test/static.test.ts
  • vitest.config.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
  • vitest.config.mjs
  • src/static.ts

Comment thread test/static.test.ts Outdated
@pi0 pi0 changed the title fix(static): security hardenings feat(static)!: security hardenings Jul 17, 2026
@pi0 pi0 changed the title feat(static)!: security hardenings feat(static)!: security hardenings and general improvements Jul 17, 2026
pi0 and others added 5 commits July 17, 2026 09:44
…h 400

- Single decodeURI pass from the wire form so encoded names
  (hello%20world.txt, caf%C3%A9.txt, 50%25.txt) resolve; %2F/%3F/%23
  stay encoded so an encoded separator never becomes a separator.
- Malformed percent-encoding (/foo%, /%ZZ) now answers 400 like
  nginx/serve-static instead of falling through.
- HEAD on a renderHTML route cancels the unused rendered body.
- TODO note on /sub -> /sub/ redirect handling.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`dotfiles` becomes `boolean | string[]`, defaulting to `[".well-known"]`.
RFC 8615 reserves that namespace for public metadata, so ACME HTTP-01
challenges and `security.txt` now work out of the box. The previous
all-or-nothing `dotfiles: true` meant renewing a certificate also
published `.env` and `.git/config` — the files the default exists to
hide. Matching is by exact segment, so `[".well-known"]` covers neither
a prefix sibling (`.well-known-backup`) nor a nested dot segment
(`.well-known/.env`).

Split `resolveFile` into `statFile` (existence) and `isContained`
(the realpath boundary), and gate variant lookup on the identity file.
A precompressed variant is only reachable next to a real file, which a
client accepting no encoding needs anyway, so an orphan `.br` was never
servable in practice. Measured per request: a miss drops 7 -> 3 syscalls
and a variant hit costs 2 -> 3. Misses are worth the trade — the
middleware falls through to the app on every unmatched route.

The lexical `startsWith(dir)` check is a pre-filter, not the boundary
(`isContained` is); say so, since nothing distinguishes it in tests.

Tests: 52 -> 64. The traversal tests asserted nothing — `new Request()`
collapses `..` in its constructor, so they passed against a `serveStatic`
with both containment checks deleted. They now build `_url` directly via
`FastURL`'s origin-form fast path, the one way an unresolved pathname
reaches here, and fail when either check is removed.

Also correct the decode comment, which claimed the URL parser had
already resolved dot segments: it does, but that is `_url.ts`'s
invariant, and decoding can surface dot segments after it runs — so
containment rests only on `join()` + `startsWith(dir)`. Drop the stale
`node:zlib` mention from the docs; this branch removed that import.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/static.test.ts`:
- Around line 291-297: The dotfile denial assertions in the test cases should
verify downstream handling rather than only checking a 404 status. Update the
test at “serves an arbitrary allow-listed segment and nothing else” and the
corresponding cases around the other referenced ranges to use the existing
expectNext helper for denied dotfiles, while preserving the successful
allow-listed response assertion.
- Around line 362-364: Add test cases to the static content negotiation
scenarios covering partially numeric q-values such as “q=0.5junk” and
out-of-range values such as “q=2”. In the relevant test table, assert identity
fallback by setting enc to null and body to PLAIN_JS, matching the existing
malformed-q case.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9a5047fc-3910-4726-9d3a-cfb75447b2aa

📥 Commits

Reviewing files that changed from the base of the PR and between ca291e8 and 00d2bcf.

📒 Files selected for processing (3)
  • docs/1.guide/4.middleware.md
  • src/static.ts
  • test/static.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/static.ts

Comment thread test/static.test.ts
Comment thread test/static.test.ts
Comment on lines +362 to +364
{ why: "honors q=0 as a refusal", accept: "br;q=0, gzip", enc: "gzip", body: "GZIP_JS" },
{ why: "honors an explicit q ranking", accept: "br;q=1.0", enc: "br", body: "BROTLI_JS" },
{ why: "treats a malformed q as a refusal", accept: "br;q=abc", enc: null, body: "PLAIN_JS" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cover partially numeric and out-of-range malformed q-values.

The implementation uses Number.parseFloat, so values such as q=0.5junk and q=2 are accepted despite the stated malformed-q refusal. Add identity-fallback cases to expose this.

Proposed test cases
 { why: "treats a malformed q as a refusal", accept: "br;q=abc", enc: null, body: "PLAIN_JS" },
+{ why: "refuses trailing garbage in q", accept: "br;q=0.5junk", enc: null, body: "PLAIN_JS" },
+{ why: "refuses an out-of-range q", accept: "br;q=2", enc: null, body: "PLAIN_JS" },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{ why: "honors q=0 as a refusal", accept: "br;q=0, gzip", enc: "gzip", body: "GZIP_JS" },
{ why: "honors an explicit q ranking", accept: "br;q=1.0", enc: "br", body: "BROTLI_JS" },
{ why: "treats a malformed q as a refusal", accept: "br;q=abc", enc: null, body: "PLAIN_JS" },
{ why: "honors q=0 as a refusal", accept: "br;q=0, gzip", enc: "gzip", body: "GZIP_JS" },
{ why: "honors an explicit q ranking", accept: "br;q=1.0", enc: "br", body: "BROTLI_JS" },
{ why: "treats a malformed q as a refusal", accept: "br;q=abc", enc: null, body: "PLAIN_JS" },
{ why: "refuses trailing garbage in q", accept: "br;q=0.5junk", enc: null, body: "PLAIN_JS" },
{ why: "refuses an out-of-range q", accept: "br;q=2", enc: null, body: "PLAIN_JS" },
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/static.test.ts` around lines 362 - 364, Add test cases to the static
content negotiation scenarios covering partially numeric q-values such as
“q=0.5junk” and out-of-range values such as “q=2”. In the relevant test table,
assert identity fallback by setting enc to null and body to PLAIN_JS, matching
the existing malformed-q case.

pi0 and others added 3 commits July 17, 2026 11:54
…a a single opened fd

- A slash-terminated URL names a directory: `/sub/` resolves only
  `sub/index.html`, no longer `sub.html` or a file named `sub` (breaking).
- Serving opens the file once and streams that fd: fstat + realpath
  containment + an inode comparison pin the served bytes to the checked
  path, closing the stat-then-createReadStream symlink swap race.
- Test harness drains unconsumed response bodies: undici does not
  propagate `body.cancel()` to a wrapped Node readable, so status/header
  -only tests leaked the file handle until GC.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he pool

`stat()` reports a candidate's mode, but `open()` acts on it a syscall later,
and `open()` on a FIFO waits for a writer that may never come. An attacker who
can write into the served root — the same one the symlink checks already assume
— can swap a regular file for a pipe in that window and park a libuv threadpool
thread. The pool is 4 threads by default, so a handful of wins stall every fs
operation in the process.

O_NONBLOCK closes the window rather than narrowing it: the open returns
immediately and the `fstat` mode check already in `openServable` declines the
pipe. Reads of regular files ignore the flag, and Windows has no O_NONBLOCK
because `open()` cannot block this way there.

A FIFO merely sitting in the root never reaches `open()` — `statFile` rejects
it first — so covering this means reproducing the lost race, which a lying
`stat` stands in for. `vi.mock` is file-wide, hence a separate test file; it
carries a guard test so the FIFO case cannot pass for the wrong reason.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.31933% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/static.ts 98.31% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

pi0 and others added 2 commits July 17, 2026 12:54
Restores on-the-fly compression, which this branch had removed wholesale to
close a CPU amplification vector. The vector was the *quality*, not the
feature: `createBrotliCompress()` defaults to BROTLI_DEFAULT_QUALITY (11),
the maximum, which costs ~12x quality 4 for a few percent of size — per
request, uncached, on the same 4-thread libuv pool as every stat/open here.

Compression also inverts what normally makes a large file self-limiting: the
response gets smaller while the server burns CPU proportional to the
*uncompressed* size, so the request stops paying for itself in bandwidth.
Hence the two bounds, not just the lower quality.

- brotli at quality 4, with BROTLI_PARAM_SIZE_HINT from the fstat'd size
- only between 1 KiB (encoded output can exceed the input) and 10 MiB
  (precompress instead — a build affords a better ratio anyway)
- a variant on disk always wins, since it costs no CPU
- HEAD skips it entirely: Node discards the body at the http layer, so the
  headers describe what GET would send and the bytes are never produced
- `compress: false` serves only what is on disk; `encodings: {}` now skips
  the disk lookup while still compressing, rather than disabling everything

`pipeline()` rather than `stream.pipe(encoded)`: `pipe` leaves the source
running when the destination is destroyed, so a client disconnecting
mid-response strands the fd until GC. Measured with 9 MiB of random bytes
(slow enough to brotli that the abort lands mid-stream): `pipe` strands 6
fds over 20 aborts and Node warns "Closing file descriptor N on garbage
collection"; `pipeline` strands none.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`encodings` now defaults off (`false`) rather than `{ br, gzip }`. Most
deployments ship no precompressed files, so probing for one is a stat that
always misses — on every compressible request, and `serveStatic` falls
through to the app on every unmatched route, so the whole non-static surface
paid it. On-the-fly compression (added in the previous commit, on by default)
already covers the compressible case, so the disk lookup is pure overhead
until a build actually produces variants.

`encodings: true` uses the former default map; a `Record` still customizes
per-encoding extensions. A variant, when configured, still takes precedence
over on-the-fly — it costs no CPU. The two switches stay independent:
`encodings: true` + `compress: false` is "disk variants only", and the
default (`encodings` off, `compress` on) is "always encode on the fly".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants