Skip to content
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

fix: write authorize error returns 303 for get method #724

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

james-d-elliott
Copy link
Contributor

@james-d-elliott james-d-elliott commented Nov 21, 2022

This presumably fixes an issue where the WriteAuthorizeError function sets a 303 See Other when it should return a 302 Found instead when the method of the original HTTP request used the method verb 'GET'. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/302 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303. The 303 See Other is typically only used when the client is being instructed to change the request method verb to 'GET' which is not relevant if it is already 'GET'.

RFC7231 Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content, section 6.4.3 302 Found.
RFC7231 Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content, section 6.4.4 303 See Other.
OAuth Security Topics, section 4.11 307 Redirect
OAuth Security Topics, section 4.1.2 Redirect URI Validation Attacks on Implicit Grant

Context: #636 (comment)

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

This presumably fixes an issue where the WriteAuthorizeError function sets a 303 See Other when it should return a 302 Found instead when the method of the original HTTP request used the method verb 'GET'. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/302 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303. The 303 See Other is typically only used when the client is being instructed to change the request method verb to 'GET' which is not relevant if it is already 'GET'.
@james-d-elliott
Copy link
Contributor Author

For reference this is NOT ready to merge. It needs to be carefully examined against the specs; and I'm far less convinced it's "correct" after reading all of the linked specs that it is actually the correct move. For the sake of security I'm leaning towards the implementer needs to make this adjustment.

@mitar
Copy link
Contributor

mitar commented Feb 15, 2024

So https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-25#name-307-redirect is pretty clear that 303 (see other) should simply always be returned:

If an HTTP redirection (and not, for example, JavaScript) is used for such a request, the authorization server SHOULD use HTTP status code 303 (See Other).

So I am not completely convinced that the option to return 302 (found) is needed, but it seems some people reported that their clients do not work with 303. So having 302 be returned for GET seems a reasonable thing for them, but we should get feedback from them if this really fixes things.

@@ -83,5 +83,11 @@ func (f *Fosite) WriteAuthorizeError(ctx context.Context, rw http.ResponseWriter
}

rw.Header().Set("Location", redirectURIString)
rw.WriteHeader(http.StatusSeeOther)

switch ar.GetRequestMethod() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should add a comment explaining and linking why is this necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants