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.

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

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

Merged
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

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.

@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.

@nodejs-github-bot

This comment was marked as outdated.

@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.

@tniessen
Copy link
Member Author

The git-rebase job failed 🤔

13:05:02 + git push [email protected]: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 '[email protected]:binary_tmp.git'

@richardlau
Copy link
Member

The git-rebase job failed 🤔

13:05:02 + git push [email protected]: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 '[email protected]: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.

@nodejs-github-bot

This comment was marked as outdated.

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 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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

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

@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
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a81d5e1

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
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 <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants