fix: restrict manifest reads and event org spoofing#1811
Conversation
|
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)
📝 WalkthroughWalkthroughServer-side resolution of a tracking user with RBAC checks was added to event handling; manifest SELECT access is restricted via a new per-row RLS policy; tests updated to cover manifest visibility and event broadcast permission scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventsFn as Events Function
participant Auth as Auth/Permission
participant DB as Supabase DB
participant Notifier as Notifier (notifyConsole/PostHog)
Client->>EventsFn: POST /events (body, headers)
EventsFn->>Auth: resolveTrackingUserId(requester, body.user_id, body.tags['app-id'])
Auth->>DB: lookup app (by app-id) and verify owner_org / check_min_rights
DB-->>Auth: permission result
Auth-->>EventsFn: trackingUserId or deny
alt allowed
EventsFn->>DB: fetch org/app context (trackedBody.user_id, appId)
EventsFn->>Notifier: dispatch notifyConsole/posthog with trackedBody & trackingUserId
Notifier-->>EventsFn: ack
EventsFn-->>Client: 200 OK
else denied
EventsFn-->>Client: 403 no_permission
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/events.test.ts (1)
17-17: Consider usingit.concurrent()for parallelism.The coding guidelines recommend using
it.concurrent()instead ofit()to maximize test parallelism. This applies to both the existing and new tests in this file.♻️ Example refactor for one test
- it('track event with apikey', async () => { + it.concurrent('track event with apikey', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` at line 17, Replace the synchronous test declarations with concurrent ones to enable parallel test execution: change the test invocation for "track event with apikey" from it(...) to it.concurrent(...), and apply the same replacement for other tests in this file (tests/events.test.ts) so functions like the "track event with apikey" test and any other it(...) declarations become it.concurrent(...).supabase/functions/_backend/private/events.ts (1)
93-99: Variable shadowing:appIdis redeclared inside the block.Line 94 declares
const appIdwhich shadows theappIddeclared at line 59. While functionally correct (the inner scope usestrackedBody.tags['app-id']which should match), this can cause confusion.♻️ Consider renaming to avoid shadowing
- if (trackedBody.user_id && trackedBody.tags && typeof trackedBody.tags['app-id'] === 'string' && trackedBody.event === 'onboarding-step-done') { - const appId = trackedBody.tags['app-id'] + if (trackedBody.user_id && trackedBody.tags && typeof trackedBody.tags['app-id'] === 'string' && trackedBody.event === 'onboarding-step-done') { + const onboardingAppId = trackedBody.tags['app-id'] await backgroundTask(c, Promise.all([ supabase .from('orgs') .select('*') .eq('id', trackedBody.user_id) .single(), supabase .from('apps') .select('*') - .eq('app_id', appId) + .eq('app_id', onboardingAppId) .single(), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/events.ts` around lines 93 - 99, There is a variable shadowing issue: the inner const appId declared in the if block (the block that checks trackedBody.tags['app-id'] and event === 'onboarding-step-done' and then calls backgroundTask with supabase.from('orgs')) reuses the same name as an earlier appId (declared around line 59). Rename the inner variable to something unique (e.g., trackedAppId or appIdFromTags) and update all uses inside that if block (including the backgroundTask call and any subsequent references within the block) so you don’t shadow the outer appId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/events.ts`:
- Around line 93-99: There is a variable shadowing issue: the inner const appId
declared in the if block (the block that checks trackedBody.tags['app-id'] and
event === 'onboarding-step-done' and then calls backgroundTask with
supabase.from('orgs')) reuses the same name as an earlier appId (declared around
line 59). Rename the inner variable to something unique (e.g., trackedAppId or
appIdFromTags) and update all uses inside that if block (including the
backgroundTask call and any subsequent references within the block) so you don’t
shadow the outer appId.
In `@tests/events.test.ts`:
- Line 17: Replace the synchronous test declarations with concurrent ones to
enable parallel test execution: change the test invocation for "track event with
apikey" from it(...) to it.concurrent(...), and apply the same replacement for
other tests in this file (tests/events.test.ts) so functions like the "track
event with apikey" test and any other it(...) declarations become
it.concurrent(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 583ff23c-9e45-4662-a587-5d1c5b230cf1
📒 Files selected for processing (5)
supabase/functions/_backend/private/events.tssupabase/migrations/20260317040310_restrict_manifest_read_access.sqlsupabase/tests/26_test_rls_policies.sqlsupabase/tests/27_test_rls_scenarios.sqltests/events.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/private/events.ts (1)
64-65: Consider the empty string case fortrackingUserId.When
c.get('auth')?.userIdis undefined,authUserIddefaults to an empty string (line 24). If norequestedUserIdis provided,resolveTrackingUserIdreturns this empty string, making theif (trackingUserId)check falsy and skipping the broadcast.This is likely the intended defensive behavior for edge cases where auth context is incomplete, but consider logging this scenario for observability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/events.ts` around lines 64 - 65, The check around trackedBody.notifyConsole and trackingUserId currently treats an empty-string authUserId (from c.get('auth')?.userId) as falsy and silently skips broadcasting; update the logic in resolveTrackingUserId / the caller where trackingUserId is used to detect the empty-string case and emit a debug/warn log (including context like authUserId and requestedUserId) before skipping the broadcast so the skipped-broadcast scenario is observable; reference resolveTrackingUserId, authUserId (c.get('auth')?.userId), trackedBody.notifyConsole, and trackingUserId to locate where to add the conditional log and return early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/events.ts`:
- Around line 64-65: The check around trackedBody.notifyConsole and
trackingUserId currently treats an empty-string authUserId (from
c.get('auth')?.userId) as falsy and silently skips broadcasting; update the
logic in resolveTrackingUserId / the caller where trackingUserId is used to
detect the empty-string case and emit a debug/warn log (including context like
authUserId and requestedUserId) before skipping the broadcast so the
skipped-broadcast scenario is observable; reference resolveTrackingUserId,
authUserId (c.get('auth')?.userId), trackedBody.notifyConsole, and
trackingUserId to locate where to add the conditional log and return early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 306814c4-67e1-4bfd-92ed-f6d68672d8aa
📒 Files selected for processing (2)
supabase/functions/_backend/private/events.tstests/events.test.ts
|
Merged origin/main and resolved the conflicts in |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/functions/_backend/private/events.ts (1)
67-79: Consider removing redundant permission check.The new
resolveTrackingUserId(line 78) now covers all permission validation including thenotifyConsolecase, making thecanAccessRequestedOrgcheck (lines 73-74) redundant. Both paths perform similar permission queries.However, keeping both provides defense-in-depth and maintains backward compatibility during the transition.
♻️ Optional: Remove redundant check if confident in new validation
app.post('/', middlewareV2(['read', 'write', 'all', 'upload']), async (c) => { const body = await parseBody<TrackOptions & { notifyConsole?: boolean }>(c) - const requestedOrgId = body.notifyConsole && typeof body.user_id === 'string' && body.user_id.length > 0 - ? body.user_id - : undefined - - if (requestedOrgId && !(await canAccessRequestedOrg(c, requestedOrgId))) - return c.json({ error: 'Forbidden' }, 403) - const requestedUserId = typeof body.user_id === 'string' ? body.user_id : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/events.ts` around lines 67 - 79, Remove the redundant org-access check by deleting the conditional that calls canAccessRequestedOrg and returns 403; rely on resolveTrackingUserId to perform the consolidated permission validation for the notifyConsole/requestedOrgId path. Ensure the existing logic that computes requestedOrgId, requestedUserId, appId, calls resolveTrackingUserId(c, requestedUserId, appId), and sets trackedBody remains unchanged so permission handling and user_id substitution flow through resolveTrackingUserId only.tests/events.test.ts (1)
53-76: Consider usingit.concurrent()for parallel execution.Per coding guidelines, test files run in parallel and should use
it.concurrent()instead ofit()to maximize parallelism. This test modifies no shared state and can safely run concurrently.The test logic itself is correct and properly validates the cross-org spoofing protection.
♻️ Suggested refactor
- it('rejects notifyConsole broadcasts for foreign organizations', async () => { + it.concurrent('rejects notifyConsole broadcasts for foreign organizations', async () => {As per coding guidelines: "ALL TEST FILES RUN IN PARALLEL; use
it.concurrent()instead ofit()to maximize parallelism"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events.test.ts` around lines 53 - 76, Replace the synchronous test declaration with a concurrent one: change the call to the test titled 'rejects notifyConsole broadcasts for foreign organizations' from it(...) to it.concurrent(...). Specifically update the test function surrounding the fetch/asserts in tests/events.test.ts (the block asserting 403 and 'Forbidden') so it uses it.concurrent to allow parallel execution; no other logic or shared-state handling needs modification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/events.ts`:
- Around line 67-79: Remove the redundant org-access check by deleting the
conditional that calls canAccessRequestedOrg and returns 403; rely on
resolveTrackingUserId to perform the consolidated permission validation for the
notifyConsole/requestedOrgId path. Ensure the existing logic that computes
requestedOrgId, requestedUserId, appId, calls resolveTrackingUserId(c,
requestedUserId, appId), and sets trackedBody remains unchanged so permission
handling and user_id substitution flow through resolveTrackingUserId only.
In `@tests/events.test.ts`:
- Around line 53-76: Replace the synchronous test declaration with a concurrent
one: change the call to the test titled 'rejects notifyConsole broadcasts for
foreign organizations' from it(...) to it.concurrent(...). Specifically update
the test function surrounding the fetch/asserts in tests/events.test.ts (the
block asserting 403 and 'Forbidden') so it uses it.concurrent to allow parallel
execution; no other logic or shared-state handling needs modification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee098ba-b4c1-4f81-9fb3-36e45bb7484f
📒 Files selected for processing (2)
supabase/functions/_backend/private/events.tstests/events.test.ts
![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: 874793756e
ℹ️ 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".
| if (await checkPermission(c, 'org.read', { orgId: requestedUserId })) { | ||
| return requestedUserId |
There was a problem hiding this comment.
user_id
resolveTrackingUserId() now uses checkPermission(c, 'org.read', { orgId: requestedUserId }) when the payload has a user_id but no tags['app-id']. For legacy API keys, checkPermission() routes through rbac_check_permission_direct(), whose org-scoped fallback drops p_apikey and only checks the owning user's membership, so a key limited to org A can still post events as org B if that user belongs to both orgs. This means the spoofing fix is incomplete for non-app-id event payloads; using the existing org-aware API-key check here is necessary to actually block cross-org attribution.
Useful? React with 👍 / 👎.
|
|
@riderx We just need you. Thank you for the pull request. We just need you to reply or fix your pull request according to the AI comments. When the AI reviewer is done and the build passes in the CI, we will merge your pull request. |
1 participant
Summary (AI generated)
public.manifestreads to org/app-scoped callers instead ofUSING (true)/private/eventsso callers cannot broadcastnotifyConsoleevents for a different org than the referenced appnotifyConsolespoof attemptsMotivation (AI generated)
Two triaged security advisories were low-risk to fix together.
public.manifestexposed cross-organization bundle metadata through an over-permissive RLS policy, and/private/eventsstill allowed org targeting that was broader than the app being referenced in the event payload.Business Impact (AI generated)
This removes a cross-tenant metadata leak and prevents fake cross-org operational events in the dashboard. That reduces customer trust risk and closes two externally reported security findings with a small, reviewable patch.
Test Plan (AI generated)
bun scripts/supabase-worktree.ts test dbbun run supabase:with-env -- bunx vitest run tests/events.test.tsbun test:backendbun test:allcurrently fails in pre-existing local CLI upload tests (tests/cli*.test.ts) with local fetch/upload errors unrelated to this patchGenerated with AI
Summary by CodeRabbit
Bug Fixes
New Features
Tests