src,permission: add --allow-addon flag by RafaelGSS · Pull Request #51183 · nodejs/node · GitHub

cc: @nodejs/security-wg

Review requested:

@nodejs/security-wg

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

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.

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.

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

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.

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.

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

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

LGTM!

Landed in 918e36e

PR-URL: #51183 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

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 <rafael.nunu@hotmail.com>

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 <rafael.nunu@hotmail.com>

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 <rafael.nunu@hotmail.com>

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 <rafael.nunu@hotmail.com>

PR-URL: #51183 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

src,permission: add --allow-addon flag

34b570b

RafaelGSS 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 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

RafaelGSS mentioned this pull request Dec 16, 2023

Permission Model - Roadmap nodejs/security-wg#898

Closed

7 tasks

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2023

tniessen reviewed Dec 16, 2023

View reviewed changes

joyeecheung reviewed Dec 19, 2023

View reviewed changes

marco-ippolito approved these changes Dec 21, 2023

View reviewed changes

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023

nodejs-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

RafaelGSS removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 21, 2023

ShogunPanda approved these changes Dec 21, 2023

View reviewed changes

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 21, 2023

nodejs-github-bot merged commit 918e36e into nodejs:main Dec 21, 2023

RafaelGSS added a commit that referenced this pull request Jan 2, 2024

src,permission: add --allow-addon flag

a69c7d7

PR-URL: #51183 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS mentioned this pull request Jan 2, 2024

v21.6.0 proposal #51342

Merged

richardlau pushed a commit that referenced this pull request Mar 25, 2024

src,permission: add --allow-addon flag

fd5efac

PR-URL: #51183 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

                   // unless explicitly allowed by the user
-                  options_->allow_native_addons = false;
+                  if (!options_->allow_addons) {
+                    options_->allow_native_addons = false;

src,permission: add --allow-addon flag by RafaelGSS · Pull Request #51183 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src,permission: add --allow-addon flag#51183
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RafaelGSS:permission-allow-addon

RafaelGSS commented Dec 16, 2023

Uh oh!

nodejs-github-bot commented Dec 16, 2023

Uh oh!

nodejs-github-bot commented Dec 16, 2023

Uh oh!

tniessen left a comment

Uh oh!

RafaelGSS commented Dec 18, 2023

Uh oh!

nodejs-github-bot commented Dec 18, 2023

Uh oh!

joyeecheung Dec 19, 2023 •
edited
Loading

Uh oh!

Uh oh!

RafaelGSS Dec 19, 2023

Uh oh!

nodejs-github-bot commented Dec 19, 2023

Uh oh!

nodejs-github-bot commented Dec 21, 2023

Uh oh!

ShogunPanda left a comment

Uh oh!

nodejs-github-bot commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src,permission: add --allow-addon flag by RafaelGSS · Pull Request #51183 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src,permission: add --allow-addon flag#51183nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom RafaelGSS:permission-allow-addonRafaelGSS/node:permission-allow-addonCopy head branch name to clipboard

Conversation

RafaelGSS commented Dec 16, 2023

Uh oh!

nodejs-github-bot commented Dec 16, 2023

Uh oh!

nodejs-github-bot commented Dec 16, 2023

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented Dec 18, 2023

Uh oh!

nodejs-github-bot commented Dec 18, 2023

Uh oh!

joyeecheung Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RafaelGSS Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 19, 2023

Uh oh!

nodejs-github-bot commented Dec 21, 2023

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

src,permission: add --allow-addon flag#51183
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RafaelGSS:permission-allow-addon

joyeecheung Dec 19, 2023 •
edited
Loading