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.

[dopry]

@n2ygk I added the tests for this and updated the _load_application implementation. I'd appreciate a quick second review if you can find a moment.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[dopry]

@realsuayip if you have time to test my changes to the PR I'd appreciate it.

[Copilot]

Pull Request Overview

This PR refactors the _load_application method in the OAuth2 validator to implement caching logic and improve error handling. The changes replace an assertion-based approach with a more robust client validation mechanism that checks cached clients before performing database lookups.

Key Changes

• Replaced assertion check with conditional logic to handle cached request.client instances
• Added comprehensive validation for cached clients (type checking, client_id matching, and usability checks)
• Improved logging messages throughout the method for better debugging
• Added extensive test coverage for the refactored _load_application method

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
oauth2_provider/oauth2_validators.py	Refactored _load_application to implement client caching logic with validation and improved logging
tests/test_oauth2_validators.py	Added 7 new test cases covering various scenarios of the refactored _load_application method including cache hits, mismatches, and usability checks
tests/test_authorization_code.py	Added test case to verify error handling when using incorrect parameter name (client instead of client_id) in token request

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Corrected spelling of 'alredy' to 'already'.

Suggested change

	""" check for cached client, to save the db hit if this has alredy been loaded """
	""" check for cached client, to save the db hit if this has already been loaded """

[Copilot]

Corrected spelling of 'erroroneously' to 'erroneously'.

Suggested change

	log.debug("request.client is not an Application, something else set request.client erroroneously, resetting request.client.")
	log.debug("request.client is not an Application, something else set request.client erroneously, resetting request.client.")

[Copilot]

Use double quotes for docstrings instead of triple-quoted strings for inline comments. Change to a regular comment using # or convert to a proper multi-line docstring.

[Copilot]

Use double quotes for docstrings instead of triple-quoted strings for inline comments. Change to a regular comment using # or convert to a proper multi-line docstring.

Suggested change

	""" cache wasn't hit, load from db """
	# cache wasn't hit, load from db