add normalizePathParam function in http.ts util#1524
add normalizePathParam function in http.ts util#15240xcucumbersalad wants to merge 1 commit intodeco-cx:mainfrom
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe update introduces public APIs (HttpError class, TypedRequestInit and TypedResponse interfaces), enhances HttpClientOptions with header processing and custom fetch support, and implements internal path parameter normalization with security validations to prevent path traversal attacks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
2 issues 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:124">
P1: Bug: `decoded.includes("..")` is overly broad — it rejects legitimate values like `file..txt` or `data..backup` that contain consecutive dots but are not path traversal attempts. Step 5 already correctly catches actual `..` path segments. This check should be removed or narrowed to match `..` only as a path segment (e.g., using a regex like `/(?:^|[\/])\.\.(?:[\/]|$)/`).</violation>
<violation number="2" location="utils/http.ts:261">
P1: Bug: Array path params (wildcard routes like `/*`) are broken. `String(param)` on an array produces comma-separated values (e.g., `"a,b"`) instead of preserving array semantics for `flatMap` to expand. This silently breaks any route using wildcard params. You need to handle the array case separately — normalize each element individually and return the array so `flatMap` can expand it.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| // Step 2: Check for path traversal in decoded value | ||
| if (decoded.includes("..")) { |
There was a problem hiding this comment.
P1: Bug: decoded.includes("..") is overly broad — it rejects legitimate values like file..txt or data..backup that contain consecutive dots but are not path traversal attempts. Step 5 already correctly catches actual .. path segments. This check should be removed or narrowed to match .. only as a path segment (e.g., using a regex like /(?:^|[\/])\.\.(?:[\/]|$)/).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/http.ts, line 124:
<comment>Bug: `decoded.includes("..")` is overly broad — it rejects legitimate values like `file..txt` or `data..backup` that contain consecutive dots but are not path traversal attempts. Step 5 already correctly catches actual `..` path segments. This check should be removed or narrowed to match `..` only as a path segment (e.g., using a regex like `/(?:^|[\/])\.\.(?:[\/]|$)/`).</comment>
<file context>
@@ -89,6 +97,69 @@ export interface HttpClientOptions {
+ }
+
+ // Step 2: Check for path traversal in decoded value
+ if (decoded.includes("..")) {
+ throw new Error(
+ `Path traversal detected in parameter '${paramName}': ${str}`,
</file context>
| if (decoded.includes("..")) { | |
| if (/(?:^|[\/])\.\.(?:[\/]|$)/.test(decoded)) { |
|
|
||
| // Normalize and validate path parameters to prevent path traversal | ||
| if (param !== undefined) { | ||
| const paramStr = String(param); |
There was a problem hiding this comment.
P1: Bug: Array path params (wildcard routes like /*) are broken. String(param) on an array produces comma-separated values (e.g., "a,b") instead of preserving array semantics for flatMap to expand. This silently breaks any route using wildcard params. You need to handle the array case separately — normalize each element individually and return the array so flatMap can expand it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/http.ts, line 261:
<comment>Bug: Array path params (wildcard routes like `/*`) are broken. `String(param)` on an array produces comma-separated values (e.g., `"a,b"`) instead of preserving array semantics for `flatMap` to expand. This silently breaks any route using wildcard params. You need to handle the array case separately — normalize each element individually and return the array so `flatMap` can expand it.</comment>
<file context>
@@ -186,6 +256,15 @@ export const createHttpClient = <T>(
+ // Normalize and validate path parameters to prevent path traversal
+ if (param !== undefined) {
+ const paramStr = String(param);
+ const normalized = normalizePathParam(paramStr, name);
+
</file context>
| const paramStr = String(param); | |
| if (Array.isArray(param)) { | |
| return param.map((p) => { | |
| const normalized = normalizePathParam(String(p), name); | |
| return encodeURIComponent(normalized); | |
| }); | |
| } | |
| const paramStr = String(param); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@utils/http.ts`:
- Around line 110-121: The iterative decode loop using the variable "decoded"
currently resets decoded to the original "str" in the catch block, which
discards the last successful decode and enables a bypass; update the catch so it
does not assign decoded = str (i.e., remove that reset) and simply break/exit
the loop or return the last successful "decoded" value so the function retains
the most recent successful decode from the while (prev !== decoded) loop rather
than reverting to the raw input.
- Around line 123-128: The early broad path-traversal guard that checks
decoded.includes("..") is causing false positives (rejects values like
"file..backup"); remove that entire Step 2 block (the if
(decoded.includes("..")) { throw new Error(... paramName, str ...) } ) and rely
on the later per-segment validation (the split-on-"/" logic that checks segments
=== ".." or "."). If you prefer to keep an early check instead of removing it,
narrow it to actual traversal patterns only (e.g., detect "../" or "..\\")
rather than any ".." substring.
- Around line 259-268: The normalization collapses wildcard arrays because it
calls String(param) on a potentially array-valued param; update the logic to
check Array.isArray(param) and, if true, map over each element calling
normalizePathParam(element, name) and then encodeURIComponent(...) so you return
an array of normalized/encoded segments (so the caller using .flatMap() expands
them), otherwise keep the current single-value flow that uses
normalizePathParam(paramStr, name) and encodeURIComponent.
🧹 Nitpick comments (3)
utils/http.ts (3)
153-158: Null byte check should also cover the decoded value before normalization.The check at Step 6 tests the
normalizedstring, but ifdecodeURIComponentfails (catch block resetsdecoded = str), a raw%00in the original string wouldn't be decoded to\0and would pass through normalization as literal characters%,0,0. Consider also checkingdecodedright after the decode loop for null bytes, since that's the layer where%00→\0conversion happens.🛡️ Move null byte check earlier (defense-in-depth)
} catch { // If decode fails, use original string decoded = str; } + // Block null bytes early (covers decoded %00) + if (decoded.includes("\0")) { + throw new Error( + `Null byte detected in parameter '${paramName}': ${str}`, + ); + } + // Step 2: Check for path traversal in decoded value
237-275: Path template parameters are not removed frommapped, causing them to also appear as query parameters.Template path params consumed during URL compilation remain in
mappedand are subsequently appended as search params (lines 279–289). This appears to be pre-existing behavior and not introduced by this PR, but it may be worth noting for correctness — callers must useexcludeFromSearchParamsto suppress the duplication.
100-161: Consider adding a max-iterations guard to the decode loop.The iterative
decodeURIComponentloop (lines 113–117) will converge in practice since each pass reduces encoding layers. However, as defensive coding, consider adding a small iteration cap (e.g., 10) to make the convergence guarantee explicit and protect against any theoretical edge case.♻️ Suggested guard
let prev = ""; - while (prev !== decoded) { + let maxIterations = 10; + while (prev !== decoded && maxIterations-- > 0) { prev = decoded; decoded = decodeURIComponent(decoded); }
| let decoded = str; | ||
| try { | ||
| // Decode multiple times to catch double-encoding | ||
| let prev = ""; | ||
| while (prev !== decoded) { | ||
| prev = decoded; | ||
| decoded = decodeURIComponent(decoded); | ||
| } | ||
| } catch { | ||
| // If decode fails, use original string | ||
| decoded = str; | ||
| } |
There was a problem hiding this comment.
Security bypass: the catch block discards intermediate decode results.
In the iterative decode loop, if the first decode succeeds but a subsequent decode throws (due to an invalid percent-sequence introduced by the prior decode), decoded is reset to the raw str, discarding the partially-decoded result. An attacker can exploit this by mixing double-encoded traversal sequences with an intentionally malformed percent-escape:
Example: %252e%252e%252f%25ZZ
- First decode succeeds:
%2e%2e%2f%ZZ - Second decode throws on
%ZZ→ catch resetsdecoded = str - All subsequent checks operate on the raw
%252e…string and see no.. - The returned value, after
encodeURIComponent, reaches the backend which may double-decode it into../
The fix is to let decoded keep its last successfully decoded value (which it already holds when the exception fires):
🔒 Proposed fix
} catch {
- // If decode fails, use original string
- decoded = str;
+ // decodeURIComponent threw; `decoded` already holds the last
+ // successfully decoded value from the previous loop iteration.
}🤖 Prompt for AI Agents
In `@utils/http.ts` around lines 110 - 121, The iterative decode loop using the
variable "decoded" currently resets decoded to the original "str" in the catch
block, which discards the last successful decode and enables a bypass; update
the catch so it does not assign decoded = str (i.e., remove that reset) and
simply break/exit the loop or return the last successful "decoded" value so the
function retains the most recent successful decode from the while (prev !==
decoded) loop rather than reverting to the raw input.
| // Step 2: Check for path traversal in decoded value | ||
| if (decoded.includes("..")) { | ||
| throw new Error( | ||
| `Path traversal detected in parameter '${paramName}': ${str}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
includes("..") is overly broad — rejects legitimate values like file..backup or v2..0.
This check fires on any occurrence of two consecutive dots anywhere in the string, not just ../ traversal sequences. Step 5 (lines 143–151) already correctly validates individual path segments against ".." and "." after splitting on /. This makes Step 2 both redundant and a source of false positives.
Consider removing Step 2 entirely and relying on the more precise Step 5, or narrowing the check to ../ and ..\ patterns:
🐛 Option A (preferred): Remove Step 2, rely on Step 5
- // Step 2: Check for path traversal in decoded value
- if (decoded.includes("..")) {
- throw new Error(
- `Path traversal detected in parameter '${paramName}': ${str}`,
- );
- }
-
// Step 3: Block absolute paths🐛 Option B: Narrow the check to actual traversal patterns
- if (decoded.includes("..")) {
+ if (/\.\.[\\/]|[\\/]\.\./.test(decoded) || decoded === "..") {📝 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.
| // Step 2: Check for path traversal in decoded value | |
| if (decoded.includes("..")) { | |
| throw new Error( | |
| `Path traversal detected in parameter '${paramName}': ${str}`, | |
| ); | |
| } | |
| // Step 3: Block absolute paths |
| // Step 2: Check for path traversal in decoded value | |
| if (decoded.includes("..")) { | |
| throw new Error( | |
| `Path traversal detected in parameter '${paramName}': ${str}`, | |
| ); | |
| } | |
| // Step 2: Check for path traversal in decoded value | |
| if (/\.\.[\\/]|[\\/]\.\./.test(decoded) || decoded === "..") { | |
| throw new Error( | |
| `Path traversal detected in parameter '${paramName}': ${str}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@utils/http.ts` around lines 123 - 128, The early broad path-traversal guard
that checks decoded.includes("..") is causing false positives (rejects values
like "file..backup"); remove that entire Step 2 block (the if
(decoded.includes("..")) { throw new Error(... paramName, str ...) } ) and rely
on the later per-segment validation (the split-on-"/" logic that checks segments
=== ".." or "."). If you prefer to keep an early check instead of removing it,
narrow it to actual traversal patterns only (e.g., detect "../" or "..\\")
rather than any ".." substring.
| // Normalize and validate path parameters to prevent path traversal | ||
| if (param !== undefined) { | ||
| const paramStr = String(param); | ||
| const normalized = normalizePathParam(paramStr, name); | ||
|
|
||
| // URL encode to prevent injection attacks | ||
| return encodeURIComponent(normalized); | ||
| } | ||
|
|
||
| return param; |
There was a problem hiding this comment.
Bug: Array path parameters (wildcards) are broken by normalization.
When the URL pattern uses a wildcard (e.g., /* or /*files), param can be an array (string[] | number[]). The original code returned the array to .flatMap(), which spread it into multiple path segments. Now, String(param) joins array elements with commas (e.g., ["docs", "file.pdf"] → "docs,file.pdf"), collapsing what should be separate segments into one.
You need to handle the array case separately, normalizing each element individually:
🐛 Proposed fix
// Normalize and validate path parameters to prevent path traversal
if (param !== undefined) {
- const paramStr = String(param);
- const normalized = normalizePathParam(paramStr, name);
-
- // URL encode to prevent injection attacks
- return encodeURIComponent(normalized);
+ const values = Array.isArray(param) ? param : [param];
+ return values.map((v) => {
+ const normalized = normalizePathParam(String(v), name);
+ return encodeURIComponent(normalized);
+ });
}#!/bin/bash
# Verify wildcard parameter usage in the codebase to assess impact
rg -n --type=ts -C3 '\*' utils/http.ts | head -40
echo "---"
# Search for wildcard path patterns in type definitions across the codebase
rg -n --type=ts 'GET \/' -g '!node_modules' | grep '\*' | head -20🤖 Prompt for AI Agents
In `@utils/http.ts` around lines 259 - 268, The normalization collapses wildcard
arrays because it calls String(param) on a potentially array-valued param;
update the logic to check Array.isArray(param) and, if true, map over each
element calling normalizePathParam(element, name) and then
encodeURIComponent(...) so you return an array of normalized/encoded segments
(so the caller using .flatMap() expands them), otherwise keep the current
single-value flow that uses normalizePathParam(paramStr, name) and
encodeURIComponent.
2 participants
What is this Contribution About?
This PR adds path traversal protection to the
createHttpClientutility by implementing automatic normalization and validation of URL path parameters.Changes:
normalizePathParam()function to sanitize and validate path parameters../,..\\, encoded variants)/etc/passwd)Security Benefits:
../path traversal attempts%2e%2e%2f)docs//file.pdf→docs/file.pdf)Example: