security: close cron + SQL-ident injection paths in backup/db tools

Two findings from the security review, both require adversarial input to
reach (MCP caller controls the args), but the blast radius exceeds what
the tool is supposed to allow:

1. **ssh_backup_schedule: cron injection via newline**
   The cron validator counted whitespace-separated tokens, so a value like
   "0 0 * * *\n* * * * * rm -rf ~" passed. shQuote preserves newlines
   inside single-quoted strings, so `printf '%s\n' ${shQuote(cronLine)}`
   installed a multi-line crontab. Fix: reject cron with any \r\n\t and
   any shell metacharacter ($, `), then split on spaces only.

2. **ssh_db_{query,list,dump,import}: SQL ident injection via database/user**
   `database` and `user` were shQuote'd for the shell but interpolated into
   SQL strings like `SHOW TABLES FROM 'name'` and `pg_database_size('name')`.
   A database name of `app'; DROP DATABASE x; --` shell-unquotes to a valid
   SQL injection payload. Fix: validate database/user against
   [A-Za-z0-9_][A-Za-z0-9_.-]{0,63} (the conservative intersection of
   MySQL/PG/Mongo identifier rules) at handler entry.

+4 regression tests (651 -> 653 passing). No existing behavior changed for
well-formed inputs.

Author: hunchom

src/tools/backup-tools.js

@@ -647,9 +647,30 @@ export async function handleSshBackupSchedule({ getConnection, args }) {
   if (!VALID_TYPES.has(backup_type)) {
     return toMcp(fail('ssh_backup_schedule', `invalid backup_type: ${backup_type}`, { server }), { format });
   }
-  if (!cron || typeof cron !== 'string' || cron.trim().split(/\s+/).length < 5) {
+  if (!cron || typeof cron !== 'string') {
     return toMcp(fail('ssh_backup_schedule', 'cron is required (e.g. "0 2 * * *")', { server }), { format });
   }
+  const cronTrimmed = cron.trim();
+  // Reject embedded newlines/CR/tab -- cron must be a single line. Without this,
+  // a caller could smuggle extra crontab entries via "0 0 * * *\nMALICIOUS\n".
+  if (/[\r\n\t]/.test(cronTrimmed)) {
+    return toMcp(fail('ssh_backup_schedule',
+      'cron must be a single line (no newlines/tabs)', { server }), { format });
+  }
+  // Reject anything that looks like it came through a shell expansion break
+  // (backticks, $() etc) even though shQuote would neutralize them --
+  // defense in depth + clearer UX on malformed input.
+  if (/[`$]/.test(cronTrimmed)) {
+    return toMcp(fail('ssh_backup_schedule',
+      'cron cannot contain shell metacharacters ($, `)', { server }), { format });
+  }
+  // A cron expression is 5 (or 6 with seconds) whitespace-separated time fields.
+  // Split on literal space only -- newlines were already rejected above.
+  const fields = cronTrimmed.split(/ +/);
+  if (fields.length < 5) {
+    return toMcp(fail('ssh_backup_schedule',
+      'cron must have at least 5 time fields (e.g. "0 2 * * *")', { server }), { format });
+  }
 
   // Refuse to schedule if a password was passed -- writing it into crontab
   // would persist the secret in plaintext (readable to anyone who gains the
@@ -696,14 +717,14 @@ export async function handleSshBackupSchedule({ getConnection, args }) {
   // job can execute correctly; the URI encodes db/host/port, not credentials.
   const fullCmd = (cmdBundle.envPrefix || '') + cmdBundle.command;
   const marker = `# claude-code-ssh-backup:${backup_type}:${targetName}`;
-  const cronLine = `${cron.trim()} ${fullCmd} ${marker}`;
+  const cronLine = `${cronTrimmed} ${fullCmd} ${marker}`;
 
   if (isPreview) {
     const plan = buildPlan({
       action: 'backup-schedule',
       target: `${server}:crontab`,
       effects: [
-        `cron: \`${cron.trim()}\``,
+        `cron: \`${cronTrimmed}\``,
         `backup_type: \`${backup_type}\``,
         backup_type === 'files'
           ? `paths: ${(paths || []).map(p => `\`${p}\``).join(', ')}`
@@ -745,7 +766,7 @@ export async function handleSshBackupSchedule({ getConnection, args }) {
 
   return toMcp(
     ok('ssh_backup_schedule', {
-      cron: cron.trim(),
+      cron: cronTrimmed,
       cron_line: cronLine,
       marker,
       backup_type,

src/tools/db-tools.js

@@ -26,6 +26,32 @@ const DEFAULT_TIMEOUT_MS = 60_000;
 const DEFAULT_LIMIT = 1000;
 const MAX_ALLOWED_LIMIT = 100_000;
 
+/**
+ * Conservative SQL-identifier validator (database / user names).
+ *
+ * shQuote() is correct for SHELL quoting but a value like `app'; DROP DATABASE x; --`
+ * shell-unquotes to a literal SQL string and becomes an INJECTED SQL token when the
+ * outer `-e '<query>'` is parsed by mysql/psql. We don't render idents as SQL string
+ * literals anywhere safe -- `SHOW TABLES FROM 'name'`, `pg_database_size('name')`,
+ * and the parameterless `mysqldump <name>` all treat the value as an identifier.
+ * So we require a syntactic subset that every mainstream DBMS allows for
+ * database/role/schema/table names:
+ *   [A-Za-z0-9_][A-Za-z0-9_.-]{0,63}
+ * Max 64 chars (MySQL cap is 64, PG is 63). `.` allowed so `schema.table` works for
+ * mongodump --db / pg_database_size callers that pass schema-qualified names.
+ * No spaces, no quotes, no semicolons, no backticks, no backslashes.
+ */
+const SQL_IDENT_RE = /^[A-Za-z0-9_][A-Za-z0-9_.-]{0,63}$/;
+function isSafeSqlIdent(s) {
+  return typeof s === 'string' && SQL_IDENT_RE.test(s);
+}
+function rejectBadIdent(tool, field, value, { server, format }) {
+  return toMcp(
+    fail(tool, `${field} contains unsafe characters (must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,63})`, { server }),
+    { format },
+  );
+}
+
 /**
  * Assemble a MongoDB URI for the `mongo` family so we can hand it to mongosh
  * via env var instead of argv. Defaults to localhost:27017 because the SSH
@@ -284,6 +310,12 @@ export async function handleSshDbQuery({ getConnection, args }) {
   if (!query || typeof query !== 'string') {
     return toMcp(fail('ssh_db_query', 'query is required'), { format });
   }
+  if (database != null && !isSafeSqlIdent(database)) {
+    return rejectBadIdent('ssh_db_query', 'database', database, { server, format });
+  }
+  if (user != null && !isSafeSqlIdent(user)) {
+    return rejectBadIdent('ssh_db_query', 'user', user, { server, format });
+  }
 
   const cappedLimit = safeInt(limit, { min: 1, max: MAX_ALLOWED_LIMIT, fallback: DEFAULT_LIMIT });
 
@@ -428,6 +460,12 @@ export async function handleSshDbList({ getConnection, args }) {
   if (!['mysql', 'postgresql', 'mongodb'].includes(db_type)) {
     return toMcp(fail('ssh_db_list', `unsupported db_type: ${db_type}`), { format });
   }
+  if (database != null && !isSafeSqlIdent(database)) {
+    return rejectBadIdent('ssh_db_list', 'database', database, { server, format });
+  }
+  if (user != null && !isSafeSqlIdent(user)) {
+    return rejectBadIdent('ssh_db_list', 'user', user, { server, format });
+  }
 
   let cmd;
   if (db_type === 'mysql') cmd = buildMySqlListCommand({ database, user });
@@ -500,6 +538,12 @@ export async function handleSshDbDump({ getConnection, args }) {
     return toMcp(fail('ssh_db_dump', `unsupported db_type: ${db_type}`), { format });
   }
   if (!database) return toMcp(fail('ssh_db_dump', 'database is required'), { format });
+  if (!isSafeSqlIdent(database)) {
+    return rejectBadIdent('ssh_db_dump', 'database', database, { server, format });
+  }
+  if (user != null && !isSafeSqlIdent(user)) {
+    return rejectBadIdent('ssh_db_dump', 'user', user, { server, format });
+  }
 
   const outPath = output_path ||
     `/tmp/${database}-${Date.now()}.${db_type === 'mongodb' ? 'archive' : 'sql'}${gzip ? '.gz' : ''}`;
@@ -593,6 +637,12 @@ export async function handleSshDbImport({ getConnection, args }) {
     return toMcp(fail('ssh_db_import', `unsupported db_type: ${db_type}`), { format });
   }
   if (!database) return toMcp(fail('ssh_db_import', 'database is required'), { format });
+  if (!isSafeSqlIdent(database)) {
+    return rejectBadIdent('ssh_db_import', 'database', database, { server, format });
+  }
+  if (user != null && !isSafeSqlIdent(user)) {
+    return rejectBadIdent('ssh_db_import', 'user', user, { server, format });
+  }
   if (!input_path) return toMcp(fail('ssh_db_import', 'input_path is required'), { format });
 
   if (isPreview) {

tests/test-backup-tools.js

@@ -256,6 +256,33 @@ await test('backup_schedule: preview shows cron plan', async () => {
   assert(parsed.data.plan.action.includes('schedule'));
 });
 
+await test('backup_schedule: rejects cron with embedded newline (injection guard)', async () => {
+  const r = await handleSshBackupSchedule({
+    getConnection: async () => { throw new Error('should not reach connection'); },
+    args: {
+      server: 's',
+      cron: '0 0 * * *\n* * * * * rm -rf ~',
+      backup_type: 'mysql', database: 'app',
+      preview: true, format: 'json',
+    },
+  });
+  assert.strictEqual(r.isError, true);
+  const parsed = JSON.parse(r.content[0].text);
+  assert(/single line|newline/i.test(parsed.error), `expected newline rejection, got: ${parsed.error}`);
+});
+
+await test('backup_schedule: rejects cron with shell metacharacters', async () => {
+  for (const bad of ['0 0 * * * `id`', '0 0 * * * $(whoami)']) {
+    const r = await handleSshBackupSchedule({
+      getConnection: async () => { throw new Error('should not reach connection'); },
+      args: { server: 's', cron: bad, backup_type: 'mysql', database: 'app', preview: true, format: 'json' },
+    });
+    assert.strictEqual(r.isError, true, `expected fail for ${JSON.stringify(bad)}`);
+    const parsed = JSON.parse(r.content[0].text);
+    assert(/shell metacharacters|\$|`/.test(parsed.error), `expected metachar rejection, got: ${parsed.error}`);
+  }
+});
+
 await test('backup_schedule: refuses password arg for DB backups (no plaintext secret in crontab)', async () => {
   for (const dbType of ['mysql', 'postgresql', 'mongodb']) {
     const r = await handleSshBackupSchedule({

tests/test-db-tools.js

@@ -126,6 +126,35 @@ await test('ssh_db_query: isSafeSelect rejection -> structured fail, NO remote c
   assert.strictEqual(r.isError, true);
 });
 
+await test('ssh_db_query: rejects database/user names with SQL metacharacters (injection guard)', async () => {
+  for (const bad of ['app\'; DROP DATABASE x; --', 'app; DROP', 'a b', 'a`b', 'a\\b']) {
+    const r = await handleSshDbQuery({
+      getConnection: async () => { throw new Error('must not connect'); },
+      args: { server: 's', db_type: 'mysql', database: bad, query: 'SELECT 1', format: 'json' },
+    });
+    assert.strictEqual(r.isError, true, `expected fail for database=${JSON.stringify(bad)}`);
+    const parsed = JSON.parse(r.content[0].text);
+    assert(parsed.error.includes('unsafe characters'),
+      `expected 'unsafe characters' in error, got: ${parsed.error}`);
+  }
+  // user field should be guarded too
+  const r2 = await handleSshDbQuery({
+    getConnection: async () => { throw new Error('must not connect'); },
+    args: { server: 's', db_type: 'mysql', query: 'SELECT 1', user: 'u\'; DROP', format: 'json' },
+  });
+  assert.strictEqual(r2.isError, true);
+});
+
+await test('ssh_db_dump: rejects database names with metacharacters', async () => {
+  const r = await handleSshDbDump({
+    getConnection: async () => { throw new Error('must not connect'); },
+    args: { server: 's', db_type: 'mysql', database: 'x\'; rm -rf /', format: 'json', preview: true },
+  });
+  assert.strictEqual(r.isError, true);
+  const parsed = JSON.parse(r.content[0].text);
+  assert(parsed.error.includes('unsafe characters'));
+});
+
 await test('ssh_db_query: `SELECT deleted_at FROM t` is accepted (old impl would falsely reject)', async () => {
   const client = new FakeClient({ script: () => ({ stdout: 'deleted_at\n2024-01-01\n', code: 0 }) });
   const r = await handleSshDbQuery({