fix(website): remove browser PAT flow, sanitize dynamic rendering, restrict CSP (#297) - #318
fix(website): remove browser PAT flow, sanitize dynamic rendering, restrict CSP (#297)#318parthrohit22 wants to merge 5 commits into
Conversation
…strict CSP The live site served a "Blog Editor" that asked any visitor for a classic GitHub personal access token with repo scope, then used it to call the GitHub API directly from client-side JS to create a branch, commit, and open a PR. A DOM-XSS on that page could expose that token. The dev branch also carried a broken half-fix: every innerHTML assignment had been blindly replaced with textContent, which closed the XSS but broke nearly the whole site's dynamic rendering in the process - the terminal's .command-text span never existed, blog posts/docs pages rendered literal <div class="..."> text instead of parsed HTML, and the "no code execution" property overlapped with "renders nothing correctly." Root causes fixed: - Remove the in-browser PAT flow entirely (github-token input, "Get Token" link, submitToGithub(), and the ~90 lines of client-side GitHub API calls for branch/commit/PR creation and image upload). Replaced with an "Export Entry" flow that formats the same entry shape and hands it to the contributor to paste into website/content.js themselves, plus a direct link to open the PR - no server-side integration added, deliberately (see issue openshield-org#297 for why that's separate scope). - Restore correct rendering with real sanitization instead of the blanket textContent workaround: every dynamically-built HTML string - this file's own template markup and markdown-derived content (blog posts, docs pages, the live editor preview) alike - now goes through one setSafeHTML() / renderMarkdown() path backed by DOMPurify (added via CDN + SRI, matching the existing marked.js/lucide loading pattern) before it reaches innerHTML. Verified directly against a payload corpus (script tags, onerror/onload/onmouseover handlers, javascript:/data: URLs, iframe srcdoc, style-attribute CSS) that nothing executes. - The 4 places that relied on an inline onclick/onerror attribute inside generated markup (blog card -> showBlogPost, docs nav -> showDocPage, FAQ toggle, contributor-preview avatar fallback) are wired with addEventListener + data-* attributes instead - DOMPurify's default config strips inline event-handler attributes from its output by design, which is exactly what closes this class of bug, so those handlers can't live in sanitized markup anymore. - Fixed a real self-XSS along the way: the contributor-preview avatar's handle was interpolated into the src="..." attribute unescaped. - Tightened the site's actual CSP (a real HTTP header in vercel.json, not a <meta> tag - which silently ignores frame-ancestors): dropped connect-src's https://api.github.com now that nothing calls it, and added base-uri/form-action/frame-ancestors 'self'. - Added an aria-label to the rules-page framework filter <select>, a pre-existing accessibility gap the new axe suite surfaced immediately. Tests: added a Playwright + axe suite (website/tests/) covering rendering correctness for every dynamic section, hash-based routing, the PAT flow's actual removal (no token input, no GitHub API calls, export flow works), an 8-payload XSS regression corpus against the live editor preview and against blog/docs content directly, and axe scans + keyboard nav across every section. Wired into CI as a new "Website (Playwright + axe)" job alongside the existing script-test job. Verified locally: 34/34 passing across repeated runs, plus the pre-existing test_toEmbedUrl.mjs suite (untouched logic, still 8/8). Closes openshield-org#297 Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
…in a test
CodeQL flagged this correctly: req.url().includes('api.github.com') would
also match a spoofed host like api.github.com.evil.com and silently stop
catching the one thing this test exists to catch. Parses the real hostname
with URL() instead, same pattern already used in toEmbedUrl() for the
video-embed allowlist.
Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
|
@parthrohit22, I am taking the lead security review on this. Before final approval, please rebase onto current |
m-khan-97
left a comment
There was a problem hiding this comment.
Parth, removing the browser PAT flow is the right security decision, and the central DOMPurify path is a major improvement over both the original unsafe renderer and the blanket textContent workaround. I traced the current dynamic sinks: the remaining innerHTML assignments are routed through sanitization, and the URL canonicalization fix is valid.
I found two release-blocking gaps against #297’s acceptance criteria:
-
The new Playwright suite does not exercise the CSP at all. Its local Python server does not apply
vercel.jsonheaders, and there is no assertion for theContent-Security-Policyresponse header or asecuritypolicyviolationevent. The issue explicitly requires Playwright coverage for CSP violations, so all 34 tests can pass while the deployed header is absent or broken. Please run the browser suite through a server that applies the production header (or add an equivalent production-header harness), assert the header itself, and add positive/negative CSP behavior coverage. -
script-srcstill permitsunsafe-inline, with many static inline handlers and two inline script blocks retained. That means the CSP is not a meaningful fallback if any HTML injection path escapes sanitization. For this security-boundary PR, please move the static handlers and inline blocks into trusted same-origin JavaScript and removeunsafe-inlinefromscript-src. If Tailwind requires inline styles, keep that decision isolated tostyle-src; it does not justify inline script execution.
The branch also needs the already-requested rebase onto current dev. Please address these on the rebased head and rerun the complete browser/security suite; I will re-review promptly.
ritiksah141
left a comment
There was a problem hiding this comment.
Requesting changes for two findings not covered by the existing review thread, both verified locally on 153a6fc.
-
The
testscript inwebsite/package.jsonis broken. It runsnode --test tests/toEmbedUrl.test.mjs, but that file does not exist (the real test lives atwebsite/test_toEmbedUrl.mjs). Runningnpm testinside website/ fails withCould not find tests/toEmbedUrl.test.mjs. CI never invokesnpm test(both website jobs call the binaries directly), which is why this slipped through. Point the script at the existing file, or move the file to match. -
The accessibility spec flakes under parallel load. In a full parallel run locally, all four axe tests in tests/accessibility.spec.js timed out inside goToSection() waiting on waitForSelector visibility (reproduced in 1 of 2 full runs; the same spec passes 7/7 serially, and a clean parallel rerun passes 34/34). Root cause: the pre-existing showSection() races a 300ms setTimeout against a requestAnimationFrame. Under CPU contention the rAF callback can land more than 300ms late, so the timeout inlines display:none on the section being activated, and the visibility wait never resolves. The fix is the pattern this suite already uses in the mobile-menu and FAQ tests: in goToSection() (tests/helpers.js), wait for DOM state instead of rendered visibility, e.g. waitForFunction that the section has the active class and its inline display is not none. The CI retries: 1 can mask this flake rather than fix it.
For the record, I agree with the two blocking points in the other review (the browser suite never exercises the CSP header despite #297 listing CSP-violation coverage in its acceptance criteria, and unsafe-inline should leave script-src) plus the rebase onto dev.
Addresses the two release-blocking gaps from m-khan-97's security review,
ritiksah141's two additional findings, and TFT444/CodeQL's earlier
comment-analysis feedback (all independently re-verified against current
head before starting):
1. The Playwright suite never exercised CSP at all - it ran against
`python3 -m http.server`, which applies no headers, so all 34 tests
could pass while the deployed vercel.json header was absent or
broken. Added tests/csp_server.py: a small stdlib-only HTTP server
that actually parses and applies vercel.json's header rules
(including Cache-Control on /assets/* stacking with the site-wide
security headers, matching Vercel's real multi-rule-match
semantics), wired into playwright.config.js as the webServer command
so every spec in this suite - not just the new one - now runs
against production-representative headers. New
tests/security.spec.js asserts the real CSP header is present with
the specific directives this fix depends on, that an inline script
injected outside the sanitized-content path is actually blocked
(securitypolicyviolation fires, the script does not run), and that
the page's own same-origin scripts still work normally under the
real header - a CSP tight enough to break the site would be its own
regression.
2. script-src kept 'unsafe-inline' for this file's static onclick/
onchange/oninput attributes, which meant CSP provided no real
defense-in-depth if sanitization were ever bypassed. Removed it
from vercel.json (style-src keeps it - Tailwind's runtime needs
inline styles, which is an unrelated, narrower allowance). Moved:
- The two inline <script> blocks (theme-flicker prevention, Tailwind
config) to theme-init.js / tailwind-config.js, same-origin
external files loaded in the exact same document position so
execution order/timing is unchanged.
- Every remaining static onclick/onchange/oninput attribute (~30
across nav, mobile menu, the editor, and rule filters) to
addEventListener calls in script.js, off data-nav-section/
data-close-mobile-menu/data-add-contributor/data-remove-image
attributes or existing element ids - the same pattern this file's
own dynamically-generated markup already used for the 4 cases
DOMPurify's stripping of inline handlers required fixing earlier
in this PR.
Updated tests/navigation.spec.js's one selector that depended on a
removed onclick attribute; swept every other spec file for the same
dependency (none found).
3. website/package.json's `test` script pointed at
tests/toEmbedUrl.test.mjs, which does not exist - the real file is
test_toEmbedUrl.mjs at the website/ root. `npm test` inside
website/ failed outright; CI never caught it because both website
CI jobs invoke the test binaries directly rather than through `npm
test`. Fixed the path; verified `npm test` now runs and passes.
4. tests/accessibility.spec.js flaked under parallel load (reproduced
locally, root-caused by ritiksah141 to a real race in the site's
own showSection(): a 300ms setTimeout that inlines display:none on
any section still lacking .active races a requestAnimationFrame
that adds .active to the section being activated, and under CPU
contention the rAF callback can land after the timeout fires,
momentarily applying display:none to the very section a test is
waiting to become visible. Rewrote goToSection() in
tests/helpers.js to wait for DOM state (the active class plus the
element's own inline display) instead of Playwright's
rendered-visibility check, matching the pattern this suite already
used for the mobile-menu and FAQ tests for the same underlying
reason.
Verified: every touched JS/Python file passes node --check /
py_compile; vercel.json is valid JSON; index.html re-parses cleanly;
every data-nav-section target matches a real section id and every
preserved element id is still present exactly once (checked
programmatically, not by eye); `npm test` (website/) passes;
tests/csp_server.py's header-matching logic verified directly against
vercel.json's actual rules (in-process, without needing a live
socket - this sandbox's Bash tool cannot open outbound/loopback
connections to a backgrounded process, confirmed by the same timeout
on the previously-CI-green unmodified `python3 -m http.server`
command, not something this change introduced). The live browser
suite itself needs to run in CI, which is a normal runner without that
constraint and is what the existing "Website (Playwright + axe)" job
already exercises.
Signed-off-by: Parth J Rohit <parthrohit60@gmail.com>
Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
test_toEmbedUrl.mjs evaluates the real script.js source in a Node vm context with a minimal stubbed document object - 'just enough DOM stubbing for the file's top-level statements to execute without crashing', per its own comment. The new static-handler wiring added in the previous commit calls document.querySelector() at script.js's top level (for the add-contributor and remove-image buttons), which the stub didn't provide, so `npm test` crashed immediately instead of reaching toEmbedUrl(). Caught by actually re-running the test suite after rebasing, not by assuming an earlier local pass still held. Added querySelector: () => null, matching the existing getElementById stub's convention exactly. Signed-off-by: Parth J Rohit <parthrohit60@gmail.com> Signed-off-by: parthrohit22 <parthrohit60@gmail.com>
|
@m-khan-97 @ritiksah141 Both blocking findings and the two additional ones are addressed on the current head, rebased onto Your two blockers, m-khan-97:
Your two findings, ritiksah141: Two things I caught myself while doing this, fixed along the way rather than leaving for a next round: removing the inline handlers broke one existing test selector ( Verified: full backend suite (845 passed, 5 skipped — pre-existing/environment-only), |
What does this PR do?
Removes the browser-based GitHub PAT flow from the website's Blog Editor, replaces the broken half-fix that was already on
dev(blanketinnerHTML→textContent, which killed the XSS but also broke nearly all of the site's dynamic rendering) with real DOMPurify-backed sanitization, and tightens the site's actual CSP.Type of change
Background
The live site's Blog Editor asked any visitor for a classic GitHub personal access token with
reposcope, then called the GitHub API directly from client-side JS using it. A DOM-XSS on that page could expose that token.devalso already carried an incomplete first attempt at the XSS half of this: everyinnerHTML =had been blindly swapped fortextContent =, which stops script execution but also means the terminal's.command-textspan never gets created, and blog posts / docs pages render literal<div class="...">text instead of parsed markup.What changed
submitToGithub(), and the ~90 lines of client-side GitHub API calls (branch creation, image upload, content commit, PR creation) are gone. Replaced with "Export Entry": formats the same entry shape the old flow built, shown for the contributor to copy and paste intowebsite/content.jsthemselves, plus a direct "Open a Pull Request" link. No server-side publishing integration added — deliberately out of scope for this issue.setSafeHTML()/renderMarkdown()inscript.js, backed by DOMPurify (loaded via CDN + SRI, matching the existingmarked.js/lucidepattern). Every dynamically-built HTML string — this file's own template markup and markdown-derived content (blog posts, docs pages, the live editor preview) alike — goes through this one path before reachinginnerHTML. This restores correct rendering for every section that was broken (terminal, blog list/detail, docs, rules, events, releases, FAQ, showcase, contributors, playground) while actually closing the XSS, rather than trading one problem for the other.on*attributes from its output by design (that's the mechanism that closes the XSS), so the 4 places that relied on one inside dynamically-generated markup (blog card click, docs-nav click, FAQ toggle, contributor-preview avataronerrorfallback) are wired withaddEventListener+data-*attributes instead.src="..."unescaped; now goes throughescapeHTML()like every other interpolated value on the page.vercel.json, not a<meta>tag (a<meta>tag silently ignoresframe-ancestors, which I only found by testing it directly — see commit for the correction). Droppedconnect-src'shttps://api.github.comnow that nothing calls it, and addedbase-uri 'self',form-action 'self',frame-ancestors 'self'.<select>had no accessible name; the new axe suite caught it immediately, so it's fixed rather than the CI gate starting red on day one for something unrelated.Testing
Added
website/tests/(Playwright +@axe-core/playwright):rendering.spec.js— every dynamic section renders as real DOM, not escaped markup; specifically re-tests the terminal's.command-textbug and blog-post rendering that were broken ondev.navigation.spec.js— hash-based routing, direct#blog/<id>and#docs/<id>URLs, mobile menu.editor-removal.spec.js— no token input anywhere in the DOM,submitToGithubis undefined, the client never callsapi.github.com, and the export/copy flow works (including the required-fields error path).xss-regression.spec.js— an 8-payload corpus (script tag,onerror,onload,onmouseover,javascript:/data:URLs,iframe srcdoc, style-attribute CSS) run through the live editor preview, plus direct blog-post and docs-page injection tests, all asserting on an actual "did script execute" flag rather than just inspecting the resulting HTML string.accessibility.spec.js— axe scans (WCAG 2 A/AA) across every section, plus a keyboard-only FAQ-toggle test.Two things worth flagging since I hit them directly rather than assuming:
dev):cdn.tailwindcss.com's SRI+CORS combination fails to load in this Playwright/headless environment, andlucide@1.24.0has no"github"icon under that name. Filtered explicitly intests/helpers.jswith a comment explaining why, not silently ignored..hiddenutility class has no actual CSS effect there even though the class token is correctly toggled — several assertions checkclassList.contains('hidden')directly rather than Playwright's rendered-visibility helpers, which would otherwise depend on a third-party CDN loading reliably in CI.Also ran, unchanged logic:
node website/test_toEmbedUrl.mjs— 8/8 passing.Local runs: 34/34 Playwright tests passing across 3 repeated runs (no flakiness observed).
Website (Playwright + axe)job; existingWebsite (script tests)job untouched)Related issue
Closes #297
Checklist
Signed-off-bytrailer (git commit -s; seedocs/dco.md)frontend/per CONTRIBUTING.md,website/has no ESLint config; rannode --checkand the full test suite instead)fix/description