From 325741fb1e760826185b5e899961bdb4e1c49b31 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Tue, 8 Aug 2017 14:12:52 +0100 Subject: [PATCH] Work around Chrome bug causing sidebar to become invisible Work around a Chrome bug [1] that can cause the sidebar to become invisible if: 1. The sidebar app is loaded from a Chrome extension AND 2. The current tab was opened by clicking a link inside the sidebar app in a different tab. When the issue occurs, the sidebar web app loads and runs normally but is just not visible on screen. This happens due to an internal issue in Chrome which can be avoided adding `rel="noopener"` to all "normal" [2] links in the client that open URLs in a new tab/window. Doing so enables Chrome to use a separate process for the Hypothesis client in the new tab in step (2) than the one used for the Hypothesis client in step (1). This change also prevents potential tab-jacking attacks in all browsers that support `rel="noopener"`. Fixes #516 [1] https://bugs.chromium.org/p/chromium/issues/detail?id=753314 [2] ie. Those which do not use JS to handle the link --- src/sidebar/app.js | 4 ++ .../util/disable-opener-for-external-links.js | 31 ++++++++++++++ .../disable-opener-for-external-links-test.js | 41 +++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 src/sidebar/util/disable-opener-for-external-links.js create mode 100644 src/sidebar/util/test/disable-opener-for-external-links-test.js diff --git a/src/sidebar/app.js b/src/sidebar/app.js index cc3f3c2df5a..53cd04ceb9a 100644 --- a/src/sidebar/app.js +++ b/src/sidebar/app.js @@ -1,6 +1,7 @@ 'use strict'; var addAnalytics = require('./ga'); +var disableOpenerForExternalLinks = require('./util/disable-opener-for-external-links'); var getApiUrl = require('./get-api-url'); var serviceConfig = require('./service-config'); require('../shared/polyfills'); @@ -30,6 +31,9 @@ settings.apiUrl = getApiUrl(settings); // _before_ Angular is require'd for the first time. document.body.setAttribute('ng-csp', ''); +// Prevent tab-jacking. +disableOpenerForExternalLinks(document.body); + var angular = require('angular'); // autofill-event relies on the existence of window.angular so diff --git a/src/sidebar/util/disable-opener-for-external-links.js b/src/sidebar/util/disable-opener-for-external-links.js new file mode 100644 index 00000000000..df6124d6551 --- /dev/null +++ b/src/sidebar/util/disable-opener-for-external-links.js @@ -0,0 +1,31 @@ +'use strict'; + +/** + * Prevent windows or tabs opened via links under `root` from accessing their + * opening `Window`. + * + * This makes links with `target="blank"` attributes act as if they also had + * the `rel="noopener"` [1] attribute set. + * + * In addition to preventing tab-jacking [2], this also enables multi-process + * browsers to more easily use a new process for instances of Hypothesis in the + * newly-opened tab and works around a bug in Chrome [3] + * + * [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types#noopener + * [2] https://mathiasbynens.github.io/rel-noopener/ + * [3] https://bugs.chromium.org/p/chromium/issues/detail?id=753314 + * + * @param {Element} root - Root element + */ +function disableOpenerForExternalLinks(root) { + root.addEventListener('click', event => { + if (event.target.tagName === 'A') { + var linkEl = event.target; + if (linkEl.target === '_blank') { + linkEl.rel = 'noopener'; + } + } + }); +} + +module.exports = disableOpenerForExternalLinks; diff --git a/src/sidebar/util/test/disable-opener-for-external-links-test.js b/src/sidebar/util/test/disable-opener-for-external-links-test.js new file mode 100644 index 00000000000..060b322cf47 --- /dev/null +++ b/src/sidebar/util/test/disable-opener-for-external-links-test.js @@ -0,0 +1,41 @@ +'use strict'; + +var disableOpenerForExternalLinks = require('../disable-opener-for-external-links'); + +describe('sidebar.util.disable-opener-for-external-links', () => { + var containerEl; + var linkEl; + + beforeEach(() => { + containerEl = document.createElement('div'); + linkEl = document.createElement('a'); + containerEl.appendChild(linkEl); + document.body.appendChild(containerEl); + }); + + afterEach(() => { + containerEl.remove(); + }); + + function clickLink() { + linkEl.dispatchEvent(new Event('click', { + bubbles: true, + cancelable: true, + })); + } + + it('disables opener for external links', () => { + linkEl.target = '_blank'; + + disableOpenerForExternalLinks(containerEl); + clickLink(); + + assert.equal(linkEl.rel, 'noopener'); + }); + + it('does not disable opener for internal links', () => { + disableOpenerForExternalLinks(containerEl); + clickLink(); + assert.equal(linkEl.rel, ''); + }); +});