fix(auth): upgrade PIN hashing to Argon2id v2 with migration (#2399) - #2464
fix(auth): upgrade PIN hashing to Argon2id v2 with migration (#2399)#2464Kaushikgupta469 wants to merge 7 commits into
Conversation
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds PIN key-derivation versioning and helpers, updates verification to detect and upgrade older KDF hashes, exposes an auth-machine event to persist upgrades, wires upgrade callbacks through the passcode screen and controller, updates tests/mocks, and refreshes talisman checksums. ChangesPIN Hash Versioning and Upgrade Flow
Sequence DiagramsequenceDiagram
participant User
participant PasscodeScreen
participant PasscodeVerify
participant PasscodeScreenController
participant authService
participant commonUtil as encodePinHash/parsePinHash
participant PIN_KDF as PIN_KDF_PROFILES
User->>PasscodeScreen: unlock with stored hash
PasscodeScreen->>PasscodeVerify: render with onUpgrade callback
PasscodeVerify->>commonUtil: parsePinHash(stored)
commonUtil-->>PasscodeVerify: {version, hash}
PasscodeVerify->>PIN_KDF: lookup config for version
PasscodeVerify->>PasscodeVerify: compute candidate hash via hashData
alt candidate equals stored
PasscodeVerify->>PIN_KDF: lookup CURRENT_PIN_KDF_VERSION
PasscodeVerify->>PasscodeVerify: compute upgraded hash
PasscodeVerify->>commonUtil: encodePinHash(v2, newHash)
commonUtil-->>PasscodeVerify: v2$newHash
PasscodeVerify->>PasscodeScreenController: onUpgrade(v2$newHash)
PasscodeScreenController->>authService: UPGRADE_PASSCODE_HASH(v2$newHash)
authService->>authService: setPasscode & storeContext
else mismatch
PasscodeVerify-->>PasscodeVerify: call onError and return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/PasscodeVerify.test.tsx (1)
37-70: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd behavioral tests for the new verify/upgrade branches.
These snapshots don't exercise the security-critical paths added in this PR: legacy
v1success withonUpgrade, currentv2success without upgrade, and mismatch callingonError. CapturingPinInput'sonDoneand asserting the callbacks would make regressions here visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/PasscodeVerify.test.tsx` around lines 37 - 70, The tests only cover snapshots and miss behavioral branches for PasscodeVerify: add unit tests that simulate PinInput's onDone to exercise legacy v1 success path invoking onUpgrade, current v2 success path invoking onSuccess without upgrade, and failure path invoking onError; locate the PasscodeVerify component tests and add three test cases that render PasscodeVerify with appropriate props (including a stored v1 hashed passcode and onUpgrade mock, a v2 hashed passcode and onSuccess mock, and a mismatch case with onError mock), call or trigger the PinInput.onDone handler (or simulate entering the pin) and assert the correct callbacks (onUpgrade/onSuccess/onError) are called with expected arguments.
🧹 Nitpick comments (3)
shared/constants.test.ts (1)
225-240: ⚡ Quick winAssert the exact
v2profile, not loose minimums.This suite still passes if
iterationsdrops to1orparallelismchanges, which weakens the protection this regression test is supposed to give. For a security config, pin the full expectedv2object.✅ Suggested assertion
it('v2 profile follows OWASP 2024 mobile guidance (memory-hard Argon2id)', () => { - const v2 = PIN_KDF_PROFILES.v2; - expect(v2.mode).toBe('argon2id'); - expect(v2.memory).toBeGreaterThanOrEqual(19 * 1024); - expect(v2.hashLength).toBeGreaterThanOrEqual(32); - expect(v2.iterations).toBeGreaterThanOrEqual(1); + expect(PIN_KDF_PROFILES.v2).toEqual({ + iterations: 2, + memory: 19 * 1024, + parallelism: 1, + hashLength: 32, + mode: 'argon2id', + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/constants.test.ts` around lines 225 - 240, The test should assert the exact v2 KDF profile instead of loose minimums; update the 'v2 profile follows OWASP 2024 mobile guidance' spec to compare PIN_KDF_PROFILES.v2 to the full expected object (including mode: 'argon2id', memory, hashLength, iterations, parallelism, and any other fields) and keep the CURRENT_PIN_KDF_VERSION and PIN_KDF_PROFILES references intact so the assertion fails if any parameter (e.g., iterations or parallelism) changes; use a strict equality check (or deep equality assertion) against the canonical v2 profile object rather than greaterThanOrEqual checks.shared/commonUtil.test.ts (1)
43-75: ⚡ Quick winAdd a future-version regression case.
The new parser coverage never exercises an unsupported version-shaped prefix. Add a case like
v3$abcdefso a future unknown version can't silently turn into a broken login path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/commonUtil.test.ts` around lines 43 - 75, Add a regression test that ensures parsePinHash preserves unknown version-shaped prefixes: add an it case in the "PIN hash version helpers" suite that calls parsePinHash('v3$abcdef') and expects {version: 'v3', hash: 'abcdef'} (similar to the existing v2 test), referencing parsePinHash (and encodePinHash if you want a round-trip) so future unknown versions aren’t misinterpreted as legacy v1.shared/constants.ts (1)
124-136: ⚡ Quick winNarrow
PIN_KDF_PROFILESto the declared version union.
Record<string, Argon2iConfig>throws away the finite version set introduced here, so typos and incomplete future additions won't fail at compile time. Defining the union first and typing the map asRecord<PinKdfVersion, Argon2iConfig>keeps the KDF surface exhaustive.♻️ Proposed tightening
-export const PIN_KDF_PROFILES: Record<string, Argon2iConfig> = { +export type PinKdfVersion = 'v1' | 'v2'; + +export const PIN_KDF_PROFILES: Record<PinKdfVersion, Argon2iConfig> = { v1: argon2iConfig, v2: { iterations: 2, memory: 19 * 1024, parallelism: 1, hashLength: 32, mode: 'argon2id', }, }; - -export type PinKdfVersion = 'v1' | 'v2'; export const CURRENT_PIN_KDF_VERSION: PinKdfVersion = 'v2';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/constants.ts` around lines 124 - 136, Define the PinKdfVersion union before the map and narrow PIN_KDF_PROFILES to Record<PinKdfVersion, Argon2iConfig> so the keys are exhaustive and typos are caught; specifically, move or declare export type PinKdfVersion = 'v1' | 'v2' prior to PIN_KDF_PROFILES, change the type of PIN_KDF_PROFILES from Record<string, Argon2iConfig> to Record<PinKdfVersion, Argon2iConfig>, and ensure the map contains entries for 'v1' and 'v2' (matching the existing argon2iConfig and v2 object) while keeping CURRENT_PIN_KDF_VERSION typed as PinKdfVersion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machines/auth.test.ts`:
- Around line 144-149: The test only validates the UPGRADE_PASSCODE_HASH event
creator (AuthEvents.UPGRADE_PASSCODE_HASH) instead of exercising the state
machine transition; update the spec to send the UPGRADE_PASSCODE_HASH event into
the authorized machine instance (call the authorized service/transition) and
then assert that the machine updated context.passcode and called storeContext
with the new context; locate references to authorized,
AuthEvents.UPGRADE_PASSCODE_HASH, context.passcode, and storeContext in the test
and replace the simple .toEqual check with a run/send of the event and
expectations on the machine context and mocked storeContext invocation.
In `@machines/auth.ts`:
- Around line 69-71: The UPGRADE_PASSCODE_HASH event is currently defined at the
machine root (UPGRADE_PASSCODE_HASH with actions
['setPasscode','storeContext']), allowing any code with authService access to
overwrite the passcode hash; move/scope that event definition into the verified
unlock state (the state where PIN checks succeed) instead of the root events so
only that state's transitions can trigger it. Locate the UPGRADE_PASSCODE_HASH
entry and remove it from the top-level events, then add the same event mapping
inside the verifiedUnlock state's events/transitions (or guard it with a
state-specific condition) so only verifiedUnlock can call actions setPasscode
and storeContext; update any tests or callers that assumed root-level handling
accordingly.
---
Outside diff comments:
In `@components/PasscodeVerify.test.tsx`:
- Around line 37-70: The tests only cover snapshots and miss behavioral branches
for PasscodeVerify: add unit tests that simulate PinInput's onDone to exercise
legacy v1 success path invoking onUpgrade, current v2 success path invoking
onSuccess without upgrade, and failure path invoking onError; locate the
PasscodeVerify component tests and add three test cases that render
PasscodeVerify with appropriate props (including a stored v1 hashed passcode and
onUpgrade mock, a v2 hashed passcode and onSuccess mock, and a mismatch case
with onError mock), call or trigger the PinInput.onDone handler (or simulate
entering the pin) and assert the correct callbacks (onUpgrade/onSuccess/onError)
are called with expected arguments.
---
Nitpick comments:
In `@shared/commonUtil.test.ts`:
- Around line 43-75: Add a regression test that ensures parsePinHash preserves
unknown version-shaped prefixes: add an it case in the "PIN hash version
helpers" suite that calls parsePinHash('v3$abcdef') and expects {version: 'v3',
hash: 'abcdef'} (similar to the existing v2 test), referencing parsePinHash (and
encodePinHash if you want a round-trip) so future unknown versions aren’t
misinterpreted as legacy v1.
In `@shared/constants.test.ts`:
- Around line 225-240: The test should assert the exact v2 KDF profile instead
of loose minimums; update the 'v2 profile follows OWASP 2024 mobile guidance'
spec to compare PIN_KDF_PROFILES.v2 to the full expected object (including mode:
'argon2id', memory, hashLength, iterations, parallelism, and any other fields)
and keep the CURRENT_PIN_KDF_VERSION and PIN_KDF_PROFILES references intact so
the assertion fails if any parameter (e.g., iterations or parallelism) changes;
use a strict equality check (or deep equality assertion) against the canonical
v2 profile object rather than greaterThanOrEqual checks.
In `@shared/constants.ts`:
- Around line 124-136: Define the PinKdfVersion union before the map and narrow
PIN_KDF_PROFILES to Record<PinKdfVersion, Argon2iConfig> so the keys are
exhaustive and typos are caught; specifically, move or declare export type
PinKdfVersion = 'v1' | 'v2' prior to PIN_KDF_PROFILES, change the type of
PIN_KDF_PROFILES from Record<string, Argon2iConfig> to Record<PinKdfVersion,
Argon2iConfig>, and ensure the map contains entries for 'v1' and 'v2' (matching
the existing argon2iConfig and v2 object) while keeping CURRENT_PIN_KDF_VERSION
typed as PinKdfVersion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef6a9767-28f0-4f6a-a2d4-87761a0c07c3
📒 Files selected for processing (13)
.talismanrccomponents/PasscodeVerify.test.tsxcomponents/PasscodeVerify.tsxmachines/auth.test.tsmachines/auth.tsmachines/auth.typegen.tsscreens/PasscodeScreen.test.tsxscreens/PasscodeScreen.tsxscreens/PasscodeScreenController.tsshared/commonUtil.test.tsshared/commonUtil.tsshared/constants.test.tsshared/constants.ts
…ation (inji#2399) Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machines/auth.test.ts`:
- Around line 153-166: The action stubs in authMachine.withConfig (e.g.,
requestStoredContext, storeContext, setContext, setBiometrics, setLanguage,
setAppSetupComplete, setPasscodeSalt, setOnboardingDone, setInitialDownloadDone,
setTourGuide, setIsToggleFromSettings) are empty functions and trigger
`@typescript-eslint/no-empty-function`; replace each empty function with a jest
mock (jest.fn()) or a shared no-op mock to satisfy the linter and enable
assertions; update the same pattern for the corresponding action stubs around
lines 177-193 as well so all action values in the withConfig call use jest.fn()
or the shared noop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd797782-a430-48f7-8d21-ca11cbb29b27
📒 Files selected for processing (3)
.talismanrcmachines/auth.test.tsmachines/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .talismanrc
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
machines/auth.test.ts (1)
153-166: ⚡ Quick winExtract a shared
withConfigtest helper for the repeated mock maps.These mocked
actionsblocks are now duplicated across most transition specs, so any future auth-machine action rename/addition will require touching a dozen tests. A small factory likebuildTestMachine({services, guards, actions})ormockAuthConfig()would keep the newUPGRADE_PASSCODE_HASHcases aligned with the rest of the suite and reduce drift.Also applies to: 177-193, 317-329, 342-354, 367-379, 395-407, 423-435, 448-460, 473-485, 498-510, 528-540, 558-570
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machines/auth.test.ts` around lines 153 - 166, Tests duplicate the same mocked actions map passed to authMachine.withConfig across many specs; create a shared test helper (e.g., mockAuthConfig or buildTestMachine) that returns authMachine.withConfig populated with the common actions map and optional overrides for services/guards/actions, then update each test (currently calling authMachine.withConfig directly) to use this helper so future action renames only need one change; reference the existing authMachine.withConfig call and the actions mock keys (requestStoredContext, storeContext, setContext, setBiometrics, setLanguage, setAppSetupComplete, setPasscodeSalt, setOnboardingDone, setInitialDownloadDone, setTourGuide, setIsToggleFromSettings) when implementing the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@machines/auth.test.ts`:
- Around line 153-166: Tests duplicate the same mocked actions map passed to
authMachine.withConfig across many specs; create a shared test helper (e.g.,
mockAuthConfig or buildTestMachine) that returns authMachine.withConfig
populated with the common actions map and optional overrides for
services/guards/actions, then update each test (currently calling
authMachine.withConfig directly) to use this helper so future action renames
only need one change; reference the existing authMachine.withConfig call and the
actions mock keys (requestStoredContext, storeContext, setContext,
setBiometrics, setLanguage, setAppSetupComplete, setPasscodeSalt,
setOnboardingDone, setInitialDownloadDone, setTourGuide,
setIsToggleFromSettings) when implementing the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf4c599c-f2eb-44de-94d3-9286336b1a01
📒 Files selected for processing (2)
.talismanrcmachines/auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .talismanrc
|
@coderabbitai review the latest commit with you findings |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch key implementation files
echo "=== shared/constants.ts ==="
fd -e ts constants.ts shared/ --exec cat {}
echo ""
echo "=== shared/commonUtil.ts ==="
fd -e ts commonUtil.ts shared/ --exec cat {}
echo ""
echo "=== components/PasscodeVerify.tsx ==="
fd -e tsx PasscodeVerify.tsx components/ --exec cat {}
echo ""
echo "=== screens/PasscodeScreen.tsx ==="
fd -e tsx PasscodeScreen.tsx screens/ --exec cat {}
echo ""
echo "=== screens/PasscodeScreenController.ts ==="
fd -e ts PasscodeScreenController.ts screens/ --exec cat {}
echo ""
echo "=== machines/auth.ts (relevant sections) ==="
fd -e ts auth.ts machines/ --exec cat {}Length of output: 29997
Review: PR
|
…andling (inji#2399) Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
|
@coderabbitai review the latest commit. If anything is wrong or can be improved, suggest a solution. Also verify the changes align with the security fix described in issue and pr Description. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Issue `#2399` ==="
gh issue view 2399 --repo inji/inji-wallet 2>/dev/null || gh issue view 2399 --repo mosip/inji-wallet 2>/dev/null || echo "Could not fetch issue"
echo ""
echo "=== PR `#2464` latest commit info ==="
gh pr view 2464 --repo mosip/inji-wallet --json headRefName,headRefOid,commits 2>/dev/null | jq '.commits[-1]' 2>/dev/null || echo "Could not fetch PR info"
echo ""
echo "=== PR `#2464` changed files ==="
gh pr diff 2464 --repo mosip/inji-wallet --name-only 2>/dev/null || echo "Could not fetch diff"Length of output: 6143 🏁 Script executed: #!/bin/bash
echo "=== shared/constants.ts ==="
fd -e ts -g 'constants.ts' shared/ --exec cat {} 2>/dev/null || find . -path '*/shared/constants.ts' -exec cat {} \;
echo ""
echo "=== shared/commonUtil.ts ==="
fd -e ts -g 'commonUtil.ts' shared/ --exec cat {} 2>/dev/null || find . -path '*/shared/commonUtil.ts' -exec cat {} \;
echo ""
echo "=== components/PasscodeVerify.tsx ==="
find . -path '*/components/PasscodeVerify.tsx' -exec cat {} \;
echo ""
echo "=== screens/PasscodeScreen.tsx ==="
find . -path '*/screens/PasscodeScreen.tsx' -exec cat {} \;
echo ""
echo "=== screens/PasscodeScreenController.ts ==="
find . -path '*/screens/PasscodeScreenController.ts' -exec cat {} \;
echo ""
echo "=== machines/auth.ts ==="
find . -path '*/machines/auth.ts' -exec cat {} \;
echo ""
echo "=== shared/commonUtil.test.ts (PIN hash helpers section) ==="
find . -path '*/shared/commonUtil.test.ts' -exec cat {} \;
echo ""
echo "=== shared/constants.test.ts ==="
find . -path '*/shared/constants.test.ts' -exec cat {} \;Length of output: 52465
Latest Commit Review — PR
|
| # | Finding | Status |
|---|---|---|
| 1 | PIN_HASH_VERSION_SEPARATOR was dead code |
Fixed — now imported and used in both encodePinHash and parsePinHash |
| 2 | No error handling in setPasscode |
Fixed — try/catch with controller.setError(...) added |
| 6 | onError not called on unexpected errors in PasscodeVerify |
Fixed — props.onError(t('passcodeVerifyError', ...)) is now called in the outer catch |
🔴 Critical: Scope Mismatch with Issue #2399
Issue #2399 is specifically about argon2iConfigForPasswordAndPhoneNumber (backup/restore path) using 500 iterations instead of the recommended 600,000. The issue title, description, and code location all point to this config:
// shared/constants.ts — lines ~145-151 — UNCHANGED in this PR
export const argon2iConfigForPasswordAndPhoneNumber: Argon2iConfig = {
// TODO: expected iterations for hashing password and phone Number is 600000
iterations: 500, // ← still critically weak; NOT fixed by this PR
memory: 16 * 1024,
parallelism: 2,
hashLength: 30,
mode: 'argon2id',
};This PR instead fixes PIN/passcode hashing (argon2iConfig with 5 iterations → Argon2id v2), which is a valid and important fix, but it is a different vulnerability from the one described in #2399. The backup encryption path remains unaddressed.
The test in shared/constants.test.ts even mislabels the weakness:
it('argon2iConfigForPasswordAndPhoneNumber should have high iterations', () => {
expect(argon2iConfigForPasswordAndPhoneNumber.iterations).toBe(500); // 500 is NOT highRecommendation: Either update the PR description to correctly scope the fix (PIN hashing, not backup hashing), or open a follow-up issue/PR for argon2iConfigForPasswordAndPhoneNumber. The current PR should not close #2399, as the root issue remains open.
🟡 Medium: Remaining Open Items
1. SETUP_PASSCODE still unconditionally disables biometrics
In screens/PasscodeScreenController.ts:
SETUP_PASSCODE: () => {
authService.send(AuthEvents.SETUP_PASSCODE(passcode));
settingsService?.send(SettingsEvents.TOGGLE_BIOMETRIC_UNLOCK(false, true)); // fires for ALL setups
},A brand-new user setting up their PIN for the first time will also trigger TOGGLE_BIOMETRIC_UNLOCK(false, true), which is a side-effect that doesn't apply to them. Gate it:
SETUP_PASSCODE: () => {
authService.send(AuthEvents.SETUP_PASSCODE(passcode));
if (useSelector(authService, selectIsBiometricToggleFromSettings)) {
settingsService?.send(SettingsEvents.TOGGLE_BIOMETRIC_UNLOCK(false, true));
}
},2. PIN_KDF_PROFILES key type should be PinKdfVersion
// current — allows any string key, no compiler enforcement
export const PIN_KDF_PROFILES: Record<string, Argon2iConfig> = { ... };
// recommended
export const PIN_KDF_PROFILES: Record<PinKdfVersion, Argon2iConfig> = { ... };3. Magic string 'v1' in parsePinHash
The fallback { version: 'v1', hash: stored } hardcodes a string with no named constant. Add:
// shared/constants.ts
export const LEGACY_PIN_KDF_VERSION: PinKdfVersion = 'v1';
// shared/commonUtil.ts
import { LEGACY_PIN_KDF_VERSION, PIN_HASH_VERSION_SEPARATOR } from './constants';
export const parsePinHash = (stored: string): { version: string; hash: string } => {
if (!stored) return { version: LEGACY_PIN_KDF_VERSION, hash: stored };
const sep = stored.indexOf(PIN_HASH_VERSION_SEPARATOR);
if (sep === -1 || !/^v\d+$/.test(stored.slice(0, sep))) {
return { version: LEGACY_PIN_KDF_VERSION, hash: stored };
}
return { version: stored.slice(0, sep), hash: stored.slice(sep + 1) };
};🟢 Verified Correct
- Argon2id v2 parameters
{t=2, m=19456 KiB, p=1, len=32}satisfy OWASP 2024 mobile minimums ✅ - Migration is non-blocking — upgrade failure is swallowed/logged and login proceeds ✅
UPGRADE_PASSCODE_HASHis scoped tounauthorizedstate only — prevents replay from other states ✅parsePinHashdefensive handling — empty string, no separator, non-v<N>prefix all fall back tov1✅- 16-byte random salt via
generateSecureRandom— sufficient for Argon2id ✅ - Setup confirmation path uses
PasscodeVerifywithoutonUpgrade— correct, since a new hash is always the current version ✅ - Test suite covers round-trip, legacy, empty, and malformed inputs ✅
Summary Table
| Priority | Item | Status |
|---|---|---|
| 🔴 Critical | argon2iConfigForPasswordAndPhoneNumber (500 iter, backup path) — the actual #2399 issue — still unaddressed |
Open |
| 🟡 Medium | SETUP_PASSCODE unconditionally disables biometrics |
Open |
| 🟡 Medium | PIN_KDF_PROFILES key type should be PinKdfVersion |
Open |
| 🟡 Medium | Magic 'v1' string in parsePinHash |
Open |
| ✅ Fixed | PIN_HASH_VERSION_SEPARATOR dead code |
Resolved |
| ✅ Fixed | Error handling in setPasscode |
Resolved |
| ✅ Fixed | onError in PasscodeVerify catch block |
Resolved |
The PIN hashing upgrade itself is well-implemented. The main action needed is clarifying the relationship to issue #2399, since the backup encryption weakness (argon2iConfigForPasswordAndPhoneNumber) is still present. Would you like me to open a separate issue to track the backup path fix?
Signed-off-by: Kaushik Gupta <146950347+Kaushikgupta469@users.noreply.github.com>
1 participant
Replace weak Argon2i PIN hashing (iterations: 5) with Argon2id v2 (memory-hard, OWASP 2024). Existing users with legacy hashes are silently migrated to v2 on next successful login
Issue ticket number and link
Relates #2399
Summary by CodeRabbit