Fix exception status codes in security http schemes

[gostekk]

Hi!

TL;DR: This PR changes exception response status codes in security http schemes from 403_FORBIDDEN to 401_UNAUTHORIZED.

Long HTTPBearer and HTTPDigest security schemes used as a dependency are returning a 403_FORBIDDEN status on scheme Exception. This should be 401_UNAUTHORIZED status because we can't identify user without token or with invalid credentials.

There's issue mentioning this: #2026

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (cf73051) 100.00% compared to head (cf9a1e2) 100.00%. Report is 1036 commits behind head on master.

:exclamation: Current head cf9a1e2 differs from pull request most recent head 62bdb6c. Consider uploading reports for the commit 62bdb6c to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##            master     #2120     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          540       239    -301     
  Lines        13969      7079   -6890     
===========================================
- Hits         13969      7079   -6890     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

📝 Docs preview for commit cf9a1e27e54e412244217f20291d6994073aaf4a at: https://5f7751fc8bcb68537c8b9835--fastapi.netlify.app

[aksel]

I was just about to open a similar PR myself, and noticed yours here :smile:

In general, the status code for invalid authorization seems to depend on the application. The use case I am building would require 401 for a missing (or malformed, e.g. not /Bearer [a-z0-9]+/i) Authorization header.

But I have seen other status codes being used.

400: If header was malformed. In my opinion, this is bad form. But I've seen it, to make client-side error handling easier; any request parameter - be it header, body, query, whatever - must match this format, and if not, 400 is sent. 400 is then handled client side as something like Your request did not match the specification blablabla

403: For example if an auth header was provided, but malformed, you may wish to respond as if it was valid auth, but insufficient scope. This also matches the RFC, in my opinion:

If authentication credentials were provided in the request, the server considers them insufficient to grant access. The client SHOULD NOT automatically repeat the request with the same credentials. The client MAY repeat the request with new or different credentials. However, a request might be forbidden for reasons unrelated to the credentials.

404: E.g. if you wish to obfuscate the endpoint.

This was a long winded comment, in order to justify the following: I think the constructor should accept a parameter status_code, which defaults to 401.

[zolaemil]

I'd like to add my support for this PR.

In response to @aksel:

403: For example if an auth header was provided, but malformed, you may wish to respond as if it was valid auth, but insufficient scope. This also matches the RFC, in my opinion

It is true that the quoted definition of 403 fits the scenario you describe. However, the definition of 401 in the same RFC fits it better:

The 401 (Unauthorized) status code indicates that the request has not been applied because it lacks valid authentication credentials for the target resource.

[aksel]

However, the definition of 401 in the same RFC fits it better

I agree.

[ghost]

@gostekk Thanks for putting this change together. Are there plans to merge / release this soon?

[adriangb]

Bumping this. Would be great to get this merged.

[jtemporal]

Also stopping by to support this change! I had conversations about this status code thing this week with colleagues, it would be good to see this get merged. =)

[ericrdgz]

Would love to see this fixed. Any update on this?

[dreinon]

Hi @tiangolo! Sorry to tag you but just doing it in case you missed this one, since this PR is supported by a few people already. Thanks!

[HansBambel]

Would be nice to have this fixed

[chrysaor]

Recently, I found this problem. I think it's a good point to separate error codes. When it will be merged to a master branch?

[alexmaurizio]

Hello! Pinging in to this PR. We've just had this discussion with colleagues and we agree that the 403 when the auth is missing is not a correct default.

We agree all applications have peculiar standards and needs and it's okay to return different codes, but this is about default values, not custom behaviour.

We all agree 401 is a better fit in the unauth case instead of a 403.

[github-actions[bot]]

📝 Docs preview for commit 8234c43d4ff24ea279c725e48efd3c91522028b2 at: https://6313873bea02943b90f61713--fastapi.netlify.app

[tiangolo]

Hey there! Yep, this makes sense and I've been wanting to fix this, actually, since the first moment I was writing it for the first time while reading all the standards and specs. :sweat_smile:

But here's the thing:

The server generating a 401 response MUST send a WWW-Authenticate header field containing at least one challenge applicable to the target resource.

Ref: https://www.rfc-editor.org/rfc/rfc7235#section-3.1

I haven't been able to put the time to figure out what are the correct challenges for each security scheme, with the right spec references, etc. ...only for some of them.

That's why I haven't updated the rest, because 403 is not as precise as 401, but it's not wrong. While a 401 without WWW-Authenticate header would be, by the spec, wrong.

If you (or anyone else) can find the right references to the related specs that define the specific challenges that should be used for each one of those security schemes, I would love to have this in. It could also be independent PRs, assuming you could find only references for one of the schemes but not the others, I could take that in a single PR.

[github-actions[bot]]

📝 Docs preview for commit 62bdb6c1e208b81eb23a1280b2c3c82e4f40ccc4 at: https://639cce45c3d3b814d81d1ccc--fastapi.netlify.app

[danclaytondev]

This is now a discussion but I think shoud remain an open issue.

[ordinary-jamie]

I think RFC 6750 "The OAuth 2.0 Authorization Framework: Bearer Token Usage", specifically section 3 "The WWW-Authenticate Response Header Field" enumerates the required WWW-Authenticate challenge for the bearer auth-scheme.

In summary:

• MUST use the auth-scheme value "Bearer".
• MUST have one or more auth-param values following auth-scheme
• SHOULD include the error attribute to provide the client with the reason why the access request was declined.
• MAY include the error_description attribute to provide developers a human-readable explanation that is not meant to be displayed to end-users.
• MAY include the error_uri attribute with an absolute URI identifying a human-readable web page explaining the error.
• MAY use realm attribute to indicate the scope of protection in the manner described in HTTP/1.1 [RFC2617].
• MAY use scope field, a space-delimited list of case-sensitive scope values indicating the required scope of the access token for accessing the requested resource.

An example would be

     HTTP/1.1 401 Unauthorized
     WWW-Authenticate: Bearer realm="example",
                       error="invalid_token",
                       error_description="The access token expired"

Ref: https://datatracker.ietf.org/doc/html/rfc6750#section-3