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

test-cli-permission tests fail when running with CWD containing /tmp #54021

Closed
jellelicht opened this issue Jul 24, 2024 · 1 comment · Fixed by #54188
Closed

test-cli-permission tests fail when running with CWD containing /tmp #54021

jellelicht opened this issue Jul 24, 2024 · 1 comment · Fixed by #54188
Labels
permission Issues and PRs related to the Permission Model test Issues and PRs related to the tests.

Comments

@jellelicht
Copy link

jellelicht commented Jul 24, 2024 •
edited
Loading

Version

20.15.1

Platform

Linux revint 6.6.40 #1 SMP PREEMPT_DYNAMIC 1 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

# NB, after building node under /tmp:
cd /tmp
./node /path/to/test/parallel/test-cli-permission-deny-fs.js
# or
./node /path/to/test/parallel/test-cli-permission-multiple-allow.js

How often does it reproduce? Is there a required condition?

Be in (any) subdirectory of /tmp (as CWD) when running these tests.

What is the expected behavior? Why is that the expected behavior?

The test succeeding, and not being dependent on CWD for a pass/fail.

What do you see instead?

node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'true'
- 'false'
    at Object.<anonymous> (/tmp/guix-build-node-20.15.1.drv-0/node-v20.15.1/test/parallel/test-cli-permission-deny-fs.js:46:10)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'true',
  expected: 'false',
  operator: 'strictEqual'
}

and

node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ 'true'
- 'false'
    at Object.<anonymous> (/tmp/guix-build-node-20.15.1.drv-0/node-v20.15.1/test/parallel/test-cli-permission-multiple-allow.js:27:10)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'true',
  expected: 'false',
  operator: 'strictEqual'
}

Additional information

There's also some other code in test/parallel/test-cli-permission-deny-fs.js related to CWD which seems to be brittle.
Shouldn't tests fail or succeed regardless of:

  • location of the node binary
  • CWD
@kapouer
Copy link
Contributor

kapouer commented Jul 25, 2024

Failure can be seen on debian ci too

@VoltrexKeyva VoltrexKeyva added test Issues and PRs related to the tests. permission Issues and PRs related to the Permission Model labels Jul 25, 2024
sendoru added a commit to sendoru/node that referenced this issue Aug 3, 2024
@sendoru
test: use relative paths in test-cli-permission tests
effea4b 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: nodejs#54021
sendoru added a commit to sendoru/node that referenced this issue Aug 3, 2024
@sendoru
test: skip the last test case when /tmp is firstPath
615b1b7 
Differing from the other subdirectories of `/` `/tmp` has rwx
permissions for all users. In the case where `firstPath` is `/tmp`,
the last test case will fail, as it expects granting permission to
`firstPath` will be failed. This commit skips the last test case when
`firstPath` is `/tmp`.

Fixes: nodejs#54021
@sendoru sendoru mentioned this issue Aug 3, 2024
Merged
targos pushed a commit that referenced this issue Aug 14, 2024
@sendoru @targos
test: use relative paths in test-cli-permission tests
67f7574 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: #54021
PR-URL: #54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Aug 14, 2024
@sendoru @targos
test: use relative paths in test-cli-permission tests
3e25be7 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: #54021
PR-URL: #54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
EarlyRiser42 pushed a commit to EarlyRiser42/node that referenced this issue Aug 14, 2024
@sendoru @EarlyRiser42
test: use relative paths in test-cli-permission tests
bd10d6b 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: nodejs#54021
PR-URL: nodejs#54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Sep 21, 2024
@sendoru @targos
test: use relative paths in test-cli-permission tests
3222543 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: #54021
PR-URL: #54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Sep 26, 2024
@sendoru @targos
test: use relative paths in test-cli-permission tests
6f86a02 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: #54021
PR-URL: #54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Oct 2, 2024
@sendoru @targos
test: use relative paths in test-cli-permission tests
fcf82ad 
`process.permission.has("fs")` checks if the process has permission
for all files under `cwd`. Granting permission for `/tmp` and running
tests with `cwd` containing `/tmp` will make the funtion return
`true`, differing from expected results. Using relative paths ensures
test paths are not `cwd` itself.

Fixes: #54021
PR-URL: #54188
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
permission Issues and PRs related to the Permission Model test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: fix test-cli-permission tests when cwd contains /tmp sendoru/node
3 participants
@kapouer @jellelicht @VoltrexKeyva