fix(security): a bearer token gated on a substring, not a host

CodeQL raised py/incomplete-url-substring-sanitization on
scripts/check_action_refs.py, and unlike the three alerts recorded in
SECURITY.md it was right about the mechanism, not just the smell.

    if token and "api.github.com" in url:
        request.add_header("Authorization", f"Bearer {token}")

That substring is present in `https://elsewhere.example/?next=api.github.com`,
in `https://api.github.com.evil.example/`, and in
`https://evil.example/api.github.com/`. What hangs off the test is whether
a GitHub token is attached, so the loose form hands a credential to any
host that cares to mention the name.

Not exploitable today -- every URL in that file is built from a literal
host -- which is exactly how a check like this survives review until the
day someone makes the host a parameter.

Now compares urlsplit(url).hostname, and four tests hold it there: the
real API host still gets the token (or the rest would prove nothing),
three lookalikes get none, and pypi.org -- the other host this same
function reaches -- gets none either.

SECURITY.md renumbered to the current alert ids and now distinguishes the
three analyser false positives from this one, which was a defect and was
fixed rather than explained away.

Author: webdevsamran

SECURITY.md

@@ -51,8 +51,15 @@ Out of scope:
 ## Static-analysis findings that are open on purpose
 
 CodeQL (`security-extended`) runs on every push. Three alerts are open and are
-not defects. They are recorded here rather than only in a dismissal box, so the
-reasoning is reviewable and can be re-checked when the code changes.
+not defects. A fourth was raised and **was** a defect -- a bearer token attached
+after an `"api.github.com" in url` substring test, which is true of any host that
+mentions the name. That one was fixed in `scripts/check_action_refs.py` rather
+than explained away, and `tests/test_secrets_do_not_leak.py` now proves three
+lookalike hosts receive no credential.
+
+The three below are different in kind. They are recorded here rather than only in
+a dismissal box, so the reasoning is reviewable and can be re-checked when the
+code changes.
 
 All three come from CodeQL's **name-based** sensitive-data classifier: it treats
 identifiers spelled `key`, `secret` or `password` as sensitive sources and then
@@ -63,8 +70,8 @@ plants real-shaped secrets and asserts on the actual bytes.
 
 | Alert | Query | Location | Why it fires, and why it is wrong |
 |---|---|---|---|
-| #4, #5 | `py/clear-text-logging-sensitive-data` | `tooltrace/cli/main.py` (`_emit`) | A verification key reaches `a2a.report()`, whose result is printed. Inside, the key is used only as the first argument to `hmac.new` and `hmac.compare_digest`; the returned structure holds signature *states*, algorithm names and prose. Two tests drive both output paths with a real key and assert it appears in neither stdout nor stderr. |
-| #3 | `py/clear-text-storage-sensitive-data` | `tooltrace/security/redaction.py` (`write_record`) | The record contains `residual_secret_classes` -- the **labels** of secret patterns that still match after sanitisation, such as `aws_access_key`, never the matched text. `Finding` documented that in a comment and nothing checked it; a test now plants an email, a card number and an AWS key and asserts none appears in the report, the record, or either file written to disk. |
+| #2, #3 | `py/clear-text-logging-sensitive-data` | `tooltrace/cli/main.py` (`_emit`) | A verification key reaches `a2a.report()`, whose result is printed. Inside, the key is used only as the first argument to `hmac.new` and `hmac.compare_digest`; the returned structure holds signature *states*, algorithm names and prose. Two tests drive both output paths with a real key and assert it appears in neither stdout nor stderr. |
+| #1 | `py/clear-text-storage-sensitive-data` | `tooltrace/security/redaction.py` (`write_record`) | The record contains `residual_secret_classes` -- the **labels** of secret patterns that still match after sanitisation, such as `aws_access_key`, never the matched text. `Finding` documented that in a comment and nothing checked it; a test now plants an email, a card number and an AWS key and asserts none appears in the report, the record, or either file written to disk. |
 
 These are not suppressed in source. A `# codeql[...]` comment on `_emit` would
 blanket every command that prints anything, which is exactly the kind of

scripts/check_action_refs.py

@@ -38,6 +38,7 @@
 import re
 import sys
 import urllib.error
+import urllib.parse
 import urllib.request
 from pathlib import Path
 
@@ -69,8 +70,14 @@ def _get_json(url: str) -> object | None:
         url,
         headers={"Accept": "application/vnd.github+json", "User-Agent": "check-action-refs"},
     )
+    # Compare the parsed host, never a substring. `"api.github.com" in url` is
+    # true of `https://elsewhere.example/?x=api.github.com`, and what hangs off
+    # this test is whether a credential is attached -- so the loose form is a
+    # token-disclosure bug waiting for the day the URL stops being a literal.
+    # CodeQL flagged exactly this, and unlike the three alerts recorded in
+    # SECURITY.md it was right about the mechanism.
     token = os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN")
-    if token and "api.github.com" in url:
+    if token and urllib.parse.urlsplit(url).hostname == "api.github.com":
         request.add_header("Authorization", f"Bearer {token}")
     try:
         # Fixed https hosts, built below from literals.

tests/test_secrets_do_not_leak.py

@@ -2,9 +2,18 @@
 
 Six CodeQL alerts landed on this repository, all `high`, three of them naming
 this file's subject: clear-text logging in `cli/main.py` and clear-text storage
-in `security/redaction.py`. Every one turned out to be a false positive for the
-vulnerability it claimed -- the analyser cannot see through `report()` or
-`redaction_report()` and assumed taint propagated.
+in `security/redaction.py`. Those three were false positives for the
+vulnerability they claimed -- the analyser cannot see through `report()` or
+`redaction_report()` and assumed taint propagated. They are recorded, with
+reasons, in SECURITY.md.
+
+A seventh was not a false positive, and the last section here is about it:
+`scripts/check_action_refs.py` decided whether to attach a bearer token with
+`"api.github.com" in url`. That substring is present in
+`https://elsewhere.example/?next=api.github.com`, so the loose test would have
+handed a GitHub token to any host that cared to mention the name. Not
+exploitable while every URL was built from a literal -- which is exactly how a
+check like that survives review until the day it isn't.
 
 "Assumed" is the operative word, in both directions. Nothing here *proved* the
 key never reached the output, or that the redaction record never quoted what it
@@ -188,3 +197,62 @@ def test_the_record_still_reports_that_it_found_something(tmp_path: Path) -> Non
     )
     record = redaction_report(bundle)
     assert record["pii_findings"], "nothing was detected, so the leak check proves nothing"
+
+
+# --- a token goes to one host, and host means host ----------------------------
+
+
+def _captured_request(url: str, monkeypatch) -> object:
+    """Run `_get_json` against `url`, returning the Request it would have sent."""
+    import importlib.util
+    import sys as _sys
+    import urllib.error
+    import urllib.request
+
+    spec = importlib.util.spec_from_file_location(
+        "ttb_check_action_refs_test",
+        Path(__file__).resolve().parent.parent / "scripts" / "check_action_refs.py",
+    )
+    assert spec and spec.loader
+    module = importlib.util.module_from_spec(spec)
+    _sys.modules[spec.name] = module
+    spec.loader.exec_module(module)
+
+    seen: list[object] = []
+
+    def fake_urlopen(request, *a, **k):
+        seen.append(request)
+        raise urllib.error.URLError("not sent")
+
+    monkeypatch.setenv("GITHUB_TOKEN", SECRET)
+    monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen)
+    with pytest.raises(urllib.error.URLError):
+        module._get_json(url)
+    return seen[0]
+
+
+def test_the_api_host_gets_the_token(monkeypatch) -> None:
+    """The check has to still work, or the test below proves nothing."""
+    request = _captured_request("https://api.github.com/repos/o/r/contents/action.yml", monkeypatch)
+    assert request.get_header("Authorization") == f"Bearer {SECRET}"
+
+
+@pytest.mark.parametrize(
+    "url",
+    [
+        # Each of these contains the literal "api.github.com", and none of them
+        # IS api.github.com. A substring test sent the credential to all three.
+        "https://elsewhere.example/collect?next=api.github.com",
+        "https://api.github.com.evil.example/repos/o/r",
+        "https://evil.example/api.github.com/repos/o/r",
+    ],
+)
+def test_a_lookalike_host_never_gets_the_token(url: str, monkeypatch) -> None:
+    request = _captured_request(url, monkeypatch)
+    assert request.get_header("Authorization") is None, f"the token was sent to {url}"
+
+
+def test_pypi_does_not_get_a_github_token(monkeypatch) -> None:
+    """The same function reaches two hosts; only one of them is authenticated."""
+    request = _captured_request("https://pypi.org/pypi/tooltrace-bench/json", monkeypatch)
+    assert request.get_header("Authorization") is None