[Bug] GenerateSkillModal sends the user's access token to an arbitrary external host (and in the JSON body)

.changeset/web-generate-modal-token-leak-1037.md

@@ -0,0 +1,5 @@
+---
+"ornn-web": patch
+---
+
+Harden the "Generate skill" modal (security). The user's access token is no longer attached as a bearer header when fetching an OpenAPI spec from a non-NyxID host (it was previously sent to whatever host the service's spec URL pointed at), and the redundant `userToken`/`proxyUrl` fields are dropped from the generate request body. The markdown-reference upload now enforces a 10 MiB per-file size limit before reading the file into memory. (#1037)

ornn-web/src/components/form/FolderFileUpload.tsx

@@ -30,6 +30,7 @@
 import { useState, useRef, useCallback } from "react";
 import { UPLOADABLE_FOLDERS, type UploadableFolder } from "@/types/skillPackage";
 import { formatFileSize } from "@/utils/formatters";
+import { MAX_UPLOAD_FILE_SIZE_BYTES } from "@/utils/constants";
 import { useTranslation } from "react-i18next";
 
 export interface FolderFileUploadProps {
@@ -49,13 +50,6 @@ const FOLDER_LABELS: Record<UploadableFolder, string> = {
   assets: "assets/",
 };
 
-/**
- * Per-file size cap (#655). 10 MiB. Backend caps total uncompressed at
- * ~100 MB; per-file 10 MiB keeps a single oversize artifact from
- * eating the whole package budget and signals the limit early.
- */
-const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024;
-
 export function FolderFileUpload({
   files,
   onUpload,
@@ -72,12 +66,12 @@ export function FolderFileUpload({
     (file: File) => {
       // #655 guard 1 — size cap. Reject before the parent ever sees
       // the file, so its package-state stays clean.
-      if (file.size > MAX_FILE_SIZE_BYTES) {
+      if (file.size > MAX_UPLOAD_FILE_SIZE_BYTES) {
         setUploadError(
           t("guided.fileTooLarge", {
             name: file.name,
             size: formatFileSize(file.size),
-            max: formatFileSize(MAX_FILE_SIZE_BYTES),
+            max: formatFileSize(MAX_UPLOAD_FILE_SIZE_BYTES),
           }),
         );
         return;
@@ -195,7 +189,7 @@ export function FolderFileUpload({
       {/* Per-file size hint — always visible so users know the cap
           before they pick the file. */}
       <p className="font-text text-[11px] text-meta/60">
-        {t("guided.fileSizeHint", { max: formatFileSize(MAX_FILE_SIZE_BYTES) })}
+        {t("guided.fileSizeHint", { max: formatFileSize(MAX_UPLOAD_FILE_SIZE_BYTES) })}
       </p>
 
       {/* File list grouped by folder */}

ornn-web/src/components/skill/GenerateSkillModal.test.tsx

@@ -0,0 +1,204 @@
+/**
+ * GenerateSkillModal tests — pins the #1037 security contract.
+ *
+ * Three independent regressions are locked in here:
+ *
+ *   1. Bearer scoping. The OpenAPI spec is fetched with a raw
+ *      `fetch(openapiRef.value)`. The user's access token may ONLY ride
+ *      along when the spec URL's origin matches the configured NyxID
+ *      proxy host (the cross-origin host that legitimately needs it).
+ *      A spec URL pointing at any other host must get NO Authorization
+ *      header — otherwise the token leaks cross-origin.
+ *
+ *   2. Dead body fields. The generate request once posted
+ *      `userToken: accessToken` (the bearer, in a JSON body) and a
+ *      `proxyUrl` — both unread by the server. Neither may appear in
+ *      the apiPost body.
+ *
+ *   3. Upload size cap. The markdown-reference upload reads the whole
+ *      file into memory via FileReader.readAsText. A >10 MiB `.md`
+ *      file must be rejected before that read, with an inline error and
+ *      no reference added. Non-`.md` files stay rejected too.
+ *
+ * @module components/skill/GenerateSkillModal.test
+ */
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { render, fireEvent, screen, waitFor, cleanup } from "@testing-library/react";
+
+// vi.mock factories are hoisted above module-scope consts, so the host
+// literal is published through vi.hoisted to be reachable from both the
+// config factory below and the test bodies.
+const { NYXID_HOST, ACCESS_TOKEN } = vi.hoisted(() => ({
+  NYXID_HOST: "https://nyx-api.example.com",
+  ACCESS_TOKEN: "test-access-token",
+}));
+
+vi.mock("@/config", () => ({
+  config: {
+    apiBaseUrl: "",
+    nyxidApiBaseUrl: NYXID_HOST,
+    nyxidWebBaseUrl: "https://nyx.example.com",
+    nyxidOauthAuthorizeUrl: "",
+    nyxidOauthTokenUrl: "",
+    nyxidOauthClientId: "",
+    nyxidOauthRedirectUri: "",
+    nyxidLogoutUrl: "",
+    nyxidSettingsUrl: "",
+    posthogApiKey: "",
+    posthogProjectId: "",
+    posthogHost: "",
+  },
+}));
+
+// apiPost captured so we can assert the generate request body.
+const apiPost = vi.fn();
+vi.mock("@/services/apiClient", () => ({
+  apiPost: (...args: unknown[]) => apiPost(...args),
+}));
+
+// Fixed bearer for the trusted-host assertions (ACCESS_TOKEN hoisted above).
+vi.mock("@/stores/authStore", () => ({
+  useAuthStore: (selector: (s: { accessToken: string | null }) => unknown) =>
+    selector({ accessToken: ACCESS_TOKEN }),
+}));
+
+// Echo i18n keys + interpolated opts so assertions can match on the key.
+vi.mock("react-i18next", () => ({
+  useTranslation: () => ({
+    t: (key: string, opts?: Record<string, unknown>) => {
+      if (!opts) return key;
+      const parts = Object.entries(opts).map(([k, v]) => `${k}=${v}`).join(" ");
+      return `${key} (${parts})`;
+    },
+  }),
+}));
+
+// translateError just surfaces the fallback message in tests.
+vi.mock("@/utils/translateError", () => ({
+  translateError: (_err: unknown, fallback: string) => fallback,
+}));
+
+import { GenerateSkillModal } from "./GenerateSkillModal";
+
+function makeFile(name: string, sizeBytes: number, type = "text/markdown"): File {
+  const blob = new Blob([new Uint8Array(sizeBytes)], { type });
+  return new File([blob], name, { type });
+}
+
+function setup(openapiSpecUrl: string | null) {
+  const onSuccess = vi.fn();
+  const onClose = vi.fn();
+  const utils = render(
+    <GenerateSkillModal
+      isOpen
+      onClose={onClose}
+      onSuccess={onSuccess}
+      serviceId="svc-1"
+      serviceName="Example Service"
+      openapiSpecUrl={openapiSpecUrl}
+      repositoryUrl={null}
+      homepageUrl={null}
+    />,
+  );
+  return { onSuccess, onClose, ...utils };
+}
+
+/** Grab the headers object handed to the most recent fetch() call. */
+function lastFetchHeaders(fetchMock: ReturnType<typeof vi.fn>): Record<string, string> {
+  const call = fetchMock.mock.calls.at(-1);
+  const init = (call?.[1] ?? {}) as RequestInit;
+  return (init.headers ?? {}) as Record<string, string>;
+}
+
+describe("GenerateSkillModal (#1037)", () => {
+  beforeEach(() => {
+    apiPost.mockReset();
+    apiPost.mockResolvedValue({ data: { guid: "g", name: "example-skill", serviceId: "svc-1" }, error: null });
+  });
+
+  it("withholds the bearer when the spec host is NOT the NyxID host", async () => {
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      json: async () => ({ openapi: "3.0.0" }),
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    setup("https://evil.attacker.example.com/api/openapi.json");
+    fireEvent.click(screen.getByText("Proceed"));
+
+    await waitFor(() => expect(fetchMock).toHaveBeenCalled());
+    const headers = lastFetchHeaders(fetchMock);
+    expect(headers.Authorization).toBeUndefined();
+
+    vi.unstubAllGlobals();
+    cleanup();
+  });
+
+  it("attaches the bearer when the spec host IS the NyxID host", async () => {
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      json: async () => ({ openapi: "3.0.0" }),
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    setup(`${NYXID_HOST}/api/v1/proxy/s/example/api/openapi.json`);
+    fireEvent.click(screen.getByText("Proceed"));
+
+    await waitFor(() => expect(fetchMock).toHaveBeenCalled());
+    const headers = lastFetchHeaders(fetchMock);
+    expect(headers.Authorization).toBe(`Bearer ${ACCESS_TOKEN}`);
+
+    vi.unstubAllGlobals();
+    cleanup();
+  });
+
+  it("posts neither userToken nor proxyUrl in the generate body", async () => {
+    const fetchMock = vi.fn().mockResolvedValue({
+      ok: true,
+      json: async () => ({ openapi: "3.0.0" }),
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    setup(`${NYXID_HOST}/api/v1/proxy/s/example/api/openapi.json`);
+    fireEvent.click(screen.getByText("Proceed"));
+
+    await waitFor(() => expect(apiPost).toHaveBeenCalled());
+    const [, body] = apiPost.mock.calls[0] as [string, Record<string, unknown>];
+    expect(body).not.toHaveProperty("userToken");
+    expect(body).not.toHaveProperty("proxyUrl");
+    expect(body).toHaveProperty("references");
+    expect(body).toHaveProperty("serviceName", "Example Service");
+
+    vi.unstubAllGlobals();
+    cleanup();
+  });
+
+  it("rejects an oversize .md upload without adding a reference", async () => {
+    setup(null);
+    // The Modal renders through createPortal into document.body, so the
+    // hidden file input lives outside the render container.
+    const fileInput = document.body.querySelector('input[type="file"]') as HTMLInputElement;
+
+    const huge = makeFile("big.md", 11 * 1024 * 1024);
+    fireEvent.change(fileInput, { target: { files: [huge] } });
+
+    expect(await screen.findByText(/guided\.fileTooLarge/)).toBeInTheDocument();
+    // No markdown chip was added (the filename never renders as a label).
+    expect(screen.queryByText("big.md")).not.toBeInTheDocument();
+    cleanup();
+  });
+
+  it("rejects a non-.md upload", async () => {
+    setup(null);
+    // The Modal renders through createPortal into document.body, so the
+    // hidden file input lives outside the render container.
+    const fileInput = document.body.querySelector('input[type="file"]') as HTMLInputElement;
+
+    const wrong = makeFile("notes.txt", 1024, "text/plain");
+    fireEvent.change(fileInput, { target: { files: [wrong] } });
+
+    expect(await screen.findByText(/Only \.md/)).toBeInTheDocument();
+    expect(screen.queryByText("notes.txt")).not.toBeInTheDocument();
+    cleanup();
+  });
+});

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

@@ -12,6 +12,9 @@ import { Badge } from "@/components/ui/Badge";
 import { Input } from "@/components/ui/Input";
 import { useAuthStore } from "@/stores/authStore";
 import { apiPost } from "@/services/apiClient";
+import { config } from "@/config";
+import { MAX_UPLOAD_FILE_SIZE_BYTES } from "@/utils/constants";
+import { formatFileSize } from "@/utils/formatters";
 import { translateError } from "@/utils/translateError";
 
 interface Reference {
@@ -29,7 +32,6 @@ interface GenerateSkillModalProps {
   onSuccess: (skillName: string) => void;
   serviceId: string;
   serviceName: string;
-  proxyUrl: string;
   openapiSpecUrl: string | null;
   repositoryUrl: string | null;
   homepageUrl: string | null;
@@ -58,13 +60,28 @@ function isValidUrl(str: string): boolean {
   }
 }
 
+/**
+ * The OpenAPI spec URL is built against the NyxID proxy host (a
+ * cross-origin host that legitimately needs the same bearer as the
+ * `/services/:id` fetch). The bearer must NEVER leak to any other host —
+ * a user-added reference URL could point anywhere. Gate the
+ * Authorization header on an exact origin match with the configured
+ * NyxID API base. Malformed URLs fail closed (no header attached).
+ */
+function isTrustedSpecHost(url: string): boolean {
+  try {
+    return new URL(url).origin === new URL(config.nyxidApiBaseUrl).origin;
+  } catch {
+    return false;
+  }
+}
+
 export function GenerateSkillModal({
   isOpen,
   onClose,
   onSuccess,
   serviceId,
   serviceName,
-  proxyUrl,
   openapiSpecUrl,
   repositoryUrl,
   homepageUrl,
@@ -128,6 +145,22 @@ export function GenerateSkillModal({
       setUrlError("Only .md (markdown) files are supported");
       return;
     }
+    // Reject oversize files BEFORE reading the whole thing into memory.
+    // FileReader.readAsText buffers the entire file, so a 100 MB upload
+    // would otherwise be read whole. Mirror FolderFileUpload's
+    // size-before-process ordering and the shared 10 MiB per-file cap.
+    if (file.size > MAX_UPLOAD_FILE_SIZE_BYTES) {
+      setUrlError(
+        t("guided.fileTooLarge", {
+          name: file.name,
+          size: formatFileSize(file.size),
+          max: formatFileSize(MAX_UPLOAD_FILE_SIZE_BYTES),
+        }),
+      );
+      // Reset the input so re-selecting the same file fires onChange again.
+      if (fileInputRef.current) fileInputRef.current.value = "";
+      return;
+    }
     const reader = new FileReader();
     reader.onload = () => {
       const content = reader.result as string;
@@ -155,8 +188,12 @@ export function GenerateSkillModal({
       if (openapiRef) {
         setCurrentStepMsg(STEP_MESSAGES.fetching_spec);
         try {
+          // Only forward the bearer to the trusted NyxID proxy host. A
+          // user-added spec URL could point anywhere — sending the token
+          // there would leak the user's credential cross-origin.
+          const useAuth = Boolean(accessToken) && isTrustedSpecHost(openapiRef.value);
           const specResp = await fetch(openapiRef.value, {
-            headers: accessToken ? { Authorization: `Bearer ${accessToken}` } : {},
+            headers: useAuth ? { Authorization: `Bearer ${accessToken}` } : {},
           });
           if (specResp.ok) {
             const specJson = await specResp.json();
@@ -191,8 +228,6 @@ export function GenerateSkillModal({
       const res = await apiPost<{ guid: string; name: string; serviceId: string }>(
         `/api/v1/admin/system-skills/${serviceId}/generate`,
         {
-          userToken: accessToken,
-          proxyUrl,
           references: promptRefs,
           serviceName,
         },

ornn-web/src/pages/ServiceDetailPage.tsx

@@ -183,7 +183,6 @@ export function ServiceDetailPage() {
             }}
             serviceId={service.id}
             serviceName={service.name}
-            proxyUrl={service.proxyUrl}
             openapiSpecUrl={service.openapiProxyUrl}
             repositoryUrl={service.repositoryUrl}
             homepageUrl={service.homepageUrl}

ornn-web/src/utils/constants.ts

@@ -76,3 +76,13 @@ export const MAX_FILE_SIZE_BYTES = 50 * 1024 * 1024;
 export const MAX_FILE_SIZE_LABEL = "50 MB";
 export const ACCEPTED_FILE_TYPES = [".tar.gz", ".zip"];
 export const MAX_TAGS = 10;
+
+/**
+ * Per-file upload cap (10 MiB) shared by the package builder's
+ * FolderFileUpload and the GenerateSkillModal markdown-reference upload.
+ * The backend / ZIP pipeline caps total uncompressed at ~100 MB; this
+ * per-file ceiling keeps a single oversize artifact (e.g. a 100 MB file
+ * read whole into memory via FileReader) from eating the whole budget
+ * and surfaces the limit before the user assembles a doomed package.
+ */
+export const MAX_UPLOAD_FILE_SIZE_BYTES = 10 * 1024 * 1024;