-
Notifications
You must be signed in to change notification settings - Fork 211
Fix/jwt output #1452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/jwt output #1452
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors the GCP JWT verifier circuit to use packed byte representations for outputs, extends the preprocessing logic to handle a second eat_nonce field extracted from the payload, and refines JSON field extraction parsing to correctly handle quoted values terminated by either closing brackets or commas. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // https://cloud.google.com/confidential-computing/confidential-space/docs/connect-external-resources | ||
| // EAT nonce (payload.eat_nonce[0]) | ||
| var MAX_EAT_NONCE_B64_LENGTH = 99; // Max length for base64url string (74 bytes decoded = 99 b64url chars) | ||
| var MAX_EAT_NONCE_B64_LENGTH = 74; // Max length for base64url string (74 bytes decoded = 99 b64url chars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect base64 length constant truncates valid nonces
The MAX_EAT_NONCE_B64_LENGTH was changed from 99 to 74, but the variable represents base64url character length, not decoded byte length. The comment even states "74 bytes decoded = 99 b64url chars". This causes the circuit to reject valid nonces with base64 lengths between 75-99 characters. The prepare.ts file still uses 99, creating an inconsistency. The length check at line 167 and the ExtractAndVerifyJSONField template instantiation will fail for full-length nonces that are valid per the GCP spec.
Additional Locations (1)
| component is_value_after_quote = IsEqual(); | ||
| is_value_after_quote.in[0] <== char_at_plus2; | ||
| is_value_after_quote.in[1] <== value_start; | ||
| is_quote.out * (1 - is_value_after_quote.out) === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing value_offset constraint allows arbitrary value extraction
The constraint value_offset === expected_value_offset was removed, which previously enforced that the extracted value position matched the expected location relative to the verified key. For the array format (:["value"]), there's now no constraint linking value_offset to the key position. A malicious prover could provide a key_offset pointing to the correct key while using a value_offset pointing to a different valid-looking value elsewhere in the JSON, effectively extracting an unintended value while passing verification.
|
|
||
| const eatNonce1Base64url = payload.eat_nonce[1]; | ||
| console.log(`[INFO] eat_nonce[1] (base64url): ${eatNonce1Base64url}`); | ||
| console.log(`[INFO] eat_nonce[1] string length: ${eatNonce1Base64url.length} characters`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing array length check before accessing second nonce
The code accesses payload.eat_nonce[1] at line 284 without verifying the array has at least 2 elements. The existing validation at line 240 only checks payload.eat_nonce.length === 0, not length < 2. If a JWT contains only one nonce in the eat_nonce array, eatNonce1Base64url will be undefined, and accessing .length on line 286 will throw a TypeError and crash the preparation script.
Note
Packs
eat_nonce[0]and image hash outputs, tightens nonce length handling, improves JSON array/value parsing, addseat_nonce[1]support in prepare, and introduces a GCP JWT test script.gcp_jwt_verifier.circom):PackBytesforeat_nonce[0]andimage_hash.MAX_EAT_NONCE_B64_LENGTHto74and use it in max-length check; compute packed chunk sizes.@openpassport/zk-email-circuits/utils/bytes.circom.verifyJSONFieldExtraction.circom)::with a quote or with:["; validatevalue_offsetvia first value char.]or,after the closing quote.prepare.ts):eat_nonce[1]; emiteat_nonce_1_b64_length.test-gcp-jwt-verifierscript inpackage.json.Written by Cursor Bugbot for commit 32a9dc7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.