refactor: clean up upload integration test — expectRlsDenied, expectSuccess, DRY SQL#1098
Merged
pyramation merged 2 commits intofeat/rls-alice-bob-integration-testfrom May 10, 2026
Conversation
…uccess, superuser verification, DRY SQL
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…onnection isolation concern)
d9ecaf9
into
feat/rls-alice-bob-integration-test
2 checks passed
3 tasks
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
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
Test-only refactor of
upload.integration.test.tsand its SQL seed fixture, following code review on the prior PR. No test logic or coverage changes — all 34 tests still exercise the same scenarios.Three changes:
expectMutationDenied→expectRlsDenied— The old helper accepted any error as "denied" (including validation errors, 500s, typos). The new helper validates that the denial is specifically from RLS: checks forpermission denied,new row violates row-level security, orinsufficient_privilegein the error message, or a null mutation result (0-row RLS USING filter). Any other failure shape now throws, preventing tests from passing for the wrong reason.expectSuccesshelper — Extracts the repeatedexpect(res.status).toBe(200); expect(res.body.errors).toBeUndefined()pattern (~20 instances) into a single function that returnsres.body.datafor chaining.DRY SQL schema — Extracted a
_test_create_storage_schema(schema_name)PL/pgSQL function to eliminate ~110 lines of duplicated table definitions across Alice/Bob/Mallory. RLS policies remain inline since they differ per tenant. Function is dropped at the end of the seed file.Updates since last revision
pg.query()superuser check for post-attack data integrity. This was reverted after review feedback: using a separate PG connection introduces a transaction isolation concern. The verification now uses the same HTTP → server → pool path (postGraphQLViaApi+expectSuccess) as the mutations themselves, ensuring we read from the same transactional context.pgdestructuring andPgTestClientimport.Review & Testing Checklist for Human
expectRlsDeniedstring matching is complete — Are there other PG error messages for RLS denial that aren't covered by the three patterns (permission denied,new row violates row-level security,insufficient_privilege)? If any current test's denial uses a different message, it will now fail instead of silently passing._test_create_storage_schemausesEXECUTE format(...)with dynamic SQL. Confirm the generated table structure matches the original static DDL (column types, constraints, comments, grants).pnpm test -- upload.integrationNotes
_test_create_storage_schemafunction usesCREATE FUNCTION(notCREATE OR REPLACE) since this is a fresh test database — appropriate for a test fixture.feat/rls-alice-bob-integration-test, so only Socket Security checks run in CI. The full 52-check suite will run once merged towardmain.Link to Devin session: https://app.devin.ai/sessions/94a2728a9c414500bead29cbbc829c15
Requested by: @pyramation