Skip to content

src,permission: add --allow-addon flag#51183

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Dec 21, 2023
Merged

src,permission: add --allow-addon flag #51183

nodejs-github-bot merged 1 commit into from
Dec 21, 2023
+94 −1

Conversation

@RafaelGSS
Copy link
Member

@RafaelGSSRafaelGSS commented Dec 16, 2023

Ref: nodejs/security-wg#898 (comment)

cc: @nodejs/security-wg

@RafaelGSSRafaelGSS added semver-minor PRs that contain new features and should be released in the next minor version. permission Issues and PRs related to the Permission Model labels Dec 16, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2023

Review requested:

  • @nodejs/security-wg

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Dec 16, 2023
@RafaelGSSRafaelGSS mentioned this pull request Dec 16, 2023
Closed
7 tasks
@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 16, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/56333/

tniessen
tniessen reviewed Dec 16, 2023
View reviewed changes
Copy link
Member

@tniessentniessen left a comment

Choose a reason for hiding this comment

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

Is my understanding correct that the existing --addons flag is not supposed to affect the permission model? That might be a bit confusing, but on the other hand, this separation between feature control (--[no-]addons) and permissions (--experimental-permission --allow-addons) might actually be good.

@RafaelGSS
Copy link
MemberAuthor

RafaelGSS commented Dec 18, 2023

this separation between feature control (--[no-]addons) and permissions (--experimental-permission --allow-addons) might actually be good.

That's the idea, I believe having specific flags to allow operations (even if a similar flag exists - --addons) is the best DX and more maintainable from the permission model side.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 18, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/56374/

joyeecheung
joyeecheung reviewed Dec 19, 2023
View reviewed changes
src/env.cc
// unless explicitly allowed by the user
options_->allow_native_addons = false;
if (!options_->allow_addons){
options_->allow_native_addons = false;
Copy link
Member

@joyeecheungjoyeecheungDec 19, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Instead of overloading the value used for --addons, can we just add allow_addons into the considerations in Environment::no_native_addons()? Also it would be clearer if allow_addons is renamed to something like allow_addons_in_permissions to differentiate this with --addons.

Copy link
MemberAuthor

@RafaelGSSRafaelGSSDec 19, 2023

Choose a reason for hiding this comment

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

I'm following a pattern we use on the permission model:

  • --allow-child-process
  • --allow-worker
  • --allow-fs-read
  • --allow-fs-write

I feel naming it as --allow-addons-in-permissions would be odd. The reason I'm not touching the Environment::no_native_addons() logic is that it would be less clear from a permission model perspective if we handle permissions in a separate point of nodejs initialization/getter. That's pretty much what I told Tobias.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/56393/

@RafaelGSSRafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 21, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2023

Commit Queue failed
- Loading data for nodejs/node/pull/51183 ✔ Done loading data for nodejs/node/pull/51183 ----------------------------------- PR info ------------------------------------ Title src,permission: add --allow-addon flag (#51183) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:permission-allow-addon -> nodejs:main Labels c++, semver-minor, process, needs-ci, permission Commits 1 - src,permission: add --allow-addon flag Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/51183 Reviewed-By: Marco Ippolito ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/51183 Reviewed-By: Marco Ippolito -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 16 Dec 2023 16:10:58 GMT ✔ Approvals: 1 ✔ - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/51183#pullrequestreview-1792882483 ✘ This PR needs to wait 51 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-12-19T18:26:42Z: https://ci.nodejs.org/job/node-test-pull-request/56393/ - Querying data for job/node-test-pull-request/56393/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7288256191

@RafaelGSSRafaelGSS removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 21, 2023
ShogunPanda
ShogunPanda approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@ShogunPandaShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@RafaelGSSRafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023
@nodejs-github-botnodejs-github-bot merged commit 918e36e into nodejs:mainDec 21, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 21, 2023

Landed in 918e36e

RafaelGSS added a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
src,permission: add --allow-addon flag
a69c7d7
PR-URL: #51183 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
2024-01-04, Version 21.6.0 (Current)
1f6efcb
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: TODO
@RafaelGSSRafaelGSS mentioned this pull request Jan 2, 2024
Merged
RafaelGSS added a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
2024-01-04, Version 21.6.0 (Current)
ea1ea6c
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
2024-01-04, Version 21.6.0 (Current)
a8630a7
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
2024-01-04, Version 21.6.0 (Current)
5c87469
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Jan 3, 2024
@RafaelGSS
2024-01-04, Version 21.6.0 (Current)
52915b6
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 5, 2024
@RafaelGSS
2024-01-09, Version 21.6.0 (Current)
f3105d3
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 11, 2024
@RafaelGSS
2024-01-15, Version 21.6.0 (Current)
f38a9a5
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
RafaelGSS added a commit that referenced this pull request Jan 15, 2024
@RafaelGSS
2024-01-15, Version 21.6.0 (Current)
e133e51
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <[email protected]>
Medhansh404 pushed a commit to Medhansh404/node that referenced this pull request Jan 19, 2024
@RafaelGSS@Medhansh404
2024-01-15, Version 21.6.0 (Current)
2958d92
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@RafaelGSS@richardlau
src,permission: add --allow-addon flag
fd5efac
PR-URL: #51183 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
@richardlaurichardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessentniessentniessen left review comments

@joyeecheungjoyeecheungjoyeecheung left review comments

@ShogunPandaShogunPandaShogunPanda approved these changes

@marco-ippolitomarco-ippolitomarco-ippolito approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.permissionIssues and PRs related to the Permission ModelprocessIssues and PRs related to the process subsystem.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@RafaelGSS@nodejs-github-bot@ShogunPanda@tniessen@joyeecheung@marco-ippolito