From cc35675afdf9832c5ba47754dc89e49d8f1bc8ba Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sat, 25 Apr 2026 20:31:48 -0400 Subject: [PATCH 1/2] feat(registry): swap property readers to catalog UNION (PR 4a of #3177) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Property-side readers in federated-index-db.ts and property-db.ts now union the catalog-side publishers cache (PR 1 of #3177) with the legacy discovered_properties / discovered_publishers tables. Crawl-sourced properties that landed via the new writer (PR 2 / #3218) but missed the legacy table — Setupad escalation #218's gatavo.com is the canonical example — now surface on the registry endpoints alongside legacy data. Functions changed: - hasValidAdagents: returns true if either publishers (with source_type='adagents_json') or discovered_publishers has a positive signal. Three-state contract preserved (null when neither has the domain; false only when legacy says false and catalog has no row). - getPropertiesForDomain (federated-index-db): UNION over discovered_properties and properties extracted from publishers.adagents_json JSONB, deduped on (publisher_domain, name, property_type) with legacy preferred so callers holding a discovered_properties.id keep working. Catalog-only rows surface property_rid as id (also a UUID; same call sites). - getDiscoveredPropertiesByDomain (property-db): same UNION shape. - getAllPropertiesForRegistry: three-way UNION (hosted + legacy crawl + catalog-only). Hosted wins over crawl (existing precedence carve-out preserved); legacy wins over catalog within the crawl side so the agent_count subquery against agent_property_authorizations remains meaningful during the dual-write window. - getPropertyRegistryStats: mirrors the registry's three-way UNION so a domain with both legacy and catalog rows is counted exactly once. Authorization-side readers (getPropertiesForAgent, findAgentsForPropertyIdentifier, getPublisherDomainsForAgent) stay on legacy tables — those need the authorization model decision still deferred from PR 1 and ship in PR 4b. PR 3 baseline coverage (registry-reader-baseline-* test files) all pass unchanged: 27 properties + 12 public-endpoints + 24 authorizations + 3 mcp = 66 cases. Sibling registry tests (registry-overlay-schema, registry-search, registry-feed, registry-pipeline-client, registry-crawler-cache) also pass — 66 more cases, no regressions. Refs #3177. Builds on #3195 (schema), #3218 (writer cutover), #3221 (reader baseline). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../registry-property-readers-catalog.md | 23 +++ server/src/db/federated-index-db.ts | 85 +++++++-- server/src/db/property-db.ts | 174 +++++++++++++----- 3 files changed, 226 insertions(+), 56 deletions(-) create mode 100644 .changeset/registry-property-readers-catalog.md diff --git a/.changeset/registry-property-readers-catalog.md b/.changeset/registry-property-readers-catalog.md new file mode 100644 index 0000000000..bc9ec36c42 --- /dev/null +++ b/.changeset/registry-property-readers-catalog.md @@ -0,0 +1,23 @@ +--- +--- + +Property-side readers in `federated-index-db.ts` and `property-db.ts` now +union the catalog-side `publishers` cache (PR 1 of #3177) with the legacy +`discovered_properties` and `discovered_publishers` tables. Crawl-sourced +properties that landed via the new writer path but missed the legacy +table — gatavo.com surfaced via Setupad escalation #218 — now appear on +the registry surfaces alongside legacy data. + +Affects `hasValidAdagents`, `getPropertiesForDomain`, +`getDiscoveredPropertiesByDomain`, `getAllPropertiesForRegistry`, and +`getPropertyRegistryStats`. Authorization-side readers +(`getPropertiesForAgent`, `findAgentsForPropertyIdentifier`, +`getPublisherDomainsForAgent`) stay on legacy tables — those need the +authorization model decision and ship in PR 4b. + +Legacy wins on collisions so callers that hold a +`discovered_properties.id` keep dereferencing it correctly during the +dual-write window. After PR 5 drops the legacy tables the legacy half +of the union goes away. + +Refs #3177. Builds on #3195 / #3218 / #3221. diff --git a/server/src/db/federated-index-db.ts b/server/src/db/federated-index-db.ts index 9e9b150ce0..a7a27e052c 100644 --- a/server/src/db/federated-index-db.ts +++ b/server/src/db/federated-index-db.ts @@ -155,19 +155,33 @@ export class FederatedIndexDatabase { } /** - * Check whether a publisher domain has a valid adagents.json (from crawl data). - * Returns true if any record for this domain has has_valid_adagents = true, - * null if the domain has never been discovered. + * Check whether a publisher domain has a valid adagents.json. + * + * Reads from both the catalog-side `publishers` overlay (PR 1 of #3177) + * and the legacy `discovered_publishers` table during the dual-write + * window. A presence in `publishers` with `source_type='adagents_json'` + * means the crawler successfully validated and cached the file — that + * always wins. Otherwise fall back to bool_or over discovered_publishers + * so the historical three-state contract (true / false / null) is + * preserved when only the legacy path has data. */ async hasValidAdagents(domain: string): Promise { - const result = await query<{ has_valid: boolean }>( - `SELECT bool_or(has_valid_adagents) as has_valid - FROM discovered_publishers - WHERE domain = $1`, + const result = await query<{ catalog_present: boolean; legacy_or: boolean | null }>( + `SELECT + EXISTS( + SELECT 1 FROM publishers + WHERE domain = $1 AND source_type = 'adagents_json' + ) AS catalog_present, + (SELECT bool_or(has_valid_adagents) + FROM discovered_publishers + WHERE domain = $1) AS legacy_or`, [domain] ); - if (!result.rows[0] || result.rows[0].has_valid === null) return null; - return result.rows[0].has_valid; + const row = result.rows[0]; + if (!row) return null; + if (row.catalog_present) return true; + if (row.legacy_or === null) return null; + return row.legacy_or; } /** @@ -423,13 +437,58 @@ export class FederatedIndexDatabase { } /** - * Get all properties for a publisher domain + * Get all properties for a publisher domain. + * + * UNION over the legacy `discovered_properties` table and the catalog-side + * `publishers.adagents_json` JSONB during the dual-write window of #3177. + * The legacy half wins on (publisher_domain, name, property_type) + * collisions so callers that hold a `discovered_properties.id` keep + * dereferencing it correctly. Catalog-only rows surface for properties + * that landed via the new writer path but didn't get a legacy row (e.g. + * post-seed gatavo.com / Setupad #218). After PR 5 the legacy half is + * removed and the query collapses to catalog-only. */ async getPropertiesForDomain(domain: string): Promise { const result = await query( - `SELECT * FROM discovered_properties - WHERE publisher_domain = $1 - ORDER BY property_type, name`, + `WITH unioned AS ( + SELECT id, property_id, publisher_domain, property_type, name, + identifiers, tags, discovered_at, last_validated, expires_at, + 0 AS src_priority + FROM discovered_properties + WHERE publisher_domain = $1 + UNION ALL + SELECT + cp.property_rid AS id, + prop->>'property_id' AS property_id, + p.domain AS publisher_domain, + prop->>'property_type' AS property_type, + prop->>'name' AS name, + COALESCE(prop->'identifiers', '[]'::jsonb) AS identifiers, + COALESCE( + ARRAY(SELECT jsonb_array_elements_text(prop->'tags')), + ARRAY[]::text[] + ) AS tags, + cp.created_at AS discovered_at, + p.last_validated AS last_validated, + p.expires_at AS expires_at, + 1 AS src_priority + FROM publishers p + CROSS JOIN LATERAL jsonb_array_elements(p.adagents_json->'properties') AS prop + LEFT JOIN catalog_properties cp + ON cp.property_id = prop->>'property_id' + AND cp.created_by = 'adagents_json:' || p.domain + WHERE p.domain = $1 + AND p.source_type = 'adagents_json' + AND prop->>'name' IS NOT NULL + AND prop->>'property_type' IS NOT NULL + ), deduped AS ( + SELECT DISTINCT ON (publisher_domain, name, property_type) + id, property_id, publisher_domain, property_type, name, + identifiers, tags, discovered_at, last_validated, expires_at + FROM unioned + ORDER BY publisher_domain, name, property_type, src_priority + ) + SELECT * FROM deduped ORDER BY property_type, name`, [domain] ); return result.rows.map(row => this.deserializeProperty(row)); diff --git a/server/src/db/property-db.ts b/server/src/db/property-db.ts index 7e8bb0904d..57d552a70f 100644 --- a/server/src/db/property-db.ts +++ b/server/src/db/property-db.ts @@ -211,7 +211,13 @@ export class PropertyDatabase { // ========== Discovered Properties ========== /** - * Get discovered properties by publisher domain + * Get discovered properties by publisher domain. + * + * UNION over the legacy `discovered_properties` table and properties + * extracted from `publishers.adagents_json` JSONB. Legacy wins on + * collisions during the #3177 dual-write window so consumers that hold a + * `discovered_properties.id` keep working; catalog-only rows surface for + * post-seed properties that never landed in the legacy table. */ async getDiscoveredPropertiesByDomain(domain: string): Promise( - 'SELECT * FROM discovered_properties WHERE publisher_domain = $1', + `WITH unioned AS ( + SELECT id, property_id, publisher_domain, property_type, name, + identifiers, tags, source_type, 0 AS src_priority + FROM discovered_properties + WHERE publisher_domain = $1 + UNION ALL + SELECT + cp.property_rid AS id, + prop->>'property_id' AS property_id, + p.domain AS publisher_domain, + prop->>'property_type' AS property_type, + prop->>'name' AS name, + COALESCE(prop->'identifiers', '[]'::jsonb) AS identifiers, + COALESCE( + ARRAY(SELECT jsonb_array_elements_text(prop->'tags')), + ARRAY[]::text[] + ) AS tags, + 'adagents_json'::text AS source_type, + 1 AS src_priority + FROM publishers p + CROSS JOIN LATERAL jsonb_array_elements(p.adagents_json->'properties') AS prop + LEFT JOIN catalog_properties cp + ON cp.property_id = prop->>'property_id' + AND cp.created_by = 'adagents_json:' || p.domain + WHERE p.domain = $1 + AND p.source_type = 'adagents_json' + AND prop->>'name' IS NOT NULL + AND prop->>'property_type' IS NOT NULL + ) + SELECT DISTINCT ON (publisher_domain, name, property_type) + id, property_id, publisher_domain, property_type, name, + identifiers, tags, source_type + FROM unioned + ORDER BY publisher_domain, name, property_type, src_priority`, [domain.toLowerCase()] ); return result.rows.map((row) => ({ @@ -271,7 +310,16 @@ export class PropertyDatabase { // ========== Property Registry (Combined View) ========== /** - * Get all properties (hosted + discovered) for registry view + * Get all properties (hosted + discovered) for registry view. + * + * Three-way UNION over hosted_properties (community/enriched), the legacy + * discovered_properties table, and the catalog-side publishers cache + * (PR 1 of #3177). A domain only surfaces once per query — hosted wins + * over crawl, and within the crawl side legacy wins over catalog so + * `agent_count` keeps being computed from the existing + * agent_property_authorizations join during the dual-write window. After + * PR 5 the legacy crawl half is removed and the catalog branch alone + * answers crawl-sourced rows. */ async getAllPropertiesForRegistry(options: ListPropertiesOptions = {}): Promise( ` - -- Hosted properties - SELECT - publisher_domain as domain, - COALESCE(source_type, 'hosted') as source, - COALESCE(jsonb_array_length(adagents_json->'properties'), 0)::int as property_count, - COALESCE(jsonb_array_length(adagents_json->'authorized_agents'), 0)::int as agent_count, - domain_verified as verified - FROM hosted_properties - WHERE is_public = true - AND (review_status IS NULL OR review_status = 'approved') - AND ($1::text IS NULL OR publisher_domain ILIKE $1) - - UNION ALL - - -- Discovered properties (from crawled adagents.json) - SELECT - publisher_domain as domain, - CASE WHEN source_type = 'adagents_json' OR source_type IS NULL THEN 'adagents_json' ELSE 'discovered' END as source, - COUNT(*)::int as property_count, - (SELECT COUNT(DISTINCT apa.agent_url) FROM agent_property_authorizations apa - JOIN discovered_properties dp2 ON apa.property_id = dp2.id - WHERE dp2.publisher_domain = discovered_properties.publisher_domain)::int as agent_count, - true as verified - FROM discovered_properties - WHERE ($1::text IS NULL OR publisher_domain ILIKE $1) - AND publisher_domain NOT IN (SELECT publisher_domain FROM hosted_properties WHERE is_public = true) - GROUP BY publisher_domain, source_type - + WITH hosted AS ( + SELECT + publisher_domain as domain, + COALESCE(source_type, 'hosted') as source, + COALESCE(jsonb_array_length(adagents_json->'properties'), 0)::int as property_count, + COALESCE(jsonb_array_length(adagents_json->'authorized_agents'), 0)::int as agent_count, + domain_verified as verified, + 0 as src_priority + FROM hosted_properties + WHERE is_public = true + AND (review_status IS NULL OR review_status = 'approved') + AND ($1::text IS NULL OR publisher_domain ILIKE $1) + ), + legacy AS ( + SELECT + publisher_domain as domain, + CASE WHEN source_type = 'adagents_json' OR source_type IS NULL THEN 'adagents_json' ELSE 'discovered' END as source, + COUNT(*)::int as property_count, + (SELECT COUNT(DISTINCT apa.agent_url) FROM agent_property_authorizations apa + JOIN discovered_properties dp2 ON apa.property_id = dp2.id + WHERE dp2.publisher_domain = discovered_properties.publisher_domain)::int as agent_count, + true as verified, + 1 as src_priority + FROM discovered_properties + WHERE ($1::text IS NULL OR publisher_domain ILIKE $1) + AND publisher_domain NOT IN (SELECT domain FROM hosted) + GROUP BY publisher_domain, source_type + ), + catalog_only AS ( + SELECT + p.domain as domain, + 'adagents_json'::text as source, + COALESCE(jsonb_array_length(p.adagents_json->'properties'), 0)::int as property_count, + COALESCE(jsonb_array_length(p.adagents_json->'authorized_agents'), 0)::int as agent_count, + true as verified, + 2 as src_priority + FROM publishers p + WHERE p.source_type = 'adagents_json' + AND ($1::text IS NULL OR p.domain ILIKE $1) + AND p.domain NOT IN (SELECT domain FROM hosted) + AND p.domain NOT IN (SELECT domain FROM legacy) + ) + SELECT domain, source, property_count, agent_count, verified + FROM ( + SELECT * FROM hosted + UNION ALL + SELECT * FROM legacy + UNION ALL + SELECT * FROM catalog_only + ) all_sources ORDER BY domain LIMIT $2 OFFSET $3 `, @@ -331,7 +401,12 @@ export class PropertyDatabase { } /** - * Get aggregated stats for the property registry (counts by source type) + * Get aggregated stats for the property registry (counts by source type). + * + * Mirrors getAllPropertiesForRegistry's three-way UNION (hosted + legacy + * discovered + catalog-only publishers) so a domain that has both a + * legacy crawl row and a catalog row is counted exactly once. Hosted + * wins over crawl; legacy wins over catalog within the crawl side. */ async getPropertyRegistryStats(search?: string): Promise> { const escapedSearch = search ? search.replace(/[%_\\]/g, '\\$&') : null; @@ -339,22 +414,35 @@ export class PropertyDatabase { const result = await query<{ source: string; count: number }>( ` - SELECT source, COUNT(*)::int as count FROM ( - -- Hosted properties - SELECT COALESCE(source_type, 'hosted') as source + WITH hosted AS ( + SELECT publisher_domain AS domain, COALESCE(source_type, 'hosted') as source FROM hosted_properties WHERE is_public = true AND (review_status IS NULL OR review_status = 'approved') AND ($1::text IS NULL OR publisher_domain ILIKE $1) - + ), + legacy AS ( + SELECT publisher_domain AS domain, + CASE WHEN source_type = 'adagents_json' OR source_type IS NULL THEN 'adagents_json' ELSE 'discovered' END as source + FROM discovered_properties + WHERE ($1::text IS NULL OR publisher_domain ILIKE $1) + AND publisher_domain NOT IN (SELECT domain FROM hosted) + GROUP BY publisher_domain, source_type + ), + catalog_only AS ( + SELECT p.domain AS domain, 'adagents_json'::text AS source + FROM publishers p + WHERE p.source_type = 'adagents_json' + AND ($1::text IS NULL OR p.domain ILIKE $1) + AND p.domain NOT IN (SELECT domain FROM hosted) + AND p.domain NOT IN (SELECT domain FROM legacy) + ) + SELECT source, COUNT(*)::int AS count FROM ( + SELECT source FROM hosted UNION ALL - - -- Discovered properties - SELECT CASE WHEN source_type = 'adagents_json' OR source_type IS NULL THEN 'adagents_json' ELSE 'discovered' END as source - FROM discovered_properties - WHERE ($1::text IS NULL OR publisher_domain ILIKE $1) - AND publisher_domain NOT IN (SELECT publisher_domain FROM hosted_properties WHERE is_public = true) - GROUP BY publisher_domain, source_type + SELECT source FROM legacy + UNION ALL + SELECT source FROM catalog_only ) sub GROUP BY source `, From 58c3327c9d430fd220fbe8415a89a565f6cd0172 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sun, 26 Apr 2026 05:55:10 -0400 Subject: [PATCH 2/2] fix(registry): harden property readers against malformed adagents.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code- and security-review on PR #3244 converged on one Must Fix and a companion writer-side defense. publishers.adagents_json is publisher- controlled JSONB (the migration 432 comment is explicit about it), and the AdAgentsManager validator only type-checks `authorized_agents` — nothing forces `properties`, `tags`, or `identifiers` to be arrays. A publisher serving a JSON-valid manifest with `properties: "x"` (string) or any non-array value used to crash the readers via: jsonb_array_elements(p.adagents_json->'properties') -- 22023 error jsonb_array_length(p.adagents_json->'properties') -- 22023 error jsonb_array_elements_text(prop->'tags') -- 22023 error For per-domain readers (getPropertiesForDomain, getDiscoveredProperties ByDomain) that's a per-tenant 500. For the registry list/stats readers the catalog_only CTE selects across every publisher, so one poisoned manifest takes down the entire `properties://registry` MCP listing. Two layers of defense: 1. SQL-side jsonb_typeof guards. Every jsonb_array_elements / jsonb_array_length call in the four readers now wraps its argument with `CASE WHEN jsonb_typeof(...) = 'array' THEN ... ELSE '[]'::jsonb END` (or `0` for length). Identifiers and tags get the same guard so a property with `tags: "shopping"` or `identifiers: null` no longer throws. 2. Writer-side normalization. publisher-db.upsertAdagentsCache now normalizes `properties` and `authorized_agents` to arrays before stringifying into JSONB. Defends against future validator regressions and rows that landed before this PR. Both belt and suspenders so the readers are safe regardless of writer hygiene, and the writer doesn't rely on validator details that may loosen later. Also adds: - migration 433 — partial index on catalog_properties(created_by, property_id) WHERE created_by LIKE 'adagents_json:%'. The new readers do a per-property LATERAL join against catalog_properties; without this index the lookup is a sequential scan per row. - registry-reader-poisoned-manifest.test.ts — 6 cases that seed malformed bodies directly via SQL (bypassing the writer's normalization) and assert each reader returns []/0 instead of throwing. All 66 PR 3 baseline cases still pass. 14 crawler-cache + 16 overlay- schema cases also pass. 6 new poisoned-manifest cases pass — 96 cases total green. Refs #3177, #3244. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/src/db/federated-index-db.ts | 16 +- .../433_catalog_adagents_lookup_index.sql | 16 ++ server/src/db/property-db.ts | 32 +++- server/src/db/publisher-db.ts | 17 +- .../registry-reader-poisoned-manifest.test.ts | 145 ++++++++++++++++++ 5 files changed, 214 insertions(+), 12 deletions(-) create mode 100644 server/src/db/migrations/433_catalog_adagents_lookup_index.sql create mode 100644 server/tests/integration/registry-reader-poisoned-manifest.test.ts diff --git a/server/src/db/federated-index-db.ts b/server/src/db/federated-index-db.ts index a7a27e052c..824766b950 100644 --- a/server/src/db/federated-index-db.ts +++ b/server/src/db/federated-index-db.ts @@ -463,9 +463,15 @@ export class FederatedIndexDatabase { p.domain AS publisher_domain, prop->>'property_type' AS property_type, prop->>'name' AS name, - COALESCE(prop->'identifiers', '[]'::jsonb) AS identifiers, + CASE WHEN jsonb_typeof(prop->'identifiers') = 'array' + THEN prop->'identifiers' + ELSE '[]'::jsonb END AS identifiers, COALESCE( - ARRAY(SELECT jsonb_array_elements_text(prop->'tags')), + ARRAY(SELECT jsonb_array_elements_text( + CASE WHEN jsonb_typeof(prop->'tags') = 'array' + THEN prop->'tags' + ELSE '[]'::jsonb END + )), ARRAY[]::text[] ) AS tags, cp.created_at AS discovered_at, @@ -473,7 +479,11 @@ export class FederatedIndexDatabase { p.expires_at AS expires_at, 1 AS src_priority FROM publishers p - CROSS JOIN LATERAL jsonb_array_elements(p.adagents_json->'properties') AS prop + CROSS JOIN LATERAL jsonb_array_elements( + CASE WHEN jsonb_typeof(p.adagents_json->'properties') = 'array' + THEN p.adagents_json->'properties' + ELSE '[]'::jsonb END + ) AS prop LEFT JOIN catalog_properties cp ON cp.property_id = prop->>'property_id' AND cp.created_by = 'adagents_json:' || p.domain diff --git a/server/src/db/migrations/433_catalog_adagents_lookup_index.sql b/server/src/db/migrations/433_catalog_adagents_lookup_index.sql new file mode 100644 index 0000000000..9d2c54739c --- /dev/null +++ b/server/src/db/migrations/433_catalog_adagents_lookup_index.sql @@ -0,0 +1,16 @@ +-- Index supporting the property-side reader cutover (PR 4a of #3177). +-- +-- The new readers in federated-index-db.ts and property-db.ts join +-- catalog_properties on (created_by = 'adagents_json:', +-- property_id = ) inside a CROSS JOIN LATERAL over +-- the publisher's manifest. Without this index, each manifest property +-- triggers a sequential scan of catalog_properties — O(N×M) for a +-- publisher with M properties against N catalog rows. +-- +-- The partial WHERE clause keeps the index narrow: only adagents-sourced +-- rows ever match this lookup pattern. Community/system/seed rows are +-- still served by their own classification/status indexes. + +CREATE INDEX IF NOT EXISTS idx_catalog_properties_adagents_lookup + ON catalog_properties (created_by, property_id) + WHERE created_by LIKE 'adagents_json:%'; diff --git a/server/src/db/property-db.ts b/server/src/db/property-db.ts index 57d552a70f..aa32a1602f 100644 --- a/server/src/db/property-db.ts +++ b/server/src/db/property-db.ts @@ -251,15 +251,25 @@ export class PropertyDatabase { p.domain AS publisher_domain, prop->>'property_type' AS property_type, prop->>'name' AS name, - COALESCE(prop->'identifiers', '[]'::jsonb) AS identifiers, + CASE WHEN jsonb_typeof(prop->'identifiers') = 'array' + THEN prop->'identifiers' + ELSE '[]'::jsonb END AS identifiers, COALESCE( - ARRAY(SELECT jsonb_array_elements_text(prop->'tags')), + ARRAY(SELECT jsonb_array_elements_text( + CASE WHEN jsonb_typeof(prop->'tags') = 'array' + THEN prop->'tags' + ELSE '[]'::jsonb END + )), ARRAY[]::text[] ) AS tags, 'adagents_json'::text AS source_type, 1 AS src_priority FROM publishers p - CROSS JOIN LATERAL jsonb_array_elements(p.adagents_json->'properties') AS prop + CROSS JOIN LATERAL jsonb_array_elements( + CASE WHEN jsonb_typeof(p.adagents_json->'properties') = 'array' + THEN p.adagents_json->'properties' + ELSE '[]'::jsonb END + ) AS prop LEFT JOIN catalog_properties cp ON cp.property_id = prop->>'property_id' AND cp.created_by = 'adagents_json:' || p.domain @@ -345,8 +355,12 @@ export class PropertyDatabase { SELECT publisher_domain as domain, COALESCE(source_type, 'hosted') as source, - COALESCE(jsonb_array_length(adagents_json->'properties'), 0)::int as property_count, - COALESCE(jsonb_array_length(adagents_json->'authorized_agents'), 0)::int as agent_count, + (CASE WHEN jsonb_typeof(adagents_json->'properties') = 'array' + THEN jsonb_array_length(adagents_json->'properties') + ELSE 0 END)::int as property_count, + (CASE WHEN jsonb_typeof(adagents_json->'authorized_agents') = 'array' + THEN jsonb_array_length(adagents_json->'authorized_agents') + ELSE 0 END)::int as agent_count, domain_verified as verified, 0 as src_priority FROM hosted_properties @@ -373,8 +387,12 @@ export class PropertyDatabase { SELECT p.domain as domain, 'adagents_json'::text as source, - COALESCE(jsonb_array_length(p.adagents_json->'properties'), 0)::int as property_count, - COALESCE(jsonb_array_length(p.adagents_json->'authorized_agents'), 0)::int as agent_count, + (CASE WHEN jsonb_typeof(p.adagents_json->'properties') = 'array' + THEN jsonb_array_length(p.adagents_json->'properties') + ELSE 0 END)::int as property_count, + (CASE WHEN jsonb_typeof(p.adagents_json->'authorized_agents') = 'array' + THEN jsonb_array_length(p.adagents_json->'authorized_agents') + ELSE 0 END)::int as agent_count, true as verified, 2 as src_priority FROM publishers p diff --git a/server/src/db/publisher-db.ts b/server/src/db/publisher-db.ts index 644ad2e809..ec8928dd52 100644 --- a/server/src/db/publisher-db.ts +++ b/server/src/db/publisher-db.ts @@ -77,6 +77,19 @@ export class PublisherDatabase { try { await client.query('BEGIN'); + // Normalize array fields before caching. The validator only enforces + // `authorized_agents` shape, so a publisher serving a JSON-valid file + // with `properties: "x"` could otherwise land non-array JSONB that + // breaks downstream readers (jsonb_array_elements / jsonb_array_length + // error on non-arrays). Belt-and-suspenders for the SQL-side guards. + const safeManifest: AdagentsManifest = { + ...input.manifest, + properties: Array.isArray(input.manifest.properties) ? input.manifest.properties : [], + authorized_agents: Array.isArray(input.manifest.authorized_agents) + ? input.manifest.authorized_agents + : [], + }; + await client.query( `INSERT INTO publishers (domain, adagents_json, source_type, last_validated, expires_at) VALUES ($1, $2::jsonb, 'adagents_json', NOW(), $3) @@ -86,10 +99,10 @@ export class PublisherDatabase { last_validated = NOW(), expires_at = EXCLUDED.expires_at, updated_at = NOW()`, - [domain, JSON.stringify(input.manifest), input.expiresAt ?? null] + [domain, JSON.stringify(safeManifest), input.expiresAt ?? null] ); - const properties = Array.isArray(input.manifest.properties) ? input.manifest.properties : []; + const properties = Array.isArray(safeManifest.properties) ? safeManifest.properties : []; for (let i = 0; i < properties.length; i += 1) { const savepoint = `prop_${i}`; await client.query(`SAVEPOINT ${savepoint}`); diff --git a/server/tests/integration/registry-reader-poisoned-manifest.test.ts b/server/tests/integration/registry-reader-poisoned-manifest.test.ts new file mode 100644 index 0000000000..3613a8d593 --- /dev/null +++ b/server/tests/integration/registry-reader-poisoned-manifest.test.ts @@ -0,0 +1,145 @@ +/** + * Regression coverage for the malformed-manifest DoS surfaced by both + * reviewers on PR 4a of #3177. The validator (adagents-manager.ts) only + * type-checks `authorized_agents`; a publisher serving a JSON-valid + * manifest with `properties: "x"` (or any non-array) used to crash + * jsonb_array_elements / jsonb_array_length in the readers, taking down + * the public registry listing for everyone. + * + * Two layers of defense exercised here: + * - publisher-db.ts upsertAdagentsCache normalizes `properties` / + * `authorized_agents` to arrays before stringifying into JSONB. + * - federated-index-db.ts / property-db.ts readers wrap the JSONB + * operators with `jsonb_typeof = 'array'` guards. + * + * The test forces the worst case by writing the malformed body directly, + * bypassing the writer's normalization, then asserts every reader returns + * an empty/zero result rather than throwing. + */ +import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'; +import type { Pool } from 'pg'; +import { initializeDatabase, closeDatabase } from '../../src/db/client.js'; +import { runMigrations } from '../../src/db/migrate.js'; +import { FederatedIndexDatabase } from '../../src/db/federated-index-db.js'; +import { PropertyDatabase } from '../../src/db/property-db.js'; + +const POISONED_DOMAIN = 'poisoned.registry-baseline.example'; + +describe('Registry readers tolerate malformed publishers.adagents_json bodies', () => { + let pool: Pool; + let fedDb: FederatedIndexDatabase; + let propDb: PropertyDatabase; + + beforeAll(async () => { + pool = initializeDatabase({ + connectionString: + process.env.DATABASE_URL || 'postgresql://adcp:localdev@localhost:5432/adcp_test', + }); + await runMigrations(); + fedDb = new FederatedIndexDatabase(); + propDb = new PropertyDatabase(); + }); + + async function clearFixtures() { + await pool.query('DELETE FROM publishers WHERE domain = $1', [POISONED_DOMAIN]); + await pool.query('DELETE FROM discovered_properties WHERE publisher_domain = $1', [POISONED_DOMAIN]); + await pool.query('DELETE FROM discovered_publishers WHERE domain = $1', [POISONED_DOMAIN]); + } + + beforeEach(async () => { + await clearFixtures(); + }); + + afterAll(async () => { + await clearFixtures(); + await closeDatabase(); + }); + + /** + * Write the manifest body directly into publishers.adagents_json, + * bypassing the writer's normalization. This simulates either: + * - a row that landed before publisher-db.ts grew its normalization, or + * - a future regression where the writer's guard slips. + */ + async function seedPoisonedManifest(body: unknown): Promise { + await pool.query( + `INSERT INTO publishers (domain, adagents_json, source_type, last_validated) + VALUES ($1, $2::jsonb, 'adagents_json', NOW()) + ON CONFLICT (domain) DO UPDATE SET + adagents_json = EXCLUDED.adagents_json, + source_type = 'adagents_json', + last_validated = NOW()`, + [POISONED_DOMAIN, JSON.stringify(body)] + ); + } + + it('getPropertiesForDomain returns [] when properties is a string', async () => { + await seedPoisonedManifest({ authorized_agents: [], properties: 'evil' }); + await expect(fedDb.getPropertiesForDomain(POISONED_DOMAIN)).resolves.toEqual([]); + }); + + it('getDiscoveredPropertiesByDomain returns [] when properties is an object', async () => { + await seedPoisonedManifest({ authorized_agents: [], properties: { not: 'an array' } }); + await expect(propDb.getDiscoveredPropertiesByDomain(POISONED_DOMAIN)).resolves.toEqual([]); + }); + + it('getPropertiesForDomain tolerates a non-array tags field on a manifest property', async () => { + await seedPoisonedManifest({ + authorized_agents: [], + properties: [ + { + property_id: 'p1', + property_type: 'website', + name: 'Bad Tags Site', + identifiers: [{ type: 'domain', value: POISONED_DOMAIN }], + tags: 'shopping', + }, + ], + }); + const props = await fedDb.getPropertiesForDomain(POISONED_DOMAIN); + expect(props).toHaveLength(1); + expect(props[0].tags).toEqual([]); + }); + + it('getPropertiesForDomain tolerates a non-array identifiers field on a manifest property', async () => { + await seedPoisonedManifest({ + authorized_agents: [], + properties: [ + { + property_id: 'p1', + property_type: 'website', + name: 'Bad Identifiers Site', + identifiers: 'not-an-array', + }, + ], + }); + const props = await fedDb.getPropertiesForDomain(POISONED_DOMAIN); + expect(props).toHaveLength(1); + expect(props[0].identifiers).toEqual([]); + }); + + it('getAllPropertiesForRegistry tolerates a poisoned publisher and still returns clean rows', async () => { + // Poisoned manifest with non-array properties — would have killed the + // whole catalog_only CTE before the jsonb_typeof guards. + await seedPoisonedManifest({ authorized_agents: 12, properties: 'x' }); + + // Without throwing, the listing should run. The poisoned domain itself + // surfaces (via the catalog_only branch) with zero counts. + const rows = await propDb.getAllPropertiesForRegistry({ + search: 'poisoned.registry-baseline', + limit: 50, + }); + const poisonedRow = rows.find((r) => r.domain === POISONED_DOMAIN); + expect(poisonedRow).toBeTruthy(); + expect(poisonedRow!.property_count).toBe(0); + expect(poisonedRow!.agent_count).toBe(0); + }); + + it('getPropertyRegistryStats tolerates a poisoned publisher', async () => { + await seedPoisonedManifest({ authorized_agents: null, properties: false }); + // Just checking it doesn't throw — the poisoned row counts toward + // adagents_json under the catalog_only branch. + const stats = await propDb.getPropertyRegistryStats('poisoned.registry-baseline'); + expect(stats.total).toBeGreaterThanOrEqual(0); + }); +});