Forbid drop operation from symlink'ed annex (e.g. due to being cloned with --reckless=ephemeral) to prevent data-loss #6959
Conversation
mih
added
severity-critical
I agree |
Codecov Report
@@ Coverage Diff @@
## maint #6959 +/- ##
==========================================
+ Coverage 89.97% 90.96% +0.98%
==========================================
Files 354 354
Lines 46259 46290 +31
==========================================
+ Hits 41622 42108 +486
+ Misses 4637 4182 -455
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Crippledfs where no symlinjs supported failure is relevant |
| @@ -727,6 +727,9 @@ def _kill_dataset(ds): | |||
| def _drop_allkeys(ds, repo, force=False, jobs=None): | |||
| """ | |||
| """ | |||
| assert not (repo.dot_git / 'annex').is_symlink(), \ | |||
| "Dropping from a symlinked annex is unsupported to prevent data-loss" | |||
There was a problem hiding this comment.
In general I am good with such a stop gap feature (after testing is fixed up on crippled fs). But also such message could confuse some naive user a little. We might better rely first on
[datalad "clone"]
reckless = ephemeral
being present in repo.config -- and inform user that dropping in ephemeral clone (the term they must have heard about since they had to use it to get here) is not supported. But indeed checking for symlink is more robust and could be the 2nd line of defense if so desired.
There was a problem hiding this comment.
I disagree. Whether or not this config item is still set makes little difference re the outcome -- it remains data loss.
This conclusion matches the outcome of #4750
I don't understand what "2nd line of defense if so desired" wants to say.
There was a problem hiding this comment.
If I decode @yarikoptic's comment correctly, its about rewording the message under certain circumstances:
1: Check if the config item about an ephemeral clone
2: If it is present, reword the warning to talk about "ephemeral clone" instead of "symlinked annex"
The symlink check and warning would co-exist and catch a symlinked annex even without the ephemeral config.
Is that interpretation correct?
There was a problem hiding this comment.
For now let's just merge, and if @mih decides to improve UX, could be in a follow up PR.
There was a problem hiding this comment.
I will adjust the title to be little more "obvious" and reflective of the change.
This is a blunt, but effective measure to prevent a data security issue, where dropping annex keys from 'here' is actually also dropping them from 'origin' -- thereby potentially deleting the last existing copy. Even once a more elegant error anticipation and handling is implemented, it should be kept for safety reasons. Fixes datalad#6948
yarikoptic
changed the title
This is a blunt, but effective measure to prevent a data security issue,
where dropping annex keys from 'here' is actually also dropping them
from 'origin' -- thereby potentially deleting the last existing copy.
Even once a more elegant error anticipation and handling is implemented,
it should be kept for safety reasons.
Closes #6948
PS: I will be working on a more elegant wrapping of this fix. But given the severity, this should be merged and released ASAP. #4750 continues to remain as a reminder.
I came to the conclusion that it might be best to remove the entire feature. I filed #6961
Spellchecker errors are addressed in #6956