Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access Token Request media type should be application/x-www-form-urlencoded #1067

Closed
baxang opened this issue Mar 29, 2018 · 6 comments
Closed
Milestone

Comments

@baxang
Copy link
Contributor

baxang commented Mar 29, 2018

Steps to reproduce

curl --include \
     --request POST \
     --header "Content-Type: application/json" \
     --data-binary '{"client_id":"9e48cc9491746c04ee5732b31e7cbb777352e066be149c878da955845e2f71c9", "grant_type": "password", "username": "[email protected]", "password": "secret"}' \
'http://localhost:3000/oauth/token'

I added Doorkeeper to an existing Rails 4.2 app and it's been working pretty well. Then I found out one of clients (a native mobile app) is sending auth requests in JSON format which is just by accident as other API endpoints are using the JSON API spec.

According to the spec, Access Token Request has parameters using the "application/x-www-form-urlencoded" format so I think it is better only allowing the correct media type for token requests.

Expected behavior

Returning a 415 Unsupported media type

Actual behavior

A successful authorization response is returned.

System configuration

Ruby version: 4.2

@nbulaj
Copy link
Member

nbulaj commented Mar 29, 2018

I don't think we can break compatibility for our current users and force them to rewrite their native / other apps to use this.

@baxang
Copy link
Contributor Author

baxang commented Mar 30, 2018

I understand your concern about backward compatibility - can we consider introducing a flag (say, config.strict_content_type = true) in a new major version at least? I think this compatibility is accidental and any reasonably written OAuth client should be standard compliant.

@nbulaj
Copy link
Member

nbulaj commented Apr 3, 2018

I understand your concern about backward compatibility - can we consider introducing a flag (say, config.strict_content_type = true) in a new major version at least?

This one make sense. Also it would be great to add a deprecation message that will inform developers about necessity of using required media type for the applications using the API. Be default this option now must be false, bunt in future update (5.2 for example) we will change it to true.

Do you want to send a PR ?

@nbulaj nbulaj added this to the 5.0 milestone Apr 3, 2018
@hallucinations
Copy link
Contributor

@nbulaj, @baxang The specs also talks about the the character encoding of the request should be UTF-8.

The client makes a request to the token endpoint by sending the
following parameters using the "application/x-www-form-urlencoded"
format per Appendix B with a character encoding of UTF-8 in the HTTP
request entity-body:

This essentially means that the request should have the following header

Content-type: application/x-www-form-urlencoded; charset=utf-8

Is my understanding correct? Should we enforce character encoding as well?

@baxang
Copy link
Contributor Author

baxang commented Apr 8, 2018

Thanks for additional checkups @hallucinations! I think you're right. Requests should be UTF-8 encoded.

FYI, I'm working on a patch @nbulaj

@baxang
Copy link
Contributor Author

baxang commented Apr 8, 2018

I've noticed that none of example requests in the spec specifies charset although appendix B clearly says all payloads MUST be encoded using UTF-8. I think it is reasonable to not requiring ;charset=utf-8 bit in the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants