fix(website): remove browser PAT flow, sanitize dynamic rendering, restrict CSP (#297)

[parthrohit22]

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 (blanket innerHTML → 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

• Bug fix
• Dashboard/front-end work

Background

The live site's Blog Editor asked any visitor for a classic GitHub personal access token with repo scope, then called the GitHub API directly from client-side JS using it. A DOM-XSS on that page could expose that token. dev also already carried an incomplete first attempt at the XSS half of this: every innerHTML = had been blindly swapped for textContent =, which stops script execution but also means the terminal's .command-text span never gets created, and blog posts / docs pages render literal <div class="..."> text instead of parsed markup.

What changed

• PAT flow removed entirely — the token input, "Get Token" link, 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 into website/content.js themselves, plus a direct "Open a Pull Request" link. No server-side publishing integration added — deliberately out of scope for this issue.
• Real sanitization, not the blanket textContent workaround — added setSafeHTML() / renderMarkdown() in script.js, backed by DOMPurify (loaded via CDN + SRI, matching the existing marked.js/lucide pattern). 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 reaching innerHTML. 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.
• Inline event handlers migrated off generated markup — DOMPurify's default config strips inline 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 avatar onerror fallback) are wired with addEventListener + data-* attributes instead.
• A real self-XSS fixed along the way — the contributor-preview avatar's GitHub handle was interpolated into src="..." unescaped; now goes through escapeHTML() like every other interpolated value on the page.
• CSP tightened — the site's actual CSP is a real HTTP header in vercel.json, not a <meta> tag (a <meta> tag silently ignores frame-ancestors, which I only found by testing it directly — see commit for the correction). Dropped connect-src's https://api.github.com now that nothing calls it, and added base-uri 'self', form-action 'self', frame-ancestors 'self'.
• One pre-existing accessibility bug fixed — the rules-page framework <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-text bug and blog-post rendering that were broken on dev.
• 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, submitToGithub is undefined, the client never calls api.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:

• Two console messages are pre-existing and unrelated (verified byte-identical against dev): cdn.tailwindcss.com's SRI+CORS combination fails to load in this Playwright/headless environment, and lucide@1.24.0 has no "github" icon under that name. Filtered explicitly in tests/helpers.js with a comment explaining why, not silently ignored.
• Because Tailwind doesn't load in that same environment, its .hidden utility class has no actual CSS effect there even though the class token is correctly toggled — several assertions check classList.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).

• All CI checks pass (added a new Website (Playwright + axe) job; existing Website (script tests) job untouched)
• Returns correct JSON output — n/a, no API/JSON endpoint changed
• No hardcoded credentials or secrets — the whole point of this PR is removing the one place the site asked a visitor for one

Related issue

Closes #297

Checklist

• Every commit includes a DCO Signed-off-by trailer (git commit -s; see docs/dco.md)
• My code follows the rule template in CONTRIBUTING.md — n/a, not a scanner rule; followed the JS coding standards (ESLint scope is frontend/ per CONTRIBUTING.md, website/ has no ESLint config; ran node --check and the full test suite instead)
• I added or updated the matching CLI playbook — n/a, not a scanner rule
• I added or updated all four compliance framework mappings — n/a, not a scanner rule
• I have not committed any real Azure credentials
• My branch name follows the convention: fix/description

[m-khan-97]

@parthrohit22, I am taking the lead security review on this. Before final approval, please rebase onto current dev; the branch is currently two commits behind following #307 and #308. I will review the source and security tests against the rebased head so the evidence matches what would actually merge.

[m-khan-97]

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:

1. The new Playwright suite does not exercise the CSP at all. Its local Python server does not apply vercel.json headers, and there is no assertion for the Content-Security-Policy response header or a securitypolicyviolation event. 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.

2. script-src still permits unsafe-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 remove unsafe-inline from script-src. If Tailwind requires inline styles, keep that decision isolated to style-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]

Requesting changes for two findings not covered by the existing review thread, both verified locally on 153a6fc.

1. The test script in website/package.json is broken. It runs node --test tests/toEmbedUrl.test.mjs, but that file does not exist (the real test lives at website/test_toEmbedUrl.mjs). Running npm test inside website/ fails with Could not find tests/toEmbedUrl.test.mjs. CI never invokes npm 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.

2. 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.

[parthrohit22]

@m-khan-97 @ritiksah141 Both blocking findings and the two additional ones are addressed on the current head, rebased onto dev:

Your two blockers, m-khan-97:

1. CSP was never exercised. Added tests/csp_server.py, a small server that actually applies vercel.json's real header rules (including Cache-Control on /assets/* stacking with the site-wide security headers, matching Vercel's real multi-rule-match behavior) — wired into playwright.config.js as the webServer command, so every spec in the suite now runs against production-representative headers, not just the new one. New tests/security.spec.js asserts the real 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, nothing executes), and that the page's own scripts still work normally under the real header.
2. unsafe-inline removed from script-src. The two inline <script> blocks moved to theme-init.js/tailwind-config.js (same-origin, same document position, so execution order/timing is unchanged). Every remaining static onclick/onchange/oninput attribute (~30 of them) is now wired in script.js via addEventListener, off data-* attributes or existing ids — the same pattern this file's dynamically-generated markup already used for the 4 cases DOMPurify required fixing earlier in this PR. style-src keeps unsafe-inline (Tailwind's runtime), unchanged.

Your two findings, ritiksah141:
3. website/package.json's test script pointed at a file that doesn't exist — fixed the path, verified npm test now runs and passes.
4. The accessibility flake — root-caused exactly as you described (the 300ms-setTimeout-vs-rAF race in showSection()). Rewrote goToSection() in tests/helpers.js to wait for DOM state (the active class + the element's own inline display) instead of rendered visibility, matching the pattern already used for the mobile-menu/FAQ tests.

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 (navigation.spec.js had a button[onclick*=...] selector — fixed to use the new data-nav-section attribute), and the new document.querySelector() calls in script.js broke the Node vm-based toEmbedUrl() unit test's minimal DOM stub — added the missing stub method.

Verified: full backend suite (845 passed, 5 skipped — pre-existing/environment-only), npm test, ruff clean, and now — since I can't run a live browser locally in this environment — the real CI run: all 20 checks green, including Website (Playwright + axe), which is the one that actually exercises everything above end to end.

[m-khan-97]

Parth, I verified the current head end to end. The browser PAT flow is removed; dynamic HTML and Markdown sinks remain centralized through DOMPurify; the inline script blocks and static event-handler attributes are gone; and script-src no longer permits unsafe-inline. The Playwright server now applies the actual vercel.json headers, and the security suite proves both sides of the boundary: injected inline script is blocked with a CSP violation while the site’s allowed scripts and navigation continue to work. Ritik’s additional findings are also closed: npm test targets the real file and the accessibility wait no longer depends on the flaky rendered-visibility race.

I ran this head independently: the unit suite passed, followed by all 37 Playwright/axe/CSP/XSS tests. GitHub’s 21 checks are green as well. Approving.

[m-khan-97]

@ritiksah141, your two additional blockers are fixed on 66778fc: npm test now points to the real test file, and the accessibility wait no longer relies on the flaky rendered-visibility race. I independently ran the unit suite and all 37 Playwright/axe/CSP/XSS tests successfully and approved the current head. Please rereview and clear your remaining change request if your verification agrees.

[parthrohit22]

@ritiksah141 Both your findings — the broken npm test path and the accessibility-spec flake — were fixed in the same commit m-khan-97 just re-verified and approved on (66778fc): the script now points at the real test_toEmbedUrl.mjs, and goToSection() waits on DOM state instead of the rendered-visibility race that was causing the parallel-run timeouts. Nothing further to do on those two — whenever you get a chance, a fresh look would be great.

[parthrohit22]

@ritiksah141 this branch is now rebased onto the latest dev (was 1 commit behind #279), CI re-running.

Re-review requested — your two findings from the 153a6fc review look resolved on the current tip:

1. website/package.json test script — now node --test test_toEmbedUrl.mjs, which points at the file that actually exists. npm test inside website/ passes (10/10 assertions).
2. accessibility.spec.js parallel flake — goToSection() in tests/helpers.js now waits on DOM state (active class + inline display not none) instead of rendered visibility, matching the mobile-menu/FAQ pattern, so it no longer races the 300ms setTimeout vs rAF.

The CSP points from the other review are also covered: the browser suite now exercises the CSP response header, and unsafe-inline is gone from script-src.

[TFT444]

Thank you for removing 'unsafe-inline' from script-src and adding form-action 'self'. Both are real improvements.

One issue remains: frame-ancestors 'self' in a CSP tag is silently ignored by all major browsers per the CSP Level 2 spec. Only HTTP response headers enforce this directive. Since GitHub Pages cannot set HTTP response headers, this directive provides no clickjacking protection regardless of where it appears in the document.

The current state is worse than simply omitting it: verify-site.mjs now checks that frame-ancestors 'self' is present in the meta CSP, so CI reports this as verified when the browser ignores it entirely. That gives false confidence that clickjacking protection exists.

Please either remove frame-ancestors 'self' from both the CSP string and the verify check and add a comment explaining the GitHub Pages limitation, or keep the directive but update the verify check to note the limitation and not treat its presence as security evidence. The first option is cleaner.

[m-khan-97]

Parth, I reassessed fba8ae9 after the Astro migration. My previous approval concerned a different implementation and its HTTP-header test server; it cannot validate this new meta-policy implementation.

Tanvir's finding is still present: Base.astro includes frame-ancestors in a meta CSP, and verify-site.mjs requires it as if it were effective protection. Browsers do not enforce frame-ancestors from meta policies (https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy/frame-ancestors).

Please remove that directive from the meta policy and its required-directive check, and document the hosting limitation. If clickjacking protection is required, supply it through an actual HTTP response header on supported hosting and verify that response. Keep the supported script-src/form-action improvements. Please also verify the built Astro site in a browser so tightening script-src does not break its actual scripts.

[parthrohit22]

@TFT444 @m-khan-97 Re-review requested for the meta-CSP finding. The current head removes frame-ancestors from the document-level CSP and from verify-site.mjs; the layout and README now explicitly state that GitHub Pages cannot provide the HTTP header required for clickjacking protection. The verifier therefore checks only directives browsers enforce from a meta policy, while retaining script-src 'self' and form-action 'self'.\n\nValidation completed: npm run check passes locally for both CMS configurations, and all required GitHub checks are passing. There is no browser-test job configured for this Astro site; I have not represented the static build check as an interactive browser verification.