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:
📝 WalkthroughWalkthroughAdds a new public function Changes
Sequence DiagramsequenceDiagram
participant Client
participant Func as is_allowed_action(apikey, appid)
participant Auth as ServiceRole/Auth
participant DB as Database
participant Storage as storage.objects
Client->>Func: call is_allowed_action(apikey, appid)
Func->>Auth: validate API key / resolve identity
Auth-->>Func: identity (user_id, key_mode)
alt API key valid
Func->>DB: lookup app -> org_id
DB-->>Func: return org_id
Func->>Auth: check org ownership/permissions
Auth-->>Func: allowed? (true/false)
Func->>Storage: evaluated by storage policies (auth.uid or apikey ownership)
Storage-->>Func: access permitted?
Func-->>Client: return allowed boolean
else invalid key
Func-->>Client: return false
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/tests/45_test_apikey_oracle_permissions.sql (1)
57-71: Consider testing the two-argumentget_user_idoverload for service_role.The service_role tests verify
get_user_id(apikey)but notget_user_id(apikey, app_id). Adding this would complete the coverage symmetry with the denial tests.🧪 Suggested additional test
SELECT is( get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea'), '6aa76066-55ef-4238-ade6-0b32334a4097'::uuid, 'service role should still call get_user_id' ); +SELECT + is( + get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app'), + '6aa76066-55ef-4238-ade6-0b32334a4097'::uuid, + 'service role should still call get_user_id with app_id' + ); + SELECT is( get_org_perm_for_apikey('ae6e7458-c46d-4c00-aa3b-153b0b8520ea', 'com.demo.app'),Update the plan to
SELECT plan(9);accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/tests/45_test_apikey_oracle_permissions.sql` around lines 57 - 71, Add a test that calls the two-argument overload get_user_id(apikey, app_id) under the service_role context to mirror the existing single-argument check and ensure symmetry with the denial tests; specifically, after authenticate_as_service_role(), add an is(...) assertion that compares get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea','com.demo.app') to the expected UUID (same expected value used for the single-arg test) and a descriptive message, and update the test plan count to SELECT plan(9); so the new test is accounted for.
🤖 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/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql`:
- Around line 20-47: The is_allowed_action function validates API keys but lacks
the matching permission lockdown; after the is_allowed_action ("apikey" "text",
"appid" "text") function definition add explicit REVOKE ALL for anon,
authenticated, and public and then GRANT EXECUTE to service_role (mirroring the
pattern used for get_user_id and get_org_perm_for_apikey) so only service_role
can call this sensitive function.
---
Nitpick comments:
In `@supabase/tests/45_test_apikey_oracle_permissions.sql`:
- Around line 57-71: Add a test that calls the two-argument overload
get_user_id(apikey, app_id) under the service_role context to mirror the
existing single-argument check and ensure symmetry with the denial tests;
specifically, after authenticate_as_service_role(), add an is(...) assertion
that compares get_user_id('ae6e7458-c46d-4c00-aa3b-153b0b8520ea','com.demo.app')
to the expected UUID (same expected value used for the single-arg test) and a
descriptive message, and update the test plan count to SELECT plan(9); so the
new test is accounted for.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61cb4981-9877-4605-a6ac-e19441864159
📒 Files selected for processing (5)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sqlsupabase/tests/07_auth_functions.sqlsupabase/tests/10_utility_functions.sqlsupabase/tests/19_test_identity_functions.sqlsupabase/tests/45_test_apikey_oracle_permissions.sql
|
CI check failed on March 16, 2026. The failing case is , which errors with . I did not apply a quick fix here because the same failure signature is also present on other Riderx PRs and looks like a shared/base-branch permission regression rather than something isolated to this branch. |
|
CI check |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql (1)
24-51:⚠️ Potential issue | 🟠 MajorLock down
public.is_allowed_action(text, text)execute privileges.
SECURITY DEFINERwas updated at Line 24, but this migration does not explicitly revoke/grant execute for this function. Please apply the same service-role-only ACL pattern used above.Suggested patch
CREATE OR REPLACE FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") RETURNS boolean @@ $$; + +REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "anon"; +REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "authenticated"; +REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "public"; +GRANT EXECUTE ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") TO "service_role";As per coding guidelines: “Apply
REVOKE ALL ... FROM PUBLICto every PostgreSQL function overload and grant only required roles (e.g.,service_role,authenticated)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql` around lines 24 - 51, The new SECURITY DEFINER function public.is_allowed_action(apikey text, appid text) lacks explicit ACL changes; update the migration to revoke all exec privileges from PUBLIC for the is_allowed_action(text,text) overload and then grant EXECUTE only to the service role (and other permitted roles per pattern, e.g., authenticated) so the function follows the service-role-only ACL pattern used elsewhere; reference the function by its exact signature public.is_allowed_action(text,text) when adding the REVOKE ALL ON FUNCTION ... FROM PUBLIC and the subsequent GRANT EXECUTE ON FUNCTION ... TO service_role (and any other allowed roles).
🤖 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/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql`:
- Around line 24-51: The new SECURITY DEFINER function
public.is_allowed_action(apikey text, appid text) lacks explicit ACL changes;
update the migration to revoke all exec privileges from PUBLIC for the
is_allowed_action(text,text) overload and then grant EXECUTE only to the service
role (and other permitted roles per pattern, e.g., authenticated) so the
function follows the service-role-only ACL pattern used elsewhere; reference the
function by its exact signature public.is_allowed_action(text,text) when adding
the REVOKE ALL ON FUNCTION ... FROM PUBLIC and the subsequent GRANT EXECUTE ON
FUNCTION ... TO service_role (and any other allowed roles).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52bfb2d6-8dcb-4e69-88f7-c5526591f107
📒 Files selected for processing (1)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql (1)
50-77: Consider cachingget_apikey_header()to avoid duplicate subquery evaluation.The
get_apikey_header()is invoked twice within the OR branch (lines 63-65 and 69-71). While PostgreSQL may apply common subexpression elimination, explicitly caching the value would be clearer and ensure consistent behavior across PostgreSQL versions.♻️ Alternative pattern using a single subquery reference
- OR ( - public.get_user_id( - ( - SELECT - "public"."get_apikey_header" () - ) - )::text = ("storage"."foldername" ("name")) [0] - AND public.is_allowed_capgkey( - ( - SELECT - "public"."get_apikey_header" () - ), - '{all}'::"public"."key_mode" [], - ("storage"."foldername" ("name")) [1] - ) - ) + OR ( + SELECT + public.get_user_id(_apikey)::text = ("storage"."foldername" ("name")) [0] + AND public.is_allowed_capgkey( + _apikey, + '{all}'::"public"."key_mode" [], + ("storage"."foldername" ("name")) [1] + ) + FROM (SELECT "public"."get_apikey_header"() AS _apikey) AS _header + )This pattern applies to all four policies (DELETE, UPDATE, INSERT, SELECT).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql` around lines 50 - 77, The policy calls public.get_apikey_header() twice; compute it once and reuse it to avoid duplicated subquery evaluation and ensure consistent results. Replace the OR branch that invokes get_apikey_header() with a single WITH subquery that selects public.get_apikey_header() AS apikey (e.g. WITH apikey AS (SELECT public.get_apikey_header() AS val) SELECT ... FROM apikey) and then use that apikey value for both public.get_user_id(...) and public.is_allowed_capgkey(...); update the USING expression in the policy (named "Allow user or apikey to delete they own folder in apps") to reference this single apikey value instead of calling get_apikey_header() twice.
🤖 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/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql`:
- Around line 50-77: The policy calls public.get_apikey_header() twice; compute
it once and reuse it to avoid duplicated subquery evaluation and ensure
consistent results. Replace the OR branch that invokes get_apikey_header() with
a single WITH subquery that selects public.get_apikey_header() AS apikey (e.g.
WITH apikey AS (SELECT public.get_apikey_header() AS val) SELECT ... FROM
apikey) and then use that apikey value for both public.get_user_id(...) and
public.is_allowed_capgkey(...); update the USING expression in the policy (named
"Allow user or apikey to delete they own folder in apps") to reference this
single apikey value instead of calling get_apikey_header() twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c44454c-cfc7-43a5-a8af-9b58477a86a9
📒 Files selected for processing (2)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sqlsupabase/tests/45_test_apikey_oracle_permissions.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/tests/45_test_apikey_oracle_permissions.sql
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql (1)
20-47:⚠️ Potential issue | 🟠 Major
is_allowed_action(text,text)still needs explicit privilege lockdown.
CREATE OR REPLACE FUNCTIONdoes not reset existing grants. Without explicit REVOKE/GRANT statements after this definition, prior broader EXECUTE permissions can persist.As per coding guidelines, "Apply `REVOKE ALL ... FROM PUBLIC` to every PostgreSQL function overload and grant only required roles (e.g., `service_role`, `authenticated`)."🔒 Proposed grant fix
+REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "anon"; +REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "authenticated"; +REVOKE ALL ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") FROM "public"; +GRANT EXECUTE ON FUNCTION "public"."is_allowed_action" ("apikey" "text", "appid" "text") TO "service_role";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql` around lines 20 - 47, The function public.is_allowed_action(text,text) was redefined but existing EXECUTE grants aren’t reset; after the CREATE OR REPLACE FUNCTION block add explicit privilege lockdown: REVOKE ALL ON FUNCTION public.is_allowed_action(text,text) FROM PUBLIC and then GRANT only the minimal roles that need execute (e.g., service_role and authenticated) — ensure you reference the exact overload name public.is_allowed_action(text,text) when revoking/granting so prior broader permissions are removed and only the intended roles retain EXECUTE.
🤖 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/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql`:
- Around line 58-59: RLS policies compare auth.uid() to elements of the array
returned by the function "storage"."foldername"("name") but use 0-based indexing
which yields NULL; update every occurrence of ("storage"."foldername"("name"))
[0] used in user_id equality checks to use 1-based indexing
("storage"."foldername"("name")) [1] instead (affecting the comparisons that
involve ("auth"."uid"()) = ("storage"."foldername"("name"))[0]); ensure you
change all instances referenced in the comment so the policies correctly compare
("auth"."uid"()) to the first array element.
---
Duplicate comments:
In `@supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql`:
- Around line 20-47: The function public.is_allowed_action(text,text) was
redefined but existing EXECUTE grants aren’t reset; after the CREATE OR REPLACE
FUNCTION block add explicit privilege lockdown: REVOKE ALL ON FUNCTION
public.is_allowed_action(text,text) FROM PUBLIC and then GRANT only the minimal
roles that need execute (e.g., service_role and authenticated) — ensure you
reference the exact overload name public.is_allowed_action(text,text) when
revoking/granting so prior broader permissions are removed and only the intended
roles retain EXECUTE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fe18c86-5c82-4b3e-9467-2b1dc4e02b23
📒 Files selected for processing (1)
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql
supabase/migrations/20260313112417_restrict-apikey-oracle-rls-fixes.sql
Outdated
Show resolved
Hide resolved
|
CI check |
|
CI triage: The regression points at I did not push a speculative patch. |
![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: abcf767da2
ℹ️ 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".
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "authenticated"; |
There was a problem hiding this comment.
These grants keep the oracle open to every signed-in user. supabase/tests/45_test_apikey_oracle_permissions.sql:31-52 now codifies that any authenticated account can call get_user_id(...) and get_org_perm_for_apikey(...), which is enough to probe guessed Capgo keys and learn both whether a key is valid and what access it has on a target app. I checked the repo for callers and only found service-side/security-definer usage (supabase/functions/_backend/utils/pg_files.ts:12-27, supabase/schemas/prod.sql:3626, supabase/schemas/prod.sql:6456-6462), so exposing these RPCs to authenticated is unnecessary and defeats the stated restriction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88dd2bd127
ℹ️ 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".
| -- ============================================================================ | ||
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text") FROM "anon"; | ||
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") FROM "anon"; | ||
| REVOKE ALL ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") FROM "anon"; |
There was a problem hiding this comment.
Revoking anon execute here breaks the existing backend SQL suite: supabase/tests/14_test_apikey.sql and supabase/tests/24_test_data_functions.sql still call get_org_perm_for_apikey(...) without tests.authenticate_as_service_role() or any authenticated context, so they now hit permission denied for function get_org_perm_for_apikey. This commit updated 07_auth_functions.sql, 10_utility_functions.sql, and 19_test_identity_functions.sql, but these two remaining files were left behind, so bun test:backend will start failing immediately.
Useful? React with 👍 / 👎.
|
Run tests fails across CLI/API-key paths with |
|
PR #1793: CI is failing in the auth/RLS path around |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68929d8466
ℹ️ 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".
| FROM "public"."apps" AS "app" | ||
| WHERE | ||
| "app"."app_id" = ("storage"."foldername" ("name")) [2] | ||
| AND "app"."user_id"::text = ("storage"."foldername" ("name")) [1] |
There was a problem hiding this comment.
public.apps RLS
For API-key requests against the apps storage bucket, this new FROM public.apps lookup now runs under the caller's public.apps SELECT policy instead of only using the old security-definer helpers. I checked supabase/schemas/prod.sql:15596, and that policy calls check_min_rights(...); check_min_rights in supabase/schemas/prod.sql:1337-1356 enforces org 2FA/password-policy gates before the row is visible. The previous bucket policy only used get_user_id()/is_allowed_capgkey(), so valid app-owner keys that used to work will now be rejected as soon as the org enables those auth requirements (or otherwise loses app.read visibility). If this migration is only meant to remove the oracle, the app lookup needs to avoid caller-side RLS.
Useful? React with 👍 / 👎.
|
PR #1793 is failing across the CLI/API-key auth suite, mainly and , where assertions now get . That looks like a shared auth behavior regression rather than a small PR-local bug, so I did not try to force a branch-only patch. |
|
CI is still red on https://github.com/Cap-go/capgo/actions/runs/23273324029/job/67670721977. The failure is broad across the CLI/API-key auth suite: |
|
CI is failing because the API-key auth path is still calling into the old |
|
I checked #1793. The latest failing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b300051ec1
ℹ️ 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".
| @@ -0,0 +1,190 @@ | |||
| -- ============================================================================ | |||
There was a problem hiding this comment.
I checked .github/workflows/build_and_deploy.yml:62-63, which deploys schema changes with plain supabase db push, and this repository already contains later migrations such as supabase/migrations/20260319164053_fix_manifest_select_rls.sql. Supabase tracks only migration timestamps in supabase_migrations.schema_migrations, and its CLI docs note that older missing files are only replayed when --include-all is used. Because this new file is backdated to 20260313112417, any staging/production database that has already applied the March 17-19 migrations will skip it entirely, so the RPC grant changes and storage policy fixes in this commit never reach deployed environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6de5bed08
ℹ️ 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".
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text") FROM "anon"; | ||
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") FROM "anon"; | ||
| REVOKE ALL ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") FROM "anon"; | ||
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text") FROM "public"; | ||
| REVOKE ALL ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") FROM "public"; | ||
| REVOKE ALL ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") FROM "public"; | ||
|
|
||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "service_role"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "service_role"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "service_role"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_user_id" ("apikey" "text", "app_id" "text") TO "authenticated"; | ||
| GRANT EXECUTE ON FUNCTION "public"."get_org_perm_for_apikey" ("apikey" "text", "app_id" "text") TO "authenticated"; |
There was a problem hiding this comment.
This migration hardens get_user_id/get_org_perm_for_apikey, but it still leaves the same probe open through other helpers. public.is_app_owner(apikey, appid) remains executable by anon/authenticated (supabase/schemas/prod.sql:17466-17468) and directly wraps public.get_user_id(apikey) (supabase/schemas/prod.sql:6456-6461); public.get_org_owner_id(apikey, app_id) is likewise still public (supabase/schemas/prod.sql:17225-17227, 4540-4563). The updated suite even continues to call is_app_owner(...) anonymously in supabase/tests/19_test_identity_functions.sql:176-184, so unauthenticated callers can still probe guessed keys/apps after this migration.
Useful? React with 👍 / 👎.
|
PR #1793 is still failing on the broader API-key auth regression, not a single local test. The current CI failures in , , and all stop at , so the blocker is the auth/oracle migration behavior rather than a one-line test tweak. The remaining unresolved review threads on need a broader pass before this can go green. |
|
PR #1793 is still failing on the broader API-key auth regression, not a single local test. The current CI failures in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eadc02126
ℹ️ 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".
| (SELECT "auth"."uid" ())::text = ("storage"."foldername" ("name")) [1] | ||
| ) |
There was a problem hiding this comment.
App artifacts are generated as orgs/{owner_org}/apps/{app_id}/... in supabase/functions/_backend/private/upload_link.ts:59-60, and the upload middleware validates that same shape in supabase/functions/_backend/files/files.ts:440-469. With that layout, storage.foldername(name)[1] is the literal orgs, so this predicate is always false for signed-in users, and the repeated [2] lookups below read the org id instead of the app id. Because the same off-by-one indexing is copied into all four recreated apps-bucket policies, valid read/write/delete/insert requests will start getting rejected after this migration.
Useful? React with 👍 / 👎.
|
CI is blocked by repo-wide lint failures in src/components/dashboard/InviteTeammateModal.vue:25 and src/pages/onboarding/organization.vue:613-614, which are unrelated to this branch. The remaining live review threads are still the migration/RLS items around oracle RPC grants, apps-bucket auth, migration timestamping, and path handling. #1793 |
|
CI is blocked by repo-wide lint failures in #1793. The failing run stops in () and (). I did not patch this branch because the failures are outside the PR diff and need a repo-wide lint fix or rebase. |
|
Correction: CI is blocked by repo-wide lint failures in #1793. The failing run stops in |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed86a001d8
ℹ️ 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".
| "app"."app_id" = ("storage"."foldername" ("name")) [2] | ||
| AND "app"."user_id"::text = ("storage"."foldername" ("name")) [1] |
There was a problem hiding this comment.
upload_link.ts:59-60 and files.ts:447-469 show that app artifacts are stored under orgs/{owner_org}/apps/{app_id}/..., so the recreated apps-bucket policies need to validate against the app's org, not a user UUID. Here the policy compares the path to auth.uid()/app.user_id, which means even after correcting the storage.foldername(...) indexes, valid read/write/delete/insert requests will still be rejected whenever the owner user id differs from the org id (the normal case). The same user-vs-org mismatch is duplicated in all four recreated policies.
Useful? React with 👍 / 👎.
|
Status update for #1793: This PR still has 7 unresolved review threads. The latest CI run is red in the shared CLI suite, with failures in That is too broad for a safe one-pass fix, so I left the branch unchanged and reported the blocker instead. |
1 participant
Summary\n\n- Restrict API key oracle RPC access so / are service-role only, with explicit unauthenticated/authenticated denial.\n- Fix to validate key ownership and app existence before granting access.\n- Replace apps storage bucket RLS policy expressions to avoid oracle lookups with /.\n- Add pgTAP coverage for oracle permission behavior across anonymous, authenticated, and service-role roles.
Summary by CodeRabbit
New Features
Bug Fixes
Tests