GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — permission: fix spawnSync permission check by RafaelGSS · Pull Request #46975 · nodejs/node

Conversation

@RafaelGSS

@RafaelGSS RafaelGSS commented Mar 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

It was missing in the original PR #44004.

Fixes: https://github.com/nodejs-private/node-private/issues/394

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Mar 6, 2023
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 6, 2023
cjihrig
cjihrig approved these changes Mar 6, 2023
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Please also include:

Fixes: https://github.com/nodejs-private/node-private/issues/394

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 6, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 6, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50232/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Mar 6, 2023
rules: add permission subsystem nodejs/core-validate-commit#105
Merged
@RafaelGSS
RafaelGSS force-pushed the fix/add-permission-spawn-check branch from 41d0d74 to 693b833 Compare March 6, 2023 19:42
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 6, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 6, 2023
addaleax
addaleax approved these changes Mar 6, 2023
View reviewed changes
@RafaelGSS
permission: fix spawnSync permission check
67f3bce 
Fixes: nodejs-private/node-private#394

Signed-off-by: RafaelGSS <[email protected]>
@RafaelGSS
RafaelGSS force-pushed the fix/add-permission-spawn-check branch from 693b833 to 67f3bce Compare March 6, 2023 22:07
@RafaelGSS

RafaelGSS commented Mar 6, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/50242/ ✅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Mar 7, 2023
CI Reliability 2023-03-07 nodejs/reliability#516
Open
19 tasks
@RafaelGSS RafaelGSS added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Mar 7, 2023
@github-actions

github-actions Bot commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @RafaelGSS. Please 👍 to approve.
MoLow and cjihrig reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Mar 7, 2023

Copy link
Copy Markdown
Member Author

Requesting fast-track because I'll work on the v19.8.0 proposal tomorrow and we need to have this patch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS added the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 7, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Mar 7, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 7, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/46975
✔  Done loading data for nodejs/node/pull/46975
----------------------------------- PR info ------------------------------------
Title      permission: fix spawnSync permission check (#46975)
Author     Rafael Gonzaga  (@RafaelGSS)
Branch     RafaelGSS:fix/add-permission-spawn-check -> nodejs:main
Labels     child_process, c++, fast-track, needs-ci
Commits    1
 - permission: fix spawnSync permission check
Committers 1
 - RafaelGSS 
PR-URL: https://github.com/nodejs/node/pull/46975
Fixes: https://github.com/nodejs-private/node-private/issues/394
Reviewed-By: Colin Ihrig 
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46975
Fixes: https://github.com/nodejs-private/node-private/issues/394
Reviewed-By: Colin Ihrig 
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - permission: fix spawnSync permission check
   ℹ  This PR was created on Mon, 06 Mar 2023 16:46:11 GMT
   ✔  Approvals: 2
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/46975#pullrequestreview-1326742875
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/46975#pullrequestreview-1327221779
   ℹ  This PR is being fast-tracked
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-03-06T22:59:26Z: https://ci.nodejs.org/job/node-test-pull-request/50242/
- Querying data for job/node-test-pull-request/50242/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4356000066
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau
richardlau approved these changes Mar 7, 2023
View reviewed changes
@RafaelGSS

RafaelGSS commented Mar 7, 2023

Copy link
Copy Markdown
Member Author

Landed in b164038
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS added a commit that referenced this pull request Mar 7, 2023
@RafaelGSS
permission: fix spawnSync permission check
b164038 
Fixes: nodejs-private/node-private#394

Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #46975
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@RafaelGSS RafaelGSS closed this Mar 7, 2023
@tniessen

tniessen commented Mar 7, 2023

Copy link
Copy Markdown
Member

Requesting fast-track because I'll work on the v19.8.0 proposal tomorrow and we need to have this patch.

Maybe we should consider letting it bake on main for a while longer, given that this kind of bug would otherwise trigger the security release workflow?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Mar 7, 2023

Copy link
Copy Markdown
Member Author

Maybe we should consider letting it bake on main for a while longer, given that this kind of bug would otherwise trigger the security release workflow?

I don't see much value in it, at least for this particular feature. It's useful when it's a significant change or notable change being released in an LTS release. @cjihrig found the bug as a Node.js maintainer, I don't see other Node.js contributors finding it as they are unlikely to use it. Errors like this will be encountered (if any) by users and as you stated, would be fixed in a security release -- but, I don't see the baking time avoiding it though.

However, if you feel strongly about the baking time of this feature, I can postpone it to v19.9.0. Just let me know.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Mar 7, 2023

Copy link
Copy Markdown
Member

That's a good point. I don't feel strongly about it. I am only bringing this up because I think it is very likely that this feature will lead to vulnerability reports shortly after being released, simply because it significantly increases the attack surface and has only been reviewed by very few people so far. Letting it bake might lead to more issues like this being found that can be fixed publicly, but it might just as well be unlikely.
RafaelGSS reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS

RafaelGSS commented Mar 7, 2023

Copy link
Copy Markdown
Member Author

Well, skipping it for v19.8.0 won't hurt. Let's release the permission model in v19.9.0 then. I will include the blocked label.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS added the blocked PRs that are blocked by other issues or PRs. label Mar 7, 2023
@RafaelGSS RafaelGSS mentioned this pull request Mar 7, 2023
src,process: initial permission model implementation #44004
Merged
This was referenced Mar 8, 2023
CI Reliability 2023-03-08 nodejs/reliability#517
Open
CI Reliability 2023-03-09 nodejs/reliability#518
Open
CI Reliability 2023-03-10 nodejs/reliability#519
Open
CI Reliability 2023-03-11 nodejs/reliability#520
Open
CI Reliability 2023-03-12 nodejs/reliability#521
Open
targos pushed a commit that referenced this pull request Mar 13, 2023
@RafaelGSS @targos
permission: fix spawnSync permission check
4329c49 
Fixes: https://github.com/nodejs-private/node-private/issues/394

Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #46975
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@targos targos added backport-blocked-v19.x and removed blocked PRs that are blocked by other issues or PRs. labels Mar 14, 2023
This was referenced Mar 14, 2023
Permission logic in fs.open(), fs.openSync(), and fsPromises.open() can easily be bypassed #47090
Closed
Permission model restrictions imposed through process.permission.deny can be bypassed through case-insensitive paths #47105
Closed
@RafaelGSS RafaelGSS mentioned this pull request Apr 3, 2023
2023-04-18, Version 20.0.0 (Current) #47381
Merged
@RafaelGSS RafaelGSS added dont-land-on-v20.x and removed dont-land-on-v20.x labels Apr 3, 2023
@tniessen tniessen added the permission Issues and PRs related to the Permission Model. label Aug 10, 2023
@coderabbitai coderabbitai Bot mentioned this pull request Sep 12, 2026
📦🔥：confine importer writes to destination OpenINF/sdk#16
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@richardlau richardlau richardlau approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. fast-track PRs proposed for a shorter-than-standard waiting period before landing. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@RafaelGSS @nodejs-github-bot @tniessen @addaleax @cjihrig @richardlau @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.