fix(templates): contain local: template ids and stop deriving credential paths from name: (#1900)

[vybe]

Fixes #1900

Contains local: template-id resolution on the read path, and stops credential staging from deriving a filesystem path out of a template's untrusted name: field. Two independent sinks, two different triggers, two different blast radii — they are kept separate below on purpose.

---

Sink 1 — get_local_template (template_service.py:534, unvalidated join at :539)

Trigger: any authenticated principal — every role, including an agent-scoped MCP key that resolves to its owner — issuing a bare GET /api/templates/local:<id>. The route is @router.get("/{template_id:path}") behind Depends(get_current_user); the :path converter captures /, which is precisely why the traversal is reachable over HTTP rather than being a theoretical string bug.

Impact: read any parseable template.yaml the backend process can reach, disclosed in a 200 response — including other tenants' uploads under /data/deployed-templates. The response echoes display_name / description / resources / skills / capabilities / use_cases / data_paths / required_credentials, i.e. arbitrary YAML subtrees, not merely strings.

Demonstrated over real routing + real auth:

GET /api/templates/local:../outside  →  200  {"display_name": "SECRET OUTSIDE", ...}

Fix: the new public template_service.contained_template_dir(name, root) applies the same two-step barrier the create path has had since #950 — a name-shape allowlist (^[a-zA-Z0-9][a-zA-Z0-9_.-]*$, explicit .. reject) before any path math, then resolve() on both sides plus is_relative_to. A rejection returns None, so the router's 404 is byte-identical to an unknown template — no error code, no path, no root name. A distinguishable rejection would itself be a new enumeration oracle, which is exactly what #1759's single-sentence 404 exists to close.

Sink 2 — generate_credential_files (template_service.py:824, name:-derived join at :873)

Trigger: a creator-role caller performing a deploy_local_agent upload, then creating an agent from it. Strictly narrower than Sink 1 — this is not reachable by a bare authenticated GET.

Impact: another tenant's credential-bearing .mcp.json staged into the attacker's own agent. An uploaded name: ../../data/deployed-templates/<victim> was joined onto a hardcoded root and the resulting file read into the new agent's credential files.

Reproduced verbatim: a planted MARKER-CROSS-TENANT-SECRET appeared inside the .mcp.json staged for the attacker's agent.

Two compounding facts made this live rather than latent:

1. _TemplateResolution.github_template_path has zero assignment sites (4 refs, all reads) — so the safe if template_base_path: arm was dead code, and the vulnerable else was the only branch that ever executed.
2. name: is not a directory name. 5 shipped templates declare a display string there — sleep-echo, test-counter, test-delegator, test-echo, test-leak-hook (e.g. name: "Test Echo Agent") — so local:test-echo was looking for <curated>/Test Echo Agent/.mcp.json.

Fix: crud._stage_config_files now threads the directory it already validated through _safe_local_template_path, extracted as crud._resolve_local_template_dir. That makes the #1759 seam-agreement property structural across three seams (resolver, /template bind decision, credential stager) instead of two-by-convention. The residual template_base_path is None arm has no caller today and is kept fail-closed through the same barrier.

What was NOT vulnerable

Create-path id resolution was already contained by crud._safe_local_template_path (crud.py:145) since #950. The blanket claim "the create path is vulnerable" is false — it was specifically the credential-staging derivation inside it.

---

Behaviour delta — measured, not the flattering version

A deploy-local template that both declares credentials.mcp_servers and ships a .mcp.json now has that file staged at all, where the old curated-root lookup always missed. Two consequences, and the second is a loss:

• the staged file wins over the archive's raw copy (startup.sh copies /generated-creds/.mcp.json unconditionally and after the template-copy block);
• the sole production caller passes an empty agent_credentials map (CRED-002 — real values are injected after creation, not at staging), so ${VAR} is rewritten to "".

It is not true that such templates "now get ${VAR} substitution" or that substitution "finally applies". Placeholders are blanked, not substituted: {"TOKEN": "${MY_TOKEN}"} stages as {"TOKEN": ""}. Hardcoded (non-${VAR}) entries survive verbatim. The durable record of a server's required variables remains .mcp.json.template, which is pre-populated untouched.

The corrected description is the one in docs/memory/feature-flows/template-processing.md. (Commit 23581c19's message contains the earlier, incorrect "gets ${VAR} substitution" framing — that wording was the review stage's MEDIUM finding and is superseded by the docs and by this description.)

A second latent bug fixed en route

TypeError: unsupported operand type(s) for /: 'PosixPath' and 'int' at template_service.py:873 — a non-string name: raised an uncaught 500 on agent create. Absorbed by the isinstance(name, str) gate in contained_template_dir; covered by a test over 123 / None / ["a"] / {"a":1} / 1.5 / True, verified red pre-fix.

---

Evidence

• All 30 REPRO tests verified falsifiable — run against reverted source: 19 failed / 11 passed, and every one of the 11 pre-fix-green tests is explicitly labelled HYGIENE / anti-over-block / no-regression / landmine-guard. No decorative tests. The landmine guard was re-proved: resolving only the candidate (not the root) fails exactly that one test.
• Adversarial sweep: 55 hostile inputs → zero escapes, zero raises, zero disclosures. Symlink loops, a 200 KB name, and a regex-backtracking probe all handled in 0.6 ms with no superlinear blowup — which matters because this endpoint carries no rate limit. The 404 body is byte-identical to the unknown-template case.
• Full tests/unit/ — 6024 passed / 18 skipped / 0 failed (pristine 5993 +31). Re-measured at ship on this exact branch tip: 6024 passed, 18 skipped in 464.84s, with 4 test_1771* property files excluded because hypothesis (pinned in tests/requirements-test.txt) is absent from the local interpreter and PEP 668 blocks installing it there — all 4 are byte-identical to origin/dev and untouched by this branch.
• Directly-affected files (test_1900_template_id_traversal, test_1759_template_root_parity, test_ent128a_catalog_resilience, test_local_templates_listing, test_1484_create_agent_characterization, test_1759_local_template_not_found) — 157 passed.
• Related-module grep set — 372 passed / 1 skipped, stable across 5 random-order runs.
• /verify-local --skip-agent PASS; integration 70 passed / 13 skipped / 2 deselected.
• tests/test_templates.py now collects 6 (was 5), and its traversal assertion passed against a real branch-built backend when run manually. See the honesty note below — no automated job runs that file.

Test-runner honesty note

tests/test_templates.py is root-level and has no automated runner at all. Every automated stage collects a subdirectory — CI runs pytest unit/, /verify-local runs pytest unit/ then pytest integration/ — so neither collects tests/*.py. Its assertions must be run manually against a booted backend (cd tests && pytest test_templates.py). The CI-gated guard that actually protects this fix is tests/unit/test_1900_template_id_traversal.py. The final commit on this branch corrects a docstring that previously claimed otherwise.

Pre-existing red (not branch-caused)

test_1069_voip_call_path_param::test_flat_path_params_are_agent_name_not_name fails with an ImportError under fastapi ≥ 0.141.1 (get_flat_dependant removed; reached via the fastapi>=0.115.0 floor). It passes on a pinned fastapi 0.136.1 environment, so it is environment-dependent, not branch-dependent. This branch leaves both that test file and routers/voip.py byte-identical to origin/dev (verified with git diff --quiet).

---

CodeQL note

If a new py/path-injection alert fires on contained_template_dir, the correct response is dismissal citing the two-step barrier — regex allowlist before path math, then resolve() + is_relative_to(), returning the checked value. It is not to reshape the helper or the if/else. #1793 had to revert exactly such a reshape; for that reason the condition here was added without re-indenting the body.

Relationship to enterprise#14

enterprise#14 is an HTTP registry manifest resolving through the existing github:owner/repo clone path — not a second filesystem-path namespace. The narrow, shippable argument for landing this first: enterprise#14 edits this same resolver family in this same module and keeps local: live, so handing it an already-contained module with a public importable helper (contained_template_dir) is cheaper than landing beside an open traversal.

---

Follow-ups (listed, deliberately not filed)

Two are live pre-existing bugs found incidentally — neither is introduced here:

1. A deploy-local template declaring credentials.env_file already has its user-supplied credentials wiped at first boot: staging writes KEY= blanks to /generated-creds/.env, and startup.sh:387-389 copies that over the merged archive .env.
2. /generated-creds copies are not .trinity-initialized-gated, so they re-clobber .mcp.json / .env on every restart, discarding operator edits.

Plus:

3. The severed github_template_path wire (zero writers). This PR stops depending on it; deleting it is a separate change.
4. Extract the shared name allowlist to a dependency-free leaf module, once crud._safe_local_template_path — a CodeQL barrier sitting under the refactor(agents): decompose create_agent_internal (830 lines, CC 120) with characterization tests first #1484 fence — can be touched deliberately.
5. Rate-limit GET /api/templates/{id}. This is the reason rejections log at DEBUG rather than WARNING: with no rate limit, a per-rejection warning is an authenticated log-flood primitive.
6. get_local_templates() still enumerates via iterdir(), so a root-escaping symlink planted inside the root lists but 404s on detail — a deliberate, documented asymmetry (planting one needs local filesystem write access, not a request).
7. tests/test_templates.py has no automated runner — move it under tests/integration/, or add a job that collects root-level files.

Merge-order note

Four files collide with sibling PRs (no rebase needed today — origin/dev is unmoved at 8e924526, this branch's exact base): docs/memory/requirements/core-agent.md §4.1 (all three PRs), docs/memory/feature-flows.md, docs/memory/feature-flows/template-processing.md, tests/unit/test_local_templates_listing.py. First-merged wins.

🤖 Generated with Claude Code

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[AndriiPasternak31]

Approving. Ran /review, /validate-pr and /cso --diff against 032d8672. 0 critical, 0 security findings, 2 vulnerabilities closed. Everything below was verified by execution, not by reading the description — and every number I re-measured came out equal to or better than what the PR claims.

---

The barrier holds

Two independent adversarial passes:

• 59 hostile inputs (traversal, percent-encoded, absolute, UNC, null-byte, backslash, sibling-escape <root>-evil, escaping symlink, symlink loop, symlink-to-root, 200 KB ReDoS probe, 6 non-string types) → 0 escapes, 0 raises, 7.34 ms.
• A fresh-context agent instructed to refute the barrier ran ~700,000 inputs including an exhaustive sweep of the entire Basic Multilingual Plane at 1 and 2 characters, on both 3.12 and prod 3.13 → UPHELD, 0 escapes, 0 exceptions.

It killed the unicode-homoglyph hypothesis outright: [a-zA-Z0-9] is a literal codepoint range, not a unicode class, and there is no re.IGNORECASE, so accepted codepoints above 0x7F = zero. ／ U+FF0F, ． U+FF0E, ․ U+2024, ff, İ, 𝟏 and full-width forms are all rejected at step 1. (And macOS applies NFD, never NFKC, so a compatibility decomposition into / can't happen on any platform Trinity runs on.)

It also confirmed the containment step earns its keep — for a planted <root>/sibling → ../templates-evil:

str(resolved).startswith(str(root)) = True    <- a naive check WOULD have escaped
resolved.is_relative_to(root)       = False   <- what :491 actually uses

The docstring's justification at :463-466 is empirically correct, not just plausible. Step ordering verified independently: every rejection also fires against a nonexistent root, so the allowlist provably precedes the first filesystem call.

The 404 stays byte-identical — tested wider than the PR does

test_1900_router_404_leaks_no_path covers one shape. I swept 16 shapes across every distinct rejection path — barrier-reject (.., %2E%2E%2F, ..%2f, ....//, absolute, leading dot, separator, backslash, empty, leading -), contained-but-is_dir()-false, unknown, near-miss, and unprefixed — comparing status, full body, content-length and content-type:

All 15 local: shapes plus the unprefixed one are byte-identical to unknown-template: 404, {"detail":"Template not found"}, content-length: 31.

The only divergence in the sweep was github:no/such-repo (200), which is pre-existing get_github_template behaviour on a different prefix and a different code path — untouched here.

Structural reasons, traced rather than assumed: one shared raise-site at routers/templates.py:48; rejection returns None so no error code/path/root name reaches the caller; a malformed escaped file can't 500 into a distinguishable response (_build_local_template returns None from the ent#128 broad except); and the DEBUG log can't become an oracle either — _sanitize_for_warning strips non-printables and truncates at 80 chars, which I verified.

The tests are real

Reverted both source files to base and re-ran the guards:

19 failed, 71 passed
FAILED test_1900_router_rejects_traversal_id
  AssertionError: ('/api/templates/local:../outside', 200,
   '{"id":"local:outside","display_name":"SECRET OUTSIDE",...}')

19 failed matches the claimed 19 exactly, and the disclosure reproduces verbatim. Source restored; worktree byte-identical to head.

Full test evidence

Run	Result
Directly-affected suites @ head	157 passed (matches the PR)
Full local tests/unit/ @ head	6079 passed, 14 skipped, 1 xfailed, 0 failed (333s) — higher than the stated 6024 because all four test_1771* hypothesis property files ran on this interpreter
Security guard tests	47 passed — credential-paths parity + model-context parity (Invariant #5), #186 enumeration uniformity, #1159 agent-auth header guard, #1759 root parity
Falsifiability (reverted source)	19 failed / 71 passed
CI	20 SUCCESS / 3 SKIPPED / 0 failures

Claims I checked independently — all held

• _TemplateResolution.github_template_path has 4 references, all reads, zero assignments → the safe arm genuinely was dead code and the vulnerable else was the only executing branch.
• _resolve_template (crud.py:2133) runs strictly before _stage_config_files (:2164), and config.template is never reassigned anywhere in the codebase → the "cannot newly raise" comment holds.
• crud.py:2164 is the sole production caller; the github: branch never sets template_data, so the template_base_path is None arm is genuinely dead.
• Every remaining /-join in template_service.py (:504, :1202, :1203, :1211) takes an already-contained directory; deploy.py's write path derives from a server-generated version_name behind its own #950 guard.
• The else-arm root change (/agent-configs/templates-or-CWD-relative → _local_templates_dir()) is identical in-container and strictly more predictable outside one.
• CodeQL dismissals will survive the merge: prior dismissed py/path-injection alerts 240/242/243/244/245/247 carry most_recent_instance.ref = refs/heads/main.

CodeQL: 269 and 270 hold exactly; 271's conclusion holds but its rationale covers half the taint

269 and 270 are line-accurate — 270 is literally the barrier's own join with is_relative_to on the next line.

271 is worth a comment edit. The flagged line's mcp_template_path has two producers:

954:  if template_base_path:
955:      mcp_template_path = Path(template_base_path) / ".mcp.json"   # <- producer A
956:  else:
981:      mcp_template_path = fallback_root / ".mcp.json"              # <- producer B

The dismissal discusses only B. A is safe today only because github_template_path has zero writers — and this PR's own call-site comment says "github_template_path first so the field's eventual revival wins", with follow-up #3 contemplating that revival. A dismissal is permanent and repo-global: it silences the line, including for taint arriving by a path the reviewer never examined. Suggest extending the comment to name both producers. Not a defect in the shipped code.

Notes (none blocking)

1. Docstring invariants are false under planted symlinks. <root>/selfdot → . returns root itself (is_relative_to is reflexive), contradicting "resolve name as a direct child of root"; <root>/alias → scout returns <root>/scout, so the id round-trips as local:scout, not local:alias, contradicting "the basename … is unchanged". Reproduced twice, independently. Neither escapes — both stay inside the root and both need local filesystem write against a :ro mount, i.e. the same class already disclosed honestly as follow-up #6. Doc-accuracy nit only.
2. contained_template_dir's "never raises" is Python-version-dependent. On ≤3.12 Path.resolve() raises RuntimeError: Symlink loop via check_eloop; 3.13 removed that path. Unreachable in prod for three independent reasons (prod is python:3.13-slim; the templates root is :ro in both compose files; grep -rn "os.symlink\|symlink_to" src/backend/ returns nothing). Flagging because the unit suite runs 3.11 while the image is 3.13 — pre-existing and repo-wide, not this PR's problem.
3. The "CodeQL recognises this as a barrier" wording (template_service.py:461-463 + the mirror in template-processing.md) was falsified by this PR's own scan — 270 fired on the barrier's own join line. Outcome is right; the wording will make the next author doubt a correct guard. Suggest "the shape a py/path-injection alert can be dismissed against".
4. The missing ## Security Considerations / ## Revision History entries are a pre-existing lapse, not a regression here. I checked before raising it: that Security Considerations list has no entry for #950, #1759 or ent#128 either, and #1759 appears 8× in the body and 0× in the Revision History. This PR is consistent with its predecessors. Backfilling all four is a docs-hygiene follow-up, not a condition on a P1 security fix.
5. docs/memory/feature-flows.md:28 — the 2026-08-01 row sits below two 2026-07-31 rows in an otherwise newest-first table. Cosmetic, and that file is a collision file anyway.

Merge-time actions (mechanical, at the button)

• Hand-write the squash message. GitHub concatenates every commit body, so 23581c19's superseded "gets ${VAR} substitution" framing would land verbatim in dev's permanent history. Use the corrected version: placeholders are blanked to "", not substituted.
• Ignore the nightly bot's "merge conflict" comment — it is a CI bug, not a conflict. Root-caused and reproduced: backend-unit-nightly.yml fetches both dev (:88) and the PR head (:109) at --depth=1, then merges two histories with no common ancestor → fatal: refusing to merge unrelated histories → merge_conflict=true. All 9 open PRs carry the comment, 8 of them MERGEABLE. Filed separately. mergeable=MERGEABLE, baseRefOid == origin/dev tip, 20 checks ran and passed — a genuinely conflicting PR produces zero checks.
• When rebasing #1934/#1936, re-assert this fix's guards survived. Two of the four collision files carry security surface: tests/unit/test_local_templates_listing.py (the read-path test_1900_* guards) and template-processing.md (the #1900 containment section). A conflict resolved --theirs would delete them with green CI, because the tests would no longer exist to fail.
• Optional: extend CodeQL 271's dismissal comment per the note above.

---

Why this one reads as trustworthy

The behaviour-delta section is the strongest signal in the PR. It names a loss (${VAR} blanked to "", staged file overwriting the archive copy), explicitly forbids the flattering restatement in three separate docs, and pins the measured behaviour with a test. That is the opposite of the usual failure mode, and it earns the rest of the description credibility — which the independent verification then bore out.

The higher-severity sink is closed at the source (crud threads the already-validated directory) rather than patched at the sink, the residual arm is kept fail-closed for a future caller, and the wiring test specifically defends against the one failure mode — extracted-but-not-wired — that every other test in the suite would miss.

🤖 Review assisted by Claude Code