Merge pull request #19 from AikidoSec/safer-line-removal

Only remove lines from shell scripts if we're sure it's safe to remove

src/shell-integration/helpers.js

@@ -28,11 +28,35 @@ export function removeLinesMatchingPattern(filePath, pattern) {
   }
 
   const fileContent = fs.readFileSync(filePath, "utf-8");
-  const lines = fileContent.split(os.EOL);
+  const lines = fileContent.split(/[\r\n\u2028\u2029]+/);
-  const updatedLines = lines.filter((line) => !pattern.test(line));
+  const updatedLines = lines.filter((line) => !shouldRemoveLine(line, pattern));
   fs.writeFileSync(filePath, updatedLines.join(os.EOL), "utf-8");
 }
 
+const maxLineLength = 100;
+function shouldRemoveLine(line, pattern) {
+  const isPatternMatch = pattern.test(line);
+
+  if (!isPatternMatch) {
+    return false;
+  }
+
+  if (line.length > maxLineLength) {
+    // safe-chain only adds lines shorter than maxLineLength
+    // so if the line is longer, it must be from a different 
+    // source and could be dangerous to remove
+    return false;
+  }
+
+  if (line.includes("\n") || line.includes("\r") || line.includes("\u2028") || line.includes("\u2029")) {
+    // If the line contains newlines, something has gone wrong in splitting
+    // \u2028 and \u2029 are Unicode line separator characters (line and paragraph separators)
+    return false;
+  }
+
+  return true;
+}
+
 export function addLineToFile(filePath, line) {
   if (!fs.existsSync(filePath)) {
     fs.writeFileSync(filePath, "", "utf-8");

src/shell-integration/helpers.spec.js

@@ -0,0 +1,113 @@
+import { describe, it, beforeEach, afterEach, mock } from "node:test";
+import assert from "node:assert";
+import { tmpdir } from "node:os";
+import fs from "node:fs";
+import path from "path";
+
+describe("removeLinesMatchingPatternTests", () => {
+  let testFile;
+
+  beforeEach(() => {
+    // Create temporary test file
+    testFile = path.join(tmpdir(), `test-helpers-${Date.now()}.txt`);
+
+    // Mock the os module to override EOL
+    mock.module("node:os", {
+      namedExports: {
+        EOL: "\r\n", // Simulate Windows line endings
+        tmpdir: tmpdir,
+        platform: () => "linux"
+      }
+    });
+  });
+
+  afterEach(() => {
+    // Clean up test files
+    if (fs.existsSync(testFile)) {
+      fs.unlinkSync(testFile);
+    }
+
+    // Reset mocks
+    mock.reset();
+  });
+
+
+  it("should handle mixed line endings without wiping entire file", async () => {
+    // Import helpers after setting up the mock
+    const { removeLinesMatchingPattern } = await import("./helpers.js");
+    
+    // Create a file with Unix line endings but os.EOL expects Windows
+    const fileContent = [
+      "# keep this line",
+      "alias npm='remove-this'", 
+      "# keep this line too",
+      "alias yarn='remove-this-too'",
+      "# final line to keep"
+    ].join("\n"); // File has Unix line endings
+    
+    fs.writeFileSync(testFile, fileContent, "utf-8");
+    
+    // Try to remove lines containing 'alias'
+    const pattern = /alias.*=/;
+    removeLinesMatchingPattern(testFile, pattern);
+    
+    const result = fs.readFileSync(testFile, "utf-8");
+    
+    // This test will fail because the function splits on '\r\n' but file uses '\n'
+    // So it treats the entire content as one line and if any part matches, removes everything
+    assert.ok(result.includes("keep this line"), "Should preserve non-matching lines");
+    assert.ok(result.includes("final line to keep"), "Should preserve final line");
+  });
+
+  it("should handle mixed line endings with short matching content", async () => {
+    // Import helpers after setting up the mock
+    const { removeLinesMatchingPattern } = await import("./helpers.js");
+    
+    // Create a file with Unix line endings, but make the entire content short 
+    // to bypass the maxLineLength protection
+    const fileContent = [
+      "# keep1",
+      "alias x=y", // Short alias line that should be removed
+      "# keep2"
+    ].join("\n"); // File has Unix line endings, total length < 100 chars
+    
+    fs.writeFileSync(testFile, fileContent, "utf-8");
+    
+    // Try to remove lines containing 'alias'
+    const pattern = /alias/;
+    removeLinesMatchingPattern(testFile, pattern);
+    
+    const result = fs.readFileSync(testFile, "utf-8");
+    
+    // This should now be protected by the newline detection
+    assert.ok(result.includes("keep1"), "Should preserve first line");
+    assert.ok(result.includes("keep2"), "Should preserve third line");
+  });
+
+  it("should handle Unicode line separators that bypass newline detection", async () => {
+    // Import helpers after setting up the mock
+    const { removeLinesMatchingPattern } = await import("./helpers.js");
+    
+    // Use Unicode line separator (U+2028) and paragraph separator (U+2029)
+    // These are considered line breaks but aren't \n or \r
+    const fileContent = [
+      "keep this",
+      "alias test=value",
+      "keep that"
+    ].join("\u2028"); // Unicode line separator
+    
+    fs.writeFileSync(testFile, fileContent, "utf-8");
+    
+    // Try to remove lines containing 'alias'
+    const pattern = /alias/;
+    removeLinesMatchingPattern(testFile, pattern);
+    
+    const result = fs.readFileSync(testFile, "utf-8");
+    
+    // This could still wipe everything if split() treats it as one line
+    // but the content doesn't contain \n or \r so passes the newline check
+    assert.ok(result.includes("keep this"), "Should preserve first part");
+    assert.ok(result.includes("keep that"), "Should preserve last part");
+  });
+
+});