fix(skills): redact secrets on every mural CLI output channel

[Jamie Kim (jkim323)]

Pull Request

Description

The mural CLI leaked unredacted secret material on multiple output channels. main() printed raw MuralError text to stderr, the four structured error envelopes were serialized without redaction, _emit_json wrote unredacted payloads to stdout, and _transport.py embedded raw response bodies into error messages. OAuth responses, refresh-token failures, and asset-upload failures could therefore surface bearer tokens, client secrets, and SAS query strings in terminal output and CI logs.

This PR turns redaction into a structural barrier at the emit boundary rather than a text filter applied after serialization, then extends that barrier to every channel a later security sweep found still uncovered.

Core redaction barrier

• All three main() stderr handlers pass error text through _redact before printing.
• New _emit_json_error redacts and emits error envelopes to stderr; the four envelopes (human_authored_widget_protected, tag_merge_conflict, AREA_CAPACITY_EXCEEDED, bulk_atomic_abort) route through it.
• New _redact_payload redacts the payload structure before serialization. Regex-scrubbing serialized JSON was structurally unsafe: the form-value pattern consumed closing quotes and produced output that no longer parsed, and escaped quotes in nested strings defeated the JSON pattern. It walks str, dict, list, tuple, set, and frozenset, and is key-aware, so a mapping key matching _REDACT_KEYS is masked regardless of its value type or quoting. That is strictly stronger than the previous string-level behavior.
• _emit_json (stdout) redacts through the same barrier.
• New _error_excerpt in _transport.py redacts and then truncates response bodies at MAX_ERROR_EXCERPT_CHARS (512). Ordering is deliberate: redacting first means truncation cannot sever a key name from its value and defeat a substitution.

Follow-on hardening

A security sweep of this branch (OWASP Top 10, MCSB, Secure by Design, OWASP MCP, OWASP Agentic) found five coverage gaps in the barrier above. All five are closed here.

• Azure SAS signatures now mask regardless of host casing. The SAS pattern lacked re.IGNORECASE while _validate_asset_url normalizes with parsed.hostname.lower(), so a mixed-case host such as acct.Blob.Core.Windows.Net passed validation and then defeated redaction, putting a full sig= into a debug log line. A standalone sig= pattern was added as well, because an upstream error body can quote SAS parameters without the URL and truncation can strip the host prefix.
• _build_api_error now shares the excerpt bound. _error_excerpt covered five of six exception-construction sites. The sixth carries every non-2xx API response and was bounded only by MURAL_MAX_BODY_BYTES (16 MiB default), four orders of magnitude above the 512-character cap.
• The record output channel now redacts. _emit_records and _emit_record printed _format_output(...) directly while _emit_json redacted, which left the primary stdout data path as the only channel without a barrier.
• Logout transparency restored. A CodeQL Autofix commit on this branch replaced removed.append(key) with removed.append("***") in _logout_remove_credentials. The masked values are the env-style identifiers in _KNOWN_CREDENTIAL_KEYS, not credential values, so mural auth logout reported (keys: ***, ***, ***) and no secret was ever protected. The change also left the equivalent errors[key] path uncut, so it did not achieve its own stated goal. Reverted, and code scanning alert 723 is dismissed as a false positive: the expression it flags is the redaction sink itself, which CodeQL does not model as a sanitizer.
• Threat models corrected. Both SECURITY.md and docs/security/security-model.md described an "embedded stdio MCP server" that does not exist. The package registers no mcp subcommand, implements no JSON-RPC framing, and exposes no tool dispatch; the only MCP artifact is a MCPInvalidParamsError validation exception documented as retained for CLI helper compatibility. The repo-wide model additionally carried a full "Mural Skill MCP Server" entry asserting a python -m mural mcp deployment and scope re-checks "at dispatch". Both now describe the real surface: an argparse CLI whose stdout and stderr are captured into agent context through a terminal tool. The genuine hardening items from that entry were relocated rather than dropped.

Test isolation fix

test_autoload_credentials_returns_none_when_file_absent never pinned a credential backend, so resolve_backend fell through to auto and read the developer's real keyring. It failed on any machine holding real Mural credentials, and its assertion form (assert "MURAL_CLIENT_ID" not in env) made pytest render the whole mapping into the failure message, printing live secret values. It now pins MURAL_CREDENTIAL_BACKEND=file, reuses the existing _isolate_credential_env helper, and compares key sets so a failure reports names only.

Out of scope: the residual print(*) sites in _cli_auth.py, _commands.py, and _operations.py, which #2756 explicitly defers to #2460. The security sweep also recorded lower-severity residuals (mapping-key redaction, argparse writing raw argv before the guarded block, SAS scope and expiry validation) that are not addressed here.

Related Issue(s)

Fixes #2756

Type of Change

Code & Documentation:

• Bug fix (non-breaking change fixing an issue)
• Documentation update

AI Artifacts:

• Copilot skill (.github/skills/*/SKILL.md)

Other:

• Script/automation (.ps1, .sh, .py)

Testing

Run from .github/skills/experimental/mural:

• uv run ruff check . - all checks passed
• uv run ruff format --check . - 44 files already formatted
• uv run pytest tests -q - 860 passed, 143 skipped, 0 failed

The suite has no failures. The one previously failing test is the environment-dependent credential test described above, fixed in this PR.

Repository gates: npm run lint:py, npm run lint:frontmatter, npm run lint:tables, and npm run validate:skills all clean (74 skills, 0 errors, 0 warnings).

44 test functions were added across four files:

• tests/test_redaction.py (29) - main() error sinks and source-level pins, _redact_payload container coverage, sensitive mapping keys, _emit_json stdout channel, SAS host casing and bare-signature masking, record-channel redaction
• tests/test_mural_transport.py (11) - _error_excerpt unit behavior, _build_api_error bounding and masking, end-to-end transport coverage
• tests/test_logout_transparency.py (4) - removed_keys content and rendered summary, which no test previously pinned
• tests/test_credential_storage.py - the isolation fix described above

Each security fix was checked for vacuity by temporarily reverting the source change and confirming the new tests fail. That check earned its keep: it caught that two of the four logout-transparency tests exercise only the renderer and pass against the regressed code, so the coverage claim is scoped to the two that do fail.

Checklist

Required Checks

• Documentation is updated (if applicable)
• Files follow existing naming conventions
• Changes are backwards compatible (if applicable)
• Tests added for new functionality (if applicable)

Security Considerations

• This PR does not contain any sensitive or NDA information
• Any new dependencies have been reviewed for security issues
• Security-related scripts follow the principle of least privilege

Additional Notes

Two points worth a reviewer's attention.

Alert 723 is dismissed, not fixed. CodeQL flags print(json.dumps(_redact_payload(payload), indent=2)) in _emit_json, which is the redaction sink introduced by this PR. _redact_payload is not modeled as a sanitizer, so the safest path in the module is the one most likely to be flagged. Reverting the Autofix restores the flagged dataflow, which is why the dismissal is a prerequisite rather than a cleanup step. Modeling the sanitizer properly is tracked as follow-up work.

Record-channel redaction can over-mask user content. Routing _emit_records through _redact_payload means a Mural sticky note whose text legitimately reads code=ABC123 now renders as code=*** in list output. That is a real behavior change on the primary data path, accepted because the alternative leaves the busiest stdout channel as the only one without a barrier, and because _emit_json already accepts the same trade-off for envelopes.

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.87%. Comparing base (3c3dc02) to head (0b1f3b8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...kills/experimental/mural/scripts/mural/__init__.py	57.14%	3 Missing ⚠️
...lls/experimental/mural/scripts/mural/_transport.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2775      +/-   ##
==========================================
- Coverage   82.98%   78.87%   -4.12%     
==========================================
  Files         183      113      -70     
  Lines       33787    15779   -18008     
  Branches       25       25              
==========================================
- Hits        28038    12445   -15593     
+ Misses       5746     3331    -2415     
  Partials        3        3              

Flag	Coverage Δ
docusaurus	89.92% <ø> (ø)
pester	83.83% <ø> (+0.37%)	⬆️
pytest	69.73% <90.00%> (-13.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...lls/experimental/mural/scripts/mural/_constants.py	100.00% <100.00%> (ø)
...skills/experimental/mural/scripts/mural/_output.py	88.77% <100.00%> (+2.19%)	⬆️
...lls/experimental/mural/scripts/mural/_transport.py	84.66% <91.66%> (+2.09%)	⬆️
...kills/experimental/mural/scripts/mural/__init__.py	84.21% <57.14%> (+3.16%)	⬆️

... and 75 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Eval Execution

✅ Status: Passed

• Artifacts evaluated: 0
• Specs run: 0
• Assertions passed: 0
• Assertions failed (blocking): 0
• Assertions failed (advisory): 0
• Failed specs (merge-blocking): 0

No changed AI artifacts required evaluation.