Wrong typing for expires in set_cookie

[maartenbreddels]

I think the type for

starlette/starlette/responses.py

Line 108 in 55b0cbd

expires: typing.Optional[int] = None,

This should be a date according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

So I think a better type is Optional[str] , or Optional[str | datetime] and provide the right formatting.

[Kludex]

I have the feeling we had this discussion before... Need to find where. 🤔

EDIT: Maybe not?
EDIT2: It was not discussed before.

[Kludex]

Hold on... Does the field expires is even used by the browser as is?

EDIT: It is, I've checked on Google Chrome. It works as if it was seconds. Can someone double check this?

[Kludex]

Based on RFC-6265, we should send a string in cookie date format, as the OP said.

We have even reference in code using expires as it should (introduced 4 years ago in #158):

starlette/starlette/middleware/sessions.py

Lines 76 to 82 in 9a1e885

	header_value = "{session_cookie}={data}; path={path}; {expires}{security_flags}".format( # noqa E501
	session_cookie=self.session_cookie,
	data="null",
	path=self.path,
	expires="expires=Thu, 01 Jan 1970 00:00:00 GMT; ",
	security_flags=self.security_flags,
	)

[Kludex]

Questions:

1. Why the current expires works?
2. Should we change the type from Optional[int] (Union[int, None]) to Union[int, str, None]?
3. Should we accept datetime instead, and let Starlette format as cookie date?

[maartenbreddels]

1. I think it doesn't work (maybe on chrome) when you pass an int, but ignoring typing you can safely pass a str in the right format (as I do now).
2. I think Optional[Union[datetime, str]] makes the most sense, as I don't think int is described in any standard (that I know of)
3. that would be great.

[Kludex]

It would be cool to understand why it works on Chrome 👀

[Kludex]

PR welcome to change the type annotation, and add datetime, with conversion to the format mentioned.

[oskipa]

Hi, there! I created a pull request adding the changes requested by @Kludex . I made a point to follow them strictly.

As I was working on the issue, two questions came to me, which I think it is good to discuss.

1. Integers are used by other frameworks to set expiration dates in seconds. So even though one can argue that integers are not valid cookie date tokens, it has been interpreted so by other frameworks, servers, and browsers

2. This change may break existing code.

For these two reasons, I would suggest to add int to the allow types, for this value to be cast to string, and then set as the expiration date, as a courtesy for existing code.

If we want to insist that int shouldn't be allowed, we could add the suggestion above along with a deprecated warning about supporting ints for expirations.

Again, these are suggestions. This is the first time I have attempted to contribute to this project, so whatever you decide is best :) Thanks for making this beautiful framework.

[maartenbreddels]

My best guess is that if you set is as int, it does not work and you get a session cookie. If you don’t use mypy (I did, but it never raised an error until now, strange but off-topic) you just need to pass it the right date format (which I did). I think optional date str would be perfect.

On Fri, 30 Sep 2022 at 21:51, Marcelo Trylesinski ***@***.***> wrote: Questions: 1. Why the current expires works? 2. Should we change the type from Optional[int] (Union[int, None]) to Union[int, str, None]? 3. Should we accept datetime instead, and let Starlette format as cookie date <https://www.rfc-editor.org/rfc/rfc6265#section-5.1.1>? — Reply to this email directly, view it on GitHub <#1878 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANPEPN43EFTVSFLL4WZT2DWA5ADRANCNFSM6AAAAAAQZYGOLA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Maarten Breddels Software engineer / consultant / data scientist Python / C++ / Javascript / Jupyter www.maartenbreddels.com / vaex.io ***@***.*** +31 6 2464 0838 <+31+6+24640838> [image: Twitter] <https://twitter.com/maartenbreddels>[image: Github] <https://github.com/maartenbreddels>[image: LinkedIn] <https://linkedin.com/in/maartenbreddels>[image: Skype]

[Kludex]

Integers are used by other frameworks to set expiration dates in seconds. So even though one can argue that integers are not valid cookie date tokens, it has been interpreted so by other frameworks, servers, and browsers

Which frameworks, servers and browsers?

[foolishhugo]

Integers are used by other frameworks to set expiration dates in seconds. So even though one can argue that integers are not valid cookie date tokens, it has been interpreted so by other frameworks, servers, and browsers

Which frameworks, servers and browsers?

Php, for example, uses seconds from epoch, https://www.php.net/manual/en/function.setcookie.php

I thought I saw more examples, but maybe it was just PHP :)

[Kludex]

@maartenbreddels if we just accept datetime it's enough for you, right?

[maartenbreddels]

I would also support string, unless I’m 100+% sure with datetime we can cover the whole spec (which in reality is unlikely). The risk of adding no escape hatch is just too high in practice. So I’d say datetime | str to be safe.

[Kludex]

Why is it unlikely?