Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions bin/jss.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,49 @@ program
.description('JavaScript Solid Server - A minimal, fast, JSON-LD native Solid server')
.version(pkg.version);

/**
* Convert a camelCase option name back to its kebab-case CLI form for
* error messages (`singleUserName` → `single-user-name`).
*/
function camelToKebab (name) {
return name.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase();
}

/**
* Reject any option value that looks like another flag (#103).
*
* Commander happily consumes the next argv as a value, so
* `jss start --single-user-name --idp`
* silently sets `singleUserName="--idp"` and the IdP flag is lost.
* This validator runs as a `preAction` hook for every subcommand, so
* any option with a missing value gets a clear error instead of a
* confusing downstream failure ("issuer has no registration endpoint",
* "Single-user: --idp" in the banner, etc.).
*/
program.hook('preAction', (_thisCommand, actionCommand) => {
const opts = actionCommand.opts();
for (const [key, value] of Object.entries(opts)) {
const flag = camelToKebab(key);
if (typeof value === 'string' && value.startsWith('--')) {
console.error(
`Error: --${flag} value "${value}" looks like a flag, not a value.\n` +
`Hint: did you forget to provide a value? e.g. --${flag} someValue`
);
process.exit(1);
}
// Numeric options (parseInt-coerced like --port) silently produce
// NaN when given a flag like `--idp`. Catch that too — same root
// cause, different surface.
if (typeof value === 'number' && Number.isNaN(value)) {
console.error(
`Error: --${flag} got a non-numeric value (parsed as NaN).\n` +
`Hint: did you forget to provide a number? e.g. --${flag} 8080`
);
process.exit(1);
}
}
});

/**
* Start command
*/
Expand Down
88 changes: 88 additions & 0 deletions test/cli-flag-like-values.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/**
* Regression tests for #103 — `bin/jss.js` must reject option values
* that look like flags (e.g. `--single-user-name --idp`) instead of
* silently using `--idp` as the username and breaking IdP setup.
*
* These spawn the CLI as a subprocess and assert exit-code + stderr.
*/

import { describe, it } from 'node:test';
import assert from 'node:assert';
import { spawnSync } from 'node:child_process';
import os from 'node:os';
import path from 'node:path';
import { fileURLToPath } from 'node:url';

const __dirname = path.dirname(fileURLToPath(import.meta.url));
const BIN = path.join(__dirname, '..', 'bin', 'jss.js');

// Timeout cap so the suite can never hang if the validator regresses
// and `start` actually tries to bind a port instead of exiting early.
const RUN_TIMEOUT_MS = 10_000;

function runCli(args) {
const r = spawnSync(process.execPath, [BIN, ...args], {
encoding: 'utf8',
timeout: RUN_TIMEOUT_MS,
killSignal: 'SIGKILL'
});
// spawnSync sets `signal` to the kill signal when timeout fires. Treat
// that as a hard test failure rather than letting downstream
// assertions on stderr accidentally pass.
assert.strictEqual(
r.signal, null,
`CLI did not exit within ${RUN_TIMEOUT_MS}ms — likely the preAction ` +
`validator regressed and \`start\` is actually trying to listen. ` +
`args: ${JSON.stringify(args)}; partial stderr: ${r.stderr}`
);
return r;
}

describe('bin/jss.js — flag-like option values (#103)', () => {
it('rejects `--single-user-name --idp` with a clear error', () => {
const r = runCli(['start', '--single-user-name', '--idp']);
assert.notStrictEqual(r.status, 0, 'exit code should be non-zero');
assert.match(r.stderr, /--single-user-name value "--idp" looks like a flag/);
assert.match(r.stderr, /Hint: did you forget to provide a value\?/);
});

it('rejects another option swallowing a flag (covers --idp-issuer too)', () => {
// Commander's behaviour: it greedily consumes the next argv as the
// value, which is the whole reason the bug exists. We use a flag
// commander doesn't know about so commander doesn't reroute it
// through its own argument-count error. The dummy name is
// collision-proof — if anyone ever adds a real `--bogus-...` flag
// matching this pattern, the duplication is the bigger problem.
const FAKE_FLAG = '--__jss103_unlikely_cli_option__';
const r = runCli(['start', '--idp-issuer', FAKE_FLAG]);
assert.notStrictEqual(r.status, 0);
assert.match(
r.stderr,
new RegExp(`--idp-issuer value "${FAKE_FLAG}" looks like a flag`)
);
});

it('rejects `--port --idp` (numeric option → NaN) with helpful error', () => {
const r = runCli(['start', '--port', '--idp']);
assert.notStrictEqual(r.status, 0);
assert.match(r.stderr, /--port got a non-numeric value/);
assert.match(r.stderr, /Hint: did you forget to provide a number\?/);
});

it('accepts a real value and reaches normal config processing', () => {
// --print-config exits 0 cleanly after dumping config; this proves
// the validator doesn't false-positive on legitimate values.
// Use os.tmpdir() rather than a hard-coded /tmp/... so the test is
// portable across platforms (and matches the rest of the suite).
const tmpRoot = path.join(os.tmpdir(), 'jss-103-sanity-doesnotneedtoexist');
const r = runCli(['start',
'--port', '4582',
'--root', tmpRoot,
'--single-user-name', 'alice',
Comment on lines +78 to +81
'--print-config'
]);
assert.strictEqual(r.status, 0,
`expected clean exit, got ${r.status}; stderr: ${r.stderr}`);
assert.match(r.stdout, /Configuration:/);
});
});