prevent removal of a PublishedStorage's root dir #705
Merged
Conversation
This file contains 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
2 tasks
smira
reviewed
Feb 22, 2018
files/public_test.go
Outdated
|
|
||
| // Actual dir | ||
| dirStorage := NewPublishedStorage(pwd, "", "") | ||
| c.Assert(func() { dirStorage.RemoveDirs("", nil) }, PanicMatches, "trying to remove empty path") |
There was a problem hiding this comment.
panic message doesn't match the code (trying to remove the root directory)
presently there is no use case where we need this. on the other hand, passing empty paths into any of the remove methods is indicative of a bug. this is particularly dangerous as this can temporarily smash the publish root but later restore it again when actually publishing. this makes for super nasty and hard to track down problems. to guard against this simply disallow root dir removal using empty strings. should we find a use case for this in the future we can always revisit this (FTR: I think very explicitly API should be used so everyone knows what is going on and you can't accidentally run it)
hsitter
force-pushed
the
prevent-root-remove
branch
from
9bcdaf0
to
b893c0a
Compare
|
Ah yes indeed. Rebased and fixed. |
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.
Requirements
All new code should be covered with tests, documentation should be updated. CI should pass.
Description of the Change
presently there is no use case where we need this. on the other hand,
passing empty paths into any of the remove methods is indicative of a bug.
this is particularly dangerous as this can temporarily smash the publish
root but later restore it again when actually publishing. this makes
for super nasty and hard to track down problems.
to guard against this simply disallow root dir removal using empty
strings. should we find a use case for this in the future we can always
revisit this (FTR: I think very explicitly API should be used so everyone
knows what is going on and you can't accidentally run it)
Checklist