-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
fix: add permission name when accessing a special file errors #25085
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test to ensure it's actually fixed and doesn't regress in the future?
I found something weird with tests import { assertThrows } from "jsr:@std/assert";
Deno.test({ permissions: { read: true } }, function execPathPerm() {
console.log(Deno.permissions.querySync({ "name": "read" }));
if (Deno.build.os === "linux") {
assertThrows(
() => {
const a = Deno.readTextFileSync("/proc/net/dev");
console.log(a);
},
Deno.errors.PermissionDenied,
"Requires read access to <exec_path>, run again with the --allow-read flag",
);
}
}); if I run this on the main branch with deno test -A, it doesn't error as expected, seems somehow like even though I specified only read = true, the tests are passing allow-all permission in implicit way, so it doesn't throw |
tests/unit/os_test.ts
Outdated
`PermissionDenied: Requires all access to "/proc/net/dev", run again with the --allow-all flag`, | ||
); | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out and cargo test js_unit_tests::os_test
runs fine with this, so I reverted to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... it was passing locally and now it's failing. I probably wasn't running it correctly. I'm looking into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to revert my changes and follow up in a separate pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
fix #25055