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

Work around Chrome bug causing sidebar to become invisible #523

Merged
merged 1 commit into from
Sep 5, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/sidebar/app.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -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
Expand Down
31 changes: 31 additions & 0 deletions src/sidebar/util/disable-opener-for-external-links.js
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice docstring, which is important for a kind of obscure fix like this

*
* @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;
41 changes: 41 additions & 0 deletions src/sidebar/util/test/disable-opener-for-external-links-test.js
Original file line number Diff line number Diff line change
@@ -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, '');
});
});