fix!: keep device secrets off the command line and out of -vvv and --dry-run output

[ktn-jamf]

Why

The repo policy forbids secrets on the command line. Three commands took a device secret as a plain flag value, which ps, shell history and CI logs can read. Two log paths also printed secrets:

• Secret flags. pro computer-inventory set-recovery-lock --new-password, pro mobile-devices lock --pin and pro mobile-devices clear-passcode --unlock-token took the secret on argv. A test showed each value in ps output while the command ran.
• -vvv logs. The body redaction missed many token, PIN, passcode and keystore fields. A test counted 22 secret-bearing body shapes that printed unredacted, including pin, unlockToken, serverToken and keystoreBytes.
• --dry-run previews. All three previews printed the request body verbatim: Pro and Classic, Platform (which also serves the gateway Security Cloud commands) and Security Cloud. A test showed an SMTP client secret, an LDAP password, a UEM client secret, a ZTNA pre-shared key and an Authorization header in the preview.

Found by the 2026-09-29 security scan. The findings were "Recovery Lock password, lock PIN and unlock token accepted as CLI flag values" (MEDIUM), "-vvv body redaction misses token, pin, keystore and camelCase token fields" (LOW) and "--dry-run prints unredacted request bodies containing credentials to stderr" (LOW).

What changed

Breaking: the secret flags read from a file.

Old	New
set-recovery-lock --new-password X	--new-password-file <path>, or a prompt with echo off on a terminal
set-recovery-lock with no password (clear)	--clear
mobile-devices lock --pin X	--pin-file <path>
clear-passcode --unlock-token X	--unlock-token-file <path> (- reads stdin)

• Without a file, --no-input refuses. An empty file is refused.
• An error names the flag but never the path, because a mistyped secret could be the path.
• An unknown old flag gets a hint that points at the new name, and the hint does not echo the value.
• set-auto-admin-password uses the same reader.
• A stray positional argument is shown as <redacted> on every command that takes a secret file, and on both erase commands.

One redaction rule, used by every log and preview path. It lives in a new package, internal/redact.

• A field is redacted when its last word is token, pin, passcode, keystore (also keystore bytes and keystore file), authorization (also authorization header), challenge, signature or credential(s). The old credential words still match anywhere in the name. Numbers are redacted only under a pin or passcode name.
• Names that only mention one of these words are not redacted. For example, tokenUrl, tokenEndpoint, authorizationEndpoint, keystoreFileName, bootstrapTokenEscrowedStatus and pinned stay visible.
• Every leaf inside a FileVault institutional_recovery_key is redacted. Its text-only inventory status, such as "Not Present", is kept. generator/classic now reads the same path list, so the --set refusal and the redaction cannot drift apart.
• Plist <key>…</key><string>…</string> pairs are redacted when the key names a credential. This covers raw, entity-escaped and JSON-embedded plists, including a character reference between the two elements.
• A string array under a credential name is redacted.
• These paths now use the rule:
  • the -vvv request and response body logs
  • all three --dry-run previews
  • the response bodies quoted in client error messages
  • the --> request lines, where credential query parameters and userinfo passwords are masked
  • the JCDS download errors, where the pre-signed query is removed

Verification

go test ./internal/redact/ ./internal/client/ -count=1
go test ./internal/commands/ -run 'Redact|DryRun|Secret|StrayPositional|JCDS|RemovedSecretFlags' -count=1

The first commit adds the redaction repro tests alone, and they fail. The verbose test reported 17 leaks in the client and 5 in the platform transport, and all 5 dry-run cases printed their canary. The secret-flag tree walk failed on the three flags before the change and passes after it. Each review fix was mutation-checked.

Two existing tests changed, with approval:

• mcp_report_exit_test.go: the MCP sweep's exemption key moved from unlock-token to unlock-token-file.
• positional_args_test.go: two tests were deleted, because the flag they depended on no longer exists. Stray-positional redaction is still covered, by new tests scoped to secret-file commands.

The review ran security twice and test quality once. The second security round confirmed every first-round fix, and found no slow regular expressions, even on a multi-megabyte body.

Accepted, not changed. These shapes are still not redacted: plist <data> and <integer> values under a credential key, CDATA inside the FileVault container, and an object nested under credentials with a non-credential child name. The review found no Jamf payload that uses them.

🤖 Generated with Claude Code

[neilmartin83]

Warning

⚠️ Needs changes. Moves three device secrets off argv into --*-file flags and routes every -vvv, --dry-run and error-body path through one new redactor.
Blocking: (1), (2).

Rating: 3/5

• Would be a 5 with stdin dropped from readSecretFile and CDATA handled in xmlElementRe.
• Coverage: scope-reviewer has never run on this PR. The rating reflects the dimensions that were searched, not the whole diff.

⚠️ IMPORTANT (1) (security, via security-reviewer) — internal/commands/pro_device_actions.go:1761: --*-file - adds a stdin credential channel the policy forbids

readSecretFile reads os.Stdin when the path is -. clear-passcode advertises this in its flag help (:1138), its Long and an Example (printf '%s' "$UNLOCK_TOKEN" | … --unlock-token-file -). --pin-file, --new-password-file and set-auto-admin-password --password-file share the reader, so they accept - too, undocumented. .claude/rules/credentials-and-auth.md reads: "Never accept credentials (passwords, tokens, client secrets) via CLI flags or stdin". It names --token-stdin as a never-add flag, and says human credentials take "no stdin". Nothing else in the tree reads a secret from stdin. A PR that brings these commands into the policy should not open a new exception to it.

Failure scenario: printf '%s' "$TOKEN" | jamf-cli pro md clear-passcode --serial X --unlock-token-file - --yes is accepted, and --help ships it as the recommended CI form. That is the --token-stdin shape the CRITICAL policy rules out.

Suggested fix:

-	if path == "-" {
-		data, err = io.ReadAll(os.Stdin)
-	} else {
-		data, err = os.ReadFile(path)
-	}
+	data, err := os.ReadFile(path)

Drop the "-" for stdin wording, the stdin Example and the "pass --yes" note from clear-passcode, and the - row from the CHANGELOG table. If stdin is wanted for device secrets, amend the policy file in this PR instead.

Fixed when: no --*-file secret flag reads os.Stdin, or credentials-and-auth.md carries an explicit exception for device secrets. A test pins whichever was chosen.

⚠️ IMPORTANT (2) (silent-failure, via silent-failure-hunter+security-reviewer) — internal/redact/redact.go:157: a CDATA-wrapped credential passes redact.Body unchanged

xmlElementRe's text run is [^<]*, and <![CDATA[ opens with <, so <password><![CDATA[p&ss]]></password> never matches. Body returns the bytes untouched and gives no signal. CDATA is how a hand-written Classic body carries an SMTP, LDAP or distribution-point password containing & or <. The PR description accepts CDATA only inside the FileVault container. This is the general element case. Both lanes reproduced it on a copy of this file.

Failure scenario: jamf-cli -n pro classic-smtp-server update --from-file smtp.xml, where smtp.xml holds <password><![CDATA[p&ss<1]]></password> → [dry-run] Request body: prints the password to stderr and the CI log. -vvv does the same on the request and on a Classic GET that echoes it.

Suggested fix:

-	xmlElementRe = regexp.MustCompile(`<(?P<name>` + namePattern + `)(\s[^>]*)?>[^<]*</[^>]*>`)
+	xmlElementRe = regexp.MustCompile(`(?s)<(?P<name>` + namePattern + `)(\s[^>]*)?>(?:[^<]|<!\[CDATA\[.*?\]\]>)*</[^>]*>`)

Group indices are unchanged. Checked on a copy: the CDATA password redacts, a CDATA <name> stays readable, and redact_test.go still passes. xmlLeafTextRe inside redactContainers has the same [^<]* run and wants the same alternation.

Fixed when: <password><![CDATA[x]]></password> redacts to <password>[REDACTED]</password> in a redact_test.go case, and a sibling CDATA under a non-credential name is left intact.

Review coverage and scope

• Design and architecture: shared internal/redact; generator reads CredentialFieldPaths from it
• Correctness: Body, URL, readSecret/readSecretFile, --clear mutual exclusion, renamedFlags hints
• Security: stdin channel (1), CDATA (2); canary not echoed by removed flags or stray positionals
• Test coverage: redact, client, commands suites pass at head; mutations static-only
• Reliability: secret read precedes any request; MCP child stdin is /dev/null
• Performance: Body linear, about 0.2 s/MB; gated on -vvv/-n/error paths
• Documentation currency: CHANGELOG entries; no stale --pin/--new-password/--unlock-token in docs, site or wiki sources
• Project rules compliance: credentials-and-auth.md, coding-style.md, classic-api.md; violation is (1)
• [na] Frontend, cross-repo contracts.

Diff: 21 files, +1612/−306, head b3bc951. Lanes run: security-reviewer, silent-failure-hunter, devil-advocate, test-quality-reviewer, usability-reviewer, performance-reviewer. Never run: scope-reviewer, skipped; fidelity-reviewer, inapplicable. Conventions: 1 root CLAUDE.md, 3 rules, from the worktree.

What's done well

• ✅ Pointing the Classic --set refusal and the log redactor at one CredentialFieldPaths list removes a drift the two copies would otherwise have had.
• ✅ readSecretFile strips the *fs.PathError path, so a secret typed where its path belongs never reaches the error envelope.

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

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

[neilmartin83]

Warning

⚠️ Needs changes. Moves three device secrets off argv into --*-file flags and routes every -vvv, --dry-run and error-body path through one new redactor. The only new commit is the merge of #407. Its code resolution is correct, but its CHANGELOG.md resolution deletes 250 lines of released history.
Blocking: (1), (2), (3).

Rating: 3/5

• Would be a 5 with the CHANGELOG restored, stdin dropped from readSecretFile, and CDATA handled in xmlElementRe.
• The rating holds from round 1. Both round-1 findings are open on unchanged lines, and (1) is new from the merge.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/pro_device_actions.go:1761	2	🔴 Open	No change at the cited lines, and no reply. Now (2)
(2)	internal/redact/redact.go:157	2	🔴 Open	The merge edited only the CredentialFieldPaths doc comment. xmlElementRe is unchanged. Now (3)

⚠️ IMPORTANT (1) (correctness, merge resolution) — CHANGELOG.md:1045: the merge of #407 truncates the CHANGELOG and deletes 250 lines of v1.28.0 release notes

At b3bc951 the file had 1,287 lines, and at b3ad3f2, main had 1,257. At e85ce0e it has 1,067. It ends mid-word on as t with no trailing content. git diff b3ad3f2..e85ce0e -- CHANGELOG.md shows the PR's two new Unreleased entries, which are correct. It also shows a hunk @@ -1004,254 +1064,4 @@ that removes everything after the pro audit -o raw bullet in v1.28.0: the section-banner, patch-status --scan-failures, --field and stray-positional bullets, every ### Changed/### Added/### Fixed block under it, and the Classic display_in entry. A stray line holding only ` is inserted at :1045. git merge-tree against current main reports no conflict, so this lands silently with the PR.

Failure scenario: after this merges, v1.28.0's notes on main stop mid-sentence. Users lose the record of the breaking stray-positional refusal (exit 2) and the --field/--out-file behaviour changes. Every later PR inherits the truncated file.

Suggested fix:

git checkout b3ad3f29 -- CHANGELOG.md
# re-insert this PR's two Unreleased sections:
#   "### Breaking — device secrets are read from a file, not a flag value"
#   "### Behaviour — `-vvv`, `--dry-run` and error messages redact more credentials"
# above "### Breaking — `--set` refuses credential fields …"

Fixed when: git diff origin/main...HEAD -- CHANGELOG.md shows only additions inside ## Unreleased, and the file ends with the specs/classic/schemas.json sentence as on main.

⚠️ IMPORTANT (2) (security, via security-reviewer) — internal/commands/pro_device_actions.go:1761: --*-file - adds a stdin credential channel the policy forbids

Carried from round 1, open 2 rounds. readSecretFile reads os.Stdin when the path is -. clear-passcode advertises this in its flag help, its Long and an Example. --pin-file, --new-password-file and set-auto-admin-password --password-file share the reader. .claude/rules/credentials-and-auth.md says: "Never accept credentials (passwords, tokens, client secrets) via CLI flags or stdin."

Failure scenario: printf '%s' "$TOKEN" | jamf-cli pro md clear-passcode --serial X --unlock-token-file - --yes is accepted, and --help presents it as the recommended CI form.

Suggested fix:

-	if path == "-" {
-		data, err = io.ReadAll(os.Stdin)
-	} else {
-		data, err = os.ReadFile(path)
-	}
+	data, err := os.ReadFile(path)

Also remove the - wording, the stdin Example, and the or - for stdin cell in the CHANGELOG table (CHANGELOG.md:26). The alternative is to amend the policy file in this PR.

Fixed when: no --*-file secret flag reads os.Stdin, or credentials-and-auth.md carries an explicit exception for device secrets. A test pins whichever was chosen.

⚠️ IMPORTANT (3) (silent-failure, via silent-failure-hunter+security-reviewer) — internal/redact/redact.go:157: a CDATA-wrapped credential passes redact.Body unchanged

Carried from round 1, open 2 rounds. xmlElementRe's text run is [^<]*. <![CDATA[ opens with <, so <password><![CDATA[p&ss]]></password> never matches, and Body returns the bytes untouched. xmlLeafTextRe (:159) has the same [^<]* run.

Failure scenario: jamf-cli -n pro classic-smtp-server update --from-file smtp.xml, with <password><![CDATA[p&ss<1]]></password>, prints the password in [dry-run] Request body:. -vvv does the same.

Suggested fix:

-	xmlElementRe = regexp.MustCompile(`<(?P<name>` + namePattern + `)(\s[^>]*)?>[^<]*</[^>]*>`)
+	xmlElementRe = regexp.MustCompile(`(?s)<(?P<name>` + namePattern + `)(\s[^>]*)?>(?:[^<]|<!\[CDATA\[.*?\]\]>)*</[^>]*>`)

Fixed when: a redact_test.go case shows <password><![CDATA[x]]></password> redacting to <password>[REDACTED]</password>, and a sibling CDATA element under a non-credential name stays intact.

PR decomposition

Scope Review

Recommendation: deliver as a single PR. The PR has three concerns, all fixing security-scan findings about leaked secrets: the flag rename, internal/redact and its -vvv/error-body wiring, and the dry-run previews. The dry-run slice depends on internal/redact. Only the rename slice is independent, and it is the smallest. Re-slicing would reset a full panel review and multiply rebase conflicts with #402, #405 and #408 in root.go, client.go and CHANGELOG.md. The CHANGELOG concern it raised is (1). The 95 lines removed from positional_args_test.go are deliberate, per the PR description, and are covered by the new secret-file tests.

Review coverage and scope

• Design and architecture: parser.credentialPathSuffixes now prepends redact.CredentialFieldPaths, so --set refusal and log redaction still share one list after #407 moved the matcher
• Correctness: merge resolution of generator/classic/schema.go (dropped the duplicate isCredentialField), generator/parser/credfield.go, internal/redact/redact.go (doc comment) and dryrun_redaction_test.go (the body now goes through stdin, since #407 refuses clientSecret via --set). All correct. The CHANGELOG resolution is (1)
• Security: (2) and (3) carried. The merge opens no new channel
• Test coverage: go test ./generator/... ./internal/redact/ ./internal/client/ and the redaction, dry-run and secret suites in ./internal/commands/ pass at e85ce0e. CI is green
• Reliability: unchanged since round 1
• Documentation currency: covered by (1)
• Project rules compliance: credentials-and-auth.md, coding-style.md and classic-api.md. The violation is (2)
• [na] Performance: no change since round 1. Frontend and cross-repo contracts do not apply

Incremental diff b3bc951..e85ce0e: one merge commit (origin/main @ b3ad3f2, #407), graded through --remerge-diff. git merge-tree against current main (which includes #404) is clean. Lanes run: scope-reviewer, never run before and eligible at more than 500 lines. Never run: fidelity-reviewer, which does not apply because there is no linked spec. Conventions: 1 root CLAUDE.md and 3 rules, read from the session checkout. The merge changes neither of them.

What's done well

• ✅ The merge commit message names each resolution decision and why, so the conflict could be graded from the message alone.

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

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

[neilmartin83]

Warning

⚠️ Needs changes. Moves three device secrets off argv into --*-file flags and routes every -vvv, --dry-run and error-body path through one new redactor. No commits since round 2. This round closes the two findings that reached three rounds: both are promoted, with their merge impact stated.
Blocking: (1), (2), (3).

Rating: 3/5

• Would be a 5 with the CHANGELOG restored, stdin dropped from readSecretFile, and CDATA handled in xmlElementRe.
• The rating holds from round 2. The code is byte-identical to e85ce0e, and both promotions keep their findings at IMPORTANT, so the number does not move.

Prior findings status

#	Location	Rounds	State	Notes
(1)	CHANGELOG.md:1045	2	🔴 Open	No change, no reply. Still 1,067 lines against 1,257 on main, and still ends on as t
(2)	internal/commands/pro_device_actions.go:1761	3	🔴 Open	No change, no reply. Promoted this round, see (2)
(3)	internal/redact/redact.go:157	3	🔴 Open	No change, no reply. Re-confirmed by execution this round. Promoted, see (3)

⚠️ IMPORTANT (1) (correctness, merge resolution, open 2 rounds) — CHANGELOG.md:1045: the merge of #407 truncates the CHANGELOG and deletes 250 lines of v1.28.0 release notes

Carried from round 2. At e85ce0e the file has 1,067 lines, and origin/main has 1,257. The file ends mid-word on as t. Everything after the pro audit -o raw bullet in v1.28.0 is gone, and a stray line holding only ` sits at :1045. git merge-tree against current main is still clean, so this lands silently.

Failure scenario: after merge, v1.28.0's notes on main stop mid-sentence. The record of the breaking stray-positional refusal (exit 2) and of the --field/--out-file changes is lost, and every later PR inherits the truncated file.

Suggested fix:

git checkout b3ad3f29 -- CHANGELOG.md
# then re-insert this PR's two Unreleased sections above
# "### Breaking — `--set` refuses credential fields …"

Fixed when: git diff origin/main...HEAD -- CHANGELOG.md shows only additions inside ## Unreleased, and the file ends as it does on main.

⚠️ IMPORTANT (2) (security, via security-reviewer, open 3 rounds) — internal/commands/pro_device_actions.go:1761: --*-file - adds a stdin credential channel the policy forbids

Carried from round 1. readSecretFile reads os.Stdin when the path is -. --unlock-token-file advertises this in its flag help (:1138) and its Long (:1113). --pin-file, --new-password-file and set-auto-admin-password --password-file share the reader (:1067, :1118, :1738). .claude/rules/credentials-and-auth.md says: "Never accept credentials (passwords, tokens, client secrets) via CLI flags or stdin."

Failure scenario: printf '%s' "$TOKEN" | jamf-cli pro md clear-passcode --serial X --unlock-token-file - --yes is accepted, and --help presents it as the supported form.

Disposition (round 3) — promoted. Merge impact: merging adds a dedicated stdin channel for three device secrets, which the repo's one CRITICAL policy forbids by name, and teaches it in --help. Dropping - costs CI nothing, because --unlock-token-file <(printf '%s' "$TOKEN") hands os.ReadFile a /dev/fd path with the same effect. The existing --body-file-or-stdin body that can carry a Find My PIN (:241) predates this PR and is not a precedent for a new secret-only flag.

Suggested fix:

-	if path == "-" {
-		data, err = io.ReadAll(os.Stdin)
-	} else {
-		data, err = os.ReadFile(path)
-	}
+	data, err := os.ReadFile(path)

Also remove the - wording from the flag help, the Long and the Example, and the or - for stdin cell in the CHANGELOG table. The alternative is to amend credentials-and-auth.md in this PR with an explicit, reasoned exception.

Fixed when: no --*-file secret flag reads os.Stdin, or the policy file carries the exception. A test pins whichever was chosen.

⚠️ IMPORTANT (3) (silent-failure, via silent-failure-hunter+security-reviewer, open 3 rounds) — internal/redact/redact.go:157: a CDATA-wrapped credential passes redact.Body unchanged

Carried from round 1. xmlElementRe's text run is [^<]*, and <![CDATA[ opens with <, so the element never matches. xmlLeafTextRe (:159) has the same run. Re-confirmed this round by running redact.Body from the PR head:

in : <smtp_server><password><![CDATA[p&ss<1]]></password></smtp_server>
out: <smtp_server><password><![CDATA[p&ss<1]]></password></smtp_server>
in : <smtp_server><password>plain</password></smtp_server>
out: <smtp_server><password>[REDACTED]</password></smtp_server>

Failure scenario: jamf-cli -n pro classic-smtp-server update --from-file smtp.xml, with the password in CDATA because it contains & or <, prints the password in [dry-run] Request body:. -vvv does the same.

Disposition (round 3) — promoted. Merge impact: this is not a regression against main, which redacts none of these bodies. But the PR's stated contract is one rule used by every log and preview path, and it ships with a verified bypass on exactly the passwords most likely to be CDATA-wrapped, the ones with XML metacharacters. The fix is one regular expression plus a test.

Suggested fix:

-	xmlElementRe = regexp.MustCompile(`<(?P<name>` + namePattern + `)(\s[^>]*)?>[^<]*</[^>]*>`)
+	xmlElementRe = regexp.MustCompile(`(?s)<(?P<name>` + namePattern + `)(\s[^>]*)?>(?:[^<]|<!\[CDATA\[.*?\]\]>)*</[^>]*>`)

Fixed when: a redact_test.go case shows <password><![CDATA[x]]></password> redacting to <password>[REDACTED]</password>, and a sibling CDATA element under a non-credential name stays intact.

Review coverage and scope

• Design and architecture: unchanged since round 2
• Correctness: (1) re-verified against current origin/main (cc702a4, includes #404). git merge-tree is clean
• Security: (2) and (3) re-verified at the cited lines. (3) re-confirmed by execution, with internal/redact copied unmodified from the PR head into a scratch module
• Test coverage: CI is green at e85ce0e
• Reliability: unchanged since round 2
• Documentation currency: covered by (1)
• Project rules compliance: the violation is (2)
• [na] Performance: no change since round 1. Frontend and cross-repo contracts do not apply

Incremental diff e85ce0e..e85ce0e: empty. The round ran because the abort gate's fifth condition failed: (2) and (3) reached three rounds with no disposition. No PR comments or description edits since round 2, and no labels. Lanes run: none. Every eligible lane is already in ever. Never run: fidelity-reviewer, which does not apply because there is no linked spec. Conventions: .claude/rules/credentials-and-auth.md read from the review worktree, plus the root CLAUDE.md, coding-style.md and classic-api.md from the session checkout. The PR changes none of the four.

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

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

[ktn-jamf]

Thanks, Neil. All three findings are addressed in the push that ends at 66dc08e.

(1) Truncated CHANGELOG — bf2a747. This is the merge of current main. CHANGELOG.md is now main's file, with this PR's two Unreleased sections placed first. That also restores the v1.28.0 notes the earlier merge cut off. Check: git diff origin/main -- CHANGELOG.md shows one hunk under ## Unreleased, deletes nothing, and the file ends as it does on main. (The later commit for finding 2 changes only lines inside this PR's own section.)

(2) --*-file - read a credential from stdin — 66dc08e. readSecretFile no longer reads stdin. - is refused with a usage error (exit 2) that asks for a file path, on --new-password-file, --pin-file, --unlock-token-file and set-auto-admin-password --password-file. The - wording is gone from clear-passcode's flag help, Long and Example, and from the CHANGELOG table. A process substitution such as <(printf '%s' "$TOKEN") still works, because it is a path. The new TestSecretFileFlags_RefuseStdin covers each flag. Proof: with the test in place and the refusal not yet applied, all four subtests failed, and they pass with it.

(3) CDATA not redacted — fd859d3 (plus a3327c3, formatting only). The element pattern and the credential-container leaf pattern now accept CDATA sections, including ones that span lines, inside the text run. The new cases include your <smtp_server><password><![CDATA[p&ss<1]]></password></smtp_server>, <password><![CDATA[x]]></password>, a multi-line CDATA, a CDATA <name> sibling that stays intact, and a CDATA key inside institutional_recovery_key. Proof: before the fix, every CDATA case failed. A plain-text case with surrounding whitespace already passed, and it stays as a guard.

Two existing tests changed, with the author's approval:

• The stdin subtests of TestSecretFileFlags_SendTheFileContent and the empty pipe subtests of TestSecretFileFlags_RefuseAnEmptySource are deleted, because they asserted the stdin behaviour finding 2 removes (66dc08e).
• notACredentialPrinter gains pro mobile-devices clear-passcode as a command that sets a device secret, beside set-recovery-lock (05a72bc). The guard from fix(mcp): judge run_command on the command and flags cobra resolves #402 requires the entry, because this branch's clear-passcode help names the unlock token.

Also from the merge: 0ed5d8b classifies the three new --*-file flags as credential paths held to --input-dir over MCP, as --password-file already is.

Gates on 66dc08e: go test ./... has no failures, make lint reports 0 issues, and make verify-generated reports the generated code is up to date.

Follow-ups, not in this PR:

• Read the secret only after the target is validated (pro_device_actions.go, set-recovery-lock).
• A distinct exit code for a missing or unreadable secret file.
• A test for the empty-password guard, which only the no-echo prompt reaches.
• Redaction of other body shapes in internal/redact.
• The MCP secret-file exemption test note.
• CHANGELOG wording in the device-secrets section.
• Call-site tests for the redaction in the platform helpers.
• The Security Cloud client error body, which predates this PR.

[neilmartin83]

Tip

✅ Merge-ready. All three blocking findings are fixed in the push ending at 66dc08e. The CHANGELOG is main's file plus this PR's sections, - is refused on every device-secret --*-file flag, and CDATA-wrapped credentials redact. I checked the last two on pro-nmartin.

Rating: 5/5

• Every promoted finding is fixed and verified, two of them on the wire. The remaining items are the follow-ups Keaton listed. None blocks.

Prior findings status

#	Location	Rounds	State	Notes
(1)	CHANGELOG.md	3	✅ Fixed	bf2a7479. git diff origin/main -- CHANGELOG.md deletes 0 lines. The file is 1,439 lines against main's 1,379, and its last two lines match main's, so the v1.28.0 notes are back
(2)	internal/commands/pro_device_actions.go:1756	4	✅ Fixed	66dc08e. readSecretFile refuses - with exitcode.Usage and otherwise reads only os.ReadFile. TestSecretFileFlags_RefuseStdin passes for all four flags. Wire: --unlock-token-file - exits 2 with "does not read stdin; pass the path of a file holding the secret". The remaining os.Stdin uses are the existing request body (:724) and the no-echo TTY prompt (:1743), neither of which this finding was about
(3)	internal/redact/redact.go:157	4	✅ Fixed	fd859d3f. I ran redact.Body from the PR head, copied unmodified into a scratch module, on 8 cases. These redact: your SMTP case, a bare CDATA, a multi-line CDATA, text mixed with CDATA, a split ]]> CDATA, and two CDATA passwords around a CDATA <name>, which stays intact. So does a nested ldap_server/account/password. Wire: -n pro classic-smtp-server update 1 --from-file with <password><![CDATA[PR406&SENT<INEL]]></password> previewed <password>[REDACTED]</password>. With -vvv added, the sentinel appeared 0 times

Review coverage and scope

• Delta since e85ce0e: the main merge (bf2a7479) and the five commits listed above, 8 files and +79/−36 outside the merge
• Two existing tests changed. The stdin and empty pipe subtests are deleted because they asserted the behaviour (2) removes. clear-passcode is added to notACredentialPrinter, which the #402 guard requires because its help names the unlock token. Both changes are correct
• 0ed5d8b7 holds the three new --*-file flags to --input-dir over MCP, as --password-file already is
• Gates: go test ./internal/... ./generator/... reports no failures. CI ci passes at 66dc08e. git merge-tree with current main is clean
• Wire, on pro-nmartin: a --dry-run update, which sends no write, and the - refusal on a nonexistent serial. No device was touched and nothing was created
• [?] Edge case, not raised as a finding: an XML comment inside the element (<password><!-- c --><![CDATA[s]]></password>) is not redacted. No Jamf body or schema emits that shape. It belongs with "redaction of other body shapes" in Keaton's follow-up list
• Project rules: credentials-and-auth.md (the stdin clause is now satisfied), coding-style.md, classic-api.md and the root CLAUDE.md, from the session load

Grading: author commits since round 3, plus a main merge. No lanes dispatched, since every eligible lane is already in ever. Head graded: 66dc08e.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 66dc08e}