fix(passport-signature): guard ECDSA bruteforce returns and add safe fallbacks #1386
Conversation
WalkthroughPassport signature parsing logic is refactored to add conditional returns in brute-force signature determination based on hashAlgorithm validity. DSC certificate parsing now guards algorithm extraction with presence checks, and the main passport parser defers brute-force computation to occur after DSC parsing with automatic fallback to DSC-derived values. Changes
Sequence DiagramsequenceDiagram
participant Parser as parsePassportData
participant DSC as parseDscCertificateData
participant BruteForce as brutforceSignatureAlgorithm
Parser->>DSC: Parse DSC certificate data
activate DSC
DSC->>DSC: Guard: Check details validity
alt details has hashAlgorithm & signatureAlgorithm
DSC->>DSC: Use details values
else details incomplete
DSC->>DSC: Fallback to cscaParsed
end
DSC-->>Parser: Return dsc data (may be partial)
deactivate DSC
Parser->>BruteForce: Compute signature algorithm
activate BruteForce
BruteForce->>BruteForce: Check hashAlgorithm validity
alt hashAlgorithm is truthy
BruteForce-->>Parser: Return signature object
else hashAlgorithm falsy
BruteForce-->>Parser: Return undefined
end
deactivate BruteForce
Parser->>Parser: Resolve values (brute-force OR dsc OR defaults)
rect rgb(200, 220, 255)
Note over Parser: resolvedSignatureAlgorithm = bruteForce?.signatureAlgorithm OR dsc?.signatureAlgorithm OR default
Note over Parser: resolvedHashAlgorithm = bruteForce?.hashAlgorithm OR dsc?.hashAlgorithm OR default
Note over Parser: resolvedSaltLength = bruteForce?.saltLength OR 0
end
Parser->>Parser: Construct return object with resolved values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
common/src/utils/passports/passport_parsing/brutForcePassportSignature.ts (1)
13-53: Consider adding explicit return type annotation for type safety.The function
brutforceSignatureAlgorithmcan returnundefinedwhen no matching algorithm is found (which is intentional per the PR objectives), but lacks an explicit return type. This could lead to type safety issues downstream.Consider adding an explicit return type:
-export function brutforceSignatureAlgorithm(passportData: PassportData) { +export function brutforceSignatureAlgorithm(passportData: PassportData): { signatureAlgorithm: string; hashAlgorithm: string; saltLength: number } | undefined {common/src/utils/passports/passport_parsing/brutForceDscSignature.ts (1)
14-45: Consider adding explicit return type annotation for type safety.Similar to
brutforceSignatureAlgorithm, this function lacks an explicit return type and can returnundefinedwhen no matching algorithm is found.Consider adding an explicit return type:
-export function brutforceSignatureAlgorithmDsc(dsc: CertificateData, csca: CertificateData) { +export function brutforceSignatureAlgorithmDsc(dsc: CertificateData, csca: CertificateData): { signatureAlgorithm: string; hashAlgorithm: string; saltLength: number } | undefined {common/src/utils/passports/passport_parsing/parsePassportData.ts (1)
126-139: Simplify redundant nullish coalescing on line 130.The
?? undefinedon line 130 is redundant since the value is alreadyundefinedifbrutForcedPublicKeyDetailsisundefinedor lackssaltLength. This inconsistency with lines 128-129 adds unnecessary noise.Apply this diff for consistency:
const brutForcedPublicKeyDetails = brutforceSignatureAlgorithm(passportData); let resolvedSignatureAlgorithm = brutForcedPublicKeyDetails?.signatureAlgorithm as string | undefined; let resolvedSignedAttrHashFunction = brutForcedPublicKeyDetails?.hashAlgorithm as string | undefined; - let resolvedSaltLength = (brutForcedPublicKeyDetails?.saltLength as number | undefined) ?? undefined; + let resolvedSaltLength = brutForcedPublicKeyDetails?.saltLength as number | undefined;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/utils/passports/passport_parsing/brutForceDscSignature.ts(1 hunks)common/src/utils/passports/passport_parsing/brutForcePassportSignature.ts(1 hunks)common/src/utils/passports/passport_parsing/parseDscCertificateData.ts(1 hunks)common/src/utils/passports/passport_parsing/parsePassportData.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
common/src/utils/passports/passport_parsing/brutForceDscSignature.tscommon/src/utils/passports/passport_parsing/brutForcePassportSignature.tscommon/src/utils/passports/passport_parsing/parseDscCertificateData.tscommon/src/utils/passports/passport_parsing/parsePassportData.ts
common/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
common/src/**/*.{ts,tsx,js,jsx}: Review shared utilities for:
- Reusability and modular design
- Type safety and error handling
- Side-effect management
- Documentation and naming clarity
Files:
common/src/utils/passports/passport_parsing/brutForceDscSignature.tscommon/src/utils/passports/passport_parsing/brutForcePassportSignature.tscommon/src/utils/passports/passport_parsing/parseDscCertificateData.tscommon/src/utils/passports/passport_parsing/parsePassportData.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to noir/crates/dg1/src/dg1/dg1.nr : Document Verification Processing validates international travel documents using ICAO standards, processes DSC verification, and handles multiple signature algorithms.
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Always validate certificates for passport data.
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to contracts/contracts/IdentityVerificationHubImplV2.sol : Identity Verification Hub should manage multi-step verification for passports and EU ID cards, handle document attestation with zero-knowledge proofs, and implement verification paths: E-PASSPORT and EU_ID_CARD.
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
common/src/utils/passports/passport_parsing/brutForcePassportSignature.tscommon/src/utils/passports/passport_parsing/parsePassportData.ts
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Always validate certificates for passport data.
Applied to files:
common/src/utils/passports/passport_parsing/parsePassportData.ts
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to noir/crates/dg1/src/dg1/dg1.nr : Document Verification Processing validates international travel documents using ICAO standards, processes DSC verification, and handles multiple signature algorithms.
Applied to files:
common/src/utils/passports/passport_parsing/parsePassportData.ts
🧬 Code graph analysis (1)
common/src/utils/passports/passport_parsing/parsePassportData.ts (1)
common/src/utils/passports/passport_parsing/brutForcePassportSignature.ts (1)
brutforceSignatureAlgorithm(13-53)
| if (details && details.hashAlgorithm && details.signatureAlgorithm) { | ||
| cscaHashAlgorithm = details.hashAlgorithm; | ||
| cscaSignatureAlgorithm = details.signatureAlgorithm; | ||
| cscaSaltLength = details.saltLength; | ||
| } else if (cscaParsed) { | ||
| cscaHashAlgorithm = cscaParsed.hashAlgorithm; | ||
| cscaSignatureAlgorithm = cscaParsed.signatureAlgorithm; | ||
| cscaSaltLength = 0; | ||
| } |
There was a problem hiding this comment.
Guard against undefined cscaSaltLength in return object.
The fallback logic doesn't cover all cases. If both conditions fail (no valid details and no cscaParsed), cscaSaltLength remains undefined, but the interface DscCertificateMetaData expects it to be a number (line 13). This creates a type safety issue.
Apply this diff to ensure a safe default:
if (details && details.hashAlgorithm && details.signatureAlgorithm) {
cscaHashAlgorithm = details.hashAlgorithm;
cscaSignatureAlgorithm = details.signatureAlgorithm;
cscaSaltLength = details.saltLength;
} else if (cscaParsed) {
cscaHashAlgorithm = cscaParsed.hashAlgorithm;
cscaSignatureAlgorithm = cscaParsed.signatureAlgorithm;
cscaSaltLength = 0;
+ } else {
+ // Fallback to safe defaults if no valid data is available
+ cscaSaltLength = 0;
}Alternatively, update the interface to allow undefined values if that's the intended behavior.
📝 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.
| if (details && details.hashAlgorithm && details.signatureAlgorithm) { | |
| cscaHashAlgorithm = details.hashAlgorithm; | |
| cscaSignatureAlgorithm = details.signatureAlgorithm; | |
| cscaSaltLength = details.saltLength; | |
| } else if (cscaParsed) { | |
| cscaHashAlgorithm = cscaParsed.hashAlgorithm; | |
| cscaSignatureAlgorithm = cscaParsed.signatureAlgorithm; | |
| cscaSaltLength = 0; | |
| } | |
| if (details && details.hashAlgorithm && details.signatureAlgorithm) { | |
| cscaHashAlgorithm = details.hashAlgorithm; | |
| cscaSignatureAlgorithm = details.signatureAlgorithm; | |
| cscaSaltLength = details.saltLength; | |
| } else if (cscaParsed) { | |
| cscaHashAlgorithm = cscaParsed.hashAlgorithm; | |
| cscaSignatureAlgorithm = cscaParsed.signatureAlgorithm; | |
| cscaSaltLength = 0; | |
| } else { | |
| // Fallback to safe defaults if no valid data is available | |
| cscaSaltLength = 0; | |
| } |
🤖 Prompt for AI Agents
common/src/utils/passports/passport_parsing/parseDscCertificateData.ts around
lines 38-46: cscaSaltLength can remain undefined if neither details nor
cscaParsed is present, violating the DscCertificateMetaData number type; ensure
a safe numeric default by initializing cscaSaltLength to 0 when you declare it
or add an explicit final else that sets cscaSaltLength = 0 before the return (or
alternatively change the DscCertificateMetaData type to allow undefined if that
is intended).
| signedAttrHashFunction: resolvedSignedAttrHashFunction, | ||
| signatureAlgorithm: resolvedSignatureAlgorithm, | ||
| saltLength: resolvedSaltLength || 0, |
There was a problem hiding this comment.
Add fallbacks for signedAttrHashFunction and signatureAlgorithm to prevent undefined values.
While saltLength has a fallback to 0 (line 158), signedAttrHashFunction (line 156) and signatureAlgorithm (line 157) can be undefined if both brute-force and parsedDsc fail. The interface PassportMetadata likely expects these to be strings, creating a type mismatch.
Apply this diff to add safe fallbacks:
signedAttrSize: passportData.signedAttr?.length || 0,
- signedAttrHashFunction: resolvedSignedAttrHashFunction,
- signatureAlgorithm: resolvedSignatureAlgorithm,
+ signedAttrHashFunction: resolvedSignedAttrHashFunction || 'unknown',
+ signatureAlgorithm: resolvedSignatureAlgorithm || 'unknown',
saltLength: resolvedSaltLength || 0,This aligns with the existing pattern of using 'unknown' as a fallback (e.g., line 159).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In common/src/utils/passports/passport_parsing/parsePassportData.ts around lines
156 to 158, add safe fallbacks for signedAttrHashFunction and signatureAlgorithm
to avoid undefined values: if resolvedSignedAttrHashFunction or
resolvedSignatureAlgorithm are undefined, default them to the string 'unknown'
(similar to the existing pattern used for other fields) while keeping saltLength
defaulting to 0.
1 participant
Rationale:
Note
Guards ECDSA brute-force to return only on successful hash detection and adds safe DSC/CSCA fallbacks in parsing to ensure valid signature/hash/salt metadata.
brutForcePassportSignature.tsandbrutForceDscSignature.tsto return only when ahashAlgorithmis found.rsapss.parsePassportData.ts: resolvesignatureAlgorithm,signedAttrHashFunction, andsaltLengthfrom brute-force; if missing, fall back toparseCertificateSimpleoutputs withsaltLength=0.parseDscCertificateData.ts: after brute-force, if details are missing, fall back to parsed CSCA (cscaParsed) algorithms and setsaltLength=0; ensure curve/exponent and bit-length derived from parsed CSCA.Written by Cursor Bugbot for commit af7ca24. This will update automatically on new commits. Configure here.
Summary by CodeRabbit