Skip to content

Commit

Permalink
Parse set header cookies properly (mitmproxy#5084)
Browse files Browse the repository at this point in the history
#### Description

Currently when an empty cookie attribute (`Secure`, `HttpOnly` ...) is
encountered while parsing a `Set-Cookie` header it will create a
`CookieAttrs` object containing a (key, value) pair with an empty string
for the attribute value ie:

```python
CookieAttrs[('Secure', ''), ('HttpOnly', ''), ('Path', '/')]
``` 
Resulting in an updated `Set-Cookie` header for the `Response` object
with invalid values for those empty attributes ie:
```python
(b'SetCookie', b'value=XYZ; Secure=; HttpOnly=; Path=/')
``` 
My browser (Firefox 95.0.1) does not pickup these attributes so the
cookie looses them.

______

This fix replaces the empty string attribute for empty cookie attributes
by the value `None` ie:

```python
CookieAttrs[('Secure', None), ('HttpOnly', None), ('Path', '/')]
``` 

So that they can be told apart from attributes with intentional empty
string values when setting the updated header, which results in a
properly formatted header:

```python
(b'SetCookie', b'value=XYZ; Secure; HttpOnly; Path=/')
``` 

#### Checklist

 - [x] I have updated tests where applicable.
 - [x] I have added an entry to the CHANGELOG.

Co-authored-by: Lucas FICHEUX <lficheux@corp.free.fr>
  • Loading branch information
Speedlulu and Lucas FICHEUX committed Dec 2, 2023
1 parent 72679e5 commit 43bbcef
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 22 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,8 @@

## Unreleased: mitmproxy next

* Fix empty cookie attributes being set to `Key=` instead of `Key`
([#5084](https://github.com/mitmproxy/mitmproxy/pull/5084), @Speedlulu)


## 14 November 2023: mitmproxy 10.1.5
Expand Down
17 changes: 11 additions & 6 deletions mitmproxy/net/http/cookies.py
Expand Up @@ -48,8 +48,8 @@ def _reduce_values(values):
return values[-1]


TSetCookie = tuple[str, str, CookieAttrs]
TPairs = list[list[str]] # TODO: Should be List[Tuple[str,str]]?
TSetCookie = tuple[str, str | None, CookieAttrs]
TPairs = list[tuple[str, str | None]]


def _read_until(s, start, term):
Expand Down Expand Up @@ -169,10 +169,10 @@ def _read_set_cookie_pairs(s: str, off=0) -> tuple[list[TPairs], int]:
rhs = rhs + "," + trail

# as long as there's a "=", we consider it a pair
pairs.append([lhs, rhs])
pairs.append((lhs, rhs))

elif lhs:
pairs.append([lhs, rhs])
pairs.append((lhs, None))

# comma marks the beginning of a new cookie
if off < len(s) and s[off] == ",":
Expand Down Expand Up @@ -206,10 +206,15 @@ def _format_pairs(pairs, specials=(), sep="; "):
"""
vals = []
for k, v in pairs:
if k.lower() not in specials and _has_special(v):
if v is None:
val = k
elif k.lower() not in specials and _has_special(v):
v = ESCAPE.sub(r"\\\1", v)
v = '"%s"' % v
vals.append(f"{k}={v}")
val = f"{k}={v}"
else:
val = f"{k}={v}"
vals.append(val)
return sep.join(vals)


Expand Down
30 changes: 15 additions & 15 deletions test/mitmproxy/net/http/test_cookies.py
Expand Up @@ -93,23 +93,23 @@ def test_cookie_roundtrips():

def test_parse_set_cookie_pairs():
pairs = [
["=", [[["", ""]]]],
["=;foo=bar", [[["", ""], ["foo", "bar"]]]],
["=;=;foo=bar", [[["", ""], ["", ""], ["foo", "bar"]]]],
["=uno", [[["", "uno"]]]],
["one=uno", [[["one", "uno"]]]],
["one=un\x20", [[["one", "un\x20"]]]],
["one=uno; foo", [[["one", "uno"], ["foo", ""]]]],
["=", [[("", "")]]],
["=;foo=bar", [[("", ""), ("foo", "bar")]]],
["=;=;foo=bar", [[("", ""), ("", ""), ("foo", "bar")]]],
["=uno", [[("", "uno")]]],
["one=uno", [[("one", "uno")]]],
["one=un\x20", [[("one", "un\x20")]]],
["one=uno; foo", [[("one", "uno"), ("foo", None)]]],
[
"mun=1.390.f60; "
"expires=sun, 11-oct-2015 12:38:31 gmt; path=/; "
"domain=b.aol.com",
[
[
["mun", "1.390.f60"],
["expires", "sun, 11-oct-2015 12:38:31 gmt"],
["path", "/"],
["domain", "b.aol.com"],
("mun", "1.390.f60"),
("expires", "sun, 11-oct-2015 12:38:31 gmt"),
("path", "/"),
("domain", "b.aol.com"),
]
],
],
Expand All @@ -120,10 +120,10 @@ def test_parse_set_cookie_pairs():
"path=/",
[
[
["rpb", r"190%3d1%2616726%3d1%2634832%3d1%2634874%3d1"],
["domain", ".rubiconproject.com"],
["expires", "mon, 11-may-2015 21:54:57 gmt"],
["path", "/"],
("rpb", r"190%3d1%2616726%3d1%2634832%3d1%2634874%3d1"),
("domain", ".rubiconproject.com"),
("expires", "mon, 11-may-2015 21:54:57 gmt"),
("path", "/"),
]
],
],
Expand Down
2 changes: 1 addition & 1 deletion test/mitmproxy/test_http.py
Expand Up @@ -569,7 +569,7 @@ def test_get_cookies_with_parameters(self):
assert attrs["domain"] == "example.com"
assert attrs["expires"] == "Wed Oct 21 16:29:41 2015"
assert attrs["path"] == "/"
assert attrs["httponly"] == ""
assert attrs["httponly"] is None

def test_get_cookies_no_value(self):
resp = tresp()
Expand Down

0 comments on commit 43bbcef

Please sign in to comment.