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

wrap-session middleware doesn't url-decode the cookie before using it. #28

Closed
wants to merge 1 commit into from

Conversation

ghoseb
Copy link
Contributor

@ghoseb ghoseb commented Aug 19, 2011

I faced a weird bug today wherein the browser was sending the cookie string back to the server after url-encoding it and wrap-session tried decrypting the cookie without decoding the encoded cookie.

I am attaching a trivial patch that fixes the problem. url-decoding is idempotent so it won't affect anything even when the cookie is not url-encoded.

@weavejester
Copy link
Collaborator

This patch seems like it's fixing the symptoms, rather than the root cause of the problem.

If cookie values are not being url-encoded under some circumstances, this is a problem to be fixed in the cookie middleware. Do you know under what conditions this bug occurs?

@ghoseb
Copy link
Contributor Author

ghoseb commented Aug 23, 2011

Hi,

I am not sure if the cookie middleware should be able to tell if the cookie data is url-encoded or not because not all cookies will get url-encoded. I can reproduce the problem when I use the session middlware to store the session in an encrypted cookie. When the encrypted data is base64 encoded it yields a string with characters like = and / in it which get url-encoded by the browser on its way back to %3D and %2F respectively but the session middleware doesn't decode the string before trying to decrypt it.

If you still believe it should be in the cookie middleware I am willing to rework the patch.

Thanks.

@weavejester
Copy link
Collaborator

It is unlikely that the browser is the cause. Browsers do not automatically url-encode cookies, and whilst it's possible that there could be some Javascript url-encoding cookie values, it doesn't seem that likely.

The Ring cookie middleware does url-encode cookie values in order to ensure special characters can be safely added. Perhaps this problem is caused by the cookie middleware being applied twice, or perhaps there is some url-encoding going on elsewhere that is causing this problem.

But in all cases, we need to discover the cause of the bug before we attempt a solution. If we do not understand why the session key is mysteriously url-encoded, then we shouldn't try and fix it by patching the symptoms. We should instead attempt to understand the cause, and fix the problem at the root.

@weavejester weavejester closed this Oct 9, 2011
dcmoore referenced this pull request in dcmoore/ring Nov 16, 2012
Remove reflection warnings in ring-core
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.

None yet

2 participants