[Misc] Gate/scope user-directory enumeration on /users/search and /users/resolve

.changeset/auto-816-user-directory-enumeration.md

@@ -0,0 +1,6 @@
+---
+"ornn-api": patch
+"ornn-web": patch
+---
+
+Harden the user-directory endpoints against enumeration: /users/search now rejects empty and single-character queries, and both /users/search and /users/resolve are rate-limited per user. The collaborator typeahead asks for at least 2 characters before searching.

ornn-api/src/domains/users/routes.test.ts

@@ -0,0 +1,227 @@
+/**
+ * User-directory route tests (#816).
+ *
+ * Pins the enumeration hardening for GET /users/search + /users/resolve:
+ *   - empty / 1-char `q` is rejected at the validateQuery seam (400) and
+ *     never reaches the repository (no DB hit, no directory walk).
+ *   - a real ≥2-char prefix returns 200 and the email field stays in the
+ *     response (the collaborator typeahead matches on email prefix —
+ *     positive control so a later "strip email" change can't silently
+ *     break the picker).
+ *   - both routes share ONE per-user rate-limit budget (`users-directory`
+ *     label) — bursting either past the cap yields 429 with Retry-After
+ *     and RateLimit-Remaining: 0; the limit is shared so an enumerator
+ *     can't dodge the search cap by pivoting to resolve.
+ *
+ * The route module reads its limit/window/min-q from env at import time
+ * (module-level consts), so we set a small per-min cap BEFORE importing
+ * it via a dynamic import — keeps the burst loop short.
+ *
+ * @module domains/users/routes.test
+ */
+
+import { beforeEach, describe, expect, test } from "bun:test";
+import { Hono } from "hono";
+import { AppError, buildProblemJsonBody } from "../../shared/types/index";
+import type { AuthVariables } from "../../middleware/nyxidAuth";
+import { __resetRateLimitForTests } from "../../middleware/rateLimit";
+
+// --- Pin the env BEFORE the routes module is imported -----------------
+// The module captures RL_MAX / RL_WINDOW_MS / MIN_Q at import time, so a
+// small cap here keeps the burst tests fast and deterministic.
+const TEST_RL_MAX = 5;
+process.env.ORNN_USER_DIRECTORY_RATELIMIT_PER_MIN = String(TEST_RL_MAX);
+process.env.ORNN_USER_DIRECTORY_RATELIMIT_WINDOW_MS = "60000";
+delete process.env.ORNN_USER_SEARCH_MIN_Q; // exercise the default floor of 2
+
+// Dynamic import so the env above is visible when the module's
+// top-level consts are evaluated.
+const { createUserRoutes } = await import("./routes");
+
+// --- Fake repository --------------------------------------------------
+// Spy on searchByEmailPrefix so the "never called on invalid q" cases can
+// assert it. Only the two methods the routes touch are implemented; the
+// cast to the repo type keeps the route factory's contract honest without
+// dragging in Mongo.
+interface SearchSpy {
+  calls: Array<{ prefix: string; limit: number }>;
+}
+
+function makeFakeRepo() {
+  const searchSpy: SearchSpy = { calls: [] };
+  const repo = {
+    async searchByEmailPrefix(prefix: string, limit: number) {
+      searchSpy.calls.push({ prefix, limit });
+      return [
+        {
+          userId: "u-alex",
+          email: "alex@x.test",
+          displayName: "Alex",
+        },
+      ];
+    },
+    async findByUserIds(ids: readonly string[]) {
+      return ids.map((id) => ({
+        userId: id,
+        email: `${id}@x.test`,
+        displayName: id,
+      }));
+    },
+  };
+  // The route factory only calls the two methods above. Cast through
+  // unknown so we don't have to stub the full UserDirectoryRepository.
+  return { repo: repo as unknown as Parameters<typeof createUserRoutes>[0]["userDirectoryRepo"], searchSpy };
+}
+
+// --- App harness ------------------------------------------------------
+// Mirrors the rateLimit.test.ts harness: stub auth so the limiter's
+// default keyBy resolves a per-user bucket, mount the real routes, and
+// translate AppError → problem+json the way the global handler does.
+function makeApp(repo: Parameters<typeof createUserRoutes>[0]["userDirectoryRepo"]) {
+  const app = new Hono<{ Variables: AuthVariables }>();
+  app.use("*", async (c, next) => {
+    const userId = c.req.header("x-test-user") ?? "u1";
+    c.set("auth" as never, { userId, email: `${userId}@x.test` } as never);
+    await next();
+  });
+  app.route("/", createUserRoutes({ userDirectoryRepo: repo }));
+  app.onError((err, c) => {
+    if (err instanceof AppError) {
+      const body = buildProblemJsonBody({
+        statusCode: err.statusCode,
+        code: err.code,
+        message: err.message,
+        instance: c.req.path,
+        requestId: null,
+      });
+      return c.json(body, err.statusCode as never, {
+        "Content-Type": "application/problem+json",
+      });
+    }
+    return c.json({ error: { code: "internal_error", message: String(err) } }, 500);
+  });
+  return app;
+}
+
+describe("GET /users/search — q validation (#816)", () => {
+  beforeEach(() => __resetRateLimitForTests());
+
+  test("empty q → 400 and searchByEmailPrefix is NOT called", async () => {
+    const { repo, searchSpy } = makeFakeRepo();
+    const app = makeApp(repo);
+    const res = await app.request("/users/search?q=", {
+      headers: { "x-test-user": "alice" },
+    });
+    expect(res.status).toBe(400);
+    const body = (await res.json()) as { code: string; status: number };
+    expect(body.code).toBe("invalid_query");
+    expect(body.status).toBe(400);
+    expect(searchSpy.calls.length).toBe(0);
+  });
+
+  test("1-char q → 400 and searchByEmailPrefix is NOT called", async () => {
+    const { repo, searchSpy } = makeFakeRepo();
+    const app = makeApp(repo);
+    const res = await app.request("/users/search?q=a", {
+      headers: { "x-test-user": "bob" },
+    });
+    expect(res.status).toBe(400);
+    expect(searchSpy.calls.length).toBe(0);
+  });
+
+  test("2-char q → 200, spy called once with the prefix, email present", async () => {
+    const { repo, searchSpy } = makeFakeRepo();
+    const app = makeApp(repo);
+    const res = await app.request("/users/search?q=al", {
+      headers: { "x-test-user": "carol" },
+    });
+    expect(res.status).toBe(200);
+    expect(searchSpy.calls.length).toBe(1);
+    expect(searchSpy.calls[0]?.prefix).toBe("al");
+    const body = (await res.json()) as {
+      data: { items: Array<{ email: string; userId: string }> };
+    };
+    // Positive typeahead control: email stays in the response shape.
+    expect(body.data.items[0]?.email).toBe("alex@x.test");
+    // RFC 9239 limit header reflects the configured cap on a 200.
+    expect(res.headers.get("RateLimit-Limit")).toBe(String(TEST_RL_MAX));
+  });
+});
+
+describe("GET /users/search — rate limit (#816)", () => {
+  beforeEach(() => __resetRateLimitForTests());
+
+  test("burst of RL_MAX+1 (same user) → last is 429 with Retry-After + Remaining 0", async () => {
+    const { repo } = makeFakeRepo();
+    const app = makeApp(repo);
+    const headers = { "x-test-user": "dave" };
+
+    // First RL_MAX requests succeed.
+    for (let i = 0; i < TEST_RL_MAX; i++) {
+      const ok = await app.request("/users/search?q=al", { headers });
+      expect(ok.status).toBe(200);
+    }
+    // The next one is over the cap.
+    const denied = await app.request("/users/search?q=al", { headers });
+    expect(denied.status).toBe(429);
+    expect(denied.headers.get("Content-Type")).toContain("application/problem+json");
+    expect(denied.headers.get("Retry-After")).not.toBeNull();
+    expect(denied.headers.get("RateLimit-Remaining")).toBe("0");
+    const body = (await denied.json()) as { code: string; status: number };
+    expect(body.code).toBe("rate_limited");
+    expect(body.status).toBe(429);
+  });
+
+  test("different users have independent budgets", async () => {
+    const { repo } = makeFakeRepo();
+    const app = makeApp(repo);
+    // Exhaust user A.
+    for (let i = 0; i < TEST_RL_MAX; i++) {
+      await app.request("/users/search?q=al", { headers: { "x-test-user": "eve" } });
+    }
+    const aDenied = await app.request("/users/search?q=al", {
+      headers: { "x-test-user": "eve" },
+    });
+    expect(aDenied.status).toBe(429);
+    // User B is untouched.
+    const bOk = await app.request("/users/search?q=al", {
+      headers: { "x-test-user": "frank" },
+    });
+    expect(bOk.status).toBe(200);
+  });
+});
+
+describe("GET /users/resolve — shared rate-limit budget (#816)", () => {
+  beforeEach(() => __resetRateLimitForTests());
+
+  test("burst on /users/resolve → 429 after RL_MAX+1 for one user", async () => {
+    const { repo } = makeFakeRepo();
+    const app = makeApp(repo);
+    const headers = { "x-test-user": "grace" };
+    for (let i = 0; i < TEST_RL_MAX; i++) {
+      const ok = await app.request("/users/resolve?ids=u1,u2", { headers });
+      expect(ok.status).toBe(200);
+    }
+    const denied = await app.request("/users/resolve?ids=u1,u2", { headers });
+    expect(denied.status).toBe(429);
+    expect(denied.headers.get("Retry-After")).not.toBeNull();
+    expect(denied.headers.get("RateLimit-Remaining")).toBe("0");
+  });
+
+  test("search + resolve draw from ONE shared per-user budget (same label)", async () => {
+    const { repo } = makeFakeRepo();
+    const app = makeApp(repo);
+    const headers = { "x-test-user": "heidi" };
+    // Spend the budget across both endpoints: a search + resolves.
+    const first = await app.request("/users/search?q=al", { headers });
+    expect(first.status).toBe(200);
+    for (let i = 1; i < TEST_RL_MAX; i++) {
+      const ok = await app.request("/users/resolve?ids=u1", { headers });
+      expect(ok.status).toBe(200);
+    }
+    // RL_MAX requests spent across the two routes → the (RL_MAX+1)th on
+    // EITHER route is denied because they share the bucket.
+    const denied = await app.request("/users/resolve?ids=u9", { headers });
+    expect(denied.status).toBe(429);
+  });
+});

ornn-api/src/domains/users/routes.ts

@@ -16,10 +16,36 @@ import {
   nyxidAuthMiddleware,
 } from "../../middleware/nyxidAuth";
 import { validateQuery, getValidatedQuery } from "../../middleware/validate";
+import { rateLimit } from "../../middleware/rateLimit";
+import { createLogger } from "../../shared/logger";
 import type { UserDirectoryRepository } from "./repository";
 
+const logger = createLogger("userRoutes");
+
+/**
+ * Minimum `q` length for the user-directory typeahead (#816). Empty /
+ * 1-char queries are rejected with 400 so an authenticated caller can't
+ * walk the entire directory one prefix at a time (enumeration). Clamped
+ * to a floor of 2 — operators can raise it via env but never below 2.
+ */
+const MIN_Q = Math.max(2, Number(process.env.ORNN_USER_SEARCH_MIN_Q) || 2);
+
+/**
+ * Per-user (per-IP for the rare anon path) burst budget shared across the
+ * whole directory surface (#816). Both /users/search and /users/resolve
+ * mount the SAME limiter label, so the budget is a single shared
+ * allowance — an enumerator can't sidestep the search cap by pivoting to
+ * resolve. Defaults: 30 req / 60s. Env-tunable.
+ */
+const RL_WINDOW_MS = Number(process.env.ORNN_USER_DIRECTORY_RATELIMIT_WINDOW_MS) || 60_000;
+const RL_MAX = Number(process.env.ORNN_USER_DIRECTORY_RATELIMIT_PER_MIN) || 30;
+
 const searchQuerySchema = z.object({
-  q: z.string().max(256).optional().default(""),
+  // #816 — require a real prefix. Empty / 1-char `q` now 400s via the
+  // validateQuery seam (dropped `.optional().default("")`). The repo's
+  // empty-q branch stays intact because admin/quota/routes.ts depends on
+  // it; the enumeration gate lives HERE in the route, not the repo.
+  q: z.string().trim().min(MIN_Q).max(256),
   limit: z.coerce.number().int().min(1).max(50).optional().default(10),
 });
 
@@ -33,6 +59,14 @@ export function createUserRoutes(
   const { userDirectoryRepo } = config;
   const app = new Hono<{ Variables: AuthVariables }>();
   const auth = nyxidAuthMiddleware();
+  // Shared per-user budget across the whole directory surface (#816).
+  // One limiter instance, one label → /users/search and /users/resolve
+  // draw from the same bucket.
+  const directoryRateLimit = rateLimit({
+    windowMs: RL_WINDOW_MS,
+    max: RL_MAX,
+    label: "users-directory",
+  });
 
   /**
    * GET /users/search?q=<email-prefix>&limit=<N>
@@ -41,13 +75,28 @@ export function createUserRoutes(
    * targets, and we intentionally don't gate this behind admin. Result
    * set is scoped to users who have actually interacted with Ornn
    * (have a directory row).
+   *
+   * Issue #816 — option (a): reject empty/1-char q + per-user rate
+   * limit. Email stays in the response because the collaborator
+   * typeahead matches on email prefix; the repository empty-q branch is
+   * intentionally left intact because admin/quota/routes.ts depends on
+   * it — the enumeration gate lives HERE in the route, not the repo.
+   *
+   * Mount order: auth → directoryRateLimit → validateQuery → handler,
+   * so the limiter keys on the authenticated userId (set upstream) and
+   * runs before the schema check.
    */
   app.get(
     "/users/search",
     auth,
+    directoryRateLimit,
     validateQuery(searchQuerySchema, "invalid_query"),
     async (c) => {
       const parsed = getValidatedQuery<z.infer<typeof searchQuerySchema>>(c);
+      logger.debug(
+        { qLen: parsed.q.length, limit: parsed.limit },
+        "user directory search",
+      );
       const results = await userDirectoryRepo.searchByEmailPrefix(
         parsed.q,
         parsed.limit,
@@ -65,8 +114,12 @@ export function createUserRoutes(
    * email-prefix search can't match on a UUID, so we need a direct
    * id→row lookup. Unknown ids (users who never signed into Ornn)
    * are silently dropped from the response.
+   *
+   * Shares the `users-directory` rate-limit budget with /users/search
+   * (#816) — same label, same per-user bucket — so an enumerator can't
+   * dodge the search cap by pivoting to resolve.
    */
-  app.get("/users/resolve", auth, async (c) => {
+  app.get("/users/resolve", auth, directoryRateLimit, async (c) => {
     const raw = c.req.query("ids") ?? "";
     const ids = raw
       .split(",")

ornn-web/src/components/skill/PermissionsModal.tsx

@@ -95,7 +95,11 @@ export function PermissionsModal({ isOpen, onClose, skill }: PermissionsModalPro
   }, [isOpen, skill]);
 
   const debouncedQuery = useDebouncedValue(userQuery.trim(), 200);
-  const shouldSearch = !isPublic && (userInputFocused || debouncedQuery.length > 0);
+  // Only query at 2+ chars. The backend now rejects empty/1-char `q` with
+  // 400 (#816), so firing on bare focus or a single keystroke would surface
+  // an error toast for a query the user never finished typing. Below the
+  // threshold we render a "keep typing" hint instead of a results list.
+  const shouldSearch = !isPublic && debouncedQuery.length >= 2;
   const { data: suggestions = [] } = useQuery({
     queryKey: ["users-search", debouncedQuery],
     queryFn: () => searchUsersByEmail(debouncedQuery, 8),
@@ -417,6 +421,19 @@ export function PermissionsModal({ isOpen, onClose, skill }: PermissionsModalPro
                   className="w-full bg-card rounded border border-accent/20 bg-elevated px-3 py-2 font-text text-sm text-strong focus:outline-none focus:border-accent/60"
                   disabled={isPublic}
                 />
+                {userInputFocused &&
+                  !isPublic &&
+                  debouncedQuery.length < 2 &&
+                  userQuery.trim().length < 2 && (
+                    <div className="absolute left-0 right-0 bottom-full mb-1 z-10 rounded bg-card border border-accent/20 card-impression">
+                      <p className="px-3 py-2 font-text text-xs text-meta italic">
+                        {t(
+                          "permissions.searchHint",
+                          "Type at least 2 characters to search.",
+                        )}
+                      </p>
+                    </div>
+                  )}
                 {userInputFocused && suggestions.length > 0 && (
                   <div className="absolute left-0 right-0 bottom-full mb-1 z-10 rounded bg-card border border-accent/20 card-impression max-h-52 overflow-y-auto">
                     {suggestions.map((s) => (

ornn-web/src/i18n/en.json

@@ -421,6 +421,7 @@
     "privateTitle": "Private",
     "privateDesc": "Only you and platform admins can see this skill. Active when nothing above is set.",
     "searchPlaceholder": "type an email to find a user...",
+    "searchHint": "Type at least 2 characters to search.",
     "removeUser": "Remove",
     "noOrgs": "No organizations to choose from.",
     "noUsersYet": "No users added yet.",

ornn-web/src/i18n/zh.json

@@ -421,6 +421,7 @@
     "privateTitle": "私有",
     "privateDesc": "只有你和平台管理员能看到此技能。当上面所有选项都未启用时自动生效。",
     "searchPlaceholder": "输入邮箱查找用户…",
+    "searchHint": "请输入至少 2 个字符进行搜索。",
     "removeUser": "移除",
     "noOrgs": "没有可选择的组织。",
     "noUsersYet": "还没有添加用户。",