Skip to content

gh-92936: allow double quote in cookie values #113663

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

Merged
merged 2 commits into from
Aug 8, 2025

Conversation

nburns
Copy link
Contributor

@nburns nburns commented Jan 2, 2024

As detailed more extensively in: #92936 it's not uncommon to see cookies with json and therefore double quotes in them. While IMO a well behaved application would base64 or otherwise encode those values, there are numerous services putting double quotes in the cookies, and it would be nice to have support for this directly in python so various http server/client/util libraries don't need to implement their own cookie parsing. Additionally browsers are tolerant of and handle double quotes in cookie values so SimpleCookie should also.

Before this change SimpleCookie will without error drop cookie values that appear after a value with a double quote in them, which can lead to some very confusing and hard to debug issues when implementing http clients and servers.
After this change SimpleCookie allows a double quote character in the values section of the cookie while making no attempt to determine if the value is valid json or anything else (since that's a application/usecase specific concern)

Downstream issues:
aio-libs/aiohttp#7993
yt-dlp/yt-dlp#4780

@@ -442,7 +442,7 @@ def OutputString(self, attrs=None):
( # Optional group: there may not be a value.
\s*=\s* # Equal Sign
(?P<val> # Start of group 'val'
"(?:[^\\"]|\\.)*" # Any doublequoted string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

webob handles cookies with quotes more robustly: https://github.com/Pylons/webob/blob/main/src/webob/cookies.py#L335

@nburns nburns force-pushed the allow-double-quote-in-cookie-values branch 2 times, most recently from 78dce80 to 1b78ed1 Compare January 8, 2024 18:22
@nburns nburns force-pushed the allow-double-quote-in-cookie-values branch from 1b78ed1 to 3eae48a Compare January 12, 2024 15:51
@nburns
Copy link
Contributor Author

nburns commented Jan 12, 2024

@orsenthil sorry for the random mention, but I saw you were active on some other cookie related issues in the past and wondered if you could review this or help me get pointed in the direction to find someone to review this?

@nburns nburns force-pushed the allow-double-quote-in-cookie-values branch from 95af939 to e9b3cd5 Compare September 4, 2024 16:45
@danbirken
Copy link

I have nothing to add to this other than to say that I spent about 4 hours today debugging an issue caused and it almost drove me to madness before I found the problem. This is a particularly pernicious issue because the error is caused by a level of the infrastructure you expect to never fail.

I'm sure this impacts people all the time and they have no idea why their thing doesn't work and just give up.

Keep fighting the good fight trying to get this merged! I have no power to help you but the PR looks good to me.

Copy link
Member

@orsenthil orsenthil left a comment

Choose a reason for hiding this comment

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

Added an additional test case demonstrating the need for a lazy match in regex.

Copy link
Member

@orsenthil orsenthil left a comment

Choose a reason for hiding this comment

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

  1. Formatted the test case.
  2. Added an additional test for lazy matching of quoted characters, which would fail if the match was greedy.

@orsenthil
Copy link
Member

@nburns ,and @danbirken - thanks for raising this issue. Apologize for not looking into this or responding to the ping.

@python-cla-bot
Copy link

python-cla-bot bot commented Aug 8, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@nburns nburns force-pushed the allow-double-quote-in-cookie-values branch 2 times, most recently from f88d0a7 to 72c88f9 Compare August 8, 2025 16:42
nburns and others added 2 commits August 8, 2025 16:50
Co-authored-by: Senthil Kumaran <senthil@python.org>
@nburns nburns force-pushed the allow-double-quote-in-cookie-values branch from 72c88f9 to 081ace2 Compare August 8, 2025 16:50
@nburns
Copy link
Contributor Author

nburns commented Aug 8, 2025

@orsenthil Thanks for taking a look, I applied your suggestion and fixed up the tests, also updated the news entry and signed the CLA, is there anything else I can do to help move this PR forward?

@orsenthil
Copy link
Member

I applied your suggestion and fixed up the tests, also updated the news entry

Thank you for updating and fixing the tests. I didn't realize the failure when I ran it yesterday. Thanks.

@orsenthil orsenthil merged commit d7dbde8 into python:main Aug 8, 2025
46 checks passed
@picnixz
Copy link
Member

picnixz commented Aug 8, 2025

Can we have a follow-up PR here that adds a What's New entry and updates the documentation of http.cookies please?

@picnixz
Copy link
Member

picnixz commented Aug 8, 2025

Also, please be careful but we have some examples with quotes on the online docs so maybe they should be updated.

@nburns
Copy link
Contributor Author

nburns commented Aug 8, 2025

Sure, I'll take a look

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL8 Refleaks 3.x (tier-1) has failed when building commit d7dbde8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/259/builds/3132) and take a look at the build logs.
  4. Check if the failure is related to this commit (d7dbde8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/259/builds/3132

Failed tests:

  • test.test_multiprocessing_fork.test_processes

Failed subtests:

  • test_interrupt - test.test_multiprocessing_fork.test_processes.WithProcessesTestProcess.test_interrupt

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 588, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 570, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 566, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-2103' pid=185712 parent=169200 started daemon>


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/process.py", line 320, in _bootstrap
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 518, in _sleep_some_event
    time.sleep(100)
    ~~~~~~~~~~^^^^^
KeyboardInterrupt
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 588, in test_interrupt
    exitcode = self._kill_process(multiprocessing.Process.interrupt)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 570, in _kill_process
    self.assertEqual(join(), None)
                     ~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 250, in __call__
    return self.func(*args, **kwds)
           ~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/process.py", line 156, in join
    res = self._popen.wait(timeout)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 44, in wait
    return self.poll(os.WNOHANG if timeout == 0.0 else 0)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/multiprocessing/popen_fork.py", line 28, in poll
    pid, sts = os.waitpid(self.pid, flag)
               ~~~~~~~~~~^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64.refleak/build/Lib/test/_test_multiprocessing.py", line 566, in handler
    raise RuntimeError('join took too long: %s' % p)
RuntimeError: join took too long: <Process name='Process-1' pid=196164 parent=196162 started daemon>

@nburns
Copy link
Contributor Author

nburns commented Aug 8, 2025

updated the docs in #137566

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.

5 participants