mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-07-27 16:49:51 -07:00
fix(sync): harden sync CLI, IPC, and UI paths from CodeRabbit review
- reject option-like tokens as flag values (--snapshot --force wrote a file named --force); --flag=-value still works - PowerShell remote quoting uses single-quoted literals so $() in a quoted path cannot expand - sync-hosts.json written via temp file + rename; a crash mid-write truncated it and the reader's corrupt-fallback dropped every host - cancelled sync child escalates SIGTERM -> SIGKILL after 5s grace - NDJSON progress events validated field-by-field before casting - snapshot filenames include milliseconds to avoid same-second overwrite - syncAutoScheduler.stop() wired into will-quit cleanup - sync --ui exclusivity also rejects --make-temp/--remove-temp/--json - document --sync-window in app help; group --make-temp/--remove-temp under modes in sync usage
This commit is contained in:
@@ -62,3 +62,20 @@ test('parseSyncCliTokens mirrors launcher sync validation', () => {
|
||||
assert.equal(parseSyncCliTokens(['sync', 'h', 'extra']).kind, 'error');
|
||||
assert.equal(parseSyncCliTokens(['sync', '--snapshot']).kind, 'error');
|
||||
});
|
||||
|
||||
test('parseSyncCliTokens rejects an option-like token where a value is required', () => {
|
||||
// Without this guard `--snapshot --force` writes a snapshot to a file literally
|
||||
// named "--force" and silently drops the flag.
|
||||
assert.deepEqual(parseSyncCliTokens(['sync', '--snapshot', '--force']), {
|
||||
kind: 'error',
|
||||
message: 'Missing value for --snapshot.',
|
||||
});
|
||||
assert.deepEqual(parseSyncCliTokens(['sync', '--remove-temp', '--force']), {
|
||||
kind: 'error',
|
||||
message: 'Missing value for --remove-temp.',
|
||||
});
|
||||
// The `--flag=<value>` form still accepts values that begin with "-".
|
||||
const parsed = parseSyncCliTokens(['sync', '--snapshot=-weird-name.sqlite']);
|
||||
assert.equal(parsed.kind, 'run');
|
||||
assert.equal(parsed.kind === 'run' && parsed.args.syncSnapshotPath, '-weird-name.sqlite');
|
||||
});
|
||||
|
||||
@@ -64,7 +64,10 @@ export function parseSyncCliTokens(tokens: readonly string[]): ParsedSyncCli {
|
||||
continue;
|
||||
}
|
||||
const value = rest[i + 1];
|
||||
if (value === undefined) {
|
||||
// An option-like token is never a value: `--snapshot --force` must fail
|
||||
// loudly instead of writing a snapshot to a file named "--force". Paths
|
||||
// that really do start with "-" can still be passed as `--snapshot=-x`.
|
||||
if (value === undefined || value.startsWith('-')) {
|
||||
return { kind: 'error', message: `Missing value for ${token}.` };
|
||||
}
|
||||
assignValue(value);
|
||||
@@ -144,6 +147,8 @@ export function syncCliUsage(): string {
|
||||
' <host> Sync stats with an SSH destination (user@host or ssh alias)',
|
||||
' --snapshot <file> Write a consistent snapshot of the local stats database',
|
||||
' --merge <file> Merge a snapshot database file into the local stats database',
|
||||
' --make-temp Create a sync temp directory and print its path (used over SSH)',
|
||||
' --remove-temp <dir> Remove a sync temp directory created by --make-temp',
|
||||
'',
|
||||
'Options:',
|
||||
' --push Only merge local stats into the SSH host',
|
||||
@@ -153,8 +158,6 @@ export function syncCliUsage(): string {
|
||||
' --remote-cmd <cmd> SubMiner app or launcher command to run on the remote host',
|
||||
' -f, --force Skip the running-app safety check',
|
||||
' --json Emit machine-readable NDJSON progress output',
|
||||
' --make-temp Create a sync temp directory and print its path (used over SSH)',
|
||||
' --remove-temp <dir> Remove a sync temp directory created by --make-temp',
|
||||
' --log-level <level> Log level',
|
||||
' --help Show this help',
|
||||
' --version Show the SubMiner version',
|
||||
|
||||
@@ -160,3 +160,17 @@ test('quoteForRemoteShell quotes per flavor and rejects unsafe Windows values',
|
||||
assert.throws(() => quoteForRemoteShell('windows-cmd', 'a"b'), /Refusing to quote/);
|
||||
assert.throws(() => quoteForRemoteShell('windows-powershell', 'a\nb'), /Refusing to quote/);
|
||||
});
|
||||
|
||||
test('quoteForRemoteShell does not let PowerShell expand a quoted value', () => {
|
||||
// PowerShell expands $(...) and $var inside double quotes, so a single-quoted
|
||||
// literal is the only safe form; '' is the escape for an embedded quote.
|
||||
assert.equal(
|
||||
quoteForRemoteShell('windows-powershell', 'C:/tmp/$(calc.exe)'),
|
||||
`'C:/tmp/$(calc.exe)'`,
|
||||
);
|
||||
assert.equal(quoteForRemoteShell('windows-powershell', "C:/tmp/it's"), `'C:/tmp/it''s'`);
|
||||
assert.equal(
|
||||
quoteForRemoteShell('windows-powershell', 'C:/Users/First Last/Temp/subminer-sync-ab'),
|
||||
`'C:/Users/First Last/Temp/subminer-sync-ab'`,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -100,15 +100,21 @@ export function detectRemoteShellFlavor(
|
||||
}
|
||||
|
||||
/**
|
||||
* Quote one argument for the detected remote shell. Windows shells have no
|
||||
* safe single-quote escaping (cmd treats ' literally; PowerShell and cmd
|
||||
* disagree on embedded double quotes), so quote-containing values are
|
||||
* rejected instead of escaped — sync only ever quotes paths it composed.
|
||||
* Quote one argument for the detected remote shell. PowerShell expands $(...)
|
||||
* and $var inside double quotes, so it gets a single-quoted literal ('' escapes
|
||||
* a quote). cmd.exe has no single-quote form and treats ' literally, so it keeps
|
||||
* double quotes and rejects values carrying a double quote of their own.
|
||||
*/
|
||||
export function quoteForRemoteShell(flavor: RemoteShellFlavor, value: string): string {
|
||||
if (flavor === 'posix') return shellQuote(value);
|
||||
if (value.includes('"') || /[\r\n]/.test(value)) {
|
||||
throw new Error(`Refusing to quote a value with quotes or newlines for a Windows shell: ${value}`);
|
||||
if (/[\r\n]/.test(value)) {
|
||||
throw new Error(`Refusing to quote a value with newlines for a Windows shell: ${value}`);
|
||||
}
|
||||
if (flavor === 'windows-powershell') {
|
||||
return `'${value.replaceAll("'", "''")}'`;
|
||||
}
|
||||
if (value.includes('"')) {
|
||||
throw new Error(`Refusing to quote a value with quotes for a Windows shell: ${value}`);
|
||||
}
|
||||
return `"${value}"`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user