feat(gateway): operator push contract and VAPID config foundation#1152
Conversation
fa82337 to
9a351dc
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Clean, well-scoped first slice: contract DTOs + VAPID config, disabled by default, no routes/dispatch/dependency yet. The security posture (no-oracle validators, non-enumerable private key, fail-closed enablement, version-pin regression test) is consistent with existing gateway conventions and is backed by thorough tests. All 2979 gateway tests pass locally.
Blocking issues
None
Non-blocking concerns
-
assertValidVapidSubjectaccepts an addresslessmailto:(packages/gateway/src/web/operator-push/vapid.ts:100).new URL('mailto:')andnew URL('mailto: ')both parse with protocolmailto:and an empty path, so a subject of"mailto:"passes validation. This is operator-controlled config (not attacker input) andweb-pushwould reject it downstream, but a non-empty-target check would fail closed earlier with a clearer error. Low priority. -
base64url validation is length/prefix-based, not canonical (
vapid.ts:56,:81). The guard checks the alphabet, decoded byte length, and (for public keys) the0x04prefix, but does not enforce canonical base64url round-trip the wayGATEWAY_OPERATOR_CSRF_SECRETdoes inconfig.ts:889. A non-canonical encoding that still decodes to the right byte count would pass here. Since the decoded bytes are the source of truth for signing, this is acceptable, but worth a note for when the signing path lands. -
Rotation config allows an identical current/previous keypair with only a differing
keyVersion(config.ts:997). The only cross-key check iskeyVersioninequality; the same public/private material under two versions is accepted. Harmless for a rollout window, but a public-key equality guard would catch an obvious copy-paste misconfiguration.
Missing tests
None that block. Coverage is strong (disabled/enabled modes, stray-material-ignored, partial-previous rejection, dedupe-window boundaries incl. 0/1e5/100abc, key-version equality, private-key non-serialization and non-echo in errors, contract-version pin, and no-oracle parse guards). Two small gaps worth adding when convenient: an explicit assertion that mailto:-without-target is rejected (would document the intended contract), and a parse-guard test for a keys object missing one of p256dh/auth while the other is present.
Risk assessment (LOW/MED/HIGH): LOW
Regression likelihood: LOW — additive surface, disabled by default, no route/dispatch wiring, contract version intentionally unchanged with a pinning test guarding a reflexive bump. Security exposure: LOW — private key held non-enumerable and validated by no-oracle validators; serialization/error-leak paths are explicitly tested. Blast radius: LOW — no existing operator stream or endpoint behavior changes; the dashboard's exact-match SSE ready-frame gate never observes this REST surface.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 29034335727 |
| Cache | hit |
| Session | ses_0b9dfe579ffeakLcTic8Sj8yWM |
Strip internal unit/requirement identifiers (leftover planning-doc references) from gateway config comments and test titles; the comments now describe behavior directly.
Add the Gateway-side operator Web Push contract DTOs (VAPID public key, subscription metadata, subscribe/unsubscribe requests) and the VAPID configuration surface, disabled by default. No dispatch, routes, or web-push dependency yet. The VAPID private key is validated by fixed-reason validators and held as a non-enumerable config property so it never lands in a serialized config object or a thrown error. The operator contract version is intentionally unchanged: push adds an additive REST surface that the dashboard SSE drift gate does not observe, so the exact-match contract version stays put.
9a351dc to
7f80ef0
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review after 7f80ef0. Both non-blocking concerns from the prior (dismissed) review have been addressed, each with a matching test. Still a clean, well-scoped foundation slice — contract DTOs + VAPID config, disabled by default, no routes/dispatch/dependency. All 2983 gateway tests pass locally (up from 2979).
Verified fixes:
- Addressless
mailto:now rejected (vapid.ts:113-115): amailto:subject with an empty path throwsmailto: URL must include a non-empty address. Covered byvapid.test.ts:144-151for bothmailto:andmailto:. - Identical rotation keypair now rejected (
config.ts:1001-1003): a previous keypair sharing the current public key throwsprevious key must differ from the current key, complementing the existing key-version inequality check. Covered byconfig.test.ts:2897.
Blocking issues
None
Non-blocking concerns
- base64url validation is length/prefix-based, not canonical (
vapid.ts:56,:81). The guard checks the alphabet, decoded byte length, and (for public keys) the0x04prefix, but does not enforce canonical base64url round-trip the wayGATEWAY_OPERATOR_CSRF_SECRETdoes inconfig.ts:889. Since the decoded bytes are the source of truth for signing, this is acceptable at this layer; worth revisiting when the signing path lands. (Carried over from the prior review, intentionally deferred.)
Missing tests
None. Coverage is strong and now includes the two fixes: addressless-mailto: rejection (both empty and whitespace paths), current/previous public-key equality rejection, plus the prior suite (disabled/enabled modes, stray-material-ignored, partial-previous rejection, dedupe-window boundaries, key-version equality, private-key non-serialization and non-echo in errors, contract-version pin, and no-oracle parse guards).
Risk assessment (LOW/MED/HIGH): LOW
Regression likelihood: LOW — additive surface, disabled by default, no route/dispatch wiring, contract version intentionally unchanged with a pinning test guarding a reflexive bump. Security exposure: LOW — private key held non-enumerable and validated by no-oracle validators; serialization/error-leak paths are explicitly tested. Blast radius: LOW — no existing operator stream or endpoint behavior changes; the dashboard's exact-match SSE ready-frame gate never observes this REST surface.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 29037135064 |
| Cache | hit |
| Session | ses_0b9dfe579ffeakLcTic8Sj8yWM |
Adds the Gateway-side foundation for operator Web Push notifications — the server counterpart to the dashboard push work (fro-bot/dashboard#108). This first slice defines the contract and configuration surface only; it ships disabled by default with no dispatch, no routes, and no
web-pushdependency yet.What's here
PushSubscription.toJSON()shape, each with a hand-rolled parse guard.GATEWAY_OPERATOR_PUSH_*): disabled mode ignores VAPID material entirely; enabled mode requires a complete, valid keypair + subject + key version and fails closed on any missing or malformed field. Supports an optional previous keypair for a rollout window.Design notes
/operator/push/*REST surface that the dashboard's exact-match SSE ready-frame drift gate never observes, so bumping the version constant would be unnecessary and would break every operator stream for a dashboard not redeployed in lockstep. A regression test pins the constant so a reflexive bump fails CI.Scope
This is the first unit of a phased rollout: the durable subscription store, authenticated routes, and the fail-soft dispatch path (which gates on adding the
web-pushdependency) follow in subsequent PRs. Plan:docs/plans/2026-07-08-002-feat-operator-push-gateway-plan.md.