fix: code review security and quality fixes#109
Merged
Conversation
The space_admins_manage and members_read policies on space_access_config used (auth.jwt() ->> 'space_id')::uuid which reads from the JWT root. This project stores space_id in app_metadata, so the correct accessor is current_space_id(). This is the same bug that was fixed for import_jobs in migration 20260320181450. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The users_update_own RLS policy allows members to UPDATE their own row but has no column-level restriction. A malicious user could bypass server actions and directly modify plan_id, status, has_twenty_four_seven, and other security-sensitive fields via the Supabase client. This trigger rejects changes to protected columns unless the caller is service_role, a space admin, or a platform admin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The members_read SELECT policy on space_access_config exposed all columns including nuki_api_token to any authenticated member. Replace with a SECURITY DEFINER function that returns only safe columns, and restrict direct table SELECT to space admins. Update the /access page to use the new RPC. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add email, UUID, and status validation at API boundaries in platform admin actions (addPlatformAdmin, removePlatformAdmin, updateTenantStatus, updatePlatformFee) per project convention of Zod at API boundaries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous implementation checked admin count and deleted in separate queries, allowing a race where two concurrent removals could both pass the count check and leave zero admins. Replace with an atomic RPC that deletes only if count > 1 in a single statement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Math.random() is not cryptographically secure. For physical access PINs, use Node's crypto.randomInt() which provides a CSPRNG. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
updateFeatureFlag accepted arbitrary keys from the client, allowing injection of unexpected keys into the features JSONB column. Add an allowlist check matching the FEATURE_FLAGS defined in the UI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Space-level webhook handlers used spaceId! (non-null assertion) but spaceId can be null when the connected account lookup fails to find a space. Add an explicit null check with error logging before dispatching to handlers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The server action accepted a raw API token and made an external Nuki API call without verifying the caller is authenticated. Add getSpaceId() check to ensure only authenticated users with space context can invoke. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
getApplicationFeesSummary and getPaymentVolume were limited to 100 records, silently truncating results for active platforms. Use Stripe's async iterator (for-await) to paginate through all records. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The booking overlap, desk conflict, fixed desk, and pass checks were performed as separate queries before calling the RPC, creating a race window where two concurrent bookings could both pass validation. Move all conflict checks inside the SECURITY DEFINER function where they run within the same transaction as the INSERT. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comments clarifying that middleware handles authentication (layer 1) while the (dashboard)/layout.tsx handles platform admin authorization (layer 2). All protected routes must live under (dashboard). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The email notification logger used createClient() (user-scoped) which could fail silently if RLS on notifications_log restricts inserts. Infrastructure logging should use createAdminClient() to bypass RLS and guarantee writes succeed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Next.js 16 renames middleware.ts to proxy.ts, which runs on Node.js runtime instead of edge. Rename in both apps/web and apps/admin, remove the config.matcher export (proxy runs on all requests), and add early returns for static assets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Add type definitions for remove_platform_admin and get_member_access_config functions introduced in new migrations. Required for TypeScript to recognize the RPC calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Comprehensive fixes from code review of the
devbranch. Addresses 3 critical security issues, 6 high-severity bugs, and 4 medium-severity improvements.Critical (security)
space_access_configpolicies used wrong JWT path, breaking the feature for all non-platform-adminsusers_update_ownRLS policy allowed self-update ofplan_id,status,has_twenty_four_sevenetc. via direct Supabase clientnuki_api_tokenwas readable by any authenticated member viamembers_readSELECT policyHigh
removePlatformAdminwith atomic RPCMath.random()withcrypto.randomInt()for door PIN generationupdateFeatureFlagkey parameterspaceIdin Stripe webhook routingfetchNukiSmartlocksserver actionMedium
create_booking_with_creditsRPCBonus
middleware.ts→proxy.tsin both apps for Next.js 16New migrations
20260326081642_fix_access_config_rls_jwt_path.sql20260326082946_restrict_member_self_update_columns.sql20260326083158_restrict_access_config_member_view.sql20260326090126_atomic_remove_platform_admin.sql20260326091259_add_booking_conflict_check_to_rpc.sqlTest plan
supabase db resetto verify all migrations apply cleanly/accesspage as a member — should show access codes via RPCproxy.ts(nomiddleware.ts)🤖 Generated with Claude Code