fix: avoid early DKIM verification failure with multiple signatures#294
fix: avoid early DKIM verification failure with multiple signatures#294Olexandr88 wants to merge 1 commit intozkemail:mainfrom
Conversation
📝 WalkthroughWalkthroughThe DKIM verification logic was changed to try multiple candidate signatures per signing domain: DNS/archive key absence now returns an empty array; candidates are filtered by domain and verified in sequence, returning on first success and emitting specific errors if none succeed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/src/dkim/index.ts
🧰 Additional context used
🪛 Biome (2.1.2)
packages/helpers/src/dkim/index.ts
[error] 173-173: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (1)
packages/helpers/src/dkim/index.ts (1)
126-128: Behavioral change is accurate: returning empty array instead of throwing, with proper downstream error handling.When the resolver returns an empty array,
getPublicKey()throws anENODATAerror which is caught byDkimVerifierand converted to aneutralverification result. This allows the verification loop to continue processing other signatures rather than failing immediately, enabling the PR's goal of handling multiple signatures without early failures. The final result still fails appropriately if no valid signatures are found for the target domain.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/helpers/src/dkim/index.ts (2)
161-169: Critical: Loop returns the first candidate without verifying it passes.The try-catch block only wraps a simple assignment (
candidate.headers = dkimVerifier.headers) and a return statement, neither of which will throw an error. This means the loop will always return the first candidate without checking if its verification actually passed, defeating the entire purpose of this PR.The actual verification check happens in the outer
verifyDKIMSignaturefunction at line 78, which is too late to try alternative candidates.🔎 Proposed fix to verify candidate status before returning
let lastError: Error | undefined; for (const candidate of candidateResults) { - try { - candidate.headers = dkimVerifier.headers; - return candidate; - } catch (e) { - lastError = e as Error; - continue; + candidate.headers = dkimVerifier.headers; + + // Check if this candidate passed verification + if (candidate.status?.result === 'pass') { + return candidate; } + + // Capture the failure reason and try the next candidate + lastError = new Error( + `DKIM verification failed: ${candidate.status?.comment || 'unknown error'}` + ); }This ensures that only a successfully verified candidate is returned, and verification failures are captured to provide meaningful error messages.
171-173: Critical: Missing closing brace fortryVerifyDKIMfunction.The file ends unexpectedly at line 173 without closing the
tryVerifyDKIMfunction that begins at line 99. This syntax error will prevent the module from loading.🔎 Proposed fix
throw lastError ?? new Error( `All DKIM signatures failed for domain ${domainToVerifyDKIM}` ); +}
🧹 Nitpick comments (1)
packages/helpers/src/dkim/index.ts (1)
149-157: LGTM! Clear error handling for missing DKIM signatures.The filtering logic correctly identifies candidate results for the specified domain and provides a clear error message when no matching signatures are found.
Optional: Consider improving type safety
If possible, replace the
anytype with a proper type definition for the DKIM result objects to improve type safety:- const candidateResults = dkimVerifier.results.filter( - (d: any) => d.signingDomain === domainToVerifyDKIM - ); + const candidateResults = dkimVerifier.results.filter( + (d) => d.signingDomain === domainToVerifyDKIM + );This assumes
dkimVerifier.resultshas proper typing. If not, consider adding type definitions for the DkimVerifier class.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/src/dkim/index.ts
🧰 Additional context used
🪛 Biome (2.1.2)
packages/helpers/src/dkim/index.ts
[error] 173-173: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (1)
packages/helpers/src/dkim/index.ts (1)
126-128: The behavior change here is intentional and properly handled. Returning an empty array when no DNS/archive keys are found allows theDkimVerifierto continue processing and report aneutralverification status instead of throwing immediately. This enables the parent function to provide a complete verification result with the specific reason for failure.The error handling is already in place: when
getPublicKeyreceives an empty array, it throws anENODATAerror that theDkimVerifiercatches and converts to aneutralstatus with the comment'no key'. The parentverifyDKIMSignaturefunction then checks if the result is not'pass'and throws an error that includes this comment, ensuring users see the specific reason for failure ("no key") rather than a generic DNS resolution error.No action needed—the design correctly supports the PR objective of handling multiple signatures while propagating appropriate error messages.
1 participant
Avoid early DKIM verification failure when multiple signatures are present.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.