Skip to content

fix(s3): apply the ACL and the source conditions a copy carries - #1482

Open
gaul wants to merge 1 commit into
ministackorg:mainfrom
gaul:s3-copy-acl-and-conditions
Open

fix(s3): apply the ACL and the source conditions a copy carries#1482
gaul wants to merge 1 commit into
ministackorg:mainfrom
gaul:s3-copy-acl-and-conditions

Conversation

@gaul

@gaul gaul commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

CopyObject dropped x-amz-acl, so a copy addressed public-read landed private and stayed that way -- the caller had no way to tell except by reading the ACL back. It now permissions the destination as a put does, and refuses an unknown value rather than ignoring it. A copy without one leaves the destination private rather than inheriting whatever the key it replaced carried.

UploadPartCopy ignored all four x-amz-copy-source-if-* headers and copied the range regardless, which is the hazard a conditional read exists to prevent: the part is assembled from a source that has since changed. CopyObject already judged them correctly, so the block moves into a helper both operations ask -- one place to be right, and no chance of the two drifting apart.

CopyObject dropped x-amz-acl, so a copy addressed public-read landed
private and stayed that way -- the caller had no way to tell except by
reading the ACL back.  It now permissions the destination as a put
does, and refuses an unknown value rather than ignoring it.  A copy
without one leaves the destination private rather than inheriting
whatever the key it replaced carried.

UploadPartCopy ignored all four x-amz-copy-source-if-* headers and
copied the range regardless, which is the hazard a conditional read
exists to prevent: the part is assembled from a source that has since
changed.  CopyObject already judged them correctly, so the block moves
into a helper both operations ask -- one place to be right, and no
chance of the two drifting apart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Docker image for this PR has been published: ministackorg/ministack-preview-build:pr-1482-e84649d9

@Nahuel990

Copy link
Copy Markdown
Collaborator

@gaul You are inferrinf these from AI. To avoid the back-and-forth that happened in 1451, please first raise an issue with the repro steps showing where Ministack drifted from AWS. Once that’s identified, we can fix it directly. I'm closing these ones

@gaul

gaul commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

I filed the requested issues. All of my PRs were sourced from s3-tests running against S3Proxy running against ministack. I apologize for the noise of 1451 but generally these PRs are grounded in a reality, albeit Ceph's reality. It would be good to include s3-tests in ministack's CI now that most issues are fixed. I have already included ministack alongside localstack in S3Proxy's CI with a handful of s3-tests exclusions.

@Nahuel990 Nahuel990 reopened this Aug 25, 2026
@github-actions

Copy link
Copy Markdown

Docker image for this PR has been published: ministackorg/ministack-preview-build:pr-1482-e84649d9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants