From f77c3eedd929fac333db53ecc4bc4f08ab6bb765 Mon Sep 17 00:00:00 2001 From: Paul Lizer Date: Tue, 29 Sep 2026 18:49:43 -0400 Subject: [PATCH 1/2] Extend the XSS PR guardrail to the V2 React/TypeScript UI scripts/check_xss_sinks.py now inspects .ts, .tsx, .jsx and .mjs files with a small TypeScript/JSX lexer and eleven React-aware rules: raw HTML through dangerouslySetInnerHTML or DOM HTML sinks, code evaluation, javascript: URLs, dynamic JSX and DOM URL attributes, navigation sinks, raw-HTML markdown options, and ports of the inline-handler, attribute-interpolation and marked.parse checks. Declaration files and application/v2_ui/public/vendor/ are skipped. Output for existing .js, .html and .py files is unchanged apart from the vendored V2 libraries, which are now skipped too. A full-mode run over the 462 inspected V2 source files goes from 59 raw hits to 28 after tuning, and every remaining hit has been reviewed as safe. The per-finding reasons are in XSS_PR_GUARDRAILS_V2_BASELINE_TRIAGE.md. xss-sink-check.yml now covers the new suffixes. It, broken-access-control-check, swagger-route-check, python-syntax-check, malicious-pr-security-review and codeql (push and pull_request) also run for paullizer-react-v2-ui. Each entry is marked temporary until the V2 branch merges into Development, following the feature/admin-settings-ia precedent. xss-full-scan.yml adds an on-demand whole-file scan that mirrors the Broken Access Control full scan. The XSS instruction applies to the new file types and gains a React/TypeScript section. The audit prompt covers the V2 source and batches its file list, because the whole-application list is longer than the Windows command-line limit. The feature doc gains a V2 section, and the Broken Access Control and Swagger docs note the temporary trigger. The self-test gains eight TypeScript tests that raise explicitly, so they still check under python -O. Tooling only: no application change and no version bump. Refs #1571 Refs #1543 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../xss-prevention.instructions.md | 71 +- .../prepare-for-pull-request.prompt.md | 5 +- .../xss-full-audit-and-remediation.prompt.md | 49 +- .../workflows/broken-access-control-check.yml | 1 + .github/workflows/codeql.yml | 8 +- .../malicious-pr-security-review.yml | 1 + .github/workflows/python-syntax-check.yml | 1 + .github/workflows/swagger-route-check.yml | 1 + .github/workflows/xss-full-scan.yml | 97 + .github/workflows/xss-sink-check.yml | 11 + .../v0.241.001/SWAGGER_PR_ROUTE_CHECK.md | 1 + .../BROKEN_ACCESS_CONTROL_PR_GUARDRAILS.md | 2 + .../features/v0.241.022/XSS_PR_GUARDRAILS.md | 150 +- .../XSS_PR_GUARDRAILS_V2_BASELINE_TRIAGE.md | 153 + .../test_xss_guardrails_checker.py | 456 ++- scripts/check_xss_sinks.py | 2515 ++++++++++++++++- 16 files changed, 3499 insertions(+), 23 deletions(-) create mode 100644 .github/workflows/xss-full-scan.yml create mode 100644 docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS_V2_BASELINE_TRIAGE.md diff --git a/.github/instructions/xss-prevention.instructions.md b/.github/instructions/xss-prevention.instructions.md index aa9d156d0..6c88c2347 100644 --- a/.github/instructions/xss-prevention.instructions.md +++ b/.github/instructions/xss-prevention.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: '**/*.js, **/*.html, **/*.py' +applyTo: '**/*.js, **/*.html, **/*.py, **/*.ts, **/*.tsx, **/*.jsx, **/*.mjs' --- # Security: XSS Prevention and Browser Rendering @@ -110,19 +110,78 @@ When a static HTML shell is genuinely simpler, it is acceptable only if: - It contains no `${...}` interpolation or dynamic concatenation - Untrusted values are populated afterward with `textContent`, `setAttribute(...)`, or `dataset` +## React and TypeScript (V2 UI) + +The V2 UI in `application/v2_ui/src` is React 18 with TypeScript. React removes most string-to-HTML risk, but not all of it, so the same trust-boundary thinking applies. + +### What React 18 escapes + +- Text rendered as JSX children, such as `

{message.text}

`, is inserted as text, never parsed as HTML. +- String values in attributes are escaped, so a quote in a value cannot break out of the attribute. + +### What React 18 does not protect + +- **URL-bearing props.** React 18 only logs a development warning for a `javascript:` URL in `href`, `src`, `action` or `formAction`; React 19 blocks it, React 18 does not. Treat these props, plus `srcDoc`, `xlinkHref` and router `to` on `Link`, `NavLink` and `Navigate`, as sinks. +- **`dangerouslySetInnerHTML`.** The value is parsed as HTML with no escaping. +- **Refs and DOM APIs.** `ref.current.innerHTML`, `outerHTML`, `insertAdjacentHTML`, `document.write`, `createContextualFragment`, and `DOMParser` output appended to the page all bypass React. +- **DOM URL properties.** `element.href`, `element.src`, `form.action`, `frame.srcdoc` and `setAttribute('href', ...)` are not React props, so React never inspects them. +- **Navigation.** `window.open(url)`, `location.href = url`, `location = url`, and `location.assign(url)` or `location.replace(url)` can run a `javascript:` URL with the page's origin. +- **String-to-code APIs.** `eval`, `new Function`, and `setTimeout`/`setInterval` with a string argument. + +### Required patterns + +- Render untrusted text as JSX children. Never route text through `dangerouslySetInnerHTML` to get formatting. +- Use `dangerouslySetInnerHTML` only with the output of `DOMPurify.sanitize(...)`, either inline or through a clearly named `const`. Rich renderers must also run their own safe mode first: KaTeX with `trust: false`, Mermaid with `securityLevel: 'strict'` and `htmlLabels: false`. +- Give every dynamic URL prop or navigation value one of these safe sources: + - a literal, or a same-origin path built from a literal prefix and `encodeURIComponent(...)` segments; + - a reviewed URL builder (the lists are `TS_SAME_ORIGIN_URL_BUILDERS` and `TS_HREF_URL_BUILDERS` in `scripts/check_xss_sinks.py`; add a builder there only after reviewing that it cannot return a script URL); + - a `sanitize*Url`, `normalize*Url` or `safe*Url` helper; + - a guard such as `isSafeOriginHref(url)` or an `http(s)` scheme test, in the same expression or in an enclosing `if` or early return. +- `iframe`, `script`, `embed`, `object` and worker sources are stricter. A pinned `https://host/` prefix or a same-origin path with encoded segments is fine; an arbitrary host or `//` is not. +- Keep `react-markdown` raw HTML off. Do not add `rehype-raw`, `allowDangerousHtml: true`, or a `urlTransform` that returns the URL unchanged; keep the default URL transform. +- `` and media sources are not script sinks, so the checker does not flag them. Still pass them through the same helpers when the value comes from a user. + +### React examples + +```tsx +

{message.text}

+Open +Open workspace +
+``` + +Unsafe, because nothing proves the value's scheme or origin: + +```tsx +Open +
+window.location.href = searchParams.get('next') ?? '/'; +``` + +### Suppressions in TypeScript + +In `.ts`, `.tsx`, `.jsx` and `.mjs` files the suppression token counts only when a reason follows it on the same line. Put it within four lines above the finding: + +```tsx +{/* xss-check: ignore reviewed: value is restricted to https URLs before it is stored */} +Homepage +``` + +A bare token does not suppress anything; the finding stays and says the reason is missing. + ## PR Review Checklist -For any JavaScript, HTML, or Python change that affects browser rendering: +For any JavaScript, TypeScript, HTML, or Python change that affects browser rendering: 1. Identify the trust boundary for every value that reaches the browser. -2. Prefer DOM node creation and `textContent` for untrusted text. -3. Normalize dynamic URLs before assigning them to clickable or loadable attributes. +2. Prefer DOM node creation and `textContent` for untrusted text; in React, render it as JSX children. +3. Normalize dynamic URLs before assigning them to clickable or loadable attributes, React props, or navigation calls. 4. If HTML rendering is required, document the sanitizer boundary explicitly. 5. Add or update a regression test when untrusted data reaches a browser-rendering path. ## Workflow Guardrail -This repository includes a Development PR check in `.github/workflows/xss-sink-check.yml` backed by `scripts/check_xss_sinks.py`. +This repository includes a PR check in `.github/workflows/xss-sink-check.yml` backed by `scripts/check_xss_sinks.py`. It runs on PRs into `Development` and, until the V2 branch merges into `Development`, on PRs into `paullizer-react-v2-ui`. It checks the added lines of changed `.js`, `.html`, `.py`, `.ts`, `.tsx`, `.jsx` and `.mjs` files under `application/`, and skips vendored browser assets such as `application/v2_ui/public/vendor/`. `.github/workflows/xss-full-scan.yml` runs the same checker over whole files on demand. If a reviewed exception is unavoidable, add the suppression token below near the specific line and include a justification comment: @@ -130,4 +189,4 @@ If a reviewed exception is unavoidable, add the suppression token below near the xss-check: ignore ``` -Use that escape hatch rarely. It is for reviewed legacy exceptions, not for normal rendering code. \ No newline at end of file +Use that escape hatch rarely. It is for reviewed legacy exceptions, not for normal rendering code. In TypeScript-family files the justification must follow the token on the same line. \ No newline at end of file diff --git a/.github/prompts/prepare-for-pull-request.prompt.md b/.github/prompts/prepare-for-pull-request.prompt.md index 796750887..c75e20765 100644 --- a/.github/prompts/prepare-for-pull-request.prompt.md +++ b/.github/prompts/prepare-for-pull-request.prompt.md @@ -100,11 +100,12 @@ When Python route files changed: `python functional_tests/route_tests/test_route_unauthenticated_policy_contract.py` `python functional_tests/route_tests/test_route_policy_test_coverage.py` -When JavaScript, HTML, or Python browser-rendering surfaces changed: +When JavaScript, TypeScript, HTML, or Python browser-rendering surfaces changed: -- Run `python scripts/check_xss_sinks.py` against changed `application/**/*.js`, `application/**/*.html`, and `application/**/*.py` files. +- Run `python scripts/check_xss_sinks.py` against changed `application/**/*.js`, `application/**/*.html`, `application/**/*.py`, `application/**/*.ts`, `application/**/*.tsx`, `application/**/*.jsx`, and `application/**/*.mjs` files. - If validating only committed changes, prefer changed-line mode: `python scripts/check_xss_sinks.py --base-sha /Development --head-sha HEAD `. + For a PR into the temporary V2 branch `paullizer-react-v2-ui`, use `/paullizer-react-v2-ui` as the base instead. - If the working tree has uncommitted changes, use `--full-file` for those files because they are not represented by `HEAD`. When Python authorization-sensitive surfaces changed: diff --git a/.github/prompts/xss-full-audit-and-remediation.prompt.md b/.github/prompts/xss-full-audit-and-remediation.prompt.md index f59163454..4cf8f004b 100644 --- a/.github/prompts/xss-full-audit-and-remediation.prompt.md +++ b/.github/prompts/xss-full-audit-and-remediation.prompt.md @@ -7,7 +7,7 @@ agent: "agent" # XSS Full Audit And Remediation -Run a full local SimpleChat XSS audit and remediation workflow. This is broader than the GitHub Actions PR check in [.github/workflows/xss-sink-check.yml](../workflows/xss-sink-check.yml): the action checks changed application lines, while this prompt should inspect the whole browser-rendering surface and produce a plan to fix real risks. +Run a full local SimpleChat XSS audit and remediation workflow. This is broader than the GitHub Actions PR check in [.github/workflows/xss-sink-check.yml](../workflows/xss-sink-check.yml): the action checks changed application lines, while this prompt should inspect the whole browser-rendering surface and produce a plan to fix real risks. The on-demand [.github/workflows/xss-full-scan.yml](../workflows/xss-full-scan.yml) workflow runs the same checker over whole files and is a useful first pass, but it does not replace the manual review below. Use the repository guardrails in [.github/instructions/xss-prevention.instructions.md](../instructions/xss-prevention.instructions.md), [.github/instructions/local_browser_assets.instructions.md](../instructions/local_browser_assets.instructions.md), and the deterministic checker in [scripts/check_xss_sinks.py](../../scripts/check_xss_sinks.py). Prefer the repo's existing rendering helpers and Bootstrap patterns over new abstractions. @@ -15,6 +15,7 @@ Use the repository guardrails in [.github/instructions/xss-prevention.instructio - Work in the SimpleChat repository root. - Treat JavaScript, HTML/Jinja, and Python routes/helpers that send data to the browser as the audit surface. +- On branches that contain the V2 React UI, also treat the TypeScript source in `application/v2_ui/src` (`.ts`, `.tsx`, `.jsx`, `.mjs`) as audit surface. Skip vendored minified libraries under `application/v2_ui/public/vendor/`; the checker skips them too. - Do not revert unrelated user changes. - Start with a concise plan and keep it updated as findings are triaged and fixed. - If the user asks for `scan only` or `stop after plan`, do not edit files. @@ -27,15 +28,19 @@ Use the repository guardrails in [.github/instructions/xss-prevention.instructio 2. Identify tracked application files in scope: ```powershell -$xssFiles = git ls-files "application/**/*.js" "application/**/*.html" "application/**/*.py" +$xssFiles = @(git ls-files "application/**/*.js" "application/**/*.html" "application/**/*.py" "application/**/*.ts" "application/**/*.tsx" "application/**/*.jsx" "application/**/*.mjs") ``` -3. Run the deterministic checker in full-file mode, saving the output if it is large: +3. Run the deterministic checker in full-file mode, saving the output if it is large. The full file list is longer than the 32,767-character limit Windows puts on a command line, so pass it in batches, and read every batch's output rather than only the last exit code: ```powershell -python scripts/check_xss_sinks.py --full-file $xssFiles +for ($i = 0; $i -lt $xssFiles.Count; $i += 200) { + python scripts/check_xss_sinks.py --full-file $xssFiles[$i..([Math]::Min($i + 199, $xssFiles.Count - 1))] +} ``` +TypeScript-family findings start with a rule id such as `[ts-jsx-url-attribute]`. The reviewed V2 baseline, with the reason each finding is safe, is in [docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS_V2_BASELINE_TRIAGE.md](../../docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS_V2_BASELINE_TRIAGE.md). Compare against it so already-reviewed sites are not re-triaged from scratch, but re-check any baseline site whose code has changed. + 4. Run the guardrail self-test when the checker, workflow, XSS instructions, or this prompt are part of the work: ```powershell @@ -54,6 +59,13 @@ rg -n "innerHTML|outerHTML|insertAdjacentHTML|\.html\(|onclick=|onerror=|onload= rg -n "`[^`]*\$\{|href=.*\$\{|src=.*\$\{|title=.*\$\{|style=.*\$\{|data-[A-Za-z0-9_-]+=.*\$\{" application/single_app/static application/single_app/templates ``` +7. When `application/v2_ui/src` exists, search it for React and DOM sinks that bypass JSX escaping: + +```powershell +rg -n "dangerouslySetInnerHTML|innerHTML|outerHTML|insertAdjacentHTML|document\.write|createContextualFragment|DOMParser|eval\(|new Function|javascript:|rehype-raw|allowDangerousHtml|urlTransform" application/v2_ui/src +rg -n "(href|src|action|formAction|srcDoc|xlinkHref|to)=\{|window\.open\(|location\.(href|assign|replace)|\.setAttribute\(" application/v2_ui/src +``` + If shell quoting gets in the way, use equivalent `rg` searches or the VS Code search tool, but preserve the same coverage. ## Manual Audit Checklist @@ -74,6 +86,17 @@ Review these sink categories: - Python `Markup(...)` and Jinja `|safe` on values that can contain user, model, file, or service-provided content. - Static HTML shell exceptions that are no longer static because they interpolate runtime data. +For the V2 React UI, also review these sink categories. React 18 escapes JSX text children and attribute strings, but it does not block `javascript:` URLs in URL props (it only warns in development), and it never inspects values that go straight to the DOM: + +- `dangerouslySetInnerHTML` whose `__html` is not `DOMPurify.sanitize(...)` output, or whose renderer (KaTeX, Mermaid) runs without its safe mode (`trust: false`, `securityLevel: 'strict'`). +- `innerHTML`/`outerHTML` assignments through refs, `insertAdjacentHTML`, `document.write`, `createContextualFragment`, and `DOMParser` output appended to the page. +- `eval`, `new Function`, and string arguments to `setTimeout`/`setInterval`. +- Dynamic `href`, `src`, `action`, `formAction`, `srcDoc` and `xlinkHref` props, and router `to` props on `Link`, `NavLink` and `Navigate`, that are not a literal, a same-origin path, a reviewed URL builder, a `sanitize*Url`/`normalize*Url` helper, or guarded by `isSafeOriginHref(...)` or an `http(s)` scheme test. +- Navigation sinks: `window.open(...)`, `location.href = ...`, `location = ...`, and `location.assign(...)`/`location.replace(...)` with a dynamic value. +- DOM URL properties such as `element.href`, `element.src`, `form.action`, `frame.srcdoc`, and `setAttribute('href' | 'src' | 'action' | 'srcdoc', ...)`. +- `react-markdown` options that re-enable raw HTML or unfiltered URLs: `rehype-raw`, `allowDangerousHtml`, or a `urlTransform` that returns its input unchanged. +- Props that forward a URL into one of these sinks from another component. The checker sees only the file with the sink, so trace the value back through its callers. + ## Triage And Plan Group findings before editing: @@ -105,6 +128,13 @@ Use these fixes by default: - Keep static modal/card/table shells fully static, then populate untrusted fields with DOM APIs. - Remove unnecessary `Markup(...)` and Jinja `|safe`; if HTML is required, make the sanitizer boundary explicit and covered by tests. +For V2 React components: + +- Render untrusted text as JSX children instead of building HTML strings. +- Keep `dangerouslySetInnerHTML` only for sanitizer output: `{{ __html: DOMPurify.sanitize(html) }}` or a clearly named sanitized `const`. +- Build same-origin links from a literal path and `encodeURIComponent(...)` segments, or use an existing reviewed URL builder. Guard external links with `isSafeOriginHref(...)` or an explicit `http(s)` scheme test before they reach `href`, `window.open`, or `location`. +- Keep `react-markdown` on its defaults: no `rehype-raw`, no `allowDangerousHtml`, and the default URL transform. + Avoid broad mechanical rewrites. Preserve behavior, accessibility, Bootstrap state classes, and local asset references. ## Verification @@ -112,14 +142,17 @@ Avoid broad mechanical rewrites. Preserve behavior, accessibility, Bootstrap sta After each remediation batch, run the narrowest reliable checks first, then broaden before final reporting: - `node --check ` for changed JavaScript files. +- `npm --prefix application/v2_ui run typecheck` for changed V2 TypeScript files. - `python -m py_compile ` for changed Python files. - Relevant functional or UI tests for the edited workflow. -- `python scripts/check_xss_sinks.py --full-file ` for edited application JS/HTML/Python files. -- Full audit rerun before completion when the user asked for a complete fix pass: +- `python scripts/check_xss_sinks.py --full-file ` for edited application JS/HTML/Python/TypeScript files. +- Full audit rerun before completion when the user asked for a complete fix pass, in batches as in Baseline Discovery: ```powershell -$xssFiles = git ls-files "application/**/*.js" "application/**/*.html" "application/**/*.py" -python scripts/check_xss_sinks.py --full-file $xssFiles +$xssFiles = @(git ls-files "application/**/*.js" "application/**/*.html" "application/**/*.py" "application/**/*.ts" "application/**/*.tsx" "application/**/*.jsx" "application/**/*.mjs") +for ($i = 0; $i -lt $xssFiles.Count; $i += 200) { + python scripts/check_xss_sinks.py --full-file $xssFiles[$i..([Math]::Min($i + 199, $xssFiles.Count - 1))] +} ``` - `git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --check`. diff --git a/.github/workflows/broken-access-control-check.yml b/.github/workflows/broken-access-control-check.yml index 551a6b1d2..b78a4520e 100644 --- a/.github/workflows/broken-access-control-check.yml +++ b/.github/workflows/broken-access-control-check.yml @@ -4,6 +4,7 @@ on: pull_request: branches: - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) paths: - 'application/**/*.py' - 'scripts/check_broken_access_control.py' diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 9c96490c8..97a8d361b 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -13,9 +13,13 @@ name: "CodeQL Advanced" on: push: - branches: [ "Development" ] + branches: + - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) pull_request: - branches: [ "Development" ] + branches: + - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) workflow_dispatch: jobs: diff --git a/.github/workflows/malicious-pr-security-review.yml b/.github/workflows/malicious-pr-security-review.yml index e7438de22..bd2e6b73f 100644 --- a/.github/workflows/malicious-pr-security-review.yml +++ b/.github/workflows/malicious-pr-security-review.yml @@ -4,6 +4,7 @@ on: pull_request: branches: - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) paths: - 'application/**' - 'deployers/**' diff --git a/.github/workflows/python-syntax-check.yml b/.github/workflows/python-syntax-check.yml index d00535aa2..215697ccb 100644 --- a/.github/workflows/python-syntax-check.yml +++ b/.github/workflows/python-syntax-check.yml @@ -5,6 +5,7 @@ on: branches: - main - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) paths: - 'application/single_app/**.py' - '.github/workflows/python-syntax-check.yml' diff --git a/.github/workflows/swagger-route-check.yml b/.github/workflows/swagger-route-check.yml index 57e5e3bac..e22727573 100644 --- a/.github/workflows/swagger-route-check.yml +++ b/.github/workflows/swagger-route-check.yml @@ -6,6 +6,7 @@ on: - main - Development - Staging + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) paths: - 'application/single_app/**/*.py' - 'scripts/check_swagger_routes.py' diff --git a/.github/workflows/xss-full-scan.yml b/.github/workflows/xss-full-scan.yml new file mode 100644 index 000000000..57bf0e256 --- /dev/null +++ b/.github/workflows/xss-full-scan.yml @@ -0,0 +1,97 @@ +name: XSS Full Scan + +on: + workflow_dispatch: + inputs: + target_paths: + description: Space-delimited tracked paths to scan for XSS sink patterns (.js, .html, .py, .ts, .tsx, .jsx, .mjs). + required: false + default: application + fail_on_findings: + description: Fail the workflow when findings are detected. Keep false for baseline/advisory runs. + required: true + type: boolean + default: false + run_self_tests: + description: Run the XSS checker regression tests before the audit. + required: true + type: boolean + default: true + +permissions: + contents: read + +jobs: + xss-full-scan: + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.12' + + - name: Run XSS guardrail self-test + if: inputs.run_self_tests + run: | + python functional_tests/test_xss_guardrails_checker.py + + - name: Collect tracked XSS surface files + id: xss-files + env: + TARGET_PATHS: ${{ inputs.target_paths }} + run: | + set -euo pipefail + + : > xss-surface-files.txt + for target_path in $TARGET_PATHS; do + git ls-files -- "$target_path" | grep -E '\.(js|html|py|ts|tsx|jsx|mjs)$' >> xss-surface-files.txt || true + done + + sort -u xss-surface-files.txt -o xss-surface-files.txt + file_count=$(wc -l < xss-surface-files.txt | tr -d ' ') + echo "file_count=$file_count" >> "$GITHUB_OUTPUT" + + if [[ "$file_count" == "0" ]]; then + echo "No tracked XSS surface files found for target paths: $TARGET_PATHS" + exit 0 + fi + + echo "Collected $file_count file(s) for the XSS full scan. Vendored browser assets are skipped by the checker." + + - name: Run XSS full scan + id: xss-scan + if: steps.xss-files.outputs.file_count != '0' + env: + FAIL_ON_FINDINGS: ${{ inputs.fail_on_findings }} + run: | + set +e + + mapfile -t xss_files < xss-surface-files.txt + python scripts/check_xss_sinks.py --full-file "${xss_files[@]}" 2>&1 | tee xss-full-scan-report.txt + checker_status=${PIPESTATUS[0]} + + echo "checker_status=$checker_status" >> "$GITHUB_OUTPUT" + if [[ "$checker_status" -ne 0 ]]; then + echo "XSS full scan found findings. Review xss-full-scan-report.txt." + if [[ "$FAIL_ON_FINDINGS" == "true" ]]; then + exit "$checker_status" + fi + fi + + exit 0 + + - name: Upload XSS full scan report + if: always() && steps.xss-files.outputs.file_count != '0' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: xss-full-scan-report + path: | + xss-surface-files.txt + xss-full-scan-report.txt + if-no-files-found: ignore diff --git a/.github/workflows/xss-sink-check.yml b/.github/workflows/xss-sink-check.yml index d416163fa..2e5b3fe93 100644 --- a/.github/workflows/xss-sink-check.yml +++ b/.github/workflows/xss-sink-check.yml @@ -4,13 +4,19 @@ on: pull_request: branches: - Development + - paullizer-react-v2-ui # Temporary: remove when paullizer-react-v2-ui merges into Development (#1571) paths: - 'application/**/*.js' - 'application/**/*.html' - 'application/**/*.py' + - 'application/**/*.ts' + - 'application/**/*.tsx' + - 'application/**/*.jsx' + - 'application/**/*.mjs' - 'scripts/check_xss_sinks.py' - 'functional_tests/test_xss_guardrails_checker.py' - '.github/workflows/xss-sink-check.yml' + - '.github/workflows/xss-full-scan.yml' - '.github/instructions/xss-prevention.instructions.md' - '.github/prompts/xss-full-audit-and-remediation.prompt.md' @@ -38,10 +44,15 @@ jobs: - 'application/**/*.js' - 'application/**/*.html' - 'application/**/*.py' + - 'application/**/*.ts' + - 'application/**/*.tsx' + - 'application/**/*.jsx' + - 'application/**/*.mjs' xss_guardrails: - 'scripts/check_xss_sinks.py' - 'functional_tests/test_xss_guardrails_checker.py' - '.github/workflows/xss-sink-check.yml' + - '.github/workflows/xss-full-scan.yml' - '.github/instructions/xss-prevention.instructions.md' - '.github/prompts/xss-full-audit-and-remediation.prompt.md' - 'docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS.md' diff --git a/docs/explanation/features/v0.241.001/SWAGGER_PR_ROUTE_CHECK.md b/docs/explanation/features/v0.241.001/SWAGGER_PR_ROUTE_CHECK.md index 5c40c2f0b..a3f13aff3 100644 --- a/docs/explanation/features/v0.241.001/SWAGGER_PR_ROUTE_CHECK.md +++ b/docs/explanation/features/v0.241.001/SWAGGER_PR_ROUTE_CHECK.md @@ -18,6 +18,7 @@ Dependencies: Architecture overview: - The workflow runs on `pull_request` events for `main`, `Development`, and `staging`. +- It also runs on pull requests into the V2 React UI branch `paullizer-react-v2-ui`. That trigger is marked temporary in the workflow file and should be removed when the V2 branch merges into Development (#1571). - It scopes execution to changes in `application/single_app/**/*.py`, the checker script, and the workflow file itself. - The workflow uses `tj-actions/changed-files` to gather only edited Python files. - The checker script parses each changed file with Python AST and inspects route decorators on Flask route functions. diff --git a/docs/explanation/features/v0.241.022/BROKEN_ACCESS_CONTROL_PR_GUARDRAILS.md b/docs/explanation/features/v0.241.022/BROKEN_ACCESS_CONTROL_PR_GUARDRAILS.md index a97413002..fffb156d8 100644 --- a/docs/explanation/features/v0.241.022/BROKEN_ACCESS_CONTROL_PR_GUARDRAILS.md +++ b/docs/explanation/features/v0.241.022/BROKEN_ACCESS_CONTROL_PR_GUARDRAILS.md @@ -54,6 +54,8 @@ Key behaviors: This keeps the primary PR gate fast while still validating the guardrail itself when the guardrail code changes. +The workflow also runs on pull requests into the V2 React UI branch `paullizer-react-v2-ui`, so Python changes made there get the same check. That trigger is marked temporary in the workflow file and should be removed when the V2 branch merges into Development (#1571). + ## Local Usage Run the checker against full files from the repository root: diff --git a/docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS.md b/docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS.md index b696daa71..bc70c7132 100644 --- a/docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS.md +++ b/docs/explanation/features/v0.241.022/XSS_PR_GUARDRAILS.md @@ -6,6 +6,8 @@ Fixed/Implemented in version: **0.241.021** This feature adds a prevention-focused XSS guardrail for pull requests into Development. It is designed to catch new browser-side sink regressions before they merge, especially when code arrives from contributors who are not using the local Copilot or CLAUDE instruction set. +The guardrail also covers the V2 React/TypeScript UI in `application/v2_ui/src`, and it runs on pull requests into the V2 branch `paullizer-react-v2-ui` until that branch merges into Development. See [V2 React/TypeScript Coverage](#v2-reacttypescript-coverage). + ## Scope This change affects: @@ -13,7 +15,9 @@ This change affects: - the repository instruction set in `.github/instructions/xss-prevention.instructions.md` - the changed-file checker in `scripts/check_xss_sinks.py` - the Development pull-request workflow in `.github/workflows/xss-sink-check.yml` +- the on-demand whole-file scan in `.github/workflows/xss-full-scan.yml` - the guardrail regression test in `functional_tests/test_xss_guardrails_checker.py` +- the full-audit prompt in `.github/prompts/xss-full-audit-and-remediation.prompt.md` ## Technical Details @@ -44,7 +48,8 @@ It also explicitly blocks the sink patterns that caused the recurring XSS findin Key behaviors: -- Scans only supported source files: `.js`, `.html`, and `.py` +- Scans only supported source files: `.js`, `.html`, and `.py`, plus the TypeScript-family files `.ts`, `.tsx`, `.jsx`, and `.mjs` described below +- Skips vendored browser assets, such as the classic OpenLayers and SimpleMDE copies and the V2 libraries under `application/v2_ui/public/vendor/` - Supports pull-request diff mode through `--base-sha` and `--head-sha` - Limits checks to added lines in CI when revision metadata is provided - Emits GitHub Actions error annotations for each blocking issue @@ -59,12 +64,21 @@ Key behaviors: This keeps the primary PR gate fast while still validating the guardrail itself when the guardrail code changes. +The workflow also runs on pull requests into `paullizer-react-v2-ui`. That trigger is marked temporary in the workflow file and should be removed when the V2 branch merges into Development. + ## Local Usage Run the checker against full files from the repository root: ```powershell python scripts/check_xss_sinks.py --full-file application/single_app/static/js/chat/chat-messages.js +python scripts/check_xss_sinks.py --full-file application/v2_ui/src/components/chat/CitationChip.tsx +``` + +Check only the lines a branch added, the way the pull-request workflow does: + +```powershell +python scripts/check_xss_sinks.py --base-sha origin/paullizer-react-v2-ui --head-sha HEAD ``` Run the guardrail self-test: @@ -83,6 +97,126 @@ xss-check: ignore That token should remain rare. New rendering code should use the safe DOM patterns instead. +## V2 React/TypeScript Coverage + +V2 coverage was added for GitHub issue #1571 while the application was at version **0.261.203**. It is a tooling-only change, so the application version did not change. + +### Why V2 needs its own rules + +The V2 UI in `application/v2_ui/src` is React 18 with TypeScript. React escapes JSX text and attribute strings, so most of the string-to-HTML sinks the classic rules look for do not appear there. React 18 does not close every path, though: + +- It still renders `javascript:` URLs in `href`, `src`, `action`, `formAction` and `xlinkHref`, with only a development warning. React 19 blocks them; React 18 does not. +- `dangerouslySetInnerHTML` inserts its value as raw HTML. +- Refs and browser APIs (`innerHTML`, `insertAdjacentHTML`, `DOMParser`, `window.open`, `location`) bypass React entirely. +- `react-markdown` is safe on its defaults, but `rehype-raw`, `allowDangerousHtml` or a pass-through `urlTransform` turns raw HTML or unfiltered URLs back on. + +The classic regexes also match JSX text, comments and type annotations as if they were code, so the TypeScript-family files get a separate pipeline. + +### File types + +| Files | How they are checked | +|---|---| +| `.js`, `.html`, `.py` | The existing regex checks, unchanged. | +| `.tsx`, `.jsx` | The TypeScript pipeline with JSX enabled. | +| `.ts`, `.mjs` | The TypeScript pipeline with JSX disabled, so `` generics and comparisons are not read as tags. | +| `.d.ts`, `.d.mts`, `.d.cts` | Skipped. Declaration files hold types only. | +| `application/v2_ui/public/vendor/` | Skipped. These are vendored minified builds of Chart.js, DOMPurify, KaTeX and Mermaid. | + +The V2 build output in `application/single_app/static/v2` is not tracked, so it is never scanned. + +### How the TypeScript pipeline reads code + +The checker lexes each file once. It separates comments, string and template literals, regular-expression literals, JSX text and JSX expression containers from code, and each rule matches against code only. As a result, comments, JSX text, string contents and type annotations do not produce findings, and `as` casts, non-null assertions (`value!`), generics, optional chaining and multi-line JSX attributes are handled. + +The checker has no type information and does not follow values across files. It decides whether a value is safe from its local shape: literals, template-literal prefixes, helper names, reviewed builders, guards, and the local bindings it can resolve in the same file. When it cannot resolve a value, it reports the finding rather than assuming the value is safe. + +### Rules + +Every TypeScript-family finding starts with its rule id, for example `[ts-jsx-url-attribute]`. + +| Rule id | What it flags | What it accepts | +|---|---|---| +| `ts-dangerous-inner-html` | `dangerouslySetInnerHTML`, as a JSX prop or an object key, whose `__html` is not sanitized. | `DOMPurify.sanitize(...)`, another `*purify*.sanitize(...)`, `sanitize*(...)` or `escape*Html(...)` result, inline, through a local `const`, or from a `useMemo(...)` whose every return is sanitized. Static strings. | +| `ts-html-sink` | `innerHTML`/`outerHTML` assignments, `insertAdjacentHTML`, `document.write`, `createContextualFragment`, and `DOMParser.parseFromString` output that is inserted into the page. | The same sanitized or static values. | +| `ts-code-eval` | `eval(...)`, `Function(...)` with or without `new`, and `setTimeout`/`setInterval` whose first argument is a string literal. | Function arguments to timers. | +| `ts-javascript-url` | A `javascript:` URL inside a string or template literal. | A literal that is exactly `javascript:`, such as an entry in a scheme blocklist. Comments and JSX text. | +| `ts-jsx-url-attribute` | Dynamic `href`, `xlinkHref`, `formAction` and `srcDoc` props; `action` on `
`; `src` on `