Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
16 changes: 14 additions & 2 deletions apps/web/src/components/security-agent/SecurityAgentContext.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';

import { createContext, useContext, useState, useCallback, useMemo } from 'react';
import { createContext, useContext, useState, useCallback, useMemo, useRef } from 'react';
import { useTRPC } from '@/lib/trpc/utils';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { toast } from 'sonner';
Expand Down Expand Up @@ -121,6 +121,7 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen

const [startingAnalysisIds, setStartingAnalysisIds] = useState<Set<string>>(new Set());
const [gitHubError, setGitHubError] = useState<string | null>(null);
const toggleEnabledInFlightRef = useRef(false);

// Permission status query
const { data: permissionData, isLoading: isLoadingPermission } = useQuery(
Expand Down Expand Up @@ -217,6 +218,9 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
onError: error => {
toast.error('Failed to toggle Security Agent', { description: error.message });
},
onSettled: () => {
toggleEnabledInFlightRef.current = false;
},
})
);

Expand Down Expand Up @@ -330,6 +334,9 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
onError: error => {
toast.error('Failed to toggle Security Agent', { description: error.message });
},
onSettled: () => {
toggleEnabledInFlightRef.current = false;
},
})
);

Expand Down Expand Up @@ -472,10 +479,15 @@ export function SecurityAgentProvider({ organizationId, children }: SecurityAgen
selectedRepositoryIds: number[];
}
) => {
if (toggleEnabledInFlightRef.current) return;
toggleEnabledInFlightRef.current = true;

if (isOrg && organizationId) {
orgSetEnabledMutate({ organizationId, isEnabled: enabled, ...repositorySelection });
} else {
} else if (!isOrg) {
personalSetEnabledMutate({ isEnabled: enabled, ...repositorySelection });
} else {
toggleEnabledInFlightRef.current = false;
}
},
[isOrg, organizationId, orgSetEnabledMutate, personalSetEnabledMutate]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';

import { useState, useCallback, useMemo } from 'react';
import { useState, useCallback, useMemo, useRef } from 'react';
import { Tabs, TabsContent, TabsList, TabsTrigger } from '@/components/ui/tabs';
import { Alert, AlertDescription, AlertTitle } from '@/components/ui/alert';
import { Button } from '@/components/ui/button';
Expand Down Expand Up @@ -56,6 +56,7 @@ export function SecurityAgentPageClient({ organizationId }: SecurityAgentPageCli
const [dismissDialogOpen, setDismissDialogOpen] = useState(false);
const [startingAnalysisIds, setStartingAnalysisIds] = useState<Set<string>>(new Set());
const [gitHubError, setGitHubError] = useState<string | null>(null);
const toggleEnabledInFlightRef = useRef(false);
const [sortBy, setSortBy] = useState<'severity_desc' | 'severity_asc' | 'sla_due_at_asc'>(
'severity_desc'
);
Expand Down Expand Up @@ -223,6 +224,9 @@ export function SecurityAgentPageClient({ organizationId }: SecurityAgentPageCli
onError: error => {
toast.error('Failed to toggle Security Agent', { description: error.message });
},
onSettled: () => {
toggleEnabledInFlightRef.current = false;
},
})
);

Expand Down Expand Up @@ -292,6 +296,9 @@ export function SecurityAgentPageClient({ organizationId }: SecurityAgentPageCli
onError: error => {
toast.error('Failed to toggle Security Agent', { description: error.message });
},
onSettled: () => {
toggleEnabledInFlightRef.current = false;
},
})
);

Expand Down Expand Up @@ -525,6 +532,9 @@ export function SecurityAgentPageClient({ organizationId }: SecurityAgentPageCli
selectedRepositoryIds: number[];
}
) => {
if (toggleEnabledInFlightRef.current) return;
toggleEnabledInFlightRef.current = true;

if (isOrg && organizationId) {
orgSetEnabledMutate({
organizationId,
Expand All @@ -538,6 +548,8 @@ export function SecurityAgentPageClient({ organizationId }: SecurityAgentPageCli
repositorySelectionMode: repositorySelection.repositorySelectionMode,
selectedRepositoryIds: repositorySelection.selectedRepositoryIds,
});
} else {
toggleEnabledInFlightRef.current = false;
}
},
[isOrg, organizationId, orgSetEnabledMutate, personalSetEnabledMutate]
Expand Down
157 changes: 156 additions & 1 deletion apps/web/src/lib/security-agent/db/security-findings.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from '@jest/globals';
import { db } from '@/lib/drizzle';
import { db, pool } from '@/lib/drizzle';
import { security_findings, agent_configs } from '@kilocode/db/schema';
import { eq, and } from 'drizzle-orm';
import { insertTestUser } from '@/tests/helpers/user.helper';
Expand Down Expand Up @@ -137,6 +137,161 @@ describe('upsertSecurityFinding', () => {
expect(row.severity).toBe('critical');
});

it('returns the same row for concurrent first upserts on the same source key', async () => {
const user = await insertTestUser();
const owner: SecurityReviewOwner = { userId: user.id };
const repo = 'test-org/concurrent-upsert-repo';

const results = await Promise.all(
Array.from({ length: 5 }, () =>
upsertSecurityFinding({
...makeFinding({ source_id: '11' }),
owner,
repoFullName: repo,
})
)
);

const insertedResults = results.filter(result => result.wasInserted);
const updatedResults = results.filter(result => !result.wasInserted);

expect(new Set(results.map(result => result.findingId)).size).toBe(1);
expect(insertedResults).toHaveLength(1);
expect(insertedResults[0]?.previousStatus).toBeNull();
expect(updatedResults.map(result => result.previousStatus)).toEqual([
'open',
'open',
'open',
'open',
]);
expect(updatedResults.map(result => result.effectiveStatus)).toEqual([
'open',
'open',
'open',
'open',
]);

const rows = await db
.select()
.from(security_findings)
.where(
and(
eq(security_findings.repo_full_name, repo),
eq(security_findings.source, 'dependabot'),
eq(security_findings.source_id, '11')
)
);

expect(rows).toHaveLength(1);
});

it('does not let a stale first-insert racer overwrite the winner', async () => {
const user = await insertTestUser();
const owner: SecurityReviewOwner = { userId: user.id };
const repo = 'test-org/stale-first-insert-race-repo';
const sourceId = '13';
const client = await pool.connect();

try {
await client.query('BEGIN');
await client.query(
`INSERT INTO security_findings (
owned_by_user_id,
repo_full_name,
source,
source_id,
severity,
package_name,
package_ecosystem,
title,
status,
fixed_at,
raw_data
) VALUES ($1, $2, 'dependabot', $3, 'critical', 'lodash', 'npm', 'Prototype Pollution in lodash', 'fixed', $4, $5::jsonb)`,
[
user.id,
repo,
sourceId,
'2026-01-16T00:00:00.000Z',
JSON.stringify({ ...rawDependabotAlertFixture, state: 'fixed' }),
]
);

const staleUpsert = upsertSecurityFinding({
...makeFinding({ source_id: sourceId, status: 'open', severity: 'high' }),
owner,
repoFullName: repo,
});

await new Promise(resolve => setTimeout(resolve, 50));
await client.query('COMMIT');

const result = await staleUpsert;

expect(result.wasInserted).toBe(false);
expect(result.previousStatus).toBe('fixed');
expect(result.effectiveStatus).toBe('fixed');

const [row] = await db
.select()
.from(security_findings)
.where(
and(
eq(security_findings.repo_full_name, repo),
eq(security_findings.source, 'dependabot'),
eq(security_findings.source_id, sourceId)
)
);

expect(row.status).toBe('fixed');
expect(row.severity).toBe('critical');
} finally {
await client.query('ROLLBACK').catch(() => undefined);
client.release();
}
});

it('preserves superseded status fields while refreshing sync metadata', async () => {
const user = await insertTestUser();
const owner: SecurityReviewOwner = { userId: user.id };
const repo = 'test-org/superseded-preserve-repo';

const first = await upsertSecurityFinding({
...makeFinding({ source_id: '12' }),
owner,
repoFullName: repo,
});

await db
.update(security_findings)
.set({
status: 'ignored',
ignored_reason: `superseded:${first.findingId}`,
ignored_by: 'system',
})
.where(eq(security_findings.id, first.findingId));

const second = await upsertSecurityFinding({
...makeFinding({ source_id: '12', status: 'open', severity: 'critical' }),
owner,
repoFullName: repo,
});

expect(second.wasInserted).toBe(false);
expect(second.previousStatus).toBe('ignored');
expect(second.effectiveStatus).toBe('ignored');

const [row] = await db
.select()
.from(security_findings)
.where(eq(security_findings.id, first.findingId));

expect(row.status).toBe('ignored');
expect(row.ignored_reason).toBe(`superseded:${first.findingId}`);
expect(row.ignored_by).toBe('system');
expect(row.severity).toBe('critical');
});

it('uses repo_full_name + source + source_id as the unique key', async () => {
const user = await insertTestUser();
const owner: SecurityReviewOwner = { userId: user.id };
Expand Down
Loading
Loading