FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

permission: fix chmod,chown,link, and lutimes by RafaelGSS · Pull Request #47529 · nodejs/node · GitHub

/ node Public

permission: fix chmod,chown,link, and lutimes - #47529

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RafaelGSS:permission/improve-coverage
Apr 13, 2023
ConversationCommits (1)ChecksFiles changed
Merged

permission: fix chmod,chown,link, and lutimes#47529
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RafaelGSS:permission/improve-coverage

Conversation

RafaelGSS commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

fs.chmod, fs.chown, fs.link , and fs.lutimes wasn't handled properly by the permission model. This PR fixes it and increase the coverage of all file system API using permission model
cc: @nodejs/security-wg

permission: fix chmod,chown improve fs coverage
3e537cd 
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 12, 2023
RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023

nodejs-github-bot commented Apr 12, 2023

Copy link
Copy Markdown
Collaborator

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

marco-ippolito approved these changes Apr 12, 2023
View reviewed changes

marco-ippolito left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

github-actions Bot mentioned this pull request Apr 13, 2023
Open
36 tasks

nodejs-github-bot commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

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

RafaelGSS added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 13, 2023

github-actions Bot commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @RafaelGSS. Please 👍 to approve.

RafaelGSS commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

I need to include it on v20.0.0 proposal for security reasons.

tniessen reviewed Apr 13, 2023
View reviewed changes

tniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The title and description only mention chmod and chown, but based on the diff, it looks like there are also issues with link and lutimes.

tniessen added the security Issues and PRs related to security. label Apr 13, 2023
RafaelGSS changed the title permission: fix chmod,chown improve fs coverage permission: fix chmod,chown,link, and lutimes Apr 13, 2023

tniessen commented Apr 13, 2023

Copy link
Copy Markdown
Member

I need to include it on v20.0.0 proposal for security reasons.

We've previously delayed releasing the permission model whenever a new vulnerability was found. #44004 (comment) suggested a "a baking-time of 1 release for this feature (after landing all the patches)". It's not semver-major so we could land it in 20.1.0 instead, but I assume that's not really an option because 20.x is picking up everything from the main branch.

RafaelGSS commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

I need to include it on v20.0.0 proposal for security reasons.

We've previously delayed releasing the permission model whenever a new vulnerability was found. #44004 (comment) suggested a "a baking-time of 1 release for this feature (after landing all the patches)". It's not semver-major so we could land it in 20.1.0 instead, but I assume that's not really an option because 20.x is picking up everything from the main branch.

To not land it on v20.x we would need a revert PR to all affected PRs, which I'm not considering as an option for now.

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
nodejs-github-bot merged commit 1323992 into nodejs:main Apr 13, 2023

nodejs-github-bot commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

Landed in 1323992

RafaelGSS mentioned this pull request Apr 13, 2023
Merged
RafaelGSS added a commit that referenced this pull request Apr 13, 2023
permission: fix chmod,chown improve fs coverage
8bcf0a4 
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: #47529
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
github-actions Bot mentioned this pull request Apr 15, 2023
Open
33 tasks
tniessen added the permission Issues and PRs related to the Permission Model label Aug 10, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tniessen tniessen left review comments
benjamingr benjamingr approved these changes
marco-ippolito marco-ippolito approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model security Issues and PRs related to security.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL