Re: [PATCHv2 0/13] credential helpers

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2011, 23:34 UTC
Message-ID: <7vehwdcob3.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111209231800.GA14376@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted 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);
>  }

Looks very sane.

> We _could_ modify credential_match() to automatically reject such a
> pattern at that level,...

I do not think that is such a good idea to modify "match()" function either, as I agree match with empty has its uses, but that does not stop "rewrite_credential_file()" from being safe by default, no? After all, the one that makes the decision to drop things that match the pattern is that function (it chooses to give NULL to match_cb).

Show 5 quoted lines

> 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