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

fs.copyFile on Mac OS does not respect permissions #26936

Closed
akhoury opened this issue Mar 26, 2019 · 11 comments
Closed

fs.copyFile on Mac OS does not respect permissions #26936

akhoury opened this issue Mar 26, 2019 · 11 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. macos Issues and PRs related to the macOS platform / OSX.

Comments

@akhoury
Copy link

akhoury commented Mar 26, 2019

  • Version: v11.2.0
  • Platform: Darwin iMac.local 18.2.0 Darwin Kernel Version 18.2.0: Thu Dec 20 20:46:53 PST 2018; root:xnu-4903.241.1~1/RELEASE_X86_64 x86_64
  • Subsystem:

When setting a file mode to 444 using chmodSync, then trying to copy another file over it, on Windows and on Linux, copyFile will throw an error, but on a Mac it does not, and it will override the file

fs.chmodSync(dest, '444');
// then try to 
fs.copyFile(source, dest, function (err) {
    // 	i do expect an err here, 
   // but there is no error on Mac
});

i even check the stat.mode using parseInt(fs.statSync(dest).mode.toString(8), 10) and it does in fact say 100444

This is a sample when everything is working well
https://repl.it/repls/CylindricalGlisteningCondition

However, when i run it on my mac, I get this

This platform is darwin
fs.stat.mode ./dest: 100444
/Users/akhoury/rcloned/code/NodeBB/rrr.js:26
                throw new Error(`This should've thrown an error!`);
                ^

Error: This should've thrown an error!

Thanks

@Trott
Copy link
Member

Trott commented Mar 27, 2019

I'm able to replicate this bug on Mojave. Seems like libuv is somehow running copyfile() with COPYFILE_UNLINK set or else there's a bug in macOS? @nodejs/fs @nodejs/libuv @nodejs/platform-macos

@Trott Trott added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. macos Issues and PRs related to the macOS platform / OSX. labels Mar 27, 2019
@Trott
Copy link
Member

Trott commented Mar 27, 2019

Confirmed with Node.js 8.x, 10.x, 11.x, and current master branch. (fs.copyFile() does not exist in Node.js 6.x.)

@Trott
Copy link
Member

Trott commented Mar 27, 2019

I tried adding this to libuv's fs.c file and recompiling but it didn't change anything...

  // Turn off unlinking of the destination file.
  // Refs: https://github.com/nodejs/node/issues/26936
  flags &= ~(1 << 21); /* COPYFILE_UNLINK */

(Of course, it's entirely likely I'm doing the bit math wrong or something simple like that.....)

@richardlau
Copy link
Member

I'm able to replicate this bug on Mojave. Seems like libuv is somehow running copyfile() with COPYFILE_UNLINK set or else there's a bug in macOS? @nodejs/fs @nodejs/libuv @nodejs/platform-macos

I'm not a macOS person, but being able to bypass file permissions sounds like an OS bug.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 27, 2019

This seems to be related to permissions and probably not a bug we can control. I'm also seeing no error by default, but if I run with sudo -u nobody then I get an EPERM error.

@Trott
Copy link
Member

Trott commented Mar 27, 2019

I'm not a macOS person, but being able to bypass file permissions sounds like an OS bug.

Yeah, definitely agree after playing around with different copyfile() flags inside libuv. Astonishingly, it seems that copyfile(3) on macOS does not respect file permissions. We can't be the first people to ever experience this, though... 🤔

@Trott
Copy link
Member

Trott commented Mar 27, 2019

libuv's fs.c has Apple-specific code for uv__fs_copyfile() added by @cjihrig in libuv/libuv#1465. Do you know if there was a reason other than "use analogous native functionality when you can" for using different code for macOS? Removing it all and using the code used by other UNIX-like operating systems fixes this problem. node continues to pass all tests too. So maybe that's the way to go?

Trott added a commit to Trott/libuv that referenced this issue Mar 27, 2019
Using copyfile(3) on macOS apparently results in situations where file
permissions are ignored. Using the same code for other UNIX-like
operating systems seems to fix the issue.

Refs: nodejs/node#26936 (comment)
@Trott
Copy link
Member

Trott commented Mar 27, 2019

libuv/libuv#2233

Trott added a commit to Trott/io.js that referenced this issue Mar 27, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: nodejs#26936
Ref: libuv/libuv#2233
@Trott
Copy link
Member

Trott commented Mar 27, 2019

#26939

@vtjnash
Copy link
Contributor

vtjnash commented Mar 29, 2019

I had recently noticed this behavior divergence also, as it can occur in the context of symlinks (hard or soft): libuv/libuv#2199 (comment). Turning that into an issue has been on my TODO list (and providing analysis on what it seems like it should do), but I think this should already address it.

I was actually a bit surprised by that the proposed behavior here is the desired choice, since it means that a copy may end up with different attributes from the original file, and it uses either the permissions of the hardlink or of the containing directory, if the destination didn't exist at the time. The result is not necessarily more nor less restrictive, just different.

But it seems that what libuv does on unix is the explicit choice for the C++17 standard for std::filesystem::copy_file and is also what unix cp appears to do. So that seems like a good track record to follow. By contrast, cp -f behaves either like "unix" (if the dest file could be opened for write) or like "macos" (if the dest file could not).

Shell test script:

echo 1 > file1
echo 2 > file2
ln file1 file_link
# optional: chmod a-w file1
cp file2 file_link # or cp -f
cat file1 # prints 2

Trott added a commit to Trott/io.js that referenced this issue Mar 30, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: nodejs#26936
Ref: libuv/libuv#2233

PR-URL: nodejs#26939
Refs: nodejs#26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: #26936
Ref: libuv/libuv#2233

PR-URL: #26939
Refs: #26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 8, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: #26936
Ref: libuv/libuv#2233

PR-URL: #26939
Refs: #26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
cjihrig pushed a commit to libuv/libuv that referenced this issue Apr 11, 2019
Using copyfile(3) on macOS apparently results in situations where file
permissions are ignored. Using the same code for other UNIX-like
operating systems seems to fix the issue.

Refs: nodejs/node#26936 (comment)
PR-URL: #2233
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented May 2, 2019

This was fixed by #27241.

@cjihrig cjihrig closed this as completed May 2, 2019
MylesBorins pushed a commit that referenced this issue May 16, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: #26936
Ref: libuv/libuv#2233

PR-URL: #26939
Refs: #26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this issue May 16, 2019
On macOS, fs.copyFile() may not respect file permissions. There is a PR
for libuv that should fix this, but until it lands and we can either
float a patch or upgrade libuv, have a known_issues test.

Ref: #26936
Ref: libuv/libuv#2233

PR-URL: #26939
Refs: #26936
Refs: libuv/libuv#2233
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
Development

No branches or pull requests

5 participants