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..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,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, expires_in, max_object_bytes) return UploadPolicy( write=HttpPostPolicyGrant( kind="http-post-policy", @@ -231,7 +229,13 @@ def issue_upload_policy( def signed_post( self, url_prefix: str, *, expires_in: int = 3600, max_bytes: int | None = None ) -> dict | None: - bucket, prefix = self._split(url_prefix) + 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. 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 93140d4c..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() @@ -58,3 +65,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"