Repository navigation
fix(url): close the pathname and url-engine gaps found in #12864 - #13196
Merged
Merged
Conversation
Three separate defects surfaced while normalizing URL paths. Each is small and
independent; they share this PR because they are the same review surface.
1. The constructor only encoded non-ASCII in `pathname`, so `new URL()` kept
raw spaces and delimiters that the WHATWG path percent-encode set escapes:
`new URL("https://x/a b<c").pathname` was `/a b<c` where Node reports
`/a%20b%3Cc`. `create_url_object` now uses the same path encoder as the
`pathname` setter, and that encoder gains `^`, which Node escapes in paths
(`/a^b` → `/a%5Eb`) although the standard's set omits it. `%` stays outside
the set, so existing escapes and stray `%` pass through unchanged, and
tab/LF/CR are stripped from the input instead of encoded.
2. Assigning `pathname` on an opaque URL wrote to it: `new URL("mailto:a/../b")`
then `url.pathname = "q/../r"` rewrote the opaque path to `mailto:q/../r`.
Node ignores the assignment, because a URL whose path is not a segment list
has nothing to set.
3. A program importing `node:url` as a default or namespace object
(`import url from "node:url"`) and calling only its helpers linked no
`url-engine`: the HIR carries `NativeModuleRef("url")` rather than the
`module: "url"` marker the feature scan looked for, so `domainToASCII` /
`domainToUnicode` silently fell back to plain IDNA (`"a/b"` instead of
Node's `"a"`). Detecting that token fixes it, and with it the last two
failing lines of `test_gap_6375_legacy_url_parse_format.ts`, which uses
exactly that import shape.
New node-parity cases: `path-percent-encoding.ts` (constructor escapes, `%`
passthrough, whitespace stripping, opaque-vs-list `pathname` writes) and
`default-import-helpers.ts` (deliberately constructs no URL, which is the
trigger for defect 3). `--suite node-suite --module url` is 71/71, and the
URL `test-files` corpus shows no new failure: `test_gap_6375` now passes.
|
Caution Review failedThe pull request is closed. Finishing Touches
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 |
`url.resolve("/a/b", "../c")` returned `///c` where Node returns `/c`: the
helper delegated to the WHATWG `resolve_url`, which models absolute URLs, so a
base without a scheme was parsed as host + path and then re-emitted with a
`//host` prefix it never had.
The resolver is now a port of Node's `Url.prototype.resolveObject` segment walk.
The base keeps its scheme and authority, the relative path replaces the base's
last segment, and dot segments are resolved with the `up` counter that lets a
*relative* base climb above its own root (`("a/b", "../../c")` is `../c`) while
an absolute one cannot. `?query` keeps the base path verbatim, a `#hash` keeps
the base query, and an absolute or protocol-relative target still replaces
everything. `legacy_url_join` and its `url`-crate dependency are gone.
New case `test-parity/node-suite/url/legacy/resolve-relative-base.ts` pins 39
base/reference pairs; `--suite node-suite --module url` is 72/72 and the URL
`test-files` corpus shows no new failure.
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
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.
Summary
Four URL defects found while normalizing paths in #13195, each independent and each confirmed against the pinned Node oracle. They are pathname/engine/resolver behavior, not parser shape, so they land separately from #13195.
#13195 has merged, so this is based on
mainand its diff contains only the follow-up work.Changes
crates/perry-runtime/src/url/parse.rs—create_url_objectencodespathnamewith the shared path encoder instead of the non-ASCII-only one.crates/perry-runtime/src/url/url_class.rs— the path encode set gains^;percent_encode_pathbecomes the single encoder both paths use;js_url_set_pathnamereturns early for an opaque URL.crates/perry/src/commands/compile/collect_modules/feature_detect.rs— theurl-enginedetector recognizesNativeModuleRef("url"), the marker a default/namespacenode:urlimport leaves in the HIR.crates/perry-runtime/src/url/node_compat.rs—url.resolveis a port of Node'sresolveObject: it no longer delegates to the WHATWG resolver, andlegacy_url_joinplus itsurl-crate dependency are gone.test-parity/node-suite/url/url/path-percent-encoding.ts,.../default-import-helpers.ts,.../legacy/resolve-relative-base.ts— new node-parity cases.changelog.d/…— fragment.The three defects
Constructor path encoding.
new URL("https://x/a b<c").pathnamewas/a b<c; Node reports/a%20b%3Cc. The setter had the right encoder, the constructor did not.^is now escaped too (/a^b→/a%5Eb), matching Node even though the standard's path set omits it.%is outside the set, so%20,%zzand%2Gpass through untouched, and tab/LF/CR are stripped rather than encoded.Opaque
pathnamewrites.new URL("mailto:a/../b")+url.pathname = "q/../r"rewrote the path tomailto:q/../r. Node ignores the assignment: a URL without a segment list has nothing to set. List paths (https:,file:,foo:/a) still take the write.url-enginenot linked for default imports.import url from "node:url"+ only helper calls producedNativeModuleRef("url")in the HIR, which the detector did not match, so the engine was never installed anddomainToASCII/domainToUnicodefell back to plain IDNA:domainToASCII("a/b")returned"a/b"where Node returns"a". Adding that token fixes it — and the two remaining failing lines oftest_gap_6375_legacy_url_parse_format.ts, which uses exactly that import shape.Legacy
url.resolvewith a non-absolute base.url.resolve("/a/b", "../c")returned///c; Node returns/c. The helper delegated to the WHATWGresolve_url, which models absolute URLs, so a scheme-less base was parsed as host + path and re-emitted with a//hostprefix it never had. The resolver now walks segments like Node'sresolveObject: the base keeps its scheme and authority, the relative path replaces the base's last segment, and theupcounter lets a relative base climb above its own root (("a/b", "../../c")is../c) while an absolute one cannot.?querykeeps the base path verbatim and#hashkeeps the base query.Related issue
Refs #12864 (follow-up to #13195)
Test plan
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static— clean, plusscripts/gc_call_effects/sidecars.sh target/releasePERRY_SKIP_BUILD=1 ./run_parity_tests.sh --suite node-suite --module url— 72/72 PASS (69 from fix(url): remove dot segments and fold special-scheme separators #13195 + the 3 new cases)node --experimental-strip-types(diffempty; 39 base/reference pairs in the resolver case)/a%20b%3Cc, ignored opaque write,domainToASCII("a/b") === "a"without aURLconstructor,url.resolve("/a/b", "../c") === "/c")test-filescorpus (29 cases): no new failure, andtest_gap_6375_legacy_url_parse_format.ts— failing before this change — now passescargo test --workspace …— not run locally; CI'scargo-testjob is the backstopcargo fmt --check -p perry-runtimecleanChecklist
feat:/fix:/docs:/chore:prefix convention used in the log