From e02155828522c3a65f51306e816a10bb37f9d8ff Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 26 Sep 2026 12:59:51 +0000 Subject: [PATCH] Only offer the DOM-grep test route in XSS gate hint when it can apply The fix hint in scripts/check-xss-sinks.sh always suggested "add a DOM-grep test in test*.js", but CI's --diff step never sets PREFLIGHT_TEST_FILES, so that route is dead in CI. The hint misled an agent and a reviewer into believing an existing test covered a sink (PR #99); CI then failed. The gate itself is unchanged (no detection or allowlist edits). The hint now: - mentions the test route only when PREFLIGHT_TEST_FILES is non-empty; - otherwise suggests escapeHtml/escapeAttr/esc, DOM construction with createElement + textContent / non-URL setAttribute, or the PR-body opt-out plus xss-optout label, and notes that tests do not count. The header comment documents the CI scope, and AGENTS.md gains a short note on what the CI gate accepts. test-preflight-xss-gate.js asserts the hint wording with and without PREFLIGHT_TEST_FILES. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Rd9Nh9Z2A9UgMupvLHCKUr --- AGENTS.md | 8 ++++++ scripts/check-xss-sinks.sh | 22 ++++++++++++++- test-preflight-xss-gate.js | 57 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 6fc1db4bc..e58939d94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -182,6 +182,14 @@ If you need to know how something works — a flag, a field, a timing, a behavio ### Theming All colors MUST use CSS variables. Never hardcode `#hex` values outside of `:root` definitions. The customizer controls colors via `THEME_CSS_MAP` in customize.js. If you add a new color, add it as a CSS variable and map it in the customizer. +### XSS preflight gate (CI) +`scripts/check-xss-sinks.sh --diff` runs on every PR and flags HTML sinks (`innerHTML`, `bindPopup`, URL/`on*` `setAttribute`, …) that interpolate a node-controlled field. In CI a flagged line passes only via: +- an escape helper on the same line (`escapeHtml` / `escapeAttr` / `esc`), or +- building the DOM with `createElement` + `textContent` / non-URL `setAttribute` instead of an HTML string — **the preferred fix for false positives**, or +- `PREFLIGHT-XSS-OPTOUT: : reason="…"` in the PR body **plus** the `xss-optout` label. + +Existing or same-PR tests do **not** count in CI: the workflow never sets `PREFLIGHT_TEST_FILES`, so that route only applies to local/skill-side runs that set it. + ### Shared Helpers (roles.js) - `getNodeStatus(role, lastSeenMs)` → 'active' | 'stale' - `getHealthThresholds(role)` → `{ staleMs, degradedMs, silentMs }` diff --git a/scripts/check-xss-sinks.sh b/scripts/check-xss-sinks.sh index 716fce1d7..fbd9148e9 100755 --- a/scripts/check-xss-sinks.sh +++ b/scripts/check-xss-sinks.sh @@ -27,6 +27,17 @@ # and apply the same rules to added lines only. # BASE defaults to origin/master. # +# CI scope: the PR-only `--diff` step in .github/workflows/deploy.yml sets +# PREFLIGHT_PR_BODY and PREFLIGHT_PR_LABELS but NEVER PREFLIGHT_TEST_FILES. +# The same-PR DOM-grep test route therefore only applies to local and +# skill-side runs that set the variable themselves. In CI a flagged sink +# passes only via an escape helper on the line, DOM construction +# (createElement + textContent / non-URL setAttribute) instead of an HTML +# string, or the PR-body opt-out + `xss-optout` label. This is deliberate: +# the sham-test check (basename + marker grep) is too weak to trust in CI. +# The fix hint mirrors this and only mentions the test route when +# PREFLIGHT_TEST_FILES is non-empty. +# # The canonical pr-preflight gate (skill-side) consumes the same allowlist # format documented inline below. # @@ -325,7 +336,16 @@ def emit_finding(file, lineno, token, sink): print(f"ℹ️ {file}:{lineno}: flagged token '{token}' in {sink} — author opt-out in PR body (xss-optout label + ≥40ch reason)") return False print(f"❌ {file}:{lineno}: flagged: {token} (sink: {sink})") - print(f" fix: wrap with escapeHtml(...) / escapeAttr(...) — or add a DOM-grep test in test*.js asserting the payload renders inert — or add 'PREFLIGHT-XSS-OPTOUT: {file}:{lineno} reason=\"...(≥40 chars)...\"' to the PR body AND apply the xss-optout label.") + # Only offer the same-PR test route when this run can honor it. CI's + # --diff step never sets PREFLIGHT_TEST_FILES, so there it is dead. + test_route = (" — or add a DOM-grep test in test*.js (listed in PREFLIGHT_TEST_FILES)" + " asserting the payload renders inert") if TEST_FILES else "" + print(f" fix: wrap with escapeHtml(...) / escapeAttr(...) / esc(...)" + f" — or build the DOM with createElement + textContent (setAttribute for non-URL, non-on* attributes) instead of an HTML string" + f"{test_route}" + f" — or add 'PREFLIGHT-XSS-OPTOUT: {file}:{lineno} reason=\"...(≥40 chars)...\"' to the PR body AND apply the xss-optout label.") + if not TEST_FILES: + print(" note: PREFLIGHT_TEST_FILES is not set, so existing or same-PR tests do not count as coverage (CI never sets it).") return True def scan_lines(lines_with_no): diff --git a/test-preflight-xss-gate.js b/test-preflight-xss-gate.js index 0897d74ed..63691ca93 100644 --- a/test-preflight-xss-gate.js +++ b/test-preflight-xss-gate.js @@ -94,6 +94,63 @@ for (const c of cases) { } } +// Fix-hint wording. CI's --diff step never sets PREFLIGHT_TEST_FILES, so +// the same-PR DOM-grep test route is dead there. The hint must only offer +// that route when the variable is set and non-empty — otherwise authors +// (and reviewers) assume an existing test covers the sink and CI fails. +function runGate(file, testFiles) { + const env = Object.assign({}, process.env); + if (testFiles === undefined) delete env.PREFLIGHT_TEST_FILES; + else env.PREFLIGHT_TEST_FILES = testFiles; + return spawnSync('bash', [SCRIPT, '--file', path.join(FIXTURE_DIR, file)], { + env, + encoding: 'utf8', + }); +} + +const hintCases = [ + { label: 'PREFLIGHT_TEST_FILES unset → no DOM-grep test hint', + testFiles: undefined, domTestHint: false }, + { label: 'PREFLIGHT_TEST_FILES empty → no DOM-grep test hint', + testFiles: '', domTestHint: false }, + // test-good-2.js does not name bad-1-template-literal.js, so the sink is + // still flagged — but the route exists in this run, so the hint stays. + { label: 'PREFLIGHT_TEST_FILES set (non-covering) → DOM-grep test hint', + testFiles: path.join(FIXTURE_DIR, 'test-good-2.js'), domTestHint: true }, +]; + +for (const h of hintCases) { + const res = runGate('bad-1-template-literal.js', h.testFiles); + const out = res.stdout || ''; + const problems = []; + if (res.status !== 1) problems.push(`expected exit 1, got ${res.status}`); + if (out.includes('DOM-grep test') !== h.domTestHint) { + problems.push(`DOM-grep test hint ${h.domTestHint ? 'missing' : 'present'}`); + } + for (const needle of ['escapeHtml', 'textContent', 'PREFLIGHT-XSS-OPTOUT', 'xss-optout label']) { + if (!out.includes(needle)) problems.push(`hint missing "${needle}"`); + } + if (problems.length) { + console.error(`FAIL: hint — ${h.label}: ${problems.join('; ')}`); + console.error(' stdout:', out.trim()); + failed++; + } else { + console.log(`PASS: hint — ${h.label}`); + } +} + +// Every bad fixture run without test files must omit the dead route. +for (const c of cases.filter(c => c.expect === 1 && !c.tests)) { + const res = runGate(c.file, undefined); + if (res.status !== 1 || (res.stdout || '').includes('DOM-grep test')) { + console.error(`FAIL: hint — ${c.file} without PREFLIGHT_TEST_FILES mentions DOM-grep test (exit ${res.status})`); + console.error(' stdout:', (res.stdout || '').trim()); + failed++; + } else { + console.log(`PASS: hint — ${c.file} omits DOM-grep test route in CI mode`); + } +} + if (failed > 0) { console.error(`\n${failed} assertion(s) failed.`); process.exit(1);