From fc9d44053420245c36f86b5330d76053966b132d Mon Sep 17 00:00:00 2001 From: parthrohit22 Date: Fri, 28 Aug 2026 17:07:59 +0100 Subject: [PATCH 1/4] fix(website): remove browser PAT flow, sanitize dynamic rendering, restrict 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
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 #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 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 + - + + + + Open a Pull Request +
-

Tokens are never stored. Requires repo scope to create branches and PRs.

+ +

diff --git a/website/package-lock.json b/website/package-lock.json new file mode 100644 index 00000000..b32d94c6 --- /dev/null +++ b/website/package-lock.json @@ -0,0 +1,102 @@ +{ + "name": "openshield-website", + "version": "0.0.0", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "openshield-website", + "version": "0.0.0", + "devDependencies": { + "@axe-core/playwright": "^4.10.2", + "@playwright/test": "^1.56.0" + } + }, + "node_modules/@axe-core/playwright": { + "version": "4.13.0", + "resolved": "https://registry.npmjs.org/@axe-core/playwright/-/playwright-4.13.0.tgz", + "integrity": "sha512-6YLx+kxXu5GJceG4ozFg+33a2EMTdjYwWGloJ3sb9Kta5pp+ZNS53uxGVog5JetIY8s++P5UrtX+cri+u0VAVg==", + "dev": true, + "license": "MPL-2.0", + "dependencies": { + "axe-core": "~4.13.0" + }, + "peerDependencies": { + "playwright-core": ">= 1.0.0" + } + }, + "node_modules/@playwright/test": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.62.1.tgz", + "integrity": "sha512-DTcUc8qii+cpHvtOwggMtBRMjKZHXYWdw8syRYu2vtzuq4Wxphqq4NfCs5Zt44L6mA8rfDfj+PHnxFc/FeK6mQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright": "1.62.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, + "node_modules/axe-core": { + "version": "4.13.0", + "resolved": "https://registry.npmjs.org/axe-core/-/axe-core-4.13.0.tgz", + "integrity": "sha512-UzGt8zg7Ny8djbYMhxl2zuEevVa7r2gJjYY5Lwr1xM7+XU2nd6CkIWFTVcCIbAP63vSz71NaVyyuSk9lHKcy0A==", + "dev": true, + "license": "MPL-2.0", + "engines": { + "node": ">=4" + } + }, + "node_modules/fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": "^8.16.0 || ^10.6.0 || >=11.0.0" + } + }, + "node_modules/playwright": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.62.1.tgz", + "integrity": "sha512-0M+L3LAD8/nm554LOla9Ayx0j0tmFZ0FBcoQ7F1VuVHpM/XpiC8RcDzBQB8W5+hA8L22THxELzeF+2WcUzvcLg==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright-core": "1.62.1" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + }, + "optionalDependencies": { + "fsevents": "2.3.2" + } + }, + "node_modules/playwright-core": { + "version": "1.62.1", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.62.1.tgz", + "integrity": "sha512-wPYSwEBJY9GHraISXqyqtx0na0LpO3XEX7jNDhntbex7tzUS7kLnZsOlFruFJB4Hi/rhDMjXGqHewDZ68nYZVw==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=20" + } + } + } +} diff --git a/website/package.json b/website/package.json new file mode 100644 index 00000000..26cd5a9b --- /dev/null +++ b/website/package.json @@ -0,0 +1,15 @@ +{ + "name": "openshield-website", + "private": true, + "version": "0.0.0", + "type": "module", + "scripts": { + "test": "node --test tests/toEmbedUrl.test.mjs", + "test:e2e": "playwright test", + "test:e2e:install": "playwright install --with-deps chromium" + }, + "devDependencies": { + "@axe-core/playwright": "^4.10.2", + "@playwright/test": "^1.56.0" + } +} diff --git a/website/playwright.config.js b/website/playwright.config.js new file mode 100644 index 00000000..1461ea60 --- /dev/null +++ b/website/playwright.config.js @@ -0,0 +1,20 @@ +import { defineConfig } from '@playwright/test'; + +export default defineConfig({ + testDir: './tests', + testMatch: '**/*.spec.js', + fullyParallel: true, + forbidOnly: !!process.env.CI, + retries: process.env.CI ? 1 : 0, + reporter: process.env.CI ? 'list' : 'html', + use: { + baseURL: 'http://127.0.0.1:4173', + trace: 'retain-on-failure', + }, + webServer: { + command: 'python3 -m http.server 4173 --bind 127.0.0.1', + url: 'http://127.0.0.1:4173/index.html', + reuseExistingServer: !process.env.CI, + timeout: 15000, + }, +}); diff --git a/website/script.js b/website/script.js index a54b0c0c..f7b2f94b 100644 --- a/website/script.js +++ b/website/script.js @@ -14,17 +14,39 @@ function escapeHTML(str) { return p.innerHTML; } +// Every dynamically-built HTML string - whether it's this file's own +// template markup or markdown rendered from a blog post / the live editor +// preview - goes through this before it ever reaches innerHTML. DOMPurify's +// default config strips ' }, + { name: 'img onerror', markdown: '' }, + { name: 'svg onload', markdown: '' }, + { + // hit staying undefined alone wouldn't prove much here - nothing in + // this test clicks the link, so a survived javascript: href would + // pass that check regardless. Verified directly (not guessed) that + // DOMPurify drops the href attribute entirely rather than leaving a + // neutered one: the tag survives as a bare click me. + name: 'javascript: link', + markdown: '[click me](javascript:window.__xssHit="jsurl")', + assertOutput: (html) => expect(html).not.toMatch(/]*href/i), + }, + { name: 'iframe srcdoc', markdown: '' }, + { + // DOMPurify allows the style attribute through (its URI allowlist + // only governs attributes actually interpreted as a URI, e.g. + // href/src) - the literal text "javascript:" can legitimately + // survive inside a style attribute's CSS. That's not a bypass: no + // current browser executes url(javascript:...) in CSS, which is + // exactly what `hit` staying undefined above already proves. + name: 'style expression', + markdown: '
x
', + }, + { name: 'event handler on real tag', markdown: '

hover me

' }, + { + // Same reasoning as the javascript: link case above - verified + // directly that the href is dropped entirely, not neutered. + name: 'data: URL script', + markdown: 'link', + assertOutput: (html) => expect(html).not.toMatch(/]*href/i), + }, +]; + +test.describe('XSS regression corpus', () => { + for (const { name, markdown, assertOutput } of PAYLOADS) { + test(`${name} payload does not execute in the live editor preview`, async ({ page }) => { + await gotoAndInit(page); + await page.addInitScript(() => { window.__xssHit = undefined; }); + await goToSection(page, 'blog-editor'); + await page.selectOption('#edit-type', 'blog'); + await page.fill('#edit-title', 'XSS Test'); + await page.fill('#edit-content', markdown); + await page.waitForTimeout(150); // debounce the input-driven preview render + + const hit = await page.evaluate(() => window.__xssHit); + expect(hit, `payload executed: ${name}`).toBeUndefined(); + + // The load-bearing assertion is `hit` above (proves nothing + // executed) - these are structural sanity checks on top of it, + // not a substitute. Deliberately not asserting the sanitized + // output never contains the literal text "javascript:" anywhere: + // DOMPurify allows a style attribute's CSS text through (its + // ALLOWED_URI_REGEXP only blocks javascript: in an attribute + // that's actually interpreted as a URI, e.g. href/src), and no + // current browser executes url(javascript:...) inside CSS - a + // real payload text surviving there is inert, not a bypass. + const previewHTML = await page.locator('#editor-preview').innerHTML(); + expect(previewHTML).not.toMatch(/Also has ', + }); + showBlogPost('malicious-test-post'); + }); + const hit = await page.evaluate(() => window.__xssHit); + expect(hit).toBeUndefined(); + }); + + test('the docs page sanitizes the same way', async ({ page }) => { + await gotoAndInit(page); + await page.addInitScript(() => { window.__xssHit = undefined; }); + await page.evaluate(() => { + siteContent.docs.unshift({ + id: 'malicious-doc', + title: 'Malicious Doc', + content: '', + }); + showDocPage('malicious-doc'); + }); + const hit = await page.evaluate(() => window.__xssHit); + expect(hit).toBeUndefined(); + }); +}); diff --git a/website/vercel.json b/website/vercel.json index b12e58a0..7efe6987 100644 --- a/website/vercel.json +++ b/website/vercel.json @@ -16,7 +16,7 @@ { "key": "Permissions-Policy", "value": "camera=(), microphone=(), geolocation=()" }, { "key": "Content-Security-Policy", - "value": "default-src 'self'; script-src 'self' 'unsafe-inline' https://cdn.tailwindcss.com https://unpkg.com https://cdn.jsdelivr.net https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com https://fonts.googleapis.com; font-src 'self' https://fonts.gstatic.com https://cdnjs.cloudflare.com; img-src 'self' data: https://github.com https://avatars.githubusercontent.com; frame-src https://www.youtube.com https://player.vimeo.com; connect-src 'self' https://api.github.com; object-src 'none';" + "value": "default-src 'self'; script-src 'self' 'unsafe-inline' https://cdn.tailwindcss.com https://unpkg.com https://cdn.jsdelivr.net https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com https://fonts.googleapis.com; font-src 'self' https://fonts.gstatic.com https://cdnjs.cloudflare.com; img-src 'self' data: https://github.com https://avatars.githubusercontent.com; frame-src https://www.youtube.com https://player.vimeo.com; connect-src 'self'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'self';" } ] } From 153a6fc2deb4570841f6e06990c5041a5d989ba2 Mon Sep 17 00:00:00 2001 From: parthrohit22 Date: Fri, 28 Aug 2026 17:11:22 +0100 Subject: [PATCH 2/4] fix(website): parse the actual host instead of a URL substring match 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 --- website/tests/editor-removal.spec.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/website/tests/editor-removal.spec.js b/website/tests/editor-removal.spec.js index a199df3a..04f28508 100644 --- a/website/tests/editor-removal.spec.js +++ b/website/tests/editor-removal.spec.js @@ -22,7 +22,19 @@ test.describe('The browser-based GitHub token flow is removed', () => { test('the client never calls the GitHub API', async ({ page }) => { const apiCalls = []; page.on('request', (req) => { - if (req.url().includes('api.github.com')) apiCalls.push(req.url()); + // Parse the actual host rather than a substring match on the + // full URL - a plain .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 (flagged by CodeQL - fixing it for real, not just to + // clear the check). + let host; + try { + host = new URL(req.url()).hostname; + } catch { + return; + } + if (host === 'api.github.com') apiCalls.push(req.url()); }); await gotoAndInit(page); await goToSection(page, 'blog-editor'); From 96b1fd03a0aa9137b10f154964f5b734c3c7ca98 Mon Sep 17 00:00:00 2001 From: parthrohit22 Date: Sat, 29 Aug 2026 14:20:44 +0100 Subject: [PATCH 3/4] fix(website): exercise CSP in tests, remove unsafe-inline, fix flake 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 + @@ -47,36 +44,7 @@ - +