fix(mcp)!: refuse the writes that change who can log in to a Jamf product

[ktn-jamf]

Summary

MCP run_command refused pro accounts create, update and apply because they set a login password. Other writes grant a login with no password in the request, and those still ran. A prompt-injected model could do any of these:

• Create a Classic administrator account, or change an existing administrator's e-mail address.
• Bind an account group to an LDAP server that the model runs.
• Point SAML SSO at its own identity provider and turn off the SSO bypass.
• Send password-reset mail to its own SMTP server.
• Create a Jamf Protect console user or change a Protect role, or set a Jamf School login password or group ACL.

Each of these gives a login that works outside the MCP server. This PR closes two security-scan findings: "MCP refuses pro accounts but not Classic account twins that mint a Jamf Pro admin login" and "MCP allows sso-settings update to point Jamf Pro SAML login at a model-chosen IdP" (both CWE-863). It also closes the Protect and Jamf School gap of the same class. A background security review of the first fix commit flagged that gap as an incomplete deny-list.

Change

A new refusal reason, refusedChangesLoginAuthority, covers these writes:

• create, update and, where it exists, apply on pro classic-account-users, classic-account-groups, classic-ldap-servers, cloud-ldap and cloud-azure, plus pro cloud-ldap update-mappings.
• pro classic-smtp-server update and pro smtp-server update.
• pro sso-settings update, disable, cert create, cert update and oidc-broker-config update, plus the sso-settings-cert spelling.
• platform sso-connections create and update.
• apply on protect users, groups and roles, and on school users and groups. These read their body only from a file, so they ran only with that file inside --input-dir.
• protect restore, which replays a Protect backup, its roles, groups and users included. It is refused whole, -n included, because no guard reads its --resources and --exclude values.

Reads, delete, history notes and connection tests on these resources still run. That matches pro accounts delete, which was not refused before either. The mcp serve help, the run_command tool description, the MCP section of agent_context.md (printed by jamf-cli agent-context) and CHANGELOG.md list the new refusals.

TestMCP_RefusesEveryWriteThatChangesWhoCanLogIn walks every leaf under these resources. It fails on any leaf that runs over MCP without a named reason. So a write that a later spec adds fails CI until someone classifies it. It also fails on a stale allowance. TestMCP_RefusesCommandsThatPrintOrSetALoginCredential now holds the exact argv from both findings, including an alias and a leading flag.

The walk also covers pro oidc and the read-only login resources (pro account-groups, classic-accounts, classic-ldap, ldap), so a write a later spec adds under them fails CI too. pro oidc generate-certificate stays allowed, with its reason recorded: it replaces the keystore Jamf Pro signs its own OIDC messages with, generated server-side and never returned. TestMCPPolicyTexts_NameEveryLoginAuthorityRefusal requires the mcp serve help, the run_command description and agent_context.md to name the resource of every refusedChangesLoginAuthority entry.

Existing tests changed

• notACredentialPrinter in mcp_secret_leaves_test.go. I removed the exemptions for pro sso-settings update, pro smtp-server update, pro cloud-ldap update, both SSO cert updates, oidc-broker-config update, platform sso-connections create and update, and protect restore. These commands are now refused, and the classification test rejects an entry that is both refused and exempt.

Verification

• The first two fix commits each sit on top of a commit that holds only its failing test. Squash-merge this PR, so that the red commits do not reach main.
• go test ./... -count=1 passes, and golangci-lint run ./internal/commands/... reports 0 issues.
• I drove a local build of mcp serve over stdio JSON-RPC against fake profiles that pointed at https://127.0.0.1:1. Every attack argv from the findings was refused, including -q pro sso-settings-cert update and -q platform ssoc update 1. The Protect and School apply writes were refused too, with their body files inside --input-dir. protect restore --input <dir> --yes, with and without --resources users,roles,groups and with -n, was refused too; the build before that fix let it through to resolving role "Full Admin". The read controls (classic-account-users list, sso-settings get, platform sso-connections list, protect users list) passed the guard and stopped only at the refused connection.

Not in scope

• platform sso-domains create and verify still run. A domain grants a login only through an SSO connection, and SSO connections are now refused.
• pro api-roles and pro api-integrations writes still run. They change what an API client may do, but the model cannot get a client's secret, because client-credentials is refused. That reason does not cover one case: pro api-roles update can raise the privileges of a role that an existing client already uses, the server's own client included. A refused classic-account-groups update does the same for an account group. Whether API-client authority belongs in this class is left open for a follow-up.

🤖 Generated with Claude Code

[ktn-jamf]

Warning

⚠️ Needs changes. Refuses over MCP the writes that change who can log in to a Jamf product, held by a fail-closed leaf walk.
Blocking: (1), (2). Two nice-to-have suggestions sit in the collapsed section.

Rating: 3/5

• Would be a 5 with protect restore refused in (1) and the agent guide's refusal list brought current in (2).

Findings

⚠️ IMPORTANT (1) (security, via security-reviewer+devil-advocate) — internal/commands/mcp.go:690-692: protect restore writes Protect users, groups and roles and is not refused

protect restore (protect_backup.go, RunE near :1737) applies a backup directory roles, then groups, then users. Those three table entries (:584, :606, :645) carry no RestoreSkipReason. mcpRefusedCommands refuses protect backup (:705) and the three apply leaves, but refuseOverMCP (:878) matches only the path or a path plus a space, so jamf-cli protect restore passes. --input is a pathRead flag (:756), so restore runs from inside --input-dir. The PR body names that exact precondition as its reason to refuse apply. The walk cannot see it, because restore is a sibling of the listed resources.

Failure scenario: mcp serve --input-dir D, with a backup-shaped directory under D whose users/ document grants an admin role → the model runs protect restore --input D/bk --yes → a Protect console login or role is created or changed. That is the grant this PR refuses for protect users apply.

Suggested fix:

 	{"jamf-cli protect roles apply", refusedChangesLoginAuthority},
+	{"jamf-cli protect restore", refusedChangesLoginAuthority},

Add "protect restore" to loginAuthorityResources, and name it in the mcp serve help, the run_command description and the CHANGELOG entry.

Fixed when: buildChildArgs("prod", []string{"protect", "restore", "--input", <dir>}) returns an MCP refusal naming "who can log in", and dropping the entry fails TestMCP_RefusesEveryWriteThatChangesWhoCanLogIn.

⚠️ IMPORTANT (2) (documentation) — internal/commands/agent_context.md:106-122: the agent guide's MCP refusal list omits every refusal this PR adds

jamf-cli agent-context prints this file to agents. Its MCP section enumerates what run_command rejects. It names "the commands that set a Jamf Pro login password" and stops there. None of the refusedChangesLoginAuthority paths appear in it. #402 updated this paragraph when it added the pro accounts refusals. agent_context.go:12-19 makes this sync the contributor's job, backed by the documentation-currency check of PR review.

Failure scenario: an agent reads agent-context and plans a run that calls pro classic-account-users create over MCP, which the list implies is allowed → the call is refused, and the guide contradicts the server it describes.

Suggested fix: after the pro accounts create, update, apply clause, add a clause matching the mcp serve help: "the writes that change who can log in to a Jamf product (create, update and apply on the Classic account users, account groups and LDAP servers, cloud-ldap and cloud-azure; the two SMTP updates; the SSO settings, cert and OIDC broker writes; platform sso-connections writes; apply on Protect users, groups and roles and School users and groups)".

Fixed when: every resource the mcp serve help lists under the login-authority bullet also appears in agent_context.md's MCP section.

Nice-to-have suggestions (2 items)

💡 NICE-TO-HAVE (3) (test-coverage, via test-quality-reviewer+devil-advocate) — internal/commands/mcp.go:117-126, :264-272: the help and the run_command description restate the list by hand, and no test ties them to mcpRefusedCommands

Deleting the Protect and School clause from the tool description left every TestMCP* test green. Both texts also omit the sso-settings-cert spelling that the CHANGELOG names.

Fixed when: removing any refusedChangesLoginAuthority resource from either text fails a test, modelled on mcp_payload_policy_text_test.go.

💡 NICE-TO-HAVE (4) (test-coverage, via security-reviewer+devil-advocate+test-quality-reviewer) — internal/commands/mcp_login_authority_test.go:17-36: the resource list omits pro oidc and the read-only login resources

pro oidc generate-certificate carries Update SSO Settings (pro/generated/oidc.go:137), the privilege the refused sso-settings disable carries, and runs unclassified. pro account-groups, classic-accounts, classic-ldap and ldap are absent too, so a write a later spec adds under them passes the walk.

Fixed when: those resources are listed, and generate-certificate is refused or allowed with a reason.

This covers all findings — addressing the above gets this PR to merge-ready.

Review coverage and scope

• Design and architecture: per-path deny list plus walk; sibling write path is (1)
• Correctness: refuseOverMCP prefix match, alias resolution via cobra Find
• Security: whole command tree swept for login-granting writes; gap is (1)
• Test coverage: walk mutation-checked, 3 of 3 killed; text sync is (3)
• Documentation currency: help, tool description, CHANGELOG current; agent_context.md is (2)
• Project rules compliance: coding-style.md, credentials-and-auth.md, classic-api.md
• [na] Performance, reliability, simplification, frontend.

Diff: 4 files, +237/−19, head bbe51467, plus protect_backup.go, agent_context.md, pro/generated/oidc.go, the built command tree. Tier: high by edit shape (guard list). Lanes run: security-reviewer, test-quality-reviewer, devil-advocate. Eligible, not dispatched under the high-risk cap of 3: usability-reviewer, silent-failure-hunter. Never run: fidelity-reviewer skipped (no spec), performance-reviewer inapplicable, scope-reviewer inapplicable. Conventions: 1 root CLAUDE.md, 3 rules, from the worktree; no nested CLAUDE.md (walked root → internal/commands/).

What's done well

✅ The walk fails in both directions: on a write with no reason, and on a stale or contradicting allowance. Mutation checks confirm it catches a dropped refusal, including cloud-ldap update-mappings, which no argv case covers.

_{🤖 Generated by the pr-review:review skill v1.43.0 · reviewed head bbe51467}

[ktn-jamf]

Tip

✅ Merge-ready. Refuses over MCP the writes that change who can log in to a Jamf product, protect restore now included, and holds the help, tool description and agent guide to the refusal list. No critical or important issues found.
See the collapsed section for 2 nice-to-have suggestions.

Rating: 5/5

• Round 1's blocking items (1) and (2) are fixed, and verified against a live MCP probe. The 2 nice-to-have suggestions below are optional and do not hold the merge.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/mcp.go:701	1	✅ Fixed	protect restore refused. The walk and an argv case hold it, and a dropped entry fails the walk
(2)	internal/commands/agent_context.md:118	1	✅ Fixed	MCP section lists every login-authority refusal and is held by the new sync test
(3)	internal/commands/mcp.go:264	1	⚠️ Partial	Sync test added. Its matcher misses one shape, now (1) below
(4)	internal/commands/mcp_login_authority_test.go:17	1	✅ Fixed	Walk covers pro oidc, the read-only login resources and protect restore

Nice-to-have suggestions (2 items)

💡 NICE-TO-HAVE (1) (test-coverage, via test-quality-reviewer+security-reviewer) — internal/commands/mcp_login_authority_test.go:194: the sync matcher makes the product optional, so 'groups' in the School clause satisfies protect groups

Dropping 'groups' from the Protect clause of both the help and loginAuthorityToolNote left TestMCPPolicyTexts_NameEveryLoginAuthorityRefusal green. protect roles has the same shape.

Fixed when: removing a resource from one product's clause fails the test while the other product's clause stays.

💡 NICE-TO-HAVE (2) (documentation, via devil-advocate) — internal/commands/mcp.go:126-127, CHANGELOG.md:38-40: both say protect restore "applies a backup's roles, groups and users"

Restore replays the whole backup, plans and analytics included (protect_backup.go restore Long), and the refusal blocks all of it, the dry run included. The CHANGELOG's reason should name the missing guard on --resources/--exclude, which is the real reason, rather than the model choosing them.

Fixed when: both texts say restore is refused whole, -n included.

Review coverage and scope

• Design and architecture: whole-command protect restore refusal matches protect backup
• Correctness: refuseOverMCP prefix match on resolved path; alias handling unchanged
• Security: Protect and School trees swept for other restore or import paths; none found
• Test coverage: dropping restore entry kills the walk; matcher gap is (1)
• Documentation currency: help, tool note, agent guide, CHANGELOG; wording is (2)
• Project rules compliance: coding-style.md, credentials-and-auth.md, classic-api.md
• [na] Performance, reliability, simplification, frontend.

Diff: incremental 5 files, +94/−14 since bbe51467, head 2e4c6a69, plus protect_backup.go, skills/skills/jamf-migrate/SKILL.md, specs/JamfProAPI.yaml. PR description edited this round: removed derived counts and restated the restore bullet. Lanes run: security-reviewer, test-quality-reviewer, devil-advocate (re-spawned, paths changed; first dispatch lost to a session limit, retried once). Eligible, not dispatched under the high-risk cap of 3: usability-reviewer, silent-failure-hunter. Never run: fidelity-reviewer skipped (no spec), performance-reviewer inapplicable, scope-reviewer inapplicable. Conventions: 1 root CLAUDE.md, 3 rules, from the worktree; no nested CLAUDE.md (walked root → internal/commands/).

What's done well

✅ The run_command description now reads the refusal list from one constant that a test checks, so the model's copy of the list can no longer drift from the one in the code.

_{🤖 Generated by the pr-review:review skill v1.43.0 · reviewed head 2e4c6a69}