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.

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

Sandbox offscreen poc #9352

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Sandbox offscreen poc #9352

wants to merge 1 commit into from

Conversation

fungairino
Copy link
Collaborator

What does this PR do?

Discussion

  • Were there multiple approaches you were deciding between?

Demo

  • Paste a screenshot or demo video here

Future Work

  • Work for the issue/ticket that will be in a follow-up PR

Checklist

Leave all that are relevant and check off as completed

  • This PR requires a security review
  • This PR introduces a new library: double check it's MIT/Apache2/permissively licensed
  • This PR requires a node/npm version update: let the team know on #engineering
  • This PR requires a documentation change (link to old docs)
  • This PR requires a tutorial update (link to old tutorials)
  • This PR requires a feature flag
  • This PR requires a environment variable change
  • Added jest or playwright tests and/or storybook stories

For more information on our expectations for the PR process, see the
code review principles doc

@fungairino
Copy link
Collaborator Author

Thoughts on this approach @fregante ? Do you see any gotchas with moving the sandbox execution to the off screen page?

This is still missing better type checking, error handling/retries and tests, but the main flow works for nunjucks in this poc

Copy link

Playwright test results

failed  12 failed
skipped  132 skipped

Details

report  Open report ↗︎
stats  144 tests across 47 suites
duration  34 seconds
commit  7144016
info  For more information on how to debug and view this report, see our readme

Failed tests

chrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user
msedge-setup › setup/affiliated.setup.ts › authenticate with affiliated user
chrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user
msedge-setup › setup/affiliated.setup.ts › authenticate with affiliated user
chrome-setup › setup/affiliated.setup.ts › authenticate with affiliated user
msedge-setup › setup/affiliated.setup.ts › authenticate with affiliated user
chrome-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
msedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
chrome-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
msedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
chrome-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user
msedge-setup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user

Skipped tests

chrome › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick
msedge › tests/bricks/sidebarEffects.spec.ts › sidebar effect bricks › toggle sidebar brick
chrome › tests/deployments/deploymentActivation.spec.ts › activate a deployed mod in the extension console
msedge › tests/deployments/deploymentActivation.spec.ts › activate a deployed mod in the extension console
chrome › tests/extensionConsole/activation.spec.ts › can activate a mod with no config options
chrome › tests/extensionConsole/activation.spec.ts › can activate a mod with built-in integration
chrome › tests/extensionConsole/activation.spec.ts › validates activating a mod with required integrations
chrome › tests/extensionConsole/activation.spec.ts › can activate a mod with a database
chrome › tests/extensionConsole/activation.spec.ts › activating a mod when the quickbar shortcut is not configured
chrome › tests/extensionConsole/activation.spec.ts › can activate a mod via url
msedge › tests/extensionConsole/activation.spec.ts › can activate a mod with no config options
msedge › tests/extensionConsole/activation.spec.ts › can activate a mod with built-in integration
msedge › tests/extensionConsole/activation.spec.ts › validates activating a mod with required integrations
msedge › tests/extensionConsole/activation.spec.ts › can activate a mod with a database
msedge › tests/extensionConsole/activation.spec.ts › activating a mod when the quickbar shortcut is not configured
msedge › tests/extensionConsole/activation.spec.ts › can activate a mod via url
chrome › tests/extensionConsole/modsPage.spec.ts › can open mod in the workshop
msedge › tests/extensionConsole/modsPage.spec.ts › can open mod in the workshop
chrome › tests/extensionConsole/zapierModal.spec.ts › Zapier Integration Modal › can copy Zapier key and close modal
msedge › tests/extensionConsole/zapierModal.spec.ts › Zapier Integration Modal › can copy Zapier key and close modal
chrome › tests/modLifecycle.spec.ts › create, run, package, and update mod
msedge › tests/modLifecycle.spec.ts › create, run, package, and update mod
chrome › tests/pageEditor/addStarterBrick.spec.ts › Add new mod with different starter brick components
chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add new mod with different starter brick components
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
chrome › tests/pageEditor/brickActions.spec.ts › brick actions panel behavior
msedge › tests/pageEditor/brickActions.spec.ts › brick actions panel behavior
chrome › tests/pageEditor/brickConfiguration.spec.ts › brick configuration
msedge › tests/pageEditor/brickConfiguration.spec.ts › brick configuration
chrome › tests/pageEditor/copyMod.spec.ts › copying a mod that uses the PixieBrix API is copied correctly
chrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
msedge › tests/pageEditor/copyMod.spec.ts › copying a mod that uses the PixieBrix API is copied correctly
msedge › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
chrome › tests/pageEditor/liveEditing.spec.ts › live editing behavior
msedge › tests/pageEditor/liveEditing.spec.ts › live editing behavior
chrome › tests/pageEditor/logsPane.spec.ts › can view error logs
msedge › tests/pageEditor/logsPane.spec.ts › can view error logs
chrome › tests/pageEditor/modEditorPane.spec.ts › mod editor pane behavior
msedge › tests/pageEditor/modEditorPane.spec.ts › mod editor pane behavior
chrome › tests/pageEditor/moveOrCopyModComponent.spec.ts › Create new mod by moving mod component
chrome › tests/pageEditor/moveOrCopyModComponent.spec.ts › Create new mod by copying a mod component
msedge › tests/pageEditor/moveOrCopyModComponent.spec.ts › Create new mod by moving mod component
msedge › tests/pageEditor/moveOrCopyModComponent.spec.ts › Create new mod by copying a mod component
chrome › tests/pageEditor/saveMod.spec.ts › can save a new trigger mod
chrome › tests/pageEditor/saveMod.spec.ts › shows error notification when updating a public mod without incrementing the version
msedge › tests/pageEditor/saveMod.spec.ts › can save a new trigger mod
msedge › tests/pageEditor/saveMod.spec.ts › shows error notification when updating a public mod without incrementing the version
chrome › tests/pageEditor/specialPages.spec.ts › Restricted browser page
chrome › tests/pageEditor/specialPages.spec.ts › Unavailable mod
chrome › tests/pageEditor/specialPages.spec.ts › Page Editor reload
msedge › tests/pageEditor/specialPages.spec.ts › Restricted browser page
msedge › tests/pageEditor/specialPages.spec.ts › Unavailable mod
msedge › tests/pageEditor/specialPages.spec.ts › Page Editor reload
chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/regressions/formFlicker.spec.ts › forms flickering due to components unexpectedly unmounting/remounting › #8320: Search field loses focus while typing on snippet mod
msedge › tests/regressions/formFlicker.spec.ts › forms flickering due to components unexpectedly unmounting/remounting › #8320: Search field loses focus while typing on snippet mod
chrome › tests/regressions/hideModalsOnPageEditorRefresh.spec.ts › should hide add brick modal when Page Editor refreshes
msedge › tests/regressions/hideModalsOnPageEditorRefresh.spec.ts › should hide add brick modal when Page Editor refreshes
chrome › tests/regressions/sandboxBrickErrors.spec.ts › #8821: ensure Javascript script errors are thrown during brick runtime
msedge › tests/regressions/sandboxBrickErrors.spec.ts › #8821: ensure Javascript script errors are thrown during brick runtime
chrome › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links from the sidebar doesn't crash browser
msedge › tests/regressions/sidebarLinks.spec.ts › #8206: clicking links from the sidebar doesn't crash browser
chrome › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page
msedge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page
chrome › tests/runtime/allFrames.spec.ts › 8527: availability allFrames declaration
msedge › tests/runtime/allFrames.spec.ts › 8527: availability allFrames declaration
chrome › tests/runtime/customEvents.spec.ts › custom event brick functionality
msedge › tests/runtime/customEvents.spec.ts › custom event brick functionality
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
chrome › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8157: can insert at cursor from side bar
chrome › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu
msedge › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8157: can insert at cursor from side bar
msedge › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu
chrome › tests/runtime/localIntegrations.spec.ts › Local Integrations Page › can create a new integration
chrome › tests/runtime/localIntegrations.spec.ts › Local Integrations Page › #8067: blank numeric text integration configuration field validated on save
msedge › tests/runtime/localIntegrations.spec.ts › Local Integrations Page › can create a new integration
msedge › tests/runtime/localIntegrations.spec.ts › Local Integrations Page › #8067: blank numeric text integration configuration field validated on save
chrome › tests/runtime/modVariables/variableSync.spec.ts › Mod Variable Sync › session variable cross-tab sync
chrome › tests/runtime/modVariables/variableSync.spec.ts › Mod Variable Sync › tab variable sync
msedge › tests/runtime/modVariables/variableSync.spec.ts › Mod Variable Sync › session variable cross-tab sync
msedge › tests/runtime/modVariables/variableSync.spec.ts › Mod Variable Sync › tab variable sync
chrome › tests/runtime/sandbox.spec.ts › nunjucks template execution in sandbox
msedge › tests/runtime/sandbox.spec.ts › nunjucks template execution in sandbox
chrome › tests/runtime/screenshotTab.spec.ts › screenshot tab brick functionality
msedge › tests/runtime/screenshotTab.spec.ts › screenshot tab brick functionality
chrome › tests/runtime/setInputValue.spec.ts › can set input value
msedge › tests/runtime/setInputValue.spec.ts › can set input value
chrome › tests/runtime/sidebar/sidebarActivation.spec.ts › initiates sidebar mod activation from activate url click
chrome › tests/runtime/sidebar/sidebarActivation.spec.ts › does not redirect to non-pixiebrix domain
msedge › tests/runtime/sidebar/sidebarActivation.spec.ts › initiates sidebar mod activation from activate url click
msedge › tests/runtime/sidebar/sidebarActivation.spec.ts › does not redirect to non-pixiebrix domain
chrome › tests/runtime/sidebar/sidebarAuth.spec.ts › Connect action in partner auth sidebar takes user to the Extension Console
msedge › tests/runtime/sidebar/sidebarAuth.spec.ts › Connect action in partner auth sidebar takes user to the Extension Console
chrome › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › can open sidebar immediately from iframe without focus dialog
chrome › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
chrome › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › prevents host page styles from leaking into dialog
msedge › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › can open sidebar immediately from iframe without focus dialog
msedge › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › shows focus dialog in top-level frame
msedge › tests/runtime/sidebar/sidebarController.spec.ts › sidebar controller › prevents host page styles from leaking into dialog
chrome › tests/runtime/sidebar/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
chrome › tests/runtime/sidebar/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation
msedge › tests/runtime/sidebar/sidebarNavigation.spec.ts › sidebar mod panels are persistent during navigation
msedge › tests/runtime/sidebar/sidebarNavigation.spec.ts › sidebar form and temporary panels are unavailable after navigation
chrome › tests/runtime/sidebar/sidebarPanelTheme.spec.ts › custom sidebar theme css file is applied to all levels of sidebar document
msedge › tests/runtime/sidebar/sidebarPanelTheme.spec.ts › custom sidebar theme css file is applied to all levels of sidebar document
chrome › tests/runtime/srcdocFrames.spec.ts › 8143: mods can run in srcdoc iframes
msedge › tests/runtime/srcdocFrames.spec.ts › 8143: mods can run in srcdoc iframes
chrome › tests/runtime/textSnippets.spec.ts › text snippet shortcut functionality
msedge › tests/runtime/textSnippets.spec.ts › text snippet shortcut functionality
chrome › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can toggle the sidebar from the floating action button and view the related mod's sidebar panel
chrome › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can hide the floating action button
msedge › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can toggle the sidebar from the floating action button and view the related mod's sidebar panel
msedge › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can hide the floating action button
chrome › tests/smoke/modsPage.spec.ts › extension console mods page smoke test › can view available mods
msedge › tests/smoke/modsPage.spec.ts › extension console mods page smoke test › can view available mods
chrome › tests/smoke/pageEditor.spec.ts › page editor smoke test › can open the page editor and connect to an open tab
msedge › tests/smoke/pageEditor.spec.ts › page editor smoke test › can open the page editor and connect to an open tab
chrome › tests/smoke/sidebar.spec.ts › sidebar page smoke test › can open the sidebar from selection menu action and view the related mod's sidebar panel
msedge › tests/smoke/sidebar.spec.ts › sidebar page smoke test › can open the sidebar from selection menu action and view the related mod's sidebar panel
chrome › tests/smoke/workshopPage.spec.ts › extension console workshop smoke test › can navigate to workshop page without a username
msedge › tests/smoke/workshopPage.spec.ts › extension console workshop smoke test › can navigate to workshop page without a username
chrome › tests/telemetry/errors.spec.ts › can report errors to telemetry service
chrome › tests/telemetry/errors.spec.ts › can report a service worker error to telemetry service
chrome › tests/telemetry/errors.spec.ts › can report an indexdb error to telemetry service
msedge › tests/telemetry/errors.spec.ts › can report errors to telemetry service
msedge › tests/telemetry/errors.spec.ts › can report a service worker error to telemetry service
msedge › tests/telemetry/errors.spec.ts › can report an indexdb error to telemetry service
chrome › tests/workshop/createMod.spec.ts › can create a new mod from a yaml definition and update it
msedge › tests/workshop/createMod.spec.ts › can create a new mod from a yaml definition and update it

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 3.77358% with 51 lines in your changes missing coverage. Please review.

Project coverage is 75.20%. Comparing base (8318d74) to head (7144016).
Report is 411 commits behind head on main.

Files with missing lines Patch % Lines
src/tinyPages/offscreenProtocol.ts 0.00% 23 Missing ⚠️
src/tinyPages/offscreen.ts 0.00% 18 Missing ⚠️
src/tinyPages/offscreenDocumentController.ts 0.00% 5 Missing ⚠️
src/sandbox/messenger/api.ts 0.00% 3 Missing ⚠️
src/background/background.ts 0.00% 1 Missing ⚠️
src/utils/injectIframe.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9352      +/-   ##
==========================================
+ Coverage   74.24%   75.20%   +0.96%     
==========================================
  Files        1332     1373      +41     
  Lines       40817    41859    +1042     
  Branches     7634     7778     +144     
==========================================
+ Hits        30306    31482    +1176     
+ Misses      10511    10377     -134     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fregante
Copy link
Contributor

Thoughts on this approach @fregante ? Do you see any gotchas with moving the sandbox execution to the off screen page?

You should probably be able to skip the internal sandboxed iframe since offscreen documents have limited API access as well.

This is still missing better type checking, error handling/retries and tests, but the main flow works for nunjucks in this poc

webext-messenger can already handle all of this, including the types

@@ -88,3 +89,4 @@ initModUpdater();
initWalkthroughModalTrigger();
void initRestrictUnauthenticatedUrlAccess();
initStateControllerListeners();
void ensureOffscreenDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to create a new document every time the service worker is restarted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a quick hack to just show that it can be done. As you note, we should make sure that this is only done once

@twschiller
Copy link
Contributor

twschiller commented Oct 25, 2024

You should probably be able to skip the internal sandboxed iframe since offscreen documents have limited API access as well.

We should double-check with the Chrome DevRel team to see whether that's acceptable under their remote code policy. This page on remote-code doesn't mention off-screen documents: https://developer.chrome.com/docs/extensions/develop/migrate/remote-hosted-code

I think we might need to use the sandboxed iframe to support concurrency with 1/ error reporting code, and 2/ our voice streaming code that's also in the offscreen document

Do you see any gotchas with moving the sandbox execution to the off-screen page?

Some considerations (not blockers):

  • All tabs share the same sandbox, so a malicious mod could see data being processed by other tabs. (Not a concern under our mod threat model)
  • I eventually want to support JS package imports in the JS brick (e.g., via https://unpkg.com/). So we might want to spin up a new sandbox for certain JS brick runs to get package isolation. (Or the package import feature can just require that imported package won't modify globals)

@twschiller twschiller self-requested a review October 25, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants