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.copySync fails if user does not own file #199

Closed
bdcribbs opened this issue Dec 8, 2015 · 13 comments · Fixed by #301
Closed

fs.copySync fails if user does not own file #199

bdcribbs opened this issue Dec 8, 2015 · 13 comments · Fixed by #301
Assignees
Milestone

Comments

@bdcribbs
Copy link

bdcribbs commented Dec 8, 2015

fs.copySync fails when the user running the script does not own the destination file, even when the user actually has sufficient permissions to perform the action.

The error occurs if the file exists, and clobber is true. In that case, copySync attempts to chmod the file prior to unlinking it (prior to performing the copy). Since the user doesn't own the file, the chmod fails with EPERM, even though the operation could have succeeded -- provided the user has sufficient permissions to unlink the file and recreate it (which depends on the permissions of the directory the file is in -- not the file itself).

The line in question is here:
https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy-sync/copy-file-sync.js#L12

bdcribbs referenced this issue Dec 8, 2015
@jprichardson
Copy link
Owner

Thanks so much! Would you be willing to add a test?

@bdcribbs
Copy link
Author

I'm not sure how we'd write such a test, since it requires creating a file as one user account and then attempting to manipulate it with another user account.

@jprichardson
Copy link
Owner

Good point. Maybe throw that line in a try/catch block?

@jprichardson jprichardson added this to the 1.0 milestone Dec 17, 2015
@bdcribbs
Copy link
Author

A try/catch would work. Another point of view is that attempting to copy over the contents of a read-only file really should fail, because that's what read-only means.

@jprichardson
Copy link
Owner

Another point of view is that attempting to copy over the contents of a read-only file really should fail, because that's what read-only means.

Hah. Excellent point as well.

So for a test, couldn't we just set a couple of files to be read only and try to copy over them and assert for an error? The error should have a descriptive message indicating that the destination is read-only.

@bdcribbs
Copy link
Author

That works, and also matches how I would expect it to function (I was somewhat surprised when I found out copySync was calling chmod in the first place).

It's worth considering that this would revert the change that was originally made for #183, but in my opinion, it's more appropriate for the caller to chmod the file themselves if they need to.

If you'd like I could throw together a pull request, but I didn't want to revert #183 without your agreement.

@jprichardson
Copy link
Owner

@bdcribbs thanks for referencing the issue. In context it makes more sense. Don't you think that when clobber (aka overwrite) is set to true, it should overwrite regardless of whether the file is read only?

Pinging: @bartland for his feedback.

@bdcribbs
Copy link
Author

I see. I thought clobber was for overwriting files that already exist (EEXIST), not EPERM. Now the behavior makes more sense.

Since there are actually two concerns here, how about two options instead one? "clobberExisting" and "clobberReadonly". That way, there is zero ambiguity about what each option does.

For backwards compatibility, "clobber" could be equivalent to setting both of the new options to true.

@bartland
Copy link
Contributor

Seems a reasonable idea for more fine grained control given we would still have "clobber" for both.

In regard to #183, the reason for introducing the chmod call was to support node 0.10 (bug?). If support for 0.10 was dropped, the chmod call could be dropped. Which is what I'd prefer.

@jprichardson
Copy link
Owner

Since there are actually two concerns here, how about two options instead one? "clobberExisting" and "clobberReadonly". That way, there is zero ambiguity about what each option does.

Yes, I like this idea.

In regard to #183, the reason for introducing the chmod call was to support node 0.10 (bug?). If support for 0.10 was dropped, the chmod call could be dropped. Which is what I'd prefer.

I see. That makes sense. I'll be dropping support for Node v0.10 in April 2016. At that time, the next Ubuntu LTS will be released. Ubuntu LTS v0.14 defaults to Node v0.10.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 27, 2016

@jprichardson Shall I try removing the chmod call?

@jprichardson
Copy link
Owner

@jprichardson Shall I try removing the chmod call?

Yep, sounds good.

@purepear
Copy link

purepear commented Jan 4, 2018

I have the same issue with copy. Node user have permissions to copy the file to a different user's directory but fs.copy then fails to chmod the copied file and the whole thing fails Error: ENOENT: no such file or directory, chmod /some-direcotry/file.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants