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

Validate cookie values wrapped in double-quotes #33765

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
8 participants
@thismachinechills
Copy link

thismachinechills commented Jul 6, 2018

Servers sometimes send headers with cookie values that are encapsulated in double-quotes. Dart should validate values surrounded with double-quotes instead of throwing a FormatException.

This addresses Issue #33327 directly. I applied the solution to this problem that was solved in Go's code base.

thismachinechills added some commits Jul 6, 2018

Validate cookie values wrapped in double-quotes
Servers sometimes send headers with cookie values that are encapsulated in double-quotes. Dart should validate values surrounded with double-quotes instead of throwing a FormatException.
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jul 6, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no label Jul 6, 2018

@thismachinechills

This comment has been minimized.

Copy link
Author

thismachinechills commented Jul 6, 2018

I signed it!

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jul 6, 2018

CLAs look good, thanks!

@cbracken

This comment has been minimized.

Copy link
Member

cbracken commented Jul 7, 2018

/cc @zanderso

@kevmoo

This comment has been minimized.

Copy link
Member

kevmoo commented Jul 8, 2018

@thismachinechills

This comment has been minimized.

Copy link
Author

thismachinechills commented Aug 8, 2018

I've made changes as per @zanderso's comments.

Please see commits made to this pull request here.

A test was added in PR #34102

@zanderso

This comment has been minimized.

Copy link
Member

zanderso commented Aug 8, 2018

@kevmoo Will the new commit get mirrored over to Gerrit?

@kevmoo

This comment has been minimized.

Copy link
Member

kevmoo commented Aug 8, 2018

I'm not sure – @alexander-doroshko @whesse ?

@alexander-doroshko

This comment has been minimized.

Copy link

alexander-doroshko commented Aug 9, 2018

I'm not sure – @alexander-doroshko @whesse ?

@kevmoo, sorry, what's the question for me?

@whesse

This comment has been minimized.

Copy link
Member

whesse commented Aug 9, 2018

No, the last commit to this pull request was not mirrored over to the Gerrit CL https://dart-review.googlesource.com/c/sdk/+/63920
I don't know why that is.

As always, a pull request on SDK should never be landed directly, but the Gerrit CL created from it should be submitted (via CQ, usually).

It should work to check out the Gerrit CL on a new branch, using git cl patch --force, and to fetch the source of this pull request from the foreign remote, do a local merge (or fast-forward merge), and upload the new commit to Gerrit with git cl upload.

Since this change was small, I edited directly in the Gerrit UI to add it.
The Gerrit CL is now up-to-date, and can be committed.
It would be better to have the added test in the same CL, but that seems hard to do at this point.

@whesse

This comment has been minimized.

Copy link
Member

whesse commented Aug 9, 2018

This pull request has both a syntax error, which prevents it from building, and a logical error, which makes the test fail. Both of these have been fixed in the linked Gerrit CL, but we should not accept pull requests where the author has neither built the code nor tested it.

@whesse

This comment has been minimized.

Copy link
Member

whesse commented Aug 9, 2018

If this change allows Dart to also send invalid cookies with quote characters, as well as receive them, then we should not accept it as-is.

I think we want to continue validating the cookies that a Dart program sends out according to the strict rules, even if we allow incoming cookies to violate them in this way.

Can the author verify that this won't allow Dart to create and send cookies that violate the standard? Maybe write a test?

It would be best to do further work on the Gerrit CL, since in order to compile this change and run tests, you need to download depot_tools and make a source checkout anyway.

@whesse

This comment has been minimized.

Copy link
Member

whesse commented Aug 9, 2018

OK, looks like the latest spec allows quotes surrounding the whole value, but then those quotes must be retained as part of the value, not dropped:

http://www.ietf.org/rfc/rfc6265.txt
"plus for compatibility with any poor idiots who actually implemented the earlier RFCs it also banned backslash and quotes, other than quotes wrapping the whole value (but in that case the quotes are still considered part of the value, not an encoding scheme). So that leaves you with the alphanums plus:"

@dart-bot dart-bot closed this in a9ad427 Aug 10, 2018

@Sense545

This comment has been minimized.

Copy link

Sense545 commented Jan 24, 2019

This commit breaks cookies which have an empty value but no quotes when server tires to expire it, e.g.
CookieName=; expires=somedateinthepast
The change

+ if (value[0] == '"' && value[value.length - 1] == '"') {
+  value = value.substring(1, value.length - 1);
+ }

should have checked if value.length > 0 before doing it's thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.