Commit 974b1de0 authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo

Remove `authority` field from discovery message

Following these two comments:
* https://github.com/hypothesis/client/pull/3929/files#r750399422
* https://github.com/hypothesis/client/pull/3929/files#r750414036

I am proposing in this PR to remove the anti-collision field `authority`
from the message that is send to discover frames. I believe the message
contains enough information to avoid to be confused by other unrelated
`postMessage` in the host frame.
parent 9dd2e994
...@@ -59,7 +59,6 @@ export class PortFinder { ...@@ -59,7 +59,6 @@ export class PortFinder {
const postRequest = () => { const postRequest = () => {
this._hostFrame.postMessage( this._hostFrame.postMessage(
{ {
authority: 'hypothesis',
frame1: this._source, frame1: this._source,
frame2: target, frame2: target,
type: 'request', type: 'request',
...@@ -91,7 +90,6 @@ export class PortFinder { ...@@ -91,7 +90,6 @@ export class PortFinder {
const { data, ports } = /** @type {MessageEvent} */ (event); const { data, ports } = /** @type {MessageEvent} */ (event);
if ( if (
isMessageEqual(data, { isMessageEqual(data, {
authority: 'hypothesis',
frame1: this._source, frame1: this._source,
frame2: target, frame2: target,
type: 'offer', type: 'offer',
......
...@@ -90,28 +90,24 @@ export class PortProvider { ...@@ -90,28 +90,24 @@ export class PortProvider {
this._allowedMessages = [ this._allowedMessages = [
{ {
allowedOrigin: '*', allowedOrigin: '*',
authority: 'hypothesis',
frame1: 'guest', frame1: 'guest',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
}, },
{ {
allowedOrigin: '*', allowedOrigin: '*',
authority: 'hypothesis',
frame1: 'guest', frame1: 'guest',
frame2: 'sidebar', frame2: 'sidebar',
type: 'request', type: 'request',
}, },
{ {
allowedOrigin: this._hypothesisAppsOrigin, allowedOrigin: this._hypothesisAppsOrigin,
authority: 'hypothesis',
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
}, },
{ {
allowedOrigin: this._hypothesisAppsOrigin, allowedOrigin: this._hypothesisAppsOrigin,
authority: 'hypothesis',
frame1: 'notebook', frame1: 'notebook',
frame2: 'sidebar', frame2: 'sidebar',
type: 'request', type: 'request',
...@@ -213,7 +209,7 @@ export class PortProvider { ...@@ -213,7 +209,7 @@ export class PortProvider {
return; return;
} }
const { authority, frame1, frame2 } = match; const { frame1, frame2 } = match;
const channel = /** @type {Channel} */ (`${frame1}-${frame2}`); const channel = /** @type {Channel} */ (`${frame1}-${frame2}`);
let windowChannelMap = this._channels.get(channel); let windowChannelMap = this._channels.get(channel);
...@@ -231,7 +227,7 @@ export class PortProvider { ...@@ -231,7 +227,7 @@ export class PortProvider {
} }
/** @type {Message} */ /** @type {Message} */
const message = { authority, frame1, frame2, type: 'offer' }; const message = { frame1, frame2, type: 'offer' };
const options = { channel, message, origin, source }; const options = { channel, message, origin, source };
// `sidebar-host` channel is an special case, because it is created in the // `sidebar-host` channel is an special case, because it is created in the
......
// Because there are many `postMessages` on the `host` frame, the SOURCE property
// is added to the hypothesis `postMessages` to identify the provenance of the
// message and avoid listening to messages that could have the same properties
// but different source. This is not a security feature but an
// anti-collision mechanism.
const AUTHORITY = 'hypothesis';
/** /**
* These types are the used in by `PortProvider` and `PortFinder` for the * These types are the used in by `PortProvider` and `PortFinder` for the
* inter-frame discovery and communication processes. * inter-frame discovery and communication processes.
...@@ -12,7 +5,6 @@ const AUTHORITY = 'hypothesis'; ...@@ -12,7 +5,6 @@ const AUTHORITY = 'hypothesis';
* @typedef {'guest'|'host'|'notebook'|'sidebar'} Frame * @typedef {'guest'|'host'|'notebook'|'sidebar'} Frame
* *
* @typedef Message * @typedef Message
* @prop {AUTHORITY} authority
* @prop {Frame} frame1 * @prop {Frame} frame1
* @prop {Frame} frame2 * @prop {Frame} frame2
* @prop {'offer'|'request'} type * @prop {'offer'|'request'} type
...@@ -36,7 +28,7 @@ function isMessage(data) { ...@@ -36,7 +28,7 @@ function isMessage(data) {
} }
} }
return data.authority === AUTHORITY; return true;
} }
/** /**
......
...@@ -4,7 +4,6 @@ import { PortFinder } from '../port-finder'; ...@@ -4,7 +4,6 @@ import { PortFinder } from '../port-finder';
const MAX_WAIT_FOR_PORT = 1000 * 5; const MAX_WAIT_FOR_PORT = 1000 * 5;
describe('PortFinder', () => { describe('PortFinder', () => {
const authority = 'hypothesis';
const frame1 = 'guest'; const frame1 = 'guest';
const type = 'offer'; const type = 'offer';
let portFinder; let portFinder;
...@@ -52,7 +51,6 @@ describe('PortFinder', () => { ...@@ -52,7 +51,6 @@ describe('PortFinder', () => {
sendPortProviderOffer({ sendPortProviderOffer({
data: { data: {
authority,
frame1, frame1,
frame2: target, frame2: target,
type, type,
...@@ -100,7 +98,6 @@ describe('PortFinder', () => { ...@@ -100,7 +98,6 @@ describe('PortFinder', () => {
portFinder.discover(target).then(port => (resolvedPort = port)); portFinder.discover(target).then(port => (resolvedPort = port));
sendPortProviderOffer({ sendPortProviderOffer({
data: { data: {
authority,
frame1: source, frame1: source,
frame2: target, frame2: target,
type, type,
...@@ -128,7 +125,7 @@ describe('PortFinder', () => { ...@@ -128,7 +125,7 @@ describe('PortFinder', () => {
assert.callCount(window.postMessage, 21); assert.callCount(window.postMessage, 21);
assert.alwaysCalledWithExactly( assert.alwaysCalledWithExactly(
window.postMessage, window.postMessage,
{ authority, frame1, frame2: target, type: 'request' }, { frame1, frame2: target, type: 'request' },
'*' '*'
); );
......
import { delay } from '../../test-util/wait'; import { delay } from '../../test-util/wait';
import { PortProvider } from '../port-provider'; import { PortProvider } from '../port-provider';
const authority = 'hypothesis';
describe('PortProvider', () => { describe('PortProvider', () => {
let portProvider; let portProvider;
...@@ -38,7 +36,6 @@ describe('PortProvider', () => { ...@@ -38,7 +36,6 @@ describe('PortProvider', () => {
portProvider.destroy(); portProvider.destroy();
await sendPortFinderRequest({ await sendPortFinderRequest({
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -61,7 +58,6 @@ describe('PortProvider', () => { ...@@ -61,7 +58,6 @@ describe('PortProvider', () => {
portProvider.destroy(); portProvider.destroy();
portProvider = new PortProvider(window.location.origin); portProvider = new PortProvider(window.location.origin);
const data = { const data = {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -83,22 +79,9 @@ describe('PortProvider', () => { ...@@ -83,22 +79,9 @@ describe('PortProvider', () => {
describe('listens for port requests', () => { describe('listens for port requests', () => {
[ [
// Disabled this check because it make axes-core to crash
// Reported: https://github.com/dequelabs/axe-core/pull/3249
//{ data: null, reason: 'if message is null' },
{
data: {
authority: 'dummy', // invalid authority
frame1: 'sidebar',
frame2: 'host',
type: 'request',
},
reason: 'contains an invalid authority',
},
{ {
data: { data: {
authority, frame1: 'dummy', // invalid source
frame1: 'host', // invalid source
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
}, },
...@@ -106,7 +89,6 @@ describe('PortProvider', () => { ...@@ -106,7 +89,6 @@ describe('PortProvider', () => {
}, },
{ {
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'dummy', // invalid target frame2: 'dummy', // invalid target
type: 'request', type: 'request',
...@@ -115,16 +97,14 @@ describe('PortProvider', () => { ...@@ -115,16 +97,14 @@ describe('PortProvider', () => {
}, },
{ {
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'offer', // invalid type type: 'dummy', // invalid type
}, },
reason: 'contains an invalid type', reason: 'contains an invalid type',
}, },
{ {
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -134,7 +114,6 @@ describe('PortProvider', () => { ...@@ -134,7 +114,6 @@ describe('PortProvider', () => {
}, },
{ {
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -158,7 +137,6 @@ describe('PortProvider', () => { ...@@ -158,7 +137,6 @@ describe('PortProvider', () => {
it('responds to a valid port request', async () => { it('responds to a valid port request', async () => {
portProvider.listen(); portProvider.listen();
const data = { const data = {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -178,7 +156,6 @@ describe('PortProvider', () => { ...@@ -178,7 +156,6 @@ describe('PortProvider', () => {
it('responds to the first valid port request but ignores additional requests', async () => { it('responds to the first valid port request but ignores additional requests', async () => {
portProvider.listen(); portProvider.listen();
const data = { const data = {
authority,
frame1: 'guest', frame1: 'guest',
frame2: 'sidebar', frame2: 'sidebar',
type: 'request', type: 'request',
...@@ -202,7 +179,6 @@ describe('PortProvider', () => { ...@@ -202,7 +179,6 @@ describe('PortProvider', () => {
portProvider.listen(); portProvider.listen();
await sendPortFinderRequest({ await sendPortFinderRequest({
data: { data: {
authority,
frame1: 'sidebar', frame1: 'sidebar',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
...@@ -214,7 +190,6 @@ describe('PortProvider', () => { ...@@ -214,7 +190,6 @@ describe('PortProvider', () => {
sidebarPort.onmessage = handler; sidebarPort.onmessage = handler;
const data = { const data = {
authority,
frame1: 'guest', frame1: 'guest',
frame2: 'sidebar', frame2: 'sidebar',
type: 'request', type: 'request',
...@@ -236,7 +211,6 @@ describe('PortProvider', () => { ...@@ -236,7 +211,6 @@ describe('PortProvider', () => {
const handler = sinon.stub(); const handler = sinon.stub();
portProvider.on('hostPortRequest', handler); portProvider.on('hostPortRequest', handler);
const data = { const data = {
authority,
frame1: 'guest', frame1: 'guest',
frame2: 'host', frame2: 'host',
type: 'request', type: 'request',
......
...@@ -2,7 +2,6 @@ import { isMessageEqual, isSourceWindow } from '../port-util'; ...@@ -2,7 +2,6 @@ import { isMessageEqual, isSourceWindow } from '../port-util';
describe('port-util', () => { describe('port-util', () => {
describe('isMessageEqual', () => { describe('isMessageEqual', () => {
const authority = 'hypothesis';
const frame1 = 'guest'; const frame1 = 'guest';
const frame2 = 'sidebar'; const frame2 = 'sidebar';
const type = 'offer'; const type = 'offer';
...@@ -10,7 +9,6 @@ describe('port-util', () => { ...@@ -10,7 +9,6 @@ describe('port-util', () => {
[ [
{ {
data: { data: {
authority,
frame1, frame1,
frame2, frame2,
type, type,
...@@ -20,7 +18,6 @@ describe('port-util', () => { ...@@ -20,7 +18,6 @@ describe('port-util', () => {
}, },
{ {
data: { data: {
authority,
frame1, frame1,
frame2, frame2,
type, type,
...@@ -33,9 +30,13 @@ describe('port-util', () => { ...@@ -33,9 +30,13 @@ describe('port-util', () => {
expectedResult: false, expectedResult: false,
reason: 'data is null', reason: 'data is null',
}, },
{
data: 'dummy',
expectedResult: false,
reason: 'data is string',
},
{ {
data: { data: {
authority,
// frame1 property missing // frame1 property missing
frame2, frame2,
type, type,
...@@ -45,7 +46,6 @@ describe('port-util', () => { ...@@ -45,7 +46,6 @@ describe('port-util', () => {
}, },
{ {
data: { data: {
authority,
frame1, frame1,
frame2: 9, // wrong type frame2: 9, // wrong type
type, type,
...@@ -55,7 +55,6 @@ describe('port-util', () => { ...@@ -55,7 +55,6 @@ describe('port-util', () => {
}, },
{ {
data: { data: {
authority,
extra: 'dummy', // additional extra: 'dummy', // additional
frame1, frame1,
frame2, frame2,
...@@ -66,7 +65,6 @@ describe('port-util', () => { ...@@ -66,7 +65,6 @@ describe('port-util', () => {
}, },
{ {
data: { data: {
authority,
frame1: 'dummy', // different frame1: 'dummy', // different
frame2, frame2,
type, type,
...@@ -76,7 +74,6 @@ describe('port-util', () => { ...@@ -76,7 +74,6 @@ describe('port-util', () => {
}, },
{ {
data: { data: {
authority,
frame1, frame1,
frame2, frame2,
type, type,
...@@ -88,7 +85,6 @@ describe('port-util', () => { ...@@ -88,7 +85,6 @@ describe('port-util', () => {
].forEach(({ data, expectedResult, reason }) => { ].forEach(({ data, expectedResult, reason }) => {
it(`returns '${expectedResult}' because the ${reason}`, () => { it(`returns '${expectedResult}' because the ${reason}`, () => {
const result = isMessageEqual(data, { const result = isMessageEqual(data, {
authority,
frame1, frame1,
frame2, frame2,
type, type,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment