Skip to content

Commit e02b1f7

Browse files
committed
gh-154546: Make http.cookies.BaseCookie.load() atomic on error
BaseCookie.load() parses the whole cookie string before applying it, but the key and attribute legality checks run during the apply phase (inside Morsel.set() and Morsel.__setitem__()). An illegal key or unknown $-prefixed attribute part-way through the string therefore raised CookieError only after the earlier cookies had already been written to the jar, leaving it in a partially-updated state. Build the morsels in a temporary mapping and only commit them to the cookie jar once the entire string has been validated, so load() is now all-or-nothing. Existing morsels are copied before being updated, so a failing load() no longer mutates cookies that were already present.
1 parent 6c026ad commit e02b1f7

3 files changed

Lines changed: 49 additions & 3 deletions

File tree

Lib/http/cookies.py

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,11 @@ def __parse_string(self, str, patt=_CookiePattern):
624624
# Invalid cookie string
625625
return
626626

627-
# The cookie string is valid, apply it.
627+
# The cookie string parsed cleanly. Build the morsels in a temporary
628+
# mapping and only mutate self once every key and attribute has been
629+
# validated, so that an illegal key or attribute later in the string
630+
# cannot leave the cookie jar partially updated (gh-154546).
631+
built = {} # key -> Morsel, staged until the whole string is valid
628632
M = None # current morsel
629633
for tp, key, value in parsed_items:
630634
if tp == TYPE_ATTRIBUTE:
@@ -633,8 +637,18 @@ def __parse_string(self, str, patt=_CookiePattern):
633637
else:
634638
assert tp == TYPE_KEYVALUE
635639
rval, cval = value
636-
self.__set(key, rval, cval)
637-
M = self[key]
640+
if key in built:
641+
M = built[key]
642+
elif key in self:
643+
# Preserve attributes already set on an existing morsel,
644+
# but work on a copy so self stays untouched until commit.
645+
M = self[key].copy()
646+
else:
647+
M = Morsel()
648+
M.set(key, rval, cval)
649+
built[key] = M
650+
for key, M in built.items():
651+
dict.__setitem__(self, key, M)
638652

639653

640654
class SimpleCookie(BaseCookie):

Lib/test/test_http_cookies.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,34 @@ def test_illegal_chars(self):
320320
with self.assertRaises(cookies.CookieError):
321321
C.load(rawdata)
322322

323+
def test_load_does_not_apply_partial_state_on_error(self):
324+
# BaseCookie.load() is all-or-nothing: if the raw string contains an
325+
# illegal key or attribute that raises CookieError, none of the
326+
# cookies from that string are applied to the jar (gh-154546).
327+
for rawdata in (
328+
"a=1; b,c=2; d=3", # illegal key in the middle
329+
"good=1; bad,key=2", # illegal key at the end
330+
"a=1; $foo=2", # unknown $-prefixed attribute
331+
):
332+
with self.subTest(rawdata=rawdata):
333+
C = cookies.SimpleCookie()
334+
with self.assertRaises(cookies.CookieError):
335+
C.load(rawdata)
336+
self.assertEqual(dict(C), {})
337+
self.assertEqual(C.output(), '')
338+
339+
def test_load_error_leaves_existing_cookies_untouched(self):
340+
# A load() that raises CookieError must not mutate cookies that were
341+
# already present in the jar (gh-154546).
342+
C = cookies.SimpleCookie()
343+
C['existing'] = 'kept'
344+
C['existing']['path'] = '/here'
345+
with self.assertRaises(cookies.CookieError):
346+
C.load("new=1; bad,key=2")
347+
self.assertEqual(list(C), ['existing'])
348+
self.assertEqual(C['existing'].value, 'kept')
349+
self.assertEqual(C['existing']['path'], '/here')
350+
323351
def test_comment_quoting(self):
324352
c = cookies.SimpleCookie()
325353
c['foo'] = '\N{COPYRIGHT SIGN}'
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
:meth:`http.cookies.BaseCookie.load` is now atomic: if the cookie string
2+
contains an illegal key or attribute that raises
3+
:exc:`~http.cookies.CookieError`, the cookie jar is left unchanged instead of
4+
being partially updated with the cookies that preceded the error.

0 commit comments

Comments
 (0)