Security hardening: RCE upgrade, role-aware RLS, OAuth connector binding - #21
Merged
Merged
Conversation
…sons Three low-risk security fixes from the full-project audit. None changes application behaviour; verified with tsc, 565/565 tests, lint and a production build. - rate_limit_buckets: enable RLS and revoke anon/authenticated grants. The table backing every application rate limit was created without RLS, so stock Supabase default privileges left it directly writable over PostgREST by any signed-in user, making all limiters bypassable. Its only accessor is the consume_rate_limit security definer function, which bypasses RLS, so no code path changes. - getWorkspaceSettings / getWorkspaceSettingsForDisplay: require a session and workspace:read on the caller-supplied workspaceId. Every export in a 'use server' file is a POST-reachable endpoint; these two trusted their argument. RLS made it non-exploitable today, but the check keeps the action safe if the reader is ever moved to the admin client. - Cron secret and Meta webhook verify token: compare with timingSafeStringEqual instead of ===. The webhook POST path already used timingSafeEqual; this brings the two remaining comparisons in line. Migration is committed but NOT yet applied to the Supabase project.
…xes) Next.js 16.2.12 was affected by two unauthenticated remote code execution advisories, and the app actively enables the feature one of them targets: - GHSA-2xp9-vwfh-vxw4: unauthenticated RCE in the Image Optimization API via AVIF. next.config.ts configures remotePatterns for cdninstagram.com and fbcdn.net, so this code path is live. - GHSA-p293-qw3h-jr36: unauthenticated RCE on Windows-hosted servers. sharp was additionally held at the vulnerable 0.35.3 by this project's own overrides block (GHSA-rgj7-g3m4-5g8c, libheif). Raised to 0.35.4. eslint-config-next moved to 16.3.5 in lockstep, matching the repo's convention of pinning both exactly. npm audit --omit=dev now reports 0 vulnerabilities, down from 1 critical and 1 high. The 4 remaining advisories are dev-only (eslint, vitest, js-yaml) and do not ship to production. Verified: tsc 0 errors, 565/565 tests, production build OK, lint 0 errors. Lint gained 2 warnings from a new rule in the upgraded config (no-location-assign-relative-destination) flagging pre-existing window.location.href use in two Instagram connect components; left unchanged as they are warnings on working navigation behaviour.
process-automations/graph-executor.ts maps the action_send_email node type to the automation-worker-send-email edge function, but that function was missing from both scripts/deploy-managed-supabase.mjs and scripts/setup-check.mjs while every other automation worker was listed in both. A fresh managed deployment therefore never shipped it, so send-email automation nodes would fail at runtime against a newly provisioned project. The function already exists and already carries the assertInternalInvoke guard, so this only adds it to the deploy and preflight manifests. Verified all eight workers dispatched by graph-executor are now present in both manifests. 565/565 tests pass.
…h_path Live Supabase advisors still reported three findings that 20260702110000_advisor_security_and_fk_index_remediation.sql had started but not completed, because later migrations added functions that missed the same treatment: - anon/authenticated_security_definer_function_executable (5 each): four trigger-only SECURITY DEFINER functions were callable over PostgREST by anon and authenticated. Each has exactly one CREATE TRIGGER definition and zero application rpc() call sites, so nothing needs REST execute. Revoked, the same way 20260702110000 handled create_default_settings and create_default_workspace_settings. - is_member_of(uuid) is the fifth function in those advisor findings and is deliberately left executable: it backs 16 RLS policy expressions in the baseline schema and revoking it would break row-level security. That exception is already documented in 20260702110000. - function_search_path_mutable (2): claim_webhook_inbox_events and claim_automation_actions had no pinned search_path. Both are SECURITY INVOKER with fully schema-qualified bodies and execute granted only to service_role and the worker roles, so this is deterministic-resolution hygiene, not a privilege fix. Signatures were read from the live database rather than inferred. 565/565 tests pass. Not yet applied.
is_member_of(uuid) resolves membership only, so the baseline "Member access" policies on these two tables — both FOR ALL with no FOR clause — granted every member, viewer included, full read and write. Combined with the table-level GRANT ALL TO authenticated that both tables carry (confirmed live: authenticated=arwdDxtm/postgres), a viewer could bypass the application entirely, call PostgREST with their own JWT, and overwrite provider credentials: telegram_bot_token, the AI provider keys, and the Instagram access_token/refresh_token. decryptSecretIfNeeded() returns any value lacking an enc: prefix verbatim, so an attacker-written plaintext token would then be used as-is by the runtime. Adds has_workspace_role(uuid, text[]) — is_member_of plus a role constraint, SECURITY DEFINER with pinned search_path — and splits each FOR ALL policy into a member SELECT plus owner/admin INSERT/UPDATE/DELETE. This does not change application behaviour. Every session-client write path to these tables already enforces the same restriction, verified route by route: workspace_settings via requireWorkspacePermission(..., 'settings:write') and social_accounts via 'integrations:write', both mapping to ["owner","admin"] in WORKSPACE_PERMISSION_ROLE_MAP. Reads are untouched and remain open to every member. Service-role callers bypass RLS. The two worker-role SELECT policies on social_accounts are left in place. Scoped deliberately to the two credential-bearing tables. The same role-blind pattern exists on other baseline tables and needs the same per-table review of write paths before it can be changed safely. 565/565 tests pass. Not yet applied.
The Developer API OAuth connector had two flaws in the same subsystem. redirect_uri was unbound. /register returned a client_id but persisted nothing, so /authorize had no registered set to compare against and validated only that the URI started with "https://". Because the authorization code encrypts the operator's raw Developer API key, and /api/developer/mcp unwraps it back into a Bearer header, an attacker could send a workspace owner an authorize link pointing at their own host. The consent page renders on the genuine SwiftFlow origin asking for exactly what the product legitimately asks for, so approving it hands the attacker a code that exchanges into full workspace API access. PKCE does not help: it binds the code to whoever made the request, which in that flow is the attacker. Authorization codes were replayable. verifyDeveloperOAuthCode only decrypted and checked exp, with no server-side record, so a code stayed valid for its full 10 minute TTL even after the legitimate client redeemed it. Changes: - New developer_oauth_clients and developer_oauth_used_codes tables, both service_role only. - /register now persists client_id with its redirect_uris and rejects a registration that has no usable HTTPS URI. - /authorize resolves client_id and requires redirect_uri to exactly match a registered URI, on both GET and POST. Failures render an error page and never redirect, so an unregistered URI cannot be used to bounce the user. - Codes carry a jti; /token claims it after all other checks pass, so a replay is refused while a failed PKCE attempt does not burn a valid code. jti is optional on the payload type so codes minted before this deploy still verify; they expire within the 10 minute TTL. Matching is exact string comparison with no normalization, prefix matching or wildcards, since those are what make redirect_uri validation bypassable. Verified nothing is actively connected before changing the flow: 2 API keys exist, both used, but last use was 2026-07-23. Existing access and refresh tokens are unaffected either way, as they verify by decryption rather than client lookup. 570 tests pass, up from 565: adds regression coverage for unregistered redirect_uri, unknown client_id, registration without HTTPS redirect, code replay, and non-consumption on PKCE failure.
Completes the work started in 20260912140000. Eleven more baseline tables had
policies that resolved workspace membership but not role, so any member —
viewer included — could bypass the application through PostgREST and write
content, automations, inbox rows, brand profiles, and third-party service
credentials.
Write roles are taken from the permission each table's own routes already
enforce via requireWorkspacePermission, per WORKSPACE_PERMISSION_ROLE_MAP:
content:write (owner/admin/editor) - posts, comments, conversations,
messages
automation:write (owner/admin) - automations, automation_logs,
automation_scheduled_executions,
processed_comments
settings:write (owner/admin) - external_services,
workspace_brand_profiles
analytics:sync (owner/admin) - analytics_snapshots
Reads are unchanged: every member keeps SELECT everywhere. Service-role
callers bypass RLS, and the existing "Service role has full access" policies
are left alone — each is gated by an auth.jwt() role check in its USING
clause, so it never widens access for a normal user. Worker-role policies are
untouched.
automation_logs and automation_scheduled_executions carry no workspace_id, so
both are scoped through their parent automation, matching the shape of the
policies they replace. This was caught by checking the live schema rather than
assuming the column existed.
Not changed: workspaces, workspace_members, workspace_invites and
publishing_automations already constrain role in their own policies, and
chat_sessions is scoped per user rather than by workspace role.
570/570 tests pass. Not yet applied.
The Supabase 0.5 GB warning is not application data. At 492 MB total, the public schema — all SwiftFlow data — was 15 MB (3%). The rest was byproducts of scheduler-tick-cron firing every minute: - cron.job_run_details: 289,479 real rows back to 2026-02-23 (195 MB). pg_cron has no built-in retention, so this grows unbounded at ~1440 rows/day. - net._http_response: only 360 live rows over ~6 hours (pg_net expires its own rows) but 271 MB of heap with 0 dead tuples — bloat that will never shrink on its own. This migration schedules purge-cron-run-history nightly to trim cron.job_run_details to 7 days, which stops the growth. It is idempotent: the job is unscheduled first so a re-run or db reset cannot duplicate it. Reclaiming the space already on disk is deliberately NOT in the migration — TRUNCATE is destructive and does not belong in schema history. It is documented in docs/operations/database-size-reclaim.md with the reasoning for why each truncate is safe. VACUUM FULL is not an option: both tables are owned by supabase_admin rather than postgres. TRUNCATE is, and returns the space immediately. Worth recording: the existing retention-cleanup edge function and workspace_retention_policies target public-schema tables totalling ~15 MB, so enabling them would not have addressed this warning.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This branch was successfully deployed
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.
Full-project security audit plus the fixes, verified against the live database at each step.
Highest impact
Next.js 16.2.12 -> 16.3.5, sharp -> 0.35.4. Two unauthenticated RCE advisories, one of them in the Image Optimization API, which
next.config.tsactively enables viaremotePatterns. sharp was additionally pinned to the vulnerable 0.35.3 by this repo's ownoverridesblock.npm audit --omit=devwent from 1 critical + 1 high to 0.Role-aware RLS across 18 tables.
is_member_of()resolved membership but not role, and 18 tables carriedGRANT ALL TO authenticated, so aviewercould bypass the app entirely and write through PostgREST — including Instagram tokens, Telegram bot tokens, and AI provider keys. Addshas_workspace_role()and splits the blanketFOR ALLpolicies into member SELECT plus role-constrained writes, mirroring the permissions the routes already enforce.OAuth connector could leak Developer API keys.
/registerpersisted nothing, so/authorizevalidatedredirect_urionly as "starts with https://". Since the authorization code encrypts the operator's raw API key, an attacker could have a workspace owner approve a consent page on the genuine origin and receive a code redeemable for full API access. Codes were also replayable for their full 10 minute TTL. Now: clients are persisted,redirect_urimust exactly match a registered URI, and codes are single-use.Also included
search_pathpinned on 2 more.rate_limit_buckets: RLS enabled and grants revoked.use serverexports that trusted a caller-suppliedworkspaceId.automation-worker-send-emailwas dispatched by the graph executor but missing from both deploy manifests, so fresh deploys never shipped it.Verification
tsc 0 errors, 570/570 tests (up from 565), lint 0 errors, production build passing. Every database change was applied and then confirmed by live query — the final check classified all 56 write-capable policies across the 18 tables and found zero still role-blind.
Two findings retracted after live verification
Worth recording, since the original audit was wrong about both:
rate_limit_bucketswas reported as exploitable on the assumption that stock Supabase default privileges granted it toanon/authenticated. Liverelaclwas empty (owner-only). Rate limits were never bypassable. The migration is kept as hygiene.netschema, which PostgREST does not route to, and it is load-bearing for the every-minutescheduler-tickheartbeat. "Fixing" it would have broken automation for a vulnerability that did not exist.Deployment notes
All migrations that code depends on are already applied to production, so this is safe to merge and deploy as-is. One migration is still pending and has no code dependency:
npx supabase db push --linked # applies 20260912170000_prune_cron_run_historyBehaviour change to be aware of: a
vieweroreditorhitting PostgREST directly with a non-admin token will now be refused writes. Nothing changes through the app, where every write path already required owner/admin. An existing MCP connector would need to re-register to authorize again; existing access and refresh tokens keep working.🤖 Generated with Claude Code