fix(security): mask resolved run secrets

[yairfalse]

Summary

• collect per-run resolved secret values from literal secret-like task env, secret_refs, and OIDC credentials
• centralize secret-pattern matching and redact structured values under secret-like keys
• apply engine-env plus run-scoped masking to occurrences, notifications, attestations, run summaries, and outbox payloads
• rebased onto current main after C2 (fix(team): enforce per-team coordinator authz #219) so the PR includes the latest coordinator authz baseline

Review fixes

• added OIDC credential collection coverage using an injected fake OIDC service
• added a full egress regression test that runs a pipeline and asserts file/literal/OIDC secrets are masked in .sykli/occurrence.json, notification webhook payload, run-summary HTTP wire body, and attestation.json
• confirmed key-name redaction is already covered by SecretMaskerTest
• added a collection-site comment marking the task.env injection contract

Verification

• cd core && mix format
• cd core && mix test (1770 tests, 0 failures, 1 skipped, 13 excluded)
• cd core && mix credo
• cd core && mix escript.build
• test/blackbox/run.sh (168 passed, 0 expected-red, 0 failed)

Audit note

Verified S3 against current code: occurrence enrichment and outbox only collected engine env values, while secret_refs, OIDC credentials, and literal task env secrets were injected at execution time and not threaded into masking boundaries.

[yairfalse]

Review

Overview

Completes audit finding S3. Collects per-run resolved secret values from three injection sources (literal task env matching secret patterns, secret_refs incl. file, and OIDC creds), threads them via a new TaskResult.secret_values field, aggregates run-level in Sykli.run_secret_values/1, and applies SecretMasker.mask_deep at every egress: occurrence persist, attestation write, notification webhook, outbox enqueue, run-summary encode (wire), run-client publish. Plus a new centralized Sykli.Services.SecretPatterns module and a key-name redaction "monster upgrade" in SecretMasker.

Strengths (verified in diff)

• Explicit threading, no globals. Per-task collection → TaskResult.secret_values → run-level aggregation in sykli.ex run_secret_values/1, then threaded as :secrets opts to every masking call. Conforms to project conventions (no Process dictionary).
• Centralization is real. SecretPatterns is the single source; outbox.ex's duplicate secret_env_patterns/0 + secrets/0 are deleted.
• Disk-wire parity closed. RunSummary.encode(summary, secrets: ...) is the one encode path, called identically by the outbox queue and by RunClient.publish — disk and wire produce the same masked bytes.
• Attestation correctly covered without touching attestation.ex. The choke point is write_attestation(attestation, path, secrets) in enrichment.ex — masks before writing.
• Key-name redaction is recursive and type-complete. mask_value/1 handles binary/map/list/nil/other; preserves structure while masking the entire subtree under a secret-like key. The removed mask_deep(data, []) shortcut is intentional (key-name redaction must run even with empty explicit secrets) — correct.
• All three injection sources collected at the right points in executor.ex (literal env via values_from_pairs, refs via the now-tupled resolve_secret_refs/1 return, OIDC via Map.values).

Suggestions (priority order)

1. End-to-end masking test is missing. secret_masking_test.exs asserts values are collected into TaskResult.secret_values (executor side). It does not assert those values are actually masked in .sykli/occurrence.json, the notification webhook payload, the run-summary wire bytes, or attestation.json. The threading is correct in code, but a regression test on the bytes that actually leave would protect against a future refactor accidentally dropping the secrets: opt — exactly the class of bug C4's IPv6 review caught. Recommend adding one focused test that runs a pipeline with a file secret + an oidc/literal secret, then refute raw value / assert mask on each of the four output surfaces.
2. OIDC source isn't exercised by the new test. Only file + literal are covered. Add a test that stubs OIDCService.exchange or injects an OIDC env and asserts result.secret_values contains it.
3. Confirm SecretMasker has a test for the key-name redaction branch. The diff didn't touch secret_masker_test.exs; verify (or add) a test that a value under a secret-like key gets masked even when the value isn't in the explicit secrets list. That's the monster-upgrade contract.
4. Minor — duplicate encode. maybe_sync_run_summary encodes once for the outbox, and RunClient.publish re-encodes inside (also with :secrets). Both deterministic, output matches → not a bug, just redundant work. Could pass the already-encoded payload to publish_raw directly. Not blocking.

Risks (none severe)

• New injection sources go unseen. Collection is enumerative (literal env / secret_refs / OIDC). A future code path resolving a new secret source into task.env without updating the collection would silently leak. Worth a one-line comment at the collection site naming this contract.
• Mask-replaces-type for non-string values. mask_value/1 falls through to do: @mask for any non-binary leaf, so a numeric value under a secret-like key becomes the mask string. Acceptable (secrets are virtually always strings) — flag only if shape preservation is ever guaranteed downstream.

Security

• Threading is explicit and immutable — no risk of a global mask list racing across runs.
• The masker handles structured data correctly; no obvious bypass via nested lists/maps under secret-like keys.
• The outbox-masked-but-wire-not divergence the audit called out is the exact thing this fixes.

Performance

• Removed empty-secrets shortcut means mask_deep always walks the structure for key-name checks. Linear in payload size; negligible against any I/O.

Conventions

• CHANGELOG entry present. Confirm docs/team-mode-security.md is updated to describe the new run-scoped masking guarantees (dual-surface story).

Verdict

Approve with the two test additions (end-to-end masking + OIDC collection). Code is correct, well-threaded, and closes audit S3 including the disk-wire divergence and the centralization the audit asked for. The main gap is test coverage of the outputs (not just the collection) — fix that and this is rock-solid.

[yairfalse]

Addressed the review gaps in commit 3b4a8a7.

Changes:

• Added OIDC collection coverage via an injected fake OIDC service.
• Added a full egress regression test that runs a pipeline and asserts file/literal/OIDC secrets are masked in occurrence.json, notification webhook payload, run-summary HTTP wire body, and attestation.json.
• Confirmed key-name redaction was already covered in SecretMaskerTest.
• Added a collection-site comment marking the task.env secret-injection contract.

Local verification:

• cd core && mix format
• cd core && mix test — 1770 tests, 0 failures, 1 skipped, 13 excluded
• cd core && mix credo
• cd core && mix escript.build
• test/blackbox/run.sh — 168 passed, 0 expected-red, 0 failed