Skip to content

test(credential): make the binding tests fail when binding is removed - #360

Open
Jaro-c wants to merge 1 commit into
developfrom
test/bind-tests-that-can-fail
Open

Jaro-c wants to merge 1 commit into
developfrom
test/bind-tests-that-can-fail

Conversation

@Jaro-c

@Jaro-c Jaro-c commented Sep 8, 2026

Copy link
Copy Markdown
Member

The four tests named for purpose and subject binding asserted nothing about binding. Each called Issue once per purpose or subject, so the two tokens were independent CSPRNG draws and the hashes differed because the tokens differed. Deleting writeField(mac, purpose) from computeHash left all four green.

test with purpose dropped, before
TestIssue_DifferentPurposesDifferentHashes pass
TestIssue_DifferentSubjectsDifferentHashes pass
TestVerify_WrongPurposeRejected pass
TestVerify_WrongSubjectRejected pass

The control was defended, but by accident. The sabotage failed TestComputeHash_LengthPrefixSurvivesEmbeddedNULs and one seed of FuzzVerify, two tests about NUL handling and length prefixing that happen to hold purpose and subject fixed. Narrowing either one would have removed the only real coverage while every test still carrying the word "binding" stayed green.

What changed

The Issue tests now compare computeHash directly with the token held fixed, so exactly one field varies and a pass is evidence about that field.

The Verify tests are written as the attack they are named for: one real credential replayed into another flow. Same token, same stored hash, only the purpose or the subject different. Presenting a different token against a different hash, which is what they did, fails on the token alone and proves nothing.

Each Verify test now also asserts the credential verifies under its own purpose and subject before asserting the cross-flow rejection. Without that, a Verify that rejected everything would pass.

Measured after

Dropping purpose from computeHash fails the two purpose tests and leaves the subject tests green. Dropping subject does the reverse. Each test is now about the field it is named after, and the sabotages compiled in both directions.

Also removed a stray _ = time.Now() and its now-unused import.

Provenance

Found by a MiniMax-M3 pass over the package. It predicted both halves before anything was run: that all four tests would stay green, and that the NUL tests would catch the sabotage incidentally. Both held.

Closes #358

The four tests named for purpose and subject binding asserted nothing
about binding. Each called Issue once per purpose or subject, so the two
tokens were independent CSPRNG draws and the hashes differed because the
tokens differed. Deleting writeField(mac, purpose) from computeHash left
all four green.

The control was defended, but by accident: the sabotage failed
TestComputeHash_LengthPrefixSurvivesEmbeddedNULs and one seed of
FuzzVerify, two tests about NUL handling that happen to hold purpose and
subject fixed. Narrowing either would have removed the only real
coverage with every test still named "binding" staying green.

The Issue tests now compare computeHash directly with the token held
fixed, so exactly one field varies. The Verify tests are written as the
attack they are about: one real credential replayed into another flow,
same token and same stored hash, only the purpose or the subject
different. Presenting a different token against a different hash, which
is what they did, fails on the token alone.

Each Verify test also asserts the credential verifies under its own
purpose and subject first, so a Verify that rejected everything cannot
pass them.

Measured after: dropping purpose fails the two purpose tests and leaves
the subject tests green, and dropping subject does the reverse. Each
test is now about the field it is named after.

Also removed a stray `_ = time.Now()` and its import.

Closes #358

Signed-off-by: Jose <75870284+Jaro-c@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant