chore: release package (rc) - #2419
Conversation
2dbaecd to
33dae48
Compare
|
Verified and ready to merge — blocked on one approving review. Everything in this PR checks out; I reviewed it line by line before touching it:
Why it's stuck
That matters because Ladon is what produces the approving review this repo's What's neededOne approving review, then it can merge. I deliberately did not work around this: approving on a maintainer's behalf, admin-merging, or relaxing the ruleset would each subvert the gate rather than satisfy it. Everything upstream is already merged and green — #2431, #2433, #2421. Worth fixing separatelyThe release gate depends on an AI review action that fails on PRs containing no reviewable code (this diff is version bumps, a CHANGELOG, and a lockfile). Every release is one Ladon failure away from silently stalling, with nothing surfacing the cause. Either exempt |
|
Thanks for the thorough write-up, @bokelley. Understood — PR is clean, Ladon's The Ladon exemption for Generated by Claude Code |
d7232af to
f356ce8
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — automated Changesets release PR (rc bump 13.0.0-rc.4 → 13.0.0-rc.5).
Checked:
- All four version sites (package.json, package-lock.json, client-shim/package.json, src/lib/version.ts) are consistent; adcp stays 3.1.8.
- CHANGELOG.md regenerated and pre-mode changeset names appended to .changeset/pre.json — normal Changesets machinery, not a hand-edited version line.
- No src/lib/** behavior, no protocol/client transport changes; witness-not-translator and official-transport rules not implicated.
- Only observation is generated lockfile churn (dropped peer: true markers) with no version/integrity changes — Low, non-blocking.
No critical/high/medium findings. gated_paths is false and no author team gate applies, so review_decision: REVIEW_REQUIRED does not trigger row 2. Falls through to row 9.
This PR was opened by the Changesets release GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.
mainis currently in pre mode so this branch has prereleases rather than normal releases. If you want to exit prereleases, runchangeset pre exitonmain.Releases
@adcp/sdk@13.0.0-rc.5
Major Changes
607e2a6: Security hardening across the SSRF, request-signature, webhook, and OAuth-callback boundaries. Several items change behavior — read the notes before upgrading.
createPinAndBindFetchno longer follows redirects. Both SSRF guards only ever saw the URL the caller passed: the synchronous scheme/CIDR pre-check runs before the request, and undici skipsconnect.lookupfor IP-literal hosts. ALocation:hop therefore reached its destination unevaluated, so a buyer-registered webhook host could answer302 Location: https://169.254.169.254/…and take the signed POST to the cloud metadata service. A 3xx now surfaces to the caller as an ordinary non-2xx response, and the mode is forced rather than defaulted — passingredirect: 'follow'will not re-enable following. Callers that need to follow a hop should re-enter the fetch with the new URL so the full policy runs on it.targeting-helpersalready did this at its call site; webhook delivery did not.createExpressVerifierno longer verifies against an empty body surrogate.resolveRawBodydemandedreq.rawBodyonly whencontent-length > 0.Transfer-Encoding: chunkedrequests carry noContent-Length, so they fell through and the entire pipeline evaluated a body of''—hasBodycame from the same value, waiving thecontent-typecoverage requirement, and acontent-digestcomparison hashed the empty string — after whichnext()handed a downstream parser's real body to the handler. The test is now for positive proof that no body exists, rather than for evidence that one does. Enumerating body signals cannot work: HTTP/2 forbidsTransfer-Encodingoutright and makescontent-lengthoptional, so a DATA-frame body carries neither header and slipped through a signal-based check.req.rawBodyis therefore required on any body-bearing method (POST/PUT/PATCH/DELETE), as well as whenever aContent-Length,Transfer-Encoding, or already-populatedreq.bodyindicates a body — failing closed withrequest_signature_header_malformedand an error that names the exact middleware to add. ABufferrawBodyis now accepted too, which is what the canonicalexpress.json({ verify })recipe actually produces; only accepting a string 401'd correctly-wired apps.validateAgentUrlshares one SSRF policy with discovery probes. It previously enforced only whenNODE_ENV === 'production', so an unset or orchestrator-strippedNODE_ENVdisabled the check entirely. It now delegates toclassifyProbeUrl, which is deliberately not keyed onNODE_ENV— a staging image runningNODE_ENV=testmust not inherit a looser SSRF posture than production. The range policy: loopback allowed (abusing it already requires on-host access, and every local dev loop and mock-server test targets it), RFC-1918/link-local/ULA refused unless the operator setsADCP_ALLOW_INTERNAL_PROBES=1(the CLI's existing--allow-httpswitch), cloud metadata refused even then. Hand-rolled literal comparisons are gone in favour of thenet/address-guardsCIDR classifiers, which fixes bracketed[::1](Node returns the bracketed form, so the old'::1'compare never matched),127.0.0.2, CGNAT, and IPv4-mapped IPv6, and stops treating registered names like10.example.comas private.Two gaps closed in the shared policy while wiring this up: cloud-metadata hostnames (
metadata.google.internaland friends) are now refused — they are registered names, so no CIDR classifier ever saw them, and GCP metadata-by-name needs no IP literal — and a trailing root dot (localhost.,metadata.google.internal.) no longer bypasses name matching.net/address-guardsalso gains192.0.0.0/24(Oracle Cloud IMDS lives at192.0.0.192, outside the169.254.0.0/16everyone else uses),192.88.99.0/24, and240.0.0.0/4, whichWEBHOOK_SSRF_POLICYalready denied — the two tables had diverged.This function does not resolve DNS and never did — it is a literal-host gate, not a rebinding defense. DNS-level protection lives in
ssrfSafeFetch/createPinAndBindFetch, and the MCP/A2A transports do not yet route through them.Webhook registrations no longer advertise a placeholder credential.
push_notification_config.authenticationandreporting_webhook.authenticationwere emitted with the constantplaceholder_secret_min_32_characters_requiredwhenever nowebhookSecretwas configured. Perpush-notification-config.jsonthat block is a scheme selector, not a fallback: its presence opts the seller into legacy HMAC-SHA256 and its absence selects the RFC 9421 webhook profile. The placeholder therefore downgraded every secretless webhook to legacy HMAC keyed by a value that shipped in the source.The two sites differ because their schemas do:
push_notification_config.authenticationis optional, so the block is simply omitted when no real secret backs it, which selects RFC 9421.reporting-webhook.jsonmakesauthenticationrequired for all of AdCP 3.x (the requirement lifts in 4.0), so the block cannot be omitted and no honest value exists. The library's automaticreporting_webhookinjection is therefore skipped with a one-time warning when there is nowebhookSecretand the caller supplied noauthentication. Thecreate_media_buyitself is unaffected. SetwebhookSecret, or pass an explicitreporting_webhook.authentication, to keep automated reporting delivery.Receiving without a
webhookSecretnow fails closed.verifyAndParseWebhookreturns a newwebhook_unverifiablecode instead of accepting: with no secret the legacy HMAC profile has nothing to verify against, and this client does not yet verify the RFC 9421 profile that an omittedauthenticationblock selects, so accepting would dispatch a caller-supplied payload to async and activity handlers and update task status on nothing but its shape. SetallowUnauthenticatedWebhooks: trueto opt back in — only safe when the receiver route is unreachable from outside your network.The CLI's
--waitpath registered a hardcodedwebhookSecret: 'cli-webhook-secret', a constant published in the npm tarball; becauseauthenticationis a selector, that actively downgraded every--waitwebhook to legacy HMAC keyed by a value anyone can read out of the package. It now generates a per-invocation random secret.push_notification_configis version-gated: v2.5 makesauthenticationrequired and has no 9421 selector semantics, so omitting the block there would be schema-invalid rather than a mode selection. With no secret, a v2 seller gets no registration (and a warning) instead of a fabricated credential.reporting_webhook'sauthenticationis also treated as atomic rather than field-merged. A field-level merge crossedschemesfrom the caller withcredentialsfromwebhookSecret, so a caller passingschemes: ['Bearer']with no credential had the HMAC shared secret registered as a Bearer token — which the seller then sends in cleartext on every delivery. And a caller who passesreporting_webhookexplicitly with no credential available now gets an error rather than having the field silently dropped; only the library's own auto-injection is skipped quietly.The CLI OAuth callback is bound to the request that started it.
CLIFlowHandlergenerated a randomstatebut never compared it — the loopback callback resolved the pending authorization on the first request to reach the path, letting any local process inject or cancel a flow, with code substitution available wherever the authorization server does not strictly bind PKCE to the code. The handler now capturesstatefrom the authorization URL and requires a constant-time match on the callback, before interpreting any other callback parameter — readingerror=access_deniedfirst would have left the cancellation half of the same hole open, letting any local process abort a login with no knowledge of the state.redirectToAuthorizationthrows if the URL carries nostateor uses a non-http(s) scheme.Scope, stated plainly: this defeats a process that cannot see the authorization URL, which is the ordinary case. It does not defeat a same-user process that reads the URL out of this process's argv while the browser opens. PKCE is what protects the code exchange there.
Windows browser launch no longer goes through
cmd.exe.spawn('cmd', ['/c', 'start', '', url])makes the interpreter the child process, socmdparses&,|, and^in the URL even withshellunset — and authorization URLs contain&by construction and derive partly from a discoveredauthorization_endpoint. Now usesrundll32 url.dll,FileProtocolHandler.Two smaller fixes. The webhook verifier's loopback exemption from the https-only
@target-urirule usedhostname.startsWith('127.'), which also matched registered names like127.attacker.examplethat resolve anywhere; it now requires a real IPv4 literal in127.0.0.0/8,localhost, or IPv6 loopback.PropertyListAdapter.generateTokenbuilt a returnedauth_tokenfromMath.randomand now usescrypto.randomBytes.testAgentno longer logs credentials. It passed the wholeTestOptionsobject tologger.info, and the default loggerJSON.stringifys its context toconsole.log— so bearer tokens, Basic passwords, OAuth access/refresh tokens, client-credential secrets, test-kit API keys, and caller-supplied header values all went to stdout and CI logs on every run. The context is now built from an allowlist of loggable fields: masking known secrets was not enough, becauseTestOptions._clientholds a live client whose own fields lead straight back to the same credentials, so a denylist over that object graph could not be made safe. Shapes are kept — which auth scheme ran, which header names were in play — since that is the debuggable part.Debug logs share one redactor, and it now covers both webhook registrations.
mcp.tsmasked only the idempotency key, the sibling Tasks path also maskedpush_notification_config.authentication, and the A2A path maskedtokenas well — three behaviors across three call sites. All now use one helper.That helper masks
authenticationandtokenon bothpush_notification_configandreporting_webhook. The second matters more than it looks:reporting-webhook.jsonmakesauthenticationrequired for all of AdCP 3.x, so acreate_media_buyregistering reporting always carries a real HMAC secret inreporting_webhook.authentication.credentials— on the call most likely to be debug-logged. Masking only the push config left that in plaintext.Redirecting webhook endpoints fail fast instead of burning the retry budget.
isTerminalStatustreated 3xx as retryable, so a receiver that permanently redirects (apex→www, http→https, trailing-slash canonicalization — the most common webhook misconfiguration) consumed every attempt and reported a bareHTTP 302. A redirect is now terminal, the error names theLocationand why it was not followed, and the operator bucket isHTTP_REDIRECTrather thanUNKNOWN.Docs: the Express verifier snippets in
README.mdanddocs/guides/SIGNING-GUIDE.mdcalled arawBodyMiddleware()that does not exist anywhere in the repo, so following them produced a blanket 401 under the stricter raw-body guard. Both now show theexpress.json({ verify: adapter.rawBodyVerify })form.structuredSerialize/structuredDeserializetreat__proto__as data.JSON.parseproduces a genuine own__proto__key, so theout[key] = …rebuild reachedObject.prototype's setter and replaced the result object's prototype with caller data instead of copying a field. Assignment now goes throughObject.defineProperty, so the key round-trips losslessly as an own property.7273918: Breaking:
createTenantStore'srefAccessis now required, with no default.It previously defaulted to
'ref-routed', under whichaccounts.resolvereturns whatever tenant the buyer's ref names without consulting the authenticated principal. That is correct for an agency hub whose single credential legitimately spans tenants — and a cross-tenant spend hole for a hub whose tenants are unrelated clients, sinceresolveis the account path forcreate_media_buyandupdate_media_buy. Nothing in the code can distinguish those two deployments, so a default silently picked one.Both values remain available and neither behavior changed; only the choice is now explicit.
'auth-scoped'— a ref resolving to a tenant other thanresolveFromAuth(ctx)returnsnull(framework emitsACCOUNT_NOT_FOUND). Correct for most multi-tenant deployments.'ref-routed'— previous default. Keep it only if one credential is supposed to span tenants, and layer aresolve-presetsguard (requireAccountMatch/requireAdvertiserMatch/requireOrgScope) viacomposeMethod.Migration is one line per
createTenantStorecall. TypeScript reports it at the call site; the helper also throws at construction on a missing or unrecognized value, so JS adopters andas anycasts can't fall through to the permissive branch silently.Note that
refAccessgovernsresolveonly —upsert/syncGovernanceenforce the per-entry tenant gate either way. An adopter who had verified the sync-tool gate was never covered onresolve, which is what made the old default easy to miss.skills/build-holdco-agent/SKILL.md— the doc an adopter actually follows — never mentionedrefAccesswhile its front-matter promised "per-tenant data isolation". It now documents the choice, adds a "What the helper does NOT guarantee" section, andskills/cross-cutting.mdno longer describes the helper as an unqualified isolation gate.Minor Changes
3861d4a: Expose
PublishedPostAsset,ZipAsset,CardAsset,PixelTrackerAsset,VASTTrackerAsset, andDAASTTrackerAssetfrom the package root and@adcp/sdk/types, and include every generated registry-backed variant inAssetInstance.BREAKING NOTE: exhaustive
AssetInstanceswitches must add cases forzip,published_post,card,pixel_tracker,vast_tracker, anddaast_tracker. These are additive protocol variants that the SDK previously omitted from its public union.Patch Changes
774c5ff: Fall back to the legacy
initializehandshake when a server refuses theserver/discoverversion-negotiation probe with 401/403 and the probe alreadycarried a static credential.
mode: 'auto'documents that "definitive legacy signals (and anythingunrecognized) fall back to the plain legacy
initializehandshake", but the MCPclient's classifier treats 401/403 as the sole terminal probe outcome. A server
that rejects the modern discovery method while accepting the very same
credential on
initializeandtools/listwas therefore unreachable, and thecaller was told to supply an
auth_tokenit was already sending.The fallback is deliberately narrow. With no credential a probe 401 is the
server's own challenge and still reaches the caller, so the
WWW-Authenticate/RFC 9728 walk is unaffected; with an OAuth provider a 401still propagates as the provider's cue to refresh. Only a static
token/header credential refused on the probe alone triggers the retry, and if
that credential really is bad the legacy connect fails on its own and surfaces
the server's 401.
f8f24fb: Soft-fail legacy format_ids projection for canonical wire mode. When a package's legacy format_ids cannot be converted to canonical format_option_refs, return the package unchanged rather than throwing so get_media_buys responses for existing buys remain usable.
0852df0: Sync
schemas/registry/registry.yamlandsrc/lib/registry/types.generated.tswith the upstream AdCP registry.Generated output only — no hand edits and no behavior change. The registry types are published, so this is a type-surface addition rather than a no-op; existing type references are unaffected (additive).
Regenerate with
npm run sync-schemas:all && npm run generate-types && npm run generate-registry-types -- --sync.607e2a6: feat(oauth): add
requireBrowserBindingand warn whenexpectedStateis omitted fromcompleteWebOAuthFlowCompleteWebFlowOptions.expectedStatewas optional and silently skipped when absent — the flow was replay-protected via atomic consume but not browser-bound. Now:expectedStateemits aconsole.warnpointing callers to the session-cookie pattern.requireBrowserBinding: truepromotes the omission to aBrowserBindingRequiredError(strict mode for frameworks where session cookies are always available).BrowserBindingRequiredErrorclass is exported from@adcp/sdk/auth/oauth.Parallels the
CLIFlowHandlerstate-binding hardened in the same security PR.