Fix possible crash in validator when 'client' key is mentioned in request body

[realsuayip]

Description of the Change

When sending a request to oauth2_provider.views.base.TokenView, if client is sent via request body (typo of client_id), the server crashes with:

'str' object has no attribute 'is_usable'

(str object being client in request body)

I'm not sure why request.client is set to this attribute; I'm guessing request body is added to oauth request as attrs.

I changed the check so that application would be fetched if the type does not match.

Checklist

• PR only contains one change (considered splitting up PR)
• unit-test added
• documentation updated
• CHANGELOG.md updated (only for user relevant changes)
• author name in AUTHORS

[dopry]

@realsuayip Thanks for the PR. I have two asks.

1. Can you dig into why request.client was set to something else? This could be an interactive custom code from your project, a conflict with package, or a symptom of a bug else where in oauth_toolkit or oauth_lib. I'd like us to be sure of what we are fixing.
2. We need to include a test to help avoid regressions.

[codecov]

Codecov Report

Merging #1252 (1c74222) into master (13a6143) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1252   +/-   ##
=======================================
  Coverage   97.29%   97.29%           
=======================================
  Files          31       31           
  Lines        1996     1997    +1     
=======================================
+ Hits         1942     1943    +1     
  Misses         54       54           

Impacted Files	Coverage Δ
oauth2_provider/oauth2_validators.py	94.00% <100.00%> (+0.01%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[realsuayip]

In oauthlib.oauth2.rfc6749.endpoints, create_token_response is used to generate an instance of oauthLib.common.Request.

In order to create this instance there are multiple parameters required, one of them being body, which includes the POST body. body (along with headers etc.) is extracted from Django's HttpRequest using _extract_params method of oauth2_provider.oauth2_backends.OAuthLibCore

Essentially, Request.body receives HttpRequest.POST.items(). Request body then stores all this data in an internal dict, and the class itself acts like AttrDict i.e., it allows request.anything as long as anything is mentioned in body.

The problem is, request.client is also used internally to assign Application instance to the request, when I send client in POST data, I override this behavior since the library only checks the existence of the key, not the contents.

Solutions that come to my mind:

1 - The solution in current PR, which checks for request.client contents.
2 - We could modify _extract_params method so that only the valid fields are passed (such as code, code_verifier, grant_type etc.).

There might other internal attributes which might be affected from this (in which case solution 2 would be better), I noticed refresh_token_instance, but I'm not sure about that one.

So, what do you think we should do?

[dopry]

Well it seems like the simple solution is to correct your typo....

However, you are right DOT could provide a better Developer Experience here.

Given your case, you intended to pass a client_id, but instead passed client. It could very well be that you didn't want the user to be logged into whatever the fallback client_id. That would be an erroneous situation that if we let it happen might created more subtle and harder to diagnose problems for you or your front end teams depending on your IDp.

I don't think falling back to a default as I believe this PR is proposing would be a good solution because of that. I could be misinterpreting the flow here though.

In theory the inputs to these endpoints should be restricted to those in the specification. Along that line of thinking any extraneous input could be considered an error, especially so if it conflicts with an internally reserved property on the request like in this case.

I think it would be better to validate the input and raise an error if we find a param that conflicts with a reserved property like client.

Alternatively we could silently ignore unknown properties as you proposed, but that could also lead to unexpected results for our users... although another lay of validation may handle that....

@realsuayip do validation errors seems like a reasonable course of action to you, or you do you have other thoughts based on my feedback?

[realsuayip]

do validation errors seems like a reasonable course of action to you, or you do you have other thoughts based on my feedback?

Yes, I think it's totally acceptable.

I think it would be better to validate the input and raise an error if we find a param that conflicts with a reserved property like client.

Does that mean we would only raise validation errors in case there is a conflict? If so, thats a bit odd. Maybe we should consider raising errors if given key is not valid.

However, you are right DOT could provide a better Developer Experience here.

Yes, besides, my main concern is exposing an endpoint where you can consistently crash the server. Some guy might just spam this endpoint with invalid requests just to deplete Sentry quotas 😁

[dopry]

I think it would be better to validate the input and raise an error if we find a param that conflicts with a reserved property like client.

Does that mean we would only raise validation errors in case there is a conflict? If so, thats a bit odd. Maybe we should consider raising errors if given key is not valid.

I feel there are valid reasons to pass other arguments like ?utm_* tags for marketing and activity contribution, or a GA session id from a cookie to use GA measurement protocol server side to track a token issue event and associate it with a users session. Or maybe transactions ids to associate the span on the token issuance with a user session for debugging.

For that reason I would lean toward of conservative of approach of only dis-allowing things we know will cause errors and conflict with our code.

However, you are right DOT could provide a better Developer Experience here.

Yes, besides, my main concern is exposing an endpoint where you can consistently crash the server. Some guy might just spam this endpoint with invalid requests just to deplete Sentry quotas 😁

That sounds like a reason to fix your typo. ;)

[realsuayip]

That sounds like a reason to fix your typo. ;)

To be clear, any Django application using django-oauth-toolkit endpoints could be crashed consistently using a malicious request.

[dopry]

That sounds like a reason to fix your typo. ;)

To be clear, any Django application using django-oauth-toolkit endpoints could be crashed consistently using a malicious request.

That was meant as a joke. The rest of the feedback stands. I'm not going to approve a solution that overwrites the client property in this case. We should validate the input property that is conflicting and only that property and we should allow other query parameters for use cases such as analytics attribution and span tracking for telemetry.

[n2ygk]

@dopry @realsuayip Where do we stand with this? Should I close it or @realsuayip will you be making the changes suggested by @dopry?

[dopry]

@n2ygk I think we should leave it open or convert it an issue if @realsuayip doesn't have the bandwidth. It is after all a bug we need to address.