From 0515c6a3434cc4c7f74da241df6d9985d0c069ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 2 Aug 2026 08:17:26 +0200 Subject: [PATCH 1/2] fix: clear the last polynomial-redos instance in swift-cache sanitizeCacheName used /^-+|-+$/g to trim edge dashes, the same js/polynomial-redos pattern PR #1546 retired everywhere else. Replace it with the linear-time trim used there, and add a counterfactual regression test that fails against the old regex on a long interior dash run. --- src/utils/__tests__/swift-cache.test.ts | 13 ++++++++++++- src/utils/swift-cache.ts | 17 +++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/utils/__tests__/swift-cache.test.ts b/src/utils/__tests__/swift-cache.test.ts index 14cceda11c..915cdb0acc 100644 --- a/src/utils/__tests__/swift-cache.test.ts +++ b/src/utils/__tests__/swift-cache.test.ts @@ -13,7 +13,7 @@ vi.mock('../exec.ts', () => ({ })); import { runCmd } from '../exec.ts'; -import { compileSwiftSourceFile } from '../swift-cache.ts'; +import { compileSwiftSourceFile, sanitizeCacheName } from '../swift-cache.ts'; const mockRunCmd = vi.mocked(runCmd); @@ -100,6 +100,17 @@ test('cache lock timeout reports the lock path', async () => { expect(mockRunCmd).not.toHaveBeenCalled(); }); +test('sanitizeCacheName trims a long interior dash run without polynomial regex backtracking', () => { + const value = `x${'-'.repeat(100_000)}x`; + + const start = Date.now(); + const result = sanitizeCacheName(value); + const elapsedMs = Date.now() - start; + + expect(elapsedMs).toBeLessThan(1_000); + expect(result).toBe(value); +}); + function writeSourceFile(source = 'print("recording")'): string { const sourcePath = path.join(tmpDir, 'recording-overlay.swift'); fs.writeFileSync(sourcePath, source); diff --git a/src/utils/swift-cache.ts b/src/utils/swift-cache.ts index e1ce0c7d5d..e8b87730a5 100644 --- a/src/utils/swift-cache.ts +++ b/src/utils/swift-cache.ts @@ -168,8 +168,21 @@ function isExecutableFile(filePath: string): boolean { } } -function sanitizeCacheName(value: string): string { - return value.replaceAll(/[^A-Za-z0-9._-]/g, '-').replaceAll(/^-+|-+$/g, '') || 'swift-helper'; +export function sanitizeCacheName(value: string): string { + return trimEdgeDashes(value.replaceAll(/[^A-Za-z0-9._-]/g, '-')) || 'swift-helper'; +} + +/** + * Linear-time edge trim. The regex form (`/^-+|-+$/g`) backtracks + * polynomially on long dash runs (CodeQL js/polynomial-redos), and cache + * names are derived from caller-supplied strings. + */ +function trimEdgeDashes(value: string): string { + let start = 0; + let end = value.length; + while (start < end && value[start] === '-') start += 1; + while (end > start && value[end - 1] === '-') end -= 1; + return value.slice(start, end); } function hashParts(parts: Array): string { From 96d7e9b90d1cf6c9039217f3c8d56b296c7bcaa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 2 Aug 2026 09:51:06 +0200 Subject: [PATCH 2/2] fix: keep sanitizeCacheName private, drive redos/fallback pins through compileSwiftSourceText Addresses PR #1549 reviewer feedback: sanitizeCacheName was exported solely so the regression test could import it, which docs/agents/testing.md's test-interface rule forbids. Reverted the export and rewrote the test to exercise the sanitizer through compileSwiftSourceText, an existing production seam that already calls it. - Timing pin: a cache name with a 100k-char interior dash run still resolves in sub-second time (the call may reject once it reaches disk I/O due to the OS path-component length limit, but that happens only after the now-fast sanitize step, so timing the settle either way still proves no catastrophic backtracking). - Fallback pin: a cache name that sanitizes to nothing (e.g. '---') still produces the 'swift-helper' fallback, observed via the returned executable path. Counterfactuals (see PR comment for full output): - Restoring the retired `/^-+|-+$/g` regex trim made the timing pin fail: 3428ms >= 1000ms. - Removing the `|| 'swift-helper'` fallback made the fallback pin fail: basename did not start with 'swift-helper-'. --- src/utils/__tests__/swift-cache.test.ts | 24 ++++++++++++++++++++---- src/utils/swift-cache.ts | 2 +- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/utils/__tests__/swift-cache.test.ts b/src/utils/__tests__/swift-cache.test.ts index 915cdb0acc..9ab475d37b 100644 --- a/src/utils/__tests__/swift-cache.test.ts +++ b/src/utils/__tests__/swift-cache.test.ts @@ -13,7 +13,7 @@ vi.mock('../exec.ts', () => ({ })); import { runCmd } from '../exec.ts'; -import { compileSwiftSourceFile, sanitizeCacheName } from '../swift-cache.ts'; +import { compileSwiftSourceFile, compileSwiftSourceText } from '../swift-cache.ts'; const mockRunCmd = vi.mocked(runCmd); @@ -100,15 +100,31 @@ test('cache lock timeout reports the lock path', async () => { expect(mockRunCmd).not.toHaveBeenCalled(); }); -test('sanitizeCacheName trims a long interior dash run without polynomial regex backtracking', () => { +test('compileSwiftSourceText resolves a cache name with a long interior dash run in sub-second time', async () => { + // Regression pin for the polynomial-regex ReDoS fix in `sanitizeCacheName`'s edge-dash + // trim. The interior dashes are never touched by the trim, so a correct sanitizer never + // needs to inspect this whole run — only a backtracking one pays for its length. const value = `x${'-'.repeat(100_000)}x`; const start = Date.now(); - const result = sanitizeCacheName(value); + // The cache name is long enough to exceed the filesystem's path-component limit, so the + // call is expected to reject once it reaches disk I/O; that happens only *after* the + // (now fast) sanitize step this test pins, so timing the settle either way still proves + // no catastrophic backtracking occurred. + await compileSwiftSourceText({ source: 'print(1)', cacheName: value }).catch(() => {}); const elapsedMs = Date.now() - start; expect(elapsedMs).toBeLessThan(1_000); - expect(result).toBe(value); +}); + +test('compileSwiftSourceText falls back to swift-helper when the cache name sanitizes to nothing', async () => { + const executablePath = await compileSwiftSourceText({ + source: 'print(1)', + cacheName: '---', + }); + + expect(path.basename(executablePath).startsWith('swift-helper-')).toBe(true); + expect(fs.statSync(executablePath).mode & 0o111).not.toBe(0); }); function writeSourceFile(source = 'print("recording")'): string { diff --git a/src/utils/swift-cache.ts b/src/utils/swift-cache.ts index e8b87730a5..a111f26e05 100644 --- a/src/utils/swift-cache.ts +++ b/src/utils/swift-cache.ts @@ -168,7 +168,7 @@ function isExecutableFile(filePath: string): boolean { } } -export function sanitizeCacheName(value: string): string { +function sanitizeCacheName(value: string): string { return trimEdgeDashes(value.replaceAll(/[^A-Za-z0-9._-]/g, '-')) || 'swift-helper'; }