Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized RBAC request validation and JSON parsing were added via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant App as Hono App
participant Auth as Auth Middleware
participant Validator as rbac_validation
participant DB as Database
Client->>App: HTTP request (params/body)
App->>Auth: authenticate request
alt auth fails
Auth-->>Client: 401 Unauthorized
else auth succeeds
App->>Validator: validate params/body (sValidator / validateJsonBody)
alt validation fails
Validator-->>Client: 400 { error: "..." }
else validation succeeds
App->>DB: perform action (query/insert/update/delete)
DB-->>App: result
App-->>Client: 200/204 response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
![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: 023453b7de
ℹ️ 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: 4
🧹 Nitpick comments (1)
eslint.config.js (1)
18-27: Remove duplicateObject.groupBypolyfill block.This block is redundant after the earlier guard at Line 3; it adds dead/duplicate fallback logic and increases config drift risk.
♻️ Proposed cleanup
-if (!Object.groupBy) { - Object.groupBy = function groupBy(items, callbackfn) { - return items.reduce((groups, item, index) => { - const key = callbackfn(item, index) - groups[key] ??= [] - groups[key].push(item) - return groups - }, Object.create(null)) - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 18 - 27, Remove the duplicate Object.groupBy polyfill block: delete the second if (!Object.groupBy) { function groupBy(...) { ... } } block so only the original guard-and-definition remains; ensure the only definition is the earlier guarded polyfill for Object.groupBy and no other duplicate fallback exists.
🤖 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/private/groups.ts`:
- Around line 30-31: The route-level param/body validators (sValidator('param',
orgIdParamSchema, invalidOrgIdHook)) are being executed before authentication,
causing anonymous requests with bad input to return 400 instead of 401; reorder
the middleware so authentication runs first (move the auth handler before
sValidator) for the app.get('/:org_id', app.post, app.put and app.delete routes
in this file (look for usages of sValidator, orgIdParamSchema, invalidOrgIdHook
and the route handlers that extract c.req.valid), and apply the same change to
the other affected declarations noted in the comment so auth always executes
prior to validation.
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 122-123: The route-level validators (sValidator(...)) are running
before the authentication/Unauthorized check, causing malformed anonymous
requests to return 400 instead of 401; fix by ensuring auth enforcement runs
before validation: reorder middleware so the authentication check/middleware
(the Unauthorized check used elsewhere in this file) executes prior to
sValidator (or add a dedicated auth middleware ahead of sValidator) for
app.get('/:org_id') and apply the same change to the POST, PATCH and DELETE
routes referenced (the handlers around the other sValidator usages). Ensure you
reference and preserve the existing Unauthorized/auth check symbol when
reordering so validation only runs after auth.
In `@supabase/functions/_backend/private/roles.ts`:
- Line 16: The route registers currently run sValidator(...) before the
authentication guard, causing unauthenticated requests with invalid params to
return 400 instead of 401; reorder middleware so the auth guard runs first
(e.g., register authGuard before sValidator in the middleware list for
app.get('/', ...) and the other affected route around lines 55-56), ensuring
authGuard(...) executes before sValidator(...) so authentication is enforced
prior to validation.
In `@tests/private-rbac-validation.unit.test.ts`:
- Around line 19-23: Replace the type alias named StandardSchema with an
interface declaration to satisfy ts/consistent-type-definitions; keep the same
shape and method signature (the nested '~standard' property and its validate
method returning StandardSchemaV1.Result<unknown> |
Promise<StandardSchemaV1.Result<unknown>>), i.e., convert the "type
StandardSchema = { ... }" declaration into "interface StandardSchema { ... }"
preserving the property and validate signature.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 18-27: Remove the duplicate Object.groupBy polyfill block: delete
the second if (!Object.groupBy) { function groupBy(...) { ... } } block so only
the original guard-and-definition remains; ensure the only definition is the
earlier guarded polyfill for Object.groupBy and no other duplicate fallback
exists.
🪄 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: 6466c357-75dd-4d3a-b91b-7db0d75d14ca
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksupabase/functions/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
eslint.config.jspackage.jsonsupabase/functions/_backend/private/groups.tssupabase/functions/_backend/private/rbac_validation.tssupabase/functions/_backend/private/role_bindings.tssupabase/functions/_backend/private/roles.tssupabase/functions/_backend/utils/pg.tssupabase/functions/deno.jsontests/private-rbac-validation.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79a091d128
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c355dd926c
ℹ️ 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".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
supabase/functions/_backend/private/role_bindings.ts (1)
122-129:⚠️ Potential issue | 🟠 MajorAuth check runs after param validation — potential API regression.
The
sValidatormiddleware at line 123 runs before the handler body, meaning an invalidorg_idfrom an unauthenticated request returns400 Invalid org_idinstead of401 Unauthorized. This changes the error contract for anonymous requests with malformed params.If preserving "401 before 400" semantics is required, consider extracting the auth check into a middleware that runs before
sValidator:Suggested pattern
+function requireAuth(c: Context, next: () => Promise<void>) { + if (!c.get('auth')?.userId) { + return c.json({ error: 'Unauthorized' }, 401) + } + return next() +} + // GET /private/role_bindings/:org_id - List role bindings for an org -app.get('/:org_id', sValidator('param', orgIdParamSchema, invalidOrgIdHook), async (c) => { +app.get('/:org_id', requireAuth, sValidator('param', orgIdParamSchema, invalidOrgIdHook), async (c) => { const { org_id: orgId } = c.req.valid('param') - const userId = c.get('auth')?.userId - - if (!userId) { - return c.json({ error: 'Unauthorized' }, 401) - } + const userId = c.get('auth')!.userIdApply the same ordering to POST (line 190), PATCH (line 325), and DELETE (line 428) routes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/role_bindings.ts` around lines 122 - 129, The current route handlers attach sValidator('param', orgIdParamSchema, invalidOrgIdHook) before performing the auth check inside the handler, causing malformed org_id from anonymous requests to return 400 instead of the desired 401; fix by extracting the auth check into a middleware that runs before sValidator (e.g., create an auth middleware that checks c.get('auth')?.userId and returns 401 if missing) and apply it to the routes that validate org_id—specifically the GET '/:org_id' handler (where sValidator is used), and likewise reorder middleware for the POST, PATCH, and DELETE org_id routes so the auth middleware runs before sValidator('param', orgIdParamSchema, invalidOrgIdHook).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@supabase/functions/_backend/private/role_bindings.ts`:
- Around line 122-129: The current route handlers attach sValidator('param',
orgIdParamSchema, invalidOrgIdHook) before performing the auth check inside the
handler, causing malformed org_id from anonymous requests to return 400 instead
of the desired 401; fix by extracting the auth check into a middleware that runs
before sValidator (e.g., create an auth middleware that checks
c.get('auth')?.userId and returns 401 if missing) and apply it to the routes
that validate org_id—specifically the GET '/:org_id' handler (where sValidator
is used), and likewise reorder middleware for the POST, PATCH, and DELETE org_id
routes so the auth middleware runs before sValidator('param', orgIdParamSchema,
invalidOrgIdHook).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e1eb6e4-b7d1-438f-b734-9882d8f413fa
📒 Files selected for processing (4)
supabase/functions/_backend/private/groups.tssupabase/functions/_backend/private/rbac_validation.tssupabase/functions/_backend/private/role_bindings.tstests/private-rbac-validation.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/functions/_backend/private/groups.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120ace77b9
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/private/rbac_validation.ts`:
- Around line 76-77: In the two JSON-parse catch blocks inside
rbac_validation.ts (the catch around reading/parsing the body and the second
similar catch), stop passing the thrown parse `error` into simpleErrorWithStatus
as `{ error }` (which overwrites the legacy `error` code); instead preserve the
helper's `error` field by passing the legacy code only and move the raw parse
information into a different key such as `{ parseError: String(error) }` or `{
parse_error: String(error) }` (or omit sensitive internals entirely), so call
simpleErrorWithStatus(...) with the legacy error code and additional info under
a non-conflicting property; update both occurrences where `{ error }` is
currently passed.
🪄 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: bc95cd0d-352a-42f0-afc6-6ea34379293c
📒 Files selected for processing (2)
supabase/functions/_backend/private/rbac_validation.tstests/private-rbac-validation.unit.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/organization-api.test.ts (1)
124-127: Assert API key payload shape before dereferencingid/key.The optional response type plus non-null assertions can hide contract regressions and fail later with less-clear errors. Validate shape immediately after the status check.
Suggested change
- const createdKey = await createResponse.json<{ id?: number, key?: string, error?: string, message?: string }>() - expect(createResponse.status, JSON.stringify(createdKey)).toBe(200) - readOnlyKey = createdKey.key! - readOnlyKeyId = createdKey.id! + const createdKey = await createResponse.json<{ id?: number, key?: string, error?: string, message?: string }>() + expect(createResponse.status, JSON.stringify(createdKey)).toBe(200) + const parsedKey = z.object({ id: z.number(), key: z.string() }).parse(createdKey) + readOnlyKey = parsedKey.key + readOnlyKeyId = parsedKey.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/organization-api.test.ts` around lines 124 - 127, The test dereferences createdKey.id and createdKey.key with non-null assertions after parsing createResponse, which can mask payload regressions; update the test around createResponse / createdKey to assert the payload shape immediately after the status check by verifying createdKey has both id and key and they are the expected types (e.g., id is a number and key is a string) before assigning to readOnlyKey and readOnlyKeyId (references: variables createdKey, createResponse, readOnlyKey, readOnlyKeyId).
🤖 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/organization-api.test.ts`:
- Around line 124-127: The test dereferences createdKey.id and createdKey.key
with non-null assertions after parsing createResponse, which can mask payload
regressions; update the test around createResponse / createdKey to assert the
payload shape immediately after the status check by verifying createdKey has
both id and key and they are the expected types (e.g., id is a number and key is
a string) before assigning to readOnlyKey and readOnlyKeyId (references:
variables createdKey, createResponse, readOnlyKey, readOnlyKeyId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6caecc43-ed49-42d4-842b-ec5914dec52c
📒 Files selected for processing (1)
tests/organization-api.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa196b0cf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1759fbee2
ℹ️ 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".
riderx
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337b3d6e2e
ℹ️ 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".
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef309db13d
ℹ️ 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".
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7342740533
ℹ️ 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".
| v_is_super_admin := EXISTS ( | ||
| SELECT 1 | ||
| FROM public.org_users | ||
| WHERE org_users.org_id = NEW.org_id | ||
| AND org_users.user_id = auth.uid() | ||
| AND org_users.app_id IS NULL | ||
| AND org_users.channel_id IS NULL | ||
| AND org_users.user_right = public.rbac_right_super_admin()::public.user_min_right |
There was a problem hiding this comment.
The new check_org_user_privileges() migration replaces the legacy-path super-admin gate from check_min_rights('super_admin', ...) to a direct org_users lookup, which drops the built-in 2FA and password-policy enforcement in check_min_rights_legacy. For orgs with RBAC disabled, authenticated super-admins who do not satisfy these controls can now pass the trigger’s v_is_super_admin check and proceed with privileged org_users writes that were previously blocked by the legacy gate.
Useful? React with 👍 / 👎.
1 participant
Summary (AI generated)
zodzoddependency and Deno import-map entries, and rename the old contract test toplugin-validation.test.tsMotivation (AI generated)
The branch started as a narrow RBAC pilot, but the migration target is now the full repository validator surface. This update carries the migration through the remaining backend runtime paths and test assertions so the codebase no longer depends on Zod directly.
Business Impact (AI generated)
This standardizes validation on ArkType across the backend, reduces validator drift between endpoints and tests, and removes a deprecated parallel validation stack. It also keeps plugin compatibility behavior explicit on high-traffic endpoints instead of splitting logic between two schema libraries.
Test Plan (AI generated)
bun lint:backendbun typecheckbunx vitest run tests/plugin-validation.test.ts tests/admin-stats.unit.test.ts tests/queue-consumer-message-shape.unit.test.tsGenerated with AI