fix(http): preserve caller's URL encoding in path params#1600
fix(http): preserve caller's URL encoding in path params#1600hugo-ccabral wants to merge 1 commit into
Conversation
normalizePathParam decoded the input repeatedly while the path-builder re-encoded only once, collapsing the caller's intended encoding level (e.g. "%2520" became "%20"). The wildcard interpolation also encoded "/" as "%2F", breaking wildcard route segments. Together this broke admin's daemon fs reads for filenames containing %-escaped characters or spaces. Validate against a fully-decoded copy (so encoded path-traversal is still caught, including double-encoded variants) but return the original input verbatim and skip the re-encoding step. This restores the pre-#1525 pass-through behaviour while keeping the traversal, absolute-path, and null-byte protections introduced there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThis PR refactors HTTP path parameter validation in ChangesPath Parameter Validation Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/http.ts`:
- Around line 119-121: The catch that swallows decodeURIComponent errors should
instead treat undecodable input as invalid: in the decode block (where
decodeURIComponent is called and the local variable decoded is set) do not leave
the original encoded string in place on error—return/throw a validation failure
so the caller (e.g., the path validation logic around decoded) rejects the
input; update that catch to mark the input as undecodable (for example by
returning false or throwing a specific error) so inputs like `%2e%2e%2f%GGfoo`
cannot bypass the `../` check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| } catch { | ||
| // If decode fails, keep the last successful decoded value | ||
| // Do not reset to original string as that would bypass security checks | ||
| // Partial decode is acceptable for validation purposes. | ||
| } |
There was a problem hiding this comment.
Consider rejecting inputs that fail to fully decode.
If decodeURIComponent throws on malformed sequences (e.g., %GG), validation proceeds against the original encoded string. An input like %2e%2e%2f%GGfoo would bypass the ../ check since decoded remains unchanged at %2e%2e%2f%GGfoo.
While standard URL decoders should also fail on such input, some non-conforming servers or proxies may decode valid sequences while ignoring invalid ones. Rejecting undecodable input would close this edge case.
🛡️ Suggested hardening
} catch {
- // Partial decode is acceptable for validation purposes.
+ throw new Error(
+ `Invalid URL encoding in parameter '${paramName}'`,
+ );
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/http.ts` around lines 119 - 121, The catch that swallows
decodeURIComponent errors should instead treat undecodable input as invalid: in
the decode block (where decodeURIComponent is called and the local variable
decoded is set) do not leave the original encoded string in place on
error—return/throw a validation failure so the caller (e.g., the path validation
logic around decoded) rejects the input; update that catch to mark the input as
undecodable (for example by returning false or throwing a specific error) so
inputs like `%2e%2e%2f%GGfoo` cannot bypass the `../` check.
![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 (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="utils/http.ts">
<violation number="1" location="utils/http.ts:120">
P2: Security: When `decodeURIComponent` throws on malformed sequences (e.g., `%2e%2e%2f%GGfoo`), the entire decode fails and `decoded` remains equal to the original encoded string. The subsequent `includes("../")` check won't detect the traversal because it's still percent-encoded. If a downstream proxy partially decodes valid sequences while ignoring invalid ones, this could allow a traversal bypass. Consider rejecting inputs that fail to fully decode rather than silently proceeding with the un-decoded value.</violation>
<violation number="2" location="utils/http.ts:137">
P2: Allowing `.` path segments lets `URL()` collapse the path (e.g. `/users/.` -> `/users/`), so a literal path param can resolve to a different endpoint.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const segments = decoded.replace(/\\/g, "/").split("/"); | ||
| for (const segment of segments) { | ||
| if (segment === ".." || segment === ".") { | ||
| if (segment === "..") { |
There was a problem hiding this comment.
P2: Allowing . path segments lets URL() collapse the path (e.g. /users/. -> /users/), so a literal path param can resolve to a different endpoint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/http.ts, line 137:
<comment>Allowing `.` path segments lets `URL()` collapse the path (e.g. `/users/.` -> `/users/`), so a literal path param can resolve to a different endpoint.</comment>
<file context>
@@ -97,78 +97,57 @@ export interface HttpClientOptions {
+ const segments = decoded.replace(/\\/g, "/").split("/");
for (const segment of segments) {
- if (segment === ".." || segment === ".") {
+ if (segment === "..") {
throw new Error(
`Invalid path segment in parameter '${paramName}'`,
</file context>
| if (segment === "..") { | |
| if (segment === ".." || segment === ".") { |
| } catch { | ||
| // If decode fails, keep the last successful decoded value | ||
| // Do not reset to original string as that would bypass security checks | ||
| // Partial decode is acceptable for validation purposes. |
There was a problem hiding this comment.
P2: Security: When decodeURIComponent throws on malformed sequences (e.g., %2e%2e%2f%GGfoo), the entire decode fails and decoded remains equal to the original encoded string. The subsequent includes("../") check won't detect the traversal because it's still percent-encoded. If a downstream proxy partially decodes valid sequences while ignoring invalid ones, this could allow a traversal bypass. Consider rejecting inputs that fail to fully decode rather than silently proceeding with the un-decoded value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/http.ts, line 120:
<comment>Security: When `decodeURIComponent` throws on malformed sequences (e.g., `%2e%2e%2f%GGfoo`), the entire decode fails and `decoded` remains equal to the original encoded string. The subsequent `includes("../")` check won't detect the traversal because it's still percent-encoded. If a downstream proxy partially decodes valid sequences while ignoring invalid ones, this could allow a traversal bypass. Consider rejecting inputs that fail to fully decode rather than silently proceeding with the un-decoded value.</comment>
<file context>
@@ -97,78 +97,57 @@ export interface HttpClientOptions {
} catch {
- // If decode fails, keep the last successful decoded value
- // Do not reset to original string as that would bypass security checks
+ // Partial decode is acceptable for validation purposes.
}
</file context>
| // Partial decode is acceptable for validation purposes. | |
| throw new Error( | |
| `Invalid URL encoding in parameter '${paramName}'`, | |
| ); |
1 participant
Summary
Regression introduced by #1525 (commit 3ff0d90): when admin tries to fetch a file whose name contains a space (or any pre-encoded character), the path on the wire arrived wrong, causing 404s.
normalizePathParamdecoded the input in a loop until fixed point, thenencodePathSegmentre-encoded only once — collapsing the caller's encoding level by one. The same path also encoded/as%2F, breaking wildcard route segments (later partially fixed in #1528 / #1533).Repro:
filepath = "/.deco/blocks/pages-Home%2520ETC-330706.json"GET /fs/file/.deco%2Fblocks%2Fpages-Home%20ETC-330706.jsonGET /fs/file/.deco/blocks/pages-Home%2520ETC-330706.json✓ (matches pre-Tavano/fix path traversal #1525 behaviour)Fix
encodePathSegmenthelper.This restores the pre-#1525 pass-through behaviour while preserving the security checks #1525 introduced:
../,..\, absolute paths, and null bytes.Security
All attacks still blocked by validating the fully-decoded form:
../../etc/passwd→../literal match..%2F..%2Fpasswd→ multi-decode →../../passwd→ caught..%252F..%252Fpasswd→ decoded to fixed point → caught/etc/passwd,%2Fetc%2Fpasswd→ absolute path check on decoded formfoo\0bar→ null-byte check on both raw and decoded..\) → checked explicitlyOne intentional relaxation: dropped the
segment === "."rejection (a bare.segment is harmless; only..is a traversal vector). Happy to add it back if preferred.Test plan
/.deco/blocks/pages-Home%2520ETC-330706.jsonvia the daemon fs/read loader../,..%2F,..%252F, leading/) still rejected with HTTP 400🤖 Generated with Claude Code
Summary by cubic
Preserves caller URL encoding in HTTP path params to fix 404s and broken wildcard routes for pre-encoded filenames (e.g.
%2520). Restores pre-#1525 pass-through while keeping traversal and absolute-path protections.encodePathSegment; stop encoding/as%2Fin wildcard params.../,..\, leading/or\, and null bytes (checked on raw and decoded)..segments again (only..is blocked).Written for commit 8fdc9bd. Summary will update on new commits. Review in cubic
Summary by CodeRabbit