Skip to content

fix: cannot set mode to false #51

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

Closed
wants to merge 1 commit into from
Closed

fix: cannot set mode to false #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ function writeFileSync (filename, data, options) {
const removeOnExitHandler = onExit(cleanup)

try {
fd = fs.openSync(tmpfile, 'w', options.mode)
fd = fs.openSync(tmpfile, 'w', options.mode || 0o666)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default should be tighter than 666 which will grant write ability to everyone on the system. A better alternative is 644, which grants read-only to everyone on the box and read-write to the owner of the file.

Suggested change
fd = fs.openSync(tmpfile, 'w', options.mode || 0o666)
fd = fs.openSync(tmpfile, 'w', options.mode || 0o644)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because this value before umask — so, 666 will be equal to 644 (the same as 777 for dir will be equal to 755). That's why 666 it is default value not only in NodeJS, but also in Go.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see. In that case then it's fine :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.cyberciti.biz/tips/understanding-linux-unix-umask-value-usage.html

files are created with the access mode 666 and directories with 777

The default umask 002 used for normal user. With this mask default directory permissions are 775 and default file permissions are 664.

Copy link
Contributor Author

@develar develar Jul 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it — you request to remove "write" permission for group (644 vs 664). Hmm.... yes, probably makes sense, but... if options.mode is set to false, it means according to docs that

chown from being ran you can also pass false, in which case the file will be created with the current user's credentials.

and also means, that write-file-atomic should not affect defaults in any way. So, I still think that we should not specify any custom value and obey defaults — NodeJS uses 666 by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeach... according to my experience, file mode is not a way to set file mode — it heavily depends on system umask, so, you are:

  • or need to deal somehow with umask (pre-calculate and so on) and hope that it will work
  • or simply specify some default mode and then use chmod directly to set wanted permissions (as it is currently implemented — reliable way).

As the current problem is that our custom logic (additional chown/chmod) conflicts with snap sandboxing rules (as result, app cannot start correctly), and not security, we just need to use defaults and do not do more than we need. In this case security is not even to be considered — since it is snap daemon responsibility to care about security (and dir is sandboxed anyway). write-file-atomic just need to write file atomic :)

(yes, I know, that directory permissions doesn't prevent writing to file and I am not security expert, but purpose of this PR is not to change security of created files, but give an ability to disable custom chmod/chown for whatever reason client of this module can have).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0o666 is the appropriate default here. If the user has set their umask to 0o2 instead of 0o22, then we must assume there's some kind of reason for that (ie, that they wish files to be group-writable by default, which is not uncommon, and not unreasonable in many contexts). Same for 0o777 for directories.

Node.js programs can alter the umask by calling process.umask().

if (options.tmpfileCreated) {
options.tmpfileCreated(tmpfile)
}
Expand Down