-
Notifications
You must be signed in to change notification settings - Fork 731
feat(security-contacts): emails reachability & noise filtering [CM-1324] #4353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
1183504
0597438
0228f92
22339ca
9a2c12c
9db3785
3ce2401
f671ff7
d1e868b
8de3130
9b501d8
a3ff9fd
b063400
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ALTER TABLE security_contacts ADD COLUMN IF NOT EXISTS reachable BOOLEAN NOT NULL DEFAULT TRUE; | ||
|
|
||
| ALTER TABLE security_contacts ADD COLUMN IF NOT EXISTS reachability_reason TEXT; | ||
|
cursor[bot] marked this conversation as resolved.
Comment on lines
+1
to
+2
Comment on lines
+1
to
+2
Comment on lines
+1
to
+2
Comment on lines
+1
to
+2
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- Supports fetchBatch's eligibility WHERE clause (contacts_last_refreshed IS NULL, | ||
| -- and the daily/weekly age comparisons), which previously had no supporting index. | ||
| CREATE INDEX IF NOT EXISTS repos_contacts_last_refreshed_idx | ||
| ON repos (contacts_last_refreshed); | ||
|
Comment on lines
+3
to
+4
Comment on lines
+3
to
+4
Comment on lines
+3
to
+4
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,16 +16,17 @@ import { extractSecurityMd } from './extractors/securityMd' | |
| import { extractSecurityTxt } from './extractors/securityTxt' | ||
| import { githubApiGet } from './githubToken' | ||
| import { reconcile } from './reconcile' | ||
| import { resolveCdpEmails } from './resolveCdpEmails' | ||
| import { deriveGithubHandlesFromNoreplyEmails, resolveCdpEmails } from './resolveCdpEmails' | ||
| import { | ||
| Extractor, | ||
| ExtractorDeps, | ||
| ProcessRepoResult, | ||
| RawContact, | ||
| RepoPackage, | ||
| RepoPolicies, | ||
| RepoTarget, | ||
| } from './types' | ||
| import { markRepoAttempted, writeContacts } from './writeContacts' | ||
| import { writeContactsBatch } from './writeContacts' | ||
|
|
||
| const log = getServiceChildLogger('security-contacts') | ||
|
|
||
|
|
@@ -120,9 +121,8 @@ export function buildBaseDeps(config: Config): Omit<ExtractorDeps, 'repoTree'> { | |
| export async function processRepo( | ||
| target: RepoTarget, | ||
| baseDeps: Omit<ExtractorDeps, 'repoTree'>, | ||
| qx: QueryExecutor, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweep ignores reachable-only contactsMedium Severity The batch sweep treats any non-deleted Reviewed by Cursor Bugbot for commit 8de3130. Configure here. |
||
| cdpQx: QueryExecutor, | ||
| ): Promise<void> { | ||
| ): Promise<ProcessRepoResult> { | ||
| // One tree fetch per repo, shared by extractors that probe well-known paths. | ||
| let repoTree: ExtractorDeps['repoTree'] = { paths: null } | ||
| try { | ||
|
|
@@ -142,8 +142,7 @@ export async function processRepo( | |
| { repoId: target.repoId, errMsg: failed.reason?.message }, | ||
| 'Extractor failed — preserving existing data', | ||
| ) | ||
| await markRepoAttempted(qx, target.repoId) | ||
| return | ||
| return { repoId: target.repoId, status: 'extractor-failed' } | ||
| } | ||
|
|
||
| let contacts: RawContact[] = [] | ||
|
|
@@ -163,6 +162,8 @@ export async function processRepo( | |
| contacts = contacts.filter((c) => c.channel !== 'github-pvr') | ||
| } | ||
|
|
||
| contacts.push(...deriveGithubHandlesFromNoreplyEmails(contacts)) | ||
|
|
||
|
|
||
| const handleContacts = contacts.filter((c) => c.channel === 'github-handle') | ||
| if (handleContacts.length > 0) { | ||
| try { | ||
|
|
@@ -176,7 +177,7 @@ export async function processRepo( | |
| } | ||
|
|
||
| const scored = reconcile(contacts) | ||
| await writeContacts(qx, target.repoId, scored, policies) | ||
| return { repoId: target.repoId, status: 'ok', contacts: scored, policies } | ||
| } | ||
|
|
||
| export async function processBatch( | ||
|
|
@@ -199,26 +200,57 @@ export async function processBatch( | |
| log.warn({ errMsg: (err as Error).message }, 'Heartbeat failed') | ||
| } | ||
| }, 30_000) | ||
| let outcomes: ProcessRepoResult[] | ||
| const extractionStartedAt = Date.now() | ||
| try { | ||
| // A cancelled task (superseded by a newer activity attempt) is left to throw so it stops | ||
| // scheduling further repos instead of racing the new attempt. | ||
| await mapWithConcurrency(targets, CONCURRENCY, async (target) => { | ||
| // | ||
| // Extraction is collected here and persisted afterward in one batched call (below) rather | ||
| // than per-repo, since per-repo writes meant up to CONCURRENCY concurrent transactions | ||
| // against a packages-db pool sized for far fewer connections — the actual sweep bottleneck. | ||
| outcomes = await mapWithConcurrency(targets, CONCURRENCY, async (target) => { | ||
|
Comment on lines
+209
to
+212
|
||
| if (cancellationSignal().aborted) { | ||
| throw new Error( | ||
| 'Security contacts batch cancelled — superseded by a newer activity attempt', | ||
| ) | ||
| } | ||
| try { | ||
| await processRepo(target, deps, qx, cdpQx) | ||
| return await processRepo(target, deps, cdpQx) | ||
| } catch (err) { | ||
| log.error( | ||
| { repoId: target.repoId, errMsg: (err as Error).message }, | ||
| 'Repo processing failed', | ||
| ) | ||
| // Best-effort: keeps a persistently-failing repo from hot-looping the sweep. | ||
| await markRepoAttempted(qx, target.repoId).catch(() => undefined) | ||
| return { repoId: target.repoId, status: 'extractor-failed' as const } | ||
| } | ||
| }) | ||
|
|
||
| const extractionDurationMs = Date.now() - extractionStartedAt | ||
| const ok = outcomes.filter((o) => o.status === 'ok').length | ||
| log.info( | ||
| { | ||
| ok, | ||
| failed: outcomes.length - ok, | ||
| durationMs: extractionDurationMs, | ||
| reposPerSec: Number((outcomes.length / (extractionDurationMs / 1000)).toFixed(1)), | ||
| }, | ||
| 'Security contacts batch extraction complete — persisting', | ||
| ) | ||
|
|
||
| // Heartbeat is kept alive through persistence too: a slow/lock-blocked write can outlast the | ||
| // 2-minute heartbeat timeout just like extraction can, and letting the timer stop early would | ||
| // let Temporal retry this activity while the original attempt is still writing. | ||
| const writeStartedAt = Date.now() | ||
| await writeContactsBatch(qx, outcomes) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cancel drops extracted batch workMedium Severity Batch processing now collects every Additional Locations (1)Reviewed by Cursor Bugbot for commit d1e868b. Configure here. |
||
| const writeDurationMs = Date.now() - writeStartedAt | ||
| log.info( | ||
| { | ||
| durationMs: writeDurationMs, | ||
| reposPerSec: Number((outcomes.length / (writeDurationMs / 1000)).toFixed(1)), | ||
| }, | ||
| 'Security contacts batch persistence complete', | ||
| ) | ||
| } finally { | ||
| clearInterval(heartbeatTimer) | ||
| } | ||
|
Comment on lines
254
to
256
Comment on lines
254
to
256
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { parseGitHubNoreplyEmail } from '@crowd/common' | ||
| import { | ||
| findMembersByGithubHandles, | ||
| findResolvableEmailsForMembers, | ||
|
|
@@ -13,6 +14,28 @@ function latestTimestamp(provenance: ProvenanceEntry[]): string { | |
| : times.reduce((a, b) => (new Date(b).getTime() > new Date(a).getTime() ? b : a)) | ||
| } | ||
|
|
||
| // A GitHub noreply email already encodes its owner's handle — derive it so the handle can be | ||
| // resolved to a real address the same way any other github-handle contact is. The noreply email | ||
| // itself is kept as the provenance source, so the resolved contact's origin stays traceable. | ||
| export function deriveGithubHandlesFromNoreplyEmails(contacts: RawContact[]): RawContact[] { | ||
| const derived: RawContact[] = [] | ||
| for (const c of contacts) { | ||
| if (c.channel !== 'email') continue | ||
| const handle = parseGitHubNoreplyEmail(c.value) | ||
| if (!handle) continue | ||
|
Comment on lines
+24
to
+25
Comment on lines
+23
to
+25
|
||
| derived.push({ | ||
| channel: 'github-handle', | ||
| value: handle, | ||
| role: c.role, | ||
| tier: c.tier, | ||
| provenance: [ | ||
| { source: c.value, sourceTier: c.tier, fetchedAt: latestTimestamp(c.provenance) }, | ||
| ], | ||
| }) | ||
| } | ||
| return derived | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitLab noreply handles not derivedMedium Severity
Reviewed by Cursor Bugbot for commit b063400. Configure here. |
||
| } | ||
|
|
||
| export async function resolveCdpEmails( | ||
| cdpQx: QueryExecutor, | ||
| handleContacts: RawContact[], | ||
|
|
@@ -51,8 +74,9 @@ export async function resolveCdpEmails( | |
| channel: 'email' as const, | ||
| value: email, | ||
| role: contact.role, | ||
| handle: contact.value, | ||
| tier: contact.tier, | ||
| provenance: [{ source, sourceTier: contact.tier, fetchedAt }], | ||
| provenance: [...contact.provenance, { source, sourceTier: contact.tier, fetchedAt }], | ||
|
Comment on lines
+77
to
+79
|
||
| })) | ||
| }) | ||
| }) | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.