Update from task 2f67eb41-8e6b-46ce-b795-65c5e4ad925f#1
Open
saksham1991999 wants to merge 2 commits into
Open
Conversation
Key features implemented: - Fixed critical authentication token validation vulnerability in repository layer to prevent RLS bypass attacks - Enhanced Supabase client initialization with explicit token validation and secure fallback handling - Updated database schema to consolidate duplicate ENUM values and ensure data consistency - Improved error handling and logging for repository operations with detailed context - Added comprehensive table indexing strategy for optimized query performance - Enhanced row-level security configuration for all application tables - Implemented robust dependency injection registry for repository testing - Added detailed database schema documentation and comments - Fixed concurrency control with semaphore limiting for database operations The changes address critical security vulnerabilities while improving data consistency, performance, and maintainability across the repository layer and database schema.
📝 WalkthroughWalkthroughConsolidated .gitignore additions; repository layer now fails fast when building token-scoped clients (no admin fallback); small API updates switching status value from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Review Summary by QodoFix authentication vulnerability and improve data consistency
WalkthroughsDescription• Fixed critical authentication token validation vulnerability preventing RLS bypass attacks • Enhanced error handling in repository layer with explicit exception raising instead of silent fallback • Consolidated duplicate ENUM values in database schema for data consistency • Added optimized database indexes for improved query performance Diagramflowchart LR
A["Token Validation"] -->|"Exception on Failure"| B["Raise RepositoryError"]
B -->|"Prevent RLS Bypass"| C["Enhanced Security"]
D["Database Schema"] -->|"Remove Duplicate ENUM"| E["Data Consistency"]
F["Add Indexes"] -->|"Optimize Queries"| G["Performance"]
File Changes1. madnis-media/apps/backend/app/db/repository.py
|
Code Review by Qodo
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .vscode/ | ||
| .idea/ | ||
| *.swp | ||
| *.swo |
There was a problem hiding this comment.
🔴 .gitignore contains markdown code fence artifacts making it malformed
The .gitignore file starts (line 1) and ends (line 71) with triple backticks (```), which are markdown code fence markers accidentally included. While these patterns won't match real files, they indicate the content was copy-pasted from a markdown block without removing the fences. The file is also missing several project-specific ignore patterns that existed before (e.g., .claude/, logs/, storage/, video-creator-workspace/) which may cause previously-ignored files to be tracked.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
45
to
50
| CREATE TYPE message_role AS ENUM ( | ||
| 'user', | ||
| 'assistant', | ||
| 'system' | ||
| 'system', | ||
| 'ai' | ||
| ); |
There was a problem hiding this comment.
1. Edited 001_core_schema.sql migration 📘 Rule violation ☼ Reliability
The PR modifies an existing Supabase migration file instead of appending a new migration, which breaks the deterministic migration chain as the source of truth. Schema changes (e.g., new ENUM values and indexes) should be introduced in a new migration file under madnis-media/supabase/migrations/.
Agent Prompt
## Issue description
`madnis-media/supabase/migrations/001_core_schema.sql` was edited to change schema (ENUM values / indexes). The migration chain should remain deterministic by appending new migration files rather than modifying existing ones.
## Issue Context
Create a new migration file (next sequence number) that applies the intended changes using `ALTER TYPE ... ADD VALUE` (or an equivalent safe pattern) and `CREATE INDEX ...`, and revert `001_core_schema.sql` back to its previous committed state.
## Fix Focus Areas
- madnis-media/supabase/migrations/001_core_schema.sql[45-50]
- madnis-media/supabase/migrations/001_core_schema.sql[71-84]
- madnis-media/supabase/migrations/001_core_schema.sql[295-301]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
75
to
80
| 'paused', | ||
| 'generating', | ||
| 'processing', | ||
| 'complete', | ||
| 'completed', | ||
| 'approved', | ||
| 'rejected', |
There was a problem hiding this comment.
2. Enum 'complete' removed 🐞 Bug ≡ Correctness
supabase/migrations/001_core_schema.sql removes 'complete' from generation_status, but backend code/tests still set and assert status='complete', so fresh databases will reject writes and the migration contract test will fail.
Agent Prompt
## Issue description
The baseline migration `001_core_schema.sql` defines `generation_status` without `'complete'`, but the backend and tests still use `'complete'`. This will break fresh DB setups (enum insert/update failures) and fails `test_generation_status_enum_covers_all_expected_values`.
## Issue Context
- Backend uses both `GenerationStatus.complete` and `GenerationStatus.completed`.
- Tests treat `001_core_schema.sql` as a contract for expected enum values.
## Fix Focus Areas
- madnis-media/supabase/migrations/001_core_schema.sql[71-84]
- madnis-media/apps/backend/tests/test_migration_contracts.py[11-67]
- madnis-media/apps/backend/app/models/domain.py[103-137]
## Expected fix
- Put `'complete'` back into `generation_status` in `001_core_schema.sql` (keeping backward compatibility).
- If you want to deprecate `'complete'`, do it at the application layer (stop emitting it) and/or add a forward data-migration to normalize values, but do not remove the enum label from the baseline schema while code/tests still rely on it.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="madnis-media/supabase/migrations/001_core_schema.sql">
<violation number="1" location="madnis-media/supabase/migrations/001_core_schema.sql:301">
P2: This index is being added to an already-versioned baseline migration, so existing databases will not get it. Add this index in a new forward migration to avoid schema drift.</violation>
</file>
<file name=".gitignore">
<violation number="1" location=".gitignore:1">
P3: Remove Markdown code-fence lines from `.gitignore`; they are being committed as literal ignore patterns.</violation>
<violation number="2" location=".gitignore:36">
P2: Re-add `logs/` and `storage/` directory ignores; `*.log` alone does not prevent generated output directories from being committed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| CREATE INDEX idx_assets_approved ON assets(approved); | ||
| CREATE INDEX idx_assets_quality_score ON assets(quality_score); | ||
| CREATE INDEX idx_assets_project_file_size ON assets(project_id, ((metadata->>'file_size'))); | ||
| CREATE INDEX idx_assets_project_status_updated ON assets(project_id, ((metadata->>'status')), updated_at DESC); |
There was a problem hiding this comment.
P2: This index is being added to an already-versioned baseline migration, so existing databases will not get it. Add this index in a new forward migration to avoid schema drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At madnis-media/supabase/migrations/001_core_schema.sql, line 301:
<comment>This index is being added to an already-versioned baseline migration, so existing databases will not get it. Add this index in a new forward migration to avoid schema drift.</comment>
<file context>
@@ -298,6 +298,7 @@ CREATE INDEX idx_assets_asset_type ON assets(asset_type);
CREATE INDEX idx_assets_approved ON assets(approved);
CREATE INDEX idx_assets_quality_score ON assets(quality_score);
CREATE INDEX idx_assets_project_file_size ON assets(project_id, ((metadata->>'file_size')));
+CREATE INDEX idx_assets_project_status_updated ON assets(project_id, ((metadata->>'status')), updated_at DESC);
-- ============================================================================
</file context>
| .env.* | ||
|
|
||
| # Logs | ||
| *.log |
There was a problem hiding this comment.
P2: Re-add logs/ and storage/ directory ignores; *.log alone does not prevent generated output directories from being committed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .gitignore, line 36:
<comment>Re-add `logs/` and `storage/` directory ignores; `*.log` alone does not prevent generated output directories from being committed.</comment>
<file context>
@@ -1,15 +1,87 @@
+.env.*
+
+# Logs
+*.log
+
+# Coverage
</file context>
Suggested change
| *.log | |
| *.log | |
| logs/ | |
| storage/ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
madnis-media/apps/backend/app/api/routes/assets.py (1)
416-422:⚠️ Potential issue | 🟠 MajorAdd magic-byte validation before setting
completedinupload_asset.At Line 421, this path marks the asset as
completedwithout signature validation. Unlike the project upload flow (Lines 547-555), this allows content-type spoofed files to be persisted as successful uploads.Suggested fix
logger.debug( "Upload content stored", extra={"asset_id": str(asset_id), "bytes": bytes_written, "content_type": content_type}, ) + # Validate file signature before marking upload complete. + try: + _validate_file_magic_bytes(target_file, content_type) + except Exception: + if target_file.exists(): + try: + target_file.unlink() + except OSError: + pass + await asset_repo.update_asset(asset_id, {"status": GenerationStatus.failed.value}) + raise + try: updated = await asset_repo.update_asset( asset_id, { "file_url": str(target_file), "status": GenerationStatus.completed.value,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@madnis-media/apps/backend/app/api/routes/assets.py` around lines 416 - 422, The upload_asset path currently sets GenerationStatus.completed when calling asset_repo.update_asset with file_url/status/metadata without validating file magic bytes; update upload_asset to perform the same signature/magic-byte validation used in the project upload flow (the logic around Lines 547-555) before calling asset_repo.update_asset — if the magic-byte check fails, do not set status to GenerationStatus.completed and instead mark failed or raise; ensure you reference and reuse the existing validation helper/function (or inline the same checks) so asset_repo.update_asset is only invoked with completed after successful magic-byte validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@madnis-media/apps/backend/app/api/routes/assets.py`:
- Around line 416-422: The upload_asset path currently sets
GenerationStatus.completed when calling asset_repo.update_asset with
file_url/status/metadata without validating file magic bytes; update
upload_asset to perform the same signature/magic-byte validation used in the
project upload flow (the logic around Lines 547-555) before calling
asset_repo.update_asset — if the magic-byte check fails, do not set status to
GenerationStatus.completed and instead mark failed or raise; ensure you
reference and reuse the existing validation helper/function (or inline the same
checks) so asset_repo.update_asset is only invoked with completed after
successful magic-byte validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c14d83f-ddf3-414f-b186-a703ac784ef5
📒 Files selected for processing (5)
.gitignoremadnis-media/apps/backend/app/api/routes/assets.pymadnis-media/apps/backend/app/api/routes/export.pymadnis-media/supabase/migrations/009_generation_status_complete.sqlmadnis-media/supabase/migrations/010_add_missing_indexes.sql
✅ Files skipped from review due to trivial changes (2)
- madnis-media/supabase/migrations/010_add_missing_indexes.sql
- madnis-media/supabase/migrations/009_generation_status_complete.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="madnis-media/supabase/migrations/010_add_missing_indexes.sql">
<violation number="1" location="madnis-media/supabase/migrations/010_add_missing_indexes.sql:6">
P0: This index references a non-existent `assets.status` column, which will cause the migration to fail.</violation>
<violation number="2" location="madnis-media/supabase/migrations/010_add_missing_indexes.sql:14">
P0: This index uses `shots.scene_id`, but that column is not defined in the schema, so the migration will fail.</violation>
</file>
<file name="madnis-media/supabase/migrations/009_generation_status_complete.sql">
<violation number="1" location="madnis-media/supabase/migrations/009_generation_status_complete.sql:12">
P2: Adding `complete` to `generation_status` without updating DB aggregations breaks equivalence: completed-shot stats will undercount rows stored as `complete`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| -- Index for shots by project and scene (common in shot queries) | ||
| CREATE INDEX IF NOT EXISTS idx_shots_project_scene | ||
| ON shots(project_id, scene_id); |
There was a problem hiding this comment.
P0: This index uses shots.scene_id, but that column is not defined in the schema, so the migration will fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At madnis-media/supabase/migrations/010_add_missing_indexes.sql, line 14:
<comment>This index uses `shots.scene_id`, but that column is not defined in the schema, so the migration will fail.</comment>
<file context>
@@ -0,0 +1,34 @@
+
+-- Index for shots by project and scene (common in shot queries)
+CREATE INDEX IF NOT EXISTS idx_shots_project_scene
+ON shots(project_id, scene_id);
+
+-- Index for conversation messages by project and role (used in chat history)
</file context>
|
|
||
| -- Composite index for filtering assets by project and status (common in pipeline queries) | ||
| CREATE INDEX IF NOT EXISTS idx_assets_project_status | ||
| ON assets(project_id, status); |
There was a problem hiding this comment.
P0: This index references a non-existent assets.status column, which will cause the migration to fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At madnis-media/supabase/migrations/010_add_missing_indexes.sql, line 6:
<comment>This index references a non-existent `assets.status` column, which will cause the migration to fail.</comment>
<file context>
@@ -0,0 +1,34 @@
+
+-- Composite index for filtering assets by project and status (common in pipeline queries)
+CREATE INDEX IF NOT EXISTS idx_assets_project_status
+ON assets(project_id, status);
+
+-- Composite index for assets by project and asset_type (used in asset listing)
</file context>
| WHERE enumlabel = 'complete' | ||
| AND enumtypid = 'generation_status'::regtype | ||
| ) THEN | ||
| ALTER TYPE generation_status ADD VALUE 'complete'; |
There was a problem hiding this comment.
P2: Adding complete to generation_status without updating DB aggregations breaks equivalence: completed-shot stats will undercount rows stored as complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At madnis-media/supabase/migrations/009_generation_status_complete.sql, line 12:
<comment>Adding `complete` to `generation_status` without updating DB aggregations breaks equivalence: completed-shot stats will undercount rows stored as `complete`.</comment>
<file context>
@@ -0,0 +1,17 @@
+ WHERE enumlabel = 'complete'
+ AND enumtypid = 'generation_status'::regtype
+ ) THEN
+ ALTER TYPE generation_status ADD VALUE 'complete';
+ END IF;
+END $$;
</file context>
Comment on lines
+13
to
+14
| CREATE INDEX IF NOT EXISTS idx_shots_project_scene | ||
| ON shots(project_id, scene_id); |
There was a problem hiding this comment.
🔴 Migration 010 creates index on non-existent scene_id column in shots table
The shots table (defined in 001_core_schema.sql:307-339) has no scene_id column — it has shot_id, location_id, etc., but never scene_id. No subsequent migration (002–009) adds this column either. The statement CREATE INDEX IF NOT EXISTS idx_shots_project_scene ON shots(project_id, scene_id) will fail with a PostgreSQL error at deployment time, blocking all subsequent migrations including the other indexes in this file.
Prompt for agents
The shots table (defined in madnis-media/supabase/migrations/001_core_schema.sql lines 307-339) does not have a scene_id column. The available columns include shot_id, shot_type, location_id, status, order_index, etc. Either this index should reference a different column (e.g. shot_id or location_id), or it should be removed entirely. Verify which column is actually used in common query patterns for shots by project and scene, and update or remove the index accordingly.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+5
to
+6
| CREATE INDEX IF NOT EXISTS idx_assets_project_status | ||
| ON assets(project_id, status); |
There was a problem hiding this comment.
🔴 Migration 010 creates index on non-existent status column in assets table
The assets table (defined in 001_core_schema.sql:274-293) has no status column. Asset status is stored inside the metadata JSONB column, as confirmed by AssetRepository.update_asset() at madnis-media/apps/backend/app/db/repository.py:948-950 which writes status into metadata["status"]. The statement CREATE INDEX IF NOT EXISTS idx_assets_project_status ON assets(project_id, status) will fail with a PostgreSQL error at deployment time, blocking this and all subsequent migrations.
Suggested change
| CREATE INDEX IF NOT EXISTS idx_assets_project_status | |
| ON assets(project_id, status); | |
| CREATE INDEX IF NOT EXISTS idx_assets_project_status | |
| ON assets(project_id, ((metadata->>'status'))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
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
2 participants
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.
This PR was created by qwen-chat coder for task 2f67eb41-8e6b-46ce-b795-65c5e4ad925f.
Summary by cubic
Fixes an auth gap by enforcing token validation to prevent RLS bypass and standardizes status handling to use "completed". Adds back-compat for "complete", performance indexes, and expands
.gitignore.Bug Fixes
RepositoryErroron failure with contextual logging.GenerationStatus.completedin asset upload and export routes.Migration
aitomessage_role; support bothcompleteandcompletedingeneration_status; addidx_assets_project_status_updatedand new composite indexes across assets, shots, conversation_messages, pipeline_audit_log, and exports.completed—legacycompleteremains accepted.Written for commit f010f36. Summary will update on new commits.