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

proposal: sandbox feature improvements #116

Open
lowlighter opened this issue Nov 25, 2024 · 2 comments
Open

proposal: sandbox feature improvements #116

lowlighter opened this issue Nov 25, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@lowlighter
Copy link
Contributor

Granular permissions

Instead of just sandbox: true, I think we should rely on Deno.PermissionOptionsObject, which would make it closer to what Deno.test and deno WebWorker offer in terms of API, in addition to offer more flexibility.

Also maybe a prompt option could be added to decide whether to use Deno.permissions.request or Deno.permissions.query ? Though I don't think there's many use case where someone would want to run some pages in interactive permissions and other in non-interactives). So maybe we can scrap this one.

Example:

await browser.newPage("https://example.com", {
 sandbox: { 
   prompt: false, 
   permissions: { net: ["google.com"], read: true }
 } 
});

Realm-aware permissions

If the above gets implemented, the #validateRequest method will need to be refactored.

Currently, we use the main realm Deno.permissions object, which also mean that any permissions accepted within sandbox also "leaks" to the main realm and could be see as a security issue.

To avoid re-implementing deno's logic of validating permissions, maybe the easier way to would be to spawn a new WebWorker with the provided permissions descriptor (new WebWorker(..., { deno: { permissions } }) and launch the validation requests from there.

It'd probably create a small overhead, but I think the tradeoff is acceptable as it's an advanced feature and improve security too.

Use the new --allow-imports for <script>

Currently we treat imports from script tag as "net" (like deno used to for dynamic imports), but they have now introduced a new "imports" permissions (though the query descriptor has been forgotten upstream: denoland/deno#27050)

The fetch paused event of CDP is able to know what is trying to be loaded (https://chromedevtools.github.io/devtools-protocol/tot/Network/#type-ResourceType), in particular "Script" element.

So to match the new deno behavior, it may makes more sense to align too on this, since the <script> are in a sense dynamic imports.

It would also be a nice QOL, because you'd be able to distinguish scripts from other resources (images, etc.)

@lino-levan lino-levan added the enhancement New feature or request label Nov 29, 2024
@lino-levan
Copy link
Owner

I'm a fan. This would be really cool. We should still allow an inherit mode or something like that but more granular control is cool.

@lowlighter
Copy link
Contributor Author

The Deno.PermissionOptionsObject actually already allows the "inherit" (both at granular and global levels), and supports booleans too.

The only thing is that maybe "true" should be replaced by "inherit" by astral for QOL, because passing true when permissions are restricted in main realm throw escalation errors since you're trying to grant more permissions than allowed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants