pkg/atomicwriter: disallow symlinks for now, add more tests and touch-up GoDoc#49741
Merged
thaJeztah merged 5 commits intomoby:masterfrom Apr 4, 2025
Merged
Conversation
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The implementation uses "os.Rename" to move the temporary file to the destination, which does not follow symlinks, and because of this would replace a symlink with a file. We can consider adding support for symlinked files in future, so that WriteFile can be used as a drop-in replacement for `os.WriteFile()` but in the meantime, let's produce an error so that nobody can depend on this. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Previously, we were silently discarding this situation and hoping that it would work; let's produce an error instead (we can add additional filemodes when they arrive and if we need them) Signed-off-by: Sebastiaan van Stijn <[email protected]>
While the target-file does not have to exist, its parent must, and must be a directory. This adds a test-case to verify the behavior if the parent is not a directory. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Rewrite `validateDestination` to first check if the destination path
exists. This slightly simplifies the logic (allowing returning
early in each step of the validation) and slightly improves the
error produced.
Before this, the error confusingly would mention the full path
not being a directory. While this _does_ match what `os.Writefile`
would return, it's .. confusing:
failed to stat output path: lstat ./not-a-dir/new-file.txt: not a directory
After this, the error would mention the directory that doesn't exist:
invalid output path: stat ./not-a-dir: not a directory
A slight optimization is made as well, now checking for _both_ "."
and ".." as special case, as either path should exist given any current
working directory (unless the working directory has been deleted, but we'd
fail further down the line).
With this change in order, we can also merge `validateFileMode` into
`validateDestination`.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
kind/enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
vvoland
approved these changes
Apr 4, 2025
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
3 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.
pkg/atomicwriter: disallow symlinked files for now
The implementation uses "os.Rename" to move the temporary file to
the destination, which does not follow symlinks, and because of this
would replace a symlink with a file.
We can consider adding support for symlinked files in future, so that
WriteFile can be used as a drop-in replacement for
os.WriteFile()but in the meantime, let's produce an error so that nobody can depend
on this.
pkg/atomicwriter: error on unknown file-modes
Previously, we were silently discarding this situation and hoping that
it would work; let's produce an error instead (we can add additional
filemodes when they arrive and if we need them)
pkg/atomicwriter: add test for parent dir not being a directory
While the target-file does not have to exist, its parent must, and must
be a directory. This adds a test-case to verify the behavior if the
parent is not a directory.
pkg/atomicwriter: return early if parent directory is invalid
Rewrite
validateDestinationto first check if the destination pathexists. This slightly simplifies the logic (allowing returning
early in each step of the validation) and slightly improves the
error produced.
Before this, the error confusingly would mention the full path
not being a directory. While this does match what
os.Writefilewould return, it's .. confusing:
After this, the error would mention the directory that doesn't exist:
A slight optimization is made as well, now checking for both "."
and ".." as special case, as either path should exist given any current
working directory (unless the working directory has been deleted, but we'd
fail further down the line).
With this change in order, we can also merge
validateFileModeintovalidateDestination.- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)