Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 57 additions & 35 deletions services/apps/packages_worker/src/go/activities.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import { Context } from '@temporalio/activity'

import { logAuditFieldChanges } from '@crowd/data-access-layer/src/packages'
import {
getOrCreateRepoByUrl,
logAuditFieldChanges,
upsertPackageRepo,
} from '@crowd/data-access-layer/src/packages'
import type { QueryExecutor } from '@crowd/data-access-layer/src/queryExecutor'
import { getServiceChildLogger } from '@crowd/logging'

import { getGoConfig } from '../config'
import { getPackagesDb } from '../db'
import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl'

import { fetchStatus } from './pkgGoDevClient'
import { fetchLatest } from './proxyClient'
Expand All @@ -21,7 +26,7 @@ export interface GoScanCursor {
after: string
}

type GoRow = { purl: string; name: string }
type GoRow = { id: string; purl: string; name: string }

// Two independent purl-keyset cursors — one for critical packages, one for everything else —
// each ordered/paginated purely by purl so WHERE and ORDER BY always match (no gaps, no
Expand All @@ -37,7 +42,7 @@ async function getGoBatch(
): Promise<GoRow[]> {
if (batchSize <= 0) return []
return qx.select(
`SELECT purl, name FROM packages
`SELECT id::text AS id, purl, name FROM packages
WHERE ecosystem = 'go' AND is_critical = $(isCritical) AND purl > $(after)
ORDER BY purl ASC
LIMIT $(limit)`,
Expand Down Expand Up @@ -75,7 +80,7 @@ export async function enrichGoVersionsBatch(

const { fetchTimeoutMs, proxyConcurrency } = getGoConfig()

const enrichOne = async (row: { purl: string; name: string }): Promise<void> => {
const enrichOne = async (row: GoRow): Promise<void> => {
Context.current().heartbeat(row.purl)
const result = await fetchLatest(row.name, fetchTimeoutMs)
if (isFetchError(result)) {
Expand All @@ -85,37 +90,54 @@ export async function enrichGoVersionsBatch(
)
return
}
const changed = await qx.selectOne(
`WITH old AS (
SELECT latest_version AS v, latest_release_at AS t, repository_url AS r
FROM packages WHERE purl = $(purl)
),
upd AS (
UPDATE packages p SET
latest_version = $(version),
latest_release_at = $(releaseAt),
repository_url = COALESCE($(repoUrl), p.repository_url),
last_synced_at = NOW()
WHERE p.purl = $(purl)
RETURNING latest_version AS v, latest_release_at AS t, repository_url AS r
)
SELECT
(SELECT v FROM old) IS DISTINCT FROM (SELECT v FROM upd) AS v_changed,
(SELECT t FROM old) IS DISTINCT FROM (SELECT t FROM upd) AS t_changed,
(SELECT r FROM old) IS DISTINCT FROM (SELECT r FROM upd) AS r_changed`,
{
version: result.version,
releaseAt: result.releaseAt,
repoUrl: result.repoUrl,
purl: row.purl,
},
)
const changedFields = [
changed?.v_changed ? 'packages.latest_version' : null,
changed?.t_changed ? 'packages.latest_release_at' : null,
changed?.r_changed ? 'packages.repository_url' : null,
].filter(Boolean) as string[]
await logAuditFieldChanges(qx, PROXY_SOURCE, row.purl, changedFields)
const repo = result.repoUrl ? canonicalizeRepoUrl(result.repoUrl) : null
Comment thread
mbani01 marked this conversation as resolved.

await qx.tx(async (t) => {
const changed = await t.selectOne(
`WITH old AS (
SELECT latest_version AS v, latest_release_at AS t, repository_url AS r
FROM packages WHERE purl = $(purl)
),
upd AS (
UPDATE packages p SET
latest_version = $(version),
latest_release_at = $(releaseAt),
repository_url = COALESCE($(repoUrl), p.repository_url),
last_synced_at = NOW()
WHERE p.purl = $(purl)
RETURNING latest_version AS v, latest_release_at AS t, repository_url AS r
)
SELECT
(SELECT v FROM old) IS DISTINCT FROM (SELECT v FROM upd) AS v_changed,
(SELECT t FROM old) IS DISTINCT FROM (SELECT t FROM upd) AS t_changed,
(SELECT r FROM old) IS DISTINCT FROM (SELECT r FROM upd) AS r_changed`,
{
version: result.version,
releaseAt: result.releaseAt,
repoUrl: repo?.url ?? null,
purl: row.purl,
},
)
const changedFields = [
changed?.v_changed ? 'packages.latest_version' : null,
changed?.t_changed ? 'packages.latest_release_at' : null,
changed?.r_changed ? 'packages.repository_url' : null,
].filter(Boolean) as string[]

if (repo) {
const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl(
t,
repo.url,
repo.host,
)
changedFields.push(...repoChanged)

const linkChanged = await upsertPackageRepo(t, row.id, repoId, 'declared', 0.8)
Comment thread
mbani01 marked this conversation as resolved.
Comment thread
mbani01 marked this conversation as resolved.
Comment thread
mbani01 marked this conversation as resolved.
changedFields.push(...linkChanged)
Comment on lines +132 to +142
}
Comment thread
cursor[bot] marked this conversation as resolved.

await logAuditFieldChanges(t, PROXY_SOURCE, row.purl, changedFields)
})
}

for (let i = 0; i < rows.length; i += proxyConcurrency) {
Expand Down
27 changes: 27 additions & 0 deletions services/apps/packages_worker/src/nuget/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from './types'

const SERVICE_INDEX_URL = 'https://api.nuget.org/v3/index.json'
const FLAT_CONTAINER_BASE_URL = 'https://api.nuget.org/v3-flatcontainer'

interface ServiceIndexResource {
'@id': string
Expand Down Expand Up @@ -144,3 +145,29 @@ export async function fetchRegistration(
throw err
}
}

// The registration API never exposes the nuspec <repository> element — it must be read from the
// raw nuspec XML, served by the flat-container endpoint.
Comment on lines +149 to +150
export async function fetchNuspec(
packageId: string,
version: string,
): Promise<string | NuGetFetchError> {
const lowerId = packageId.toLowerCase()
const lowerVersion = version.toLowerCase()

try {
const resp = await axios.get<string>(
`${FLAT_CONTAINER_BASE_URL}/${lowerId}/${lowerVersion}/${lowerId}.nuspec`,
{
responseType: 'text',
transformResponse: (data) => data,
timeout: 15000,
},
Comment on lines +159 to +165
)
return resp.data
} catch (err) {
const classified = classifyError(err)
if (classified) return classified
throw err

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nuspec fetch aborts enrichment

Medium Severity

fetchNuspec rethrows most HTTP and network errors after registration has already succeeded. processPackage does not treat those as optional, so a transient flat-container failure can mark the whole package as errored instead of continuing without nuspec data.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit db5be1a. Configure here.

}
}
36 changes: 23 additions & 13 deletions services/apps/packages_worker/src/nuget/normalize.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import { XMLParser } from 'fast-xml-parser'

import { canonicalizeRepoUrl } from '../utils/canonicalizeRepoUrl'

import {
NormalizedNuGetPackage,
NormalizedNuGetVersion,
Expand Down Expand Up @@ -26,18 +30,20 @@ function parsePublishedDate(published: string | undefined): Date | null {
return !isNaN(date.getTime()) && date.getUTCFullYear() > 1900 ? date : null
}

const SCM_HOSTS = ['github.com', 'gitlab.com', 'bitbucket.org']
const nuspecParser = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: '@_' })

function normalizeRepoUrl(url: string | undefined): string | null {
if (!url) return null
return url
.trim()
.replace(/\.git$/, '')
.replace(/^git\+/, '')
.replace(/^git:\/\//, 'https://')
.replace(/^http:\/\/github\.com\//, 'https://github.com/')
export function parseNuspecRepositoryUrl(nuspecXml: string): string | null {
try {
const doc = nuspecParser.parse(nuspecXml)
const url = doc?.package?.metadata?.repository?.['@_url']
return typeof url === 'string' && url.trim() !== '' ? url.trim() : null
} catch {
return null
}
Comment on lines +33 to +42
}

const SCM_HOSTS = ['github.com', 'gitlab.com', 'bitbucket.org']

function isScmUrl(url: string | undefined): boolean {
if (!url) return false
try {
Expand All @@ -64,6 +70,7 @@ export function normalizeNuGetPackage(
packageId: string,
searchResult: NuGetSearchItem | null,
registration: NuGetRegistrationIndex,
nuspecXml?: string | null,
): NormalizedNuGetPackage {
const allLeaves = registration.items.flatMap((page) => page.items ?? [])
const allEntries: NuGetCatalogEntry[] = allLeaves.map((leaf) => leaf.catalogEntry)
Expand Down Expand Up @@ -96,10 +103,13 @@ export function normalizeNuGetPackage(
...(latestEntry ? [latestEntry] : []),
]
: [...allEntries].reverse()
const nuspecRepoUrl = entriesForRepo.find((e) => e.repository?.url)?.repository?.url
const catalogRepoUrl = entriesForRepo.find((e) => e.repository?.url)?.repository?.url
const fetchedNuspecRepoUrl = nuspecXml ? parseNuspecRepositoryUrl(nuspecXml) : null
const nuspecRepoUrl = fetchedNuspecRepoUrl ?? catalogRepoUrl
const declaredRepositoryUrl = nuspecRepoUrl ?? null
const repoCandidate = nuspecRepoUrl ?? (isScmUrl(homepage) ? homepage : null)
const repositoryUrl = normalizeRepoUrl(repoCandidate ?? undefined)
const repo =
(nuspecRepoUrl ? canonicalizeRepoUrl(nuspecRepoUrl) : null) ??
(isScmUrl(homepage) ? canonicalizeRepoUrl(homepage) : null)
Comment thread
mbani01 marked this conversation as resolved.
Comment on lines +110 to +112

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbani01 do you think it maybe worth toreview this ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4d69373

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Homepage used when declared repo fails

Medium Severity

normalizeNuGetPackage resolves repo with nullish coalescing after canonicalizeRepoUrl on nuspecRepoUrl. When a declared repository URL is present but cannot be canonicalized, the code still links via the SCM-shaped homepage. That contradicts the nearby comment and can create incorrect package_repos rows while declaredRepositoryUrl stays different.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 36f39e4. Configure here.

Comment on lines +108 to +112

const keywords = searchResult?.tags && searchResult.tags.length > 0 ? searchResult.tags : null

Expand Down Expand Up @@ -154,7 +164,7 @@ export function normalizeNuGetPackage(
description,
homepage: homepage || null,
declaredRepositoryUrl,
repositoryUrl,
repo,
licenses,
licensesRaw,
keywords,
Expand Down
34 changes: 31 additions & 3 deletions services/apps/packages_worker/src/nuget/runNuGetEnrichmentLoop.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
IDbNuGetVersionUpsert,
NuGetPackageToSync,
QueryExecutor,
getOrCreateRepoByUrl,
listNuGetPackagesToSync,
logAuditFieldChange,
markNuGetPackageError,
Expand All @@ -10,12 +11,13 @@ import {
upsertMaintainer,
upsertNuGetPackage,
upsertNuGetVersionsBatch,
upsertPackageRepo,
} from '@crowd/data-access-layer'
import { getServiceChildLogger } from '@crowd/logging'

import { getNuGetConfig } from '../config'

import { fetchRegistration, fetchSearch } from './client'
import { fetchNuspec, fetchRegistration, fetchSearch } from './client'
import { normalizeNuGetPackage } from './normalize'
import { BatchResult, isNuGetFetchError } from './types'

Expand Down Expand Up @@ -96,7 +98,15 @@ async function processPackage(

const searchItem = isNuGetFetchError(searchResult) ? null : searchResult

const normalized = normalizeNuGetPackage(packageId, searchItem, registrationResult)
const preliminary = normalizeNuGetPackage(packageId, searchItem, registrationResult)

let nuspecXml: string | null = null
if (preliminary.latestVersion) {
const nuspecResult = await fetchNuspec(packageId, preliminary.latestVersion)
Comment on lines +101 to +105
nuspecXml = isNuGetFetchError(nuspecResult) ? null : nuspecResult

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nuspec fetch failure blocks package sync

Medium Severity

After registration and search succeed, fetchNuspec can throw on transient HTTP or network errors. That aborts processPackage, marks the package in error, and skips the DB upsert even though enrichment used to complete using catalog and homepage fallbacks when nuspec was not fetched.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9ac2223. Configure here.

}

const normalized = normalizeNuGetPackage(packageId, searchItem, registrationResult, nuspecXml)

await withDeadlockRetry(() =>
qx.tx(async (t) => {
Expand All @@ -108,7 +118,7 @@ async function processPackage(
description: normalized.description,
homepage: normalized.homepage,
declaredRepositoryUrl: normalized.declaredRepositoryUrl,
repositoryUrl: normalized.repositoryUrl,
repositoryUrl: normalized.repo?.url ?? null,
licenses: normalized.licenses,
licensesRaw: normalized.licensesRaw,
keywords: normalized.keywords,
Expand All @@ -122,6 +132,24 @@ async function processPackage(
})
pkgChanged.forEach((f) => changed.add(f))

if (normalized.repo) {
const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl(
t,
normalized.repo.url,
normalized.repo.host,
)
repoChanged.forEach((f) => changed.add(f))

const linkChanged = await upsertPackageRepo(
Comment thread
mbani01 marked this conversation as resolved.
t,
packageDbId.toString(),
repoId,
'declared',
Comment on lines +143 to +147
0.8,
Comment thread
mbani01 marked this conversation as resolved.
)
Comment thread
mbani01 marked this conversation as resolved.
Comment on lines +143 to +149
linkChanged.forEach((f) => changed.add(f))
}

if (normalized.versions.length > 0) {
const versionRows: IDbNuGetVersionUpsert[] = normalized.versions.map((v) => ({
packageId: packageDbId,
Expand Down
4 changes: 3 additions & 1 deletion services/apps/packages_worker/src/nuget/types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { CanonicalRepo } from '../utils/canonicalizeRepoUrl'

export interface NuGetConfig {
batchSize: number
concurrency: number
Expand Down Expand Up @@ -73,7 +75,7 @@ export interface NormalizedNuGetPackage {
description: string | null
homepage: string | null
declaredRepositoryUrl: string | null
repositoryUrl: string | null
repo: CanonicalRepo | null
licenses: string[] | null
licensesRaw: string | null
keywords: string[] | null
Expand Down
11 changes: 6 additions & 5 deletions services/apps/packages_worker/src/utils/canonicalizeRepoUrl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,18 @@ export function canonicalizeRepoUrl(raw: string): CanonicalRepo | null {
const segments = u.pathname.split('/').filter(Boolean)
if (segments.length < 2) return null

let owner = segments[0]
let name = segments[1].replace(/\.git$/, '')
if (!owner || !name) return null
const isKnownHost = hostname in HOST_ENUM
let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1)
let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '')
Comment on lines +71 to +73
Comment on lines +72 to +73
Comment on lines +71 to +73
Comment on lines +71 to +73
if (!name || ownerPath.length === 0 || ownerPath.some((seg) => !seg)) return null

if (CASE_INSENSITIVE_HOSTS.has(hostname)) {
owner = owner.toLowerCase()
ownerPath = ownerPath.map((seg) => seg.toLowerCase())
name = name.toLowerCase()
}

return {
url: `https://${hostname}/${owner}/${name}`,
url: `https://${hostname}/${[...ownerPath, name].join('/')}`,
host: HOST_ENUM[hostname] ?? 'other',
Comment on lines +72 to 83
Comment thread
mbani01 marked this conversation as resolved.
}
Comment thread
mbani01 marked this conversation as resolved.
}
Loading