-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ignore unsupported http authentication contexts #4839
Conversation
src/transports/http.c
Outdated
@@ -246,7 +246,7 @@ static int parse_authenticate_response( | |||
|
|||
git_vector_foreach(www_authenticate, i, challenge) { | |||
if (auth_context_match(&context, t, challenge_match, challenge) < 0) | |||
return -1; | |||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
One problem here is that it seems that we have overloaded the return value of auth_context_match
, and it's returning -1
when there was no scheme match. I think that might be the root cause of the problems here.
I think that we should change auth_context_match
to return 0
(and leave context
unset) when there's no match, instead of returning -1
. I think that this bit can then remain unchanged - on auth_context_match
returning -1
on error, it will propagate this error, and returning 0
without setting context
, it will continue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this might be a better idea.
The only other usage is by apply_credentials
and I'm not sure if this needs to do something more to handle the situation where return value is 0 but the output context is NULL?
Lines 169 to 192 in a8d447f
static int apply_credentials(git_buf *buf, http_subtransport *t) | |
{ | |
git_cred *cred = t->cred; | |
git_http_auth_context *context; | |
/* Apply the credentials given to us in the URL */ | |
if (!cred && t->connection_data.user && t->connection_data.pass) { | |
if (!t->url_cred && | |
git_cred_userpass_plaintext_new(&t->url_cred, | |
t->connection_data.user, t->connection_data.pass) < 0) | |
return -1; | |
cred = t->url_cred; | |
} | |
if (!cred) | |
return 0; | |
/* Get or create a context for the best scheme for this cred type */ | |
if (auth_context_match(&context, t, credtype_match, &cred->credtype) < 0) | |
return -1; | |
return context->next_token(buf, context, cred); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, good call. A quick glance suggests that yes, we should probably return 0
there but I'd appreciate another set of eyes. :)
Pushed a new commit such that |
auth_context_match returns 0 instead of -1 for unknown schemes to not fail in situations where some authentication schemes are supported and others are not. apply_credentials is adjusted to handle auth_context_match returning 0 without producing authentication context.
Will clean this up by squashing the commits into one and making sure whitespace matches the rest of the file. |
Thanks for this! |
auth_context_match
returns -1 when the authentication context is not supported,but this made
parse_authenticate_response
fail in situations where some authenticationcontexts are supported and others are not.
parse_authenticate_response
will instead ignore authentication contexts whereauth_context_match
returns -1This specifically handles recent changes to visualstudio.com that responds with:
when asking for authentication and the http transport didn't know what to do with
WWW-Authenticate: Bearer
returningerror=-1
without any further explanation.I presume the intersection of libgit2 and visualstudio.com users are mostly using the winhttp transport but this change improves behaviour when using the plain http transport.