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

Fixing a parsing problem for cookie data. #104

Closed
wants to merge 1 commit into from
Closed

Fixing a parsing problem for cookie data. #104

wants to merge 1 commit into from

Conversation

wladston
Copy link

Quotes are valid to appear surrounding cookie data, and thus should not
be removed.

Quotes are valid to appear surrounding cookie data, and thus should not
be removed.
@mcdonc
Copy link
Member

mcdonc commented Aug 10, 2013

I have to plead ignorance on this. Do you have any citation for your assertion that the current behavior is incorrect?

@wladston
Copy link
Author

mcdonc, I think all ASCII characters are valid to be part of cookie-data, including the quotation mark character. I searched a little bit and found nothing related to invalid formats of cookies, or forbidden cookie characters.

@mcdonc
Copy link
Member

mcdonc commented Aug 19, 2013

I'll probably need to see some positive evidence before changing things.

@wladston
Copy link
Author

The thing is, changing the informed cookie data just seems plain wrong to me. Check the unit test at issue #103, if you think that test should really fail, then you can discard this pull request.

@digitalresistor
Copy link
Member

Since I was bored ...

Read a bunch of RFC's and all that fun stuff, and here is the relevant one:

http://tools.ietf.org/html/rfc6265#section-4.1.1

More specifically it states that characters in the following ranges are valid:

%x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E

Which equals this:

0x21: !
0x23-2B: #$%&'()*+
0x2D-3A: -./0123456789: 
0x3C-5B: <=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[
0x5D-7E: ]^_`abcdefghijklmnopqrstuvwxyz{|}~

Specifically it states this:

                   ; US-ASCII characters excluding CTLs,
                   ; whitespace, DQUOTE, comma, semicolon,
                   ; and backslash

The only time the DQUOTE (double quote) is allowed to appear is around the cookie-octet:

cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )

Basically cookie-octet with quotes surrounding it is the same as cookie-octet without the surrounding double quotes.

Thus "cookie-val" is the same as cookie-val, and expecting the double quotes to be preserved as part of the cookie value is wrong since the spec states they are not a valid part of the cookie-octet.

@wladston: If you want to store data that is outside of the characters listed above, may I suggest base64 encoding the data.

@mcdonc: The current way WebOb handles this is valid.

@wladston wladston closed this Nov 11, 2013
@wladston
Copy link
Author

@bertjwregeer, impressive, you are correct. Thanks for the research!

marcospri added a commit to hypothesis/lms that referenced this pull request Apr 10, 2024
We had to remove the white space character from the cookie as is not
allowed, see:

Pylons/webob#104 (comment)
marcospri added a commit to hypothesis/lms that referenced this pull request Apr 10, 2024
We had to remove the white space character from the cookie as is not
allowed, see:

Pylons/webob#104 (comment)
marcospri added a commit to hypothesis/lms that referenced this pull request Apr 10, 2024
We had to remove the white space character from the cookie as is not
allowed, see:

Pylons/webob#104 (comment)
marcospri added a commit to hypothesis/lms that referenced this pull request Apr 10, 2024
We had to remove the white space character from the cookie as is not
allowed, see:

Pylons/webob#104 (comment)
marcospri added a commit to hypothesis/lms that referenced this pull request Apr 11, 2024
We had to remove the white space character from the cookie as is not
allowed, see:

Pylons/webob#104 (comment)
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

3 participants