Contract: Normalize session expiry units at input boundaries#336
Contract: Normalize session expiry units at input boundaries#336Abolax123 wants to merge 7 commits into
Conversation
Cover high session-key storage growth behavior and validate instance TTL threshold policy across read/write paths, plus document expected limits and rejection behavior for extreme inputs. Made-with: Cursor
Verify storage unlock results before transitioning to authenticated state, keep locked state on invalid credentials, and add regression tests for lock-manager and router invalid-password paths. Made-with: Cursor
Add explicit coverage that non-owner upgrade attempts are rejected and that owner-authorized upgrades still succeed with version increment behavior preserved. Made-with: Cursor
Define an explicit seconds-vs-milliseconds normalization rule, normalize expiry once in add/refresh paths, and add regression tests for second and millisecond inputs including legacy refresh normalization. Made-with: Cursor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR transitions wallet unlock from synchronous to async, integrating Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as UnlockScreen
participant AuthCtx as AuthGuard (Context)
participant StorageMgr as SecureStorageManager
participant Storage as Encrypted Storage
User->>UI: Enter password & submit
UI->>UI: Clear prior error
UI->>AuthCtx: await unlockWallet(password)
AuthCtx->>StorageMgr: unlock(password)
StorageMgr->>Storage: Validate & decrypt
alt Valid password
Storage-->>StorageMgr: Success (true)
StorageMgr-->>AuthCtx: Promise<true>
AuthCtx->>AuthCtx: Set isUnlocked=true, hasOnboarded=true
AuthCtx-->>UI: Promise<true>
UI->>UI: Navigate to Home
else Invalid password
Storage-->>StorageMgr: Fail (false)
StorageMgr-->>AuthCtx: Promise<false>
AuthCtx->>AuthCtx: Set isUnlocked=false
AuthCtx-->>UI: Promise<false>
UI->>UI: Display error: "Invalid password"
UI->>UI: Remain on Unlock screen
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 13 minutes and 56 seconds.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/account/src/lib.rs (1)
268-271:⚠️ Potential issue | 🟠 Major
session_key_addedevent still publishes the raw, non-normalizedexpires_at.The session key is stored with
normalized_expires_atat line 264, but the event payload at line 270 uses the original caller-providedexpires_at. As a result, indexers/SDK consumers subscribing tosession_key_addedwill observe a millisecond timestamp (e.g.1_735_689_600_000) whileget_session_key()returns the seconds value (1_735_689_600). That breaks the "canonical seconds representation" property the PR is trying to establish and silently corrupts off-chain analytics that rely on the documented event schema inREADME.md(line 185) which declaresexpires_atas the expiration timestamp.🐛 Proposed fix
// Emit session_key_added event env.events() - .publish((events::session_key_added(&env),), (public_key, expires_at)); + .publish( + (events::session_key_added(&env),), + (public_key, normalized_expires_at), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 268 - 271, The event publish uses the caller-provided expires_at instead of the stored normalized_expires_at; change the env.events().publish call that emits events::session_key_added to pass normalized_expires_at (the value saved with the session key) rather than the original expires_at so subscribers and get_session_key() see the same canonical seconds timestamp; update the tuple arguments to (public_key, normalized_expires_at) where events::session_key_added is invoked.
🧹 Nitpick comments (7)
apps/extension-wallet/src/router/index.tsx (2)
260-269: No pending guard around the async unlock — allows duplicate submissions.
handleUnlockawaitsunlockWallet(password)but the submit button stays enabled while in flight. If the underlyingstorage.unlockdoes any non-trivial work (KDF, async decryption), the user can press Enter or click Unlock multiple times and trigger overlapping unlock attempts. This is mostly cosmetic, but with strong KDF parameters it can stack expensive operations and obscure error feedback.♻️ Suggested fix: track a pending flag
const [password, setPassword] = React.useState(''); const [error, setError] = React.useState<string | null>(null); + const [pending, setPending] = React.useState(false); const from = (location.state as { from?: string } | null)?.from ?? '/home'; async function handleUnlock(event: React.FormEvent<HTMLFormElement>) { event.preventDefault(); + if (pending) return; setError(null); - const didUnlock = await unlockWallet(password); - if (!didUnlock) { - setError('Invalid password. Please try again.'); - return; + setPending(true); + try { + const didUnlock = await unlockWallet(password); + if (!didUnlock) { + setError('Invalid password. Please try again.'); + return; + } + navigate(from, { replace: true }); + } finally { + setPending(false); } - navigate(from, { replace: true }); }And on the submit button:
- <PrimaryButton disabled={!password.trim()} type="submit"> - Unlock + <PrimaryButton disabled={!password.trim() || pending} type="submit"> + {pending ? 'Unlocking…' : 'Unlock'} </PrimaryButton>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/index.tsx` around lines 260 - 269, The handleUnlock function allows concurrent submissions; add an isPending (or isUnlocking) React state and guard to prevent duplicate submits: in handleUnlock (and related unlockWallet call) return early if isPending is true, set isPending = true immediately before awaiting unlockWallet(password), and set isPending = false in a finally block so it always resets; also use isPending to disable the submit button (and optionally the form) so the UI cannot trigger overlapping unlock attempts and to provide proper feedback while unlockWallet runs.
282-288: Stale error stays visible while user retypes.Once
erroris set, it remains rendered until the next submit. Clearing it on input change avoids the awkward "still showing Invalid password while I type the right one" state.♻️ Suggested fix
<input className="mt-2 w-full rounded-xl border border-border bg-background px-3 py-3 text-sm outline-none transition focus:border-primary" - onChange={(event) => setPassword(event.target.value)} + onChange={(event) => { + setPassword(event.target.value); + if (error) setError(null); + }} placeholder="Enter your password" type="password" value={password} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/index.tsx` around lines 282 - 288, The password input currently calls setPassword onChange but leaves the existing error visible; update the onChange handler for the password input (the element using value={password} and setPassword) to also clear the form error (call setError('') or setError(null)/setError(undefined) depending on how error state is typed) before or after setPassword so the "Invalid password" message is removed as the user types.apps/extension-wallet/src/router/AuthGuard.tsx (2)
110-116: Silently swallowing unlock errors hurts diagnosability.
catch {}collapses every failure mode (wrong password, decryption error, storage adapter throw, async I/O failure) into the same "invalid password" UI inUnlockScreen. For non-credential errors this is misleading and you lose any signal in the console.Suggest at minimum logging at
warnso operational issues (corrupt storage, adapter failure) are surfaced during debugging:♻️ Proposed change
- } catch { + } catch (err) { + // eslint-disable-next-line no-console + console.warn('[auth] storage.unlock threw', err); setAuthState((current) => ({ ...current, isUnlocked: false, })); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/AuthGuard.tsx` around lines 110 - 116, The catch block in AuthGuard.tsx that calls setAuthState(...) is swallowing all errors silently; change the anonymous catch to capture the error (e.g., catch (err)) and log it at warn level before setting isUnlocked:false so operational issues are visible; update the catch in the unlock flow where setAuthState is used (reference: setAuthState in AuthGuard.tsx / UnlockScreen unlock handling) to call your existing logger.warn or console.warn with a clear message plus the error object, preserving the current state update and return false behavior.
30-37: Module-level singleton — acceptable, but consider lifecycle.The lazy module-level
storageManagerworks fine in the extension runtime (single JS context) and is correctly intercepted by the test mock. Two minor caveats worth documenting or rethinking:
- The instance survives React unmounts and HMR reloads — if you ever introduce per-account or per-profile storage adapters, this singleton becomes a bug.
- Tests that want to fully reset SDK state across test cases will need
vi.resetModules()in addition tomockReset(), since the singleton holds the sameMockSecureStorageManagerinstance for the file's lifetime. Today this is fine because state lives onmockUnlock/mockLock, but it's a subtle coupling.If a per-component instance is acceptable,
React.useState(() => getStorageManager())[0](or just calling the getter inline) makes the lifecycle explicit and theuseMemoon line 65 unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/AuthGuard.tsx` around lines 30 - 37, The module-level singleton storageManager/getStorageManager pattern persists across React unmounts and HMR and can leak across tests; either document these lifecycle caveats and note tests must call vi.resetModules() to fully reset the singleton, or switch to a per-component lifecycle by replacing the global reliance with a component-local instance (e.g., using React.useState(() => getStorageManager())[0] or calling getStorageManager() inline) and remove the now-unnecessary useMemo — update references to SecureStorageManager and createStorageAdapter accordingly so each component controls the storage manager lifecycle.apps/extension-wallet/src/router/__tests__/router.test.tsx (1)
119-134: Test correctly validates the failure path.Drives the actual
unlockWallet → storage.unlock → falsepath, asserts the a11yrole="alert"element contains the expected copy, and verifies the user remains on/unlock(heading present,/homeheading absent). One small consideration: it doesn't assert thatmockUnlockwas actually called with'wrong-password', which would catch a regression where the password isn't forwarded into the SDK. Optional add:expect(mockUnlock).toHaveBeenCalledWith('wrong-password');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/extension-wallet/src/router/__tests__/router.test.tsx` around lines 119 - 134, Add an assertion to ensure the unlock flow actually received the typed password: after the user click in the failing unlock test, add expect(mockUnlock).toHaveBeenCalledWith('wrong-password'); so the test verifies mockUnlock (the mocked storage.unlock/unlockWallet path) was invoked with the entered password and prevents regressions where the password isn't forwarded.contracts/account/README.md (1)
101-116: Document the new expiry unit contract foradd_session_key/refresh_session_key_ttl.This PR introduces a public input contract (values
< 1_000_000_000_000are seconds,>=are milliseconds and converted to seconds) and a canonical seconds storage representation, but the README's Session Keys section still only declaresexpires_at: u64with no mention of unit handling. SDK and frontend integrators reading this README will not learn about the threshold or the fact that the emitted/storedexpires_atmay differ from the value they passed in. Consider adding a short subsection (next to the Session Keys interface) describing:
- the seconds-vs-milliseconds threshold,
- that values are normalized to seconds at the entry points,
- that
refresh_session_key_ttlwill rewrite legacy millisecond-stored entries to seconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/README.md` around lines 101 - 116, Update the "Session Keys" README section to document the new expiry-unit contract: explain that add_session_key and refresh_session_key_ttl accept a public input where values < 1_000_000_000_000 are treated as seconds and values >= 1_000_000_000_000 are treated as milliseconds (and converted to seconds), state that expiry values are normalized to canonical seconds at the entry points (add_session_key/refresh_session_key_ttl) so the stored/emitted SessionKey.expires_at may differ from the original input, and note that refresh_session_key_ttl will migrate legacy millisecond-stored entries into seconds; reference the function names add_session_key, refresh_session_key_ttl, get_session_key and the SessionKey storage representation so integrators know where to look.contracts/account/src/lib.rs (1)
1322-1371: Cast onINSTANCE_BUMP_AMOUNTis redundant; otherwise the test reads well.
INSTANCE_BUMP_AMOUNTis alreadyu32(line 115) andenv.ledger().sequence()returnsu32, soINSTANCE_BUMP_AMOUNT as u32at line 1344 is a no-op cast. Minor cleanup.Also worth double-checking:
env.storage().instance().get_ttl()is only callable from inside the contract context in some soroban-sdk versions; if you see panics here in CI under different SDK versions, you may need to wrap the read inenv.as_contract(&contract_id, || ...).Cleanup
- env.ledger().set_sequence_number( - env.ledger() - .sequence() - .saturating_add(INSTANCE_BUMP_AMOUNT as u32), - ); + env.ledger().set_sequence_number( + env.ledger().sequence().saturating_add(INSTANCE_BUMP_AMOUNT), + );Please confirm that
env.storage().instance().get_ttl()is callable from outsideas_contractin the soroban-sdk version pinned inCargo.tomlfor this crate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/account/src/lib.rs` around lines 1322 - 1371, Remove the redundant cast on INSTANCE_BUMP_AMOUNT in the ledger sequence bump (replace INSTANCE_BUMP_AMOUNT as u32 with INSTANCE_BUMP_AMOUNT) since INSTANCE_BUMP_AMOUNT is already u32 and env.ledger().sequence() returns u32; also verify the pinned soroban-sdk version in Cargo.toml to ensure env.storage().instance().get_ttl() is allowed outside contract context, and if CI shows panics, wrap the get_ttl() reads in env.as_contract(&contract_id, || { ... }) around the ttl calls (references: INSTANCE_BUMP_AMOUNT, env.ledger().sequence(), env.storage().instance().get_ttl(), env.as_contract(&contract_id, || ...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/account/src/lib.rs`:
- Around line 397-402: The calculation for ledgers_to_live in the
ledgers_to_live block is inconsistent with the DAY_IN_LEDGERS constant:
DAY_IN_LEDGERS is computed using 5 seconds-per-ledger but the code uses
(expires_at - current_timestamp) / 4 and comments “4 seconds-per-ledger”; fix by
making the seconds-per-ledger assumption consistent—either change the divisor to
5 in the ledgers_to_live computation or adjust DAY_IN_LEDGERS to use 4; update
the comment accordingly so ledgers_to_live, DAY_IN_LEDGERS, and their comments
all use the same seconds-per-ledger value (referencing DAY_IN_LEDGERS,
ledgers_to_live, expires_at, and current_timestamp).
- Around line 366-378: The expiry check in execute() can be bypassed for legacy
millisecond timestamps; normalize expiry before any comparison by calling
Self::normalize_expiry_timestamp(...) on the loaded session (or normalize inside
Self::get_session_key) so that env.ledger().timestamp() is compared to canonical
seconds; update execute() to use the normalized value (and persist the
normalized session if changed) or add a one-time migration during
initialize/upgrade to scan DataKey::SessionKey entries and rewrite expires_at to
normalized seconds to prevent legacy keys from bypassing expiry checks.
- Around line 1246-1279: The tests
test_upgrade_owner_path_succeeds_and_increments_version and
test_upgrade_non_owner_is_rejected_and_version_unchanged call
client.try_upgrade(&wasm_hash) with a synthetic BytesN hash that was never
uploaded, so first upload a real WASM blob via
env.deployer().upload_contract_wasm(...) and use the returned hash for
try_upgrade instead of BytesN::from_array; capture the Result from
AncoreAccountClient::try_upgrade, assert it is Ok for the owner test (and Err
for the non-owner test), and then assert the version changes (or remains)
accordingly; update both tests to replace the synthetic hash creation and
ignored result (let _ =) with the upload call and proper result assertions.
---
Outside diff comments:
In `@contracts/account/src/lib.rs`:
- Around line 268-271: The event publish uses the caller-provided expires_at
instead of the stored normalized_expires_at; change the env.events().publish
call that emits events::session_key_added to pass normalized_expires_at (the
value saved with the session key) rather than the original expires_at so
subscribers and get_session_key() see the same canonical seconds timestamp;
update the tuple arguments to (public_key, normalized_expires_at) where
events::session_key_added is invoked.
---
Nitpick comments:
In `@apps/extension-wallet/src/router/__tests__/router.test.tsx`:
- Around line 119-134: Add an assertion to ensure the unlock flow actually
received the typed password: after the user click in the failing unlock test,
add expect(mockUnlock).toHaveBeenCalledWith('wrong-password'); so the test
verifies mockUnlock (the mocked storage.unlock/unlockWallet path) was invoked
with the entered password and prevents regressions where the password isn't
forwarded.
In `@apps/extension-wallet/src/router/AuthGuard.tsx`:
- Around line 110-116: The catch block in AuthGuard.tsx that calls
setAuthState(...) is swallowing all errors silently; change the anonymous catch
to capture the error (e.g., catch (err)) and log it at warn level before setting
isUnlocked:false so operational issues are visible; update the catch in the
unlock flow where setAuthState is used (reference: setAuthState in AuthGuard.tsx
/ UnlockScreen unlock handling) to call your existing logger.warn or
console.warn with a clear message plus the error object, preserving the current
state update and return false behavior.
- Around line 30-37: The module-level singleton storageManager/getStorageManager
pattern persists across React unmounts and HMR and can leak across tests; either
document these lifecycle caveats and note tests must call vi.resetModules() to
fully reset the singleton, or switch to a per-component lifecycle by replacing
the global reliance with a component-local instance (e.g., using
React.useState(() => getStorageManager())[0] or calling getStorageManager()
inline) and remove the now-unnecessary useMemo — update references to
SecureStorageManager and createStorageAdapter accordingly so each component
controls the storage manager lifecycle.
In `@apps/extension-wallet/src/router/index.tsx`:
- Around line 260-269: The handleUnlock function allows concurrent submissions;
add an isPending (or isUnlocking) React state and guard to prevent duplicate
submits: in handleUnlock (and related unlockWallet call) return early if
isPending is true, set isPending = true immediately before awaiting
unlockWallet(password), and set isPending = false in a finally block so it
always resets; also use isPending to disable the submit button (and optionally
the form) so the UI cannot trigger overlapping unlock attempts and to provide
proper feedback while unlockWallet runs.
- Around line 282-288: The password input currently calls setPassword onChange
but leaves the existing error visible; update the onChange handler for the
password input (the element using value={password} and setPassword) to also
clear the form error (call setError('') or setError(null)/setError(undefined)
depending on how error state is typed) before or after setPassword so the
"Invalid password" message is removed as the user types.
In `@contracts/account/README.md`:
- Around line 101-116: Update the "Session Keys" README section to document the
new expiry-unit contract: explain that add_session_key and
refresh_session_key_ttl accept a public input where values < 1_000_000_000_000
are treated as seconds and values >= 1_000_000_000_000 are treated as
milliseconds (and converted to seconds), state that expiry values are normalized
to canonical seconds at the entry points
(add_session_key/refresh_session_key_ttl) so the stored/emitted
SessionKey.expires_at may differ from the original input, and note that
refresh_session_key_ttl will migrate legacy millisecond-stored entries into
seconds; reference the function names add_session_key, refresh_session_key_ttl,
get_session_key and the SessionKey storage representation so integrators know
where to look.
In `@contracts/account/src/lib.rs`:
- Around line 1322-1371: Remove the redundant cast on INSTANCE_BUMP_AMOUNT in
the ledger sequence bump (replace INSTANCE_BUMP_AMOUNT as u32 with
INSTANCE_BUMP_AMOUNT) since INSTANCE_BUMP_AMOUNT is already u32 and
env.ledger().sequence() returns u32; also verify the pinned soroban-sdk version
in Cargo.toml to ensure env.storage().instance().get_ttl() is allowed outside
contract context, and if CI shows panics, wrap the get_ttl() reads in
env.as_contract(&contract_id, || { ... }) around the ttl calls (references:
INSTANCE_BUMP_AMOUNT, env.ledger().sequence(),
env.storage().instance().get_ttl(), env.as_contract(&contract_id, || ...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ee54c2d-19cb-475c-b89a-48ea43f61e71
📒 Files selected for processing (7)
apps/extension-wallet/src/router/AuthGuard.tsxapps/extension-wallet/src/router/__tests__/router.test.tsxapps/extension-wallet/src/router/index.tsxapps/extension-wallet/src/security/__tests__/lock-manager.test.tsapps/extension-wallet/src/security/lock-manager.tscontracts/account/README.mdcontracts/account/src/lib.rs
| let mut session_key = Self::get_session_key(env.clone(), public_key.clone()) | ||
| .ok_or(ContractError::SessionKeyNotFound)?; | ||
|
|
||
| Self::extend_session_key_ttl(&env, &public_key, session_key.expires_at); | ||
| // Normalize legacy values here so refresh always operates on canonical seconds. | ||
| let normalized_expires_at = Self::normalize_expiry_timestamp(session_key.expires_at); | ||
| if session_key.expires_at != normalized_expires_at { | ||
| session_key.expires_at = normalized_expires_at; | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::SessionKey(public_key.clone()), &session_key); | ||
| } | ||
|
|
||
| Self::extend_session_key_ttl(&env, &public_key, normalized_expires_at); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd contracts/account/src && wc -l lib.rsRepository: ancore-org/ancore
Length of output: 72
🏁 Script executed:
cd contracts/account/src && head -n 400 lib.rs | tail -n 50Repository: ancore-org/ancore
Length of output: 2065
🏁 Script executed:
cd contracts/account/src && sed -n '190,210p' lib.rsRepository: ancore-org/ancore
Length of output: 1074
🏁 Script executed:
cd contracts/account/src && sed -n '80,150p' lib.rsRepository: ancore-org/ancore
Length of output: 2080
🏁 Script executed:
cd contracts/account/src && sed -n '1,50p' lib.rsRepository: ancore-org/ancore
Length of output: 1714
🏁 Script executed:
cd contracts/account/src && rg "upgrade|migrate" -nRepository: ancore-org/ancore
Length of output: 1220
🏁 Script executed:
cd contracts/account/src && rg "add_session_key|create_session_key" -A 15 -nRepository: ancore-org/ancore
Length of output: 14194
🏁 Script executed:
cd contracts/account/src && sed -n '250,350p' lib.rsRepository: ancore-org/ancore
Length of output: 3204
🏁 Script executed:
cd contracts/account/src && git log --oneline --all -- lib.rs | head -20Repository: ancore-org/ancore
Length of output: 1472
🏁 Script executed:
cd contracts/account/src && git show HEAD:lib.rs | grep -A 20 "normalize_expiry_timestamp" || echo "Not in HEAD"Repository: ancore-org/ancore
Length of output: 233
🏁 Script executed:
cd contracts && find . -type f \( -name "*.md" -o -name "MIGRATION*" -o -name "UPGRADE*" \) | head -10Repository: ancore-org/ancore
Length of output: 92
🏁 Script executed:
cd contracts/account/src && sed -n '865,900p' lib.rsRepository: ancore-org/ancore
Length of output: 1503
Session key expiry check at execute() (line 203) should normalize before comparison to prevent bypass for legacy keys.
The conditional rewrite and TTL extension are correct, but the vulnerability is real: if any legacy millisecond-stored session keys exist in persistent storage (from before normalization was added), execute() will bypass expiry checks. The comparison env.ledger().timestamp() >= session.expires_at treats millisecond values (1.7 × 10¹²) as effectively never-expiring against seconds ( 1.7 × 10⁹).
New keys are safe because add_session_key() normalizes on write (line 254), and refresh handles individual keys, but this leaves an unprotected read path. Consider:
- Normalize in
get_session_key()or directly inexecute()'s expiry check, or - Add a one-time migration on contract upgrade/initialize to scan and fix any legacy keys in storage.
The test test_refresh_session_key_ttl_normalizes_legacy_milliseconds_expiry (line 865) indicates developers are already aware of this scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/account/src/lib.rs` around lines 366 - 378, The expiry check in
execute() can be bypassed for legacy millisecond timestamps; normalize expiry
before any comparison by calling Self::normalize_expiry_timestamp(...) on the
loaded session (or normalize inside Self::get_session_key) so that
env.ledger().timestamp() is compared to canonical seconds; update execute() to
use the normalized value (and persist the normalized session if changed) or add
a one-time migration during initialize/upgrade to scan DataKey::SessionKey
entries and rewrite expires_at to normalized seconds to prevent legacy keys from
bypassing expiry checks.
| let ledgers_to_live = if expires_at > current_timestamp { | ||
| // Using 4 seconds-per-ledger + 1 day buffer to guarantee it outlives expiry | ||
| ((expires_at_secs - current_timestamp) / 4) as u32 + DAY_IN_LEDGERS | ||
| ((expires_at - current_timestamp) / 4) as u32 + DAY_IN_LEDGERS | ||
| } else { | ||
| DAY_IN_LEDGERS // 1 day default buffer | ||
| }; |
There was a problem hiding this comment.
Inconsistent seconds-per-ledger assumption with DAY_IN_LEDGERS.
DAY_IN_LEDGERS (line 114) is computed assuming 5 seconds per ledger, but the new comment and arithmetic here assume 4 seconds per ledger ((expires_at - current_timestamp) / 4). Either both should use 5 (correct on Stellar mainnet/testnet) or both should use 4. The current mix overestimates ledgers_to_live by ~25% relative to the DAY_IN_LEDGERS baseline used for the threshold and the default branch — fine as a buffer, but the comment is misleading and the math should be self-consistent.
🛠️ Suggested fix
- let ledgers_to_live = if expires_at > current_timestamp {
- // Using 4 seconds-per-ledger + 1 day buffer to guarantee it outlives expiry
- ((expires_at - current_timestamp) / 4) as u32 + DAY_IN_LEDGERS
+ let ledgers_to_live = if expires_at > current_timestamp {
+ // 5 seconds-per-ledger (matches DAY_IN_LEDGERS) + 1 day buffer
+ ((expires_at - current_timestamp) / 5) as u32 + DAY_IN_LEDGERS
} else {
DAY_IN_LEDGERS // 1 day default buffer
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let ledgers_to_live = if expires_at > current_timestamp { | |
| // Using 4 seconds-per-ledger + 1 day buffer to guarantee it outlives expiry | |
| ((expires_at_secs - current_timestamp) / 4) as u32 + DAY_IN_LEDGERS | |
| ((expires_at - current_timestamp) / 4) as u32 + DAY_IN_LEDGERS | |
| } else { | |
| DAY_IN_LEDGERS // 1 day default buffer | |
| }; | |
| let ledgers_to_live = if expires_at > current_timestamp { | |
| // 5 seconds-per-ledger (matches DAY_IN_LEDGERS) + 1 day buffer | |
| ((expires_at - current_timestamp) / 5) as u32 + DAY_IN_LEDGERS | |
| } else { | |
| DAY_IN_LEDGERS // 1 day default buffer | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/account/src/lib.rs` around lines 397 - 402, The calculation for
ledgers_to_live in the ledgers_to_live block is inconsistent with the
DAY_IN_LEDGERS constant: DAY_IN_LEDGERS is computed using 5 seconds-per-ledger
but the code uses (expires_at - current_timestamp) / 4 and comments “4
seconds-per-ledger”; fix by making the seconds-per-ledger assumption
consistent—either change the divisor to 5 in the ledgers_to_live computation or
adjust DAY_IN_LEDGERS to use 4; update the comment accordingly so
ledgers_to_live, DAY_IN_LEDGERS, and their comments all use the same
seconds-per-ledger value (referencing DAY_IN_LEDGERS, ledgers_to_live,
expires_at, and current_timestamp).
| #[test] | ||
| fn test_upgrade_owner_path_succeeds_and_increments_version() { | ||
| let env = Env::default(); | ||
| let contract_id = env.register_contract(None, AncoreAccount); | ||
| let client = AncoreAccountClient::new(&env, &contract_id); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| client.initialize(&owner); | ||
| assert_eq!(client.get_version(), 1); | ||
|
|
||
| env.mock_all_auths(); | ||
| let wasm_hash = BytesN::from_array(&env, &[42u8; 32]); | ||
| let _ = client.try_upgrade(&wasm_hash); | ||
|
|
||
| // Owner-authorized upgrade should increment version. | ||
| assert_eq!(client.get_version(), 2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_upgrade_non_owner_is_rejected_and_version_unchanged() { | ||
| let env = Env::default(); | ||
| let contract_id = env.register_contract(None, AncoreAccount); | ||
| let client = AncoreAccountClient::new(&env, &contract_id); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| client.initialize(&owner); | ||
| assert_eq!(client.get_version(), 1); | ||
|
|
||
| // No owner auth is provided; require_auth() should reject this upgrade attempt. | ||
| let wasm_hash = BytesN::from_array(&env, &[7u8; 32]); | ||
| let result = client.try_upgrade(&wasm_hash); | ||
| assert!(result.is_err()); | ||
| assert_eq!(client.get_version(), 1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the test env handles update_current_contract_wasm with arbitrary hashes
# and confirm the test behavior the author intended.
rg -nP --type=rust -C3 'update_current_contract_wasm'
fd -t f 'Cargo.toml' contracts | xargs rg -nP 'soroban-sdk\s*=' -C1Repository: ancore-org/ancore
Length of output: 883
🌐 Web query:
soroban-sdk testutils update_current_contract_wasm behavior with non-installed hash
💡 Result:
The soroban-sdk::Env::deployer.update_current_contract_wasm(wasm_hash) method, available only with the "testutils" feature enabled, replaces the executable (WASM) of the currently executing contract with the WASM identified by the provided wasm_hash (a BytesN<32>). This is a test utility for simulating contract upgrades during unit tests. Key behavior: - The WASM blob identified by wasm_hash must already be present in the ledger, typically uploaded via env.deployer.upload_contract_wasm(wasm_bytes) which returns the hash. - If a non-installed (non-existent) hash is provided, the host function invocation fails with a HostError. Specifically, the Soroban host will return an error indicating the WASM code cannot be found (likely Error(ContractInstance, InvalidExecutable) or similar, based on analogous deployment behaviors where missing WASM causes simulation failures). - The update is deferred: It does not take effect immediately during the current invocation. The new executable is applied only after the current transaction successfully completes. - In test environments (Env::default or similar), this allows testing upgrade logic without deploying to a real network. - Usage example from docs and code samples: let env = Env::default; let new_wasm_hash = env.deployer.upload_contract_wasm(&new_wasm_bytes); env.deployer.update_current_contract_wasm(new_wasm_hash); // Update applies post-invocation; test subsequent calls use new WASM. For non-test environments, contract upgrades follow the standard Stellar/Soroban deployment flow (upload WASM, then deploy/update instance via transactions), not this testutils method. This is grounded in the official Rust SDK documentation for soroban_sdk::deploy::Deployer (latest version), which explicitly states the prerequisite, and consistent with Soroban host error patterns for missing WASM references during simulations/invocations.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/deploy/struct.Deployer.html
- 2: https://linkedin.com/in/naghme-mohammadifar
- 3: https://github.com/code-423n4/2026-04-k2/blob/main/contracts/price-oracle/src/upgrade.rs
- 4: Calling
update_current_contract_wasmfrom contract in test doesn't work stellar/rs-soroban-env#1089 - 5: stellar contract install error stellar/rs-soroban-sdk#1438
🏁 Script executed:
# Examine the try_upgrade implementation and its error handling
cd contracts/account/src && sed -n '290,320p' lib.rsRepository: ancore-org/ancore
Length of output: 1001
🏁 Script executed:
# Search for the complete upgrade function implementation including error handling
rg -nP --type=rust -A 25 'fn upgrade|fn try_upgrade' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 1634
🏁 Script executed:
# Check if there's any WASM upload or mocking in the test file
rg -nP --type=rust -B 10 'test_upgrade_owner_path_succeeds' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 444
🏁 Script executed:
# Find try_upgrade implementation
rg -nP --type=rust -A 15 'fn try_upgrade|#\[contract\]' contracts/account/src/lib.rs | head -60Repository: ancore-org/ancore
Length of output: 674
🏁 Script executed:
# Look for contract methods/exports and the client generated code
rg -nP --type=rust '#\[contractimpl\]' -A 5 contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 363
🏁 Script executed:
# Check if any WASM is actually uploaded before the test
rg -nP --type=rust -B 20 'test_upgrade_owner_path_succeeds' contracts/account/src/lib.rs | tail -30Repository: ancore-org/ancore
Length of output: 821
🏁 Script executed:
# Get the full test implementation
rg -nP --type=rust -A 20 'test_upgrade_owner_path_succeeds_and_increments_version' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 970
🏁 Script executed:
# Find where AncoreAccountClient is generated/defined and understand try_upgrade
rg -nP --type=rust 'AncoreAccountClient|try_upgrade' contracts/account/src/lib.rs | head -20Repository: ancore-org/ancore
Length of output: 1480
🏁 Script executed:
# Check if there's a separate test module file
find contracts/account -name '*test*' -o -name '*spec*'Repository: ancore-org/ancore
Length of output: 2970
🏁 Script executed:
# Check if there are any test snapshots for the upgrade test (may indicate test passed)
ls -la contracts/account/test_snapshots/test/ | grep -i upgradeRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Look for how soroban-sdk handles update_current_contract_wasm errors
rg -nP 'update_current_contract_wasm' contracts/account/src/lib.rs -A 3 -B 3Repository: ancore-org/ancore
Length of output: 316
🏁 Script executed:
# Check if there's documentation or error handling around deployer calls
rg -nP --type=rust 'deployer\(\)' contracts/account/src/lib.rs -B 2 -A 2Repository: ancore-org/ancore
Length of output: 232
🏁 Script executed:
# Check if there are CI/test configurations that show test results
find . -name '*.yml' -o -name '*.yaml' | xargs grep -l 'test' 2>/dev/null | head -5Repository: ancore-org/ancore
Length of output: 169
🏁 Script executed:
# Look for any WASM upload or preparation in the test or module
rg -nP --type=rust 'upload_contract_wasm|deployer' contracts/account/src/lib.rs | head -20Repository: ancore-org/ancore
Length of output: 87
🏁 Script executed:
# Check the actual behavior by looking at Soroban documentation or examples in the repo
rg -nP 'env.deployer' contracts/ -A 2 | head -40Repository: ancore-org/ancore
Length of output: 249
🏁 Script executed:
# Check CI workflow to understand if tests pass
cat .github/workflows/ci.yml | head -100Repository: ancore-org/ancore
Length of output: 2437
🏁 Script executed:
# Look for test invocation and any WASM preparation
rg -nP 'cargo test|test_upgrade' .github/workflows/ci.yml -B 2 -A 2Repository: ancore-org/ancore
Length of output: 185
🏁 Script executed:
# Search for any error handling or Result types in upgrade context
rg -nP --type=rust 'Result.*upgrade|try_upgrade' contracts/account/src/lib.rs -B 3 -A 3Repository: ancore-org/ancore
Length of output: 673
🏁 Script executed:
# Check if update_current_contract_wasm is documented or wrapped differently
rg -nP --type=rust 'pub fn update_current_contract_wasm' --max-count 5Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Look at the generated client to understand try_upgrade behavior
rg -nP '#\[derive.*Client\]|impl.*Client' contracts/account/src/lib.rs -B 2 -A 5Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Search for documentation or examples of similar upgrade patterns in soroban
web_search
soroban-sdk deploy.Deployer update_current_contract_wasm test environment behavior invalid hashRepository: ancore-org/ancore
Length of output: 161
🏁 Script executed:
# Search for upload_contract_wasm usage in the repository to understand the pattern
rg -nP 'upload_contract_wasm' contracts/Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Look for other test patterns with deployer to understand how upgrade tests should work
rg -nP --type=rust 'deployer\(\)\.upload|deployer\(\)\.update' contracts/ -B 2 -A 2Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check if there's any documentation or patterns in soroban examples
find . -name '*.md' -o -name '*.rs' | xargs grep -l 'upgrade.*wasm\|deployer.*update' 2>/dev/null | head -5Repository: ancore-org/ancore
Length of output: 121
🏁 Script executed:
# Check the README for any documentation about upgrade testing
cat contracts/account/README.md | grep -A 10 -B 10 -i upgradeRepository: ancore-org/ancore
Length of output: 3384
🏁 Script executed:
# Look at error handling around deployer calls - check if try_ prefix wraps errors
rg -nP --type=rust 'fn try_' contracts/account/src/lib.rs | head -10Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check how soroban SDK generated clients wrap contract methods
rg -nP --type=rust 'try_.*Result' contracts/account/src/lib.rs -B 2 -A 2Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check what happens when try_upgrade is called - look at the full error handling flow
rg -nP --type=rust -B 5 -A 10 'fn try_upgrade' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Search for the generated client methods and how soroban SDK handles try_ prefixes
rg -nP --type=rust 'pub.*fn try_' contracts/account/src/lib.rs -A 5Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Try to find any soroban examples or similar patterns in the repo
find . -name '*.rs' | xargs grep -l 'register_contract.*AncoreAccount' | xargs cat | grep -A 30 'test_upgrade'Repository: ancore-org/ancore
Length of output: 1898
🏁 Script executed:
# Check if there are any comments or documentation about how soroban testutils handles hashes
rg -nP --type=rust 'mock.*wasm|test.*wasm|update_current_contract_wasm' contracts/account/src/lib.rs -B 3 -A 3Repository: ancore-org/ancore
Length of output: 316
🏁 Script executed:
# Look for any integration test or example that shows proper upgrade testing with uploaded WASM
find . -name '*.rs' | xargs grep -l 'upload_contract_wasm.*update_current_contract_wasm' 2>/dev/nullRepository: ancore-org/ancore
Length of output: 45
🏁 Script executed:
# Check the soroban SDK version and see if there's any changelog mentioning testutils behavior
cat contracts/Cargo.toml | grep -A 5 -B 5 'soroban-sdk'Repository: ancore-org/ancore
Length of output: 236
🏁 Script executed:
# Try to understand soroban testutils behavior by checking if there are comments or examples
rg -nP --type=rust 'Env::default|register_contract' contracts/account/src/lib.rs -B 2 -A 2 | head -30Repository: ancore-org/ancore
Length of output: 1171
🏁 Script executed:
# Look for any documentation about transaction atomicity/rollback in soroban
find . -name '*.md' | xargs grep -i 'rollback\|atomic\|transaction' 2>/dev/null | head -20Repository: ancore-org/ancore
Length of output: 1564
🏁 Script executed:
# Check if the tests actually run and what the error would be
# by looking for test output or snapshots
cat contracts/account/test_snapshots/test/test_upgrade_owner_path_succeeds_and_increments_version.1.json 2>/dev/null || echo "No snapshot found"Repository: ancore-org/ancore
Length of output: 78
🏁 Script executed:
# Check if snapshot exists and also verify by looking at the test file structure
ls -la contracts/account/test_snapshots/test/ | wc -l
# Compare with actual tests
rg -nP '#\[test\]' contracts/account/src/lib.rs | wc -lRepository: ancore-org/ancore
Length of output: 66
🏁 Script executed:
# Check if try_upgrade is a soroban-sdk generated method by looking at the module structure
rg -nP --type=rust 'mod.*test|#\[cfg.*test\]' contracts/account/src/lib.rs -B 2 -A 2 | head -40Repository: ancore-org/ancore
Length of output: 168
🏁 Script executed:
# Verify the exact behavior: search for how update_current_contract_wasm works in tests
rg -nP --type=rust -B 10 'env.mock_all_auths.*upgrade|upgrade.*env.mock' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Check if upgrade tests are excluded or skipped
rg -nP --type=rust '#\[ignore\]|#\[skip\]' contracts/account/src/lib.rs -B 3 | grep -A 3 upgradeRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Look at the actual test output by checking if there's a test report or CI artifacts
find . -name '*.json' -path '*/test*' | head -20Repository: ancore-org/ancore
Length of output: 1817
🏁 Script executed:
# Check if soroban testutils really doesn't validate the hash by examining testutils behavior documentation in soroban SDK
# Let's look at the Cargo.lock to see exact versions
grep -A 5 'soroban-sdk' contracts/Cargo.lock | head -20Repository: ancore-org/ancore
Length of output: 114
🏁 Script executed:
# Critical: Check if upgrade test snapshots exist - list all snapshot files
find contracts/account/test_snapshots -name '*upgrade*'Repository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# Count snapshots vs tests to see if upgrade tests are missing
echo "Total test snapshots:" && find contracts/account/test_snapshots/test -name '*.json' | wc -l
echo "Upgrade test snapshots:" && find contracts/account/test_snapshots/test -name '*upgrade*' | wc -l
echo "All tests in lib.rs:" && rg -c '#\[test\]' contracts/account/src/lib.rsRepository: ancore-org/ancore
Length of output: 135
🏁 Script executed:
# Look at what the snapshots directory contains
ls -1 contracts/account/test_snapshots/test/ | grep -i upgradeRepository: ancore-org/ancore
Length of output: 43
🏁 Script executed:
# List all test names from lib.rs
rg -nP '#\[test\]' contracts/account/src/lib.rs -A 1 | grep 'fn test'Repository: ancore-org/ancore
Length of output: 1799
🏁 Script executed:
# List all snapshot files to see what tests they cover
ls -1 contracts/account/test_snapshots/test/ | sed 's/\.1\.json//'Repository: ancore-org/ancore
Length of output: 1414
🏁 Script executed:
# Compare the two lists to see which tests are missing snapshots
echo "=== Tests in lib.rs ===" && rg -nP '#\[test\]' contracts/account/src/lib.rs -A 1 | grep 'fn test' | sed 's/.*fn //' | sed 's/().*//' | sort
echo "=== Tests with snapshots ===" && ls -1 contracts/account/test_snapshots/test/ | sed 's/\.1\.json//' | sortRepository: ancore-org/ancore
Length of output: 2761
Address missing WASM upload in upgrade tests.
The test calls client.try_upgrade(&wasm_hash) with a synthetic hash [42u8; 32] that is never uploaded. According to soroban-sdk documentation, update_current_contract_wasm() requires the WASM blob to be pre-installed in the ledger via env.deployer().upload_contract_wasm(...). Providing a non-existent hash causes a HostError.
The test only verifies version bookkeeping (that it increments from 1 to 2) but doesn't actually confirm the WASM upgrade succeeds. The result of try_upgrade is ignored (let _ = ...), so any failure is silently discarded. Additionally, both upgrade tests lack snapshots in test_snapshots/test/, suggesting they may not be running successfully in CI.
To make this a faithful upgrade test, upload real WASM before calling upgrade:
let new_wasm_bytes = vec![/* valid WASM bytecode */];
let wasm_hash = env.deployer().upload_contract_wasm(&new_wasm_bytes);
let result = client.try_upgrade(&wasm_hash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/account/src/lib.rs` around lines 1246 - 1279, The tests
test_upgrade_owner_path_succeeds_and_increments_version and
test_upgrade_non_owner_is_rejected_and_version_unchanged call
client.try_upgrade(&wasm_hash) with a synthetic BytesN hash that was never
uploaded, so first upload a real WASM blob via
env.deployer().upload_contract_wasm(...) and use the returned hash for
try_upgrade instead of BytesN::from_array; capture the Result from
AncoreAccountClient::try_upgrade, assert it is Ok for the owner test (and Err
for the non-owner test), and then assert the version changes (or remains)
accordingly; update both tests to replace the synthetic hash creation and
ignored result (let _ =) with the upload call and proper result assertions.
|
@Abolax123 resolvev conflicts |
Resolve conflicts in extension-wallet AuthGuard (storage + verifier + unlockError), router unlock UI, router tests, lock manager, and account contract session key paths. Align test storage mock types and unblock ESLint no-undef/no-empty. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Use global TextEncoder/TextDecoder and WebCrypto-only paths so Vite can build the extension wallet. Replace Buffer-based hex/base64 helpers with small UTF-8/binary routines. Stellar verify keeps type-only Buffer cast for upstream typings. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Summary
< 1_000_000_000_000are treated as seconds>= 1_000_000_000_000are treated as milliseconds and converted to secondsadd_session_keyandrefresh_session_key_ttlentry pointsTests
test_add_session_key_normalizes_milliseconds_expiry_to_secondstest_add_session_key_keeps_seconds_expiry_unchangedtest_refresh_session_key_ttl_normalizes_legacy_milliseconds_expirySecurity impact
Closes #213
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation