fix(import): validate JSON-import media bytes + destination like the archive path#154
Draft
DavidBabinec wants to merge 1 commit into
Draft
fix(import): validate JSON-import media bytes + destination like the archive path#154DavidBabinec wants to merge 1 commit into
DavidBabinec wants to merge 1 commit into
Conversation
…archive path
The JSON SiteBundle import (`POST /admin/api/cms/import`) wrote each
`media[].bytesBase64` to `join(uploadsDir, storagePath)` with only a
traversal check — no magic-byte MIME detection, no SVG sanitisation, no
extension check. Because `published/` lives under `uploadsDir` and is served
verbatim as `text/html` with no CSP by the public router (same origin as
`/admin`), a caller holding only `data.import` (no step-up, no
`content.manage`) could write `storagePath: "published/current/index.html"`
with `<script>` bytes and achieve stored XSS → same-origin admin account
takeover. The archive import path already gated its writes; the JSON path did
not.
Fix at the source by making that gate shared and applying it to both paths:
- Rename `importArchiveMediaValidation.ts` → `importMediaValidation.ts`
(now used by both import routes) and split it into two focused gates:
- `validateAndSanitizeMediaBytes(bytes, {storagePath, mimeType})` — content:
real MIME must match the declared type + extension; SVG is sanitised.
No accepted media MIME maps to `.html`/`.js`, so script writes are rejected.
- `resolveMediaWriteTarget(uploadsDir, storagePath)` — destination: no
traversal, and never a reserved served subtree (`published/`, `plugins/`,
`fonts/`).
- Route the JSON import media write through both gates (previously unguarded).
- Route the archive path through the same two gates (behaviour-preserving:
it already buffered bytes for MIME detection and still moves non-SVG files).
Tests: new `importMediaValidation.test.ts` proves HTML/JS, MIME-mismatch,
extension-laundering, and reserved-subtree writes are rejected and SVG scripts
stripped; existing archive media test still passes through the shared gate;
architecture gate allowlist updated for the rename.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
What & why
The JSON
SiteBundleimport endpoint (POST /admin/api/cms/import) wrote eachmedia[].bytesBase64tojoin(uploadsDir, storagePath)guarded only by a traversal check — no magic-byte MIME detection, no SVG sanitisation, no extension check (the archive import path already had all three).Because
published/lives underuploadsDirand the public router serves it verbatim astext/htmlwith no CSP, on the same origin as/admin, a caller holding onlydata.import(merge-add needs no step-up and nocontent.manage) could POSTstoragePath: "published/current/index.html"with<script>bytes and land arbitrary same-origin HTML/JS → stored XSS → admin account takeover (mint an owner role / reset the owner password / exfiltrate).Fix (at the source, shared)
Made the existing gate shared and applied it to both import paths:
importArchiveMediaValidation.ts→importMediaValidation.ts(now used by both routes), split into two focused gates:validateAndSanitizeMediaBytes(bytes, {storagePath, mimeType})— content: detected MIME must match the declared type and the storagePath extension; SVG is sanitised. No accepted media MIME maps to.html/.js, so script writes are rejected outright.resolveMediaWriteTarget(uploadsDir, storagePath)— destination: no traversal (viaassertPathWithin), and never a reserved served subtree (published/,plugins/,fonts/).Impact
data.import→ same-origin account-takeover primitive. No API/behaviour change for legitimate imports.Verification
bun test server/handlers/cms/__tests__/importMediaValidation.test.ts server/handlers/cms/__tests__/importArchiveMedia.test.ts— 14 pass (HTML/JS, MIME-mismatch, extension-laundering, reserved-subtree writes all rejected; SVG scripts stripped).bun test src/__tests__/architecture/cms-handlers-capability-gated.test.ts— pass (allowlist updated for the rename).bunx tsc -b— clean.eslinton touched files — clean.icon-catalog-integrity,bundle-size-budgets) confirmed failing on the untouched base; not part of this change.🤖 Generated with Claude Code