[codex] Clarify notification config semantics#4982
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Right shape for a pre-GA clarification: schema, MDX, and changeset all agree on the four load-bearing invariants — whole-set atomicity, tuple-scoped proof reuse, paused-config write-time SSRF, and dry_run MUST NOT touch the network.
Things I checked
- Changeset level.
minoris correct.notification_configs[]first landed in3.1.0-beta.x(post-#4730), nostatic/schemas/v3/...materialization exists, so generalizing the activation challenge from "wholesale feed subscribers" to "any new or changed active subscriber" is in-beta tightening, not a v3 break. - XOR on
webhook-challenge-response.json.anyOf+not: {required:[challenge,token]}correctly expresses exactly-one.additionalProperties: falsecloses the surface.webhook-challenge-response.json:23-40. - Conditional on
delivery_auth.mode.if/then/allOfrequirescredential_fingerprintiff mode isBearer/HMAC-SHA256, forbids it forrfc9421.additionalProperties: falsecloses the object; enum gates future modes.webhook-challenge.json:56-100. - No new undiscriminated
oneOf. Challenge usesif/then; response usesanyOf/not. Audit walker untouched. - Schema-vs-docs coherence. Atomicity (
sync_accounts.mdx:193), tuple scope (sync_accounts.mdx:207), paused write-time checks (notification-config.json:230), anddry_runno-network rule (sync_accounts.mdx:239) match between schema descriptions and MDX prose. - Echo on response.
sync-accounts-response.json:148-155correctly$refsnotification-config.jsonand notes credentials redacted. index.jsonregistration. Both new schemas wired undercoreatindex.json:417-424.
Follow-ups (non-blocking — file as issues)
- Receiver-side match list omits
account_idandsubscriber_id.sync_accounts.mdx:207andsecurity.mdx:149require the receiver to reject unlessseller_agent_url,delivery_auth, andevent_typesmatch the pending registration. A receiver registered at multiple sellers, or facing a malicious upstream, can be cross-account-confused into echoing a challenge whoseaccount_id/subscriber_iddon't match its expectation. Both fields are required inwebhook-challenge.json:412-420; add them to the receiver's MUST-match list. - Tuple binding is in prose, not on the wire. The response echoes only
challenge(ortoken). Sellers keying their pending-challenge store by the random value alone are vulnerable to in-flight replay across tuples at the same seller. Either tighten the seller-side language to "MUST look up the pending challenge by the full tuple, not by challenge value" or require the response to echoaccount_id+subscriber_id. - Paused-then-activated language. "Reactivation MUST repeat full SSRF validation … for any tuple without current valid proof" (
notification-config.json:230) is correct but reads ambiguously for the never-active path: anactive: falseregistration that flips toactive: truefor the first time has no prior proof, and a buggy reader might interpret "current valid proof" as not required. Tighten to "any tuple that has never received a successful proof, or whose last proof has expired, MUST be re-challenged before becomingactive: true." - Pin
/schemas/latest/URLs at next release. The JSX comment atsync_accounts.mdx:209-210flags this. Better as a tracked issue than an inline TODO that ages with the file. - Duplicated normative prose. The
notification_configsdescription insync-accounts-request.json:63is now ~3.5KB of normative text duplicated in the MDX. Currently coherent, but two copies of the same MUSTs is a drift hazard.
Minor nits (non-blocking)
delivery_auth.modecasing.webhook-challenge.json:43-47lists"Bearer"and"HMAC-SHA256"capitalized. Confirm this matches the existingauth-scheme.json/notification-config.authentication.schemeliterals so receivers can string-compare without a transform.tokenalias parser differential.webhook-challenge-response.json:23-40accepts either field. The schema correctly forbids both, but lenient hand-rolled validators that short-circuit onanyOfcould accept both with different values. The aliases are unreleased per the same inline comment — easiest fix is to droptokenbefore anyone deploys against it; otherwise require byte-equality when both are present.
The webhook.challenge type const is a notable departure from the standard task-envelope discriminator pattern, but acceptable here — this is an out-of-band proof payload, not a task surface.
LGTM. Follow-ups noted above.
There was a problem hiding this comment.
LGTM. The proof-of-control shape is right: tuple-scoped to (account_id, subscriber_id, normalized url, auth mode/credential binding, normalized event_types), challenge POST signed with the seller's RFC 9421 key regardless of delivery_auth.mode, write-time SSRF enforced even for active: false. Closes the public-endpoint hijack class that SSRF validation alone doesn't cover.
Things I checked
- Schema-vs-docs coherence across all eight touched surfaces — the same normative claims (omit/empty/non-empty replace, subscriber_id keying, failure atomicity, RFC 9421 signing of the challenge POST, paused-config carve-out, no
account.*lifecycle events) land identically insync_accounts.mdx,list_accounts.mdx,security.mdx,notification-config.json,account.json,sync-accounts-request.json,sync-accounts-response.json, andnotification-type.json. delivery_auth.modecasing (Bearer,HMAC-SHA256) matchesstatic/schemas/source/enums/auth-scheme.json— no wire-level enum drift. The newrfc9421value is intentionally not inauth-scheme.json(absence-of-block has historically encoded 9421); the challenge surface lifts that to an explicit scalar so the receiver can verify intent.webhook-challenge-response.jsonXOR encoding is correct draft-07:anyOf(challenge|token)+not.required:[challenge,token]+additionalProperties: false. JSON-Schema-validating receivers get the XOR for free.- Pattern regexes: challenge
^[A-Za-z0-9_.:-]{32,255}$gives ~192 bits at the floor — fine for single-use.^[a-f0-9]{64}$is strict lowercase SHA-256 hex. index.jsonregistration follows the same shape as thewholesale-feed-webhooksibling.- Anchors:
#endpoint-proof-of-controlresolves against### Endpoint proof of controlinsync_accounts.mdx;security.mdxcross-link points at the right slug. audit-oneof— no new undiscriminated oneOf.delivery_authusesallOf+if/thenkeyed onmode. Response XOR usesanyOf+not.- Changeset categorization: new optional schemas, no existing-field renames or required-flips, prose-only tightening elsewhere →
minoris right. [codex]PR title prefix and4977-…changeset filename match the repo conventions visible in recent commits.
Follow-ups (non-blocking — file as issues)
-
delivery_auth.credential_fingerprintis a salt-free SHA-256 of a counterparty-known secret.static/schemas/source/core/webhook-challenge.json:50-54. For credentials at thenotification-config.json:credentials.minLength: 32floor (or drawn from any recognizable generator format), the hash is offline-brute-forceable. It's also a stable cross-seller deanonymizer for buyers that reuse one Bearer token across multiple sellers — a hostile or breached seller log can correlate "subscriber on seller A using credentialH" with "subscriber on seller B using credentialH" without ever seeing the secret. The seller already knows the binding from the signed registration; the receiver does not need a derivative of its own secret echoed back. Cleanest fix: drop the field and replace with an opaque seller-generatedbinding_id(random nonce stable for the proof's lifetime). Fallback: keyed HMAC with a per-receiver salt (HMAC(brand_domain, credential)), so the same credential reused across sellers produces different fingerprints. Worth resolving before the first conforming seller emits a legacy-mode challenge — hard to remove once it's in counterparty logs in the wild. -
delivery_auth.modeis not inrequired.static/schemas/source/core/webhook-challenge.json:37-100.{"delivery_auth": {}}passes structural validation because bothif/thenclauses guard onrequired: ["mode"]— withmodemissing, neither fires and thecredential_fingerprintenforcement is silently bypassable. Add"required": ["mode"]to thedelivery_authsubschema. -
tokenalias deserves a sunset note or a citation.static/schemas/source/core/webhook-challenge-response.json:15-21andsync_accounts.mdx:229. Two field names for the same wire value is cruft every implementation has to accept forever. If there is real shipped seller traffic usingtoken(Slackurl_verificationprecedent is the likely lineage), cite it in the changeset. Otherwise marktokenwithx-deprecated: trueand a 4.0 removal note, mirroring the legacy-auth deprecation cadence innotification-config.json:31. -
Lift the response XOR into normative prose.
sync_accounts.mdx:229currently says "Sellers MUST also accept the backward-compatible alias" without saying "MUST reject when both are present." Hand-rolled parsers will skip the XOR. Add: "Sellers MUST reject responses that include bothchallengeandtoken, and MUST treat a missing-or-empty echo field as proof failure." -
Re-challenge cadence is unbounded.
sync_accounts.mdx:207says "Sellers MAY re-challenge on their own proof-expiration policy" with no floor. Worth a normative SHOULD-NOT-more-often-than-N-hours (24h is a reasonable default) plus a 429-is-transient clause for receiver-side rate-limiting. -
Receiver-verify field list under-specified.
sync_accounts.mdx:207says the receiver MUST reject unlessseller_agent_url,delivery_auth, andevent_typesmatch — but the proof-scope tuple also includesaccount_idandsubscriber_id, both of which the schema requires on the challenge. Expand the list to all five, or split the section into explicit "Seller obligations" and "Receiver obligations" subsections so SDK authors implementing the receiver side don't ship a bare echo. -
State that the challenge body is covered by the RFC 9421 signature.
sync_accounts.mdx:207says the POST is signed but doesn't restate that the body is incontent-digest/@signature-paramscoverage per the AdCP webhook profile. One sentence closes a theoretical body-rewriting attack on a non-conforming verifier.
Minor nits (non-blocking)
-
RFC 2119 casing drift on the paused-config carve-out.
sync_accounts.mdx:201usesMAY skip;notification-config.json:56,sync-accounts-request.jsonactivation-proof block, andsecurity.mdx:102usemay skip. Same permission, different casing across four surfaces — pick one. -
SSRF write-time checklist phrasing.
sync_accounts.mdx:201says "HTTPS parsing, hostname normalization, and reserved-range rejection on write";security.mdx:102and the schemas say "URL parsing, HTTPS, hostname normalization, and reserved-range rejection at write time". Same intent, different checklist — standardize on the four-item form. -
dry_runcarve-out is page-only.sync_accounts.mdx:237documentsdry_run: true MUST NOT send network challenges; no schema description carries it. Agents readingsync-accounts-request.jsonalone won't pick this up. One-line addition to the request-schema description covers it.
Approving on the strength of the proof-of-control design plus the cross-surface coherence. The credential_fingerprint design is the one item worth resolving before the first seller ships against this; the schema required: ["mode"] is a clean one-line fix.
fed40b9 to
f5193c3
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. The schema/docs prose is internally consistent, the new webhook-challenge.json and webhook-challenge-response.json slot cleanly under core/, and the changeset is wired up.
Things I checked
- New
webhook-challenge.jsonandwebhook-challenge-response.jsonagree onchallengefield bounds and pattern (^[A-Za-z0-9_.:-]{32,255}$). Example tokenexample-challenge-token-000000000000is 36 chars and matches. - The
if/thenondelivery_auth.modeatstatic/schemas/source/core/webhook-challenge.json:56-97correctly requirescredential_fingerprintforBearer/HMAC-SHA256and forbids it forrfc9421—not: { required: ["credential_fingerprint"] }is draft-07 valid for "property must be absent." - XOR shape in
webhook-challenge-response.json(anyOf+not.required) does not tripscripts/audit-oneof.mjs— the walker atscripts/audit-oneof.mjs:255only descends intooneOf.anyOfis unwatched. Pragmatic. static/schemas/source/enums/notification-type.jsonenum array is byte-identical; only the top-leveldescriptionchanged.- Schema prose in
sync-accounts-request.json,account.json, andnotification-config.jsonis mutually consistent withdocs/accounts/tasks/sync_accounts.mdxon the proof tuple(account_id, subscriber_id, normalized url, authentication mode/credential binding, normalized event_types), on idempotency keyed bysubscriber_id, and on atomicity (no partial replacement on validation failure). index.jsonregisters both new schemas.- Changeset present at
.changeset/4977-notification-configs-semantics.mdwithminor. - Challenge POST itself is signed with RFC 9421 even when delivery selects legacy auth (
webhook-challenge.json:5) — right call for downgrade resistance: a legacy-credential attacker can't mint a challenge.credential_fingerprintas SHA-256 of the legacy credential is a one-way commit; leaks nothing on the wire absent low-entropy credentials.
Follow-ups (non-blocking — file as issues)
- Semver call on the proof-of-control tightening is worth a maintainer sanity check. The old
notification-config.jsontext required proof for wholesale feed subscribers; the new text requires it for any new or changed active subscriber (static/schemas/source/core/notification-config.json:18). A seller that shipped v3.x without provingcreative.status_changed-only subscribers is now non-conformant. The changeset frames this as a clarification ("Standardize endpoint proof-of-control"), which is defensible — but a stricter read calls it a normative tightening that warrantsmajor. Worth a thread on the WG list before the next release cuts. - Spell out the canonicalization of
credential_fingerprint.webhook-challenge.json:52says "SHA-256 hex fingerprint of the exact legacy credential string" — "exact" is doing a lot of work. Add whether bytes are taken raw, whether whitespace/case is preserved, encoding (UTF-8 assumed). Otherwise a buyer and seller can disagree on the hash and the challenge will fail-closed in a way that's hard to debug. - Add a line to the changeset explicitly calling out the scope expansion. "Previously scoped to wholesale feed subscribers; now applies to any new or changed active account-level webhook subscriber." Helps adopters diffing against current behavior catch it.
- Consider sunsetting the
tokenalias.webhook-challenge-response.jsonaccepts bothchallengeandtokenfor backward compatibility — useful at introduction, but the longer it stays the longer every seller carries two-branch verification code. A deprecation note on thetokenfield with a target version would be the right shape.
Minor nits (non-blocking)
anyOf+not.requiredfor the XOR works butoneOfof tworequiredclauses would be more idiomatic.static/schemas/source/core/webhook-challenge-response.json:21-39. The current shape was likely chosen to keepaudit-oneof.mjsquiet — if so, leave a one-line schema comment saying so, otherwise the next contributor will "fix" it back tooneOfand trip the gate.
Approving on the strength of the schema-vs-docs coherence and the cleanly scoped surface. The semver question is the only thing I'd want a second pair of eyes on, and it's not a block — the wire shapes are unchanged and the new schemas are additive.
There was a problem hiding this comment.
Request changes. One schema gap on a new security-sensitive surface — delivery_auth.mode isn't enforced as required, so the conditional if/then discriminator on webhook-challenge.json is not total and the documented receiver-side invariant ("reject unless delivery_auth matches pending registration") cannot be checked against an empty object. One-line fix.
Things I checked
webhook-challenge.json:37-100—delivery_auth.allOf[if/then]branches both gate onrequired: ["mode"]. A payload withdelivery_auth: {}matches neither branch and validates as-is.modeneeds to be required at thedelivery_authlevel.webhook-challenge-response.json:23-40— "exactly one ofchallenge/token" viaanyOf+not.required[both]is sound and doesn't tripscripts/audit-oneof.mjs(only walksoneOf). No baseline ratchet needed.- Cross-file
notification_configs[]semantics acrossnotification-config.json,sync-accounts-request.json,sync-accounts-response.json,core/account.json, andsync_accounts.mdx— consistent on replacement semantics,subscriber_idkeying, omit-vs-[], write-only credentials, and proof-of-control activation. - Changeset exists (
.changeset/4977-notification-configs-semantics.md), markedminor. New schemas are additive; the rest tightens descriptions. Borderline — see follow-up. - Example challenge token
"example-challenge-token-000000000000"(36 chars) validates against^[A-Za-z0-9_.:-]{32,255}$. - The "RFC 9421-signed challenge even when delivery_auth selects legacy mode" rule is consistent with existing AdCP downgrade-resistance posture.
Must fix
delivery_auth.modeis not enforced as required.static/schemas/source/core/webhook-challenge.json:37-100. The twoif/thenbranches each includerequired: ["mode"]inside theif, so whenmodeis absent neither branch fires. With no top-levelrequired: ["mode"]ondelivery_auth, an empty object validates and the discriminator is unverifiable. Add"required": ["mode"]ondelivery_auth. One line.
Follow-ups (non-blocking — file as issues)
credential_fingerprintexposure to an unproven URL.webhook-challenge.json:50-54. The SHA-256 of the buyer's legacy credential ships in the challenge body to a URL that has not yet proved control. Squatter / typo'd receivers capture the fingerprint before registration ever completes. Ifnotification_config.authentication.credentialsis a user-chosen 32-char string (the current floor inpush-notification-config.json), the fingerprint is offline-crackable. Either drop the fingerprint from the challenge body and surface it after proof completes, or raise the entropy floor onauthentication.credentialsin the same surface.- Response should echo
account_idandsubscriber_id.webhook-challenge-response.jsononly echoes the challenge value. A multi-tenant receiver with no pending-registration discipline can blindly echo and the seller can't tell. Adding tuple identifiers (withadditionalProperties: false) forces receivers to bind their decision to a specific pending registration. TodayadditionalProperties: falseactively prevents an honest receiver from including them as defense-in-depth. - Proof tuple should pin the resolved IP, not just the hostname. Reactivation does fresh SSRF, but active records continue honoring cached proof against a hostname that may have re-resolved into a reserved range. Bind the proof tuple to the connect-pinned IP at proof time, or require periodic re-validation for active subscribers.
- Changeset type is borderline. Pre-PR
security.mdxqualified proof-of-control as "high-volume event families, including wholesale feed webhooks"; this PR broadens to all account-level subscribers and elevates the unsigned-challenge case to an RFC 9421 MUST. If the WG reading is "the qualifier was illustrative,"minoris right. If any seller shipped to the narrow reading, this ismajor. Worth a one-line confirmation from the WG before merge. - Challenge tuple documented as including URL but payload omits it. Receiver disambiguation relies on RFC 9421's
@target-uriderived component. A redundanturlin the body lets receivers detect host-rewriting intermediaries without that dependency. - Challenge regex permits chars outside base64url.
webhook-challenge.json:18andwebhook-challenge-response.json:13,20use^[A-Za-z0-9_.:-]{32,255}$while the prose recommends base64url. Tighten to^[A-Za-z0-9_-]{32,255}$or drop the base64url claim. Entropy is fine either way (~190 bits floor). - Retry-with-same-challenge needs a receiver-side rule. Seller-side single-use is preserved, but receivers seeing a duplicate POST can't tell whether it's a legitimate retry, an in-flight duplicate, or a replay. Spec the receiver as idempotent within a short window (e.g., 60s) and require the seller's one-retry budget to sit inside that window.
- Cross-protocol consistency note on
push_notification_config. Add a sentence topush-notification-config.jsonstating the proof-of-control flow does NOT apply to per-task transport; authenticity there comes from RFC 9421 and the echoedtoken. Avoids implementers extrapolating challenge-on-every-callback and creating a fresh SSRF amplification vector on every async task. dry_runhonesty.dry_run: truecorrectly forbids network challenges, but the response should distinguish "would succeed because tuple already has cached proof" from "would require a proof step on the real call" so callers don't read dry_run success as endpoint validity.
Minor nits
- Response-side prose is request-shaped.
static/schemas/source/account/sync-accounts-response.json:150says "re-sending an existingsubscriber_idreplaces that subscriber's config" — readers don't re-send. Rephrase as "Entries are uniquely keyed by account-scopedsubscriber_id." - Duplicate length bounds.
webhook-challenge.json:13-19andwebhook-challenge-response.json:11-13,18-20setminLength/maxLengthredundantly with the{32,255}quantifier in the pattern. Harmless.
Ship after delivery_auth.required: ["mode"] lands and the changeset categorization is confirmed with the WG.
f5193c3 to
b6ee210
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean clarification PR — declarative-replace semantics, proof-of-control tuple, and the no-account.*-lifecycle disclaimer all land coherently across six schema files and three docs files.
Things I checked
- Proof tuple
(account_id, subscriber_id, normalized url, authentication mode/credential binding, normalized event_types)is byte-identical acrossstatic/schemas/source/account/sync-accounts-request.json:63,docs/accounts/tasks/sync_accounts.mdx:207, and the required-field set onstatic/schemas/source/core/webhook-challenge.json:413-421. No drift. delivery_auth.allOfif/then onwebhook-challenge.json:358-400correctly forbidscredential_fingerprintforrfc9421and requires it for Bearer/HMAC-SHA256.additionalProperties: falseplus thenot.requiredbranch is the right shape.- The
event_typesrejection field-path is consistent:notification-config.json,sync-accounts-request.json, andsync_accounts.mdx:86,125all pointerror.fieldataccounts[i].notification_configs[j].event_typesfor bad types andnotification_configs[j].urlfor proof failures. enums/notification-type.jsonchange is description-only — no enum values added or removed, noenumDescriptionstouched. Backward-compatible.- Changeset present at
.changeset/4977-notification-configs-semantics.md, typeminor. New schemas are additive; no field renames, no required flips on existing fields; no breaking wire change. index.json:413-424wires the two new schemas into thecoresection.webhook-challenge-response.jsonanyOf[required challenge | required token] + not[required challenge AND token]is semantically exactly-one-of under draft-07. Correct, even if it's a different idiom from the rest of the repo.
Follow-ups (non-blocking — file as issues)
- Exactly-one-of idiom drift.
static/schemas/source/core/webhook-challenge-response.json:260-277uses top-levelanyOf+notfor exactly-one-of, while the rest of the repo (e.g.,account/sync-accounts-response.json:177-220) usesoneOfwith reciprocalnot.requiredinside each branch and is baselined inscripts/oneof-discriminators.baseline.json. The chosen form sidestepsaudit-oneof.mjssilently. Either restructure as a two-branchoneOfand ratchet the baseline, or add a one-line comment in the file justifying the deviation so a future reader doesn't think the walker missed it. - Anti-replay surface on
webhook-challenge.json. No explicitnonce/issued_at/expires_at. Anti-replay leans entirely on RFC 9421'screated/expiressignature parameters plus single-use scoping in prose (sync_accounts.mdxline nearcryptographically random, single-use). Worth confirming that's the intentional pivot vs the Standard-Webhookswebhook-idnonce convention, and saying so in the schema description so implementers don't reinvent anoncefield. - Normative broadening worth a migration note. Prior text on
notification-config.jsonurldescription scoped the MUST to wholesale-feed subscribers; the new text broadens to "any new or changed active subscriber." Theor equivalent proof-of-controlescape hatch survives so this is not a wire break, but a previously-conformant wholesale-feed-only implementer now has more surface to cover. A one-line migration note in the changeset would help. - Bound the
urlelement of the proof tuple explicitly.webhook-challenge.json:5says the POST is sent "after URL normalization and SSRF validation," and the tuple includesnormalized url, but the body carries nourlfield — the URL is bound only via the POST destination. An interpretive reader could reasonably ask whether they should add aurlfield. Recommend adding a sentence: "Receivers MUST treat the request URL itself as the boundurlelement of the tuple."
Minor nits (non-blocking)
- Prose alternation between "account-anchored events" and "account-anchored resource events" across
notification-config.json:5,security.mdx, andsync_accounts.mdx. Harmless, but worth picking one phrase in a follow-up sweep.
Approved on the strength of the proof-tuple coherence and the schema/docs alignment.
Summary
sync_accounts.accounts[].notification_configs[]replacement/idempotency semantics, including omission, empty arrays, subscriber_id keying, response echoing, and failure atomicity.account.*lifecycle events; account status remainslist_accountspolling or one-shotsync_accounts.push_notification_config.Validation
npm run build:schemasnpm run test:schemasnpm run test:json-schemanpm run test:examplesnpm run lint:schema-links -- --checknpm run test:schema-linksnpm run test:oneof-discriminatorsgit diff --checktest:unit,test:test-dynamic-imports,test:callapi-state-change,typecheck