Offer more details to users about the ownership check on FAT32 #3887
Conversation
dscho
force-pushed
the
skip-ownership-test-on-fat
branch
from
543f9dc to
a6d7d31
Compare
|
This feels like reintroducing the vulnerability that the whole ownership check is supposed to fix. Are you sure this shouldn't at least produce a big warning? |
|
Hmm. You're right: just because the user can't fix the issue doesn't mean that they're not vulnerable. Let me mull this over for a bit. |
dscho
force-pushed
the
skip-ownership-test-on-fat
branch
from
a6d7d31 to
c246ed6
Compare
So I mulled over this and agree with @rimrul that we cannot relax the ownership check. If the user runs Git on a file system that does not record ownership information, the safest option is to require an explicit exception (via that |
dscho
changed the title
Let's also hint to the user in the error message that they can debug this further. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
skip-ownership-test-on-fat
branch
from
c246ed6 to
815a1bc
Compare
compat/mingw.c
Outdated
| /* | ||
| * On FAT32 volumes, ownership is not actually recorded. | ||
| */ | ||
| warning("'%s' is on a file system that cannot represent ownership", path); |
There was a problem hiding this comment.
@derrickstolee do you think "that does not record ownership" would be more accurate/elegant?
There was a problem hiding this comment.
I agree that "does not record" is the most accurate. I read "cannot represent" as "cannot tell me" which is different than "doesn't even keep track of".
The FAT file system has no concept of ACLs. Therefore, it cannot store any ownership information anyway, and the `GetNamedSecurityInfoW()` call pretends that everything is owned "by the world". Let's special-case that scenario and tell the user what's going on, at least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. This addresses git-for-windows#3886 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
skip-ownership-test-on-fat
branch
from
815a1bc to
75f23a9
Compare
When Git indicates an unsafe directory due to the file system (e.g. FAT32) being unable to record ownership, Git [now gives better hints](git-for-windows/git#3887). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Offer more details to users about the ownership check on FAT32
Offer more details to users about the ownership check on FAT32
Offer more details to users about the ownership check on FAT32
Offer more details to users about the ownership check on FAT32
We don't.
I simulated it, but @clzls confirmed that it works. |
Since FAT cannot store ownership information, users cannot do anything about "unsafe ownership" there. Except exempt the directory via the
safe.directorymechanism. Inform the users about this (rather than the bogus "World owns this" message).Since we cannot know at the time the ownership check is performed whether Git will require a valid repository or not, we keep mum in the general case, and only say something if the
GIT_TEST_DEBUG_UNSAFE_DIRECTORIESknob is on.Speaking of that knob, we now hint to the users that they can use this knob to find out more, in case that the ownership fails (and Git absolutely requires a valid repository).
This addresses #3886