Skip to content

Commit 27e301f

Browse files
fix: address Copilot review 4 on PR #7
Four follow-ups from the fourth pass: 1. Validate --port: reject non-numeric, out-of-range (<1, >65535), and trailing-garbage values at parse time. parseInt('abc') was silently producing NaN, which then propagated to JSS as `--port NaN` and crashed the server with a confusing error. 2. Validate the persisted JWT secret on read. If <root>/.token-secret exists but is empty / whitespace-only / too short (<32 chars), warn and regenerate rather than silently weakening signing. Empty file could happen via a botched edit, a failed `cp`, or a partial write. 3. Differentiate the LAN-exposure warning based on whether the password is env-supplied. Previously the warning always claimed the server was exposing "well-known me/me credentials," even when the user had supplied a strong password via env — a false statement and a confusing one. Now: default rung-1 keeps the original well-known-credentials wording; env-supplied gets a more neutral "exposes single-user sign-in" warning that suggests strong-password discipline and HTTPS for production. 4. Stop re-exposing the env-supplied password on the JSS subprocess argv. `ps`, service-manager logs, and other local users could read what we'd carefully hidden from the banner. The fix: pass `--single-user-password` on argv only when it's the literal rung-1 placeholder ('me' has no secrecy property). For env- supplied passwords, JSS picks up JSS_SINGLE_USER_PASSWORD from process.env directly (already inherited from jspod's env spread). Verified via `ps` — JSS argv now: `--port ... --host ... --root ... --notifications --conneg --no-multiuser --single-user --idp`, no password. Refs #1 #3 #6
1 parent 8de8cbd commit 27e301f

1 file changed

Lines changed: 44 additions & 7 deletions

File tree

index.js

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,17 @@ for (let i = 0; i < args.length; i++) {
7272
const arg = args[i];
7373

7474
if (arg === '--port' || arg === '-p') {
75-
options.port = parseInt(requireValue(arg, args[++i]), 10);
75+
const raw = requireValue(arg, args[++i]);
76+
const parsed = parseInt(raw, 10);
77+
// Reject non-numeric / out-of-range / privileged ports. parseInt('abc')
78+
// returns NaN, which would silently propagate to JSS as `--port NaN`
79+
// and produce a confusing crash deep in the server.
80+
if (!Number.isInteger(parsed) || parsed < 1 || parsed > 65535 || String(parsed) !== raw.trim()) {
81+
console.error(chalk.red(`✗ Invalid port: ${raw}`));
82+
console.error(chalk.dim('Port must be an integer in the range 1-65535.'));
83+
process.exit(1);
84+
}
85+
options.port = parsed;
7686
} else if (arg === '--host' || arg === '-h') {
7787
// Strip optional brackets from IPv6 literals so a user-friendly
7888
// `--host [::1]` paste-in stays canonical. formatUrl re-adds the
@@ -156,7 +166,14 @@ function resolveTokenSecret(rootDir) {
156166
if (process.env.TOKEN_SECRET) return process.env.TOKEN_SECRET;
157167
const secretFile = join(rootDir, '.token-secret');
158168
if (existsSync(secretFile)) {
159-
return readFileSync(secretFile, 'utf8').trim();
169+
const loaded = readFileSync(secretFile, 'utf8').trim();
170+
// Guard against a truncated / empty / accidentally-overwritten
171+
// secret file. A short-or-empty secret would silently weaken JWT
172+
// signing — regenerate and warn rather than ship the bad value.
173+
if (loaded.length >= 32) return loaded;
174+
console.warn(chalk.yellow(
175+
`⚠ ${secretFile} is empty or too short (${loaded.length} chars); regenerating.`
176+
));
160177
}
161178
const secret = randomBytes(48).toString('base64');
162179
writeFileSync(secretFile, secret, { mode: 0o600 });
@@ -221,10 +238,21 @@ if (options.auth && !options.multiuser) {
221238
/^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(options.host) ||
222239
options.host === '::1';
223240
if (!isLoopback) {
224-
console.log('\n' + chalk.bold.red('⚠ Warning: ') + chalk.yellow(
225-
`--host ${options.host} exposes the well-known me/me credentials beyond localhost.`
226-
));
227-
console.log(chalk.dim(' Set JSS_SINGLE_USER_PASSWORD=... before running, or bind to 127.0.0.1.'));
241+
if (RUNG_1_PASSWORD_FROM_ENV) {
242+
// Custom password from env. Still worth warning the user that
243+
// their sign-in is now reachable from anywhere this host
244+
// answers, but no longer accurate to call the credentials
245+
// "well-known."
246+
console.log('\n' + chalk.bold.red('⚠ Warning: ') + chalk.yellow(
247+
`--host ${options.host} exposes single-user sign-in beyond localhost.`
248+
));
249+
console.log(chalk.dim(' Make sure your JSS_SINGLE_USER_PASSWORD is strong, and use HTTPS in production.'));
250+
} else {
251+
console.log('\n' + chalk.bold.red('⚠ Warning: ') + chalk.yellow(
252+
`--host ${options.host} exposes the well-known me/me credentials beyond localhost.`
253+
));
254+
console.log(chalk.dim(' Set JSS_SINGLE_USER_PASSWORD=... before running, or bind to 127.0.0.1.'));
255+
}
228256
}
229257
}
230258

@@ -263,7 +291,16 @@ if (options.multiuser) {
263291
// every subsequent start is a no-op (JSS is idempotent on the seed).
264292
jssArgs.push('--no-multiuser', '--single-user');
265293
if (options.auth) {
266-
jssArgs.push('--idp', '--single-user-password', RUNG_1_PASSWORD);
294+
jssArgs.push('--idp');
295+
// Pass the rung-1 placeholder on argv (it has no secrecy property
296+
// — anyone reading the docs already knows the literal 'me'). For
297+
// an env-supplied password, *don't* re-expose it on argv where
298+
// `ps`, service-manager logs, and other local users can read it.
299+
// JSS reads JSS_SINGLE_USER_PASSWORD from env directly when no
300+
// CLI flag is given, and we forward process.env to the child.
301+
if (!RUNG_1_PASSWORD_FROM_ENV) {
302+
jssArgs.push('--single-user-password', RUNG_1_PASSWORD);
303+
}
267304
}
268305
}
269306

0 commit comments

Comments
 (0)