fix(store): use T::KIND in Alias::scope instead of hardcoded PublicKey::KIND#3002
Conversation
…y::KIND Alias::scope gated on the hardcoded PublicKey::KIND, so it only decoded the scope for PublicKey aliases and returned None for ContextId and ApplicationId aliases regardless of the type parameter T. Use T::KIND so the kind check matches the requested alias type, mirroring Alias::alias. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kb2bva1xjc8NrRcQ5FVbd8
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 1 agents | Quality score: 32% | Review time: 46.6s
💡 Suggestion (1)
1. No unit tests for Alias::scope covering the fixed bug
File: crates/store/src/key/alias.rs (line 183-200) | Consensus: 1/1 agents ✓
The bug fix changes bytes[0] == PublicKey::KIND to bytes[0] == T::KIND, which means scope::<ContextId>() and scope::<ApplicationId>() previously always returned None (since their KIND values 1 and 3 were compared against PublicKey::KIND = 2). There are no tests in this file (or found in the store crate) that exercise Alias::scope for any type other than PublicKey, so the regression that existed before this fix was undetected and the fix itself is unverified. A test that constructs an Alias::new::<ContextId>(...) and then calls .scope::<ContextId>() (and asserts it is Some) plus a cross-kind check (.scope::<PublicKey>() returns None) would pin the correct behaviour.
Suggested fix:
Add a `#[cfg(test)] mod tests` block in `alias.rs` with at least: (1) round-trip test for each `Aliasable` type via `Alias::new` + `.scope::<T>()`, and (2) a cross-kind test asserting `scope::<WrongType>()` returns `None`.
Found by: security-reviewer
🤖 Generated by MeroReviewer | Review ID: review-e772d24c
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 32% | Review time: 301.2s
💡 Suggestion (1)
1. No tests cover the fixed scope() method for non-PublicKey types
File: crates/store/src/key/alias.rs (line 183-200) | Consensus: 1/1 agents ✓
The PR description states "Existing tests should cover this change", but there are no #[test] blocks in alias.rs and no call sites for Alias::scope found anywhere in the codebase. The bug (hardcoded PublicKey::KIND) would have been caught immediately by a test that calls scope::<ContextId>() or scope::<ApplicationId>() on an alias created for those types. Without such a test the regression can silently reappear.
Suggested fix:
Add unit tests in `alias.rs` that: (1) create an `Alias::new::<ContextId>(...)`, call `.scope::<ContextId>()` and assert `Some(_)`, then call `.scope::<PublicKey>()` and assert `None`; (2) repeat for `ApplicationId`. This directly exercises the fixed line and guards against future regressions.
Found by: security-reviewer
🤖 Generated by AI Code Reviewer | Review ID: review-acf12d5f
Description
Fixed a bug in
Alias::scope()where the scope kind check was hardcoded toPublicKey::KINDinstead of using the generic type parameterT::KIND. This caused the method to incorrectly validate aliases for non-PublicKey types.The change allows the method to properly work with any
Aliasabletype by checking against the correct kind for the generic type being used.Test plan
Existing tests should cover this change. The fix ensures that
Alias::scope()correctly validates aliases for allAliasabletypes, not justPublicKey.Wire contract (SDK gate)
N/A - This is an internal store API change with no wire contract implications.
Documentation update
N/A
https://claude.ai/code/session_01Kb2bva1xjc8NrRcQ5FVbd8