fix: Backslash incorrectly decoded (#3962)

[DreierF]

Summary by CodeRabbit

• Bug Fixes

  • Pathname parsing now preserves percent-encoded sequences (e.g., encoded backslashes) within path segments instead of decoding them, preventing unintended changes to parameters.

• Tests

  • Added coverage to verify encoded characters in path parameters remain intact during parsing.

[coderabbitai]

Walkthrough

Removes per-segment in-path URI decoding for percent-encoded sequences in regular pathname segments; segments are now left as-is. Adds a test ensuring encoded backslashes in path parameters are preserved.

Changes

Cohort / File(s)	Summary
Path decoding change packages/router-core/src/path.ts	Removed logic that split percent-encoded sequences (e.g., %25-delimited) and decoded each pathname segment; partToMatch is now preserved without per-segment decodeURI decoding.
Test: preserve encoded backslash packages/router-core/tests/path.test.ts	Added a test asserting parsing of /foo%5Cbar preserves the %5C (backslash) in the resulting path segment.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant parsePathname
  participant PathLogic

  Caller->>parsePathname: parse raw pathname (e.g. "/foo%5Cbar")
  parsePathname->>PathLogic: identify partToMatch (regular segment)
  alt Previous behavior
    PathLogic->>PathLogic: split on "%25" and decode segments (decodeURI)
    PathLogic->>parsePathname: rejoin decoded segments
  else New behavior
    PathLogic-->>parsePathname: return partToMatch unchanged
  end
  parsePathname-->>Caller: return parsed segments (preserve %5C)

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• fix(router-core): Non-nested paths #5169: Modifies pathname-segment decoding logic in packages/router-core/src/path.ts, closely related to this change.

Poem

🐰 Along the paths I nimbly hop,
Encoded bars I let them stop,
%5C stays where it belongs,
No extra decode, no wrongs,
Tests sing softly — hop, hop, hop! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix: Backslash incorrectly decoded (#3962)" is clearly and directly related to the main changes in the changeset. The code modification removes in-path decoding for percent-encoded sequences (particularly affecting %5C, which represents backslashes), and the test addition validates that backslashes in paths like '/foo%5Cbar' are preserved without decoding. The title is concise, specific, and uses a standard commit convention that clearly conveys the primary fix being implemented. Someone reviewing the repository history would immediately understand that this PR addresses backslash handling in path parsing.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4490a45 and e675ee2.

📒 Files selected for processing (2)

• packages/router-core/src/path.ts (1 hunks)
• packages/router-core/tests/path.test.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• packages/router-core/src/path.ts
• packages/router-core/tests/path.test.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/router-core/tests/path.test.ts (1)

348-353: Consider adding a complementary matching test.

The test correctly verifies that backslashes and percent signs in parameters are properly encoded during path interpolation. However, given that the PR fixes incorrect decoding behavior (per the title), consider adding a matching test that verifies the reverse operation works correctly.

Add this test in the matchPathname → path param(s) matching section to verify bidirectional correctness:

{
  name: 'should correctly decode backslashes and percent-encoded sequences in path parameters',
  input: '/b/test%5Bs%5C%252F.%5C%252Fparameter%5D',
  matchingOptions: {
    to: '/b/$postId',
  },
  expectedMatchedParams: { 
    postId: 'test[s\\%2F.\\%2Fparameter]' 
  },
},

This ensures that:

1. The encoded path can be matched against the route pattern
2. The parameter is correctly decoded back to the original value
3. The fix for incorrect decoding is validated in both directions

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4693e0e and 4490a45.

📒 Files selected for processing (2)

• packages/router-core/src/path.ts (1 hunks)
• packages/router-core/tests/path.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

• packages/router-core/tests/path.test.ts
• packages/router-core/src/path.ts

packages/router-core/**

📄 CodeRabbit inference engine (AGENTS.md)

Keep framework-agnostic core router logic in packages/router-core/

Files:

• packages/router-core/tests/path.test.ts
• packages/router-core/src/path.ts

🔇 Additional comments (1)

packages/router-core/src/path.ts (1)

348-352: LGTM: Pathname segments now correctly preserved without parsing-layer decoding.

Removing the URI decoding logic from baseParsePathname is the correct fix. The git history confirms this commit ("fix: Backslash incorrectly decoded #3962") addresses a double-decoding bug.

Verification shows:

• No parsePathname tests pass encoded strings and expect decoding (no % in test inputs)
• decodeURIComponent is already in place in matchByPath (lines 688, 692, 744, 783) for boundary decoding
• The three-layer architecture is intact: parse (preserve as-is) → match (decode at boundaries) → interpolate (encode at boundaries)
• This change aligns with the correct encoding/decoding pattern

[nlynzaad]

Thanks for the this.

When replicating the test on its own into main, without the accompanied fix, I'm expecting the test to fail. Unfortunately, this passes already without the above changes.

We should have a failing test to which the applied changes resolve the issue. This is to ensure that we both understand the need for the change and to ensure future updates don't inadvertently break the functionality without the test giving a false positive.

[DreierF]

🤦‍♂️ Good catch @nlynzaad! The new test fails without the change.