Enhance path normalization to support Windows file paths in createHtt…#1527
Enhance path normalization to support Windows file paths in createHtt…#1527
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughSecurity enhancement to path traversal guard in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="utils/http.ts">
<violation number="1" location="utils/http.ts:202">
P0: Critical bug: `path.win32.normalize()` converts `/` to `\`, so `normalizeWin32(compiled) !== compiled` is **always true** for any path containing a `/`. This breaks every multi-segment HTTP client request with a spurious path traversal error.
To detect Windows-style path traversal (e.g. `..\\..\\etc\\passwd`), check for backslashes in the compiled path instead:</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (normalize(compiled) !== compiled) { | ||
| if ( | ||
| normalize(compiled) !== compiled || | ||
| normalizeWin32(compiled) !== compiled |
There was a problem hiding this comment.
P0: Critical bug: path.win32.normalize() converts / to \, so normalizeWin32(compiled) !== compiled is always true for any path containing a /. This breaks every multi-segment HTTP client request with a spurious path traversal error.
To detect Windows-style path traversal (e.g. ..\\..\\etc\\passwd), check for backslashes in the compiled path instead:
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/http.ts, line 202:
<comment>Critical bug: `path.win32.normalize()` converts `/` to `\`, so `normalizeWin32(compiled) !== compiled` is **always true** for any path containing a `/`. This breaks every multi-segment HTTP client request with a spurious path traversal error.
To detect Windows-style path traversal (e.g. `..\\..\\etc\\passwd`), check for backslashes in the compiled path instead:</comment>
<file context>
@@ -196,7 +197,10 @@ export const createHttpClient = <T>(
- if (normalize(compiled) !== compiled) {
+ if (
+ normalize(compiled) !== compiled ||
+ normalizeWin32(compiled) !== compiled
+ ) {
throw new Error(
</file context>
![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
🤖 Fix all issues with AI agents
In `@utils/http.ts`:
- Around line 200-203: The check using normalizeWin32(compiled) is
false-positive because compiled is built with forward slashes (join("/")) so
normalizeWin32 converts them to backslashes and will never equal compiled; fix
by replacing the normalizeWin32(compiled) equality check with an explicit
Win32-traversal/backslash presence check on the POSIX-normalized path (e.g.
reject if compiled contains "\\" or sequences like "..\\" or ".\\") and remove
the node:path/win32 import, or if you prefer to keep normalizeWin32, first
convert compiled's slashes to backslashes (compiled.replace(/\//g,'\\')) before
calling normalizeWin32 and comparing; update the logic around
normalize(compiled), normalizeWin32, and compiled accordingly.
| if ( | ||
| normalize(compiled) !== compiled || | ||
| normalizeWin32(compiled) !== compiled | ||
| ) { |
There was a problem hiding this comment.
Critical: normalizeWin32 converts / → \, breaking all multi-segment paths.
compiled is built by .join("/") on line 198, so it always uses forward slashes (e.g. "api/v1/users/123"). normalizeWin32 converts all forward slashes to backslashes, so normalizeWin32("api/v1/users/123") returns "api\\v1\\users\\123", which will never equal compiled. This means every request with more than one path segment will throw a path-traversal error.
Instead, you should only check for the presence of Win32 traversal sequences (..\\, .\\, or lone backslashes) in the already POSIX-normalized path:
🐛 Proposed fix
- if (
- normalize(compiled) !== compiled ||
- normalizeWin32(compiled) !== compiled
- ) {
+ if (
+ normalize(compiled) !== compiled ||
+ compiled.includes("\\")
+ ) {This keeps the existing POSIX normalization check and additionally rejects any backslash in the path, which prevents Win32-style traversal (e.g. ..\\..\\etc\\passwd) without false-positiving on normal forward-slash paths. The node:path/win32 import can then be removed.
Alternatively, if you want to keep using normalizeWin32, you must first convert compiled to use backslashes before comparing:
- if (
- normalize(compiled) !== compiled ||
- normalizeWin32(compiled) !== compiled
- ) {
+ const compiledWin32 = compiled.replaceAll("/", "\\");
+ if (
+ normalize(compiled) !== compiled ||
+ normalizeWin32(compiledWin32) !== compiledWin32
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| normalize(compiled) !== compiled || | |
| normalizeWin32(compiled) !== compiled | |
| ) { | |
| if ( | |
| normalize(compiled) !== compiled || | |
| compiled.includes("\\") | |
| ) { |
🤖 Prompt for AI Agents
In `@utils/http.ts` around lines 200 - 203, The check using
normalizeWin32(compiled) is false-positive because compiled is built with
forward slashes (join("/")) so normalizeWin32 converts them to backslashes and
will never equal compiled; fix by replacing the normalizeWin32(compiled)
equality check with an explicit Win32-traversal/backslash presence check on the
POSIX-normalized path (e.g. reject if compiled contains "\\" or sequences like
"..\\" or ".\\") and remove the node:path/win32 import, or if you prefer to keep
normalizeWin32, first convert compiled's slashes to backslashes
(compiled.replace(/\//g,'\\')) before calling normalizeWin32 and comparing;
update the logic around normalize(compiled), normalizeWin32, and compiled
accordingly.
1 participant
Summary by cubic
Extend path normalization in createHttpClient to also check Windows-style paths (node:path/win32). This closes a gap in path traversal detection on Windows and keeps behavior consistent across platforms.
Written for commit 9b1ddcd. Summary will update on new commits.
Summary by CodeRabbit