Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized

From: Jeff King <peff@peff.net>
Date: Apr 27, 2016, 20:15 UTC
Message-ID: <20160427201512.GA8073@sigill.intra.peff.net>
In-Reply-To: <1461787832.11504.1.camel@twopensource.com>

On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote:

Show 16 quoted lines

> On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote:
> 
> > There is another call to refname_is_safe() in resolve_ref_unsafe(),
> > which applies the sanity check to the string from a symref.  We seem
> > to allow
> > 
> >     $ git symbolic-ref refs/heads/SSS refs/heads//master
> > 
> > and we end up storing "ref: refs/heads//master" (or make a symbolic
> > link with doubled slashes), but the current code considers the
> > resulting symbolic link as "dangling".  Again, this change moves the
> > rejection a bit earlier in the codepath, without changing the end
> > result, I'd think.
> 
> I think we should disallow that -- refname_is_safe should probably call
> (or be replaced with calls to) check_refname_format.

I thought the point is that one is a lesser check than the other, and we need different rules for different situations. So we might allow deletion on a refname that does not pass check_refname_format(), but we must make sure it is not going to cause any mischief (e.g., escaping ".git" and deleting random files).

But anything writing a _new_ refname (whether the actual ref, or referencing it via a symref) should be using check_refname_format() before writing.

-Peff