search

pinojs / sonic-boom

Extremely fast utf8 only stream implementation
MIT License
261 stars 41 forks source link

Allow users to specify the creating file mode #129

Closed mcollina closed 2 years ago

mcollina commented 2 years ago

Files created by sonic-boom and pino have the default of 0o666 as every other file created by Node.js: https://nodejs.org/api/fs.html#fsopenpath-flags-mode-callback.

We should support making this configurable.

jsumners commented 2 years ago

This test demonstrates how passing a mode doesn't always change to the expected permissions.

Testing fs.open in my local machine I was getting identical results.

On the other hand testing with fs.chmod changed the file permissions as expected, , so this seems to be the way fs.open works at the moment. https://github.com/pinojs/sonic-boom/pull/130#pullrequestreview-852853281

Given that setting permissions with fs.open seems non-deterministic, and fs.chmod means nothing in Windows (https://nodejs.org/dist/latest-v16.x/docs/api/fs.html#filehandlechmodmode), I vote we do not attempt to implement this. It puts us in the realm of being system administrators by trying to handle all of the various scenarios that can be devised around file permissions.

Instead, we should document that the use should take care to run their processes as unprivileged users and that any files created should be created in locations where the parent tree has appropriate permissions to deny access to other users.

mcollina commented 2 years ago

Given that setting permissions with fs.open seems non-deterministic

Why is it non-deterministic?

jsumners commented 2 years ago

Why is it non-deterministic?

The starting sentence of that quote:

This test demonstrates how passing a mode doesn't always change to the expected permissions.

If telling fs.open to set a mode does not always set that mode, then what is the point?

mcollina commented 2 years ago

If telling fs.open to set a mode does not always set that mode, then what is the point?

That's not correct for that test. The problem that is facing is that different OS run their processes with different umask by default and that specific test is getting filtered by the umask.

jsumners commented 2 years ago

I think that is further support for my argument.

mcollina commented 2 years ago

Passing the option allows people to do what they like. It's a pass-through parameter to Node.js core for us and it should be documented as such. If people want to use it, that's up to them.