fix(security): store webhook secrets by reference and migrate plaintext (Webhook Feature Package)

[andrexibiza]

Part of #84834 — Webhook Feature Package Task 8. Closes #77471.

Current Task 8 semantics

The published implementation closes the four concrete review defects while preserving the original write → resolve → verify → atomic switch → scrub migration contract.

Secret-reference migration

• legacy route/config plaintext remains untouched until every staged secret is persisted and resolves back to the exact original value;
• route/config switch is atomic and migration receipts contain references/status only, never secret values;
• pre-switch failures preserve the source and suppress backend exception detail that may contain secret material;
• backup scrubbing happens only after the reference-backed route/config is live.

Review hardening

• malformed hermes webhook subscribe updates that find neither secret_ref nor legacy secret mint a fresh secret and persist only its reference instead of saving a credential-less route;
• WEBHOOK_REVOLUTION_TASK8_MIGRATION_COMMAND_V1 was removed and a regression pins its absence;
• webhook secret writers share a bounded cross-process lock under the active Hermes home; production migration holds it across the complete read → persist → verify → switch transaction;
• CLI/migration secret resolution delegates to the gateway's existing WebhookAdapter._resolve_secret_ref authority instead of carrying a second get_secret → env fallback chain;
• unresolved references fail closed in the CLI test path rather than using an empty HMAC key.

Current cleanup head

Head: aa639d58e87cb4a7f7476a93ec874afb8b342337.

The two artifacts/webhook-repair/.../task-08 receipt files have been removed from the branch. The review surface is now 26 files rather than 28; this is cleanup only and does not inherit the prior head's verification.

Verification truth

The previous correctness head e32bc0a13808413740dc5ca020816bed5a4eeb39 had exact-head green CI 32388358900, Docker 32388356067, and Nix 32388356107.

Those receipts are now historical because the branch moved to aa639d58.... A fresh exact-head matrix is required; no green is inherited.

Reviewability / topology

The maintainer objection to the historical large diff remains valid. This cleanup is not the final semantic compression. Task 8 ownership must contract further to secret-reference persistence/resolution/migration surfaces and focused tests. Adjacent ownership is explicitly elsewhere:

• effective webhook config: fix(webhook): preserve profile-correct effective config through semantic train compression #85002
• authenticated intake/idempotency/raw bounds: fix(webhook): close Task 10 intake identity and raw-bound contracts #90236
• provider signature authority: refactor(webhook): isolate explicit provider signature authority #85318
• profile admission/completion ownership: feat(webhook): run route completion scripts after the agent run (Webhook Feature Package) #85645
• terminal partial delivery truth: feat(webhook): add multi-target delivery fan-out (Webhook Feature Package) #85644

Related #85054.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(security): store webhook secrets by reference and migrate plaintext (Webhook Feature Package)

The migration design is the right shape: write → resolve → verify → atomic switch → scrub, value-free receipts, and from None to keep backend exceptions (which may embed the secret) out of tracebacks. Points:

1. _cmd_subscribe edge case leaves a route with no secret — hermes_cli/webhook.py: on update with no --secret where the existing route has neither secret_ref nor secret, both secret_ref and secret end up empty and the route is saved without any credential — it will then fail at gateway startup with a confusing message. Fall back to generating a fresh secret or error out in that branch.

2. Pipeline watermarks in production code — # WEBHOOK_REVOLUTION_TASK8_MIGRATION_COMMAND_V1 in hermes_cli/webhook.py; remove.

3. Migration runs on every route reload — gateway/platforms/webhook.py _reload_dynamic_routes calls migrate_webhook_routes(subs_path) on every mtime-gated reload until no plaintext remains. It is idempotent, but it re-reads/re-parses the file and (on first run) writes into the profile .env from inside a long-running gateway process, concurrently with CLI writers — and _default_store (save_env_value) is not protected by the WebhookRouteStore lock. Consider a one-shot "migrated" marker or a store-level migration flag, and confirm .env writes are serialized.

4. Duplicated resolution logic — _resolve_route_secret (hermes_cli/webhook.py) and _resolve_secret_ref (gateway/platforms/webhook.py) implement the same get_secret → env fallback chain twice (the gateway version also special-cases WEBHOOK_SECRET). One shared helper would prevent the two from drifting.

5. Good: _is_webhook_secret_config_key blocks config set ...webhook...secret with a clear remediation, and get_config_value masks scalar WEBHOOK_SECRET reads that redact_config_value would pass through.

[egilewski]

too large to review safely

This PR changes 1295 production lines before tests and docs. Please split it or add a focused justification if it should stay together.

Signed: GPT-5.6-luna-high in Codex

[andrexibiza]

Addressed the four concrete Task 8 findings on the published branch at e32bc0a13808413740dc5ca020816bed5a4eeb39:

1. No-secret update edge: an update of a malformed legacy route with neither secret_ref nor plaintext secret now mints a fresh secret, persists it, and writes only the returned reference. A route is never saved back with no credential.
2. Campaign watermark: WEBHOOK_REVOLUTION_TASK8_MIGRATION_COMMAND_V1 is gone from production code; a regression asserts it stays gone.
3. Writer serialization: webhook secret writers now share a bounded cross-process O_EXCL lock under the active Hermes home. Production migration holds that lock across read → persist → resolve/verify → atomic switch, so the gateway migration path cannot race CLI .env writes. The new concurrency regression proves one active writer at a time.
4. Resolution ownership: CLI/migration resolution no longer carries a second get_secret → env fallback chain. hermes_cli.webhook_secrets.resolve_webhook_secret() delegates to the gateway's existing WebhookAdapter._resolve_secret_ref authority.

The existing value-free migration error behavior and pre-switch byte-preservation are retained. Exact-head CI 32388358900 and Docker 32388356067 are green; Nix 32388356107 is still running, so I am not claiming the full matrix until that exact-head receipt lands.

The separate reviewability objection is also correct; I am not asking anyone to review the historical 28-file train as-is. I am treating the current branch as semantic source while compressing Task 8 onto the current webhook ownership graph, rather than defending stale stacked spillover.

[andrexibiza]

Agreed on reviewability. I am not defending the 1,295-production-line historical shape or asking maintainers to review it.

The branch accumulated adjacent webhook-task ownership before the campaign boundaries were cleaned up. The correct end state is a semantic Task 8 slice: secret-reference persistence/resolution, migration, CLI secret surfaces, and focused security/regression tests. Profile admission belongs to #85645; effective config belongs to #85002; authenticated intake/idempotency belongs to #90236; provider signature authority belongs to #85318.

I have fixed the substantive Task 8 defects first so compression has a correct semantic source. Current exact head is e32bc0a13808413740dc5ca020816bed5a4eeb39; CI and Docker are green and Nix is still running. The next branch rewrite will remove the stale cross-task/artifact surface rather than trying to justify it.

[andrexibiza]

Exact-head hosted verification receipt

The cleanup head named in the body, aa639d58e87cb4a7f7476a93ec874afb8b342337, now has its own repository-hosted acceptance matrix:

• CI 32395764829 — success
• Docker Build, Test, and Publish 32395763941 — success
• Nix flake check 32395763927 — success
• unresolved formal review threads — 0

This closes the body’s explicit “fresh exact-head matrix required” residue. It does not waive the separately documented reviewability/topology requirement to contract Task 8 further around secret-reference persistence/resolution/migration ownership before landing.