Re: [PATCHv2 0/13] credential helpers

[Jeff King <peff@peff.net>]

On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > It works, and it detects truncated output both ways properly (I know
> > because I had to update every test, since the old output was missing the
> > end-of-credential marker).
> >
> > It makes me a little sad, because the original format (relying on EOF)
> > was so Unix-y.
> 
> It saddens me, too.  A reasonable middle ground would be to stop treating
> an empty input as "no restriction" but "never matches".
> 
> I suspect that it is far more likely for a helper to fail (due to
> configuration errors, for example) before it produces any output than
> after it gives some but not all output lines.

Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:

diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
 
 static void remove_credential(const char *fn, struct credential *c)
 {
-	rewrite_credential_file(fn, c, NULL);
+	/*
+	 * Sanity check that we actually have something to match
+	 * against. The input we get is a restrictive pattern,
+	 * so technically a blank credential means "erase everything".
+	 * But it is too easy to accidentally send this, since it is equivalent
+	 * to empty input. So explicitly disallow it, and require that the
+	 * pattern have some actual content to match.
+	 */
+	if (c->protocol || c->host || c->path || c->username)
+		rewrite_credential_file(fn, c, NULL);
 }
 
 static int lookup_credential(const char *fn, struct credential *c)


We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:

  [credential "https://example.com/foo.git"]
          helper = ...
  [credential "https://example.com"]
          helper = ...
  [credential "https://"]
          helper = ...
  [credential]
          helper = ...

The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).

So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).

-Peff