1845870 - Cookie per host quota enforcement is off by one

Status: ASSIGNED

(Core :: Networking: Cookies, defect, P3)

Description

[Daniel Veditz [:dveditz]]

In CookieCommons.h we define kCookieQuotaPerHost = 150;, but after a purge we have 151 cookies left. To trigger a purge we have to have 181 cookies, but that appears intended because we define kMaxCookiesPerHost = 180;

STR:

1. pick a domain you don't care about, like https://example.org, and clear all it's cookies
2. in the DevTools console on that site run

for (i = 1; i <= 181; i++)  document.cookie = `c${String(i).padStart(3,0)}=0`;
document.cookie.split(';').length

Expected result: 150
Actual result: 151

Being off by one isn't that big a deal because the quota is more of a suggestion in the spec. But we differ from Chrome and that might have unwanted effects on some site someday.

Comment 1

[Daniel Veditz [:dveditz]]

In CookieStorage.cpp when we go to add our 181st cookie we find that we already have kMaxCookiesPerHost (180) stored
https://searchfox.org/mozilla-central/rev/50588a0b728b365afdd298debd35e8302efe7850/netwerk/cookie/CookieStorage.cpp#535

So we calculate how many cookies we ought to delete and get 180-150 = 30:

     uint32_t limit = mMaxCookiesPerHost - mCookieQuotaPerHost;

So we delete that many and are left with the expected 150 cookies for that host. But remember we were in the middle of adding a cookie when we checked the quotas. So now we add that new cookie and end up with 151.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Should be trivial to fix. @Dylan?

Comment 3

[tannal2409]

Attachment: Bug 1845870 - Cookie per host quota shouldn't be greater than mMaxCookiesPerHost. r?edgul

Comment 4

[tannal2409]

Attachment: Bug 1845870 - Cookie per host quota shouldn't be greater than mMaxCookiesPerHost. r?edgul

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 6

[edgul]

Thanks for your work on this tannel,
Anything I can do to help this along?

Comment 7

[tannal2409]

Hi, I updated the revision, please take a look again when you have a moment.
I finally got that trick test pass.
Thank you for reminding me about this bug.

Comment 8

[tannal2409]

(In reply to edgul from comment #6)

Thanks for your work on this tannel,
Anything I can do to help this along?

I have a few questions:

1. Should I add new tests for these changes?
2. Where should I add them?
3. How should I implement them?

Comment 9

[edgul]

The coverage we have looks sufficient. Just need to make sure all the exiting tests pass in a way that maintains their integrity. I'll review your patch again.