Security hardening: dependencies, CSP, session revocation, md render DoS, body limits - #14
Merged
Conversation
adm-zip 0.5.18 -> 0.6.0 closes the crafted-zip 4 GB allocation (GHSA-xcpc-8h2w-3j85). That one matters here: the allocation happens while parsing the archive, before saveZipArtifact gets to run its file count, path, symlink, and uncompressed-size checks. Also picks up body-parser 1.20.6 (size limit silently disabled on an invalid limit), fast-uri 3.1.5 (host confusion), and MCP SDK 1.30.0, which allows the patched @hono/node-server 2.x. npm audit is clean. Smoke suite passes, including the zip deploy, asset serve, and zip duplicate paths. Checked /mcp separately since smoke does not cover it: initialize answers 200, unauthenticated 401.
…pages The dashboard is the one page holding the admin session cookie and it was served with no CSP and no framing protection at all. It now sends a same-origin policy (Google Fonts is the only third party it loads), X-Frame-Options: DENY, nosniff, no-referrer, and no-store. FRAME_CSP, which covers the viewer wrapper, the password prompt, and the not-found page, picks up object-src 'none', base-uri 'none', form-action 'self', and frame-ancestors 'none'. Embedding still works: an iframe load carries Sec-Fetch-Dest: iframe and /a/:slug serves that raw, so an embedder never receives the wrapper. What it does stop is framing the unlock prompt to clickjack it. Artifact bodies keep ARTIFACT_CSP unchanged, so published pages are as permissive as before. Smoke suite passes. Drove the dashboard in Chromium: login, list, and fonts all fine, zero CSP violations on /.
One HMAC secret signed both the admin session cookie and every capability link, so rotating it on a password change would have broken every share link already handed out. That is why the code did not rotate it, which left a real gap: changing the password is exactly what an admin does after a cookie leak, and it did not sign the attacker out. Split the two. sessionSecret keeps signing capability links and unlock cookies. A new adminSecret signs only the admin session cookie, and a password change rotates it, then re-issues a cookie so the browser the admin is sitting at stays signed in. An instance upgrading from a single-secret auth.json has no adminSecret yet, so one is generated on first use. That signs out admin sessions issued before the upgrade. Share links are untouched. Login also now hashes against a decoy credential when the username does not match, so an unknown username costs the same as a wrong password. Measured before: about 1ms vs 30ms, which told an attacker the admin's username. Measured after: 29ms vs 34ms. Setup now writes an auth log line, since whoever reaches an instance with no admin claims it. Smoke suite passes. Added a direct check for the new behavior: the pre-change cookie 401s, the re-issued cookie works, and a private artifact's share link still 302s after the change.
md artifacts render at serve time, and marked.parse is synchronous. A 4 MB document costs ~390ms of blocked event loop per view, 8 MB costs ~780ms. Public artifacts are readable by anyone holding the link, so that was an amplification channel: publish once, then every GET stalls the whole server for everyone. Rendered HTML is now cached per artifact and per md config, bounded at 48 MB of rendered bytes with least-recent eviction. Every write path (publish, replace, patch, rename, duplicate, delete) drops the slug's entries, so an edit still shows on the next view. Measured on a 4 MB artifact: 388ms cold, 7-11ms warm, and /healthz answers in 17ms while 12 concurrent readers hammer it. Behaviour is unchanged. Verified an edit is visible immediately after PUT, and that flipping the global md font shows up on the next view and again on the way back. Full smoke suite passes.
express.json ran as global middleware with a 10 MB limit, and body parsing happens before routing. An unauthenticated caller could make the server allocate and parse 10 MB of JSON on /api/auth/login before the rate limiter ever saw the request, then repeat. Credential routes (/api/auth/*, /a/:slug/unlock) now get a 16 kB parser. Publish routes keep 10 MB json and 50 MB zip. Verified: a 1 MB login body returns 413, a normal login returns 200, and a 5 MB publish still returns 201. Full smoke suite passes.
Operator-visible behaviour change from the session-secret split, plus the 16 kB body cap on credential routes.
All four shells (frame, md, password, not-found) link fonts.googleapis.com and both CSPs blocked it, so those fonts never loaded and every artifact view logged a CSP error. Published artifacts that use a webfont were blocked the same way. FRAME_CSP gets fonts.googleapis.com on style-src and a font-src for fonts.gstatic.com. ARTIFACT_CSP gets both hosts on default-src rather than a narrow style-src pair, because adding an explicit style-src there would stop styles falling back to default-src and break artifacts that load CSS from cdnjs, unpkg, or jsdelivr. A stylesheet cannot execute script, so this does not widen the script surface. The cost is that a viewer's IP now reaches Google on any page that requests a webfont. Self-hosting the font files would avoid both that and the CSP entry. Checked all five shell pages in Chromium: fonts load, zero CSP violations, down from four blocked pages. Full smoke suite passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five hardening passes over the server, each one tested before moving to the next. No behaviour changes for publishers or readers. One operator-visible change, called out below.
What changed
1. Dependencies.
npm auditwas reporting five advisories and now reports none.The one that mattered here is
adm-zip(high, GHSA-xcpc-8h2w-3j85): a crafted archive triggers a 4 GB allocation while parsing, beforesaveZipArtifactgets to run its file-count, path, symlink, and uncompressed-size checks. That needed the 0.6.0 major. Also picked upbody-parser1.20.6 (size limit silently disabled on an invalid limit),fast-uri3.1.5, and MCP SDK 1.30.0, which allows the patched@hono/node-server2.x.2. Dashboard CSP and framing. The dashboard is the one page carrying the admin session cookie, and it was served with no CSP and no framing protection. It now sends a same-origin policy (Google Fonts is the only third party it loads),
X-Frame-Options: DENY, nosniff, no-referrer, no-store.FRAME_CSP(viewer wrapper, password prompt, not-found page) gainedobject-src 'none',base-uri 'none',form-action 'self',frame-ancestors 'none'. Embedding is unaffected: an iframe load carriesSec-Fetch-Dest: iframeand/a/:slugserves that raw, so an embedder never receives the wrapper. What it does stop is framing the unlock prompt to clickjack it.Artifact bodies keep
ARTIFACT_CSPexactly as it was, so published pages are as permissive as before.3. Password change now evicts stolen admin sessions. One HMAC secret signed both the admin session cookie and every capability link, so rotating it on a password change would have broken every share link already handed out. That is why the code did not rotate it, which left a real gap: changing the password is exactly what an admin does after a cookie leak, and it did not sign the attacker out.
The two secrets are now separate.
sessionSecretkeeps signing capability links and unlock cookies. A newadminSecretsigns only the admin session cookie, and a password change rotates it, then re-issues a cookie so the browser the admin is sitting at stays signed in.Login also hashes against a decoy credential when the username does not match, so an unknown username costs the same as a wrong password. Before: ~1 ms vs ~30 ms, which told an attacker the admin's username. After: 29 ms vs 34 ms.
4. Markdown render DoS. md artifacts render at serve time and
marked.parseis synchronous. Measured: 1 MB costs ~130 ms of blocked event loop, 4 MB ~390 ms, 8 MB ~780 ms. Public artifacts are readable by anyone holding the link, so this was an amplification channel: publish once, then every GET stalls the whole server for everyone.Rendered HTML is now cached per artifact and per md config, bounded at 48 MB of rendered bytes with least-recent eviction. Every write path drops the slug's entries, so an edit still shows on the next view.
5. Credential body limits.
express.jsonran globally at 10 MB, and body parsing happens before routing, so an unauthenticated caller could make the server parse 10 MB of JSON on/api/auth/loginbefore the rate limiter saw the request./api/auth/*and/a/:slug/unlocknow cap at 16 kB. Publish routes keep 10 MB json and 50 MB zip.6. Google Fonts unblocked. All four shells (
frame,md,password,not-found) linkfonts.googleapis.com, and both CSPs blocked it, so those fonts never loaded and every artifact view logged a CSP error. Published artifacts using a webfont were blocked the same way.FRAME_CSPgetsfonts.googleapis.comonstyle-srcplus afont-srcforfonts.gstatic.com.ARTIFACT_CSPgets both hosts ondefault-srcrather than a narrowstyle-src/font-srcpair, because adding an explicitstyle-srcthere would stop styles falling back todefault-srcand break artifacts that load CSS from cdnjs, unpkg, or jsdelivr.A stylesheet cannot execute script, so this does not widen the script surface. The cost is that a viewer's IP reaches Google on any page requesting a webfont. Self-hosting the font files would avoid both that and the CSP entry.
Operator-visible change
Changing the admin password now signs out every other admin session. The browser making the change stays signed in. An instance upgrading from a single-secret
auth.jsonhas noadminSecretyet, so one is generated on first use, which signs out admin sessions issued before the upgrade. Share links are untouched. Documented indocs/auth.md.Testing
The full smoke suite (57 checks) ran after every round and passes. On top of it:
/healthzanswers in 6 ms while 12 concurrent readers hammer it; an edit is visible immediately afterPUT; flipping the global md font shows on the next view and again on the way back./mcp, so it was checked directly after the SDK bump.initializereturns 200, unauthenticated returns 401./.Follow-up worth considering
Self-hosting the four font families would drop
fonts.googleapis.comandfonts.gstatic.comback out of both policies and stop artifact views from reaching Google at all. Not done here.