diff --git a/apps/api/plane/settings/storage.py b/apps/api/plane/settings/storage.py index e4a978bd2b1..4e970c3709c 100644 --- a/apps/api/plane/settings/storage.py +++ b/apps/api/plane/settings/storage.py @@ -68,19 +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 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 = [ - {"bucket": self.aws_storage_bucket_name}, ["content-length-range", 1, file_size], {"Content-Type": file_type}, ] - # Add condition for the object name (key) - if object_name.startswith("${filename}"): - 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: # 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 00856aeecb6..5485d3b5b69 100644 --- a/apps/api/plane/tests/unit/settings/test_storage.py +++ b/apps/api/plane/tests/unit/settings/test_storage.py @@ -116,6 +116,89 @@ 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}" + # 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, + { + "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, {