Grade a note by everyone who wrote the note - #410
Merged
Conversation
CommitLore — record lintTrailers: clean — 1 commit in Active constraints for the paths this PR touchesLimits (92)
Ruled out (213)
Warnings (50)
Truncated: 35 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
A record stored in `refs/notes/commitlore` was graded using the annotated commit's author. The notes ref is an ordinary ref, so anyone who could push it could attach arbitrary text to a commit written by a trusted author and have it served as `[directive]` -- the grade the plugin instructions tell an agent to treat as a constraint. The commit author never wrote the record and could not see it in their own message. `noteAuthorsOf` walks the notes ref and attributes each note to the identities that wrote it. A note git cannot attribute has no entry, and a record with no known author already grades `claim`. Every writer, not the latest. Attributing a note to its most recent writer was the first fix here, and it holds the forgery open one merge further along: `git notes merge -s cat_sort_uniq` concatenates two writers' notes into a single blob, and the newest commit touching that blob is whichever writer went second. Where the trusted writer goes second, the attacker's text in the same blob inherits their grade. Checked against git rather than reasoned about, then pinned by a test that was confirmed to fail against the latest-writer rule. The control matters as much as the fix: trusting a note's real author must still produce `directive`. Refusing to trust notes at all would pass a test named after this bug while breaking the mirror for every repository that uses it, so the test asserts both directions. `query.ts` and `inject.ts` each carried their own copy of the per-declaration grading loop -- the situation `query.ts`'s own comment warns about, that two implementations of one policy is one implementation and one hole. This fix would otherwise have had to be written twice, and the merge case corrected twice. The loop is now `gradeDeclarations` in `grade.ts` and both routes call it. Limit: the notes ref is an ordinary ref with no signature requirement, so authorship there is a claim about who wrote the text and not proof of it Ruled-out: refusing to grade any notes-sourced record above claim | it passes the same regression test while breaking the notes mirror for repositories that legitimately use it, which is why the control case asserts that a note by a trusted author still grades directive Ruled-out: attributing a note to its most recent writer | a cat_sort_uniq notes merge puts two writers' text in one blob, so the later writer's grade would cover the earlier writer's content -- verified against git, and the test for it fails against this rule Ruled-out: requiring a signature on the notes ref before trusting it | commit authorship is not signature-verified either, so demanding it of notes alone sets an inconsistent bar and is a much larger change than the forgery needs Warn: a note is now graded against every identity that has written it and keeps the floor, so one untrusted writer anywhere in a note's history holds that note at claim Blast: system Undo: easy Certainty: firm Verified: a RED test reproduced the issue's table on both consumer routes before the fix and passes after it, including the control and the merge case, and the merge case was re-run against the latest-writer rule to confirm it fails there Provenance: authored Record-Id: r-note409
MongLong0214
force-pushed
the
fix-issue-409
branch
from
August 7, 2026 00:15
7cb6184 to
0dff3e4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #409.
What was wrong
A record in
refs/notes/commitlorewas graded by the annotated commit'sauthor. The notes ref is an ordinary ref, so anyone able to push it could attach
text to a trusted author's commit and have it served as
[directive]— thegrade the plugin instructions tell an agent to treat as a constraint. The commit
author never wrote it and cannot see it in their own message.
The fix
noteAuthorsOfwalks the notes ref and attributes each note to the identitiesthat actually wrote it.
gradeDeclarationsgrades each declaration by whoeverwrote that declaration and keeps the floor — the rule the module already
applies across commits, extended to the axis this issue exposed.
Reproducing the issue's table, on both consumer routes:
--trusted-authortrusted@corpdirectiveclaimattacker@evilclaimdirectivenobody@nowhereclaimclaimThe middle row is the control. Refusing to trust notes at all would pass a test
named after this bug while breaking the mirror for repositories that use it, so
the test asserts both directions.
A second hole, found while verifying the first
The first version of this fix attributed a note to its most recent writer.
That was going to ship as an
Unverified:note about notes-ref merges. Checkingit against git instead of reasoning about it showed the forgery survives:
git notes merge -s cat_sort_uniqconcatenates two writers' notes into oneblob, and the newest commit touching that blob is whichever writer went second.
So wherever the trusted writer goes second, the attacker's text in the same blob
inherits their grade. A note is therefore graded against every identity that
has written it, keeping the floor. The regression test for this was re-run
against the latest-writer rule and confirmed to fail there.
Also in this change
query.tsandinject.tseach carried their own copy of the per-declarationgrading loop — the situation
query.ts's own comment warns about, that twoimplementations of one policy is one implementation and one hole. Without
merging them this fix, and then the merge correction, would each have had to be
written twice. Both routes now call
gradeDeclarations.Cost, stated plainly
A note is held at
claimif any writer in its history is untrusted, and amirrored record is graded on both its authorships. A notes mirror written by a
bot identity therefore grades
claimuntil that identity is listed as a trustedauthor. That is the fail-closed direction and it appears in the record's reason,
but it is a behaviour change for anyone running the mirror under a bot.
Verification
test/grade-notes-author.test.ts— 5 cases, red before the fix and greenafter, including the control and the merge case.
spec/verify.sh: 26 fixtures + README example sync + vocab table OK.npm run typecheck,tsc -p bench/tsconfig.json --noEmit,npm run buildwith
dist/committed,npm run bench:verify,scripts/check-readme-numbers.mjs,dist/cli.js validate --range,dist/cli.js doctor.