Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing

From: Jeff King <peff@peff.net>
Date: Jun 6, 2024, 08:08 UTC
Message-ID: <20240606080819.GB658959@coredump.intra.peff.net>
In-Reply-To: <dcbbd00f-1730-41fd-90d3-c7b070c4f17d@nvidia.com>

On Wed, Jun 05, 2024 at 09:45:32AM -0700, Aaron Plattner wrote:

Show 16 quoted lines

> > And in that case you really want to retain the "query" parts of the
> > credential after the reject. In this toy example you could just move the
> > url-to-cred parsing into the loop, but in the real world it's often more
> > complicated.
> > 
> > Arguably even the original code is a bit questionable for this, because
> > we don't know if the username came from a helper or from the user, or if
> > it was part of the original URL (e.g., "https://user@example.com/"
> > should prompt only for the password). But it feels like this hunk is
> > making it worse.
> 
> The comment above credential_reject() mentions that it is "readying the
> credential for another call to `credential_fill`" which does imply that you
> can use it again right away without having to fill in the protocol / host /
> path fields. So you're probably right that this should remain the way it
> was.

Heh, OK. I was the one who wrote that comment originally, which I guess is why it was in the back of my mind. ;)

As I said, clearing "username" is a little questionable there. But it also gives the user a chance to update the field, so maybe it's not so bad. There might be other fields in the same boat, but I think you'd really have to think about each one. I'm content to leave the code as it is for now, and if somebody comes up with a case where reject+fill doesn't behave as they expect, we can think about it further.

Show 8 quoted lines

> > The rest of the patch made sense to me, though. As would using
> > credential_clear_secrets() here to replace the equivalent lines.
> 
> That's certainly fine with me. Using credential_clear_secrets() to just
> replace those two lines would definitely keep the original behavior of this
> code.
> 
> I'll send a v3 patch to do that.

Great, thanks!

-Peff