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

A limit on redirect_uri only up to 255 chars #902

Closed
2 tasks done
shaddeus opened this issue Dec 11, 2020 · 10 comments · Fixed by #903
Closed
2 tasks done

A limit on redirect_uri only up to 255 chars #902

shaddeus opened this issue Dec 11, 2020 · 10 comments · Fixed by #903
Labels

Comments

@shaddeus
Copy link
Contributor

shaddeus commented Dec 11, 2020

Describe the bug
Cannot handle redirect_uri longer then 255 chars. RFC 7230 recommends to design system to be capable to work with URI at least to 8000 chars long.

It is RECOMMENDED that all HTTP senders and recipients support, at a minimum, request-line lengths of 8000 octets.

To Reproduce
Send a request to AuthorizationView with redirect_uri longer than 255 chars.

Expected behavior
To save redirect_uri into Grant model.

Version
1.3.3

  • I have tested with the latest published release and it's still a problem.
  • I have tested with the master branch and it's still a problem.
@shaddeus shaddeus added the bug label Dec 11, 2020
@MattBlack85
Copy link
Contributor

@shaddeus thanks for raising this issue, can you provide a failing test case?

@shaddeus
Copy link
Contributor Author

@MattBlack85 Thanks for super fast response. Yes, I can provide failing test case (unit test, yes?). I will try to send a merge request with fix.
I am going to change AbstractGrant.redirect_uri to models.TextField(). Would it be OK?

@MattBlack85
Copy link
Contributor

@shaddeus sounds like a plan!

@shaddeus
Copy link
Contributor Author

Unfortunatelly I have a problem to write a corrent unit test for this. The problem depends on DB representation models.CharField. SQLite transform VARCHAR(255) into TEXTdata type.
https://sqlite.org/datatype3.html#affinity_name_examples

So the unit test passes always. The test must be run on another DB engine to fail.

Should I append this "useless" test or not?

@MattBlack85
Copy link
Contributor

@shaddeus I'd say let's add it, we may want to discuss then if we should test against another DB or that would be enough

shaddeus pushed a commit to shaddeus/django-oauth-toolkit that referenced this issue Dec 11, 2020
RFC 7230 recommends to design system to be capable to work with URI at least
to 8000 chars long. This commit allows handle redirect_uri that is over 255
chars.

Fixes issue jazzband#902.
shaddeus pushed a commit to shaddeus/django-oauth-toolkit that referenced this issue Dec 11, 2020
RFC 7230 recommends to design system to be capable to work with URI at least
to 8000 chars long. This commit allows handle redirect_uri that is over 255
chars.

Fixes issue jazzband#902.
@folt
Copy link
Contributor

folt commented Dec 12, 2020

Why not select the URLField for solution?

@shaddeus
Copy link
Contributor Author

shaddeus commented Dec 14, 2020

@folt Hi. URLField is nice a semantic idea! URLField extends CharField class with default max_length=200. I'm thinking about oauth2_provider DEFAULTS setting max_lenght parameter for URLField redirect_uri called for example REDIRECT_URI_MAX_LENGTH? And a default value could be 8000?

@folt
Copy link
Contributor

folt commented Dec 14, 2020

The requirement of 8000 characters for a link makes it impossible to use CharField and the fields that inherit from it (URLField). The idea of using a TextField looks appealing.
I propose to make on the basis of TextField your own field in which you need to put a validaror from the URLField.

@shaddeus
Copy link
Contributor Author

The requirement of 8000 characters for a link makes it impossible to use CharField and the fields that inherit from it (URLField).

Why? In Django doc is written that CharField is for a string field, for small- to large-sized strings. And a note for max_length for backend databases says

Any fields that are stored with VARCHAR column types may have their max_length restricted to 255 characters if you are using unique=True for the field.

But we don't use unique=True parameter. So we are not affected with this limitation.

For example MySQL and PostgreSQL have a limit 65535 chars for VARCHAR. But Yes, I'm not sure for other DB.

@mtucker
Copy link

mtucker commented Dec 16, 2020

I have a similar issue with the token field on AccessToken. I've had success following some of the guidelines for generating a JWT token but often those JWTs exceed the varchar(255) limit on the token field.

settings.py

OAUTH2_PROVIDER = {
    "ACCESS_TOKEN_GENERATOR": "myapp.auth.tokens.generate_jwt_access_token",
}

myapp.auth.tokens.generate_jwt_access_token

from oauthlib.common import generate_signed_token

key = "<private_key>"

def generate_jwt_access_token(request, refresh_token=False):
    request.claims = {
        "foo": "bar"
    }
    return generate_signed_token(key, request)

shaddeus pushed a commit to shaddeus/django-oauth-toolkit that referenced this issue Dec 18, 2020
MattBlack85 pushed a commit that referenced this issue Dec 18, 2020
* Disable 255 chars length limit for redirect uri (#902)

RFC 7230 recommends to design system to be capable to work with URI at least
to 8000 chars long. This commit allows handle redirect_uri that is over 255
chars.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants