fix(gh-wrapper): rename the seat-token env key out of the PAPERCLIP_ namespace (BLO-18927)

scripts/gh-token-wrapper.sh

@@ -14,33 +14,64 @@
 # existing.
 #
 # A second, volume-free delivery path exists for credentials bound per-agent
-# rather than mounted fleet-wide: see PAPERCLIP_GITHUB_TOKEN_VALUE below.
+# rather than mounted fleet-wide: see GH_SEAT_TOKEN_VALUE below.
 set -eu
 
 TOKEN_FILE="${PAPERCLIP_GITHUB_TOKEN_FILE:-/paperclip/.secrets/github-token/token}"
 REAL_GH="${GH_TOKEN_WRAPPER_REAL_GH:-/usr/bin/gh.real}"
 
-# PAPERCLIP_GITHUB_TOKEN_VALUE carries a token *value* rather than a path, for
-# credentials delivered by the scoped secret-binding path (per-agent /
-# per-project env bindings) instead of by a mounted secret volume. It exists so
-# a credential can be given to specific agents without mounting it into every
-# agent pod: the k8s adapters propagate every main-container secret volume into
-# every Job pod with no agent or tenant filter (BLO-18927, BLO-18970), so a
-# volume-delivered secret is necessarily fleet-wide.
+# GH_SEAT_TOKEN_VALUE carries a token *value* rather than a path, for
+# credentials delivered by the scoped secret-binding path (agent-scoped env
+# bindings only) instead of by a mounted secret volume. Project, environment and
+# routine scope are NOT delivery routes for this key: AGENT_SCOPE_ONLY_ENV_KEYS
+# (server/src/services/heartbeat.ts) strips it from all three, so that a
+# lower-trust writer cannot select the identity every `gh` call runs as.
+# It exists so a credential can be given to specific agents without mounting it
+# into every agent pod: the k8s adapters propagate every main-container secret
+# volume into every Job pod with no agent or tenant filter (BLO-18927,
+# BLO-18970), so a volume-delivered secret is necessarily fleet-wide.
+#
+# The name deliberately does NOT start with `PAPERCLIP_`. Do not "fix" it for
+# consistency with the FILE variable below — the prefix is load-bearing in the
+# opposite direction. `isPaperclipRuntimeEnvKey` (server/src/services/
+# heartbeat.ts) strips every `PAPERCLIP_*` key out of adapter, environment,
+# project and routine env, and agent-scope binding resolution reads that
+# already-stripped config. A `PAPERCLIP_`-prefixed name is therefore
+# unreachable from the very binding path this branch exists to serve: it would
+# be silently deleted server-side and fall through to the file branch with no
+# error anywhere. That guard is correct and must stay — it stops user config
+# overriding paperclip's own runtime env — so the credential moves out of its
+# namespace instead. See BLO-18927 step 2.
+#
+# The FILE variable keeps its `PAPERCLIP_` prefix on purpose, for the same
+# reason inverted: it selects a path on disk, and being strippable is what
+# stops project/environment config from redirecting the file branch.
 #
 # Precedence: value > file. Both are *explicit caller selections* of an identity
 # for one invocation — the same trust model the FILE variable already had, since
-# a caller could always point that at a file it wrote. This is deliberately NOT
-# GH_TOKEN: the override below must keep clobbering GH_TOKEN unconditionally
-# (BLO-13241), so GH_TOKEN cannot double as an input without reopening that bug.
-if [ "${PAPERCLIP_GITHUB_TOKEN_VALUE+x}" = x ]; then
+# a caller could always point that at a file it wrote.
+#
+# Dropping the `PAPERCLIP_` prefix would otherwise have widened who can set this
+# key: environment/project/routine env are overlaid *after* agent-scope
+# resolution, so the lowest-trust writer would win and could swap the identity
+# `gh` runs as, or park whitespace here and fail every invocation with exit 64.
+# The prefix used to prevent that for free. It is now prevented explicitly
+# instead: AGENT_SCOPE_ONLY_ENV_KEYS in server/src/services/heartbeat.ts strips
+# this key from environment, project and routine env, so only an agent-scoped
+# secret binding can set it. Keep those two in sync — renaming here without
+# renaming there silently reopens the hole.
+#
+# This is deliberately NOT GH_TOKEN: the override below must keep clobbering
+# GH_TOKEN unconditionally (BLO-13241), so GH_TOKEN cannot double as an input
+# without reopening that bug.
+if [ "${GH_SEAT_TOKEN_VALUE+x}" = x ]; then
   # Trim surrounding whitespace only — a secret that arrives via a templated
   # env binding routinely picks up a trailing newline, and tabs/spaces are just
   # as likely as CR/LF from a YAML block scalar. Deliberately a trim rather than
   # a delete: `tr -d` would silently splice "ghu_aaa\nbbb" into the single
   # plausible-looking token "ghu_aaabbb" and authenticate as nobody-in-
   # particular, which is the failure mode this whole branch exists to avoid.
-  TOKEN="${PAPERCLIP_GITHUB_TOKEN_VALUE}"
+  TOKEN="${GH_SEAT_TOKEN_VALUE}"
   while :; do
     case "${TOKEN}" in
       [[:space:]]*) TOKEN="${TOKEN#?}" ;;
@@ -54,15 +85,15 @@ if [ "${PAPERCLIP_GITHUB_TOKEN_VALUE+x}" = x ]; then
   # continuing would run `gh` under whatever ambient GH_TOKEN/GITHUB_TOKEN the
   # caller happened to inherit — an unintended identity, silently.
   if [ -z "${TOKEN}" ]; then
-    echo "gh-token-wrapper: PAPERCLIP_GITHUB_TOKEN_VALUE is set but holds only whitespace; refusing to run with ambient auth" >&2
+    echo "gh-token-wrapper: GH_SEAT_TOKEN_VALUE is set but holds only whitespace; refusing to run with ambient auth" >&2
     exit 64
   fi
   case "${TOKEN}" in
     *[[:space:]]*)
       # No GitHub token format contains whitespace, so this is either a
       # concatenation of two values or a corrupted binding. Refuse rather than
       # guess which half was meant. The value itself is never echoed.
-      echo "gh-token-wrapper: PAPERCLIP_GITHUB_TOKEN_VALUE contains embedded whitespace; refusing to guess at the intended token" >&2
+      echo "gh-token-wrapper: GH_SEAT_TOKEN_VALUE contains embedded whitespace; refusing to guess at the intended token" >&2
       exit 64
       ;;
   esac

scripts/gh-token-wrapper.test.mjs

@@ -29,13 +29,13 @@ function withTempDir(fn) {
 // branch has to clear all of them, because the wrapper's whole job is to pick a
 // branch based on which are set — inheriting one from the ambient environment
 // silently re-points the test at a different branch than it names. This is not
-// hypothetical: these tests run inside agent pods, and PAPERCLIP_GITHUB_TOKEN_VALUE
+// hypothetical: these tests run inside agent pods, and GH_SEAT_TOKEN_VALUE
 // is exactly what the scoped secret-binding path (BLO-18927) exports there.
 const WRAPPER_CREDENTIAL_ENV_VARS = [
   "GH_TOKEN",
   "GITHUB_TOKEN",
   "PAPERCLIP_GITHUB_TOKEN_FILE",
-  "PAPERCLIP_GITHUB_TOKEN_VALUE",
+  "GH_SEAT_TOKEN_VALUE",
 ];
 
 // The single way any test in this file builds an environment. Starts from a
@@ -57,7 +57,7 @@ function runWrapper(dir, { tokenFileContent, tokenValue, args = ["api", "user"]
   const env = sanitizedEnv({ GH_TOKEN_WRAPPER_REAL_GH: stubGhPath });
 
   if (tokenValue !== undefined) {
-    env.PAPERCLIP_GITHUB_TOKEN_VALUE = tokenValue;
+    env.GH_SEAT_TOKEN_VALUE = tokenValue;
   }
 
   if (tokenFileContent !== undefined) {
@@ -183,7 +183,7 @@ test("logs a diagnostic to stderr and falls back when the token file exists but
   });
 });
 
-// PAPERCLIP_GITHUB_TOKEN_VALUE — credentials delivered by the scoped
+// GH_SEAT_TOKEN_VALUE — credentials delivered by the scoped
 // secret-binding path rather than a mounted secret volume (BLO-18927).
 
 test("exports a token supplied by value when no token file exists", () => {
@@ -232,7 +232,7 @@ function runMalformedValue(dir, tokenValue) {
     env: sanitizedEnv({
       GH_TOKEN_WRAPPER_REAL_GH: stubGhPath,
       PAPERCLIP_GITHUB_TOKEN_FILE: tokenFilePath,
-      PAPERCLIP_GITHUB_TOKEN_VALUE: tokenValue,
+      GH_SEAT_TOKEN_VALUE: tokenValue,
       GH_TOKEN: "user_supplied_override",
       GITHUB_TOKEN: "user_supplied_override",
     }),
@@ -252,7 +252,7 @@ for (const [label, tokenValue] of [
     withTempDir((dir) => {
       const proc = runMalformedValue(dir, tokenValue);
       assert.equal(proc.status, 64);
-      assert.match(proc.stderr, /PAPERCLIP_GITHUB_TOKEN_VALUE is set but holds only whitespace/);
+      assert.match(proc.stderr, /GH_SEAT_TOKEN_VALUE is set but holds only whitespace/);
       assert.equal(proc.stdout, "");
     });
   });
@@ -294,7 +294,7 @@ test("a token supplied by value overrides a pre-existing GH_TOKEN in the caller'
 
     const env = sanitizedEnv({
       GH_TOKEN_WRAPPER_REAL_GH: stubGhPath,
-      PAPERCLIP_GITHUB_TOKEN_VALUE: "ghu_userseat",
+      GH_SEAT_TOKEN_VALUE: "ghu_userseat",
       GH_TOKEN: "user_supplied_override",
       GITHUB_TOKEN: "user_supplied_override",
     });
@@ -353,7 +353,7 @@ test("Dockerfile.runtime points git's credential helper at the wrapper, not gh.r
 // environment. Before the sanitized-env helper, that run failed two tests: the
 // GH_TOKEN-override test authenticated as the inherited value instead of the
 // file's, and the unreadable-file test never emitted its diagnostic, because
-// the inherited PAPERCLIP_GITHUB_TOKEN_VALUE sent both down the value branch.
+// the inherited GH_SEAT_TOKEN_VALUE sent both down the value branch.
 // A plain assertion inside a single test cannot catch that class of bug — the
 // leak is in how each test builds its environment, so the check has to be a
 // second run of every test under a dirty one.
@@ -366,7 +366,7 @@ if (!process.env.GH_TOKEN_WRAPPER_TEST_NESTED) {
       GH_TOKEN: "ambient_caller_token",
       GITHUB_TOKEN: "ambient_caller_token",
       PAPERCLIP_GITHUB_TOKEN_FILE: path.join(os.tmpdir(), "ambient-token-does-not-exist"),
-      PAPERCLIP_GITHUB_TOKEN_VALUE: "ghu_ambient_scoped_binding",
+      GH_SEAT_TOKEN_VALUE: "ghu_ambient_scoped_binding",
     };
     // node:test sets NODE_TEST_CONTEXT=child-v8 in every test-file subprocess.
     // Inheriting it makes the nested run report through the v8 serializer to a

server/src/__tests__/heartbeat-model-profile.test.ts

@@ -151,3 +151,94 @@ describe("heartbeat model profile application", () => {
     expect(isConfigurationIncompleteFailedRun({ errorCode: "provider_quota" })).toBe(false);
   });
 });
+
+// The merged config is handed to resolveExecutionRunAdapterConfig as
+// `executionRunConfig`, which treats it wholesale as agent scope and strips only
+// `PAPERCLIP_*`. issue.assigneeAdapterOverrides.adapterConfig is overlaid into it
+// last and accepts arbitrary keys, so without withAgentScopedEnvProvenance an
+// issue override reaches agent scope — the boundary BLO-18927 exists to draw.
+// GH_SEAT_TOKEN_VALUE selects the identity every `gh` invocation authenticates
+// as, so both introducing and dropping it are exploits, and both are asserted.
+describe("mergeModelProfileAdapterConfig agent-scope-only env boundary", () => {
+  const noProfile = {
+    requested: null,
+    requestedBy: null,
+    applied: null,
+    configSource: null,
+    fallbackReason: null,
+    adapterConfig: null,
+  } as const;
+
+  it("does not let an issue adapter override introduce a seat token the agent never had", () => {
+    const merged = mergeModelProfileAdapterConfig({
+      baseConfig: { env: { AGENT_ONLY: "agent-only" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { env: { GH_SEAT_TOKEN_VALUE: "issue-attacker" } },
+    });
+
+    expect(merged.env).not.toHaveProperty("GH_SEAT_TOKEN_VALUE");
+  });
+
+  it("does not let an issue adapter override replace an agent-scoped seat token", () => {
+    const merged = mergeModelProfileAdapterConfig({
+      baseConfig: { env: { GH_SEAT_TOKEN_VALUE: "agent-seat-token" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { env: { GH_SEAT_TOKEN_VALUE: "issue-attacker" } },
+    });
+
+    expect(merged.env).toMatchObject({ GH_SEAT_TOKEN_VALUE: "agent-seat-token" });
+  });
+
+  // Whitespace in the key fails every `gh` invocation with exit 64, so denial is
+  // as much an exploit as substitution. The shallow overlay spread replaces the
+  // agent's `env` wholesale, which is how an override reaches this without ever
+  // naming the key.
+  it("does not let an issue adapter override drop or blank an agent-scoped seat token", () => {
+    const blanked = mergeModelProfileAdapterConfig({
+      baseConfig: { env: { GH_SEAT_TOKEN_VALUE: "agent-seat-token" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { env: { GH_SEAT_TOKEN_VALUE: "   " } },
+    });
+    const displaced = mergeModelProfileAdapterConfig({
+      baseConfig: { env: { GH_SEAT_TOKEN_VALUE: "agent-seat-token" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { env: { UNRELATED: "issue-only" } },
+    });
+
+    expect(blanked.env).toMatchObject({ GH_SEAT_TOKEN_VALUE: "agent-seat-token" });
+    expect(displaced.env).toMatchObject({ GH_SEAT_TOKEN_VALUE: "agent-seat-token" });
+  });
+
+  it("does not let a model profile overlay introduce a seat token", () => {
+    const merged = mergeModelProfileAdapterConfig({
+      baseConfig: { env: { AGENT_ONLY: "agent-only" } },
+      modelProfile: {
+        ...noProfile,
+        applied: "cheap",
+        configSource: "agent_runtime",
+        adapterConfig: { env: { GH_SEAT_TOKEN_VALUE: "profile-attacker" } },
+      },
+      issueAdapterConfig: null,
+    });
+
+    expect(merged.env).not.toHaveProperty("GH_SEAT_TOKEN_VALUE");
+  });
+
+  // The boundary is scoped to AGENT_SCOPE_ONLY_ENV_KEYS; every other key keeps
+  // the pre-existing shallow-overlay semantics the tests above this rely on.
+  it("leaves non-agent-scope-only overlay semantics unchanged", () => {
+    const merged = mergeModelProfileAdapterConfig({
+      baseConfig: { model: "primary", env: { SHARED: "agent", AGENT_ONLY: "agent-only" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { env: { SHARED: "issue" } },
+    });
+    const untouched = mergeModelProfileAdapterConfig({
+      baseConfig: { model: "primary", env: { SHARED: "agent" } },
+      modelProfile: { ...noProfile },
+      issueAdapterConfig: { model: "issue-explicit" },
+    });
+
+    expect(merged).toEqual({ model: "primary", env: { SHARED: "issue" } });
+    expect(untouched).toEqual({ model: "issue-explicit", env: { SHARED: "agent" } });
+  });
+});