Skip to content

Commit e935283

Browse files
committed
Fix race condition in content exclusion that causes intermittent bypass
Concurrent isIgnored() calls could cache stale 'not ignored' results when a content exclusion rule fetch was in progress. This happened because: 1. After yielding at getRepositoryFetchUrls, a concurrent caller could have already started a fetch and seeded the cache with empty patterns. The current caller would skip the fetch (patterns already seeded, lastRuleFetch just set) and match against empty rules. 2. The glob result cache was only cleared at the start of the fetch, so any 'false' entries written during the fetch window persisted until the next 30-minute refresh. Fix: - Re-check _contentExclusionFetchPromise after the getRepositoryFetchUrls yield point and wait for any in-progress fetch before pattern matching. - Clear both glob and regex result caches at the end of the fetch to invalidate stale entries written by concurrent callers. Fixes github/Copilot-Controls#650
1 parent b5dd5ac commit e935283

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

extensions/copilot/src/platform/ignore/node/remoteContentExclusion.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ export class RemoteContentExclusion implements IDisposable {
124124
this._logService.trace(`Fetching content exclusions, due to ${this.shouldFetchContentExclusionRules(repoMetadata) ? 'repository change' : 'stale cache'}.`);
125125
this._lastRuleFetch = Date.now();
126126
await raceCancellationError(this.makeContentExclusionRequest(), token);
127+
} else if (this._contentExclusionFetchPromise) {
128+
// A concurrent caller may have started a fetch while we were awaiting
129+
// getRepositoryFetchUrls above. Wait for it to complete so we match
130+
// against the actual rules instead of the empty seed entries.
131+
await raceCancellationError(this._contentExclusionFetchPromise, token);
127132
}
128133

129134
const minimatchConfig = {
@@ -259,7 +264,9 @@ export class RemoteContentExclusion implements IDisposable {
259264
* Not recommended to call directly and instead use {@link makeContentExclusionRequest} as that ensures only one call is pending at any time
260265
*/
261266
private async _contentExclusionRequest(): Promise<void> {
262-
// Clear the result cache as new rules will come and therefore it is no longer valid
267+
// Clear the result cache as new rules will come and therefore it is no longer valid.
268+
// Note: we clear again at the end of this method to invalidate any stale entries
269+
// written by concurrent isIgnored() calls that ran while the fetch was in flight.
263270
this._ignoreGlobResultCache.clear();
264271
const startTime = Date.now();
265272
const capiClientService = this._capiClientService;
@@ -300,6 +307,10 @@ export class RemoteContentExclusion implements IDisposable {
300307
await updateRulesForRepos(batch);
301308
}
302309
this._lastRuleFetch = Date.now();
310+
// Clear result caches again to invalidate any stale entries written by concurrent
311+
// isIgnored() calls that matched against the empty seed entries during the fetch.
312+
this._ignoreGlobResultCache.clear();
313+
this._ignoreRegexResultCache.clear();
303314
this._logService.info(`Fetched content exclusion rules in ${Date.now() - startTime}ms`);
304315

305316
// Log the fetched rules to the request logger for debugging visibility

extensions/copilot/src/platform/ignore/node/test/remoteContentExclusion.spec.ts

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import { beforeEach, describe, expect, suite, test, vi } from 'vitest';
77
import { CancellationToken } from '../../../../util/vs/base/common/cancellation';
8+
import { Barrier } from '../../../../util/vs/base/common/async';
89
import { URI } from '../../../../util/vs/base/common/uri';
910
import { IAuthenticationService } from '../../../authentication/common/authentication';
1011
import { ICAPIClientService } from '../../../endpoint/common/capiClient';
@@ -232,4 +233,80 @@ suite('RemoteContentExclusion', () => {
232233
expect(mockGitService.getRepositoryFetchUrlsCallCount).toBe(0);
233234
});
234235
});
236+
237+
describe('concurrent isIgnored calls', () => {
238+
test('should not cache false for files that match rules fetched by a concurrent call', async () => {
239+
// This test reproduces the race condition where:
240+
// 1. Call A enters isIgnored, yields at getRepositoryFetchUrls
241+
// 2. Call B enters isIgnored, yields at getRepositoryFetchUrls
242+
// 3. Call A resumes, triggers content exclusion fetch, yields at network request
243+
// 4. Call B resumes, finds empty seed patterns, must NOT cache "false"
244+
const repoRoot = '/workspace/my-repo';
245+
246+
// Use a barrier to control when getRepositoryFetchUrls resolves,
247+
// ensuring both calls are in-flight before either proceeds.
248+
const gitBarrier = new Barrier();
249+
let gitCallCount = 0;
250+
mockGitService.getRepositoryFetchUrls = vi.fn().mockImplementation(() => {
251+
gitCallCount++;
252+
return gitBarrier.wait().then(() => ({
253+
rootUri: URI.file(repoRoot),
254+
remoteFetchUrls: ['https://github.com/org/repo.git']
255+
}));
256+
});
257+
258+
// Set up the CAPI mock to return exclusion rules with a pattern that matches.
259+
// The response is an array — one entry per repo URL requested.
260+
// Use **/keyword/** to match any file inside a keyword/ directory.
261+
mockCAPIClientService.setMockResponse({
262+
ok: true,
263+
json: () => Promise.resolve([{
264+
rules: [{ paths: ['**/keyword/**'], source: { name: 'org', type: 'Organization' } }],
265+
last_updated_at: Date.now()
266+
}]),
267+
} as any);
268+
269+
const fileA = URI.file('/workspace/my-repo/keyword/keyword.py');
270+
const fileB = URI.file('/workspace/my-repo/keyword/extra.py');
271+
272+
// Start both isIgnored calls concurrently (both will block at getRepositoryFetchUrls)
273+
const resultA = remoteContentExclusion.isIgnored(fileA, CancellationToken.None);
274+
const resultB = remoteContentExclusion.isIgnored(fileB, CancellationToken.None);
275+
276+
// Both calls should be waiting at the git barrier
277+
expect(gitCallCount).toBe(2);
278+
279+
// Release the barrier — both calls proceed
280+
gitBarrier.open();
281+
282+
// Both files should be correctly identified as ignored
283+
expect(await resultA).toBe(true);
284+
expect(await resultB).toBe(true);
285+
});
286+
287+
test('should correctly exclude files when rules arrive after concurrent calls start', async () => {
288+
// Similar to above but with the non-git-file path (no repository)
289+
mockGitService.setRepositoryFetchUrls(undefined);
290+
291+
// Use a barrier to control when the CAPI request resolves
292+
const capiBarrier = new Barrier();
293+
mockCAPIClientService.setMockResponse({
294+
ok: true,
295+
json: () => capiBarrier.wait().then(() => [{
296+
rules: [{ paths: ['**/secret/**'], source: { name: 'org', type: 'Organization' } }],
297+
last_updated_at: Date.now()
298+
}]),
299+
} as any);
300+
301+
const secretFile = URI.file('/project/secret/config.py');
302+
303+
// Start isIgnored — it will trigger a fetch that blocks on the capiBarrier
304+
const resultPromise = remoteContentExclusion.isIgnored(secretFile, CancellationToken.None);
305+
306+
// Release CAPI response so rules load
307+
capiBarrier.open();
308+
309+
expect(await resultPromise).toBe(true);
310+
});
311+
});
235312
});

0 commit comments

Comments
 (0)