From 3ea3596baa963d9a2d228194c11a06f8b6982a96 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 7 Oct 2026 07:20:41 -0700 Subject: [PATCH] refactor(store)!: remove ObjectStore.signed_post issue_upload_policy is the upload policy API: typed, with the uploader's path and file field names, and a lifetime the store either honours or refuses. signed_post returned boto3's raw {url, fields} and behaved differently per store (S3 kept the prefix as given until recently; GCS returned None without a signer and silently clamped the expiry). Nothing calls it now, so it goes: from the base class, the S3 and GCS stores and the routing store. S3ObjectStore._post_policy and GCSObjectStore._post_policy take a required max_bytes, since issue_upload_policy always passes one. S3's upload-policy coverage is in test_object_transfer_grants; GCS keeps its bucket-root case as an issue_upload_policy test. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../store/object_store/gcs_object_store.py | 17 ++-- .../store/object_store/object_store.py | 14 --- .../store/object_store/s3_object_store.py | 11 +-- src/agent_env/store/routing.py | 3 - tst/unit/store/gcs_object_store_test.py | 24 ++---- tst/unit/store/s3_signed_post_test.py | 86 ------------------- tst/unit/task/local_run_write_fence_test.py | 2 +- 7 files changed, 14 insertions(+), 143 deletions(-) delete mode 100644 tst/unit/store/s3_signed_post_test.py diff --git a/src/agent_env/store/object_store/gcs_object_store.py b/src/agent_env/store/object_store/gcs_object_store.py index e8c5d2f7..19a294cf 100644 --- a/src/agent_env/store/object_store/gcs_object_store.py +++ b/src/agent_env/store/object_store/gcs_object_store.py @@ -289,14 +289,7 @@ def issue_upload_policy( expires_at=expires_at, ) - def signed_post( - self, url_prefix: str, *, expires_in: int = 3600, max_bytes: int | None = None - ) -> dict | None: - if not self._can_sign(): - return None - return self._post_policy(url_prefix, min(expires_in, self._max_signed_seconds), max_bytes) - - def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int | None) -> dict: + def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int) -> dict: """Built by hand: the client library's helper adds an exact-key condition, which would confine the policy to one object. The prefix ends in ``/``, so that ``root`` does not admit ``root-evil/``.""" @@ -304,10 +297,10 @@ def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int | None) now = datetime.now(UTC).replace(microsecond=0) timestamp = now.strftime("%Y%m%dT%H%M%SZ") credential = f"{self._signer.signer_email}/{now:%Y%m%d}/auto/storage/goog4_request" - conditions: list = [["starts-with", "$key", prefix], ["starts-with", "$Content-Type", ""]] - if max_bytes is not None: - conditions.append(["content-length-range", 0, max_bytes]) - conditions += [ + conditions: list = [ + ["starts-with", "$key", prefix], + ["starts-with", "$Content-Type", ""], + ["content-length-range", 0, max_bytes], {"bucket": bucket}, {"x-goog-date": timestamp}, {"x-goog-credential": credential}, diff --git a/src/agent_env/store/object_store/object_store.py b/src/agent_env/store/object_store/object_store.py index edab2be5..215e2280 100644 --- a/src/agent_env/store/object_store/object_store.py +++ b/src/agent_env/store/object_store/object_store.py @@ -164,20 +164,6 @@ def signed_put_url(self, object_url: str, expires_in: int = 3600) -> str | None: or None if the backend can't produce one (e.g. local filesystem).""" return None - def signed_post( - self, url_prefix: str, *, expires_in: int = 3600, max_bytes: int | None = None - ) -> dict | None: - """Credentials letting a remote party upload objects it names itself, anywhere - under ``url_prefix``. None if the backend can't produce them. - - Prefix-scoped so the uploader owns its file layout — the caller bounds where - it may write, not what it may call things. That is also why the grant is a - dict and not a url: a signed url covers one key, since the key is part of - what is signed, so a prefix needs a policy the uploader submits as form - fields. Pass the whole value on, not just ``url``. - """ - return None - def shared_credentials_env(self) -> dict[str, str]: """Environment variables handing credentials to the agents and env services agent-env deploys, so they can use this store directly. None by default; a deployment that wants 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 c898ab66..40249860 100644 --- a/src/agent_env/store/object_store/s3_object_store.py +++ b/src/agent_env/store/object_store/s3_object_store.py @@ -226,19 +226,12 @@ def issue_upload_policy( expires_at=datetime.now(UTC) + timedelta(seconds=expires_in), ) - 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: + def _post_policy(self, url_prefix: str, expires_in: int, max_bytes: int) -> 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]) + conditions: list = [["starts-with", "$key", prefix], ["content-length-range", 0, max_bytes]] # POST policies are default-deny on extra form fields: without this the # uploader's Content-Type 403s and objects store as binary/octet-stream. conditions.append(["starts-with", "$Content-Type", ""]) diff --git a/src/agent_env/store/routing.py b/src/agent_env/store/routing.py index 71442e35..5b8b8016 100644 --- a/src/agent_env/store/routing.py +++ b/src/agent_env/store/routing.py @@ -552,9 +552,6 @@ def signed_get_url(self, object_url: str, expires_in: int = 3600) -> str | None: def signed_put_url(self, object_url: str, expires_in: int = 3600) -> str | None: return self._writing(object_url).signed_put_url(object_url, expires_in) - def signed_post(self, url_prefix: str, *, expires_in: int = 3600, max_bytes: int | None = None) -> dict | None: - return self._writing(url_prefix).signed_post(url_prefix, expires_in=expires_in, max_bytes=max_bytes) - def shared_credentials_env(self) -> dict[str, str]: """None: an ``@local`` run's objects reach its sandboxes through grants, staging or pushed bytes, so the configured store's credentials, the caller's own, stay with the caller.""" diff --git a/tst/unit/store/gcs_object_store_test.py b/tst/unit/store/gcs_object_store_test.py index c9304075..e1c7a10d 100644 --- a/tst/unit/store/gcs_object_store_test.py +++ b/tst/unit/store/gcs_object_store_test.py @@ -511,24 +511,12 @@ def test_an_upload_policy_lasts_at_most_what_its_signer_can_sign(store, client): def test_without_a_signer_there_is_no_upload_policy(unsigned): with pytest.raises(GrantUnavailableError, match="no signer"): unsigned.issue_upload_policy(f"gs://{BUCKET}/ns/", max_object_bytes=8, expires_in=600) - assert unsigned.signed_post(f"gs://{BUCKET}/ns/") is None - - -def test_signed_post_is_shaped_as_the_s3_store_s_and_clamped_to_the_signer(store, client): - post = store.signed_post(f"gs://{BUCKET}/snap/", expires_in=10**7) - assert set(post) == {"url", "fields"} and post["fields"]["key"] == "snap/${filename}" - document = _policy(post["fields"]) - assert _expiration(document) <= datetime.now(UTC) + timedelta(seconds=43200) - assert not any(isinstance(c, list) and c[0] == "content-length-range" for c in document["conditions"]) - slashless = _policy(store.signed_post(f"gs://{BUCKET}/snap")["fields"]) - assert ["starts-with", "$key", "snap/"] in slashless["conditions"] - root = store.signed_post(f"gs://{BUCKET}/")["fields"] - assert root["key"] == "${filename}" and ["starts-with", "$key", ""] in _policy(root)["conditions"] - bounded = _policy(store.signed_post(f"gs://{BUCKET}/snap/", max_bytes=5)["fields"]) - assert ["content-length-range", 0, 5] in bounded["conditions"] - keyed = GcsObjectStore(client, BUCKET, signer=_key_credentials()) - expiration = _expiration(_policy(keyed.signed_post(f"gs://{BUCKET}/snap/", expires_in=10**7)["fields"])) - assert datetime.now(UTC) + timedelta(seconds=43200) < expiration <= datetime.now(UTC) + timedelta(seconds=604800) + + +def test_an_upload_policy_at_the_bucket_root_admits_every_key(store, signer): + write = store.issue_upload_policy(f"gs://{BUCKET}/", max_object_bytes=8, expires_in=600).write + assert write.fields["key"] == "${filename}" + assert ["starts-with", "$key", ""] in _policy(write.fields)["conditions"] def test_changelog_capture_gets_the_object_form(store): diff --git a/tst/unit/store/s3_signed_post_test.py b/tst/unit/store/s3_signed_post_test.py deleted file mode 100644 index 39b9680e..00000000 --- a/tst/unit/store/s3_signed_post_test.py +++ /dev/null @@ -1,86 +0,0 @@ -"""``ObjectStore.signed_post``: the policy handed to boto3, not S3's response to -it — the round trip is integration territory.""" -from __future__ import annotations - -from agent_env.store.object_store.local.store import LocalFilesystemObjectStore -from agent_env.store.object_store.s3_object_store import S3ObjectStore - -BUCKET = "artifact-bucket" - - -class _StubClient: - def __init__(self): - self.calls: list[dict] = [] - - def generate_presigned_post(self, **kwargs): - self.calls.append(kwargs) - return {"url": f"https://{BUCKET}.s3.amazonaws.com/", "fields": {"key": kwargs["Key"]}} - - -def _sign(prefix: str = "captures/one/", **kwargs): - client = _StubClient() - store = S3ObjectStore(client, BUCKET) - grant = store.signed_post(f"s3://{BUCKET}/{prefix}", **kwargs) - return client.calls[0], grant - - -def test_the_key_is_a_template_the_uploader_substitutes(): - call, _ = _sign() - assert call["Bucket"] == BUCKET - assert call["Key"] == "captures/one/${filename}" - 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() - assert ["starts-with", "$Content-Type", ""] in call["Conditions"] - - -def test_no_size_bound_unless_asked_for(): - call, _ = _sign() - assert not [c for c in call["Conditions"] if c[0] == "content-length-range"] - - -def test_max_bytes_becomes_a_content_length_range(): - """The one guard a signed url cannot express.""" - call, _ = _sign(max_bytes=1024) - assert ["content-length-range", 0, 1024] in call["Conditions"] - - -def test_the_grant_is_the_fields_not_the_url(): - _, grant = _sign() - assert set(grant) == {"url", "fields"} - # The url is the bare bucket endpoint; it carries no authorization. - assert grant["url"].rstrip("/").endswith(".s3.amazonaws.com") - - -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" diff --git a/tst/unit/task/local_run_write_fence_test.py b/tst/unit/task/local_run_write_fence_test.py index 477d728b..a2d292dc 100644 --- a/tst/unit/task/local_run_write_fence_test.py +++ b/tst/unit/task/local_run_write_fence_test.py @@ -36,7 +36,7 @@ LOCAL_TASK = "@local/~/bundle/tasks/t" LOCAL_ENV = "@local/~/bundle/envs/tickets" _DOCUMENT_WRITES = ("insert", "update", "update_one_and_get", "replace", "delete") -_OBJECT_WRITES = ("put", "put_file", "put_file_at", "signed_put_url", "signed_post", "issue_write_grant", "issue_upload_policy") +_OBJECT_WRITES = ("put", "put_file", "put_file_at", "signed_put_url", "issue_write_grant", "issue_upload_policy") def _refusing(cls, methods):