Trharris/messageproxy #2361
Trharris/messageproxy #2361
Conversation
| } | ||
| }, | ||
| ); | ||
| if (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen) { |
There was a problem hiding this comment.
Both of these new GlobalVars start out with a value of false when the app is initialized. allowMessageProxy is only true if an undocumented function is called. webAuthWindowOpen is only true if authentication.authenticate is called from an app running in a browser, and it is changed back to false as soon as that authentication window is closed.
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| sendMessageResponseToChild(message.id, message.uuid, args, isPartialResponse); | ||
| runtime; |
There was a problem hiding this comment.
This section of code takes the func and args from the message TeamsJS received from its child and sends them in a new post message up to the parent. It also sets up a handler for dealing with the response, passing the response back down to the child (using sendMessageResponseToChild) instead of handling the returned data itself.
| @@ -177,7 +175,7 @@ export function handleThemeChange(theme: string): void { | |||
| HandlersPrivate.themeChangeHandler(theme); | |||
| } | |||
|
|
|||
| if (Communication.childWindow) { | |||
| if (Communication.childWindow && (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen)) { | |||
There was a problem hiding this comment.
This and the load handler below have all been here for a long time, so I decided for now to gate them behind the allowMessageProxy logic instead of outright removing them.
| @@ -248,17 +246,9 @@ async function handleBeforeUnload(): Promise<void> { | |||
|
|
|||
| if (HandlersPrivate.beforeSuspendOrTerminateHandler) { | |||
| await HandlersPrivate.beforeSuspendOrTerminateHandler(); | |||
| if (Communication.childWindow) { | |||
There was a problem hiding this comment.
These sections of code were added specifically for some Sharepoint scenarios that are no longer supported. Because of that I opted just to remove the logic around passing the event to the child entirely. We could alternatively choose to play it safe and gate these behind the allowMessageProxy check instead of we wished.
There was a problem hiding this comment.
Thinking about it more over the weekend, I should probably leave these in (and makes sure they're tested). While SharePoint is the reason they were added, it's possible people are using them for auth windows or other use cases. Will get to this "soon"
| @@ -328,6 +328,7 @@ export namespace authentication { | |||
| } finally { | |||
| Communication.childWindow = null; | |||
| Communication.childOrigin = null; | |||
| GlobalVars.webAuthWindowOpen = false; | |||
There was a problem hiding this comment.
As far as I can tell, every time the auth window is closed by TeamsJS it is done using this function. Because of that, I am using this as a bottleneck to set webAuthWindowOpen back to false.
| @@ -364,6 +366,8 @@ export namespace authentication { | |||
| : Communication.currentWindow.screenY; | |||
| left += Communication.currentWindow.outerWidth / 2 - width / 2; | |||
| top += Communication.currentWindow.outerHeight / 2 - height / 2; | |||
|
|
|||
| GlobalVars.webAuthWindowOpen = true; | |||
There was a problem hiding this comment.
Immediately before opening an auth window we set webAuthWindowOpen to true
| @@ -1,10 +1,8 @@ | |||
| import { | |||
| Communication, | |||
There was a problem hiding this comment.
All of the changes that were removed in this file were only added for Sharepoint scenarios which are no longer supported. As with the above example, we could choose to gate these behind an allowMessageProxy check if we wanted to be safer.
Description
Work In Progress: Feel free to read and ask questions, but there is still more work (and tests) needed before this is ready for checkin.
We'd like to disable message proxying from child windows in as many situations as we can. Message proxying is when an application that is using teamsJS receives a post message from a child window (embedded iframe or new window that the app opened) and "repeats" that message up to the host.
Based on research from PM, we believe that the only legitimate use of this functionality if when the
authenticate.authenticationfunction is called from an app running in a web browser. In those cases, TeamsJS opens a new window for the auth to happen in to avoid the browser popup blocker. In that case, we still want TeamsJS to allow messages from the auth window to be repeated up to the host. In all other cases, we want to prevent message proxy-ing by default, only allowing it for apps that truly need that functionality. I'll go through the PR and leave some comments highlighting interesting bits for reviewers to pay attention to.Main changes in the PR:
Validation
Validation performed:
Unit Tests added:
<Yes/No>
End-to-end tests added:
<Yes/No>
Additional Requirements
Change file added:
<Yes/No>
Related PRs:
Next/remaining steps:
Screenshots: