fix(clone): close four security findings on the #94 clone endpoint

CLAUDE.md

@@ -653,6 +653,27 @@ against a real server whose `PATH` holds a fake `git`.
   The directory name is one path segment (`DIR_NAME_RE`), joined onto a parent that
   passed `isPathAllowed()` — a registered workdir widens that allowlist, which is why
   the gate cannot be skipped here any more than in `POST /api/projects`.
+- **The AUTHORITY is user-supplied too, and the leading-`-` check does not see it.**
+  `ssh://-oProxyCommand=id/x` and `git@-oProxyCommand=id:a/b` are URLs whose HOST is
+  an ssh option (CVE-2017-1000117) — the option sits behind the scheme or the `@`.
+  `authorityIsSafe()` drops the `:<port>` and refuses any `@`-part opening with `-`.
+  Modern git passes `--` to ssh itself, so this is a belt on a brace; the rule that
+  nothing handed to git may look like an option may not hold only on a patched binary.
+- **The parent is re-checked after `realpath`, and the target is `lstat`ed.**
+  `isPathAllowed()` reads a STRING, so a symlink inside an allowed root is a legal
+  path pointing anywhere on disk — the same layer-2 reasoning as `pwd -P` in the
+  remote file browser. And `existsSync()` is false for a DANGLING symlink, which git
+  would happily follow and populate; `lstat` sees the link itself.
+- **The timeout kills the process GROUP, and two clones run at a time.** `git clone`
+  is a parent to `git-remote-https` / `ssh` / `index-pack`; killing the `git` pid
+  alone leaves those writing into the tree the handler is about to `rmSync`. The
+  child is `detached` on POSIX for that reason (not on Windows, where it opens a
+  console). `MAX_CONCURRENT_CLONES` is 2 — each clone holds its request open for as
+  long as it runs, so an unbounded one is a disk-filling primitive behind one button.
+- **The HOST is not filtered, deliberately.** Any reachable `http(s)`/`ssh`/`git`
+  host is clonable, including RFC1918 and loopback — cloning from an internal GitLab
+  is the feature. The endpoint sits BELOW `auth.authMiddleware`, so this is a
+  logged-in user's own fetch, not an open proxy.
 - **It must fail, not wait.** stdin is closed, `GIT_TERMINAL_PROMPT=0`, no TTY: a
   credential or host-key question fails at once (measured: a private/nonexistent GitHub
   URL answers in ~0.4 s with git's own line). `CCS_GIT_CLONE_TIMEOUT_MS` (10 min) is the

git-clone.js

@@ -18,6 +18,12 @@
 // - THE DIRECTORY NAME IS A SINGLE PATH SEGMENT. Derived from the URL or given by
 //   the user, it is joined onto a parent that passed isPathAllowed(); `..`, `/` or a
 //   leading `.` would let the clone land somewhere else.
+// - AND THE AUTHORITY IS PART OF THE ARGV. `ssh://-oProxyCommand=id/x` and
+//   `git@-oProxyCommand=id:x/y` are URLs whose HOST is an ssh option — CVE-2017-1000117.
+//   The leading `-` check on the whole string does not see them, because the option is
+//   behind the scheme or the `@`. Current git passes `--` to ssh itself, so this is a
+//   belt on top of a brace; the rule above says nothing we hand git may look like an
+//   option, and a rule that holds only on a patched binary is not that rule.
 
 const ALLOWED_PROTOCOLS = ['http', 'https', 'ssh', 'git'];
 
@@ -31,6 +37,12 @@ const URL_RE = /^(?:https?|ssh|git):\/\/[^\s/@]+(?:@[^\s/@]+)?(?::\d+)?\/[^\s]+$
 // scp-like: git@github.com:user/repo.git — no scheme, exactly one ':' after the host.
 const SCP_RE = /^[A-Za-z0-9._-]+@[A-Za-z0-9._-]+:[^\s:]+$/;
 
+// Every `@`-separated part of an authority reaches ssh/git as argv; none may open
+// with `-`. The trailing `:<port>` is dropped first so a port cannot hide the host.
+function authorityIsSafe(authority) {
+  return authority.replace(/:\d+$/, '').split('@').every(part => part && !part.startsWith('-'));
+}
+
 /**
  * @param {unknown} raw
  * @returns {{ url: string, repoName: string } | null} null when the URL is not one
@@ -41,6 +53,12 @@ function parseCloneUrl(raw) {
   const url = raw.trim();
   if (!url || url.startsWith('-') || /[\s\0]/.test(url)) return null;
   if (!URL_RE.test(url) && !SCP_RE.test(url)) return null;
+  // `<user>@<host>[:port]` — the part before the path in a URL, before the `:` in
+  // an scp-like address.
+  const authority = URL_RE.test(url)
+    ? url.slice(url.indexOf('://') + 3).split('/')[0]
+    : url.split(':')[0];
+  if (!authorityIsSafe(authority)) return null;
   const tail = url.replace(/\/+$/, '').split(/[/:]/).pop().replace(/\.git$/i, '');
   if (!DIR_NAME_RE.test(tail)) return null;
   return { url, repoName: tail };
@@ -68,4 +86,4 @@ function cloneEnv(base = process.env) {
   return { ...base, GIT_TERMINAL_PROMPT: '0', GIT_ALLOW_PROTOCOL: ALLOWED_PROTOCOLS.join(':') };
 }
 
-module.exports = { ALLOWED_PROTOCOLS, parseCloneUrl, isValidDirName, isValidBranch, cloneArgs, cloneEnv };
+module.exports = { ALLOWED_PROTOCOLS, parseCloneUrl, authorityIsSafe, isValidDirName, isValidBranch, cloneArgs, cloneEnv };

server.js

@@ -9607,6 +9607,11 @@ function registerLocalProject(name, workdir) {
 // Local projects only: a clone on a remote host would need to run over SSH, and
 // the remote project flow already takes an existing path.
 const GIT_CLONE_TIMEOUT_MS = parseInt(process.env.CCS_GIT_CLONE_TIMEOUT_MS || '600000', 10) || 600000;
+// A clone holds its request open for as long as it runs, writes into the filesystem
+// and costs a network fetch. Two at a time is generous for a tool one person drives;
+// without a cap, a page that fires the button in a loop fills the disk.
+const MAX_CONCURRENT_CLONES = 2;
+let clonesInFlight = 0;
 app.post('/api/projects/clone', (req, res) => {
   const { url, branch = '', parentDir, name = '', dirName = '', shallow = false } = req.body || {};
   const parsed = gitClone.parseCloneUrl(url);
@@ -9615,28 +9620,47 @@ app.post('/api/projects/clone', (req, res) => {
   if (dirName && !gitClone.isValidDirName(dirName)) return res.status(400).json({ error: 'invalid directory name' });
   if (!parentDir || typeof parentDir !== 'string') return res.status(400).json({ error: 'parentDir required' });
   if (!isPathAllowed(parentDir)) return res.status(403).json({ error: 'path not allowed' });
-  const parent = path.resolve(parentDir);
+  // The parent is resolved to its PHYSICAL path and re-checked: isPathAllowed() reads
+  // a string, so a symlink inside an allowed root otherwise points the clone anywhere
+  // on disk. Same rule the remote browser's layer 2 applies with `pwd -P`.
+  let parent;
+  try { parent = fs.realpathSync(path.resolve(parentDir)); } catch { return res.status(400).json({ error: 'parent directory does not exist' }); }
+  if (!isPathAllowed(parent)) return res.status(403).json({ error: 'path not allowed' });
   let parentIsDir = false;
   try { parentIsDir = fs.statSync(parent).isDirectory(); } catch {}
   if (!parentIsDir) return res.status(400).json({ error: 'parent directory does not exist' });
   const dir = dirName || parsed.repoName;
   const target = path.join(parent, dir);
-  if (fs.existsSync(target)) return res.status(409).json({ error: `already exists: ${target}` });
+  // lstat, not existsSync: a DANGLING symlink at the target does not "exist", and git
+  // would follow it and populate whatever it names.
+  let targetTaken = true;
+  try { fs.lstatSync(target); } catch { targetTaken = false; }
+  if (targetTaken) return res.status(409).json({ error: `already exists: ${target}` });
+  if (clonesInFlight >= MAX_CONCURRENT_CLONES) return res.status(429).json({ error: 'another clone is already running' });
 
   const args = gitClone.cloneArgs({ url: parsed.url, target, branch, shallow: !!shallow });
   let stderr = '';
   let child;
+  // `git clone` is a parent to git-remote-https / ssh / index-pack, and killing it
+  // alone leaves those writing into the tree the timeout handler is about to remove.
+  // A detached child is its own process GROUP, so one kill reaches all of them.
+  const ownGroup = process.platform !== 'win32';
   try {
     // stdin is closed, GIT_TERMINAL_PROMPT=0 and no TTY: a credential or host-key
     // question fails at once instead of holding the request until the timeout.
-    child = spawnProc('git', args, { cwd: parent, env: gitClone.cloneEnv(), stdio: ['ignore', 'ignore', 'pipe'] });
+    child = spawnProc('git', args, { cwd: parent, env: gitClone.cloneEnv(), stdio: ['ignore', 'ignore', 'pipe'], detached: ownGroup });
   } catch (e) { return res.status(500).json({ error: e.message }); }
+  clonesInFlight++;
   child.stderr.on('data', d => { if (stderr.length < 8192) stderr += String(d); });
-  const timer = setTimeout(() => { try { child.kill('SIGKILL'); } catch {} }, GIT_CLONE_TIMEOUT_MS);
+  const timer = setTimeout(() => {
+    try { if (ownGroup) process.kill(-child.pid, 'SIGKILL'); else child.kill('SIGKILL'); }
+    catch { try { child.kill('SIGKILL'); } catch {} }
+  }, GIT_CLONE_TIMEOUT_MS);
   let settled = false;
   const finish = (code, err) => {
     if (settled) return;
     settled = true;
+    clonesInFlight--;
     clearTimeout(timer);
     if (err || code !== 0) {
       // git removes its own half-clone on failure; a SIGKILL from the timer does not.

test/git-clone.test.js

@@ -39,6 +39,13 @@ check('embedded whitespace is refused', G.parseCloneUrl('git@h:a b'), null);
 check('a repo name of `..` is refused', G.parseCloneUrl('https://h/x/..'), null);
 check('a dot-leading repo name is refused', G.parseCloneUrl('https://h/x/.git'), null);
 check('non-string input is refused', G.parseCloneUrl({ toString: () => 'https://h/a/b' }), null);
+// CVE-2017-1000117: the host itself is an ssh option. The leading `-` check on the
+// whole string does not see it — it sits behind the scheme or the `@`.
+check('an option-shaped host behind the scheme is refused', G.parseCloneUrl('ssh://-oProxyCommand=id/x/y'), null);
+check('an option-shaped host behind the @ is refused', G.parseCloneUrl('git@-oProxyCommand=id:a/b'), null);
+check('an option-shaped user is refused', G.parseCloneUrl('ssh://-u@h/a/b'), null);
+check('a port does not hide the host', G.parseCloneUrl('ssh://git@host:2222/a/b.git').repoName, 'b');
+check('a password in the userinfo is still a URL', G.parseCloneUrl('https://user:pw@h/a/b').repoName, 'b');
 
 console.log('\n— git-clone.js: argv and environment —');
 check('argv puts -- before the URL and target', G.cloneArgs({ url: 'U', target: 'T' }), ['clone', '--', 'U', 'T']);
@@ -65,6 +72,10 @@ process.on('exit', () => { for (const d of [APP_DIR, HOME_DIR, BIN_DIR, OUTSIDE]
 fs.mkdirSync(path.join(APP_DIR, 'data'), { recursive: true });
 const WORKSPACE = path.join(APP_DIR, 'workspace');
 fs.mkdirSync(WORKSPACE, { recursive: true });
+// The endpoint answers with the PHYSICAL parent (realpath), so on macOS, where
+// /var is a symlink to /private/var, the path it returns is not the one composed
+// here. Requests still send the unresolved WORKSPACE — that is what exercises it.
+const WS_REAL = fs.realpathSync(WORKSPACE);
 const GIT_LOG = path.join(APP_DIR, 'git-calls.log');
 
 // Fake git: one line of JSON per call — argv, cwd, the two env vars we care about.
@@ -75,6 +86,7 @@ PATH=/usr/bin:/bin:$PATH
 printf '%s\\n' "$(node -e 'console.log(JSON.stringify({argv:process.argv.slice(1),cwd:process.cwd(),prompt:process.env.GIT_TERMINAL_PROMPT,allow:process.env.GIT_ALLOW_PROTOCOL}))' -- "$@")" >> "${GIT_LOG}"
 for last; do :; done
 mkdir -p "$last/.git"
+case "$*" in *slow*) sleep 3;; esac
 case "$*" in *boom*) echo "fatal: repository 'boom' not found" >&2; exit 128;; esac
 exit 0
 `, { mode: 0o755 });
@@ -122,17 +134,24 @@ const gitCalls = () => fs.existsSync(GIT_LOG) ? fs.readFileSync(GIT_LOG, 'utf8')
   check('a parent that does not exist is 400', (await clone({ url: 'https://h/a/b', parentDir: path.join(WORKSPACE, 'nope') })).status, 400);
   fs.mkdirSync(path.join(WORKSPACE, 'taken'));
   check('an existing target is 409', (await clone({ url: 'https://h/a/taken.git', parentDir: WORKSPACE })).status, 409);
+  // A symlink inside an allowed root is a string that passes isPathAllowed() and a
+  // directory that is somewhere else — so the parent is re-checked after realpath.
+  fs.symlinkSync(OUTSIDE, path.join(WORKSPACE, 'linkout'));
+  check('a parent that is a symlink out of the allowed roots is 403', (await clone({ url: 'https://h/a/b', parentDir: path.join(WORKSPACE, 'linkout') })).status, 403);
+  // A DANGLING symlink does not "exist" — git would follow it and populate its target.
+  fs.symlinkSync(path.join(OUTSIDE, 'nowhere'), path.join(WORKSPACE, 'ghost'));
+  check('a target that is a dangling symlink is 409, not a clone', (await clone({ url: 'https://h/a/ghost.git', parentDir: WORKSPACE })).status, 409);
   check('none of the refusals reached git', gitCalls().length, 0);
 
   console.log('\n— a clone that succeeds —');
   const ok = await clone({ url: 'https://github.com/acme/widget.git', parentDir: WORKSPACE, branch: 'dev' });
   check('answers 200 ok', [ok.status, ok.json?.ok], [200, true]);
-  const target = path.join(WORKSPACE, 'widget');
+  const target = path.join(WS_REAL, 'widget');
   check('workdir is <parent>/<repo>', ok.json?.workdir, target);
   check('the directory exists', fs.existsSync(path.join(target, '.git')), true);
   const call = gitCalls()[0];
   check('git argv: clone --branch dev -- <url> <target>', call.argv, ['clone', '--branch', 'dev', '--', 'https://github.com/acme/widget.git', target]);
-  check('git ran in the parent', fs.realpathSync(call.cwd), fs.realpathSync(WORKSPACE));
+  check('git ran in the parent', fs.realpathSync(call.cwd), WS_REAL);
   check('git was told never to prompt', call.prompt, '0');
   check('git was pinned to the transport allowlist', call.allow, 'http:https:ssh:git');
   const projects = (await api('GET', '/api/projects')).json;
@@ -146,12 +165,20 @@ const gitCalls = () => fs.existsSync(GIT_LOG) ? fs.readFileSync(GIT_LOG, 'utf8')
   const bad = await clone({ url: 'https://h/a/boom.git', parentDir: WORKSPACE, name: 'named' });
   check('answers 502', bad.status, 502);
   check('with git\'s own last stderr line', bad.json?.error, "fatal: repository 'boom' not found");
-  check('the half-made directory is removed', fs.existsSync(path.join(WORKSPACE, 'boom')), false);
+  check('the half-made directory is removed', fs.existsSync(path.join(WS_REAL, 'boom')), false);
   check('and no project was registered', (await api('GET', '/api/projects')).json.some(p => p.name === 'named'), false);
 
+  console.log('\n— the concurrency cap —');
+  const slowA = clone({ url: 'https://h/a/slow1.git', parentDir: WORKSPACE });
+  const slowB = clone({ url: 'https://h/a/slow2.git', parentDir: WORKSPACE });
+  await sleep(400);
+  check('a third clone while two run is 429', (await clone({ url: 'https://h/a/slow3.git', parentDir: WORKSPACE })).status, 429);
+  check('the two that were admitted still succeed', (await Promise.all([slowA, slowB])).map(r => r.status), [200, 200]);
+  check('and the slot is given back', (await clone({ url: 'https://h/a/after.git', parentDir: WORKSPACE })).status, 200);
+
   console.log('\n— dirName and name overrides —');
   const named = await clone({ url: 'git@github.com:acme/widget.git', parentDir: WORKSPACE, dirName: 'widget2', name: 'Widget Two', shallow: true });
-  check('dirName picks the folder', named.json?.workdir, path.join(WORKSPACE, 'widget2'));
+  check('dirName picks the folder', named.json?.workdir, path.join(WS_REAL, 'widget2'));
   check('shallow adds --depth 1', gitCalls().pop().argv.slice(0, 3), ['clone', '--depth', '1']);
   check('name is the project name', (await api('GET', '/api/projects')).json.find(p => p.id === named.json.id)?.name, 'Widget Two');