Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions apps/api/plane/settings/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,24 @@ 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},
]

# 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=<value>` below also makes boto3 add
# `"key": <value>` to the returned `fields[]` automatically — no
# manual injection needed.
if object_name.startswith("${filename}"):
conditions.append(["starts-with", "$key", object_name[: -len("${filename}")]])
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
else:
fields["key"] = object_name
conditions.append({"key": object_name})

# Generate the presigned POST URL
try:
Expand Down
44 changes: 44 additions & 0 deletions apps/api/plane/tests/unit/settings/test_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,50 @@ 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,
{
Expand Down