Re: [PATCHv2 0/13] credential helpers

[Jeff King <peff@peff.net>]

On Tue, Dec 06, 2011 at 01:40:17PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >     ... You can now
> >     do: "git credential-store erase </dev/null" to erase everything
> >     (since you have provided no restrictions, it matches everything).
> 
> That "justification" does not sound so true to me but perhaps that is
> because it is unclear what "erase" means and what it means to give the
> operation parameters.

It's not meant to be a justification, but rather an explanation. I think
the behavior is probably too dangerous to leave.

> When I see "erase $foo", I would find it natural if $foo meant "if there
> is something that matches $foo, then please remove it, but keep everything
> else intact", and not the other way around "Match the existing entries
> against a pattern (or a set of matching patterns) I am giving you, and
> drop all the rest". So if I happen to give you an empty set, I would
> expect nothing is removed.

It does do the first thing you mentioned (you provide one pattern $foo,
and we match the pattern you have given). It's just that the pattern you
have specified is "everything". The problem is not in the matching, but
in the pattern specification language.

This pattern:

  protocol=https
  host=github.com

means "match everything that uses https _and_ has a host of github.com".
The username and path fields are not present, which implicitly means
"don't care about them".

Similarly, this pattern:

  protocol=https

means "match everything that uses https". Everything else is not
specified, and therefore we allow anything.

Then what does the empty pattern do? It cares about nothing, and
therefore matches everything.

By itself, I don't think that is a problem. It's something you might
want to specify, and it's logically consistent with the way the patterns
are matched.  What is dangerous, though, is that failing to provide
input is byte-wise identical to the empty pattern. And that's why I say
it's a pattern specification problem.

A rough BNF for the pattern format is something like:

  pattern = *line
  line = key "=" value
  key = *<any byte except NUL, "=", or "\n">
  value = *<any byte except NUL or "\n">

Because the pattern takes 0 or more lines and no terminator, we can't
distinguish between empty or truncated input and the empty pattern. So
one solution would be:

  pattern = *line "\n"

i.e., require a blank line terminator.

Does that explain the issue better?

-Peff