Skip to content

fix(rest/python): assign checkout id server side instead of trusting the request - #167

Open
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix-checkout-id-omit
Open

fix(rest/python): assign checkout id server side instead of trusting the request#167
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix-checkout-id-omit

Conversation

@vishkaty

@vishkaty vishkaty commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Observed

POST /checkout-sessions returns HTTP 500 when the request carries a non-string top-level id. id is annotated ucp_request: omit (server-assigned), so a request that includes it is schema-valid and a conformant platform may send anything there — but the reference crashes on a non-string value.

Full reproducing request (a spec-valid create; the id is the only variable):

POST /checkout-sessions
UCP-Agent: profile="https://spck.dev/agent"
idempotency-key: <uuid>   request-id: <uuid>   Content-Type: application/json

{"id":123,"currency":"USD","line_items":[{"id":"li_1","quantity":1,
 "item":{"id":"bouquet_roses","price":1000},"totals":[]}],
 "payment":{"instruments":[],"handlers":[]},"status":"incomplete",
 "ucp":{"version":"2026-04-08"},"totals":[],"links":[]}
→ HTTP 500

Controls: id omitted → 201, id:"x" (string) → 201.

Root cause: the generated CheckoutCreateRequest declares no id field (omit at 2026-04-08) but extra="allow" admits a client id of any type; create_checkout read it back with getattr(checkout_req, "id", None) and passed it into Checkout(id=…), whose id is a required string, raising an uncaught pydantic ValidationError.

This is the direct sibling of #156 (which fixed the same class for currency by determining it server-side instead of reading it from the request).

Fix

services/checkout_service.py — assign the id server-side unconditionally instead of trusting the request:

-    checkout_id = getattr(checkout_req, "id", None) or str(uuid.uuid4())
+    checkout_id = str(uuid.uuid4())

The update path has no analogue (it takes the id from the URL, never a body id). Class check: every other server-managed omit field (status, totals, links, ucp, expires_at, and currency from #156) already returns 201 on a wrong-typed value; id was the last one reading the request unsanitized.

Verification

  • New test test_create_assigns_id_server_side (integration_test.py, alongside fix(rest/python): determine currency server side instead of reading it from the create request #156's omit tests) fails on the old code with the exact ValidationError, passes after; kill-tested.
  • Full uv run pytest: 135 passed. Happy-path client end to end: exit 0. Pinned pre-commit (ruff + ruff-format) clean.
  • Live: the request above returns 201 with a server-assigned string id after the change.
  • Why CI missed it: every existing test builds payloads with a string id, so a non-string id was never exercised.

Note

A few existing lifecycle tests were reusing the client-chosen id in follow-up URLs; they now use the server-returned id (what the happy-path client already did). There is an adjacent class of unrelated extras (e.g. continue_url, order, messages, platform) that still 500 by colliding with response-only fields — out of scope here; happy to send a follow-up that builds the checkout from declared request fields only.

…the request

A schema-valid checkout create carrying a non-string top-level `id`
returns HTTP 500. checkout.json annotates `id` with `ucp_request: omit`
(the business assigns it), so the generated CheckoutCreateRequest
declares no id field; extra="allow" then admits a client-sent `id` of
any JSON type as an extra member, keeping the request schema-valid.
create_checkout read that extra (`getattr(checkout_req, "id", None)`)
and passed it verbatim into the Checkout response model, where a
non-string raised an uncaught pydantic ValidationError:

    id
      Input should be a valid string [type=string_type, input_value=123]

Observed vs expected, reproduced against main:

    POST /checkout-sessions
    UCP-Agent: profile="https://spck.dev/agent"
    idempotency-key: <key>
    request-id: <id>
    Content-Type: application/json

    {"id":123,"currency":"USD","line_items":[{"id":"li_1","quantity":1,
     "item":{"id":"bouquet_roses","price":1000},"totals":[]}],
     "payment":{"instruments":[],"handlers":[]},"status":"incomplete",
     "ucp":{"version":"2026-04-08"},"totals":[],"links":[]}

    -> 500 Internal Server Error (expected: 201)

Controls: the same request with `id` omitted, or with a string id,
returns 201.

This is the same defect class as the currency read fixed in Universal-Commerce-Protocol#156: the
server determines an omit field and never takes it from the request.
The create path now always assigns its own uuid and ignores any
client-sent id. The update path never read the body id (the id comes
from the URL path), so it has no analogue of this defect.

Accounting for the other omit-annotated checkout fields at 2026-04-08
(status, totals, links, ucp, expires_at, currency): each is either
excluded from the request dump before the response model is constructed
or already determined server side (currency since Universal-Commerce-Protocol#156), and wrong-typed
values for each were verified to return 201 on unmodified main.

Why the existing suite missed it: every lifecycle test built its payload
through _create_checkout_payload, which always sets a string id, and
then addressed follow-up calls with that same id, so a non-string id was
never sent and the id echo was baked into the tests as a contract. Those
tests now use the server-returned id, matching what the happy-path
client already does, and a new test covers non-string, string, and
omitted id on create.
@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Aug 5, 2026
@carolinerg1 carolinerg1 added devops status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Aug 6, 2026
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.

3 participants