chore: zk regex version upgrade #264
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
wryonik
force-pushed
the
chore/zk-regex-version-upgrade
branch
from
b8e7ab2 to
dd6e2e2
Compare
WalkthroughThe pull request refactors the body hash regex extraction mechanism in EmailVerifier by introducing a new compatibility layer with updated component signatures. Test suites are updated to handle a new Changes
Sequence DiagramsequenceDiagram
autonumber
participant Test as Test Suite
participant EmailVerifier as EmailVerifier Circuit
participant BodyHashRegex as BodyHashRegex Component
participant Witness as Witness Output
Note over Test,Witness: Old Flow
Test->>EmailVerifier: ignoreBodyHashCheck: false
EmailVerifier->>EmailVerifier: Extract body hash directly
EmailVerifier->>Witness: Output circuit result
Test->>Witness: circuit.assertOut(publicOutput)
Note over Test,Witness: New Flow
Test->>EmailVerifier: ignoreBodyHashCheck: true
alt Body data available
EmailVerifier->>BodyHashRegex: Pass (matchStart, matchLength, currStates, nextStates, captureGroupIds, indices)
BodyHashRegex->>BodyHashRegex: Extract 44-byte capture
BodyHashRegex->>EmailVerifier: Return capture1, isValid
else Body data unavailable
EmailVerifier->>EmailVerifier: Skip body hash extraction
Note over EmailVerifier: Guard: console.log "Skipping body hash"
end
EmailVerifier->>Witness: Write witness array
Test->>Witness: Manual witness extraction (slice at index 4)
Test->>Witness: String comparison via toEqual
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve a significant but coherent refactoring: new component signatures with updated wiring, a new compatibility layer template, extended test suite modifications following a consistent pattern, and input generation logic updates. While affecting multiple files across different areas (circuits, tests, helpers), the changes follow a clear architectural pattern, with repetitive test modifications that reduce per-file cognitive load. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/circuits/tests/email-verifier.test.ts (1)
186-205: Critical: Body hash tampering test is disabled instead of fixed.The TODO on line 186 indicates this test fails to detect tampered body hashes. Rather than fixing the validation logic, the test was disabled with
ignoreBodyHashCheck: trueand a guard. This represents a potential security gap.Body hash validation is a critical DKIM security feature. This test should either:
- Be fixed to properly validate body hash tampering, or
- Be removed entirely with a clear explanation of why body hash validation is no longer required
Do not leave a failing security test disabled with a TODO. The current state suggests body hash tampering might not be detected.
🧹 Nitpick comments (1)
packages/circuits/package.json (1)
10-10: Exact version pinning removes patch update flexibility.The dependency changed from
"^2.3.1"to"2.3.2"(exact pinning). While this ensures consistency, it prevents automatic patch updates that might include bug fixes.Also note: This version (2.3.2) differs from the helpers package which uses 2.2.2-alpha.0 (flagged in packages/helpers/package.json review).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
.github/workflows/action.yml(1 hunks)packages/circuits/email-verifier.circom(3 hunks)packages/circuits/package.json(2 hunks)packages/circuits/tests/email-verifier-with-body-mask.test.ts(1 hunks)packages/circuits/tests/email-verifier-with-header-mask.test.ts(2 hunks)packages/circuits/tests/email-verifier-with-qp-encoded-sha-precompute-selector.test.ts(1 hunks)packages/circuits/tests/email-verifier-with-soft-line-breaks.test.ts(1 hunks)packages/circuits/tests/email-verifier.test.ts(11 hunks)packages/circuits/tests/test-circuits/email-verifier-test.circom(1 hunks)packages/circuits/tests/test-circuits/email-verifier-with-body-mask-test.circom(1 hunks)packages/circuits/tests/test-circuits/email-verifier-with-header-mask-test.circom(1 hunks)packages/circuits/tests/test-circuits/email-verifier-with-soft-line-breaks-test.circom(1 hunks)packages/circuits/utils/body_hash_regex_compat.circom(1 hunks)packages/helpers/package.json(1 hunks)packages/helpers/src/input-generators.ts(3 hunks)packages/helpers/src/sha-utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/helpers/src/input-generators.ts (1)
packages/helpers/src/constants.ts (1)
MAX_HEADER_PADDED_BYTES(2-2)
⏰ 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: run_circuit_tests
🔇 Additional comments (26)
.github/workflows/action.yml (1)
29-33: Circom v2.1.9 download is compatible and correctly configured.Verification confirmed: Circom v2.1.9 (released April 23, 2024) is a valid release with no breaking changes. It requires Circom 2 (Circom v2.x), which matches the repository's dependency on zk-regex-circom 2.3.2. The wget URL format and pinned version are correct.
packages/helpers/src/sha-utils.ts (1)
68-68: LGTM: Type assertion for downstream compatibility.The type assertion aligns the return type with the expanded body hash data structures introduced in this PR without affecting runtime behavior.
packages/helpers/src/input-generators.ts (3)
5-6: LGTM: New imports support body hash regex functionality.The imports correctly bring in the zk-regex compiler utilities and the body hash graph JSON resource needed for the new body hash extraction mechanism.
20-26: LGTM: Type extensions for body hash regex inputs.The new optional fields are well-structured and appropriately typed for the body hash regex extraction mechanism. The optional nature maintains backward compatibility.
258-278: Add null safety guards for body hash circuit inputs.The code directly accesses properties from
bodyHashCircuitInputswithout verifying they exist. While optional chaining (?.) is used for array methods, if the arrays areundefined, calling.map()on them will throw a runtime error.Specific concerns:
- Lines 273-277: If
currStates,nextStates,captureGroupIds,captureGroupStarts, orcaptureGroupStartIndicesare undefined, the optional chaining returns undefined but doesn't prevent the.map()call, which would throw- Line 277:
bodyHashCircuitInputs.matchStartis used in arithmetic without checking if it's defined, which could produceNaNApply defensive checks:
// Flatten body hash inputs into direct circuit inputs circuitInputs.bodyHashMatchStart = bodyHashCircuitInputs.matchStart; circuitInputs.bodyHashMatchLength = bodyHashCircuitInputs.matchLength; -circuitInputs.bodyHashCurrStates = bodyHashCircuitInputs.currStates?.map((s: any) => Number(s)); -circuitInputs.bodyHashNextStates = bodyHashCircuitInputs.nextStates?.map((s: any) => Number(s)); -circuitInputs.bodyHashCaptureGroup1Id = bodyHashCircuitInputs.captureGroupIds?.[0]?.map((s: any) => Number(s)); -circuitInputs.bodyHashCaptureGroup1Start = bodyHashCircuitInputs.captureGroupStarts?.[0]?.map((s: any) => Number(s)); -circuitInputs.bodyHashCaptureGroupStartIndices = bodyHashCircuitInputs.captureGroupStartIndices?.map((s: any) => Number(s) - bodyHashCircuitInputs.matchStart); +circuitInputs.bodyHashCurrStates = bodyHashCircuitInputs.currStates ? bodyHashCircuitInputs.currStates.map((s: any) => Number(s)) : undefined; +circuitInputs.bodyHashNextStates = bodyHashCircuitInputs.nextStates ? bodyHashCircuitInputs.nextStates.map((s: any) => Number(s)) : undefined; +circuitInputs.bodyHashCaptureGroup1Id = bodyHashCircuitInputs.captureGroupIds?.[0] ? bodyHashCircuitInputs.captureGroupIds[0].map((s: any) => Number(s)) : undefined; +circuitInputs.bodyHashCaptureGroup1Start = bodyHashCircuitInputs.captureGroupStarts?.[0] ? bodyHashCircuitInputs.captureGroupStarts[0].map((s: any) => Number(s)) : undefined; +circuitInputs.bodyHashCaptureGroupStartIndices = bodyHashCircuitInputs.captureGroupStartIndices && bodyHashCircuitInputs.matchStart !== undefined + ? bodyHashCircuitInputs.captureGroupStartIndices.map((s: any) => Number(s) - bodyHashCircuitInputs.matchStart) + : undefined;Likely an incorrect or invalid review comment.
packages/circuits/tests/email-verifier-with-soft-line-breaks.test.ts (1)
40-40: Verify that disabling body hash check is intentional.The test now skips body hash validation by setting
ignoreBodyHashCheck: true. Given this PR introduces new body hash regex wiring, ensure this change is intentional and not masking incomplete implementation.Consider:
- Is the new body hash regex mechanism not yet compatible with this test scenario?
- Should this test be updated to validate the new body hash extraction instead of bypassing it?
packages/circuits/tests/test-circuits/email-verifier-with-header-mask-test.circom (1)
5-5: Verify the fifth parameter change from 0 to 1.The EmailVerifier instantiation changed the fifth argument from 0 to 1. Based on the PR context, this likely enables a body masking feature. Ensure this change aligns with the test's intent (which is focused on header masking based on the filename).
packages/circuits/tests/email-verifier-with-qp-encoded-sha-precompute-selector.test.ts (1)
40-40: Verify that disabling body hash check is intentional.Similar to the soft line breaks test, this test now bypasses body hash validation. Given the test's focus on QP-encoded SHA precompute selectors, ensure the new body hash regex mechanism is compatible with this scenario or document why the check must be disabled.
packages/circuits/tests/test-circuits/email-verifier-test.circom (1)
5-5: Verify the fifth parameter change from 0 to 1.The EmailVerifier instantiation changed the fifth argument from 0 to 1, consistent with changes in other test circuits. Confirm this parameter change is correct for this test scenario.
packages/helpers/package.json (1)
18-19: Verify alpha version stability and confirm version mismatch is intentional.The version mismatch is confirmed: packages/helpers uses
@zk-email/[email protected]while packages/circuits uses@zk-email/[email protected]. Additionally, helpers also depends on@zk-email/[email protected](alpha).Clarify whether:
- This version discrepancy is intentional and tested for compatibility
- The alpha versions are production-ready or if stable versions should be used
packages/circuits/tests/test-circuits/email-verifier-with-body-mask-test.circom (1)
5-5: LGTM - Consistent with body hash check bypass.The parameter change aligns with the test suite update that sets
ignoreBodyHashCheck: true, supporting the new optional body hash validation flow.packages/circuits/tests/test-circuits/email-verifier-with-soft-line-breaks-test.circom (1)
5-5: LGTM - Consistent with body hash check bypass.The parameter change aligns with the broader refactor to support optional body hash validation.
packages/circuits/tests/email-verifier-with-header-mask.test.ts (2)
43-43: LGTM - Test configuration updated for optional body hash check.Setting
ignoreBodyHashCheck: trueis appropriate for this header-masking-focused test.
56-63: Verify the hardcoded witness index assumption.The hardcoded
maskedHeaderStartIndex = 4assumes a fixed witness layout. If the circuit's public output ordering changes (e.g., additional outputs added before the masked header), this test will silently produce incorrect results.Consider verifying the witness layout programmatically or document the expected witness structure more explicitly. You could add an assertion to validate the witness length matches expectations:
+ // Verify witness has expected structure + expect(witness.length).toBeGreaterThanOrEqual(maskedHeaderStartIndex + expectedMaskedHeader.length); + const maskedHeaderStartIndex = 4; // Skip first 4 elements (1,pubkeyHash, shaHi, shaLo)packages/circuits/tests/email-verifier-with-body-mask.test.ts (2)
43-43: LGTM - Test configuration updated.Setting
ignoreBodyHashCheck: truealigns with the body-masking test focus.
62-69: Same hardcoded witness index concern as header mask test.The hardcoded
maskedBodyStartIndex = 4assumes a fixed witness layout that could break silently if circuit outputs change.Consider adding witness length validation as suggested in the header mask test review.
packages/circuits/tests/email-verifier.test.ts (2)
40-40: LGTM - Appropriate flag for header-only tests.Setting
ignoreBodyHashCheck: trueis appropriate for tests focused on header verification without body validation.Also applies to: 55-55
233-234: Witness index access updated.The assertion now uses
witness[1]directly. While this works, it shares the same brittleness concerns as other hardcoded witness indices throughout the test suite.packages/circuits/utils/body_hash_regex_compat.circom (5)
10-21: LGTM - Template interface design is sound.The template parameters and signal declarations are well-structured. The 44-byte capture length correctly accommodates a Base64-encoded 32-byte hash with padding.
23-27: LGTM - Length validation enforces fixed capture size.The constraint properly enforces that only 44-byte matches are accepted, preventing variable-length captures.
29-33: LGTM - Capture extraction uses SelectRegexReveal.The extraction logic properly wires the haystack and start index. The correctness relies on the
SelectRegexRevealcomponent from./regex.circom.
35-43: LGTM - Alignment validation ensures consistency.The constraint ensures the capture group start index matches the match start position, maintaining consistency across input signals.
52-54: LGTM - isValid output follows ZK circuit pattern.The
isValid <== 1output is appropriate for a ZK circuit that fails witness generation on invalid inputs rather than returning a boolean validation result. The actual validation occurs through the=== 1constraints onisLen44andisStartAligned.packages/circuits/email-verifier.circom (3)
5-5: LGTM - Include path updated to compatibility wrapper.The change from the external package to the local compatibility wrapper aligns with the zk-regex version upgrade strategy.
114-122: LGTM - New body hash regex inputs properly scoped.The new input signals are correctly placed within the
ignoreBodyHashCheck != 1block and properly sized to match theBodyHashRegextemplate expectations.Note: This confirms why tests with
ignoreBodyHashCheck: truedon't have body-related data - these inputs are conditionally compiled out of the circuit.
133-146: LGTM - BodyHashRegex component properly integrated.The new component is correctly instantiated and wired:
- Template parameters match the input signal sizes
- All inputs are properly connected
- isValid assertion ensures validation passes
- Base64 decoding of capture1 produces the expected 32-byte hash
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | ||
| if (!emailVerifierInputs.emailBody) { | ||
| console.log("Skipping body mask test - body data not available when ignoreBodyHashCheck is true"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test may silently skip due to configuration it sets.
The test sets ignoreBodyHashCheck: true on line 43, which may cause emailBody to be unavailable, triggering this guard to skip the test. This creates a scenario where the test effectively disables itself without failing.
Either:
- Remove
ignoreBodyHashCheck: trueif body data is required for this test, or - Update the input generator to provide
emailBodyeven whenignoreBodyHashCheckis true, or - Change the guard to fail explicitly rather than silently skipping:
- if (!emailVerifierInputs.emailBody) {
- console.log("Skipping body mask test - body data not available when ignoreBodyHashCheck is true");
- return;
- }
+ expect(emailVerifierInputs.emailBody).toBeDefined();📝 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.
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | |
| if (!emailVerifierInputs.emailBody) { | |
| console.log("Skipping body mask test - body data not available when ignoreBodyHashCheck is true"); | |
| return; | |
| } | |
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | |
| expect(emailVerifierInputs.emailBody).toBeDefined(); |
🤖 Prompt for AI Agents
In packages/circuits/tests/email-verifier-with-body-mask.test.ts around lines 49
to 53, the test can silently skip because the test sets ignoreBodyHashCheck:
true (line ~43) which causes emailBody to be undefined and triggers the early
return; fix by either removing ignoreBodyHashCheck: true if this test requires
body data, or update the input generator to always populate emailBody even when
ignoreBodyHashCheck is true, or replace the silent skip with an explicit test
failure/assertion that emailBody must be present (e.g., throw or expect
toBeDefined) so the CI fails instead of quietly skipping the test.
| ignoreBodyHashCheck: true, | ||
| } | ||
| ); | ||
|
|
||
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | ||
| if (!emailVerifierInputs.emailBody) { | ||
| console.log("Skipping body tamper test - body data not available when ignoreBodyHashCheck is true"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test for body tampering detection is effectively disabled.
This test validates that the circuit rejects tampered body data, but it sets ignoreBodyHashCheck: true (line 140), which may cause the body validation it's testing to be skipped. The guard then returns early without verification.
Remove ignoreBodyHashCheck: true from this test to ensure body tampering is actually validated:
const emailVerifierInputs = generateEmailVerifierInputsFromDKIMResult(
dkim,
{
maxHeadersLength: 640,
maxBodyLength: 768,
- ignoreBodyHashCheck: true,
}
);
-
- // Skip this test when body data isn't available due to ignoreBodyHashCheck
- if (!emailVerifierInputs.emailBody) {
- console.log("Skipping body tamper test - body data not available when ignoreBodyHashCheck is true");
- return;
- }📝 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.
| ignoreBodyHashCheck: true, | |
| } | |
| ); | |
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | |
| if (!emailVerifierInputs.emailBody) { | |
| console.log("Skipping body tamper test - body data not available when ignoreBodyHashCheck is true"); | |
| return; | |
| } | |
| const emailVerifierInputs = generateEmailVerifierInputsFromDKIMResult( | |
| dkim, | |
| { | |
| maxHeadersLength: 640, | |
| maxBodyLength: 768, | |
| } | |
| ); |
🤖 Prompt for AI Agents
In packages/circuits/tests/email-verifier.test.ts around lines 140 to 148, the
test currently sets ignoreBodyHashCheck: true which disables the body-hash
validation and then early-returns when emailBody is missing, effectively
skipping the body-tamper assertion; remove the ignoreBodyHashCheck: true entry
from the test input so the circuit performs body validation, ensure
emailVerifierInputs.emailBody is provided (or create a proper body fixture) and
remove the guard/early return so the tampered-body path runs and the test can
assert that tampering is rejected.
| ignoreBodyHashCheck: true, | ||
| } | ||
| ); | ||
| emailVerifierInputs.emailBody![768 - 1] = "1"; | ||
|
|
||
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | ||
| if (!emailVerifierInputs.emailBody) { | ||
| console.log("Skipping body padding tamper test - body data not available when ignoreBodyHashCheck is true"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test for body padding tampering detection is effectively disabled.
Same issue as the body tampering test - this test should validate padding integrity but may skip validation due to ignoreBodyHashCheck: true.
Remove ignoreBodyHashCheck: true to ensure the test validates padding tampering:
const emailVerifierInputs = generateEmailVerifierInputsFromDKIMResult(
dkimResult,
{
maxHeadersLength: 640,
maxBodyLength: 768,
- ignoreBodyHashCheck: true,
}
);
-
- // Skip this test when body data isn't available due to ignoreBodyHashCheck
- if (!emailVerifierInputs.emailBody) {
- console.log("Skipping body padding tamper test - body data not available when ignoreBodyHashCheck is true");
- return;
- }📝 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.
| ignoreBodyHashCheck: true, | |
| } | |
| ); | |
| emailVerifierInputs.emailBody![768 - 1] = "1"; | |
| // Skip this test when body data isn't available due to ignoreBodyHashCheck | |
| if (!emailVerifierInputs.emailBody) { | |
| console.log("Skipping body padding tamper test - body data not available when ignoreBodyHashCheck is true"); | |
| return; | |
| } | |
| const emailVerifierInputs = generateEmailVerifierInputsFromDKIMResult( | |
| dkimResult, | |
| { | |
| maxHeadersLength: 640, | |
| maxBodyLength: 768, | |
| } | |
| ); |
🤖 Prompt for AI Agents
In packages/circuits/tests/email-verifier.test.ts around lines 165 to 173, the
body padding tamper test is effectively disabled because ignoreBodyHashCheck:
true causes emailBody to be unavailable and the test to early-return; remove the
ignoreBodyHashCheck: true option (or set it to false) so emailBody is present
and the test exercises padding integrity, and delete the conditional
early-return that skips the test when emailBody is missing.
| signal sumStates; | ||
| sumStates <== 0; | ||
| for (var i = 0; i < maxStatesLength; i++) { | ||
| sumStates <== sumStates + currStates[i] + nextStates[i] + captureGroup1Id[i] + captureGroup1Start[i]; | ||
| } |
There was a problem hiding this comment.
Critical: Invalid constraint accumulation in state sum loop.
Line 50 uses <== (constrain and assign) inside a loop accumulation, which is incorrect. Each iteration creates a new constraint that sumStates equals the current sum, overwriting the previous constraint. This doesn't properly accumulate or constrain the states.
If the intent is to force all inputs to be read (preventing unconstrained inputs), use a proper accumulation pattern:
- signal sumStates;
- sumStates <== 0;
+ var sumStates = 0;
for (var i = 0; i < maxStatesLength; i++) {
- sumStates <== sumStates + currStates[i] + nextStates[i] + captureGroup1Id[i] + captureGroup1Start[i];
+ sumStates += currStates[i] + nextStates[i] + captureGroup1Id[i] + captureGroup1Start[i];
}
+ // If you need to constrain this, use a signal outside the loop
+ signal sumStatesSignal <== sumStates;However, if the goal is just to ensure inputs are read, the compiler should handle this. Clarify the intent of this loop.
📝 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.
| signal sumStates; | |
| sumStates <== 0; | |
| for (var i = 0; i < maxStatesLength; i++) { | |
| sumStates <== sumStates + currStates[i] + nextStates[i] + captureGroup1Id[i] + captureGroup1Start[i]; | |
| } | |
| var sumStates = 0; | |
| for (var i = 0; i < maxStatesLength; i++) { | |
| sumStates += currStates[i] + nextStates[i] + captureGroup1Id[i] + captureGroup1Start[i]; | |
| } | |
| // If you need to constrain this, use a signal outside the loop | |
| signal sumStatesSignal <== sumStates; |
Description
zk regex version upgrade
Summary by CodeRabbit
Release Notes
New Features
ignoreBodyHashCheckflag.Chores
Tests