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.

Sign up for GitHub

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: insecure iframe messaging #297

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+12 −6
Open

fix: insecure iframe messaging #297

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3255484
Fix insecure iframe messaging
chaitanyapotti Sep 1, 2021
ededac8
use origin parameter instead of uri to account for path, query, hash etc
chaitanyapotti Sep 4, 2021
5b07385
add origin as ctor parameter
chaitanyapotti Sep 6, 2021
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
7 changes: 5 additions & 2 deletions src/transports/PostMessageIframeTransport.ts
View file
Original file line number Diff line number Diff line change
@@ -5,10 +5,12 @@ class PostMessageIframeTransport extends Transport {
public uri: string;
public frame: undefined | null | Window;
public postMessageID: string;
public origin: string;

constructor(uri: string) {
Copy link
Member

Choose a reason for hiding this comment

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

we'll still need the origin to come from the constructor here. We have to leave the origin to be specified by the consumers here, by passing it into the constructor. This is because we can't make an assumption about where they'd like the post message communication to come from.

In a dev case for instance they may want *, in a cross domain case they may want the origin to come from a window they launched that has a separate domain they know. If we lock them in it takes away the choice.

The default to * after speaking with @shanejonas we both feel like the security change warrants a version that makes people explicitly state *, so they know what assumptions they're making about the transport.

The pr looks great, and thanks for the questions and comments and getting this done!

super();
this.uri = uri;
this.origin = new URL(uri).origin;
this.postMessageID = `post-message-transport-${Math.random()}`;
}
public createWindow(uri: string): Promise<Window | null> {
@@ -28,7 +30,8 @@ class PostMessageIframeTransport extends Transport {
});
}
private messageHandler = (ev: MessageEvent) => {
this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
if (ev.origin === this.origin)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (ev.origin === this.origin)
if (ev.orgin === this.origin || ev.origin === "*")

this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
}
public connect(): Promise<any> {
const urlRegex = /^(http|https):\/\/.*$/;
@@ -46,7 +49,7 @@ class PostMessageIframeTransport extends Transport {
const prom = this.transportRequestManager.addRequest(data, null);
const notifications = getNotifications(data);
if (this.frame) {
this.frame.postMessage((data as IJSONRPCData).request, "*");
this.frame.postMessage((data as IJSONRPCData).request, this.origin);
this.transportRequestManager.settlePendingRequest(notifications);
}
return prom;
7 changes: 5 additions & 2 deletions src/transports/PostMessageWindowTransport.ts
View file
Original file line number Diff line number Diff line change
@@ -18,10 +18,12 @@ class PostMessageTransport extends Transport {
public uri: string;
public frame: undefined | null | Window;
public postMessageID: string;
public origin: string;

constructor(uri: string) {
super();
this.uri = uri;
this.origin = new URL(uri).origin;
this.postMessageID = `post-message-transport-${Math.random()}`;
}

@@ -36,7 +38,8 @@ class PostMessageTransport extends Transport {
}

private messageHandler = (ev: MessageEvent) => {
this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
if (ev.origin === this.origin)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (ev.origin === this.origin)
if (ev.origin === this.origin || ev.origin === "*")

this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
}

public connect(): Promise<any> {
@@ -55,7 +58,7 @@ class PostMessageTransport extends Transport {
const prom = this.transportRequestManager.addRequest(data, null);
const notifications = getNotifications(data);
if (this.frame) {
this.frame.postMessage((data as IJSONRPCData).request, this.uri);
this.frame.postMessage((data as IJSONRPCData).request, this.origin);
this.transportRequestManager.settlePendingRequest(notifications);
}
return prom;