Harden build upload proxy against path traversal#1764
Conversation
📝 WalkthroughWalkthroughThe Tus upload proxy implements URL parsing, path decoding, and path traversal protection by validating against null characters and path segment traversal (e.g., Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as Upload Handler
participant Parser as URL Parser
participant Validator as Path Validator
participant Resolver as URL Resolver
participant Builder as Builder Service
participant Response as Response Handler
Client->>Handler: POST /upload/path
Handler->>Parser: Parse request URL pathname
Parser->>Parser: Extract originalPath
Parser->>Validator: Decode tusPath with decodeURIComponent
alt Invalid Encoding
Validator->>Handler: Throw 400 invalid_path
Handler->>Client: 400 Bad Request
else Valid Encoding
Validator->>Validator: Check for null chars, '.', '..' segments
alt Path Traversal Detected
Validator->>Handler: Throw 400 invalid_path
Handler->>Client: 400 Bad Request
else Valid Path
Validator->>Resolver: Provide validated path
Resolver->>Resolver: Construct base upload URL
Resolver->>Resolver: Resolve relative tusPath
Resolver->>Resolver: Validate no escape outside directory
alt Escape Detected
Resolver->>Handler: Throw 400 invalid_path
Handler->>Client: 400 Bad Request
else Safe Path
Resolver->>Handler: Return safeTusPath & resolvedURL
Handler->>Builder: Forward request with x-api-key header
Builder->>Builder: Process upload
Builder-->>Handler: 201 Created + Location header
Handler->>Response: Rewrite Location header if needed
Response-->>Client: 201 Created
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96040f4b9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| vi.clearAllMocks() |
There was a problem hiding this comment.
These tests run with it.concurrent(...) but share module-level mocks, and this afterEach calls vi.restoreAllMocks()/vi.clearAllMocks() globally; when one test finishes first, it can restore mocks that another concurrent test is still using (notably the globalThis.fetch spy in the success case), creating nondeterministic CI failures or unexpected real network calls. Either run this suite serially or isolate mock state per test without global restore in concurrent mode.
Useful? React with 👍 / 👎.
![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 `@tests/build-upload-security.test.ts`:
- Around line 85-109: The test currently captures the error with .catch()
causing TypeScript to infer a union (Response | HTTPException) and preventing
property access; change each test to await tusProxy(...) inside a try/catch:
call await tusProxy(context as any, jobId, ...) in the try and force a test
failure if it resolves, then in the catch block assert
expect(err).toBeInstanceOf(HTTPException), cast err to HTTPException (e.g. const
e = err as HTTPException) and then check e.cause with
expect(e.cause).toMatchObject(...); this uses tusProxy, HTTPException,
fakeContext and jobId to locate the code to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e68f3e7-06f4-4210-9241-c29a221f58dc
📒 Files selected for processing (2)
supabase/functions/_backend/public/build/upload.tstests/build-upload-security.test.ts
| it.concurrent('rejects path traversal attempts before forwarding to builder', async () => { | ||
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%2e%2e/jobs`, 'PATCH') | ||
|
|
||
| const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | ||
| .catch(err => err as HTTPException) | ||
|
|
||
| expect(error).toBeInstanceOf(HTTPException) | ||
| expect(error.cause).toMatchObject({ | ||
| error: 'invalid_path', | ||
| message: 'Invalid upload path', | ||
| }) | ||
| }) | ||
|
|
||
| it.concurrent('rejects invalidly encoded paths before forwarding to builder', async () => { | ||
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%`, 'PATCH') | ||
|
|
||
| const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | ||
| .catch(err => err as HTTPException) | ||
|
|
||
| expect(error).toBeInstanceOf(HTTPException) | ||
| expect(error.cause).toMatchObject({ | ||
| error: 'invalid_path', | ||
| message: 'Invalid upload path encoding.', | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Fix TypeScript type narrowing for pipeline to pass.
The pipeline fails because tusProxy returns Promise<Response>, so the .catch() result type is Response | HTTPException. TypeScript doesn't narrow after expect().toBeInstanceOf(), causing TS2339 on .cause access.
🐛 Proposed fix: restructure assertions for proper type narrowing
- it.concurrent('rejects path traversal attempts before forwarding to builder', async () => {
- const context = fakeContext(`http://localhost/build/upload/${jobId}/%2e%2e/jobs`, 'PATCH')
-
- const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
- .catch(err => err as HTTPException)
-
- expect(error).toBeInstanceOf(HTTPException)
- expect(error.cause).toMatchObject({
- error: 'invalid_path',
- message: 'Invalid upload path',
- })
- })
-
- it.concurrent('rejects invalidly encoded paths before forwarding to builder', async () => {
- const context = fakeContext(`http://localhost/build/upload/${jobId}/%`, 'PATCH')
-
- const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
- .catch(err => err as HTTPException)
-
- expect(error).toBeInstanceOf(HTTPException)
- expect(error.cause).toMatchObject({
- error: 'invalid_path',
- message: 'Invalid upload path encoding.',
- })
- })
+ it.concurrent('rejects path traversal attempts before forwarding to builder', async () => {
+ const context = fakeContext(`http://localhost/build/upload/${jobId}/%2e%2e/jobs`, 'PATCH')
+
+ const result = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
+ .catch(err => err)
+
+ expect(result).toBeInstanceOf(HTTPException)
+ if (result instanceof HTTPException) {
+ expect(result.cause).toMatchObject({
+ error: 'invalid_path',
+ message: 'Invalid upload path',
+ })
+ }
+ })
+
+ it.concurrent('rejects invalidly encoded paths before forwarding to builder', async () => {
+ const context = fakeContext(`http://localhost/build/upload/${jobId}/%`, 'PATCH')
+
+ const result = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
+ .catch(err => err)
+
+ expect(result).toBeInstanceOf(HTTPException)
+ if (result instanceof HTTPException) {
+ expect(result.cause).toMatchObject({
+ error: 'invalid_path',
+ message: 'Invalid upload path encoding.',
+ })
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.concurrent('rejects path traversal attempts before forwarding to builder', async () => { | |
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%2e%2e/jobs`, 'PATCH') | |
| const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | |
| .catch(err => err as HTTPException) | |
| expect(error).toBeInstanceOf(HTTPException) | |
| expect(error.cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path', | |
| }) | |
| }) | |
| it.concurrent('rejects invalidly encoded paths before forwarding to builder', async () => { | |
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%`, 'PATCH') | |
| const error = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | |
| .catch(err => err as HTTPException) | |
| expect(error).toBeInstanceOf(HTTPException) | |
| expect(error.cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path encoding.', | |
| }) | |
| }) | |
| it.concurrent('rejects path traversal attempts before forwarding to builder', async () => { | |
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%2e%2e/jobs`, 'PATCH') | |
| const result = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | |
| .catch(err => err) | |
| expect(result).toBeInstanceOf(HTTPException) | |
| if (result instanceof HTTPException) { | |
| expect(result.cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path', | |
| }) | |
| } | |
| }) | |
| it.concurrent('rejects invalidly encoded paths before forwarding to builder', async () => { | |
| const context = fakeContext(`http://localhost/build/upload/${jobId}/%`, 'PATCH') | |
| const result = await tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | |
| .catch(err => err) | |
| expect(result).toBeInstanceOf(HTTPException) | |
| if (result instanceof HTTPException) { | |
| expect(result.cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path encoding.', | |
| }) | |
| } | |
| }) |
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 92-92: TypeScript error TS2339: Property 'cause' does not exist on type 'Response | HTTPException'. Property 'cause' does not exist on type 'Response'.
[error] 105-105: TypeScript error TS2339: Property 'cause' does not exist on type 'Response | HTTPException'. Property 'cause' does not exist on type 'Response'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/build-upload-security.test.ts` around lines 85 - 109, The test
currently captures the error with .catch() causing TypeScript to infer a union
(Response | HTTPException) and preventing property access; change each test to
await tusProxy(...) inside a try/catch: call await tusProxy(context as any,
jobId, ...) in the try and force a test failure if it resolves, then in the
catch block assert expect(err).toBeInstanceOf(HTTPException), cast err to
HTTPException (e.g. const e = err as HTTPException) and then check e.cause with
expect(e.cause).toMatchObject(...); this uses tusProxy, HTTPException,
fakeContext and jobId to locate the code to update.
|
1 participant
Summary (AI generated)
/uploadnamespace.Motivation (AI generated)
Business Impact (AI generated)
Test Plan (AI generated)
bun lintbun lint:backendbunx eslint supabase/functions/_backend/public/build/upload.ts tests/build-upload-security.test.tsSummary by CodeRabbit
Bug Fixes
Tests