Repository navigation
fix: escape admin dashboards and sandbox active static content (stored XSS) - #176
Open
9xj89gzrtw-hue wants to merge 4 commits into
Open
9xj89gzrtw-hue wants to merge 4 commits into
9xj89gzrtw-hue wants to merge 4 commits into
Conversation
normalizePath collapsed only the first repeated slash group (missing /g flag), so paths like a///b///c.txt normalized to a/b///c.txt. The db-client normalizes file paths on write and again on lookup; a non-idempotent normalizer can make those comparisons inconsistent. - replace(/\/\/+/g, "/") now collapses every group - new tests: multi-group collapse, idempotence, windows separators and mixed separators, preprocessing edge cases
…scircuit#173) resolveFileProxy resolved user-controlled segments with join(diskPath, relativePath) without checking for .. segments, so a request like /files/download/local/../secret.txt read files outside the proxy root (CWE-22); the http flavor forwarded the same traversal upstream. - resolveFileProxy rejects any relative path containing a .. segment (400) - resolveDiskProxy additionally require the resolved path to stay inside resolve(diskPath), independent of platform separator handling - unit + route-level regression tests using raw HTTP (clients normalize dot-dot segments)
POST /files/upsert is unauthenticated, so anything it stores must be treated as user content everywhere it is rendered. Admin dashboards rendered file_path/event payloads unescaped (stored XSS on the same origin as /admin), and /files/static served user HTML/SVG/JS with script-executing content types on that origin. - new escapeHtml() util; applied to admin files list, file details (display strings; href attribute values escaped without changing normal-path URLs) and events list (type + details JSON) - /files/static sets X-Content-Type-Options: nosniff on every response and Content-Security-Policy: sandbox on active content types (text/html, text/javascript, image/svg+xml, application/xml), including proxied responses; bytes unchanged - route-level regression tests (49/49 pass full suite)
Author
|
/claim #175 |
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.
Fixes #175
Vulnerability chain
POST /files/upsertis unauthenticated, so anything stored must be rendered as untrusted. Before this change:${file.file_path}(list + details, including<title>),${file.file_id},${file.created_at},${event_type}and the JSONified event payload containing the user-suppliedinitiator. A file written with path/<script>alert(1)</script>/poc.htmlexecuted for every visitor of/adminpages (stored XSS)./files/staticserved stored content withtext/html,image/svg+xml,text/javascript,application/xmlon the same origin as/admin— script execution one navigation away from the dashboards.Fix
lib/utils/escape-html.ts(escapeHtml()), applied to every user-controlled interpolation inadmin/files/list,admin/files/getandadmin/events/list. Display strings escaped; href attribute values escaped as well, preserving the exact URL text for normal paths so existing consumers (and the existing admin page test) keep working./files/static:X-Content-Type-Options: nosniffon every response andContent-Security-Policy: sandboxon active content types (text/html,text/javascript,image/svg+xml,application/xml) — including proxied responses re-wrapped dynamically. Body bytes and content types are otherwise unchanged.Tests
tests/routes/admin-xss-escape.test.ts: scripted file_path rendered only as entities on list + details; scriptedinitiatorescaped on the events page; normal file data renders unchanged.tests/routes/static-active-content-headers.test.ts:.html/.svg/.jsresponses carry sandbox CSP + nosniff;.txtgets nosniff only; body bytes unchanged in all cases.Full suite 49/49 pass; tsc clean;
bun run format:checkexit 0.