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..824766b950 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,68 @@ 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, + CASE WHEN jsonb_typeof(prop->'identifiers') = 'array' + THEN prop->'identifiers' + ELSE '[]'::jsonb END AS identifiers, + COALESCE( + 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, + 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( + 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 + 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/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 7e8bb0904d..aa32a1602f 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, + CASE WHEN jsonb_typeof(prop->'identifiers') = 'array' + THEN prop->'identifiers' + ELSE '[]'::jsonb END AS identifiers, + COALESCE( + 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( + 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 + 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 +320,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, + (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 + 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, + (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 + 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 +419,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 +432,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 `, 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); + }); +});