From d79f1eba0621b558b96e731b8a05d96035bae2ca Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 18:45:01 +0000 Subject: [PATCH 01/12] ci: close two gate bypasses and test the exemption mechanism MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-negotiable-5 gate had two bypasses, both found by review as #22 was merging, both live on main: 1. `/* // NETRELISH-ALLOW-ENDPOINT: adr-0006 */` was accepted, because `line.partition("//")` scanned whatever followed an embedded `//`. A commented-out block containing a marker silently approved the line below it. 2. Approval was keyed to the SARIF startLine, so one marker approved every finding on that line. Adding a call beside an approved one got past the gate. Both are now closed, and a third hole the previous implementation documented as unclosable — a `//` inside a string literal ahead of the marker — is closed with them, because the fix is a real scanner rather than another string match. A marker now only counts inside genuine line-comment text, decided by scanning the file once while tracking the three contexts that can hide or reveal a `//`: nested block comments, string literals, and raw strings of any hash count, multiline forms included. Unterminated constructs fail closed for free. Shared lines fail closed: a marker names a line, not a call, so a line carrying more than one gate finding cannot be approved by one. Findings are deduplicated on position first, so the same result appearing in two SARIF files is still one finding rather than a false shared line. Every defect this mechanism has had — five now — has been in its enforcement rather than its design, and each was caught by a human reading it, or not caught at all, because a heredoc inside a workflow cannot be run. So the logic moves to scripts/codeql_gate.py with 44 tests beside it, and ci.yml gains a Ubuntu job that runs them on every push — deliberately outside the CodeQL workflow, which cannot build this project on the current xcode-27 image. A gate whose only proof of correctness lives inside the job that is currently red is not proven at all. Also renumbers one of two ADRs that both claimed 0006. The gate resolves a citation by globbing docs/adr/NNNN-*.md, so `adr-0006` named two unrelated decisions and could not be cited at all; it now fails closed on ambiguity, and a test asserts the repo never reintroduces the clash. The later of the two (direct-build updates, 2026-09-23) becomes 0007; the keychain ADR of 2026-09-21 keeps 0006. Sources/ is untouched. No app behaviour, no endpoint. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/ci.yml | 16 +- .github/workflows/codeql.yml | 139 +----- .gitignore | 4 + docs/RELEASING.md | 2 +- ...pdates.md => 0007-direct-build-updates.md} | 2 +- scripts/codeql_gate.py | 423 ++++++++++++++++++ scripts/tests/test_codeql_gate.py | 372 +++++++++++++++ 7 files changed, 831 insertions(+), 127 deletions(-) rename docs/adr/{0006-direct-build-updates.md => 0007-direct-build-updates.md} (97%) create mode 100644 scripts/codeql_gate.py create mode 100644 scripts/tests/test_codeql_gate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4cf958e..f3da1a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,9 +1,10 @@ name: CI -# Runs on every push and PR. Three things must hold: +# Runs on every push and PR. Four things must hold: # 1. generated files match their sources (Tokens.swift, Symbols.swift, Sources/Pantry/Models, NetRelish.xcodeproj) # 2. both product flavors build — AppStore and Direct (DIRECT_BUILD) # 3. the tests pass (hosted NetRelishTests + unhosted GeneratorTests) +# 4. the non-negotiable-5 gate still enforces what it claims to # Read-only token. Every job here checks out, regenerates, builds or tests — # nothing writes back to the repository. Without this block the workflow @@ -87,3 +88,16 @@ jobs: with: name: design-kit-snapshots path: design-kit-snapshots/*.png + + gate-tests: + name: CodeQL gate self-test + # Ubuntu, and not xcode-27, on purpose. This tests scripts/codeql_gate.py, + # which is pure Python and touches no Swift — and it MUST keep running while + # the CodeQL workflow cannot build this project on the macOS image. A gate + # whose only proof of correctness lives inside the job that is currently red + # is not proven at all. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (node24) + - name: The gate enforces what it claims to + run: python3 -m unittest discover -s scripts/tests -v diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index a006a60..bfc20dd 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -151,132 +151,23 @@ jobs: category: "/language:swift" output: .codeql-results - # analyze uploads findings; it never fails the job on them. Without this - # step the gate is a report, not a gate — the CodeQL check can sit green - # with an alert on it, so requiring it in branch protection would still let - # a fifth endpoint merge. Gating repo-side works too (code scanning's - # check-failure severity, or a ruleset), but that is a console toggle a - # fork, a clone or a forgetful afternoon loses. This travels with the repo. + # analyze uploads findings; it never fails the job on them, and there is + # no `fail-on` input. Without this step the gate is a report, not a gate — + # the CodeQL check can sit green with an alert on it, so requiring it in + # branch protection would still let a fifth endpoint merge. Gating + # repo-side works too (code scanning's check-failure severity, or a + # ruleset), but that is a console toggle a fork, a clone or a forgetful + # afternoon loses. This travels with the repo. + # + # The logic is in scripts/codeql_gate.py, not inline here. It has had five + # defects, every one in its enforcement rather than its design, and a + # heredoc cannot be tested — so each was caught by a human reading it, or + # not caught at all. It now has a suite in scripts/tests/ that the + # `CodeQL gate self-test` job in ci.yml runs on every push, including + # while THIS job cannot build the project. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a # red CodeQL job, which is the opposite of what the split was for. - name: Fail if the gate fired - run: | - python3 - <<'PY' - import glob, json, pathlib, re, sys - from urllib.parse import unquote, urlparse - - GATE = "netrelish/outbound-network-call" - - # An approved exception is a comment on the flagged line or the one - # directly above it, citing the ADR that approved it: - # - # // NETRELISH-ALLOW-ENDPOINT: adr-0007 — RFC 3161 timestamping - # - # It lives beside the code rather than in a list, so it shows up in - # every diff that touches the call, cannot drift away from what it - # exempts, and dies with the line. The ADR reference is required - # because CLAUDE.md already says an uncovered decision gets one, and - # an exception with no written reason is how a rule rots. - # - # Dismissing the alert in the Security tab deliberately does NOT - # satisfy this: that is invisible from the repository. - MARKER = re.compile(r"NETRELISH-ALLOW-ENDPOINT:\s*(adr-\d{4})") - - def exemption(line): - """The marker, but only in a // comment and naming a real ADR. - - Two bypasses this closes. Searching raw source matched the token - inside a string literal, so a line like - let s = "NETRELISH-ALLOW-ENDPOINT: adr-0001" was an exemption. - And checking only the adr-NNNN SHAPE meant adr-9999 passed with - no written decision existing at all, which made the ADR - requirement decoration rather than a control. - - Still not a Swift parser: a // inside a string literal ahead of - the marker would fool it. That is far narrower than either hole - above, and closing it properly needs parsed source, not text. - """ - _, sep, comment = line.partition("//") - if not sep: - return None - found = MARKER.search(comment) - if not found: - return None - number = found.group(1)[len("adr-"):] - if not list(pathlib.Path("docs/adr").glob(number + "-*.md")): - print("::error::" + found.group(1) - + " is cited but no docs/adr/" + number + "-*.md exists") - return None - return found.group(1) - - def source_path(uri): - """The checkout-relative path for a SARIF artifactLocation. - - SARIF gives a URI, not a filename. CodeQL usually emits a - repo-relative one, but a file:///... form (often with a - uriBaseId) is equally legal and pathlib.Path would treat the - whole thing as a literal relative name. The source would then - never open, no marker would be found, and a VALID exemption - would block the build with a misleading message. - - Fails closed either way, but closed-and-wrong is still wrong. - """ - parsed = urlparse(uri) - raw = unquote(parsed.path) if parsed.scheme == "file" else unquote(uri) - path = pathlib.Path(raw) - if path.is_absolute(): - try: - path = path.relative_to(pathlib.Path.cwd()) - except ValueError: - pass # genuinely outside the checkout; leave it absolute - return path - - paths = sorted(glob.glob(".codeql-results/*.sarif")) - if not paths: - # A missing SARIF must never read as "clean" — that is precisely - # the false assurance this whole query pack exists to prevent. - print("::error::no SARIF in .codeql-results; the gate did not run") - sys.exit(1) - - approved, blocking = [], [] - for path in paths: - with open(path) as f: - sarif = json.load(f) - for run in sarif.get("runs", []): - for result in run.get("results", []): - if result.get("ruleId") != GATE: - continue - where = result["locations"][0]["physicalLocation"] - uri = where["artifactLocation"]["uri"] - line = where.get("region", {}).get("startLine", 1) - - source, found = source_path(uri), None - if source.is_file(): - body = source.read_text(errors="replace").splitlines() - for candidate in body[max(0, line - 2):line]: - found = found or exemption(candidate) - else: - print("::error::cannot read " + str(source) + " (from SARIF uri " - + uri + "); an exemption there could not be checked") - - entry = (str(source), line, result["message"]["text"], found) - (approved if found else blocking).append(entry) - - for uri, line, _, found in approved: - print(f"::notice file={uri},line={line}::approved endpoint ({found})") - for uri, line, message, _ in blocking: - print(f"::error file={uri},line={line}::{message}") - - if approved: - print(f"\n{len(approved)} approved exception(s), re-printed every run so they stay visible.") - - if blocking: - print(f"\n{GATE} fired {len(blocking)} time(s) with no approved exception. " - "CLAUDE.md non-negotiable 5 permits page loads, StoreKit, RFC 3161 " - "timestamping and direct-build licence activation, and nothing else.") - sys.exit(1) - - print(f"{GATE}: clean across {len(paths)} SARIF file(s).") - PY + run: python3 scripts/codeql_gate.py .codeql-results diff --git a/.gitignore b/.gitignore index e00168a..a9aa727 100644 --- a/.gitignore +++ b/.gitignore @@ -29,3 +29,7 @@ AuthKey_*.p8 # logs *.log + +# Python bytecode from scripts/ and its tests +__pycache__/ +*.pyc diff --git a/docs/RELEASING.md b/docs/RELEASING.md index afe15d0..700093d 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -23,7 +23,7 @@ The direct build has **no updater, deliberately**: no Sparkle, no appcast, nothing that polls. A new version is announced by email to the address that bought the licence, and About links to the download page. Decided by Ryan on 2026-09-23; the reasoning, and what would overturn it, are in -`docs/adr/0006-direct-build-updates.md`. +`docs/adr/0007-direct-build-updates.md`. The constraint that drove it is CLAUDE.md §5, which lists every call the app may make and ends "Add an endpoint and you've broken this rule." An appcast diff --git a/docs/adr/0006-direct-build-updates.md b/docs/adr/0007-direct-build-updates.md similarity index 97% rename from docs/adr/0006-direct-build-updates.md rename to docs/adr/0007-direct-build-updates.md index a85c815..84a6245 100644 --- a/docs/adr/0006-direct-build-updates.md +++ b/docs/adr/0007-direct-build-updates.md @@ -1,4 +1,4 @@ -# ADR 0006 — The direct build announces updates; it does not fetch them +# ADR 0007 — The direct build announces updates; it does not fetch them **Status:** accepted (Ryan, 2026-09-23) · **Date:** 2026-09-23 · **PR:** docs: ADR for the direct-build updater diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py new file mode 100644 index 0000000..c5e38a6 --- /dev/null +++ b/scripts/codeql_gate.py @@ -0,0 +1,423 @@ +#!/usr/bin/env python3 +"""The gate for CLAUDE.md non-negotiable 5, and the exemption mechanism it honours. + +`github/codeql-action/analyze` uploads findings but never fails the job, and it +has no `fail-on` input. So the workflow enforces the rule itself: analyze writes +SARIF, and this script fails the build if `netrelish/outbound-network-call` +produced any result that is not approved in writing. A MISSING SARIF also fails +— a gate that silently passes when it did not run is exactly the false assurance +this query pack exists to prevent. + +Gating repo-side would work too (code scanning's check-failure severity, or a +ruleset), but that is a console toggle a fork, a clone or a forgetful afternoon +loses. This travels with the repo. + +WHY THIS IS A FILE AND NOT A HEREDOC IN THE WORKFLOW +---------------------------------------------------- +It used to be a heredoc. Every defect this mechanism has had — five now — was in +its ENFORCEMENT rather than its design: + + 1. no approved path at all + 2. markers matched inside string literals; cited ADRs never had to exist + 3. SARIF URIs treated as plain filenames + 4. markers inside /* block comments */ accepted + 5. one marker approving every finding that shared its line + +Each idea was right; each implementation had a hole. A heredoc cannot be tested, +so every one of those was caught by a human reading it, or not caught at all. +The mechanism is now a module with a test suite beside it in scripts/tests/, and +that suite runs on every push — including while CodeQL itself cannot build this +project. See scripts/tests/test_codeql_gate.py. + +FAIL CLOSED, ALWAYS +------------------- +Every ambiguity here resolves toward blocking the build. An unreadable source +file, an ADR citation matching two files, two findings sharing one line, a +mangled SARIF URI: all block. Being wrong in the blocking direction costs +someone an explanation. Being wrong in the approving direction costs the rule +its meaning. +""" + +from __future__ import annotations + +import argparse +import glob +import json +import pathlib +import re +import sys +from urllib.parse import unquote, urlparse + +# The gate. `netrelish/remote-capable-url-read` is the audit and is deliberately +# NOT gated: it has two legitimate baseline hits, and failing on it would teach +# everyone to ignore a red CodeQL job — the opposite of what the split was for. +GATE = "netrelish/outbound-network-call" + +# An approved exception is a comment on the flagged line or the one directly +# above it, citing the ADR that approved it: +# +# // NETRELISH-ALLOW-ENDPOINT: adr-0007 — RFC 3161 timestamping +# +# It lives beside the code rather than in a list, so it shows up in every diff +# that touches the call, cannot drift away from what it exempts, and dies with +# the line. The ADR reference is required because CLAUDE.md already says an +# uncovered decision gets one, and an exception with no written reason is how a +# rule rots. +# +# Dismissing the alert in the Security tab deliberately does NOT satisfy this: +# that is invisible from the repository. +MARKER = re.compile(r"NETRELISH-ALLOW-ENDPOINT:\s*(adr-\d{4})") + + +# -------------------------------------------------------------------------- +# Where a marker is allowed to live +# -------------------------------------------------------------------------- + +def line_comments(source: str) -> dict[int, str]: + """Map 1-based line number -> the text of the genuine `//` comment on it. + + A marker only counts inside real line-comment text. Deciding what that means + is the whole job, and text matching is not up to it: + + let s = "NETRELISH-ALLOW-ENDPOINT: adr-0001" // a string, not a comment + /* // NETRELISH-ALLOW-ENDPOINT: adr-0006 */ // commented-out, not a comment + let u = "https://x/" // NETRELISH-ALLOW-ENDPOINT: adr-0007 <- this one is real + + The earlier `line.partition("//")` accepted the first two. The second was + the dangerous one: a commented-out block that happens to contain a marker + silently approves whatever line follows it. + + So this scans the file once, front to back, tracking the three lexical + contexts that can hide or reveal a `//`: block comments (which NEST in + Swift), string literals, and raw string literals (`#"..."#`, any number of + hashes, where the escape is `\\#`). Multiline `\"\"\"..\"\"\"` too, since a + marker inside one spans lines and would otherwise look like a comment. + + Unterminated constructs fail closed for free: an unclosed `/*` swallows the + rest of the file, so no further line records a comment, so nothing below it + can be approved. + + This is a comment/string scanner, not a Swift parser, and that is the + correct scope — it needs to answer one question, and it answers it for every + way Swift can spell these three things. + """ + comments: dict[int, str] = {} + source_length = len(source) + index = 0 + line = 1 + + depth = 0 # block-comment nesting depth; 0 means we are not in one + in_string = False + multiline = False # the \"\"\" form, which may contain newlines + hashes = 0 # raw-string delimiter count: #"..."# is 1, ##"..."## is 2 + + def advance(count: int) -> None: + """Step `count` characters, keeping the line number honest.""" + nonlocal index, line + line += source.count("\n", index, index + count) + index += count + + while index < source_length: + char = source[index] + + if char == "\n": + # A single-line string cannot span a newline. Reaching one means the + # source is malformed; resync at the line break rather than treating + # the remainder of the file as string content. + if in_string and not multiline: + in_string = False + advance(1) + continue + + if depth: + if source.startswith("/*", index): + depth += 1 + advance(2) + elif source.startswith("*/", index): + depth -= 1 + advance(2) + else: + advance(1) + continue + + if in_string: + closer = '"""' if multiline else '"' + if source.startswith("\\" + "#" * hashes, index): + advance(1 + hashes + 1) # backslash, its hashes, the escaped character + elif source.startswith(closer + "#" * hashes, index): + in_string = False + advance(len(closer) + hashes) + else: + advance(1) + continue + + # Ordinary code. + if source.startswith("//", index): + end = source.find("\n", index) + end = source_length if end == -1 else end + # First genuine `//` wins: everything after it on the line is inside + # that same comment. + comments.setdefault(line, source[index + 2:end]) + advance(end - index) + continue + + if source.startswith("/*", index): + depth = 1 + advance(2) + continue + + # A run of `#` only opens a string when a quote follows it; `#if` and + # `#Preview` are not string literals. + run = 0 + while index + run < source_length and source[index + run] == "#": + run += 1 + quote_at = index + run + if quote_at < source_length and source[quote_at] == '"': + in_string = True + hashes = run + multiline = source.startswith('"""', quote_at) + advance(run + (3 if multiline else 1)) + continue + + advance(1) + + return comments + + +def cited_adr(comment: str, repo_root: pathlib.Path, report: "Report") -> str | None: + """The ADR this comment cites, if it cites exactly one that exists. + + Checking only the adr-NNNN SHAPE meant `adr-9999` passed with no written + decision existing at all, which made the ADR requirement decoration rather + than a control. So the file has to be there. + + It also has to be the ONLY file there. `docs/adr/` has held two different + ADRs both numbered 0006, and a citation that resolves to two unrelated + decisions approves against whichever one the reader assumes. That is an + ambiguous exemption, so it is refused and reported. + """ + found = MARKER.search(comment) + if not found: + return None + + citation = found.group(1) + number = citation[len("adr-"):] + matches = sorted(p.name for p in (repo_root / "docs" / "adr").glob(number + "-*.md")) + + if not matches: + report.errors.append( + citation + " is cited but no docs/adr/" + number + "-*.md exists" + ) + return None + + if len(matches) > 1: + report.errors.append( + citation + " is ambiguous: it matches " + ", ".join(matches) + + ". Renumber so the citation names one decision." + ) + return None + + return citation + + +# -------------------------------------------------------------------------- +# Reading what CodeQL produced +# -------------------------------------------------------------------------- + +def source_path(uri: str) -> pathlib.Path: + """The checkout-relative path for a SARIF artifactLocation. + + SARIF gives a URI, not a filename. CodeQL usually emits a repo-relative one, + but a file:///... form (often with a uriBaseId) is equally legal, and + pathlib.Path would treat the whole thing as a literal relative name. The + source would then never open, no marker would be found, and a VALID + exemption would block the build with a misleading message. + + Fails closed either way, but closed-and-wrong is still wrong. + """ + parsed = urlparse(uri) + raw = unquote(parsed.path) if parsed.scheme == "file" else unquote(uri) + path = pathlib.Path(raw) + if path.is_absolute(): + try: + path = path.relative_to(pathlib.Path.cwd()) + except ValueError: + pass # genuinely outside the checkout; leave it absolute + return path + + +class Finding: + """One gate result, reduced to the parts that decide its fate.""" + + __slots__ = ("path", "line", "start_column", "end_column", "message") + + def __init__(self, path, line, start_column, end_column, message): + self.path = path + self.line = line + self.start_column = start_column + self.end_column = end_column + self.message = message + + def identity(self): + """What makes two findings the same call rather than two calls. + + The same result can appear in more than one SARIF file. Columns + distinguish two genuinely different calls that share a line, so they + belong in the key — otherwise deduplication would quietly merge the very + case the shared-line rule exists to catch. + """ + return (self.path, self.line, self.start_column, self.end_column, self.message) + + +class Report: + def __init__(self): + self.approved = [] # (Finding, adr) + self.blocking = [] # (Finding, reason) + self.errors = [] # strings, printed as ::error and forcing failure + self.sarif_count = 0 + + @property + def ok(self) -> bool: + return not self.blocking and not self.errors + + +def evaluate(results_dir, repo_root=None) -> Report: + """Decide the build. Pure: it reads, it does not print or exit.""" + repo_root = pathlib.Path(repo_root or pathlib.Path.cwd()) + report = Report() + + paths = sorted(glob.glob(str(pathlib.Path(results_dir) / "*.sarif"))) + report.sarif_count = len(paths) + if not paths: + # A missing SARIF must never read as "clean" — that is precisely the + # false assurance this whole query pack exists to prevent. + report.errors.append( + "no SARIF in " + str(results_dir) + "; the gate did not run" + ) + return report + + findings = {} + for path in paths: + with open(path) as handle: + sarif = json.load(handle) + for run in sarif.get("runs", []): + for result in run.get("results", []): + if result.get("ruleId") != GATE: + continue + where = result["locations"][0]["physicalLocation"] + region = where.get("region", {}) + finding = Finding( + path=str(source_path(where["artifactLocation"]["uri"])), + line=region.get("startLine", 1), + start_column=region.get("startColumn"), + end_column=region.get("endColumn"), + message=result["message"]["text"], + ) + findings[finding.identity()] = finding + + # Group by the unit the exemption mechanism can actually address. A marker + # names a line, not a call, so a line carrying two gate findings cannot be + # approved by one — that was bypass 5: adding a second call beside an + # already-approved one got past the gate. Fail closed instead of inventing + # an approval the author never wrote. + lines: dict[tuple, list[Finding]] = {} + for finding in findings.values(): + lines.setdefault((finding.path, finding.line), []).append(finding) + + source_cache: dict[str, dict[int, str] | None] = {} + + for (path, line), group in sorted(lines.items()): + if path not in source_cache: + candidate = repo_root / path + if candidate.is_file(): + source_cache[path] = line_comments( + candidate.read_text(errors="replace") + ) + else: + source_cache[path] = None + report.errors.append( + "cannot read " + path + + "; an exemption there could not be checked" + ) + comments = source_cache[path] + + adr = None + if comments is not None: + # The flagged line, or the one directly above it. + for candidate_line in (line - 1, line): + if adr: + break + text = comments.get(candidate_line) + if text: + adr = cited_adr(text, repo_root, report) + + if len(group) > 1: + reason = ( + str(len(group)) + " gate findings share this line, so one marker " + "cannot approve them individually — split them across lines and " + "give each its own exemption" + ) + for finding in sorted(group, key=lambda f: (f.start_column or 0)): + report.blocking.append((finding, reason)) + elif adr: + report.approved.append((group[0], adr)) + else: + report.blocking.append((group[0], group[0].message)) + + return report + + +# -------------------------------------------------------------------------- +# Saying it out loud +# -------------------------------------------------------------------------- + +def main(argv=None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "results", nargs="?", default=".codeql-results", + help="directory holding the SARIF that `analyze` wrote", + ) + parser.add_argument( + "--repo-root", default=".", + help="checkout root that SARIF paths and docs/adr resolve against", + ) + args = parser.parse_args(argv) + + report = evaluate(args.results, args.repo_root) + + for message in report.errors: + print("::error::" + message) + for finding, adr in report.approved: + print( + "::notice file=" + finding.path + ",line=" + str(finding.line) + + "::approved endpoint (" + adr + ")" + ) + for finding, reason in report.blocking: + print( + "::error file=" + finding.path + ",line=" + str(finding.line) + + "::" + reason + ) + + if report.approved: + print( + "\n" + str(len(report.approved)) + + " approved exception(s), re-printed every run so they stay visible." + ) + + if report.blocking: + print( + "\n" + GATE + " fired " + str(len(report.blocking)) + + " time(s) with no approved exception. CLAUDE.md non-negotiable 5 " + "permits page loads, StoreKit, RFC 3161 timestamping and direct-build " + "licence activation, and nothing else." + ) + + if not report.ok: + return 1 + + print(GATE + ": clean across " + str(report.sarif_count) + " SARIF file(s).") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py new file mode 100644 index 0000000..c96147b --- /dev/null +++ b/scripts/tests/test_codeql_gate.py @@ -0,0 +1,372 @@ +#!/usr/bin/env python3 +"""Tests for the non-negotiable-5 gate. + +Every defect this exemption mechanism has had was in its enforcement rather +than its design, and every one was found by a human reading the code — because +the code was a heredoc inside a workflow and could not be run any other way. +Each named bypass below has a test here, and the tests run on every push, +independently of whether CodeQL itself can build the project. + +Run: python3 -m unittest discover -s scripts/tests +""" + +import json +import pathlib +import sys +import tempfile +import unittest + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parents[1])) + +import codeql_gate # noqa: E402 + +REPO = pathlib.Path(__file__).resolve().parents[2] + +MARKER = "NETRELISH-ALLOW-ENDPOINT: adr-0007" +CALL = "_ = try await URLSession.shared.data(from: url)" + + +def sarif(*results): + return {"runs": [{"results": list(results)}]} + + +def result(uri, line, rule=codeql_gate.GATE, message="outbound network call", + start_column=None, end_column=None): + region = {"startLine": line} + if start_column is not None: + region["startColumn"] = start_column + if end_column is not None: + region["endColumn"] = end_column + return { + "ruleId": rule, + "message": {"text": message}, + "locations": [{ + "physicalLocation": { + "artifactLocation": {"uri": uri}, + "region": region, + } + }], + } + + +class Checkout: + """A throwaway checkout: some Swift, some ADRs, some SARIF.""" + + def __init__(self, stack): + self.root = pathlib.Path(stack.enter_context(tempfile.TemporaryDirectory())) + (self.root / "docs" / "adr").mkdir(parents=True) + (self.root / ".codeql-results").mkdir() + self.adr("0007-timestamping.md") + + def adr(self, name): + (self.root / "docs" / "adr" / name).write_text("# ADR\n") + + def swift(self, name, body): + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body) + return path + + def results(self, document, name="swift.sarif"): + (self.root / ".codeql-results" / name).write_text(json.dumps(document)) + + def evaluate(self): + return codeql_gate.evaluate(self.root / ".codeql-results", self.root) + + +class ScannerTests(unittest.TestCase): + """Where a marker is, and is not, a comment.""" + + def comment_on(self, source, line): + return codeql_gate.line_comments(source).get(line) + + def test_whole_line_comment(self): + self.assertIn(MARKER, self.comment_on("// " + MARKER + "\n" + CALL, 1)) + + def test_trailing_comment_after_code(self): + self.assertIn(MARKER, self.comment_on(CALL + " // " + MARKER, 1)) + + def test_doc_comment_counts(self): + self.assertIn(MARKER, self.comment_on("/// " + MARKER, 1)) + + def test_marker_in_string_literal_is_not_a_comment(self): + self.assertIsNone(self.comment_on('let s = "// ' + MARKER + '"', 1)) + + def test_marker_in_block_comment_is_not_a_comment(self): + # Bypass 4. `line.partition("//")` accepted this. + self.assertIsNone(self.comment_on("/* // " + MARKER + " */", 1)) + + def test_marker_in_multi_line_block_comment_is_not_a_comment(self): + source = "/* disabled for now\n // " + MARKER + "\n*/\n" + CALL + self.assertIsNone(self.comment_on(source, 2)) + + def test_nested_block_comment_stays_closed(self): + # Swift block comments nest, so the first `*/` does not end this one. + source = "/* outer /* inner */ // " + MARKER + " */\n" + CALL + self.assertIsNone(self.comment_on(source, 1)) + + def test_code_after_nested_block_comment_is_code_again(self): + source = "/* a /* b */ c */ // " + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_unterminated_block_comment_swallows_the_rest(self): + source = "/* oops\n// " + MARKER + "\n" + CALL + self.assertEqual(codeql_gate.line_comments(source), {}) + + def test_comment_after_a_string_containing_slashes(self): + # The hole the previous implementation documented as unclosable: a `//` + # inside a string literal ahead of the real comment. + source = 'let u = URL(string: "https://example.com")! // ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_escaped_quote_does_not_end_the_string(self): + source = 'let s = "a \\" // ' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_raw_string_is_not_a_comment(self): + self.assertIsNone(self.comment_on('let s = #"// ' + MARKER + '"#', 1)) + + def test_raw_string_with_two_hashes(self): + self.assertIsNone(self.comment_on('let s = ##"// ' + MARKER + '"##', 1)) + + def test_quote_hash_inside_double_hash_raw_string_does_not_close_it(self): + source = 'let s = ##"a "# b // ' + MARKER + '"##' + self.assertIsNone(self.comment_on(source, 1)) + + def test_multiline_string_is_not_a_comment(self): + source = 'let s = """\n// ' + MARKER + '\n"""\n' + CALL + self.assertIsNone(self.comment_on(source, 2)) + + def test_line_after_multiline_string_is_code_again(self): + source = 'let s = """\nbody\n"""\n// ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 4)) + + def test_hash_directives_do_not_open_a_string(self): + source = "#if DIRECT_BUILD\n#Preview { EmptyView() }\n#endif\n// " + MARKER + self.assertIn(MARKER, self.comment_on(source, 4)) + + def test_line_numbers_survive_multiline_constructs(self): + source = 'let s = """\na\nb\n"""\n/* x\ny */\n// ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 7)) + + def test_first_comment_on_the_line_wins(self): + self.assertEqual(self.comment_on("// a // b", 1), " a // b") + + +class CitationTests(unittest.TestCase): + """A marker is only an exemption if it names exactly one real decision.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = pathlib.Path(self._tmp.name) + (self.root / "docs" / "adr").mkdir(parents=True) + self.addCleanup(self._tmp.cleanup) + + def cite(self, comment): + report = codeql_gate.Report() + return codeql_gate.cited_adr(comment, self.root, report), report + + def test_real_adr_is_accepted(self): + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + adr, report = self.cite(" " + MARKER) + self.assertEqual(adr, "adr-0007") + self.assertEqual(report.errors, []) + + def test_absent_adr_is_refused(self): + adr, report = self.cite(" NETRELISH-ALLOW-ENDPOINT: adr-9999") + self.assertIsNone(adr) + self.assertIn("no docs/adr/9999", report.errors[0]) + + def test_ambiguous_adr_number_is_refused(self): + # docs/adr/ has held two different decisions both numbered 0006. + (self.root / "docs" / "adr" / "0006-one.md").write_text("#\n") + (self.root / "docs" / "adr" / "0006-two.md").write_text("#\n") + adr, report = self.cite(" NETRELISH-ALLOW-ENDPOINT: adr-0006") + self.assertIsNone(adr) + self.assertIn("ambiguous", report.errors[0]) + self.assertIn("0006-one.md", report.errors[0]) + self.assertIn("0006-two.md", report.errors[0]) + + def test_comment_without_a_marker_is_not_an_error(self): + adr, report = self.cite(" just an ordinary comment") + self.assertIsNone(adr) + self.assertEqual(report.errors, []) + + +class GateTests(unittest.TestCase): + """End to end: SARIF in, verdict out.""" + + def setUp(self): + import contextlib + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.checkout = Checkout(self.stack) + + def test_missing_sarif_fails(self): + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertIn("the gate did not run", report.errors[0]) + + def test_clean_sarif_passes(self): + self.checkout.results(sarif()) + self.assertTrue(self.checkout.evaluate().ok) + + def test_audit_rule_alone_passes(self): + self.checkout.swift("Sources/A.swift", CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 1, rule="netrelish/remote-capable-url-read") + )) + self.assertTrue(self.checkout.evaluate().ok) + + def test_unapproved_gate_hit_blocks(self): + self.checkout.swift("Sources/A.swift", CALL) + self.checkout.results(sarif(result("Sources/A.swift", 1))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(len(report.blocking), 1) + + def test_marker_on_the_line_above_approves(self): + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(report.approved[0][1], "adr-0007") + + def test_marker_on_the_same_line_approves(self): + self.checkout.swift("Sources/A.swift", CALL + " // " + MARKER) + self.checkout.results(sarif(result("Sources/A.swift", 1))) + self.assertTrue(self.checkout.evaluate().ok) + + def test_marker_two_lines_above_does_not_approve(self): + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n\n" + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 3))) + self.assertFalse(self.checkout.evaluate().ok) + + def test_block_comment_marker_does_not_approve(self): + # Bypass 4, end to end. + self.checkout.swift("Sources/A.swift", "/* // " + MARKER + " */\n" + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertFalse(self.checkout.evaluate().ok) + + def test_string_literal_marker_does_not_approve(self): + self.checkout.swift("Sources/A.swift", 'let s = "' + MARKER + '"\n' + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertFalse(self.checkout.evaluate().ok) + + def test_unknown_adr_does_not_approve(self): + self.checkout.swift( + "Sources/A.swift", + "// NETRELISH-ALLOW-ENDPOINT: adr-9999\n" + CALL, + ) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertTrue(any("9999" in e for e in report.errors)) + + def test_two_findings_on_one_line_fail_closed(self): + # Bypass 5: one valid marker used to approve every finding on the line, + # so a second call beside an approved one got in free. + self.checkout.swift( + "Sources/A.swift", + "// " + MARKER + "\n" + CALL + "; " + CALL, + ) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=5, end_column=40), + result("Sources/A.swift", 2, start_column=60, end_column=95), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(len(report.blocking), 2) + self.assertEqual(report.approved, []) + self.assertIn("share this line", report.blocking[0][1]) + + def test_the_same_finding_in_two_sarif_files_is_one_finding(self): + # Deduplication must not turn a duplicate report into a shared line. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + hit = result("Sources/A.swift", 2, start_column=5, end_column=40) + self.checkout.results(sarif(hit), "a.sarif") + self.checkout.results(sarif(hit), "b.sarif") + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(len(report.approved), 1) + + def test_two_approved_findings_on_separate_lines(self): + self.checkout.swift( + "Sources/A.swift", + "// " + MARKER + "\n" + CALL + "\n// " + MARKER + "\n" + CALL, + ) + self.checkout.results(sarif( + result("Sources/A.swift", 2), + result("Sources/A.swift", 4), + )) + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(len(report.approved), 2) + + def test_unreadable_source_blocks(self): + self.checkout.results(sarif(result("Sources/Gone.swift", 1))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertTrue(any("cannot read" in e for e in report.errors)) + + def test_file_uri_resolves(self): + path = self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result(path.as_uri(), 2))) + self.assertTrue(self.checkout.evaluate().ok) + + def test_percent_encoded_uri_resolves(self): + self.checkout.swift("Sources/My App.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/My%20App.swift", 2))) + self.assertTrue(self.checkout.evaluate().ok) + + +class ExitCodeTests(unittest.TestCase): + """main() is what CI actually runs.""" + + def setUp(self): + import contextlib + self.stack = contextlib.ExitStack() + self.addCleanup(self.stack.close) + self.checkout = Checkout(self.stack) + + def run_main(self): + return codeql_gate.main([ + str(self.checkout.root / ".codeql-results"), + "--repo-root", str(self.checkout.root), + ]) + + def test_clean_exits_zero(self): + self.checkout.results(sarif()) + self.assertEqual(self.run_main(), 0) + + def test_blocking_exits_one(self): + self.checkout.swift("Sources/A.swift", CALL) + self.checkout.results(sarif(result("Sources/A.swift", 1))) + self.assertEqual(self.run_main(), 1) + + def test_missing_sarif_exits_one(self): + self.assertEqual(self.run_main(), 1) + + def test_approved_exits_zero(self): + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertEqual(self.run_main(), 0) + + +class RepositoryTests(unittest.TestCase): + """Invariants of this repo that the gate depends on.""" + + def test_adr_numbers_are_unique(self): + seen = {} + for path in sorted((REPO / "docs" / "adr").glob("[0-9][0-9][0-9][0-9]-*.md")): + seen.setdefault(path.name[:4], []).append(path.name) + clashes = {n: f for n, f in seen.items() if len(f) > 1} + self.assertEqual( + clashes, {}, + "Two ADRs share a number, so NETRELISH-ALLOW-ENDPOINT: adr-NNNN " + "cannot name one decision: " + repr(clashes), + ) + + +if __name__ == "__main__": + unittest.main() From fb54a28018b1412fa4bcbfcdd2cc9f43449fe58d Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 18:56:36 +0000 Subject: [PATCH 02/12] ci: close three more gate holes found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot found three, all correct, all failing open — which is the direction that matters. Same pattern as the five before them: the design was right, the enforcement had a gap. 1. Extended regex literals. `#/ ... /#` at any hash count was not a tracked context, so `#/ // NETRELISH-ALLOW-ENDPOINT: adr-0007 /#` read as a comment and approved the line below. Extended regex ignores whitespace, so that spelling is natural rather than contrived. Now skipped to its matching delimiter, unterminated included. The bare `/.../` form needs no handling: two unescaped slashes would end the literal at the first, so `//` cannot occur inside one. 2. A trailing marker also served as the comment "above" the next line — the shared-line bypass wearing a newline: _ = try await URLSession.shared.data(from: a) // ...ALLOW...: adr-0007 _ = try await URLSession.shared.data(from: b) Both were approved by one exemption. The line above is now offered only when it is not itself flagged; the standalone-comment-above form is untouched, since a line holding only a comment is never a finding. 3. The message was part of positional identity, so the same call site reported with different wording in two SARIF files split into two findings and blocked a valid exemption. Columns now settle identity when present. When they are absent the message stays in the key, because nothing else can tell two calls on a line apart — splitting one finding blocks, merging two approves, and only one of those errs safely. Also fixes a reference this branch missed: the table in docs/RELEASING.md still pointed the direct-build decision at "ADR 0006", which after the renumber is the unrelated Keychain ADR. The remaining 0006 references in Sources/ and project.yml are all about identity and are correct as they stand. Eleven regression tests, one per hole and one per edge the fixes touch. 55 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- docs/RELEASING.md | 2 +- scripts/codeql_gate.py | 63 ++++++++++++++++++---- scripts/tests/test_codeql_gate.py | 89 +++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 11 deletions(-) diff --git a/docs/RELEASING.md b/docs/RELEASING.md index 700093d..a661286 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -17,7 +17,7 @@ Two builds ship from one codebase (`#if DIRECT_BUILD`): | Build | Signs with | Distributed by | Updates by | | :-- | :-- | :-- | :-- | | App Store | Apple Distribution | App Store Connect | the App Store | -| Direct | Developer ID | *undecided* | email — by design, see ADR 0006 | +| Direct | Developer ID | *undecided* | email — by design, see ADR 0007 | The direct build has **no updater, deliberately**: no Sparkle, no appcast, nothing that polls. A new version is announced by email to the address that diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index c5e38a6..74bb9ac 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -166,19 +166,38 @@ def advance(count: int) -> None: advance(2) continue - # A run of `#` only opens a string when a quote follows it; `#if` and - # `#Preview` are not string literals. + # A run of `#` opens a string when a quote follows it and an extended + # regex literal when a slash does; `#if` and `#Preview` are neither. run = 0 while index + run < source_length and source[index + run] == "#": run += 1 - quote_at = index + run - if quote_at < source_length and source[quote_at] == '"': + opener_at = index + run + + if opener_at < source_length and source[opener_at] == '"': in_string = True hashes = run - multiline = source.startswith('"""', quote_at) + multiline = source.startswith('"""', opener_at) advance(run + (3 if multiline else 1)) continue + if run and opener_at < source_length and source[opener_at] == "/": + # Extended regex literal, `#/ ... /#` at any hash count, possibly + # spanning lines. Its contents are pattern text, so a `//` in there + # is not a comment — and in extended syntax whitespace is ignored, + # which makes `#/ // NETRELISH-ALLOW-ENDPOINT: adr-0007 /#` read + # exactly like an approval to the naive scanner. Skip to the + # matching delimiter; unterminated swallows the rest, which fails + # closed. + # + # The BARE `/.../` form is not tracked, and does not need to be: two + # unescaped slashes in a row would end the literal at the first one, + # so the sequence `//` cannot occur inside one. + closer = "/" + "#" * run + end = source.find(closer, opener_at + 1) + end = source_length if end == -1 else end + len(closer) + advance(end - index) + continue + advance(1) return comments @@ -261,12 +280,22 @@ def __init__(self, path, line, start_column, end_column, message): def identity(self): """What makes two findings the same call rather than two calls. - The same result can appear in more than one SARIF file. Columns - distinguish two genuinely different calls that share a line, so they - belong in the key — otherwise deduplication would quietly merge the very - case the shared-line rule exists to catch. + The same result can appear in more than one SARIF file, so findings are + deduplicated — but deduplicating too eagerly would quietly merge the + very case the shared-line rule exists to catch. + + Columns settle it when they are present: a position is a call site, and + the message is only description, so two reports of one position are one + finding however they are worded. + + When columns are absent there is nothing left to tell two calls on one + line apart, so the message stays in the key. That can only split one + finding into two, which blocks; merging two into one would approve. Of + the two ways to be wrong, this is the one that fails closed. """ - return (self.path, self.line, self.start_column, self.end_column, self.message) + if self.start_column is not None and self.end_column is not None: + return (self.path, self.line, self.start_column, self.end_column) + return (self.path, self.line, None, None, self.message) class Report: @@ -326,6 +355,18 @@ def evaluate(results_dir, repo_root=None) -> Report: source_cache: dict[str, dict[int, str] | None] = {} + # A trailing marker approves the call it sits beside, and nothing else. The + # line above is offered as a second home for it only when that line is not + # itself flagged — otherwise + # + # _ = try await URLSession.shared.data(from: a) // ...ALLOW...: adr-0007 + # _ = try await URLSession.shared.data(from: b) + # + # would let one exemption cover both calls: the shared-line bypass wearing a + # newline. The standalone-comment-above form is unaffected, because a line + # holding only a comment is never a finding. + flagged = set(lines) + for (path, line), group in sorted(lines.items()): if path not in source_cache: candidate = repo_root / path @@ -347,6 +388,8 @@ def evaluate(results_dir, repo_root=None) -> Report: for candidate_line in (line - 1, line): if adr: break + if candidate_line != line and (path, candidate_line) in flagged: + continue text = comments.get(candidate_line) if text: adr = cited_adr(text, repo_root, report) diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index c96147b..e6085a6 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -141,6 +141,29 @@ def test_line_after_multiline_string_is_code_again(self): source = 'let s = """\nbody\n"""\n// ' + MARKER self.assertIn(MARKER, self.comment_on(source, 4)) + def test_extended_regex_literal_is_not_a_comment(self): + # Extended regex ignores whitespace, so the marker inside one reads + # exactly like an approval to a scanner that does not know the form. + self.assertIsNone(self.comment_on("let r = #/ // " + MARKER + " /#", 1)) + + def test_extended_regex_literal_with_two_hashes(self): + self.assertIsNone(self.comment_on("let r = ##/ // " + MARKER + " /##", 1)) + + def test_slash_hash_inside_double_hash_regex_does_not_close_it(self): + source = "let r = ##/ a /# b // " + MARKER + " /##" + self.assertIsNone(self.comment_on(source, 1)) + + def test_code_after_a_regex_literal_is_code_again(self): + self.assertIn(MARKER, self.comment_on("let r = #/ a /# // " + MARKER, 1)) + + def test_multiline_regex_literal_is_not_a_comment(self): + source = "let r = #/\n// " + MARKER + "\n/#\n" + CALL + self.assertIsNone(self.comment_on(source, 2)) + + def test_unterminated_regex_literal_swallows_the_rest(self): + source = "let r = #/ oops\n// " + MARKER + "\n" + CALL + self.assertEqual(codeql_gate.line_comments(source), {}) + def test_hash_directives_do_not_open_a_string(self): source = "#if DIRECT_BUILD\n#Preview { EmptyView() }\n#endif\n// " + MARKER self.assertIn(MARKER, self.comment_on(source, 4)) @@ -280,6 +303,72 @@ def test_two_findings_on_one_line_fail_closed(self): self.assertEqual(report.approved, []) self.assertIn("share this line", report.blocking[0][1]) + def test_trailing_marker_does_not_approve_the_next_flagged_line(self): + # The shared-line bypass wearing a newline: a marker beside one call + # must not also serve as the comment "above" the call below it. + self.checkout.swift( + "Sources/A.swift", + CALL + " // " + MARKER + "\n" + CALL, + ) + self.checkout.results(sarif( + result("Sources/A.swift", 1, start_column=5, end_column=46), + result("Sources/A.swift", 2, start_column=5, end_column=46), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(len(report.approved), 1) # line 1, beside its marker + self.assertEqual(len(report.blocking), 1) + self.assertEqual(report.blocking[0][0].line, 2) + + def test_standalone_comment_above_still_approves_a_flagged_line(self): + # The guard above must not break the ordinary form, where the line above + # holds only the comment and is never itself a finding. + self.checkout.swift( + "Sources/A.swift", + "// " + MARKER + "\n" + CALL + "\n// " + MARKER + "\n" + CALL, + ) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=5, end_column=46), + result("Sources/A.swift", 4, start_column=5, end_column=46), + )) + self.assertTrue(self.checkout.evaluate().ok) + + def test_regex_literal_marker_does_not_approve(self): + self.checkout.swift( + "Sources/A.swift", + "let r = #/ // " + MARKER + " /#\n" + CALL, + ) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertFalse(self.checkout.evaluate().ok) + + def test_one_position_reworded_is_still_one_finding(self): + # Same call site, different message text: description, not identity. + # Splitting it would invent a shared line and block a valid exemption. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=5, end_column=46, + message="outbound network call"), + ), "a.sarif") + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=5, end_column=46, + message="outbound network call via URLSession"), + ), "b.sarif") + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(len(report.approved), 1) + + def test_columnless_findings_on_one_line_still_fail_closed(self): + # With no columns there is nothing to tell two calls apart, so the + # message stays in the key and the shared line blocks. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, message="call to data(from:)"), + result("Sources/A.swift", 2, message="call to bytes(from:)"), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(len(report.blocking), 2) + def test_the_same_finding_in_two_sarif_files_is_one_finding(self): # Deduplication must not turn a duplicate report into a shared line. self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) From 31c8becaa72edbf464338fd2934d4e5adfe3c592 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:13:27 +0000 Subject: [PATCH 03/12] ci: model string interpolation in the marker scanner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixth hole, same class as the other five, found by review. It fails open. Swift interpolation returns to CODE inside a string literal, that code can open another string, and that one can interpolate again. The scanner held string state in three flat variables, so a nested opening quote was read as the outer string's closer and the scan fell out into "code" mid-literal: let s = "\("// NETRELISH-ALLOW-ENDPOINT: adr-0007")" That recorded a line comment and approved the call below it. Flags cannot express nesting, so the contexts are now a stack. Every construct pushes and its terminator pops: strings and raw strings at any hash count, their multiline forms, extended regex literals, and interpolations — which also count their own parentheses, so `\(f(g(x)))` ends at the right one rather than the first. `\(` is tested before the generic escape, since it is a prefix of it. A `//` is recorded only when the stack is empty. One inside an interpolation would comment out the closing paren and quote, so it cannot appear in code that compiles, and code that does not compile is code CodeQL never flagged. Skipping it there costs nothing and cannot approve anything. Newline resync now pops only the directly-enclosing single-line string. A newline inside an interpolation is legal within a multiline literal, and guessing wrong would resync out of a construct that is genuinely open; not resyncing merely swallows more of the file, which approves nothing. Nine regression tests: nested, raw-string (`\#(`), doubly nested and multiline interpolations; a comment genuinely following one; nested parens; `\\(` not opening one; a string opened inside an interpolation still hiding a marker; and the whole thing end to end through the gate. 64 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- scripts/codeql_gate.py | 99 +++++++++++++++++++++++-------- scripts/tests/test_codeql_gate.py | 43 ++++++++++++++ 2 files changed, 117 insertions(+), 25 deletions(-) diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 74bb9ac..aa14180 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -87,29 +87,44 @@ def line_comments(source: str) -> dict[int, str]: the dangerous one: a commented-out block that happens to contain a marker silently approves whatever line follows it. - So this scans the file once, front to back, tracking the three lexical - contexts that can hide or reveal a `//`: block comments (which NEST in - Swift), string literals, and raw string literals (`#"..."#`, any number of - hashes, where the escape is `\\#`). Multiline `\"\"\"..\"\"\"` too, since a - marker inside one spans lines and would otherwise look like a comment. + So this scans the file once, front to back, tracking every lexical context + that can hide or reveal a `//`: block comments (which NEST in Swift), string + literals, raw strings (`#"..."#` at any hash count, where the escape is + `\\#`), extended regex literals (`#/.../#`), the multiline forms of each, + and string interpolation. - Unterminated constructs fail closed for free: an unclosed `/*` swallows the - rest of the file, so no further line records a comment, so nothing below it - can be approved. + Interpolation is why the contexts are a STACK rather than a few flags. + `\\(` returns to code INSIDE a string, that code can open another string, + and that one can interpolate again. Flat state mistook the nested opening + quote for the outer closer and fell out into "code" mid-literal, so + + let s = "\\("// NETRELISH-ALLOW-ENDPOINT: adr-0007")" + + recorded a line comment and approved the call below it. Each construct now + pushes; its terminator pops; a `//` is only recorded as a comment when the + stack is empty, which is the one place a genuine comment can be. + + Unterminated constructs fail closed for free: an unclosed `/*`, string or + `#/` swallows the rest of the file, so no further line records a comment, + so nothing below it can be approved. This is a comment/string scanner, not a Swift parser, and that is the correct scope — it needs to answer one question, and it answers it for every - way Swift can spell these three things. + way Swift can spell these constructs. """ comments: dict[int, str] = {} source_length = len(source) index = 0 line = 1 - depth = 0 # block-comment nesting depth; 0 means we are not in one - in_string = False - multiline = False # the \"\"\" form, which may contain newlines - hashes = 0 # raw-string delimiter count: #"..."# is 1, ##"..."## is 2 + depth = 0 # block-comment nesting depth; 0 means we are not in one + + # Innermost context last. Either ["str", hashes, multiline] or + # ["interp", paren_depth] — the latter is code again, inside a literal. + stack: list[list] = [] + + def in_string() -> bool: + return bool(stack) and stack[-1][0] == "str" def advance(count: int) -> None: """Step `count` characters, keeping the line number honest.""" @@ -124,8 +139,14 @@ def advance(count: int) -> None: # A single-line string cannot span a newline. Reaching one means the # source is malformed; resync at the line break rather than treating # the remainder of the file as string content. - if in_string and not multiline: - in_string = False + # + # Only the directly-enclosing string is resynced. A newline inside + # an interpolation is legal within a multiline literal, and guessing + # which one this is would risk resyncing out of a construct that is + # genuinely open. Not resyncing only swallows more of the file, + # which approves nothing. + if in_string() and not stack[-1][2]: + stack.pop() advance(1) continue @@ -140,24 +161,53 @@ def advance(count: int) -> None: advance(1) continue - if in_string: + if in_string(): + _, hashes, multiline = stack[-1] + escape = "\\" + "#" * hashes closer = '"""' if multiline else '"' - if source.startswith("\\" + "#" * hashes, index): - advance(1 + hashes + 1) # backslash, its hashes, the escaped character + + # Interpolation first: `\(` is also a prefix of the generic escape, + # and treating it as one would skip the paren and read the rest of + # the interpolation as string content. + if source.startswith(escape + "(", index): + stack.append(["interp", 0]) + advance(len(escape) + 1) + elif source.startswith(escape, index): + advance(len(escape) + 1) # the escape, then the character it escapes elif source.startswith(closer + "#" * hashes, index): - in_string = False + stack.pop() advance(len(closer) + hashes) else: advance(1) continue - # Ordinary code. + # Code — either the top level, or inside an interpolation. + if stack: # ["interp", paren_depth] + if char == "(": + stack[-1][1] += 1 + advance(1) + continue + if char == ")": + if stack[-1][1]: + stack[-1][1] -= 1 + else: + stack.pop() # interpolation over; the string resumes + advance(1) + continue + if source.startswith("//", index): end = source.find("\n", index) end = source_length if end == -1 else end # First genuine `//` wins: everything after it on the line is inside # that same comment. - comments.setdefault(line, source[index + 2:end]) + # + # Recorded only at the top level. A `//` inside an interpolation + # would comment out the closing paren and quote, so it cannot occur + # in code that compiles — and code that does not compile is code + # CodeQL never flagged. Skipping it here costs nothing and cannot + # approve anything. + if not stack: + comments.setdefault(line, source[index + 2:end]) advance(end - index) continue @@ -174,10 +224,9 @@ def advance(count: int) -> None: opener_at = index + run if opener_at < source_length and source[opener_at] == '"': - in_string = True - hashes = run - multiline = source.startswith('"""', opener_at) - advance(run + (3 if multiline else 1)) + is_multiline = source.startswith('"""', opener_at) + stack.append(["str", run, is_multiline]) + advance(run + (3 if is_multiline else 1)) continue if run and opener_at < source_length and source[opener_at] == "/": diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index e6085a6..fe02d6a 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -141,6 +141,41 @@ def test_line_after_multiline_string_is_code_again(self): source = 'let s = """\nbody\n"""\n// ' + MARKER self.assertIn(MARKER, self.comment_on(source, 4)) + def test_marker_in_an_interpolated_string_is_not_a_comment(self): + # Flat state mistook the nested opening quote for the outer closer and + # fell out into "code" mid-literal, recording the marker as a comment. + source = 'let s = "\\("// ' + MARKER + '")"\n' + CALL + self.assertIsNone(self.comment_on(source, 1)) + + def test_marker_in_a_raw_string_interpolation_is_not_a_comment(self): + source = 'let s = #"\\#("// ' + MARKER + '")"#' + self.assertIsNone(self.comment_on(source, 1)) + + def test_marker_in_doubly_nested_interpolation_is_not_a_comment(self): + source = 'let s = "\\("\\("// ' + MARKER + '")")"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_marker_in_a_multiline_string_interpolation_is_not_a_comment(self): + source = 'let s = """\n\\("// ' + MARKER + '")\n"""' + self.assertIsNone(self.comment_on(source, 2)) + + def test_comment_after_an_interpolation_is_a_comment(self): + source = 'let s = "\\(a)" // ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_nested_parentheses_inside_an_interpolation(self): + # The interpolation ends at its own matching paren, not the first one. + source = 'let s = "\\(f(g(x)))" // ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_escaped_backslash_does_not_open_an_interpolation(self): + source = 'let s = "\\\\(a)" // ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_string_opened_inside_an_interpolation_still_hides_a_marker(self): + source = 'let s = "\\(x)" + "// ' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + def test_extended_regex_literal_is_not_a_comment(self): # Extended regex ignores whitespace, so the marker inside one reads # exactly like an approval to a scanner that does not know the form. @@ -333,6 +368,14 @@ def test_standalone_comment_above_still_approves_a_flagged_line(self): )) self.assertTrue(self.checkout.evaluate().ok) + def test_interpolated_string_marker_does_not_approve(self): + self.checkout.swift( + "Sources/A.swift", + 'let s = "\\("// ' + MARKER + '")"\n' + CALL, + ) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertFalse(self.checkout.evaluate().ok) + def test_regex_literal_marker_does_not_approve(self): self.checkout.swift( "Sources/A.swift", From d0a9808b3b52f76e0db56b93166c6c8fcda3d161 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:27:27 +0000 Subject: [PATCH 04/12] ci: three more fail-open paths in the gate, one of them mine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three from review, all high, all approving a call nobody approved. 1. `docs/adr/NNNN-*.md` was globbed without checking what matched. glob returns directories, so a directory named `0007-placeholder.md/` satisfied a citation with no written decision anywhere inside it — an approval backed by nothing. Matches are now filtered with is_file(). 2. A missing SARIF `startLine` defaulted to 1. A finding whose location is unknown was therefore handed line 1, and a marker that happened to sit there approved it. An unlocatable finding cannot be exempted at all: it is now a gate error, which blocks, and no location is guessed. Non-integer and non-positive values are rejected the same way. 3. The columnless deduplication key included the message, and I defended that on the review thread with an argument that does not hold. Two separate calls on one line almost always carry the IDENTICAL message, so the key merged them into one finding and a single marker approved both — the shared-line bypass coming back through the door I had just closed, in the commit that closed it. Findings with columns are still deduplicated by position, because a position is a call site and the message is only description. Findings without columns are now never deduplicated, since nothing about them can tell one call from another. That can only over-count, which blocks. Five regression tests: a directory matching an ADR citation, and a real ADR beside one; a missing startLine and an invalid one; and two columnless findings sharing a line with the same message. Verified the directory case is not vacuous — glob does return it. 69 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- scripts/codeql_gate.py | 63 ++++++++++++++++++++++--------- scripts/tests/test_codeql_gate.py | 50 ++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index aa14180..7a6fcfc 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -270,7 +270,13 @@ def cited_adr(comment: str, repo_root: pathlib.Path, report: "Report") -> str | citation = found.group(1) number = citation[len("adr-"):] - matches = sorted(p.name for p in (repo_root / "docs" / "adr").glob(number + "-*.md")) + # `is_file()` because glob also returns directories, and a directory named + # `0007-placeholder.md/` would otherwise satisfy the citation with no + # written decision anywhere in it — an approval backed by nothing. + matches = sorted( + p.name for p in (repo_root / "docs" / "adr").glob(number + "-*.md") + if p.is_file() + ) if not matches: report.errors.append( @@ -326,25 +332,26 @@ def __init__(self, path, line, start_column, end_column, message): self.end_column = end_column self.message = message + @property + def locatable(self) -> bool: + """Whether this finding can be told apart from another on its line.""" + return self.start_column is not None and self.end_column is not None + def identity(self): """What makes two findings the same call rather than two calls. - The same result can appear in more than one SARIF file, so findings are - deduplicated — but deduplicating too eagerly would quietly merge the - very case the shared-line rule exists to catch. - - Columns settle it when they are present: a position is a call site, and + Only ever asked of a locatable finding. A position is a call site, and the message is only description, so two reports of one position are one finding however they are worded. - When columns are absent there is nothing left to tell two calls on one - line apart, so the message stays in the key. That can only split one - finding into two, which blocks; merging two into one would approve. Of - the two ways to be wrong, this is the one that fails closed. + Findings WITHOUT columns are never deduplicated at all, because nothing + about them can distinguish one call from another — two separate calls on + one line usually carry the identical message, so any key built from the + message would merge them, and a single marker would then approve both. + That is the shared-line bypass returning through the back door. Keeping + every occurrence instead can only over-count, which blocks. """ - if self.start_column is not None and self.end_column is not None: - return (self.path, self.line, self.start_column, self.end_column) - return (self.path, self.line, None, None, self.message) + return (self.path, self.line, self.start_column, self.end_column) class Report: @@ -374,7 +381,9 @@ def evaluate(results_dir, repo_root=None) -> Report: ) return report - findings = {} + findings = {} # locatable, deduplicated by position + loose = [] # columnless, never deduplicated (see Finding.identity) + for path in paths: with open(path) as handle: sarif = json.load(handle) @@ -384,14 +393,32 @@ def evaluate(results_dir, repo_root=None) -> Report: continue where = result["locations"][0]["physicalLocation"] region = where.get("region", {}) + where_file = str(source_path(where["artifactLocation"]["uri"])) + + # Defaulting a missing startLine to 1 was not fail-closed: a + # finding whose location is unknown would be handed line 1, and + # a marker that happened to sit there would approve it. An + # unlocatable finding cannot be exempted at all. + start_line = region.get("startLine") + if isinstance(start_line, bool) or not isinstance(start_line, int) \ + or start_line < 1: + report.errors.append( + "a gate finding in " + where_file + " has no usable " + "startLine, so it cannot be located or exempted" + ) + continue + finding = Finding( - path=str(source_path(where["artifactLocation"]["uri"])), - line=region.get("startLine", 1), + path=where_file, + line=start_line, start_column=region.get("startColumn"), end_column=region.get("endColumn"), message=result["message"]["text"], ) - findings[finding.identity()] = finding + if finding.locatable: + findings[finding.identity()] = finding + else: + loose.append(finding) # Group by the unit the exemption mechanism can actually address. A marker # names a line, not a call, so a line carrying two gate findings cannot be @@ -399,7 +426,7 @@ def evaluate(results_dir, repo_root=None) -> Report: # already-approved one got past the gate. Fail closed instead of inventing # an approval the author never wrote. lines: dict[tuple, list[Finding]] = {} - for finding in findings.values(): + for finding in list(findings.values()) + loose: lines.setdefault((finding.path, finding.line), []).append(finding) source_cache: dict[str, dict[int, str] | None] = {} diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index fe02d6a..5b01c01 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -245,6 +245,21 @@ def test_ambiguous_adr_number_is_refused(self): self.assertIn("0006-one.md", report.errors[0]) self.assertIn("0006-two.md", report.errors[0]) + def test_a_directory_named_like_an_adr_is_refused(self): + # glob() returns directories too, so `0007-placeholder.md/` would + # satisfy the citation with no written decision inside it. + (self.root / "docs" / "adr" / "0007-placeholder.md").mkdir() + adr, report = self.cite(" " + MARKER) + self.assertIsNone(adr) + self.assertIn("no docs/adr/0007", report.errors[0]) + + def test_a_real_adr_beside_a_directory_of_the_same_number(self): + (self.root / "docs" / "adr" / "0007-placeholder.md").mkdir() + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + adr, report = self.cite(" " + MARKER) + self.assertEqual(adr, "adr-0007") # the directory is not a clash + self.assertEqual(report.errors, []) + def test_comment_without_a_marker_is_not_an_error(self): adr, report = self.cite(" just an ordinary comment") self.assertIsNone(adr) @@ -412,6 +427,41 @@ def test_columnless_findings_on_one_line_still_fail_closed(self): self.assertFalse(report.ok) self.assertEqual(len(report.blocking), 2) + def test_columnless_findings_with_the_same_message_fail_closed(self): + # The common case, and the one a message-based key got wrong: two + # separate calls on one line usually carry identical text, so keying on + # the message merged them and one marker approved both. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, message="outbound network call"), + result("Sources/A.swift", 2, message="outbound network call"), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(len(report.blocking), 2) + self.assertEqual(report.approved, []) + + def test_a_finding_without_a_start_line_cannot_be_approved(self): + # Defaulting to line 1 let a marker on line 1 approve a finding whose + # location is unknown. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + hit = result("Sources/A.swift", 1) + del hit["locations"][0]["physicalLocation"]["region"]["startLine"] + self.checkout.results(sarif(hit)) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("no usable startLine" in e for e in report.errors)) + + def test_a_finding_with_an_invalid_start_line_cannot_be_approved(self): + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + hit = result("Sources/A.swift", 1) + hit["locations"][0]["physicalLocation"]["region"]["startLine"] = 0 + self.checkout.results(sarif(hit)) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertTrue(any("no usable startLine" in e for e in report.errors)) + def test_the_same_finding_in_two_sarif_files_is_one_finding(self): # Deduplication must not turn a duplicate report into a shared line. self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) From 00f20a0b3f4c5ac6805fd597b86d005c88fe2159 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:33:31 +0000 Subject: [PATCH 05/12] ci: the gate trusted files that were never in the checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more from review, both high, both reproduced here before fixing. Both approve a call using evidence that does not exist in this repository. 1. A symlink satisfied the ADR requirement. `is_file()` follows links, so `docs/adr/0007-anything.md -> /etc/hosts` was accepted as the written decision — reproduced, and the citation resolved cleanly. Matches must now be regular files that actually live here: symlinks are skipped alongside the directories the previous commit excluded. A symlink beside a real ADR of the same number is ignored rather than read as an ambiguity. 2. SARIF named the file whose comments decide approval, and nothing constrained it. An absolute `file:` URI, or a relative one containing `..`, escaped repo_root when joined and was read anyway, so a location pointing at any file on the runner carrying a valid marker was approved. Reproduced with a SARIF pointing at /tmp: the gate printed "approved endpoint (adr-0007)" and exited 0. Source paths are now resolved and required to stay inside the checkout. Resolution follows symlinks, so a tracked source file pointing outside is refused for the same reason. A path that does not stay inside is an exemption that could not be checked, which blocks. Five regression tests: a symlinked ADR alone and beside a real one; a SARIF location outside the checkout by absolute URI, by `..` traversal, and by a tracked symlink. 74 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- scripts/codeql_gate.py | 45 ++++++++++++++++++++---- scripts/tests/test_codeql_gate.py | 58 +++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 7a6fcfc..42a635e 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -270,12 +270,16 @@ def cited_adr(comment: str, repo_root: pathlib.Path, report: "Report") -> str | citation = found.group(1) number = citation[len("adr-"):] - # `is_file()` because glob also returns directories, and a directory named - # `0007-placeholder.md/` would otherwise satisfy the citation with no - # written decision anywhere in it — an approval backed by nothing. + # A match counts only if it is a regular file that actually lives here. + # + # `glob` also returns directories, so `0007-placeholder.md/` would satisfy + # the citation with no written decision inside it. And `is_file()` follows + # symlinks, so `0007-anything.md -> /etc/hosts` would satisfy it with a + # decision that is not in the repository at all. Both are approvals backed + # by nothing, which is the failure this ADR requirement exists to prevent. matches = sorted( p.name for p in (repo_root / "docs" / "adr").glob(number + "-*.md") - if p.is_file() + if not p.is_symlink() and p.is_file() ) if not matches: @@ -320,6 +324,29 @@ def source_path(uri: str) -> pathlib.Path: return path +def inside_checkout(repo_root: pathlib.Path, path: str) -> pathlib.Path | None: + """The source file, but only if it really is one of ours. + + SARIF is input, and this one names the file whose comments decide whether a + network call is approved. Nothing constrained it: an absolute `file:` URI, + or a relative one containing `..`, escaped `repo_root` when joined and was + read anyway — so a location pointing at any file on the runner that happens + to contain a valid marker was approved. + + Resolution also follows symlinks, so a tracked source file pointing outside + the checkout is rejected for the same reason. + + Returns None when the path does not stay inside, which the caller treats as + an exemption it could not check, and therefore blocks. + """ + try: + resolved = (repo_root / path).resolve() + resolved.relative_to(repo_root.resolve()) + except (ValueError, OSError, RuntimeError): + return None + return resolved + + class Finding: """One gate result, reduced to the parts that decide its fate.""" @@ -445,8 +472,14 @@ def evaluate(results_dir, repo_root=None) -> Report: for (path, line), group in sorted(lines.items()): if path not in source_cache: - candidate = repo_root / path - if candidate.is_file(): + candidate = inside_checkout(repo_root, path) + if candidate is None: + source_cache[path] = None + report.errors.append( + path + " is outside the checkout, so an exemption there " + "could not be trusted" + ) + elif candidate.is_file(): source_cache[path] = line_comments( candidate.read_text(errors="replace") ) diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index 5b01c01..1d8ef56 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -260,6 +260,25 @@ def test_a_real_adr_beside_a_directory_of_the_same_number(self): self.assertEqual(adr, "adr-0007") # the directory is not a clash self.assertEqual(report.errors, []) + def test_a_symlinked_adr_is_refused(self): + # is_file() follows symlinks, so this would otherwise accept a decision + # that is not in the repository at all. + outside = self.root / "elsewhere.md" + outside.write_text("# not an ADR\n") + (self.root / "docs" / "adr" / "0007-fake.md").symlink_to(outside) + adr, report = self.cite(" " + MARKER) + self.assertIsNone(adr) + self.assertIn("no docs/adr/0007", report.errors[0]) + + def test_a_symlink_beside_a_real_adr_is_not_a_clash(self): + outside = self.root / "elsewhere.md" + outside.write_text("# not an ADR\n") + (self.root / "docs" / "adr" / "0007-fake.md").symlink_to(outside) + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + adr, report = self.cite(" " + MARKER) + self.assertEqual(adr, "adr-0007") + self.assertEqual(report.errors, []) + def test_comment_without_a_marker_is_not_an_error(self): adr, report = self.cite(" just an ordinary comment") self.assertIsNone(adr) @@ -491,6 +510,45 @@ def test_unreadable_source_blocks(self): self.assertFalse(report.ok) self.assertTrue(any("cannot read" in e for e in report.errors)) + def test_a_source_outside_the_checkout_is_not_trusted(self): + # A SARIF location naming any file on the runner that happens to carry + # a valid marker used to be read, and approved. + import tempfile + with tempfile.TemporaryDirectory() as elsewhere: + planted = pathlib.Path(elsewhere) / "Evil.swift" + planted.write_text("// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result(planted.as_uri(), 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("outside the checkout" in e for e in report.errors)) + + def test_a_traversing_relative_uri_is_not_trusted(self): + import tempfile + with tempfile.TemporaryDirectory() as elsewhere: + planted = pathlib.Path(elsewhere) / "Evil.swift" + planted.write_text("// " + MARKER + "\n" + CALL) + hops = pathlib.Path(*([".."] * 12)) / planted.relative_to("/") + self.checkout.results(sarif(result(str(hops), 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("outside the checkout" in e for e in report.errors)) + + def test_a_symlinked_source_pointing_outside_is_not_trusted(self): + import tempfile + with tempfile.TemporaryDirectory() as elsewhere: + planted = pathlib.Path(elsewhere) / "Evil.swift" + planted.write_text("// " + MARKER + "\n" + CALL) + link = self.checkout.root / "Sources" / "A.swift" + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to(planted) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("outside the checkout" in e for e in report.errors)) + def test_file_uri_resolves(self): path = self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) self.checkout.results(sarif(result(path.as_uri(), 2))) From f54f4112a50b57deb43341ae814de69fa9e1acd4 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:41:43 +0000 Subject: [PATCH 06/12] ci: a regex's own delimiter, a symlinked parent, and two spellings of one path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round five. Three more fail-open holes, all reproduced before fixing, plus the two stale counts in my own documentation. 1. The bare `/.../` regex form. I wrote that it needed no handling because "two unescaped slashes would end the literal at the first, so the sequence `//` cannot occur inside one" — true, and beside the point. The literal's own ending supplies the second slash: in `let r = /\//` the escaped slash and the closing delimiter are textually `//`, and everything after was read as a comment. `let r = /\//; let s = "NETRELISH-ALLOW-ENDPOINT: adr-0007"` was an exemption. An escape in code is now opaque — skip it and whatever it escapes — which leaves the closing delimiter with nothing to pair with. Regex and division never have to be told apart, which is not something this scanner should attempt. 2. The extended-regex terminator used source.find("/#"), which matched the `/` of an escaped `\/` and ended the literal early, leaving the rest of the pattern to be read as code. The search is now escape-aware. 3. ADR citations checked the matched file but not its PARENT, so `docs/adr -> /tmp/decisions` returned perfectly ordinary regular files from outside the checkout. Resolution now goes through inside_checkout, which resolves the whole path instead of inspecting its last component. 4. SARIF picks the path spelling, and the raw string was the grouping key. So `Sources/A.swift` and `Sources/../Sources/A.swift` put two findings on one physical line into two groups of one, and a single marker approved both — the shared-line rule defeated by punctuation. Locations are canonicalised to checkout-relative paths as they are read, before anything groups on them. Also corrects the enforcement history in this module's header and in codeql.yml, both of which still said five. It is seventeen: five before the module was extracted, twelve found by review of the branch that extracted it, every one failing open, one of them introduced by the fix for another. The header now lists them, because the ratio is the point — reading this code carefully has never once been sufficient. Six regression tests: a bare regex's closing delimiter, a real comment after one, an escaped delimiter inside an extended regex, an ADR behind a symlinked parent, two spellings of one path sharing a line, and a traversing URI that lands back inside being canonicalised. 80 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 15 +++-- scripts/codeql_gate.py | 101 +++++++++++++++++++++++------- scripts/tests/test_codeql_gate.py | 51 +++++++++++++++ 3 files changed, 140 insertions(+), 27 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index bfc20dd..2672d4d 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -159,12 +159,15 @@ jobs: # ruleset), but that is a console toggle a fork, a clone or a forgetful # afternoon loses. This travels with the repo. # - # The logic is in scripts/codeql_gate.py, not inline here. It has had five - # defects, every one in its enforcement rather than its design, and a - # heredoc cannot be tested — so each was caught by a human reading it, or - # not caught at all. It now has a suite in scripts/tests/ that the - # `CodeQL gate self-test` job in ci.yml runs on every push, including - # while THIS job cannot build the project. + # The logic is in scripts/codeql_gate.py, not inline here. It has had + # seventeen defects — five before it was extracted, and twelve more found + # by review of the branch that extracted it — every one in its + # enforcement rather than its design, and every one failing OPEN. A + # heredoc cannot be tested, so the first five were caught by a human + # reading it, or not caught at all. It now has a suite in scripts/tests/ + # that the `CodeQL gate self-test` job in ci.yml runs on every push, + # including while THIS job cannot build the project. The module header + # lists all seventeen; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 42a635e..ddffb2a 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -25,9 +25,25 @@ Each idea was right; each implementation had a hole. A heredoc cannot be tested, so every one of those was caught by a human reading it, or not caught at all. -The mechanism is now a module with a test suite beside it in scripts/tests/, and -that suite runs on every push — including while CodeQL itself cannot build this -project. See scripts/tests/test_codeql_gate.py. + +That list is the HISTORICAL set — the five that existed before this file did. +Review of the branch that extracted it found TWELVE more, every one of them +failing open: markers hidden in extended regex literals, in interpolated nested +strings, and behind a bare regex's own closing delimiter; a trailing marker +exempting the line below it; ADR citations satisfied by a directory, by a +symlink, and by a symlinked parent directory; a missing SARIF start line +defaulting to 1; columnless findings merging on their message; source paths +escaping the checkout entirely; and two spellings of one path splitting a +shared line into two. One of the twelve was introduced by the fix for another, +and defended in review before it was checked. + +Seventeen defects, not one of them in the design. That is the case for the +suite in scripts/tests/, which runs on every push — including while CodeQL +itself cannot build this project. See scripts/tests/test_codeql_gate.py. + +The lesson is in the ratio rather than the number: reading this code carefully +has never once been sufficient. Anything added here needs a test that fails +without it. FAIL CLOSED, ALWAYS ------------------- @@ -238,15 +254,38 @@ def advance(count: int) -> None: # matching delimiter; unterminated swallows the rest, which fails # closed. # - # The BARE `/.../` form is not tracked, and does not need to be: two - # unescaped slashes in a row would end the literal at the first one, - # so the sequence `//` cannot occur inside one. + # Escape-aware, because `source.find("/#")` matched the `/` of an + # escaped `\/` and ended the literal early — leaving the rest of the + # pattern to be read as code, where a `//` in it became a comment. closer = "/" + "#" * run - end = source.find(closer, opener_at + 1) - end = source_length if end == -1 else end + len(closer) + scan, end = opener_at + 1, source_length + while scan < source_length: + if source[scan] == "\\": + scan += 2 + continue + if source.startswith(closer, scan): + end = scan + len(closer) + break + scan += 1 advance(end - index) continue + if char == "\\": + # An escape in code is opaque: skip it and whatever it escapes. + # + # This is what makes the BARE `/.../` regex form safe to leave + # untracked. I claimed two unescaped slashes would end such a + # literal at the first, so `//` could not occur inside one. True, + # and beside the point: the literal's own ending supplies the + # second slash. In `let r = /\//` the `\/` and the closing `/` are + # textually `//`, and the rest of the line was read as a comment. + # + # Skipping `\/` as one token leaves the closing `/` alone, with + # nothing after it to pair with. Disambiguating regex from division + # is not needed, and is not something this scanner should attempt. + advance(2) + continue + advance(1) return comments @@ -273,13 +312,20 @@ def cited_adr(comment: str, repo_root: pathlib.Path, report: "Report") -> str | # A match counts only if it is a regular file that actually lives here. # # `glob` also returns directories, so `0007-placeholder.md/` would satisfy - # the citation with no written decision inside it. And `is_file()` follows + # the citation with no written decision inside it. `is_file()` follows # symlinks, so `0007-anything.md -> /etc/hosts` would satisfy it with a - # decision that is not in the repository at all. Both are approvals backed - # by nothing, which is the failure this ADR requirement exists to prevent. + # decision that is not in the repository at all. And checking only the + # matched file left its PARENT unchecked, so `docs/adr -> /tmp/decisions` + # handed back perfectly ordinary regular files from outside the checkout. + # + # Hence `inside_checkout`, which resolves the whole path rather than + # inspecting its last component: every one of those is an approval backed + # by something this repository does not contain, which is the failure the + # ADR requirement exists to prevent. matches = sorted( p.name for p in (repo_root / "docs" / "adr").glob(number + "-*.md") if not p.is_symlink() and p.is_file() + and inside_checkout(repo_root, p) is not None ) if not matches: @@ -324,7 +370,7 @@ def source_path(uri: str) -> pathlib.Path: return path -def inside_checkout(repo_root: pathlib.Path, path: str) -> pathlib.Path | None: +def inside_checkout(repo_root: pathlib.Path, path) -> pathlib.Path | None: """The source file, but only if it really is one of ours. SARIF is input, and this one names the file whose comments decide whether a @@ -396,6 +442,7 @@ def ok(self) -> bool: def evaluate(results_dir, repo_root=None) -> Report: """Decide the build. Pure: it reads, it does not print or exit.""" repo_root = pathlib.Path(repo_root or pathlib.Path.cwd()) + root = repo_root.resolve() report = Report() paths = sorted(glob.glob(str(pathlib.Path(results_dir) / "*.sarif"))) @@ -420,7 +467,23 @@ def evaluate(results_dir, repo_root=None) -> Report: continue where = result["locations"][0]["physicalLocation"] region = where.get("region", {}) - where_file = str(source_path(where["artifactLocation"]["uri"])) + named = source_path(where["artifactLocation"]["uri"]) + + # Canonicalise here, before this path becomes a grouping key. + # + # SARIF chooses the spelling, and `Sources/A.swift` and + # `Sources/../Sources/A.swift` are the same file under two of + # them. Grouping on the raw string put two findings on one + # physical line into two groups of one, and a single marker + # approved both — the shared-line rule defeated by punctuation. + located = inside_checkout(repo_root, named) + if located is None: + report.errors.append( + str(named) + " is outside the checkout, so an exemption " + "there could not be trusted" + ) + continue + where_file = str(located.relative_to(root)) # Defaulting a missing startLine to 1 was not fail-closed: a # finding whose location is unknown would be handed line 1, and @@ -472,14 +535,10 @@ def evaluate(results_dir, repo_root=None) -> Report: for (path, line), group in sorted(lines.items()): if path not in source_cache: - candidate = inside_checkout(repo_root, path) - if candidate is None: - source_cache[path] = None - report.errors.append( - path + " is outside the checkout, so an exemption there " - "could not be trusted" - ) - elif candidate.is_file(): + # Already canonical and already known to be inside the checkout — + # both were settled when the finding was read. + candidate = root / path + if candidate.is_file(): source_cache[path] = line_comments( candidate.read_text(errors="replace") ) diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index 1d8ef56..e9791f3 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -176,6 +176,22 @@ def test_string_opened_inside_an_interpolation_still_hides_a_marker(self): source = 'let s = "\\(x)" + "// ' + MARKER + '"' self.assertIsNone(self.comment_on(source, 1)) + def test_bare_regex_closing_delimiter_does_not_start_a_comment(self): + # In `/\//` the escaped slash and the closing delimiter are textually + # `//`, so the rest of the line was read as a comment. + source = r'let r = /\//; let s = "' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_a_real_comment_after_a_bare_regex_still_counts(self): + source = r'let r = /\// // ' + MARKER + self.assertIn(MARKER, self.comment_on(source, 1)) + + def test_escaped_delimiter_does_not_end_an_extended_regex(self): + # `find("/#")` matched the `/` of an escaped `\/` and ended the literal + # early, leaving the rest of the pattern to be read as code. + source = r'let r = #/ a\/# b // ' + MARKER + ' /#' + self.assertIsNone(self.comment_on(source, 1)) + def test_extended_regex_literal_is_not_a_comment(self): # Extended regex ignores whitespace, so the marker inside one reads # exactly like an approval to a scanner that does not know the form. @@ -279,6 +295,19 @@ def test_a_symlink_beside_a_real_adr_is_not_a_clash(self): self.assertEqual(adr, "adr-0007") self.assertEqual(report.errors, []) + def test_an_adr_behind_a_symlinked_parent_is_refused(self): + # Checking only the matched file left its parent unchecked, so + # `docs/adr -> /tmp/decisions` returned ordinary regular files from + # outside the checkout. + import tempfile + with tempfile.TemporaryDirectory() as elsewhere: + (pathlib.Path(elsewhere) / "0007-planted.md").write_text("# not ours\n") + (self.root / "docs" / "adr").rmdir() + (self.root / "docs" / "adr").symlink_to(elsewhere) + adr, report = self.cite(" " + MARKER) + self.assertIsNone(adr) + self.assertIn("no docs/adr/0007", report.errors[0]) + def test_comment_without_a_marker_is_not_an_error(self): adr, report = self.cite(" just an ordinary comment") self.assertIsNone(adr) @@ -510,6 +539,28 @@ def test_unreadable_source_blocks(self): self.assertFalse(report.ok) self.assertTrue(any("cannot read" in e for e in report.errors)) + def test_two_spellings_of_one_path_are_one_line(self): + # SARIF chooses the spelling. Grouping on the raw string put two + # findings on one physical line into two groups of one, and a single + # marker approved both. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=5, end_column=46), + result("Sources/../Sources/A.swift", 2, start_column=60, end_column=101), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertEqual(len(report.blocking), 2) + self.assertIn("share this line", report.blocking[0][1]) + + def test_a_traversing_uri_that_lands_back_inside_is_canonicalised(self): + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/../Sources/A.swift", 2))) + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(report.approved[0][0].path, "Sources/A.swift") + def test_a_source_outside_the_checkout_is_not_trusted(self): # A SARIF location naming any file on the runner that happens to carry # a valid marker used to be read, and approved. From 026d2740260ba512e34844400677042215fbe24e Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:47:58 +0000 Subject: [PATCH 07/12] ci: a citation prefix, the wrong checkout, and columns that were not columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round six. Three more fail-open holes, all reproduced. Together they produced the worst single case yet: two unapproved network calls in one checkout, approved by a marker in a DIFFERENT checkout, exit 0. 1. The citation pattern was not token-bounded. `\d{4}` matched the first four digits of `adr-00070` and captured `adr-0007`, so a malformed citation was accepted whenever ADR 0007 happened to exist. `adr-0007foo` likewise. The number must now end where the citation ends; the documented forms (`adr-0007`, and the same followed by an em dash, comma or full stop) are pinned by a test so the guard cannot quietly reject real markers. 2. source_path rebased absolute paths against Path.cwd(), which is not necessarily the checkout being judged. With --repo-root elsewhere, a `file:////Sources/A.swift` URI was rewritten relative and then looked up under the OTHER root — and where a file existed there too, the gate read it and approved the finding with a marker from a checkout the finding had nothing to do with. Absolute paths now stay absolute; canonicalising against the root actually in use is inside_checkout's job, and it is the only thing that knows which root that is. 3. `locatable` treated any non-None column as a position, so two distinct findings both reported at startColumn 0 shared an identity and collapsed into one, which a single marker approved. Columns are now validated as real 1-based coordinates; a malformed one is no position at all, so the finding counts as columnless and is never deduplicated. The same validation now covers startLine, replacing the open-coded check. Also updates both defect counts: twenty, of which fifteen were found by review of this branch. Nine regression tests, including the cross-checkout case driven with the working directory actually changed, so it exercises the bug rather than a description of it. 85 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 +- scripts/codeql_gate.py | 84 +++++++++++++++++++++---------- scripts/tests/test_codeql_gate.py | 64 +++++++++++++++++++++++ 3 files changed, 124 insertions(+), 28 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 2672d4d..1dc2a86 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # seventeen defects — five before it was extracted, and twelve more found + # twenty defects — five before it was extracted, and fifteen more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all seventeen; read it before adding anything here. + # lists all twenty; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index ddffb2a..62500d0 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,19 +27,22 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found TWELVE more, every one of them +Review of the branch that extracted it found FIFTEEN more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested -strings, and behind a bare regex's own closing delimiter; a trailing marker -exempting the line below it; ADR citations satisfied by a directory, by a -symlink, and by a symlinked parent directory; a missing SARIF start line -defaulting to 1; columnless findings merging on their message; source paths -escaping the checkout entirely; and two spellings of one path splitting a -shared line into two. One of the twelve was introduced by the fix for another, -and defended in review before it was checked. - -Seventeen defects, not one of them in the design. That is the case for the -suite in scripts/tests/, which runs on every push — including while CodeQL -itself cannot build this project. See scripts/tests/test_codeql_gate.py. +strings, and behind a bare regex's own closing delimiter; an escaped delimiter +ending an extended regex early; a trailing marker exempting the line below it; +ADR citations satisfied by a directory, by a symlink, by a symlinked parent +directory, and by a longer number that merely started with a real one; a +missing SARIF start line defaulting to 1; columnless findings merging on their +message; malformed columns merging two findings into one; source paths escaping +the checkout entirely; absolute paths rebased against the working directory +rather than the checkout being judged; and two spellings of one path splitting +a shared line into two. One of the fifteen was introduced by the fix for +another, and defended in review before it was checked. + +Twenty defects, not one of them in the design. That is the case for the suite +in scripts/tests/, which runs on every push — including while CodeQL itself +cannot build this project. See scripts/tests/test_codeql_gate.py. The lesson is in the ratio rather than the number: reading this code carefully has never once been sufficient. Anything added here needs a test that fails @@ -82,7 +85,12 @@ # # Dismissing the alert in the Security tab deliberately does NOT satisfy this: # that is invisible from the repository. -MARKER = re.compile(r"NETRELISH-ALLOW-ENDPOINT:\s*(adr-\d{4})") +# +# The trailing guard matters: `\d{4}` alone matched the first four digits of +# `adr-00070` and captured `adr-0007`, so a malformed citation was silently +# accepted whenever ADR 0007 happened to exist. The number must end where the +# citation ends. +MARKER = re.compile(r"NETRELISH-ALLOW-ENDPOINT:\s*(adr-\d{4})(?![\w-])") # -------------------------------------------------------------------------- @@ -358,16 +366,31 @@ def source_path(uri: str) -> pathlib.Path: exemption would block the build with a misleading message. Fails closed either way, but closed-and-wrong is still wrong. + + An absolute path is left absolute. It used to be rebased against + `Path.cwd()`, which is not necessarily the checkout being evaluated: with a + `--repo-root` elsewhere, `file:////Sources/A.swift` was rewritten to + `Sources/A.swift` and then looked up under the OTHER root. If a file lived + there too, the gate read it — and approved the finding using a marker from + a checkout the finding had nothing to do with. + + Canonicalising against the root that is actually in use is `inside_checkout`'s + job, and it is the only one that knows which root that is. """ parsed = urlparse(uri) raw = unquote(parsed.path) if parsed.scheme == "file" else unquote(uri) - path = pathlib.Path(raw) - if path.is_absolute(): - try: - path = path.relative_to(pathlib.Path.cwd()) - except ValueError: - pass # genuinely outside the checkout; leave it absolute - return path + return pathlib.Path(raw) + + +def positive_int(value): + """The value if it is a real 1-based SARIF coordinate, else None. + + `isinstance(True, int)` is True in Python, so booleans are excluded + explicitly rather than by accident. + """ + if isinstance(value, bool) or not isinstance(value, int) or value < 1: + return None + return value def inside_checkout(repo_root: pathlib.Path, path) -> pathlib.Path | None: @@ -407,7 +430,14 @@ def __init__(self, path, line, start_column, end_column, message): @property def locatable(self) -> bool: - """Whether this finding can be told apart from another on its line.""" + """Whether this finding can be told apart from another on its line. + + Only real columns count. Treating any non-None value as trustworthy + meant two distinct findings both reported at `startColumn: 0, + endColumn: 0` shared an identity and collapsed into one, which a single + marker then approved. Invalid columns carry no position, so they are + columnless, and columnless findings are never merged. + """ return self.start_column is not None and self.end_column is not None def identity(self): @@ -489,9 +519,8 @@ def evaluate(results_dir, repo_root=None) -> Report: # finding whose location is unknown would be handed line 1, and # a marker that happened to sit there would approve it. An # unlocatable finding cannot be exempted at all. - start_line = region.get("startLine") - if isinstance(start_line, bool) or not isinstance(start_line, int) \ - or start_line < 1: + start_line = positive_int(region.get("startLine")) + if start_line is None: report.errors.append( "a gate finding in " + where_file + " has no usable " "startLine, so it cannot be located or exempted" @@ -501,8 +530,11 @@ def evaluate(results_dir, repo_root=None) -> Report: finding = Finding( path=where_file, line=start_line, - start_column=region.get("startColumn"), - end_column=region.get("endColumn"), + # Validated, not merely present: a malformed column is no + # position at all, so the finding counts as columnless and + # is never deduplicated against another. + start_column=positive_int(region.get("startColumn")), + end_column=positive_int(region.get("endColumn")), message=result["message"]["text"], ) if finding.locatable: diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index e9791f3..c76375a 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -308,6 +308,28 @@ def test_an_adr_behind_a_symlinked_parent_is_refused(self): self.assertIsNone(adr) self.assertIn("no docs/adr/0007", report.errors[0]) + def test_a_longer_number_is_not_this_citation(self): + # `\d{4}` matched the first four digits of adr-00070 and captured + # adr-0007, so a malformed citation rode in on a real ADR. + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + adr, report = self.cite(" NETRELISH-ALLOW-ENDPOINT: adr-00070") + self.assertIsNone(adr) + self.assertEqual(report.errors, []) # no citation at all, not a bad one + + def test_a_suffixed_citation_is_not_this_citation(self): + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + adr, _ = self.cite(" NETRELISH-ALLOW-ENDPOINT: adr-0007foo") + self.assertIsNone(adr) + + def test_the_documented_citation_forms_still_match(self): + (self.root / "docs" / "adr" / "0007-timestamping.md").write_text("#\n") + for text in (" " + MARKER, + " " + MARKER + " — RFC 3161 timestamping", + " " + MARKER + ", and see below", + " " + MARKER + "."): + with self.subTest(text=text): + self.assertEqual(self.cite(text)[0], "adr-0007") + def test_comment_without_a_marker_is_not_an_error(self): adr, report = self.cite(" just an ordinary comment") self.assertIsNone(adr) @@ -561,6 +583,48 @@ def test_a_traversing_uri_that_lands_back_inside_is_canonicalised(self): self.assertTrue(report.ok) self.assertEqual(report.approved[0][0].path, "Sources/A.swift") + def test_malformed_columns_do_not_merge_two_findings(self): + # locatable accepted any non-None column, so two distinct findings both + # at startColumn 0 shared an identity and collapsed into one. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=0, end_column=0), + result("Sources/A.swift", 2, start_column=0, end_column=0), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertEqual(len(report.blocking), 2) + + def test_absolute_paths_are_judged_against_the_given_root_not_the_cwd(self): + # source_path rebased absolute paths against Path.cwd(), so a + # file:// URI under the current directory was rewritten relative and + # looked up under a DIFFERENT --repo-root. Where a file existed there + # too, its marker approved a finding from the other checkout. + import os, tempfile + with tempfile.TemporaryDirectory() as other: + elsewhere = pathlib.Path(other) + (elsewhere / "Sources").mkdir() + real = elsewhere / "Sources" / "A.swift" + real.write_text(CALL + "\n") # no marker here + + decoy = self.checkout.root / "Sources" / "A.swift" + decoy.parent.mkdir(parents=True, exist_ok=True) + decoy.write_text("// " + MARKER + "\n" + CALL) # marker here + + self.checkout.results(sarif(result(real.as_uri(), 1))) + + cwd = os.getcwd() + os.chdir(elsewhere) + try: + report = self.checkout.evaluate() + finally: + os.chdir(cwd) + + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("outside the checkout" in e for e in report.errors)) + def test_a_source_outside_the_checkout_is_not_trusted(self): # A SARIF location naming any file on the runner that happens to carry # a valid marker used to be read, and approved. From 29c356aec8847e130869f303e9348f7b92beb284 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 19:53:43 +0000 Subject: [PATCH 08/12] ci: an unknown URI scheme, and a range that ran backwards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round seven. Two more fail-open holes, both reproduced. 1. Only `file:` was parsed as a URI; every other scheme fell through to Path as a relative filename. `http://evil` became `http:/evil`, and had the checkout contained that path, its markers would have been read and the finding approved — from a URI that names no source file at all. A scheme this does not understand is now a gate error. The guard is narrow on purpose. A relative reference whose FIRST segment contains a colon is invalid per RFC 3986 and must be written `./Odd:Name.swift`, so urlparse reading it as a scheme is correct. Ordinary paths are untouched, because a scheme cannot contain a slash — `Sources/A:B.swift` still resolves, and there is a test pinning that so this cannot start silently refusing real files. 2. positive_int validated each column alone, so `startColumn: 10, endColumn: 5` passed both checks and counted as a position. Two findings carrying that same impossible range collided in the identity map and one marker approved both — the same collapse as the previous round, through a gap the previous round's fix did not cover. A range that runs backwards carries no position, so it is columnless, and columnless findings are never merged. Twenty-two defects now, seventeen of them found by review of this branch. Three regression tests: an unsupported scheme whose path exists in the checkout and carries a marker, an ordinary path with a colon after a slash, and two findings sharing an impossible range. 88 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 +-- scripts/codeql_gate.py | 56 ++++++++++++++++++++++++------- scripts/tests/test_codeql_gate.py | 31 +++++++++++++++++ 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 1dc2a86..a1cc67a 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # twenty defects — five before it was extracted, and fifteen more found + # twenty-two defects — five before it was extracted, and seventeen more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all twenty; read it before adding anything here. + # lists all twenty-two; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 62500d0..7f48900 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,20 +27,22 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found FIFTEEN more, every one of them +Review of the branch that extracted it found SEVENTEEN more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested strings, and behind a bare regex's own closing delimiter; an escaped delimiter ending an extended regex early; a trailing marker exempting the line below it; ADR citations satisfied by a directory, by a symlink, by a symlinked parent directory, and by a longer number that merely started with a real one; a missing SARIF start line defaulting to 1; columnless findings merging on their -message; malformed columns merging two findings into one; source paths escaping -the checkout entirely; absolute paths rebased against the working directory -rather than the checkout being judged; and two spellings of one path splitting -a shared line into two. One of the fifteen was introduced by the fix for -another, and defended in review before it was checked. - -Twenty defects, not one of them in the design. That is the case for the suite +message; malformed columns, and then impossible column RANGES, merging two +findings into one; source paths escaping the checkout entirely; a URI scheme +this did not understand becoming a relative path inside it; absolute paths +rebased against the working directory rather than the checkout being judged; +and two spellings of one path splitting a shared line into two. One of the +seventeen was introduced by the fix for another, and defended in review before +it was checked. + +Twenty-two defects, not one of them in the design. That is the case for the suite in scripts/tests/, which runs on every push — including while CodeQL itself cannot build this project. See scripts/tests/test_codeql_gate.py. @@ -376,8 +378,21 @@ def source_path(uri: str) -> pathlib.Path: Canonicalising against the root that is actually in use is `inside_checkout`'s job, and it is the only one that knows which root that is. + + Returns None for a URI that does not name a local file. Anything other than + `file:` and a plain relative reference used to fall through to `Path`, so + `http://evil` became the relative path `http:/evil` — and had the checkout + contained one, its markers would have been read. A scheme this does not + understand is not a source file, so the caller fails closed instead. + + A relative reference whose first segment contains a colon is invalid per + RFC 3986 and must be written `./Odd:Name.swift`; urlparse reads the part + before that colon as a scheme, so such a URI is refused. `Sources/A:B.swift` + is unaffected — a scheme cannot contain a slash. """ parsed = urlparse(uri) + if parsed.scheme not in ("", "file"): + return None raw = unquote(parsed.path) if parsed.scheme == "file" else unquote(uri) return pathlib.Path(raw) @@ -432,13 +447,21 @@ def __init__(self, path, line, start_column, end_column, message): def locatable(self) -> bool: """Whether this finding can be told apart from another on its line. - Only real columns count. Treating any non-None value as trustworthy + Only a real RANGE counts. Treating any non-None value as trustworthy meant two distinct findings both reported at `startColumn: 0, endColumn: 0` shared an identity and collapsed into one, which a single - marker then approved. Invalid columns carry no position, so they are - columnless, and columnless findings are never merged. + marker then approved. Validating each column on its own was not enough + either: `startColumn: 10, endColumn: 5` passed both checks and two + findings carrying that same impossible range collided the same way. + + An invalid range carries no position, so it is columnless — and + columnless findings are never merged. """ - return self.start_column is not None and self.end_column is not None + return ( + self.start_column is not None + and self.end_column is not None + and self.end_column >= self.start_column + ) def identity(self): """What makes two findings the same call rather than two calls. @@ -497,7 +520,14 @@ def evaluate(results_dir, repo_root=None) -> Report: continue where = result["locations"][0]["physicalLocation"] region = where.get("region", {}) - named = source_path(where["artifactLocation"]["uri"]) + uri = where["artifactLocation"]["uri"] + named = source_path(uri) + if named is None: + report.errors.append( + "a gate finding names " + uri + ", which is not a local " + "file, so it cannot be located or exempted" + ) + continue # Canonicalise here, before this path becomes a grouping key. # diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index c76375a..f6e09be 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -583,6 +583,37 @@ def test_a_traversing_uri_that_lands_back_inside_is_canonicalised(self): self.assertTrue(report.ok) self.assertEqual(report.approved[0][0].path, "Sources/A.swift") + def test_an_unsupported_uri_scheme_is_refused(self): + # `http://evil` fell through to Path and became the relative path + # `http:/evil`, which would have been read had the checkout held one. + planted = self.checkout.root / "http:" / "evil" + planted.parent.mkdir(parents=True, exist_ok=True) + planted.write_text("// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("http://evil", 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("not a local file" in e for e in report.errors)) + + def test_an_ordinary_path_containing_a_colon_still_works(self): + # A colon after a slash is not a scheme, so this must not be refused. + self.checkout.swift("Sources/A:B.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/A:B.swift", 2))) + self.assertTrue(self.checkout.evaluate().ok) + + def test_an_impossible_column_range_does_not_merge_two_findings(self): + # Each column was validated alone, so startColumn 10 / endColumn 5 + # passed and two findings carrying it collided. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + self.checkout.results(sarif( + result("Sources/A.swift", 2, start_column=10, end_column=5), + result("Sources/A.swift", 2, start_column=10, end_column=5), + )) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertEqual(len(report.blocking), 2) + def test_malformed_columns_do_not_merge_two_findings(self): # locatable accepted any non-None column, so two distinct findings both # at startColumn 0 shared an identity and collapsed into one. From ae4051d6e6c80f45601a9adb722f12dc8ccfa09f Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 20:00:04 +0000 Subject: [PATCH 09/12] ci: a SARIF that ran no gate query was reading as a clean one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round eight. Two more, both reproduced, and the second is the worst kind this file can have: it is the exact failure the module's own docstring claims to prevent. 1. The finding identity omitted endLine, so two distinct multi-line regions sharing a path, startLine, startColumn and endColumn collapsed into one and a single marker approved both. endLine is now in the key; adding a component can only split a group, never merge one, so it errs toward blocking. Validity had to follow: endColumn < startColumn is NORMAL when a region spans lines, so the backwards-range check from the previous round now applies only within a single line. A multi-line region staying locatable has its own test, because getting that wrong would quietly push every multi-line finding into the columnless path. 2. A present-but-empty SARIF passed. `{"runs": []}` — or runs from an analysis that never loaded this pack — produced no gate results, and "no results" was indistinguishable from "no findings". The gate printed "clean" and exited 0. This file already refuses to read a MISSING SARIF as clean, and says why in its header. A report that says nothing is the same false assurance with an extra step, and a query that silently stops running is precisely how enforcement disappears without anyone noticing. A clean verdict now requires the rule to appear somewhere — in a run's tool metadata, driver or extension, since a query pack lands in the latter, or in a result. Either is proof the query was part of the analysis. CAVEAT, stated because it cannot be tested here: this expectation about CodeQL's SARIF metadata is unverified against real output, for the same reason everything else in this PR is — CodeQL cannot build this project. If CodeQL omits rules that produced no results, this blocks every clean run. That is loud, immediate and fail-closed, which is the correct direction to be wrong in; the alternative is the silent pass above. The test helper now builds SARIF that attests the query ran, because that is what real CodeQL emits and what a clean verdict now needs. Three of the existing tests failed against this change, correctly. Twenty-four defects now, nineteen found by review of this branch. Two of the nineteen were produced by fixing another. Seven regression tests. 95 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 +- scripts/codeql_gate.py | 71 +++++++++++++++++++++++---- scripts/tests/test_codeql_gate.py | 79 ++++++++++++++++++++++++++++++- 3 files changed, 141 insertions(+), 13 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index a1cc67a..ed2c85a 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # twenty-two defects — five before it was extracted, and seventeen more found + # twenty-four defects — five before it was extracted, and nineteen more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all twenty-two; read it before adding anything here. + # lists all twenty-four; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 7f48900..4cd9a04 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,7 +27,7 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found SEVENTEEN more, every one of them +Review of the branch that extracted it found NINETEEN more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested strings, and behind a bare regex's own closing delimiter; an escaped delimiter ending an extended regex early; a trailing marker exempting the line below it; @@ -38,11 +38,17 @@ findings into one; source paths escaping the checkout entirely; a URI scheme this did not understand becoming a relative path inside it; absolute paths rebased against the working directory rather than the checkout being judged; -and two spellings of one path splitting a shared line into two. One of the -seventeen was introduced by the fix for another, and defended in review before -it was checked. +two spellings of one path splitting a shared line into two; a finding identity +that ignored endLine, merging two distinct multi-line regions; and — worst of +them — a SARIF that ran no gate query at all reading as a clean one, which is +the missing-SARIF false assurance with an extra step. -Twenty-two defects, not one of them in the design. That is the case for the suite +Two of the nineteen were produced by fixing another: one introduced outright +and defended in review before it was checked, one a gap its own predecessor's +fix did not cover. A fix that is correct about what it checks and silent about +what it does not is the shape to watch for here. + +Twenty-four defects, not one of them in the design. That is the case for the suite in scripts/tests/, which runs on every push — including while CodeQL itself cannot build this project. See scripts/tests/test_codeql_gate.py. @@ -397,6 +403,33 @@ def source_path(uri: str) -> pathlib.Path: return pathlib.Path(raw) +def declares_gate(sarif) -> bool: + """Whether this SARIF is evidence that the gate query actually ran. + + The module already refuses to read a MISSING SARIF as clean. A present one + that says nothing is the same false assurance with an extra step: a report + containing `{"runs": []}`, or runs from a analysis that never loaded this + pack, produced no gate results — and "no results" was indistinguishable + from "no findings". A query that silently stops running is exactly how + enforcement disappears without anyone noticing. + + So a clean verdict now requires the rule to appear somewhere: in a run's + tool metadata (driver or extension, since a query pack lands in the latter) + or in a result. Either is proof the query was part of the analysis. + """ + for run in sarif.get("runs") or []: + tool = run.get("tool") or {} + components = [tool.get("driver") or {}] + list(tool.get("extensions") or []) + for component in components: + for rule in component.get("rules") or []: + if isinstance(rule, dict) and rule.get("id") == GATE: + return True + for result in run.get("results") or []: + if result.get("ruleId") == GATE: + return True + return False + + def positive_int(value): """The value if it is a real 1-based SARIF coordinate, else None. @@ -434,11 +467,14 @@ def inside_checkout(repo_root: pathlib.Path, path) -> pathlib.Path | None: class Finding: """One gate result, reduced to the parts that decide its fate.""" - __slots__ = ("path", "line", "start_column", "end_column", "message") + __slots__ = ("path", "line", "end_line", "start_column", "end_column", "message") - def __init__(self, path, line, start_column, end_column, message): + def __init__(self, path, line, start_column, end_column, message, end_line=None): self.path = path self.line = line + # SARIF: endLine defaults to startLine, so a single-line region is + # exactly the case where it is absent. + self.end_line = end_line if end_line is not None else line self.start_column = start_column self.end_column = end_column self.message = message @@ -460,7 +496,8 @@ def locatable(self) -> bool: return ( self.start_column is not None and self.end_column is not None - and self.end_column >= self.start_column + and self.end_line >= self.line + and (self.end_line > self.line or self.end_column >= self.start_column) ) def identity(self): @@ -476,8 +513,14 @@ def identity(self): message would merge them, and a single marker would then approve both. That is the shared-line bypass returning through the back door. Keeping every occurrence instead can only over-count, which blocks. + + `end_line` is in the key because leaving it out merged two distinct + multi-line regions that happened to share a start and an end column. + Adding a component to this key can only ever SPLIT a group, never merge + one, so it errs in the blocking direction. """ - return (self.path, self.line, self.start_column, self.end_column) + return (self.path, self.line, self.end_line, + self.start_column, self.end_column) class Report: @@ -511,9 +554,11 @@ def evaluate(results_dir, repo_root=None) -> Report: findings = {} # locatable, deduplicated by position loose = [] # columnless, never deduplicated (see Finding.identity) + attested = False for path in paths: with open(path) as handle: sarif = json.load(handle) + attested = declares_gate(sarif) or attested for run in sarif.get("runs", []): for result in run.get("results", []): if result.get("ruleId") != GATE: @@ -566,12 +611,20 @@ def evaluate(results_dir, repo_root=None) -> Report: start_column=positive_int(region.get("startColumn")), end_column=positive_int(region.get("endColumn")), message=result["message"]["text"], + end_line=positive_int(region.get("endLine")), ) if finding.locatable: findings[finding.identity()] = finding else: loose.append(finding) + if not attested: + report.errors.append( + "no SARIF in " + str(results_dir) + " mentions " + GATE + + " — the gate query did not run, and a report with no evidence of " + "it is not a clean one" + ) + # Group by the unit the exemption mechanism can actually address. A marker # names a line, not a call, so a line carrying two gate findings cannot be # approved by one — that was bypass 5: adding a second call beside an diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index f6e09be..70a2cba 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -26,8 +26,23 @@ CALL = "_ = try await URLSession.shared.data(from: url)" -def sarif(*results): - return {"runs": [{"results": list(results)}]} +def sarif(*results, declares=True): + """A SARIF run, by default one that attests the gate query ran. + + Real CodeQL reports the queries it ran in tool metadata, and a clean + verdict now requires that evidence — `declares=False` is the report that + contains none, which must never read as clean. + """ + run = {"results": list(results)} + if declares: + run["tool"] = { + "driver": {"name": "CodeQL"}, + "extensions": [{ + "name": "netrelish/swift-rules", + "rules": [{"id": codeql_gate.GATE}], + }], + } + return {"runs": [run]} def result(uri, line, rule=codeql_gate.GATE, message="outbound network call", @@ -350,6 +365,41 @@ def test_missing_sarif_fails(self): self.assertFalse(report.ok) self.assertIn("the gate did not run", report.errors[0]) + def test_a_sarif_that_never_ran_the_gate_is_not_clean(self): + # `{"runs": []}` produced no gate results, and "no results" was + # indistinguishable from "no findings". A query that silently stops + # running is how enforcement disappears unnoticed. + self.checkout.results({"runs": []}) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertTrue(any("did not run" in e for e in report.errors)) + + def test_a_run_without_the_gate_rule_is_not_clean(self): + # A real-looking run from an analysis that never loaded this pack. + self.checkout.results(sarif(declares=False)) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertTrue(any("did not run" in e for e in report.errors)) + + def test_a_gate_result_alone_attests_the_query_ran(self): + # Evidence can come from a result as well as from tool metadata. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + self.checkout.results(sarif(result("Sources/A.swift", 2), declares=False)) + self.assertTrue(self.checkout.evaluate().ok) + + def test_the_gate_rule_declared_in_the_driver_also_attests(self): + self.checkout.results({"runs": [{ + "tool": {"driver": {"name": "CodeQL", + "rules": [{"id": codeql_gate.GATE}]}}, + "results": [], + }]}) + self.assertTrue(self.checkout.evaluate().ok) + + def test_attestation_may_come_from_any_sarif_in_the_directory(self): + self.checkout.results({"runs": []}, "a.sarif") + self.checkout.results(sarif(), "b.sarif") + self.assertTrue(self.checkout.evaluate().ok) + def test_clean_sarif_passes(self): self.checkout.results(sarif()) self.assertTrue(self.checkout.evaluate().ok) @@ -601,6 +651,31 @@ def test_an_ordinary_path_containing_a_colon_still_works(self): self.checkout.results(sarif(result("Sources/A:B.swift", 2))) self.assertTrue(self.checkout.evaluate().ok) + def test_distinct_multi_line_regions_do_not_merge(self): + # The identity omitted endLine, so two regions sharing a start and an + # end column collapsed however far apart they ended. + self.checkout.swift( + "Sources/A.swift", + "// " + MARKER + "\n_ = a(b(\n c))\n d))", + ) + hit_a = result("Sources/A.swift", 2, start_column=5, end_column=9) + hit_b = result("Sources/A.swift", 2, start_column=5, end_column=9) + hit_a["locations"][0]["physicalLocation"]["region"]["endLine"] = 3 + hit_b["locations"][0]["physicalLocation"]["region"]["endLine"] = 4 + self.checkout.results(sarif(hit_a, hit_b)) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertEqual(len(report.blocking), 2) + + def test_a_multi_line_region_is_still_locatable(self): + # endColumn < startColumn is normal when the region spans lines. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n_ = a(\n b)") + hit = result("Sources/A.swift", 2, start_column=40, end_column=5) + hit["locations"][0]["physicalLocation"]["region"]["endLine"] = 3 + self.checkout.results(sarif(hit)) + self.assertTrue(self.checkout.evaluate().ok) + def test_an_impossible_column_range_does_not_merge_two_findings(self): # Each column was validated alone, so startColumn 10 / endColumn 5 # passed and two findings carrying it collided. From f15b46fa0050e037fd38644f7156c028407e320d Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 20:07:09 +0000 Subject: [PATCH 10/12] ci: a bare regex's punctuation steered the scanner out of its string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round nine. One hole, reproduced. A bare `/.../` literal was still ordinary code to the scanner, and its pattern text can contain the exact characters the scanner steers by: let s = "\(/[)]"/) // NETRELISH-ALLOW-ENDPOINT: adr-0007" The regex's `)` closed the interpolation and its `"` closed the outer string, so the marker — which is string content, not a comment — was recorded as one and approved the call below. Two rounds ago I argued the bare form needed no tracking, because escape opacity left its closing delimiter with nothing to pair with. That answered the delimiter, and said nothing about the contents. The literal is now opaque as a whole. The interesting part is the other direction. Naively treating every `/` as a regex opener would read `a/b // NETRELISH-ALLOW-ENDPOINT: adr-0007` as a literal running up to the comment's own slashes, losing a REAL marker and blocking a legitimate exemption. So this follows Swift's own rule: `/.../` is a regex where an EXPRESSION is expected, division after a value — approximated by the previous significant character, since after an identifier, a number, `)`, `]` or a closing quote we are past a value. A `/` with no closing partner on the line is not a regex either, and falls through rather than swallowing the rest of the line. Seven regression tests, and most of them guard that second direction rather than the hole: division with a trailing marker, division twice on one line, a regex literal alone, a comment after one, and a lone slash. Getting this wrong the safe way would still be wrong. 102 total. Twenty-five defects now, twenty found by review of this branch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 +- scripts/codeql_gate.py | 71 +++++++++++++++++++++++++++++-- scripts/tests/test_codeql_gate.py | 33 ++++++++++++++ 3 files changed, 102 insertions(+), 6 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index ed2c85a..91083a3 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # twenty-four defects — five before it was extracted, and nineteen more found + # twenty-five defects — five before it was extracted, and twenty more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all twenty-four; read it before adding anything here. + # lists all twenty-five; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 4cd9a04..1ff783b 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,7 +27,7 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found NINETEEN more, every one of them +Review of the branch that extracted it found TWENTY more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested strings, and behind a bare regex's own closing delimiter; an escaped delimiter ending an extended regex early; a trailing marker exempting the line below it; @@ -41,14 +41,16 @@ two spellings of one path splitting a shared line into two; a finding identity that ignored endLine, merging two distinct multi-line regions; and — worst of them — a SARIF that ran no gate query at all reading as a clean one, which is -the missing-SARIF false assurance with an extra step. +the missing-SARIF false assurance with an extra step; and a bare regex inside +an interpolation whose own `)` and `"` steered the scanner out of the string +it was in. -Two of the nineteen were produced by fixing another: one introduced outright +Two of the twenty were produced by fixing another: one introduced outright and defended in review before it was checked, one a gap its own predecessor's fix did not cover. A fix that is correct about what it checks and silent about what it does not is the shape to watch for here. -Twenty-four defects, not one of them in the design. That is the case for the suite +Twenty-five defects, not one of them in the design. That is the case for the suite in scripts/tests/, which runs on every push — including while CodeQL itself cannot build this project. See scripts/tests/test_codeql_gate.py. @@ -286,6 +288,25 @@ def advance(count: int) -> None: advance(end - index) continue + if char == "/" and _regex_may_start(source, index) \ + and index + 1 < source_length and source[index + 1] not in " \t\n/*": + # A bare `/.../` literal. Its contents are pattern text, and that + # text can contain the punctuation this scanner steers by: in + # + # let s = "\(/[)]"/) // NETRELISH-ALLOW-ENDPOINT: adr-0007" + # + # the regex's `)` closed the interpolation and its `"` closed the + # outer string, so the marker — which is string content — was + # recorded as a comment. Making the literal opaque keeps the + # scanner inside the construct it is actually in. + # + # A `/` with no closing partner on the line is not a regex, so it + # falls through rather than swallowing the rest of the line. + closes = _bare_regex_end(source, index) + if closes is not None: + advance(closes - index) + continue + if char == "\\": # An escape in code is opaque: skip it and whatever it escapes. # @@ -403,6 +424,48 @@ def source_path(uri: str) -> pathlib.Path: return pathlib.Path(raw) +def _regex_may_start(source: str, index: int) -> bool: + """Whether a `/` at `index` opens a bare regex rather than dividing. + + Swift resolves this by grammar: `/.../` is a regex where an EXPRESSION is + expected, and division after a value. Approximated by the previous + significant character — after an identifier, a number, `)`, `]` or a closing + quote we are past a value, so the slash divides. Anywhere else (`=`, `(`, + `,`, `return`, start of line) an expression is expected. + + Without this, `a/b // NETRELISH-ALLOW-ENDPOINT: adr-0007` would be read as + a regex running up to the comment's own slashes, and a REAL marker would be + lost. That direction merely blocks, but blocking a legitimate exemption is + still a bug. + """ + scan = index - 1 + while scan >= 0 and source[scan] in " \t": + scan -= 1 + if scan < 0: + return True + previous = source[scan] + return not (previous.isalnum() or previous in '_)]"') + + +def _bare_regex_end(source: str, index: int) -> int | None: + """Where the bare regex opening at `index` ends, or None if it does not. + + Swift's closing delimiter is an unescaped `/` not preceded by whitespace, + on the same line. A `/` with no such partner is not a regex at all, so the + caller leaves it alone rather than swallowing the rest of the line. + """ + length = len(source) + scan = index + 1 + while scan < length and source[scan] != "\n": + if source[scan] == "\\": + scan += 2 + continue + if source[scan] == "/" and source[scan - 1] not in " \t": + return scan + 1 + scan += 1 + return None + + def declares_gate(sarif) -> bool: """Whether this SARIF is evidence that the gate query actually ran. diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index 70a2cba..4029315 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -207,6 +207,31 @@ def test_escaped_delimiter_does_not_end_an_extended_regex(self): source = r'let r = #/ a\/# b // ' + MARKER + ' /#' self.assertIsNone(self.comment_on(source, 1)) + def test_a_bare_regex_inside_interpolation_keeps_its_punctuation(self): + # `/[)]"/` carries the very characters the scanner steers by: the `)` + # closed the interpolation and the `"` closed the outer string, so the + # marker — which is string content — was recorded as a comment. + source = r'let s = "\(/[)]"/) // ' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_division_is_not_mistaken_for_a_regex(self): + # The failure in the other direction: reading `a/b` as a regex would + # run it up to the comment's own slashes and lose a REAL marker. + self.assertIn(MARKER, self.comment_on("let x = a/b // " + MARKER, 1)) + + def test_division_between_two_values_on_one_line(self): + self.assertIsNone(self.comment_on("let x = a/b + c/d", 1)) + + def test_a_regex_literal_is_opaque(self): + self.assertIsNone(self.comment_on("let r = /[a-z]+/", 1)) + + def test_a_comment_after_a_regex_literal_still_counts(self): + self.assertIn(MARKER, self.comment_on("let r = /[a-z]+/ // " + MARKER, 1)) + + def test_a_lone_slash_is_not_a_regex(self): + # No closing partner on the line, so it must not swallow the rest. + self.assertIn(MARKER, self.comment_on("let x = f(/ ) // " + MARKER, 1)) + def test_extended_regex_literal_is_not_a_comment(self): # Extended regex ignores whitespace, so the marker inside one reads # exactly like an approval to a scanner that does not know the form. @@ -511,6 +536,14 @@ def test_interpolated_string_marker_does_not_approve(self): self.checkout.results(sarif(result("Sources/A.swift", 2))) self.assertFalse(self.checkout.evaluate().ok) + def test_bare_regex_in_interpolation_marker_does_not_approve(self): + self.checkout.swift( + "Sources/A.swift", + r'let s = "\(/[)]"/) // ' + MARKER + '"\n' + CALL, + ) + self.checkout.results(sarif(result("Sources/A.swift", 2))) + self.assertFalse(self.checkout.evaluate().ok) + def test_regex_literal_marker_does_not_approve(self): self.checkout.swift( "Sources/A.swift", From a8a01ac013eaa507e76a14bd10881326a4bafcba Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 20:12:50 +0000 Subject: [PATCH 11/12] ci: a regex after `return`, and a malformed endLine passing as absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round ten. Two holes, both reproduced, and both are the previous round's fixes being correct about what they addressed and silent about the rest. 1. `_regex_may_start` read the previous CHARACTER, so after `return` it saw a letter, concluded "past a value", and refused to treat `/[)]"/` as a regex. Its punctuation was live again and steered the scanner out of the string containing it — hole 23 exactly, one keyword away from where I fixed it. It now reads the previous TOKEN. An identifier is only a value if it is not one of the keywords that end an expression context: return, throw, try, await, case, where, in, is, as and the rest. None of them can be a variable name, so reading them this way cannot misfire. `return a/b` still divides, because the token before the slash is `a`, not `return`. That has a test, along with division after `)`, since widening what counts as a regex risks swallowing a real marker. 2. A present-but-malformed `endLine` was indistinguishable from an absent one: positive_int returned None for both and the constructor defaulted both to startLine. A finding with broken coordinates could therefore share an identity with a sound single-line one, and a single marker approved both. Findings now carry whether their coordinates are trusted, and an untrusted one follows malformed columns into the columnless path, where nothing is ever merged. An ABSENT endLine still means a single-line region and still deduplicates — with its own test, because losing that would quietly stop deduplication working at all. Twenty-seven defects now, twenty-two found by review of this branch. Four of those twenty-two were produced by fixing another. Six regression tests. 108 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 +- scripts/codeql_gate.py | 75 ++++++++++++++++++++++++------- scripts/tests/test_codeql_gate.py | 42 +++++++++++++++++ 3 files changed, 102 insertions(+), 19 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 91083a3..67b5f17 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # twenty-five defects — five before it was extracted, and twenty more found + # twenty-seven defects — five before it was extracted, and twenty-two more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all twenty-five; read it before adding anything here. + # lists all twenty-seven; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 1ff783b..5c990e9 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,7 +27,7 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found TWENTY more, every one of them +Review of the branch that extracted it found TWENTY-TWO more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested strings, and behind a bare regex's own closing delimiter; an escaped delimiter ending an extended regex early; a trailing marker exempting the line below it; @@ -41,16 +41,18 @@ two spellings of one path splitting a shared line into two; a finding identity that ignored endLine, merging two distinct multi-line regions; and — worst of them — a SARIF that ran no gate query at all reading as a clean one, which is -the missing-SARIF false assurance with an extra step; and a bare regex inside -an interpolation whose own `)` and `"` steered the scanner out of the string -it was in. +the missing-SARIF false assurance with an extra step; a bare regex inside an +interpolation whose own `)` and `"` steered the scanner out of the string it +was in, and then the same again for a regex following `return`, which the +previous-character test read as a value; and a present-but-malformed endLine +being indistinguishable from an absent one. -Two of the twenty were produced by fixing another: one introduced outright +Four of the twenty-two were produced by fixing another: one introduced outright and defended in review before it was checked, one a gap its own predecessor's fix did not cover. A fix that is correct about what it checks and silent about what it does not is the shape to watch for here. -Twenty-five defects, not one of them in the design. That is the case for the suite +Twenty-seven defects, not one of them in the design. That is the case for the suite in scripts/tests/, which runs on every push — including while CodeQL itself cannot build this project. See scripts/tests/test_codeql_gate.py. @@ -424,27 +426,52 @@ def source_path(uri: str) -> pathlib.Path: return pathlib.Path(raw) +# Swift keywords after which an expression begins, so a `/` following one opens +# a regex rather than dividing. None of these can be an identifier, so reading +# them this way cannot misfire on a variable name. +EXPRESSION_KEYWORDS = frozenset({ + "return", "throw", "try", "await", "case", "where", "in", "is", "as", + "if", "guard", "while", "repeat", "else", "do", "defer", "yield", + "let", "var", "and", "or", "not", "some", "any", "each", +}) + + def _regex_may_start(source: str, index: int) -> bool: """Whether a `/` at `index` opens a bare regex rather than dividing. Swift resolves this by grammar: `/.../` is a regex where an EXPRESSION is expected, and division after a value. Approximated by the previous - significant character — after an identifier, a number, `)`, `]` or a closing + significant token — after an identifier, a number, `)`, `]` or a closing quote we are past a value, so the slash divides. Anywhere else (`=`, `(`, - `,`, `return`, start of line) an expression is expected. - - Without this, `a/b // NETRELISH-ALLOW-ENDPOINT: adr-0007` would be read as - a regex running up to the comment's own slashes, and a REAL marker would be - lost. That direction merely blocks, but blocking a legitimate exemption is - still a bug. + `,`, start of line) an expression is expected. + + An identifier is not always a value, which the first version of this missed: + the keywords below all END an expression context rather than producing one, + so `return /[)]"/` is a regex even though the character before the slash is + a letter. Left unrecognised, that regex's punctuation was live again and + steered the scanner out of the string containing it. + + Going the other way costs something real too: `a/b // + NETRELISH-ALLOW-ENDPOINT: adr-0007` must not be read as a regex running up + to the comment's own slashes, because that loses a REAL marker. Only + blocks — but blocking a legitimate exemption is still a bug. """ scan = index - 1 while scan >= 0 and source[scan] in " \t": scan -= 1 if scan < 0: return True + previous = source[scan] - return not (previous.isalnum() or previous in '_)]"') + if previous in ')]"': + return False + if not (previous.isalnum() or previous == "_"): + return True + + end = scan + 1 + while scan >= 0 and (source[scan].isalnum() or source[scan] == "_"): + scan -= 1 + return source[scan + 1:end] in EXPRESSION_KEYWORDS def _bare_regex_end(source: str, index: int) -> int | None: @@ -530,14 +557,24 @@ def inside_checkout(repo_root: pathlib.Path, path) -> pathlib.Path | None: class Finding: """One gate result, reduced to the parts that decide its fate.""" - __slots__ = ("path", "line", "end_line", "start_column", "end_column", "message") + __slots__ = ("path", "line", "end_line", "start_column", "end_column", + "message", "trusted") - def __init__(self, path, line, start_column, end_column, message, end_line=None): + def __init__(self, path, line, start_column, end_column, message, + end_line=None, trusted=True): self.path = path self.line = line # SARIF: endLine defaults to startLine, so a single-line region is # exactly the case where it is absent. + # + # `trusted` is how an absent endLine is told apart from a PRESENT but + # malformed one. Both arrive here as None, and defaulting both to + # startLine let a finding with broken coordinates share an identity + # with a sound single-line one — so one marker approved both. A region + # that reports an impossible end is not a position, and follows + # malformed columns into the columnless path. self.end_line = end_line if end_line is not None else line + self.trusted = trusted self.start_column = start_column self.end_column = end_column self.message = message @@ -557,7 +594,8 @@ def locatable(self) -> bool: columnless findings are never merged. """ return ( - self.start_column is not None + self.trusted + and self.start_column is not None and self.end_column is not None and self.end_line >= self.line and (self.end_line > self.line or self.end_column >= self.start_column) @@ -675,6 +713,9 @@ def evaluate(results_dir, repo_root=None) -> Report: end_column=positive_int(region.get("endColumn")), message=result["message"]["text"], end_line=positive_int(region.get("endLine")), + # Present-but-malformed is not the same as absent. + trusted=("endLine" not in region + or positive_int(region.get("endLine")) is not None), ) if finding.locatable: findings[finding.identity()] = finding diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index 4029315..920dec6 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -214,6 +214,24 @@ def test_a_bare_regex_inside_interpolation_keeps_its_punctuation(self): source = r'let s = "\(/[)]"/) // ' + MARKER + '"' self.assertIsNone(self.comment_on(source, 1)) + def test_a_regex_after_an_expression_keyword_is_opaque(self): + # The character before the slash is a letter, but `return` ends an + # expression context rather than producing a value — so this is a + # regex, and its `)` and `"` must not steer the scanner. + source = r'let s = "\({ return /[)]"/ }()) // ' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_a_regex_after_try_is_opaque(self): + source = r'let s = "\({ try /[)]"/ }()) // ' + MARKER + '"' + self.assertIsNone(self.comment_on(source, 1)) + + def test_division_after_a_keyword_is_still_division(self): + # `return a/b` divides: the token before the slash is `a`, not `return`. + self.assertIn(MARKER, self.comment_on("return a/b // " + MARKER, 1)) + + def test_division_after_a_closing_paren(self): + self.assertIn(MARKER, self.comment_on("let x = f(y)/2 // " + MARKER, 1)) + def test_division_is_not_mistaken_for_a_regex(self): # The failure in the other direction: reading `a/b` as a regex would # run it up to the comment's own slashes and lose a REAL marker. @@ -701,6 +719,30 @@ def test_distinct_multi_line_regions_do_not_merge(self): self.assertEqual(report.approved, []) self.assertEqual(len(report.blocking), 2) + def test_a_malformed_end_line_does_not_merge_with_a_sound_finding(self): + # Absent and present-but-invalid both became None and defaulted to + # startLine, so a finding with broken coordinates shared an identity + # with a sound single-line one. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL + "; " + CALL) + sound = result("Sources/A.swift", 2, start_column=5, end_column=46) + broken = result("Sources/A.swift", 2, start_column=5, end_column=46) + broken["locations"][0]["physicalLocation"]["region"]["endLine"] = 0 + self.checkout.results(sarif(sound, broken)) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertEqual(len(report.blocking), 2) + + def test_an_absent_end_line_is_still_a_single_line_region(self): + # The distinction must not cost the ordinary case its deduplication. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + hit = result("Sources/A.swift", 2, start_column=5, end_column=46) + self.checkout.results(sarif(hit), "a.sarif") + self.checkout.results(sarif(hit), "b.sarif") + report = self.checkout.evaluate() + self.assertTrue(report.ok) + self.assertEqual(len(report.approved), 1) + def test_a_multi_line_region_is_still_locatable(self): # endColumn < startColumn is normal when the region spans lines. self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n_ = a(\n b)") From 407bc6a6dac4c6675fe7e649000e05150f016055 Mon Sep 17 00:00:00 2001 From: Ryan Shepherd Date: Thu, 24 Sep 2026 20:19:16 +0000 Subject: [PATCH 12/12] ci: a file:// URI's authority was discarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round eleven. One hole, reproduced, and it stood in the same function as the guard that was supposed to have covered it. `file://attacker//Sources/A.swift` names a file on another host. source_path used only `parsed.path` and dropped `netloc`, so that URI resolved against THIS checkout and was approved by the local marker there. Two rounds ago I added a scheme check to that same function, for exactly this class of problem — a URI that does not name a local file being treated as one. I validated the scheme and never looked at the field beside it. Fifth time in this PR that a fix has been right about what it addressed and silent about the rest of its category, and the first where the gap was one line from the fix. Per RFC 8089 only an empty authority or `localhost` means this machine; anything else is refused, case-insensitively. `file:///path` and `file://localhost/path` both still resolve, the latter with its own test, because tightening this risks refusing legitimate locations and that failure is silent — the gate simply blocks and the call reads as unapproved. Twenty-eight defects now, twenty-three found by review of this branch. Five of those twenty-three were produced by fixing another. Two regression tests. 110 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BTRH1R9NLhPzzAYH6Ginhf --- .github/workflows/codeql.yml | 4 ++-- scripts/codeql_gate.py | 25 ++++++++++++++++++------- scripts/tests/test_codeql_gate.py | 19 +++++++++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 67b5f17..5befdc9 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -160,14 +160,14 @@ jobs: # afternoon loses. This travels with the repo. # # The logic is in scripts/codeql_gate.py, not inline here. It has had - # twenty-seven defects — five before it was extracted, and twenty-two more found + # twenty-eight defects — five before it was extracted, and twenty-three more found # by review of the branch that extracted it — every one in its # enforcement rather than its design, and every one failing OPEN. A # heredoc cannot be tested, so the first five were caught by a human # reading it, or not caught at all. It now has a suite in scripts/tests/ # that the `CodeQL gate self-test` job in ci.yml runs on every push, # including while THIS job cannot build the project. The module header - # lists all twenty-seven; read it before adding anything here. + # lists all twenty-eight; read it before adding anything here. # # Only the gate fails the build. netrelish/remote-capable-url-read has two # legitimate baseline hits; failing on it would teach everyone to ignore a diff --git a/scripts/codeql_gate.py b/scripts/codeql_gate.py index 5c990e9..d465255 100644 --- a/scripts/codeql_gate.py +++ b/scripts/codeql_gate.py @@ -27,7 +27,7 @@ so every one of those was caught by a human reading it, or not caught at all. That list is the HISTORICAL set — the five that existed before this file did. -Review of the branch that extracted it found TWENTY-TWO more, every one of them +Review of the branch that extracted it found TWENTY-THREE more, every one of them failing open: markers hidden in extended regex literals, in interpolated nested strings, and behind a bare regex's own closing delimiter; an escaped delimiter ending an extended regex early; a trailing marker exempting the line below it; @@ -44,15 +44,17 @@ the missing-SARIF false assurance with an extra step; a bare regex inside an interpolation whose own `)` and `"` steered the scanner out of the string it was in, and then the same again for a regex following `return`, which the -previous-character test read as a value; and a present-but-malformed endLine -being indistinguishable from an absent one. +previous-character test read as a value; a present-but-malformed endLine +being indistinguishable from an absent one; and a `file://host/path` URI whose +authority was discarded, resolving another machine's path against this +checkout. -Four of the twenty-two were produced by fixing another: one introduced outright +Five of the twenty-three were produced by fixing another: one introduced outright and defended in review before it was checked, one a gap its own predecessor's fix did not cover. A fix that is correct about what it checks and silent about what it does not is the shape to watch for here. -Twenty-seven defects, not one of them in the design. That is the case for the suite +Twenty-eight defects, not one of them in the design. That is the case for the suite in scripts/tests/, which runs on every push — including while CodeQL itself cannot build this project. See scripts/tests/test_codeql_gate.py. @@ -414,6 +416,12 @@ def source_path(uri: str) -> pathlib.Path: contained one, its markers would have been read. A scheme this does not understand is not a source file, so the caller fails closed instead. + The AUTHORITY has to be checked too, which the first version of that guard + missed while standing in the same function: `file://attacker/x/A.swift` + names a file on another host, and discarding `netloc` resolved its path + against this checkout and approved it from a local marker. Per RFC 8089 + only an empty authority or `localhost` is local. + A relative reference whose first segment contains a colon is invalid per RFC 3986 and must be written `./Odd:Name.swift`; urlparse reads the part before that colon as a scheme, so such a URI is refused. `Sources/A:B.swift` @@ -422,8 +430,11 @@ def source_path(uri: str) -> pathlib.Path: parsed = urlparse(uri) if parsed.scheme not in ("", "file"): return None - raw = unquote(parsed.path) if parsed.scheme == "file" else unquote(uri) - return pathlib.Path(raw) + if parsed.scheme == "file": + if parsed.netloc.lower() not in ("", "localhost"): + return None + return pathlib.Path(unquote(parsed.path)) + return pathlib.Path(unquote(uri)) # Swift keywords after which an expression begins, so a `/` following one opens diff --git a/scripts/tests/test_codeql_gate.py b/scripts/tests/test_codeql_gate.py index 920dec6..0466563 100644 --- a/scripts/tests/test_codeql_gate.py +++ b/scripts/tests/test_codeql_gate.py @@ -696,6 +696,25 @@ def test_an_unsupported_uri_scheme_is_refused(self): self.assertEqual(report.approved, []) self.assertTrue(any("not a local file" in e for e in report.errors)) + def test_a_remote_file_authority_is_refused(self): + # `file://attacker//Sources/A.swift` names a file on another + # host; discarding the authority resolved its path against this + # checkout and approved it from a local marker. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + local = (self.checkout.root / "Sources" / "A.swift").resolve() + self.checkout.results(sarif(result("file://attacker" + str(local), 2))) + report = self.checkout.evaluate() + self.assertFalse(report.ok) + self.assertEqual(report.approved, []) + self.assertTrue(any("not a local file" in e for e in report.errors)) + + def test_localhost_is_a_local_file_authority(self): + # RFC 8089: an empty authority and `localhost` both mean this machine. + self.checkout.swift("Sources/A.swift", "// " + MARKER + "\n" + CALL) + local = (self.checkout.root / "Sources" / "A.swift").resolve() + self.checkout.results(sarif(result("file://localhost" + str(local), 2))) + self.assertTrue(self.checkout.evaluate().ok) + def test_an_ordinary_path_containing_a_colon_still_works(self): # A colon after a slash is not a scheme, so this must not be refused. self.checkout.swift("Sources/A:B.swift", "// " + MARKER + "\n" + CALL)