Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughAdds typed HTTP primitives and an HttpError class, extends HttpClientOptions, and implements path-parameter normalization/validation integrated into URL construction and debug logging for the HTTP utility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UrlBuilder as URL Builder
participant Normalizer as normalizePathParam
participant Fetcher as fetcher
participant Processor as processHeaders
participant Server as External API
Client->>UrlBuilder: build URL from pattern + params
UrlBuilder->>Normalizer: normalize each path param (value, name)
Normalizer-->>UrlBuilder: sanitized/encoded segment or throw HttpError
UrlBuilder-->>Client: final sanitized URL
Client->>Fetcher: send request to final URL (using configured fetcher)
Fetcher->>Server: HTTP request
Server-->>Fetcher: HTTP response
Fetcher->>Processor: provide response.headers (if configured)
Processor-->>Client: processed headers
Fetcher-->>Client: response body or throw HttpError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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:124">
P1: Bug: `decoded.includes("..")` is overly broad — it rejects any parameter value containing two consecutive dots (e.g., `file..name`, `v1..v2`) even when they aren't path traversal. Step 5 already correctly validates individual path segments with `segment === ".."`. This substring check should be removed or narrowed to a segment-based regex to avoid false positives on legitimate inputs.</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 any parameter value containing two consecutive dots (e.g., file..name, v1..v2) even when they aren't path traversal. Step 5 already correctly validates individual path segments with segment === "..". This substring check should be removed or narrowed to a segment-based regex to avoid false positives on legitimate inputs.
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 any parameter value containing two consecutive dots (e.g., `file..name`, `v1..v2`) even when they aren't path traversal. Step 5 already correctly validates individual path segments with `segment === ".."`. This substring check should be removed or narrowed to a segment-based regex to avoid false positives on legitimate inputs.</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 (decoded.split(/[\/\\]/).some(seg => seg === "..")) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@utils/http.ts`:
- Around line 124-128: The thrown errors currently include raw user input (the
variable `str`) which can be reflected into HTTP responses; update the
validation to avoid echoing `str` in error messages (use a generic message
referencing `paramName` and the issue, e.g., "Invalid parameter" or "Path
traversal detected") and ensure callers of this validation (where `decoded`,
`paramName`, and `str` are used) catch these thrown errors and translate them
into a generic 400 response body without including the original input.
- Around line 259-279: The change applies encodeURIComponent to normalized path
params (using normalizePathParam) which encodes slashes so single-string
wildcard params like "path/to/file" become "path%2Fto%2Ffile" and no longer
create multiple path segments; update the public documentation and inline
comment near the wildcard handling code (the block that handles
Array.isArray(param) and the single-value path branch) to explain this
behavioral change and instruct callers to pass wildcard segments as arrays
(e.g., ["path","to","file"]) if they relied on literal slashes, and add/adjust
one unit test for wildcard routing to assert the new behavior; mention
encodeURIComponent, normalizePathParam, and the wildcard route pattern (/*) in
the docs/comment.
🧹 Nitpick comments (2)
utils/http.ts (2)
100-161:normalizePathParamis well-designed; two observations worth noting.
Over-broad
..check (line 124):decoded.includes("..")rejects any parameter containing two consecutive dots anywhere, including legitimate filenames likemy..file.txt. This is the safe/conservative choice for a security function, but document this trade-off so callers know.Null byte check ordering (Step 6): Consider moving the null-byte check (lines 153-158) before the normalization regex in Step 4. A null byte in
decodedcould potentially affect regex behavior in some engines. Practically this is fine in V8/Deno, but checking and rejecting null bytes as early as possible is a defense-in-depth best practice.Suggested reorder: check null bytes right after Step 2
// 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 null bytes (moved up for defense-in-depth) + if (decoded.includes("\0")) { + throw new Error( + `Null byte detected in parameter '${paramName}': ${str}`, + ); + } + // Step 3: Block absolute paths if (decoded.startsWith("/") || decoded.startsWith("\\")) {(and remove the original Step 6 block)
110-121: Iterative decoding loop is solid, but consider a max-iteration guard.The
while (prev !== decoded)loop terminates when decoding produces no change. In theory this always converges, but a maliciously crafted string that somehow causesdecodeURIComponentto oscillate (extremely unlikely) would loop forever. A simple iteration cap (e.g., 10) provides defense-in-depth at zero cost.Suggested safeguard
let decoded = str; try { let prev = ""; - while (prev !== decoded) { + let iterations = 0; + while (prev !== decoded && iterations++ < 10) { prev = decoded; decoded = decodeURIComponent(decoded); } } catch {
guitavano
force-pushed
the
tavano/fix-path-traversal
branch
from
b1f503e to
f9b4d4a
Compare
…alidation in createHttpClient
2 participants
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by cubic
Hardened the HTTP client against path traversal with strict per-parameter normalization, validation, and URL-encoding. Invalid params now return a generic 400 error; also standardized debug redaction and fixed a typo.
Written for commit 4c2fd24. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes