Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR adds a pre-check that fetches build_request by builder_job_id across build endpoints (start, cancel, logs, status), validates the returned app_id, enforces authorization against that app_id, and scopes subsequent DB updates to the validated app_id for RLS compliance. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BuildEndpoint as Build Endpoint
participant Supabase as Supabase RLS
participant Builder as Builder Service
Client->>BuildEndpoint: Request (builder_job_id, appId, API key)
BuildEndpoint->>Supabase: Query build_requests by builder_job_id
Supabase-->>BuildEndpoint: build_request { app_id, owner_org, platform }
alt no build_request or app_id mismatch
BuildEndpoint-->>Client: 401 Unauthorized / error
else app_id matches
BuildEndpoint->>Supabase: Permission check (app.read_*/app.write) using build_request.app_id
alt permission denied
Supabase-->>BuildEndpoint: Deny
BuildEndpoint-->>Client: 401 Unauthorized / error
else permission granted
BuildEndpoint->>Builder: Forward operation (start/logs/cancel/status) with validated app_id
Builder-->>BuildEndpoint: Response
BuildEndpoint->>Supabase: Update build_requests (filter eq('app_id', build_request.app_id))
Supabase-->>BuildEndpoint: Update confirmed
BuildEndpoint-->>Client: Success / stream / status
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_backend/public/build/status.ts (1)
126-140:⚠️ Potential issue | 🟡 MinorLogging uses
platform(user-supplied) butrecordBuildTimeusesresolvedPlatform.Line 132 logs the user-supplied
platformin the "Recording build time" message, while Line 140 correctly passesresolvedPlatformtorecordBuildTime. This inconsistency could be confusing when debugging build-time discrepancies.🔧 Fix: log the actual platform being recorded
cloudlog({ requestId: c.get('requestId'), message: 'Recording build time', job_id, org_id, build_time_seconds: buildTimeSeconds, - platform, + platform: resolvedPlatform, })supabase/functions/_backend/public/build/start.ts (1)
41-56:⚠️ Potential issue | 🟠 Major
markBuildAsFaileddoes not scope the update byapp_id, unlike other update paths.All other
build_requestsupdates in this PR (status.ts Line 109, cancel.ts Line 96, start.ts Line 197) include.eq('app_id', buildRequest.app_id)for defense-in-depth. This helper omits it, relying solely onbuilder_job_id+ RLS.While RLS provides a safety net, the inconsistency means a caller within the same org who provides a valid
jobIdbut wrongappIdwould fail the app_id check at Line 125, thenmarkBuildAsFailedcould succeed in marking the build as failed — effectively allowing an org member to fail another app's build by providing mismatched parameters.Consider accepting
appIdas a parameter and adding it as a filter, or skippingmarkBuildAsFailedwhen the failure reason is an app_id mismatch (since the user may not own the build at all).🔧 Option A: add app_id parameter and filter
async function markBuildAsFailed( c: Context, jobId: string, errorMessage: string, apikeyKey: string, + appId?: string, ): Promise<void> { // Use authenticated client - RLS will enforce access const supabase = supabaseApikey(c, apikeyKey) - const { error: updateError } = await supabase + let query = supabase .from('build_requests') .update({ status: 'failed', last_error: errorMessage, updated_at: new Date().toISOString(), }) .eq('builder_job_id', jobId) + if (appId) { + query = query.eq('app_id', appId) + } + const { error: updateError } = await queryThen at Line 134, skip the call or pass
buildRequest?.app_id:- await markBuildAsFailed(c, jobId, errorMsg, apikeyKey) + // Don't mark as failed when the mismatch means we may not own this build
🤖 Fix all issues with AI agents
In `@tests/build-job-scope.test.ts`:
- Around line 2-3: Reorder the named imports so getSupabaseClient appears before
USER_ID in the import list (adjust the import statement containing BASE_URL,
ORG_ID, USER_ID, getSupabaseClient, resetAndSeedAppData, resetAppData) to
satisfy perfectionist/sort-named-imports; then update the test title strings
passed to describe and it (the descriptions in the tests using describe and it
in this file) so their first character is lowercase (fix the five title
occurrences flagged) to satisfy test/prefer-lowercase-title.
- Around line 90-107: Replace direct URL construction using BASE_URL in this
test with the test helper getEndpointUrl to ensure requests route through the
Cloudflare Worker; specifically change the instantiation new
URL(`${BASE_URL}/build/status`) to call getEndpointUrl('/build/status')
(imported from tests/test-utils.ts) and use the returned string/URL when
building the request for the GET /build/status test that references jobIdB and
appA (and apply the same replacement for other occurrences in this file that
construct endpoints with BASE_URL such as the tests around lines referencing
jobIdB at 110, 127, 142).
tests/build-job-scope.test.ts
Outdated
| import { afterAll, beforeAll, describe, expect, it } from 'vitest' | ||
| import { BASE_URL, ORG_ID, USER_ID, getSupabaseClient, resetAndSeedAppData, resetAppData } from './test-utils.ts' |
There was a problem hiding this comment.
ESLint errors: fix import order and lowercase test titles.
Static analysis flags two categories of errors that need fixing before merge (per coding guidelines, "Backend ESLint must pass before commit"):
- Line 3:
getSupabaseClientshould come beforeUSER_IDin the named imports (perfectionist/sort-named-imports). - Lines 5, 90, 109, 126, 141:
describeandittitles must begin with a lowercase letter (test/prefer-lowercase-title).
🔧 Proposed fixes
-import { BASE_URL, ORG_ID, USER_ID, getSupabaseClient, resetAndSeedAppData, resetAppData } from './test-utils.ts'
+import { BASE_URL, getSupabaseClient, ORG_ID, resetAndSeedAppData, resetAppData, USER_ID } from './test-utils.ts'-describe('Build Endpoints Job/App Binding', () => {
+describe('build endpoints job/app binding', () => {- it.concurrent('GET /build/status denies cross-app job_id with allowed app_id', async () => {
+ it.concurrent('gET /build/status denies cross-app job_id with allowed app_id', async () => {Actually, the lowercase rule applies to the first character of the description string:
- it.concurrent('GET /build/status denies cross-app job_id with allowed app_id', async () => {
+ it.concurrent('get /build/status denies cross-app job_id with allowed app_id', async () => {
- it.concurrent('GET /build/logs/:jobId denies cross-app jobId with allowed app_id', async () => {
+ it.concurrent('get /build/logs/:jobId denies cross-app jobId with allowed app_id', async () => {
- it.concurrent('POST /build/cancel/:jobId denies cross-app jobId with allowed app_id', async () => {
+ it.concurrent('post /build/cancel/:jobId denies cross-app jobId with allowed app_id', async () => {
- it.concurrent('POST /build/start/:jobId denies cross-app jobId with allowed app_id', async () => {
+ it.concurrent('post /build/start/:jobId denies cross-app jobId with allowed app_id', async () => {As per coding guidelines, "Backend ESLint must pass before commit; run bun lint:backend for backend files."
📝 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.
| import { afterAll, beforeAll, describe, expect, it } from 'vitest' | |
| import { BASE_URL, ORG_ID, USER_ID, getSupabaseClient, resetAndSeedAppData, resetAppData } from './test-utils.ts' | |
| import { afterAll, beforeAll, describe, expect, it } from 'vitest' | |
| import { BASE_URL, getSupabaseClient, ORG_ID, resetAndSeedAppData, resetAppData, USER_ID } from './test-utils.ts' |
🧰 Tools
🪛 ESLint
[error] 3-3: Expected "getSupabaseClient" to come before "USER_ID".
(perfectionist/sort-named-imports)
🤖 Prompt for AI Agents
In `@tests/build-job-scope.test.ts` around lines 2 - 3, Reorder the named imports
so getSupabaseClient appears before USER_ID in the import list (adjust the
import statement containing BASE_URL, ORG_ID, USER_ID, getSupabaseClient,
resetAndSeedAppData, resetAppData) to satisfy perfectionist/sort-named-imports;
then update the test title strings passed to describe and it (the descriptions
in the tests using describe and it in this file) so their first character is
lowercase (fix the five title occurrences flagged) to satisfy
test/prefer-lowercase-title.
| it.concurrent('GET /build/status denies cross-app job_id with allowed app_id', async () => { | ||
| const url = new URL(`${BASE_URL}/build/status`) | ||
| url.searchParams.set('job_id', jobIdB) | ||
| url.searchParams.set('app_id', appA) | ||
| url.searchParams.set('platform', 'android') | ||
|
|
||
| const response = await fetch(url.toString(), { | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: readKey, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }) | ||
|
|
||
| expect(response.status).toBe(400) | ||
| const data = await response.json() as { error: string } | ||
| expect(data).toHaveProperty('error', 'unauthorized') | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getEndpointUrl exists in test-utils and how it's used by other tests
rg -n 'getEndpointUrl' tests/test-utils.ts
echo "---"
rg -n 'getEndpointUrl' tests/ --glob '*.test.ts' -C2 | head -80Repository: Cap-go/capgo
Length of output: 5167
🏁 Script executed:
#!/bin/bash
# Check the build-job-scope.test.ts file to see what BASE_URL is and how it's currently used
rg -n 'BASE_URL\|/build' tests/build-job-scope.test.ts | head -40
echo "---"
# Check if /build endpoints are handled by any worker
rg -n '/build' cloudflare_workers/ -t ts | head -30Repository: Cap-go/capgo
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check BASE_URL definition in build-job-scope.test.ts
rg -n 'BASE_URL' tests/build-job-scope.test.ts | head -20
echo "---"
# Check how getEndpointUrl is defined in test-utils
sed -n '25,35p' tests/test-utils.tsRepository: Cap-go/capgo
Length of output: 920
🏁 Script executed:
#!/bin/bash
# Get the full definition of getEndpointUrl function
sed -n '29,60p' tests/test-utils.ts
echo "---"
# Check BASE_URL definition
rg -n '^export.*BASE_URL|^const BASE_URL' tests/test-utils.ts -A2Repository: Cap-go/capgo
Length of output: 2477
Use getEndpointUrl() for correct worker routing instead of constructing URLs with BASE_URL.
The coding guidelines mandate using getEndpointUrl() from tests/test-utils.ts for backend tests to ensure correct Cloudflare Workers routing. Replace new URL(\${BASE_URL}/build/status`)withgetEndpointUrl('/build/status'). This also applies to other endpoint constructions in this file that use BASE_URL` directly (e.g., lines 110, 127, 142).
🧰 Tools
🪛 ESLint
[error] 90-90: its should begin with lowercase
(test/prefer-lowercase-title)
🤖 Prompt for AI Agents
In `@tests/build-job-scope.test.ts` around lines 90 - 107, Replace direct URL
construction using BASE_URL in this test with the test helper getEndpointUrl to
ensure requests route through the Cloudflare Worker; specifically change the
instantiation new URL(`${BASE_URL}/build/status`) to call
getEndpointUrl('/build/status') (imported from tests/test-utils.ts) and use the
returned string/URL when building the request for the GET /build/status test
that references jobIdB and appA (and apply the same replacement for other
occurrences in this file that construct endpoints with BASE_URL such as the
tests around lines referencing jobIdB at 110, 127, 142).
There was a problem hiding this comment.
Pull request overview
This PR tightens native build endpoint authorization by binding job_id/jobId to its owning app_id via build_requests lookups under RLS, preventing cross-app access when a caller mixes an allowed app_id with another app’s builder job ID.
Changes:
- Add
build_requestslookups to bindbuilder_job_id→app_idbefore calling the builder in build status/start/logs/cancel endpoints. - Scope
build_requestsstatus updates with bothbuilder_job_idandapp_id. - Add a Vitest regression test ensuring cross-app job access is denied for status/logs/cancel/start endpoints.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/build-job-scope.test.ts | Adds an integration test validating cross-app job_id access is rejected even when the caller has access to the provided app_id. |
| supabase/functions/_backend/public/build/status.ts | Resolves the job’s owning app/org via build_requests under RLS before fetching builder status; scopes status update by app_id. |
| supabase/functions/_backend/public/build/start.ts | Verifies jobId belongs to the requested appId under RLS before starting the build; scopes running-status update by app_id. |
| supabase/functions/_backend/public/build/logs.ts | Verifies jobId belongs to appId under RLS before proxying logs; uses the resolved app for disconnect cancellation. |
| supabase/functions/_backend/public/build/cancel.ts | Verifies jobId belongs to appId under RLS before cancelling; scopes cancelled-status update by app_id. |
| const { data: buildRequest, error: buildRequestError } = await supabase | ||
| .from('build_requests') | ||
| .select('app_id') | ||
| .eq('builder_job_id', jobId) |
There was a problem hiding this comment.
The build_request lookup is only filtered by builder_job_id. To make the authorization check deterministic and avoid ambiguity if job IDs collide, also filter by the provided appId in the query.
| .eq('builder_job_id', jobId) | |
| .eq('builder_job_id', jobId) | |
| .eq('app_id', appId) |
| org_id, | ||
| apikey.user_id, | ||
| job_id, | ||
| platform, | ||
| resolvedPlatform, | ||
| buildTimeSeconds, | ||
| ) | ||
| } |
There was a problem hiding this comment.
recordBuildTime() now uses resolvedPlatform derived from the build_request row. Ensure any related telemetry/debug logs in this code path also use the resolved platform (not the request param), otherwise logs can become misleading when investigating billing discrepancies.
| const { data: buildRequest, error: buildRequestError } = await supabase | ||
| .from('build_requests') | ||
| .select('app_id, owner_org, platform') | ||
| .eq('builder_job_id', job_id) |
There was a problem hiding this comment.
The build_request lookup is only filtered by builder_job_id and then uses maybeSingle(). Since builder_job_id is not guaranteed unique at the DB level, this can return a “multiple rows” error and incorrectly surface as internal_error. Consider also filtering by app_id (and/or enforcing uniqueness) so the lookup is deterministic and doesn’t depend on maybeSingle() behavior.
| .eq('builder_job_id', job_id) | |
| .eq('builder_job_id', job_id) | |
| .eq('app_id', app_id) |
| // Bind jobId to appId under RLS before calling the builder. | ||
| // This prevents cross-app access by mixing an allowed app_id with another app's jobId. | ||
| const supabase = supabaseApikey(c, apikeyKey) | ||
| const { data: buildRequest, error: buildRequestError } = await supabase | ||
| .from('build_requests') | ||
| .select('app_id') | ||
| .eq('builder_job_id', jobId) | ||
| .maybeSingle() |
There was a problem hiding this comment.
The build_request lookup is only filtered by builder_job_id. For tighter job/app binding and to avoid ambiguity if job IDs ever collide, also filter by the caller-provided appId (so you only ever fetch the row you intend to authorize).
| if (!buildRequest || buildRequest.app_id !== appId) { | ||
| const errorMsg = 'You do not have permission to start builds for this app' | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Unauthorized start build (job/app mismatch or not accessible)', | ||
| job_id: jobId, | ||
| app_id: appId, | ||
| user_id: apikey.user_id, | ||
| }) | ||
| await markBuildAsFailed(c, jobId, errorMsg, apikeyKey) | ||
| alreadyMarkedAsFailed = true | ||
| throw simpleError('unauthorized', errorMsg) |
There was a problem hiding this comment.
On job/app mismatch you call markBuildAsFailed() using only jobId. markBuildAsFailed() updates build_requests by builder_job_id alone, so if job IDs are not globally unique this could update multiple rows (or the wrong app’s row). Consider scoping the failure update by app_id as well (e.g., pass buildRequest.app_id/appId into markBuildAsFailed and include it in the update filter).
| const { data: buildRequest, error: buildRequestError } = await supabase | ||
| .from('build_requests') | ||
| .select('app_id') | ||
| .eq('builder_job_id', jobId) |
There was a problem hiding this comment.
The build_request lookup is only filtered by builder_job_id. For strict job/app binding (and to avoid errors if job IDs collide), also filter by appId in the query so you only authorize against the intended row.
| .eq('builder_job_id', jobId) | |
| .eq('builder_job_id', jobId) | |
| .eq('app_id', appId) |
|
|
/tip @Judel777 $100 |
|
🎉🎈 @Judel777 has been awarded $100 by Capgo! 🎈🎊 |
Summary (AI generated)
build_requests(RLS) and authorize against the resolved app before calling the builder.Motivation (AI generated)
job_idwith an allowedapp_id.Business Impact (AI generated)
Test plan (AI generated)
bun lint:backendbun lintbunx vitest run tests/build-job-scope.test.tsScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lint.Summary by CodeRabbit