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

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

quoted

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.

quoted

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