[codex] Harden encrypted bundle update invariant#2015
Merged
Conversation
Contributor
|
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 PostgreSQL trigger function ChangesEncrypted Bundle Validation Trigger
Tests, Helpers & Versioning Flow
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API
participant DB as DB
participant Apps as "public.apps"
participant Orgs as "public.orgs"
Client->>API: INSERT/UPDATE app_versions (NEW)
API->>DB: SQL executes -> BEFORE trigger fires
DB->>Apps: SELECT org_id FROM public.apps WHERE id = NEW.app_id
Apps-->>DB: return org_id (or null)
DB->>Orgs: SELECT enforce_encrypted_bundles, required_encryption_key FROM public.orgs WHERE id = org_id
Orgs-->>DB: return policy
DB->>DB: evaluate bundle_was_ready & is_bundle_encrypted(NEW.session_key)
DB->>DB: if required key -> compare NEW.key_id with required prefix/length-tolerant match
alt validation fails
DB-->>API: RAISE EXCEPTION (encryption_required / key_mismatch / bundle_already_ready)
API-->>Client: error
else validation passes
DB-->>API: RETURN NEW
API-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
riderx
force-pushed
the
codex/fix-encrypted-bundle-update-invariant
branch
from
b1fa38e to
365b15a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/enforce-encrypted-bundles.test.ts (1)
445-492: ⚡ Quick winReset org enforcement in
finallyblocks for these cases.Each test enables
enforce_encrypted_bundleson the shared org and disables it only on the success path. If an assertion or insert fails early, the rest of the file inherits the wrong org state and starts failing for secondary reasons. Wrapping the enable/disable pair intry/finallywould keep failures local.Also applies to: 494-545, 547-611, 613-668
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/enforce-encrypted-bundles.test.ts` around lines 445 - 492, The test enables org-level enforcement via getSupabaseClient().from('orgs').update({ enforce_encrypted_bundles: true }) but only disables it on the success path, which can leak state when assertions or inserts fail; wrap the enable/disable pair in a try/finally in the test "should reject direct update that changes session_key after the bundle is ready" (and the other affected tests at the ranges noted) so that after enabling with ORG_ID_ENCRYPTED you always reset enforce_encrypted_bundles to false in the finally block; locate the setup calls using ORG_ID_ENCRYPTED and getSupabaseClient()/from('orgs') and move the disabling update into finally to guarantee state cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sql`:
- Around line 52-61: Current logic trusts NEW.owner_org before consulting the
apps table which can be stale due to trigger ordering; change the resolution
order so the code first SELECTs apps.owner_org INTO org_id FROM public.apps
WHERE apps.app_id = NEW.app_id, and only if that SELECT yields NULL (no matching
app) fall back to using NEW.owner_org; update the block that assigns org_id (the
IF/ELSE around NEW.owner_org and the SELECT) accordingly so org_id is derived
from NEW.app_id first and reference force_valid_owner_org_app_versions in the
comment if helpful.
In `@tests/enforce-encrypted-bundles.test.ts`:
- Around line 149-153: Don't mutate the shared seeded API key (APIKEY_ENCRYPTED)
in the suite setup/teardown; instead, use getSupabaseClient() to INSERT a
dedicated test API key row (set limited_to_apps to [APP_NAME_ENCRYPTED]) for
this test file and store that new key value in a suite-local constant, use that
constant in assertions, and DELETE that row in afterAll to clean up; also
convert tests in this file to it.concurrent() where appropriate so files can run
in parallel. Ensure you reference the created fixture key rather than updating
APIKEY_ENCRYPTED and remove the update calls shown calling
.from('apikeys').update(...) .
---
Nitpick comments:
In `@tests/enforce-encrypted-bundles.test.ts`:
- Around line 445-492: The test enables org-level enforcement via
getSupabaseClient().from('orgs').update({ enforce_encrypted_bundles: true }) but
only disables it on the success path, which can leak state when assertions or
inserts fail; wrap the enable/disable pair in a try/finally in the test "should
reject direct update that changes session_key after the bundle is ready" (and
the other affected tests at the ranges noted) so that after enabling with
ORG_ID_ENCRYPTED you always reset enforce_encrypted_bundles to false in the
finally block; locate the setup calls using ORG_ID_ENCRYPTED and
getSupabaseClient()/from('orgs') and move the disabling update into finally to
guarantee state cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7c3a6da-2768-4acd-8bdd-11e3c907b04c
📒 Files selected for processing (2)
supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sqltests/enforce-encrypted-bundles.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sql (1)
110-114: 💤 Low valueSimplify the key comparison logic.
The second condition is redundant: when
org_required_keyis 20 chars, it duplicates condition 1; when 21 chars, it can never match sincebundle_key_idis always 20 chars. Since the database constraint ensuresrequired_encryption_keyis exactly 20 or 21 chars, the comparison can be simplified to just check the first 20 characters of the org key:IF NOT bundle_key_id = LEFT(org_required_key, 20) THEN🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sql` around lines 110 - 114, The existing IF condition compares bundle_key_id against org_required_key with two branches; simplify it by replacing the OR expression with a single comparison that checks the first 20 characters of org_required_key: use bundle_key_id = LEFT(org_required_key, 20) (remove the LEFT(bundle_key_id, LENGTH(org_required_key)) branch and LENGTH usage) so the IF becomes IF NOT bundle_key_id = LEFT(org_required_key, 20) THEN, relying on the enforced 20/21 char invariant of org_required_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sql`:
- Around line 110-114: The existing IF condition compares bundle_key_id against
org_required_key with two branches; simplify it by replacing the OR expression
with a single comparison that checks the first 20 characters of
org_required_key: use bundle_key_id = LEFT(org_required_key, 20) (remove the
LEFT(bundle_key_id, LENGTH(org_required_key)) branch and LENGTH usage) so the IF
becomes IF NOT bundle_key_id = LEFT(org_required_key, 20) THEN, relying on the
enforced 20/21 char invariant of org_required_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e814ac91-0ebf-4555-b7b1-0ff102822a39
📒 Files selected for processing (4)
supabase/migrations/20260505193449_harden_encrypted_bundle_update_invariant.sqltests/app.test.tstests/enforce-encrypted-bundles.test.tstests/files-security.test.ts
…undle-update-invariant # Conflicts: # tests/files-security.test.ts
|
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 (AI generated)
Motivation (AI generated)
The security advisory GHSA-4qwf-mrgx-mvfx reported that encrypted-bundle enforcement was bypassable after insert by directly clearing app_versions.session_key. This keeps the invariant enforced on update as well.
Business Impact (AI generated)
This preserves an organization administrator's encrypted-bundle policy and prevents scoped API keys from weakening OTA bundle protection metadata after upload.
Test Plan (AI generated)
Generated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Tests