fix(portal): canonicalize nullifier_hash in OIDC authorize (H1 #3896406) - #2162
Open
Gr1dlock wants to merge 2 commits into
Open
fix(portal): canonicalize nullifier_hash in OIDC authorize (H1 #3896406)#2162Gr1dlock wants to merge 2 commits into
Gr1dlock wants to merge 2 commits into
Conversation
POST /api/v1/oidc/authorize used the raw nullifier_hash for the nullifier lookup, insert, OIDC auth code, and id_token sub/email, while verifyProof normalizes the same value internally before verification. Case/prefix/leading-zero re-encodings therefore passed proof verification but produced distinct downstream identities, letting one verified human mint multiple stable OIDC subjects and create sibling nullifier rows (RP-side Sybil). Now canonicalizes with canonicalizeNullifierHash right after verifyProof succeeds and uses the canonical value for every identity-forming sink, mirroring the v2/verify handler. Fixes HackerOne #3896406. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The integration suite stubbed the entire @/api/helpers/verify module with only verifyProof, so the handler's new canonicalizeNullifierHash call resolved to undefined and threw a 500 (Integration Tests CI failure). Spread the real module and mock only verifyProof (the sequencer I/O call) so pure helpers stay real and future exports don't break this again. Co-Authored-By: Claude Opus 4.8 (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
1 participant
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.
PR Type
Description
POST /api/v1/oidc/authorizepassed the raw, client-suppliednullifier_hashinto the nullifier lookup, the nullifier insert, the OIDC auth code, and the id_tokensub/email, whileverifyProofnormalizes the same value internally (decodeToHexString) before verification — so case /0x-prefix / leading-zero re-encodings of one nullifier all passed proof verification but produced byte-distinct downstream identities, letting a single orb-verified human mint multiple stable OIDC subjects for the same app and create sibling rows inpublic.nullifier(RP-side Sybil). This PR canonicalizes the nullifier with the existingcanonicalizeNullifierHashhelper immediately afterverifyProofsucceeds and uses the canonical value for every identity-forming sink, exactly as the v2/verify handler already does. As a side effect it also closes a concurrency race: two re-encodings submitted with different proofs now collide on the byte-exactunique_nullifier_hashconstraint instead of inserting sibling rows. The change is safe for existing data becausecanonicalizeNullifierHashis idempotent on the standard0x+64-hex form IDKit emits and already stores. Fixes HackerOne #3896406.Checklist
🤖 Generated with Claude Code