[codex] Fix build log disconnect cancellation authorization#1954
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 56 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe stream proxy for building logs now conditionally registers an abort listener only after verifying the caller has Changes
Sequence DiagramsequenceDiagram
participant Client
participant StreamProxy as Stream Proxy
participant PermCheck as Permission Checker
participant BuildService as Builder Service
Client->>StreamProxy: Connect & Request Logs
StreamProxy->>PermCheck: Check app.read_logs
PermCheck-->>StreamProxy: Permission OK
StreamProxy->>BuildService: Fetch Logs
BuildService-->>StreamProxy: Log Stream
StreamProxy-->>Client: Stream Response (text/event-stream)
alt Caller has app.build_native
StreamProxy->>PermCheck: Check app.build_native
PermCheck-->>StreamProxy: Permission OK
StreamProxy->>StreamProxy: Register abort listener
Client->>StreamProxy: Disconnect
StreamProxy->>BuildService: POST /cancel {app_id}
BuildService-->>StreamProxy: Cancel OK
else Caller lacks app.build_native
StreamProxy->>PermCheck: Check app.build_native
PermCheck-->>StreamProxy: Permission Denied
StreamProxy->>StreamProxy: Skip abort listener
Client->>StreamProxy: Disconnect
Note over StreamProxy: No cancel triggered
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 543919bc7e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/logs.ts`:
- Around line 164-169: The optional RBAC call checkPermission(c,
'app.build_native', { appId: buildRequest.app_id }) must not be allowed to throw
and block log streaming; wrap the permission check in a try/catch (or otherwise
handle its promise) and only add the c.req.raw.signal.addEventListener that
calls cancelBuildOnDisconnect(builderUrl, builderApiKey, jobId,
buildRequest.app_id, requestId) if the check resolves to true, while on error
treat it as a no-permission case (optionally log a warning) and continue serving
logs so a transient RBAC/store error does not prevent stream availability.
In `@tests/build-logs-disconnect-auth.test.ts`:
- Around line 121-123: Replace the timing-dependent post-abort setTimeout(0)
waits after controller.abort() in the tests with deterministic synchronization:
for the read-only test remove the wait entirely (no await needed after
controller.abort()), and for the authorized test replace the setTimeout with
awaiting an explicit "cancel request observed" promise exposed by the fetch mock
(have the fetch mock resolve a named promise/Deferred when it sees the
abort/cancel and await that promise after calling controller.abort()). Update
both occurrences (around the controller.abort() call and the similar block
later) to use those deterministic waits so CI won't flake.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f91a82a4-dd1d-4f9d-b99f-77a24ef2524f
📒 Files selected for processing (2)
supabase/functions/_backend/public/build/logs.tstests/build-logs-disconnect-auth.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
1 participant
Summary (AI generated)
app.build_nativebefore wiring the/build/logs/:jobIddisconnect cancellation pathMotivation (AI generated)
The security advisory GHSA-95g7-xwwx-j737 identified a privilege inversion where a read-only principal could cancel a native build indirectly through the deprecated log-stream endpoint. This change aligns the disconnect side effect with the explicit cancel endpoint authorization boundary.
Business Impact (AI generated)
This closes a native-build disruption path that could let read-only API keys interfere with customer CI/CD workflows. Tightening that boundary protects trust in Capgos RBAC model and reduces operational support risk.
Test Plan (AI generated)
bunx vitest run tests/build-logs-disconnect-auth.test.ts --reporter=verbosebunx vitest run tests/build-job-scope.test.ts --reporter=verbosebun lintbun lint:backendbunx eslint tests/build-logs-disconnect-auth.test.tsbun typecheckbun test:backendGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests