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

Add a warning when a cookie's length exceeds 4096 bytes #5959

Merged
merged 12 commits into from
Oct 6, 2021
Merged

Add a warning when a cookie's length exceeds 4096 bytes #5959

merged 12 commits into from
Oct 6, 2021

Conversation

anesabml
Copy link
Contributor

@anesabml anesabml commented Aug 20, 2021

What do these changes do?

Add warning when a cookie's length exceeds the RFC 6265 minimum client support.

Are there changes in behavior for the user?

The user might see a warning if a cookie size exceeds the RFC 6265 minimum client support.

Related issue number

Fixes #5634

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@anesabml anesabml requested a review from asvetlov as a code owner August 20, 2021 13:04
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 20, 2021
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #5959 (c119613) into master (079e9c5) will increase coverage by 33.12%.
The diff coverage is 83.33%.

❗ Current head c119613 differs from pull request most recent head 9fc9d57. Consider uploading reports for the commit 9fc9d57 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5959       +/-   ##
===========================================
+ Coverage        0   33.12%   +33.12%     
===========================================
  Files           0       44       +44     
  Lines           0     9863     +9863     
  Branches        0     1594     +1594     
===========================================
+ Hits            0     3267     +3267     
- Misses          0     6514     +6514     
- Partials        0       82       +82     
Flag Coverage Δ
unit 33.08% <83.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiohttp/helpers.py 43.54% <83.33%> (ø)
aiohttp/formdata.py 18.27% <0.00%> (ø)
aiohttp/http.py 100.00% <0.00%> (ø)
aiohttp/resolver.py 40.00% <0.00%> (ø)
aiohttp/typedefs.py 100.00% <0.00%> (ø)
aiohttp/http_websocket.py 23.92% <0.00%> (ø)
aiohttp/web_request.py 38.97% <0.00%> (ø)
aiohttp/web_exceptions.py 77.25% <0.00%> (ø)
aiohttp/locks.py 35.71% <0.00%> (ø)
aiohttp/connector.py 17.59% <0.00%> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079e9c5...9fc9d57. Read the comment docs.

@webknjaz
Copy link
Member

cc @panagiks

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Aug 29, 2021

So, in Firefox it only counts key + value, limited to 4096. But, in Chromium it counts the length of the raw cookie header, regardless of how much data is actually stored.

So, to be on the safe side, we should limit the total output to 4096 bytes. A good test might be roughly:

async def test_warn_large_cookie(buf: Any, writer: Any) -> None:
    resp = Response()
    resp.set_cookie("foo", "8"*4070, max_age=2600)
    req = make_request("GET", "/", writer=writer)

    await resp.prepare(req)  # No warning
    await resp.write_eof()

    cookie = re.search(b"Set-Cookie: (.*?)\r\n", buf).group(1)
    assert len(cookie) == 4096
    buf[:] = b""

    resp = Response()
    resp.set_cookie("foo", "8"*4071, max_age=2600)
    req = make_request("GET", "/", writer=writer)

    with pytest.warns(UserWarning, match="size of the cookie"):
        await resp.prepare(req)
    await resp.write_eof()

    cookie = re.search(b"Set-Cookie: (.*?)\r\n", buf).group(1)
    assert len(cookie) == 4097

I think this would also need a new way to run tests with dev mode. I don't see any examples online about how to do this, but maybe just marking tests which need dev mode would work, then we can add an extra pytest invocation which only runs the dev mode tests with something like python -X dev -m pytest -m dev_mode.

@Dreamsorcerer
Copy link
Member

Although, if the check becomes trivial at runtime, then maybe we don't need dev mode after all.

@webknjaz
Copy link
Member

But, in Chromium it counts the length of the raw cookie header, regardless of how much data is actually stored.

We should be RFC-compliant. Deviating from the standards leads to chaos: https://xkcd.com/927/.

@Dreamsorcerer
Copy link
Member

But, in Chromium it counts the length of the raw cookie header, regardless of how much data is actually stored.

We should be RFC-compliant. Deviating from the standards leads to chaos: https://xkcd.com/927/.

The RFC isn't that clear on precisely what counts. But, Chrome has the most pessimistic approach, so if we warn on this case, we can be sure to catch all possible issues. i.e. No browser is going to drop cookies that are smaller than what Chrome is accepting (and if they did, it would definitely be violating the RFC, whereas Chrome is more of a grey-area due to the imprecise explanation in the RFC).

On this occasion, I think it makes more sense to take the pessimistic approach and play it safe. Maybe to avoid confusion to users, we shouldn't mention the 4096 bytes in the warning and just say something like "Sending a large cookie. Some browsers may ignore this."

@webknjaz webknjaz enabled auto-merge (squash) October 6, 2021 10:53
@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2021

Thanks, everyone! I've set it to auto-merge on CI success.

@webknjaz webknjaz merged commit e5af76a into aio-libs:master Oct 6, 2021
@patchback

This comment has been minimized.

@aio-libs-github-bot

This comment has been minimized.

@webknjaz webknjaz added backport:skip Skip backport bot and removed backport-3.8 labels Oct 6, 2021
@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2021

This change cannot be backported to 3.8 because it relies on the code that hasn't been (and can't be) backported: #5197 (comment).

@Dreamsorcerer
Copy link
Member

I don't see why, the code just needs to be added to the Response class instead of CookieMixin, at the end of this function:
https://github.com/aio-libs/aiohttp/blob/3.8/aiohttp/web_response.py#L255

@anesabml anesabml deleted the cookie-length branch October 6, 2021 16:38
@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2021

Maybe. But my point is that it wouldn't be the same thing exactly. But if anybody wants to make a manual effort, feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip Skip backport bot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[server] Add warning when a cookie's length would exceed the RFC 6265 minimum user-agent support
3 participants