Fix/hardware crypto context logic #425
Merged
Conversation
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
Contributor
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Onboarding/Verify UI
participant KeyCtrl as KeyController
participant Store
participant KMFactory as KeyManagerFactory
participant KM as KeyManager
UI->>KeyCtrl: check hardware availability / user action
alt hardware unavailable
UI->>UI: show "Hardware Not Available" (pre-verification option)
UI-->>KeyCtrl: request pre-verification path
else hardware available
UI->>KeyCtrl: proceed to verification/onboarding flow
end
KeyCtrl->>Store: read "fake" flag
Store-->>KeyCtrl: fake (true/false)
KeyCtrl->>KMFactory: getKeyManagerForContext(keyId, context, isFake)
rect rgb(230,245,230)
Note over KMFactory: Selection logic
alt isFake = true OR context = "pre-verification"
KMFactory->>KMFactory: choose software key manager
else hardware available
KMFactory->>KMFactory: choose hardware key manager
else
KMFactory->>KMFactory: fallback to software manager
end
end
KMFactory-->>KM: instantiate manager
KM-->>KeyCtrl: return KeyManager
KeyCtrl-->>UI: provide manager / continue flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
4-4: Remove unused import.The
UserControllertype is imported but never used in this file.Apply this diff to remove the unused import:
-import type { UserController } from "./user";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/key.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts (1)
infrastructure/eid-wallet/src/lib/crypto/types.ts (2)
KeyManager(5-39)KeyManagerConfig(44-48)
infrastructure/eid-wallet/src/lib/global/controllers/key.ts (1)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts (1)
KeyManagerFactory(10-106)
🔇 Additional comments (1)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts (1)
85-94: LGTM! Hardware usage logic correctly implements requirements.The implementation properly addresses the stated objectives from issue #423:
- Hardware module is never used when
isFakeis true (lines 89-90)- Hardware module is never used for "pre-verification" context (line 91)
- Hardware is only used when available via runtime check (line 92)
The short-circuit evaluation order is optimal, avoiding unnecessary hardware availability checks when
isFakeis true.
coodos
force-pushed
the
fix/hardware-crypto-context-logic
branch
from
9390d42 to
f80bf7a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
infrastructure/eid-wallet/src-tauri/gen/android/.idea/deviceManager.xmlis excluded by!**/gen/**
📒 Files selected for processing (3)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts(2 hunks)infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte(4 hunks)infrastructure/eid-wallet/src/routes/(auth)/verify/+page.svelte(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
infrastructure/eid-wallet/src/lib/crypto/KeyManagerFactory.ts (1)
infrastructure/eid-wallet/src/lib/crypto/types.ts (2)
KeyManager(5-39)KeyManagerConfig(44-48)
⏰ 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). (1)
- GitHub Check: build
Comment on lines
+42
to
+78
| // Actually try to generate a test hardware key | ||
| const testKeyId = `hardware-test-${Date.now()}`; | ||
| console.log( | ||
| "Testing hardware key generation with test key:", | ||
| testKeyId, | ||
| ); | ||
| try { | ||
| const { manager, created } = await globalState.keyService.ensureKey( | ||
| testKeyId, | ||
| "onboarding", | ||
| ); | ||
| console.log( | ||
| "Test key result - Manager type:", | ||
| manager.getType(), | ||
| "Created:", | ||
| created, | ||
| ); | ||
| // Check if we got hardware manager and it actually created a key | ||
| if (manager.getType() !== "hardware") { | ||
| throw new Error("Got software fallback instead of hardware"); | ||
| } | ||
| // Hardware works! Clean up test key and proceed | ||
| console.log("Hardware keys are working"); | ||
| checkingHardware = false; | ||
| } catch (keyError) { | ||
| console.error("Hardware key test failed:", keyError); | ||
| showHardwareError = true; | ||
| checkingHardware = false; | ||
| } | ||
| } catch (err) { | ||
| console.error("Error checking hardware:", err); | ||
| showHardwareError = true; | ||
| checkingHardware = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Stop generating throwaway hardware keys during capability check
ensureKey actually runs the full hardware credential ceremony. That means the “capability check” now forces a biometric/PIN prompt, and any cancellation or timeout will be misread as “hardware unavailable”, pushing real hardware-capable users into the pre-verification path. On top of that, every click leaves behind a hardware-test-<timestamp> credential that we never clean up, which quickly pollutes the authenticator and can exhaust limited key slots. Please switch this probe to the existing isHardwareAvailable() call (or similar non-destructive check) and drop the throwaway key creation altogether.
Recommended change:
@@
- checkingHardware = true;
- showHardwareError = false;
- error = null;
-
- try {
- if (!globalState) {
- globalState = getContext<() => GlobalState>("globalState")();
- }
-
- // Actually try to generate a test hardware key
- const testKeyId = `hardware-test-${Date.now()}`;
- console.log(
- "Testing hardware key generation with test key:",
- testKeyId,
- );
-
- try {
- const { manager, created } = await globalState.keyService.ensureKey(
- testKeyId,
- "onboarding",
- );
- console.log(
- "Test key result - Manager type:",
- manager.getType(),
- "Created:",
- created,
- );
-
- // Check if we got hardware manager and it actually created a key
- if (manager.getType() !== "hardware") {
- throw new Error("Got software fallback instead of hardware");
- }
-
- // Hardware works! Clean up test key and proceed
- console.log("Hardware keys are working");
- checkingHardware = false;
- } catch (keyError) {
- console.error("Hardware key test failed:", keyError);
- showHardwareError = true;
- checkingHardware = false;
- }
- } catch (err) {
- console.error("Error checking hardware:", err);
- showHardwareError = true;
- checkingHardware = false;
- }
+ checkingHardware = true;
+ showHardwareError = false;
+ error = null;
+
+ try {
+ if (!globalState) {
+ globalState = getContext<() => GlobalState>("globalState")();
+ }
+
+ const hardwareAvailable =
+ await globalState.keyService.isHardwareAvailable();
+
+ checkingHardware = false;
+ showHardwareError = !hardwareAvailable;
+ } catch (err) {
+ console.error("Error checking hardware:", err);
+ showHardwareError = true;
+ checkingHardware = false;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
in infrastructure/eid-wallet/src/routes/(auth)/onboarding/+page.svelte around
lines 42-78, stop creating throwaway hardware keys with ensureKey because it
triggers the full credential ceremony and leaves persistent test credentials;
instead call the service's non-destructive probe (e.g., isHardwareAvailable() or
similar) to detect hardware capability, remove the testKeyId generation and
ensureKey invocation, and use the boolean result to set showHardwareError and
checkingHardware accordingly; also remove any catch branches specific to key
creation and only handle probe errors (set showHardwareError=true,
checkingHardware=false) without creating or attempting to clean up credentials.
Contributor
There was a problem hiding this comment.
nack, that's the only way to do it
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the key service implementation to see what methods are available
echo "=== Searching for KeyService class definition ==="
ast-grep --pattern 'class KeyService {
$$$
}'
echo -e "\n=== Searching for isHardwareAvailable method ==="
rg -n "isHardwareAvailable" --type ts
echo -e "\n=== Searching for hardware capability check methods ==="
rg -nP "(checkHardware|isHardware|hardwareAvailable|canUseHardware)" --type ts -C2
echo -e "\n=== Finding KeyService file ==="
fd -e ts -e js "KeyService" --full-path
echo -e "\n=== Searching for KeyManager availability checks ==="
rg -n "getType\(\)" --type ts -g "*KeyManager*" -C2
Length of output: 21879
🏁 Script executed:
#!/bin/bash
# Get the full implementation of isHardwareAvailable to understand what it checks
echo "=== KeyManagerFactory.isHardwareAvailable implementation ==="
ast-grep --pattern 'static async isHardwareAvailable(): Promise<boolean> {
$$$
}'
echo -e "\n=== Find the full KeyManagerFactory file ==="
fd "KeyManagerFactory.ts" --full-path
echo -e "\n=== Get lines 65-80 of KeyManagerFactory to see isHardwareAvailable ==="
find . -name "KeyManagerFactory.ts" -exec sed -n '65,80p' {} \;
echo -e "\n=== Check what getHardwareKeyManager does ==="
ast-grep --pattern 'static getHardwareKeyManager() {
$$$
}'
Length of output: 880
🏁 Script executed:
#!/bin/bash
# Check what the exists() method does in HardwareKeyManager
echo "=== HardwareKeyManager.exists() implementation ==="
cat infrastructure/eid-wallet/src/lib/crypto/HardwareKeyManager.ts
echo -e "\n=== Check if exists() might trigger a credential ceremony ==="
rg -n "exists.*credential|exists.*prompt|exists.*biometric" --type ts -C3
Length of output: 4760
@coodos, understood — if generating a real test key is the only reliable way to verify hardware support, I'll defer to your experience with the platform.
That said, the test keys created with hardware-test-${Date.now()} are never cleaned up. Each "Get Started" click will leave behind a persistent credential in the hardware keystore, which could accumulate over time or exhaust limited key slots on some devices.
Would you consider adding cleanup logic after the hardware check completes (success or failure)? Something like:
try {
const { manager, created } = await globalState.keyService.ensureKey(
testKeyId,
"onboarding",
);
// ... existing checks ...
// Clean up test key
if (created && manager.getType() === "hardware") {
// Delete the test key if your KeyManager supports it
// or track it for later cleanup
}
} catch (keyError) {
// ... existing error handling ...
}If the hardware API doesn't expose a delete/cleanup method, at minimum tracking these test key IDs for bulk cleanup later would prevent them from accumulating indefinitely.
xPathin
approved these changes
Nov 13, 2025
coodos
approved these changes
Nov 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of change
Updates the context provision logic to never use hardware module if user isfake, if context is pre verification, after all this, check if harware key manager exists before determining to use hardware module or not
Issue Number
closes #423
Type of change
How the change has been tested
Device test
Change checklist
Summary by CodeRabbit
New Features
Improvements