Skip to content

Commit 28471d0

Browse files
melvincarvalhomelvincarvalho
committed
Address second Copilot round on JavaScriptSolidServer#281
1. Read-first. Previous revision did mkdirSync + chmodSync on the parent dir unconditionally at entry, so a pre-provisioned secret on a read-only filesystem would make startup fail. Now the fast path reads the existing file and returns it before touching the dir at all; chmod of the dir/file runs best-effort on that path so perm-tighten failures can't block using a valid secret. 2. Atomic write via temp + rename. writeFileSync(flag: 'wx') created the file before writing content, so another process hitting EEXIST could observe an empty file. Switch to writing a per-process temp file and renameSync'ing it into place, which on POSIX is atomic — concurrent readers always see either the old content or the new complete content, never a half-written file. 3. Throw after exit() in production. exit is injectable; if a caller stubbed it (e.g. tests) the old code returned undefined and let downstream use an invalid secret. Now we still call exit(1) for real production processes, then raise so a stubbed exit can't leak. 4. chmodBestEffort now swallows all chmod errors (not just EPERM/ENOTSUP) — perm tightening is defensive, never load-bearing. Adds two tests: throw-after-stubbed-exit (regression guard for JavaScriptSolidServer#3) and read-from-a-not-writable-parent-dir (regression guard for JavaScriptSolidServer#1).
1 parent b9801af commit 28471d0

2 files changed

Lines changed: 88 additions & 45 deletions

File tree

‎src/auth/token-secret.js‎

Lines changed: 41 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,67 +13,72 @@ 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.
16+
// Tighten permissions on POSIX, best-effort. No-op on Windows (ACLs) and
17+
// on read-only filesystems — we never want perm-tightening to block using
18+
// an otherwise-valid secret.
1819
function chmodBestEffort(target, mode) {
1920
try {
2021
fs.chmodSync(target, mode);
21-
} catch (e) {
22-
if (e.code !== 'EPERM' && e.code !== 'ENOTSUP') throw e;
22+
} catch {
23+
// Intentionally swallow — perms are defensive hardening, not required.
2324
}
2425
}
2526

2627
/**
2728
* Read a persisted secret from `filePath`, or generate one and write it
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.
29+
* (with dir mode 0700 and file mode 0600) if the file is missing.
30+
*
31+
* Read-first: if the file already exists and is non-empty we return it
32+
* without trying to mkdir or tighten the containing directory. Deployments
33+
* with a pre-provisioned secret on a read-only filesystem boot cleanly.
34+
*
35+
* Concurrent-startup safe: new secrets are written to a per-process temp
36+
* file in the same directory and `renameSync`'d into place, so another
37+
* process reading the target never sees a half-written file. If a peer
38+
* process won the rename we fall back to reading their value.
3239
*
33-
* Anything other than ENOENT on read (permission denied, read-only FS, …)
34-
* throws.
40+
* Anything other than ENOENT on the initial read (permission denied,
41+
* corrupt FS, …) propagates.
3542
*/
3643
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.
4044
const dir = path.dirname(filePath);
41-
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
42-
chmodBestEffort(dir, 0o700);
4345

44-
// Fast path: file already exists and is non-empty.
46+
// Fast path: pre-existing non-empty file. No mkdir/chmod attempt on the
47+
// parent dir here — a read-only FS must not fail this path.
4548
try {
4649
const existing = fs.readFileSync(filePath, 'utf8').trim();
4750
if (existing) {
51+
chmodBestEffort(dir, 0o700);
4852
chmodBestEffort(filePath, 0o600);
4953
return existing;
5054
}
5155
} catch (e) {
5256
if (e.code !== 'ENOENT') throw e;
5357
}
5458

59+
// Slow path: create it. Only touch the FS with writes from here on.
60+
fs.mkdirSync(dir, { recursive: true, mode: 0o700 });
61+
chmodBestEffort(dir, 0o700);
62+
5563
const generated = crypto.randomBytes(32).toString('hex');
64+
// Atomic write: fully write a temp file, then rename into place. On
65+
// POSIX the rename is atomic, so concurrent readers see either the old
66+
// content or the new complete content — never a half-written file.
67+
const tmpPath = `${filePath}.${crypto.randomBytes(8).toString('hex')}.tmp`;
5668
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;
69+
fs.writeFileSync(tmpPath, generated, { mode: 0o600 });
70+
fs.renameSync(tmpPath, filePath);
6371
} catch (e) {
64-
if (e.code !== 'EEXIST') throw e;
72+
try { fs.unlinkSync(tmpPath); } catch { /* ignore */ }
73+
throw e;
6574
}
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.
74-
fs.writeFileSync(filePath, generated, { mode: 0o600 });
7575
chmodBestEffort(filePath, 0o600);
76-
return generated;
76+
77+
// Multiple processes racing each produce a different secret; only the
78+
// last renamer's value sticks on disk. Re-read so every process ends up
79+
// using the winning secret and token verification stays consistent.
80+
const persisted = fs.readFileSync(filePath, 'utf8').trim();
81+
return persisted || generated;
7782
}
7883

7984
/**
@@ -103,7 +108,9 @@ export function resolveTokenSecret({
103108
log.error(`SECURITY ERROR: TOKEN_SECRET not set and ${secretPath} is not writable (${e.message}).`);
104109
log.error(`Set TOKEN_SECRET explicitly, or grant write access to ${path.dirname(secretPath)}.`);
105110
exit(1);
106-
return undefined; // for tests that stub `exit`
111+
// `exit` is injectable; if a caller stubs it out we must not silently
112+
// return undefined and let downstream code use an invalid secret.
113+
throw new Error(`Failed to resolve TOKEN_SECRET in production: ${e.message}`);
107114
}
108115
const ephemeral = crypto.randomBytes(32).toString('hex');
109116
log.warn(`WARNING: Could not persist TOKEN_SECRET (${e.message}). Using ephemeral secret; tokens will not survive restarts.`);

‎test/token-secret.test.js‎

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,22 @@ describe('readOrWritePersistedSecret', () => {
7777
assert.strictEqual(fs.statSync(p).mode & 0o777, 0o600);
7878
assert.strictEqual(fs.statSync(path.dirname(p)).mode & 0o777, 0o700);
7979
});
80+
81+
it('reads a pre-existing secret even when the parent dir is not writable', { skip: process.platform === 'win32' || process.getuid?.() === 0 }, () => {
82+
// Simulates a read-only deployment: secret provisioned ahead of time,
83+
// parent dir not writable for the current user. Must not block startup.
84+
const p = path.join(tmpDir, 'readonly-parent', '.jss', 'token.secret');
85+
fs.mkdirSync(path.dirname(p), { recursive: true, mode: 0o700 });
86+
const expected = 'b'.repeat(64);
87+
fs.writeFileSync(p, expected);
88+
fs.chmodSync(path.dirname(p), 0o500); // r-x, no write
89+
try {
90+
const s = readOrWritePersistedSecret(p);
91+
assert.strictEqual(s, expected);
92+
} finally {
93+
fs.chmodSync(path.dirname(p), 0o700); // let after()'s rmSync clean up
94+
}
95+
});
8096
});
8197

8298
describe('resolveTokenSecret', () => {
@@ -126,25 +142,45 @@ describe('resolveTokenSecret', () => {
126142

127143
it('hard-exits in production when persistence fails', () => {
128144
let exitCode;
129-
resolveTokenSecret({
130-
env: { NODE_ENV: 'production' },
131-
secretPath: buildUnwritable('prod'),
132-
log: silentLog,
133-
exit: (code) => { exitCode = code; },
145+
assert.throws(() => {
146+
resolveTokenSecret({
147+
env: { NODE_ENV: 'production' },
148+
secretPath: buildUnwritable('prod'),
149+
log: silentLog,
150+
exit: (code) => { exitCode = code; }, // stubbed — doesn't actually terminate
151+
});
134152
});
153+
// exit(1) must still have been invoked even though we throw afterwards,
154+
// so a non-stubbed production process actually terminates.
135155
assert.strictEqual(exitCode, 1);
136156
});
137157

158+
it('throws after exit so a stubbed exit() cannot leak undefined downstream', () => {
159+
// Regression: earlier versions returned undefined "for tests" after
160+
// calling exit(), which could let callers continue with an invalid
161+
// secret when exit is stubbed.
162+
assert.throws(
163+
() => resolveTokenSecret({
164+
env: { NODE_ENV: 'production' },
165+
secretPath: buildUnwritable('no-leak'),
166+
log: silentLog,
167+
exit: () => {},
168+
}),
169+
/TOKEN_SECRET/
170+
);
171+
});
172+
138173
it('production error message references the actual secret directory', () => {
139174
const secretPath = buildUnwritable('custom-path');
140175
const errors = [];
141-
resolveTokenSecret({
142-
env: { NODE_ENV: 'production' },
143-
secretPath,
144-
log: { warn: () => {}, error: (msg) => errors.push(msg) },
145-
exit: () => {},
176+
assert.throws(() => {
177+
resolveTokenSecret({
178+
env: { NODE_ENV: 'production' },
179+
secretPath,
180+
log: { warn: () => {}, error: (msg) => errors.push(msg) },
181+
exit: () => {},
182+
});
146183
});
147-
// Guidance line should point at the dirname we actually tried to write.
148184
assert.ok(
149185
errors.some(m => m.includes(path.dirname(secretPath))),
150186
`expected an error to mention ${path.dirname(secretPath)}, got: ${errors.join(' | ')}`

0 commit comments

Comments
 (0)