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

Jump to bottom

Trharris/messageproxy #2361

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Trharris/messageproxy #2361

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d4a912d
Initial scaffolding for disabling message proxying
TrevorJoelHarris Jun 6, 2024
b23b194
Remove event proxying entirely
TrevorJoelHarris Jun 13, 2024
b332327
Remove more event proxying
TrevorJoelHarris Jun 13, 2024
fd64430
Disable more event proxying in the default case
TrevorJoelHarris Jun 14, 2024
a056a8c
Allow message proxying while the auth window is open in the web
TrevorJoelHarris Jun 14, 2024
eea0a6a
Merge branch 'main' into trharris/messageproxy
TrevorJoelHarris Jun 14, 2024
c96bd89
Unit test validating the auth window variable is set correctly
TrevorJoelHarris Jun 14, 2024
4515444
Adding unit tests for authentication window proxying
TrevorJoelHarris Jun 15, 2024
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
39 changes: 24 additions & 15 deletions packages/teams-js/src/internal/communication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { ApiName, ApiVersionNumber, getApiVersionTag } from '../internal/telemetry';
import { FrameContexts } from '../public/constants';
import { SdkError } from '../public/interfaces';
import { latestRuntimeApiVersion } from '../public/runtime';
import { latestRuntimeApiVersion, runtime } from '../public/runtime';
import { version } from '../public/version';
import { GlobalVars } from './globalVars';
import { callHandler } from './handlers';
Expand Down Expand Up @@ -752,11 +752,15 @@ function isPartialResponse(evt: DOMMessageEvent): boolean {
return evt.data.isPartialResponse === true;
}

const handleChildMessageLogger = communicationLogger.extend('handleChildMessage');

/**
* @internal
* Limited to Microsoft-internal use
*/
function handleChildMessage(evt: DOMMessageEvent): void {
const logger = handleChildMessageLogger;

if ('id' in evt.data && 'func' in evt.data) {
// Try to delegate the request to the proper handler, if defined
const message = deserializeMessageRequest(evt.data as SerializedMessageRequest);
Expand All @@ -766,20 +770,25 @@ function handleChildMessage(evt: DOMMessageEvent): void {
// @ts-ignore
sendMessageResponseToChild(message.id, message.uuid, Array.isArray(result) ? result : [result]);
} else {
// No handler, proxy to parent
sendMessageToParent(
getApiVersionTag(ApiVersionNumber.V_2, ApiName.Tasks_StartTask),
message.func,
message.args,
(...args: any[]): void => {
if (Communication.childWindow) {
const isPartialResponse = args.pop();
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
sendMessageResponseToChild(message.id, message.uuid, args, isPartialResponse);
}
},
);
if (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// No handler, proxy to parent
sendMessageToParent(
getApiVersionTag(ApiVersionNumber.V_2, ApiName.Tasks_StartTask),
message.func,
message.args,
(...args: any[]): void => {
if (Communication.childWindow) {
const isPartialResponse = args.pop();
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
sendMessageResponseToChild(message.id, message.uuid, args, isPartialResponse);
runtime;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
},
);
} else {
logger('Message from child not allowed to proxy: %o', message);
}
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/teams-js/src/internal/globalVars.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ export class GlobalVars {
public static hostClientType: string | undefined = undefined;
public static clientSupportedSDKVersion: string;
public static printCapabilityEnabled = false;
public static allowMessageProxy = false;
public static webAuthWindowOpen = false;
}
22 changes: 6 additions & 16 deletions packages/teams-js/src/internal/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { LoadContext, ResumeContext } from '../public/interfaces';
import { pages } from '../public/pages';
import { runtime } from '../public/runtime';
import { Communication, sendMessageEventToChild, sendMessageToParent } from './communication';
import { GlobalVars } from './globalVars';
import { ensureInitialized } from './internalAPIs';
import { getLogger } from './telemetry';
import { isNullOrUndefined } from './typeCheckUtilities';
Expand Down Expand Up @@ -86,9 +87,6 @@ export function callHandler(name: string, args?: unknown[]): [true, unknown] | [
callHandlerLogger('Invoking the registered handler for message %s with arguments %o', name, args);
const result = handler.apply(this, args);
return [true, result];
} else if (Communication.childWindow) {
sendMessageEventToChild(name, args);
return [false, undefined];
} else {
callHandlerLogger('Handler for action message %s not found.', name);
return [false, undefined];
Expand Down Expand Up @@ -177,7 +175,7 @@ export function handleThemeChange(theme: string): void {
HandlersPrivate.themeChangeHandler(theme);
}

if (Communication.childWindow) {
if (Communication.childWindow && (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

sendMessageEventToChild('themeChange', [theme]);
}
}
Expand All @@ -201,12 +199,12 @@ function handleLoad(loadContext: LoadContext): void {
const resumeContext = convertToResumeContext(loadContext);
if (HandlersPrivate.resumeHandler) {
HandlersPrivate.resumeHandler(resumeContext);
if (Communication.childWindow) {
if (Communication.childWindow && (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen)) {
sendMessageEventToChild('load', [resumeContext]);
}
} else if (HandlersPrivate.loadHandler) {
HandlersPrivate.loadHandler(loadContext);
if (Communication.childWindow) {
if (Communication.childWindow && (GlobalVars.allowMessageProxy || GlobalVars.webAuthWindowOpen)) {
sendMessageEventToChild('load', [loadContext]);
}
}
Expand Down Expand Up @@ -248,17 +246,9 @@ async function handleBeforeUnload(): Promise<void> {

if (HandlersPrivate.beforeSuspendOrTerminateHandler) {
await HandlersPrivate.beforeSuspendOrTerminateHandler();
if (Communication.childWindow) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

sendMessageEventToChild('beforeUnload');
} else {
readyToUnload();
}
readyToUnload();
} else if (!HandlersPrivate.beforeUnloadHandler || !HandlersPrivate.beforeUnloadHandler(readyToUnload)) {
if (Communication.childWindow) {
sendMessageEventToChild('beforeUnload');
} else {
readyToUnload();
}
readyToUnload();
}
}

Expand Down
10 changes: 10 additions & 0 deletions packages/teams-js/src/public/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,16 @@ export namespace app {
return openLinkHelper(getApiVersionTag(appTelemetryVersionNumber, ApiName.App_OpenLink), deepLink);
}

/**
* @hidden
* Undocumented function used to re-enable message proxy-ing for applications that request one.
*
* By default, message proxy-ing is disabled for all applications.
*/
export function setAllowMessageProxy(value: boolean = false): void {
GlobalVars.allowMessageProxy = value;
}

/**
* A namespace for enabling the suspension or delayed termination of an app when the user navigates away.
* When an app registers for the registerBeforeSuspendOrTerminateHandler, it chooses to delay termination.
Expand Down
4 changes: 4 additions & 0 deletions packages/teams-js/src/public/authentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ export namespace authentication {
} finally {
Communication.childWindow = null;
Communication.childOrigin = null;
GlobalVars.webAuthWindowOpen = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

Expand All @@ -342,6 +343,7 @@ export namespace authentication {
function openAuthenticationWindow(authenticateParameters: AuthenticateParameters): void {
// Close the previously opened window if we have one
closeAuthenticationWindow();

// Start with a sensible default size
let width = authenticateParameters.width || 600;
let height = authenticateParameters.height || 400;
Expand All @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Immediately before opening an auth window we set webAuthWindowOpen to true

// Open a child window with a desired set of standard browser features
Communication.childWindow = Communication.currentWindow.open(
fullyQualifiedURL.href,
Expand Down
13 changes: 1 addition & 12 deletions packages/teams-js/src/public/pages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import {
Communication,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

sendAndHandleSdkError,
sendAndHandleStatusAndReason,
sendAndHandleStatusAndReasonWithDefaultError,
sendAndUnwrap,
sendMessageEventToChild,
sendMessageToParent,
} from '../internal/communication';
import { registerHandler, registerHandlerHelper } from '../internal/handlers';
Expand Down Expand Up @@ -600,8 +598,6 @@ export namespace pages {
const saveEventType = new SaveEventImpl(result);
if (saveHandler) {
saveHandler(saveEventType);
} else if (Communication.childWindow) {
sendMessageEventToChild('settings.save', [result]);
} else {
// If no handler is registered, we assume success.
saveEventType.notifySuccess();
Expand Down Expand Up @@ -710,8 +706,6 @@ export namespace pages {
const removeEventType = new RemoveEventImpl();
if (removeHandler) {
removeHandler(removeEventType);
} else if (Communication.childWindow) {
sendMessageEventToChild('settings.remove', []);
} else {
// If no handler is registered, we assume success.
removeEventType.notifySuccess();
Expand Down Expand Up @@ -843,12 +837,7 @@ export namespace pages {

function handleBackButtonPress(): void {
if (!backButtonPressHandler || !backButtonPressHandler()) {
if (Communication.childWindow) {
// If the current window did not handle it let the child window
sendMessageEventToChild('backButtonPress', []);
} else {
navigateBack();
}
navigateBack();
}
}

Expand Down
94 changes: 68 additions & 26 deletions packages/teams-js/test/private/privateAPIs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { GlobalVars } from '../../src/internal/globalVars';
import { MessageRequest, MessageResponse } from '../../src/internal/messageObjects';
import { UserSettingTypes, ViewerActionTypes } from '../../src/private/interfaces';
import {
Expand Down Expand Up @@ -385,9 +386,10 @@ describe('AppSDK-privateAPIs', () => {
});

it('should treat messages to frameless windows as coming from the child', async () => {
GlobalVars.allowMessageProxy = true;
utils.initializeAsFrameless(['https://www.example.com']);

// Simulate recieving a child message as a frameless window
// Simulate receiving a child message as a frameless window
await utils.processMessage({
origin: 'https://www.example.com',
source: utils.childWindow,
Expand All @@ -400,43 +402,48 @@ describe('AppSDK-privateAPIs', () => {

// The frameless window should send a response back to the child window
expect(utils.childMessages.length).toBe(1);
GlobalVars.allowMessageProxy = false;
});

it('should properly pass partial responses to nested child frames ', async () => {
it('post messages to frameless windows should be ignored if message proxy-ing is disabled', async () => {
utils.initializeAsFrameless(['https://www.example.com']);

// Simulate recieving a child message as a frameless window
// Simulate receiving a child message as a frameless window
await utils.processMessage({
origin: 'https://www.example.com',
source: utils.childWindow,
data: {
id: 100,
func: 'testPartialFunc1',
args: ['testArgs'],
id: 0,
func: 'themeChange',
args: ['testTheme'],
} as MessageResponse,
} as MessageEvent);

// Send a partial response back
const parentMessage = utils.findMessageByFunc('testPartialFunc1');
await utils.respondToNativeMessage(parentMessage, true, {});

// The child window should properly receive the partial response plus
// the original event
expect(utils.childMessages.length).toBe(2);
const secondChildMessage = utils.childMessages[1];
expect(utils.childMessages[0].func).toBe('testPartialFunc1');
expect(secondChildMessage.isPartialResponse).toBeTruthy();
// The message should have been ignored because message proxy-ing is disabled
expect(utils.childMessages.length).toBe(0);
});

// Pass the final response (non partial)
await utils.respondToNativeMessage(parentMessage, false, {});
it('Proxy messages from child window to parent if message proxy-ing is allowed', async () => {
app.setAllowMessageProxy(true);
await utils.initializeWithContext('content', null, ['https://teams.microsoft.com']);
await utils.processMessage({
origin: 'https://outlook.office.com',
source: utils.childWindow,
data: {
id: 100,
func: 'backButtonClick',
args: [],
} as MessageResponse,
} as MessageEvent);

// The child window should properly receive the non-partial response
expect(utils.childMessages.length).toBe(3);
const thirdChildMessage = utils.childMessages[2];
expect(thirdChildMessage.isPartialResponse).toBeFalsy();
const message = utils.findMessageByFunc('backButtonClick');
expect(message).not.toBeNull();
expect(utils.messages.length).toBe(2);
expect(message).not.toBeNull();
app.setAllowMessageProxy(false);
});

it('Proxy messages to child window', async () => {
it('Do not proxy messages from child window to parent by default', async () => {
await utils.initializeWithContext('content', null, ['https://teams.microsoft.com']);
await utils.processMessage({
origin: 'https://outlook.office.com',
Expand All @@ -449,12 +456,47 @@ describe('AppSDK-privateAPIs', () => {
} as MessageEvent);

const message = utils.findMessageByFunc('backButtonClick');
expect(message).not.toBeNull();
expect(message).toBeNull();
expect(utils.messages.length).toBe(1);
});

it('should properly pass partial responses to nested child frames if message proxy-ing is allowed', async () => {
app.setAllowMessageProxy(true);
utils.initializeAsFrameless(['https://www.example.com']);

// Simulate receiving a child message as a frameless window
await utils.processMessage({
origin: 'https://www.example.com',
source: utils.childWindow,
data: {
id: 100,
func: 'testPartialFunc1',
args: ['testArgs'],
} as MessageResponse,
} as MessageEvent);

// Send a partial response back
const parentMessage = utils.findMessageByFunc('testPartialFunc1');
await utils.respondToNativeMessage(parentMessage, true, {});

// The child window should properly receive the partial response plus
// the original event
expect(utils.childMessages.length).toBe(1);
const childMessage = utils.findMessageInChildByFunc('backButtonClick');
expect(childMessage).not.toBeNull();
const firstChildMessage = utils.childMessages[0];
expect(firstChildMessage.isPartialResponse).toBeTruthy();

// Pass the final response (non partial)
await utils.respondToNativeMessage(parentMessage, false, {});

// The child window should properly receive the non-partial response
expect(utils.childMessages.length).toBe(2);
const secondChildMessage = utils.childMessages[1];
expect(secondChildMessage.isPartialResponse).toBeFalsy();
app.setAllowMessageProxy(false);
});

// Also look at other uses of handleChildMessage in the codebase to make sure no other proxy-ing going on

describe('sendCustomMessage', () => {
it('should successfully pass message and provided arguments', async () => {
await utils.initializeWithContext('content');
Expand Down
Loading
Loading