Address security and type safety issues in experimental messages implementation#181
Address security and type safety issues in experimental messages implementation#181Copilot wants to merge 3 commits intofeat/experimental-messagesfrom
Conversation
- Add timeout/AbortSignal support to sendExperimentalRequest - Validate event.source to prevent spoofing attacks - Change targetOrigin from '*' to configurable parameter - Update onFallbackRequest return type to Promise<unknown> - Extract mockExtra helper in AppRenderer tests Co-authored-by: idosal <[email protected]>
- Move parent window check to start of function - Reorder function declarations to avoid forward references - Remove unnecessary blank line in test file Co-authored-by: idosal <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds infrastructure for “experimental” JSON-RPC messages between guest iframes and the host AppRenderer, while tightening request handling and updating related typing/tests in the TypeScript SDKs.
Changes:
- Extended
sendExperimentalRequestwith timeout and AbortSignal support plus listener cleanup. - Updated
onFallbackRequesttyping to returnPromise<unknown>and refactored related test helpers. - Minor test cleanup in the web-component renderer test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
sdks/typescript/server/src/index.ts |
Adds options (timeout/AbortSignal/targetOrigin) and cleanup logic to sendExperimentalRequest. |
sdks/typescript/client/src/components/AppRenderer.tsx |
Widens onFallbackRequest return type and adjusts fallback handler wiring. |
sdks/typescript/client/src/components/__tests__/AppRenderer.test.tsx |
Extracts a reusable createMockExtra() helper for fallback handler tests. |
sdks/typescript/client/src/components/__tests__/UIResourceRendererWC.test.tsx |
Removes an eslint suppression but leaves a formatting artifact. |
| @@ -228,20 +232,71 @@ let _experimentalRequestId = 0; | |||
| export function sendExperimentalRequest( | |||
| method: string, | |||
| params?: Record<string, unknown>, | |||
| options?: { | |||
| signal?: AbortSignal; | |||
| timeout?: number; | |||
| targetOrigin?: string; | |||
| }, | |||
| ): Promise<unknown> { | |||
| const id = ++_experimentalRequestId; | |||
| const timeout = options?.timeout ?? 30000; | |||
| const targetOrigin = options?.targetOrigin ?? '*'; | |||
|
|
|||
There was a problem hiding this comment.
targetOrigin currently defaults to '*' (and the JSDoc documents that default). For postMessage-based RPC this is an unsafe default because it allows sending requests to any origin. Prefer requiring an explicit targetOrigin (or deriving it from a trusted source like the embedding origin) and update the docs accordingly.
| } | ||
|
|
||
| const data = event.data; | ||
| if (data?.jsonrpc === '2.0' && data?.id === id) { | ||
| window.removeEventListener('message', handler); | ||
| cleanup(); | ||
| if (data.error) { |
There was a problem hiding this comment.
The message handler validates event.source but does not validate event.origin. Without checking the expected origin, a compromised/untrusted embedding page (or an unexpected parent origin) could spoof responses for in-flight request IDs. Add an origin check tied to a configured/derived expected origin before accepting the response.
| // not yet in the Apps spec like "sampling/createMessage") | ||
| bridge.fallbackRequestHandler = async (request, extra) => { | ||
| if (onFallbackRequestRef.current) { | ||
| return onFallbackRequestRef.current(request, extra); | ||
| return onFallbackRequestRef.current(request, extra) as Promise<AppResult>; | ||
| } |
There was a problem hiding this comment.
onFallbackRequest is typed as Promise<unknown>, but the result is cast to Promise<AppResult> when wiring bridge.fallbackRequestHandler. This cast bypasses type safety and makes it easy to return a value that AppBridge/transport can’t handle. Consider changing onFallbackRequest to return Promise<AppResult> (or narrowing/validating the returned value) and removing the cast.
| @@ -141,7 +141,6 @@ describe('UIResourceRendererWC', () => { | |||
| const el = document.createElement('ui-resource-renderer'); | |||
|
|
|||
There was a problem hiding this comment.
There’s a stray whitespace-only line here (line 144) after removing the eslint-disable comment. Please delete it to avoid trailing-whitespace noise and keep the test formatted consistently.
Addresses code review feedback on PR #176's experimental messages feature, fixing security vulnerabilities and type system limitations.
Security Hardening
sendExperimentalRequest now validates message sources and prevents resource leaks:
event.source === window.parentto prevent spoofingtargetOriginparameter (defaults to'*')AbortSignalfor manual cancellationType System Improvements
onFallbackRequest return type changed from
Promise<Record<string, unknown>>toPromise<unknown>:sampling/createMessagewithout castingAppResultmaintains AppBridge compatibilityTest Improvements
createMockExtra()helper to eliminate repetitive mock object creationRequestHandlerExtrainstead ofas nevercasts💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.