From a713207db058d6639e34421ec6d2ecd5f0663329 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Tue, 6 Oct 2026 22:50:36 -0700 Subject: [PATCH 1/2] refactor(store): S3 signs upload policies through a private _post_policy, as GCS does issue_upload_policy built its policy by calling the public signed_post. It now calls a private _post_policy, which signed_post also calls, so signed_post behaves as before. A store layered on S3ObjectStore can then issue upload policies, and later make signed_post a shim over issue_upload_policy, without the two calling each other. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../store/object_store/s3_object_store.py | 9 ++++++--- tst/unit/store/s3_signed_post_test.py | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/agent_env/store/object_store/s3_object_store.py b/src/agent_env/store/object_store/s3_object_store.py index 259d7eca..488bc3c7 100644 --- a/src/agent_env/store/object_store/s3_object_store.py +++ b/src/agent_env/store/object_store/s3_object_store.py @@ -214,9 +214,7 @@ def issue_upload_policy( ) -> UploadPolicy: self._check_sigv4_expiry(expires_in) self._require_long_term_credentials(expires_in) - post = self.signed_post( - prefix_url.rstrip("/") + "/", expires_in=expires_in, max_bytes=max_object_bytes - ) + post = self._post_policy(prefix_url.rstrip("/") + "/", expires_in, max_object_bytes) return UploadPolicy( write=HttpPostPolicyGrant( kind="http-post-policy", @@ -231,6 +229,11 @@ def issue_upload_policy( def signed_post( self, url_prefix: str, *, expires_in: int = 3600, max_bytes: int | None = None ) -> dict | None: + return self._post_policy(url_prefix, expires_in, max_bytes) + + def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int | None) -> dict: + """A presigned POST for any key below ``url_prefix``: the key is a condition the uploader + fills in, not part of what is signed.""" bucket, prefix = self._split(url_prefix) conditions: list = [["starts-with", "$key", prefix]] if max_bytes is not None: diff --git a/tst/unit/store/s3_signed_post_test.py b/tst/unit/store/s3_signed_post_test.py index 93140d4c..64ae5f3c 100644 --- a/tst/unit/store/s3_signed_post_test.py +++ b/tst/unit/store/s3_signed_post_test.py @@ -58,3 +58,22 @@ def test_the_grant_is_the_fields_not_the_url(): def test_a_backend_that_cannot_sign_returns_none(tmp_path): """The ABC default.""" assert LocalFilesystemObjectStore(str(tmp_path)).signed_post(f"file://{tmp_path}/x/") is None + + +def test_an_upload_policy_is_signed_without_going_through_signed_post(): + """So a store that builds ``signed_post`` from ``issue_upload_policy`` can't recurse.""" + + class _Store(S3ObjectStore): + def signed_post(self, *args, **kwargs): + raise AssertionError("issue_upload_policy went through signed_post") + + client = _StubClient() + store = _Store(client, BUCKET) + store._check_sigv4_expiry = store._require_long_term_credentials = lambda expires_in: None + + policy = store.issue_upload_policy(f"s3://{BUCKET}/captures/one", max_object_bytes=1024, expires_in=600) + + (call,) = client.calls + assert call["Key"] == "captures/one/${filename}" + assert ["content-length-range", 0, 1024] in call["Conditions"] + assert policy.write.path_field == "key" and policy.write.file_field == "file" From 806dc431c92953d327511b6b2938e575a05e24c7 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Tue, 6 Oct 2026 23:02:35 -0700 Subject: [PATCH 2/2] Scope S3 upload policies to a directory whichever entry point is used signed_post passed its prefix through as given, so a prefix without a trailing slash admitted sibling keys (root also matched root-evil/). _post_policy now appends the slash, as the GCS store's does, and issue_upload_policy no longer needs to. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/agent_env/store/object_store/s3_object_store.py | 7 ++++--- tst/unit/store/s3_signed_post_test.py | 11 +++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/agent_env/store/object_store/s3_object_store.py b/src/agent_env/store/object_store/s3_object_store.py index 488bc3c7..c898ab66 100644 --- a/src/agent_env/store/object_store/s3_object_store.py +++ b/src/agent_env/store/object_store/s3_object_store.py @@ -214,7 +214,7 @@ def issue_upload_policy( ) -> UploadPolicy: self._check_sigv4_expiry(expires_in) self._require_long_term_credentials(expires_in) - post = self._post_policy(prefix_url.rstrip("/") + "/", expires_in, max_object_bytes) + post = self._post_policy(prefix_url, expires_in, max_object_bytes) return UploadPolicy( write=HttpPostPolicyGrant( kind="http-post-policy", @@ -233,8 +233,9 @@ def signed_post( def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int | None) -> dict: """A presigned POST for any key below ``url_prefix``: the key is a condition the uploader - fills in, not part of what is signed.""" - bucket, prefix = self._split(url_prefix) + fills in, not part of what is signed. The prefix ends in ``/``, so that ``root`` does not + admit ``root-evil/``.""" + bucket, prefix = self._split(url_prefix.rstrip("/") + "/") conditions: list = [["starts-with", "$key", prefix]] if max_bytes is not None: conditions.append(["content-length-range", 0, max_bytes]) diff --git a/tst/unit/store/s3_signed_post_test.py b/tst/unit/store/s3_signed_post_test.py index 64ae5f3c..39b9680e 100644 --- a/tst/unit/store/s3_signed_post_test.py +++ b/tst/unit/store/s3_signed_post_test.py @@ -17,10 +17,10 @@ def generate_presigned_post(self, **kwargs): return {"url": f"https://{BUCKET}.s3.amazonaws.com/", "fields": {"key": kwargs["Key"]}} -def _sign(**kwargs): +def _sign(prefix: str = "captures/one/", **kwargs): client = _StubClient() store = S3ObjectStore(client, BUCKET) - grant = store.signed_post(f"s3://{BUCKET}/captures/one/", **kwargs) + grant = store.signed_post(f"s3://{BUCKET}/{prefix}", **kwargs) return client.calls[0], grant @@ -31,6 +31,13 @@ def test_the_key_is_a_template_the_uploader_substitutes(): assert ["starts-with", "$key", "captures/one/"] in call["Conditions"] +def test_a_prefix_without_a_trailing_slash_is_a_directory(): + """So ``captures/one`` does not admit ``captures/one-evil/``.""" + call, _ = _sign("captures/one") + assert call["Key"] == "captures/one/${filename}" + assert ["starts-with", "$key", "captures/one/"] in call["Conditions"] + + def test_content_type_is_admitted(): # Without this the uploader's Content-Type 403s as an extra form field. call, _ = _sign()