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

BREAKING: don't inherit permissions by default #13668

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

lucacasonato
Copy link
Member

Previously specifying permissions: {} was the same as specifying
permissions: "inherit". Now it will be the same as permissions: "none".
Not specifying any permissions (permissions: undefined) still means
permissions: "inherit".

This is a breaking change. I want to try land this soon, as the current
behaviour is really impractical. If this can not be changed right now, I
am proposing it as a 2.0 change.

Previously specifying permissions: {} was the same as specifying
permissions: "inherit". Now it will be the same as permissions: "none".
Not specifying any permissions (permissions: undefined) still means
permissions: "inherit".

This is a breaking change. I want to try land this soon, as the current
behaviour is really impractical. If this can not be changed right now, I
am proposing it as a 2.0 change.
@crowlKats
Copy link
Member

Completely in favour of this change. But this is massively breaking, we should hold on for it to 2.0

@bartlomieju
Copy link
Member

I'm also in favor of this change but even though it's breaking I think we should ship it in 1.20. This behavior is less surprising and if some user hits the problem with it I think they will appreciate the soundness of new behavior.

@kitsonk
Copy link
Contributor

kitsonk commented Mar 10, 2022

I'm in favour of doing it for 1.20. Yes, it is a breaking change, but one could argue it is a security issue, which if it breaks some people doing things that are unsafe, we have helped them.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, let's land it

@bartlomieju bartlomieju merged commit a7bef54 into denoland:main Mar 16, 2022
@lucacasonato
Copy link
Member Author

Why exactly did we land this with no tests?

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

4 participants