| Message ID | 20201023081711.GB4012156@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 899bc21bfa02fc82cf1bb4dd9026ea3a24114874 |
| Headers | show |
| Series | documentation symlink restrictions for .git* files | expand |
On Fri, Oct 23, 2020 at 04:17:11AM -0400, Jeff King wrote:
> Subject: [PATCH] documentation symlink restrictions for .git* files
Oops, that should be "document symlink" of course.
-Peff
On 23/10/2020 09:17, Jeff King wrote: > On Wed, Oct 21, 2020 at 12:19:25AM +0100, Philip Oakley wrote: > >> On 05/10/2020 13:16, Jeff King wrote: >>> On Mon, Oct 05, 2020 at 03:17:51AM -0400, Jeff King wrote: >>> >>>> About 2 years ago as part of a security release we made it illegal to >>>> have a symlinked .gitmodules file (refusing it both in the index and via >>>> fsck). At the time we discussed (on the security list) outlawing >>>> symlinks for other .git files in the same way, but we decided not to do >>>> so as part of the security release, as it wasn't strictly necessary. >> Is this something that should be recorded in the documentation, either as a >> simple (sensible) limitation, or explicitly as a security related safety >> measure? >> >> I didn't see any changes to the .txt docs in the change list below. > Yeah, that's a good point. > > How about this (on top of jk/symlinked-dotgitx-files)? > > -- >8 -- > Subject: [PATCH] documentation symlink restrictions for .git* files > > We outlawed symbolic link versions of various .git files in 10ecfa7649 > (verify_path: disallow symlinks in .gitmodules, 2018-05-04) and > dd4c2fe66b (verify_path(): disallow symlinks in .gitattributes and > .gitignore, 2020-10-05). The reasons are discussed in detail there, but > we never adjusted the documentation to let users know. > > This hasn't been a big deal since the point is that such setups were > mildly broken and thought to be unusual anyway. But it certainly doesn't > hurt to be clear and explicit about it. > > Suggested-by: Philip Oakley <philipoakley@iee.email> > Signed-off-by: Jeff King <peff@peff.net> > --- > Documentation/gitattributes.txt | 7 +++++++ > Documentation/gitignore.txt | 5 +++++ > Documentation/gitmodules.txt | 8 ++++++++ > 3 files changed, 20 insertions(+) > > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index 2d0a03715b..9a2ce4f1ea 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -1241,6 +1241,13 @@ to: > [attr]binary -diff -merge -text > ------------ > > +NOTES > +----- > + > +Note that Git does not allow a `.gitattributes` file within the working > +tree to be a symbolic link, and will refuse to check out such a tree > +entry. This keeps behavior consistent when the file is accessed from > +the index or a tree versus from the filesystem. > > EXAMPLES > -------- > diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt > index d47b1ae296..7e9a1d49d6 100644 > --- a/Documentation/gitignore.txt > +++ b/Documentation/gitignore.txt > @@ -149,6 +149,11 @@ not tracked by Git remain untracked. > To stop tracking a file that is currently tracked, use > 'git rm --cached'. > > +Note that Git does not allow a `.gitignore` file within the working tree > +to be a symbolic link, and will refuse to check out such a tree entry. > +This keeps behavior consistent when the file is accessed from the index > +or a tree versus from the filesystem. > + > EXAMPLES > -------- > > diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt > index 539b4e1997..2b884be3c7 100644 > --- a/Documentation/gitmodules.txt > +++ b/Documentation/gitmodules.txt > @@ -98,6 +98,14 @@ submodule.<name>.shallow:: > shallow clone (with a history depth of 1) unless the user explicitly > asks for a non-shallow clone. > > +NOTES > +----- > + > +Note that Git does not allow the `.gitmodules` file within a working > +tree to be a symbolic link, and will refuse to check out such a tree > +entry. This keeps behavior consistent when the file is accessed from the > +index or a tree versus from the filesystem, and helps Git reliably > +enforce security checks of the file contents. > > EXAMPLES > -------- The text looks good to me, with security point explicitly mentioned just for .gitmodules file. However, is placing the Note so far down appropriate (.gitattributes and .gitignore), given that there is within the descriptions a discussion of the priority order for finding those files? Philip
On Mon, Oct 26, 2020 at 10:18:18PM +0000, Philip Oakley wrote: > > +NOTES > > +----- > > + > > +Note that Git does not allow the `.gitmodules` file within a working > > +tree to be a symbolic link, and will refuse to check out such a tree > > +entry. This keeps behavior consistent when the file is accessed from the > > +index or a tree versus from the filesystem, and helps Git reliably > > +enforce security checks of the file contents. > > > > EXAMPLES > > -------- > The text looks good to me, with security point explicitly mentioned just > for .gitmodules file. > > However, is placing the Note so far down appropriate (.gitattributes and > .gitignore), given that there is within the descriptions a discussion of > the priority order for finding those files? Definitely a "NOTES" section should go in that spot, but possibly the text should be in the "DESCRIPTION" section. I was worried about cluttering that early part with a detail that most people wouldn't care too much about. Looks like my patch is in 'next'; do you want to propose a patch moving it around on top? -Peff
Jeff King <peff@peff.net> writes: > Definitely a "NOTES" section should go in that spot, but possibly the > text should be in the "DESCRIPTION" section. I was worried about > cluttering that early part with a detail that most people wouldn't care > too much about. > > Looks like my patch is in 'next'; do you want to propose a patch moving > it around on top? It probably is possible to tweak the introductory text this way without being unnecessarily loud and keep the NOTES section where it is. I personally found that the placement of new text was OK, but this may be overly compensated by my tendency to discount things that we have discussed recently, which our mind often consider (only for relative a short moment) as more important than they are, relative to other things in the same document. Documentation/gitattributes.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt index 2d0a03715b..fd2de7344a 100644 --- i/Documentation/gitattributes.txt +++ w/Documentation/gitattributes.txt @@ -13,8 +13,8 @@ $GIT_DIR/info/attributes, .gitattributes DESCRIPTION ----------- -A `gitattributes` file is a simple text file that gives -`attributes` to pathnames. +A `gitattributes` file is a simple text file (it cannot be a +symbolic link to anything) that gives `attributes` to pathnames. Each line in `gitattributes` file is of form:
On Mon, Oct 26, 2020 at 04:32:27PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Definitely a "NOTES" section should go in that spot, but possibly the > > text should be in the "DESCRIPTION" section. I was worried about > > cluttering that early part with a detail that most people wouldn't care > > too much about. > > > > Looks like my patch is in 'next'; do you want to propose a patch moving > > it around on top? > > It probably is possible to tweak the introductory text this way > without being unnecessarily loud and keep the NOTES section where it > is. > [...] > DESCRIPTION > ----------- > > -A `gitattributes` file is a simple text file that gives > -`attributes` to pathnames. > +A `gitattributes` file is a simple text file (it cannot be a > +symbolic link to anything) that gives `attributes` to pathnames. I worried that even a short mention like this would be distracting. Not because it's so long, but because it's right there in the very first sentence, and I really think this is a corner case that most people would not even think about. So it is helpful if you are looking for info on symlinks and these files, but probably clutter if you are looking for something else. I have to admit I don't feel all that strongly either way, though. -Peff
Jeff King <peff@peff.net> writes: >> DESCRIPTION >> ----------- >> >> -A `gitattributes` file is a simple text file that gives >> -`attributes` to pathnames. >> +A `gitattributes` file is a simple text file (it cannot be a >> +symbolic link to anything) that gives `attributes` to pathnames. > > I worried that even a short mention like this would be distracting. Not > because it's so long, but because it's right there in the very first > sentence, and I really think this is a corner case that most people > would not even think about. > > So it is helpful if you are looking for info on symlinks and these > files, but probably clutter if you are looking for something else. > > I have to admit I don't feel all that strongly either way, though. I don't, either, and as I said, I found that the placement of new text was OK.
On 27/10/2020 18:45, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >>> DESCRIPTION >>> ----------- >>> >>> -A `gitattributes` file is a simple text file that gives >>> -`attributes` to pathnames. >>> +A `gitattributes` file is a simple text file (it cannot be a >>> +symbolic link to anything) that gives `attributes` to pathnames. >> I worried that even a short mention like this would be distracting. Not >> because it's so long, but because it's right there in the very first >> sentence, and I really think this is a corner case that most people >> would not even think about. >> >> So it is helpful if you are looking for info on symlinks and these >> files, but probably clutter if you are looking for something else. >> >> I have to admit I don't feel all that strongly either way, though. > I don't, either, and as I said, I found that the placement of new > text was OK. > > I do think that the extra text above is the right thing to do. We should be informing readers early about things that are expressly prohibited. Leaving just the note till nearly the end of the rather long attributes/ignore man pages makes it very hard to discover for frustrated users, which would accidentally reinforce the idea of the docs being poor (rather than being focussed). Philip
Philip Oakley <philipoakley@iee.email> writes: > On 27/10/2020 18:45, Junio C Hamano wrote: >> Jeff King <peff@peff.net> writes: >> >>>> DESCRIPTION >>>> ----------- >>>> >>>> -A `gitattributes` file is a simple text file that gives >>>> -`attributes` to pathnames. >>>> +A `gitattributes` file is a simple text file (it cannot be a >>>> +symbolic link to anything) that gives `attributes` to pathnames. >>> I worried that even a short mention like this would be distracting. Not >>> because it's so long, but because it's right there in the very first >>> sentence, and I really think this is a corner case that most people >>> would not even think about. >>> >>> So it is helpful if you are looking for info on symlinks and these >>> files, but probably clutter if you are looking for something else. >>> >>> I have to admit I don't feel all that strongly either way, though. >> I don't, either, and as I said, I found that the placement of new >> text was OK. >> >> > I do think that the extra text above is the right thing to do. > > We should be informing readers early about things that are expressly > prohibited. It needs to be balanced, though. If it is so common a temptation among readers to make a symbolic link out of the tracked contents, perhaps it is worth telling them that they shouldn't do so, but I just do not get the feeling that it is the case.
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 2d0a03715b..9a2ce4f1ea 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -1241,6 +1241,13 @@ to: [attr]binary -diff -merge -text ------------ +NOTES +----- + +Note that Git does not allow a `.gitattributes` file within the working +tree to be a symbolic link, and will refuse to check out such a tree +entry. This keeps behavior consistent when the file is accessed from +the index or a tree versus from the filesystem. EXAMPLES -------- diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt index d47b1ae296..7e9a1d49d6 100644 --- a/Documentation/gitignore.txt +++ b/Documentation/gitignore.txt @@ -149,6 +149,11 @@ not tracked by Git remain untracked. To stop tracking a file that is currently tracked, use 'git rm --cached'. +Note that Git does not allow a `.gitignore` file within the working tree +to be a symbolic link, and will refuse to check out such a tree entry. +This keeps behavior consistent when the file is accessed from the index +or a tree versus from the filesystem. + EXAMPLES -------- diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt index 539b4e1997..2b884be3c7 100644 --- a/Documentation/gitmodules.txt +++ b/Documentation/gitmodules.txt @@ -98,6 +98,14 @@ submodule.<name>.shallow:: shallow clone (with a history depth of 1) unless the user explicitly asks for a non-shallow clone. +NOTES +----- + +Note that Git does not allow the `.gitmodules` file within a working +tree to be a symbolic link, and will refuse to check out such a tree +entry. This keeps behavior consistent when the file is accessed from the +index or a tree versus from the filesystem, and helps Git reliably +enforce security checks of the file contents. EXAMPLES --------