Open Bug 1451307 Opened 6 years ago Updated 2 years ago

Tracking Protection fails to report blocked requests made in iframes

Categories

(Firefox :: Protections UI, defect, P3)

defect

Tracking

()

REOPENED

People

(Reporter: ke5trel, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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/
Johann, Andrew suggested you might have an opinion on how to prioritize this. It doesn't seem like a major concern to me.
Component: WebExtensions: General → WebExtensions: Frontend
Flags: needinfo?(jhofmann)
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.
Flags: needinfo?(jhofmann)
Priority: -- → P3
This is a symptom of a larger problem: we don't run the URL classifier on about:* pages.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
(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.
Status: RESOLVED → REOPENED
Component: WebExtensions: Frontend → Tracking Protection
Product: Toolkit → Firefox
Resolution: DUPLICATE → ---
Summary: Tracking Protection silently breaks extension pages without showing shield icon with doorhanger to disable → Tracking Protection UI is not shown when all blocked requests are made in iframes
(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.
Priority: P3 → P2
I can look into this...
Assignee: nobody → jhofmann
Status: REOPENED → ASSIGNED
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 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...
Attachment #8969429 - Flags: review?(valentin.gosu) → review?(honzab.moz)
(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 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).
Attachment #8969430 - Flags: review?(gijskruitbosch+bugs) → 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
Attachment #8969429 - Flags: review?(honzab.moz) → 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?
Attachment #8969429 - Flags: review?(francois) → 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.
Attachment #8969431 - Flags: review?(francois) → review+
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).
Attachment #8969429 - Flags: review?(honzab.moz)
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?
Attachment #8969429 - Flags: review?(honzab.moz) → 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
Attachment #8969429 - Flags: review?(francois) → review+
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
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
Flags: needinfo?(jhofmann)
(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
See Also: → 1404610
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.
Flags: needinfo?(jhofmann)
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
Attachment #8972645 - Flags: review?(francois) → review+
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
The metabug this blocks is not resolved duplicate.  Johann, do we need to make this blocking something else?
Blocks: privacy-ui
No longer blocks: 1461530
Flags: needinfo?(jhofmann)
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.
See Also: → 1518049

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.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → WORKSFORME

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.

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Tracking Protection UI is not shown when all blocked requests are made in iframes → Tracking Protection fails to report blocked requests made in iframes

(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?

Assignee: jhofmann → nobody
Flags: needinfo?(ke5trel)

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.

Flags: needinfo?(ke5trel)

Ah, right, for the Level 2 list this is still a bug, which makes this less of a priority I'd argue.

Priority: P2 → P5

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:

  1. Install uBlock Origin (prevents TP from detecting other trackers).
  2. Go to https://reddit.com and sign up from the main page (not from https://reddit.com/register which does not use an iframe).
  3. Skip email step.
  4. Enter username and password.
  5. 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.

Silently breaking reddit signup sounds like something that should be more than P5 - Johann?

Flags: needinfo?(jhofmann)

(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.

Flags: needinfo?(jhofmann)
Priority: P5 → P3
See Also: → 1590696
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.