Skip to content

Commit b9801af

Browse files
melvincarvalhomelvincarvalho
committed
Address Copilot review on JavaScriptSolidServer#281
1. Race-safe write. Persisted-secret write now uses an exclusive flag ('wx'); on EEXIST we re-read the file. Two JSS processes starting at the same time converge on the same secret rather than clobbering. 2. Enforce permissions on pre-existing dir/file. mkdirSync({mode}) and writeFileSync({mode}) only apply on creation. Chmod dir to 0700 and file to 0600 on every call, best-effort (swallowing EPERM/ENOTSUP on Windows). 3. Production error message now references path.dirname(secretPath) rather than a hardcoded "~/.jss/", matching the injectable path. Adds three test cases: recovery from an empty pre-existing file, perm tightening on a looser pre-existing layout, and the error-message pointing at the actual directory.
1 parent bd6973c commit b9801af

2 files changed

Lines changed: 87 additions & 6 deletions

File tree

‎src/auth/token-secret.js‎

Lines changed: 50 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,22 +13,66 @@ import path from 'path';
1313

1414
export const DEFAULT_SECRET_PATH = path.join(os.homedir(), '.jss', 'token.secret');
1515

16+
// Tighten permissions on POSIX. No-op on Windows, which uses ACLs and
17+
// surfaces EPERM on chmod — swallow that specifically.
18+
function chmodBestEffort(target, mode) {
19+
try {
20+
fs.chmodSync(target, mode);
21+
} catch (e) {
22+
if (e.code !== 'EPERM' && e.code !== 'ENOTSUP') throw e;
23+
}
24+
}
25+
1626
/**
1727
* Read a persisted secret from `filePath`, or generate one and write it
18-
* (with dir mode 0700 and file mode 0600) if the file is missing.
19-
* Anything other than ENOENT (permission denied, read-only FS, …) throws.
28+
* (with dir mode 0700 and file mode 0600) if the file is missing. Safe
29+
* against multiple JSS processes starting in parallel: the write uses an
30+
* exclusive flag and we re-read on EEXIST, so every process converges on
31+
* the same secret rather than racing.
32+
*
33+
* Anything other than ENOENT on read (permission denied, read-only FS, …)
34+
* throws.
2035
*/
2136
export function readOrWritePersistedSecret(filePath = DEFAULT_SECRET_PATH) {
37+
// Always ensure the dir exists and is 0700 — enforce perms on every call,
38+
// not only when we're the one creating it. mkdirSync({mode}) is only
39+
// applied on creation, so an existing dir with looser mode needs chmod.
40+
const dir = path.dirname(filePath);
41+
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
42+
chmodBestEffort(dir, 0o700);
43+
44+
// Fast path: file already exists and is non-empty.
2245
try {
2346
const existing = fs.readFileSync(filePath, 'utf8').trim();
24-
if (existing) return existing;
47+
if (existing) {
48+
chmodBestEffort(filePath, 0o600);
49+
return existing;
50+
}
2551
} catch (e) {
2652
if (e.code !== 'ENOENT') throw e;
2753
}
54+
2855
const generated = crypto.randomBytes(32).toString('hex');
29-
fs.mkdirSync(path.dirname(filePath), { recursive: true, mode: 0o700 });
30-
// mode is honoured on POSIX; silently ignored on Windows (uses ACLs).
56+
try {
57+
// Exclusive create — concurrent processes can't overwrite each other's
58+
// freshly generated secrets. mode is honoured on POSIX; Windows ignores
59+
// it (uses ACLs).
60+
fs.writeFileSync(filePath, generated, { mode: 0o600, flag: 'wx' });
61+
chmodBestEffort(filePath, 0o600);
62+
return generated;
63+
} catch (e) {
64+
if (e.code !== 'EEXIST') throw e;
65+
}
66+
67+
// Another process won the race — read what they wrote.
68+
const existing = fs.readFileSync(filePath, 'utf8').trim();
69+
if (existing) {
70+
chmodBestEffort(filePath, 0o600);
71+
return existing;
72+
}
73+
// Same fallback as before for a pre-existing empty file.
3174
fs.writeFileSync(filePath, generated, { mode: 0o600 });
75+
chmodBestEffort(filePath, 0o600);
3276
return generated;
3377
}
3478

@@ -57,7 +101,7 @@ export function resolveTokenSecret({
57101
} catch (e) {
58102
if (env.NODE_ENV === 'production') {
59103
log.error(`SECURITY ERROR: TOKEN_SECRET not set and ${secretPath} is not writable (${e.message}).`);
60-
log.error('Set TOKEN_SECRET explicitly, or grant write access to ~/.jss/.');
104+
log.error(`Set TOKEN_SECRET explicitly, or grant write access to ${path.dirname(secretPath)}.`);
61105
exit(1);
62106
return undefined; // for tests that stub `exit`
63107
}

‎test/token-secret.test.js‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,27 @@ describe('readOrWritePersistedSecret', () => {
5656
const unwritable = path.join(blockerFile, '.jss', 'token.secret');
5757
assert.throws(() => readOrWritePersistedSecret(unwritable));
5858
});
59+
60+
it('recovers when the secret file already exists but is empty', () => {
61+
// Simulates the lose-a-race case: another process created the file
62+
// between our read and our write. Exclusive-create fails EEXIST and
63+
// we fall back to reading / (if empty) writing without wx.
64+
const p = path.join(tmpDir, 'empty', '.jss', 'token.secret');
65+
fs.mkdirSync(path.dirname(p), { recursive: true });
66+
fs.writeFileSync(p, '');
67+
const s = readOrWritePersistedSecret(p);
68+
assert.strictEqual(s.length, 64);
69+
assert.strictEqual(fs.readFileSync(p, 'utf8').trim(), s);
70+
});
71+
72+
it('tightens permissions when the file already exists with loose mode', { skip: process.platform === 'win32' }, () => {
73+
const p = path.join(tmpDir, 'loose', '.jss', 'token.secret');
74+
fs.mkdirSync(path.dirname(p), { recursive: true, mode: 0o755 });
75+
fs.writeFileSync(p, 'a'.repeat(64), { mode: 0o644 });
76+
readOrWritePersistedSecret(p);
77+
assert.strictEqual(fs.statSync(p).mode & 0o777, 0o600);
78+
assert.strictEqual(fs.statSync(path.dirname(p)).mode & 0o777, 0o700);
79+
});
5980
});
6081

6182
describe('resolveTokenSecret', () => {
@@ -114,6 +135,22 @@ describe('resolveTokenSecret', () => {
114135
assert.strictEqual(exitCode, 1);
115136
});
116137

138+
it('production error message references the actual secret directory', () => {
139+
const secretPath = buildUnwritable('custom-path');
140+
const errors = [];
141+
resolveTokenSecret({
142+
env: { NODE_ENV: 'production' },
143+
secretPath,
144+
log: { warn: () => {}, error: (msg) => errors.push(msg) },
145+
exit: () => {},
146+
});
147+
// Guidance line should point at the dirname we actually tried to write.
148+
assert.ok(
149+
errors.some(m => m.includes(path.dirname(secretPath))),
150+
`expected an error to mention ${path.dirname(secretPath)}, got: ${errors.join(' | ')}`
151+
);
152+
});
153+
117154
it('falls back to an ephemeral secret outside production when persistence fails', () => {
118155
const s = resolveTokenSecret({
119156
env: {},

0 commit comments

Comments
 (0)