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: resolve reference to an absolute path only for fs permission #47930

Merged
merged 1 commit into from
May 11, 2023
Merged

permission: resolve reference to an absolute path only for fs permission #47930

merged 1 commit into from
May 11, 2023

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented May 9, 2023 •
edited
Loading

For other candidate permissions, such as "net" or "env", this patch will pass the given reference without resolving it to an absolute path.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels May 9, 2023
@daeyeon
Copy link
Member Author

daeyeon commented May 9, 2023

/cc @nodejs/security-wg

@daeyeon
permission: resolve reference to absolute path only for fs permission
74bc8eb 
For other candidate permissions, such as "net" or "env", this patch
will pass the reference without resolving it to an absolute path.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon force-pushed the main.fix/permission-230509.Tue.917e branch from cbfbe91 to 74bc8eb Compare May 9, 2023 05:24
@tniessen
Copy link
Member

tniessen commented May 9, 2023

What other, non-path references are planned?

@daeyeon
Copy link
Member Author

daeyeon commented May 9, 2023

AFAIK, it hasn't been discussed yet. I assume that, in the case of "env" permission, environment variable names would be used like: permission.has("env.read", "FOO,BOO").

Although it isn't entirely clear, the conversion to an absolute path seems to be applied to the "fs" permission only, and there seems to be a plan to move it to the C++ side in the future.

@marco-ippolito
Copy link
Member

marco-ippolito commented May 9, 2023 •
edited
Loading

AFAIK, it hasn't been discussed yet. I assume that, in the case of "env" permission, environment variable names would be used like: permission.has("env.read", "FOO,BOO").

Although it isn't entirely clear, the conversion to an absolute path seems to be applied to the "fs" permission only, and there seems to be a plan to move it to the C++ side in the future.

I'm currently working on moving the resolve path to c++ side, so this check will be handled there @RafaelGSS

@daeyeon
Copy link
Member Author

daeyeon commented May 9, 2023

Thanks for confirming. It would be ideal to perform only type validation for the reference in JS side and let each permission C++ implementation handle parsing separately.

@RafaelGSS
Copy link
Member

That's correct. Eventually, we'll have env or net that accept non-path parameters.

Thanks for confirming. It would be ideal to perform only type validation for the reference in JS side and let each permission C++ implementation handle parsing separately.

Correct, this is how it will work.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@daeyeon daeyeon added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 9, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4eec362 into nodejs:main May 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 4eec362

@daeyeon daeyeon deleted the main.fix/permission-230509.Tue.917e branch May 11, 2023 16:32
targos pushed a commit that referenced this pull request May 12, 2023
@daeyeon @targos
permission: resolve reference to absolute path only for fs permission
af86625 
For other candidate permissions, such as "net" or "env", this patch
will pass the reference without resolving it to an absolute path.

Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #47930
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@targos targos mentioned this pull request May 15, 2023
Merged
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito 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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@daeyeon @tniessen @marco-ippolito @RafaelGSS @nodejs-github-bot @danielleadams