Repository navigation
fix(rest/nodejs): drop a profile webhook_url that is not an http(s) URL instead of storing it - #264
Conversation
damaz91
left a comment
There was a problem hiding this comment.
Thank you @vishkaty for the thorough investigation, clear root-cause write-up (especially catching the silent data: response.ok and single-element array coercion behaviors), and clean fix!
Validating orderCap.config.webhook_url as an http:/https: URL string in parseAgentProfile and dropping malformed values with a warning brings the Node.js sample into clean parity with the Python reference server (#257), while preserving existing valid platform.webhook_url configs across checkout updates and keeping unsigned profile handling compatible with the conformance suite. The network-free route tests in test/agent_profile.test.ts are well-structured and comprehensive.
Optional, non-blocking observation for future hardening: Node's new URL() accepts URLs containing embedded userinfo credentials (e.g., http://user:pass@example.com/hook), whereas fetch() rejects them at request construction time (Request cannot be constructed from a URL that includes credentials). notifyWebhook already catches delivery errors safely, so this is completely non-blocking here, though guarding !(target.username || target.password) (similar to assertProfileUrlAllowed in src/utils/signature.ts) could filter those out up front as well.
LGTM!
…RL instead of storing it parseAgentProfile returned whatever value the platform profile carried under config.webhook_url on the order capability. The schema types the member as a string with format uri (order.json, $defs/platform_schema), but an ftp URL, an integer, an object, an array, a data: URL, a bare string or an http URL carrying credentials was stored on the checkout, echoed under platform.webhook_url on every response, and handed to notifyWebhook on completion, where new URL() throws inside signRequest for an integer or an object, fetch() rejects an ftp scheme after three attempts and 300 ms of backoff inside the complete request, and fetch() refuses to construct a request from a URL that carries credentials, after the same three attempts. A data: URL is worse: fetch resolves it with ok true, so the order_placed event is treated as delivered while nothing left the process. An array holding one URL is coerced to a string by new URL() and fetch() and delivered, so that schema violation is never seen at all. Only an http(s) URL without credentials is a deliverable target, so any other value is now dropped with a log line, the way every other profile fault is treated, and the platform member is left absent. The credentials check is the one assertProfileUrlAllowed applies to a profile URL. An update carrying a malformed value keeps the valid one already stored. The Python reference drops a malformed value the same way since Universal-Commerce-Protocol#257; it keeps a URL carrying credentials, which its httpx client delivers to with basic auth, so the two references part on that one shape. Eleven cases in test/agent_profile.test.ts drive the profile through app.request() as a data: URI: the valid control, eight malformed shapes on create and get, each asserting the drop is logged once with the value, the update twin, and a completion that must attempt no delivery. With the gate excised and the tests kept, ten go red.
31bdd08 to
d670357
Compare
|
Thanks for the review. The observation is taken: a URL carrying userinfo passed the gate on |
The nodejs reference stores whatever value a platform profile publishes under the order
capability config
webhook_url, echoes it underplatform.webhook_urlon every response forthat checkout, and hands it to the delivery path on completion. The schema types the member as
a string with
format: uri(order.json,$defs/platform_schema, v2026-04-08), and thePython reference gates the same value since #257, so the two references disagree today.
Probed on a fresh clone of main
fe191a3f(node v22.14.0, withsrc/index.ts:177readingthe listen port from the environment and nothing else changed), the platform profile served
by a local stub over http and a local receiver as the valid target. Each row is a create
carrying
UCP-Agent: profile="..."naming a profile whose order capability config carries thevalue shown, then get, update and complete with the same header.
webhook_urlplatform.webhook_urlhttp://127.0.0.1:<port>/hook(control)order_placeddelivery, complete answers in 5 msftp://platform.invalid/hookFailed to notify webhook at ftp://platform.invalid/hook TypeError: fetch failedlogged, complete answers in 309 ms, nothing delivered42Failed to notify webhook at 42 TypeError: Invalid URLlogged, nothing delivered{"url": "..."}Failed to notify webhook at [object Object] TypeError: Invalid URLlogged, nothing delivereddata:text/plain,okfetchresolves adata:URL withoktrue, so the event is treated as deliveredmailto:ops@platform.invalidfetch failed, 305 msxts-not-a-url-2030Invalid URL["http://127.0.0.1:<port>/hook-array"]order_placeddelivery to/hook-arrayand nothing logged:new URLandfetchcoerce a one element array to its stringhttp://user:pass@127.0.0.1:<port>/hookFailed to notify webhook at http://user:pass@127.0.0.1:<port>/hook TypeError: Request cannot be constructed from a URL that includes credentialslogged, complete answers in 304 ms, nothing deliveredhttp://user:pass@example.com/hookThe completion itself answers 200 with
status: completedin every row, so the order is placedeither way. What the stored value changes is what the business does on every order event
afterwards: it retries, throws or silently succeeds against a target that can never receive a
delivery. The
data:row is the one that matters:notifyWebhookreturns onresponse.ok(
:240), so anorder_placedfor that platform is treated as delivered while nothing left theprocess. The array row is silent for the opposite reason: the coerced string happens to be
deliverable, so the schema violation never surfaces anywhere. The two credentials rows are the
shape the review of this change pointed at:
new URLaccepts userinfo andfetchrefuses toconstruct the request, on node 20.18.0, 22.14.0, 23.1.0 and 24.22.1 alike, so the three
attempts and 300 ms of backoff are spent on a target that no delivery can reach.
Where it happens, on main
fe191a3f.parseAgentProfile(src/api/checkout.ts:106)returns
{ webhook_url: orderCap.config.webhook_url }for any truthy value (:149to:150);the TypeScript annotation says string but nothing checks it at runtime.
createCheckoutspreads the request body (
:745) and then setsplatform: platformConfig(:755), so aclient cannot inject the member through the body (probed: a create with
platformin the bodyand no
UCP-Agentanswers 201 with the member absent); the profile is the only source, whichis why this is a profile validation question and not a request validation one.
notifyWebhook(
:168) guards on truthiness only (:173) and passes the value tosignRequest(:220),which calls
new URL(url)(src/utils/signature.ts:487), then tofetch(:231), inside aloop of three attempts with 100 and 200 ms of backoff (
:208,:258). For the ftp, mailtoand credentials rows the log line comes from the inner
catch(:249to:251) on the thirdattempt, since
new URLaccepts all three andfetchrejects them; for the integer, theobject and the bare string it comes from the outer
catch(:266), becausenew URLthrowsinside
signRequestbefore any attempt; both print the same text, and the completion carrieson either way. Neither function changed between
e7c9c124, the base of the previous head ofthis branch, and
fe191a3f: #258 changedcompleteCheckoutand the import block only.Spec at the version this server declares (
UCP_VERSION = "2026-04-08",src/utils/config.ts:15). Businesses MUST fetch and validate the platform profile(
overview.md, Business Requirements, item 1), and a fetched profile that violates schema mapsto
profile_malformed, 422 (overview.md, Error Codes).ucp.jsontypes a capabilityconfigas an open object whose structure each entity schema defines, andorder.jsondefinesthat structure for this capability as a required
webhook_urlof type string, format uri;order.md(Webhook URL Configuration) says the platform provides its webhook URL in thatconfig and the business discovers it from the profile. The same text stands at the commit the
v2026-04-08tag named in September (a25a4a24), at the one it names today (9c9d6fce) andon main (
58c7ab92, where the page isdocs/specification/shopping/order/index.md). The specprovides the error code without saying whether a business must reject or may ignore such a
profile, and says nothing about credentials in the URL.
The change, in
src/api/checkout.tsonly.parseAgentProfilenow accepts only a stringthat
new URL()parses with anhttp:orhttps:protocol and no username or password, anddrops anything else with a log line, which is how the function already treats a profile it
cannot fetch or parse. The credentials check is the one
assertProfileUrlAllowedapplies to aprofile URL (
src/utils/signature.ts:605). The platform member is then absent, so no deliveryis attempted against a target that cannot receive one. An update whose profile carries a
malformed value keeps the valid one already stored, because the update path only replaces
platformwhen the parse returns a config (:852to:855). This matches the lenient dropthe Python reference adopted in #257 (
rest/python/server/routes/ucp_implementation.py:120to
:132,HttpUrl): on the wire against the same mainfe191a3f, the first seven malformedprofiles answer 201 there with the member absent and
Ignoring malformed webhook_url in profile from ...logged, and the control is stored and delivered. Of the shapes in the table, only the integer, the object,the bare string and the array violate the schema (checked against
$defs/platform_schemaat9c9d6fcewith a Draft 2020-12 validator and an rfc3986 format checker; the array failstype: string);format: uriadmits theftp:,mailto:,data:and credentials values,which fail only as delivery targets. A 422
profile_malformedat create would therefore fitfour of the nine and carry the wrong description for the other five, so the gate here is on
deliverability, the posture the Python reference took in #257. On the credentials rows the two
references part: measured on the same wire, the Python reference stores and echoes both
credentials values and delivers to the loopback one, the receiver seeing the signed
order_placedevent with anAuthorization: Basicheader built from the userinfo, becausepydantic
HttpUrlaccepts the value and httpx sends the userinfo as basic auth (pydantic2.14.2 and httpx 0.28.1, what the server declarations
pydantic>=2.12.0andhttpx>=0.26.0resolve to today). So the Python reference can deliver there and node cannot; each reference
gates on what its own client can deliver to, and no Python change is proposed.
Why the suite did not catch it. No test drives a profile with a
webhook_urlthroughparseAgentProfile:webhook.test.tsandwebhook_signing.test.tssetplatformon thecheckout object directly, and the profiles
signature.test.tsandsigning.test.tsservedeclare no capability at all, an empty or version only
ucpbeside the keys.Tests.
test/agent_profile.test.tswires the checkout routes the waylifecycle.test.tsdoes and sends the profile as a
data:URI, so the eleven cases run with no network: thevalid control, eight malformed shapes on create and on get, each also asserting that the drop
is logged once with the value, the update twin, and a completion that must attempt no
delivery. Against the unfixed tree ten fail: eight with the raw value stored and echoed, the
update twin with the stored valid value replaced by the ftp one, and the completion with a
delivery attempted to
ftp://platform.invalid/hook. With the change all eleven pass; with thechange excised and the tests kept, the same ten go red. Each line of the gate has a case that
needs it: a variant that keeps only the type check leaves seven red, one that drops the
protocol check five, one that stores the dropped result ten, one that rejects
https:two(the control and the update twin, which creates with the control value), one that removes the
log line eight, one that removes the type check one, the array, because every other non string
shape makes
new URLthrow anyway, and one that removes the credentials check one, thecredentials case. One variant survives: returning
target.hrefinstead of the value, which isthe same string for every URL the suite uses, so nothing in the suite can tell the two apart
and the schema admits either form.
Checks run locally on this branch.
npm ci,npm run buildandnpm testpass on node20.18.0, 22.14.0 and 24.22.1, the workflow matrix: 186 pass, 0 fail, 0 skipped on each,
against 175 on main
fe191a3f, the eleven new cases being the whole difference. The pinnedpre-commit run --all-filespasses all twelve hooks. Node 20 needs one step on this machinethat CI does not: better-sqlite3 12.11.1 ships prebuilt binaries for node ABI 127, 137, 141
and 147 only, none for ABI 115, so node 20 compiles it from source (ubuntu in CI does the
same). Here that compile needed
npm ci --ignore-scripts, then the fallback half of theinstall script of the package (
node-gyp rebuild --release) run with node-gyp 11.0.0 insteadof the 10.1.0 npm bundles, carrying a one line local patch that seeds the Xcode version cache
because no Command Line Tools receipt is registered on this machine, with
CXXFLAGSpointingat the SDK libc++ headers, then
npm rebuild esbuild fseventsfor the other packages with aninstall script.
new URLandfetchtreat the credentials rows identically on 20.18.0,22.14.0, 23.1.0 and 24.22.1. Re-probing the ten rows above against the patched server gives
201 with the member absent on create, get, update and complete for every malformed shape,
Ignoring malformed webhook_url in agent profilelogged with the value, no delivery attemptedand the completion answering in 1 to 2 ms, and the valid control still delivers once; an
update carrying the ftp profile over a checkout created with the control keeps the control,
where main replaces it. The three rows that were silent or slow for a reason other than a
thrown URL on main (
data:, the array and the two credentials rows) now log at create, and norow that logged on main goes quiet.
Review. The one observation on the earlier head of this branch,
31bdd085, is adopted:a URL carrying userinfo passed the gate there, because
new URLaccepts it and its protocolis
http:, whilefetchrejects it at request construction, so the earlier head stored andechoed it and spent the three attempts at completion exactly as main does (the two credentials
rows above, measured on
31bdd085as well). The check now matchesassertProfileUrlAllowed.This head is rebased from
e7c9c124ontofe191a3f; the change tocheckout.tsis otherwisethe one that was reviewed. #265, now rebased onto the same main, changes the two create and
update handlers and
transactions.tsin the same file set; a three way merge of the two ineither order gives one tree, and the combined suite passes, 191 of 191.