fix(launchpad): scope tracked-sensitive-files to what the cohort owns - #1984
fix(launchpad): scope tracked-sensitive-files to what the cohort owns#1984tucktuck101 wants to merge 1 commit into
Conversation
`audit` failed on `launchpad` trunk, and therefore on every branch cut after
2026-08-28, over six files nobody here wrote.
The check is not wrong about them. Four of the six genuinely contain private key
material:
apns-test-cert-only.pem CERTIFICATE
apns-test-encrypted-identity.pem ENCRYPTED PRIVATE KEY, CERTIFICATE
apns-test-identity.pem PRIVATE KEY, CERTIFICATE
apns-test-key-only.pem PRIVATE KEY
apns-test-mismatched-identity.pem PRIVATE KEY, CERTIFICATE
apple-app-attestation-root.pem CERTIFICATE
They arrived in `c432a111c` ("feat(mobile): push notifications MVP (block#6269)"),
which is an ancestor of `block/buzz@main`, and they are byte-for-byte identical
here. They are APNs test fixtures in upstream's public repository. This fork
operates the upstream product rather than developing it: it cannot fix them, and
it does not move or rename upstream paths.
So this narrows by OWNERSHIP, not by exemption. An allowlist would be a standing
hole, and this check deliberately has none — a suffix exemption was removed after
one let a tracked `seed/authorized_keys.example` through, which is the shape #68
documents a real past incident for. Adding the mechanism back for these six would
undo that.
IDENTITY, NOT ORIGIN. `security_audit_classifier.classify` already answered "did
this path exist upstream", and that is not enough: an inherited filename is
exactly where cohort-added key material could hide. `divergence()` compares blob
OIDs and returns one of `fork-added`, `inherited-modified`, `inherited-identical`
or `indeterminate`. Only `inherited-identical` licenses a skip; the other three
are cohort-accountable or unknown.
The OID comes from the same single `git fetch` the classifier already performed —
`git ls-tree` without `--name-only` is one column wider and costs no extra
network. `fetch_upstream_paths` and `classify` are untouched, so this is additive.
It is also the classifier's first caller: it was built for #62 and never wired.
Unreachable upstream reports INDETERMINATE, naming the paths — neither PASS nor
FAIL is honest when ownership cannot be established, and INDETERMINATE is visibly
not-green so it cannot be mistaken for clean. Upstream-identical hits are named
in the PASS detail: a skip nobody can see is indistinguishable from a check that
stopped looking.
The pre-existing controls now patch the upstream fetch rather than reaching the
network to decide something their own fixtures already know. Four controls added:
upstream-identical does not fail and is named; a cohort-added key still fails
alongside upstream fixtures; an inherited path modified here still fails;
unreachable upstream is INDETERMINATE, not PASS.
Verified against this repository:
status : pass
detail : no cohort-owned tracked file matches a sensitive shape (5718 tracked
files checked); 6 sensitive-shaped file(s) are byte-for-byte
upstream's and not cohort-owned: crates/buzz-push-gateway/tests/
fixtures/apns-test-cert-only.pem; ... apple-app-attestation-root.pem
Tests: 88 security-audit tests pass (14 in this check's own suite, 4 new).
Closes #1965
Fixture content is deliberately not key-shaped: the check matches on path shape
and never reads content, and an earlier revision that used a real PEM header
made `gitleaks-secret-scan` fail on two of its own lines. A control for a
secret-detection suite must not itself be the thing it detects. That revision
lived on `audit/1965-upstream-owned-fixtures`; because the PR-scoped gitleaks
scan reads the commits a branch adds rather than its tip, and force-pushing is
barred, this branch carries the work as one clean commit instead.
Signed-off-by: tucktuck101 <jeffreytaylorrobertson@gmail.com>
tucktuck101
added
bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed commit 3a7e7b6c5958c7a45a8ef5c6b14f6fcf01cdfd73 against merge base 1ed55e980b0043f92d9c652e6a39a8e49345389c.
Incomplete
This review is INCOMPLETE and must not be read as a full pass:
- no dimension was actually reviewed: the pipeline ran the 'default_reviewer' stub reviewer, which reports every dimension clean without reading it (a real dimension reviewer is #116)
Containment
No containment findings.
Fetched and empty: pr_issue_comments, pr_review_bodies, pr_review_comments.
Automated containment covers the delimiter boundary and unambiguous injection tells only. It does not cover injection phrased as ordinary, unremarkable prose. The absence of a containment finding is not evidence that this pull request contains no injection attempt.
Authority — recorded, and partially declinedInstruction given this session by @tucktuck101, quoted verbatim, transcription noise intact:
Acknowledgement, as requested: @tucktuck101 states he takes responsibility for any fixes required as a result of merging these pull requests without a second reviewer. What I did not do, and why. I have not used
ADR-0052 part G says the same: "Delegated authority never bypasses CI or any rule GitHub enforces." The delegated-authority framework exists precisely so a human's instruction lets an agent act inside the gates; it explicitly does not extend to acting around them, and it anticipates the capability being available. The actual gate, for the record. What is available instead, in preference order:
Nothing here is a judgement about the change. It is only about who may open the gate and how. |
1 participant
Summary
auditfailed onlaunchpadtrunk, and so on every branch cut after 2026-08-28, over six files nobody in this cohort wrote. The check is not wrong about them — four genuinely contain private key material — but they are byte-for-byte upstream's APNs test fixtures, and this fork cannot fix them. This narrows the check by ownership rather than adding an exemption, because the check deliberately has no exemption mechanism and an earlier one caused a real miss.Feature
N/A - single-issue PR
Related issue
Closes #1965
Issue type
Bug
Agent provenance
Objective
A
tracked-sensitive-filescheck that passes onlaunchpadtrunk while still failing on any sensitive-shaped file this cohort added or modified.Impacted components
Approach and rejected alternatives
The six files, classified by content — headers only, no key material printed:
They arrived in
c432a111c("feat(mobile): push notifications MVP (block#6269)"), an ancestor ofblock/buzz@main, and are byte-for-byte identical here (git rev-parseon fork and upstream returns the same OIDs).block/buzzis public, so they have always been public — consistent with deliberately generated APNs test fixtures.Rejected: an allowlist or path exemption. This was my first instinct and it is wrong.
_matches_sensitive_shapehas no exemption mechanism on purpose: a*.examplesuffix exemption was removed after it let a trackedseed/authorized_keys.examplethrough, which is exactly the shape #68 documents a real past incident for. Re-adding the mechanism for these six would undo a control that was hardened deliberately.Rejected: match on content instead of filename. Would not help — four of the six really do contain
PRIVATE KEYblocks, so a content rule still fires.Rejected: untrack them, or generate them at test time. Both edit upstream-owned test code, which this fork does not do.
Rejected: raise it upstream only. Arguably correct owner, and worth doing separately, but it leaves trunk red indefinitely.
Chosen: scope by ownership. A fork that operates the upstream product is accountable for what it wrote, not for what it inherited unchanged.
divergence()returnsfork-added,inherited-modified,inherited-identicalorindeterminate, and onlyinherited-identicallicenses a skip.Identity, not origin — this is the part that matters.
classify()already answered "did this path exist upstream", and that is not sufficient: an inherited filename is precisely where cohort-added key material could hide.divergence()compares blob OIDs, so a path present upstream but modified here still fails. The OID comes from the same singlegit fetchthe classifier already ran —git ls-treewithout--name-onlyis one column wider and costs no extra network.fetch_upstream_pathsandclassifyare untouched, so the change is additive. Incidentally this is the classifier's first caller: it was written for #62 and never wired to anything.Unreachable upstream reports INDETERMINATE, naming the paths. Neither PASS nor FAIL is honest when ownership cannot be established, and INDETERMINATE is visibly not-green so it cannot be read as clean. Upstream-identical hits are named in the PASS detail — a skip nobody can see is indistinguishable from a check that stopped looking.
Verification
Command run:
Raw output:
The negative control is the acceptance criterion demonstrated rather than asserted: with the six upstream fixtures present and correctly excluded, one cohort-added key still fails the run.
Not verified
security_audit.pyharness was not run to completion locally. It downloads and checksum-verifies a pinned gitleaks binary and exceeded a 600s timeout on this machine. Only the check I changed was run against the repository, plus all 88 unit controls. CI on this PR is the first end-to-end run.fetch_upstream_blobsis unproven. The workflow already usesfetch-depth: 0because the classifier fetchesblock/buzz, but no job has ever exercised that fetch — this is the classifier's first caller, so its behaviour on a runner is untested.apns-test-*, live in a test fixtures directory, and are public in upstream — but nothing here proves they are unused elsewhere.fork-addedat its old path and would fail. No such case exists today.git ls-treeparsing assumes the standard<mode> <type> <oid>\t<path>shape. Malformed entries are skipped rather than raising, which is a silent-ish failure mode.Authority
N/A - no approval or merge performed by an agent; this PR is submitted for human review.
Deferred blockers
none
Security implications
This narrows a security control, so it deserves the scrutiny. What it gives up: a sensitive-shaped file that is byte-for-byte identical to upstream's no longer fails. What it keeps: everything the cohort added (
fork-added), everything the cohort changed (inherited-modified), and everything whose ownership cannot be established (indeterminate, which is not-green). The negative control above proves a cohort-added key still fails with the upstream fixtures present.The alternative was an exemption list, which would have been a permanent hole matched by name rather than by content identity — strictly weaker than this, and against the documented intent of the function it would have modified.
One genuine new dependency: correctness now rests on
git fetchof a public upstream reaching the network. Failure is handled as INDETERMINATE rather than PASS, so a network outage cannot silently turn the check green.Escalations
This PR replaces fix(launchpad): scope tracked-sensitive-files to what the cohort owns #1983. That branch's first commit used a real
-----BEGIN PRIVATE KEY-----header as fixture content, whichgitleaks-secret-scancorrectly flagged. The PR-scoped scan reads the commits a branch adds, not its tip, so removing the header in a follow-up commit did not clear it — and force-pushing is barred. The work is carried here as one clean commit. fix(launchpad): scope tracked-sensitive-files to what the cohort owns #1983 is closed as superseded, not abandoned.Upstream arguably should not ship private-key-shaped test fixtures, even generated ones, and this PR does not tell them so. Worth a separate upstream report; I have not filed one because that is a decision about how this cohort talks to
block/buzz, not a code fix.I recommended the allowlist approach first and it was wrong. The check's own comments document why exemptions were removed, and I proposed re-adding the mechanism before reading them. Recording it because the reasoning matters more than the outcome: a security control's history is part of its specification.
Four of the six files really do contain private keys. If anyone believes one of them could be a real credential rather than a fixture, this stops being a CI problem and becomes an upstream security report, and this PR should be held.