1440263 - browser.cookies.remove does not remove cookies with ../ in path, or any Path values that are not canonical

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Lusito]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Steps to reproduce:

- Visit salzundbrot.com
- Notice a cookie "lang" being set with a path of "/usr/home/sites/salzundbrot.com/web/suche/yform/yform/php/../../"
- try to remove them with browser.cookies.remove


Actual results:

The cookie was not removed.


Expected results:

The cookie should have been removed.

Using firefox's own cookie manager in the options page I can delete the cookie, but not with the web-extensions API.
I've even tried normalizing the path to this without success:
"/usr/home/sites/salzundbrot.com/web/suche/yform/"

Comment 1

[Kris Maglione [:kmag]]

Can you please update a testcase add-on showing the problem?

Comment 2

[Lusito]

My add-on forget me not has this issue:
https://addons.mozilla.org/en-US/firefox/addon/forget_me_not/
Source:
https://github.com/Lusito/forget-me-not/

Or do you need an add-on with just this particular problem?

Comment 3

[Kris Maglione [:kmag]]

A minimized testcase add-on would be ideal

Comment 4

[Lusito]

I will try to put something up tomorrow or the day after.

Comment 5

[Lusito]

See:
https://github.com/Lusito/bug1440263

I've tried with chrome, and chrome seems to have the same issue.

Comment 6

[Kris Maglione [:kmag]]

Hm.

OK, the problem here is that our URL parser automatically normalizes paths, so when you pass a URL with ./ or ../ in it, those are removed from the path that we match the cookie against. I think this is generally the expected behavior, though, so I'm reluctant to change it.

The cookie service, on the other hand, doesn't do any sort of path normalization, and the weird (and probably unintended) path of this cookie is still "/usr/home/sites/salzundbrot.com/web/suche/yform/yform/php/../../" when we try to match it.

I suppose we could change the WebExtension cookie API to normalize the cookie paths when matching, but that would be expensive, and would be different from the behavior we get from HTTP.

And I'm not actually sure whether it's even possible to make an HTTP request that would see this cookie... We'd usually normalize the URL before ever making the request, and therefore strip out the relative paths.

Dan, any ideas what we should do here? It seems like the cookie service should either normalize or ignore cookies with these types of paths.

Comment 7

[Daniel Veditz [:dveditz]]

Early cookie specs said that you could only set a path= for the path that you were on (or a subset) which would have avoided this problem. In practice that broke things because one part of a site would try to set cookies for another part of the site. Didn't break much, to be sure, but enough popular services that at the time we had to buckle and follow the herd. The most recent version (2011) of the spec enshrines this: the stored path is either the actual path from the URL (the "default path") or it's simply the value given in the cookie header. If it's nonsense (as in this case) it will never be returned to the server until it eventually expires because it can never match.
https://tools.ietf.org/html/rfc6265#section-5.2.4

The WebExtension cookie APIs using "urls" to access cookies aren't a good match for the actual storage (name/domain/path tuples)  or the spec'd behavior. Maybe we inherited these APIs from Chrome extensions, but they're a bad fit, neither matching the storage nor how cookies would be selected to be shown to a server. How do the APIs distinguish between a "twitter.com" cookie vs. a ".twitter.com" domain cookie? If you have the same name+path cookie on both which one do the APIs access? I guess we do parse "https://.twitter.com/" as a valid URL even though it's not something you can actually load so maybe that's just what you do. 

So we're kind of stuck. If you canonicalize urls you can't access some valid (if nonsensical) cookies. If you don't convert it into a true URL then you've got to invent your own URL parsing/splitting which could lead to interop issues or bugs. We shouldn't change the cookie service: it's behaving correctly.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> The WebExtension cookie APIs using "urls" to access cookies aren't a good
> match for the actual storage (name/domain/path tuples)  or the spec'd
> behavior. Maybe we inherited these APIs from Chrome extensions, but they're
> a bad fit, neither matching the storage nor how cookies would be selected to
> be shown to a server. How do the APIs distinguish between a "twitter.com"
> cookie vs. a ".twitter.com" domain cookie? If you have the same name+path
> cookie on both which one do the APIs access? I guess we do parse
> "https://.twitter.com/" as a valid URL even though it's not something you
> can actually load so maybe that's just what you do.

The way the cookies API works is essentially to let the extension read and
modify cookies as if it were the site. When an extension passes a URL to an API
function, we read and write cookies as if that site were reading or writing
them. When setting cookies for https://twitter.com/, the extension can pass
"twitter.com" or ".twitter.com" for the domain.

The problem here is that there doesn't appear to be any URL which can access
these cookies, since we canonicalize all URLs that the cookie service deals
with, and it treats /../ as a path that / can't access. Which means that there's
no URL that we can pass to remove() that can remove the cookie, because there's
no URL that can really access it.

> We shouldn't change the cookie service: it's behaving correctly.

Is it, though? What's the purpose of allowing a site to set a cookie that no
site can access? Or am I missing something, and is there some way that some page
can access these cookies?

Comment 9

[Daniel Veditz [:dveditz]]

As an example of bad API design, if you .getAll() cookies you get Cookie objects that have a separate domain, name, and path, and then to manipulate them with the other APIs you have to concatenate "https://" + domain + path to create a "url" to use the other apis.

At the risk of platform incompatibilities, could we propose extending the url-using APIs to add "domain" and "path" to the details objects, and say 
   if details contains "domain" and "path"
      use domain and path
   else if details contains "domain"
      use domain and "/"
   else if details contains "url"
      use url as currently
   else error

Without playing with it, does the "domain" value in the Cookie object distinguish between "twitter.com" and ".twitter.com"? If "twitter.com" is returned for both (a guess based on the presence of the "hostOnly" property) then the get/remove APIs would have to also accept a "hostOnly" property.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> As an example of bad API design, if you .getAll() cookies you get Cookie
> objects that have a separate domain, name, and path, and then to manipulate
> them with the other APIs you have to concatenate "https://" + domain + path
> to create a "url" to use the other apis.
> 
> At the risk of platform incompatibilities, could we propose extending the
> url-using APIs to add "domain" and "path" to the details objects, and say

Perhaps. There's a bug that suggests this. But the way the manipulation API works right now, you need the URL of a page because the API is meant to act as if that page is sending a Set-Cookie: header.

> Without playing with it, does the "domain" value in the Cookie object
> distinguish between "twitter.com" and ".twitter.com"? If "twitter.com" is
> returned for both (a guess based on the presence of the "hostOnly" property)
> then the get/remove APIs would have to also accept a "hostOnly" property.

Yes

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)
> Is it, though? What's the purpose of allowing a site to set a cookie that no
> site can access? Or am I missing something, and is there some way that some
> page can access these cookies?

A site can set cookies with paths that don't exist (we'd never know) or even, as in this case, can't possibly exist. It's fallout from the spec change that recognized real-world brokenness I mentioned before. Does it make sense? No. But it made differently-broken sites work. Cookies were a very early under-specified web feature and we have to deal with the warts that exist. A decade of trying to specify "proper" behavior wasn't working so RFC 6265 gave up and specified what people have to live with.

> The way the cookies API works is essentially to let the extension read and
> modify cookies as if it were the site. When an extension passes a URL to an API
> function, we read and write cookies as if that site were reading or writing
> them.

But you're not, because to write cookies sites specify domain + path, not a URL--and the set() api does just that. The only way a server can "remove" a cookie is to "set" the same cookie (same domain + path + name) as expired. I can see your point that get/getAll could work like a site reading cookies, except get() only returns a single cookie and a site will actually be sent multiple cookies with the same name if they exist.

For example, if you're on https://www.example.com/some/path/file.html you could get

name  domain            path
-----------------------------
id    www.example.com   /
id    www.example.com   /some/
id    www.example.com   /some/path/
id    .www.example.com  /
id    .www.example.com  /some/
id    .www.example.com  /some/path/
id    .example.com      /
id    .example.com      /some/
id    .example.com      /some/path/

Obviously ridiculous, but getting two or three cookies of the same name is not rare. According to the spec you can't count on the order cookies will be returned, but because of earlier specs most browsers are fairly consistent (though there are differences from one browser to another).

Comment 12

[Lusito]

As said, I've tried normalizing the path, which did not change anything, so my best guess is, that contrary to our statement, the web-extension API normalizes the path.

In any way, web-extensions should be able to modify and delete all cookies. Not just what the website can.
I like the idea of specifying the tuples rather than a url to delete/modify exactly the right cookie.

Comment 13

[Rob Wu [:robwu]]

In order to support proper removal of cookies with special paths, the cookies.remove API needs a path property (and domain, as pointed out in comment 9). I have also suggested that in crbug.com/834699 as part of bug 1387957.

The current API defaults to taking the pathname component of the input URL (source), but the Path can be set to an arbitrary value, including ../ and ?.

RFC 6265, section 4.1.2.4 states:

The scope of each cookie is limited to a set of paths, controlled by
the Path attribute. If the server omits the Path attribute, the user
agent will use the "directory" of the request-uri's path component as
the default value. (See Section 5.1.4 for more details.)

Comment 15

[mal]

This also affects paths with . components, e.g. /./ as seen in the navigation-20200227455 cookie set by https://visiblemerch.com/. This bug makes the Cookie-AutoDelete addon unable to remove that cookie.