Check host permissions for browser.cookies

RESOLVED FIXED in Firefox 46

Status

P1
normal
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: evilpie, Assigned: kmag)

Tracking

({dev-doc-complete})

unspecified
mozilla46
dev-doc-complete
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [cookies][permissions])

Attachments

(1 attachment, 1 obsolete attachment)

Updated

4 years ago
Whiteboard: [cookies]

Updated

3 years ago
Flags: blocking-webextensions+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1227453

Updated

3 years ago
Assignee: nobody → kmaglione+bmo
Iteration: --- → 45.3 - Dec 14
(Assignee)

Comment 3

3 years ago
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.
Attachment #8695596 - Flags: review?(wmccloskey)
(Assignee)

Updated

3 years ago
Summary: Check host permissions for chrome.cookies → Check host permissions for browser.cookies
Whiteboard: [cookies] → [cookies][permissions]
(Assignee)

Comment 4

3 years ago
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.
Attachment #8696004 - Flags: review?(wmccloskey)
(Assignee)

Updated

3 years ago
Attachment #8695596 - Attachment is obsolete: true
Attachment #8695596 - Flags: review?(wmccloskey)
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?
Attachment #8696004 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

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

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1d6e1baf6604
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
docs (written by Kris): https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies#Permissions
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.