Skip to content
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

Permission path delimiter #48944

Closed
wants to merge 10 commits into from
Closed

Permission path delimiter #48944

wants to merge 10 commits into from

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 commented Jul 27, 2023 •
edited by RafaelGSS
Loading

Fixes nodejs/security-wg#1039

This PR adds a new flag --permission-fs-path-delimiter which allows to change the path delimiter when providing a list of files to either --allow-fs-read or allow-fs-write.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 27, 2023
@mscdex
Copy link
Contributor

mscdex commented Jul 27, 2023

I think you'll need to rebase instead and get rid of all the extra commits.

@Ceres6
Copy link
Contributor Author

Ceres6 commented Jul 27, 2023

Yes, I noticed, I'm working on it

@Ceres6 Ceres6 force-pushed the permission-path-delimiter branch 2 times, most recently from d19df78 to c4cb9cc Compare July 27, 2023 20:42
RafaelGSS
RafaelGSS requested changes Jul 27, 2023
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

There are a lot of files commited that should be removed:

  • compare-no-warnings
  • node-new
  • node-old

@Ceres6 Ceres6 force-pushed the permission-path-delimiter branch 2 times, most recently from 96fc261 to aad5dc5 Compare July 27, 2023 20:45
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@RafaelGSS
Copy link
Member

We need to also include it as stability: experimental + update node.1 file

@RafaelGSS RafaelGSS added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 27, 2023
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/permissions.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/permissions.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Jul 27, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@Ceres6 Ceres6 force-pushed the permission-path-delimiter branch from 229648c to ef7277f Compare July 27, 2023 23:01
@Ceres6 Ceres6 requested a review from RafaelGSS July 27, 2023 23:08
RafaelGSS
RafaelGSS reviewed Jul 27, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
test/parallel/test-cli-permission-deny-fs.js Outdated Show resolved Hide resolved
test/parallel/test-cli-permission-deny-fs.js Outdated Show resolved Hide resolved
Ceres6 added 4 commits July 29, 2023 10:52
@Ceres6
src,permission: add permission-fs-path-delimiter flag
ec6b344 
--permission-fs-path-delimiter flag added to permission model.
If not provided default value will be comma.

Fixes: nodejs/security-wg#1039
@Ceres6
Lint and spelling fixes
23b7833
@Ceres6
format fixes
9e7b868
@Ceres6
tests moved to new test file
e98237e
@Ceres6 Ceres6 force-pushed the permission-path-delimiter branch from 513c1a2 to e98237e Compare July 29, 2023 08:55
@Ceres6 Ceres6 requested a review from RafaelGSS July 29, 2023 09:02
RafaelGSS
RafaelGSS reviewed Jul 29, 2023
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Great work!

src/permission/fs_permission.cc Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
Ceres6 and others added 2 commits July 29, 2023 15:11
@RafaelGSS RafaelGSS added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 29, 2023
RafaelGSS
RafaelGSS approved these changes Jul 29, 2023
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM with green ci

@RafaelGSS RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 29, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RafaelGSS
Copy link
Member

@Ceres6 this is failing on Windows, could you check?

@Ceres6
Copy link
Contributor Author

Ceres6 commented Jul 31, 2023

Test should be fixed now

tniessen
tniessen reviewed Jul 31, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is there some discussion leading up to this? The default (comma) is a questionable choice because it does not match path.delimiter on any supported platform and because it is a valid character within file names (unless perhaps this syntax will ever be used for anything besides file system paths?). But is adding yet another CLI option the right solution?

If you want to stick with commas by default, you might want to consider something like emcc's approach. It supports various formats (some of which are admittedly ambiguous) and requires zero configuration:

  • foo,bar
  • "foo","bar"
  • ["foo","bar"]
  • [foo,bar]

I believe --allow-fs-* can also be specified more than once, so a custom list syntax is technically not required at all. Dropping it would likely be the least ambiguous option.

(This is non-blocking. Just my two cents.)

@RafaelGSS
Copy link
Member

We've been discussing it at nodejs/security-wg#1039.

The problem with accepting multiple --allow-fs-* is the breaking change. Even though a breaking change is acceptable given the experimental status it's something I'd like to avoid.

Well... Technically, we can support both (single --allow-fs separated by a comma, a multiple a --allow-fs), but it will require some work to support both implementations such as:

  • If a single string was provided, split them by comma
    • What if the user provides a single path and doesn't want to split it by a comma?

If you want to stick with commas by default, you might want to consider something like emcc's approach

Honestly, I never heard about it. Does emcc stand for ecmascripten?

@Ceres6 Ceres6 mentioned this pull request Aug 1, 2023
Closed
@tniessen tniessen added the permission Issues and PRs related to the Permission Model label Aug 10, 2023
@RafaelGSS
Copy link
Member

Closing it in favour of #49047

@RafaelGSS RafaelGSS closed this Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen left review comments

@mscdex mscdex mscdex left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model semver-minor PRs 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.

How to includes "," or "*" char self in --allow-fs-read?
6 participants
@Ceres6 @nodejs-github-bot @mscdex @RafaelGSS @tniessen @atlowChemi