fix(link-bins): make sure user has access to change file permissions #9751
Conversation
| if (filePath !== binFilePath) { | ||
| throw new Error(`Unexpected file path: ${filePath.toString()}. You should handle this case.`) | ||
| } |
There was a problem hiding this comment.
I thought this check and throw would be useful, if in future a new functionality added to the link-bins causing stat check to be called, then this test would most probably fail, so better if it fails with a more descriptive error. I don't know, maybe its a YAGNI 🤷 Let me know what you think
…efore changing permissions
1bab584 to
9e47b01
Compare
Because it makes more sense, and its not only about the user
| // TODO: Does this happen on windows? | ||
| if (IS_WINDOWS) return true | ||
|
|
||
| const userId = process.getuid?.() |
There was a problem hiding this comment.
process.getUid is added in node: v0.1.28, but still TypeScript thinks it may be underfined. So ?.
| } | ||
| } | ||
|
|
||
| async function canFixBin (filePath: string): Promise<boolean> { | ||
| if (IS_WINDOWS) return true |
There was a problem hiding this comment.
Skips windows, as I don't know if this problem happens on Windows machines too or not?
| if (await canFixBin(cmd.path)) await fixBin(cmd.path, 0o755) | ||
| else globalWarn(`Skipped fixing bin permissions of \`${cmd.path}\` because the file is not owned by the current user`) | ||
| } catch (err: any) { // eslint-disable-line |
There was a problem hiding this comment.
What about an optimistic approach? If it fails, then check canFixBin. Also, can't we just check the error?
There was a problem hiding this comment.
Hi. Thanks for the reply.
I didn't know what the error could be, and I assumed any error could be different on different OSes. So I went the most deterministic approach for only one particular error: when user does not have permission to change file permissions.
There was a problem hiding this comment.
Still, there's no need in running canFixBin unless fixBin throws an error.
There was a problem hiding this comment.
Aaaaa I see. That's true.
For that though, I'll need to change/remove my test: "linkBins() should not try to change permissions of files not owned by current user"
I'll look into it. Thanks
Tries to fix #3699
It checks if the file its trying to
fixBinon, is actually owned by the current user. Or current user is root. Becausechmodis only allowed if current user owns the target file (or is root).