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

feat: Enable Deno APIs by default in Workers #8178

Closed
wants to merge 1 commit into from

Conversation

nayeemrmn
Copy link
Collaborator

Closes #8174.

@nayeemrmn nayeemrmn changed the title chore(unstable): Enable Deno APIs by default in Workers feat: Enable Deno APIs by default in Workers Oct 29, 2020
@lucacasonato lucacasonato added breaking change a change or feature that breaks existing semantics cli related to cli/ dir permissions related to --allow-* flags web related to Web APIs labels Oct 29, 2020
@lucacasonato
Copy link
Member

lucacasonato commented Oct 29, 2020

This is a breaking change because it would change the default permissions of new Worker(). Previously you did not have FS access by default (if parent has permission) for example.

@nayeemrmn nayeemrmn force-pushed the worker-deno-apis branch 2 times, most recently from 945df45 to be7d6ce Compare October 30, 2020 12:35
@bartlomieju
Copy link
Member

I'm in favor of this change, I especially like how it simplifies WebWorker setup in Rust (it's useful for #8381)

@nayeemrmn
Copy link
Collaborator Author

@bartlomieju Please review.

@lucacasonato
Copy link
Member

lucacasonato commented Dec 5, 2020

@nayeemrmn This is on hold because we can't land until 2.0. (Security related breaking change)

@nayeemrmn
Copy link
Collaborator Author

Previously you did not have FS access by default (if parent has permission) for example.

Hmm... was this ever an intended security mechanism? It was always arbitrary that workers inherited net permissions with fetch(), but not fs permissions as a side-effect of the functions being hidden.

The only cases where this provides new-found e.g. read permissions are where the runtime was granted --allow-read. This change fully preserves the integrity of our security flags. We didn't reasonably assure anything else.

@lucacasonato
Copy link
Member

Intended or not, it is in stable, so we have to take this into consideration before merging. People might be using workers for sandboxing.

@bartlomieju
Copy link
Member

@nayeemrmn I'm in favor of landing this PR, but I think Luca has a point. Let's discuss further on Discord

@stale
Copy link

stale bot commented Feb 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 3, 2021
@kt3k kt3k removed the stale label Feb 4, 2021
Base automatically changed from master to main February 19, 2021 14:58
@stale
Copy link

stale bot commented Apr 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 20, 2021
@bartlomieju bartlomieju removed the stale label Apr 20, 2021
@stale
Copy link

stale bot commented Jul 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 16, 2021
@satyarohith satyarohith removed the stale label Jul 16, 2021
@ry
Copy link
Member

ry commented Aug 5, 2021

I'm closing this PR because it's stale and there's a bunch of merge conflicts. But we're going to discuss this issue soon and try to come to some consensus on proceeding.
Thanks for the patch @nayeemrmn - we've got the diff here still so we can always refer back.

To be continued....

@ry ry closed this Aug 5, 2021
@nayeemrmn nayeemrmn deleted the worker-deno-apis branch August 15, 2021 18:10
@bartlomieju
Copy link
Member

@nayeemrmn can you open a new PR with this change? It's still highly desired and now that we're eyeballing 2.0 it'd be great to have it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change a change or feature that breaks existing semantics cli related to cli/ dir permissions related to --allow-* flags web related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Deno namespace by default in Workers
7 participants