fix: jwt-guard double claim issue#66
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a reverse-lookup storage map Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/jwt-guards/jwt-guard/src/lib.rs (1)
326-328: Clean upjwt_hash_claimswhen accounts unregister.The
internal_storage_unregister()method removes entries fromjwt_claimsbut leaves the corresponding entries injwt_hash_claimsorphaned. When updating claims elsewhere in the code, both maps are properly maintained together (old hash is removed, new mappings are added). The unregister flow should mirror this pattern to prevent orphaned hash-to-account mappings from accumulating.
🤖 Fix all issues with AI agents
In `@contracts/jwt-guards/jwt-guard/src/lib.rs`:
- Around line 150-161: The claim_oidc method panics for first-time claimers
because it always calls internal_unwrap_jwt_claim() and removes the old hash;
change claim_oidc so it checks whether the account already has a claim (e.g.,
using self.jwt_claims.get(&account_id) or exist check) before calling
internal_unwrap_jwt_claim() and self.jwt_hash_claims.remove(...), and only
perform the removal when an existing claim is present; then proceed to insert
the new mapping into self.jwt_claims and self.jwt_hash_claims as currently done.
- Around line 175-185: The docstring for jwt_hash_claim_of is copy-pasted and
incorrect; update the comment above the function jwt_hash_claim_of to describe
that it retrieves the AccountId associated with a JWT hash, change the argument
name description to `hash: &Vec<u8>` (the JWT hash to look up), and update the
return description to `Option<AccountId>` (the AccountId if the hash exists,
otherwise None); ensure the short summary, `# Arguments` entry, and `# Returns`
entry reflect these exact types and semantics.
- Line 1: Remove the unused import "use std::ptr::hash;" from the top of the
file; the identifier "hash" referenced in internal_unwrap_jwt_claim is a local
variable and not the imported symbol, so deleting that import will eliminate the
unused import warning without affecting functionality.
🧹 Nitpick comments (1)
contracts/jwt-guards/jwt-guard/src/lib.rs (1)
216-228: Simplify existence check and fix docstring.The match expression can be simplified with
.is_some(), and the docstring is missing the function description.♻️ Suggested simplification
+ /// Checks if a JWT hash claim exists /// # Arguments /// * `hash` - The hash of the jwt claim to check for /// /// # Returns /// * `bool` - True if jwt hash claim exists, false otherwise /// fn exist_jwt_hash_claim(&self, hash: &Vec<u8>) -> bool { - match self.jwt_hash_claims.get(hash) { - Some(_) => true, - None => false - } + self.jwt_hash_claims.get(hash).is_some() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/jwt-guards/jwt-guard/src/lib.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T10:52:36.185Z
Learnt from: GuillemGarciaDev
Repo: Peersyst/fast-auth PR: 34
File: contracts/auth0-guard/src/lib.rs:67-76
Timestamp: 2025-09-02T10:52:36.185Z
Learning: The init signature changes in contracts/auth0-guard/src/lib.rs have already been properly handled in the integration tests by adding the issuer parameter to the args_json in both contracts/auth0-guard/tests/test_integration.rs and examples/jwt-rs256-guard/tests/test_integration.rs.
Applied to files:
contracts/jwt-guards/jwt-guard/src/lib.rs
📚 Learning: 2025-09-02T10:52:36.185Z
Learnt from: GuillemGarciaDev
Repo: Peersyst/fast-auth PR: 34
File: contracts/auth0-guard/src/lib.rs:67-76
Timestamp: 2025-09-02T10:52:36.185Z
Learning: The init signature changes in contracts/auth0-guard/src/lib.rs have already been properly handled in both test files - contracts/auth0-guard/tests/test_integration.rs includes issuer values like "https://dev-gb1h5yrp85jsty.us.auth0.com/" and examples/jwt-rs256-guard/tests/test_integration.rs includes issuer as an empty string variable in all init calls.
Applied to files:
contracts/jwt-guards/jwt-guard/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-packages / Build
- GitHub Check: build-base / Build
🔇 Additional comments (2)
contracts/jwt-guards/jwt-guard/src/lib.rs (2)
19-19: LGTM!The storage constant increase from 128 to 256 bytes appropriately accounts for the new bidirectional mapping overhead.
60-60: LGTM!The new
jwt_hash_claimsfield,JwtHashClaimsprefix variant, and initialization follow existing patterns correctly.Also applies to: 68-68, 91-91
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/jwt-guards/jwt-guard/src/lib.rs (1)
322-325: Remove stale entries fromjwt_hash_claimswhen unregistering an account.The
internal_storage_unregister()implementation removes the account fromjwt_claimsbut fails to clean up the corresponding entry injwt_hash_claims, breaking the bidirectional mapping consistency. The retrieved_jwt_hashshould be used to remove the entry:self.jwt_hash_claims.remove(&_jwt_hash);before returning.This pattern is already correctly implemented in
claim_oidc()(lines 155-159), which removes fromjwt_hash_claimswhenever updating the mapping. The unregister path must follow the same discipline.
♻️ Duplicate comments (1)
contracts/jwt-guards/jwt-guard/src/lib.rs (1)
149-160: Bug: First-time claims will panic.
internal_unwrap_jwt_claimon line 155 panics if the account has no existing claim (see lines 206-213). This breaks the first-time claim flow—only re-claims would work.The logic should conditionally remove the old hash only when the account already has a claim.
🐛 Proposed fix
pub fn claim_oidc(&mut self, oidc_token_hash: Vec<u8>) { assert_eq!(oidc_token_hash.len(), 32, "OIDC token hash must be 32 bytes"); let account_id = env::predecessor_account_id(); assert!(!self.exist_jwt_hash_claim(&oidc_token_hash), "OIDC token hash already claimed"); - let hash = self.internal_unwrap_jwt_claim(&account_id); - self.jwt_hash_claims.remove(&hash.clone()); + // Remove old hash mapping only if account already has a claim + if let Some(old_hash) = self.jwt_claims.get(&account_id).cloned() { + self.jwt_hash_claims.remove(&old_hash); + } self.jwt_claims.insert(account_id.clone(), oidc_token_hash.clone()); self.jwt_hash_claims.insert(oidc_token_hash, account_id); }
🧹 Nitpick comments (1)
contracts/jwt-guards/jwt-guard/src/lib.rs (1)
215-224: Missing short description in docstring.Line 215 has an empty doc comment. Add a brief description for consistency with other methods.
📝 Suggested fix
- /// + /// Checks if a JWT hash claim exists /// # Arguments /// * `hash` - The hash of the jwt claim to check for ///
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/jwt-guards/jwt-guard/src/lib.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T10:52:36.185Z
Learnt from: GuillemGarciaDev
Repo: Peersyst/fast-auth PR: 34
File: contracts/auth0-guard/src/lib.rs:67-76
Timestamp: 2025-09-02T10:52:36.185Z
Learning: The init signature changes in contracts/auth0-guard/src/lib.rs have already been properly handled in the integration tests by adding the issuer parameter to the args_json in both contracts/auth0-guard/tests/test_integration.rs and examples/jwt-rs256-guard/tests/test_integration.rs.
Applied to files:
contracts/jwt-guards/jwt-guard/src/lib.rs
📚 Learning: 2025-09-02T10:52:36.185Z
Learnt from: GuillemGarciaDev
Repo: Peersyst/fast-auth PR: 34
File: contracts/auth0-guard/src/lib.rs:67-76
Timestamp: 2025-09-02T10:52:36.185Z
Learning: The init signature changes in contracts/auth0-guard/src/lib.rs have already been properly handled in both test files - contracts/auth0-guard/tests/test_integration.rs includes issuer values like "https://dev-gb1h5yrp85jsty.us.auth0.com/" and examples/jwt-rs256-guard/tests/test_integration.rs includes issuer as an empty string variable in all init calls.
Applied to files:
contracts/jwt-guards/jwt-guard/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-base / Build
- GitHub Check: integration-packages / Build
🔇 Additional comments (5)
contracts/jwt-guards/jwt-guard/src/lib.rs (5)
18-18: LGTM!Storage constant correctly increased to account for the new bidirectional mapping overhead.
59-59: LGTM!Reverse-lookup map correctly typed for bidirectional claim tracking.
67-67: LGTM!Storage prefix correctly added for the new map.
90-90: LGTM!Initialization follows the same pattern as the existing
jwt_claimsmap.
174-184: LGTM!Docstring correctly updated and implementation properly returns the cloned account ID.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
[TA-XXXX]: Title
Changes 🛠️
app/package
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.