Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Prevent a webpage document element to be detected as the Extension API object #153

Merged
merged 5 commits into from
Aug 15, 2018

Conversation

rpl
Copy link
Member

@rpl rpl commented Jul 21, 2018

This PR applies some changes to the webpage used by the integration test suite, to reproduce the same issue that is described in this comment on issue #68 and on https://crbug.com/865881

And the additional check browser instanceof window.Node in the polyfill sources, to detect this scenario and prevent the DOM node to be detected as an existing Extension API object (and turn the polyfill into a no-op as when it runs on Firefox where the browser API object is natively available).

…on API object when running in content scripts
The following two DOM elements ensure that the polyfill doesn't detect the
globals defined for these DOM element ids as the Extensions API objects.
-->
<div id="browser"></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another <div id="browser"></div> to demonstrate the issue that I mentioned above.

@mozilla mozilla deleted a comment from Rob--W Aug 2, 2018
@rpl
Copy link
Member Author

rpl commented Aug 2, 2018

@tophf have you deleted Rob comment by mistake?

Follows @Rob--W deleted comment for reference


In src/browser-polyfill.js:

> @@ -6,7 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
-if (typeof browser === "undefined") {
+if (typeof browser === "undefined" || browser instanceof window.Node) {

Change to:

if (typeof browser !== "object" || browser && Object.getPrototypeOf(browser) !== Object.prototype) {`

or (if you prefer shorter code or don't want to rely on Object.getPrototypeOf):

if (typeof browser === "undefined" || browser && browser.__proto__ !== Object.prototype) {

or (if you don't want to rely on Object):

if (typeof browser === "undefined" || browser && browser.__proto__ !== ({}).__proto__) {

Why? Well:

  • Using instanceof window.Node won't rule out tampering. You'll see that in this case, x is a HTMLCollection: data:text/html,<a id=x href>a</a><a id=x href>b</a><script>console.log(x)</script>
  • If someone has happened to define a primitive value as "browser", then we still want to overwrite it. (this is an accidental side-effect of my chosen implementation)

See also https://www.w3.org/TR/html5/browsers.html#named-access-on-the-window-object

@Rob--W Rob--W mentioned this pull request Aug 2, 2018
@tophf
Copy link

tophf commented Aug 2, 2018

@tophf have you deleted Rob comment by mistake?

I don't have any rights in this repo so I couldn't have deleted anyone else's comments directly. I guess github created a "thread" for my comment and Rob commented in that thread. Then I realized my comment is wrong and deleted it, which made github delete the entire "thread".

@rpl rpl requested a review from Rob--W August 15, 2018 11:04
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Do we already have an integration test that verifies that the polyfill does not overwrite the browser global in Firefox (content scripts and extension/background page)?

@rpl
Copy link
Member Author

rpl commented Aug 15, 2018

Do we already have an integration test that verifies that the polyfill does not overwrite the browser global in Firefox (content scripts and extension/background page)?

@Rob--W

We have a unit test (which simulates the behaviors in an extension page/background page):

it("does not override the global browser namespace if it already exists", () => {
const fakeChrome = {
runtime: {lastError: null},
};
const fakeBrowser = {
mycustomns: {mybrowserkey: true},
};
return setupTestDOMWindow(fakeChrome, fakeBrowser).then(window => {
deepEqual(window.browser, fakeBrowser,
"The existing browser has not been wrapped");
});
});

And an integration test which tests the behavior in a content script context on Chrome and Firefox:

test("browser api object in content script", (t) => {
t.ok(browser && browser.runtime, "a global browser API object should be defined");
t.ok(chrome && chrome.runtime, "a global chrome API object should be defined");
if (navigator.userAgent.includes("Firefox/")) {
// Check that the polyfill didn't create a polyfill wrapped browser API object on Firefox.
t.equal(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should be equal on Firefox");
// On Firefox, window is not the global object for content scripts, and so we expect window.browser to not
// be defined.
t.equal(window.browser, undefined, "window.browser is expected to be undefined on Firefox");
} else {
// Check that the polyfill has created a wrapped API namespace as expected.
t.notEqual(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should not be equal");
// On chrome, window is the global object and so the polyfilled browser API should
// be also equal to window.browser.
t.equal(browser, window.browser, "browser and window.browser should be the same object");
}
});

@Rob--W
Copy link
Member

Rob--W commented Aug 15, 2018

And an integration test which tests the behavior in a content script context on Chrome and Firefox:

test("browser api object in content script", (t) => {
t.ok(browser && browser.runtime, "a global browser API object should be defined");
t.ok(chrome && chrome.runtime, "a global chrome API object should be defined");
if (navigator.userAgent.includes("Firefox/")) {
// Check that the polyfill didn't create a polyfill wrapped browser API object on Firefox.
t.equal(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should be equal on Firefox");
// On Firefox, window is not the global object for content scripts, and so we expect window.browser to not
// be defined.
t.equal(window.browser, undefined, "window.browser is expected to be undefined on Firefox");
} else {
// Check that the polyfill has created a wrapped API namespace as expected.
t.notEqual(browser.runtime, chrome.runtime, "browser.runtime and chrome.runtime should not be equal");
// On chrome, window is the global object and so the polyfilled browser API should
// be also equal to window.browser.
t.equal(browser, window.browser, "browser and window.browser should be the same object");
}
});

The way that we inject APIs in Firefox are different for extension pages and content scripts, so a test for the background page seems desired.

browser being identical to chrome in a content script seems to be a small bug to me, because it adds promise APIs to the chrome namespace in content scripts: https://bugzilla.mozilla.org/show_bug.cgi?id=1296896
In extension pages, chrome and browser are already different. We just need to make sure that the polyfill does not replace the browser in this case.

HaNdTriX added a commit to webextension-toolbox/webextension-toolbox that referenced this pull request Aug 15, 2018
HaNdTriX added a commit to webextension-toolbox/webextension-toolbox that referenced this pull request Aug 15, 2018
HaNdTriX added a commit to webextension-toolbox/webextension-toolbox that referenced this pull request Aug 15, 2018
HaNdTriX added a commit to webextension-toolbox/webextension-toolbox that referenced this pull request Aug 15, 2018
…and fix wrong assumption in the test related to the content scripts
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with very minor nits.

// a small latency time.
browser.runtime.onMessage.addListener(async (msg, sender) => {
if (msg !== "test-api-object-in-background-page") {
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return false;? If you don't want the handler to respond, jus return void (return;).
Or make this a non-async function and call sendResponse({ ... }); in the end.

@@ -0,0 +1,2 @@
// Store a copy of the references to the original API objects.
const originalAPIObjects = {browser, chrome}; // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that in browsers without a browser API, this will point to the <div id="browser"> in the page.

return Promise.resolve({
browserIsDefined: !!browser,
chromeIsDefined: !!chrome,
browserIsUnchanged: browser === originalAPIObjects.browser,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check window.browser too?

@rpl rpl force-pushed the fix/detect-if-document-node branch from 552cfa0 to 17536d2 Compare August 15, 2018 20:28
@rpl rpl merged commit ebd2818 into mozilla:master Aug 15, 2018
@rpl rpl changed the title fix: Prevent a webpage document element to be detected as the Extension API object when running in content scripts fix: Prevent a webpage document element to be detected as the Extension API object Aug 15, 2018
balcsida pushed a commit to webextension-toolbox/webextension-toolbox that referenced this pull request Oct 4, 2018
@rpl rpl deleted the fix/detect-if-document-node branch March 15, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants