Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR enforces API-key allow-lists for app/org scope checks and makes the statistics endpoint return 401 (no_access_to_app) for apikey-authenticated requests that target apps outside the key's scope instead of 404. Tests were added to verify app-limited subkey behavior. ChangesAPI Key Scope Enforcement
Sequence Diagram(s)sequenceDiagram
participant Client
participant StatsFn as Statistics Function
participant RBAC as RBAC Utils
participant DB as Database
Client->>StatsFn: GET /statistics/app/:appId (apikey headers)
StatsFn->>RBAC: resolveAppOwnerOrg(appId)
RBAC->>DB: query app owner/org
DB-->>RBAC: app not found
RBAC-->>StatsFn: notFound
StatsFn->>RBAC: deniesExplicitApiKeyScope(scope)
RBAC-->>StatsFn: denies (limited_to_apps/orgs mismatch)
StatsFn-->>Client: 401 no_access_to_app
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 59 minutes and 31 seconds. Comment |
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/statistics.test.ts (1)
169-201: ⚡ Quick winMake this regression test self-contained (remove cross-test state dependency)
At Line 170, this test relies on
subkeyIdbeing created by a previous test, which makes it order-dependent and not parallel-safe. Create a dedicated limited subkey inside this test and clean it up infinally(then it can beit.concurrent).Proposed refactor sketch
- it('should not reveal sibling app existence outside an app-limited subkey', async () => { - expect(subkeyId).not.toBe(0) + it.concurrent('should not reveal sibling app existence outside an app-limited subkey', async () => { + const createSubkey = await fetch(`${BASE_URL}/apikey`, { + method: 'POST', + headers: headersStats, + body: JSON.stringify({ + name: 'Limited Stats Subkey - oracle test', + mode: 'read', + limited_to_apps: [APPNAME], + }), + }) + expect(createSubkey.status).toBe(200) + const localSubkey = await createSubkey.json() as { id: number } + const siblingApp = `com.stats.oracle.${randomUUID().replaceAll('-', '')}` const fakeApp = `com.stats.fake.${randomUUID().replaceAll('-', '')}` await createStatsSiblingApp(siblingApp) try { - const subkeyHeaders = { 'x-limited-key-id': String(subkeyId) } + const subkeyHeaders = { 'x-limited-key-id': String(localSubkey.id) } ... } finally { await deleteAppByAppId(siblingApp) + await deleteApikeyById(localSubkey.id) } })As per coding guidelines
tests/**/*.test.ts: “Design all tests for parallel execution across files; use it.concurrent() instead of it() to maximize parallelism within test files; create dedicated seed data for tests that modify resources or when resource state matters for assertions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/statistics.test.ts` around lines 169 - 201, The test depends on external shared state (subkeyId) so make it self-contained: create a dedicated limited subkey at the start of this test (e.g., call the same helper used elsewhere to create subkeys and capture its id into a local variable rather than using the global subkeyId), use that local id to build subkeyHeaders and the merged headers (replace use of subkeyId and headersStats with the local variables), mark the test it.concurrent(...) and ensure you delete the created subkey in the finally block along with the existing deleteAppByAppId(siblingApp) cleanup; keep the rest of the flow (createStatsSiblingApp, fetch to BASE_URL/statistics/app/..., assertions) unchanged so the test is independent and parallel-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/statistics.test.ts`:
- Around line 169-201: The test depends on external shared state (subkeyId) so
make it self-contained: create a dedicated limited subkey at the start of this
test (e.g., call the same helper used elsewhere to create subkeys and capture
its id into a local variable rather than using the global subkeyId), use that
local id to build subkeyHeaders and the merged headers (replace use of subkeyId
and headersStats with the local variables), mark the test it.concurrent(...) and
ensure you delete the created subkey in the finally block along with the
existing deleteAppByAppId(siblingApp) cleanup; keep the rest of the flow
(createStatsSiblingApp, fetch to BASE_URL/statistics/app/..., assertions)
unchanged so the test is independent and parallel-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ef14254-981c-4803-86b3-e0001f6f0c96
📒 Files selected for processing (3)
supabase/functions/_backend/public/statistics/index.tssupabase/functions/_backend/utils/rbac.tstests/statistics.test.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
401 no_access_to_app.Motivation (AI generated)
GHSA-73p9-mprg-7r75 reported that app-limited API keys could distinguish real sibling app IDs from nonexistent app IDs through different
/statistics/app/:app_idresponses.Business Impact (AI generated)
This preserves tenant isolation for app-scoped keys and removes an app existence oracle that could support enumeration or follow-on targeting.
Test Plan (AI generated)
bun run lint:backendbunx eslint tests/statistics.test.tsbun run supabase:with-env -- bunx vitest run tests/statistics.test.ts tests/statistics-retries.unit.test.tsbun run supabase:with-env -- bunx vitest run tests/rbac-permissions.test.tsbun run cli:build && vue-tsc --noEmitGenerated with AI
Summary by CodeRabbit
Bug Fixes
Tests