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

Support sandboxed script execution #158

Closed
wants to merge 7 commits into from
Closed

Support sandboxed script execution #158

wants to merge 7 commits into from

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Dec 8, 2021

Allow running scripts in a sandbox environment, such that they have access to the DOM of page, but don't share js-level state with the page.


Preview | Diff

This allows us to get a realm id back when we execute in a sandbox.
Sandboxes are always associated with a context, so change the target type to allow
specifying a sandbox name, and when the name is specifed, get or create a sandbox
with that name to execute the script in.
@sadym-chromium
Copy link
Contributor

sadym-chromium commented Feb 23, 2022

Started discussion offline:
Should the ObjectIDs for JS Node wrapper objects (and other DOM objects) be consistent in different sandboxes?

  • yes, as user can use those nodes both in page realm and in sandbox.
  • no, as the JS wrapper objects are different and pretending they are the same (share the same objectId) seems odd.

Potential solution for that would be excluding DOM instances from the serialization and consider serialization only from the ECMAScript realm perspective. Having that, another way to describe the node is needed.

Another solution is to add something like domID to DOM objects so that they can be referenced when needed across sandboxes.

Implementation-wise in CDP we face the need of somehow merging domains (Runtime (V8) runs evaluate but cannot serialize the result, as it doesn't know of DOM). Some of the CDP implementation approaches can be found in draft document BiDi Serialization in CDP.

Having that, we'd consider split DOM instances from it's JS wrappers, leaving the last in default serialization. And introducing DOM-specific serialization like describeDomObjects or decribeNodes.

@jgraham
Copy link
Member Author

jgraham commented Feb 24, 2022

I created #176 for the discussion in that comment.

<div algorithm>
To <dfn>get or create a sandbox realm</dfn> given |name| and |source browsing context|:

1. If [=context sandbox map=] does not contain |name|, set |source browsing
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this algorithm doesn't quite align with the type of context sandbox map described above. Should this step check if context sandbox map contains source browsing context instead of name since it is a weak map whose keys are browsing contexts?

<dd>

<dt>The [=module map=]
<dd>Issue: Not sure if this ought to be the module map of the assocaited
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this should be an empty module map. If I'm understanding correctly, using the document's module map would mean that any names exported by module scripts on the page are now accessible in the sandbox if it imports the modules. This would break the isolation between the two realms.

@foolip
Copy link
Member

foolip commented Sep 14, 2022

@jgraham this is in draft still, do you want review, or what's needed to unblock it?


1. If |P| is a [=member=] of an [=interface=] implemented by |wrapped|:

1. If |P| is an [=atrthtmlibute=], let |getter| be the [=attribute getter=]
Copy link
Member

Choose a reason for hiding this comment

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

Is "atrthtmlibute" a typo?

@juliandescottes
Copy link
Contributor

@jgraham this is in draft still, do you want review, or what's needed to unblock it?

It seems that part of this proposal was moved to #169 ?

@jgraham
Copy link
Member Author

jgraham commented Sep 14, 2022

Yeah, let's just close this for now; we can always ressurect the changes if we want to use any of the more detailed text about how sandboxes work in terms of the platform.

@jgraham jgraham closed this Sep 14, 2022
@foolip foolip deleted the sandbox branch September 30, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants