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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR extends ChangesReviver and Security Filtering
sequenceDiagram
participant Caller
participant destr
participant JSONparse
participant optionsReviver
Caller->>destr: destr(value, optionsOrReviver)
destr->>destr: normalize options / extract reviver
alt suspect JSON (proto-pollution)
destr->>JSONparse: JSON.parse(value, inlineReviver)
JSONparse->>destr: inlineReviver drops __proto__/constructor.prototype
destr->>optionsReviver: call options.reviver(this,key,value) when applicable
else non-suspect
destr->>JSONparse: JSON.parse(value, options.reviver)
end
JSONparse-->>Caller: parsed result
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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: 1
🧹 Nitpick comments (2)
test/index.test.ts (2)
201-207: ⚡ Quick winStrengthen huge-number test with value parity, not only type checks.
typeof === "number"can still pass on incorrect numeric results. Assert the parsed value explicitly to protect this regression path.Suggested test update
it("parses huge numbers without throwing in safeDestr", () => { const hugeNumber = "123456789012345678901234567890"; - // 默认模式下应该解析为数字类型（即便有精度损失，也应符合原生 JSON.parse 行为） - expect(typeof destr(hugeNumber)).toBe("number"); - // 严格模式下不应抛出 "Invalid JSON" 错误 - expect(typeof safeDestr(hugeNumber)).toBe("number"); + const expected = JSON.parse(hugeNumber); + expect(destr(hugeNumber)).toBe(expected); + expect(safeDestr(hugeNumber)).toBe(expected); });🤖 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 `@test/index.test.ts` around lines 201 - 207, The test currently only checks typeof for destr and safeDestr with a huge numeric string; update it to assert value parity as well (not just type) so the parsed result matches JavaScript's numeric conversion behavior. In the "parses huge numbers without throwing in safeDestr" test, after the typeof assertions for destr and safeDestr, add explicit assertions that destr(hugeNumber) and safeDestr(hugeNumber) equal Number(hugeNumber) (or the expected numeric value) to ensure both functions return the same numeric value as native conversion; reference the test name and the functions destr and safeDestr when locating where to change.
220-227: ⚡ Quick winAdd direct-argument reviver coverage for
safeDestrtoo.The overload changed for both parsers, but this test only validates direct reviver invocation on
destr.Suggested test addition
it("supports reviver as the second argument directly", () => { const input = '{"a": 1, "b": 2}'; const reviver = (key: string, value: any) => { if (key === "a") return value * 10; return value; }; expect(destr(input, reviver)).toStrictEqual({ a: 10, b: 2 }); + expect(safeDestr(input, reviver)).toStrictEqual({ a: 10, b: 2 }); });🤖 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 `@test/index.test.ts` around lines 220 - 227, Add a parallel test that verifies safeDestr accepts a reviver as the second argument just like destr: copy the existing "supports reviver as the second argument directly" test (which defines input '{"a": 1, "b": 2}' and reviver that multiplies "a" by 10) and create a new it block for safeDestr that calls safeDestr(input, reviver) and expects { a: 10, b: 2 }; ensure the new test name clearly references safeDestr so both overloads are covered.
🤖 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 `@src/index.ts`:
- Around line 80-95: The custom filtered reviver passed to JSON.parse currently
uses an arrow function and calls options.reviver(key, val), losing the correct
`this` binding; change the wrapper to a normal function (function(key, val) {
... }) and invoke the user reviver with the holder `this` (e.g.
options.reviver.call(this, key, val)) so behavior matches native JSON.parse;
keep the existing prototype/constructor filtering and warnKeyDropped calls
intact.
---
Nitpick comments:
In `@test/index.test.ts`:
- Around line 201-207: The test currently only checks typeof for destr and
safeDestr with a huge numeric string; update it to assert value parity as well
(not just type) so the parsed result matches JavaScript's numeric conversion
behavior. In the "parses huge numbers without throwing in safeDestr" test, after
the typeof assertions for destr and safeDestr, add explicit assertions that
destr(hugeNumber) and safeDestr(hugeNumber) equal Number(hugeNumber) (or the
expected numeric value) to ensure both functions return the same numeric value
as native conversion; reference the test name and the functions destr and
safeDestr when locating where to change.
- Around line 220-227: Add a parallel test that verifies safeDestr accepts a
reviver as the second argument just like destr: copy the existing "supports
reviver as the second argument directly" test (which defines input '{"a": 1,
"b": 2}' and reviver that multiplies "a" by 10) and create a new it block for
safeDestr that calls safeDestr(input, reviver) and expects { a: 10, b: 2 };
ensure the new test name clearly references safeDestr so both overloads are
covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bae2c857-fa0c-42d4-bff5-044b3aceb40b
📒 Files selected for processing (2)
src/index.tstest/index.test.ts
… pollution filter
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Description
This PR brings two significant enhancements and bug fixes to
destr, ensuring better parity with nativeJSON.parseand improving robustness when parsing huge numbers.1. Support for
reviverParameter (JSON.parseparity)destrandsafeDestrto accept a customreviverfunction, either nested in theoptionsobject or passed directly as the second argument (seamless drop-in replacement for nativeJSON.parse(text, reviver)).__proto__,constructor) first, and safely chains the remaining sanitized keys into the user's customreviver. On normal paths, it directly utilizes nativeJSON.parse(value, reviver)for optimal performance.2. Fix for Huge Numbers Being Treated as Invalid JSON
JsonSigRxregular expression was hardcoded to expect digits with max length 16 (\d{1,16}). This caused long numeric strings (e.g.,"123456789012345678901234567890") to fail validation. It fell back to returning raw strings in default mode, and incorrectly threwSyntaxError: [destr] Invalid JSONin strict (safeDestr) mode.JsonSigRxsince nativeJSON.parsehandles numeric parsing natively (even with IEEE 754 precision rounding), bringing full compliance to specification behaviors without losing error safety.🧪 Tests & Verification
test/index.test.tscovering:destrandsafeDestr.reviveroptions payload parsing.reviverargument parsing.revivercallbacks.npm run lint.Summary by CodeRabbit
New Features
Bug Fixes
Tests