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

Strip quotes for cookie values for .get() #140

Merged

Conversation

naruaway
Copy link
Contributor

@naruaway naruaway commented Aug 2, 2021

According to RFC 2965, cookie value can be "quoted-string" and in practice, some web framework (e.g. Java Spring) sometimes uses quoted value.
In such cases, web browser will send HTTP headers like Cookie: my_value_a=x; my_value_b="abc:def:xyz"; to the server and it will be more developer friendly to automatically strip quotes so that cookie.get('my_value_b') returns abc:def:xyz rather than "abc:def:xyz".

Is this this library's responsibility?

I guess so. I checked the behavior of Django and Express and both of them strip quotes under the hood.

What's the impact of this change?

koa is relying on this library. After this fix, koa can get the same behavior as Django and Express, which seems to be more reasonable and common behavior. I created an issue for koa for the discussion in koa side.

Note that this change will be breaking change so we need to bump up the major version of the library.

index.js Outdated Show resolved Hide resolved
@dougwilson dougwilson added the pr label Jan 19, 2022
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Sorry for the delay on getting to this. I rebased + simplified + added changelog.

@dougwilson dougwilson merged commit e44ddaf into pillarjs:master Jan 19, 2022
@naruaway
Copy link
Contributor Author

naruaway commented Apr 13, 2022

@dougwilson Thank you so much!
If possible, could you publish the new version? It looks like the latest one is still 0.8.0, published 3 years ago 🙏

@dougwilson
Copy link
Contributor

Hi @naruaway apologies, I didn't realize I never made a release 😂 I'll get it out today/tomorrow 👍

@naruaway
Copy link
Contributor Author

Hi @dougwilson, thanks for the quick response!
Are you going to release patch or minor, or major?

Strictly speaking this could be "breaking change" but on the other hand we could say this is a bug fix 🤔
One benefit of releasing this as a patch version would be, koa will get this change automatically without changing any koa code (they are specifying ~0.8.0)

My personal feeling is that patch version would be fine (I think no one should rely on the previous behavior considering the behavior of other frameworks like Django...) but it's your call of course 🙏

@dougwilson
Copy link
Contributor

Hi @naruaway this is not the only change in the release. It will be included in the 0.9.0 release.

@naruaway
Copy link
Contributor Author

Hi @dougwilson, oh right, it makes sense. Thanks for all your work 🙏

@naruaway
Copy link
Contributor Author

naruaway commented Jul 3, 2022

Hi @dougwilson, are you going to release 0.9.0? Sorry for pinging you but a dev from koa mentioned that they will bump up the version of cookies after 0.9.0 gets released

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

Successfully merging this pull request may close these issues.

2 participants