From 3f7b3cc7feffd91d030a27204e72c67872e3dae4 Mon Sep 17 00:00:00 2001 From: Cho Young-Hwi Date: Mon, 6 Jul 2026 11:33:32 +0000 Subject: [PATCH] [#974] Setup hardening (EPIC #967 Phase 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 5 confirmed setup/start defects + 2 folded lows. Changes confined to setup/start paths (bin/quadwork.js, server setup route, server listen); no steady-state runtime or agent-spawn behavior changes. 1. Stale-branch brick — if a `worktree-` branch survives after its dir was deleted, setup aborted with no rollback ("branch already exists"). Both the CLI wizard and the server create-worktrees route now reuse an existing branch (prune stale registration + `worktree add` re-attach), making setup re-runnable. Server logic extracted to createAgentWorktree. 2. CLI silently shipped an agent with no AGENTS.md when its seed template was missing; now hard-fails (matches the server seed-files route). 3. CLI collected a reviewer-token *path* but never the token itself, so RE1/RE2 launched pointed at an empty file. Now prompts via askSecret and writes it 0600 (matching the server save-token endpoint), or prints a clear "set it in Settings" step when skipped. 4. No server.listen error handler — a second `quadwork start` threw a raw EADDRINUSE stack. Adds server.on('error') → friendly message + exit 1. 5. No origin-mismatch check — pointing setup at a clone of a different repo than the entered slug silently seeded the wrong project. ensureGitHeadForSetup (CLI + server) now compares `git remote get-url origin` to the slug and fails clearly. New repoSlugFromRemote normalizes https/ssh/.git forms. Folds: don't force `sudo npm i -g` when the global prefix is user-writable (nvm/fnm/volta/brew); use stdio:inherit for installs so `sudo apt` password prompts actually reach the TTY. Tests: server/routes.setupHardening.test.js (6 cases — slug normalization, origin-match/mismatch guard, worktree fresh/stale-reuse/detached). Full suite 60 passed / 0 failed. EADDRINUSE + stale-branch reuse also verified end-to-end against a live server and a real git repo. Co-Authored-By: Claude Opus 4.8 --- bin/quadwork.js | 105 +++++++++++++++++--- server/index.js | 17 +++- server/routes.js | 63 +++++++++--- server/routes.setupHardening.test.js | 137 +++++++++++++++++++++++++++ 4 files changed, 295 insertions(+), 27 deletions(-) create mode 100644 server/routes.setupHardening.test.js diff --git a/bin/quadwork.js b/bin/quadwork.js index f8aff6c..c7016a2 100755 --- a/bin/quadwork.js +++ b/bin/quadwork.js @@ -80,6 +80,32 @@ function which(cmd) { return run("which", [cmd]) !== null; } +// #974: does a global `npm install -g` need sudo? True only when the npm +// global prefix dir exists and isn't writable by the current user. Windows +// never needs sudo; if the prefix can't be determined we keep the safe +// (sudo) default rather than risk an EACCES mid-install. +function npmGlobalNeedsSudo() { + if (process.platform === "win32") return false; + const prefix = run("npm", ["prefix", "-g"]); + if (!prefix) return true; + try { + fs.accessSync(prefix, fs.constants.W_OK); + return false; + } catch { + return true; + } +} + +// #974: reduce a git remote URL to its canonical `owner/repo` slug so the +// entered repo can be compared against what a reused clone actually points at. +// Handles https://github.com/owner/repo(.git), git@github.com:owner/repo(.git), +// and ssh://… forms. +function repoSlugFromRemote(url) { + if (!url) return ""; + const m = url.trim().match(/[:/]([^/:]+\/[^/]+?)(?:\.git)?\/?$/); + return m ? m[1].toLowerCase() : ""; +} + function ensureGitHeadForSetup(absDir, repo) { if (!fs.existsSync(path.join(absDir, ".git"))) { const clone = runResult("gh", ["repo", "clone", repo, absDir]); @@ -89,6 +115,18 @@ function ensureGitHeadForSetup(absDir, repo) { if (!fetch.ok) return { ok: false, error: `Fetch failed: ${fetch.output}` }; } + // #974: fail clearly if the directory is a clone of a DIFFERENT repo than the + // slug entered — otherwise setup silently seeds worktrees/branches into the + // wrong project. + const originUrl = runResult("git", ["-C", absDir, "remote", "get-url", "origin"]); + if (originUrl.ok) { + const actual = repoSlugFromRemote(originUrl.output); + const expected = String(repo || "").toLowerCase().replace(/\.git$/, ""); + if (actual && expected && actual !== expected) { + return { ok: false, error: `Origin mismatch: ${absDir} points to '${actual}', but you entered '${repo}'. Point setup at the matching clone or correct the repo slug.` }; + } + } + let headCheck = runResult("git", ["-C", absDir, "rev-parse", "--verify", "HEAD"]); if (!headCheck.ok) { const remoteHead = runResult("git", ["-C", absDir, "symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"]); @@ -250,14 +288,18 @@ async function tryInstall(rl, name, description, commands, { platform } = {}) { log("Skipped."); return false; } - const sp = spinner(`Installing ${name}...`); const [cmd, ...args] = cmdSpec; - const result = run(cmd, args, { timeout: 120000 }); - if (result !== null) { - sp.stop(true); + // #974: install commands (esp. `sudo apt install …`) may prompt for a + // password or a confirmation on the real TTY. The old `run()` piped stdio, + // which swallows the prompt and hangs / fails silently — inherit stdio so + // the operator can actually respond. No spinner here: it would fight the + // inherited install output for the same stdout. + log(`Installing ${name}...`); + try { + execFileSync(cmd, args, { stdio: "inherit", timeout: 120000 }); + ok(`${name} installed`); return true; - } else { - sp.stop(false); + } catch { warn(`Auto-install failed. You can install manually and try again.`); return false; } @@ -346,8 +388,11 @@ async function checkPrereqs(rl) { console.log(""); } - // sudo needed for global npm installs on macOS/Linux - const npmPrefix = process.platform === "win32" ? "" : "sudo "; + // #974: only escalate with sudo when the global npm prefix isn't writable by + // the current user. Node from nvm/fnm/volta/Homebrew puts the prefix under a + // user-owned dir where `sudo npm i -g` is unnecessary and often corrupts + // ownership. Fall back to sudo only when the prefix genuinely needs root. + const npmPrefix = npmGlobalNeedsSudo() ? "sudo " : ""; // Offer to install Claude Code if missing if (!hasClaude) { @@ -597,6 +642,26 @@ async function setupAgents(rl, repo) { reviewerUser = await ask(rl, "Reviewer GitHub username", ""); log("Path to a file containing a GitHub PAT for the reviewer account."); reviewerTokenPath = await ask(rl, "Reviewer token file path", path.join(os.homedir(), ".quadwork", "reviewer-token")); + // #974: previously we collected the token PATH but never the token itself, + // so RE1/RE2 launched pointed at an empty/nonexistent file and PR reviews + // failed. Prompt for the token now and write it 0600 (matching the server + // save-token endpoint); if the operator skips it, tell them exactly where + // to add it later instead of silently leaving reviewing broken. + const reviewerToken = await askSecret(rl, "Reviewer GitHub token (paste, or leave blank to set later in Settings)"); + if (reviewerToken.trim()) { + try { + const tokenDir = path.dirname(path.resolve(reviewerTokenPath)); + if (!fs.existsSync(tokenDir)) ensureSecureDir(tokenDir); + fs.writeFileSync(reviewerTokenPath, reviewerToken.trim() + "\n", { mode: 0o600 }); + try { fs.chmodSync(reviewerTokenPath, 0o600); } catch {} + ok(`Reviewer token saved to ${reviewerTokenPath} (mode 0600)`); + } catch (err) { + warn(`Could not write reviewer token to ${reviewerTokenPath}: ${err.message}`); + log(`Add it later in Settings → Reviewer Account, or write it to ${reviewerTokenPath} yourself (chmod 600).`); + } + } else { + log(`No token entered — add it later in Settings → Reviewer Account (it saves to ${reviewerTokenPath}, mode 0600).`); + } } const projectName = path.basename(absDir); @@ -616,8 +681,19 @@ async function setupAgents(rl, repo) { const wtDir = path.join(path.dirname(absDir), `${projectName}-${agent}`); if (!fs.existsSync(wtDir)) { const branchName = `worktree-${agent}`; - const branch = runResult("git", ["-C", absDir, "branch", branchName, "HEAD"]); - if (!branch.ok) { wtFailed = `${agent}: branch failed: ${branch.output}`; break; } + // #974: a prior setup can leave the `worktree-` branch behind after + // its directory was deleted. `git branch HEAD` then fails "already + // exists" and used to brick the whole re-run with no rollback. Reuse an + // existing branch instead: prune any stale worktree registration still + // holding it, then `worktree add` re-attaches. Only create the branch when + // it doesn't already exist. + const branchExists = runResult("git", ["-C", absDir, "rev-parse", "--verify", "--quiet", `refs/heads/${branchName}`]).ok; + if (branchExists) { + run("git", ["-C", absDir, "worktree", "prune"]); + } else { + const branch = runResult("git", ["-C", absDir, "branch", branchName, "HEAD"]); + if (!branch.ok) { wtFailed = `${agent}: branch failed: ${branch.output}`; break; } + } const result = run("git", ["-C", absDir, "worktree", "add", wtDir, branchName]); if (!result) { const result2 = run("git", ["-C", absDir, "worktree", "add", "--detach", wtDir, "HEAD"]); @@ -629,7 +705,14 @@ async function setupAgents(rl, repo) { // Copy AGENTS.md seed with placeholder substitution const seedSrc = path.join(TEMPLATES_DIR, "seeds", `${agent}.AGENTS.md`); const seedDst = path.join(wtDir, "AGENTS.md"); - if (fs.existsSync(seedSrc)) { + // #974: a missing seed template means the agent's role is undefined — + // shipping a worktree with no AGENTS.md is worse than failing. Hard-fail + // instead of silently skipping (matches the server seed-files route). + if (!fs.existsSync(seedSrc)) { + wtFailed = `${agent}: missing AGENTS.md seed template (templates/seeds/${agent}.AGENTS.md)`; + break; + } + { let seedContent = fs.readFileSync(seedSrc, "utf-8"); if (reviewerUser) { seedContent = seedContent.replace(/\{\{reviewer_github_user\}\}/g, reviewerUser); diff --git a/server/index.js b/server/index.js index 262284a..3c5cfd4 100644 --- a/server/index.js +++ b/server/index.js @@ -2116,7 +2116,19 @@ function runStartupMigrations(cfg) { } -if (!process.env.QUADWORK_SKIP_LISTEN) server.listen(PORT, "127.0.0.1", async () => { +if (!process.env.QUADWORK_SKIP_LISTEN) { + // #974: a second `quadwork start` (or anything already bound to PORT) makes + // server.listen emit 'error'; with no handler Node throws the raw EADDRINUSE + // stack trace. Surface a friendly, actionable message and exit cleanly. + server.on("error", (err) => { + if (err && err.code === "EADDRINUSE") { + console.error(`\nQuadWork: port ${PORT} is already in use — another instance may already be running.`); + console.error(`Run 'quadwork stop' first, or change the port in Settings, then retry.\n`); + process.exit(1); + } + throw err; + }); + server.listen(PORT, "127.0.0.1", async () => { console.log(`QuadWork server listening on http://127.0.0.1:${PORT}`); syncTriggersFromConfig(); const startupCfg = readConfig(); @@ -2174,7 +2186,8 @@ if (!process.env.QUADWORK_SKIP_LISTEN) server.listen(PORT, "127.0.0.1", async () else console.warn(`[butler] auto-start failed: ${result.error}`); } startWatchdog(); -}); + }); +} // #972: clean shutdown. Previously only stopped butler + file-chat, so Ctrl+C // orphaned the detached caffeinate process (Mac never slept again), left agent diff --git a/server/routes.js b/server/routes.js index 28006bc..bb256d6 100644 --- a/server/routes.js +++ b/server/routes.js @@ -3692,6 +3692,14 @@ function exec(cmd, args, opts) { } } +// #974: reduce a git remote URL to its canonical `owner/repo` slug (mirrors the +// CLI helper) so a reused clone can be checked against the entered repo. +function repoSlugFromRemote(url) { + if (!url) return ""; + const m = url.trim().match(/[:/]([^/:]+\/[^/]+?)(?:\.git)?\/?$/); + return m ? m[1].toLowerCase() : ""; +} + function ensureGitHeadForSetup(workingDir, repo) { const gitDir = path.join(workingDir, ".git"); if (!fs.existsSync(gitDir)) { @@ -3704,6 +3712,18 @@ function ensureGitHeadForSetup(workingDir, repo) { if (!fetch.ok) return { ok: false, error: `Fetch failed: ${fetch.output}` }; } + // #974: fail clearly if the working dir is a clone of a DIFFERENT repo than + // the slug entered — otherwise setup silently seeds worktrees into the wrong + // project. (Same guard as the CLI wizard.) + const originUrl = exec("git", ["remote", "get-url", "origin"], { cwd: workingDir }); + if (originUrl.ok) { + const actual = repoSlugFromRemote(originUrl.output); + const expected = String(repo || "").toLowerCase().replace(/\.git$/, ""); + if (actual && expected && actual !== expected) { + return { ok: false, error: `Origin mismatch: working directory points to '${actual}', but the repo entered is '${repo}'. Use the matching clone or correct the repo slug.` }; + } + } + let headCheck = exec("git", ["rev-parse", "--verify", "HEAD"], { cwd: workingDir }); if (!headCheck.ok) { const remoteHead = exec("git", ["symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"], { cwd: workingDir }); @@ -3737,6 +3757,28 @@ function ensureGitHeadForSetup(workingDir, repo) { return { ok: true }; } +// #974: create (or re-attach) a single agent worktree. A prior setup can leave +// the `worktree-` branch behind after its directory was deleted; the old +// code ran `git branch HEAD` unconditionally, which fails "already +// exists" and bricked a re-run with no rollback. Reuse an existing branch +// instead — prune any stale worktree registration still holding it, then +// `worktree add` re-attaches. `execFn` is injectable for tests. +function createAgentWorktree(workingDir, wtDir, branchName, execFn = exec) { + const branchExists = execFn("git", ["rev-parse", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd: workingDir }).ok; + if (branchExists) { + execFn("git", ["worktree", "prune"], { cwd: workingDir }); + } else { + const branch = execFn("git", ["branch", branchName, "HEAD"], { cwd: workingDir }); + if (!branch.ok) return { ok: false, error: `branch failed: ${branch.output}` }; + } + const result = execFn("git", ["worktree", "add", wtDir, branchName], { cwd: workingDir }); + if (result.ok) return { ok: true, detached: false }; + // Fallback: detached worktree (branch may be checked out in a live worktree). + const result2 = execFn("git", ["worktree", "add", "--detach", wtDir, "HEAD"], { cwd: workingDir }); + if (result2.ok) return { ok: true, detached: true }; + return { ok: false, error: result.output }; +} + // ─── GitHub helpers for Setup Wizard ────────────────────────────────────── // GitHub user info @@ -3858,20 +3900,11 @@ router.post("/api/setup", (req, res) => { const wtDir = path.join(parentDir, `${projectName}-${agent}`); if (fs.existsSync(wtDir)) { created.push(`${agent} (exists)`); continue; } const branchName = `worktree-${agent}`; - const branch = exec("git", ["branch", branchName, "HEAD"], { cwd: workingDir }); - if (!branch.ok) { - errors.push(`${agent}: branch failed: ${branch.output}`); - continue; - } - const result = exec("git", ["worktree", "add", wtDir, branchName], { cwd: workingDir }); - if (result.ok) { - created.push(agent); - } else { - // Fallback: detached worktree - const result2 = exec("git", ["worktree", "add", "--detach", wtDir, "HEAD"], { cwd: workingDir }); - if (result2.ok) created.push(`${agent} (detached)`); - else errors.push(`${agent}: ${result.output}`); - } + // #974: reuse an existing branch (see createAgentWorktree) so a re-run + // after a deleted worktree dir no longer aborts on "branch exists". + const wt = createAgentWorktree(workingDir, wtDir, branchName); + if (!wt.ok) { errors.push(`${agent}: ${wt.error}`); continue; } + created.push(wt.detached ? `${agent} (detached)` : agent); } // Pre-trust worktree directories for Claude Code agents (#599). // Running `claude -p` in a directory auto-trusts it for future sessions, @@ -5120,6 +5153,8 @@ module.exports.findLinkedPrByTitle = findLinkedPrByTitle; module.exports.pickClosingPrFromTimeline = pickClosingPrFromTimeline; module.exports.progressForItemRest = progressForItemRest; module.exports.ensureGitHeadForSetup = ensureGitHeadForSetup; +module.exports.createAgentWorktree = createAgentWorktree; +module.exports.repoSlugFromRemote = repoSlugFromRemote; // #827: expose the GITHUB.md writer for the idle-no-op regression test. No // production callers outside this file. module.exports.writeGithubFileFromSnapshot = writeGithubFileFromSnapshot; diff --git a/server/routes.setupHardening.test.js b/server/routes.setupHardening.test.js new file mode 100644 index 0000000..8bf3e2a --- /dev/null +++ b/server/routes.setupHardening.test.js @@ -0,0 +1,137 @@ +// #974: setup hardening — origin-mismatch guard, repo-slug normalization, and +// re-runnable worktree creation (reuse an existing branch instead of aborting). +// +// Plain node:assert script — run with +// `node server/routes.setupHardening.test.js`. + +const assert = require("node:assert/strict"); +const fs = require("fs"); +const path = require("path"); +const cp = require("child_process"); + +const realExecFileSync = cp.execFileSync; +const realExistsSync = fs.existsSync; +const realMkdirSync = fs.mkdirSync; +const realChmodSync = fs.chmodSync; + +let existing = new Set(); +let commands = []; +let originUrl = "https://github.com/owner/repo.git"; + +// Stub only git/gh; everything else falls through to the real impl. +cp.execFileSync = function stubExecFileSync(cmd, args, opts = {}) { + commands.push({ cmd, args: args.slice(), cwd: opts.cwd || null }); + if (cmd !== "git" && cmd !== "gh") return realExecFileSync.apply(this, arguments); + const joined = args.join(" "); + if (cmd === "git" && joined === "fetch origin --prune") return ""; + if (cmd === "git" && joined === "remote get-url origin") return `${originUrl}\n`; + if (cmd === "git" && joined === "rev-parse --verify HEAD") return "abc123\n"; + const err = new Error(`unexpected command: ${cmd} ${joined}`); + err.stderr = Buffer.from(err.message); + throw err; +}; + +fs.existsSync = (p) => existing.has(path.normalize(p)); +fs.mkdirSync = () => {}; +fs.chmodSync = () => {}; + +const routes = require("./routes"); +const { ensureGitHeadForSetup, createAgentWorktree, repoSlugFromRemote } = routes; + +function reset() { + commands = []; + existing = new Set(); + originUrl = "https://github.com/owner/repo.git"; +} + +let passed = 0; +try { + // ── repoSlugFromRemote normalization ── + reset(); + assert.equal(repoSlugFromRemote("https://github.com/owner/repo.git"), "owner/repo", "https + .git"); + assert.equal(repoSlugFromRemote("https://github.com/Owner/Repo"), "owner/repo", "https, no .git, lowercased"); + assert.equal(repoSlugFromRemote("git@github.com:owner/repo.git"), "owner/repo", "ssh scp-form"); + assert.equal(repoSlugFromRemote("ssh://git@github.com/owner/repo.git"), "owner/repo", "ssh url"); + assert.equal(repoSlugFromRemote("https://github.com/owner/repo/"), "owner/repo", "trailing slash"); + assert.equal(repoSlugFromRemote(""), "", "empty"); + console.log(" PASS: repoSlugFromRemote normalizes common remote URL forms"); + passed++; + + // ── ensureGitHeadForSetup: origin matches → proceeds to HEAD verify ── + reset(); + existing.add(path.normalize("/tmp/proj/.git")); + originUrl = "https://github.com/owner/repo.git"; + let result = ensureGitHeadForSetup("/tmp/proj", "owner/repo"); + assert.equal(result.ok, true, "matching origin proceeds"); + assert.ok(commands.some((c) => c.args.join(" ") === "remote get-url origin"), "origin is checked"); + assert.ok(commands.some((c) => c.args.join(" ") === "rev-parse --verify HEAD"), "HEAD verified after origin match"); + console.log(" PASS: matching origin passes the guard and verifies HEAD"); + passed++; + + // ── ensureGitHeadForSetup: origin mismatch → clear error, no HEAD work ── + reset(); + existing.add(path.normalize("/tmp/proj/.git")); + originUrl = "git@github.com:someone-else/other.git"; + result = ensureGitHeadForSetup("/tmp/proj", "owner/repo"); + assert.equal(result.ok, false, "mismatched origin fails"); + assert.match(result.error, /Origin mismatch/, "error names the mismatch"); + assert.match(result.error, /someone-else\/other/, "error reports the actual slug"); + assert.ok(!commands.some((c) => c.args.join(" ") === "rev-parse --verify HEAD"), "aborts before HEAD work"); + console.log(" PASS: mismatched origin fails clearly before seeding anything"); + passed++; + + // ── createAgentWorktree: fresh branch (none exists) ── + const calls = []; + const fakeExec = (results) => (cmd, args) => { + calls.push(args.join(" ")); + const key = args.join(" "); + return results[key] !== undefined ? results[key] : { ok: true, output: "" }; + }; + + calls.length = 0; + let wt = createAgentWorktree("/wd", "/wd-dev", "worktree-dev", fakeExec({ + "rev-parse --verify --quiet refs/heads/worktree-dev": { ok: false, output: "" }, + "branch worktree-dev HEAD": { ok: true, output: "" }, + "worktree add /wd-dev worktree-dev": { ok: true, output: "" }, + })); + assert.deepEqual(wt, { ok: true, detached: false }, "fresh branch created + attached"); + assert.ok(calls.includes("branch worktree-dev HEAD"), "creates the branch when absent"); + assert.ok(!calls.includes("worktree prune"), "no prune when branch is new"); + console.log(" PASS: createAgentWorktree creates a fresh branch when none exists"); + passed++; + + // ── createAgentWorktree: stale branch reuse (branch exists, dir gone) ── + calls.length = 0; + wt = createAgentWorktree("/wd", "/wd-dev", "worktree-dev", fakeExec({ + "rev-parse --verify --quiet refs/heads/worktree-dev": { ok: true, output: "abc\n" }, + "worktree prune": { ok: true, output: "" }, + "worktree add /wd-dev worktree-dev": { ok: true, output: "" }, + })); + assert.deepEqual(wt, { ok: true, detached: false }, "existing branch reused"); + assert.ok(calls.includes("worktree prune"), "prunes stale registration before re-attach"); + assert.ok(!calls.includes("branch worktree-dev HEAD"), "does NOT recreate an existing branch (the old brick)"); + console.log(" PASS: createAgentWorktree reuses an existing branch (re-runnable setup)"); + passed++; + + // ── createAgentWorktree: detached fallback when add fails ── + calls.length = 0; + wt = createAgentWorktree("/wd", "/wd-dev", "worktree-dev", fakeExec({ + "rev-parse --verify --quiet refs/heads/worktree-dev": { ok: false, output: "" }, + "branch worktree-dev HEAD": { ok: true, output: "" }, + "worktree add /wd-dev worktree-dev": { ok: false, output: "already checked out" }, + "worktree add --detach /wd-dev HEAD": { ok: true, output: "" }, + })); + assert.deepEqual(wt, { ok: true, detached: true }, "falls back to detached worktree"); + console.log(" PASS: createAgentWorktree falls back to a detached worktree"); + passed++; + + console.log(`\n${passed} passed, 0 failed\n`); +} catch (err) { + console.error("test failed:", err); + process.exitCode = 1; +} finally { + cp.execFileSync = realExecFileSync; + fs.existsSync = realExistsSync; + fs.mkdirSync = realMkdirSync; + fs.chmodSync = realChmodSync; +}