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

RafaelGSS · 2023-12-16T16:10:58Z

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

cc: @nodejs/security-wg

nodejs-github-bot · 2023-12-16T16:11:03Z

Review requested:

@nodejs/security-wg

nodejs-github-bot · 2023-12-16T16:26:15Z

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

tniessen

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 · 2023-12-18T13:39:29Z

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 · 2023-12-18T20:23:34Z

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

joyeecheung · 2023-12-19T17:05:27Z

src/env.cc

@@ -877,7 +877,9 @@ Environment::Environment(IsolateData* isolate_data,
    // The process shouldn't be able to neither
    // spawn/worker nor use addons or enable inspector
    // unless explicitly allowed by the user
-    options_->allow_native_addons = false;
+    if (!options_->allow_addons) {
+      options_->allow_native_addons = false;


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.

nodejs-github-bot · 2023-12-19T18:26:42Z

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

nodejs-github-bot · 2023-12-21T12:44:19Z

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

ShogunPanda

LGTM!

nodejs-github-bot · 2023-12-21T17:44:13Z

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: TODO

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
76 checks passed

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

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RafaelGSS commented Dec 16, 2023

nodejs-github-bot commented Dec 16, 2023

nodejs-github-bot commented Dec 16, 2023

tniessen left a comment

RafaelGSS commented Dec 18, 2023

nodejs-github-bot commented Dec 18, 2023

joyeecheung Dec 19, 2023 •

edited

Loading

RafaelGSS Dec 19, 2023

nodejs-github-bot commented Dec 19, 2023

nodejs-github-bot commented Dec 21, 2023

ShogunPanda left a comment

nodejs-github-bot commented Dec 21, 2023

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

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

Conversation

RafaelGSS commented Dec 16, 2023

nodejs-github-bot commented Dec 16, 2023

nodejs-github-bot commented Dec 16, 2023

tniessen left a comment

Choose a reason for hiding this comment

RafaelGSS commented Dec 18, 2023

nodejs-github-bot commented Dec 18, 2023

joyeecheung Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

RafaelGSS Dec 19, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 19, 2023

nodejs-github-bot commented Dec 21, 2023

ShogunPanda left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 21, 2023

joyeecheung Dec 19, 2023 •

edited

Loading