Error response doesn't follow spec

[danielchatfield]

I appreciate this is still under heavy development and this is likely just not implemented yet but thought I'd add it here so that it doesn't get lost, library looks really good so far.

As per the spec:

If the request fails due to a missing, invalid, or mismatching
redirection URI, or if the client identifier is missing or invalid,
the authorization server SHOULD inform the resource owner of the
error and MUST NOT automatically redirect the user-agent to the
invalid redirection URI.

Currently the redirect uri is used for all errors except a missing redirect_uri. Potential attack could be a redirect_uri of javascript:evil(); to perform XSS.

[aeneasr]

Nice catch :) But you can be sure that all of AuthorizeRequest's parameters are already validated. This for example is covered here and more specifically here.

But due to your inquiry I found another issue where in fact the AuthorizeRequest will be nil if an error occurs in NewAuthorizeRequest and fosite will most likely panic in WriteAuthError due to a nil pointer. Looks like NewAuthorizeRequest must always return a non-nil value for AuthorizeRequest

[danielchatfield]

They are validated but then that method returns an error which as per the README is passed to WriteAuthorizeError which then uses the redirect_uri despite it potentially being the reason why validation failed.

authorizeRequest, err := oauth2.NewAuthorizeRequest(ctx, r)
    if err != nil {
       oauth2.WriteAuthorizeError(rw, req, err)
       return
    }

[aeneasr]

Ha! Got it, nice catch!

This should do, right? ErrInvalidRequest is thrown when the redirect uri is invalid:

    if ar.GetRedirectURI().String() == "" || err == ErrInvalidRequest {
        pkg.WriteJSON(rw, rfcerr)
        return
    }

It might be smarter and more transparent making validation explicit:

    if !ar.HasValidRedirectURL() {
        pkg.WriteJSON(rw, rfcerr)
        return
    }

As promised, check the Hall of Fame