-
Notifications
You must be signed in to change notification settings - Fork 199
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.
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
Trharris/messageproxy #2361
base: main
Are you sure you want to change the base?
Trharris/messageproxy #2361
Changes from all commits
d4a912d
b23b194
b332327
fd64430
a056a8c
eea0a6a
c96bd89
4515444
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "minor", | ||
"comment": "Disabling message proxying from child windows and frames by default", | ||
"packageName": "@microsoft/teams-js", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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); | ||
|
@@ -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) { | ||
// 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section of code takes the |
||
} | ||
}, | ||
); | ||
} else { | ||
logger('Message from child not allowed to proxy: %o', message); | ||
} | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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]; | ||
|
@@ -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. Choose a reason for hiding this commentThe 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 |
||
sendMessageEventToChild('themeChange', [theme]); | ||
} | ||
} | ||
|
@@ -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]); | ||
} | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -328,6 +328,7 @@ export namespace authentication { | |
} finally { | ||
Communication.childWindow = null; | ||
Communication.childOrigin = null; | ||
GlobalVars.webAuthWindowOpen = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
} | ||
|
||
|
@@ -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; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Immediately before opening an auth window we set |
||
// Open a child window with a desired set of standard browser features | ||
Communication.childWindow = Communication.currentWindow.open( | ||
fullyQualifiedURL.href, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,8 @@ | ||
import { | ||
Communication, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
sendAndHandleSdkError, | ||
sendAndHandleStatusAndReason, | ||
sendAndHandleStatusAndReasonWithDefaultError, | ||
sendAndUnwrap, | ||
sendMessageEventToChild, | ||
sendMessageToParent, | ||
} from '../internal/communication'; | ||
import { registerHandler, registerHandlerHelper } from '../internal/handlers'; | ||
|
@@ -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(); | ||
|
@@ -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(); | ||
|
@@ -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(); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
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 ifauthentication.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.