Re: [OAUTH-WG] error codes in 4.1.2.1 and 4.2.2.1 and extension response types (8.4)

[Roger Crew <[e-mail]>]

It now occurs to me that "bug" for item (1) below is perhaps 
a bit understated; it's really more of a security issue,

since if the implementer follows what the current spec is 
apparently saying, the authorization server essentially 
becomes an open redirector.

(... Apologies if this was obvious and someone already took care of it
for the next draft, but the lack of response led me to believe this
issue was falling through the cracks.  And it seems likely I hit a
really busy spot in last month's schedule.  Thanks for your time...)

     Roger Crew
     a.k.a. someone who's attempting to implement the spec...

===== Original message =====
Date: Sun, 13 Nov 2011 16:18:52 -0800

[With respect to OAuth v2 draft 22]

I have some observations about the error responses at the authorization 
endpoint (4.1.2.1 and 4.2.2.1 for the authorization_code and implicit 
grant_types, respectively).

  (1) looks like a bug,
  (2) is an ambiguity and may also apply to Section 5.2,
  (3-5) are suggestions.

Onward...
-----
(1) 4.1.2.1, and 4.2.2.1 both say that in the case that client_id is
    provided and invalid/unknown, the auth server MUST NOT
    automatically redirect.

    However, if the client_id is MISSING, that clause of
    4.1.2.1/4.2.2.1 does not apply, and thence this becomes a
    malformed request (i.e., a required parameter is missing)
    which then results in an error='invalid_request' redirection.

    Which looks like a mistake to me, because in that situation
    what reason do we have to be trusting redirect_uri?
    
-----
(2) If the response_type is provided but unknown, is that an 
    'invalid_request' (since this is clearly an "unsupported 
    parameter value") or is it an 'unsupported_response_type'?

    Seems to me it should be the latter.  If so, then...

    Given that for ALL currently defined parameters to authorization
    endpoint requests, there are already provisions for what happens
    if the value is provided but invalid/unsupported/etc, i.e.,

       bad client_id     => do not redirect
       bad redirect_uri  => do not redirect
       bad response_type => redirect error='unsupported_response_type'
       bad scope         => redirect error='invalid_scope'
       bad state         => undetectable from the server side

    should the description for 'invalid_request' even be referring to
    "unsupported values" at all?

    A couple of alternatives (again these are for 4.1.2.1/4.2.2.1):

       invalid_request
           a required *extension* parameter is missing
           or has an unsupported value, or the request is
           otherwise malformed.

       invalid_request
           the request is malformed in some manner not covered by any
           of the other error codes defined for this response type

    Either of these would make clear that 'unsupported_response_type'
    is indeed the error code for the case of a
    provided-but-unrecognized response_type.

    Section 5.2 has the same problem w.r.t. 'unsupported_grant_type'

-----
(3) If 'server_error' and 'temporarily_unavailable' are intended to
    correspond to HTTP status codes 500 and 503, it might be good to
    say so explicitly, e.g.,

    The authorization server SHOULD, where possible, use these
    redirection error responses in preference to sending the
    corresponding status=(500|503) HTTP response in situations where
    the latter would otherwise be appropriate.

    Admittedly, there's no way an implementation is going to catch all
    of these, but I'm assuming the intent is to catch as many as
    possible?

-----
(4) The lists of error codes in 4.1.2.1 and 4.2.2.1 are essentially
    identical.  I believe these can be merged without any loss of
    clarity.  

    (... As a matter of general principle, I'm not a huge fan of
    having to chase down swaths of cut&pasted text and then having to
    use 'diff' to figure out whether/how they are different.  Better
    to combine the common stuff in a single place, use a cross
    reference and highlight the differences.  And I absolutely do not
    mind following cross-references...)

    In this case, the ONLY place where there's ANY variance at all is
    in the descriptions of 'unauthorized_client' and
    'unsupported_response_type'.

    I figure either of the following works:

        unauthorized_client
           The client is not authorized to use any of the supported
           authorization grant types implied by the requested
           response_type.

        unauthorized_client
           The client is not authorized to use this response_type.

    the latter having the advantage that we don't then get bogged
    down in the question of how response_types and grant_types relate
    to each other (see (5) below).

    And similarly for 'unsupported_response_type', viz

        unsupported_response_type
           The authorization server does not support any of the
           authorization grant types implied by the requested response
           type.

        unsupported_response_type
           The authorization server does not support this response
           type.

    at which point you can replace 4.2.2.1 with a single sentence
    referring to 4.1.2.1.

-----
(5) I am assuming that each authorization endpoint response_type is
    NOT necessarily unique to a particular authorization grant type.
    It would be helpful to state this explicitly (probably in 8.4) so
    that implementers can plan accordingly.

    And if this IS intended otherwise (i.e., that every response
    type MUST imply a particular grant type, as is the case for the
    current spec so long as no new response types are ever defined),
    then that really should be stated somewhere in 8.4.

    The reason I assume not is that this seems a straightforward (to
    me, at least) use of response_type="token code", i.e., to allow
    the server to respond EITHER as per 4.1.2 or 4.2.2 as it sees fit
    (but evidently WG members are not agreeing on whether this
    response_type is needed, hence its absence from the current spec
    other than as a remark in 8.4).

-- 
Roger Crew
[e-mail]