fix(cli): harden skillx add against large or hostile sources and credential leaks

docs/cli-reference.md

@@ -224,16 +224,20 @@ registry release, a skill added this way is **not scanned, signed or pinned in
 | `owner/repo` | A GitHub repository (default branch) |
 | `owner/repo/skill` | One skill of it, by name or directory name (same as `--skill skill`) |
 | `github:owner/repo[/path][@ref]` | A directory of the repository, at a branch or tag |
-| `https://github.com/owner/repo[/tree/<ref>[/<path>]]` | Read as `github:` |
+| `https://github.com/owner/repo[/tree/<ref>[/<path>]]` | Read as `github:`; a branch name with `/` is found by asking the remote which leading segments name a branch or tag (longest wins) |
 | `https://host/repo.git` | Any https git repository (default branch) |
 | `./dir`, `../dir`, an absolute path | A local directory |
 
 Only `https` is accepted. `http`, `ssh`, `git@host:` (scp style), `file://`,
 `ext::` and other `::` transport helpers, a URL with a query or fragment, and
 anything starting with `-` are refused as `usage_error`; git is also run with
 every other protocol disabled. A path or ref with `..`, `.`, empty, `\` or `:`
-segments is refused. `@publisher/package`, `kit:` and `collection:` belong to
-`skillx install`; `@agentkit/*` is reserved.
+segments is refused. A URL that holds an `@` but does not parse, or whose path
+holds one (`https://user:pa/ss@host/...`), is refused without being echoed,
+as is any refused source holding an `@`: percent-encode `/`, `?`, `#` and `@` in a user name or password.
+`@publisher/package`, `kit:` and `collection:` belong to `skillx install`;
+`@agentkit/*` is reserved. A ref is a branch or tag name; commit SHAs are not
+accepted.
 
 **Options:**
 | Flag | Description | Default |
@@ -253,8 +257,10 @@ is not front matter, and tags or duplicate keys reject the file, so nothing in
 a source ever runs. When the source (or `github:` path) is itself a skill, it
 is the only one; otherwise every skill up to 6 directories deep counts,
 outside `.git`, other VCS and cache directories and `node_modules`, and a
-skill's own subdirectories are its content. Skills that cannot be installed are
-listed under `skipped` with the reason.
+skill's own subdirectories are its content. Sizes are read before content: more
+than 1,000 `SKILL.md` files under the search path is a `usage_error` (point at a
+smaller path), and a `SKILL.md` over 1 MiB is skipped unread. Skills that cannot
+be installed are listed under `skipped` with the reason.
 
 - **Names.** `name` is the installed directory name and is never rewritten:
   it must be a valid alias (up to 64 lowercase letters, digits, `.`, `_`, `-`,
@@ -267,14 +273,28 @@ listed under `skipped` with the reason.
   limits (4,096 files, 32 MiB per file, 256 MiB in all) refuses the skill as
   `incompatible`. Links are never followed or copied, neither while searching
   nor while reading.
-- **Git.** The clone is shallow, single-branch and without a checkout, into a
-  temporary directory removed afterwards: files are read from git objects, so
-  no repository file is written, and no hook, filter or LFS download runs. git
-  runs without a shell, with `--` before the URL and all credential prompts
-  off (`core.askPass=`, `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=never`), so
-  a private repository works only with credentials git already has (a
+- **Git.** The clone is shallow, single-branch, without a checkout and asks
+  for no blobs (`--filter=blob:none`; a server without partial clone sends
+  them all): the listing comes from tree objects and only the files discovery
+  and the chosen skills need are fetched, so a skill in a large monorepo does
+  not download the rest. A fetched file arrives whole (git cannot report a
+  size first), so download time is bounded by the 120 s limit per git call;
+  sizes are then read before content and content is read in bounded batches.
+  No repository file is written, and no hook, filter or LFS download runs.
+  The temporary directory is removed afterwards, also on Ctrl+C, SIGTERM or
+  SIGHUP, including while git is running (exit `130`, `143`, `129`). git runs
+  without a shell, with `--` before the URL and all credential prompts off
+  (`core.askPass=`, `GIT_TERMINAL_PROMPT=0`, `GCM_INTERACTIVE=never`), so a
+  private repository works only with credentials git already has (a
   credential helper, or a token in the URL).
-- **Credentials.** A token in the URL is used to clone and nothing else: it is
+- **Credentials.** A token in the URL is used to clone and nothing else. It is
+  removed from the URL git sees and passed as an `Authorization` header through
+  `GIT_CONFIG_*` environment entries (git 2.31 or newer), so it never appears
+  in git's argument list, the clone's `.git/config` or a credential helper.
+  git would send that header on to wherever a redirect leads, so redirects are
+  not followed while a token is in use, even if your git config follows them
+  for that URL. A server that redirects (for example GitLab without `.git`)
+  fails with `usage_error`: use the repository's final URL. The token is
   removed from the recorded source and masked (`https://***@host`) in every
   message and envelope.
 

packages/cli/src/commands/add.ts

@@ -2,9 +2,10 @@ import { join } from 'node:path';
 import { Command } from 'commander';
 import chalk from 'chalk';
 import { applyAdd, planAdd } from '../lib/add/add-install.js';
-import { parseAddSource, redactCredentials, searchPathOf } from '../lib/add/add-source.js';
+import { parseAddSource, redactCredentials, resolveTreeRef, searchPathOf, treeRefCandidates } from '../lib/add/add-source.js';
 import type { AddSource } from '../lib/add/add-source.js';
 import { openGitSource } from '../lib/add/git-source.js';
+import { remoteOf, remoteRefNames } from '../lib/add/git-run.js';
 import { openLocalSource } from '../lib/add/local-source.js';
 import { discoverSkills, prepareSkill } from '../lib/add/skill-discovery.js';
 import type { SourceSnapshot } from '../lib/add/skill-discovery.js';
@@ -36,6 +37,13 @@ export function openSource(source: AddSource): SourceSnapshot {
   return openGitSource(source.cloneUrl, source.label, source.kind === 'github' ? source.ref : undefined);
 }
 
+/** A `/tree/<ref>/<path>` URL with several segments, split where the remote has a branch or tag. */
+export function settleSource(source: AddSource, transport = 'https'): AddSource {
+  const candidates = treeRefCandidates(source);
+  if (source.kind !== 'github' || candidates.length === 0) return source;
+  return resolveTreeRef(source, remoteRefNames(remoteOf(source.cloneUrl), source.label, candidates, transport));
+}
+
 /** Where a skill came from: the source plus the skill's directory in it, never credentials. */
 export function originOf(source: AddSource, dir: string): string {
   if (source.kind === 'local') return dir === '' ? source.dir : join(source.dir, ...dir.split('/'));
@@ -44,10 +52,11 @@ export function originOf(source: AddSource, dir: string): string {
 }
 
 export async function runAdd(input: string, options: AddOptions): Promise<AddResult> {
-  const source = parseAddSource(input);
-  // Resolve the runtime before any clone, so a bad --runtime fails fast.
+  const parsed = parseAddSource(input);
+  // Resolve the runtime before any network call, so a bad --runtime fails fast.
   const ctx = options.list ? null : contextFrom(options);
   if (ctx !== null) assertCanPrompt(options);
+  const source = settleSource(parsed);
   if (!options.json && source.kind !== 'local') console.error(chalk.dim(`cloning ${source.label} ...`));
   const snapshot = openSource(source);
   try {

packages/cli/src/lib/add/add-source.test.ts

@@ -1,7 +1,7 @@
 import { resolve } from 'node:path';
 import { describe, expect, it } from 'vitest';
 import { CliError } from '../output.js';
-import { parseAddSource, redactCredentials } from './add-source.js';
+import { parseAddSource, redactCredentials, resolveTreeRef, treeRefCandidates } from './add-source.js';
 import { redactError } from '../../commands/add.js';
 
 const refusal = (input: string) => {
@@ -105,4 +105,32 @@ describe('credentials in URLs', () => {
     expect(out.code).toBe('network_error');
     expect((redactError(new Error(`boom https://${token}@h`)) as Error).message).toBe('boom https://***@h');
   });
+
+  it('refuses a URL whose password holds a slash without echoing any of it', () => {
+    for (const input of ['https://user:pa/ss@example.com/r.git', 'https://user:12/ss@example.com/r.git', 'https://tok/en@example.com/r.git']) {
+      const error = refusal(input);
+      expect(error.code).toBe('usage_error');
+      expect(error.message).not.toMatch(/pa\/ss|12\/ss|tok\/en|example/);
+    }
+    expect(redactCredentials('clone https://user:pa/ss@example.com/r.git failed')).toBe('clone https://***@example.com/r.git failed');
+    expect(redactCredentials('https://u:a?b#c@h/x')).toBe('https://***@h/x');
+  });
+
+  it('keeps every segment after /tree/ until the remote says which ones name the ref', () => {
+    const source = parseAddSource('https://github.com/acme/skills/tree/feature/x/skills/a');
+    expect(source).toMatchObject({ ref: 'feature', path: 'x/skills/a', tree: ['feature', 'x', 'skills', 'a'] });
+    expect(treeRefCandidates(source)).toEqual(['feature/x/skills/a', 'feature/x/skills', 'feature/x', 'feature']);
+    expect(resolveTreeRef(source, new Set(['feature/x', 'feature']))).toEqual({
+      kind: 'github', owner: 'acme', repo: 'skills', ref: 'feature/x', path: 'skills/a', cloneUrl: 'https://github.com/acme/skills.git', label: 'github:acme/skills/skills/a@feature/x',
+    });
+    expect(resolveTreeRef(source, new Set(['feature']))).toMatchObject({ ref: 'feature', path: 'x/skills/a' });
+    const unknown = resolveTreeRef(source, new Set());
+    expect(unknown).toMatchObject({ ref: 'feature', path: 'x/skills/a' });
+    expect(unknown).not.toHaveProperty('tree');
+    // A whole URL naming only a ref, or a ref and one directory, still splits per segment.
+    expect(parseAddSource('https://github.com/acme/skills/tree/main')).not.toHaveProperty('tree');
+    expect(treeRefCandidates(parseAddSource('https://github.com/acme/skills/tree/main/skills'))).toEqual(['main/skills', 'main']);
+    // An encoded slash stays inside its segment.
+    expect(resolveTreeRef(parseAddSource('https://github.com/acme/skills/tree/rel%2F1/a/b'), new Set(['rel/1']))).toMatchObject({ ref: 'rel/1', path: 'a/b' });
+  });
 });

packages/cli/src/lib/add/add-source.ts

@@ -16,10 +16,24 @@ import { usageError } from '../install/install-errors.js';
  *
  * Credentials in a URL may be used to clone but never leave this module in a
  * label: `label` strips them, and `redactCredentials` masks them in any text.
+ * A URL that holds an `@` but does not parse, or whose path holds one, is
+ * refused without being echoed, since where its credentials end cannot be
+ * known.
  */
 
 export type AddSource =
-  | { kind: 'github'; owner: string; repo: string; path: string; ref?: string; skill?: string; cloneUrl: string; label: string }
+  | {
+      kind: 'github';
+      owner: string;
+      repo: string;
+      path: string;
+      ref?: string;
+      skill?: string;
+      /** Segments after `/tree/` when there are several: which ones name the ref is settled against the remote (`resolveTreeRef`). */
+      tree?: string[];
+      cloneUrl: string;
+      label: string;
+    }
   | { kind: 'git'; cloneUrl: string; label: string }
   | { kind: 'local'; dir: string; label: string };
 
@@ -33,14 +47,19 @@ const REF = /^(?!-)(?!.*\.\.)(?!.*\/\/)(?!.*@\{)(?!.*\.lock$)(?!.*\/$)[A-Za-z0-9
 const USAGE = 'owner/repo[/skill], github:owner/repo[/path][@ref], an https git URL or a local directory';
 
 /**
- * Masks the userinfo of every `scheme://user:secret@host` in `text`. Quotes
- * end a match, so the result of redacting serialized JSON is still JSON.
+ * Masks the userinfo of every `scheme://user:secret@host` in `text`, up to
+ * the last `@` of the URL, so a secret holding `/`, `?` or `#` is masked whole
+ * (at the cost of masking a path `@` too). Whitespace and quotes end a match,
+ * so the result of redacting serialized JSON is still JSON.
  */
 export function redactCredentials(text: string): string {
-  return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s/?#@"'<>]*(?:@[^\s/?#@"'<>]*)*@/gi, '$1***@');
+  return text.replace(/([a-z][a-z0-9+.-]*:\/\/)[^\s"'<>]*@/gi, '$1***@');
 }
 
-const refuse = (input: string, why: string) => usageError(`refusing source '${redactCredentials(input)}': ${why}`);
+const UNSHOWN_URL =
+  'refusing the source: not a valid git URL (not shown, as it may hold credentials; percent-encode `/`, `?`, `#` and `@` in a user name or password)';
+/** A refusal echoes the input only when it holds no `@`, since where a credential in it ends cannot be known for sure. */
+const refuse = (input: string, why: string) => usageError(input.includes('@') ? `refusing the source: ${why}` : `refusing source '${input}': ${why}`);
 
 function checkRepo(repo: string, input: string): void {
   if (repo === '.' || repo === '..') throw refuse(input, 'the repository name is not a name');
@@ -61,10 +80,38 @@ function checkRef(ref: string, input: string): string {
   return ref;
 }
 
-function github(input: string, owner: string, repo: string, path: string, ref: string | undefined, skill?: string): AddSource {
+const githubLabel = (owner: string, repo: string, path: string, ref?: string) => `github:${owner}/${repo}${path ? `/${path}` : ''}${ref ? `@${ref}` : ''}`;
+
+type GithubSource = Extract<AddSource, { kind: 'github' }>;
+
+function github(input: string, owner: string, repo: string, path: string, ref: string | undefined, skill?: string): GithubSource {
   checkRepo(repo, input);
-  const label = `github:${owner}/${repo}${path ? `/${path}` : ''}${ref ? `@${ref}` : ''}`;
-  return { kind: 'github', owner, repo, path, ...(ref ? { ref } : {}), ...(skill ? { skill } : {}), cloneUrl: `https://github.com/${owner}/${repo}.git`, label };
+  return { kind: 'github', owner, repo, path, ...(ref ? { ref } : {}), ...(skill ? { skill } : {}), cloneUrl: `https://github.com/${owner}/${repo}.git`, label: githubLabel(owner, repo, path, ref) };
+}
+
+/** How a `/tree/a/b/c` URL may split into a ref and a path, longest ref first; refs that are not valid names are left out. */
+function treeSplits(tree: readonly string[]): Array<{ ref: string; path: string }> {
+  return tree
+    .map((_, i) => ({ ref: tree.slice(0, tree.length - i).join('/'), path: tree.slice(tree.length - i).join('/') }))
+    .filter(({ ref }) => REF.test(ref));
+}
+
+/** The ref names a `/tree/` URL may mean, to look up on the remote. */
+export const treeRefCandidates = (source: AddSource): string[] => (source.kind === 'github' && source.tree ? treeSplits(source.tree).map((s) => s.ref) : []);
+
+/**
+ * Settles a `/tree/<ref>/<path>` URL against the refs the remote has: a branch
+ * name may hold `/`, so the longest leading segments that name a branch or tag
+ * are the ref and the rest the path. With no match the first segment stays the
+ * ref, and the clone reports it missing.
+ */
+export function resolveTreeRef(source: AddSource, remoteRefs: ReadonlySet<string>): AddSource {
+  if (source.kind !== 'github' || !source.tree) return source;
+  const { tree, ...rest } = source;
+  const split = treeSplits(tree).find((s) => remoteRefs.has(s.ref));
+  if (split === undefined) return rest;
+  const path = checkRepoPath(split.path, source.label);
+  return { ...rest, ref: split.ref, path, label: githubLabel(source.owner, source.repo, path, split.ref) };
 }
 
 function fromUrl(input: string): AddSource {
@@ -74,8 +121,11 @@ function fromUrl(input: string): AddSource {
   try {
     url = new URL(input);
   } catch {
+    if (input.includes('@')) throw usageError(UNSHOWN_URL);
     throw refuse(input, 'not a valid URL');
   }
+  // `https://user:12/ss@host` parses as host `user`: an `@` past the host means a secret may sit in the path.
+  if (url.pathname.includes('@')) throw usageError(UNSHOWN_URL);
   if (url.protocol !== 'https:') throw refuse(input, `only https git URLs are accepted, not ${url.protocol.replace(/:$/, '')}`);
   if (url.hostname === '') throw refuse(input, 'the URL has no host');
   if (url.search !== '' || url.hash !== '') throw refuse(input, 'a git URL takes no query or fragment');
@@ -90,7 +140,9 @@ function fromUrl(input: string): AddSource {
         throw refuse(input, 'the URL path is not valid percent-encoding');
       }
     };
-    return github(input, gh[1]!, gh[2]!, checkRepoPath(decode(gh[4] ?? ''), input), gh[3] ? checkRef(decode(gh[3]), input) : undefined);
+    const source = github(input, gh[1]!, gh[2]!, checkRepoPath(decode(gh[4] ?? ''), input), gh[3] ? checkRef(decode(gh[3]), input) : undefined);
+    const tree = gh[3] && gh[4] ? [gh[3], ...(gh[4].split('/').filter(Boolean))].map(decode) : [];
+    return tree.length > 1 ? { ...source, tree } : source;
   }
   const bare = new URL(url.href);
   bare.username = '';
@@ -122,7 +174,7 @@ export function parseAddSource(input: string, cwd: string = process.cwd()): AddS
   if (raw.includes('://') || raw.includes('::') || /^[^/:]+@[^/:]+:/.test(raw)) return fromUrl(raw);
   const m = SHORTHAND.exec(raw);
   if (m) return github(raw, m[1]!, m[2]!, '', undefined, m[3]);
-  throw usageError(`'${redactCredentials(raw)}' is not a source: ${USAGE}`);
+  throw usageError(`${raw.includes('@') ? 'the source' : `'${raw}'`} is not a source: ${USAGE}`);
 }
 
 /** Where discovery starts inside the source: the `github:` path, else the root. */