Closed Bug 1474651 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch window.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8991061 - Flags: review?(ehsan)
Attached patch window.patchSplinter Review
Better approach.
Attachment #8991061 - Attachment is obsolete: true
Attachment #8991061 - Flags: review?(ehsan)
Attachment #8991078 - Flags: review?(ehsan)
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+
Blocks: 1474812
Attached patch window2.patchSplinter Review
Here a test.
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)
Flags: needinfo?(amarchesini)
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
Depends on: 1475189
https://hg.mozilla.org/mozilla-central/rev/c4a35489d526
https://hg.mozilla.org/mozilla-central/rev/72443d5dcf09
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1475708
No longer depends on: 1475708
You need to log in before you can comment on or make changes to this bug.