Skip to content

fix: empty bucket rls check#958

Merged
ferhatelmas merged 1 commit intomasterfrom
ferhat/empty-bucket-rls
Apr 2, 2026
Merged

fix: empty bucket rls check#958
ferhatelmas merged 1 commit intomasterfrom
ferhat/empty-bucket-rls

Conversation

@ferhatelmas
Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Check passes id but delete uses name. It will be satisfied regardless.

What is the new behavior?

Passes name correctly as intended.

Additional context

Might be better to read the object again, delete inside the transaction and throw access denied.

@ferhatelmas ferhatelmas requested a review from a team as a code owner April 1, 2026 11:35
Copilot AI review requested due to automatic review settings April 1, 2026 11:35
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an RLS permission check bug in the bucket-empty operation where the permission probe used an object id but deletion is keyed by object name, causing the check to succeed incorrectly.

Changes:

  • Fix Storage.emptyBucket() permission probing to call deleteObject(bucketId, objectName) and fail fast if the delete doesn’t apply.
  • Extend the RLS test harness with bucket.empty and a direct DB object.seed helper operation.
  • Add a new YAML RLS scenario asserting that bucket emptying fails when object delete permission is missing.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/storage/storage.ts Corrects the delete-permission probe in emptyBucket() to use object name (and error if not deleted).
src/test/rls.test.ts Adds bucket.empty and object.seed operations to the RLS test runner and switches to shared per-test user/storage vars.
src/test/rls_tests.yaml Adds a regression test ensuring bucket.empty is denied without object delete permission.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 1, 2026

Pull Request Test Coverage Report for Build 23906146650

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 80.507%

Files with Coverage Reduction New Missed Lines %
src/http/routes/s3/index.ts 2 86.23%
src/http/plugins/signals.ts 6 84.38%
Totals Coverage Status
Change from base Build 23900406943: -0.02%
Covered Lines: 29901
Relevant Lines: 36961

💛 - Coveralls

Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
@ferhatelmas ferhatelmas force-pushed the ferhat/empty-bucket-rls branch from 6c14e46 to 2f9540b Compare April 2, 2026 14:42
@ferhatelmas ferhatelmas merged commit b1d7d9a into master Apr 2, 2026
3 checks passed
@ferhatelmas ferhatelmas deleted the ferhat/empty-bucket-rls branch April 2, 2026 14:53
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.

4 participants