fix(hooks): version outbound webhook envelope and fail closed on invalid config

[andrexibiza]

Part of #84834 — Webhook Feature Package Task 14.

Current disposition

The outbound-webhook lane is semantically restacked directly onto current main as a one-commit/three-file object.

• base: 533886c8b8eb67ff8b389b7f48e7d5e5d9c575b9
• head: d136e8562c372dc9cbf1dbacd94d79b2079431d3
• commits: 1
• files: 3
• mergeable: yes

The restack was proof-preserving: comparison from the previous base to current main showed that none of this lane's three owned files changed upstream. The new object overlays the exact previously reviewed production/test blobs onto the exact current-main tree rather than replaying historical campaign topology.

Owned contract

• versioned outbound webhook envelope (schema_version: 1);
• explicit known-field validation; inline plaintext secret is rejected with a value-free migration hint;
• exactly one opaque credential reference (secret_ref, with secret_env compatibility) and no raw environment fallback after the profile secret authority returns a scoped miss;
• unresolved explicit references disable the target rather than silently sending unsigned;
• V2 signs <timestamp>.<raw body> and receiver documentation requires freshness, constant-time HMAC verification, and delivery-ID deduplication; timestamp binding alone is not represented as replay prevention;
• redirects are refused and delivery remains bounded/fire-and-forget through the existing worker queue.

This closes the cross-profile authority defect in the old implementation: agent.secret_scope.get_secret is the sole credential resolver. UnscopedSecretError fails the target closed; unexpected runtime/import failures remain visible instead of being swallowed by a blanket except Exception.

Exact-head verification

All hosted acceptance receipts are attached directly to d136e8562c372dc9cbf1dbacd94d79b2079431d3:

• CI 32446033866 — success
• Docker 32446033398 — success
• Nix 32446033397 — success

No status is inherited from the prior head. The former exact-head Windows progress failure belonged to 71c40ca...; the current-main restack now passes that repository matrix on its own exact object.

Topology

This remains the Task 14 outbound-hook authority under #84834. It does not absorb inbound webhook intake (#90236/#90995), verifier authority (#85318), session policy (#90304), callback transport (#85675), or final Task 19 integration (#85640).

[spfcraze]

This was generated by AI during triage.

Summary:
The _resolve_request_profile extraction into WebhookProfileAdmissionMixin drops the profile_allowlist argument that webhook.py passes to profiles_to_serve, so a configured multiplex_profile_allowlist no longer restricts which /p/<profile>/ webhook prefixes are admitted.

Problems:

• gateway/platforms/webhook.py:584-590 on main passes profile_allowlist=getattr(cfg, "multiplex_profile_allowlist", None); the extracted mixin (gateway/platforms/webhook_profile_admission.py) calls profiles_to_serve(multiplex=True) with no allowlist.
• profiles_to_serve with profile_allowlist=None returns every named profile under profiles/ (hermes_cli/profiles.py), so a profile excluded via multiplex_profile_allowlist is admitted on the webhook path while the sibling gateway/platforms/api_server.py:1991 still honors the allowlist.

Solution:
Restore the argument in the mixin: profiles_to_serve(multiplex=True, profile_allowlist=getattr(cfg, "multiplex_profile_allowlist", None)).

Evidence

no deterministic fact backs this claim — model belief, not executed or read evidence

---

Checked against ae95b20 — the tip of campaign/webhook-outbound when this was written — and 2ae9693, main at the same moment.

[Enough1122]

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

fix(hooks): version outbound webhook envelope and reject unknown config fields (Webhook Feature Package)

1. Silent breaking change for inline secret configs — secret is no longer in _KNOWN_FIELDS, so an existing target using the previously documented inline secret is now rejected wholesale (target dropped with only a warning log). The new test test_inline_plaintext_secret_is_not_a_supported_field codifies this. Since a mistyped/unmigrated config now silently stops deliveries, consider emitting a clearer startup-level error or a migration hint pointing to secret_ref/secret_env so operators are not surprised. (agent/outbound_webhooks.py, _KNOWN_FIELDS ~line 278)
2. Unresolved secret reference now skips the whole target — _resolve_secret returns "" on an unresolvable reference and _parse_single_target drops the target (if secret == "": return None). Targets that previously delivered unsigned are now disabled entirely, a behavior change beyond the fail-closed intent. Worth stating explicitly in docs: a typo'd secret_env name disables the webhook rather than sending unsigned deliveries.
3. Broad except Exception swallows real failures — in _resolve_secret, from agent.secret_scope import get_secret failing (import error, not just lookup failure) is converted into value = "" and reported as "secret reference did not resolve". Narrowing the catch to expected lookup errors would make diagnosis easier.
4. V2 signature replay-safety is receiver-dependent — the timestamp is second-granularity and the new X-Hermes-Signature-V2 binds timestamp + body, but nothing documents the expected receiver-side freshness check/leeway. A brief note in the module docstring on the required verification (and clock-skew allowance) would make the "replay-safe contract" actionable.

[andrexibiza]

Rechecked this against the current PR head a1589f7c856729e0fe74ee06f26fb1c06be766f6, not the earlier ae95b20 snapshot: the profile-admission extraction is no longer in this PR at all. gateway/platforms/webhook_profile_admission.py does not exist on this head and gateway/platforms/webhook.py is not a changed file in the PR. Current upstream keeps the admission logic inline and passes profile_allowlist=getattr(cfg, "multiplex_profile_allowlist", None) to profiles_to_serve(...).

So the reported regression was real for the earlier stacked snapshot, but it is no longer present in the current Task 14 diff. I am preserving that allowlist owner rather than re-extracting it here.

[andrexibiza]

Implemented every substantive point from issuecomment-5303400105 on exact head 71c40caaf47ea3281ef2c11d2621658ab3497dc5, directly on current main f43eabee5f36e11448086ee8ee17c499958e81bf.

1. Legacy inline secret migration is now explicit

secret is still rejected as unsupported plaintext, but it is no longer reported as a generic typo. Startup parsing emits an actionable error telling the operator to move the value into the environment or active profile secret scope and configure its name through secret_ref (preferred) or secret_env. The secret value is never included in logs. A regression asserts both the migration guidance and non-disclosure.

2. Configured-but-unusable secrets cannot silently become unsigned

The parser now distinguishes two states instead of overloading "":

• no secret_ref/secret_env field → intentional unsigned target;
• an explicit reference exists but is empty, malformed, duplicated, unscoped, or unresolved → target disabled.

The module documentation says this directly, and focused tests cover missing values, whitespace, non-string values, both reference fields, unresolved references, and the intentionally unsigned case.

3. Broad exception swallowing and multiplex isolation are fixed

_resolve_secret now uses agent.secret_scope.get_secret as the sole resolution authority and catches only UnscopedSecretError, the expected fail-closed multiplex boundary. It never converts that error into an os.environ lookup. Unexpected resolver/import/state failures propagate instead of being mislabeled as a missing secret. Tests prove that a process-global value cannot leak through an unscoped multiplex read and that an unrelated resolver failure is not swallowed.

4. The receiver-side V2 replay contract is actionable

The module now specifies the complete verification sequence before receiver side effects:

1. parse X-Hermes-Timestamp as Unix seconds;
2. reject outside a bounded freshness window (300 seconds recommended, including clock skew);
3. compute HMAC-SHA256 over the exact timestamp header bytes + b"." + untouched body;
4. compare the full sha256=... value using hmac.compare_digest; and
5. accept each X-Hermes-Delivery at most once for at least that window.

It states explicitly that a valid HMAC without freshness and delivery-ID dedupe remains replayable. The V2 test also keeps the legacy X-Hermes-Signature-256 compatibility assertion.

The earlier profile-allowlist finding at issuecomment-5286750893 remains correctly classified as an earlier-stacked-head regression: the current three-file Task 14 diff does not touch profile admission.

The branch was semantically compressed from six files to exactly three, dropping stale campaign receipts and unrelated contributor-email metadata. Focused exact-source verification is green (13 passed; Python compile PASS). Hosted CI/Nix/Docker are attached to this exact head and still running; no earlier green is inherited.

[andrexibiza]

@Enough1122, thank you for the four concrete review points. They are all incorporated in the current implementation, with the exact code/test mapping in issuecomment-5358721447.

Exact-head Docker and Nix are now green. The exact-head CI workflow is still queued, so that remains the only unexecuted landing receipt.

[andrexibiza]

Verified the current compressed head 71c40caaf47ea3281ef2c11d2621658ab3497dc5 rather than the older stacked snapshots referenced by the reviews.

The substantive resolver findings are closed on this object:

• explicit references fail closed and never silently become unsigned delivery;
• exactly one of secret_ref / compatibility secret_env is accepted;
• inline plaintext secret is rejected with an actionable migration message that does not log the value;
• agent.secret_scope.get_secret is the only credential authority; UnscopedSecretError disables the target and there is no raw os.environ fallback after an authoritative scoped miss, so multiplex profile isolation is preserved;
• unexpected resolver/import/state failures are not swallowed by a blanket except Exception;
• the module now states the receiver-side V2 replay contract: timestamp freshness + constant-time HMAC verification + delivery-ID deduplication are required; timestamp binding alone is not replay prevention.

The old profile-allowlist extraction finding does not exist in this one-commit/three-file Task 14 diff and should not be carried forward as current review residue.

Hosted exact-head Docker 32391368059 and Nix 32391368015 are green. CI 32391368673 is still queued; I am not inheriting green from the prior head.

[andrexibiza]

Fresh exact-head settlement for d136e8562c372dc9cbf1dbacd94d79b2079431d3, restacked directly on current main 533886c8b8eb67ff8b389b7f48e7d5e5d9c575b9:

• CI 32446033866 — success
• Docker 32446033398 — success
• Nix 32446033397 — success

The previous exact-head red was solely test_progress_advances_while_the_orchestrator_blocks. Current main now contains the transient /progress socket-stall repair, and the refreshed one-commit/three-file outbound-webhook object passes the complete current matrix. No status is inherited from 71c40ca... or the old base.

@teknium1 — maintainer review requested. The owned contract remains narrow: versioned outbound envelopes, one profile-scoped secret-reference authority, fail-closed unresolved references, signed V2 timestamp+body input, redirect refusal, and bounded fire-and-forget delivery. This is Task 14 authority, not final webhook integration.

[andrexibiza]

@teknium1 maintainer review requested on the new exact current-main head d136e8562c372dc9cbf1dbacd94d79b2079431d3.

This is no longer a CI-repair item. Fresh exact-head acceptance is complete:

• CI 32446033866 — success
• Docker 32446033398 — success
• Nix 32446033397 — success

The branch is one commit / three files directly on 533886c8b8eb67ff8b389b7f48e7d5e5d9c575b9, and GitHub reports it mergeable. The historical Windows progress failure is not inherited; the current object passes on its own receipts. Scope remains Task 14 outbound webhook envelope/secret-reference authority only.

[andrexibiza]

Exact-head follow-up at d136e8562c372dc9cbf1dbacd94d79b2079431d3: the bounded Task 14 outbound-webhook lane has no remaining blocker.

The current one-commit/three-file restack preserves the previously reviewed contract:

• explicit opaque credential references fail closed on scoped miss and never downgrade to unsigned delivery or raw environment fallback;
• inline plaintext secrets are rejected without echoing the value;
• the envelope is versioned and V2 binds the timestamp to the raw body;
• receiver documentation correctly requires freshness, constant-time HMAC verification, and delivery-ID deduplication rather than misrepresenting timestamp binding as replay prevention;
• redirects remain refused and delivery remains bounded through the existing worker queue.

This review is scoped to #84834 Task 14. It does not claim inbound intake, verifier authority, session policy, callback transport, or final integration.

Fresh exact-head hosted evidence is green: CI 32446033866, Docker 32446033398, and Nix 32446033397. The PR is open, non-draft, and mergeable. No source, exact-head CI, review, or mergeability blocker remains on this bounded object.