fix(security): protocol & transport hardening (#27, #30, #31, #34)

[moisesja]

Summary

Resolves four scattered Low/Info findings (#27, #30, #31, #34); #32 closed as spec-compliant (no change). Each fix passed a PoC-backed adversarial red-team pass that caught two real residuals (folded in). Full suite: 648 tests green; warnings-as-errors clean (incl. cookbook).

Note: two of these reference code from before the JOSE delegation — handled below.

Fixes

#	Change
#30	OutOfBand.ReadWebRedirect validates the peer-supplied redirectUrl: returns null unless an absolute http/https URL with no userinfo and a non-private host, and returns the canonical form. Closes javascript:/data:/file:/user@host passthrough.
#34	OutOfBand.FromPlaintext (single inbound choke point) rejects an absent or whitespace-only from (FormatException, FR-OOB-01); build side tightened to match.
#31	The dispatcher implements FR-THR-04 rule 3: records each ACK request it emits (ThreadState.AckRequested) and consumes the answering pure-ACK instead of re-dispatching it. AckLoopGuard docs corrected to what's actually enforced.
#27	Transport OutboundEndpointPolicy is now nullable (null = inherit DidCommOptions'), so the outbound SSRF policy is configured in one place. Default stays strict (BlockPrivateNetworks=true).
#32	No change — the cited parser was delegated to DataProofsDotnet.Jose (doesn't surface typ); RFC 7515 §4.1.9 makes typ advisory, FR-SIG-04 has no MUST to validate on receive, and envelope kind is structural. Spec-compliant; closing with rationale.

⚠️ Behavior change (#27)

Because the transports now inherit the core policy, setting DidCommOptions.OutboundEndpointPolicy.BlockPrivateNetworks = false (or another permissive core setting) now relaxes the transport connect-time guard as well — previously each transport kept an independent strict default. This is the intended single-source consistency the issue asks for; set an explicit per-transport policy only to diverge deliberately (the facade pre-send always uses the core policy).

Adversarial red-team pass (AGENTS.md §2)

Three break-it agents, two real residuals fixed:

• web_redirect URL surfaced to consumers without scheme validation (open-redirect / javascript: URI passthrough) #30 holds vs dangerous schemes + private-IP literals (decimal/hex/IPv4-mapped all blocked). Fixed: return the canonical AbsoluteUri (strips leading control chars) and reject userinfo (good@evil phishing-display).
• OOB invitation parse does not enforce REQUIRED 'from' (FR-OOB-01) #34 — real gap fixed: IsNullOrEmpty accepted a whitespace-only from (" "/NBSP) since Message.Validate only rejects control chars → switched to IsNullOrWhiteSpace.
• Outbound SSRF policy split across three independently-defaulted OutboundEndpointPolicy objects, so a hardened policy in one is silently ignored by the others #27 holds — no SSRF bypass; default stays strict; inherit works for both transports (even without AddDidComm); no NRE. The one behavior change is documented above.
• FR-THR-04 rule 3 not enforced: no outstanding-ACK bookkeeping despite the documented claim #31 holds — no exploitable loop (rule-2 is the real barrier; rule-3 is best-effort, bounded; over-drop only suppresses inert pure-ACKs). Keying verified.

Test plan

• web_redirect URL surfaced to consumers without scheme validation (open-redirect / javascript: URI passthrough) #30: javascript:/data:/file:/relative/private-IPv4(+decimal)/IPv6-loopback(+mapped)/userinfo → null; valid https → returned; leading-control-char → canonical.
• OOB invitation parse does not enforce REQUIRED 'from' (FR-OOB-01) #34: absent / empty / whitespace-only from → FormatException FR-OOB-01.
• FR-THR-04 rule 3 not enforced: no outstanding-ACK bookkeeping despite the documented claim #31: a pure-ACK answering a dispatcher-emitted ACK request → dropped + flag cleared; an unsolicited pure-ACK → not dropped.
• Outbound SSRF policy split across three independently-defaulted OutboundEndpointPolicy objects, so a hardened policy in one is silently ignored by the others #27: WS transport with a permissive core policy and unset transport policy → localhost not SSRF-blocked (core inherited).

Closes #27, closes #30, closes #31, closes #34

🤖 Generated with Claude Code

[moisesja]

Review — PR #41: protocol & transport hardening
Overall this is well-executed. Each fix is surgical, the adversarial red-team pass caught two real residuals (whitespace-only from, non-canonical URL return), and the test coverage is solid. The #32 disposition is properly spec-referenced. A few specific things to address before merging:

1. http://2130706433/admin — blocking mechanism is unclear (security)
   The test [InlineData("http://2130706433/admin")] passes with Should().BeNull(). The IsSafeRedirectUrl guard is:

if ((parsed.HostNameType is UriHostNameType.IPv4 or UriHostNameType.IPv6)
&& IPAddress.TryParse(parsed.Host, out var ip)
&& OutboundEndpointGuard.IsPrivateOrReserved(ip))
On .NET, System.Uri may classify a decimal-integer host (2130706433) as UriHostNameType.Dns, not IPv4. If so, the guard never enters the block — and IPAddress.TryParse("2130706433", ...) would return false anyway (dotted-decimal only). The test could be green because Uri.TryCreate outright rejects the decimal form on .NET 10, not because IsPrivateOrReserved fires.

Action needed: add a comment that pins the rejection mechanism. A future refactor to a more lenient URI parser could silently re-open this hole if nobody knows which layer is doing the blocking.

2. No HTTP transport inheritance test (#27)
   SendAsync_inherits_core_OutboundEndpointPolicy_when_transport_policy_unset covers the WS path. The HTTP path in HttpDidCommBuilderExtensions uses the same inherit-when-null pattern via sp.GetService<IOptions>()?.Value... but has no equivalent test. A minimal IServiceCollection-based integration test proving the core policy reaches the HTTP ConnectCallback's OutboundEndpointGuard would close this gap.

3. AckRequested stuck-true if the expected ACK never arrives (#31)
   ThreadState.AckRequested is cleared only when the answering pure-ACK arrives. If the peer never sends it (network loss, peer bug), the flag stays true for the thread's LRU lifetime. A later different pure-ACK on the same thread would be silently dropped as DroppedAsAckLoop. This is bounded by the LRU store (#21) and rule-2 is the real loop barrier, but the AckRequested XML doc should acknowledge it: "If the expected pure-ACK is never received, the flag remains set until eviction; a later unsolicited pure-ACK on the same thread will be incorrectly dropped."

4. Open-redirect warning missing from ReadWebRedirect XML doc (#30)
   IsSafeRedirectUrl deliberately passes https://evil.example/phishing-page — documented in the CHANGELOG and inline. But the public ReadWebRedirect method itself has no XML warning that the returned URL is still an open redirect target the consumer must validate before navigating. This contract belongs in the method XML doc so it surfaces in IntelliSense — not just in internal comments and the CHANGELOG.

Minor nits (non-blocking)
Issue number references in code comments ((#34), (#30 red-team)) will rot — prefer stable spec section references.
tasks/todo*.md files committed to VCS will accumulate. Consider a tasks/.gitignore.

[moisesja]

Addressed the review in c1353ef. Per-item resolution:

#	Item	Verdict	Resolution
1	http://2130706433/admin block "incidental" (security)	False	Verified empirically: .NET 10 System.Uri canonicalizes numeric-encoded IPv4 (decimal/hex/octal/short) to dotted 127.0.0.1 with HostNameType==IPv4 before the guard runs, so IPAddress.TryParse(parsed.Host) parses it and the private/reserved block fires. The claim conflates the raw string with the canonicalized parsed.Host. Added a clarifying comment in IsSafeRedirectUrl pinning the mechanism (defense-doc against a future parser swap).
2	No HTTP transport inherit test (#27)	True	Added HttpTransportPolicyInheritanceTests — symmetric to the WS inherit test, exercising the real SocketsHttpHandler.ConnectCallback: a positive test (permissive core policy inherited → loopback not SSRF-blocked) plus a complement (no policy anywhere → default blocking refuses loopback), so the positive test can't pass vacuously.
3	AckRequested stuck-true (#31)	True (doc)	Documented on the property's <remarks>: if the answering pure-ACK never arrives the flag persists until bounded-store (#21) eviction, dropping a later unsolicited pure-ACK on that thread — a benign over-drop (pure ACKs are inert; rule-2 is the real loop barrier).
4	Open-redirect warning missing from public XML doc (#30)	True (doc)	Added <remarks> to ReadWebRedirect — a non-null result is a candidate navigation target the consumer MUST confirm before navigating; no destination allowlist is applied.

Nits: issue-# comment refs kept (they sit alongside FR/spec IDs and aid audit traceability); tasks/.gitignore not added (the contributor workflow deliberately tracks tasks/todo*.md + lessons.md).

Out-of-scope observation (not changed here): the HTTP transport surfaces connect-time failures as raw HttpRequestException rather than wrapping them in TransportException (FR-API-07), unlike the WS transport. Pre-existing and unrelated to #27/#30/#31 — flagging for a possible follow-up issue.

Full suite 650 tests green; build clean (warnings-as-errors).