fix(setup): role-aware clipboard render + JSON-escape ROLE injection

Two Task 4 review fixes for app/web/templates/install.html:

1. JSON-escape `ROLE` JS const via `{{ role | tojson }}` (defense in
   depth — removes the dependency on Jinja autoescape semantics for JS
   contexts; FastAPI's Literal validator already constrains role values).

2. Verify the analyst tile's clipboard payload is the analyst layout.
   The pre-existing role-aware plumbing (compute_default_agent_prompt
   threading role into setup_instructions_lines, picked up by the JS
   SETUP_INSTRUCTIONS_TEMPLATE array) was correct; adding regression tests
   that pin to the JS clipboard block specifically so a future inversion
   would fail loudly.

Tests: analyst clipboard contains `agnes init` + `agnes catalog` and
NOT `agnes auth import-token` / `agnes skills`; admin clipboard is the
inverse. Plus an explicit assertion that ROLE is rendered via tojson.
This commit is contained in:
ZdenekSrotyr 2026-05-04 17:43:46 +02:00
parent 44234ba3ae
commit 8091620d33
2 changed files with 117 additions and 1 deletions

View file

@ -996,7 +996,12 @@
// Role from the Jinja ctx — drives the PAT mint shape below // Role from the Jinja ctx — drives the PAT mint shape below
// (analyst tile mints a 1h scope=bootstrap-analyst PAT, admin keeps // (analyst tile mints a 1h scope=bootstrap-analyst PAT, admin keeps
// the historical 90-day general-purpose PAT). // the historical 90-day general-purpose PAT).
const ROLE = "{{ role }}"; // `tojson` produces a JSON-quoted JS string literal, which doubles as
// defense-in-depth: even if `role` ever became attacker-influenced,
// tojson would JSON-escape any embedded quotes/backslashes/script
// sequences. (Today FastAPI's Literal validator already constrains
// `role` to {"analyst","admin"}, so this is belt-and-suspenders.)
const ROLE = {{ role | tojson }};
function copyToClipboard(text) { function copyToClipboard(text) {
if (navigator.clipboard && window.isSecureContext) { if (navigator.clipboard && window.isSecureContext) {

View file

@ -109,6 +109,117 @@ def test_setup_page_admin_js_uses_general_scope(client):
assert "expires_in_days" in text # still present in the admin body assert "expires_in_days" in text # still present in the admin body
def test_setup_page_analyst_clipboard_renders_analyst_layout(client):
"""The clipboard text the analyst tile produces must be the analyst layout
(`agnes init` + `agnes catalog`), NOT the admin layout (`claude plugin
marketplace add` + admin-only flow).
The rendered HTML embeds the role-aware `setup_instructions_lines` text
into a JS array `SETUP_INSTRUCTIONS_TEMPLATE` (see
`_claude_setup_instructions.jinja`); `renderSetupInstructions` substitutes
`{server_url}` / `{token}` into that array at click time. So checking the
embedded array against the served HTML is sufficient if it carries the
analyst layout, the clipboard payload will too.
Pinning to the JS array block specifically (not the whole page) avoids
false positives from chrome / preview-mode <pre> renders elsewhere.
"""
import re
resp = client.get("/setup?role=analyst", follow_redirects=True)
assert resp.status_code == 200
text = resp.text
# Locate the JS array that holds the clipboard template body. The partial
# emits `var SETUP_INSTRUCTIONS_TEMPLATE = [...].join("\n");` — match
# everything between the `[` and the matching `]` so we can scope the
# assertions to *just* what gets pasted into Claude Code, ignoring the
# preview <pre> block that also embeds the lines.
match = re.search(
r"var\s+SETUP_INSTRUCTIONS_TEMPLATE\s*=\s*\[(.*?)\]\.join\(",
text,
re.DOTALL,
)
assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
clipboard_block = match.group(1)
# Analyst layout markers MUST be in the clipboard block.
assert "agnes init" in clipboard_block, (
"Analyst clipboard payload missing `agnes init` — is "
"compute_default_agent_prompt(role=role) being threaded into "
"setup_instructions_lines on the /setup route?"
)
assert "agnes catalog" in clipboard_block, (
"Analyst clipboard payload missing `agnes catalog` smoke verify"
)
# Admin-only markers MUST NOT be in the analyst clipboard block.
# If they are, renderSetupInstructions is producing admin layout despite
# the analyst tile being selected — the bug this test guards against.
# `agnes auth import-token` is the admin login step (analyst uses
# `agnes init` which bundles auth + workspace bootstrap).
assert "agnes auth import-token" not in clipboard_block, (
"Analyst clipboard block contains admin-only `agnes auth import-token` "
"— role plumbing is broken; analyst sees admin instructions."
)
assert "agnes skills" not in clipboard_block, (
"Analyst clipboard block contains admin-only skills setup step — "
"analyst layout should not include skills management."
)
def test_setup_page_admin_clipboard_renders_admin_layout(client):
"""Counterpart to the analyst test — admin tile MUST keep the existing
full marketplace/plugins flow byte-equivalent (no regression from Task 4).
"""
import re
resp = client.get("/setup?role=admin", follow_redirects=True)
assert resp.status_code == 200
text = resp.text
match = re.search(
r"var\s+SETUP_INSTRUCTIONS_TEMPLATE\s*=\s*\[(.*?)\]\.join\(",
text,
re.DOTALL,
)
assert match, "SETUP_INSTRUCTIONS_TEMPLATE array not found in rendered HTML"
clipboard_block = match.group(1)
# Admin layout marker MUST be present. `agnes auth import-token` is the
# admin login step (analyst replaces it with `agnes init`); `agnes skills`
# is admin-only post-auth setup. Either one anchors the admin layout
# without depending on RBAC plugin grants (which the unauthenticated
# TestClient won't have).
assert "agnes auth import-token" in clipboard_block, (
"Admin clipboard payload missing `agnes auth import-token` — "
"Task 4 should not have changed admin behavior."
)
assert "agnes skills" in clipboard_block, (
"Admin clipboard payload missing the skills setup step"
)
# Analyst-only marker MUST NOT appear in admin layout. `agnes init` is
# the analyst-only auth + workspace bootstrap; admin uses
# `agnes auth import-token` instead.
assert "agnes init" not in clipboard_block, (
"Admin clipboard block leaked the analyst `agnes init` step"
)
def test_setup_page_role_is_json_escaped(client):
"""The `ROLE` JS const must be injected via Jinja `tojson` (defense in
depth) not as a bare `"{{ role }}"` string interpolation. This makes
JS string-escaping explicit and removes the dependency on Jinja
autoescape semantics for JS contexts.
"""
resp = client.get("/setup?role=analyst", follow_redirects=True)
assert resp.status_code == 200
text = resp.text
# tojson always emits double-quoted JSON: the rendered output is exactly
# `const ROLE = "analyst";` (note: no extra space inside the quotes).
assert 'const ROLE = "analyst";' in text
def test_setup_page_js_ternary_keys_bootstrap_to_analyst(client): def test_setup_page_js_ternary_keys_bootstrap_to_analyst(client):
"""Mutation-resistant assertion: the `bootstrap-analyst` scope must sit on """Mutation-resistant assertion: the `bootstrap-analyst` scope must sit on
the truthy branch of `ROLE === "analyst"`, not on the falsy branch. the truthy branch of `ROLE === "analyst"`, not on the falsy branch.