Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions docs/decisions/2026-09-22-codeql-first-analysis.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# Decision: CodeQL default-setup first-analysis triage (2026-09-22)

**Decided by:** unit `nas-codeql-fixes`, worktree branch `claude/codeql-alert-fixes`, base `796f759` (newer than
the alert catalog's analysed commit `168a3a8`; PR #96 changed the generated-explorer manifest between the two,
so every alert location below was re-located at `796f759` before triage rather than trusted from the catalog).

**Scope:** the 10 open alerts from the repository's first CodeQL default-setup analysis
(commit `168a3a8`, alert numbers 1-10). Fixes cover only the flagged lines and their immediate helper
functions; no unrelated refactor. This closes the alert backlog so the planned `code_scanning` ruleset rule
(`security_alerts_threshold: high_or_higher`, `alerts_threshold: errors`) can be enabled without blocking
future work.

## Alerts

| # | Rule | Location (re-located at 796f759) | Classification | Action | Evidence |
|---|------|------------------------------------|-----------------|--------|----------|
| 1 | `js/xss-through-dom` | `docs/ecosystem/template.html:145` (`link()` sets `node.href = url`) | Defense in depth (build-side filtering is the primary control: `public_url()` in `scripts/build_ecosystem.py` admits only credential-free `https:` source links, and `loopback_url()` admits only explicit `http://` or `https://` links to `127.0.0.1`/`[::1]` (this PC's dashboards); neither admits a `javascript:`/`data:` scheme, and `tests/test_ecosystem_manifest.py` asserts `javascript:` URLs are stripped before the template sees them) | Fixed: added `safeHref(url)` helper that parses the URL and only assigns `href` when the protocol is `http:`/`https:`, else `about:blank`; this is a second, independent barrier at the DOM-write site in case a future data source bypasses `public_url()` | `tests/test_ecosystem_manifest.py::test_safe_href_allowlists_http_https_and_rejects_other_schemes` runs the committed helper (extracted verbatim from `template.html`, not reimplemented) under Node and asserts `javascript:`, `data:`, `vbscript:`, `file:`, and `mailto:` all resolve to `about:blank` while `https:`/`http:`/protocol-relative/relative URLs pass through unchanged; `--check` deterministic rebuild of the template |
| 2 | `js/xss-through-dom` | `docs/ecosystem/template.html:512` (`screenshot.src = "data:image/png;base64," + artifact.content_base64`) | False positive | Dismissed (not changed) | The `data:image/png;base64,` scheme prefix is a fixed source-code literal; the appended payload cannot alter the outer URI scheme, so there is no attacker-controllable sink. See `codeql-dismissals.json` entry #2. |
| 3 | `py/bad-tag-filter` | `scripts/build_ecosystem.py` `render_from_data()` (`re.findall(r"<script>(.*?)</script>", result, re.S)`) | Robustness, not an exploitable defect: the regex reads the repository's own template, and embedded data is `<`-escaped so it cannot open or close a tag | Fixed: replaced the regex with `InlineScripts`, a stdlib `html.parser.HTMLParser` that handles tag case, `</script >` and end-tag attributes like a browser. Where html.parser and a browser disagree (a self-closing `<script/>`, a script after `<!-->` on Python 3.12), the build now fails loudly: it requires no self-closing script and requires the parser's script-start count to equal the raw `"<script"` count | Coordinator check: on the real generated page the parser and the old regex return the identical single body; `InlineScripts('<SCRIPT>x()</script >')` returns `['x()']`; `scripts/build_ecosystem.py --check`; unit tests |
| 4 | `py/bad-tag-filter` | `tests/test_claude_repository_evidence.py` CSP assertion (script-body extraction) | Robustness of a test that asserts a real property (the page admits only its own hashed script) | Fixed: `ExecutableScripts` stdlib parser (excludes `type="application/json"` blocks) instead of the regex | Same equality check on `docs/ecosystem/claude-repository-evidence.html`; `python -m unittest tests.test_claude_repository_evidence` |
| 5 | `py/bad-tag-filter` | `tests/test_ecosystem_manifest.py` Node harness (extracts the template's single inline script) | Robustness | Fixed: the test's existing `Page` parser now also collects attribute-less script bodies (`inline_scripts`) | Template equality check; `python -m unittest tests.test_ecosystem_manifest` |
| 6 | `py/clear-text-storage-sensitive-data` | `tests/test_validate.py:225` (`write_binary` fixture helper) | False positive / test-only | Dismissed (not changed) | Writes a synthetic, string-concatenation-built fake Hugging Face token to disk specifically to verify the validator's own secret scanner detects and rejects it; not a real credential. See `codeql-dismissals.json` entry #6. |
| 7 | `py/clear-text-logging-sensitive-data` | `blueprints/convergence-practice/wsl-restore/run.py:147` (`print(json.dumps({'status':report['status'],'commands':len(report['commands']),'checks':len(report['checks'])}))`) | False positive | Dismissed (not changed) | The printed sink only carries `report['status']` and two `len()` counts. CodeQL's taint path reaches it through `report`, which accumulates command records from `call()`; `call()`'s `password=` keyword only ever stores a `Path` to a 0600 password *file* (`--password-file`) in `argv`/`report`, never file contents — `private_key()` writes the random password bytes directly to that file and never returns or logs them. The finding is a keyword-name match on `password`, not a flow of secret bytes to the print. See `codeql-dismissals.json` entry #7. |
| 8 | `py/incomplete-url-substring-sanitization` | `tests/test_lifecycle_capture.py:103` (mock `Routed.get` used `"sec.gov" in url`) | Real defect (small, test-scoped) | Fixed: replaced the substring check with `urlparse(url).hostname` compared exactly (`== "sec.gov"` or `.endswith(".sec.gov")`), preserving the test's intent of distinguishing SEC-origin URLs from other publishers | `python -m unittest tests/test_lifecycle_capture.py` |
| 9 | `py/clear-text-storage-sensitive-data` | `tests/test_validate.py:483` (`ScanFileForPrivateContentTests.write()` helper: `path.write_text(content, encoding="utf-8")`) | False positive / test-only | Dismissed (not changed) | This is the `write()` fixture helper shared by `ScanFileForPrivateContentTests`, not the PDF-fixture tests (those are `test_pdf_*`, lines 401-465, and write through a different `write_binary` helper — see #6). Callers of `write()` (e.g. `test_github_token_is_reported`, `test_cli_scan_file_fails_on_a_planted_secret_without_echoing_it`) pass synthetic, string-concatenation-built fake GitHub/Anthropic-style tokens to verify `scan_file_for_private_content()` and `--scan-file` detect them without echoing them back; none is a real credential. See `codeql-dismissals.json` entry #9. |
| 10 | `py/clear-text-storage-sensitive-data` | `tests/test_validate.py:517` (latin-1 fallback fixture embeds a synthetic token) | False positive / test-only | Dismissed (not changed) | Same synthetic-fake-token pattern, verifying the non-UTF-8 fallback path of `scan_file_for_private_content()`. See `codeql-dismissals.json` entry #10. |

## Alternatives considered

- **Dismiss all 10 as "used in tests".** Rejected for alerts #1, #3, #4, #5, #8: each has a small, idiomatic
fix (URL-scheme allowlisting, stdlib HTML tokenizer, exact hostname comparison) that keeps the
flagged code's or test's intent and removes the actual gap, per the project's preference for a code fix
over dismissal when the fix is small.
- **Add `re.I` to the `<script>` regexes (first pass).** Superseded: `py/bad-tag-filter` also flags patterns
that miss browser-accepted end-tag variants such as `</script >`, so case-insensitivity alone would likely
leave the three alerts open. The stdlib `html.parser` (no new dependency; the tests already used it) removes
the heuristic instead of chasing it.
- **Dismiss #3-#5 as false positives.** Rejected: the parser replacement is small, keeps exact behaviour on the
real pages, and needs no dismissal.
- **Rename the `password=` parameter in `run.py`'s `call()` to sidestep CodeQL's naming heuristic.** Rejected:
this would only game the scanner's keyword match, not fix or clarify anything; the dismissal correctly
records why the flow is safe instead.

## Evidence class

`local_integration`: `python -m unittest` over each touched test module (and the full suite) plus
`scripts/build_ecosystem.py --check`, run from this worktree. No live GitHub Actions CodeQL re-analysis was
run locally; the hosted default-setup workflow re-analyses the PR when opened, per the task's acceptance
scope.

## Independent-review resolution (2026-09-23)

An independent reviewer of the first pass (commit `65f73e2`) found the location prose for alerts #7 and #9
pointed at the wrong code (both misidentified their sink after the `796f759` re-location), and that alert #1
lacked an executed, repeatable check of the committed `safeHref` helper. All three are resolved here:

- **#7** — corrected: the flagged sink is `run.py:147`'s `print(json.dumps(...))` of `report['status']` and
two `len()` counts, reached through `report`'s accumulation of `call()`'s command records, not the earlier
`call(..., password=root/'wrong-password', ...)` line. The "false positive" conclusion is unchanged (only
a password-*file* path is ever stored, never file contents); the location description and the corresponding
`codeql-dismissals.json` comment are now accurate.
- **#9** — corrected: the flagged sink is `ScanFileForPrivateContentTests.write()` (line 483,
`path.write_text(content, encoding="utf-8")`), not the PDF-fixture tests (which use a separate
`write_binary` helper and are alert #6's site). The "used in tests" conclusion is unchanged; the location
description and dismissal comment now name the correct helper and its actual callers.
- **#1** — relabeled from "real defect" to defense in depth: `scripts/build_ecosystem.py`'s `public_url()`
(credential-free `https:` only) and `loopback_url()` (`http:`/`https:` to loopback hosts only), both present
before this unit's fix, are the primary control that keeps other schemes out of the catalog data the
template renders, with `tests/test_ecosystem_manifest.py` asserting `javascript:` URLs
are stripped at build time. The `safeHref()` DOM-write guard remains as a second, independent barrier. Its
"smoke check" claim, which named no runnable command, is replaced with an executed, repeatable check:
`tests/test_ecosystem_manifest.py::test_safe_href_allowlists_http_https_and_rejects_other_schemes` extracts
the committed helper verbatim from `template.html` (not a reimplementation) and runs it under Node,
asserting `javascript:`, `data:`, `vbscript:`, `file:`, and `mailto:` all resolve to `about:blank` while
`https:`/`http:`/protocol-relative/relative URLs pass through unchanged.

The reviewer's rerun also flagged a skipped-test-count mismatch (337 claimed vs. 338 observed) on the full
`python -m unittest discover` run. A second independent rerun of three consecutive runs showed the skip count
itself varies between runs on this host (environment-dependent skips), so neither number is a fixed
property; every run reported 0 failures and 0 errors, which is the acceptance signal.

`manifests/evidence.json`'s `sha256`/`bytes` entries for the two files this pass edited
(`tests/test_ecosystem_manifest.py`, this decision doc) were refreshed via `host_receipts.register_file()`
(the same function the project's own receipt-recording path uses) so `scripts/validate.py` stays green; this
decision doc itself was not previously registered in `files[]` and is now added.

Evidence class for this section: `local_integration` (repeated `python -m unittest`, `--check`, `validate.py`,
`validate_catalogs.py`, `evidence_manifest.py --check`, guarded `gitleaks`, all in this worktree). No live
GitHub Actions CodeQL re-analysis was run locally.
3 changes: 2 additions & 1 deletion docs/ecosystem/template.html
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@
const $ = id => document.getElementById(id);
const make = (tag, cls, content) => { const node = document.createElement(tag); if(cls) node.className = cls; if(content !== undefined) node.textContent = content; return node; };
const add = (parent, ...children) => { children.forEach(child => parent.appendChild(child)); return parent; };
const link = (label, url) => { const node = make("a", "", label); node.href = url; node.target = "_blank"; node.rel = "noopener noreferrer"; return node; };
const safeHref = url => { try { const parsed = new URL(url, location.href); return (parsed.protocol === "https:" || parsed.protocol === "http:") ? parsed.href : "about:blank"; } catch(e) { return "about:blank"; } };
const link = (label, url) => { const node = make("a", "", label); node.href = safeHref(url); node.target = "_blank"; node.rel = "noopener noreferrer"; return node; };
const layerNames = new Map(data.layers.map(layer => [layer.id, layer.name]));
document.querySelectorAll("[data-catalog-tab]").forEach(tab => { tab.hidden = !data.grand_catalogs; });
$("tab-landscape").hidden = !data.landscape;
Expand Down
25 changes: 15 additions & 10 deletions manifests/evidence.json
Original file line number Diff line number Diff line change
Expand Up @@ -7987,6 +7987,11 @@
"sha256": "4e32f464b3466e03be4122471fec9cc46886238f602a7ec3d128bfe2eac4e9e7",
"bytes": 4241
},
{
"path": "docs/decisions/2026-09-22-codeql-first-analysis.md",
"sha256": "e71a63db7692ddf497ebdcb6ec4c3e979f3afaf61944731efbee35c28f982b6b",
"bytes": 11943
},
{
"path": "docs/direct-results.md",
"sha256": "12a511fee11f22c90b9b622701d5485be9c8247167102b5768d694244f939a5b",
Expand Down Expand Up @@ -8034,8 +8039,8 @@
},
{
"path": "docs/ecosystem/template.html",
"sha256": "516cce6c31a76ffe7d75ea75ee575e5bf3f185be2bb08722b57c4f8b62275d1e",
"bytes": 102997
"sha256": "de95e0e075204efe5ade37aa11daad625035b3a2747eb89f1c2aec5ab61cdb55",
"bytes": 103223
},
{
"path": "docs/evidence.md",
Expand Down Expand Up @@ -13019,8 +13024,8 @@
},
{
"path": "scripts/build_ecosystem.py",
"sha256": "cc8ef0795733e74bcefb802d93c6162299b12eed4eb4a32484e7d3e39a6a69e3",
"bytes": 48546
"sha256": "0a84a2c596512eac0f7e470cd287b9dcbea2c0c2998e14e0f98a2f71160fd150",
"bytes": 49805
},
{
"path": "scripts/catalog_decisions.py",
Expand Down Expand Up @@ -13269,8 +13274,8 @@
},
{
"path": "tests/test_claude_repository_evidence.py",
"sha256": "36e2b158789e765614e5376565727fd7678b9769c8695c617bed4bc45c2d97d6",
"bytes": 10269
"sha256": "bc84e4527cbbd894ad524981ee5c90230552a42cb53e7088699ee9a0d2730093",
"bytes": 11326
},
{
"path": "tests/test_component_matrix.py",
Expand Down Expand Up @@ -13299,8 +13304,8 @@
},
{
"path": "tests/test_ecosystem_manifest.py",
"sha256": "b5e847263c742924ca96773e4eded4dca9d0255c3245a4209744ec588f8c054f",
"bytes": 56250
"sha256": "9468971db76fab5c185d1710f4d19e06d1b2a2c80121c65dc916023579cafdfd",
"bytes": 58693
},
{
"path": "tests/test_execution_realism.py",
Expand Down Expand Up @@ -13374,8 +13379,8 @@
},
{
"path": "tests/test_lifecycle_capture.py",
"sha256": "ab76bdda7d8df777ed5efbf740a0456f18358a3e2cd19603e2e59a80c68757b2",
"bytes": 6656
"sha256": "266554101a1fe59e709a81003ed472b0f70b2feb4029efdd5e95e4931f1a3d28",
"bytes": 6812
},
{
"path": "tests/test_lifecycle_sample.py",
Expand Down
39 changes: 37 additions & 2 deletions scripts/build_ecosystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import base64
import hashlib
import html
from html.parser import HTMLParser
import json
from pathlib import Path
import re
Expand Down Expand Up @@ -692,6 +693,38 @@ def file_url(path):
"inputs": sorted(inputs.values(), key=lambda row: row["path"]), **curated}


class InlineScripts(HTMLParser):
"""Bodies of attribute-less <script> elements (tag case, end-tag whitespace and
attributes handled like a browser). `opened` counts every script start tag so a
caller can require it to match the raw "<script" count: html.parser hides a script
inside a comment or a self-closing <script/> that a browser would still run."""

def __init__(self, text):
super().__init__(convert_charrefs=False)
self.bodies, self.body, self.opened, self.self_closed = [], None, 0, 0
self.feed(text)
self.close()

def handle_starttag(self, tag, attrs):
if tag == "script":
self.opened += 1
self.body = None if attrs else []

def handle_startendtag(self, tag, attrs):
if tag == "script":
self.opened += 1
self.self_closed += 1

def handle_data(self, data):
if self.body is not None:
self.body.append(data)

def handle_endtag(self, tag):
if tag == "script" and self.body is not None:
self.bodies.append("".join(self.body))
self.body = None


def render_from_data(data, root):
template = safe_file(root, TEMPLATE).read_text(encoding="utf-8")
require(template.count("@@DATA@@") == 1, "template must have one embedded data marker")
Expand All @@ -701,8 +734,10 @@ def render_from_data(data, root):
body += 'No data leaves this page. Public source repository: <a href="'
body += html.escape(data["repository_url"], quote=True) + '">native-agent-stack</a>.</p></noscript>'
result = template.replace("@@DATA@@", encoded).replace("<!--@@BODY@@-->", body)
scripts = re.findall(r"<script>(.*?)</script>", result, re.S)
require(len(scripts) == 1, "template must have one inline application script")
parsed = InlineScripts(result)
scripts = parsed.bodies
require(len(scripts) == 1 and not parsed.self_closed and parsed.opened == result.lower().count("<script"),
"template must have one inline application script")
script_hash = base64.b64encode(hashlib.sha256(scripts[0].encode()).digest()).decode()
return result.replace("@@SCRIPT_HASH@@", script_hash).encode("utf-8")

Expand Down
33 changes: 32 additions & 1 deletion tests/test_claude_repository_evidence.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,41 @@

import base64
import hashlib
from html.parser import HTMLParser
import json
from pathlib import Path
import re
import unittest


class ExecutableScripts(HTMLParser):
"""Bodies of <script> elements a browser would run (JSON data blocks excluded)."""

def __init__(self, text):
super().__init__(convert_charrefs=False)
self.bodies, self.body, self.opened = [], None, 0
self.feed(text)
self.close()

def handle_starttag(self, tag, attrs):
if tag == "script":
self.opened += 1
types = [value for name, value in attrs if name == "type"]
self.body = None if types[:1] == ["application/json"] else []

def handle_startendtag(self, tag, attrs):
pass # a browser ignores "/>" on <script>; leaving it uncounted makes the raw-count check fail

def handle_data(self, data):
if self.body is not None:
self.body.append(data)

def handle_endtag(self, tag):
if tag == "script" and self.body is not None:
self.bodies.append("".join(self.body))
self.body = None


ROOT = Path(__file__).resolve().parents[1]
EVIDENCE_ID = "claude-repository-evidence-20260921"
ARTIFACTS = ROOT / "evidence/artifacts" / EVIDENCE_ID
Expand Down Expand Up @@ -144,7 +173,9 @@ def test_page_admits_only_its_own_script_and_loads_images_from_the_artifact_dire
self.assertIn("connect-src 'none'", policy)
script_src = policy.split("script-src", 1)[1].split(";", 1)[0]
self.assertNotIn("unsafe", script_src)
bodies = [body for attrs, body in re.findall(r"<script([^>]*)>(.*?)</script>", page, re.S) if "application/json" not in attrs]
parsed = ExecutableScripts(page)
self.assertEqual(parsed.opened, page.lower().count("<script")) # no script hidden from the parser
bodies = parsed.bodies
self.assertEqual(len(bodies), 1)
expected = "'sha256-" + base64.b64encode(hashlib.sha256(bodies[0].encode("utf-8")).digest()).decode("ascii") + "'"
self.assertEqual(script_src.split(), [expected])
Expand Down
Loading
Loading