-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Description
I was just looking into the OAuth specs, and I noticed that the token_endpoint_auth_method can be one of a few values.
When using client_secret_post which is most commonly used when interacting with endpoints like the /oauth/token endpoint or other client-authorized endpoints, the current code seems to allow passing client_secret via query parameters, instead of only via the request body. From what I can tell OAuth never allowed passing client_secret via the query parameters.
Here's the relevant specification sections:
- https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1
- https://www.rfc-editor.org/rfc/rfc7591.html#section-2
We probably need to introduce a :from_body method and deprecate :from_params, since passing the client_secret via query parameters risks leaking the client credentials to logs on intermediaries (e.g., nginx or CDNs).
The code for :from_params is currently:
def from_params(request)
request.parameters.values_at(:client_id, :client_secret)
endWhen we probably want something like:
def from_body(request)
# not sure if there's a better method to get the values from the body whilst supporting application/json, application/x-www-form-urlencoded, and multipart/form-data
request.request_parameters.values_at(:client_id, :client_secret)
endNote: the client_id can always be passed via the query parameters, from what I can tell, as it's not a sensitive value.
I also don't have a test case yet to prove that query parameters instead of body is possible, but I'm basing this on the documentation which indicates (via the source code) that it's retrieving from the request_parameters, query_parameters or path_parameters.
This isn't really a active security risk for anyone, hence no need for security vulnerability disclosure, as it would rely on a client already doing an insecure practice.