fix(api): drop duplicate {bucket}/{key} conditions from presigned-POST policy#9005
Open
volodchenkov wants to merge 3 commits intomakeplane:previewfrom
Open
fix(api): drop duplicate {bucket}/{key} conditions from presigned-POST policy#9005volodchenkov wants to merge 3 commits intomakeplane:previewfrom
volodchenkov wants to merge 3 commits intomakeplane:previewfrom
Conversation
…T policy
`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:
<Error>
<Code>MaxMessageLengthExceeded</Code>
<Attribute>policy</Attribute>
<MaxSizeAllowed>1024</MaxSizeAllowed>
<ProposedSize>1160</ProposedSize>
</Error>
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.
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemove manually added S3 bucket/key and ChangesS3 Presigned POST Policy Fix
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/settings/storage.py (1)
71-86: ⚡ Quick winDrop the remaining manual
keyfield assignment, or verify it is still required.Botocore’s
generate_presigned_postdocs say key-related fields and conditions should not be included inFieldsorConditions; this change removes the duplicate bucket/key conditions, butfields["key"]is still injected later in the non-${filename}path, which looks redundant and may keep the implementation off the documented path. (botocore.amazonaws.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/settings/storage.py` around lines 71 - 86, The code still injects a manual key into fields when object_name does not start with "${filename}" which contradicts botocore's generate_presigned_post guidance; remove the fields["key"] = object_name assignment in the else branch (or, if you have a special non-standard S3 backend that requires it, gate that assignment behind an explicit configuration flag) so that only conditions and fields generated by boto3/botocore are used; update any tests that assumed the manual key to rely on the presigned post behavior from generate_presigned_post (refer to object_name, fields, conditions, and generate_presigned_post in the surrounding code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/plane/settings/storage.py`:
- Around line 71-86: The code still injects a manual key into fields when
object_name does not start with "${filename}" which contradicts botocore's
generate_presigned_post guidance; remove the fields["key"] = object_name
assignment in the else branch (or, if you have a special non-standard S3 backend
that requires it, gate that assignment behind an explicit configuration flag) so
that only conditions and fields generated by boto3/botocore are used; update any
tests that assumed the manual key to rely on the presigned post behavior from
generate_presigned_post (refer to object_name, fields, conditions, and
generate_presigned_post in the surrounding code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c93bbce-3518-485b-96c3-2ba2a7c9190d
📒 Files selected for processing (2)
apps/api/plane/settings/storage.pyapps/api/plane/tests/unit/settings/test_storage.py
Per CodeRabbit review on PR makeplane#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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/tests/unit/settings/test_storage.py (1)
119-162: ⚡ Quick winNew test is well-structured but doesn't cover the
${filename}upload path.The test correctly validates that the exact-key path (
"test-object") produces no hand-rolledbucket/keyconditions. However, there's no test for theobject_name.startswith("${filename}")branch. Given the finding above — that this branch may produce a duplicatestarts-withcondition that botocore already injects — a companion test would:
- Catch the redundant condition today, and
- Serve as a regression guard if the branch is removed.
➕ Suggested additional test for the `${filename}` path
`@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_filename_placeholder_no_duplicate_starts_with(self, mock_boto3): """For ${filename} keys boto3/botocore auto-injects the starts-with condition. No manual duplicate should appear in Conditions.""" 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("${filename}", "image/png", 1024) kw = mock_s3_client.generate_presigned_post.call_args[1] assert kw["Key"] == "${filename}" starts_with_key_conditions = [ c for c in kw["Conditions"] if isinstance(c, list) and len(c) == 3 and c[0] == "starts-with" and c[1] == "$key" ] # botocore will inject one; we must not inject a second. assert len(starts_with_key_conditions) <= 1, ( f"duplicate starts-with $key conditions: {starts_with_key_conditions}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/unit/settings/test_storage.py` around lines 119 - 162, Add a new unit test for the object_name.startswith("${filename}") branch of S3Storage.generate_presigned_post to ensure we don't inject a duplicate starts-with condition; create a test (e.g. test_generate_presigned_post_filename_placeholder_no_duplicate_starts_with) that patches os.environ and boto3 like the existing test, calls storage.generate_presigned_post("${filename}", "image/png", 1024), captures mock_s3_client.generate_presigned_post.call_args[1], asserts kw["Key"] == "${filename}", collects any ["starts-with", "$key", ...] conditions from kw["Conditions"] and asserts there is at most one, and also assert kw["Fields"] does not contain a hand-rolled "key" field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/settings/storage.py`:
- Around line 82-88: Remove the incorrect startswith check and its associated
comment in the presigned-post construction (the block that inspects object_name
and appends ["starts-with", "$key", ...]); botocore/boto3's
generate_presigned_post already handles keys ending with '${filename}' (it
derives the starts-with condition via an endswith check), so delete the if
object_name.startswith("${filename}"): ... conditions.append(...) block and
replace the comment with a brief note that botocore generates the required
starts-with condition for keys containing the '${filename}' placeholder (no
manual injection needed). Ensure references to object_name and the conditions
list remain otherwise unchanged.
---
Nitpick comments:
In `@apps/api/plane/tests/unit/settings/test_storage.py`:
- Around line 119-162: Add a new unit test for the
object_name.startswith("${filename}") branch of
S3Storage.generate_presigned_post to ensure we don't inject a duplicate
starts-with condition; create a test (e.g.
test_generate_presigned_post_filename_placeholder_no_duplicate_starts_with) that
patches os.environ and boto3 like the existing test, calls
storage.generate_presigned_post("${filename}", "image/png", 1024), captures
mock_s3_client.generate_presigned_post.call_args[1], asserts kw["Key"] ==
"${filename}", collects any ["starts-with", "$key", ...] conditions from
kw["Conditions"] and asserts there is at most one, and also assert kw["Fields"]
does not contain a hand-rolled "key" field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43e97853-67ce-403f-80a6-4311d95bf68e
📒 Files selected for processing (2)
apps/api/plane/settings/storage.pyapps/api/plane/tests/unit/settings/test_storage.py
…itself Per second CodeRabbit review pass on PR makeplane#9005: 1. boto3.generate_presigned_post auto-injects ["starts-with", "$key", <prefix>] 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
boto3.generate_presigned_post()automatically adds{"bucket": ...}and{"key": ...}conditions to the generated policy wheneverBucket=andKey=are passed as named arguments — see the [boto3 docs][1].S3Storage.generate_presigned_postadds them again by hand inconditions=, so the policy carries each condition twice and is ~80 byteslarger than it needs to be.
AWS S3 silently accepts the over-sized policy. Some S3-compatible backends
do not — at least one mainstream vendor enforces a hard cap of 1024
bytes on the
policyform field and rejects the upload with HTTP 400:The duplication makes attachment upload completely broken on those
backends.
After this fix, the resulting policy drops from ~1160 to ~564 bytes for a
typical attachment, well under every documented per-backend limit.
Behaviour on AWS S3 is unchanged (boto3 still injects the same conditions
on its own).
Type of Change
Test Scenarios
asserts that Conditions= no longer contains hand-rolled
{"bucket": ...} / {"key": ...} entries.
S3-compatible storage that previously rejected uploads with
MaxMessageLengthExceeded: file attachments now upload successfully.
contains the required bucket/key conditions (boto3 injects them) and
presigned uploads succeed.
References
None — no prior issue. Bug discovered while running self-hosted Plane on
an S3-compatible storage with strict policy size limits.
Summary by CodeRabbit
Bug Fixes
Tests