Skip to content

Commit 40c22d3

Browse files
jeanduplessisjeanduplessis
committed
fix(security-agent): address worker review feedback
1 parent fca33cf commit 40c22d3

4 files changed

Lines changed: 220 additions & 33 deletions

File tree

‎apps/web/src/components/security-agent/SecurityAgentContext.tsx‎

Lines changed: 111 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,24 @@ function getOptionalStringField(source: unknown, key: string): string | undefine
110110
return typeof value === 'string' ? value : undefined;
111111
}
112112

113-
const ACCEPTED_QUEUE_REFRESH_DELAYS_MS = [1000, 5000, 15000] as const;
113+
const ACCEPTED_QUEUE_POLL_INTERVAL_MS = 3000;
114+
const ACCEPTED_QUEUE_POLL_TIMEOUT_MS = 18000;
115+
116+
function listFindingsDataHasActiveAnalysis(data: unknown): boolean {
117+
if (typeof data !== 'object' || data === null) return false;
118+
119+
const runningCount = Reflect.get(data, 'runningCount');
120+
if (typeof runningCount === 'number' && runningCount > 0) return true;
121+
122+
const findings = Reflect.get(data, 'findings');
123+
if (!Array.isArray(findings)) return false;
124+
125+
return findings.some(finding => {
126+
if (typeof finding !== 'object' || finding === null) return false;
127+
const analysisStatus = Reflect.get(finding, 'analysis_status');
128+
return analysisStatus === 'pending' || analysisStatus === 'running';
129+
});
130+
}
114131

115132
type SecurityAgentProviderProps = {
116133
organizationId?: string;
@@ -125,30 +142,96 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
125142
const [startingAnalysisIds, setStartingAnalysisIds] = useState<Set<string>>(new Set());
126143
const [gitHubError, setGitHubError] = useState<string | null>(null);
127144
const toggleEnabledInFlightRef = useRef(false);
128-
const acceptedQueueRefreshTimersRef = useRef<number[]>([]);
145+
const acceptedQueuePollRef = useRef<{ intervalId: number; timeoutId: number } | null>(null);
146+
147+
const clearAcceptedQueuePoll = useCallback(() => {
148+
const activePoll = acceptedQueuePollRef.current;
149+
if (!activePoll) return;
150+
window.clearInterval(activePoll.intervalId);
151+
window.clearTimeout(activePoll.timeoutId);
152+
acceptedQueuePollRef.current = null;
153+
}, []);
154+
155+
useEffect(() => clearAcceptedQueuePoll, [clearAcceptedQueuePoll]);
156+
157+
const invalidateAcceptedQueueQueries = useCallback(() => {
158+
if (isOrg && organizationId) {
159+
const ownerInput = { organizationId };
160+
void Promise.all([
161+
queryClient.invalidateQueries({
162+
queryKey: trpc.organizations.securityAgent.listFindings.queryKey(ownerInput),
163+
}),
164+
queryClient.invalidateQueries({
165+
queryKey: trpc.organizations.securityAgent.getFinding.queryKey(ownerInput),
166+
}),
167+
queryClient.invalidateQueries({
168+
queryKey: trpc.organizations.securityAgent.getAnalysis.queryKey(ownerInput),
169+
}),
170+
queryClient.invalidateQueries({
171+
queryKey: trpc.organizations.securityAgent.getStats.queryKey(ownerInput),
172+
}),
173+
queryClient.invalidateQueries({
174+
queryKey: trpc.organizations.securityAgent.getDashboardStats.queryKey(ownerInput),
175+
}),
176+
queryClient.invalidateQueries({
177+
queryKey: trpc.organizations.securityAgent.getLastSyncTime.queryKey(ownerInput),
178+
}),
179+
queryClient.invalidateQueries({
180+
queryKey: trpc.organizations.securityAgent.getRepositories.queryKey(ownerInput),
181+
}),
182+
queryClient.invalidateQueries({
183+
queryKey: trpc.organizations.securityAgent.getOrphanedRepositories.queryKey(ownerInput),
184+
}),
185+
queryClient.invalidateQueries({
186+
queryKey: trpc.organizations.securityAgent.getAutoDismissEligible.queryKey(ownerInput),
187+
}),
188+
]);
189+
return;
190+
}
129191

130-
useEffect(
131-
() => () => {
132-
for (const timer of acceptedQueueRefreshTimersRef.current) {
133-
window.clearTimeout(timer);
192+
void Promise.all([
193+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.listFindings.queryKey() }),
194+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getFinding.queryKey() }),
195+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getAnalysis.queryKey() }),
196+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getStats.queryKey() }),
197+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getDashboardStats.queryKey() }),
198+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getLastSyncTime.queryKey() }),
199+
queryClient.invalidateQueries({ queryKey: trpc.securityAgent.getRepositories.queryKey() }),
200+
queryClient.invalidateQueries({
201+
queryKey: trpc.securityAgent.getOrphanedRepositories.queryKey(),
202+
}),
203+
queryClient.invalidateQueries({
204+
queryKey: trpc.securityAgent.getAutoDismissEligible.queryKey(),
205+
}),
206+
]);
207+
}, [isOrg, organizationId, queryClient, trpc]);
208+
209+
const cachedListFindingsHasActiveAnalysis = useCallback(() => {
210+
const queryKey = isOrg
211+
? trpc.organizations.securityAgent.listFindings.queryKey(
212+
organizationId ? { organizationId } : undefined
213+
)
214+
: trpc.securityAgent.listFindings.queryKey();
215+
216+
return queryClient
217+
.getQueriesData({ queryKey })
218+
.some(([, data]) => listFindingsDataHasActiveAnalysis(data));
219+
}, [isOrg, organizationId, queryClient, trpc]);
220+
221+
const pollAcceptedQueueMutation = useCallback(() => {
222+
clearAcceptedQueuePoll();
223+
invalidateAcceptedQueueQueries();
224+
225+
const intervalId = window.setInterval(() => {
226+
invalidateAcceptedQueueQueries();
227+
if (cachedListFindingsHasActiveAnalysis()) {
228+
clearAcceptedQueuePoll();
134229
}
135-
acceptedQueueRefreshTimersRef.current = [];
136-
},
137-
[]
138-
);
230+
}, ACCEPTED_QUEUE_POLL_INTERVAL_MS);
139231

140-
const refreshAcceptedQueueMutation = useCallback(() => {
141-
void queryClient.invalidateQueries();
142-
for (const delay of ACCEPTED_QUEUE_REFRESH_DELAYS_MS) {
143-
const timer = window.setTimeout(() => {
144-
void queryClient.invalidateQueries();
145-
acceptedQueueRefreshTimersRef.current = acceptedQueueRefreshTimersRef.current.filter(
146-
pendingTimer => pendingTimer !== timer
147-
);
148-
}, delay);
149-
acceptedQueueRefreshTimersRef.current.push(timer);
150-
}
151-
}, [queryClient]);
232+
const timeoutId = window.setTimeout(clearAcceptedQueuePoll, ACCEPTED_QUEUE_POLL_TIMEOUT_MS);
233+
acceptedQueuePollRef.current = { intervalId, timeoutId };
234+
}, [cachedListFindingsHasActiveAnalysis, clearAcceptedQueuePoll, invalidateAcceptedQueueQueries]);
152235

153236
// Permission status query
154237
const { data: permissionData, isLoading: isLoadingPermission } = useQuery(
@@ -188,7 +271,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
188271
onSuccess: () => {
189272
setGitHubError(null);
190273
toast.success('Sync queued');
191-
refreshAcceptedQueueMutation();
274+
pollAcceptedQueueMutation();
192275
},
193276
onError: error => {
194277
const message = error instanceof Error ? error.message : String(error);
@@ -209,7 +292,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
209292
trpc.organizations.securityAgent.dismissFinding.mutationOptions({
210293
onSuccess: () => {
211294
toast.success('Finding dismissed');
212-
refreshAcceptedQueueMutation();
295+
pollAcceptedQueueMutation();
213296
},
214297
onError: error => {
215298
toast.error('Failed to dismiss finding', { description: error.message });
@@ -256,7 +339,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
256339
onSuccess: async (_data, variables) => {
257340
setGitHubError(null);
258341
toast.success(manualAnalysisAdmissionCopy.successTitle);
259-
refreshAcceptedQueueMutation();
342+
pollAcceptedQueueMutation();
260343
setStartingAnalysisIds(prev => {
261344
const next = new Set(prev);
262345
next.delete(variables.findingId);
@@ -307,7 +390,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
307390
onSuccess: () => {
308391
setGitHubError(null);
309392
toast.success('Sync queued');
310-
refreshAcceptedQueueMutation();
393+
pollAcceptedQueueMutation();
311394
},
312395
onError: error => {
313396
const message = error instanceof Error ? error.message : String(error);
@@ -328,7 +411,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
328411
trpc.securityAgent.dismissFinding.mutationOptions({
329412
onSuccess: () => {
330413
toast.success('Finding dismissed');
331-
refreshAcceptedQueueMutation();
414+
pollAcceptedQueueMutation();
332415
},
333416
onError: error => {
334417
toast.error('Failed to dismiss finding', { description: error.message });
@@ -375,7 +458,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
375458
onSuccess: async (_data, variables) => {
376459
setGitHubError(null);
377460
toast.success(manualAnalysisAdmissionCopy.successTitle);
378-
refreshAcceptedQueueMutation();
461+
pollAcceptedQueueMutation();
379462
setStartingAnalysisIds(prev => {
380463
const next = new Set(prev);
381464
next.delete(variables.findingId);

‎packages/worker-utils/src/security-auto-analysis-policy.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,14 @@ export type AutoAnalysisEligibilityDecision = {
1919
boundarySkipped: boolean;
2020
};
2121

22-
const LOW_SEVERITY_RANK = 3;
22+
const LOWEST_SEVERITY_RANK = 3;
23+
const ALL_SEVERITIES_MAX_RANK = LOWEST_SEVERITY_RANK;
2324

2425
const SEVERITY_RANKS = {
2526
critical: 0,
2627
high: 1,
2728
medium: 2,
28-
low: LOW_SEVERITY_RANK,
29+
low: LOWEST_SEVERITY_RANK,
2930
} as const satisfies Record<string, AutoAnalysisSeverityRank>;
3031

3132
type KnownSeverity = keyof typeof SEVERITY_RANKS;
@@ -34,7 +35,7 @@ const MIN_SEVERITY_MAX_RANKS = {
3435
critical: SEVERITY_RANKS.critical,
3536
high: SEVERITY_RANKS.high,
3637
medium: SEVERITY_RANKS.medium,
37-
all: LOW_SEVERITY_RANK,
38+
all: ALL_SEVERITIES_MAX_RANK,
3839
} as const satisfies Record<AutoAnalysisMinSeverity, AutoAnalysisSeverityRank>;
3940

4041
function isKnownSeverity(severity: string): severity is KnownSeverity {
@@ -53,7 +54,7 @@ export function decideAutoAnalysisEligibility(
5354
params: AutoAnalysisEligibilityParams
5455
): AutoAnalysisEligibilityDecision {
5556
const normalizedSeverityRank = getSeverityRank(params.findingSeverity);
56-
const severityRank = normalizedSeverityRank ?? LOW_SEVERITY_RANK;
57+
const severityRank = normalizedSeverityRank ?? LOWEST_SEVERITY_RANK;
5758
const boundarySkipped =
5859
!params.autoAnalysisIncludeExisting &&
5960
params.autoAnalysisEnabledAt !== null &&

‎services/security-sync/src/index.test.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,67 @@ describe('manual sync dispatch', () => {
179179
expect(retry).not.toHaveBeenCalled();
180180
});
181181

182+
it('accepts legacy OAuth user IDs in manual sync commands', async () => {
183+
const queuedBatches: MessageSendRequest<SecuritySyncQueueMessage>[][] = [];
184+
const legacyUserId = 'oauth:google:1234567890';
185+
const response = await worker.fetch(
186+
new Request('https://security-sync.test/internal/manual-sync', {
187+
method: 'POST',
188+
headers: {
189+
'content-type': 'application/json',
190+
'x-internal-api-key': 'worker-secret',
191+
},
192+
body: JSON.stringify({
193+
schemaVersion: 1,
194+
owner: { userId: legacyUserId },
195+
actor: {
196+
id: legacyUserId,
197+
email: 'owner@example.com',
198+
name: 'Owner Example',
199+
},
200+
}),
201+
}),
202+
{
203+
INTERNAL_API_SECRET: { get: async () => 'worker-secret' },
204+
SYNC_QUEUE: {
205+
sendBatch: async batch => {
206+
queuedBatches.push(batch);
207+
},
208+
},
209+
} as CloudflareEnv
210+
);
211+
212+
expect(response.status).toBe(202);
213+
expect(queuedBatches[0]?.[0]?.body).toMatchObject({
214+
trigger: 'manual',
215+
owner: { userId: legacyUserId },
216+
ownerKey: `user:${legacyUserId}`,
217+
actor: {
218+
id: legacyUserId,
219+
},
220+
});
221+
222+
vi.mocked(getWorkerDb).mockReturnValue({} as never);
223+
vi.mocked(syncOwner).mockResolvedValue({ synced: 1, errors: 0, staleRepos: 0 } as never);
224+
const ack = vi.fn();
225+
const retry = vi.fn();
226+
await worker.queue(
227+
{ messages: [{ body: queuedBatches[0]?.[0]?.body, ack, retry }] } as never,
228+
{
229+
HYPERDRIVE: { connectionString: 'postgres://worker' },
230+
GIT_TOKEN_SERVICE: {},
231+
} as CloudflareEnv
232+
);
233+
234+
expect(syncOwner).toHaveBeenCalledWith(
235+
expect.objectContaining({
236+
owner: { userId: legacyUserId },
237+
})
238+
);
239+
expect(ack).toHaveBeenCalledTimes(1);
240+
expect(retry).not.toHaveBeenCalled();
241+
});
242+
182243
it('rejects migrated sync traffic when Worker command routing is paused', async () => {
183244
const response = await worker.fetch(
184245
new Request('https://security-sync.test/internal/manual-sync', { method: 'POST' }),
@@ -240,6 +301,48 @@ describe('manual dismissal dispatch', () => {
240301
});
241302
});
242303

304+
it('accepts legacy OAuth user IDs in dismissal commands', async () => {
305+
const queuedBatches: MessageSendRequest<unknown>[][] = [];
306+
const legacyUserId = 'oauth:google:1234567890';
307+
const response = await worker.fetch(
308+
new Request('https://security-sync.test/internal/dismiss-finding', {
309+
method: 'POST',
310+
headers: {
311+
'content-type': 'application/json',
312+
'x-internal-api-key': 'worker-secret',
313+
},
314+
body: JSON.stringify({
315+
schemaVersion: 1,
316+
owner: { userId: legacyUserId },
317+
actor: {
318+
id: legacyUserId,
319+
email: 'owner@example.com',
320+
name: 'Owner Example',
321+
},
322+
findingId: 'bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb',
323+
installationId: 'installation-123',
324+
reason: 'not_used',
325+
comment: 'No production usage',
326+
}),
327+
}),
328+
{
329+
INTERNAL_API_SECRET: { get: async () => 'worker-secret' },
330+
SYNC_QUEUE: {
331+
sendBatch: async batch => {
332+
queuedBatches.push(batch);
333+
},
334+
},
335+
} as CloudflareEnv
336+
);
337+
338+
expect(response.status).toBe(202);
339+
expect(queuedBatches[0]?.[0]?.body).toMatchObject({
340+
kind: 'dismiss',
341+
owner: { userId: legacyUserId },
342+
actor: { id: legacyUserId, email: 'owner@example.com', name: 'Owner Example' },
343+
});
344+
});
345+
243346
it('rejects migrated dismissal traffic when Worker command routing is paused', async () => {
244347
const response = await worker.fetch(
245348
new Request('https://security-sync.test/internal/dismiss-finding', { method: 'POST' }),

‎services/security-sync/src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { processSecurityFindingDismissal } from './dismiss';
99
const SecuritySyncOwnerSchema = z
1010
.object({
1111
organizationId: z.string().uuid().optional(),
12-
userId: z.string().uuid().optional(),
12+
userId: z.string().min(1).optional(),
1313
})
1414
.refine(value => Boolean(value.organizationId || value.userId), {
1515
message: 'owner.organizationId or owner.userId is required',

0 commit comments

Comments
 (0)