fix: address issue #1266 follow-ups from git-native transport review

.serena/project.yml

@@ -114,3 +114,7 @@ symbol_info_budget:
 # Note: the backend is fixed at startup. If a project with a different backend
 # is activated post-init, an error will be returned.
 language_backend:
+
+# list of regex patterns which, when matched, mark a memory entry as read‑only.
+# Extends the list from the global configuration, merging the two lists.
+read_only_memory_patterns: []

config-schema.json

@@ -102,23 +102,28 @@
         "properties": {
           "source": {
             "type": "string",
-            "minLength": 1
+            "minLength": 1,
+            "description": "Repository identifier (e.g., 'org/repo' or a full URL)."
           },
           "skills": {
             "type": "array",
             "items": {
               "type": "string"
-            }
+            },
+            "description": "Skill names to fetch. Omit to fetch all skills from the source."
           },
           "transport": {
             "type": "string",
-            "enum": ["github", "git"]
+            "enum": ["github", "git"],
+            "description": "Transport protocol for fetching skills. 'github' uses the GitHub REST API (default). 'git' uses the git CLI and works with any git remote."
           },
           "ref": {
-            "type": "string"
+            "type": "string",
+            "description": "Git ref (branch or tag) to fetch skills from. Defaults to the repository's default branch."
           },
           "path": {
-            "type": "string"
+            "type": "string",
+            "description": "Path to the skills directory within the repository. Defaults to 'skills'."
           }
         },
         "required": ["source"],

scripts/generate-json-schema.ts

@@ -12,9 +12,30 @@ const generatedSchema = z.toJSONSchema(ConfigFileSchema, {
   reused: "ref",
 });
 
+// Add descriptions to source entry fields (zod/mini lacks .describe())
+// Round-trip through JSON to get a plain object we can mutate without type assertions
+const schemaObj = JSON.parse(JSON.stringify(generatedSchema));
+const sourceProps = schemaObj?.properties?.sources?.items?.properties;
+if (sourceProps) {
+  if (sourceProps.source)
+    sourceProps.source.description = "Repository identifier (e.g., 'org/repo' or a full URL).";
+  if (sourceProps.skills)
+    sourceProps.skills.description =
+      "Skill names to fetch. Omit to fetch all skills from the source.";
+  if (sourceProps.transport)
+    sourceProps.transport.description =
+      "Transport protocol for fetching skills. 'github' uses the GitHub REST API (default). 'git' uses the git CLI and works with any git remote.";
+  if (sourceProps.ref)
+    sourceProps.ref.description =
+      "Git ref (branch or tag) to fetch skills from. Defaults to the repository's default branch.";
+  if (sourceProps.path)
+    sourceProps.path.description =
+      "Path to the skills directory within the repository. Defaults to 'skills'.";
+}
+
 // Add JSON Schema meta properties (override Zod's default $schema with draft-07 for broader compatibility)
 const jsonSchema = {
-  ...generatedSchema,
+  ...schemaObj,
   $schema: "http://json-schema.org/draft-07/schema#",
   $id: "https://raw.githubusercontent.com/dyoshikawa/rulesync/refs/heads/main/config-schema.json",
   title: "Rulesync Configuration",

src/config/config.ts

@@ -17,14 +17,7 @@ import {
   ToolTarget,
   ToolTargets,
 } from "../types/tool-targets.js";
-
-function hasControlCharacters(value: string): boolean {
-  for (let i = 0; i < value.length; i++) {
-    const code = value.charCodeAt(i);
-    if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) return true;
-  }
-  return false;
-}
+import { hasControlCharacters } from "../utils/validation.js";
 
 /**
  * Schema for a single source entry in the sources array.

src/lib/git-client.test.ts

@@ -197,6 +197,35 @@ describe("git-client", () => {
       expect(removeTempDirectory).toHaveBeenCalledWith("/tmp/test");
     });
 
+    it("rejects skillsPath with path traversal", async () => {
+      await expect(
+        fetchSkillFiles({ url: "https://example.com/repo.git", ref: "main", skillsPath: "../etc" }),
+      ).rejects.toThrow(GitClientError);
+      await expect(
+        fetchSkillFiles({ url: "https://example.com/repo.git", ref: "main", skillsPath: "../etc" }),
+      ).rejects.toThrow("must be a relative path");
+    });
+
+    it("rejects absolute skillsPath", async () => {
+      await expect(
+        fetchSkillFiles({
+          url: "https://example.com/repo.git",
+          ref: "main",
+          skillsPath: "/etc/passwd",
+        }),
+      ).rejects.toThrow(GitClientError);
+    });
+
+    it("rejects skillsPath with control characters", async () => {
+      await expect(
+        fetchSkillFiles({
+          url: "https://example.com/repo.git",
+          ref: "main",
+          skillsPath: "skills\x00",
+        }),
+      ).rejects.toThrow("control character");
+    });
+
     it("returns empty when skills dir missing", async () => {
       mockExecFileAsync.mockResolvedValue({ stdout: "" });
       vi.mocked(createTempDirectory).mockResolvedValue("/tmp/test");

src/lib/git-client.ts

@@ -1,5 +1,5 @@
 import { execFile } from "node:child_process";
-import { join } from "node:path";
+import { isAbsolute, join, relative } from "node:path";
 import { promisify } from "node:util";
 
 import { MAX_FILE_SIZE } from "../constants/rulesync-paths.js";
@@ -13,31 +13,18 @@ import {
   removeTempDirectory,
 } from "../utils/file.js";
 import { logger } from "../utils/logger.js";
+import { findControlCharacter } from "../utils/validation.js";
 
 const execFileAsync = promisify(execFile);
 
 /** Timeout for all git CLI operations (60 seconds). */
 const GIT_TIMEOUT_MS = 60_000;
 
 const ALLOWED_URL_SCHEMES =
-  /^(https?:\/\/|ssh:\/\/|git:\/\/|file:\/\/\/|[a-zA-Z0-9_.+-]+@[a-zA-Z0-9.-]+:[a-zA-Z0-9_.+/~-]+)/;
+  /^(https?:\/\/|ssh:\/\/|git:\/\/|file:\/\/\/).+$|^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9.-]+:[a-zA-Z0-9_.+/~-]+$/;
 
 const INSECURE_URL_SCHEMES = /^(git:\/\/|http:\/\/)/;
 
-/**
- * Check for control characters and return the position and hex code of the first one found.
- * Returns null if no control characters are found.
- */
-function findControlCharacter(value: string): { position: number; hex: string } | null {
-  for (let i = 0; i < value.length; i++) {
-    const code = value.charCodeAt(i);
-    if ((code >= 0x00 && code <= 0x1f) || code === 0x7f) {
-      return { position: i, hex: `0x${code.toString(16).padStart(2, "0")}` };
-    }
-  }
-  return null;
-}
-
 export class GitClientError extends Error {
   constructor(message: string, cause?: unknown) {
     super(message, { cause });
@@ -107,6 +94,7 @@ export async function resolveDefaultRef(url: string): Promise<{ ref: string; sha
     const ref = stdout.match(/^ref: refs\/heads\/(.+)\tHEAD$/m)?.[1];
     const sha = stdout.match(/^([0-9a-f]{40})\tHEAD$/m)?.[1];
     if (!ref || !sha) throw new GitClientError(`Could not parse default branch from: ${url}`);
+    validateRef(ref);
     return { ref, sha };
   } catch (error) {
     if (error instanceof GitClientError) throw error;
@@ -144,6 +132,17 @@ export async function fetchSkillFiles(params: {
   const { url, ref, skillsPath } = params;
   validateGitUrl(url);
   validateRef(ref);
+  if (skillsPath.split(/[/\\]/).includes("..") || isAbsolute(skillsPath)) {
+    throw new GitClientError(
+      `Invalid skillsPath "${skillsPath}": must be a relative path without ".."`,
+    );
+  }
+  const ctrl = findControlCharacter(skillsPath);
+  if (ctrl) {
+    throw new GitClientError(
+      `skillsPath contains control character ${ctrl.hex} at position ${ctrl.position}`,
+    );
+  }
   await checkGitAvailable();
   const tmpDir = await createTempDirectory("rulesync-git-");
   try {
@@ -179,11 +178,17 @@ export async function fetchSkillFiles(params: {
 }
 
 const MAX_WALK_DEPTH = 20;
+const MAX_TOTAL_FILES = 10_000;
+const MAX_TOTAL_SIZE = 100 * 1024 * 1024; // 100 MB
+
+/** Mutable context for tracking totals across recursive walkDirectory calls. */
+type WalkContext = { totalFiles: number; totalSize: number };
 
 async function walkDirectory(
   dir: string,
   baseDir: string,
   depth: number = 0,
+  ctx: WalkContext = { totalFiles: 0, totalSize: 0 },
 ): Promise<Array<{ relativePath: string; content: string; size: number }>> {
   if (depth > MAX_WALK_DEPTH) {
     throw new GitClientError(
@@ -199,7 +204,7 @@ async function walkDirectory(
       continue;
     }
     if (await directoryExists(fullPath)) {
-      results.push(...(await walkDirectory(fullPath, baseDir, depth + 1)));
+      results.push(...(await walkDirectory(fullPath, baseDir, depth + 1, ctx)));
     } else {
       const size = await getFileSize(fullPath);
       if (size > MAX_FILE_SIZE) {
@@ -208,8 +213,20 @@ async function walkDirectory(
         );
         continue;
       }
+      ctx.totalFiles++;
+      ctx.totalSize += size;
+      if (ctx.totalFiles >= MAX_TOTAL_FILES) {
+        throw new GitClientError(
+          `Repository exceeds max file count of ${MAX_TOTAL_FILES}. Aborting to prevent resource exhaustion.`,
+        );
+      }
+      if (ctx.totalSize >= MAX_TOTAL_SIZE) {
+        throw new GitClientError(
+          `Repository exceeds max total size of ${MAX_TOTAL_SIZE / 1024 / 1024}MB. Aborting to prevent resource exhaustion.`,
+        );
+      }
       const content = await readFileContent(fullPath);
-      results.push({ relativePath: fullPath.substring(baseDir.length + 1), content, size });
+      results.push({ relativePath: relative(baseDir, fullPath), content, size });
     }
   }
   return results;

src/lib/sources-lock.test.ts

@@ -29,6 +29,8 @@ vi.mock("../utils/logger.js", () => ({
 
 const { logger } = await vi.importMock<typeof import("../utils/logger.js")>("../utils/logger.js");
 
+const VALID_SHA = "a".repeat(40);
+
 describe("sources-lock", () => {
   afterEach(() => {
     vi.clearAllMocks();
@@ -64,7 +66,7 @@ describe("sources-lock", () => {
         lockfileVersion: 1,
         sources: {
           "https://github.com/org/repo": {
-            resolvedRef: "abc123",
+            resolvedRef: VALID_SHA,
             skills: {
               "skill-a": { integrity: "sha256-abc" },
               "skill-b": { integrity: "sha256-def" },
@@ -78,7 +80,7 @@ describe("sources-lock", () => {
       const lock = await readLockFile({ baseDir: testDir });
 
       expect(lock.sources["https://github.com/org/repo"]).toEqual({
-        resolvedRef: "abc123",
+        resolvedRef: VALID_SHA,
         skills: {
           "skill-a": { integrity: "sha256-abc" },
           "skill-b": { integrity: "sha256-def" },

src/lib/sources-lock.ts

@@ -1,7 +1,7 @@
 import { createHash } from "node:crypto";
 import { join } from "node:path";
 
-import { optional, z } from "zod/mini";
+import { optional, refine, z } from "zod/mini";
 
 import { RULESYNC_SOURCES_LOCK_RELATIVE_FILE_PATH } from "../constants/rulesync-paths.js";
 import { fileExists, readFileContent, writeFileContent } from "../utils/file.js";
@@ -23,7 +23,9 @@ export type LockedSkill = z.infer<typeof LockedSkillSchema>;
  */
 export const LockedSourceSchema = z.object({
   requestedRef: optional(z.string()),
-  resolvedRef: z.string(),
+  resolvedRef: z
+    .string()
+    .check(refine((v) => /^[0-9a-f]{40}$/.test(v), "resolvedRef must be a 40-character hex SHA")),
   resolvedAt: optional(z.string()),
   skills: z.record(z.string(), LockedSkillSchema),
 });