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

Improved cookie rewriting #91

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Improved cookie rewriting #91

wants to merge 5 commits into from

Conversation

emilhem
Copy link
Contributor

@emilhem emilhem commented Aug 1, 2016

Includes test!

We should look into all the places where the libCookie.parse and libCookie.serialize is used to ensure that the value isn't changed. Probably better not to use it at all!

The reason is that we change the cookie value by putting it though all the parsers (encodeURIComponent/decodeURIComponent).
We should never parse the cookie values ever, only its parameters!
@nfriedly
Copy link
Owner

nfriedly commented Aug 1, 2016

Oh, good catch. Maybe we should add a rawValue to set-cookie-parser and use that?

@emilhem
Copy link
Contributor Author

emilhem commented Aug 1, 2016

It's a good idea. Although the parser part of the name could be confusing if it returns a string.

Forgot to remove the decodeURIComponent.
@emilhem
Copy link
Contributor Author

emilhem commented Aug 1, 2016

There, I forgot to remove decodeURIComponent. It's not needed.

This way we are sure that we don't parse the cookie value.
This commit includes a replacement for libCookie.serialize to use serializeCookie instead.
It also removes libCookie completely.
@emilhem
Copy link
Contributor Author

emilhem commented Aug 1, 2016

Now libCookie is removed completely replaced by my new parseCookies function.

We no longer need this since we parse cookies ourself! :D
@nfriedly
Copy link
Owner

nfriedly commented Aug 1, 2016

Ok, let me look at this one a bit more.

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