Re: [PATCH 2/2] http: prevent redirect from dropping credentials during reauth

["Quentin Bouget" <ypsah@devyard.org>]

On Sun Feb 4, 2024 at 11:51 PM CET, Junio C Hamano wrote:
> Quentin Bouget <ypsah@devyard.org> writes:
>
> > During a re-authentication (second attempt at authenticating with a
> > remote, e.g. after a failed GSSAPI attempt), git allows the remote to
> > provide credential overrides in the redirect URL and unconditionnaly
> > drops the current HTTP credentials in favors of those, even when there
> > aren't any.
> >
> > This commit makes it so HTTP credentials are only overridden when the
> > redirect URL actually contains credentials itself.
>
> "This commit makes it so" -> "Make it so"

Will change.

> > +			char *username = NULL, *password = NULL;
> > +
> > +			if (http_auth.username)
> > +				username = xstrdup(http_auth.username);
> > +			if (http_auth.password)
> > +				password = xstrdup(http_auth.password);
>
> Not a huge deal, but we have xstrdup_or_null() helper function
> exactly for a use case like this.

Thanks, will change.

> >  			credential_from_url(&http_auth, options->base_url->buf);
> > +
> > +			if (http_auth.username)
> > +				free(username);
> > +			else if (username)
> > +				http_auth.username = username;
> > +
> > +			if (http_auth.password)
> > +				free(password);
> > +			else if (password)
> > +				http_auth.password = password;
>
> This is an interesting change.  I wonder what breaks if we
> completely ignored such credential materials forced by the remote
> via a redirect?

Me too. Maybe the original author would know. Is it OK to Cc them in
this case?

> >  			url = options->effective_url->buf;
> >  		}
> >  	}

Thanks,
Quentin