diff --git a/.changeset/web-generate-modal-token-leak-1037.md b/.changeset/web-generate-modal-token-leak-1037.md new file mode 100644 index 00000000..e99d0777 --- /dev/null +++ b/.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) diff --git a/ornn-web/src/components/form/FolderFileUpload.tsx b/ornn-web/src/components/form/FolderFileUpload.tsx index 59c525bc..b83e6ccf 100644 --- a/ornn-web/src/components/form/FolderFileUpload.tsx +++ b/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 = { 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. */}

- {t("guided.fileSizeHint", { max: formatFileSize(MAX_FILE_SIZE_BYTES) })} + {t("guided.fileSizeHint", { max: formatFileSize(MAX_UPLOAD_FILE_SIZE_BYTES) })}

{/* File list grouped by folder */} diff --git a/ornn-web/src/components/skill/GenerateSkillModal.test.tsx b/ornn-web/src/components/skill/GenerateSkillModal.test.tsx new file mode 100644 index 00000000..781ce09d --- /dev/null +++ b/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) => { + 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( + , + ); + return { onSuccess, onClose, ...utils }; +} + +/** Grab the headers object handed to the most recent fetch() call. */ +function lastFetchHeaders(fetchMock: ReturnType): Record { + const call = fetchMock.mock.calls.at(-1); + const init = (call?.[1] ?? {}) as RequestInit; + return (init.headers ?? {}) as Record; +} + +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]; + 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(); + }); +}); diff --git a/ornn-web/src/components/skill/GenerateSkillModal.tsx b/ornn-web/src/components/skill/GenerateSkillModal.tsx index 683579b5..073077f6 100644 --- a/ornn-web/src/components/skill/GenerateSkillModal.tsx +++ b/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, }, diff --git a/ornn-web/src/pages/ServiceDetailPage.tsx b/ornn-web/src/pages/ServiceDetailPage.tsx index a88aacab..8f3d0eb6 100644 --- a/ornn-web/src/pages/ServiceDetailPage.tsx +++ b/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} diff --git a/ornn-web/src/utils/constants.ts b/ornn-web/src/utils/constants.ts index 44633141..cb098da6 100644 --- a/ornn-web/src/utils/constants.ts +++ b/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;