Skip to content

Conversation

@aprilis
Copy link

@aprilis aprilis commented Dec 22, 2022

The format passed to strftime is incorrect and does not work with some browsers. It's better to pass datetime object to Django, which will handle formatting date correctly

Summary of change

I removed the .strftime(...) part converting datetime to string in incorrect way when passed to the function set_cookie in Django HttpResponse object

As a side-effect of the change, Django will add "Max-Age" parameter to every "Set-Cookie" header. I believe it's not a problem, however, to change this behavior, one should add strftime(...) back with correct date format

Related issues

Test Plan

I've applied the change and tested it with my server. The cookies are sent with correct date format now. Sign out now works for the browser where it didn't work before (any Webkit-based browser, Gnome Web for example).

obraz

Documentation changes

Not relevant

Checklist for important updates

  • Changelog has been updated
  • coreDriverInterfaceSupported.json file has been updated (if needed)
    • Along with the associated array in supertokens_python/constants.py
  • frontendDriverInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In setup.py
    • In supertokens_python/constants.py
  • Had installed and ran the pre-commit hook
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.
  • If have added a new web framework, update the supertokens_python/utils.py file to include that in the FRAMEWORKS variable
  • If added a new recipe that has a User type with extra info, then be sure to change the User type in supertokens_python/types.py

Remaining TODOs for this PR

The format passed to strftime is incorrect and does not work with some browsers. It's better to pass datetime object to Django, which will handle formatting datetime correctly
@KShivendu
Copy link
Contributor

Thanks for the PR @aprilis

As per https://docs.djangoproject.com/en/dev/ref/request-response/#django.http.HttpResponse.set_cookie and https://github.com/django/django/blob/0bd2c0c9015b53c41394a1c0989afbfd94dc2830/django/http/response.py#L239, it looks like it's safe and infact better to allow Django to take care of the values of expiry and max-age. So I think this PR is valid.

@aprilis Please update the CHANGELOG.md to mention your changes while @rishabhpoddar will also add his opinion on this. Thanks again!

@KShivendu
Copy link
Contributor

Hey @aprilis, thank you for raising this PR.

The issue reported by you is completely valid and we were able to replicate it. However, instead of removing .strftime, we'd prefer to provide it the correct time format which is already used by other frameworks and SDKs. Otherwise, it would introduce max-age header which, although seems safe, will make django inconsistent with other implementations.

Thanks again for the PR. We have to close this in favour of #270 but we really appreciate your effort :D

@KShivendu KShivendu closed this Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants