Tracking Protection fails to report blocked requests made in iframes
Categories
(Firefox :: Protections UI, defect, P3)
Tracking
()
People
(Reporter: ke5trel, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
1.60 MB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
francois
:
review+
mayhemer
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
Tracking Protection should work in extension pages just like normal pages, showing an icon in the location bar with the option to disable protection. It is important that it does not break things without feedback or recourse. For example the "Tab for a Cause" extension replaces the New Tab page with a page that shows ads when signed on. When visited directly it allows disabling TP but when shown in the New Tab page there is no warning and no option. https://addons.mozilla.org/en-US/firefox/addon/tab-for-a-cause/ https://tab.gladly.io/newtab/
Comment 1•6 years ago
|
||
Johann, Andrew suggested you might have an opinion on how to prioritize this. It doesn't seem like a major concern to me.
Comment 2•6 years ago
|
||
Yeah, it is a valid bug but not a critical issue IMO. I'm torn between P3 and P5. It would be great if someone could find the time to look into this but I have no idea who that could be.
Comment 3•6 years ago
|
||
This is a symptom of a larger problem: we don't run the URL classifier on about:* pages.
Comment 4•6 years ago
|
||
(In reply to François Marier [:francois] from comment #3) > This is a symptom of a larger problem: we don't run the URL classifier on > about:* pages. I'm not sure what you mean, the WebExtension page isn't an about: page. After spending some time on this I think this is actually a general TP problem with iframes. The Tab for a Cause extension uses a full-window iframe on its new tab page. nsChannelClassifier::SetBlockedContent is the place where we regularly set TP flags, but in the case of the Tab for a Cause extension the function consistently exited early at https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/netwerk/base/nsChannelClassifier.cpp#851. The document URI here is of course moz-extension://... while the channel load principal has the URI of the iframe. This looks like all iframe requests are categorized as "this isn't the same document anymore" and discarded before we can show the UI (though at least we still block them). I did a very cheap test on http://joo.crater.uberspace.de/tp/ and you will notice the same issue (open devtools console): the request to the FB tracker is blocked but there's no explicit console warning about TP and no shield.
Comment 5•6 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #4) > (In reply to François Marier [:francois] from comment #3) > > This is a symptom of a larger problem: we don't run the URL classifier on > > about:* pages. > > I'm not sure what you mean, the WebExtension page isn't an about: page. > After spending some time on this I think this is actually a general TP > problem with iframes. The Tab for a Cause extension uses a full-window > iframe on its new tab page. Sorry, I misinterpreted what the bug was about. Looking at the screenshot I thought it was the difference between our blocking trackers when they're on an HTTP URL and not blocking them when they're served from about:newtab. > I did a very cheap test on http://joo.crater.uberspace.de/tp/ and you will > notice the same issue (open devtools console): the request to the FB tracker > is blocked but there's no explicit console warning about TP and no shield. You're right, this is a bigger problem in the channel classifier.
Comment 6•6 years ago
|
||
I can look into this...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da9b09c9f35ca9b318d9983bd4186491ad06253b
Comment 11•6 years ago
|
||
These patches are a bit... exploratory. I don't actually know that much about channel classifier or tracking protection but I think this should work. It would be great if someone could answer my question marked with XXX in patch 1.
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8969430 [details] Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. https://reviewboard.mozilla.org/r/238190/#review243918 I'll do a detailed review tomorrow, but do you think it would be OK to put these in the 'siteIdentity' directory instead? That is kind of the UI element that this is about... and while I'm glad things are moving out of general/ it'd also be good if we didn't end up with 50 different subdirectories each with only a few tests in...
Updated•6 years ago
|
Comment 13•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #12) > Comment on attachment 8969430 [details] > Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into > their own subdirectory. > > https://reviewboard.mozilla.org/r/238190/#review243918 > > I'll do a detailed review tomorrow, but do you think it would be OK to put > these in the 'siteIdentity' directory instead? That is kind of the UI > element that this is about... and while I'm glad things are moving out of > general/ it'd also be good if we didn't end up with 50 different > subdirectories each with only a few tests in... Hm, it could be in siteIdentity, though I think trackingUI makes for a pretty decent subset of tests. It's 9 tests so I wouldn't call it a few (in addition to the 19 tests of siteIdentity that's a bunch already). So, personally I wouldn't move it to siteIdentity, but I really don't feel strongly about this either.
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8969430 [details] Bug 1451307 - Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. https://reviewboard.mozilla.org/r/238190/#review244142 I don't see anything wrong here, but the trypush is unhappy with the tour tests (and has some osx debug timeouts for the non-tour trackingUI tests which might be intermittent, but would be good to check - there's just about enough that it looks dodgy). Maybe because of the other csets or something? Or maybe because some other test in bbc/t/general makes some changes that causes the tests to pass more reliably there. Or something. Also eslint semicolon issues in browser/base/content/test/trackingUI/trackingPage_onunload.html (in the other cset).
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8969429 [details] Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. https://reviewboard.mozilla.org/r/238188/#review244170 lgtm, thanks
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8969429 [details] Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. https://reviewboard.mozilla.org/r/238188/#review244334 The approach looks good to me. I noted two small things inline. ::: netwerk/base/nsChannelClassifier.cpp:857 (Diff revision 1) > // This event might come after the user has navigated to another page. > // To prevent showing the TrackingProtection UI on the wrong page, we need to > // check that the loading URI for the channel is the same as the URI currently > - // loaded in the document. > - if (!SameLoadingURI(doc, channel)) { > + // loaded in the document that is associated with the channel (which may be > + // different than the top-level document, so we avoid ignoring all iframes). > + nsCOMPtr<nsPIDOMWindowOuter> win = GetWindowForChannel(channel); `GetWindowForChannel()` could return `nullptr` so we should check for null here before we start using that pointer. ::: netwerk/base/nsChannelClassifier.cpp:866 (Diff revision 1) > + // No frame document? This can happen when the frame itself is from > + // a tracker URL. Walk back the parent tree... > + while (!frameDoc && win != topWin) { > + win = win->GetParent(); > + frameDoc = win->GetExtantDoc(); > + } Are we guaranteed to either get a `frameDoc` or to get right up to the `topWin`? Is it possible that this could fail and turn into an infinite loop?
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8969431 [details] Bug 1451307 - Part 3 - Add a test for showing the TP UI for iframes and unloaded documents. https://reviewboard.mozilla.org/r/238192/#review244340 ::: browser/base/content/test/trackingUI/browser_trackingUI_iframe.js:1 (Diff revision 1) > +const URL = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.org") + "trackingPage_iframe.html"; Please add the public domain license header from https://www.mozilla.org/en-US/MPL/headers/ ::: browser/base/content/test/trackingUI/browser_trackingUI_unloaded_documents.js:1 (Diff revision 1) > +const URL = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.org") + "trackingPage_onunload.html"; Again, the public domain header from https://www.mozilla.org/en-US/MPL/headers/. ::: browser/base/content/test/trackingUI/trackingPage_iframe.html:2 (Diff revision 1) > +<!DOCTYPE HTML> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public I don't particularly care, but we normally use the public domain header in tests.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfa4fe80ccf70f3be11e82155fc12251bdec158
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•6 years ago
|
||
Comment on attachment 8969429 [details] Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. Honza, there's a bit that changed in this patch now, would you like to take another quick look? I realized that for iframe document loads we need to always consider the parent document URI (and it doesn't really make sense to walk up the parent chain in that case).
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8969429 [details] Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. https://reviewboard.mozilla.org/r/238188/#review244992 Makes much more sense now! Thanks. ::: netwerk/base/nsChannelClassifier.cpp:869 (Diff revision 3) > + // contains a tracking page, the window associated with the channel belongs > + // to that iframe, not its loading parent. In this case we actually want to > + // compare load URIs with the parent frame, so we step up one level. > + nsLoadFlags loadFlags = 0; > + channel->GetLoadFlags(&loadFlags); > + bool isDocumentLoad = loadFlags & nsIChannel::LOAD_DOCUMENT_URI; should we use https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/netwerk/base/nsIChannel.idl#358 ? ::: netwerk/base/nsChannelClassifier.cpp:884 (Diff revision 3) > + return NS_OK; > + } > + > + // Get the root docshell for updating the security UI. > + nsCOMPtr<nsPIDOMWindowOuter> topWin = win->GetScriptableTop(); > + nsCOMPtr<nsIDocShell> docShell = topWin->GetDocShell(); topDocShell?
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8969429 [details] Bug 1451307 - Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. https://reviewboard.mozilla.org/r/238188/#review245048
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c363f450ca91826b50a32499d80915e1fe7837
Comment 33•6 years ago
|
||
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f89d94d45ba Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. r=francois,mayhemer https://hg.mozilla.org/integration/autoland/rev/13e58d5887d8 Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. r=Gijs https://hg.mozilla.org/integration/autoland/rev/be0a9b78c224 Part 3 - Add a test for showing the TP UI for iframes and unloaded documents. r=francois
Comment 34•6 years ago
|
||
Backed out 3 changesets (bug 1451307) for Mochitest failure on toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html. CLOSED TREE Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175570115&repo=autoland&lineNumber=2760 INFO - 611 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css [task 2018-04-25T19:10:27.183Z] 19:10:27 INFO - Buffered messages finished [task 2018-04-25T19:10:27.184Z] 19:10:27 INFO - 612 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered. [task 2018-04-25T19:10:27.184Z] 19:10:27 INFO - testUpdate/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:161:7 [task 2018-04-25T19:10:29.714Z] 19:10:29 INFO - 613 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript [task 2018-04-25T19:10:29.714Z] 19:10:29 INFO - 614 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css [task 2018-04-25T19:10:29.715Z] 19:10:29 INFO - 615 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css [task 2018-04-25T19:10:29.716Z] 19:10:29 INFO - 616 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css [task 2018-04-25T19:10:29.717Z] 19:10:29 INFO - 617 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered. [task 2018-04-25T19:10:29.717Z] 19:10:29 INFO - testUpdateNotClearCompletions/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:177:7 [task 2018-04-25T19:10:32.253Z] 19:10:32 INFO - 618 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript [task 2018-04-25T19:10:32.253Z] 19:10:32 INFO - 619 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css [task 2018-04-25T19:10:32.253Z] 19:10:32 INFO - 620 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css [task 2018-04-25T19:10:32.254Z] 19:10:32 INFO - 621 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css [task 2018-04-25T19:10:32.254Z] 19:10:32 INFO - 622 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered. [task 2018-04-25T19:10:34.591Z] 19:10:34 INFO - 623 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript [task 2018-04-25T19:10:34.592Z] 19:10:34 INFO - 624 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should load clean css [task 2018-04-25T19:10:34.593Z] 19:10:34 INFO - 625 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad css [task 2018-04-25T19:10:34.593Z] 19:10:34 INFO - 626 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not import bad css [task 2018-04-25T19:10:34.593Z] 19:10:34 INFO - 627 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Gethash request is not triggered. [task 2018-04-25T19:10:34.594Z] 19:10:34 INFO - testUpdateCompletionsAfterReload/<@toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:198:7 [task 2018-04-25T19:10:40.170Z] 19:10:40 INFO - 628 INFO TEST-PASS | toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html | Should not load bad javascript Push with the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be0a9b78c2242fcc4eb2545d7fe5b58fecd250e7 Backout: https://hg.mozilla.org/integration/autoland/rev/76f35d0ecaa690917347f2b0f97180b05e7272bb
Comment 35•6 years ago
|
||
(In reply to Dorel Luca [:dluca] from comment #34) > Backed out 3 changesets (bug 1451307) for Mochitest failure on > toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html. That test already fails intermittently on Android (see bug 1456944): https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1456944
Comment 36•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb399145506f3d8311044bd93a80e342ef57b61d
Comment 37•6 years ago
|
||
Ok, at this point I'm going to file a follow-up bug for the Android test failure. I have spent way too much time on trying to get this to fail locally (and trying to get gdb running with fennec) and I didn't succeed. Note that the failures on automation aren't perma-fails either, it's more like frequent intermittents (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02d92ae0472a4b946a34dbea5ed00c74c3fad32&selectedJob=176130882). I don't see how my changes would make this test fail (only on automation, mind you) except that we might have changed some slight perf characteristics which surfaced in the dead slow debug android automation runs. I'm very skeptical that this has any real-world effects.
Comment 38•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f8a711f4ce2b3bfd43e40e73099a40b9bc102af
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 43•6 years ago
|
||
mozreview-review |
Comment on attachment 8972645 [details] Bug 1451307 - Part 4 - Disable test_bug1254766.html and test_cachemiss.html on Android. https://reviewboard.mozilla.org/r/241198/#review247714
Comment 44•6 years ago
|
||
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9f8135caee4f Part 1 - Consider iframes when comparing loading URIs in nsChannelClassifier::SetBlockedContent. r=francois,mayhemer https://hg.mozilla.org/integration/autoland/rev/7adc755a4229 Part 2 - Move trackingUI tests out of b/b/c/test/general into their own subdirectory. r=Gijs https://hg.mozilla.org/integration/autoland/rev/12ea607734b1 Part 3 - Add a test for showing the TP UI for iframes and unloaded documents. r=francois https://hg.mozilla.org/integration/autoland/rev/9e0246c2f62f Part 4 - Disable test_bug1254766.html on Android. r=francois
Comment 45•6 years ago
|
||
Backed out for failing android at toolkit/components/url-classifier/tests/mochitest/test_cachemiss.html Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9e0246c2f62f9c1e97fa9b194357e01787907213 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=177180482&repo=autoland Backout: https://hg.mozilla.org/integration/autoland/rev/791af1639cdda1681f029efd07215783ddc6aa67
Comment 46•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35886eb580d4934c371d8cd3989b404bb2afd8b4
Comment 47•6 years ago
|
||
The metabug this blocks is not resolved duplicate. Johann, do we need to make this blocking something else?
Updated•6 years ago
|
Comment 48•6 years ago
|
||
I've looked into this for quite some time now and can't figure out the deal with the Android failures on infra. It's really hard to debug Fennec locally, but nigh impossible on try. At some point I resorted to the good old binary search comment-out code method and found that this part of the code seems to cause the failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d89691bb5375e2f4f02238d6f173a61c78a6d53 However, various attempts at narrowing down the cause failed so far. I think this bug should really be fixed, especially with our renewed focus on tracking protection and the TP UI. We're now starting to report trackers by default, even with TP turned off, and that should really include tracking iframes. With more people turning on TP by default (and us potentially turning on some form of TP by default for everyone), it's crucial to give users the ability to unbreak all websites. Hence, I'm going to disable test_cachemiss.html on Android as well and post my findings in bug 1458635.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 53•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab6c8ee6c86ec7e6e149dcf477455b3fb18b978
Comment 54•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8d1b667eb51f2cf7dd24125353b9c7154215b6
Comment 56•5 years ago
|
||
This was fixed somewhere down the road during all the anti-tracking work we did, plus the new protections popup makes this issue somewhat irrelevant because you can always turn off protections for a site.
Reporter | ||
Comment 57•5 years ago
|
||
The Bug 1538048 Comment 0 test case still fails though, the TP icon and panel reports no trackers detected despite blocking vimeo. The user needs to be informed that content is being blocked.
Comment 58•5 years ago
|
||
(In reply to Kestrel from comment #57)
The Bug 1538048 Comment 0 test case still fails though, the TP icon and panel reports no trackers detected despite blocking vimeo. The user needs to be informed that content is being blocked.
On Desktop Nightly this seems to work fine for me, can you describe the issue?
Reporter | ||
Comment 59•5 years ago
|
||
The Vimeo player is not currently blocked by the default Level 1 block list, you need the Level 2 block list for this specific test case (hidden behind browser.contentblocking.customBlockList.preferences.ui.enabled
). Latest Nightly fails to inform that Vimeo is being blocked in the test case.
Comment 60•5 years ago
|
||
Ah, right, for the Level 2 list this is still a bug, which makes this less of a priority I'd argue.
Reporter | ||
Comment 61•5 years ago
|
||
Which blocklist the test case uses shouldn't make any difference, items on the Level 1 blocklist are affected in the same way. Although Level 1 items are less critical, it is important that all blocking is transparent to the user. The Level 2 blocklist has been exposed in the UI going back to FF57, hiding it in FF70 does not downgrade existing users to the Level 1 blocklist.
The "Tab for a Cause" extension is still affected with the Level 1 blocklist. When it is used for the new tab page, TP detects Social Media Trackers and Fingerprinters but the Tracking Content category is empty. Loading the page directly outside of an iframe and Tracking Content has 13 items. If these numbers are used for the TP Report then it may be significantly under-reporting.
Here's a test case in the wild:
- Install uBlock Origin (prevents TP from detecting other trackers).
- Go to https://reddit.com and sign up from the main page (not from https://reddit.com/register which does not use an iframe).
- Skip email step.
- Enter username and password.
- A captcha should appear but is blocked by TP Level 2 blocklist.
Expected:
TP shield icon turns purple indicating content is blocked, clicking on the shield shows google.com is being blocked.
Actual:
TP shield remains gray, clicking it on shows "No trackers known to Nightly were detected on this page". There is no indication a captcha should appear or that anything is being blocked, the Sign Up button simply doesn't respond.
Comment 62•5 years ago
|
||
Silently breaking reddit signup sounds like something that should be more than P5 - Johann?
Comment 63•5 years ago
|
||
(In reply to :Gijs (he/him) from comment #62)
Silently breaking reddit signup sounds like something that should be more than P5 - Johann?
This is with the Level 2 blocklist, a ton of stuff is broken with that and we're not even really supporting that setting anymore. I can see that the issue still exists with L1 though it is definitely less pronounced. So, sure, we can bump this to P3.
Updated•2 years ago
|
Description
•