UN-3487 [FIX] Restrict the S3/MinIO connector to one bucket - #2295
Merged
Merged
Conversation
The S3/MinIO connector had no bucket field, so its reach was bounded only by whatever the underlying credential (often a shared IAM role) could see account-wide. Any connector could browse into any bucket that role had access to, including other teams' data. - `bucket` is now a required field on the connector schema, matching Azure's existing (but unenforced) precedent — this one is actually enforced. - `MinioFS.get_fsspec_fs()` wraps the filesystem in fsspec's own `DirFileSystem` when a bucket is set, confining every list/read/write (and `test_credentials`) to that one bucket regardless of what the credential could otherwise reach. UCS opts out via `_REQUIRES_BUCKET`, since it restricts access through its own `path` setting instead. - Fixed a related IDOR in `file_management`: `list`/`download`/`upload` resolved a connector by a bare `ConnectorInstance.objects.get(pk=id)` with no ownership/group check, org-scoped only. Any org member holding a connector's id could browse/read/write through it regardless of sharing. Now scoped through `ConnectorInstance.objects.for_user()`, the same queryset every other connector view already uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile flagged (P1): DirFileSystem's own `.path` attribute is the wrapped bucket prefix, but file_management_helper's root-path fallback treated it as a connector-level default root and substituted it as the operation path — every root-level list/upload then resolved to `<bucket>/<bucket>/...` instead of `<bucket>/...`. - Factored the fallback into `_default_root_path()`, which now excludes DirFileSystem explicitly: its `.path` never needs re-applying, every operation already resolves relative to it. - Added regression tests exercising the real `FileManagerHelper.list_files` / `upload_file` against a bucket-scoped MinioFS with a mocked S3 backend, asserting the underlying call receives a single bucket prefix. - Fixed the same root cause in the e2e ETL fixture: its connector had no `bucket` (now required), and its configured `folders`/`outputFolder` included the bucket name as a path segment — now implicit via the connector's own scope, per the same fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… bucket TestMinoFS::test_minio asserted that connecting without a bucket lists every bucket in the account — exactly the behavior this ticket removes. Bootstrap the test bucket via a plain S3FileSystem (the connector itself can no longer discover buckets this way), then assert the bucket-scoped connection lists successfully. CI signal: this is what "test (integration)" failed on in both runs, not a new issue. The accompanying test_pg_barrier.py failures in that same job are a pre-existing, unrelated CI DNS flake (identical on both runs, "could not translate host name unstract-db"), not caused by this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ames Found by tracing the e2e ETL failure through the compose stack's own logs (docker-compose-logs.txt from the CI run), not assumed: the worker log showed "Failed to prepare input file and metadata: unstract/unstract/e2e-in-.../probe.txt" — a double bucket prefix, but from a different cause than the earlier file_management_helper fix. fsspec's DirFileSystem.walk() relpaths the directory string it yields, but not the `name` field inside each file/dir entry's own metadata dict — those still carry the wrapped fs's raw, bucket-prefixed key (ls() already fixes every entry; walk() doesn't, confirmed by reading fsspec's own source and reproducing it directly against a mocked S3FileSystem). Workflow-execution file discovery walks (UnstractFileSystem.list_files, shared by backend and workers); the UI file browser lists — which is why this was invisible to the file_management-focused testing so far. _BucketScopedFileSystem wraps DirFileSystem and relpaths both dict keys and each entry's `name` field for walk()/_walk(), so discovery sees the same bucket-relative paths ls() already produced correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile P1: the walk()/_walk() override always treated dirs/files as detail=True dicts, calling .items() unconditionally. fsspec's own default is detail=False (confirmed by reading AbstractFileSystem.walk's source), which yields plain basename lists — every caller using the default contract got an AttributeError instead of results. Also caught, verified empirically against the real base implementation: my own relpath was wrong on the dict-keyed (detail=True) path too. The dict key is already a bare basename — only each entry's own `name` field carries the bucket-qualified path. Relpath-ing the key itself would hit `DirFileSystem._relpath`'s own assertion on real data (a basename never starts with the bucket prefix); the prior test happened to pass only because its mock's key and `name` field were identical, which isn't how fsspec actually shapes walk() results. Re-checked Greptile's second (repeated) finding on this same review — the file_management_helper double-prefix fix from an earlier commit is still intact: `_BucketScopedFileSystem` is a `DirFileSystem` subclass, so the existing `isinstance(fs, DirFileSystem)` exclusion still applies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nector form The connector config form renders fields in json_schema.json's property order. Bucket landed second, right after the connector name, ahead of the credential fields users fill in first. Moved it to the end so the form still reads top-to-bottom the way users configure it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e toast Connector error messages come wrapped in a markdown code fence (handle_s3fs_exception), which CustomMarkdown renders as a bare <pre>. <pre> defaults to white-space: pre, so a long single-line message like the "Invalid Secret" one never wraps — it just overflows past the fixed-width toast instead of staying inside it. Added pre-wrap + overflow-wrap so long connector errors wrap like the rest of the toast. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kirtimanmishrazipstack
requested review from
chandrasekharan-zipstack and
jaseemjaskp
September 24, 2026 06:54
jaseemjaskp
approved these changes
Sep 24, 2026
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
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.
What
Why
On this deployment, every S3 connector shares one AWS identity, so today any connector can reach any bucket that identity can reach — including other teams' data. Locking each connector to a single bucket keeps every team confined to its own bucket.
The access-control bug was fixed alongside it because without it, restricting a connector to one bucket wouldn't have meant much — anyone who knew a connector's ID could still open it directly, sharing settings or not.
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Yes, intentionally. Any existing S3/MinIO connector without a bucket configured will stop working the next time it's used, until an admin edits it to add one. If a connector's saved folder path already started with the bucket name, that path needs to be shortened by one segment once a bucket is added, since the bucket is no longer part of the browsable path. Cloud storage (UCS) connectors are unaffected.
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.