fix: insecure iframe messaging #297

chaitanyapotti · 2021-09-01T12:11:29Z

Fixes #296

zcstarr · 2021-09-03T04:27:57Z

src/transports/PostMessageIframeTransport.ts

@@ -46,7 +47,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.uri);


Suggested change

this.frame.postMessage((data as IJSONRPCData).request, this.uri);

this.frame.postMessage((data as IJSONRPCData).request, this.origin);

the origin can easily be derived as new URL(this.uri).origin
We don't need another parameter from ctor

zcstarr · 2021-09-03T04:28:15Z

src/transports/PostMessageIframeTransport.ts

@@ -28,7 +28,8 @@ class PostMessageIframeTransport extends Transport {
    });
  }
  private messageHandler = (ev: MessageEvent) => {
-    this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
+    if (ev.origin === this.uri)


Suggested change

if (ev.origin === this.uri)

if (ev.origin === this.origin)

zcstarr · 2021-09-03T04:28:54Z

src/transports/PostMessageWindowTransport.ts

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

  private messageHandler = (ev: MessageEvent) => {
-    this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
+    if (ev.origin === this.uri)


Suggested change

if (ev.origin === this.uri)

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

zcstarr

These changes look good, there is an issue, I added some suggestions for adding origin
so the constructor should be

  constructor(uri: string, origin: string) {
  this.orgin = origin
  ...

We're debating a little if this should be a breaking change, because it's probably better to take this away, vs let it persist. You can still do star this way, but you have to set it explicitly.

This was awesome patch it up and I'll be excited to merge this in, nice find!

zcstarr

accidental approval in this state just requesting changes that were listed before

chaitanyapotti · 2021-09-04T09:14:29Z

Pls check my changes.
We should derive origin from uri instead of relying on user input.

zcstarr · 2021-09-04T15:08:17Z

Pls check my changes.
We should derive origin from uri instead of relying on user input.

The suggestion for a parameter is so users can define what urls they want to accept request from. Locking people into request coming from the same url origin, makes the api not flexible enough.

We can't universally make that assumption about the context in which client-js is operating in.

zcstarr · 2021-09-04T15:21:02Z

src/transports/PostMessageIframeTransport.ts

@@ -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) {


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!

chaitanyapotti · 2021-09-06T09:11:21Z

I've made the necessary changes

zcstarr · 2021-09-07T17:46:35Z

src/transports/PostMessageIframeTransport.ts

@@ -28,7 +30,8 @@ class PostMessageIframeTransport extends Transport {
    });
  }
  private messageHandler = (ev: MessageEvent) => {
-    this.transportRequestManager.resolveResponse(JSON.stringify(ev.data));
+    if (ev.origin === this.origin)


Suggested change

if (ev.origin === this.origin)

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

zcstarr · 2021-09-07T17:55:27Z

src/transports/PostMessageIframeTransport.ts

    super();
    this.uri = uri;
+    this.origin = origin || new URL(uri).origin;


This is awesome thanks for doing this. The only thing here is that we're going to opt for a breaking change here.

We're going to rm the default, because defaulting to origin can be a hidden breaking change to people using this in prod.

We'll do a maj. verson bump which will force people to explicitly update their clients to set what domain they'd like to use this against, when using postmessage transport.

Once this lands the team will bump the versions of generator and any other ecosystem consumers.

Suggested change

this.origin = origin || new URL(uri).origin;

this.origin = origin;

Commit message for these changes should be in the following format for semantic release

fix: patch security hole for post message iframe transport. this change forces consumers to
explicitly set the transport's acceptable origins. Improving the security using the transport.

BREAKING CHANGE: transport now takes second argument and requires origin be set for filtering
post message request.

zcstarr · 2021-09-07T17:55:44Z

src/transports/PostMessageIframeTransport.ts


-  constructor(uri: string) {
+  constructor(uri: string, origin?: string) {


Suggested change

constructor(uri: string, origin?: string) {

constructor(uri: string, origin: string) {

zcstarr · 2021-09-07T17:59:43Z

src/transports/PostMessageWindowTransport.ts

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

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


Suggested change

if (ev.origin === this.origin)

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

BelfordZ · 2023-01-05T19:44:19Z

@chaitanyapotti Thank you for the PR and issue! nice work!

Are you able to finish this one off? Happy to help get it through.

BelfordZ · 2023-01-05T19:45:10Z

@zcstarr perhaps you have some time to cherrypick his commits and make the required changes?

Fix insecure iframe messaging

3255484

chaitanyapotti changed the title ~~Fix insecure iframe messaging~~ fix: insecure iframe messaging Sep 1, 2021

zcstarr reviewed Sep 3, 2021

View reviewed changes

zcstarr approved these changes Sep 3, 2021

View reviewed changes

zcstarr requested changes Sep 3, 2021

View reviewed changes

use origin parameter instead of uri to account for path, query, hash etc

ededac8

zcstarr reviewed Sep 4, 2021

View reviewed changes

add origin as ctor parameter

5b07385

zcstarr reviewed Sep 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: insecure iframe messaging #297

fix: insecure iframe messaging #297

chaitanyapotti commented Sep 1, 2021

zcstarr Sep 3, 2021

chaitanyapotti Sep 4, 2021

zcstarr Sep 3, 2021

zcstarr Sep 3, 2021 •

edited

Loading

zcstarr left a comment

zcstarr left a comment

chaitanyapotti commented Sep 4, 2021

zcstarr commented Sep 4, 2021 •

edited

Loading

zcstarr Sep 4, 2021

chaitanyapotti commented Sep 6, 2021

zcstarr Sep 7, 2021

zcstarr Sep 7, 2021

zcstarr Sep 7, 2021

zcstarr Sep 7, 2021

zcstarr Sep 7, 2021

zcstarr Sep 7, 2021

BelfordZ commented Jan 5, 2023

BelfordZ commented Jan 5, 2023

	this.frame.postMessage((data as IJSONRPCData).request, this.uri);
	this.frame.postMessage((data as IJSONRPCData).request, this.origin);

	if (ev.origin === this.uri)
	if (ev.origin === this.origin \|\| ev.orgin === "*")

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

	this.origin = origin \|\| new URL(uri).origin;
	this.origin = origin;


		constructor(uri: string) {
		constructor(uri: string, origin?: string) {

	constructor(uri: string, origin?: string) {
	constructor(uri: string, origin: string) {

	if (ev.origin === this.origin)
	if (ev.origin === this.origin \|\| ev.origin === "*")

fix: insecure iframe messaging #297

Are you sure you want to change the base?

fix: insecure iframe messaging #297

Conversation

chaitanyapotti commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcstarr Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

zcstarr left a comment

Choose a reason for hiding this comment

zcstarr left a comment

Choose a reason for hiding this comment

chaitanyapotti commented Sep 4, 2021

zcstarr commented Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

chaitanyapotti commented Sep 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BelfordZ commented Jan 5, 2023

BelfordZ commented Jan 5, 2023

zcstarr Sep 3, 2021 •

edited

Loading

zcstarr commented Sep 4, 2021 •

edited

Loading