Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dark-bars-wink.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor maybe? I wasn't sure

---

Add path traversal protections in `joinPaths`
46 changes: 46 additions & 0 deletions packages/backend/src/util/__tests__/path.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,50 @@ describe('utils.joinPaths(...args)', () => {
it('handles no input', () => {
expect(joinPaths()).toBe('');
});

it('accepts "." and ".." within a segment (not entire segment)', () => {
// Dot not as an isolated path segment
expect(joinPaths('foo.bar', 'baz')).toBe('foo.bar/baz');
expect(joinPaths('foo..bar', 'baz')).toBe('foo..bar/baz');
expect(joinPaths('foo.', 'bar.')).toBe('foo./bar.');
expect(joinPaths('foo..', '..bar')).toBe('foo../..bar');
expect(joinPaths('foo..baz')).toBe('foo..baz');
expect(joinPaths('fo.o', 'ba..z')).toBe('fo.o/ba..z');
});

it('accepts "." and ".." inside query parameter or as value', () => {
// . and .. as values in query string should not be considered dot segments
expect(joinPaths('/api', 'users?filter=..')).toBe('/api/users?filter=..');
expect(joinPaths('/api', 'users?filter=.')).toBe('/api/users?filter=.');
expect(joinPaths('/v1', 'search?q=foo.bar..baz')).toBe('/v1/search?q=foo.bar..baz');
// . and .. within querystring, fragment, or a value
expect(joinPaths('/foo', '?bar=..&baz=.')).toBe('/foo/?bar=..&baz=.');
expect(joinPaths('/foo', '#frag..ment')).toBe('/foo/#frag..ment');
});

it('rejects literal ".." segments', () => {
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '../../../users')).toThrow();
expect(() => joinPaths('/sessions', '..')).toThrow();
});

it('rejects "." segments', () => {
expect(() => joinPaths('foo/./bar')).toThrow();
expect(() => joinPaths('foo', '.', 'bar')).toThrow();
expect(() => joinPaths('foo', './', 'bar')).toThrow();
});

it('rejects percent-encoded dot segments', () => {
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '%2e%2e/users')).toThrow();
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '%2E%2E/users')).toThrow();
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '.%2E/users')).toThrow();
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '%2e%2e%2fusers')).toThrow();
expect(() => joinPaths('/sessions', 'sess_abc', 'tokens', '%2e%2e%252fusers')).toThrow();
expect(() => joinPaths('foo', '%2e', 'bar')).toThrow();
});

it('allows legitimate URLs and ID-like segments', () => {
expect(joinPaths('https://api.clerk.com', 'v1', '/sessions/sess_abc/tokens/supabase')).toBe(
'https://api.clerk.com/v1/sessions/sess_abc/tokens/supabase',
);
});
});
30 changes: 29 additions & 1 deletion packages/backend/src/util/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,37 @@ const MULTIPLE_SEPARATOR_REGEX = new RegExp('(?<!:)' + SEPARATOR + '{1,}', 'g');

type PathString = string | null | undefined;

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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh right I misread the finding and thought he meant we'd miss out on the i == 3 loop. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Comment thread
dominic-clerk marked this conversation as resolved.

export function joinPaths(...args: PathString[]): string {
return args
const result = args
.filter(p => p)
.join(SEPARATOR)
.replace(MULTIPLE_SEPARATOR_REGEX, SEPARATOR);

for (const segment of result.split(SEPARATOR)) {
if (isDotSegment(segment)) {
throw new Error(`joinPaths: "." and ".." path segments are not allowed (received "${result}")`);
}
}

return result;
}
Loading