Fix S3 uploads landing private on ACL-based providers (opt-in public-read) - #352
Open
clydesantiago wants to merge 1 commit into
Open
Fix S3 uploads landing private on ACL-based providers (opt-in public-read)#352clydesantiago wants to merge 1 commit into
clydesantiago wants to merge 1 commit into
Conversation
S3Uploader never sent an ACL header, so on providers that honour object ACLs (AWS S3, DigitalOcean Spaces, MinIO, Backblaze B2) every upload landed private and the link macshot copies to the clipboard returned 403 AccessDenied. The upload itself succeeds, so nothing surfaces as an error and Test Connection still reports success. Add an "Make uploads publicly readable" checkbox to the S3 settings section (s3PublicRead, default off). When enabled the PUT carries x-amz-acl: public-read, and the header is added to the SigV4 signed header list so the signature still validates. Off by default because Cloudflare R2 has no object ACLs and rejects the header, and because making uploads world-readable should be a deliberate choice rather than a silent default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Problem
S3Uploadernever sends an ACL header. The signed header list is fixed at four entries:On any provider that honours object ACLs (AWS S3, DigitalOcean Spaces, MinIO, Backblaze B2), objects default to private. The upload succeeds with a 200,
Test Connectionreports "Connection successful!", and macshot copies a link to the clipboard that returns403 AccessDeniedwhen anyone opens it.Reproduced against DigitalOcean Spaces:
There is currently no way to fix this from inside macshot. The only workaround is a provider-side bucket policy.
This does not affect Cloudflare R2, which serves public objects through r2.dev or a custom domain and has no ACLs at all, which is likely why it went unnoticed.
Change
Adds a "Make uploads publicly readable" checkbox to the S3 settings section, backed by
s3PublicRead, default off.When enabled:
x-amz-acl: public-readx-amz-aclis appended to the SigV4 signed header list and re-sorted, so the signature still validatesWhen disabled the request is byte-for-byte what it is today.
Off by default for two reasons: R2 rejects the header outright, and making uploads world-readable should be a deliberate choice rather than a silent default.
Files
macshot/Upload/S3Uploader.swiftConfig.publicRead, conditional header, conditional signingmacshot/UI/Windows/SettingsWindowController.swifts3PublicReadChanged(_:)macshot/en.lproj/Localizable.stringsDeployment notes
UserDefaults.bool(forKey:)returnsfalsefor an unset key, so existing installs keep exactly today's behaviour until the user ticks the box.SettingsPortability.secretSubstringsalready contains"s3", sos3PublicReadis excluded from exports automatically by the existing fail-closed rule.s3cmd setacl --acl-public --recursiveor a bucket policy./translate-missingis run.Testing
swiftc -parseclean on both changed files;plutil -lintclean on the strings file.I do not have Xcode set up for a full build of this project, so please give it a compile before merging.