Adversarial review — PR #152

Findings

1. [HIGH] This will fail production deploy against the current kayak_data checkout.

deploy.sh unconditionally runs levels validate-dataset "$KAYAK_DATA" before any metadata sync (scripts/deploy.sh:145-159). This PR adds _check_regression() to that validator and makes any non-empty calc_expression.provenance_slug require a <dataset>/regression/ directory with the matching {md,svg,json} triple (src/kayak/cli/validate_dataset.py:170-175, :1128-1136). But the PR explicitly says S2-E1 has not moved the reports into kayak_data yet, and the current adjacent kayak_data checkout has no regression/ directory while calc_expression.csv declares 17 provenance slugs.

I reproduced the deploy gate directly from this PR head:

$ levels validate-dataset /Users/pat/tpw/kayak_data
dataset validation FAILED (/Users/pat/tpw/kayak_data):
  - regression/ directory missing but provenance_slug(s) declared: [...]

So merging this code alone bricks the next deploy before migrate/sync/build. Either gate the new <dataset>/regression validation until S2-E2/E3 actually lands the files in kayak_data, or make this PR dependent on the matching kayak_data PR and deploy them together. The current "backward-compatible and hardens existing docs/regression path in place" claim does not hold with the unconditional deploy validator.

2. [MED] The PR reverts current main operator docs for SA-teardown-C.

The diff against main deletes the deploy/SETUP.md text that tells the operator to pause/remove the retired metadata-snapshot check on healthchecks.io, and weakens the kayak_data branch-protection step from "require PR + validate check + enforce_admins" to just "enable branch protection" (deploy/SETUP.md:471-478). This looks like a stale branch carrying an older copy of the PR #150 follow-up note. If merged as-is, it removes the false-alert cleanup and the concrete branch-protection settings from the canonical deploy doc.

Verification

• levels build --output-dir /private/tmp/kayak-pr152-build with DATABASE_URL=sqlite:////Users/pat/tpw/DB/kayak.db succeeds, so the new sanitizer can process the current code-repo docs/regression set.
• pytest -q tests/test_regression.py tests/test_build_deploy.py tests/test_scripts/test_validate_dataset.py: 259 passed.
• mypy src/kayak/web/regression.py src/kayak/web/build/deploy.py src/kayak/cli/validate_dataset.py: passed.
• ruff check ...: passed.
• git diff --check 1ab29460620ed738e440fbde5d53eab6dda447f1..HEAD: clean.
• GitHub CI is green.

Adversarial review — PR #152 (S2-E1: sanitize + validate regression content)

Reviewed on the live host. This is a security PR whose trust boundary is the SVG served as a standalone document outside the page CSP — a stored-XSS surface — so I built an isolated venv with the PR's pinned deps (nh3 0.3.5, defusedxml 0.7.1, markdown) and ran a bypass battery against the actual regression.py, plus verified the build path and false-positive rate on the 25 real reports.

Verdict: strong, merge-worthy security work. The SVG/markdown sanitizers held up to every attack I threw; the architecture (allowlist + re-serialize-from-tree, nh3 backstop, fail-closed build) is right. Two defense-in-depth findings below — one a genuine (if low-severity, orphan-only) XSS path that this exact PR is the right place to close, one a robustness note. Neither is a blocker.

What I verified

SVG bypass battery — all defeated (each rejected, or stripped by re-serialization with the clean bytes served):

• elements script / foreignObject / use / image / style / a / animate / set → rejected (not on allowlist; SMIL <set attributeName=href> and <animate onbegin> blocked at the element level)
• onload/onclick and any on* → rejected; style attr → rejected; xlink:href (and any namespaced attr) → rejected
• fill="URL(http://…)" (uppercase), url(#a) url(http…), url(/**/#a), image-set()/image()/element(), url(data:…), and \75 rl( backslash-escape → all rejected; legit url(#clip0) / url( #id ) pass
• foreign-namespace <xhtml:rect> → rejected (closes the localname parser-differential); DOCTYPE/external-entity → rejected by defusedxml; CDATA </text><script> → escaped in the re-serialized output (&lt;/text&gt;…)

Markdown — nh3 strips everything dangerous: raw <script>, <img onerror>, [x](javascript:…), entity-obfuscated &#106;avascript:, external/data:/protocol-relative img src, <iframe>, raw <svg onload>, autolinked javascript: → all gone; links get rel="noopener noreferrer nofollow"; img src is local-only.

Build is fail-closed (_deploy_regression_artifacts re-raises UnsafeContentError → build aborts) and serves the re-serialized validate_svg() return (deploy.py:172-175), never the verbatim file. Zero false-positives: all 25 real SVGs + 25 JSONs pass and all 26 MDs render — the move-verbatim invariant holds.

Findings

1. [Low–Med, defense-in-depth] Unescaped report filename injected into the generated <title> — an orphan-file XSS path. In _deploy_regression_artifacts the page is built with title = path.stem.replace("_"," ") interpolated into <title>{title} — Regression analysis</title> without HTML-escaping (deploy.py ~:191). A regression file whose name contains HTML metacharacters injects: I confirmed evil</title><script>alert(1)</script>.md yields a <title>…</title><script>alert(1)</script>… page.

• Referenced reports are safe — provenance_slug is charset-constrained (_REGRESSION_SLUG_RE = ^[A-Za-z0-9_-]+$).
• The gap is orphan files: validate-dataset only warns on an orphan (and _regression_sanitize runs over reachable files only), warnings don't affect the exit code, and the build globs all *.md regardless of slug — so a dataset PR adding evil</title><script>…</script>.md passes validate-dataset (warning only) and the build emits a stored-XSS page.
• Currently low (content is still the engine repo's docs/regression, so it rides code review, and a <script>-named file is visually obvious in a diff) — but this is precisely the dataset-trust surface this PR exists to harden, and it lands fully at E2/E3 when the content moves to kayak_data. Fix: html.escape(title) (minimal and correct regardless), and ideally make the build skip/reject any stem not matching _REGRESSION_SLUG_RE so an orphan malicious-named file isn't processed at all (align the build's file set with the validator's). The page body is fine (render_markdown_safe); title is the only unescaped dynamic interpolation.

2. [Low, defense-in-depth] validate_svg accepts-and-strips processing instructions rather than rejecting them. A <?xml-stylesheet type="text/css" href="javascript:…"?> is accepted (no UnsafeContentError). I confirmed it is not exploitable today: ElementTree drops PIs on parse, so the re-serialized output is a clean <svg><rect/></svg>, and the build serves that re-serialized output. But the safety rests on that ET implementation detail — a future swap to a PI-preserving serializer (lxml, or insert_pis) would leak it — and as a validator, validate-dataset silently passes an SVG carrying a dangerous PI rather than flagging it. Consider rejecting PIs/comments explicitly so the "reject nonconforming" contract doesn't depend on the serializer's drop behavior.

Nits / notes

• JSON sidecars are validated then shutil.copy2'd verbatim (not re-serialized) — fine, since they're inert data served as .json, and the validator rejects NaN/Infinity and non-object/empty-slug shapes.
• validate_json_sidecar checks slug is a non-empty string but not that it equals the expected slug; the slug↔file tie is enforced in _check_regression, so this is fine.

Net: the core sanitizers are genuinely solid and I couldn't break them; closing the <title> escape (and tightening the build's file set to validated slugs) would make E2/E3 land clean.

Addressed in 5495b50 (plus a merge of current main). Thanks — the deploy-gate brick was a real ordering bug I'd missed.

Finding 1 (HIGH) — deploy-brick. You're right: deploy.sh runs validate-dataset against the real kayak_data on every deploy, so making the 17 declared slugs require a regression/ dir that doesn't exist until D1 would have failed the gate. Fix: the regression/ directory is the dataset's opt-in — while it's absent the reports still live engine-side (docs/regression, until the S2 file-move), so declared slugs now yield a non-fatal WARNING, not an error. Once the dir exists the check is fully enforced (the fixture covers that path). Verified:

$ levels validate-dataset /Users/pat/tpw/kayak_data
  ! 17 provenance_slug(s) declared but no regression/ directory (reports not yet managed by this dataset)
dataset validation OK (/Users/pat/tpw/kayak_data) (1 warning(s))

So E1 is now genuinely deploy-safe on its own; D1 lands the files and the check engages.

Finding 2 (MED) — reverted SETUP.md. Stale-branch artifact: this branch predated #151's merge, so the diff appeared to delete the healthchecks.io / branch-protection note. Merged current main — the #151 content is back (E1 doesn't touch SETUP.md, clean merge).

Security review finding 1 (orphan-filename XSS). Fixed both ways you suggested: the build now skips any regression file whose stem isn't a safe slug (new kayak.web.regression.is_safe_slug, shared charset with the validator), and html.escape()s the <title> as defence-in-depth. A bad<x>.{md,svg,json} is no longer processed (new test). (Also fixed a html-module shadowing the local template var, and added encoding= to the page write — the nit you flagged.)

Security review finding 2 (SVG PI/comment). validate_svg now rejects XML comments and processing instructions explicitly (e.g. <?xml-stylesheet href="javascript:…"?>), so the reject-nonconforming contract no longer rests on ElementTree dropping them. A single leading <?xml …?> declaration is still allowed. (New tests.)

All 25 real reports still pass; full gate green locally (1536 tests, wheel-smoke, ruff/mypy). Holding for your go before merge.

Adversarial rereview — PR #152 follow-up (5495b50)

No blocking findings.

I rereviewed the follow-up commit and re-ran the deploy-sensitive checks against the current adjacent kayak_data checkout and the fresh live DB copy at /Users/pat/tpw/DB/kayak.db.

Prior findings are addressed:

• The production deploy blocker is fixed. levels validate-dataset /Users/pat/tpw/kayak_data now exits 0 with the expected transitional warning:

  ! 17 provenance_slug(s) declared but no regression/ directory (reports not yet managed by this dataset)
  dataset validation OK (/Users/pat/tpw/kayak_data) (1 warning(s))
  

  Once kayak_data/regression/ exists, the stricter slug/report/sidecar/content enforcement still applies.

• The stale deploy/SETUP.md regression is fixed; the healthchecks.io cleanup note and the concrete kayak_data branch-protection settings are restored from current main.

• The orphan-filename XSS path is closed. The build skips unsafe stems (bad<x> is not processed) and escapes the generated <title> as defense in depth.

• The SVG PI/comment gap is closed. validate_svg() now rejects XML comments and processing instructions explicitly; I rechecked both <?xml-stylesheet ...?> and <!-- ... --> payloads.

Verification:

• levels validate-dataset /Users/pat/tpw/kayak_data: OK with one warning.
• levels build --output-dir /private/tmp/kayak-pr152-rereview-build with DATABASE_URL=sqlite:////Users/pat/tpw/DB/kayak.db: succeeded, 412 reaches / 225 gauges.
• pytest -q tests/test_regression.py tests/test_build_deploy.py tests/test_scripts/test_validate_dataset.py: 262 passed.
• mypy src/kayak/web/regression.py src/kayak/web/build/deploy.py src/kayak/cli/validate_dataset.py: passed.
• ruff check src/kayak/web/regression.py src/kayak/web/build/deploy.py src/kayak/cli/validate_dataset.py tests/test_regression.py tests/test_build_deploy.py tests/test_scripts/test_validate_dataset.py: passed.
• git diff --check 1ab29460620ed738e440fbde5d53eab6dda447f1..HEAD: clean.
• GitHub CI is green on the latest run.

Residual note: until the S2 file-move lands, regression content is still validated at build time from code-repo docs/regression/, while kayak_data gets the transitional warning. That matches this PR's stated E1/D1 split.

Adversarial re-review — PR #152 (after 5495b50)

Re-reviewed the fix commit on the live host: rebuilt an isolated venv with the PR deps, re-ran the bypass battery against the updated regression.py, and verified the deploy-brick scenario against the real kayak_data. (CI still running per your note; this review is on the code + my own testing, not a CI result.)

Verdict: the fixes are solid — both my round-1 findings are resolved, and the commit caught a HIGH I missed. One residual low gap in the PI fix (below), not a blocker.

Resolved

• Orphan-filename XSS (my Finding Rewrite C++ CGI to Python package with PHP web layer #1) — closed, two ways. The build now skips any regression file whose stem isn't is_safe_slug (^[A-Za-z0-9_-]+$), so evil</title><script>…</script>.md is never processed (I confirmed is_safe_slug returns False for </title>, bad<x>, a.b, ../x), and the title is html.escaped as defense-in-depth. The html→page local rename was actually required, not cosmetic: with import html at module scope, a local var named html would have made title = html.escape(...) raise UnboundLocalError. Good catch. New test_deploy_regression_skips_unsafe_stem covers it.

• SVG PI/comment (my Finding Reduce DB bloat, improve lock contention, add gauge metadata cache #2) — now explicit (in-body PIs, leading + in-body comments, and non-xml leading PIs all rejected), with a parametrized test. See the residual below.

The HIGH I missed — credit to the fix

deploy.sh runs validate-dataset against the real kayak_data on every deploy, and the new _check_regression made the 17 declared provenance_slugs require a regression/ directory that won't exist until the D1 file-move. I verified the trigger on the live host: kayak_data has 17 non-empty provenance_slugs (of 22 rows) and no regression/ dir — so the pre-fix code would have errored and bricked the E1 deploy at the validate-dataset gate (the same gate-brick class as #149/#150, which I also under-checked). The fix correctly treats an absent regression/ dir as the transitional opt-out (declared slugs → non-fatal warning), with full enforcement once the dir exists (the fixture still exercises the enforced path). Real dataset now validates clean (1 warning). This was the right call and I should have caught it in round 1.

Residual finding

[Low, defense-in-depth] Fix #3 still doesn't reject a leading <?xml-stylesheet …?> PI. The check strips a leading <?xml …?> as the XML declaration via body.startswith("<?xml") — but that prefix also matches <?xml-stylesheet, so a leading <?xml-stylesheet type="text/xsl" href="…"?> (or href="javascript:…") is treated as the declaration, stripped, and not flagged — the exact construct the commit message cites as what it's fixing. I verified both the js and xsl variants are accepted (while the in-body PI, leading/in-body comments, and <?foo?> leading PI are all correctly rejected, and the legit <?xml version?> declaration still passes).

Not exploitable — I confirmed the PI is absent from the re-serialized output (leak_in_output=False) and the build serves only that re-serialized SVG — so this is purely the "reject-nonconforming contract" not yet covering its own example. One-line fix: distinguish the declaration by the whitespace the XML spec requires after the xml target, e.g. re.match(r"<\?xml\s", body) instead of startswith("<?xml") — the PI target xml-stylesheet has a -, not whitespace, so it then falls through to the "<?" in body reject.

Re-verified, no regressions

The round-1 battery still holds against the updated code: script/onload/uppercase-URL()/foreign-namespace/xlink:href all still rejected; all 25 real reports still pass (move-verbatim invariant intact). nh3 markdown sanitization unchanged.

Net: ship-worthy once the leading-PI prefix is tightened (or explicitly accepted as known/again-defense-in-depth). Nice work closing the deploy-brick — that was the load-bearing one.

Addressed the residual in 786a9d9. Good catch — the guard didn't cover its own example.

Leading <?xml-stylesheet …?> PI. The check distinguished the XML declaration with startswith("<?xml"), which also matched the xml-stylesheet PI target, so a leading stylesheet PI was stripped as if it were the declaration. Switched to re.match(r"<\?xml\s", body) — the declaration requires whitespace after the xml target, while xml-stylesheet has a -, so it now falls through to the reject. Verified:

• leading <?xml-stylesheet …?> (js + xsl) → rejected
• genuine <?xml version="1.0"?> declaration → still accepted
• in-body PI / comments → still rejected

(Not exploitable — ElementTree drops the PI and the build serves the re-serialized SVG — but it closes the contract over its own example.)

Full gate green locally (1538 tests, wheel-smoke, ruff/mypy). All 25 real reports still pass; deploy gate on the real kayak_data still OK (1 transitional warning). Holding for your go before merge.