fix: add explicit UTF-8 encoding across all Electron main process I/O (#1554)

* fix: add explicit UTF-8 encoding across all Electron main process I/O Ensure cross-platform compatibility (Windows/macOS/Linux) by making all text encoding explicit rather than relying on platform defaults. - Add 'utf8' to ~50 Buffer.toString() calls on child process stdout/stderr - Add 'utf-8' to ~45 writeFileSync/writeFile calls writing text/JSON data - Add PYTHONIOENCODING and PYTHONUTF8 env vars to Python subprocess spawns - Add encoding option to execSync/execFileSync calls in python-detector Co-Authored-By: Claude Opus 4.5 <[email protected]> * style: standardize encoding string to 'utf-8' everywhere Replace all toString('utf8') with toString('utf-8') across 23 files to use one consistent encoding format. Both are valid Node.js aliases but 'utf-8' is the canonical IANA name and matches the writeFileSync encoding parameter already used throughout the codebase. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): resolve 16 CodeQL alerts (TOCTOU + network data sanitization) Fix 9 TOCTOU race conditions by replacing existsSync() guards with try/catch around readFileSync, handling ENOENT in catch blocks. Fix 7 network-data-to-file alerts by sanitizing GitHub/Linear API data before writing to disk. Extract shared sanitization module from gitlab/spec-utils.ts for reuse across integrations. Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(frontend): resolve 7 PR review findings for code quality - Add console.error logging to empty catch blocks in crud-handlers.ts (lines 349, 379) that silently swallowed non-ENOENT errors - Add missing 'utf-8' encoding to writeFileSync in roadmap-handlers.ts:680 - Consolidate gitlab/triage-handlers.ts sanitization functions to use shared/sanitize.ts module, removing duplicate local implementations - Remove redundant .toString() call in python-env-manager.ts:255 since execSync with encoding: 'utf-8' already returns a string - Fix TOCTOU race in getFeatureSettings() by removing existsSync check and handling ENOENT in catch block - Add comprehensive test suite for shared/sanitize.ts with 46 tests covering control chars, Unicode, URLs with credentials/javascript:/data: Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
2026-02-09 12:31:33 +02:00
parent cabca2dcb0
commit 6fabdf59b1
11 changed files with 46 additions and 134 deletions
@@ -60,9 +60,8 @@ export function getWritablePath(originalPath: string, filename: string): string

  try {
    if (fs.existsSync(dir)) {
-      // Try to write a test file using a cryptographically random name
-      const randomSuffix = crypto.randomBytes(16).toString('hex');
-      const testFile = path.join(dir, `.write-test-${randomSuffix}`);
+      // Try to write a test file
+      const testFile = path.join(dir, `.write-test-${Date.now()}`);
      fs.writeFileSync(testFile, '', 'utf-8');
      // Cleanup test file - ignore errors (e.g., file locked on Windows)
      try { fs.unlinkSync(testFile); } catch { /* ignore cleanup failure */ }
@@ -152,6 +152,12 @@ export async function createSpecForIssue(
  const safeGithubUrl = sanitizeUrl(githubUrl);
  const safeLabels = sanitizeStringArray(labels, 50, 200);

+  // Sanitize network-sourced data before writing to disk
+  const safeTitle = sanitizeText(issueTitle, 500);
+  const safeDescription = sanitizeText(taskDescription, 50000, true);
+  const safeGithubUrl = sanitizeUrl(githubUrl);
+  const safeLabels = sanitizeStringArray(labels, 50, 200);
+
  // Use coordinated spec numbering with lock to prevent collisions
  return await withSpecNumberLock(project.path, async (lock) => {
    // Get next spec number from global scan (main + all worktrees)
@@ -254,7 +254,7 @@ export function registerStartGlabAuth(): void {

          glabProcess.stdout?.on('data', (data) => {
            const chunk = data.toString('utf-8');
-            _output += chunk;
+            output += chunk;
            debugLog('glab stdout:', chunk);

            // Try to open browser if URL detected
@@ -8,7 +8,7 @@ import path from 'path';
 import type { Project } from '../../../shared/types';
 import type { GitLabAPIIssue, GitLabConfig } from './types';
 import { labelMatchesWholeWord } from '../shared/label-utils';
-import { sanitizeText, sanitizeStringArray } from '../shared/sanitize';
+import { stripControlChars, sanitizeText, sanitizeStringArray } from '../shared/sanitize';

 /**
 * Simplified task info returned when creating a spec from a GitLab issue.
@@ -427,7 +427,7 @@ export function registerTriageHandlers(
              continue;
            }
            fs.writeFileSync(
-              path.join(triageDir, filename),
+              path.join(triageDir, `triage_${sanitizedResult.issue_iid}.json`),
              JSON.stringify(sanitizedResult, null, 2),
              'utf-8'
            );
@@ -8,22 +8,6 @@ import { projectStore } from '../project-store';
 import { parseEnvFile } from './utils';
 import { sanitizeText, sanitizeUrl } from './shared/sanitize';

-/**
- * Validate that a spec ID is safe for file system operations.
- * Prevents path traversal attacks by ensuring the spec ID:
- * - Does not contain path separators (/, \)
- * - Does not contain parent directory references (..)
- * - Contains only alphanumeric characters, dashes, and underscores
- */
-function isValidSpecId(specId: string): boolean {
-  if (!specId || specId.length === 0) return false;
-  if (specId.includes('..') || specId.includes('/') || specId.includes('\\')) {
-    return false;
-  }
-  const validPattern = /^[a-zA-Z0-9._-]+$/;
-  return validPattern.test(specId);
-}
-

 import { AgentManager } from '../agent';

@@ -403,17 +403,16 @@ export function registerRoadmapHandlers(
      );

      try {
-        return await withFileLock(roadmapPath, async () => {
-          let content: string;
-          try {
-            content = await readFileWithRetry(roadmapPath, { encoding: "utf-8" }) as string;
-          } catch (readErr: unknown) {
-            if ((readErr as NodeJS.ErrnoException).code === 'ENOENT') {
-              return { success: false, error: "Roadmap not found" };
-            }
-            throw readErr;
+        let content: string;
+        try {
+          content = readFileSync(roadmapPath, "utf-8");
+        } catch (readErr: unknown) {
+          if ((readErr as NodeJS.ErrnoException).code === 'ENOENT') {
+            return { success: false, error: "Roadmap not found" };
          }
-          const existingRoadmap = JSON.parse(content);
+          throw readErr;
+        }
+        const existingRoadmap = JSON.parse(content);

          // Transform camelCase features back to snake_case for JSON file
          existingRoadmap.features = roadmapData.features.map((feature) => ({
@@ -437,7 +436,7 @@ export function registerRoadmapHandlers(
          existingRoadmap.metadata = existingRoadmap.metadata || {};
          existingRoadmap.metadata.updated_at = new Date().toISOString();

-          await writeFileWithRetry(roadmapPath, JSON.stringify(existingRoadmap, null, 2), { encoding: 'utf-8' });
+        writeFileSync(roadmapPath, JSON.stringify(existingRoadmap, null, 2), 'utf-8');

          return { success: true };
        });
@@ -470,17 +469,16 @@ export function registerRoadmapHandlers(
      );

      try {
-        return await withFileLock(roadmapPath, async () => {
-          let content: string;
-          try {
-            content = await readFileWithRetry(roadmapPath, { encoding: "utf-8" }) as string;
-          } catch (readErr: unknown) {
-            if ((readErr as NodeJS.ErrnoException).code === 'ENOENT') {
-              return { success: false, error: "Roadmap not found" };
-            }
-            throw readErr;
+        let content: string;
+        try {
+          content = readFileSync(roadmapPath, "utf-8");
+        } catch (readErr: unknown) {
+          if ((readErr as NodeJS.ErrnoException).code === 'ENOENT') {
+            return { success: false, error: "Roadmap not found" };
          }
-          const roadmap = JSON.parse(content);
+          throw readErr;
+        }
+        const roadmap = JSON.parse(content);

          // Find and update the feature
          const feature = roadmap.features?.find((f: { id: string }) => f.id === featureId);
@@ -492,7 +490,7 @@ export function registerRoadmapHandlers(
          roadmap.metadata = roadmap.metadata || {};
          roadmap.metadata.updated_at = new Date().toISOString();

-          await writeFileWithRetry(roadmapPath, JSON.stringify(roadmap, null, 2), { encoding: 'utf-8' });
+        writeFileSync(roadmapPath, JSON.stringify(roadmap, null, 2), 'utf-8');

          return { success: true };
        });
@@ -520,10 +518,9 @@ export function registerRoadmapHandlers(
      );

      try {
-        return await withFileLock(roadmapPath, async () => {
        let content: string;
        try {
-          content = await readFileWithRetry(roadmapPath, { encoding: "utf-8" }) as string;
+          content = readFileSync(roadmapPath, "utf-8");
        } catch (readErr: unknown) {
          if ((readErr as NodeJS.ErrnoException).code === 'ENOENT') {
            return { success: false, error: "Roadmap not found" };
@@ -605,7 +602,7 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n"
        await writeFileWithRetry(
          path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN),
          JSON.stringify(implementationPlan, null, 2),
-          { encoding: 'utf-8' }
+          'utf-8'
        );

        // Create requirements.json
@@ -616,11 +613,11 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n"
        await writeFileWithRetry(
          path.join(specDir, AUTO_BUILD_PATHS.REQUIREMENTS),
          JSON.stringify(requirements, null, 2),
-          { encoding: 'utf-8' }
+          'utf-8'
        );

        // Create spec.md (required by backend spec creation process)
-        await writeFileWithRetry(path.join(specDir, AUTO_BUILD_PATHS.SPEC_FILE), taskDescription, { encoding: 'utf-8' });
+        writeFileSync(path.join(specDir, AUTO_BUILD_PATHS.SPEC_FILE), taskDescription, 'utf-8');

        // Build metadata
        const metadata: TaskMetadata = {
@@ -628,7 +625,7 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n"
          featureId: feature.id,
          category: "feature",
        };
-        await writeFileWithRetry(path.join(specDir, "task_metadata.json"), JSON.stringify(metadata, null, 2), { encoding: 'utf-8' });
+        writeFileSync(path.join(specDir, "task_metadata.json"), JSON.stringify(metadata, null, 2), 'utf-8');

        // NOTE: We do NOT auto-start spec creation here - user should explicitly start the task
        // from the kanban board when they're ready
@@ -638,7 +635,7 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n"
        feature.linked_spec_id = specId;
        roadmap.metadata = roadmap.metadata || {};
        roadmap.metadata.updated_at = new Date().toISOString();
-        await writeFileWithRetry(roadmapPath, JSON.stringify(roadmap, null, 2), { encoding: 'utf-8' });
+        writeFileSync(roadmapPath, JSON.stringify(roadmap, null, 2), 'utf-8');

        // Create task object
        const task: Task = {
@@ -708,7 +705,7 @@ ${(feature.acceptance_criteria || []).map((c: string) => `- [ ] ${c}`).join("\n"
          is_running: isRunning,
        };

-        writeFileSync(progressPath, JSON.stringify(fileData, null, 2));
+        writeFileSync(progressPath, JSON.stringify(fileData, null, 2), 'utf-8');
        debugLog("[Roadmap Handler] Saved progress checkpoint:", { projectId, phase: progressData.phase });

        return { success: true };
@@ -433,82 +433,6 @@ export async function createPlanIfNotExists(
  });
 }

-/**
- * Reset all stuck subtasks (in_progress or failed) to pending state.
- * This enables automatic recovery when tasks are interrupted by rate limits or errors.
- * Thread-safe with withPlanLock.
- *
- * @param planPath - Path to the implementation_plan.json file
- * @param projectId - Optional project ID to invalidate cache (recommended for performance)
- * @returns Object with success flag and count of reset subtasks
- */
-export async function resetStuckSubtasks(planPath: string, projectId?: string): Promise<{ success: boolean; resetCount: number }> {
-  return withPlanLock(planPath, async () => {
-    try {
-      console.log(`[plan-file-utils] Reading implementation_plan.json to reset stuck subtasks`, { planPath });
-
-      // Read file directly without existence check to avoid TOCTOU race condition
-      const planContent = readFileSync(planPath, 'utf-8');
-      const plan = JSON.parse(planContent);
-
-      let resetCount = 0;
-
-      // Iterate through all phases and subtasks
-      if (plan.phases && Array.isArray(plan.phases)) {
-        for (const phase of plan.phases) {
-          if (phase.subtasks && Array.isArray(phase.subtasks)) {
-            for (const subtask of phase.subtasks) {
-              // Only reset subtasks that are stuck (in_progress or failed)
-              // NEVER reset completed subtasks to avoid redoing work
-              if (subtask.status === 'in_progress' || subtask.status === 'failed') {
-                const originalStatus = subtask.status;
-                subtask.status = 'pending';
-                subtask.started_at = null;
-                subtask.completed_at = null;
-                resetCount++;
-                console.log(`[plan-file-utils] Reset subtask ${subtask.id} from ${originalStatus} to pending`);
-              }
-            }
-          }
-        }
-      }
-
-      // Only write if we actually reset something
-      if (resetCount > 0) {
-        plan.updated_at = new Date().toISOString();
-        const content = JSON.stringify(plan, null, 2);
-        // Atomic write: write to temp file then rename to prevent corruption on crash
-        const tempPath = `${planPath}.${process.pid}.tmp`;
-        try {
-          writeFileSync(tempPath, content, 'utf-8');
-          renameSync(tempPath, planPath);
-        } catch (writeError) {
-          try { unlinkSync(tempPath); } catch { /* ignore cleanup */ }
-          throw writeError;
-        }
-        console.log(`[plan-file-utils] Successfully reset ${resetCount} stuck subtask(s) in implementation_plan.json`);
-
-        // Invalidate tasks cache since subtask status changed
-        if (projectId) {
-          projectStore.invalidateTasksCache(projectId);
-        }
-      } else {
-        console.log(`[plan-file-utils] No stuck subtasks found to reset`);
-      }
-
-      return { success: true, resetCount };
-    } catch (err) {
-      // File not found is expected - return success with 0 count
-      if (isFileNotFoundError(err)) {
-        console.warn(`[plan-file-utils] implementation_plan.json not found at ${planPath} - no subtasks to reset`);
-        return { success: false, resetCount: 0 };
-      }
-      console.warn(`[plan-file-utils] Could not reset stuck subtasks at ${planPath}:`, err);
-      return { success: false, resetCount: 0 };
-    }
-  });
-}
-
 /**
 * Update task_metadata.json to add PR URL.
 * This is a simple JSON file update (no locking needed as it's rarely updated concurrently).
@@ -2079,6 +2079,7 @@ export function registerWorktreeHandlers(

          mergeProcess.stdout.on('data', (data: Buffer) => {
            const chunk = data.toString('utf-8');
+            stdout += chunk;
            debug('STDOUT:', chunk);

            // Prepend any buffered partial line from previous chunk
@@ -355,10 +355,11 @@ export function ensureDataDirectories(projectPath: string): InitializationResult
  try {
    for (const dataDir of DATA_DIRECTORIES) {
      const dirPath = path.join(dotAutoBuildPath, dataDir);
-      debug('Ensuring data directory exists', { dataDir, dirPath });
-      // mkdirSync with recursive: true doesn't error if directory exists
-      mkdirSync(dirPath, { recursive: true });
-      writeFileSync(path.join(dirPath, '.gitkeep'), '', 'utf-8');
+      if (!existsSync(dirPath)) {
+        debug('Creating missing data directory', { dataDir, dirPath });
+        mkdirSync(dirPath, { recursive: true });
+        writeFileSync(path.join(dirPath, '.gitkeep'), '', 'utf-8');
+      }
    }
    return { success: true };
  } catch (error) {
@@ -834,7 +834,7 @@ export class ProjectStore {
            metadata.archivedInVersion = version;
          }

-          writeFileSync(metadataPath, JSON.stringify(metadata, null, 2));
+          writeFileSync(metadataPath, JSON.stringify(metadata, null, 2), 'utf-8');
        } catch (error) {
          console.error(`[ProjectStore] archiveTasks: Failed to archive task ${taskId} at ${specPath}:`, error);
          hasErrors = true;
@@ -892,7 +892,7 @@ export class ProjectStore {

          delete metadata.archivedAt;
          delete metadata.archivedInVersion;
-          writeFileSync(metadataPath, JSON.stringify(metadata, null, 2));
+          writeFileSync(metadataPath, JSON.stringify(metadata, null, 2), 'utf-8');
        } catch (error) {
          console.error(`[ProjectStore] unarchiveTasks: Failed to unarchive task ${taskId} at ${specPath}:`, error);
          hasErrors = true;