Repository navigation
oauth: fix request construction and cancellation - #2499
Merged
Merged
Conversation
We typically store files with the LF line ending in our repo. The OAuth client file was stored with CRLF in error. Fix that. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
RFC 6749 states that a client MAY send client credentials (client ID + client secret) in either the `Authorization` header, or as part of the request body (such as in form data). However, the RFC also states a client MUST NOT send the client credentials in both the request body AND the `Authorization` header at the same time. We must make sure we only send credentials in the header if configured so, and therefore also only send credentials in the body if not. Additionally, depending on the implementation of the authorization server, public clients need not send a client secret either in the request body or `Authorization` header. If we have no real value, we should not send the credentials. Note that the client ID is always required, and may be present in the request body even if the credentials are sent in the `Authorization` header, so let's always still send the client ID in the body. Finally, we have also been sending the redirect URI in the request body for flows/endpoints that did not require them - stop doing that. According to RFC 6749 only the authorization code request needs a redirect URI to be sent, and also in the corresponding token request. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
We have a cancellation token from the caller, but we have not been passing this down to the `Content.ReadAdStringAsync()` operation on the HTTP responses. Let's do this. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
dscho
approved these changes
Nov 9, 2026
dscho
left a comment
dscho
left a comment
Contributor
There was a problem hiding this comment.
Looks obviously correct to me.
Contributor
Author
|
I should add that I tested these changes against GitHub, GitLab, Bitbucket, and Gitea's hosted servers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Correct several discrepancies between the shared OAuth2 client’s request construction and RFC 6749 §3.2.1 and RFC 8628 §3.1, separately from the token-exchange feature.
Client credentials could previously be sent through both HTTP Basic and the request body, contrary to RFC 6749 §2.3’s prohibition on multiple authentication methods per request. Apply the configured method consistently to token and device-authorization requests, omit unconfigured secrets (§2.3.1), and retain body
client_idfor client identification.Stop sending
redirect_uriduring refresh requests and remove the test server’s corresponding requirement. Authorization-code requests continue to repeat the original redirect URI when it was supplied during authorization, as required by RFC 6749 §4.1.3.Also propagate cancellation to response-body reads. The OAuth client’s LF line-ending normalization is kept in a separate commit so the functional changes remain easy to review.