fix(server): reject non-git URLs at /api/repositories

saltbo · saltbo · commit 875aa18f7d9b · 2026-04-20T21:29:51.000-04:00
A leader agent in an empty-remote project improvised `file:///...` as
the repo URL. normalizeGitUrl silently passed it through, extractFullName
returned null, and the daemon looped on `gh repo clone null file:/...`
until the task was cancelled.

Close the contract end-to-end: normalizeGitUrl whitelists https/http/ssh
with at least host/owner/repo, strips trailing .git/slash combinations,
and throws 400 on anything else. extractFullName becomes a contract
function (invariant: input already normalized) and throws 500 on
non-match, tightening full_name from string|null to string. Also fix
findOrCreateRepository to only swallow UNIQUE constraint errors and
apply withFullName to the fetched row.

ak-plan skill Phase 0 now explicitly forbids inventing URLs when
`git remote -v` is empty — stop and ask the user to push first.
diff --git a/apps/web/server/repositoryRepo.ts b/apps/web/server/repositoryRepo.ts
@@ -1,20 +1,37 @@
 import type { CreateRepositoryInput, Repository } from "@agent-kanban/shared";
+import { HTTPException } from "hono/http-exception";
 import { type D1, newId } from "./db";
 
-/** Normalize git URL to canonical HTTPS form without .git suffix */
+/**
+ * Normalize git URL to canonical HTTPS form without .git suffix or trailing slash.
+ * Accepts: https://host/owner/repo, http://host/owner/repo, git@host:owner/repo.
+ * Rejects everything else (file://, local paths, single-segment paths, unknown schemes)
+ * with 400 — agents cannot invent URLs, users cannot paste local paths, and the daemon
+ * must never see an un-cloneable URL.
+ */
 export function normalizeGitUrl(url: string): string {
   const sshMatch = url.match(/^git@([^:]+):(.+?)(?:\.git)?$/);
   if (sshMatch) return `https://${sshMatch[1]}/${sshMatch[2]}`;
-  return url.replace(/\.git$/, "");
+  if (/^https?:\/\/[^/]+\/[^/]+\/.+/.test(url)) {
+    return url.replace(/(?:\.git|\/)+$/, "");
+  }
+  throw new HTTPException(400, {
+    message: `Invalid repository URL "${url}": only https://host/owner/repo, http://host/owner/repo, or git@host:owner/repo are accepted`,
+  });
 }
 
-/** Extract owner/repo from canonical HTTPS URL */
-function extractFullName(httpsUrl: string): string | null {
-  const match = httpsUrl.match(/^https?:\/\/[^/]+\/(.+?)(?:\.git)?$/);
-  return match ? match[1] : null;
+/** Extract owner/repo from a canonicalized URL. Invariant: the URL has already passed normalizeGitUrl. */
+function extractFullName(canonicalUrl: string): string {
+  const match = canonicalUrl.match(/^https?:\/\/[^/]+\/(.+)$/);
+  if (!match) {
+    throw new HTTPException(500, {
+      message: `Repository URL "${canonicalUrl}" has no owner/repo — data invariant broken (bypassed normalizeGitUrl)`,
+    });
+  }
+  return match[1];
 }
 
-function withFullName<T extends { url: string }>(repo: T): T & { full_name: string | null } {
+function withFullName<T extends { url: string }>(repo: T): T & { full_name: string } {
   return { ...repo, full_name: extractFullName(repo.url) };
 }
 
@@ -32,16 +49,16 @@ export async function createRepository(db: D1, ownerId: string, input: CreateRep
 }
 
 export async function findOrCreateRepository(db: D1, ownerId: string, input: CreateRepositoryInput): Promise<Repository> {
+  const url = normalizeGitUrl(input.url);
   try {
     return await createRepository(db, ownerId, input);
-  } catch {
-    const existing = await db
-      .prepare("SELECT * FROM repositories WHERE owner_id = ? AND url = ?")
-      .bind(ownerId, normalizeGitUrl(input.url))
-      .first<Repository>();
-    if (existing) return existing;
-    throw new Error("Failed to create or find repository");
+  } catch (err) {
+    const isUniqueViolation = err instanceof Error && err.message.includes("UNIQUE constraint failed");
+    if (!isUniqueViolation) throw err;
   }
+  const existing = await db.prepare("SELECT * FROM repositories WHERE owner_id = ? AND url = ?").bind(ownerId, url).first<Repository>();
+  if (existing) return withFullName(existing);
+  throw new Error("Failed to create or find repository");
 }
 
 export async function listRepositories(db: D1, ownerId: string, filters?: { url?: string }): Promise<Repository[]> {
diff --git a/skills/ak-plan/SKILL.md b/skills/ak-plan/SKILL.md
@@ -45,12 +45,19 @@ Parse the user's input:
 Check if this is an **existing project** or a **new product**:
 
 ```bash
-git remote -v 2>/dev/null    # has a repo? → existing project
+git remote -v 2>/dev/null    # has a remote? → existing project
 ak get repo                  # registered repos
 ```
 
-- **Existing project**: has git remote → skip to Phase 1
-- **New product**: no repo → go to Phase 0.5 (Scaffold)
+Three possible states:
+
+- **Existing project with remote** → skip to Phase 1
+- **New product (no git init yet)** → go to Phase 0.5 (Scaffold)
+- **Local-only project (git init done, no remote)** → STOP. A registered repo must have a real remote (`https://…` or `git@…`). Tell the user one of:
+  1. Push the project to GitHub first: `gh repo create <owner>/<name> --source . --push`
+  2. Or: ask them for the intended remote URL before proceeding.
+
+  **Never invent a URL** (no `file://`, no local paths, no placeholders). The agent-kanban server will reject non-http(s)/ssh URLs with 400, and even if it didn't, the daemon cannot clone local paths.
 
 ## Phase 0.5: Scaffold (new products only)
 
@@ -67,9 +74,9 @@ cd <repo-dir>
 git add -A && git commit -m "feat: project scaffold" && git push -u origin main
 ```
 
-Register with agent-kanban:
+Register with agent-kanban (URL MUST come from `git remote get-url origin` — never hand-crafted):
 ```bash
-ak create repo --name <name> --url <url>
+ak create repo --name <name> --url "$(git remote get-url origin)"
 ```
 
 The scaffold must contain enough structure for agents to start writing code immediately.
@@ -306,7 +313,7 @@ When all tasks are done, report the final summary to the user.
 - **Workflow completion is mandatory** — once this skill is invoked, the full lifecycle (plan → create → assign → monitor → review → merge all) MUST run to completion. If you are interrupted mid-workflow (user asks a side question, chat drifts to another topic, tool fails, etc.), handle the interruption and then **immediately resume the workflow from where you left off**. Never ask "should I continue monitoring?" or "do you want me to keep going?" — the answer is always yes. The only way to exit the workflow early is if the user explicitly says to stop, cancel, or abort.
 - **Follow CONTRIBUTING.md** — read the target repo's CONTRIBUTING.md before creating tasks; check PR compliance during review
 - **Prefer text output** — only use `-o json | jq` when extracting fields into variables (e.g. task IDs for `--depends-on`). For display, use default text output.
-- **Always get repo URL from `git remote -v`** — never guess
+- **Always get repo URL from `git remote get-url origin`** — never guess, never improvise. If there is no remote, stop and ask the user to push the repo first (see Phase 0). `file://`, local paths, and placeholder URLs will be rejected by the server with 400.
 - **Discuss the plan with the user before creating tasks** — don't just start creating
 - **Set depends-on at creation time** — don't leave deps for later
 - **Space API calls** — avoid triggering rate limits during batch creation
diff --git a/tests/repository-repo.test.ts b/tests/repository-repo.test.ts
@@ -33,9 +33,86 @@ describe("normalizeGitUrl", () => {
     expect(normalizeGitUrl("https://github.com/org/repo")).toBe("https://github.com/org/repo");
   });
 
-  it("passes through URL with trailing slash as-is", async () => {
+  it("strips trailing slash from HTTPS URL", async () => {
     const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
-    expect(normalizeGitUrl("https://github.com/org/repo/")).toBe("https://github.com/org/repo/");
+    expect(normalizeGitUrl("https://github.com/org/repo/")).toBe("https://github.com/org/repo");
+  });
+
+  it("accepts SSH git@ URL without .git suffix", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    expect(normalizeGitUrl("git@gitlab.com:myorg/myrepo")).toBe("https://gitlab.com/myorg/myrepo");
+  });
+
+  it("accepts http:// URL with owner/repo path", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    expect(normalizeGitUrl("http://github.example.com/owner/repo")).toBe("http://github.example.com/owner/repo");
+  });
+
+  it("rejects file:// URL with HTTPException 400", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      normalizeGitUrl("file:///Users/alice/proj");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("rejects bare local path with HTTPException 400", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      normalizeGitUrl("/Users/alice/proj");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("rejects plain string with no scheme with HTTPException 400", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      normalizeGitUrl("my-repo");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("rejects https:// URL with no owner/repo path with HTTPException 400", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      normalizeGitUrl("https://github.com");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("rejects single-segment HTTPS path with HTTPException 400", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      normalizeGitUrl("https://github.com/single-segment");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("strips .git and then trailing slash from https://github.com/org/repo/.git", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    // .git$ matches the end of "https://github.com/org/repo/.git", leaving "https://github.com/org/repo/"
+    // trailing-slash strip then removes the slash → "https://github.com/org/repo"
+    expect(normalizeGitUrl("https://github.com/org/repo/.git")).toBe("https://github.com/org/repo");
+  });
+
+  it("accepts nested group paths (gitlab.com/group/subgroup/project)", async () => {
+    const { normalizeGitUrl } = await import("../apps/web/server/repositoryRepo");
+    expect(normalizeGitUrl("https://gitlab.com/group/subgroup/project")).toBe("https://gitlab.com/group/subgroup/project");
   });
 });
 
@@ -101,11 +178,85 @@ describe("repositoryRepo", () => {
       url: "https://github.com/org/find-create-dup",
     });
     expect(second.id).toBe(first.id);
+    expect(second.full_name).toBe("org/find-create-dup");
   });
 
   it("repos are scoped to owner", async () => {
     const { listRepositories } = await import("../apps/web/server/repositoryRepo");
     const repos = await listRepositories(env.DB, "repo-test-user-2");
     expect(repos.length).toBe(0);
   });
+
+  it("createRepository rejects a file:// URL with 400", async () => {
+    const { createRepository } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      await createRepository(env.DB, "repo-test-user", { name: "bad-repo", url: "file:///Users/xudawei/skill-lake" });
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("findOrCreateRepository rejects a file:// URL with 400", async () => {
+    const { findOrCreateRepository } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      await findOrCreateRepository(env.DB, "repo-test-user", { name: "bad-repo", url: "file:///Users/xudawei/skill-lake" });
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("listRepositories rejects a file:// url filter with 400", async () => {
+    const { listRepositories } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      await listRepositories(env.DB, "repo-test-user", { url: "file:///Users/xudawei/skill-lake" });
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(400);
+  });
+
+  it("getRepository returns null for unknown id", async () => {
+    const { getRepository } = await import("../apps/web/server/repositoryRepo");
+    const result = await getRepository(env.DB, "nonexistent-id", "repo-test-user");
+    expect(result).toBeNull();
+  });
+
+  it("getRepository returns the repo for a known id", async () => {
+    const { createRepository, getRepository } = await import("../apps/web/server/repositoryRepo");
+    const repo = await createRepository(env.DB, "repo-test-user", { name: "get-repo", url: "https://github.com/org/get-repo" });
+    const result = await getRepository(env.DB, repo.id, "repo-test-user");
+    expect(result).not.toBeNull();
+    expect(result!.id).toBe(repo.id);
+    expect(result!.full_name).toBe("org/get-repo");
+  });
+
+  it("getRepository returns null when owner does not match", async () => {
+    const { createRepository, getRepository } = await import("../apps/web/server/repositoryRepo");
+    const repo = await createRepository(env.DB, "repo-test-user", { name: "scoped-repo", url: "https://github.com/org/scoped-repo" });
+    const result = await getRepository(env.DB, repo.id, "repo-test-user-2");
+    expect(result).toBeNull();
+  });
+
+  it("listRepositories throws 500 when a stored URL bypassed normalizeGitUrl", async () => {
+    // Directly insert a row with a file:// URL to simulate a corrupt DB row (invariant broken)
+    const now = new Date().toISOString();
+    await env.DB.prepare("INSERT INTO repositories (id, owner_id, name, url, created_at) VALUES (?, ?, ?, ?, ?)")
+      .bind("corrupt-id-1", "repo-test-user", "corrupt-repo", "file:///bad/path", now)
+      .run();
+    const { listRepositories } = await import("../apps/web/server/repositoryRepo");
+    let thrown: unknown;
+    try {
+      await listRepositories(env.DB, "repo-test-user");
+    } catch (err) {
+      thrown = err;
+    }
+    expect((thrown as any).status).toBe(500);
+    // Clean up the corrupt row so it doesn't affect other tests
+    await env.DB.prepare("DELETE FROM repositories WHERE id = ?").bind("corrupt-id-1").run();
+  });
 });
diff --git a/tests/routes.test.ts b/tests/routes.test.ts
@@ -285,6 +285,13 @@ describe("routes", () => {
     expect(res.status).toBe(404);
   });
 
+  it("POST /api/repositories rejects file:// URL with 400", async () => {
+    const res = await apiRequest("POST", "/api/repositories", { name: "x", url: "file:///tmp/x" }, userToken);
+    expect(res.status).toBe(400);
+    const body = (await res.json()) as any;
+    expect(body.error.message).toMatch(/file:\/\/\/tmp\/x/);
+  });
+
   // ─── Agents ───
 
   it("GET /api/agents lists agents", async () => {
