Check host permissions for browser.cookies

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: evilpie, Assigned: kmag)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [cookies][permissions])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
bug 1197417 comment 12

Updated

2 years ago
Whiteboard: [cookies]
Blocks: 1214433
Priority: -- → P1

Updated

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

Updated

2 years ago
Duplicate of this bug: 1227453
(Assignee)

Comment 2

2 years ago
Added a note to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities
Keywords: dev-doc-needed

Updated

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

Comment 3

2 years ago
Created attachment 8695596 [details] [diff] [review]
[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.
Attachment #8695596 - Flags: review?(wmccloskey)
(Assignee)

Updated

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

Comment 4

2 years ago
Created attachment 8696004 [details] [diff] [review]
[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.
Attachment #8696004 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 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

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1d6e1baf6604
Status: NEW → RESOLVED
Last Resolved: 2 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
You need to log in before you can comment on or make changes to this bug.