Skip to content

Commit 8b26e72

Browse files
committed
fix: address critical bugs from comprehensive audit
- Fix array mutation in sortedImports: copy array before sorting to prevent side effects when getter is called multiple times - Fix race condition in batch save: explicitly open documents before saving to ensure files not already open are properly saved - Add ReDoS protection: detect potentially dangerous regex patterns with nested quantifiers and warn users - Add invalid regex handling: try-catch around RegExp compilation with graceful fallback to never-matching pattern Fixes found during comprehensive audit of import grouping, batch operations, and configuration handling.
1 parent ebad5d0 commit 8b26e72

4 files changed

Lines changed: 48 additions & 13 deletions

File tree

src/commands/batch-organizer.ts

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -348,23 +348,28 @@ export class BatchOrganizer {
348348

349349
// CRITICAL: Save all modified documents to disk!
350350
// workspace.applyEdit() only modifies in-memory documents, doesn't save!
351+
// NOTE: We must explicitly open each document because files that weren't already
352+
// open may not appear in workspace.textDocuments (race condition fix).
351353
const saveFailed: string[] = [];
352-
for (const [fileUriStr] of workspaceEdit.entries()) {
353-
const doc = workspace.textDocuments.find(d => d.uri.fsPath === fileUriStr.fsPath);
354-
if (doc && !doc.isUntitled && doc.isDirty) {
355-
try {
354+
let savedCount = 0;
355+
for (const [fileUri] of workspaceEdit.entries()) {
356+
try {
357+
// Explicitly open the document to ensure it's in memory
358+
const doc = await workspace.openTextDocument(fileUri);
359+
if (!doc.isUntitled && doc.isDirty) {
356360
await doc.save();
357-
} catch (saveError) {
358-
saveFailed.push(doc.fileName);
359-
errors++;
360-
this.logger.appendLine(`[BatchOrganizer] Failed to save ${doc.fileName}: ${saveError}`);
361+
savedCount++;
361362
}
363+
} catch (saveError) {
364+
saveFailed.push(fileUri.fsPath);
365+
errors++;
366+
this.logger.appendLine(`[BatchOrganizer] Failed to save ${fileUri.fsPath}: ${saveError}`);
362367
}
363368
}
364369
if (saveFailed.length > 0) {
365370
window.showWarningMessage(`Mini TypeScript Hero: Failed to save ${saveFailed.length} file(s). Check output for details.`);
366371
}
367-
this.logger.appendLine(`[BatchOrganizer] Saved ${processed - saveFailed.length} files to disk`);
372+
this.logger.appendLine(`[BatchOrganizer] Saved ${savedCount} files to disk`);
368373
} else {
369374
window.showErrorMessage('Mini TypeScript Hero: Failed to apply some edits');
370375
this.logger.appendLine('[BatchOrganizer] Failed to apply edits');

src/imports/import-grouping/keyword-import-group.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ export class KeywordImportGroup implements ImportGroup {
1111
public readonly imports: Import[] = [];
1212

1313
public get sortedImports(): Import[] {
14-
return this.imports.sort((i1, i2) => importSort(i1, i2, this.order));
14+
// IMPORTANT: Copy array before sorting to avoid mutating the original
15+
return [...this.imports].sort((i1, i2) => importSort(i1, i2, this.order));
1516
}
1617

1718
constructor(

src/imports/import-grouping/regex-import-group.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,17 @@ import { importSort } from '../import-utilities';
33
import { ImportGroup } from './import-group';
44
import { ImportGroupOrder } from './import-group-order';
55

6+
/**
7+
* Detects potentially dangerous regex patterns that could cause catastrophic backtracking (ReDoS).
8+
* Checks for nested quantifiers like (a+)+, (a*)+, etc.
9+
*/
10+
function isPotentiallyDangerousRegex(pattern: string): boolean {
11+
// Detect nested quantifiers: (pattern)+ or (pattern)* where pattern contains + or *
12+
// This is a simple heuristic, not comprehensive
13+
const nestedQuantifierPattern = /\([^)]*[+*][^)]*\)[+*]/;
14+
return nestedQuantifierPattern.test(pattern);
15+
}
16+
617
/**
718
* Import group that processes all imports that match a certain regex (the lib name).
819
*/
@@ -11,7 +22,8 @@ export class RegexImportGroup implements ImportGroup {
1122
private readonly compiledRegex: RegExp;
1223

1324
public get sortedImports(): Import[] {
14-
const sorted = this.imports.sort((i1, i2) =>
25+
// IMPORTANT: Copy array before sorting to avoid mutating the original
26+
const sorted = [...this.imports].sort((i1, i2) =>
1527
importSort(i1, i2, this.order),
1628
);
1729
return [
@@ -39,7 +51,23 @@ export class RegexImportGroup implements ImportGroup {
3951
regexString = regexString.endsWith('/')
4052
? regexString.substring(0, regexString.length - 1)
4153
: regexString;
42-
this.compiledRegex = new RegExp(regexString);
54+
55+
// Check for potentially dangerous regex patterns (ReDoS protection)
56+
// Note: We just log to debug console, which is visible in Extension Development Host
57+
if (isPotentiallyDangerousRegex(regexString)) {
58+
// eslint-disable-next-line no-console
59+
console.warn(`[RegexImportGroup] Warning: Potentially dangerous regex pattern detected: ${this.regex}`);
60+
}
61+
62+
// Try to compile regex, use fallback if invalid
63+
try {
64+
this.compiledRegex = new RegExp(regexString);
65+
} catch {
66+
// Invalid regex - use a pattern that never matches
67+
// eslint-disable-next-line no-console
68+
console.error(`[RegexImportGroup] Invalid regex pattern: ${this.regex}`);
69+
this.compiledRegex = /(?!)/; // Never matches
70+
}
4371
}
4472

4573
public reset(): void {

src/imports/import-grouping/remain-import-group.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ export class RemainImportGroup implements ImportGroup {
1010
public readonly imports: Import[] = [];
1111

1212
public get sortedImports(): Import[] {
13-
const sorted = this.imports.sort((i1, i2) =>
13+
// IMPORTANT: Copy array before sorting to avoid mutating the original
14+
const sorted = [...this.imports].sort((i1, i2) =>
1415
importSort(i1, i2, this.order),
1516
);
1617
return [

0 commit comments

Comments
 (0)