Skip to content

fix: decode %3F back to "?" in toFileSystemPath (#427) - #428

Open
patchwright wants to merge 2 commits into
APIDevTools:mainfrom
patchwright:fix/url-decode-question-mark
Open

fix: decode %3F back to "?" in toFileSystemPath (#427)#428
patchwright wants to merge 2 commits into
APIDevTools:mainfrom
patchwright:fix/url-decode-question-mark

Conversation

@patchwright

Copy link
Copy Markdown

Problem

Fixes #427. toFileSystemPath never decodes %3F back to ?, so any $RefParser call on a local path containing a literal ? (a legal POSIX filename character) fails with ResolverError: ENOENT. Confirmed on main HEAD e8190c1.

Root cause

lib/util/url.ts has a matched pair of manual encode/decode tables for characters encodeURI/decodeURI leave alone. urlEncodePatterns (line 13-16) encodes both ? and #. urlDecodePatterns (line 19) only reverses #, $, &, ,, @? is missing, even though the comment directly above the decode loop names it explicitly ("This includes characters such as # and ?...").

Fix

Adds /%3F/g, "?" to urlDecodePatterns, in the same hex-ordered position the other pairs follow. 1 line changed in lib/util/url.ts.

How to test

$ pnpm exec vitest run test/specs/util/url.spec.ts
# before this fix: 1 failing (the new "?" round-trip test)
# after this fix:  39 passed

Reproduced end-to-end too: $RefParser.parse() on a local file named e.g. defs?1.json throws ENOENT on unpatched code and resolves correctly with the fix.

Backward compatibility

No breaking changes. Strictly adds a decode case that was previously a no-op (falling through unchanged); no existing behavior for any other character changes.


Assisted-by: Claude (code generation, reviewed and tested locally)

urlEncodePatterns encodes both # and ? when converting a filesystem
path to a URL, but urlDecodePatterns only reversed #, $, &, ,, and @.
A local path containing a literal ? (legal on POSIX filesystems) was
encoded to %3F on the way in and never decoded back on the way out,
so resolution of any such path failed with ENOENT.

Adds the missing /%3F/g, "?" pair, in the same hex-ordered position
the other pairs already follow. Adds a symmetric test for # alongside
the new ? test.
@jonluca

jonluca commented Aug 7, 2026

Copy link
Copy Markdown
Member

Thanks for the fix — the intended literal-? case works, but I don't think this is safe to merge as-is yet.

There is a double-decoding/path-aliasing regression:

original filename: defs%3F1.json
fromFileSystemPath: defs%253F1.json
decodeURI:          defs%3F1.json
new replacement:   defs?1.json

Because the new %3F → ? replacement runs after decodeURI(), a valid filename containing the literal text %3F is decoded twice. I reproduced this end-to-end: when both defs%3F1.json and defs?1.json exist, parse("defs%3F1.json") on this PR silently reads the ? sibling instead. On Windows, % is valid in filenames while ? is not, so the valid path becomes invalid. This also contradicts RFC 3986 §2.4's requirement not to decode the same string more than once.

The new tests also fail in the live Windows Node and browser jobs. They use POSIX /a/... paths outside the existing Linux isWindows() === false mock, so Windows prefixes the cwd and/or normalizes separators.

One smaller completeness issue: /%3F/g does not handle lowercase %3f, although percent-encoding hex digits are case-insensitive under RFC 3986 §2.1.

Suggested fix:

  • Decode reserved escapes before decodeURI() so each original escape is consumed once.
  • Make the %3F match case-insensitive.
  • Add regression coverage for a literal %3F filename, lowercase %3f, and an end-to-end parse/$ref.
  • Keep the literal-? filename tests POSIX-only (or place them inside the existing Linux mock).

The focused and full suites pass locally on macOS, and I did not find a direct SSRF or path-traversal issue, but the wrong-file read and failing Windows checks should be fixed before merge.

…ecoding

Addresses review feedback from @jonluca on this PR.

Root cause: fromFileSystemPath percent-escapes a literal "%" character as
"%25", so a real filename containing the literal text "%3F" round-trips
through encoding as "%253F". toFileSystemPath previously ran decodeURI()
BEFORE the manual reserved-char decode pass -- decodeURI decodes "%25" back
to a literal "%", which reveals a literal "%3F" substring that did not
exist in the encoded form. The manual pass then wrongly decoded that
revealed text a second time (%3F -> "?"), so parse("defs%3F1.json") would
silently read a "defs?1.json" sibling file instead -- a real path-aliasing
bug, and double-decoding a single escape violates RFC 3986 section 2.4.

Fix: run the manual reserved-char decode pass BEFORE decodeURI, not after.
The literal "%253F" does not contain the substring "%3F" (it's "%25"
followed by "3F"), so it's untouched by the manual pass and correctly
decoded exactly once by decodeURI (%25 -> %). Also made the reserved-char
patterns case-insensitive (RFC 3986 section 2.1: percent-encoding hex
digits are case-insensitive, so "%3f" must decode the same as "%3F" --
the previous pattern was uppercase-only).

Tests: added a regression test reproducing the exact double-decoding case
(confirmed it fails without the fix, passes with it), a lowercase-%3f case,
and moved the existing round-trip tests inside the same isWindows()=false
mock the neighboring "Handle Linux file paths" block already uses (they
were asserting absolute-POSIX-path behavior outside any Windows mock, which
is why the Windows CI job was failing). Full suite: 77/77 files, 525
passed, 0 failed, unchanged from before this commit.
@patchwright

Copy link
Copy Markdown
Author

Thanks for the thorough review — fixed all four points:

  1. Double-decoding regression: moved the manual reserved-char decode pass to run before decodeURI() instead of after. Root cause was exactly as you diagnosed — fromFileSystemPath escapes a literal % as %25, so a filename literally named defs%3F1.json round-trips as defs%253F1.json. Running decodeURI first turns %25 back into %, which reveals a literal %3F substring that wasn't there in the encoded form, and the old ordering then decoded that revealed text a second time. Running the reserved-char pass first, %253F doesn't contain the substring %3F (it's %25 followed by 3F), so it's left alone and decoded exactly once by decodeURI afterward.
  2. Case sensitivity: the reserved-char patterns now match case-insensitively (%3f decodes the same as %3F), per RFC 3986 §2.1.
  3. New regression tests: one reproducing your exact double-decoding case (verified it fails on the pre-fix code, passes now), one for the lowercase-%3f case.
  4. Windows CI: moved the round-trip tests inside the same isWindows() = false mock the neighboring "Handle Linux file paths" block already uses — they were asserting absolute-POSIX-path behavior with no Windows mock, which is why they were failing there.

Full suite: 77/77 files, 525 passed, 0 failed. Didn't add a full end-to-end parse()/$ref fixture test for time reasons — the unit-level round-trip test above exercises the exact same code path (toFileSystemPath) that the resolver calls, but happy to add one if you'd still like it before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

toFileSystemPath does not decode %3F back to "?", breaking round-trip for paths containing a literal question mark

2 participants