Skip to content

fix(rest/nodejs): drop a profile webhook_url that is not an http(s) URL instead of storing it - #264

Merged
gswetha merged 2 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/nodejs-profile-webhook-url
Nov 6, 2026
Merged

gswetha merged 2 commits into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/nodejs-profile-webhook-url

Conversation

@vishkaty

@vishkaty vishkaty commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

The nodejs reference stores whatever value a platform profile publishes under the order
capability config webhook_url, echoes it under platform.webhook_url on every response for
that 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 the
Python 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, with src/index.ts:177 reading
the 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 the
value shown, then get, update and complete with the same header.

profile webhook_url create echoed under platform.webhook_url at completion
http://127.0.0.1:<port>/hook (control) 201 yes, on create, get, update and complete one signed order_placed delivery, complete answers in 5 ms
ftp://platform.invalid/hook 201 yes, and persisted Failed to notify webhook at ftp://platform.invalid/hook TypeError: fetch failed logged, complete answers in 309 ms, nothing delivered
42 201 yes, as the integer Failed to notify webhook at 42 TypeError: Invalid URL logged, nothing delivered
{"url": "..."} 201 yes, as the object Failed to notify webhook at [object Object] TypeError: Invalid URL logged, nothing delivered
data:text/plain,ok 201 yes nothing logged and nothing delivered: fetch resolves a data: URL with ok true, so the event is treated as delivered
mailto:ops@platform.invalid 201 yes as the ftp row, fetch failed, 305 ms
xts-not-a-url-2030 201 yes as the integer row, Invalid URL
["http://127.0.0.1:<port>/hook-array"] 201 yes, as the array one signed order_placed delivery to /hook-array and nothing logged: new URL and fetch coerce a one element array to its string
http://user:pass@127.0.0.1:<port>/hook 201 yes Failed to notify webhook at http://user:pass@127.0.0.1:<port>/hook TypeError: Request cannot be constructed from a URL that includes credentials logged, complete answers in 304 ms, nothing delivered
http://user:pass@example.com/hook 201 yes as the row above, 305 ms

The completion itself answers 200 with status: completed in every row, so the order is placed
either 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: notifyWebhook returns on response.ok
(:240), so an order_placed for that platform is treated as delivered while nothing left the
process. 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 URL accepts userinfo and fetch refuses to
construct 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 (:149 to :150);
the TypeScript annotation says string but nothing checks it at runtime. createCheckout
spreads the request body (:745) and then sets platform: platformConfig (:755), so a
client cannot inject the member through the body (probed: a create with platform in the body
and no UCP-Agent answers 201 with the member absent); the profile is the only source, which
is why this is a profile validation question and not a request validation one. notifyWebhook
(:168) guards on truthiness only (:173) and passes the value to signRequest (:220),
which calls new URL(url) (src/utils/signature.ts:487), then to fetch (:231), inside a
loop of three attempts with 100 and 200 ms of backoff (:208, :258). For the ftp, mailto
and credentials rows the log line comes from the inner catch (:249 to :251) on the third
attempt, since new URL accepts all three and fetch rejects them; for the integer, the
object and the bare string it comes from the outer catch (:266), because new URL throws
inside signRequest before any attempt; both print the same text, and the completion carries
on either way. Neither function changed between e7c9c124, the base of the previous head of
this branch, and fe191a3f: #258 changed completeCheckout and 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 maps
to profile_malformed, 422 (overview.md, Error Codes). ucp.json types a capability
config as an open object whose structure each entity schema defines, and order.json defines
that structure for this capability as a required webhook_url of type string, format uri;
order.md (Webhook URL Configuration) says the platform provides its webhook URL in that
config and the business discovers it from the profile. The same text stands at the commit the
v2026-04-08 tag named in September (a25a4a24), at the one it names today (9c9d6fce) and
on main (58c7ab92, where the page is docs/specification/shopping/order/index.md). The spec
provides 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.ts only. parseAgentProfile now accepts only a string
that new URL() parses with an http: or https: protocol and no username or password, and
drops 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 assertProfileUrlAllowed applies to a
profile URL (src/utils/signature.ts:605). The platform member is then absent, so no delivery
is 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
platform when the parse returns a config (:852 to :855). This matches the lenient drop
the Python reference adopted in #257 (rest/python/server/routes/ucp_implementation.py:120
to :132, HttpUrl): on the wire against the same main fe191a3f, the first seven malformed
profiles 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_schema at
9c9d6fce with a Draft 2020-12 validator and an rfc3986 format checker; the array fails
type: string); format: uri admits the ftp:, mailto:, data: and credentials values,
which fail only as delivery targets. A 422 profile_malformed at create would therefore fit
four 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_placed event with an Authorization: Basic header built from the userinfo, because
pydantic HttpUrl accepts the value and httpx sends the userinfo as basic auth (pydantic
2.14.2 and httpx 0.28.1, what the server declarations pydantic>=2.12.0 and httpx>=0.26.0
resolve 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_url through
parseAgentProfile: webhook.test.ts and webhook_signing.test.ts set platform on the
checkout object directly, and the profiles signature.test.ts and signing.test.ts serve
declare no capability at all, an empty or version only ucp beside the keys.

Tests. test/agent_profile.test.ts wires the checkout routes the way lifecycle.test.ts
does and sends the profile as a data: URI, so the eleven cases run with no network: the
valid 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 the
change 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 URL throw anyway, and one that removes the credentials check one, the
credentials case. One variant survives: returning target.href instead of the value, which is
the 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 build and npm test pass on node
20.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 pinned
pre-commit run --all-files passes all twelve hooks. Node 20 needs one step on this machine
that 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 the
install script of the package (node-gyp rebuild --release) run with node-gyp 11.0.0 instead
of 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 CXXFLAGS pointing
at the SDK libc++ headers, then npm rebuild esbuild fsevents for the other packages with an
install script. new URL and fetch treat 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 profile logged with the value, no delivery attempted
and 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 no
row 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 URL accepts it and its protocol
is http:, while fetch rejects it at request construction, so the earlier head stored and
echoed it and spent the three attempts at completion exactly as main does (the two credentials
rows above, measured on 31bdd085 as well). The check now matches assertProfileUrlAllowed.
This head is rebased from e7c9c124 onto fe191a3f; the change to checkout.ts is otherwise
the one that was reviewed. #265, now rebased onto the same main, changes the two create and
update handlers and transactions.ts in the same file set; a three way merge of the two in
either order gives one tree, and the combined suite passes, 191 of 191.

@damaz91 damaz91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@damaz91 damaz91 added status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Oct 27, 2026
…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.
@vishkaty
vishkaty force-pushed the fix/nodejs-profile-webhook-url branch from 31bdd08 to d670357 Compare November 3, 2026 04:17
@vishkaty

vishkaty commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. The observation is taken: a URL carrying userinfo passed the gate on 31bdd085, because new URL accepts it and its protocol is http:, while fetch rejects it when the request is constructed. Measured on the wire against main fe191a3f and against 31bdd085 with a profile publishing http://user:pass@example.com/hook: create 201 with the value stored and echoed on create, get, update and complete, and the completion spending the three attempts and about 305 ms before logging TypeError: Request cannot be constructed from a URL that includes credentials, with nothing delivered (node 22.14.0 on the wire; new URL accepts the value and fetch rejects it the same way on 20.18.0, 23.1.0 and 24.22.1). The pushed head drops the value with the same log line as the other shapes, with the check written the way assertProfileUrlAllowed writes it (target.username || target.password), and adds the case to test/agent_profile.test.ts: eleven cases now, ten red with the gate excised, and a variant that removes only the credentials check leaves exactly that case red. The branch is also rebased from e7c9c124 onto fe191a3f; the change to checkout.ts is otherwise the one you reviewed. 186 pass on node 20, 22 and 24 against 175 on main; the description is updated with the two credentials rows, the rebase and the figures. One note for the record: the Python reference keeps the opposite posture on this one shape, measured on the same main with a loopback receiver: pydantic HttpUrl accepts a URL carrying credentials, the value is stored and echoed on create, get, update and complete, and httpx delivers the signed order_placed event to the receiver with an Authorization: Basic header built from the userinfo, so each reference gates on what its own client can deliver to and I have not proposed a Python change.

@gswetha
gswetha merged commit 52319df into Universal-Commerce-Protocol:main Nov 6, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants