refactor(draft): replace <webview> with <iframe> #1191
Conversation
…ve in onlook:// urls in vite)
theshadowagent
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR replaces Electron's elements with standard <iframe> elements, introducing a custom message bridge and IFrameView implementation to maintain existing functionality while moving away from Electron-specific APIs.
- Added new
FrameViewcomponent inapps/studio/src/lib/editor/engine/frameview.tsximplementing webview-like functionality through iframes with potential security concerns around postMessage origin - Implemented custom message bridge in
apps/studio/electron/preload/webview/api.tsusing window.postMessage instead of Electron's IPC - Added
onlook://protocol handler inapps/studio/electron/main/index.tsfor secure preload script delivery to iframes - Concerning use of eval() for code execution in
apps/studio/electron/preload/webview/index.tsposes security risks - Type safety issues in
apps/studio/src/lib/editor/messageBridge.tswith 'as any' casting for event handlers
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
24 file(s) reviewed, 20 comment(s)
Edit PR Review Bot Settings | Greptile
| (window as TOnlookWindow).onlook.bridge.send(WebviewChannels.WINDOW_MUTATED, { | ||
| added: Object.fromEntries(added), | ||
| removed: Object.fromEntries(removed), | ||
| }); |
There was a problem hiding this comment.
logic: Type assertion to TOnlookWindow could fail silently if window.onlook or bridge is undefined. Consider adding a null check or error handling.
| api: onlookApi, | ||
| bridge: { | ||
| send: (channel: string, data?: any, transfer?: Transferable[]) => { | ||
| window.parent.postMessage({ type: channel, data }, '*', transfer); |
There was a problem hiding this comment.
logic: Using '*' as targetOrigin in postMessage is a security risk. Should restrict to specific origins.
| window.addEventListener('message', (event) => { | ||
| handler(event, event.data); | ||
| }); |
There was a problem hiding this comment.
logic: Missing origin validation in message event listener. Should verify event.origin matches expected domains.
| window.addEventListener('message', (event) => { | |
| handler(event, event.data); | |
| }); | |
| window.addEventListener('message', (event) => { | |
| if (event.origin === window.location.origin) { | |
| handler(event, event.data); | |
| } | |
| }); |
| (window as TOnlookWindow).onlook = { | ||
| api: onlookApi, | ||
| bridge: { | ||
| send: (channel: string, data?: any, transfer?: Transferable[]) => { | ||
| window.parent.postMessage({ type: channel, data }, '*', transfer); | ||
| }, | ||
| receive: (handler: (event: MessageEvent, data?: any) => void) => { | ||
| window.addEventListener('message', (event) => { | ||
| handler(event, event.data); | ||
| }); | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
style: Direct window object modification without Object.defineProperty may allow overwriting of onlook API. Consider using Object.defineProperty with configurable: false.
| protocol.handle('onlook', (request) => { | ||
| const filePath = path.join(__dirname, '../preload/webview.js'); | ||
| return new Response(fs.readFileSync(filePath)); | ||
| }); |
There was a problem hiding this comment.
style: Reading file synchronously could block the main process. Consider using async fs.promises.readFile instead
| protocol.handle('onlook', (request) => { | |
| const filePath = path.join(__dirname, '../preload/webview.js'); | |
| return new Response(fs.readFileSync(filePath)); | |
| }); | |
| protocol.handle('onlook', async (request) => { | |
| const filePath = path.join(__dirname, '../preload/webview.js'); | |
| const content = await fs.promises.readFile(filePath); | |
| return new Response(content); | |
| }); |
| return async (e: Electron.IpcMessageEvent) => { | ||
| const webview = e.target as Electron.WebviewTag; | ||
| const webview = e.target as IFrameView; | ||
| if (!e.args || e.args.length === 0) { |
There was a problem hiding this comment.
logic: IpcMessageEvent may not be the correct event type for iframes. Need to verify event type compatibility between Electron.WebviewTag and IFrameView.
| await webview.executeJavaScript(`window.api.setWebviewId('${webview.id}')`); | ||
| await webview.executeJavaScript(`window.api.processDom()`); |
There was a problem hiding this comment.
logic: executeJavaScript method may not be available or work the same way in IFrameView as it did in WebviewTag. Need to ensure this API is properly implemented in the new IFrameView interface.
| @@ -17,7 +18,7 @@ export class WebviewMessageBridge { | |||
| }; | |||
| } | |||
|
|
|||
| register(webview: Electron.WebviewTag, id: string) { | |||
| register(webview: IFrameView, id: string) { | |||
| const handlerRemovers: (() => void)[] = []; | |||
| Object.entries(this.eventHandlers).forEach(([event, handler]) => { | |||
| webview.addEventListener(event, handler as any); | |||
There was a problem hiding this comment.
style: Using 'as any' type assertion masks potential incompatibilities between iframe and webview event systems. Consider creating proper type definitions.
| export const sendToWebview = <T>(webview: IFrameView, channel: WebviewChannels, ...args: T[]) => { | ||
| if (!webview.contentWindow) { | ||
| throw new Error('Could not call sendToWebview(): .contentWindow is null/undefined'); | ||
| } | ||
| return webview.contentWindow.postMessage({ channel, data: args.map(jsonClone) }, '*'); | ||
| }; |
There was a problem hiding this comment.
logic: The postMessage API requires proper origin validation. Consider adding a check for allowed origins before sending messages to prevent potential security vulnerabilities.
| sandbox="allow-popups allow-scripts allow-same-origin" | ||
| style={{ |
There was a problem hiding this comment.
style: sandbox permissions may be too permissive - consider if 'allow-same-origin' is necessary and review security implications
theshadowagent
force-pushed
the
refactor/webview-to-iframe
branch
from
3526525 to
48fea5a
Compare
Description
What is the purpose of this pull request?