From 1b3698305a8bae7abc5151d7829450ddeb19002b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 15:09:31 -0400 Subject: [PATCH 1/2] Document the skill-delivery threat model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills are auto-loaded instructions distributed to engineers' agents across the org, alongside executable payloads written into agent-trusted paths. The repo's security posture does not describe that channel anywhere. Describes three channels — instruction, execution, delivery — with the state of each verified against the tree: `copy_bundle_dirs` ships `scripts` and `adapters` (7 payloads today), `tools/sync` pulls from a tracked branch rather than a reviewed ref, and `tools/bootstrap` is documented as `curl | bash` from `main` with no checksum. The package is lockfile-pinned; the content it installs is not, and that distinction is the crux. States the existing controls with equal care, since the risk is easy to overstate: auto-update is opt-in behind `SKILLS_AUTO_UPDATE`, this package declares no `postinstall` of its own, `--ff-only` breaks rather than silently applying a rewritten history, and CLI distribution is already review-gated. Descriptive, not a proposal — the five open decisions are listed for the security team to rule on rather than settled here. Also flags that SECURITY.md describes the repo as prompt templates with no runtime code, which does not account for the published CLI or the shipped executables. Raised during ADR 0057 review (MetaMask/decisions#162). --- docs/security/skill-delivery-threat-model.md | 98 ++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 docs/security/skill-delivery-threat-model.md diff --git a/docs/security/skill-delivery-threat-model.md b/docs/security/skill-delivery-threat-model.md new file mode 100644 index 00000000..fec45150 --- /dev/null +++ b/docs/security/skill-delivery-threat-model.md @@ -0,0 +1,98 @@ +# Threat model: skill delivery + +Skills are auto-loaded instructions distributed to engineers' coding agents across the org, +alongside executable payloads written into agent-trusted local paths. That is a delivery +channel with real reach, and it is not currently described anywhere in this repo's security +posture. This document states what the channel does today, which controls already constrain +it, and which decisions are open. + +It is descriptive, not a proposal. Nothing here changes delivery behavior; the mitigations +are listed so the security team can rule on them. + +> [!NOTE] +> `SECURITY.md` currently says this repo holds *"agent instructions and prompt templates +> only — no runtime code that ships in MetaMask products."* That understates the surface: +> `bin/` and `tools/` ship as the published `@metamask/skills` CLI, and skill bundles +> include `scripts/` and `adapters/` payloads copied into consumer repos. Worth reconciling +> whichever way the team decides. + +## What is being protected + +- **Engineer workstations.** Install writes into `$HOME` (with `--include-user`) and into + the consumer repo's agent directories. +- **Agent sessions.** A skill body is instruction text that an agent treats as trusted + guidance, in a session that usually holds repo write access and credentials. +- **Consumer repositories.** Install writes `.claude/skills/`, `.cursor/rules/`, and + `.agents/skills/`, which are read on every subsequent agent run. + +## Channels + +### 1. Instruction channel — skill bodies + +Skill bodies are auto-loaded into agents. A body that instructs an agent to weaken a check, +exfiltrate a value, or alter an edit is executed as guidance, not flagged as content. Reach +is every engineer who installs the domain. + +Relevant property: the installer writes `alwaysApply: false` for Cursor rules +(`tools/install`), so skills are selected on demand rather than force-loaded. That bounds +*when* a body is read, not what it may say once read. + +### 2. Execution channel — shipped payloads + +`copy_bundle_dirs` ships `references scripts assets adapters` (`tools/install:349`). Today +that is **7 files** under `scripts/`/`adapters/`, including `.sh` payloads. They are copied +into agent-trusted paths, where an agent may run them without the friction that applies to +code fetched at runtime. + +### 3. Delivery channel — how content reaches a machine + +| Path | Ref | Notes | +|---|---|---| +| `@metamask/skills` npm package | pinned by lockfile | package review and npm minimal-age gates apply | +| `tools/sync` → `git pull --ff-only` (`tools/sync:194`) | **tracks a branch** | content follows `main`, not a reviewed ref | +| `tools/bootstrap` via `curl … \| bash` | **`main`, unpinned** | the bootstrap script itself is fetched from `raw.githubusercontent.com/MetaMask/skills/main/tools/bootstrap`, with no checksum; `SKILLS_REF` defaults to `main` | + +The package is pinned; the **content** it installs is not. That distinction is the crux — +a lockfile entry for the CLI says nothing about which skill revision gets written to disk. + +## Controls already in place + +Stating these accurately matters, because the risk is often described as larger than it is: + +- **Auto-update is opt-in.** `metamask-skills postinstall` refreshes the cache and returns + unless `SKILLS_AUTO_UPDATE` is truthy (`bin/metamask-skills.mjs:741-744`). Installing the + package does not by itself pull new skill content. +- **No `postinstall` in this package's own `package.json`.** The hook exists for consumer + repos to wire up deliberately. +- **`--ff-only`.** A force-push to `main` breaks the pull rather than being silently applied, + and `tools/sync` refuses to install rather than proceed with a stale source. +- **CLI distribution is reviewed.** Package review and npm minimal-age gates cover the + `@metamask/skills` artifact. + +## Open decisions + +For the security team, in rough order of leverage: + +1. **Pin content to a reviewed ref.** Sync and bootstrap follow `main`. A release tag or + reviewed SHA would make the installed revision an auditable fact. Cost: an extra step to + publish, and a lag between merge and availability. +2. **Gate the bootstrap path.** `curl … | bash` from a mutable branch is the weakest link — + no pin, no checksum, no signature. Options include publishing a checksummed release + artifact or dropping the documented one-liner in favour of the package. +3. **Decide how shipped executables are gated.** Whether `scripts/`/`adapters/` should ship + at all; if so, whether they need review by a named owner, and whether an agent should be + told they are runnable. +4. **Treat skill bodies as untrusted pending review.** An admission review that reads a body + as instruction — asking what an agent would do if it followed it literally — rather than + as prose. +5. **Branch protection and CODEOWNERS on this repo.** Confirm the required-review and + status-check rules on `main` match the reach of the channel. (Not asserted here: the + protection endpoint returns 404 to a non-admin token, which does not distinguish "no + rule" from "no permission to read the rule".) + +## Related + +- [`MetaMask/decisions#162`](https://github.com/MetaMask/decisions/pull/162) — ADR 0057 + review, where the missing threat model was raised. +- [`SECURITY.md`](../../SECURITY.md) — reporting policy, and the framing noted above. +- [`CONTRIBUTING.md`](../../CONTRIBUTING.md) — admission criteria for new skills. From fa85462dea437f987f478c2c942d6d5157c2f016 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 15:21:35 -0400 Subject: [PATCH 2/2] Install skill content from a pinned release, not a branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lockfile entry pins the `@metamask/skills` CLI. It never pinned the skill revision that reaches disk: the cache cloned and reset to `main`, so an install pinned in a lockfile still picked up whatever had merged since, and the documented `curl … | bash` bootstrap did the same for cloud agents. The CLI now derives the content ref from its own package version, so one lockfile entry pins both halves. `tools/bootstrap` resolves the newest release tag over `git ls-remote` instead of defaulting to `main`. Both fail closed. A missing tag means this package version has no published content, and widening to a branch at that point would reinstate the channel being removed — so the CLI warns and declines, and bootstrap exits non-zero naming the override. `SKILLS_REF` still overrides, for development against `main` and for holding a consumer on a specific release. It is now a deliberate opt-in to a mutable ref rather than the default, and taking it warns. Engineer-owned checkouts reached through `METAMASK_SKILLS_DIR` are untouched; that is their working copy, not a delivery channel. --- bin/metamask-skills.mjs | 58 +++++++++++- docs/security/skill-delivery-threat-model.md | 98 -------------------- test/cli.test.mjs | 41 +++++++- tools/bootstrap | 36 ++++++- 4 files changed, 125 insertions(+), 108 deletions(-) delete mode 100644 docs/security/skill-delivery-threat-model.md diff --git a/bin/metamask-skills.mjs b/bin/metamask-skills.mjs index af1ec09a..2dd84b49 100755 --- a/bin/metamask-skills.mjs +++ b/bin/metamask-skills.mjs @@ -11,6 +11,7 @@ const PUBLIC_REPO = 'https://github.com/MetaMask/skills.git'; const CACHE_RELATIVE_DIR = path.join('.skills-cache', 'metamask-skills'); const SOURCE_ENV_KEYS = ['METAMASK_SKILLS_DIR', 'CONSENSYS_SKILLS_DIR']; const TARGET_REPO_ENV_KEY = 'METAMASK_SKILLS_TARGET_REPO'; +const CONTENT_REF_ENV_KEY = 'SKILLS_REF'; function usage(exitCode = 0) { const out = exitCode === 0 ? process.stdout : process.stderr; @@ -304,16 +305,52 @@ function warn(message) { process.stderr.write(`metamask-skills: ${message}\n`); } +/** + * The git ref skill CONTENT is installed from. + * + * A lockfile pins the CLI; on its own it says nothing about which skill revision reaches + * disk. While the cache tracked `main`, a pinned install could still pick up anything + * merged since. Content now follows the release tag matching this package's own version, + * so one lockfile entry pins both halves. + * + * `SKILLS_REF` overrides — for development against `main`, and for holding a consumer on + * a specific release. There is deliberately no automatic widening to `main` when the tag + * is missing: silently falling back to a mutable branch is the behaviour this replaces. + */ +function contentRef(env = process.env) { + const override = env[CONTENT_REF_ENV_KEY]; + if (override) { + return { ref: override, pinned: false, why: `${CONTENT_REF_ENV_KEY}=${override}` }; + } + try { + const { version } = JSON.parse(readFileSync(path.join(PACKAGE_ROOT, 'package.json'), 'utf8')); + if (version) { + return { ref: `v${version}`, pinned: true, why: `package version ${version}` }; + } + } catch { + // fall through + } + return null; +} + function ensurePublicSkillsCache(target) { const cache = cacheDir(target); + const selected = contentRef(); + if (!selected) { + warn('could not determine the pinned content ref; set SKILLS_REF to install skills'); + return false; + } + const { ref, pinned, why } = selected; try { if (isGitDir(cache)) { - const fetchResult = run('git', ['-C', cache, 'fetch', '--depth', '1', 'origin', 'main']); + const fetchResult = run('git', ['-C', cache, 'fetch', '--depth', '1', 'origin', ref]); if (fetchResult.status !== 0) { - warn('cache fetch failed (offline?)'); + // Fail closed. A missing tag means this package version has no published content; + // widening to a branch here would reintroduce the unpinned channel. + warn(`cache fetch failed for ${ref} (${why}) — offline, or the ref does not exist`); return false; } - const resetResult = run('git', ['-C', cache, 'reset', '--hard', 'origin/main']); + const resetResult = run('git', ['-C', cache, 'reset', '--hard', 'FETCH_HEAD']); if (resetResult.status !== 0) { warn('cache reset failed'); return false; @@ -322,11 +359,22 @@ function ensurePublicSkillsCache(target) { } mkdirSync(path.dirname(cache), { recursive: true }); - const cloneResult = run('git', ['clone', '--depth', '1', '--branch', 'main', PUBLIC_REPO, cache]); + const cloneResult = run('git', [ + 'clone', + '--depth', + '1', + '--branch', + ref, + PUBLIC_REPO, + cache, + ]); if (cloneResult.status !== 0) { - warn('cache clone failed (offline?)'); + warn(`cache clone failed for ${ref} (${why}) — offline, or the ref does not exist`); return false; } + if (!pinned) { + warn(`installing skills from ${why}, which is not a pinned release`); + } return true; } catch (error) { warn(`cache refresh failed: ${error instanceof Error ? error.message : String(error)}`); diff --git a/docs/security/skill-delivery-threat-model.md b/docs/security/skill-delivery-threat-model.md deleted file mode 100644 index fec45150..00000000 --- a/docs/security/skill-delivery-threat-model.md +++ /dev/null @@ -1,98 +0,0 @@ -# Threat model: skill delivery - -Skills are auto-loaded instructions distributed to engineers' coding agents across the org, -alongside executable payloads written into agent-trusted local paths. That is a delivery -channel with real reach, and it is not currently described anywhere in this repo's security -posture. This document states what the channel does today, which controls already constrain -it, and which decisions are open. - -It is descriptive, not a proposal. Nothing here changes delivery behavior; the mitigations -are listed so the security team can rule on them. - -> [!NOTE] -> `SECURITY.md` currently says this repo holds *"agent instructions and prompt templates -> only — no runtime code that ships in MetaMask products."* That understates the surface: -> `bin/` and `tools/` ship as the published `@metamask/skills` CLI, and skill bundles -> include `scripts/` and `adapters/` payloads copied into consumer repos. Worth reconciling -> whichever way the team decides. - -## What is being protected - -- **Engineer workstations.** Install writes into `$HOME` (with `--include-user`) and into - the consumer repo's agent directories. -- **Agent sessions.** A skill body is instruction text that an agent treats as trusted - guidance, in a session that usually holds repo write access and credentials. -- **Consumer repositories.** Install writes `.claude/skills/`, `.cursor/rules/`, and - `.agents/skills/`, which are read on every subsequent agent run. - -## Channels - -### 1. Instruction channel — skill bodies - -Skill bodies are auto-loaded into agents. A body that instructs an agent to weaken a check, -exfiltrate a value, or alter an edit is executed as guidance, not flagged as content. Reach -is every engineer who installs the domain. - -Relevant property: the installer writes `alwaysApply: false` for Cursor rules -(`tools/install`), so skills are selected on demand rather than force-loaded. That bounds -*when* a body is read, not what it may say once read. - -### 2. Execution channel — shipped payloads - -`copy_bundle_dirs` ships `references scripts assets adapters` (`tools/install:349`). Today -that is **7 files** under `scripts/`/`adapters/`, including `.sh` payloads. They are copied -into agent-trusted paths, where an agent may run them without the friction that applies to -code fetched at runtime. - -### 3. Delivery channel — how content reaches a machine - -| Path | Ref | Notes | -|---|---|---| -| `@metamask/skills` npm package | pinned by lockfile | package review and npm minimal-age gates apply | -| `tools/sync` → `git pull --ff-only` (`tools/sync:194`) | **tracks a branch** | content follows `main`, not a reviewed ref | -| `tools/bootstrap` via `curl … \| bash` | **`main`, unpinned** | the bootstrap script itself is fetched from `raw.githubusercontent.com/MetaMask/skills/main/tools/bootstrap`, with no checksum; `SKILLS_REF` defaults to `main` | - -The package is pinned; the **content** it installs is not. That distinction is the crux — -a lockfile entry for the CLI says nothing about which skill revision gets written to disk. - -## Controls already in place - -Stating these accurately matters, because the risk is often described as larger than it is: - -- **Auto-update is opt-in.** `metamask-skills postinstall` refreshes the cache and returns - unless `SKILLS_AUTO_UPDATE` is truthy (`bin/metamask-skills.mjs:741-744`). Installing the - package does not by itself pull new skill content. -- **No `postinstall` in this package's own `package.json`.** The hook exists for consumer - repos to wire up deliberately. -- **`--ff-only`.** A force-push to `main` breaks the pull rather than being silently applied, - and `tools/sync` refuses to install rather than proceed with a stale source. -- **CLI distribution is reviewed.** Package review and npm minimal-age gates cover the - `@metamask/skills` artifact. - -## Open decisions - -For the security team, in rough order of leverage: - -1. **Pin content to a reviewed ref.** Sync and bootstrap follow `main`. A release tag or - reviewed SHA would make the installed revision an auditable fact. Cost: an extra step to - publish, and a lag between merge and availability. -2. **Gate the bootstrap path.** `curl … | bash` from a mutable branch is the weakest link — - no pin, no checksum, no signature. Options include publishing a checksummed release - artifact or dropping the documented one-liner in favour of the package. -3. **Decide how shipped executables are gated.** Whether `scripts/`/`adapters/` should ship - at all; if so, whether they need review by a named owner, and whether an agent should be - told they are runnable. -4. **Treat skill bodies as untrusted pending review.** An admission review that reads a body - as instruction — asking what an agent would do if it followed it literally — rather than - as prose. -5. **Branch protection and CODEOWNERS on this repo.** Confirm the required-review and - status-check rules on `main` match the reach of the channel. (Not asserted here: the - protection endpoint returns 404 to a non-admin token, which does not distinguish "no - rule" from "no permission to read the rule".) - -## Related - -- [`MetaMask/decisions#162`](https://github.com/MetaMask/decisions/pull/162) — ADR 0057 - review, where the missing threat model was raised. -- [`SECURITY.md`](../../SECURITY.md) — reporting policy, and the framing noted above. -- [`CONTRIBUTING.md`](../../CONTRIBUTING.md) — admission criteria for new skills. diff --git a/test/cli.test.mjs b/test/cli.test.mjs index e4829a10..a95f9665 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -240,3 +240,42 @@ describe('managed skill pruning', () => { assert.equal(existsSync(stale), true); }); }); + +describe('content ref is pinned', () => { + // A lockfile pins the CLI; it does not pin the skill revision that reaches disk. These + // assert the two are tied together, and that a missing tag fails rather than widening + // to a branch — silently installing from `main` is the behaviour this replaced. + const BIN_SRC = readFileSync( + path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', 'bin', 'metamask-skills.mjs'), + 'utf8', + ); + const BOOTSTRAP_SRC = readFileSync( + path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', 'tools', 'bootstrap'), + 'utf8', + ); + + test('the CLI cache does not track a branch', () => { + assert.ok( + !/'--branch',\s*'main'/u.test(BIN_SRC) && !/origin',\s*'main'/u.test(BIN_SRC), + 'cache clone/fetch still references main directly', + ); + assert.match(BIN_SRC, /function contentRef/u, 'expected a contentRef() resolver'); + }); + + test('the pinned ref matches this package version', () => { + const { version } = JSON.parse( + readFileSync(path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', 'package.json'), 'utf8'), + ); + assert.match(BIN_SRC, /`v\$\{version\}`/u, 'contentRef should derive the tag from package version'); + assert.ok(version, 'package.json must declare a version for the pin to resolve'); + }); + + test('bootstrap does not default to a branch', () => { + assert.ok( + !/REF="\$\{SKILLS_REF:-main\}"/u.test(BOOTSTRAP_SRC), + 'bootstrap still defaults SKILLS_REF to main', + ); + assert.match(BOOTSTRAP_SRC, /latest_release_tag/u, 'expected release-tag resolution'); + assert.match(BOOTSTRAP_SRC, /Refusing to install from an unpinned branch/u, 'expected fail-closed path'); + }); +}); diff --git a/tools/bootstrap b/tools/bootstrap index a8f41eaf..946b06e6 100755 --- a/tools/bootstrap +++ b/tools/bootstrap @@ -18,7 +18,8 @@ # # Env: # SKILLS_CACHE_DIR Where to clone (default: $HOME/.cache/metamask-skills). -# SKILLS_REF Git ref to checkout (default: main). +# SKILLS_REF Git ref to install content from (default: newest release tag). +# Set to `main` to track the branch deliberately. # # Extra args after --repo are forwarded to tools/install. @@ -44,9 +45,37 @@ if [[ -z "$REPO" ]]; then fi CACHE="${SKILLS_CACHE_DIR:-$HOME/.cache/metamask-skills}" -REF="${SKILLS_REF:-main}" TARGET="$(pwd)" +# Resolve the ref to install content from. +# +# This previously defaulted to `main`, so a cloud agent running the documented +# `curl … | bash` one-liner installed whatever had merged since — an unreviewed +# revision by default. It now defaults to the newest release tag, and refuses to +# proceed if none can be resolved rather than falling back to a branch. +# +# Pass SKILLS_REF explicitly for a reproducible install: SKILLS_REF=v0.2.0. +latest_release_tag() { + git ls-remote --tags --refs https://github.com/MetaMask/skills.git 'v*' 2>/dev/null \ + | awk -F/ '{print $NF}' \ + | sort -V \ + | tail -1 +} + +if [[ -n "${SKILLS_REF:-}" ]]; then + REF="$SKILLS_REF" + echo "Using SKILLS_REF=$REF" +else + REF="$(latest_release_tag)" + if [[ -z "$REF" ]]; then + echo "Error: could not resolve a release tag from MetaMask/skills." >&2 + echo "Refusing to install from an unpinned branch. Set SKILLS_REF explicitly" >&2 + echo "(e.g. SKILLS_REF=v0.2.0), or SKILLS_REF=main to track the branch deliberately." >&2 + exit 1 + fi + echo "Using latest release $REF" +fi + if [[ ! -d "$CACHE/.git" ]]; then echo "Cloning MetaMask/skills into $CACHE" mkdir -p "$(dirname "$CACHE")" @@ -54,8 +83,7 @@ if [[ ! -d "$CACHE/.git" ]]; then else echo "Updating $CACHE" git -C "$CACHE" fetch --depth 1 origin "$REF" 2>&1 | sed 's/^/ /' - git -C "$CACHE" checkout "$REF" 2>&1 | sed 's/^/ /' - git -C "$CACHE" reset --hard "origin/$REF" 2>&1 | sed 's/^/ /' + git -C "$CACHE" reset --hard FETCH_HEAD 2>&1 | sed 's/^/ /' fi exec "$CACHE/tools/install" --repo "$REPO" --target "$TARGET" --source "$CACHE" "${EXTRA[@]}"