Skip to content

fix: correct mutation variable shapes and expand expectRlsDenied patterns#1100

Merged
pyramation merged 1 commit intofeat/rls-alice-bob-integration-testfrom
fix/upload-test-mutation-variables
May 10, 2026
Merged

fix: correct mutation variable shapes and expand expectRlsDenied patterns#1100
pyramation merged 1 commit intofeat/rls-alice-bob-integration-testfrom
fix/upload-test-mutation-variables

Conversation

@pyramation
Copy link
Copy Markdown
Contributor

Summary

Fixes 10 failing tests in upload.integration.test.ts caused by two issues:

  1. Wrong update mutation variable field names — All update mutations used patch (e.g. { id, patch: { ... } }) but PostGraphile v5 generates table-specific field names: appFilePatch for UpdateAppFileInput and appBucketPatch for UpdateAppBucketInput. Previously the old expectMutationDenied helper silently accepted these as "denied" since the GraphQL schema validation error (Field "patch" is not defined by type "UpdateAppFileInput") was treated as any error. After PR refactor: clean up upload integration test — expectRlsDenied, expectSuccess, DRY SQL #1098 introduced the stricter expectRlsDenied, these schema validation errors are correctly rejected — the tests were never actually reaching the RLS layer.

  2. Missing RLS denial pattern for filtered deletes/updates — When RLS USING clauses filter out all matching rows, PostGraphile returns "No values were deleted in collection '...' because no values you can delete were found matching these criteria". This is a legitimate RLS denial not covered by the existing three patterns (permission denied, new row violates row-level security, insufficient_privilege). Added 'No values were' as a fourth pattern.

Review & Testing Checklist for Human

  • Verify appFilePatch / appBucketPatch match the actual generated input types — These names depend on PostGraphile's inflector. If the table names or inflector config change, these field names would too. Confirm by introspecting the schema or checking a working mutation in GraphiQL.
  • Verify 'No values were' pattern specificity — This is a substring match. Confirm no non-RLS errors in PostGraphile contain this phrase that could cause a false pass. The full message is typically "No values were deleted/updated in collection '...' because no values you can delete/update were found matching these criteria".
  • CI must pass — The integration test suite (integration-tests (graphql/server-test)) runs against real PostgreSQL + MinIO and is the only way to verify these mutations now correctly reach the RLS layer and are denied there.

Notes

  • The 4 affected update tests (Bob: bucket_id move, Bob: is_public flip, Mallory: filename update, Bob: bucket public→private) were previously passing for the wrong reason — GraphQL schema validation, not RLS. After this fix they will actually exercise the RLS policies as intended.
  • This targets the feat/rls-alice-bob-integration-test branch (PR feat: three-actor RLS + adversarial attack integration tests (Alice/Bob/Mallory) #1095).

Link to Devin session: https://app.devin.ai/sessions/94a2728a9c414500bead29cbbc829c15
Requested by: @pyramation

…erns

- Update mutations: patch → appFilePatch/appBucketPatch (PostGraphile v5 input types)
- Add 'No values were' pattern to expectRlsDenied (RLS USING-clause denials on delete/update)
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@pyramation pyramation merged commit 4f99ae3 into feat/rls-alice-bob-integration-test May 10, 2026
2 checks passed
@pyramation pyramation deleted the fix/upload-test-mutation-variables branch May 10, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant