Jeff King <peff@xxxxxxxx> writes:
> 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).
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html