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

Provide option to delete Deno namespace in worker #2717

Merged
merged 4 commits into from Aug 5, 2019

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Aug 4, 2019

Add an option to Worker creation such that worker could be sandboxed (no access to window.Deno namespace)

The condition is propagated (e.g. if a sandboxed worker tries to create another worker, it will always also be sandboxed)

This is achieved by adding a condition to per worker/isolate State and calling denoMain() with param whether to preserve or delete window.Deno

(The locations where things are attached/bounded to Deno or Deno.core is growing more nebulous. Maybe we need better comments for it)

Tests needed. Will try to add them after first-round review (to ensure no leaking of the namespace through other channels) Added

@kevinkassimo kevinkassimo force-pushed the worker/no_deno_namespace branch 2 times, most recently from 98d0b91 to 093a6ec Compare August 4, 2019 06:48
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Seems reasonable. It’s a little annoying to lay all this pipe for just this one boolean. It makes me wonder if we should future proof this but having an options object of some sort... but I think it’s not much code so probably better to just add the boolean as you did.

I could imagine wanting to do something where you restrict permissions in the child worker - say limit network access...

@kevinkassimo kevinkassimo changed the title [NEEDS TESTS] Provide option to delete Deno namespace in worker Provide option to delete Deno namespace in worker Aug 4, 2019
@kevinkassimo
Copy link
Contributor Author

I could imagine wanting to do something where you restrict permissions in the child worker - say limit network access...

Yeah sounds reasonable, though I do feel the sandboxing provided by this change is quite similar to the browser environment. Anyways we might experiment with that in another PR.

Also just added a basic integration test.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

nice test

tests/039_worker_deno_ns.ts Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Could you add something to the manual or JSDOC describing this behavior?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit ddee2df into denoland:master Aug 5, 2019
@piscisaureus piscisaureus mentioned this pull request Aug 9, 2019
@kevinkassimo kevinkassimo deleted the worker/no_deno_namespace branch December 27, 2019 07:52
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.

None yet

2 participants