From 4706f4536f99fb58d0eea264aad2b1e57d85f417 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 22 Sep 2026 20:15:45 +0530 Subject: [PATCH 1/7] UN-3487 [FIX] Require and enforce a bucket on the S3/MinIO connector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/test_connector_register.py | 17 +++++ backend/file_management/tests.py | 71 ++++++++++++++++++- backend/file_management/views.py | 27 +++++-- .../connectors/filesystems/minio/minio.py | 22 +++++- .../filesystems/minio/static/json_schema.json | 8 ++- .../connectors/filesystems/ucs/ucs.py | 4 ++ .../tests/filesystems/test_miniofs.py | 50 ++++++++++++- 7 files changed, 187 insertions(+), 12 deletions(-) diff --git a/backend/connector_v2/tests/test_connector_register.py b/backend/connector_v2/tests/test_connector_register.py index 889a961eca..b1f6e7e7bc 100644 --- a/backend/connector_v2/tests/test_connector_register.py +++ b/backend/connector_v2/tests/test_connector_register.py @@ -24,6 +24,7 @@ from connector_v2.models import ConnectorInstance MINIO_CONNECTOR_ID = "minio|c799f6e3-2b57-434e-aaac-b5daa415da19" +_BUCKET = "connector-register-test" pytestmark = pytest.mark.skipif( not all( @@ -45,12 +46,28 @@ def _credentials(secret: str | None = None) -> dict: "secret": secret or os.environ["MINIO_SECRET_ACCESS_KEY"], "endpoint_url": os.environ["MINIO_ENDPOINT_URL"], "region_name": "", + "bucket": _BUCKET, "path": "/", } +def _ensure_bucket_exists() -> None: + # UN-3487: MinioFS now requires a bucket, and `test_credentials` probes + # it directly — it must actually exist in the rig's MinIO. + from s3fs.core import S3FileSystem + + fs = S3FileSystem( + key=os.environ["MINIO_ACCESS_KEY_ID"], + secret=os.environ["MINIO_SECRET_ACCESS_KEY"], + client_kwargs={"endpoint_url": os.environ["MINIO_ENDPOINT_URL"]}, + ) + if not fs.exists(_BUCKET): + fs.mkdir(_BUCKET) + + class ConnectorRegisterTest(TestCase): def setUp(self) -> None: + _ensure_bucket_exists() self.org = Organization.objects.create( name="org-conn", display_name="Org Conn", organization_id="org-conn" ) diff --git a/backend/file_management/tests.py b/backend/file_management/tests.py index a39b155ac3..8abeb99fdc 100644 --- a/backend/file_management/tests.py +++ b/backend/file_management/tests.py @@ -1 +1,70 @@ -# Create your tests here. +"""UN-3487: `file/`, `file/download` and `file/upload` are scoped to the +caller, not just the org. + +Before this fix, `FileManagementViewSet` resolved a connector by a raw +`ConnectorInstance.objects.get(pk=id)` — any authenticated org member who +knew (or guessed) another user's connector id could browse, download from, +or upload to it, sharing settings aside. These drive the real views through +DRF's request factory, so a gate that exists only in the queryset — and +never reaches the route — is still caught. +""" + +from unittest.mock import patch + +from connector_v2.models import ConnectorInstance +from django.test import TestCase +from permissions.roles import ResourceRole +from permissions.tests.base import CoOwnerOrgTestMixin +from rest_framework import status +from rest_framework.response import Response +from rest_framework.test import APIRequestFactory, force_authenticate + +from file_management.views import FileManagementViewSet + + +class FileManagementAccessScopeTest(CoOwnerOrgTestMixin, TestCase): + def setUp(self) -> None: + self._seed_org() + self.connector = ConnectorInstance.objects.create( + connector_name="team-a-s3", + connector_id="minio|c799f6e3-2b57-434e-aaac-b5daa415da19", + connector_metadata={"bucket": "team-a-data"}, + organization=self.org, + created_by=self.owner, + ) + # `created_by` is audit-only — access runs through the membership + # table, same as every other shareable resource in this codebase. + self.connector.memberships.create(user=self.owner, role=ResourceRole.OWNER) + self.connector.memberships.create(user=self.viewer, role=ResourceRole.VIEWER) + self.factory = APIRequestFactory() + + def _list(self, actor) -> Response: + view = FileManagementViewSet.as_view({"get": "list"}) + request = self.factory.get( + "/file", {"connector_id": str(self.connector.pk), "path": "/"} + ) + force_authenticate(request, user=actor) + with ( + patch( + "file_management.views.FileManagerHelper.get_file_system", + return_value=None, + ), + patch("file_management.views.FileManagerHelper.list_files", return_value=[]), + ): + return view(request) + + def test_owner_can_list_their_own_connector(self) -> None: + self.assertEqual(self._list(self.owner).status_code, status.HTTP_200_OK) + + def test_org_admin_can_list_any_connector(self) -> None: + self.assertEqual(self._list(self.admin).status_code, status.HTTP_200_OK) + + def test_shared_viewer_can_list_the_connector(self) -> None: + self.assertEqual(self._list(self.viewer).status_code, status.HTTP_200_OK) + + def test_outsider_org_member_cannot_list_an_unshared_connector(self) -> None: + # In scope (org member), but the connector was never shared with them. + self.assertEqual(self._list(self.outsider).status_code, status.HTTP_404_NOT_FOUND) + + def test_non_org_member_cannot_list_the_connector(self) -> None: + self.assertEqual(self._list(self.stranger).status_code, status.HTTP_404_NOT_FOUND) diff --git a/backend/file_management/views.py b/backend/file_management/views.py index ea39cb1d28..6ed28d7d28 100644 --- a/backend/file_management/views.py +++ b/backend/file_management/views.py @@ -2,6 +2,7 @@ from typing import Any from connector_v2.models import ConnectorInstance +from django.db.models import QuerySet from django.http import HttpRequest from oauth2client.client import HttpAccessTokenRefreshError from rest_framework import serializers, viewsets @@ -30,8 +31,22 @@ class FileManagementViewSet(viewsets.ModelViewSet): versioning_class = URLPathVersioning - def get_queryset(self): - return ConnectorInstance.objects.all() + def get_queryset(self) -> QuerySet[ConnectorInstance]: + # Org-scoped alone isn't enough: this must also respect ownership / + # sharing, or any org member could browse another user's connector + # by guessing its id. + return ConnectorInstance.objects.for_user(self.request.user) + + def _get_connector_or_404(self, id: str) -> ConnectorInstance: + """Resolve a connector within the caller's own access scope. + + Raises the same not-found error whether the id is unknown or simply + outside `get_queryset()` — the caller can't tell those apart. + """ + try: + return self.get_queryset().get(pk=id) + except ConnectorInstance.DoesNotExist: + raise ConnectorInstanceNotFound() def get_serializer_class(self) -> serializers.Serializer: if self.action == "upload": @@ -50,13 +65,11 @@ def list(self, request: HttpRequest) -> Response: id: str = serializer.validated_data.get("connector_id") path: str = serializer.validated_data.get("path") try: - connector_instance: ConnectorInstance = ConnectorInstance.objects.get(pk=id) + connector_instance = self._get_connector_or_404(id) file_system = FileManagerHelper.get_file_system(connector_instance) files = FileManagerHelper.list_files(file_system, path) serializer = FileInfoSerializer(files, many=True) return Response(serializer.data) - except ConnectorInstance.DoesNotExist: - raise ConnectorInstanceNotFound() except HttpAccessTokenRefreshError as error: logger.error( f"HttpAccessTokenRefreshError thrown from file list, error {error}" @@ -72,7 +85,7 @@ def download(self, request: HttpRequest) -> Response: serializer.is_valid(raise_exception=True) id: str = serializer.validated_data.get("connector_id") path: str = serializer.validated_data.get("path") - connector_instance: ConnectorInstance = ConnectorInstance.objects.get(pk=id) + connector_instance = self._get_connector_or_404(id) file_system = FileManagerHelper.get_file_system(connector_instance) return FileManagerHelper.download_file(file_system, path) @@ -84,7 +97,7 @@ def upload(self, request: HttpRequest) -> Response: path: str = serializer.validated_data.get("path") uploaded_files: Any = serializer.validated_data.get("file") - connector_instance: ConnectorInstance = ConnectorInstance.objects.get(pk=id) + connector_instance = self._get_connector_or_404(id) file_system = FileManagerHelper.get_file_system(connector_instance) for uploaded_file in uploaded_files: diff --git a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py index 04063e19f0..9af0b01f1b 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py +++ b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py @@ -7,8 +7,11 @@ from typing import Any from botocore.exceptions import ClientError +from fsspec import AbstractFileSystem +from fsspec.implementations.dirfs import DirFileSystem from s3fs.core import S3FileSystem +from unstract.connectors.exceptions import ConnectorError from unstract.connectors.filesystems.unstract_file_system import UnstractFileSystem from .exceptions import ( @@ -138,12 +141,21 @@ class MinioFS(UnstractFileSystem): # known to have full access to every bucket they list, so the per-bucket # access probe in _AccessFilteredS3FileSystem can be skipped. _FS_CLASS: type[S3FileSystem] = _AccessFilteredS3FileSystem + # Override to False in a subclass whose settings restrict access some + # other way (e.g. UCS, which uses its own `path` setting instead). + _REQUIRES_BUCKET: bool = True def __init__(self, settings: dict[str, Any]): super().__init__("MinioFS/S3") key = (settings.get("key") or "").strip() secret = (settings.get("secret") or "").strip() endpoint_url = (settings.get("endpoint_url") or "").strip() + self.bucket = (settings.get("bucket") or "").strip() + if self._REQUIRES_BUCKET and not self.bucket: + raise ConnectorError( + "A bucket must be configured for this connector.", + treat_as_user_message=True, + ) client_kwargs = {} if "region_name" in settings and settings["region_name"] != "": client_kwargs = {"region_name": settings["region_name"]} @@ -324,7 +336,15 @@ def extract_modified_date(self, metadata: dict[str, Any]) -> datetime | None: ) return None - def get_fsspec_fs(self) -> S3FileSystem: + def get_fsspec_fs(self) -> AbstractFileSystem: + """Return the filesystem scoped to this connector's bucket. + + When a bucket is configured, every operation (list, read, write, + `test_credentials`) is confined to it via `DirFileSystem` — the + underlying credentials may see more, but this connector never will. + """ + if self.bucket: + return DirFileSystem(path=self.bucket, fs=self.s3) return self.s3 def test_credentials(self) -> bool: diff --git a/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json b/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json index d0ba5ed916..668b829231 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json +++ b/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json @@ -5,7 +5,8 @@ "required": [ "connectorName", "endpoint_url", - "region_name" + "region_name", + "bucket" ], "properties": { "connectorName": { @@ -13,6 +14,11 @@ "title": "Name of the connector", "default": "Unstract's S3/Minio" }, + "bucket": { + "type": "string", + "title": "Bucket Name", + "description": "Name of the bucket to be restricted to." + }, "key": { "type": "string", "title": "Key", diff --git a/unstract/connectors/src/unstract/connectors/filesystems/ucs/ucs.py b/unstract/connectors/src/unstract/connectors/filesystems/ucs/ucs.py index 82521a12af..927b2b1edf 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/ucs/ucs.py +++ b/unstract/connectors/src/unstract/connectors/filesystems/ucs/ucs.py @@ -15,6 +15,10 @@ class UnstractCloudStorage(MinioFS): # per-bucket access probe that MinioFS runs on its fsspec filesystem. # The probe adds latency and could hide a bucket on a transient S3 error. _FS_CLASS = S3FileSystem + # UCS restricts access via its own `path` setting, not `bucket` — its + # schema never carries `bucket`, so MinioFS's required-bucket check + # doesn't apply here. + _REQUIRES_BUCKET = False @staticmethod def get_id() -> str: diff --git a/unstract/connectors/tests/filesystems/test_miniofs.py b/unstract/connectors/tests/filesystems/test_miniofs.py index 9c8c4b6e25..af6bb1e2ce 100644 --- a/unstract/connectors/tests/filesystems/test_miniofs.py +++ b/unstract/connectors/tests/filesystems/test_miniofs.py @@ -5,8 +5,10 @@ import pytest from botocore.exceptions import ClientError +from fsspec.implementations.dirfs import DirFileSystem from s3fs.core import S3FileSystem from s3fs.errors import translate_boto_error +from unstract.connectors.exceptions import ConnectorError from unstract.connectors.filesystems.minio.exceptions import s3_error_code from unstract.connectors.filesystems.minio.minio import ( MinioFS, @@ -201,8 +203,7 @@ async def fake_call(_action: str, **kwargs: object) -> dict[str, object]: with ( patch( - "unstract.connectors.filesystems.minio.minio." - "S3FileSystem._lsbuckets", + "unstract.connectors.filesystems.minio.minio.S3FileSystem._lsbuckets", new=AsyncMock(return_value=parent_buckets), ), patch.object(fs, "_call_s3", new=AsyncMock(side_effect=fake_call)), @@ -241,5 +242,50 @@ def test_error_code_walks_context_when_cause_absent(self) -> None: self.assertEqual(s3_error_code(outer), "AccessDenied") +class TestMinioFSBucketRestriction(unittest.TestCase): + """Unit tests for UN-3487: `bucket` is required and enforced on MinioFS. + + All tests construct `MinioFS`/`UnstractCloudStorage` directly — no + network calls happen until an actual fsspec method is invoked. + """ + + def test_missing_bucket_raises(self) -> None: + with self.assertRaises(ConnectorError): + MinioFS({"key": "k", "secret": "s", "endpoint_url": "http://x"}) + + def test_blank_bucket_raises(self) -> None: + with self.assertRaises(ConnectorError): + MinioFS({"bucket": " ", "key": "k", "secret": "s"}) + + def test_bucket_is_stored(self) -> None: + fs = MinioFS({"bucket": "team-a-data", "key": "k", "secret": "s"}) + self.assertEqual(fs.bucket, "team-a-data") + + def test_get_fsspec_fs_scopes_to_bucket(self) -> None: + fs = MinioFS({"bucket": "team-a-data", "key": "k", "secret": "s"}) + scoped = fs.get_fsspec_fs() + self.assertIsInstance(scoped, DirFileSystem) + self.assertEqual(scoped.path, "team-a-data") + self.assertIs(scoped.fs, fs.s3) + + def test_scoped_root_listing_never_reaches_lsbuckets(self) -> None: + # The security-relevant assertion: ls("") on a bucket-scoped + # connector must resolve to listing that one bucket's contents, + # never the account-wide bucket enumeration `_lsbuckets` performs. + fs = MinioFS({"bucket": "team-a-data", "key": "k", "secret": "s"}) + with patch.object(fs.s3, "ls", return_value=[]) as mock_ls: + fs.get_fsspec_fs().ls("") + mock_ls.assert_called_once_with("team-a-data", detail=True) + + def test_ucs_does_not_require_bucket(self) -> None: + # UCS restricts access via its own `path` setting instead. + ucs = UnstractCloudStorage({"key": "k", "secret": "s", "path": "some/path"}) + self.assertEqual(ucs.bucket, "") + + def test_ucs_get_fsspec_fs_is_unscoped(self) -> None: + ucs = UnstractCloudStorage({"key": "k", "secret": "s", "path": "some/path"}) + self.assertIs(ucs.get_fsspec_fs(), ucs.s3) + + if __name__ == "__main__": unittest.main() From c48b7964611db9853b463365e3943d8ca55f53a8 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 22 Sep 2026 20:32:03 +0530 Subject: [PATCH 2/7] UN-3487 [FIX] Stop double-prefixing bucket-scoped root-level operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `//...` instead of `/...`. - 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 --- .../file_management/file_management_helper.py | 39 ++++++++++++------- backend/file_management/tests.py | 33 +++++++++++++++- tests/e2e/etl/conftest.py | 8 +++- 3 files changed, 63 insertions(+), 17 deletions(-) diff --git a/backend/file_management/file_management_helper.py b/backend/file_management/file_management_helper.py index 51cc141809..9ec3b587a4 100644 --- a/backend/file_management/file_management_helper.py +++ b/backend/file_management/file_management_helper.py @@ -11,6 +11,7 @@ from django.conf import settings from django.http import StreamingHttpResponse from fsspec import AbstractFileSystem +from fsspec.implementations.dirfs import DirFileSystem from pydrive2.files import ApiRequestError from file_management.exceptions import ( @@ -31,6 +32,27 @@ logger = logging.getLogger(__name__) +def _default_root_path( + file_system: UnstractFileSystem, fs: AbstractFileSystem, path: str +) -> str | None: + """Some connectors restrict browsing to their own default root (e.g. a + configured `path`) when the caller didn't ask for a specific one. + + `DirFileSystem.path` (used by a bucket-scoped MinioFS) is excluded: it's + the wrapped bucket prefix, not a default root, and every operation + already resolves relative to it — applying it again here would + double-prefix the path (UN-3487). + """ + if not path or path == "/": + try: + if file_system.path: + return file_system.path + except AttributeError: + if hasattr(fs, "path") and fs.path and not isinstance(fs, DirFileSystem): + return fs.path + return None + + class FileManagerHelper: @staticmethod def get_file_system(connector: ConnectorInstance) -> UnstractFileSystem: @@ -47,14 +69,8 @@ def get_file_system(connector: ConnectorInstance) -> UnstractFileSystem: @staticmethod def list_files(file_system: UnstractFileSystem, path: str) -> list[FileInformation]: fs = file_system.get_fsspec_fs() - file_path = f"{path}" - # TODO: Add below logic by checking each connector? try: - if file_system.path and (not path or path == "/"): - file_path = file_system.path - except AttributeError: - if hasattr(fs, "path") and fs.path and (not path or path == "/"): - file_path = fs.path + file_path = _default_root_path(file_system, fs, path) or path except Exception: logger.error(f"Missing path Atribute in {fs}") raise MissingConnectorParams() @@ -135,13 +151,8 @@ def upload_file( ) -> None: fs = file_system.get_fsspec_fs() - file_path = f"{path}" - try: - if file_system.path and (not path or path == "/"): - file_path = f"{file_system.path}/" - except AttributeError: - if fs.path and (not path or path == "/"): - file_path = f"{fs.path}/" + root = _default_root_path(file_system, fs, path) + file_path = f"{root}/" if root else path file_path = file_path + "/" if not file_path.endswith("/") else file_path diff --git a/backend/file_management/tests.py b/backend/file_management/tests.py index 8abeb99fdc..bc2c1d1cd3 100644 --- a/backend/file_management/tests.py +++ b/backend/file_management/tests.py @@ -9,7 +9,8 @@ never reaches the route — is still caught. """ -from unittest.mock import patch +import unittest +from unittest.mock import mock_open, patch from connector_v2.models import ConnectorInstance from django.test import TestCase @@ -19,7 +20,9 @@ from rest_framework.response import Response from rest_framework.test import APIRequestFactory, force_authenticate +from file_management.file_management_helper import FileManagerHelper from file_management.views import FileManagementViewSet +from unstract.connectors.filesystems.minio.minio import MinioFS class FileManagementAccessScopeTest(CoOwnerOrgTestMixin, TestCase): @@ -68,3 +71,31 @@ def test_outsider_org_member_cannot_list_an_unshared_connector(self) -> None: def test_non_org_member_cannot_list_the_connector(self) -> None: self.assertEqual(self._list(self.stranger).status_code, status.HTTP_404_NOT_FOUND) + + +class BucketScopedRootPathTest(unittest.TestCase): + """Regression for a Greptile finding on this PR: a bucket-scoped + connector's own `DirFileSystem.path` is its wrapped bucket prefix, not a + connector-level default root. Treating it as one double-prefixes every + root-level operation (`//...`) instead of resolving + within the bucket. + """ + + def _minio_fs(self) -> MinioFS: + return MinioFS({"bucket": "team-a-data", "key": "k", "secret": "s"}) + + def test_list_files_at_root_is_not_double_prefixed(self) -> None: + minio_fs = self._minio_fs() + with patch.object(minio_fs.s3, "ls", return_value=[]) as mock_ls: + FileManagerHelper.list_files(minio_fs, "/") + mock_ls.assert_called_once_with("team-a-data/", detail=True) + + def test_upload_file_at_root_is_not_double_prefixed(self) -> None: + minio_fs = self._minio_fs() + with patch.object(minio_fs.s3, "open", mock_open()) as mock_open_call: + FileManagerHelper.upload_file(minio_fs, "/", b"data", "report.pdf") + (called_path,), kwargs = mock_open_call.call_args + self.assertEqual(kwargs, {"mode": "wb"}) + self.assertNotIn("team-a-data/team-a-data", called_path) + self.assertTrue(called_path.startswith("team-a-data/")) + self.assertTrue(called_path.endswith("report.pdf")) diff --git a/tests/e2e/etl/conftest.py b/tests/e2e/etl/conftest.py index 7cff0c6920..2a07245a12 100644 --- a/tests/e2e/etl/conftest.py +++ b/tests/e2e/etl/conftest.py @@ -133,6 +133,7 @@ def create_connector(connector_type: str) -> str: "secret": minio_store.secret_key, "endpoint_url": minio_store.internal_url, "region_name": "us-east-1", + "bucket": minio_store.bucket, }, }, ) @@ -164,7 +165,9 @@ def create_connector(connector_type: str) -> str: "connection_type": "FILESYSTEM", "connector_instance_id": source_id, "configuration": { - "folders": [f"/{minio_store.bucket}/{input_prefix}"], + # UN-3487: the connector is now scoped to its own bucket, so + # paths resolve relative to it — no bucket prefix here. + "folders": [f"/{input_prefix}"], "processSubDirectories": False, "maxFiles": 1, "fileProcessingOrder": "unordered", @@ -179,7 +182,8 @@ def create_connector(connector_type: str) -> str: json={ "connection_type": "FILESYSTEM", "connector_instance_id": destination_id, - "configuration": {"outputFolder": f"{minio_store.bucket}/{output_prefix}"}, + # UN-3487: same as the source — no bucket prefix, it's implicit. + "configuration": {"outputFolder": output_prefix}, }, ) assert resp.status_code == 200, f"destination endpoint: {resp.text}" From eaa7826c3be4d47249a301762d74107fbfcfaf79 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 22 Sep 2026 20:40:21 +0530 Subject: [PATCH 3/7] UN-3487 [FIX] Update the live-MinIO integration test for the required bucket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../tests/filesystems/test_miniofs.py | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/unstract/connectors/tests/filesystems/test_miniofs.py b/unstract/connectors/tests/filesystems/test_miniofs.py index af6bb1e2ce..9b437e6d54 100644 --- a/unstract/connectors/tests/filesystems/test_miniofs.py +++ b/unstract/connectors/tests/filesystems/test_miniofs.py @@ -28,21 +28,30 @@ def test_minio(self) -> None: # Endpoint from the rig's testcontainers MinIO via MINIO_ENDPOINT_URL; # falls back to the local platform MinIO for manual runs. self.assertEqual(MinioFS.requires_oauth(), False) + endpoint_url = os.environ.get("MINIO_ENDPOINT_URL", "http://localhost:9000") + bucket = "rig-minio-test" + + # UN-3487: bucket is now required, so the connector can no longer + # discover it via a root bucket listing — bootstrap it directly. + bootstrap_fs = S3FileSystem( + key=os.environ["MINIO_ACCESS_KEY_ID"], + secret=os.environ["MINIO_SECRET_ACCESS_KEY"], + client_kwargs={"endpoint_url": endpoint_url}, + ) + if not bootstrap_fs.exists(bucket): + bootstrap_fs.mkdir(bucket) + fs = MinioFS( { "key": os.environ["MINIO_ACCESS_KEY_ID"], "secret": os.environ["MINIO_SECRET_ACCESS_KEY"], - "endpoint_url": os.environ.get( - "MINIO_ENDPOINT_URL", "http://localhost:9000" - ), - "path": "/", + "endpoint_url": endpoint_url, + "bucket": bucket, } ).get_fsspec_fs() - bucket = "rig-minio-test" - if not fs.exists(bucket): - fs.mkdir(bucket) - listed = [b.rstrip("/").split("/")[-1] for b in fs.ls("")] - self.assertIn(bucket, listed) + # A live connection to the bucket-scoped root must succeed, without + # needing (or being able) to see any other bucket. + fs.ls("") def _translated_error(code: str) -> BaseException: From 0d6cf9953942ff03ee51cb3621141be061612841 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 22 Sep 2026 21:00:31 +0530 Subject: [PATCH 4/7] UN-3487 [FIX] Fix DirFileSystem.walk() leaking the bucket into file names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../connectors/filesystems/minio/minio.py | 31 +++++++++++++++++-- .../tests/filesystems/test_miniofs.py | 28 +++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py index 9af0b01f1b..36f47c6c5c 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py +++ b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py @@ -23,6 +23,30 @@ logger = logging.getLogger(__name__) + +class _BucketScopedFileSystem(DirFileSystem): + """`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. Fix it here so discovery + (which walks) and browsing (which lists) agree (UN-3487). + """ + + def _relpath_entries(self, entries: dict[str, Any]) -> dict[str, Any]: + return { + self._relpath(name): {**info, "name": self._relpath(info["name"])} + for name, info in entries.items() + } + + def walk(self, path: str, *args: Any, **kwargs: Any) -> Any: + for root, dirs, files in super().walk(path, *args, **kwargs): + yield root, self._relpath_entries(dirs), self._relpath_entries(files) + + async def _walk(self, path: str, *args: Any, **kwargs: Any) -> Any: + async for root, dirs, files in super()._walk(path, *args, **kwargs): + yield root, self._relpath_entries(dirs), self._relpath_entries(files) + + # Cap concurrent per-bucket probes to avoid S3 503 SlowDown on large accounts. _MAX_CONCURRENT_BUCKET_PROBES = 16 _BUCKET_PROBE_RETRY_DELAY_SECONDS = 0.5 @@ -340,11 +364,12 @@ def get_fsspec_fs(self) -> AbstractFileSystem: """Return the filesystem scoped to this connector's bucket. When a bucket is configured, every operation (list, read, write, - `test_credentials`) is confined to it via `DirFileSystem` — the - underlying credentials may see more, but this connector never will. + `test_credentials`) is confined to it via `_BucketScopedFileSystem` — + the underlying credentials may see more, but this connector never + will. """ if self.bucket: - return DirFileSystem(path=self.bucket, fs=self.s3) + return _BucketScopedFileSystem(path=self.bucket, fs=self.s3) return self.s3 def test_credentials(self) -> bool: diff --git a/unstract/connectors/tests/filesystems/test_miniofs.py b/unstract/connectors/tests/filesystems/test_miniofs.py index 9b437e6d54..2cde2f0699 100644 --- a/unstract/connectors/tests/filesystems/test_miniofs.py +++ b/unstract/connectors/tests/filesystems/test_miniofs.py @@ -277,6 +277,34 @@ def test_get_fsspec_fs_scopes_to_bucket(self) -> None: self.assertEqual(scoped.path, "team-a-data") self.assertIs(scoped.fs, fs.s3) + def test_walk_results_are_relative_to_the_bucket(self) -> None: + # `DirFileSystem.walk()` relpaths the directory string it yields, + # but not the `name` field inside each entry's own metadata dict — + # those still carry the wrapped fs's raw, bucket-prefixed key. This + # is what workflow-execution file discovery reads (it walks, the + # UI browser lists) — a name still carrying the bucket here is what + # doubles the prefix at the point a discovered file gets opened. + fs = MinioFS({"bucket": "unstract", "key": "k", "secret": "s"}) + + def fake_walk(path: str, detail: bool = True, **kwargs: object): + yield ( + "unstract/e2e-in", + {}, + { + "unstract/e2e-in/probe.txt": { + "name": "unstract/e2e-in/probe.txt", + "type": "file", + "size": 10, + } + }, + ) + + with patch.object(fs.s3, "walk", side_effect=fake_walk): + root, dirs, files = next(fs.get_fsspec_fs().walk("e2e-in")) + self.assertEqual(root, "e2e-in") + self.assertEqual(list(files.keys()), ["e2e-in/probe.txt"]) + self.assertEqual(files["e2e-in/probe.txt"]["name"], "e2e-in/probe.txt") + def test_scoped_root_listing_never_reaches_lsbuckets(self) -> None: # The security-relevant assertion: ls("") on a bucket-scoped # connector must resolve to listing that one bucket's contents, From 4a33c7b7d0ddb28974de74d2ff602be906c9b851 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 22 Sep 2026 21:08:08 +0530 Subject: [PATCH 5/7] UN-3487 [FIX] Handle walk()'s default detail=False shape correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../connectors/filesystems/minio/minio.py | 11 +++++-- .../tests/filesystems/test_miniofs.py | 30 +++++++++++++------ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py index 36f47c6c5c..bba3e7d181 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py +++ b/unstract/connectors/src/unstract/connectors/filesystems/minio/minio.py @@ -32,9 +32,16 @@ class _BucketScopedFileSystem(DirFileSystem): (which walks) and browsing (which lists) agree (UN-3487). """ - def _relpath_entries(self, entries: dict[str, Any]) -> dict[str, Any]: + def _relpath_entries( + self, entries: dict[str, Any] | list[str] + ) -> dict[str, Any] | list[str]: + # detail=False (fsspec's own default) yields bare basenames with + # nothing to fix. detail=True yields a dict already keyed by bare + # basename — only each entry's own `name` field is bucket-qualified. + if not isinstance(entries, dict): + return entries return { - self._relpath(name): {**info, "name": self._relpath(info["name"])} + name: {**info, "name": self._relpath(info["name"])} for name, info in entries.items() } diff --git a/unstract/connectors/tests/filesystems/test_miniofs.py b/unstract/connectors/tests/filesystems/test_miniofs.py index 2cde2f0699..104669b260 100644 --- a/unstract/connectors/tests/filesystems/test_miniofs.py +++ b/unstract/connectors/tests/filesystems/test_miniofs.py @@ -284,26 +284,38 @@ def test_walk_results_are_relative_to_the_bucket(self) -> None: # is what workflow-execution file discovery reads (it walks, the # UI browser lists) — a name still carrying the bucket here is what # doubles the prefix at the point a discovered file gets opened. + # + # Matches fsspec's real shape (verified against AbstractFileSystem. + # walk): the dict is keyed by bare basename already; only each + # entry's own `name` field carries the full, bucket-qualified path. fs = MinioFS({"bucket": "unstract", "key": "k", "secret": "s"}) def fake_walk(path: str, detail: bool = True, **kwargs: object): yield ( "unstract/e2e-in", {}, - { - "unstract/e2e-in/probe.txt": { - "name": "unstract/e2e-in/probe.txt", - "type": "file", - "size": 10, - } - }, + {"probe.txt": {"name": "unstract/e2e-in/probe.txt", "type": "file"}}, ) + with patch.object(fs.s3, "walk", side_effect=fake_walk): + root, dirs, files = next(fs.get_fsspec_fs().walk("e2e-in", detail=True)) + self.assertEqual(root, "e2e-in") + self.assertEqual(list(files.keys()), ["probe.txt"]) + self.assertEqual(files["probe.txt"]["name"], "e2e-in/probe.txt") + + def test_walk_default_detail_false_is_untouched(self) -> None: + # fsspec's own default is detail=False, which yields bare basename + # lists, not dicts. The relpath fix must not assume dict shape, or + # any caller using the plain walk() contract gets an AttributeError. + fs = MinioFS({"bucket": "unstract", "key": "k", "secret": "s"}) + + def fake_walk(path: str, **kwargs: object): + yield ("unstract/e2e-in", [], ["probe.txt"]) + with patch.object(fs.s3, "walk", side_effect=fake_walk): root, dirs, files = next(fs.get_fsspec_fs().walk("e2e-in")) self.assertEqual(root, "e2e-in") - self.assertEqual(list(files.keys()), ["e2e-in/probe.txt"]) - self.assertEqual(files["e2e-in/probe.txt"]["name"], "e2e-in/probe.txt") + self.assertEqual(files, ["probe.txt"]) def test_scoped_root_listing_never_reaches_lsbuckets(self) -> None: # The security-relevant assertion: ls("") on a bucket-scoped From 9a37ce32eba80e9ee9389510b6e600ca64f8fa7e Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Wed, 23 Sep 2026 16:44:49 +0530 Subject: [PATCH 6/7] UN-3487 [FIX] Move the Bucket field to the bottom of the S3/Minio connector 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 --- .../filesystems/minio/static/json_schema.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json b/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json index 668b829231..d99c6d232b 100644 --- a/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json +++ b/unstract/connectors/src/unstract/connectors/filesystems/minio/static/json_schema.json @@ -14,11 +14,6 @@ "title": "Name of the connector", "default": "Unstract's S3/Minio" }, - "bucket": { - "type": "string", - "title": "Bucket Name", - "description": "Name of the bucket to be restricted to." - }, "key": { "type": "string", "title": "Key", @@ -42,6 +37,11 @@ "title": "Region Name", "default": "ap-south", "description": "Region of the AWS S3 account (leave blank for Minio)" + }, + "bucket": { + "type": "string", + "title": "Bucket Name", + "description": "Name of the bucket to be restricted to." } } } From 2ebfb4f03560efbc807f3b6dd8ed18b2bb85bce2 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Wed, 23 Sep 2026 16:49:43 +0530 Subject: [PATCH 7/7] UN-3487 [FIX] Wrap long S3/MinIO error text instead of overflowing the toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connector error messages come wrapped in a markdown code fence (handle_s3fs_exception), which CustomMarkdown renders as a bare 
.

 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 
---
 .../helpers/custom-markdown/CustomMarkdown.jsx         | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/frontend/src/components/helpers/custom-markdown/CustomMarkdown.jsx b/frontend/src/components/helpers/custom-markdown/CustomMarkdown.jsx
index b9a639adce..f02f69ef37 100644
--- a/frontend/src/components/helpers/custom-markdown/CustomMarkdown.jsx
+++ b/frontend/src/components/helpers/custom-markdown/CustomMarkdown.jsx
@@ -37,7 +37,15 @@ const CustomMarkdown = ({
       case "tripleCode":
         return (
           
-            
{content}

+            
+              {content}
+            

           
         );
       case "inlineCode":