fix(website): harden Astro CSP and verify the built output against it

[parthrohit22]

What does this PR do?

Hardens the Content Security Policy of the Astro website and adds build-time verification that keeps the policy and the built output in agreement.

Refs #297.

Type of change

• Security hardening
• Dashboard/front-end work

Scope note

This PR was opened before #329 migrated the website to Astro. The original work (removing the browser PAT flow, DOMPurify sanitization of innerHTML sinks, a vercel.json header CSP, the Playwright suites under website/tests/) targeted files that no longer exist: the migration deleted the browser editor, the client-side renderer and the PAT flow outright, so those changes were obsoleted rather than merged. The description below reflects only what this diff actually contains against current dev.

Closes #297 has been downgraded to Refs #297. Items 1-4 of that issue (PAT flow, innerHTML renderer) no longer have any code to fix after #329, but its Playwright/axe acceptance criteria are still open and this diff does not satisfy them, so the issue must not auto-close on merge. Rescoping or closing #297 is a maintainer decision that belongs on the issue itself.

What changed

src/layouts/Base.astro — CSP tightening

• script-src 'self' 'unsafe-inline' → script-src 'self'. With Astro emitting every hoisted script as an external same-origin file, unsafe-inline buys nothing and removing it makes the policy a real fallback against injected inline script.
• Added form-action 'self'. There is no <form> in any .astro source today, so this blocks nothing currently — pure hardening.
• connect-src keeps https://api.github.com: the hoisted script in this layout fetches the repository for the star count. It is required, not vestigial.
• frame-ancestors is deliberately not present. Browsers ignore it in a <meta http-equiv> policy, and GitHub Pages cannot set the HTTP response header that would make it effective. The comment in the file and website/README.md state plainly that clickjacking protection is not in place and needs a real response header from a configurable edge.

scripts/verify-site.mjs — production-build ratchets

• Asserts the built pages carry a CSP meta policy and that its script directive is exactly script-src 'self'. An exact match, not an "absence of unsafe-inline" check, so a future remote source also fails verification instead of quietly weakening the policy.
• Asserts object-src 'none', base-uri 'self' and form-action 'self' are present. It deliberately does not assert frame-ancestors, so CI never reports coverage that does not exist.
• Scans the built HTML for executable inline <script> blocks, which script-src 'self' would silently block on the deployed site. Data blocks (application/json, application/ld+json, speculationrules) are exempt because they are not executed. The scan walks tag indices by hand rather than using a tag-filter regex (CodeQL js/bad-tag-filter).
• Asserts every external script src is same-origin. A remote src previously passed verification and would then be CSP-blocked in the browser — the same build-green-but-site-broken case the inline scan exists to prevent.

astro.config.mjs

• Keeps inlineStylesheets: 'never' and vite.build.assetsInlineLimit: 0 so nothing gets inlined into the HTML.
• Removes the top-level build.assetsInlineLimit key: Astro's build schema has no such key and zod strips it silently, so it was dead config. The comments here and in the verifier now point at the setting that actually takes effect.

Docs

• website/README.md documents the meta-policy limitation honestly.
• docs/input-validation-audit.md had a row pointing at website/test_toEmbedUrl.mjs, which website: migrate to Astro + Decap CMS with GitHub Pages pipeline #329 removed along with the browser editor it tested. Both editor-era rows are replaced with the build-time content model that is actually in place.

Testing

• npm run check in website/ — exactly what CI runs (build, configure-cms, verify against both CMS-disabled and CMS-enabled output): passes, 14 pages, largest JS asset 482 KiB.

• Non-vacuity checked in both directions on the built output: planting <script>alert(1)</script> into dist/index.html fails verification; planting <script src="https://cdn.example.com/x.js"> fails with the new same-origin message; flipping the built CSP to script-src 'self' 'unsafe-inline' fails the exact-match ratchet; restoring passes again.

• The three application/ld+json blocks and the application/json orb-data block are correctly exempted, and all four hoisted scripts are emitted as external files under /openshield/_astro/.

• All CI checks pass

• Returns correct JSON output — n/a, no API/JSON endpoint changed

• No hardcoded credentials or secrets

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

[parthrohit22]

@TFT444 @m-khan-97 — re-review please. The frame-ancestors finding is fixed on f2e79c8, and I've now done the browser verification @m-khan-97 asked for, which I had not done when I last commented. Details below.

frame-ancestors removed (option 1, as @TFT444 preferred)

Removed from the policy itself and from the verifier's required-directive list, rather than kept with a caveat — a directive browsers ignore shouldn't be in the string at all.

Shipped policy in Base.astro:

default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';
font-src 'self' data:; img-src 'self' data:; connect-src 'self' https://api.github.com;
object-src 'none'; base-uri 'self'; form-action 'self'

verify-site.mjs now requires only directives a meta policy actually enforces:

for (const directive of ['object-src \'none\'', 'base-uri \'self\'', 'form-action \'self\'']) {

with a comment on both sides explaining why frame-ancestors is deliberately absent, and the hosting limitation documented in website/README.md: GitHub Pages cannot set response headers, so clickjacking protection needs a real Content-Security-Policy or X-Frame-Options HTTP header at the edge if the site ever moves to configurable hosting. So CI no longer reports protection that doesn't exist — which was your actual objection, and the right one.

Browser verification of the built Astro site

@m-khan-97 — you asked me to verify the built site in a browser so tightening script-src doesn't break its scripts, and you were right that npm run check can't answer that. I built dist/, served it at the real /openshield/ base path, and drove it in a browser.

The site's own scripts run fine under script-src 'self'. Zero console errors, zero securitypolicyviolation events on load. All four scripts are same-origin files; nothing inline survives:

{ "inlineScriptBlocks": 0, "inlineHandlerAttrs": 0,
  "externalScripts": ["/openshield/_astro/Hero.astro_…js", "/openshield/_astro/DemoSection.astro_…js",
                      "/openshield/_astro/Base.astro_…js", "/openshield/_astro/index.astro_…js"],
  "frameAncestorsInMeta": false }

Interactive behaviour exercised, all working, no violations:

Control	Result
Nav toggle	aria-expanded false → true
Demo carousel "Next rule"	advanced AZ-STOR-001 → AZ-NET-001
"Pause motion"	toggled to "Resume motion"
Hero three.js canvas	rendering, non-zero dimensions
Rules page search	253 → 25 entries for storage
Rules page domain filter	domainFilter=net applied

And the policy is actually enforced, not just present — injecting an inline script gets blocked:

{ "inlineScriptExecuted": false,
  "violations": ["script-src-elem <- inline", "img-src <- https://evil.example.com/x.png"] }

Swept every page (/, /rules/, /docs/, /architecture/, /blog/, /community/, /evidence/, 404): CSP present on all, frame-ancestors on none, zero inline script blocks, zero inline on* handler attributes.

npm run check passes for both CMS configurations (14 pages verified, largest JS asset 482 KiB), and all 21 checks are green on f2e79c8.

One thing I want to be accurate about: this was a manual browser run on my machine, not a CI job — there is still no automated browser suite for the Astro site. The earlier Playwright/CSP suite covered the pre-migration implementation and its HTTP-header test server, which is exactly why you withdrew the approval. If you'd like that coverage restored against the meta-policy build before this merges, say so and I'll add it here rather than as a follow-up.

[TFT444]

Re-reviewed. The frame-ancestors concern is resolved: it has been removed from the meta CSP and the verify script documents why meta policies cannot enforce it. However, Ritik's finding still stands: website/package.json test script references tests/toEmbedUrl.test.mjs which does not exist. Please fix that broken test reference before merge.

[parthrohit22]

@TFT444 @m-khan-97 @ritiksah141 addressed the remaining CSP-verifier concern in ee71795. The production verifier now requires exactly script-src 'self', so remote script sources cannot silently weaken the policy.

[ritiksah141]

Requesting changes on the PR scope and description only. The code change itself is sound and I verified it locally end to end; what blocks merge in the current state is that this PR would close #297 against acceptance criteria the diff does not meet, with a description that still describes code that no longer exists in the repo.

Blocker: "Closes #297" plus a description for the wrong codebase

Issue #297 is flagged as a release blocker and its acceptance criteria explicitly require:

• Playwright tests covering rendering, navigation, editor removal, CSP violations and an XSS regression corpus.
• Keyboard and axe checks running in CI.

This diff contains neither (no workflow changes, no test files). Merging with Closes #297 in the description would auto-close a security issue that is still open on those criteria.

The rest of the description describes the pre-migration static site: PAT flow removal in script.js, DOMPurify sanitization, a vercel.json CSP, Playwright suites under website/tests/, test_toEmbedUrl.mjs passing 8/8. All of those files were removed when #329 migrated the website to Astro, and none of that work appears in the final diff. What the diff actually is, in 4 files and +75/-6: the CSP tightening in Base.astro, external-asset build settings in astro.config.mjs, production-build assertions in verify-site.mjs, and README documentation.

One description claim is also factually wrong against the final diff: it says https://api.github.com was dropped from connect-src because nothing calls it. The site still calls it (the star-count fetch in the hoisted Base.astro script), and the CSP correctly keeps it.

What I would like before approving:

1. Rewrite the description to describe the actual diff: CSP hardening for the Astro site, the verifier ratchets, and the honest frame-ancestors documentation.
2. Drop Closes #297 or downgrade it to Part of #297 / Refs #297. If the position is that the issue should close, that is a maintainer rescoping decision that belongs on the issue: required-work items 1 through 4 were obsoleted by #329 (no PAT flow and no innerHTML renderer survive in the new site), but the Playwright/axe acceptance criteria remain open and need a tracking home.

Non-blocking findings

1. Top-level build.assetsInlineLimit: 0 in astro.config.mjs is dead config. Astro 7.3.1's build schema carries only format, assets, assetsPrefix, serverEntry, redirects, inlineStylesheets and concurrency, and zod strips unknown keys silently. The effective setting is vite.build.assetsInlineLimit: 0, which you also set. The comments in astro.config.mjs and verify-site.mjs both point at the dead key; remove the key or correct the comments.
2. The verifier excludes /admin/, and the generated Decap admin shell carries no CSP at all. The exclusion is necessary (decap-cms.min.js loads from jsdelivr and could never pass script-src 'self'), but a dedicated admin CSP would be a good follow-up: script-src 'self' https://cdn.jsdelivr.net, connect-src https://api.github.com, style-src with unsafe-inline. Today only SRI and the robots.txt exclusion protect that page.
3. The inline-script scanner does not assert that external script src values are same-origin. A remote-src script would pass verification and be silently CSP-blocked at runtime, which is the same build-green-but-site-broken class the scan exists to prevent. A one-line check that non-admin srcs start with /openshield/ would close it.
4. Theoretical scanner limits, fine to defer: an attribute value containing > truncates the attrs scan, and a literal </script> inside a JSON data block would break out in the browser because JSON.stringify does not escape <. The data is repo-static and the new CSP bounds the blast radius. Optional hardening: escape < as < in the data blocks fed to set:html.

What I verified locally

• npm run check (exactly what CI runs: build, configure-cms, verify against both CMS-disabled and CMS-enabled output): passes, 14 pages, largest JS asset 482 KiB against the 520 KiB budget.
• Non-vacuity of the new assertions, in both directions: planting <script>alert(1)</script> into the built index.html fails the verifier; restoring and flipping the built CSP to script-src 'self' 'unsafe-inline' fails the exact-match ratchet with exit 1; restoring passes again.
• The built output confirms the model: the three application/ld+json blocks and the application/json orb-data block are correctly exempted by the scanner, and all four hoisted scripts are emitted as external module files under /openshield/_astro/.
• The exact-match script-src ratchet is the right guard: a future remote source or a reintroduced unsafe-inline fails verification instead of quietly weakening the policy.
• connect-src https://api.github.com is required, not vestigial: the hoisted script in Base.astro fetches the repo for the star count, uses textContent and swallows failures.
• form-action 'self' blocks nothing today (there is no form element in any .astro source), so it is pure hardening.
• The README and Base.astro comments are accurate about frame-ancestors, report-uri/report-to and sandbox being ignored in a meta policy, and about clickjacking protection not actually being in place on GitHub Pages. The verifier deliberately not asserting frame-ancestors, so CI never reports coverage that does not exist, is exactly the right call.

The security substance here is good work. Once the description matches the diff and the #297 closure is handled explicitly, I would switch to approve.

[TFT444]

@parthrohit22 two blockers remain before this can merge:

1. website/package.json test script references tests/toEmbedUrl.test.mjs which does not exist. This was flagged on Sep 14 and is still unaddressed. Please fix the path to point at the existing test file.
2. The PR description describes the old DOMPurify/PAT-removal flow, not the actual changes in this diff (Astro CSP hardening, inline-script prevention, verifier ratchets). Please rewrite the description to match the diff, and change Closes #297 to Refs #297 since the Playwright/axe criteria from that issue were superseded by the Astro migration.

The security substance of the code is correct. Once these two are fixed this is approvable.

[parthrohit22]

@TFT444 @ritiksah141 both blockers are addressed, plus two of Ritik's non-blocking findings.

1. The broken npm test reference. There is no test script in website/package.json on this head any more — #329's Astro migration removed it along with test_toEmbedUrl.mjs and the browser editor that file tested, so the dangling path that failed on 14 Sep is gone with it. What did survive is a stale prose reference: docs/input-validation-audit.md still listed website/test_toEmbedUrl.mjs as the enforcement for website media URLs. That row and the "Website editor text" row next to it both described the deleted editor, so both are replaced with the build-time content model actually in place (Decap commits Markdown, Astro renders it at build time, no browser-side DOM sink). grep -rn toEmbedUrl now returns nothing.

2. Description and issue linkage. Rewritten from scratch to describe this diff only — the CSP tightening in Base.astro, the verifier ratchets, the astro.config.mjs build settings and the honest frame-ancestors documentation — with a scope note explaining that the original PAT-removal/DOMPurify work was obsoleted rather than merged by #329. Closes #297 is now Refs #297, and the description says plainly that the Playwright/axe acceptance criteria are still open and that rescoping #297 is a maintainer decision on the issue. Ritik — you were right about connect-src: the star-count fetch in Base.astro does call api.github.com, the old description's claim was wrong, and the new one says the directive is required rather than vestigial.

Also fixed, from the non-blocking list:

• Dead config ([CORE] Scanner engine + Azure client + 10 misconfiguration rules #1). Removed the top-level build.assetsInlineLimit key. Astro's build schema has no such key and zod strips it silently, so vite.build.assetsInlineLimit was always the effective setting. The comments in astro.config.mjs and verify-site.mjs now point at the key that actually does the work.
• Remote script srcs ([API] Flask REST API + PostgreSQL database schema #3). The verifier now asserts every external script src is same-origin. Checked non-vacuously: planting <script src="https://cdn.example.com/x.js"> into dist/index.html fails with index.html loads script https://cdn.example.com/x.js, which script-src 'self' will block on the deployed site; restoring passes.

Findings #2 (a dedicated CSP for the Decap admin shell) and #4 (the scanner's theoretical attribute/</script> limits) are follow-up material rather than something to bundle into this diff — happy to open an issue for the admin CSP if you'd like it tracked.

npm run check passes: 14 pages, largest JS asset 482 KiB against the 520 KiB budget. Re-requesting review from you both.