Add sandbox attribute to extension iframe in DevTools. - #9967
Conversation
This change grants these permissions: - `allow-scripts`: enables extension JS / Wasm execution. - `allow-forms`: permits standard form interactions. - `allow-downloads`: allows extensions to export files/logs. - `allow-popups` & `allow-popups-to-escape-sandbox`: allows opening external links. This does not grant `allow-same-origin` permission, so extensions can't access the parent window's DOM, cookies, or local storage.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces iframe sandboxing for embedded DevTools extensions to enforce origin isolation, and updates the release notes. The reviewer recommended extracting the raw sandbox configuration string into a descriptive named constant to avoid magic strings, in accordance with the repository style guide.
|
Does this fix a specific issue that's been opened? |
|
See b/535724008 |
kenzieschmoll
left a comment
kenzieschmoll
left a comment
There was a problem hiding this comment.
How was this tested? The iFrame does need to talk back and forth with the parent for the DevTools extension API, so just want to make sure everything here still works.
|
This should still allow |
Flutter Web extensions require same-origin access for flutter.js to initialize Service Workers and web storage without throwing a SecurityError. This is still secure because the extension must be enabled, and the sandbox prevents top-level navigation, popups, and modal hijacking.
|
I ran a more complete test on the These headers still prevent extensions from page-hijacking, displaying popups, etc, so this is still a good thing to do. |
|
Need to verify that the extension reload feature still works. |
This is to ensure "force reload extension" works.
|
@kenzieschmoll I verified that the "Force reload" button still reloads the extension. This is ready for another look. |
| <p id="reload-info"></p> | ||
| <script> | ||
| document.getElementById('reload-info').textContent = 'Rendered at ' + new Date().toLocaleTimeString() + ' (Token: ' + Math.random().toString(36).substring(2, 8) + ')'; | ||
|
|
||
| window.addEventListener('message', (event) => { | ||
| const data = event.data; | ||
| if (!data) return; | ||
| if (data.type === 'ping') { | ||
| window.parent.postMessage({ type: 'pong' }, '*'); | ||
| } else if (data.type === 'forceReload') { | ||
| window.location.reload(); | ||
| } | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
why do we need this in the placeholder? There is already a button on the container that surrounds the extension to reload the iFrame
There was a problem hiding this comment.
I was using it to test the reload feature, I can remove it if you want.
| /// The sandbox permissions granted to embedded extension iframes. | ||
| /// | ||
| /// Grants the extension same-origin capabilities (needed for service workers, | ||
| /// storage, etc.) while allowing javascript execution, forms, downloads, and |
There was a problem hiding this comment.
should this be "and" instead of "while"? while makes it sound like a contradiction.
There was a problem hiding this comment.
I read this as "Prevents X while allowing Y", seems fine to me?
3 participants
This change grants these permissions:
allow-scripts: enables extension JS / Wasm execution.allow-forms: permits standard form interactions.allow-downloads: allows extensions to export files/logs.allow-popups&allow-popups-to-escape-sandbox: allows opening external links.This does not grant
allow-same-originpermission, so extensions can't access the parent window's DOM, cookies, or local storage.