1210996 - Check host permissions for browser.cookies

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Tom S [:evilpie]]

bug 1197417 comment 12

Comment 2

[Kris Maglione [:kmag]]

Added a note to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities

Comment 3

[Kris Maglione [:kmag]]

Attachment: [webext] Check host permissions in the browser.cookies API

I'm not sure if this is the same permission checking behavior Chrome uses, but
it's what seemed the most correct. It may be worth asking for sec-review from
someone who works with cookies a lot, but I don't know who that would be.

I also fixed the batch-deleted observer, which didn't work at all, after I
added tests that assumed it did.

Comment 4

[Kris Maglione [:kmag]]

Attachment: [webext] Check host permissions in the browser.cookies API

I realized that I got some of the permissions checks wrong, and prevented
add-ons from reading some domain cookies their permissions should let them
read. So I spent a few hours reading specs and code, and made some additional
changes. I'm much more confident that this version is correct. But it's also
much more complicated.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8696004 [details] [diff] [review]
[webext] Check host permissions in the browser.cookies API

Review of attachment 8696004 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/ext-cookies.js
@@ +34,5 @@
> +  let matcher = extension.whiteListedHosts;
> +
> +  // First check for simple matches.
> +  let secureURI = NetUtil.newURI(`https://${cookie.rawHost}/`);
> +  if (matcher.matchesIgnoringPath(secureURI)) {

So an HTTPS permission gives you access to both HTTPS and HTTP cookies? Is that what Chrome does?

@@ +50,5 @@
> +
> +  // Things get tricker for domain cookies. The extension needs to be able
> +  // to read any cookies that could be read any host it has permissions
> +  // for. This means that our normal host matching checks won't work,
> +  // since the pttern "*://*.foo.example.com/" doesn't match ".example.com",

pattern

@@ +59,5 @@
> +  // with hosts that end with our cookie's host.
> +
> +  let { host, isSecure } = cookie;
> +
> +  for (let pattern of matcher.matchers) {

This code should live in MatchPattern.jsm, just to preserve abstraction boundaries.

@@ +61,5 @@
> +  let { host, isSecure } = cookie;
> +
> +  for (let pattern of matcher.matchers) {
> +    let scheme = pattern.scheme;
> +    if (scheme.includes("https") || (!isSecure && scheme.includes("http"))) {

Why includes and not ==?

@@ +97,5 @@
> +  // So instead, we do a similar set of checks here. Exactly what
> +  // cookies a given URL should be able to set is not well-documented,
> +  // and is not standardized in any standard that anyone actually
> +  // follows. So instead, we follow the rules used by the cookie
> +  // service.

Perhaps reference the file and function where this happens? nsCookieService::SetCookieInternal?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #5)
> @@ +34,5 @@
> > +  let matcher = extension.whiteListedHosts;
> > +
> > +  // First check for simple matches.
> > +  let secureURI = NetUtil.newURI(`https://${cookie.rawHost}/`);
> > +  if (matcher.matchesIgnoringPath(secureURI)) {
> 
> So an HTTPS permission gives you access to both HTTPS and HTTP cookies? Is
> that what Chrome does?

More or less. There isn't actually any such thing as HTTPS cookies, just
cookies with the |isSecure| flag and those without it. HTTPS sites can read
cookies with or without the flag, but HTTP sites can only read those without
it.

> @@ +61,5 @@
> > +  let { host, isSecure } = cookie;
> > +
> > +  for (let pattern of matcher.matchers) {
> > +    let scheme = pattern.scheme;
> > +    if (scheme.includes("https") || (!isSecure && scheme.includes("http"))) {
>
> Why includes and not ==?

Because those properties are arrays, rather than strings. This confused me the
first time I looked at MatchPattern.jsm too, though, so we should probably
rename that property |schemes|.

> @@ +97,5 @@
> > +  // So instead, we do a similar set of checks here. Exactly what
> > +  // cookies a given URL should be able to set is not well-documented,
> > +  // and is not standardized in any standard that anyone actually
> > +  // follows. So instead, we follow the rules used by the cookie
> > +  // service.
> 
> Perhaps reference the file and function where this happens?
> nsCookieService::SetCookieInternal?

Yeah. I'll add a comment there referencing this code too.

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/1d6e1baf6604f3ea5b980f7e3f72976c2ef6c703
Bug 1210996: [webext] Check host permissions in the browser.cookies API. r=billm

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d6e1baf6604

Comment 9

[Will Bamberg [:wbamberg]]

docs (written by Kris): https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies#Permissions