implement message channel #444
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
I really like this MessageChannel-based solution, it's way better than the global listeners/handlers we had previously! 🚀
If we have channels in place between individual iframeStampers and their iframes we can completely disregard the notion "trusted origin" I believe! The channel is opened by a trusted origin, almost by definition.
The ambiguity which existed before ("is the iframe receiving a message from its parent or from another frame?" and "is the parent receiving from its child iframe or from another frame?") doesn't exist anymore because the MessageChannel is privately owned by the iframe stamper:
- script on the parent page or other frames can't write to port1. This means the iframe knows any messages on port2 are from the right sender: the frame which instantiated it 🔒
- other frames can't write to port2 because it's owned by the iframe. This means the iframe stamper is guaranteed that messages on port1 are from the iframe it instantiated, and nobody else 🔒
| return new Promise((resolve, reject) => { | ||
| this.iframe.contentWindow?.postMessage( | ||
| { | ||
| type: IframeEventType.InjectCredentialBundle, | ||
| value: bundle, | ||
| }, | ||
| "*" | ||
| ); | ||
|
|
||
| window.addEventListener( | ||
| "message", | ||
| (event) => { | ||
| if (event.origin !== this.iframeOrigin) { | ||
| // There might be other things going on in the window, for example: react dev tools, other extensions, etc. | ||
| // Instead of erroring out we simply return. Not our event! | ||
| return; | ||
| } | ||
| if (event.data?.type === IframeEventType.BundleInjected) { | ||
| resolve(event.data["value"]); | ||
| } | ||
| if (event.data?.type === IframeEventType.Error) { | ||
| reject(event.data["value"]); | ||
| } | ||
| }, | ||
| false | ||
| ); | ||
| this.messageChannel.port1.postMessage({ | ||
| type: IframeEventType.InjectCredentialBundle, | ||
| value: bundle, | ||
| }); | ||
| this.messageChannel.port1.onmessage = (event) => { | ||
| this.onMessageHandler(event, resolve, reject); | ||
| }; |
There was a problem hiding this comment.
I don't think you need the wrapping promise here? How about modifying this function to be:
this.messageChannel.port1.postMessage({
type: IframeEventType.InjectCredentialBundle,
value: bundle,
});
return addMessageHandler();(looks like you've done that for the other functions, injectCredentialBundle is the odd one out!)
There was a problem hiding this comment.
Talked through this on Slack: while odd indeed, we'd break the interface if we made this function synchronous, because it means callers would have to remove awaits. So, it'd be a cleaner interface, but we're choosing to keep it the way it is for compatibility reasons. Let's not ask people to change their code, there's no real benefit here.
There was a problem hiding this comment.
Sorry if I'm missing something obvious — what is the difference in interface between the two? (injectCredentialBundle vs injectKeyExportBundle), i.e. why is injectCredentialBundle the odd one out?
There was a problem hiding this comment.
injectCredentialBundle returns a promise which wraps both the postMessage and onmessage logic:
return new Promise((resolve, reject) => {
this.messageChannel.port1.postMessage({
type: IframeEventType.InjectCredentialBundle,
value: bundle,
});
this.messageChannel.port1.onmessage = (event) => {
this.onMessageHandler(event, resolve, reject);
};
});
injectKeyExportBundle calls postMessage and then returns a promise which handles the onmessage logic:
this.messageChannel.port1.postMessage({
type: IframeEventType.InjectKeyExportBundle,
value: bundle,
keyFormat,
organizationId,
});
return this.addMessageHandler();
| @@ -233,35 +265,14 @@ export class IframeStamper { | |||
| organizationId: string, | |||
| keyFormat?: KeyFormat | |||
| ): Promise<boolean> { | |||
There was a problem hiding this comment.
Not sure why TS isn't complaining about this, but I believe there's a type mismatch? The return type is Promise<boolean>, but addMessageHandler returns Promise<any>.
| !this.iframe.contentWindow.postMessage | ||
| ) { | ||
| throw new Error( | ||
| "contentWindow or contentWindow.postMessage does not exist" |
There was a problem hiding this comment.
is there a scenario where we'd want granularity over this? in other words, is it particularly weird if .postMessage specifically doesn't exist?
There was a problem hiding this comment.
So TypeScript complains if you do this.iframe.contentWindow.postMessage(...) because the contentWindow is possibly null. I'm not really aware of a scenario where this would actually be the case in the wild unless there was an underlying bug in the browser itself. contentWindow is read-only so you wouldn't be able to null it out if you tried.
This check is more of a defensive coding pattern in case something is really broken.
There was a problem hiding this comment.
100% agree with being defensive here. was just curious. i'm guessing having this in the wild is a pretty rare occurrence
There was a problem hiding this comment.
Yeah I think the internet would be broken
Summary & Motivation
How I Tested These Changes
Did you add a changeset?
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.