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

doc,test: add known path resolution issue in permission model #49155

Merged
merged 1 commit into from
Aug 31, 2023
Merged

doc,test: add known path resolution issue in permission model #49155

merged 1 commit into from
Aug 31, 2023

Conversation

tniessen
Copy link
Member

As a side effect of 205f1e6, Node.js now resolves some paths differently when the permission model is enabled. While these are mostly edge cases, they are worth mentioning in the documentation. This commit also adds a known_issues test that demonstrates one such difference.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 13, 2023
@tniessen tniessen mentioned this pull request Aug 13, 2023
Closed
@RafaelGSS RafaelGSS self-requested a review August 14, 2023 00:35
RafaelGSS
RafaelGSS approved these changes Aug 14, 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.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen force-pushed the doc_test-pm-path-resolution branch from 517748a to 946da0a Compare August 21, 2023 14:43
@tniessen tniessen added request-ci Add this label to start a Jenkins CI on a PR. security Issues and PRs related to security. permission Issues and PRs related to the Permission Model doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Aug 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Aug 22, 2023
Open
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen added the review wanted PRs that need reviews. label Aug 23, 2023
This was referenced Aug 24, 2023
Open
Open
Open
Open
@tniessen tniessen force-pushed the doc_test-pm-path-resolution branch from 946da0a to e71525d Compare August 27, 2023 09:54
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member Author

The git-rebase job failed 🤔

13:05:02 + git push binary_tmp@147.75.72.255:binary_tmp.git +jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd
13:05:03 remote: fatal: bad object refs/heads/jenkins-node-test-commit-arm-fanned-44627007be7eb3934e39bdd3df7e8f2556a2cd44-binary-pi1p/cc-armv7        
13:05:03 fatal: bad object refs/heads/jenkins-node-test-commit-arm-fanned-44627007be7eb3934e39bdd3df7e8f2556a2cd44-binary-pi1p/cc-armv7
13:05:03 To 147.75.72.255:binary_tmp.git
13:05:03  ! [remote rejected]         jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd -> jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd (missing necessary objects)
13:05:03 error: failed to push some refs to 'binary_tmp@147.75.72.255:binary_tmp.git'

@richardlau
Copy link
Member

The git-rebase job failed 🤔

13:05:02 + git push binary_tmp@147.75.72.255:binary_tmp.git +jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd
13:05:03 remote: fatal: bad object refs/heads/jenkins-node-test-commit-arm-fanned-44627007be7eb3934e39bdd3df7e8f2556a2cd44-binary-pi1p/cc-armv7        
13:05:03 fatal: bad object refs/heads/jenkins-node-test-commit-arm-fanned-44627007be7eb3934e39bdd3df7e8f2556a2cd44-binary-pi1p/cc-armv7
13:05:03 To 147.75.72.255:binary_tmp.git
13:05:03  ! [remote rejected]         jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd -> jenkins-node-test-commit-windows-fanned-e71525d18c4b7f2241bdf6654d7993930d51bcfd (missing necessary objects)
13:05:03 error: failed to push some refs to 'binary_tmp@147.75.72.255:binary_tmp.git'

Unfortunately node-test-commit-arm-fanned and node-test-commit-windows-fanned are going to be broken until this is resolved. I've opened nodejs/build#3475 -- I won't be able to log onto the machine until Tuesday but maybe someone else with access will be able to take a look before then.

This was referenced Aug 28, 2023
Open
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
doc,test: add known path resolution issue in permission model
5f4527e 
As a side effect of 205f1e6, Node.js
now resolves some paths differently when the permission model is
enabled. While these are mostly edge cases, they are worth mentioning in
the documentation. This commit also adds a known_issues test that
demonstrates one such difference.
@tniessen tniessen force-pushed the doc_test-pm-path-resolution branch from e71525d to 5f4527e Compare August 29, 2023 19:32
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2023
@tniessen
Copy link
Member Author

Should work now, hopefully. The problem on Windows appears to be that, while most modern operating systems resolve symlinks while parsing a path, Windows doesn't. Literal UNC paths seem to bypass normalization on Windows, but they are subject to other restrictions that are incompatible with the test.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Aug 30, 2023
Open
16 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@tniessen
Copy link
Member Author

@nodejs/security-wg This needs to be re-approved.

@github-actions github-actions bot mentioned this pull request Aug 31, 2023
Open
18 tasks
@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Aug 31, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 31, 2023
@nodejs-github-bot nodejs-github-bot merged commit a81d5e1 into nodejs:main Aug 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in a81d5e1

This was referenced Sep 1, 2023
Open
Open
Open
Open
Open
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@tniessen @UlisesGascon
doc,test: add known path resolution issue in permission model
f45c8e1 
As a side effect of 205f1e6, Node.js
now resolves some paths differently when the permission model is
enabled. While these are mostly edge cases, they are worth mentioning in
the documentation. This commit also adds a known_issues test that
demonstrates one such difference.

PR-URL: #49155
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@tniessen @alexfernandez
doc,test: add known path resolution issue in permission model
5515036 
As a side effect of 205f1e6, Node.js
now resolves some paths differently when the permission model is
enabled. While these are mostly edge cases, they are worth mentioning in
the documentation. This commit also adds a known_issues test that
demonstrates one such difference.

PR-URL: nodejs#49155
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@tniessen tniessen mentioned this pull request Jan 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. 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. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tniessen @nodejs-github-bot @richardlau @RafaelGSS