chore(backend): Harden joinPaths against traversal#8331
chore(backend): Harden joinPaths against traversal#8331dominic-clerk wants to merge 4 commits intomainfrom
joinPaths against traversal#8331Conversation
There is no immediate vulnerability but it could be possible that someone uses one of the functions that call `joinPath` in a way that accepts user input and allows traversal on the backend API. This adds protection to avoid this problem.
🦋 Changeset detectedLatest commit: 4c61964 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dot-segment detection to the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. 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 the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/util/path.ts`:
- Around line 6-14: isDotSegment currently only rejects when decoded === "." or
"..", which lets encoded traversals like "%2e%2e%2fusers" slip through; change
isDotSegment to fully decode percent-encodings (iteratively until stable to
catch double-encoding), then reject if the decoded value equals "." or ".." or
begins with "../" or "./" or contains "/" (i.e., any dot-segment with a
separator) so encoded separators cannot bypass the check; apply the same
stricter logic to the related guard referenced in the review (the analogous
checks at the other occurrence around lines 22-25) and add regression tests that
assert rejection for "%2e%2e%2fusers", "%2e%2e%252fusers" (double-encoded) and
similar variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d240cf83-cc3a-4e78-ab33-7c3de18a7c64
📒 Files selected for processing (2)
packages/backend/src/util/__tests__/path.test.tspackages/backend/src/util/path.ts
| function isDotSegment(segment: string): boolean { | ||
| let decoded: string; | ||
| try { | ||
| decoded = decodeURIComponent(segment); | ||
| } catch { | ||
| decoded = segment; | ||
| } | ||
| return decoded === '.' || decoded === '..'; | ||
| } |
There was a problem hiding this comment.
Blocker: encoded separators bypass the dot-segment traversal check
At Line 13, the guard only rejects when the decoded segment is exactly "." or ".." once. Payloads like %2e%2e%2fusers decode to ../users and pass current validation, which can re-enable traversal if any downstream layer decodes %2f.
🔒 Suggested fix
function isDotSegment(segment: string): boolean {
- let decoded: string;
- try {
- decoded = decodeURIComponent(segment);
- } catch {
- decoded = segment;
- }
- return decoded === '.' || decoded === '..';
+ let candidate = segment;
+
+ for (let i = 0; i < 3; i++) {
+ if (candidate.split(/[\\/]/).some(part => part === '.' || part === '..')) {
+ return true;
+ }
+
+ try {
+ const next = decodeURIComponent(candidate);
+ if (next === candidate) {
+ break;
+ }
+ candidate = next;
+ } catch {
+ break;
+ }
+ }
+
+ return false;
}Please also add regression coverage for %2e%2e%2f... (and ideally double-encoded variants) to lock this down.
Also applies to: 22-25
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/util/path.ts` around lines 6 - 14, isDotSegment
currently only rejects when decoded === "." or "..", which lets encoded
traversals like "%2e%2e%2fusers" slip through; change isDotSegment to fully
decode percent-encodings (iteratively until stable to catch double-encoding),
then reject if the decoded value equals "." or ".." or begins with "../" or "./"
or contains "/" (i.e., any dot-segment with a separator) so encoded separators
cannot bypass the check; apply the same stricter logic to the related guard
referenced in the review (the analogous checks at the other occurrence around
lines 22-25) and add regression tests that assert rejection for
"%2e%2e%2fusers", "%2e%2e%252fusers" (double-encoded) and similar variants.
There was a problem hiding this comment.
I have a fix for %2e%2e%2fusers but does the double-encoded variant make sense to check here?
There was a problem hiding this comment.
@dominic-clerk — great question. It depends on what your single-encoding fix looks like, but here's the key concern:
%2e%2e%252fusers single-decodes to ..%2fusers. Because %2f is not a literal /, this passes through the segment-split step as a single token. If your isDotSegment check then calls decodeURIComponent('..%2fusers') and gets '../users', which is not strictly equal to '.' or '..', the check passes — even with the single-encoding fix, unless you also reject decoded values that contain a /.
So double-encoding matters as a bypass of the single-encoding fix, not just as a standalone concern.
Whether to defend against it here depends on what happens downstream. Most HTTP clients (fetch, node:http, etc.) re-encode the path before sending, which would normalize a double-encoded segment and prevent the traversal from reaching the server. But if any layer passes the path verbatim (some proxies, internal forwarding, or test harnesses do), a second decode re-enables the traversal.
Recommendation: Given this is an explicit security hardening PR, iterative decoding to stability is the right call. It's not complex:
function isDotSegment(segment: string): boolean {
let candidate = segment;
for (let i = 0; i < 3; i++) {
// After decoding, check if any slash-separated part is a dot segment
if (candidate.split(/[/\\]/).some(p => p === '.' || p === '..')) {
return true;
}
try {
const next = decodeURIComponent(candidate);
if (next === candidate) break; // stable — no more encoding
candidate = next;
} catch {
break;
}
}
return false;
}This catches:
../.(plain)%2e%2e/%2e(single-encoded)%2e%2e%2fusers(encoded slash embedded)%2e%2e%252fusers(double-encoded slash)
The iteration cap of 3 prevents pathological inputs from causing performance issues. I'd say it's worth including — it's a small addition for meaningful defense-in-depth.
🧠 Learnings used
Learnt from: wobsoriano
Repo: clerk/javascript PR: 7883
File: packages/backend/src/api/__tests__/M2MTokenApi.test.ts:414-419
Timestamp: 2026-02-24T18:03:27.067Z
Learning: In TypeScript files within the clerk/javascript repository, enforce explicit return type annotations for exported functions and public APIs, but allow internal test helper functions without explicit return types. This helps maintain API clarity and type safety for consumers while not burdening test helpers that are not part of the public surface.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@clerk/backend': patch | |||
There was a problem hiding this comment.
Minor maybe? I wasn't sure
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/util/path.ts`:
- Around line 8-23: The current loop checks for dot segments before decoding, so
the final decoded value isn't inspected; adjust the logic in the decode loop
around the variable candidate and the loop using i (currently i < 3) so that you
check for '.' or '..' after each decode (and also check the original candidate
once before any decode). Concretely, either move the
candidate.split(/[/\\]/).some(...) check to run after the decode step inside the
loop (so the last decode is examined) or restructure the loop to decode first
then test (ensuring the original candidate is tested once before the loop),
referencing the existing candidate variable and the for-loop that uses i < 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 30dc05a2-3bdd-4806-a77b-6728a82f6f97
📒 Files selected for processing (2)
packages/backend/src/util/__tests__/path.test.tspackages/backend/src/util/path.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/util/tests/path.test.ts
| break; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
I think CodeRabbit is right about this one. The loop runs i = 0,1,2, and at i=2 we decode a 3rd time but exit before that decoded value gets checked. So %25252e%25252e%25252fusers walks down to ../users and we still return false. Easy fix is bumping the loop to i <= 3 and breaking before the decode at i === 3, so the final decoded candidate gets checked without adding a 4th decode round
There was a problem hiding this comment.
Oooh right I misread the finding and thought he meant we'd miss out on the i == 3 loop. Thanks!
There was a problem hiding this comment.
I fixed it and changed the approach a bit. We decode until it's clean and there's a max number of 10 iterations where it becomes just silly and for sure someone's doing something strange and we reject it.
2 participants
Description
There is no immediate vulnerability but it could be possible that someone uses one of the functions that call
joinPathin a way that accepts user input and allows traversal on the backend API. This adds protection to avoid this problem.Fixes SDK-60
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change