Harden build upload proxy against path traversal#1759
Conversation
📝 WalkthroughWalkthroughThe pull request hardens the TUS upload proxy by reworking path extraction and validation logic. Changes include strict URL decoding of paths, validation against null bytes and directory traversal attempts, and verification that builder upload paths remain within the allowed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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: 918ae7a6d2
ℹ️ 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".
| const safeTusPath = decodedTusPath | ||
|
|
||
| // Construct builder TUS URL with the path | ||
| const builderTusUrl = `${builderUrl}/upload${tusPath}` | ||
| const builderTusUrl = `${builderUrl}/upload${safeTusPath}` |
There was a problem hiding this comment.
decodedTusPath is now used to build the upstream URL, so any percent-encoded reserved characters in the client URL (for example %2F, %3F, or %25) are decoded before proxying and can change which upload resource the builder sees. This breaks resumable uploads whenever the builder returns an encoded Location path (clients must replay that URL verbatim in TUS), because subsequent HEAD/PATCH requests may target a different path or become malformed. Validate on a decoded copy if needed, but forward the original encoded path segment.
Useful? React with 👍 / 👎.
riderx
force-pushed
the
riderx/fix-attachment-issue
branch
from
918ae7a to
825d80f
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/public/build/upload.ts`:
- Around line 127-170: The code currently decodes the entire tusPath and
rebuilds builderTusUrl from decodedTusPath (via builderTusUrl =
`${builderUrl}/upload${safeTusPath}`), which can change resource identity and
mishandle encoded segments; also the check decodedTusPath.includes('..') is too
broad. Fix by validating segments instead of substring checks: split
decodedTusPath on '/' and reject if any segment is exactly '..' or contains '\0'
(e.g., segments.some(s => s === '..' || s.includes('\0'))). After validation, do
not reconstruct the path from decodedTusPath—use the already-resolved and
validated resolvedTusUrl.pathname when building the forwarded URL (set
builderTusUrl using resolvedTusUrl.pathname) and remove the unsafe
string-concatenation with decodedTusPath.
In `@tests/build-upload-security.test.ts`:
- Around line 21-66: The test suite uses a shared mutable mock object
`queryBuilder` and synchronous `it()` cases which breaks parallel safety; move
creation of `queryBuilder` (with its `select`, `eq`, and `single` mocks) into a
`beforeEach()` so each test gets a fresh instance, update any per-test setup
that references `queryBuilder` (and keep `afterEach()` clearing mocks as
needed), and convert all test cases that currently use `it()` to
`it.concurrent()` (also adjust any expectations that rely on mutation order).
Ensure helper `fakeContext` remains stateless and continue using
`queryBuilder.select/eq/single` on the per-test mock.
- Around line 83-91: The test should use a percent-encoded traversal payload and
capture the thrown error object instead of relying on the return value of
expect(...).rejects; update the request URL in
tests/build-upload-security.test.ts to use '%2e%2e' (e.g.
`http://localhost/build/upload/${jobId}/%2e%2e/jobs`) so URL normalization
doesn't strip the traversal, then replace the rejects pattern with explicit
error capture: `const error = await responsePromise.catch(e => e)` followed by
`expect(error).toBeInstanceOf(HTTPException)` and
`expect(error.cause).toMatchObject({ error: 'invalid_path', message: 'Invalid
upload path' })`; this will exercise the upload.ts check that looks at
decodedTusPath.includes('..').
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9683b8a9-ed60-490a-ae7a-220a038c9758
📒 Files selected for processing (2)
supabase/functions/_backend/public/build/upload.tstests/build-upload-security.test.ts
| let decodedTusPath: string | ||
| try { | ||
| decodedTusPath = decodeURIComponent(tusPath) | ||
| } | ||
| catch { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Invalid URL encoding in upload path', | ||
| job_id: jobId, | ||
| original_path: originalPath, | ||
| upload_path: tusPath, | ||
| }) | ||
| throw quickError(400, 'invalid_path', 'Invalid upload path encoding.') | ||
| } | ||
|
|
||
| if (decodedTusPath.includes('\0') || decodedTusPath.includes('..')) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Rejected upload path traversal attempt', | ||
| job_id: jobId, | ||
| original_path: originalPath, | ||
| upload_path: decodedTusPath, | ||
| }) | ||
| throw quickError(400, 'invalid_path', 'Invalid upload path') | ||
| } | ||
|
|
||
| const baseUploadUrl = new URL(`${builderUrl}/upload`) | ||
| const resolvedTusUrl = new URL(`${builderUrl}/upload${decodedTusPath}`) | ||
| if (!resolvedTusUrl.pathname.startsWith(baseUploadUrl.pathname)) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Rejected upload path escape attempt', | ||
| job_id: jobId, | ||
| original_path: originalPath, | ||
| resolved_path: resolvedTusUrl.pathname, | ||
| builder_path: baseUploadUrl.pathname, | ||
| }) | ||
| throw quickError(400, 'invalid_path', 'Resolved upload path escapes upload directory.') | ||
| } | ||
|
|
||
| const safeTusPath = decodedTusPath | ||
|
|
||
| // Construct builder TUS URL with the path | ||
| const builderTusUrl = `${builderUrl}/upload${tusPath}` | ||
| const builderTusUrl = `${builderUrl}/upload${safeTusPath}` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n supabase/functions/_backend/public/build/upload.ts | head -200Repository: Cap-go/capgo
Length of output: 8435
🏁 Script executed:
cat -n supabase/functions/_backend/public/build/upload.ts | sed -n '200,250p'Repository: Cap-go/capgo
Length of output: 1784
🏁 Script executed:
cat -n supabase/functions/_backend/public/build/upload.ts | sed -n '200,300p'Repository: Cap-go/capgo
Length of output: 3615
🏁 Script executed:
cat -n supabase/functions/_backend/public/build/upload.ts | sed -n '116,180p'Repository: Cap-go/capgo
Length of output: 2661
🏁 Script executed:
# Check if there are any tests for this function to understand intended behavior
find supabase -name "*upload*test*" -o -name "*test*upload*" | head -20Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Verify the URL behavior with a concrete example
node <<'EOF'
// Simulate what happens with an encoded path segment
const builderUrl = "https://builder.example.com";
const jobId = "abc123";
const clientPath = "/build/upload/abc123/uploads%2Fphotos.zip";
// Extract tusPath (encoded)
const uploadPrefix = `/build/upload/${jobId}`;
const tusPath = clientPath.startsWith(`${uploadPrefix}/`)
? clientPath.slice(uploadPrefix.length)
: '/';
console.log("tusPath (encoded):", tusPath);
// Decode it (current code does this)
const decodedTusPath = decodeURIComponent(tusPath);
console.log("decodedTusPath (decoded):", decodedTusPath);
// What current code forwards (Line 170)
const currentForwardedUrl = `${builderUrl}/upload${decodedTusPath}`;
console.log("Current forwarded URL:", currentForwardedUrl);
// What should be forwarded (keep encoded)
const correctForwardedUrl = `${builderUrl}/upload${tusPath}`;
console.log("Correct forwarded URL:", correctForwardedUrl);
console.log("\nThey are different:", currentForwardedUrl !== correctForwardedUrl);
console.log("Impact: '/uploads%2Fphotos.zip' becomes '/uploads/photos.zip'");
console.log("This changes the resource identity on the builder side.");
// Test the substring check issue
const testPaths = ["foo..bar", "foo/..bar", "foo/../bar", "path/to/file.zip"];
console.log("\n--- Line 142 validation issue ---");
testPaths.forEach(path => {
console.log(`"${path}".includes('..'): ${path.includes('..')}`);
});
console.log("\n--- Proper segment-based check ---");
testPaths.forEach(path => {
const hasDotSegment = path.split('/').some(seg => seg === '.' || seg === '..');
console.log(`"${path}" has . or .. segment: ${hasDotSegment}`);
});
EOFRepository: Cap-go/capgo
Length of output: 815
Don't proxy the decoded suffix; validate path segments properly.
Line 170 reconstructs the forwarded URL from decodedTusPath, which changes the resource identity. For example, /uploads%2Fphotos.zip (a single encoded segment) becomes /uploads/photos.zip (two segments), breaking backward compatibility for clients relying on encoded path preservation. The resolvedTusUrl object created at line 154 already contains the properly normalized path after validation; reuse its pathname instead of string concatenation from the decoded path.
Additionally, line 142's check decodedTusPath.includes('..') is overgeneralized—it rejects safe filenames like foo..bar that contain .. as a substring rather than as an actual path segment. Replace it with segment-based validation.
Proposed fix
- if (decodedTusPath.includes('\0') || decodedTusPath.includes('..')) {
+ const hasDotSegment = decodedTusPath.split('/').some(segment => segment === '.' || segment === '..')
+ if (decodedTusPath.includes('\0') || hasDotSegment) {- const baseUploadUrl = new URL(`${builderUrl}/upload`)
- const resolvedTusUrl = new URL(`${builderUrl}/upload${decodedTusPath}`)
+ const baseUploadUrl = new URL(`${builderUrl}/upload/`)
+ const resolvedTusUrl = new URL(`.${tusPath}`, baseUploadUrl)- const safeTusPath = decodedTusPath
- const builderTusUrl = `${builderUrl}/upload${safeTusPath}`
+ const safeTusPath = resolvedTusUrl.pathname.slice(baseUploadUrl.pathname.length - 1) || '/'
+ const builderTusUrl = resolvedTusUrl.toString()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/functions/_backend/public/build/upload.ts` around lines 127 - 170,
The code currently decodes the entire tusPath and rebuilds builderTusUrl from
decodedTusPath (via builderTusUrl = `${builderUrl}/upload${safeTusPath}`), which
can change resource identity and mishandle encoded segments; also the check
decodedTusPath.includes('..') is too broad. Fix by validating segments instead
of substring checks: split decodedTusPath on '/' and reject if any segment is
exactly '..' or contains '\0' (e.g., segments.some(s => s === '..' ||
s.includes('\0'))). After validation, do not reconstruct the path from
decodedTusPath—use the already-resolved and validated resolvedTusUrl.pathname
when building the forwarded URL (set builderTusUrl using
resolvedTusUrl.pathname) and remove the unsafe string-concatenation with
decodedTusPath.
| describe('build upload proxy security', () => { | ||
| const jobId = 'job-traversal-test' | ||
| const appId = 'com.test.traversal.app' | ||
| const orgId = 'org-traversal' | ||
| const validUploadPath = `orgs/${orgId}/apps/${appId}/native-builds/file.zip` | ||
| const buildRequestQuery = { | ||
| data: { | ||
| app_id: appId, | ||
| owner_org: orgId, | ||
| builder_job_id: jobId, | ||
| upload_path: validUploadPath, | ||
| }, | ||
| error: null, | ||
| } | ||
|
|
||
| const queryBuilder = { | ||
| select: vi.fn().mockReturnThis(), | ||
| eq: vi.fn().mockReturnThis(), | ||
| single: vi.fn().mockResolvedValue(buildRequestQuery), | ||
| } | ||
|
|
||
| const fakeContext = (url: string, method = 'POST') => { | ||
| const request = new Request(url, { | ||
| method, | ||
| headers: { | ||
| 'Tus-Resumable': '1.0.0', | ||
| }, | ||
| }) | ||
|
|
||
| return { | ||
| req: { | ||
| url: request.url, | ||
| method: request.method, | ||
| raw: request, | ||
| header: (name: string) => request.headers.get(name), | ||
| }, | ||
| get: vi.fn().mockReturnValue('test-request-id'), | ||
| } | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks() | ||
| queryBuilder.select.mockClear() | ||
| queryBuilder.eq.mockClear() | ||
| queryBuilder.single.mockClear() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the test file
fd -t f "build-upload-security.test.ts" | head -20Repository: Cap-go/capgo
Length of output: 91
🏁 Script executed:
# If found, read the test file to verify the shared mocks and test structure
cat -n tests/build-upload-security.test.ts | head -150Repository: Cap-go/capgo
Length of output: 5687
Make the suite parallel-safe by moving mutable test doubles into beforeEach and switching to concurrent cases.
The shared queryBuilder mock object and describe-scoped mutable fixtures prevent safe parallel execution. Tests at lines 68, 94, and 120 use it() instead of it.concurrent(), which is required by repo convention. Move the mutable queryBuilder and mock setup into beforeEach(), then convert all cases to it.concurrent().
Per coding guidelines: "Design all tests for parallel execution; use it.concurrent() instead of it() to maximize parallelism; create dedicated seed data for tests that modify shared resources to avoid conflicts".
🤖 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 21 - 66, The test suite
uses a shared mutable mock object `queryBuilder` and synchronous `it()` cases
which breaks parallel safety; move creation of `queryBuilder` (with its
`select`, `eq`, and `single` mocks) into a `beforeEach()` so each test gets a
fresh instance, update any per-test setup that references `queryBuilder` (and
keep `afterEach()` clearing mocks as needed), and convert all test cases that
currently use `it()` to `it.concurrent()` (also adjust any expectations that
rely on mutation order). Ensure helper `fakeContext` remains stateless and
continue using `queryBuilder.select/eq/single` on the per-test mock.
| const context = fakeContext(`http://localhost/build/upload/${jobId}/../jobs`, 'PATCH') | ||
|
|
||
| const responsePromise = tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | ||
| const error = await expect(responsePromise).rejects.toBeInstanceOf(HTTPException) | ||
|
|
||
| expect(error.cause).toMatchObject({ | ||
| error: 'invalid_path', | ||
| message: 'Invalid upload path', | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the test file to see the current state
wc -l tests/build-upload-security.test.tsRepository: Cap-go/capgo
Length of output: 95
🏁 Script executed:
# Read the relevant sections of the test file
sed -n '68,125p' tests/build-upload-security.test.tsRepository: Cap-go/capgo
Length of output: 2076
🏁 Script executed:
# Search for fakeContext implementation
rg -n "function fakeContext|const fakeContext|export.*fakeContext" tests/ -A 5Repository: Cap-go/capgo
Length of output: 460
🏁 Script executed:
# Search for tusProxy implementation
rg -n "function tusProxy|export.*tusProxy" -A 10Repository: Cap-go/capgo
Length of output: 925
🏁 Script executed:
# Read the tusProxy implementation to understand error handling
sed -n '14,80p' supabase/functions/_backend/public/build/upload.tsRepository: Cap-go/capgo
Length of output: 2254
🏁 Script executed:
# Check for HTTPException definition and how it's thrown
rg -n "class HTTPException|throw new HTTPException" supabase/functions/_backend/ -B 2 -A 3Repository: Cap-go/capgo
Length of output: 11502
🏁 Script executed:
# Check if the test file has type errors by looking at tsconfig
fd tsconfig.json -o catRepository: Cap-go/capgo
Length of output: 181
🏁 Script executed:
# Look at the imports in the test file to understand the Vitest setup
head -30 tests/build-upload-security.test.tsRepository: Cap-go/capgo
Length of output: 1017
🏁 Script executed:
# Continue reading tusProxy to find the path validation logic
sed -n '60,130p' supabase/functions/_backend/public/build/upload.tsRepository: Cap-go/capgo
Length of output: 2573
🏁 Script executed:
# Check how the path validation works
rg -n "upload_path|Invalid upload path" supabase/functions/_backend/public/build/upload.ts -B 3 -A 3Repository: Cap-go/capgo
Length of output: 3281
🌐 Web query:
Vitest expect().rejects return type and how to capture rejected error
💡 Result:
Return type of expect(promise).rejects (Vitest)
In Vitest, .rejects is typed as Promisify<Assertions>—i.e. it returns the usual Assertions object, but every matcher becomes async (returns a Promise), so you must await the assertion. The rejected reason is “unwrapped” and passed into the matcher. [1]
// roughly
expect(promise).rejects // Promisify<Assertions>
await expect(promise).rejects.toThrow('no id') // Promise<void>How to “capture” the rejected error value
rejects is meant for asserting, not extracting. If you need the actual error object (to inspect properties, narrow types, etc.), catch it yourself:
let err: unknown
try {
await doThing()
throw new Error('expected doThing() to reject')
} catch (e) {
err = e
}
expect(err).toBeInstanceOf(Error)
expect((err as Error).message).toContain('no id')Or as an expression:
const err = await doThing().catch(e => e)
expect(err).toBeInstanceOf(Error)If you only need to assert properties without extracting, you can keep using rejects with object/asymmetric matchers:
await expect(doThing()).rejects.toThrow(
expect.objectContaining({ message: 'no id' }),
)[1][2]
Sources: Vitest expect docs on .rejects typing/behavior and needing await [1]; Vitest discussion showing the practical try/catch approach when you need to inspect the thrown value [2].
[1] (vitest.dev)
[2] (github.com)
Citations:
- 1: https://vitest.dev/api/expect
- 2: How to check if toThrowError is throwing an error with specific properties? vitest-dev/vitest#4378
🏁 Script executed:
# Verify the second test case at lines 109-117
sed -n '109,120p' tests/build-upload-security.test.tsRepository: Cap-go/capgo
Length of output: 532
🏁 Script executed:
# Check if quickError is what throws HTTPException
rg -n "function quickError|const quickError|export.*quickError" supabase/functions/_backend/ -A 5Repository: Cap-go/capgo
Length of output: 588
Fix the rejection assertions and use an encoded traversal payload.
Lines 86–88 and 112–114 fail with Property 'cause' does not exist on type 'void' because await expect(promise).rejects.toBeInstanceOf() returns Promise<void>, not the error object. Additionally, the raw ../jobs input at lines 83 and 109 is normalized by new Request() before tusProxy() validates the path, which means the traversal guard at line 142 of upload.ts (decodedTusPath.includes('..')) never sees the .. sequence. Use %2e%2e (percent-encoded dots) so the path traversal survives URL normalization and is correctly decoded and caught during validation.
Proposed fix
- const context = fakeContext(`http://localhost/build/upload/${jobId}/../jobs`, 'PATCH')
-
- const responsePromise = tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
- const error = await expect(responsePromise).rejects.toBeInstanceOf(HTTPException)
-
- expect(error.cause).toMatchObject({
+ 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 as HTTPException).cause).toMatchObject({
error: 'invalid_path',
message: 'Invalid upload path',
})- const responsePromise = tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any)
- const error = await expect(responsePromise).rejects.toBeInstanceOf(HTTPException)
-
- expect(error.cause).toMatchObject({
+ 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 as HTTPException).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.
| const context = fakeContext(`http://localhost/build/upload/${jobId}/../jobs`, 'PATCH') | |
| const responsePromise = tusProxy(context as any, jobId, { user_id: 'user-test', key: 'api-test' } as any) | |
| const error = await expect(responsePromise).rejects.toBeInstanceOf(HTTPException) | |
| expect(error.cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path', | |
| }) | |
| 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 as HTTPException).cause).toMatchObject({ | |
| error: 'invalid_path', | |
| message: 'Invalid upload path', | |
| }) |
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 88-88: TypeScript error: Property 'cause' does not exist on type 'void' (vue-tsc --noEmit).
🤖 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 83 - 91, The test should
use a percent-encoded traversal payload and capture the thrown error object
instead of relying on the return value of expect(...).rejects; update the
request URL in tests/build-upload-security.test.ts to use '%2e%2e' (e.g.
`http://localhost/build/upload/${jobId}/%2e%2e/jobs`) so URL normalization
doesn't strip the traversal, then replace the rejects pattern with explicit
error capture: `const error = await responsePromise.catch(e => e)` followed by
`expect(error).toBeInstanceOf(HTTPException)` and
`expect(error.cause).toMatchObject({ error: 'invalid_path', message: 'Invalid
upload path' })`; this will exercise the upload.ts check that looks at
decodedTusPath.includes('..').
1 participant
Summary (AI generated)
Motivation (AI generated)
Business Impact (AI generated)
Test Plan (AI generated)
Test plan
Screenshots
Checklist
bun run lint:backend && bun run lint.accordingly.
my tests
Summary by CodeRabbit
Bug Fixes
Tests