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

bpo-35824: Update http.cookies._CookiePattern #11665

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@MeiK-h
Copy link

MeiK-h commented Jan 24, 2019

Modified a regular expression to correctly parse Expires in Cookies

http.cookies.BaseCookie can't parse Expires in this format like Expires=Thu,31 Jan 2019 05:56:00 GMT;(Less space after Thu,).

I encountered this problem in actual use, Chrome, IE and Firefox can parse this string normally. Many languages, such as JavaScript, can also parse this data automatically.

I looked for mdn, and the examples they gave were spaces, but didn't do more.

I don't know the specification of pr and whether this pr is necessary. If it doesn't make sense, please close it.

English is not my native language; please excuse typing errors.

https://bugs.python.org/issue35824

@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Jan 24, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Jan 24, 2019

Please open a bpo for this. I don't know if one exists for this. I am not an expert but googling gets me to https://tools.ietf.org/html/rfc2616#section-3.3.1 that has the date format for cookie expires attribute that mandates a space character after comma. The MDN links to general Date header field so I am not sure if it can be used for cookies.

Set cookie date format RFC reference https://tools.ietf.org/html/rfc6265#section-4.1.1

Thanks for the report

@vadmium

This comment has been minimized.

Copy link
Member

vadmium commented Jan 26, 2019

In the regular expression, I think a question mark is more usual; I suggest “\s?” rather than “\s{0,1}”.

It would be good to add a test case.

@MeiK-h MeiK-h force-pushed the MeiK-h:master branch from 37f0984 to d1f0d32 Jan 27, 2019

@MeiK-h

This comment has been minimized.

Copy link
Author

MeiK-h commented Jan 27, 2019

In the regular expression, I think a question mark is more usual; I suggest “\s?” rather than “\s{0,1}”.

It would be good to add a test case.

Yes, that is obviously a better way.

Thanks for your guidance, I have modified the code and added the test. If I have other errors, please also point out.

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Jan 27, 2019

This might require a NEWS entry since the space is optional but I don't know how to word it though.

https://devguide.python.org/committing/#what-s-new-and-news-entries

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Jan 27, 2019

Also the PR title needs to be modified to the required format before committing I hope in with "bpo-35824" prefixed.

@MeiK-h MeiK-h changed the title Update http.cookies._CookiePattern bpo-35824: Update http.cookies._CookiePattern Jan 27, 2019

@MeiK-h

This comment has been minimized.

Copy link
Author

MeiK-h commented Jan 27, 2019

This might require a NEWS entry since the space is optional but I don't know how to word it though.

https://devguide.python.org/committing/#what-s-new-and-news-entries

I think it can be written like this:

Modify the parsing of Expires in Set-Cookie, Spaces and commas are both treated as delimiters.

If you think this works, I will add it and resubmit.

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Jan 27, 2019

Modify the parsing of Expires in Set-Cookie, Spaces and commas are both treated as delimiters.

LGTM. Please include "Patch by <full_name>" at the end. It will keep the PR green and can be rephrased if needed by the core dev while merging.

Thanks

@MeiK-h MeiK-h force-pushed the MeiK-h:master branch from d1f0d32 to 2847cf2 Jan 27, 2019

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