Repository navigation
Promote to main: /assert authData parse fix (cherry-pick of #210) - #211
Merged
Merged
Conversation
…ta layout (#210) parseAuthenticatorData decided whether attested credential data followed by testing the AT flag (0x40). Apple sets AT on ASSERTION authData too, where the buffer ends at 37 bytes right after the counter, so the parse walked past the end of every real assertion: assertion verification failed: The value of "offset" is out of range. It must be >= 0 and <= 35. Received 53 verifyAssertion therefore threw for every device, /assert answered 400, and no counter ever advanced: devices.sqlite holds 1807 rows, all at counter 0. Clients fell back to re-attesting on each launch, against Apple's per-key attestation rate limit, and the replay check (counter must strictly increase) has never rejected anything because it was never reached. The layout is now chosen by the caller, which knows which message it holds, rather than inferred from a flag that does not distinguish them. Attestation passes { attested: true }; assertion takes the default. Preferred over a `buf.length > 37` test so that a genuinely truncated attestation still fails loudly instead of silently parsing as an assertion. Adds the two bounds checks the attested branch lacked (room for the AAGUID and length prefix, and a credential id that fits), both reachable from untrusted input. Two test gaps let this ship green, and both are closed: - test/webauthn.test.ts's helper set AT only when it also appended attested credential data, so the combination Apple actually sends was unconstructable. It now takes a `flags` override, and a case asserts a real 37-byte AT-set assertion parses without overrunning. - test/appAttest.test.ts's end-to-end verifyAssertion fixture generates a real P-256 key and a valid signature, but built authData with AT CLEAR — which no Apple assertion does. It now sets AT before signing, so that path covers the shape production actually receives. Verified by reverting both source files to HEAD with the tests in place: 5 tests fail with the production RangeError. With the fix, tsc --noEmit is clean and 42/42 pass. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Cherry-pick of #210 (staging 14487bf) onto
main. It's a server-only fix, so it doesn't wait on an app release.Production catalog-server has already been running this fix since Oct 5, built from the same change as a local commit on the NAS host. Until this lands, rebuilding catalog-server from
mainwould quietly bring the/assertfailure back. Merging it makes the repo match what's deployed.Checked on this branch:
tsc --noEmitis clean, and vitest passes 42/42.🤖 Generated with Claude Code