From 69f50ac0ba53e11aa5b337a95573e2716b35c683 Mon Sep 17 00:00:00 2001 From: Dmitry Volodchenkov Date: Mon, 4 May 2026 00:31:28 +0300 Subject: [PATCH 1/3] fix(api): drop duplicate {bucket}/{key} conditions from presigned-POST policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `boto3.generate_presigned_post()` automatically adds `{"bucket": ...}` and `{"key": ...}` conditions to the policy whenever `Bucket=` and `Key=` are passed as named arguments. The current code adds them by hand in `conditions=` *as well*, so every generated policy carries each condition twice and is roughly 80 bytes larger than it needs to be. AWS S3 silently accepts the over-sized policy, but stricter S3-compatible backends do not — some enforce a `policy` field limit as low as 1024 bytes and reject the upload with HTTP 400: MaxMessageLengthExceeded policy 1024 1160 The duplication makes attachment upload unusable on those backends. This change removes the two hand-rolled conditions and lets boto3 add them itself. Resulting policy size drops from ~1160 to ~564 bytes for a typical attachment, well under every documented per-backend limit. Behaviour on AWS S3 is unchanged. Adds a regression test asserting that `Conditions=` no longer contains hand-rolled `bucket`/`key` entries. --- apps/api/plane/settings/storage.py | 8 +++- .../plane/tests/unit/settings/test_storage.py | 41 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/apps/api/plane/settings/storage.py b/apps/api/plane/settings/storage.py index e4a978bd2b1..ed3d46b44e0 100644 --- a/apps/api/plane/settings/storage.py +++ b/apps/api/plane/settings/storage.py @@ -68,8 +68,13 @@ def generate_presigned_post(self, object_name, file_type, file_size, expiration= expiration = self.signed_url_expiration fields = {"Content-Type": file_type} + # boto3.generate_presigned_post auto-injects {"bucket": ...} and + # {"key": ...} conditions when Bucket=/Key= are passed below; do NOT + # also add them by hand here. AWS S3 silently accepts the resulting + # over-sized policy, but stricter S3-compatible backends enforce a + # hard cap on the `policy` form field (some as low as 1024 bytes) + # and reject the upload with `MaxMessageLengthExceeded`. conditions = [ - {"bucket": self.aws_storage_bucket_name}, ["content-length-range", 1, file_size], {"Content-Type": file_type}, ] @@ -79,7 +84,6 @@ def generate_presigned_post(self, object_name, file_type, file_size, expiration= conditions.append(["starts-with", "$key", object_name[: -len("${filename}")]]) else: fields["key"] = object_name - conditions.append({"key": object_name}) # Generate the presigned POST URL try: diff --git a/apps/api/plane/tests/unit/settings/test_storage.py b/apps/api/plane/tests/unit/settings/test_storage.py index 00856aeecb6..a4a7915246e 100644 --- a/apps/api/plane/tests/unit/settings/test_storage.py +++ b/apps/api/plane/tests/unit/settings/test_storage.py @@ -116,6 +116,47 @@ def test_generate_presigned_post_uses_custom_expiration(self, mock_boto3): call_kwargs = mock_s3_client.generate_presigned_post.call_args[1] assert call_kwargs["ExpiresIn"] == 60 + @patch.dict( + os.environ, + { + "AWS_ACCESS_KEY_ID": "test-key", + "AWS_SECRET_ACCESS_KEY": "test-secret", + "AWS_S3_BUCKET_NAME": "test-bucket", + "AWS_REGION": "us-east-1", + }, + clear=True, + ) + @patch("plane.settings.storage.boto3") + def test_generate_presigned_post_does_not_duplicate_bucket_or_key(self, mock_boto3): + """boto3 auto-injects {bucket}/{key} when Bucket=/Key= are passed. + + Adding them again by hand pads the policy by ~80 bytes — a no-op on + AWS S3, but enough to overflow stricter S3-compatible backends that + cap the `policy` field at 1024 bytes and reject the upload with + `MaxMessageLengthExceeded`. + + This is the regression test for that fix. + """ + mock_s3_client = Mock() + mock_s3_client.generate_presigned_post.return_value = { + "url": "https://test-url.com", + "fields": {}, + } + mock_boto3.client.return_value = mock_s3_client + storage = S3Storage() + + storage.generate_presigned_post("test-object", "image/png", 1024) + + kw = mock_s3_client.generate_presigned_post.call_args[1] + # Bucket+Key must be passed as named args (boto3 derives conditions from them). + assert kw["Bucket"] == "test-bucket" + assert kw["Key"] == "test-object" + # Conditions must NOT contain hand-rolled {"bucket": ...} or {"key": ...}. + for cond in kw["Conditions"]: + if isinstance(cond, dict): + assert "bucket" not in cond, f"hand-rolled bucket condition: {cond}" + assert "key" not in cond, f"hand-rolled key condition: {cond}" + @patch.dict( os.environ, { From fde7baed351614be9324ae841d0725d4e25606de Mon Sep 17 00:00:00 2001 From: Dmitry Volodchenkov Date: Tue, 5 May 2026 00:30:47 +0300 Subject: [PATCH 2/3] fix(api): also drop now-redundant manual fields["key"] = object_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per CodeRabbit review on PR #9005: with Bucket=/Key= passed to boto3.generate_presigned_post() below, boto3 already adds the `key` entry to the returned `fields[]` itself — no need to set it by hand. Verified empirically: same returned `fields`, same policy size, same signature either way. The `${filename}` branch still needs its `starts-with` condition; boto3 cannot derive that from a literal Key= value. Comment updated to make that clear. Test asserts `Conditions[]` and `Fields[]` are both free of hand-rolled `key` entries. --- apps/api/plane/settings/storage.py | 8 +++++--- apps/api/plane/tests/unit/settings/test_storage.py | 3 +++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/api/plane/settings/storage.py b/apps/api/plane/settings/storage.py index ed3d46b44e0..677259f12f3 100644 --- a/apps/api/plane/settings/storage.py +++ b/apps/api/plane/settings/storage.py @@ -79,11 +79,13 @@ def generate_presigned_post(self, object_name, file_type, file_size, expiration= {"Content-Type": file_type}, ] - # Add condition for the object name (key) + # For prefix uploads (`${filename}` placeholder) we still need a + # `starts-with` condition; boto3 cannot derive that from `Key=`. + # For exact-key uploads, `Key=` below also makes boto3 add + # `"key": ` to the returned `fields[]` automatically — no + # manual injection needed. if object_name.startswith("${filename}"): conditions.append(["starts-with", "$key", object_name[: -len("${filename}")]]) - else: - fields["key"] = object_name # Generate the presigned POST URL try: diff --git a/apps/api/plane/tests/unit/settings/test_storage.py b/apps/api/plane/tests/unit/settings/test_storage.py index a4a7915246e..3355713af45 100644 --- a/apps/api/plane/tests/unit/settings/test_storage.py +++ b/apps/api/plane/tests/unit/settings/test_storage.py @@ -156,6 +156,9 @@ def test_generate_presigned_post_does_not_duplicate_bucket_or_key(self, mock_bot if isinstance(cond, dict): assert "bucket" not in cond, f"hand-rolled bucket condition: {cond}" assert "key" not in cond, f"hand-rolled key condition: {cond}" + # Fields must NOT contain a hand-rolled `key` either (boto3 derives + # it from Key= and adds it to the returned form fields itself). + assert "key" not in kw["Fields"], f"hand-rolled key field: {kw['Fields']}" @patch.dict( os.environ, From ee3d16be7b00378a1b1ee385a1ee9d43d7d1169a Mon Sep 17 00:00:00 2001 From: Dmitry Volodchenkov Date: Tue, 5 May 2026 01:01:11 +0300 Subject: [PATCH 3/3] =?UTF-8?q?fix(api):=20drop=20dead=20`${filename}`=20b?= =?UTF-8?q?ranch=20=E2=80=94=20boto3=20derives=20starts-with=20itself?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per second CodeRabbit review pass on PR #9005: 1. boto3.generate_presigned_post auto-injects ["starts-with", "$key", ] into the policy whenever the supplied Key ends with the literal "${filename}" placeholder. Verified empirically against a live S3 endpoint: Key="prefix/${filename}", Conditions=[…no starts-with…] → returned policy contains ["starts-with", "$key", "prefix/"] Adding the same condition by hand produced two identical ["starts-with", "$key", "prefix/"] entries. 2. The pre-existing branch was also logically broken: it tested `object_name.startswith("${filename}")`, while botocore's actual detection is `endswith()`. For the typical `prefix/${filename}` case `startswith()` is False — the branch never fired. For the degenerate `${filename}` (no prefix) case it produced `["starts-with", "$key", ""]` (matches any key), which is meaningless. 3. No call site in this codebase passes an `object_name` containing `${filename}` — every caller in `apps/api/plane/{api,app,space}/views/` resolves the asset key to a concrete UUID string before calling `generate_presigned_post`. The branch was dead. Removes the conditional and the "still need a starts-with" comment. Adds a companion regression test asserting the wrapper passes Key= through unchanged and does not inject any starts-with into Conditions. --- apps/api/plane/settings/storage.py | 22 ++++------- .../plane/tests/unit/settings/test_storage.py | 39 +++++++++++++++++++ 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/apps/api/plane/settings/storage.py b/apps/api/plane/settings/storage.py index 677259f12f3..4e970c3709c 100644 --- a/apps/api/plane/settings/storage.py +++ b/apps/api/plane/settings/storage.py @@ -68,25 +68,19 @@ def generate_presigned_post(self, object_name, file_type, file_size, expiration= expiration = self.signed_url_expiration fields = {"Content-Type": file_type} - # boto3.generate_presigned_post auto-injects {"bucket": ...} and - # {"key": ...} conditions when Bucket=/Key= are passed below; do NOT - # also add them by hand here. AWS S3 silently accepts the resulting - # over-sized policy, but stricter S3-compatible backends enforce a - # hard cap on the `policy` form field (some as low as 1024 bytes) - # and reject the upload with `MaxMessageLengthExceeded`. + # boto3.generate_presigned_post derives every key-related condition + # itself: it injects {"bucket": ...}, {"key": }, and (when + # `Key` ends with the literal "${filename}" placeholder) the + # corresponding ["starts-with", "$key", ] entry. Do NOT add + # any of them by hand here — AWS S3 silently accepts the resulting + # duplicates, but stricter S3-compatible backends enforce a hard + # cap on the `policy` form field (some as low as 1024 bytes) and + # reject the upload with `MaxMessageLengthExceeded`. conditions = [ ["content-length-range", 1, file_size], {"Content-Type": file_type}, ] - # For prefix uploads (`${filename}` placeholder) we still need a - # `starts-with` condition; boto3 cannot derive that from `Key=`. - # For exact-key uploads, `Key=` below also makes boto3 add - # `"key": ` to the returned `fields[]` automatically — no - # manual injection needed. - if object_name.startswith("${filename}"): - conditions.append(["starts-with", "$key", object_name[: -len("${filename}")]]) - # Generate the presigned POST URL try: # Generate a presigned URL for the S3 object diff --git a/apps/api/plane/tests/unit/settings/test_storage.py b/apps/api/plane/tests/unit/settings/test_storage.py index 3355713af45..5485d3b5b69 100644 --- a/apps/api/plane/tests/unit/settings/test_storage.py +++ b/apps/api/plane/tests/unit/settings/test_storage.py @@ -160,6 +160,45 @@ def test_generate_presigned_post_does_not_duplicate_bucket_or_key(self, mock_bot # it from Key= and adds it to the returned form fields itself). assert "key" not in kw["Fields"], f"hand-rolled key field: {kw['Fields']}" + @patch.dict( + os.environ, + { + "AWS_ACCESS_KEY_ID": "test-key", + "AWS_SECRET_ACCESS_KEY": "test-secret", + "AWS_S3_BUCKET_NAME": "test-bucket", + "AWS_REGION": "us-east-1", + }, + clear=True, + ) + @patch("plane.settings.storage.boto3") + def test_generate_presigned_post_no_manual_starts_with_for_filename_placeholder( + self, mock_boto3 + ): + """Boto3 derives ["starts-with", "$key", ] itself when Key + ends with the literal "${filename}". The wrapper must not add it + again, or the policy carries duplicate starts-with entries. + """ + mock_s3_client = Mock() + mock_s3_client.generate_presigned_post.return_value = { + "url": "https://test-url.com", + "fields": {}, + } + mock_boto3.client.return_value = mock_s3_client + storage = S3Storage() + + storage.generate_presigned_post("uploads/${filename}", "image/png", 1024) + + kw = mock_s3_client.generate_presigned_post.call_args[1] + # Key is forwarded as-is; boto3 will see the placeholder and inject + # the starts-with condition itself. + assert kw["Key"] == "uploads/${filename}" + # Conditions must NOT contain a hand-rolled starts-with. + for cond in kw["Conditions"]: + if isinstance(cond, list) and cond and cond[0] == "starts-with": + raise AssertionError( + f"hand-rolled starts-with leaked into Conditions: {cond}" + ) + @patch.dict( os.environ, {