fix(store): S3 upload policies are scoped to a directory and signed through a private _post_policy

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])

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"