1520058 - browser.cookies.remove fails to remove ipv6 address cookies

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Kenny D]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Additional info here: https://github.com/Cookie-AutoDelete/Cookie-AutoDelete/issues/485

1. await browser.cookies.set({url: 'https://[2a03:4000:6:310e:216:3eff:fe53:99b3]/', name: 'test'});

2. await browser.cookies.remove({url: 'https://[2a03:4000:6:310e:216:3eff:fe53:99b3]/', name: 'test'});

Actual results:

The cookie is set in step 1, but is not removed in step 2. This affects Cookie AutoDelete since it's always trying to remove that cookies but it is unable to.

In addition, the above steps works as expected in Chrome.

Expected results:

The cookie should have been removed.

Comment 1

[Rob Wu [:robwu]]

I can confirm this. browser.cookies.get / getAll are also affected.

A work-around is to not use browser.cookies.remove, but browser.cookies.set with a date in the past (the expirationDate property).

Note: Sometimes there are stale cookies that appear in the cookies API (not visible to websites): bug 1388873
Setting an expiry date in the past did not seem to work as a work-around, so for these, you can either ignore them (because websites won't see these cookies anyway) or temporarily unexpire then expire the cookie again, as done in https://github.com/Rob--W/cookie-manager/commit/a66de74443dee384f9b173a2a202f38de576d29f

Comment 2

[Tom S [:evilpie]]

We use Services.cookies.getCookiesFromHost to query the cookies for the URL "https://[2a03:4000:6:310e:216:3eff:fe53:99b3]/". To do this we do new URL(...).host, which returns "[2a03:4000:6:310e:216:3eff:fe53:99b3]", the function however expects the IPv6 address without brackets.

i.e this works: Services.cookies.getCookiesFromHost("2a03:4000:6:310e:216:3eff:fe53:99b3", {})

Comment 3

[Tom S [:evilpie]]

As a temporary fix your extension could do await browser.cookies.getAll({domain: "2a03:4000:6:310e:216:3eff:fe53:99b3", path: "/", name: "test"});

Comment 4

[Rob Wu [:robwu]]

Note: The "work-around" from comment 3 (by Tom) applies to the getAll/get method, not remove (this bug).

Comment 5

[ithompson4]

Hello. I am new to Open source programming. Is this bug still open? If so I would like to try my hand at working on this with some mentoring. Thank you.

Comment 6

[Rob Wu [:robwu]]

This bug is in the "Unassigned" state, which means that it is indeed available.

I assume that you are already familiar with WebExtensions / browser extensions. If not, take a look at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions

The first step to contributing is to set up a development environment and get familiar with the relevant code.
To get started, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

The implementation of the cookies API is at toolkit/components/extensions/parent/ext-cookies.js.

Tom has described the issue with the current implementation in comment 2. Given the explanation from comment 2 and my link to the source file, do you think that you can find what you need to change in order to fix the bug?

Comment 7

[violet.bugreport]

Attachment: ipv6-cookie.patch

Hi, I'm new to Mozilla community. I've just built the Firefox from scratch, now I'd like to get some bugs to contribute.

I've fixed this bug by using Services.io.newURI() instead of URL() in query() function, so that it's consistent with set(). I've also expanded the cookie test case to cover ipv6. The patch passes the mochitest on my local machine.

My patch is attached, could you please review and tell me what to do next to get this patch into the codebase?

Thank you in advance!

Comment 8

[Rob Wu [:robwu]]

Thanks for the patch violet! Since ithompson hasn't replied to my previous question, and you have already created a patch, I'm going to assign this bug to you.

We use Phabricator for review. Could you submit the patch to Phabricator?
To learn more about creating an account and submitting the patch, see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

PS. Interestingly, we used to use nsiURI in the past too. It was replaced with the URL constructor for performance: https://hg.mozilla.org/mozilla-central/rev/b5c96a5d7b546e09dabf14e2d6e24e3231c1c391
Since the resulting code doesn't behave well (for IPv6 addresses), reverting back from URL to nsIURI looks reasonable. Especially since you're using the more fitting .filePath instead of .pathQueryRef.

Comment 9

[violet.bugreport]

Thanks for the response.

I found the problem seems to be more subtle.

There are other inconsistency caused by the drop of IPv6 bracket in nsIURI API. Currently, for cookie permission check, the manifest permissions can't contain a valid IPv6 url like "*://[2a03:4000:6:310e:216:3eff:fe53:99b3]/", it must be written in the incorrect way "*://2a03:4000:6:310e:216:3eff:fe53:99b3/" to have effect.

Because toolkit/components/extensions/parent/ext-cookies.js uses extension.whiteListedHosts.matches() to check permission. In the impl file toolkit/components/extensions/MatchPattern.cpp, MatchPattern::Matches() literally compares the domain string equality:

 if (DomainIsWildcard() || mDomain == aDomain) {

I couldn't find a clean way to correct it, because all of the subtlety is caused by the drop of bracket in the widely used nsIURI API, which is also used internally in MatchPattern.

My last patch doesn't address this problem (you can see from the permission field in my testcase).

I'm not familiar enough with the codebase. Do you have any suggestions?

Comment 10

[Rob Wu [:robwu]]

Interesting find! I've reported that issue separately at bug 1529230 .
Would your immediate issue be solved if you use the http://*/* permission instead of the IPv6 address in the permission string?

Comment 11

[violet.bugreport]

The wildcard works as expected.

Comment 12

[violet.bugreport]

Attachment: Bug 1520058 - Add brackets for the host obtained from nsIURI

Previously we used URL() to query cookie while newURI() was used to set cookie.
It will cause mismatch for IPv6 url due to the drop of brackets by newURL. Thus
we use newURI to query cookie.

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 9044844 [details] [diff] [review]
ipv6-cookie.patch

When re-uploading a patch to Phabricator, please mark the previous patch as obsolete. The presence of these obsolete patches sometimes confuse those who process the checkin-needed keywords.

If you upload the patch to Phabricator without first attaching a draft in Bugzilla, then you will have only one attachment, and updates in Phabricator will automatically be reflected here.

Comment 14

[violet.bugreport]

Hi Rob,

I have two questions regarding your review comments:

• Is there any chance that we can directly fix the root cause at nsIURI?

I'm aware it's probably not a trivial change, since it's a fundamental utility being used by many other components. But from what I see when fixing these bugs, it seems the current support for IPv6 stuff here is very limited, even basic operations are still in unusable state. So I speculate that it won't cause much trouble to other components as well if we directly correct the behavior of nsIURI.

Adding brackets in mDomain is easy, but I think it's rather ugly to manually manipulate IP addr presentation here, this should have been done is the URL utility layer.

• Is it better to remove all firstPartyDomain related stuff from the patch to this bug? It seems to be a separate issue.

Note: If we can fix the unfortunate behavior of nsIURI, this bug doesn't need a patch, it will automatically be fixed.

Comment 15

[Rob Wu [:robwu]]

(In reply to violet.bugreport from comment #14)

Hi Rob,

I have two questions regarding your review comments:

• Is there any chance that we can directly fix the root cause at nsIURI?

Why do you believe nsIRUI to be the root cause of the bug?
I think that it may be easier (but still quite difficult) to start fixing this at the cookie service layer, and update the documentation of the cookie service. The current documentation doesn't even mention IP addresses: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/netwerk/cookie/nsICookieManager.idl#91-99
Modifying the cookie service to accept brackets as hosts may be difficult, as there are also other components that don't expect brackets. For example, the eTLD service that classifies domains (used by nsCookieService::GetBaseDomainFromHost) doesn't recognize "[::1]" as an IP address:

// Run this from the global JS console (after enabling debugging at about:debugging), Ctrl-Shift-J
Services.eTLD.getBaseDomainFromHost('::1');  // throws NS_ERROR_HOST_IS_IP_ADDRESS
Services.eTLD.getBaseDomainFromHost('[::1]');// throws NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS

I'm aware it's probably not a trivial change, since it's a fundamental utility being used by many other components. But from what I see when fixing these bugs, it seems the current support for IPv6 stuff here is very limited, even basic operations are still in unusable state. So I speculate that it won't cause much trouble to other components as well if we directly correct the behavior of nsIURI.

nsIURI is used all over the place, and changing the behavior of .host/.asciiHost may have unexpected results, such as database entries becoming unusable. If nsURI is really the root cause of the problem, then you can consider changing the behavior and running try jobs. We are near a merge date (next week), so such risky changes have the opportunity to bake for a long while on Nightly to catch regressions not covered by unit tests. If this wrong behavior is common, and changing host/asciiHost is infeasible, then you can also consider introducing a new member, such as hostWithBrackets.

Adding brackets in mDomain is easy, but I think it's rather ugly to manually manipulate IP addr presentation here, this should have been done is the URL utility layer.

mDomain? I'm not seeing that in your patch here. I described how the API should behave (i.e. IPv6 addresses should have brackets), and other than that you are free to propose the implementation that you consider to be best.

• Is it better to remove all firstPartyDomain related stuff from the patch to this bug? It seems to be a separate issue.

Yes. I asked for the implementation change to be (re)moved in the review at https://phabricator.services.mozilla.com/D21828#643751 . It's fine if you also want to remove the new FPD tests.

Comment 16

[violet.bugreport]

Why do you believe nsIRUI to be the root cause of the bug?

Because the utilities in netwerk/base drop the brackets that shouldn't have been dropped in the first place, this is the root cause of the inconsistency. Services.io.newURI("https://[::1]:8080").host should just return [::1] rather than ::1 in the first place.

I took a quick look at the impl, the brackets are intentionally dropped: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/netwerk/base/nsStandardURL.h#526

getBaseDomainFromHost not accepting a correct IPv6 domain is caused by PR_StringToNetAddr: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/nsprpub/pr/src/misc/prnetdb.c#2224, which is also a widely used function. It also appears in netwerk/base.

Therefore, if those widely used internal function can behave correctly, we won't have inconsistency like this bug. Because all external APIs only accept/output an IPv6 with brackets. If this root cause isn't fixed, there will likely be more troubles when IPv6 is concerned.

OTOH, there aren't many fundamental utilities that will check the validity of a URL from scrach, so I believe it can be fixed by modifying popular API like PR_StringToNetAddr and nsIRUI.

(PS: it seems there is a typo in the code directory name, netwerk -> network? is it a bug?)

Comment 17

[Rob Wu [:robwu]]

(In reply to violet.bugreport from comment #16)

Why do you believe nsIRUI to be the root cause of the bug?

Because the utilities in netwerk/base drop the brackets that shouldn't have been dropped in the first place, this is the root cause of the inconsistency. Services.io.newURI("https://[::1]:8080").host should just return [::1] rather than ::1 in the first place.

I took a quick look at the impl, the brackets are intentionally dropped: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/netwerk/base/nsStandardURL.h#526

getBaseDomainFromHost not accepting a correct IPv6 domain is caused by PR_StringToNetAddr: https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/nsprpub/pr/src/misc/prnetdb.c#2224, which is also a widely used function. It also appears in netwerk/base.

Therefore, if those widely used internal function can behave correctly, we won't have inconsistency like this bug. Because all external APIs only accept/output an IPv6 with brackets. If this root cause isn't fixed, there will likely be more troubles when IPv6 is concerned.

OTOH, there aren't many fundamental utilities that will check the validity of a URL from scrach, so I believe it can be fixed by modifying popular API like PR_StringToNetAddr and nsIRUI.

Although the most common, these methods are probably not the ones that rely on the absence of brackets. If you are really interested in "fixing" the behavior, I suggest that you do that in a follow-up bug (in the necko component), so that the resulting patch only changes the behavior of IPv6 + brackets, instead of also including a fix for this bug. Otherwise this bug can be left open for a long while, which is a pity since your patch is almost ready.
And before starting the work, ask people from Necko whether they approve of changing the semantics of nsIURI.

(PS: it seems there is a typo in the code directory name, netwerk -> network? is it a bug?)

I don't know the origin of the name. In my native language (Dutch), "netwerk" actually means "network" in English. That might be a coincidence, or not. I don't know ;)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/661bafa045c4
Add brackets for the host obtained from nsIURI r=robwu,mixedpuppy

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/661bafa045c4

Comment 20

[Madalin Cotetiu]

Attachment: removeCookiesIpv6.png

Verified in Win7x64, Mac OS X 10.14.1 using FF68.0a1 (20190513215004)
I have created a test extension that uses (1) to set a cookie and (2) to remove a cookie.
(1) browser.cookies.set({url: 'https://[2a03:4000:6:310e:216:3eff:fe53:99b3]/', name: 'test'});
(2) browser.cookies.remove({url: 'https://[2a03:4000:6:310e:216:3eff:fe53:99b3]/', name: 'test'});

In the attached screenshot it is visible that after calling (1) the cookie is set and displayed and after calling (2) the cookie is no longer displayed.

Marking this bug as verified fixed.