Closed Bug 1318800 Opened 8 years ago Closed 8 years ago

webrequest regression listening on top level loads in browser tabs

Categories

(WebExtensions :: Request Handling, defect, P1)

Product:
WebExtensions
Component:
Request Handling
Version:
49 Branch
Type:
defect

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: webrequest, triaged)

Attachments

(2 files)

Bug 1318800 fix systemPrincipal checks so top level tab loads work with webrequests,
58 bytes, text/x-review-board-request
kmag
: review+
Details
1318800 aurora version of patch
11.20 KB, patch
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Bug 1273138 added a filter preventing WebRequest from viewing loads from system principals.  This caused a regression preventing WebRequest being able to see top level content loads.  

It's overly aggressive since it uses either triggeringPrincipal or loadingPrincipal.  The top level content loads do not have a loadingPrincipal and should not be filtered.

The current check is:

let {isSystemPrincipal} = Services.scriptSecurityManager;
data.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
                          isSystemPrincipal(loadInfo.loadingPrincipal));

but should be changed to:

data.isSystemPrincipal = loadInfo.loadingPrincipal &&
                         (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
                          isSystemPrincipal(loadInfo.loadingPrincipal));
It's unfortunate that we don't already have tests for this. But given the inscrutability of the webRequest tests, I'm not really surprised.
Whiteboard: webrequest, triaged
Note that in addition to the new test, test_chrome_ext_webrequest_background_events.html fails all over if data.isSystemPrincipal is wrong.

After reading through nsILoadInfo.idl several times, I beleive I have a correct check for system principal, and the new test works with top level browser loads.
Comment on attachment 8813447 [details]
Bug 1318800 fix systemPrincipal checks so top level tab loads work with webrequests,

https://reviewboard.mozilla.org/r/94838/#review95296

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:241
(Diff revision 1)
>  
> +add_task(function* test_webRequest_tabId_browser() {
> +  function background(url) {
> +    let tabId;
> +    browser.test.onMessage.addListener((msg, expected) => {
> +      browser.tabs.remove(tabId).then(() => {

Please use async/await.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:246
(Diff revision 1)
> +      browser.tabs.remove(tabId).then(() => {
> +        browser.test.sendMessage("done");
> +      });
> +    });
> +
> +    browser.tabs.create({url}).then(tab => { tabId = tab.id; });

Please use async/await.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:268
(Diff revision 1)
> +  // expecting origin == undefined
> +  extension.sendMessage("set-expected", {expect});
> +  yield extension.awaitMessage("continue");
> +
> +  // open a tab from a system principal
> +  yield tabExt.startup();

I don't think we should rely on the tabs API using the system principal as the triggering principal. This is probably better done as a browser mochitest... But I guess that can be done in a follow-up.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:274
(Diff revision 1)
> +
> +  yield extension.awaitMessage("done");
> +  tabExt.sendMessage("done");
> +  yield tabExt.awaitMessage("done");
> +  yield tabExt.unload();
> +});

It would be nice to test whether we see requests for sub-frames of system principal documents, or window.open calls from likewise. But, again, follow-up is fine.

::: toolkit/modules/addons/WebRequest.jsm:632
(Diff revision 1)
> +      if (loadInfo.loadingPrincipal) {
> +        data.isSystemPrincipal = isSystemPrincipal(loadInfo.loadingPrincipal);
> +      } else if (loadInfo.principalToInherit) {
> +        data.isSystemPrincipal = isSystemPrincipal(loadInfo.principalToInherit);
> +      } else {
> +        data.isSystemPrincipal = isSystemPrincipal(loadInfo.triggeringPrincipal);
> +      }

isSystemPrincipal(loadInfo.loadingPrincipal ||
                      loadInfo.principalToInherit ||
                      loadInfo.triggeringPrincipal);
Attachment #8813447 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8813447 [details]
Bug 1318800 fix systemPrincipal checks so top level tab loads work with webrequests,

https://reviewboard.mozilla.org/r/94838/#review96110

::: browser/components/extensions/test/browser/browser_ext_webRequest.js:115
(Diff revision 2)
> +  let openedWindow = Services.ww.openWindow(
> +    null, Services.prefs.getCharPref("browser.chromeURL"), "_blank",
> +    "chrome,dialog=no,all", null);
> +
> +  yield TestUtils.topicObserved("browser-delayed-startup-finished",
> +                                subject => subject == openedWindow);

yield BrowserTestUtils.openNewBrowserWindow();
The last push to review fixes a cache issue when running with other tests, but has a problem with browser_ext_webNavigation_urlbar_transitions.js, which is preventing test_newWindow in browser_ext_webRequest.js from completing.  Still looking into why.
Flags: needinfo?(lgreco)
Figured it out, favicon is being loaded during some tests prior to the new test.  I then don't get the request as I was expecting in the test.  It's not important, so I added an ignore capability and am ignoring favicon if I happen to get it (which is the case when running the test solo).
Flags: needinfo?(lgreco)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/09b480229ec2
fix systemPrincipal checks so top level tab loads work with webrequests, r=kmag
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/2176b72e7e2f
fix systemPrincipal checks so top level tab loads work with webrequests, r=kmag
https://hg.mozilla.org/mozilla-central/rev/2176b72e7e2f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Approval Request Comment
[Feature/Bug causing the regression]: webrequest
[User impact if declined]: top level page loads are not observable
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no

Aurora version of patch only differs in modification to browser.ini for the test file.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:
Attachment #8815883 - Flags: approval-mozilla-aurora?
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Created attachment 8815883 [details] [diff] [review]
> 1318800 aurora version of patch

Somehow I missed the following:

> [List of other uplifts needed for the feature/fix]: none
> [Is the change risky?]: no
> [Why is the change risky/not risky?]: it's covered pretty extensively in tests now
> [String changes made/needed]: none
Comment on attachment 8815883 [details] [diff] [review]
1318800 aurora version of patch

webextensions request handling fix for aurora52
Attachment #8815883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Toolkit → WebExtensions
See Also: → 1825881
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: