fix: set 0644 permissions on uploaded asset files#1160
Merged
Conversation
os.CreateTemp defaults to 0600, so uploaded files kept owner-only permissions after the atomic rename. Add a perm parameter to WriteStreamAtomic (mirroring WriteFileAtomic) and call Chmod before writing, so assets and branding files land with world-readable 0644. Fixes #1158
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1158 by ensuring uploaded asset and branding files end up with consistent, world-readable permissions (0644) after being written via an atomic temp-file + rename flow (since os.CreateTemp defaults to 0600).
Changes:
- Extend
shared.WriteStreamAtomicto accept aperm os.FileModeand apply it to the temp file used for atomic writes. - Update asset upload and branding upload paths to pass
0o644. - Extend
WriteStreamAtomicunit test to assert the resulting file permissions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/core/shared/utils.go | Adds a perm parameter to WriteStreamAtomic and applies Chmod to the temp file used for atomic writes. |
| internal/core/shared/utils_test.go | Updates WriteStreamAtomic test to pass 0o644 and assert resulting permissions. |
| internal/core/assets/assets_service.go | Ensures uploaded page assets are written with 0o644. |
| internal/branding/branding_service.go | Ensures uploaded branding logo and favicon are written with 0o644. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move Chmod to after CopyWithLimit+Sync so partial uploads stay at 0600 until fully written. Skip the Mode().Perm() assertion on Windows where Unix permission bits are not reliably represented.
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.
os.CreateTemp defaults to 0600, so uploaded files kept owner-only permissions after the atomic rename. Add a perm parameter to WriteStreamAtomic (mirroring WriteFileAtomic) and call Chmod before writing, so assets and branding files land with world-readable 0644.
Fixes #1158