feat(bazaar): catalog integrity — soft-drop validation and routeTemplate traversal defence - #150
Conversation
…rsal defence Catalog listings arrive from clients in the payment payload, so the write path is a trust boundary. Adds submitCatalogListing as the untrusted door into the catalog, leaving registerBazaarResource as the trusted one. - listings are anchored to the payTo the payment signed; a resource identity belongs to whoever claimed it first - routeTemplate is percent-decoded to a fixed point before the traversal checks run, and the decoded form is what gets stored - field-level type, length and character limits, plus a byte ceiling on the bazaar declaration - per-payTo write rate limiting in Redis, failing closed - invalid metadata soft drops: the payment stands, the listing does not, and the reason is reported through EXTENSION-RESPONSES - control characters are stripped at read time as well as write time
|
@ezedike-evan Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
Approving. This closes #131 properly, and the part I went looking to break is the part that's done right.
The traversal defence decodes before it checks
for (let pass = 0; pass < CATALOG_LIMITS.percentDecodePasses; pass++) { … decodeURIComponent … }
…
if (segment === '.' || segment === '..') → path_traversalChecking the raw string first is the classic way to get this wrong — %2e%2e%2f contains no .., sails through, and decodes to ../ downstream. Decoding first, in a bounded multi-pass loop, also closes the double-encoded %252e%252e%252f variant that a single pass would miss. The comment explains the ordering, so the next person doesn't "optimise" the loop away.
And the tests actually exercise those variants rather than asserting the happy path — 40 of them, including:
rejects %2e%2e%2f — percent-decoded BEFORE the traversal checkrejects double-encoded traversalreports malformed encoding instead of throwingstores the decoded routeTemplate, not the submitted encoding
That last one matters: rejecting the input isn't sufficient if what you persist is still the attacker's encoding.
Also correct: rejecting :// and // (protocol-relative URLs are a real bypass), and requiring a leading /.
Two design calls I'd have argued for
Soft drop. Invalid metadata doesn't fail the payment — the payment is legitimate, only the listing is bad — and toExtensionResponses(drops) tells the seller which field and why. Silently discarding a listing after taking payment is the behaviour that generates support tickets nobody can diagnose.
The limiter fails closed. From the doc:
If Redis is unreachable the limiter fails closed — at a trust boundary, "I cannot tell whether this payer is flooding" is not a reason to accept the write.
That's the right call and the right reason. Failing open is the default mistake, and it's exactly how a rate limiter becomes decorative during the incident you needed it for.
Validation on read, not only on write is the third good instinct — rows predate this validation, and something harmless inside JSON can still be harmful to whatever renders it.
Verified on the merge result
merge into main: 0 conflicts
tsc --noEmit 0 errors
Tests 305 passed | 1 skipped (306)
My first run showed 3 failures; three re-runs were clean. That's the intermittent suite flake tracked as #151 — auth.test.ts and pairs.test.ts are its usual victims. Not yours.
Merging.
closes #131
closes #135
Picked this up unassigned — happy to be assigned it, or to close this if someone else is already on it.
Threat note
The catalog is written from the payment payload, so anyone who can pay can attempt to write to the index. This PR treats
submitCatalogListingas the untrusted door and leavesregisterBazaarResourceas the trusted one (Lens listing its own endpoints, #134).Everything hangs off one fact a client cannot forge:
authority = { payTo, network }, taken from the payment requirements the payer signed.What an attacker gets stopped by
accepts[].payTomust equal the payment's ownpayTo, and the listing's network must equal the payment's network (both thenetworkfield and the CAIP-2 id insideaccepts[]). A listing can only ever speak for the seller that paid to publish it.(network, url, httpMethod), or(network, url, toolName)for MCP) belongs to whoever claimed it first. Its owner may update it forever; anybody else is refused.routeTemplate— decoded to a fixed point before the traversal checks, so%2e%2e%2fis../at the moment it is inspected, and%252e%252e%252fdoesn't survive single-pass decoding either. Anything still changing after four passes is dropped rather than decoded further, and the decoded form is what gets stored, so the value served is the value that was validated. Also rejected: schemes,//, query strings, fragments, backslashes, control characters, bad or repeated:paramnames, and templates that don't structurally describe the path ofresource.url(otherwise a listing consolidates itself under another seller's route family).bazaar.infoandbazaar.schema, https-only URLs with no embedded credentials, atomic-unit amounts, boundedmaxTimeoutSeconds.payTorate limiting (BAZAAR_CATALOG_WRITES_PER_MIN, default 10;BAZAAR_CATALOG_WRITES_PER_DAY, default 200). The attempt is counted before the decision, so a rejected write still costs its slot.What it does not stop, stated plainly
payTo, so the listing stays attributable and revocable. Binding a listing to proof of domain control would close this and is not in this change.Soft drop
Invalid metadata never fails the payment — the payment is legitimate, only the listing is bad.
submitCatalogListingreturns{ accepted, drops }and never throws (a DB failure becomes acatalog_write_faileddrop, not an exception).toExtensionResponses(drops)builds theEXTENSION-RESPONSESbody #130 attaches to the response:{ "bazaar": { "catalog": { "accepted": false, "drops": [ { "field": "extensions.bazaar.routeTemplate", "code": "path_traversal", "message": "routeTemplate contains a path traversal segment." } ] } } }codeis stable and machine-readable so an agent branches on it instead of parsing prose;messagenever echoes the offending value back.Fail-closed rate limiting is a deliberate choice. If Redis is unreachable we cannot tell whether a payer is flooding, and at a trust boundary that is not a reason to accept the write. The cost is bounded: the payment still succeeds and the seller is told the catalog was unavailable.
Untrusted at read time too. Control characters are stripped when a row is served, not only when it is written — rows predate this validation, and a value that is harmless inside JSON can still be harmful to whatever renders it.
Files
src/bazaar/validation.ts(new) — decoding,routeTemplatevalidation, field limits, ownership checks,toExtensionResponses. Pure, no I/O.src/bazaar/rateLimit.ts(new) — per-payTominute and day windows in Redis.src/bazaar/catalog.ts—submitCatalogListing, the first-claim-wins owner check, and read-time sanitisation intoListing.registerBazaarResource's existing signature and behaviour are unchanged.docs/x402/bazaar-catalog-integrity.md,.env.example— the threat note and the two new knobs.Tests
src/__tests__/bazaarIntegrity.test.ts(40) andsrc/__tests__/bazaarRateLimit.test.ts(8), including the cases the issue asks for by name: a listing that claims another seller'spayTois refused;%2e%2e%2fis rejected as traversal (plus plain../, the double-encoded form, backslash, absolute and protocol-relative URLs,%00, and a template that doesn't match its resource path); an invalid listing drops without writing and without throwing; every field limit; and both rate-limit windows plus the fail-closed paths.npx vitest run— 293 passed, 1 skipped (37 files), full suitenpx tsc --noEmit— clean (afternpx prisma generate; the committed client is stale onmainfor unrelated models)Note on #130
This is the validation layer for automatic cataloging, which isn't merged yet, so nothing calls
submitCatalogListingin this PR — it is the function #130 should call instead ofregisterBazaarResource, andtoExtensionResponsesis the body it should attach. Wiring it into the payment path here would collide with that issue; happy to do it in this PR instead if you'd rather they land together.