Skip to content

Fix build job ownership checks #1740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
riderx wants to merge 9 commits into from
Closed

Fix build job ownership checks #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
87b26f5
feat: make RBAC the default for newly created organizations
Dalanir Mar 3, 2026
cdf423a
fix: address review findings in RBAC migration and password-policy test
Dalanir Mar 3, 2026
bbceead
Merge branch 'main' into rbac-default-new-orgs
Dalanir Mar 3, 2026
8d38db8
fix(backend): bind build control actions to job owner
riderx Mar 3, 2026
17b8c9d
test(backend): assert build job ownership checks are side-effect free
riderx Mar 3, 2026
b84019c
Merge pull request #1739 from Cap-go/rbac-default-new-orgs
Dalanir Mar 3, 2026
5a3c30b
fix(backend): bind build control actions to job owner
riderx Mar 3, 2026
f52fe31
test(backend): assert build job ownership checks are side-effect free
riderx Mar 3, 2026
5103849
Merge branch 'riderx/fix-attached-issue' of https://github.com/Cap-go…
riderx Mar 3, 2026
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
1 change: 1 addition & 0 deletions bun.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 27 additions & 15 deletions supabase/functions/_backend/public/build/cancel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,12 @@ export async function cancelBuild(
appId: string,
apikey: Database['public']['Tables']['apikeys']['Row'],
): Promise<Response> {
if (!(await checkPermission(c, 'app.build_native', { appId }))) {
const errorMsg = 'You do not have permission to cancel builds for this app'
cloudlogErr({
requestId: c.get('requestId'),
message: 'Unauthorized cancel build',
job_id: jobId,
app_id: appId,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

// Bind jobId to its request owner before calling the builder.
const supabase = supabaseApikey(c, apikey.key)
const { data: buildRequest, error: buildRequestError } = await supabase
.from('build_requests')
.select('app_id')
.eq('builder_job_id', jobId)
.eq('app_id', appId)
.maybeSingle()

if (buildRequestError) {
Expand All @@ -55,11 +42,36 @@ export async function cancelBuild(
throw simpleError('unauthorized', errorMsg)
}

if (buildRequest.app_id !== appId) {
const errorMsg = 'You do not have permission to cancel builds for this app'
cloudlogErr({
requestId: c.get('requestId'),
message: 'Unauthorized cancel build (app mismatch)',
job_id: jobId,
requested_app_id: appId,
build_request_app_id: buildRequest.app_id,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

if (!(await checkPermission(c, 'app.build_native', { appId: buildRequest.app_id }))) {
const errorMsg = 'You do not have permission to cancel builds for this app'
cloudlogErr({
requestId: c.get('requestId'),
message: 'Unauthorized cancel build',
job_id: jobId,
app_id: buildRequest.app_id,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

cloudlog({
requestId: c.get('requestId'),
message: 'Cancel build request',
job_id: jobId,
app_id: appId,
app_id: buildRequest.app_id,
user_id: apikey.user_id,
})

Expand Down Expand Up @@ -101,7 +113,7 @@ export async function cancelBuild(
updated_at: new Date().toISOString(),
})
.eq('builder_job_id', jobId)
.eq('app_id', appId)
.eq('app_id', buildRequest.app_id)

if (updateError) {
cloudlogErr({
Expand Down
43 changes: 27 additions & 16 deletions supabase/functions/_backend/public/build/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,25 +99,10 @@ export async function startBuild(
// This prevents cross-app access by mixing an allowed app_id with another app's jobId.
const supabase = supabaseApikey(c, apikeyKey)

// Security: Check if user has permission to manage builds for the supplied app
// before validating builder job ownership.
if (!(await checkPermission(c, 'app.build_native', { appId }))) {
const errorMsg = 'You do not have permission to start builds for this app'
cloudlogErr({
requestId: c.get('requestId'),
message: 'Unauthorized start build',
job_id: jobId,
app_id: appId,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

const { data: buildRequest, error: buildRequestError } = await supabase
.from('build_requests')
.select('app_id')
.eq('builder_job_id', jobId)
.eq('app_id', appId)
.maybeSingle()
Comment on lines 102 to 106
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 3, 2026

Choose a reason for hiding this comment

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

P2 Badge Avoid assuming builder job IDs are globally unique

This lookup now uses maybeSingle() on builder_job_id alone, so if build_requests contains two rows with the same job ID (allowed today because there is no unique constraint on that column), Supabase returns a multi-row error and /build/start fails with internal_error before authorization logic can run; the same pattern was introduced in /build/cancel. This turns a recoverable duplicate-data condition into a hard failure for legitimate users, so the query should either include app_id disambiguation or handle multiple matches explicitly before deciding authorization.

Useful? React with 👍 / 👎.


if (buildRequestError) {
Expand All @@ -142,7 +127,33 @@ export async function startBuild(
throw simpleError('unauthorized', errorMsg)
}

const boundAppId = appId
if (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 (app mismatch)',
job_id: jobId,
requested_app_id: appId,
build_request_app_id: buildRequest.app_id,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

// Security: Check if user has permission to manage builds for the requested build request app
if (!(await checkPermission(c, 'app.build_native', { appId: buildRequest.app_id }))) {
const errorMsg = 'You do not have permission to start builds for this app'
cloudlogErr({
requestId: c.get('requestId'),
message: 'Unauthorized start build',
job_id: jobId,
app_id: buildRequest.app_id,
user_id: apikey.user_id,
})
throw simpleError('unauthorized', errorMsg)
}

const boundAppId = buildRequest.app_id

cloudlog({
requestId: c.get('requestId'),
Expand Down
3 changes: 3 additions & 0 deletions supabase/migrations/20260302000000_rbac_default_for_new_orgs.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
-- Make RBAC the default for all newly created organizations.
-- Existing orgs are not affected (their current use_new_rbac value is preserved).
ALTER TABLE public.orgs ALTER COLUMN use_new_rbac SET DEFAULT true;
Loading
Loading