Validate malformed allowedProtocols entries in url()#653
Conversation
Close #344 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
- Coverage 94.96% 94.95% -0.01%
==========================================
Files 38 38
Lines 17764 17798 +34
Branches 4727 4743 +16
==========================================
+ Hits 16869 16900 +31
- Misses 883 886 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe PR moves validation of Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 3033-3070: Update the tests that call url({ allowedProtocols:
[...] }) to assert the exact diagnostic string in addition to the TypeError:
replace the current assert.throws(TypeError) checks with assertions that capture
the thrown error and assert both err.name === "TypeError" and err.message ===
"<expected diagnostic including the offending value>" (or use assert.throws with
an object matcher for name and message); apply this to each case referencing url
and allowedProtocols (numeric 123, null, undefined, missing trailing colon like
"https", suffix "https://", and empty string) so the tests verify the precise
error text for each offending value.
- Around line 3083-3087: The test currently only ensures url({ allowedProtocols:
["HTTP:", "http:"] }) doesn't throw; update the "should accept case-only
duplicates" spec to also call suggest() on the created parser and assert that
the suggestions are deduplicated (e.g., only one protocol entry returned,
normalized as expected). Locate the url constructor call in the test and add an
assertion against parser.suggest() (or the suggest() helper used in this file)
to verify duplicate protocol suggestions are not present.
In `@packages/core/src/valueparser.ts`:
- Around line 1279-1291: Normalize and deduplicate options.allowedProtocols
during construction: map each protocol to a canonical form (e.g.,
protocol.toLowerCase()) before validating/using it, remove case-only duplicates
while preserving iteration order (use a Set or track seen values) and then run
the existing protocol format check (/^[a-z][a-z0-9+\-.]*:$/i) against the
normalized entries; update any places that iterate options.allowedProtocols to
use the normalized/deduped array so duplicate suggestions/outputs (e.g., from
"HTTP:" vs "http:") no longer occur.
- Around line 1285-1288: The error message construction in the validation for
allowedProtocols uses JSON.stringify(protocol), which will itself throw for
certain types like bigint (e.g., 1n); change the message to safely convert the
protocol value (e.g., use String(protocol) or a safe inspect fallback) so the
validation always throws the intended TypeError; update the throw in the
allowedProtocols validation that references the protocol variable so it
interpolates a safe string representation instead of JSON.stringify(protocol).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38942b1c-1336-4f76-9b5b-9e3a360651a7
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
assert exact error messages in tests Use String() instead of JSON.stringify() for non-string protocol values to avoid crashing on BigInt inputs. Also assert the exact diagnostic string (including the offending value) in all validation tests so message regressions are caught. #653 (comment) #653 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
normalization Case-only duplicates like ["HTTP:", "http:"] previously passed validation but produced duplicate suggestions from suggest(). Protocols are now deduplicated during construction by tracking seen normalized values. #653 (comment) #653 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/core/src/valueparser.test.ts (2)
3033-3070:⚠️ Potential issue | 🟠 MajorAssert exact constructor diagnostics for invalid
allowedProtocolsentries.These checks currently assert only
TypeError, so diagnostic-message regressions won’t be caught. Please assert bothnameand the exactmessagefor each invalid input case.Based on learnings: In constructor-time validation tests in
packages/core/src/valueparser.test.ts, assert both the error type and the exact diagnostic/error message string using strict string equality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/valueparser.test.ts` around lines 3033 - 3070, Update the tests that call url({ allowedProtocols: ... }) so they assert both the error constructor name and the exact diagnostic message string instead of only TypeError; replace the current assert.throws(..., TypeError) checks with assert.throws(callable, { name: "TypeError", message: "exact expected message" }) (or equivalent strict message equality) for each invalid allowedProtocols case (numeric 123, null, undefined, missing trailing colon like "https"/"http", "://"-suffix like "https://", and empty string) so the test verifies the precise constructor-time diagnostics emitted by url and the allowedProtocols validation logic.
3083-3087:⚠️ Potential issue | 🟡 MinorStrengthen the case-only duplicate test to verify deduplicated suggestions.
This only checks construction success. It should also assert that suggestions do not duplicate for
["HTTP:", "http:"].Proposed test refinement
it("should accept case-only duplicates", () => { - assert.doesNotThrow( - () => url({ allowedProtocols: ["HTTP:", "http:"] }), - ); + const parser = url({ allowedProtocols: ["HTTP:", "http:"] }); + const suggestions = Array.from(parser.suggest!("ht")) + .map((s) => (s.kind === "literal" ? s.text : s.pattern || "")) + .sort(); + assert.deepEqual(suggestions, ["http://"]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/valueparser.test.ts` around lines 3083 - 3087, The test currently only ensures construction succeeds for url({ allowedProtocols: ["HTTP:", "http:"] })—extend it to also obtain the parser's suggestions (call suggestions() on the returned parser) and assert those suggestions are deduplicated case-insensitively (e.g., map to lower-case and assert length/contents contain only one "http:" entry); reference the url factory and its suggestions() output and verify uniqueness by lowercasing entries to catch case-only duplicates.packages/core/src/valueparser.ts (1)
1285-1288:⚠️ Potential issue | 🟡 MinorAvoid
JSON.stringify()for invalid non-string protocol rendering.At line 1287,
JSON.stringify(protocol)can throw a TypeError (e.g.,JSON.stringify(1n)for BigInt values) according to MDN, which bypasses your intended validation message. While TypeScript prevents non-strings via thereadonly string[]type, the constructor validation is a runtime safety net for cases where type assertions (as never) bypass compile-time checks, as shown in existing tests.Suggested fix
if ( typeof protocol !== "string" || !/^[a-z][a-z0-9+\-.]*:$/i.test(protocol) ) { + const rendered = typeof protocol === "string" + ? JSON.stringify(protocol) + : String(protocol); throw new TypeError( `Each allowed protocol must be a valid protocol ending with a colon` + - ` (e.g., "https:"), got: ${JSON.stringify(protocol)}.`, + ` (e.g., "https:"), got: ${rendered}.`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/valueparser.ts` around lines 1285 - 1288, The error message in valueparser.ts uses JSON.stringify(protocol) which can itself throw for values like BigInt; update the throw in the validation branch that constructs the TypeError (the code that currently calls JSON.stringify(protocol)) to use a safe string conversion (e.g., String(protocol) or typeof protocol === "string" ? protocol : String(protocol)) so the error message never throws; keep the descriptive text but replace JSON.stringify(protocol) with the safe conversion referenced above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 3033-3070: Update the tests that call url({ allowedProtocols: ...
}) so they assert both the error constructor name and the exact diagnostic
message string instead of only TypeError; replace the current assert.throws(...,
TypeError) checks with assert.throws(callable, { name: "TypeError", message:
"exact expected message" }) (or equivalent strict message equality) for each
invalid allowedProtocols case (numeric 123, null, undefined, missing trailing
colon like "https"/"http", "://"-suffix like "https://", and empty string) so
the test verifies the precise constructor-time diagnostics emitted by url and
the allowedProtocols validation logic.
- Around line 3083-3087: The test currently only ensures construction succeeds
for url({ allowedProtocols: ["HTTP:", "http:"] })—extend it to also obtain the
parser's suggestions (call suggestions() on the returned parser) and assert
those suggestions are deduplicated case-insensitively (e.g., map to lower-case
and assert length/contents contain only one "http:" entry); reference the url
factory and its suggestions() output and verify uniqueness by lowercasing
entries to catch case-only duplicates.
In `@packages/core/src/valueparser.ts`:
- Around line 1285-1288: The error message in valueparser.ts uses
JSON.stringify(protocol) which can itself throw for values like BigInt; update
the throw in the validation branch that constructs the TypeError (the code that
currently calls JSON.stringify(protocol)) to use a safe string conversion (e.g.,
String(protocol) or typeof protocol === "string" ? protocol : String(protocol))
so the error message never throws; keep the descriptive text but replace
JSON.stringify(protocol) with the safe conversion referenced above.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9246ef22-899e-483c-830e-da6a48adc432
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/valueparser.test.ts (1)
3033-3090:⚠️ Potential issue | 🟡 MinorAssert exact constructor diagnostics for invalid
allowedProtocolsentries.These checks currently match only message fragments via RegExp, which can miss regressions in the full diagnostic text. Please switch these to exact string equality for
message(while keepingname: "TypeError").Based on learnings: In constructor-time validation tests in
packages/core/src/valueparser.test.ts, assert both error type and the exact diagnostic/error message string using strict string equality rather than RegExp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/valueparser.test.ts` around lines 3033 - 3090, The tests calling url({ allowedProtocols: ... }) currently use assert.throws with a RegExp for the message; change these to assert exact string equality for the message (keep name: "TypeError") so the constructor diagnostic is matched exactly; update each assert.throws invocation that references allowedProtocols entries (e.g., numeric 123, null, undefined, "https", "http", "https://", and empty string) to provide the full expected error message string instead of a RegExp, leaving the use of the url() call and assert.throws unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 3033-3090: The tests calling url({ allowedProtocols: ... })
currently use assert.throws with a RegExp for the message; change these to
assert exact string equality for the message (keep name: "TypeError") so the
constructor diagnostic is matched exactly; update each assert.throws invocation
that references allowedProtocols entries (e.g., numeric 123, null, undefined,
"https", "http", "https://", and empty string) to provide the full expected
error message string instead of a RegExp, leaving the use of the url() call and
assert.throws unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c63735d4-7b41-4a35-903a-1eb90f4ef5b4
📒 Files selected for processing (2)
packages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/valueparser.test.ts (1)
3033-3090:⚠️ Potential issue | 🟠 MajorAssert exact constructor diagnostics instead of regex fragments.
These checks only match partial message suffixes, so wording/index regressions can slip through. For constructor-time option validation in this file, please assert the exact
TypeError.messagestring per case.Based on learnings: In constructor-time validation tests in
packages/core/src/valueparser.test.ts, assert both error type and the exact diagnostic string using strict string equality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/valueparser.test.ts` around lines 3033 - 3090, Tests around the url constructor option validation currently assert the error message with regex fragments; change each assert.throws for url({ allowedProtocols: ... }) so it asserts the exact TypeError.message string (use the exact full message text rather than a RegExp) for every case (e.g., numeric 123, null, undefined, "https", "http", "https://", and empty string). Update the assertions in packages/core/src/valueparser.test.ts referencing the url(...) constructor so the thrown error object includes name: "TypeError" and message: "the exact diagnostic text" (replace each /regex/ with the corresponding exact message literal) to ensure strict equality on constructor-time diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 3033-3090: Tests around the url constructor option validation
currently assert the error message with regex fragments; change each
assert.throws for url({ allowedProtocols: ... }) so it asserts the exact
TypeError.message string (use the exact full message text rather than a RegExp)
for every case (e.g., numeric 123, null, undefined, "https", "http", "https://",
and empty string). Update the assertions in
packages/core/src/valueparser.test.ts referencing the url(...) constructor so
the thrown error object includes name: "TypeError" and message: "the exact
diagnostic text" (replace each /regex/ with the corresponding exact message
literal) to ensure strict equality on constructor-time diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3abd13d3-4a57-4247-b9c4-fe8de7b572b5
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/valueparser.test.tspackages/core/src/valueparser.ts
1 participant
url()trustedallowedProtocolsentries at runtime without any validation. This caused several problems: non-string entries (e.g.,123) crashed withTypeError: p.toLowerCase is not a function, entries missing the trailing colon (e.g.,"https"instead of"https:") silently broke protocol matching, and entries like"https://"generated broken suggestions such ashttps://://.This change adds construction-time validation in packages/core/src/valueparser.ts that checks each entry is a string matching valid URL scheme syntax per RFC 3986, ending with a colon. Invalid entries now throw a
TypeErrorwith a clear message:This follows the same pattern established by
path()extension validation (70e2401) anddomain()/email()option validation.Closes #344