fix: validate packagist response shapes before accepting

isPackagistStatsJson and isP2Response only checked top-level presence
(package.name is a string, packages[name] is an array). A wrong
runtime type for a field consumed unconditionally downstream (.trim()
on description/repository, .filter() on maintainers, .startsWith()/
.split()/.endsWith() on version/version_normalized) threw uncaught
instead of being classified MALFORMED, so Temporal retried it as
transient — a poison pill, since the same deterministic parse failure
repeats every attempt until INGEST_MAX_ATTEMPTS gives up.

Both guards now validate every field consumed unconditionally by
normalize.ts, honoring the p2 minified-diff '__unset' sentinel.
normalizePackagistStats' maintainer filter also guards against a
rogue null/non-object array element as a second line of defense.

Signed-off-by: anilb <epipav@gmail.com>

Author: epipav

services/apps/packages_worker/src/packagist/__tests__/fetchPackage.test.ts

@@ -118,6 +118,21 @@ describe('fetchPackagistStats', () => {
     expect(await fetchPackagistStats('monolog/monolog')).toMatchObject({ kind: 'MALFORMED' })
   })
 
+  // Regression: a technically-valid-JSON body with the wrong runtime type for a field
+  // normalizePackagistStats consumes unconditionally must be classified MALFORMED, not
+  // thrown past the guard (where it would be misread as a transient failure and
+  // retried forever on the same deterministic input).
+  it.each([
+    ['description is a number', { package: { name: 'a/b', description: 123 } }],
+    ['repository is an object', { package: { name: 'a/b', repository: {} } }],
+    ['maintainers is not an array', { package: { name: 'a/b', maintainers: {} } }],
+  ])('maps a wrong-typed field (%s) → MALFORMED, not a throw', async (_desc, body) => {
+    vi.stubGlobal('fetch', vi.fn().mockResolvedValue(fakeResponse(200, body)))
+    await expect(fetchPackagistStats('monolog/monolog')).resolves.toMatchObject({
+      kind: 'MALFORMED',
+    })
+  })
+
   it('maps a body read aborted by the 30s timeout → TRANSIENT (not MALFORMED)', async () => {
     vi.useFakeTimers()
     vi.stubGlobal(
@@ -198,4 +213,39 @@ describe('fetchPackagistP2', () => {
     )
     expect(await fetchPackagistP2('monolog/monolog', null)).toMatchObject({ kind: 'MALFORMED' })
   })
+
+  // Regression: a malformed element in the version array must be classified
+  // MALFORMED, not thrown past the guard — expandComposerMetadata's Object.entries()
+  // throws on a null/non-object element, and version/version_normalized/license reach
+  // .startsWith()/.split()/.endsWith() and the SQL text[] write unconditionally.
+  it.each([
+    ['an element is null', [null]],
+    ['version is missing', [{ time: '2024-01-01' }]],
+    ['version is a number', [{ version: 123 }]],
+    ['version_normalized is a number', [{ version: '1.0.0', version_normalized: 42 }]],
+    ['license is a scalar string, not an array', [{ version: '1.0.0', license: 'MIT' }]],
+    ['homepage is an object', [{ version: '1.0.0', homepage: {} }]],
+    ['time is a number', [{ version: '1.0.0', time: 20240101 }]],
+  ])('maps a malformed version entry (%s) → MALFORMED, not a throw', async (_desc, versions) => {
+    vi.stubGlobal(
+      'fetch',
+      vi.fn().mockResolvedValue(fakeResponse(200, { packages: { 'monolog/monolog': versions } })),
+    )
+    await expect(fetchPackagistP2('monolog/monolog', null)).resolves.toMatchObject({
+      kind: 'MALFORMED',
+    })
+  })
+
+  it('accepts a version entry using the __unset diff sentinel on optional fields', async () => {
+    const versions = [
+      { version: '1.0.0', license: ['MIT'] },
+      { version: '2.0.0', license: '__unset' },
+    ]
+    vi.stubGlobal(
+      'fetch',
+      vi.fn().mockResolvedValue(fakeResponse(200, { packages: { 'monolog/monolog': versions } })),
+    )
+    const result = await fetchPackagistP2('monolog/monolog', null)
+    expect(result).not.toMatchObject({ kind: 'MALFORMED' })
+  })
 })

services/apps/packages_worker/src/packagist/__tests__/normalize.test.ts

@@ -55,6 +55,18 @@ describe('normalizePackagistStats', () => {
     expect(normalizePackagistStats(full).status).toBe('active')
   })
 
+  it('drops a null/non-object maintainer entry instead of throwing', () => {
+    // isPackagistStatsJson only guarantees maintainers is an array, not that every
+    // element is a well-formed object — a rogue null must not crash `m.name` access.
+    const stats = normalizePackagistStats({
+      name: 'a/b',
+      maintainers: [null, 'not-an-object', { name: 'seldaek' }] as never,
+    })
+    expect(stats.maintainers).toEqual([
+      { username: 'seldaek', displayName: null, email: null, role: 'maintainer' },
+    ])
+  })
+
   it('tolerates missing optional fields with nulls and empty lists', () => {
     const stats = normalizePackagistStats({ name: 'a/b' })
     expect(stats).toEqual({

services/apps/packages_worker/src/packagist/fetchPackage.ts

@@ -171,11 +171,47 @@ export async function fetchPackagistP2(
   }
 }
 
+// Validates every field normalizePackagistStats consumes unconditionally
+// (blankToNull's .trim(), the maintainers .filter()) — a wrong runtime type here must
+// surface as MALFORMED, not throw past the fast-retry/give-up path and get treated as
+// a transient failure that Temporal retries forever on the same deterministic input.
 function isPackagistStatsJson(v: unknown): v is PackagistStatsJson {
   if (typeof v !== 'object' || v === null || !('package' in v)) return false
   const pkg = (v as { package: unknown }).package
+  if (typeof pkg !== 'object' || pkg === null) return false
+  const p = pkg as Record<string, unknown>
+  if (typeof p.name !== 'string') return false
+  if (p.description != null && typeof p.description !== 'string') return false
+  if (p.repository != null && typeof p.repository !== 'string') return false
+  if (p.maintainers != null && !Array.isArray(p.maintainers)) return false
+  return true
+}
+
+// '__unset' is the p2 minified-diff sentinel for "field removed since the previous
+// entry" — a legitimate value for any optional field, not just an absent key.
+function isValidVersionField(value: unknown, isValidType: (v: unknown) => boolean): boolean {
+  return value === undefined || value === '__unset' || isValidType(value)
+}
+
+// Validates every field normalize.ts consumes unconditionally on an expanded version:
+// `version`/`version_normalized` via .startsWith()/.split()/.endsWith()
+// (isPackagistDevVersion/isPackagistPrerelease), `homepage` via blankToNull's .trim(),
+// `license` as the text[] written to versions.licenses, `time` as the timestamptz
+// written to versions.published_at. `require`/`require-dev` are already guarded at
+// their own call site (extractVersionDependencies checks typeof before Object.entries)
+// so aren't re-validated here.
+function isValidMinifiedVersion(v: unknown): boolean {
+  if (typeof v !== 'object' || v === null) return false
+  const entry = v as Record<string, unknown>
   return (
-    typeof pkg === 'object' && pkg !== null && typeof (pkg as { name?: unknown }).name === 'string'
+    typeof entry.version === 'string' &&
+    isValidVersionField(entry.version_normalized, (x) => typeof x === 'string') &&
+    isValidVersionField(entry.homepage, (x) => typeof x === 'string') &&
+    isValidVersionField(entry.time, (x) => typeof x === 'string') &&
+    isValidVersionField(
+      entry.license,
+      (x) => Array.isArray(x) && x.every((l) => typeof l === 'string'),
+    )
   )
 }
 
@@ -184,5 +220,5 @@ function isP2Response(v: unknown, name: string): v is { packages: Record<string,
   const packages = (v as { packages: unknown }).packages
   if (typeof packages !== 'object' || packages === null) return false
   const entries = (packages as Record<string, unknown>)[name]
-  return Array.isArray(entries)
+  return Array.isArray(entries) && entries.every(isValidMinifiedVersion)
 }

services/apps/packages_worker/src/packagist/normalize.ts

@@ -34,9 +34,13 @@ export function normalizePackagistStats(pkg: PackagistPackageInfo): NormalizedPa
 
   const dependents = typeof pkg.dependents === 'number' ? pkg.dependents : null
 
-  // Extract maintainers with non-empty names
+  // Extract maintainers with non-empty names. isPackagistStatsJson only guarantees
+  // maintainers is an array — a rogue null/non-object element would throw on `m.name`.
   const maintainers = (pkg.maintainers ?? [])
-    .filter((m): m is { name: string } => typeof m.name === 'string' && !!m.name)
+    .filter(
+      (m): m is { name: string } =>
+        typeof m === 'object' && m !== null && typeof m.name === 'string' && !!m.name,
+    )
     .map((m) => ({
       username: m.name,
       displayName: null,