Add default values to parameters when computing hash with hashProvide…#73
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Promptless prepared a documentation update related to this change. Triggered by reclaimprotocol/attestor-core#73 Added a changelog entry documenting the breaking change. The Review at https://app.gopromptless.ai/suggestions/be946152-3666-49e8-a18b-2b24388b4b2d |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/claims.ts`:
- Around line 119-123: In the responseMatches mapping (inside
params?.responseMatches?.map in src/utils/claims.ts) replace uses of the logical
OR operator for optional fields with the nullish coalescing operator so explicit
falsy values are preserved; specifically change occurrences that set fields from
it.invert || undefined and it.hash || undefined to use it.invert ?? undefined
and it.hash ?? undefined (so that explicit false or empty-string values are not
collapsed into undefined when hashing).
- Around line 127-133: The object mapping for responseRedactions and the earlier
invert field use the logical OR operator which converts explicit falsy values
(false, 0, '') to undefined; update the mappings so `invert: it.invert ||
undefined` and `hash: it.hash || undefined` are changed to use nullish
coalescing (`invert: it.invert ?? undefined` and `hash: it.hash ?? undefined`)
so explicit falsy values are preserved during canonicalization and keccak256
hashing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| url: params?.url ?? '', | ||
| // METHOD needs to be explicitly specified and absence or unknown method should cause error, but we're choosing to ignore it in this case | ||
| method: params?.method ?? 'GET', | ||
| body: params?.body ?? '', | ||
| responseMatches: params?.responseMatches?.map(it => ({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify all call sites and inspect whether required field validation exists upstream.
rg -nP --type=ts -C3 '\bhashProviderParams\s*\('
rg -nP --type=ts -C3 '\b(assert|validate|schema|zod|joi|throw)\b.*\b(url|method|responseMatches|responseRedactions)\b|\b(url|method|responseMatches|responseRedactions)\b.*\b(assert|validate|schema|zod|joi|throw)\b'Repository: reclaimprotocol/attestor-core
Length of output: 16963
🏁 Script executed:
# Check the call site of hashProviderParams in assert-valid-claim-request.ts
cat -n src/server/utils/assert-valid-claim-request.ts | head -180 | tail -50Repository: reclaimprotocol/attestor-core
Length of output: 1741
🏁 Script executed:
# Find the type definition for ProviderParams
rg -nP --type=ts 'type\s+ProviderParams|interface\s+ProviderParams' -A 10Repository: reclaimprotocol/attestor-core
Length of output: 761
🏁 Script executed:
# Check for zod or schema validation around params
rg -nP --type=ts 'ProviderParams.*schema|schema.*ProviderParams|zod.*ProviderParams|ProviderParams.*zod' -B 3 -A 3Repository: reclaimprotocol/attestor-core
Length of output: 55
🏁 Script executed:
# Look at the broader context of the hashProviderParams call
rg -nP --type=ts -B 20 'ctx.providerHash = hashProviderParams' src/server/utils/assert-valid-claim-request.ts | head -40Repository: reclaimprotocol/attestor-core
Length of output: 614
🏁 Script executed:
# Check what niceParseJsonObject returns and whether it can be undefined
rg -nP --type=ts 'function\s+niceParseJsonObject|const\s+niceParseJsonObject' -A 15Repository: reclaimprotocol/attestor-core
Length of output: 875
🏁 Script executed:
# Check the assertValidateProviderParams function
rg -nP --type=ts 'function\s+assertValidateProviderParams|const\s+assertValidateProviderParams' -A 20Repository: reclaimprotocol/attestor-core
Length of output: 1309
🏁 Script executed:
# Look for ProvidersConfig to understand what the http provider parameters look like
rg -nP --type=ts 'ProvidersConfig' -B 3 -A 30 | head -100Repository: reclaimprotocol/attestor-core
Length of output: 6591
🏁 Script executed:
# Look for HttpProviderParameters definition
rg -nP --type=ts 'export interface HttpProviderParameters' -A 50Repository: reclaimprotocol/attestor-core
Length of output: 3346
🏁 Script executed:
# Check the HttpProviderParametersJson schema to see which fields are required
rg -nP --type=ts 'HttpProviderParametersJson' -B 5 -A 30 | head -80Repository: reclaimprotocol/attestor-core
Length of output: 7493
Remove use of || operator for optional fields to prevent hash collisions on falsy values.
At lines 127 and 133, it.invert || undefined and it.hash || undefined collapse distinct falsy values (false, empty string, etc.) into undefined. For example, an explicit invert: false becomes indistinguishable from an omitted invert field during hashing, creating unintended collisions. Use nullish coalescing (??) instead:
Proposed fix
responseMatches: params.responseMatches?.map(it => ({
value: it.value ?? '',
type: it.type ?? 'contains',
- invert: it.invert || undefined,
+ invert: it.invert ?? undefined,
})) ?? [],
responseRedactions: params.responseRedactions?.map(it => ({
xPath: it.xPath ?? '',
jsonPath: it.jsonPath ?? '',
regex: it.regex ?? '',
- hash: it.hash || undefined,
+ hash: it.hash ?? undefined,
})) ?? [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/claims.ts` around lines 119 - 123, In the responseMatches mapping
(inside params?.responseMatches?.map in src/utils/claims.ts) replace uses of the
logical OR operator for optional fields with the nullish coalescing operator so
explicit falsy values are preserved; specifically change occurrences that set
fields from it.invert || undefined and it.hash || undefined to use it.invert ??
undefined and it.hash ?? undefined (so that explicit false or empty-string
values are not collapsed into undefined when hashing).
| invert: it.invert || undefined, | ||
| })) ?? [], | ||
| responseRedactions: params?.responseRedactions?.map(it => ({ | ||
| xPath: it.xPath ?? '', | ||
| jsonPath: it.jsonPath ?? '', | ||
| regex: it.regex ?? '', | ||
| hash: it.hash || undefined, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether explicit falsy values are used in payload construction today.
rg -nP --type=ts -C2 'invert\s*:\s*false|hash\s*:\s*([\"\'])\s*\1'Repository: reclaimprotocol/attestor-core
Length of output: 134
🏁 Script executed:
# First, let's view the actual code at the lines mentioned
cat -n src/utils/claims.ts | sed -n '120,140p'Repository: reclaimprotocol/attestor-core
Length of output: 1034
🏁 Script executed:
# Now let's search for places where invert or hash fields are set with explicit falsy values
# This will help us understand if this is a theoretical or practical issue
rg -n --type=ts "(invert\s*:\s*false|hash\s*:\s*['\"])" src/utils/claims.tsRepository: reclaimprotocol/attestor-core
Length of output: 55
🏁 Script executed:
# Let's also check the broader context of how the map function is used
rg -n --type=ts "responseRedactions.*map|claimsRedaction.*map" src/utils/claims.tsRepository: reclaimprotocol/attestor-core
Length of output: 139
🏁 Script executed:
# Find the function name and signature
rg -n --type=ts -B5 "responseRedactions.*map.*hash" src/utils/claims.ts | head -20Repository: reclaimprotocol/attestor-core
Length of output: 55
🏁 Script executed:
# Search for where this function is called and what values are passed
rg -n --type=ts "invert\s*:\s*false|hash\s*:\s*['\"]" .Repository: reclaimprotocol/attestor-core
Length of output: 1234
🏁 Script executed:
# Let's look for tests that might reveal the issue
fd --type=f -e test.ts -e spec.ts | xargs rg -l "claims" 2>/dev/null | head -5Repository: reclaimprotocol/attestor-core
Length of output: 110
🏁 Script executed:
# Find the function definition that contains these lines
rg -n --type=ts -B20 "invert: it.invert || undefined" src/utils/claims.ts | head -30Repository: reclaimprotocol/attestor-core
Length of output: 1092
🏁 Script executed:
# Check if there are any cases where invert is explicitly set to false
rg -n --type=ts "invert.*false" .Repository: reclaimprotocol/attestor-core
Length of output: 4432
🏁 Script executed:
# Look at the http-provider test to see how these fields are used
cat -n src/tests/http-provider-utils.test.ts | sed -n '1210,1280p'Repository: reclaimprotocol/attestor-core
Length of output: 4465
🏁 Script executed:
# Check the function signature to understand what it does
rg -n --type=ts -A3 "export function.*createClaimID|function createClaimID" src/utils/claims.tsRepository: reclaimprotocol/attestor-core
Length of output: 55
🏁 Script executed:
# Search for any callers that might pass explicit false for invert
rg -n --type=ts "invert\s*:\s*false" . 2>/dev/null || echo "No explicit false values found for invert"Repository: reclaimprotocol/attestor-core
Length of output: 114
🏁 Script executed:
# Check the type definition for ProviderParams to understand the optional nature of these fields
rg -n --type=ts "invert\s*\?" . | head -10Repository: reclaimprotocol/attestor-core
Length of output: 351
🏁 Script executed:
# Look at the canonicalStringify function to understand how it serializes
rg -n --type=ts "canonicalStringify|canonicalize" src/utils/claims.tsRepository: reclaimprotocol/attestor-core
Length of output: 438
🏁 Script executed:
# Let's check what canonicalize does with undefined vs other values
# Look for test cases or documentation about canonicalize behavior
rg -n --type=ts "canonicalize|canonicalStringify" src/tests/ | head -20Repository: reclaimprotocol/attestor-core
Length of output: 55
🏁 Script executed:
# Let's verify how the filtered params object is used - check if undefined properties are serialized
cat -n src/utils/claims.ts | sed -n '100,145p'Repository: reclaimprotocol/attestor-core
Length of output: 1790
🏁 Script executed:
# Search for any actual usage where hash could be an empty string or false
rg -n --type=ts "hash\s*:\s*['\"]" src/tests/ | head -5Repository: reclaimprotocol/attestor-core
Length of output: 485
🏁 Script executed:
# Let's verify what canonicalize does with undefined vs falsy values
# Check if there are any docs or tests showing canonicalize behavior
npm list canonicalize 2>/dev/null || cat package.json | grep -A2 -B2 canonicalizeRepository: reclaimprotocol/attestor-core
Length of output: 264
🏁 Script executed:
# Let's create a test to see what canonicalize does with undefined vs false
cat > /tmp/test_canonicalize.js << 'EOF'
const canonicalize = require('canonicalize');
const obj1 = { invert: false };
const obj2 = { invert: undefined };
const obj3 = {};
const obj4 = { invert: false, other: 'value' };
const obj5 = { invert: undefined, other: 'value' };
console.log("invert: false =>", canonicalize(obj1));
console.log("invert: undefined =>", canonicalize(obj2));
console.log("empty =>", canonicalize(obj3));
console.log("invert: false, other =>", canonicalize(obj4));
console.log("invert: undefined, other =>", canonicalize(obj5));
EOF
cd /tmp && npm install canonicalize > /dev/null 2>&1 && node test_canonicalize.js 2>&1 || echo "Could not test canonicalize"Repository: reclaimprotocol/attestor-core
Length of output: 1011
🏁 Script executed:
# Check if there's any test or documentation about the hash collision concern
rg -n --type=ts "hash.*collision|collision.*hash|same.*hash|duplicate.*hash" src/Repository: reclaimprotocol/attestor-core
Length of output: 4635
🏁 Script executed:
# Check the type definition to see if invert is truly optional and can be false
cat -n src/types/providers.gen.ts | sed -n '55,70p'Repository: reclaimprotocol/attestor-core
Length of output: 952
🏁 Script executed:
# Let's search for actual usage patterns of responseMatches to see if invert is ever set
rg -n --type=ts "responseMatches.*invert|invert\s*:\s*" src/ | grep -v "\.gen\.ts"Repository: reclaimprotocol/attestor-core
Length of output: 131
🏁 Script executed:
# Check if there are any tests that verify hash uniqueness or consistency
rg -n --type=ts "should produce the same hash" src/tests/claim-creation.test.ts -A 30Repository: reclaimprotocol/attestor-core
Length of output: 844
Replace || undefined with ?? undefined to preserve explicit falsy values in hash computation.
Lines 127 and 133 incorrectly use the logical OR operator, which converts explicit false, 0, or '' to undefined. When these objects are canonicalized for hashing via keccak256, distinct payloads serialize identically, breaking hash determinism. The nullish coalescing operator (??) correctly preserves explicit falsy values while only replacing null/undefined.
Proposed fix
- invert: it.invert || undefined,
+ invert: it.invert ?? undefined,
@@
- hash: it.hash || undefined,
+ hash: it.hash ?? undefined,📝 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.
| invert: it.invert || undefined, | |
| })) ?? [], | |
| responseRedactions: params?.responseRedactions?.map(it => ({ | |
| xPath: it.xPath ?? '', | |
| jsonPath: it.jsonPath ?? '', | |
| regex: it.regex ?? '', | |
| hash: it.hash || undefined, | |
| invert: it.invert ?? undefined, | |
| })) ?? [], | |
| responseRedactions: params?.responseRedactions?.map(it => ({ | |
| xPath: it.xPath ?? '', | |
| jsonPath: it.jsonPath ?? '', | |
| regex: it.regex ?? '', | |
| hash: it.hash ?? undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/claims.ts` around lines 127 - 133, The object mapping for
responseRedactions and the earlier invert field use the logical OR operator
which converts explicit falsy values (false, 0, '') to undefined; update the
mappings so `invert: it.invert || undefined` and `hash: it.hash || undefined`
are changed to use nullish coalescing (`invert: it.invert ?? undefined` and
`hash: it.hash ?? undefined`) so explicit falsy values are preserved during
canonicalization and keccak256 hashing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/http-provider-utils.test.ts (1)
496-530: Assert canonicalization, not just the new golden hash.This updates the snapshot, but it still doesn't verify the behavior introduced by this PR: params that differ only by omitted/default fields should hash identically. Without that equivalence check, the test can still pass while the normalization contract regresses.
🧪 Suggested addition
const hash = hashProviderParams(params) assert.equal(hash, '0x58e516c01cf2bb54fd567a4f31b64d0a8e99ee2569c814d31030bf2febadf270') + assert.equal( + hash, + hashProviderParams({ + ...params, + body: '', + responseRedactions: [{ xPath: './html/head/title', jsonPath: '', regex: '' }], + }) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/http-provider-utils.test.ts` around lines 496 - 530, The test currently only asserts golden hashes but doesn't verify canonicalization across equivalent inputs; add an assertion that hashProviderParams(params) equals hashProviderParams(paramsEx) (or equivalently that the computed hash for the normalized params matches the golden) so the test validates that omitted/default fields are normalized — update the test around the existing hashProviderParams calls (referencing hashProviderParams, params and paramsEx) to include a direct equality check between the two hashes in addition to any current golden assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tests/http-provider-utils.test.ts`:
- Around line 496-530: The test currently only asserts golden hashes but doesn't
verify canonicalization across equivalent inputs; add an assertion that
hashProviderParams(params) equals hashProviderParams(paramsEx) (or equivalently
that the computed hash for the normalized params matches the golden) so the test
validates that omitted/default fields are normalized — update the test around
the existing hashProviderParams calls (referencing hashProviderParams, params
and paramsEx) to include a direct equality check between the two hashes in
addition to any current golden assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0470eaec-05ff-4586-8ef2-2cc22687cd95
📒 Files selected for processing (2)
src/tests/http-provider-utils.test.tssrc/tests/http-provider.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/http-provider.test.ts
2 participants
…rParams
Description
Testing (ignore for documentation update)
Type of change
Checklist:
Additional Notes:
Summary by CodeRabbit