Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs

[Junio C Hamano <gitster@pobox.com>]

"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  blame.ignoreRevsFile::
>  	Ignore revisions listed in the file, one unabbreviated object name per
>  	line, in linkgit:git-blame[1].  Whitespace and comments beginning with
> -	`#` are ignored.  This option may be repeated multiple times.  Empty
> -	file names will reset the list of ignored revisions.  This option will
> -	be handled before the command line option `--ignore-revs-file`.
> +	`#` are ignored.  If `.git-blame-ignore-revs` exists at the root of the
> +	working tree in a non-bare repository, it is used by default.  This option
> +	may be repeated multiple times; files specified here are processed after
> +	the default file.  An empty file name will reset the list of ignored
> +	revisions from previously processed files and disable the default file.
> +	This option is handled before the command-line option `--ignore-revs-file`.

The proposed log message explains that '.git-blame-ignore-revs' is
used as the default for blame.ignoreRevsFile even when the user does
not ask to do so in order to match what hosting sites do, as it
would be confusing if the local repository behaved differently.
While wanting consistency is reasonable, the description above does
not quite match that goal.  If an untracked '.git-blame-ignore-revs'
file exists at the root of the working tree, or if a tracked one has
local changes relative to HEAD, the local repository behaves
differently from hosting sites that operate on the
'HEAD:.git-blame-ignore-revs' blob.  It may make more sense to say:
"If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
the initial element in the list of ignore-revs files.  Other files
listed in the configuration are also used, but an empty element
makes all elements that appeared before in the list forgotten."
This rule should apply whether the repository is bare or not.

The proposed log message also talks about taking only a regular file
and ignoring everything else for "security" [*], but there is
another important thing we need to worry about security-wise.
Somebody has to audit the parser for these files (one unabbreviated
object name per line, ignoring whitespace and lines starting with
'#') and ensure that the implementation is truly secure.

This is a new threat vector introduced by this change.  Without this
patch, blame.ignoreRevsFile comes only from the configuration, which
cannot point to an attacker-controlled file under our threat model.
Now, however, the parser must read upstream-controlled content at a
known path, and it must be prepared to cope with attempts to use it
as an attack vector.

[Footnote]

 * By the way, "we do not read anything from the working tree.
   'HEAD:.git-blame-ignore-revs' is the only thing that is added
   to the picture" would make it unnecessary to lstat() and ignore
   non-regular files.