Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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`
50 changes: 50 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,54 @@ 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('rejects too many layers of encoding', () => {
expect(() => joinPaths('foo', '%2525252525252525252525252541')).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',
);
});
});
34 changes: 33 additions & 1 deletion packages/backend/src/util/path.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,43 @@
const SEPARATOR = '/';
const MULTIPLE_SEPARATOR_REGEX = new RegExp('(?<!:)' + SEPARATOR + '{1,}', 'g');
const MAX_DECODES = 10;

type PathString = string | null | undefined;

function isDotSegment(segment: string): boolean {
let candidate = segment;
for (let i = 0; i <= MAX_DECODES; i++) {
// After decoding, check if any slash-separated part is a dot segment
if (candidate.split(/[/\\]/).some(p => p === '.' || p === '..')) {
return true;
}
if (i === MAX_DECODES) {
throw new Error(`joinPaths: too many layers of encoding in ${segment}`);
}
try {
const next = decodeURIComponent(candidate);
if (next === candidate) {
break;
} // stable — no more encoding
candidate = next;
} catch {
break;
}
}
return false;
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