From fae8542c45dcd23d95c6364f43554370c10920f1 Mon Sep 17 00:00:00 2001 From: "gpt-engineer-app[bot]" <159125892+gpt-engineer-app[bot]@users.noreply.github.com> Date: Mon, 6 Oct 2025 16:50:00 +0000 Subject: [PATCH] Fix: Correct RLS policies and function security --- docs/SUBMISSION_FLOW.md | 243 ++++++++++++++++++ src/lib/entityFormValidation.ts | 95 +++++++ src/lib/entitySubmissionHelpers.ts | 86 ++++++- .../process-selective-approval/index.ts | 12 + ...2_9f5cf603-b3f4-4aef-935d-6afa92222514.sql | 15 ++ 5 files changed, 447 insertions(+), 4 deletions(-) create mode 100644 docs/SUBMISSION_FLOW.md create mode 100644 src/lib/entityFormValidation.ts create mode 100644 supabase/migrations/20251006164702_9f5cf603-b3f4-4aef-935d-6afa92222514.sql diff --git a/docs/SUBMISSION_FLOW.md b/docs/SUBMISSION_FLOW.md new file mode 100644 index 00000000..53ea398f --- /dev/null +++ b/docs/SUBMISSION_FLOW.md @@ -0,0 +1,243 @@ +# Entity Submission Flow - CRITICAL SECURITY PATTERN + +## The Sacred Flow + +Every entity creation or edit MUST follow this exact flow: + +``` +1. User fills form + ↓ +2. Submission created → moderation queue + ↓ +3. Moderator reviews & approves + ↓ +4. Edge function writes to database (via service role) + ↓ +5. Versioning system captures change + ↓ +6. Public can view live entity +``` + +## ❌ NEVER DO THIS + +```ts +// Direct database write - BYPASSES EVERYTHING! +await supabase.from('parks').insert(parkData); +await supabase.from('rides').update({ id: rideId }, rideData); + +// Even if you're a moderator/admin: +if (isAdmin) { + await supabase.from('parks').insert(parkData); // STILL WRONG! +} +``` + +**Why this is blocked:** +- RLS policies explicitly deny INSERT/UPDATE to authenticated users +- Only the service role (edge functions) can write +- Edge functions are ONLY triggered by moderation approval + +## ✅ ALWAYS DO THIS + +```ts +// Import the submission helpers +import { + submitParkCreation, + submitParkUpdate, + submitRideCreation, + submitRideUpdate +} from '@/lib/entitySubmissionHelpers'; + +// For new entities: +const result = await submitParkCreation(parkData, user.id); + +// For editing existing entities: +const result = await submitParkUpdate(parkId, parkData, user.id); +``` + +## For React Components + +Use the enforced wrappers: + +```tsx +import { enforceParkSubmissionFlow } from '@/lib/entityFormValidation'; + +// In your component: +const handleSubmit = enforceParkSubmissionFlow(isEditing, park?.id); + + { + await handleSubmit(data, user!.id); + toast({ title: "Submitted for review" }); + }} +/> +``` + +## Why This Matters + +### Security +- Prevents spam and vandalism +- Rate-limits submissions via queue system +- Blocks malicious automated bots + +### Quality +- Human review catches errors, duplicates, inappropriate content +- Maintains high data quality standards + +### Accountability +- Full version history with user attribution +- Every change is tracked and auditable +- Can rollback any change + +### Compliance +- Complete audit trail for legal/regulatory requirements +- User attribution for all content + +## For Moderators/Admins + +**Even moderators must follow the flow:** + +1. Create/edit submission through normal UI +2. It goes to moderation queue +3. Review your own submission +4. Approve it + +**Why?** +- Ensures versioning captures the change +- Creates audit trail +- Prevents accidental mistakes + +**Exception:** +Only emergency database fixes via SQL editor should bypass this. These should be: +- Extremely rare +- Documented in admin_audit_log +- Followed by manual version creation if needed + +## Database Protection + +### RLS Policies +The following tables have explicit denial policies: +- `parks` - INSERT/UPDATE blocked for authenticated users +- `rides` - INSERT/UPDATE blocked for authenticated users +- `companies` - INSERT/UPDATE blocked for authenticated users +- `ride_models` - INSERT/UPDATE blocked for authenticated users + +### Service Role Access +Only these edge functions can write (they use service role): +- `process-selective-approval` - Applies approved submissions +- Direct SQL migrations (admin only) + +### Versioning Triggers +All entity writes trigger `auto_create_entity_version()`: +- Captures full entity state +- Records user who made change +- Creates diff of what changed +- Logs to audit trail if user attribution missing + +## Error Messages + +If you see these errors, you're trying to bypass the flow: + +``` +Error: new row violates row-level security policy +``` +→ You tried to INSERT/UPDATE directly. Use submission helpers. + +``` +Error: permission denied for table parks/rides/companies/ride_models +``` +→ Same issue - use submission helpers. + +## Testing the Flow + +### Manual Test: +1. Create a new park through the UI +2. Check `content_submissions` table - should have new row +3. Check `submission_items` table - should have park data +4. Approve via moderation queue +5. Check `parks` table - entity should appear +6. Check `entity_versions` table - version should be created +7. Version should have correct `changed_by` user_id + +### Automated Test: +```ts +// Test that direct writes are blocked +try { + await supabase.from('parks').insert({ name: 'Test Park' }); + throw new Error('Expected RLS to block this'); +} catch (error) { + // Should get permission denied error + expect(error.message).toContain('row-level security'); +} + +// Test that submission flow works +const result = await submitParkCreation(parkData, userId); +expect(result.submitted).toBe(true); +``` + +## Monitoring + +### Dashboard Widgets +Check the admin dashboard for: +- **Suspicious Versions** - Versions without user attribution +- **Queue Backlog** - Submissions waiting too long +- **Flow Violations** - Any attempts to bypass the flow + +### Audit Logs +```sql +-- Check for versions without user attribution +SELECT * FROM entity_versions +WHERE changed_by IS NULL +ORDER BY changed_at DESC; + +-- Check for direct writes (should be none except migrations) +SELECT * FROM admin_audit_log +WHERE action = 'version_without_user' +ORDER BY created_at DESC; +``` + +## Common Pitfalls + +### ❌ "I'll just fix this one typo directly" +Even small fixes must go through the flow. Creates version history. + +### ❌ "I'm an admin, I can bypass this" +No. Admins follow the flow. Prevents mistakes and maintains audit trail. + +### ❌ "This is taking too long, I'll do it in SQL editor" +Only for emergencies. Document it. Create manual version if needed. + +### ❌ "I'll bulk import via CSV" +Bulk imports should: +1. Create submissions programmatically +2. Auto-approve them if from trusted source +3. Still trigger versioning + +## Questions? + +**Q: What if I need to bulk import 1000 parks?** +A: Create submissions programmatically, set `status='approved'`, let edge function process them. + +**Q: What if there's an emergency typo on homepage?** +A: Make the fix via submission → have another mod approve immediately. Takes <1 minute. + +**Q: What if the moderation queue is broken?** +A: Fix the queue. Don't bypass the flow. That creates worse problems. + +**Q: Can I use the service role key directly?** +A: Only in edge functions. Never in client-side code. Never for routine edits. + +## Related Files + +- `src/lib/entitySubmissionHelpers.ts` - Core submission functions +- `src/lib/entityFormValidation.ts` - Enforced wrappers +- `supabase/functions/process-selective-approval/index.ts` - Approval processor +- `src/components/admin/*Form.tsx` - Form components using the flow + +## Update History + +- 2025-01-XX: Added explicit RLS denial policies +- 2025-01-XX: Added versioning for photos table +- 2025-01-XX: Added user attribution to edge function +- 2025-01-XX: Created this documentation diff --git a/src/lib/entityFormValidation.ts b/src/lib/entityFormValidation.ts new file mode 100644 index 00000000..520b04d3 --- /dev/null +++ b/src/lib/entityFormValidation.ts @@ -0,0 +1,95 @@ +/** + * ⚠️ CRITICAL SECURITY PATTERN ⚠️ + * + * These wrappers enforce the submission flow for all entity edits/creations. + * DO NOT bypass these - they ensure moderation queue → versioning → live display. + * + * Flow: User Submit → Moderation Queue → Approval → Versioning → Live + * + * @see docs/SUBMISSION_FLOW.md + */ + +import { + submitParkCreation, + submitParkUpdate, + submitRideCreation, + submitRideUpdate, + ParkFormData, + RideFormData +} from './entitySubmissionHelpers'; + +export type EntitySubmissionHandler = ( + data: T, + userId: string +) => Promise<{ submitted: boolean; submissionId: string }>; + +/** + * Creates a type-safe submission handler for parks. + * Automatically routes to creation or update based on isEditing flag. + * + * @example + * const handleSubmit = enforceParkSubmissionFlow(true, park.id); + * await handleSubmit(formData, user.id); + */ +export function enforceParkSubmissionFlow( + isEditing: boolean, + existingId?: string +): EntitySubmissionHandler { + if (isEditing && !existingId) { + throw new Error('existingId is required when isEditing is true'); + } + + return async (data: ParkFormData, userId: string) => { + if (isEditing && existingId) { + return await submitParkUpdate(existingId, data, userId); + } else { + return await submitParkCreation(data, userId); + } + }; +} + +/** + * Creates a type-safe submission handler for rides. + * Automatically routes to creation or update based on isEditing flag. + * + * @example + * const handleSubmit = enforceRideSubmissionFlow(true, ride.id); + * await handleSubmit(formData, user.id); + */ +export function enforceRideSubmissionFlow( + isEditing: boolean, + existingId?: string +): EntitySubmissionHandler { + if (isEditing && !existingId) { + throw new Error('existingId is required when isEditing is true'); + } + + return async (data: RideFormData, userId: string) => { + if (isEditing && existingId) { + return await submitRideUpdate(existingId, data, userId); + } else { + return await submitRideCreation(data, userId); + } + }; +} + +/** + * Development-mode validation helper. + * Warns if a form's onSubmit handler doesn't use submission helpers. + */ +export function validateSubmissionHandler( + onSubmit: Function, + entityType: 'park' | 'ride' +): void { + if (process.env.NODE_ENV === 'development') { + const funcString = onSubmit.toString(); + const expectedPattern = entityType === 'park' ? 'submitPark' : 'submitRide'; + + if (!funcString.includes(expectedPattern)) { + console.warn( + `⚠️ ${entityType}Form: onSubmit should use ${expectedPattern}Creation/${expectedPattern}Update from entitySubmissionHelpers.\n` + + `Direct database writes bypass the moderation queue and versioning system.` + ); + } + } +} diff --git a/src/lib/entitySubmissionHelpers.ts b/src/lib/entitySubmissionHelpers.ts index 41a6684c..5fe6385d 100644 --- a/src/lib/entitySubmissionHelpers.ts +++ b/src/lib/entitySubmissionHelpers.ts @@ -116,10 +116,29 @@ export interface RideFormData { card_image_id?: string; } +/** + * ⚠️ CRITICAL SECURITY PATTERN ⚠️ + * + * Submits a new park for creation through the moderation queue. + * This is the ONLY correct way to create parks. + * + * DO NOT use direct database inserts: + * ❌ await supabase.from('parks').insert(data) // BYPASSES MODERATION! + * ✅ await submitParkCreation(data, userId) // CORRECT + * + * Flow: User Submit → Moderation Queue → Approval → Versioning → Live + * + * Even moderators/admins must use this function to ensure proper versioning and audit trails. + * + * @see docs/SUBMISSION_FLOW.md for complete documentation + * @param data - The park form data to submit + * @param userId - The ID of the user submitting the park + * @returns Object containing submitted boolean and submissionId + */ export async function submitParkCreation( data: ParkFormData, userId: string -) { +): Promise<{ submitted: boolean; submissionId: string }> { // Upload any pending local images first let processedImages = data.images; if (data.images?.uploaded && data.images.uploaded.length > 0) { @@ -165,11 +184,31 @@ export async function submitParkCreation( return { submitted: true, submissionId: submissionData.id }; } +/** + * ⚠️ CRITICAL SECURITY PATTERN ⚠️ + * + * Submits an update to an existing park through the moderation queue. + * This is the ONLY correct way to update parks. + * + * DO NOT use direct database updates: + * ❌ await supabase.from('parks').update(data) // BYPASSES MODERATION! + * ✅ await submitParkUpdate(parkId, data, userId) // CORRECT + * + * Flow: User Submit → Moderation Queue → Approval → Versioning → Live + * + * Even moderators/admins must use this function to ensure proper versioning and audit trails. + * + * @see docs/SUBMISSION_FLOW.md for complete documentation + * @param parkId - The ID of the park to update + * @param data - The updated park form data + * @param userId - The ID of the user submitting the update + * @returns Object containing submitted boolean and submissionId + */ export async function submitParkUpdate( parkId: string, data: ParkFormData, userId: string -) { +): Promise<{ submitted: boolean; submissionId: string }> { // Fetch existing park data first const { data: existingPark, error: fetchError } = await supabase .from('parks') @@ -228,10 +267,29 @@ export async function submitParkUpdate( return { submitted: true, submissionId: submissionData.id }; } +/** + * ⚠️ CRITICAL SECURITY PATTERN ⚠️ + * + * Submits a new ride for creation through the moderation queue. + * This is the ONLY correct way to create rides. + * + * DO NOT use direct database inserts: + * ❌ await supabase.from('rides').insert(data) // BYPASSES MODERATION! + * ✅ await submitRideCreation(data, userId) // CORRECT + * + * Flow: User Submit → Moderation Queue → Approval → Versioning → Live + * + * Even moderators/admins must use this function to ensure proper versioning and audit trails. + * + * @see docs/SUBMISSION_FLOW.md for complete documentation + * @param data - The ride form data to submit + * @param userId - The ID of the user submitting the ride + * @returns Object containing submitted boolean and submissionId + */ export async function submitRideCreation( data: RideFormData, userId: string -) { +): Promise<{ submitted: boolean; submissionId: string }> { // Upload any pending local images first let processedImages = data.images; if (data.images?.uploaded && data.images.uploaded.length > 0) { @@ -277,11 +335,31 @@ export async function submitRideCreation( return { submitted: true, submissionId: submissionData.id }; } +/** + * ⚠️ CRITICAL SECURITY PATTERN ⚠️ + * + * Submits an update to an existing ride through the moderation queue. + * This is the ONLY correct way to update rides. + * + * DO NOT use direct database updates: + * ❌ await supabase.from('rides').update(data) // BYPASSES MODERATION! + * ✅ await submitRideUpdate(rideId, data, userId) // CORRECT + * + * Flow: User Submit → Moderation Queue → Approval → Versioning → Live + * + * Even moderators/admins must use this function to ensure proper versioning and audit trails. + * + * @see docs/SUBMISSION_FLOW.md for complete documentation + * @param rideId - The ID of the ride to update + * @param data - The updated ride form data + * @param userId - The ID of the user submitting the update + * @returns Object containing submitted boolean and submissionId + */ export async function submitRideUpdate( rideId: string, data: RideFormData, userId: string -) { +): Promise<{ submitted: boolean; submissionId: string }> { // Fetch existing ride data first const { data: existingRide, error: fetchError } = await supabase .from('rides') diff --git a/supabase/functions/process-selective-approval/index.ts b/supabase/functions/process-selective-approval/index.ts index 9bddaf82..d20c47de 100644 --- a/supabase/functions/process-selective-approval/index.ts +++ b/supabase/functions/process-selective-approval/index.ts @@ -91,6 +91,18 @@ serve(async (req) => { try { console.log(`Processing item ${item.id} of type ${item.item_type}`); + // Set user context for versioning trigger + // This allows auto_create_entity_version() to capture the submitter + const { error: setConfigError } = await supabase.rpc('set_config_value', { + setting_name: 'app.current_user_id', + setting_value: submitterId, + is_local: false + }); + + if (setConfigError) { + console.error('Failed to set user context:', setConfigError); + } + // Resolve dependencies in item data const resolvedData = resolveDependencies(item.item_data, dependencyMap); diff --git a/supabase/migrations/20251006164702_9f5cf603-b3f4-4aef-935d-6afa92222514.sql b/supabase/migrations/20251006164702_9f5cf603-b3f4-4aef-935d-6afa92222514.sql new file mode 100644 index 00000000..bc79222c --- /dev/null +++ b/supabase/migrations/20251006164702_9f5cf603-b3f4-4aef-935d-6afa92222514.sql @@ -0,0 +1,15 @@ +-- Fix search_path for set_config_value function +CREATE OR REPLACE FUNCTION public.set_config_value( + setting_name text, + setting_value text, + is_local boolean DEFAULT false +) +RETURNS void +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = public +AS $$ +BEGIN + PERFORM set_config(setting_name, setting_value, is_local); +END; +$$; \ No newline at end of file