Closed Bug 1474651 Opened 3 years ago Closed 3 years ago

Grant storage access to 3rd party, tracking resource on


(Firefox :: Security, enhancement)

Not set



Firefox 63
Tracking Status
firefox63 --- fixed


(Reporter: baku, Assigned: baku)


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



(2 files, 1 obsolete file)

When 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]

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: <>

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 directly like this one, and another that a document from a random domain (not a tracker) like 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
Grant storage access to 3rd party, tracking resource on, r=ehsan
Grant storage access to 3rd party, tracking resource on - 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:

[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
Grant storage access to 3rd party, tracking resource on, r=ehsan
Grant storage access to 3rd party, tracking resource on - tests, r=me
Depends on: 1475189
Closed: 3 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.