Grant storage access to 3rd party, tracking resource on window.open()

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 months ago
When window.open(URL) is executed by a 3rd party context, in the TP list, we want to add URL to the first party access storage list.
(Assignee)

Comment 1

10 months ago
Posted patch window.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8991061 - Flags: review?(ehsan)
(Assignee)

Comment 2

10 months ago
Posted patch window.patchSplinter Review
Better approach.
Attachment #8991061 - Attachment is obsolete: true
Attachment #8991061 - Flags: review?(ehsan)
Attachment #8991078 - Flags: review?(ehsan)

Comment 3

10 months ago
Comment on attachment 8991078 [details] [diff] [review]
window.patch

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

r=me with the below addressed, especially the comment about tests!

::: dom/base/nsContentUtils.h
@@ +2976,5 @@
>                                            nsIChannel* aChannel,
>                                            nsIURI* aURI);
>  
>    /*
> +   * Returns true if this window's channel has been marked as tracking resource.

Nit: s/as/as a/

::: dom/base/nsGlobalWindowOuter.cpp
@@ +7074,5 @@
> +  }
> +
> +  nsCOMPtr<nsIURI> url;
> +  nsresult rv = NS_NewURI(getter_AddRefs(url), aUrl.get(),
> +                          document->GetDocBaseURI());

You have a bug here in parsing this URL: you're not taking the document's charset encoding into account!  Compare to this other place where we parse the URL around the same place: <https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/base/nsGlobalWindowOuter.cpp#7133>

In order to avoid this kind of bug, and also to avoid paying the cost of URL parsing twice, please factor out the URL Parsing from SecurityCheckURL() into OpenInternal() and then pass the nsIURI* argument to SecurityCheckURL() and use the returned nsresult value to decide whether to call MaybeAllowStorageForOpenedWindow().  You can also pass the nsIURI* argument to this function instead of the aUrl string, and use it when calling GetUTFOrigin().

::: toolkit/components/antitracking/test/browser/3rdPartyOpen.html
@@ -5,5 @@
>  <body>
>  <h1>hi!</h1>
>  <script>
>  
> -SpecialPowers.wrap(document).userInteractionForTesting();

Doing this will effectively remove the test that we have for bug 1469993, right?

How about having two tests, one that uses the document that gets window.open()ed directly like this one, and another that window.open()s a document from a random domain (not a tracker) like example.org which loads another document from the tracker domain and then we get user interaction on the tracker domain?  That way we would be testing both cases...

(It's fine if you want to add that test in a follow-up bug, up to you.)
Attachment #8991078 - Flags: review?(ehsan) → review+
(Assignee)

Updated

10 months ago
Blocks: 1474812
(Assignee)

Comment 4

10 months ago
Posted patch window2.patchSplinter Review
Here a test.

Comment 5

10 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acee48580902
Grant storage access to 3rd party, tracking resource on window.open(), r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05247b25d5e
Grant storage access to 3rd party, tracking resource on window.open() - tests, r=me
Backed out 2 changesets (bug 1474651) for lint failure in /builds/worker/checkouts/gecko/toolkit/components/antitracking/test/browser/popup.html:8:126 on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f05247b25d5ec562d3a0d3d829074ee8c7049535&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=187573522
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=187573522&repo=mozilla-inbound&lineNumber=246
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=39906cba4821a17fb776c8b427c81e00d7e17dc7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

[task 2018-07-11T10:44:28.478Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-07-11T10:49:38.752Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/antitracking/test/browser/popup.html:8:126 | Missing semicolon. (semi)
[taskcluster 2018-07-11 10:49:39.414Z] === Task Finished ===
[taskcluster 2018-07-11 10:49:39.414Z] Unsuccessful task run with exit code: 1 completed in 528.88 seconds
Flags: needinfo?(amarchesini)
(Assignee)

Updated

10 months ago
Flags: needinfo?(amarchesini)

Comment 7

10 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a35489d526
Grant storage access to 3rd party, tracking resource on window.open(), r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/72443d5dcf09
Grant storage access to 3rd party, tracking resource on window.open() - tests, r=me
(Assignee)

Updated

10 months ago
Depends on: 1475189

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c4a35489d526
https://hg.mozilla.org/mozilla-central/rev/72443d5dcf09
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

Updated

10 months ago
Depends on: 1475708

Updated

10 months ago
No longer depends on: 1475708
Depends on: 1476324
Depends on: 1480901
Depends on: 1480899
You need to log in before you can comment on or make changes to this bug.