Fix #1: Fix windows required permissions for .env #2
Conversation
|
I tried to add some tests to verify this behavior. I cannot run the tests suite right now as I'm on windows and all the prepare |
index.js
Outdated
| @@ -61,7 +67,10 @@ const envy = (input) => { | |||
| return filterObj(camelizedGlobalEnv, camelizedExampleEnvKeys); | |||
| } | |||
|
|
|||
| if (checkMode(envPath, permissionMask) !== ownerReadWrite) { | |||
| if (isWindows() && checkMode(envPath, permissionMask) !== windowsPermission) { | |||
| throw new Error(`File permissions are unsafe. Make them 666 '${envPath}'`); | |||
There was a problem hiding this comment.
Make them 666
But the closest you can make them is 555, right? And if you somehow found a way to make them 666, this would fail anyway.
Also, is there a way to make them 555 from the command line on Windows? Would be nice to tell the user how to fix it. From what I read, maybe icacls could do it.
An alternative approach we could take here would be:
if (checkMode(envPath, num.S_IWGRP | num.S_IWOTH) === 0) {
throw new Error('...');
}That asserts that the writable bits for the "group" and "others" are off, as opposed to checking that all readable and executable bits are on and all writable bits are off.
There was a problem hiding this comment.
I'm not aware of a command line way. I've been doing it from windows configuration panels but indeed it would be nice to do it with the command line. I wonder if you can call that command from bash or if you want to open Powershell or something similar.
There was a problem hiding this comment.
Do you want me to update the PR with the "group" and "others" write bits off?
package.json
Outdated
| @@ -31,7 +31,8 @@ | |||
| "dependencies": { | |||
| "camelcase": "^4.1.0", | |||
| "camelcase-keys": "^4.1.0", | |||
| "filter-obj": "^1.1.0" | |||
| "filter-obj": "^1.1.0", | |||
| "is-wsl": "1.1.0" | |||
There was a problem hiding this comment.
Indeed sorry, my default NPM settings are --save-exact. I'll change it.
There was a problem hiding this comment.
Yeah, I figured. I had that for a while, too. Gave it up because it is a false sense of security. It only locks down your top level dependencies. If any of those dependencies have semver ranges and a sub-dependency violates semver (or has other issues), you're probably still going to be exposed to it. Updates tend to be good, anyway. And when you really need to lock things down properly, there is shrinkwrap.
There was a problem hiding this comment.
Yeah I might reconsider it indeed.
test.js
Outdated
| fixture('unsafe-perm-windows-env-777'); | ||
| }, Error); | ||
| const filepath = path.join('fixture', 'unsafe-perm-env-640', '.env'); | ||
| t.is(err.message, `File permissions are unsafe. Fix: chmod 600 '${filepath}'`); |
There was a problem hiding this comment.
The Fix: part of this error message doesn't align with what is thrown in the Windows environment.
test.js
Outdated
| const err = t.throws(() => { | ||
| fixture('unsafe-perm-windows-env-777'); | ||
| }, Error); | ||
| const filepath = path.join('fixture', 'unsafe-perm-env-640', '.env'); |
There was a problem hiding this comment.
The filepath here doesn't match the fixture that was actually used.
There was a problem hiding this comment.
Oups sorry, went a bit too quick on this one and the previous one as well.
|
Thanks, @Nargonath! ❤️ Windows support has been released. |
No description provided.