improv: validation

Author: tomasz-stefaniak

core/util/sanitization.vitest.ts

@@ -1,5 +1,6 @@
 import { execSync } from "child_process";
 import { describe, expect, it } from "vitest";
+import { normalizeRepoUrl } from "./repoUrl";
 import { sanitizeShellArgument, validateGitHubRepoUrl } from "./sanitization";
 
 describe("sanitizeShellArgument", () => {
@@ -87,6 +88,81 @@ describe("validateGitHubRepoUrl", () => {
     expect(validateGitHubRepoUrl(null as any)).toBe(false);
     expect(validateGitHubRepoUrl(undefined as any)).toBe(false);
   });
+
+  describe("validation after normalization", () => {
+    it("should validate normalized URLs to prevent bypass", () => {
+      // Test that validation works on normalized output
+      const inputs = [
+        "owner/repo",
+        "[email protected]:owner/repo.git",
+        "https://github.com/owner/repo.git",
+        "ssh://[email protected]/owner/repo.git",
+      ];
+
+      inputs.forEach((input) => {
+        const normalized = normalizeRepoUrl(input);
+        expect(validateGitHubRepoUrl(normalized)).toBe(true);
+      });
+    });
+
+    it("should catch dangerous URLs even after normalization", () => {
+      // These should still be dangerous after normalization
+      const dangerous = [
+        "owner/repo; rm -rf /",
+        "owner/repo && malicious",
+        "owner/repo | cat /etc/passwd",
+      ];
+
+      dangerous.forEach((input) => {
+        // Should be blocked before normalization
+        expect(validateGitHubRepoUrl(input)).toBe(false);
+
+        // Even if somehow normalized, should still be invalid
+        const normalized = normalizeRepoUrl(input);
+        expect(validateGitHubRepoUrl(normalized)).toBe(false);
+      });
+    });
+
+    it("should handle edge cases where normalization changes URL structure", () => {
+      // Test URLs that change during normalization
+      const testCases = [
+        {
+          input: "Owner/Repo.git/",
+          normalized: "https://github.com/owner/repo",
+          shouldBeValid: true,
+        },
+        {
+          input: "[email protected]:owner/repo.git",
+          normalized: "https://github.com/owner/repo",
+          shouldBeValid: true,
+        },
+      ];
+
+      testCases.forEach(({ input, normalized, shouldBeValid }) => {
+        const actualNormalized = normalizeRepoUrl(input);
+        expect(actualNormalized).toBe(normalized);
+        expect(validateGitHubRepoUrl(actualNormalized)).toBe(shouldBeValid);
+      });
+    });
+
+    it("should prevent validation bypass via URL encoding or special chars", () => {
+      // These tests ensure that validation happens AFTER normalization
+      // preventing attackers from bypassing validation via encoding or transformation
+
+      // Currently validateGitHubRepoUrl blocks these, but this test ensures
+      // the pattern of "normalize then validate" is maintained
+      const potentialBypass = [
+        "../../../etc/passwd",
+        "owner/../malicious",
+        "owner/repo`whoami`",
+        "owner/repo$(whoami)",
+      ];
+
+      potentialBypass.forEach((input) => {
+        expect(validateGitHubRepoUrl(input)).toBe(false);
+      });
+    });
+  });
 });
 
 /**

extensions/vscode/src/extension/VsCodeMessenger.ts

@@ -300,19 +300,19 @@ export class VsCodeMessenger {
         // Get repo name/URL
         const repoName = await this.ide.getRepoName(workspaceDir);
         if (repoName) {
-          // Validate repo URL to prevent injection attacks
-          if (!validateGitHubRepoUrl(repoName)) {
+          // Normalize the URL first to get canonical form
+          const normalized = normalizeRepoUrl(repoName);
+
+          // Validate the normalized URL to prevent injection attacks
+          // This ensures we validate what we'll actually use, not just the input
+          if (!validateGitHubRepoUrl(normalized)) {
             vscode.window.showErrorMessage(
               "Invalid repository format. Please ensure you're using a valid GitHub repository.",
             );
             return;
           }
-          // If repo name looks like "owner/repo", convert to GitHub URL
-          if (repoName.includes("/") && !repoName.startsWith("http")) {
-            repoUrl = `https://github.com/${repoName}`;
-          } else {
-            repoUrl = repoName;
-          }
+
+          repoUrl = normalized;
         }
 
         // Get current branch
@@ -464,14 +464,29 @@ export class VsCodeMessenger {
           return;
         }
 
+        // Validate the repo URL from API response to prevent injection attacks
+        if (!validateGitHubRepoUrl(repoUrl)) {
+          vscode.window.showErrorMessage(
+            "Invalid repository URL from agent session. Please contact support.",
+          );
+          return;
+        }
+
         // Get workspace directories
         const workspaceDirs = await this.ide.getWorkspaceDirs();
         if (workspaceDirs.length === 0) {
           vscode.window.showErrorMessage("No workspace folder is open.");
           return;
         }
 
+        // Normalize and validate again to ensure the normalized form is safe
         const normalizedAgentRepo = normalizeRepoUrl(repoUrl);
+        if (!validateGitHubRepoUrl(normalizedAgentRepo)) {
+          vscode.window.showErrorMessage(
+            "Invalid repository URL after normalization. Please contact support.",
+          );
+          return;
+        }
 
         // Find the workspace that matches the agent's repo URL
         let matchingWorkspace: string | null = null;