feat(gateway): operator push dispatch path with connect-time SSRF guard

[marcusrbrown]

Adds the Web Push dispatch path to the gateway operator push feature — the payload builder, trigger/dedupe policy, and the web-push-backed sender that actually delivers notifications. This is the first slice that carries a runtime dependency; it stays inert until push config is present and no event source calls it yet (the approval/failed-run/logout hooks come next).

What this adds

• web-push@3.6.7 (+ @types/web-push devDep) — the only module allowed to import it is push-sender.ts. It handles RFC 8291 payload encryption and RFC 8292 VAPID signing internally. Supply chain checked before adding: latest version, 0 advisories, 17 transitive packages (small crypto/util libs), minimist on the patched line, MPL-2.0.
• push-sender.ts — wraps sendNotification behind a PushSender seam returning a closed PushRelayResult union (accepted / dead-subscription / retryable / payload-too-large / error). Callers never see a raw WebPushError, an endpoint, a response body, or headers.
• payload-builder.ts, trigger-policy.ts, dedupe-cache.ts — pure. Payloads carry only fixed copy keys + neutral {type, route} and, when known, an allowlisted failure label; never repo name, prompt, command, run output, raw failure kind, or any secret.
• dispatcher.ts — lists an operator's active records, re-reads ownership immediately before each send (linearizable against endpoint transfer), sends, and marks dead subscriptions inactive. Fire-and-forget and fail-soft: a relay failure never blocks approvals, run execution, Discord, or SSE.

Connect-time SSRF guard

The parse-time endpoint validator (already on main) explicitly can't defend against DNS rebinding. This closes that gap: the sender builds a per-send https.Agent whose DNS lookup validates the resolved IP and connects to that exact address with no re-resolution — a hostname that resolves to loopback/private/link-local/cloud-metadata is refused before the socket opens. Both guards share one IP-classification module (ip-classification.ts, extracted from the validator) so they can't drift. The agent path is used, never proxy (web-push ignores the agent when a proxy is set), pinned by a test.

Deferred (noted, not gaps)

• No event-source wiring yet — the dispatcher is standalone and disabled until hooked up.
• DNS-layer timeout, per-loop total timeout, and the O(n) store scan are acceptable at v1 scale (fire-and-forget, OS resolver bounds lookups, small operator count); revisit from production signal.
• Cross-restart dedupe is out of scope — the in-memory window is a best-effort guard against in-process retry storms.

Verification

Gateway type-check, lint, and full suite green (3140 tests). The SSRF guard, the relay-response classification, the transfer-race skip, and the no-leak invariants all carry dedicated tests; bun install --frozen-lockfile is clean.

[fro-bot]

Verdict: CONDITIONAL

CONDITIONAL = can merge after addressing the listed blocking issues.

The design is careful and the security posture (closed result unions, no-leak invariants, fail-soft dispatch, dual-layer SSRF, linearizable ownership re-check) is well thought through and well tested. However, the headline connect-time SSRF guard is broken on the real Node connect path, and its tests only pass because they mock away that path. That must be fixed before merge.

Blocking issues

1. createGuardedLookup breaks under autoSelectFamily (default-on in Node 20+; this repo runs Node 24) — the guarded happy path never connects. (packages/gateway/src/web/operator-push/push-sender.ts:68-84)

Node's net.connect/tls.connect enable Happy Eyeballs (autoSelectFamily) by default since Node 19/20. Under it, Node invokes a custom lookup with options.all === true and expects the callback to return an array of {address, family} entries. The guarded lookup hardcodes all: false and always calls back a single address string, so Node rejects with Invalid IP address: undefined and every real send resolves to outcome: 'error'.

Reproduced against Node 24 with the exact lookup shape from this file:

lookup called with options: {"hints":32,"all":true}
resolved single: 172.66.147.243 4
ERROR: Invalid IP address: undefined

A fix that honors all: true (resolve with {all: true}, classify every returned address, block if any is internal, pass the array through) connects successfully. Note the security nuance: when all is true you must classify each candidate, not just the first — otherwise Happy Eyeballs could still connect to an internal address you didn't check.

This defeats the primary purpose of the PR: the connect-time DNS-rebinding guard. Please fix the lookup to handle the all: true branch (or disable autoSelectFamily on the agent and pin that with a test — though handling all is the more robust choice).

2. The push-sender tests do not exercise the real Node connect path, which is why this bug is invisible. (packages/gateway/src/web/operator-push/push-sender.test.ts:180-225, 244-259)

Both guard tests mock node:https with a FakeAgent whose createConnection calls options.lookup?.('push.example.com', {family: 0}, cb) — i.e. all is never set. The guard is therefore only ever tested against a shape the real Node runtime does not use. Add a test that drives the lookup with {all: true} and asserts (a) an array callback for allowed addresses and (b) a block when any resolved address is internal. This is what would have caught issue #1.

Non-blocking concerns

• dispatchToRecord re-verifies ownership, then markDead (dispatcher.ts:147) is keyed only by {operatorId, endpoint}. A transfer landing between verify and a 410 could, in principle, target a record now owned by someone else; the store's markDead internally checks operator ownership (subscription-store.ts:955), so this is safe today — but it depends on that internal check. Worth a one-line comment noting the dependency.
• createGuardedLookup discards options.hints when re-calling dns.lookup (only family is forwarded). Harmless for correctness here, but if autoSelectFamily handling is added, preserve hints to keep parity with Node's intended resolution.
• The family string-form branch ('IPv4'/'IPv6') is handled but not covered by a test; low risk since Node passes numeric families in practice.

Missing tests

• Real-connect-path lookup test with options.all === true (see blocking issue #2) — both the allowed-array case and the internal-address block case.
• A test asserting that when all: true returns a mix of one public and one internal address, the connection is blocked (guards against classifying only the first entry).
• isBlockedResolvedAddress for a resolved family-6 hex-mapped internal IPv4 (e.g. ::ffff:7f00:1); the dotted-mapped-public case is covered but the internal hex-mapped resolved form is not.

Risk assessment (MED)

• Regression likelihood: MED-HIGH for the feature itself — the guarded send path is currently non-functional on the deployment runtime, though the PR is inert (no event source calls the dispatcher yet), so nothing in production regresses today. The blast radius is contained to the new, unwired operator-push module.
• Security exposure: LOW in the current wired state (dispatcher disabled), but the connect-time guard — the security control this PR exists to add — does not actually run successfully as written; shipping it wired-up without fixing #1 would ship a guard that fails-closed by erroring on all sends (safe-by-accident) rather than correctly allowing legitimate public relays while blocking internal ones. Fix before any event-source wiring lands.
• No public API breaking changes. Supply-chain addition (web-push@3.6.7) is reasonably vetted per the PR description and import-restricted to push-sender.ts.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	29122228854
Cache	hit
Session	ses_0b23b30f1ffelN7wflmAPsU4y1

[fro-bot]

Verdict: PASS

Re-review of c8d21db ("handle autoSelectFamily all:true in push SSRF lookup guard"). Both blocking issues from my prior review are resolved, the three missing-test items are added, and the non-blocking concerns are also addressed. I re-verified the fix against the real Node 24 connect path and ran the full gateway suite (3144 passing).

Blocking issues

None. Both prior blockers are fixed:

1. autoSelectFamily / all:true lookup — FIXED (push-sender.ts:80-117). The guarded lookup now normalizes number-form options, resolves internally with all: true, classifies every returned candidate (for (const entry of addresses)), and reshapes the callback to the caller's requested form (array when all:true, single {address, family} otherwise). I reproduced the original failure on Node 24 and confirmed the new shape connects:

lookup opts: {"hints":32,"all":true}
CONNECTED OK — fix verified

The security nuance I flagged (classify all candidates, not just the first) is honored — a single internal address anywhere in the Happy-Eyeballs set aborts the connection before any socket opens.

2. Tests now exercise the real connect shape — FIXED (push-sender.test.ts:283-343, 347-405, 407-...). New tests drive the lookup with {all: true} and assert (a) an array callback for all-public candidates, (b) the whole connection is blocked when the array mixes a public and an internal address, and (c) a single internal address via the array shape is blocked. The dns.lookup all:true invocation itself is asserted. These would have caught the original bug.

Non-blocking concerns

• Prior concern about createGuardedLookup discarding options.hints is resolved — {...normalizedOptions, ...} now preserves hints and other fields while overriding only family/all.
• The markDead ownership-dependency note (dispatcher.ts) stands as a minor documentation nit only; behavior is safe because the store re-checks operator ownership. Not blocking.
• toNumericFamily cleanly handles the string-form ('IPv4'/'IPv6') family branch; still only indirectly exercised, but low risk.

Missing tests

None outstanding. The three items I called out are now present:

• all:true allowed-array path and internal-address block (push-sender.test.ts).
• Mixed public+internal array → blocked.
• isBlockedResolvedAddress('::ffff:7f00:1', 6) hex-mapped internal IPv4 → blocked (ip-classification.test.ts:108).

Risk assessment (LOW)

• Regression likelihood: LOW. The guarded send path now functions correctly on the deployment runtime (Node 24), verified end-to-end against real net.connect. Module remains unwired (no event source calls the dispatcher), so blast radius stays contained to the new operator-push module. Full gateway suite green (3144 tests).
• Security exposure: LOW. The connect-time SSRF guard now does what it was built to do: allow legitimate public relays while blocking loopback/private/link-local/metadata resolutions across every Happy-Eyeballs candidate. Dual-layer (parse-time + connect-time) posture intact via the shared classification module.
• No public API breaking changes. Supply-chain addition (web-push@3.6.7) remains import-restricted to push-sender.ts.

Approving.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	29123704496
Cache	hit
Session	ses_0b23b30f1ffelN7wflmAPsU4y1