diff --git a/.changeset/auto-806-playground-skill-bola.md b/.changeset/auto-806-playground-skill-bola.md new file mode 100644 index 00000000..ea9af75b --- /dev/null +++ b/.changeset/auto-806-playground-skill-bola.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Fix an authorization gap where the playground could load a private skill's full contents without checking the caller's read access. `getSkillJson` now requires a caller actor and enforces `canReadSkill` for both the `skillId` and `load_skill` paths, so a private skill is only readable by its owner, users/orgs it is shared with, or a platform admin. diff --git a/.changeset/auto-807-skill-name-traversal.md b/.changeset/auto-807-skill-name-traversal.md new file mode 100644 index 00000000..b9a16fc8 --- /dev/null +++ b/.changeset/auto-807-skill-name-traversal.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Reject path-traversal skill names on the `skip_validation` import path. The lenient frontmatter extractor now enforces the same kebab-case name rule as the strict path, and the GitHub-mirror folder builder refuses unsafe names, preventing a crafted skill name from writing outside its own folder in the public mirror. diff --git a/.changeset/auto-808-quota-toctou.md b/.changeset/auto-808-quota-toctou.md new file mode 100644 index 00000000..f6e9b7b8 --- /dev/null +++ b/.changeset/auto-808-quota-toctou.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Close a quota time-of-check/time-of-use race: the per-user/surface quota is now reserved atomically at check time (a conditional increment guarded by the cap) instead of being read first and charged after the LLM call, so concurrent requests can no longer exceed the cap. Failed or aborted calls release the reservation. diff --git a/.changeset/auto-809-playground-chat-rate-limit.md b/.changeset/auto-809-playground-chat-rate-limit.md new file mode 100644 index 00000000..9567a0cc --- /dev/null +++ b/.changeset/auto-809-playground-chat-rate-limit.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Add per-user rate limit (20/min) to POST /playground/chat (#809). diff --git a/.changeset/auto-810-bound-skip-dos.md b/.changeset/auto-810-bound-skip-dos.md new file mode 100644 index 00000000..d64e5130 --- /dev/null +++ b/.changeset/auto-810-bound-skip-dos.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Bound unbounded MongoDB skip() pagination (CWE-770): page ceiling + maxTimeMS on public skill queries (#810). diff --git a/.changeset/auto-811-ssrf-dns-rebind-parity.md b/.changeset/auto-811-ssrf-dns-rebind-parity.md new file mode 100644 index 00000000..a98a606f --- /dev/null +++ b/.changeset/auto-811-ssrf-dns-rebind-parity.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Close DNS-rebind SSRF gap: route chrono-storage, chrono-sandbox, and all NyxID/LLM-gateway outbound requests through a shared fetch-time assertPublicResolvedAddress preflight (#811). diff --git a/.changeset/auto-812-idempotency-sse-passthrough.md b/.changeset/auto-812-idempotency-sse-passthrough.md new file mode 100644 index 00000000..50b0aa12 --- /dev/null +++ b/.changeset/auto-812-idempotency-sse-passthrough.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Idempotency middleware skips capture for streaming (text/event-stream) responses so SSE streams are delivered unbuffered and never persisted (#812). diff --git a/.changeset/auto-813-rate-limit-xff-trusted-hop.md b/.changeset/auto-813-rate-limit-xff-trusted-hop.md new file mode 100644 index 00000000..4abc26bc --- /dev/null +++ b/.changeset/auto-813-rate-limit-xff-trusted-hop.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Rate-limit keys anonymous traffic on a trusted-position X-Forwarded-For hop (configurable via ORNN_TRUSTED_PROXY_HOPS), not the spoofable leftmost token (#813, CWE-348). diff --git a/.changeset/auto-814-rate-limit-single-replica-contract.md b/.changeset/auto-814-rate-limit-single-replica-contract.md new file mode 100644 index 00000000..298a4087 --- /dev/null +++ b/.changeset/auto-814-rate-limit-single-replica-contract.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Harden the rate limiter's single-replica by-design contract (code + deployment guard) and pin per-pod isolation under test; shared-store backing for multi-replica is tracked in #837 (#814). diff --git a/.changeset/auto-815-share-non-member-org.md b/.changeset/auto-815-share-non-member-org.md new file mode 100644 index 00000000..40fd437d --- /dev/null +++ b/.changeset/auto-815-share-non-member-org.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Fix CWE-862 (#815): PUT /skills/:id/permissions rejects sharing a skill into an org the caller is not a member of (403 not_org_member); platform admins exempt. diff --git a/.changeset/auto-817-pino-redact-secrets.md b/.changeset/auto-817-pino-redact-secrets.md new file mode 100644 index 00000000..b48dc596 --- /dev/null +++ b/.changeset/auto-817-pino-redact-secrets.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Harden log redaction: token, accessToken, userAccessToken, clientSecret and privateKey are now censored in all Pino logger roots (shared logger, bootstrap, entrypoint), sourced from a single exported REDACT_PATHS constant. diff --git a/.changeset/auto-818-github-identifier-hardening.md b/.changeset/auto-818-github-identifier-hardening.md new file mode 100644 index 00000000..c54f1471 --- /dev/null +++ b/.changeset/auto-818-github-identifier-hardening.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Harden GitHub identifier validation: mirror settings owner/repo now enforce the same naming rules as the mirror routes (shared constants), and repo pull identifiers reject "." / ".." path-traversal segments. diff --git a/.changeset/auto-819-clamp-sandbox-timeout.md b/.changeset/auto-819-clamp-sandbox-timeout.md new file mode 100644 index 00000000..562d25aa --- /dev/null +++ b/.changeset/auto-819-clamp-sandbox-timeout.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Clamp the playground sandbox timeout_secs to the advertised 1-600 range and default non-numeric values to 60s. diff --git a/.changeset/auto-820-remove-dead-authz-helper.md b/.changeset/auto-820-remove-dead-authz-helper.md new file mode 100644 index 00000000..201341ff --- /dev/null +++ b/.changeset/auto-820-remove-dead-authz-helper.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Remove the unused requireOwnerOrAdmin authz middleware (dead code; the live skill-ownership policy is canManageSkill). diff --git a/.changeset/auto-821-encryption-key-jsdoc.md b/.changeset/auto-821-encryption-key-jsdoc.md new file mode 100644 index 00000000..d6452768 --- /dev/null +++ b/.changeset/auto-821-encryption-key-jsdoc.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Correct the SkillConfig.encryptionKey JSDoc to match the enforced contract (mandatory ≥32 chars, no dev fallback, fail-fast at boot) and add tests pinning loadConfig() ConfigError behavior. diff --git a/.changeset/auto-826-actor-plumbing.md b/.changeset/auto-826-actor-plumbing.md new file mode 100644 index 00000000..56139915 --- /dev/null +++ b/.changeset/auto-826-actor-plumbing.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Require an explicit authorization actor on the playground chat path (drop the SYSTEM_ACTOR fallback) and de-duplicate the route-level actor builds behind a single buildActorContext helper so they cannot drift. diff --git a/.changeset/auto-827-quota-reservation-followups.md b/.changeset/auto-827-quota-reservation-followups.md new file mode 100644 index 00000000..1f002a6f --- /dev/null +++ b/.changeset/auto-827-quota-reservation-followups.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Thread the quota reservation timestamp through to charge-on-completion so per-model analytics reconcile against the reserved month bucket (fixes a benign month-boundary straddle). Add route-level integration tests covering model-resolution failure (used unchanged) and aborted/errored streams (slot released). Note for consumers: /me/quota remaining reflects in-flight reservations — used is bumped at reserve time and refunded on system-error/abort. diff --git a/.changeset/auto-832-safefetch-redirect-ssrf.md b/.changeset/auto-832-safefetch-redirect-ssrf.md new file mode 100644 index 00000000..4a8b963a --- /dev/null +++ b/.changeset/auto-832-safefetch-redirect-ssrf.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +Close an SSRF redirect-hop bypass: safeFetch now follows redirects via a bounded manual loop that re-validates each hop's host against the public-address guard and strips cross-host credentials, instead of blindly following 3xx to unvalidated targets. diff --git a/.changeset/auto-842-org-membership-unresolved.md b/.changeset/auto-842-org-membership-unresolved.md new file mode 100644 index 00000000..0ba1df73 --- /dev/null +++ b/.changeset/auto-842-org-membership-unresolved.md @@ -0,0 +1,5 @@ +--- +"ornn-api": patch +--- + +setSkillPermissions now distinguishes an unresolved org-membership read (forwarded token absent or NyxID unreachable) from a confirmed non-membership: sharing a skill into an org while memberships are unresolved returns a retryable 503 org_membership_unavailable instead of a misleading 403 not_org_member. Confirmed non-members still get 403. Read-path visibility is unchanged (still fail-soft). diff --git a/.changeset/auto-859-landing-hero-text-clip.md b/.changeset/auto-859-landing-hero-text-clip.md new file mode 100644 index 00000000..0cfb0e5c --- /dev/null +++ b/.changeset/auto-859-landing-hero-text-clip.md @@ -0,0 +1,5 @@ +--- +"ornn-web": patch +--- + +Fix landing hero text being clipped at short/wide viewport ratios: re-encode the intro video + poster to a 2.4:1 canvas (blurred-scene edge fill) so the burned-in captions survive object-cover cropping, and drop the min-height that forced the hero past the viewport. Video stays full-bleed. diff --git a/.changeset/hero-video-4k.md b/.changeset/hero-video-4k.md new file mode 100644 index 00000000..1c7b6197 --- /dev/null +++ b/.changeset/hero-video-4k.md @@ -0,0 +1,5 @@ +--- +"ornn-web": patch +--- + +Upscale the landing hero intro video + poster to 4K (5184×2160, AI-upscaled from the 1080p master via Real-ESRGAN, ~64MB) for crisper rendering on retina/4K displays. No layout change — the 2.4:1 canvas + object-cover framing from the previous fix is preserved. diff --git a/.changeset/landing-video-hero.md b/.changeset/landing-video-hero.md new file mode 100644 index 00000000..cf32082f --- /dev/null +++ b/.changeset/landing-video-hero.md @@ -0,0 +1,5 @@ +--- +"ornn-web": minor +--- + +Replace the scroll-scrub landing hero with a full-bleed, autoplaying, muted, looping background intro video (static poster under reduced-motion). diff --git a/deployment/.env.sample.ornn b/deployment/.env.sample.ornn index 20a7cbaf..6d0591bd 100644 --- a/deployment/.env.sample.ornn +++ b/deployment/.env.sample.ornn @@ -17,6 +17,7 @@ MAX_PACKAGE_SIZE_BYTES= ALLOWED_ORIGINS= ORNN_PUBLIC_ORIGIN= ORNN_URL_ALLOWLIST_CIDR= +ORNN_TRUSTED_PROXY_HOPS= AGENTSEAL_PYTHON=/opt/agentseal/bin/python AGENTSEAL_SCRIPT=/opt/agentseal/scan_skill.py ENCRYPTION_KEY=<32+ char passphrase, e.g. openssl rand -hex 32> diff --git a/deployment/ornn-api/deployment.yaml b/deployment/ornn-api/deployment.yaml index 37e3f0de..966f4722 100644 --- a/deployment/ornn-api/deployment.yaml +++ b/deployment/ornn-api/deployment.yaml @@ -6,6 +6,8 @@ metadata: labels: app: ornn-api spec: + # Pinned to 1: the in-process rate limiter (ornn-api/src/middleware/rateLimit.ts) keeps per-pod buckets. + # Do NOT raise >1 or add an HPA without a shared-store backend first (#837) — per-pod buckets multiply the effective limit by replica count. replicas: 1 selector: matchLabels: @@ -54,6 +56,8 @@ spec: value: "${ORNN_PUBLIC_ORIGIN}" - name: ORNN_URL_ALLOWLIST_CIDR value: "${ORNN_URL_ALLOWLIST_CIDR}" + - name: ORNN_TRUSTED_PROXY_HOPS + value: "${ORNN_TRUSTED_PROXY_HOPS}" - name: AGENTSEAL_PYTHON value: "${AGENTSEAL_PYTHON}" - name: AGENTSEAL_SCRIPT diff --git a/docs/CONVENTIONS.md b/docs/CONVENTIONS.md index 56953d63..aa6da648 100644 --- a/docs/CONVENTIONS.md +++ b/docs/CONVENTIONS.md @@ -96,6 +96,7 @@ Optional: `detail`, `errors[]`. | `resource_conflict` | 409 | State conflict (duplicate, concurrent modification, etc.) | | `rate_limited` | 429 | Caller exceeded rate limit | | `upstream_unavailable` | 502 / 503 | Dependency (NyxID, LLM, sandbox, ...) failed | +| `org_membership_unavailable` | 503 | NyxID org-membership lookup unresolved — forwarded token absent or lookup failed. Retryable | | `internal_error` | 500 | Unhandled server error | New codes require convention-doc update. Handlers MUST NOT invent ad-hoc codes. diff --git a/ornn-api/src/bootstrap.ts b/ornn-api/src/bootstrap.ts index 06039a2d..f4aad46d 100644 --- a/ornn-api/src/bootstrap.ts +++ b/ornn-api/src/bootstrap.ts @@ -147,24 +147,36 @@ import { buildSpec } from "./openapi/specBuilder"; // Error handler import { AppError, buildProblemJsonBody } from "./shared/types/index"; +// Shared redaction list — single source of truth for sensitive log +// fields, shared with index.ts and the createLogger factory. +import { REDACT_PATHS } from "./shared/logger"; + export interface BootstrapResult { app: Hono; shutdown: () => Promise; } -export async function bootstrap(config: SkillConfig): Promise { +/** + * Test-only dependency overrides. Production callers pass nothing — the + * single optional second argument lets integration tests substitute the + * `NyxLlmClient` with an in-process fake (see `tests/mocks/llmGateway.ts`) + * so quota-charge / per-model accounting flows run without real network + * IO. The override, when present, replaces the single `nyxLlmClient` that + * the playground, skill-gen, search, and audit domains all share. + */ +export interface BootstrapOverrides { + /** Substitute the shared LLM gateway client (integration tests only). */ + llmClient?: NyxLlmClient; +} + +export async function bootstrap( + config: SkillConfig, + overrides?: BootstrapOverrides, +): Promise { const logger = pino({ level: config.logLevel, ...(config.logPretty ? { transport: { target: "pino-pretty" } } : {}), - redact: { - paths: [ - "req.headers.authorization", - "req.headers[\"x-api-key\"]", - "*.password", - "*.secret", - "*.apiKey", - ], - }, + redact: { paths: REDACT_PATHS }, }).child({ service: "ornn-api" }); logger.info("Bootstrapping ornn-api service..."); @@ -436,10 +448,12 @@ export async function bootstrap(config: SkillConfig): Promise { // selection still resolves through this single client. Backend-eng-2 // will swap this for a per-surface provider lookup once the // `llm_providers` collection ships. - const nyxLlmClient = new NyxLlmClient({ - resolver: async () => resolveLlmProviderForSurface("playground"), - saTokenProvider, - }); + const nyxLlmClient = + overrides?.llmClient ?? + new NyxLlmClient({ + resolver: async () => resolveLlmProviderForSurface("playground"), + saTokenProvider, + }); // ---- Repositories ---- const skillRepo = new SkillRepository(db); diff --git a/ornn-api/src/clients/llmModelListClient.ssrf.test.ts b/ornn-api/src/clients/llmModelListClient.ssrf.test.ts new file mode 100644 index 00000000..d4c04e84 --- /dev/null +++ b/ornn-api/src/clients/llmModelListClient.ssrf.test.ts @@ -0,0 +1,101 @@ +/** + * Tests for #832: `LlmModelListClient` routes BOTH of its outbound + * sites through `safeFetch`, so each is refused when its host rebinds to + * 169.254.169.254 at fetch time: + * + * 1. The model-list URL fetch (`fetch({ modelListUrl, auth })`). + * 2. The OAuth2 `tokenUrl` client_credentials exchange, taken when + * `auth.kind === "tokenUrl"` — an explicitly named site because it + * POSTs `client_secret` to an operator-supplied host. The refusal + * MUST fire before the secret leaves the process. + * + * The client catches `SsrfRefusalError` and rethrows it as-is, so the + * caller still sees the refusal type. We assert the fetch spy recorded + * zero calls for the refused site. + * + * dns is stubbed before the client imports so the shared preflight + * (bound in `url.ts` at module load) sees the rebind — same host-aware + * idiom as the nyxid ssrf tests. + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; +import type { + ApiFormat, + LlmProviderAuth, +} from "../domains/settings/llmProviders/types"; + +const REBIND_HOST = "rebind.test"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { LlmModelListClient } = await import("./llmModelListClient"); +const { SsrfRefusalError } = await import("../infra/url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +beforeEach(() => { + fetchCalls = []; + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push(url); + return new Response(JSON.stringify({ data: [], access_token: "tok" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +const apiFormat: ApiFormat = "responses"; + +describe("LlmModelListClient SSRF preflight (#832)", () => { + it("refuses a rebound modelListUrl before reading the catalog", async () => { + const auth: LlmProviderAuth = { kind: "apiKey", apiKey: "secret-key" }; + await expect( + new LlmModelListClient().fetch({ + modelListUrl: "http://rebind.test/v1/models", + apiFormat, + auth, + }), + ).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("refuses a rebound OAuth2 tokenUrl before POSTing client_secret", async () => { + // Named site: the auth.kind === "tokenUrl" client_credentials + // exchange. tokenUrl rebinds to the metadata host — the preflight + // must refuse before `client_secret` is posted. The model-list URL + // is a benign public host that is never reached because auth header + // construction fails first. + const auth: LlmProviderAuth = { + kind: "tokenUrl", + tokenUrl: "http://rebind.test/oauth/token", + clientId: "cid", + clientSecret: "super-secret", + }; + await expect( + new LlmModelListClient().fetch({ + modelListUrl: "http://public-models.example.com/v1/models", + apiFormat, + auth, + }), + ).rejects.toBeInstanceOf(SsrfRefusalError); + // Neither the token endpoint nor the model-list endpoint was hit. + expect(fetchCalls).toHaveLength(0); + }); +}); diff --git a/ornn-api/src/clients/llmModelListClient.ts b/ornn-api/src/clients/llmModelListClient.ts index 66994ac7..1e2ffab7 100644 --- a/ornn-api/src/clients/llmModelListClient.ts +++ b/ornn-api/src/clients/llmModelListClient.ts @@ -38,10 +38,8 @@ import type { LlmProviderAuth, } from "../domains/settings/llmProviders/types"; import type { ModelListFetcher } from "../domains/settings/llmProviders/service"; -import { - assertPublicResolvedAddress, - SsrfRefusalError, -} from "../infra/url"; +import { safeFetch } from "../infra/safeFetch"; +import { SsrfRefusalError } from "../infra/url"; const logger = createLogger("llmModelListClient"); @@ -76,27 +74,6 @@ function redactCredentials(body: string): string { .replace(/"(access_token|api_key|apiKey|token)"\s*:\s*"[^"]*"/g, '"$1":"[REDACTED]"'); } -/** - * Pre-flight SSRF check + AbortSignal wrapper around `fetch`. Centralised - * so the model-list and OAuth2 token-exchange paths share one - * implementation. - */ -async function safeFetch( - url: string, - init: RequestInit, -): Promise { - let parsed: URL; - try { - parsed = new URL(url); - } catch { - throw new Error(`Invalid URL: '${url}'`); - } - await assertPublicResolvedAddress(parsed.hostname); - - const signal = AbortSignal.timeout(FETCH_TIMEOUT_MS); - return fetch(url, { ...init, signal }); -} - export class LlmModelListClient implements ModelListFetcher { /** * Fetch the upstream catalog. Returns `[{ id, displayName }]` — @@ -119,7 +96,10 @@ export class LlmModelListClient implements ModelListFetcher { let resp: Response; try { - resp = await safeFetch(args.modelListUrl, { headers }); + resp = await safeFetch(args.modelListUrl, { + headers, + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), + }); } catch (err) { // Distinguish SSRF refusal (operator-actionable) from generic // transport failures so the admin "Refresh" toast can render @@ -219,6 +199,7 @@ export class LlmModelListClient implements ModelListFetcher { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: body.toString(), + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), }); } catch (err) { if (err instanceof SsrfRefusalError) { diff --git a/ornn-api/src/clients/nyxid/base.ts b/ornn-api/src/clients/nyxid/base.ts index 52e0a374..fc8bb4db 100644 --- a/ornn-api/src/clients/nyxid/base.ts +++ b/ornn-api/src/clients/nyxid/base.ts @@ -17,6 +17,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; const logger = createLogger("nyxidSaTokenProvider"); /** @@ -82,7 +83,7 @@ export class NyxidSaTokenProvider { client_id: clientId, client_secret: clientSecret, }); - const resp = await fetch(tokenUrl, { + const resp = await safeFetch(tokenUrl, { method: "POST", headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: body.toString(), diff --git a/ornn-api/src/clients/nyxid/llm.ssrf.test.ts b/ornn-api/src/clients/nyxid/llm.ssrf.test.ts new file mode 100644 index 00000000..8257f4bb --- /dev/null +++ b/ornn-api/src/clients/nyxid/llm.ssrf.test.ts @@ -0,0 +1,100 @@ +/** + * Tests for #832: the LLM gateway client (`NyxLlmClient`) routes every + * outbound call through `safeFetch`, so a gateway host that rebinds to + * 169.254.169.254 at fetch time is refused before the bearer token is + * sent on the wire. + * + * Both surfaces — `complete()` (non-streaming) and `stream()` (SSE) — + * are named call-sites. We inject a working SA token provider so the + * refusal lands at the GATEWAY fetch (the site under test), not the SA + * token exchange, and assert the `globalThis.fetch` spy recorded zero + * calls (the refusal short-circuits before any network I/O). + * + * dns is stubbed before the client imports so the shared preflight + * (bound in `url.ts` at module load) sees the rebind — same host-aware + * idiom as `ssrf.test.ts`. + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; +import type { NyxidSaTokenProvider } from "./base"; + +const REBIND_HOST = "rebind.test"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { NyxLlmClient } = await import("./llm"); +const { SsrfRefusalError } = await import("../../infra/url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +beforeEach(() => { + fetchCalls = []; + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push(url); + return new Response(JSON.stringify({ output: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +// Working SA token provider — the bearer is ready BEFORE the gateway +// fetch, so any refusal must come from the gateway preflight, proving +// the token never hits the wire. +const stubSa = { + getAccessToken: async () => "sa-token", +} as unknown as NyxidSaTokenProvider; + +function makeClient() { + return new NyxLlmClient({ + resolver: async () => ({ + gatewayUrl: "http://rebind.test", + apiKey: "", + apiFormat: "responses" as const, + }), + saTokenProvider: stubSa, + }); +} + +const params = { + model: "gpt-x", + input: [{ role: "user" as const, content: "hi" }], +}; + +describe("NyxLlmClient SSRF preflight (#832) — LLM gateway sites", () => { + it("complete() refuses a rebound gatewayUrl before sending the bearer", async () => { + await expect(makeClient().complete(params)).rejects.toBeInstanceOf( + SsrfRefusalError, + ); + expect(fetchCalls).toHaveLength(0); + }); + + it("stream() refuses a rebound gatewayUrl before sending the bearer", async () => { + const iter = makeClient().stream(params); + // `stream` is an async generator — the refusal surfaces on first + // pull, before any SSE frame is read. + await expect((async () => { + for await (const _ of iter) { + /* unreachable — refusal throws before the first event */ + } + })()).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); +}); diff --git a/ornn-api/src/clients/nyxid/llm.ts b/ornn-api/src/clients/nyxid/llm.ts index 1f958eca..80201d44 100644 --- a/ornn-api/src/clients/nyxid/llm.ts +++ b/ornn-api/src/clients/nyxid/llm.ts @@ -25,6 +25,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; import type { ApiFormat } from "../../domains/settings/llmProviders/types"; import type { NyxidSaTokenProvider } from "./base"; @@ -375,7 +376,7 @@ export class NyxLlmClient { ? buildChatCompletionBody(params, true) : buildResponsesBody(params, true); - const response = await fetch(`${gatewayUrl}${path}`, { + const response = await safeFetch(`${gatewayUrl}${path}`, { method: "POST", headers: { "Authorization": authHeader, @@ -424,7 +425,7 @@ export class NyxLlmClient { ? buildChatCompletionBody(params, false) : buildResponsesBody(params, false); - const response = await fetch(`${gatewayUrl}${path}`, { + const response = await safeFetch(`${gatewayUrl}${path}`, { method: "POST", headers: { "Authorization": authHeader, diff --git a/ornn-api/src/clients/nyxid/llmCatalog.ts b/ornn-api/src/clients/nyxid/llmCatalog.ts index 665b3159..63b84358 100644 --- a/ornn-api/src/clients/nyxid/llmCatalog.ts +++ b/ornn-api/src/clients/nyxid/llmCatalog.ts @@ -17,6 +17,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; import type { NyxidConfigResolver, NyxidSaTokenProvider } from "./base"; const logger = createLogger("nyxLlmCatalogClient"); @@ -72,7 +73,7 @@ export class NyxLlmCatalogClient { async listUpstreamModels(): Promise { const url = await this.resolveUrl(); const token = await this.saTokenProvider.getAccessToken(); - const resp = await fetch(url, { + const resp = await safeFetch(url, { headers: { Authorization: `Bearer ${token}` }, }); if (!resp.ok) { diff --git a/ornn-api/src/clients/nyxid/orgs.ts b/ornn-api/src/clients/nyxid/orgs.ts index 6ab914dd..2d4f1db7 100644 --- a/ornn-api/src/clients/nyxid/orgs.ts +++ b/ornn-api/src/clients/nyxid/orgs.ts @@ -12,6 +12,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; import type { NyxidConfigResolver, NyxidSaTokenProvider } from "./base"; const logger = createLogger("nyxidOrgsClient"); @@ -75,7 +76,7 @@ export class NyxidOrgsClient { async listUserOrgs(userAccessToken: string): Promise { const baseUrl = await this.resolveBaseUrl(); const url = `${baseUrl}/api/v1/orgs`; - const resp = await fetch(url, { + const resp = await safeFetch(url, { headers: { Authorization: `Bearer ${userAccessToken}` }, }); if (!resp.ok) { @@ -137,7 +138,7 @@ export class NyxidOrgsClient { const url = `${baseUrl}/api/v1/orgs/${encodeURIComponent(orgUserId)}/members`; let resp: Response; try { - resp = await fetch(url, { headers: { Authorization: `Bearer ${token}` } }); + resp = await safeFetch(url, { headers: { Authorization: `Bearer ${token}` } }); } catch (err) { logger.warn({ err, orgUserId }, "NyxID listOrgMembers fetch threw"); return []; diff --git a/ornn-api/src/clients/nyxid/service.ts b/ornn-api/src/clients/nyxid/service.ts index 2404fc2c..8fe0955f 100644 --- a/ornn-api/src/clients/nyxid/service.ts +++ b/ornn-api/src/clients/nyxid/service.ts @@ -14,6 +14,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; import type { NyxidConfigResolver } from "./base"; const logger = createLogger("nyxidServiceClient"); @@ -99,7 +100,7 @@ export class NyxidServiceClient { } try { const baseUrl = await this.resolveBaseUrl(); - const resp = await fetch(`${baseUrl}/api/v1/services`, { + const resp = await safeFetch(`${baseUrl}/api/v1/services`, { headers: { Authorization: `Bearer ${userAccessToken}` }, }); if (!resp.ok) { @@ -187,7 +188,7 @@ export class NyxidServiceClient { } try { const baseUrl = await this.resolveBaseUrl(); - const resp = await fetch(`${baseUrl}/api/v1/services`, { + const resp = await safeFetch(`${baseUrl}/api/v1/services`, { headers: { Authorization: `Bearer ${saToken}` }, }); if (!resp.ok) { diff --git a/ornn-api/src/clients/nyxid/ssrf.test.ts b/ornn-api/src/clients/nyxid/ssrf.test.ts new file mode 100644 index 00000000..dddde240 --- /dev/null +++ b/ornn-api/src/clients/nyxid/ssrf.test.ts @@ -0,0 +1,189 @@ +/** + * Tests for #811: the NyxID-family clients route every outbound request + * through `safeFetch`, closing the DNS-rebind gap for both the + * SA token-exchange path (`base.ts`) and the `baseApiUrl` service calls + * (`service.ts`). A NyxID host that rebinds to 169.254.169.254 at fetch + * time is refused before the client_secret / bearer token is sent. + * + * dns is stubbed before the client imports so the shared preflight + * (bound in `url.ts` at module load) sees the rebind. Asserting the + * `globalThis.fetch` spy recorded zero calls proves the swap is live for + * both the token path and a `baseApiUrl` sibling. + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; + +// `mock.module` is process-global and `url.ts` binds the dns namespace +// at first load, so the stub cannot be cleanly torn down per-file. We +// therefore make it host-aware: ONLY the rebind hostname resolves to +// the cloud-metadata address; every other host (including the public +// hostnames sibling tests like llm.test.ts / service.test.ts use) +// resolves to a benign public IP, so the leaked mock is harmless. +const REBIND_HOST = "rebind.test"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { NyxidSaTokenProvider } = await import("./base"); +const { NyxidServiceClient } = await import("./service"); +const { NyxidOrgsClient } = await import("./orgs"); +const { NyxidUserServicesClient } = await import("./userServices"); +const { NyxLlmCatalogClient } = await import("./llmCatalog"); +const { SsrfRefusalError } = await import("../../infra/url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +beforeEach(() => { + fetchCalls = []; + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push(url); + return new Response(JSON.stringify({ access_token: "tok", services: [] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +describe("NyxidSaTokenProvider SSRF preflight (#811)", () => { + function makeProvider() { + return new NyxidSaTokenProvider(async () => ({ + tokenUrl: "http://rebind.test/oauth/token", + clientId: "cid", + clientSecret: "secret", + })); + } + + it("getAccessToken() refuses a rebound token host before posting client_secret", async () => { + await expect(makeProvider().getAccessToken()).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("allowlisted token host passes the preflight and reaches fetch", async () => { + process.env[ALLOWLIST_ENV] = "rebind.test"; + const token = await makeProvider().getAccessToken(); + expect(token).toBe("tok"); + expect(fetchCalls).toEqual(["http://rebind.test/oauth/token"]); + }); +}); + +describe("NyxidServiceClient SSRF preflight (#811) — baseApiUrl sibling", () => { + function makeClient() { + return new NyxidServiceClient({ + resolver: async () => ({ baseApiUrl: "http://rebind.test" }), + }); + } + + it("listServicesForCaller() refuses a rebound baseApiUrl before sending the bearer token", async () => { + // listServicesForCaller fail-soft-catches errors and returns []; we + // assert via the fetch spy that the network call never fired and the + // refusal short-circuited the request. + const out = await makeClient().listServicesForCaller("user-token"); + expect(out).toEqual([]); + expect(fetchCalls).toHaveLength(0); + }); + + it("listActiveServiceIdsAsPlatform() refuses a rebound baseApiUrl (SA path)", async () => { + const ids = await makeClient().listActiveServiceIdsAsPlatform("sa-token"); + // Fail-soft returns null when the SSRF refusal is caught. + expect(ids).toBeNull(); + expect(fetchCalls).toHaveLength(0); + }); + + it("allowlisted baseApiUrl passes the preflight and reaches fetch", async () => { + process.env[ALLOWLIST_ENV] = "rebind.test"; + await makeClient().listServicesForCaller("user-token"); + expect(fetchCalls).toEqual(["http://rebind.test/api/v1/services"]); + }); +}); + +// --------------------------------------------------------------------------- +// #832 — per-call-site SSRF smoke tests. Each named outbound site must +// route through `safeFetch`, so a rebound resolver host is refused before +// the network call (and before any bearer/credential is sent). We assert +// the fetch spy recorded ZERO calls; the surfaced behaviour matches each +// client's documented contract (throw on fail-closed reads, [] on +// fail-soft reads). +// --------------------------------------------------------------------------- + +describe("NyxidOrgsClient SSRF preflight (#832) — orgs sites", () => { + // listUserOrgs is fail-closed (throws); listOrgMembers is fail-soft + // (returns [] and logs). A working SA token provider is injected so + // the listOrgMembers refusal lands at the /members fetch, not the SA + // token exchange. + const stubSa = { + getAccessToken: async () => "sa-token", + } as unknown as InstanceType; + + function makeClient() { + return new NyxidOrgsClient({ + resolver: async () => ({ baseApiUrl: "http://rebind.test" }), + saTokenProvider: stubSa, + }); + } + + it("listUserOrgs() refuses a rebound baseApiUrl before sending the user bearer", async () => { + await expect(makeClient().listUserOrgs("user-token")).rejects.toBeInstanceOf( + SsrfRefusalError, + ); + expect(fetchCalls).toHaveLength(0); + }); + + it("listOrgMembers() refuses a rebound baseApiUrl (fail-soft []) before sending the SA bearer", async () => { + const out = await makeClient().listOrgMembers("org-user-id"); + expect(out).toEqual([]); + expect(fetchCalls).toHaveLength(0); + }); +}); + +describe("NyxidUserServicesClient SSRF preflight (#832) — user-services site", () => { + function makeClient() { + return new NyxidUserServicesClient({ + resolver: async () => ({ baseApiUrl: "http://rebind.test" }), + }); + } + + it("listUserServices() refuses a rebound baseApiUrl before sending the user bearer", async () => { + await expect( + makeClient().listUserServices("user-token"), + ).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); +}); + +describe("NyxLlmCatalogClient SSRF preflight (#832) — catalog site", () => { + // A working SA token provider is injected so the refusal lands at the + // catalog `/models` fetch (the named site), not the token exchange. + const stubSa = { + getAccessToken: async () => "sa-token", + } as unknown as InstanceType; + + function makeClient() { + return new NyxLlmCatalogClient({ + resolver: async () => ({ baseApiUrl: "http://rebind.test" }), + saTokenProvider: stubSa, + }); + } + + it("listUpstreamModels() refuses a rebound catalog host before reading the model list", async () => { + await expect(makeClient().listUpstreamModels()).rejects.toBeInstanceOf( + SsrfRefusalError, + ); + expect(fetchCalls).toHaveLength(0); + }); +}); diff --git a/ornn-api/src/clients/nyxid/userServices.ts b/ornn-api/src/clients/nyxid/userServices.ts index 9f5f1078..07efc4ea 100644 --- a/ornn-api/src/clients/nyxid/userServices.ts +++ b/ornn-api/src/clients/nyxid/userServices.ts @@ -21,6 +21,7 @@ */ import { createLogger } from "../../shared/logger"; +import { safeFetch } from "../../infra/safeFetch"; import type { NyxidConfigResolver } from "./base"; const logger = createLogger("nyxidUserServicesClient"); @@ -76,7 +77,7 @@ export class NyxidUserServicesClient { async listUserServices(userAccessToken: string): Promise { const baseUrl = await this.resolveBaseUrl(); const url = `${baseUrl}/api/v1/user-services`; - const resp = await fetch(url, { + const resp = await safeFetch(url, { headers: { Authorization: `Bearer ${userAccessToken}` }, }); if (!resp.ok) { diff --git a/ornn-api/src/clients/sandboxClient.test.ts b/ornn-api/src/clients/sandboxClient.test.ts new file mode 100644 index 00000000..4193ccf8 --- /dev/null +++ b/ornn-api/src/clients/sandboxClient.test.ts @@ -0,0 +1,101 @@ +/** + * Tests for #811: SandboxClient outbound calls route through `safeFetch`. + * A chrono-sandbox host that rebinds to a private/metadata address at + * fetch time is refused before the SA bearer token leaves the process. + * + * dns is stubbed → 169.254.169.254 before the client import so the + * shared preflight (bound in `url.ts` at load) catches the rebind. The + * zero-fetch-call assertion proves the swap is live. + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; + +// `mock.module` is process-global and `url.ts` binds the dns namespace +// at first load, so the stub cannot be cleanly torn down per-file. We +// therefore make it host-aware: ONLY the rebind hostname resolves to +// the cloud-metadata address; every other host resolves to a benign +// public IP, so the leaked mock is harmless to sibling tests. +const REBIND_HOST = "rebind.test"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { SandboxClient } = await import("./sandboxClient"); +const { SsrfRefusalError } = await import("../infra/url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +beforeEach(() => { + fetchCalls = []; + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push(url); + return new Response(JSON.stringify({ success: true }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +function makeClient() { + return new SandboxClient({ + resolver: async () => ({ baseUrl: "http://rebind.test" }), + getAccessToken: async () => "sa-token-xyz", + }); +} + +/** Drain an async generator to force the underlying request to fire. */ +async function drain(gen: AsyncGenerator): Promise { + for await (const _ of gen) { + /* discard */ + } +} + +describe("SandboxClient SSRF preflight (#811)", () => { + it("execute() (POST path) refuses a rebound host before issuing the request", async () => { + await expect( + makeClient().execute({ script: "print(1)", language: "python" }), + ).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("deleteSession() refuses a rebound host before issuing the request", async () => { + await expect(makeClient().deleteSession("s1")).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("listSessions() refuses a rebound host before issuing the request", async () => { + await expect(makeClient().listSessions()).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("executeStream() (SSE path) refuses a rebound host before issuing the request", async () => { + await expect( + drain(makeClient().executeStream({ script: "print(1)", language: "python" })), + ).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("allowlisted host passes the preflight and reaches fetch", async () => { + process.env[ALLOWLIST_ENV] = "rebind.test"; + const res = await makeClient().execute({ script: "print(1)", language: "python" }); + expect(res.success).toBe(true); + expect(fetchCalls).toHaveLength(1); + expect(fetchCalls[0]).toBe("http://rebind.test/execute"); + }); +}); diff --git a/ornn-api/src/clients/sandboxClient.ts b/ornn-api/src/clients/sandboxClient.ts index f1161f96..eacc0d55 100644 --- a/ornn-api/src/clients/sandboxClient.ts +++ b/ornn-api/src/clients/sandboxClient.ts @@ -5,6 +5,7 @@ */ import { createLogger } from "../shared/logger"; +import { safeFetch } from "../infra/safeFetch"; const logger = createLogger("sandboxClient"); // ── Shared types ────────────────────────────────────────────────────── @@ -296,7 +297,7 @@ export class SandboxClient { const baseUrl = await this.resolveBaseUrl(); const auth = await this.authHeaders(); - const response = await fetch(`${baseUrl}/sessions/${sessionId}`, { + const response = await safeFetch(`${baseUrl}/sessions/${sessionId}`, { method: "DELETE", headers: auth, }); @@ -315,7 +316,7 @@ export class SandboxClient { const baseUrl = await this.resolveBaseUrl(); const auth = await this.authHeaders(); - const response = await fetch(`${baseUrl}/sessions`, { headers: auth }); + const response = await safeFetch(`${baseUrl}/sessions`, { headers: auth }); if (!response.ok) { const text = await response.text().catch(() => ""); throw new Error(`List sessions failed (${response.status}): ${text}`); @@ -329,7 +330,7 @@ export class SandboxClient { private async post(path: string, body: Record): Promise { const baseUrl = await this.resolveBaseUrl(); const auth = await this.authHeaders(); - const response = await fetch(`${baseUrl}${path}`, { + const response = await safeFetch(`${baseUrl}${path}`, { method: "POST", headers: { "Content-Type": "application/json", ...auth }, body: JSON.stringify(body), @@ -347,7 +348,7 @@ export class SandboxClient { private async *streamSSE(path: string, body: Record): AsyncGenerator { const baseUrl = await this.resolveBaseUrl(); const auth = await this.authHeaders(); - const response = await fetch(`${baseUrl}${path}`, { + const response = await safeFetch(`${baseUrl}${path}`, { method: "POST", headers: { "Content-Type": "application/json", ...auth }, body: JSON.stringify(body), diff --git a/ornn-api/src/clients/storageClient.test.ts b/ornn-api/src/clients/storageClient.test.ts new file mode 100644 index 00000000..3f5e97a2 --- /dev/null +++ b/ornn-api/src/clients/storageClient.test.ts @@ -0,0 +1,97 @@ +/** + * Tests for #811: StorageClient outbound calls route through `safeFetch`, + * so a chrono-storage host that resolves to a private/metadata address + * at fetch time is refused BEFORE the SA bearer token is sent. + * + * We stub `node:dns/promises` → 169.254.169.254 (cloud metadata) before + * importing the client so the shared `assertPublicResolvedAddress` + * (bound in `url.ts` at load) sees the rebind. The assertion that the + * `globalThis.fetch` spy recorded zero calls is what proves the swap is + * live — revert `fetch`→`safeFetch` and these tests fail. + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; + +// `mock.module` is process-global and `url.ts` binds the dns namespace +// at first load, so the stub cannot be cleanly torn down per-file. We +// therefore make it host-aware: ONLY the rebind hostname resolves to +// the cloud-metadata address; every other host (including the public +// hostnames sibling tests use) resolves to a benign public IP. That way +// the leaked mock is harmless to `llm.test.ts` / `service.test.ts`. +const REBIND_HOST = "rebind.test"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { StorageClient } = await import("./storageClient"); +const { SsrfRefusalError } = await import("../infra/url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +beforeEach(() => { + fetchCalls = []; + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + fetchCalls.push(url); + return new Response(JSON.stringify({ data: { url: "x" } }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +function makeClient() { + return new StorageClient({ + resolver: async () => ({ baseUrl: "http://rebind.test", bucket: "b" }), + getAccessToken: async () => "sa-token-xyz", + }); +} + +describe("StorageClient SSRF preflight (#811)", () => { + it("upload() refuses a rebound host before issuing the request", async () => { + await expect( + makeClient().upload("b", "k", new Uint8Array([1]), "text/plain"), + ).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("delete() refuses a rebound host before issuing the request", async () => { + await expect(makeClient().delete("b", "k")).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("getPresignedUrl() refuses a rebound host before issuing the request", async () => { + await expect(makeClient().getPresignedUrl("b", "k")).rejects.toBeInstanceOf( + SsrfRefusalError, + ); + expect(fetchCalls).toHaveLength(0); + }); + + it("copy() refuses a rebound host before issuing the request", async () => { + await expect(makeClient().copy("b", "s", "d")).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toHaveLength(0); + }); + + it("allowlisted host passes the preflight and reaches fetch", async () => { + process.env[ALLOWLIST_ENV] = "rebind.test"; + const out = await makeClient().upload("b", "k", new Uint8Array([1]), "text/plain"); + expect(out).toEqual({ url: "x" }); + expect(fetchCalls).toHaveLength(1); + expect(fetchCalls[0]).toContain("http://rebind.test/api/buckets/b/objects"); + }); +}); diff --git a/ornn-api/src/clients/storageClient.ts b/ornn-api/src/clients/storageClient.ts index 43a205b4..a2574f52 100644 --- a/ornn-api/src/clients/storageClient.ts +++ b/ornn-api/src/clients/storageClient.ts @@ -5,6 +5,7 @@ */ import { createLogger } from "../shared/logger"; +import { safeFetch } from "../infra/safeFetch"; const logger = createLogger("storageClient"); export interface IStorageClient { @@ -67,7 +68,7 @@ export class StorageClient implements IStorageClient { const url = `${baseUrl}/api/buckets/${bucket}/objects?${params.toString()}`; const auth = await this.authHeaders(); - const res = await fetch(url, { + const res = await safeFetch(url, { method: "POST", headers: { "Content-Type": contentType, ...auth }, body: data as unknown as BodyInit, @@ -90,7 +91,7 @@ export class StorageClient implements IStorageClient { const url = `${baseUrl}/api/buckets/${bucket}/objects?${params.toString()}`; const auth = await this.authHeaders(); - const res = await fetch(url, { method: "DELETE", headers: auth }); + const res = await safeFetch(url, { method: "DELETE", headers: auth }); if (!res.ok) { const text = await res.text().catch(() => ""); @@ -114,7 +115,7 @@ export class StorageClient implements IStorageClient { const url = `${baseUrl}/api/buckets/${bucket}/presigned-url?${params.toString()}`; const auth = await this.authHeaders(); - const res = await fetch(url, { method: "GET", headers: auth }); + const res = await safeFetch(url, { method: "GET", headers: auth }); if (!res.ok) { const text = await res.text().catch(() => ""); @@ -131,7 +132,7 @@ export class StorageClient implements IStorageClient { const url = `${baseUrl}/api/buckets/${bucket}/objects/copy`; const auth = await this.authHeaders(); - const res = await fetch(url, { + const res = await safeFetch(url, { method: "POST", headers: { "Content-Type": "application/json", ...auth }, body: JSON.stringify({ sourceKey, destKey }), diff --git a/ornn-api/src/domains/admin/quota/routes.ts b/ornn-api/src/domains/admin/quota/routes.ts index f823bc3c..3a4a39de 100644 --- a/ornn-api/src/domains/admin/quota/routes.ts +++ b/ornn-api/src/domains/admin/quota/routes.ts @@ -24,6 +24,7 @@ import { } from "../../../middleware/nyxidAuth"; import { validateBody, getValidatedBody } from "../../../middleware/validate"; import { AppError } from "../../../shared/types/index"; +import { MAX_PAGE } from "../../../shared/cursor"; import type { UserDirectoryRepository } from "../../users/repository"; import type { QuotaService } from "../../quota/service"; import { @@ -239,7 +240,9 @@ export function createAdminQuotaRoutes( auth, requirePermission(QUOTA_ADMIN_PERMISSION), async (c) => { - const page = Math.max(1, Number(c.req.query("page")) || 1); + // Clamp to MAX_PAGE so a huge ?page= can't drive an unbounded + // `.skip()` scan in the grant-audit query (CWE-770, #810). + const page = Math.min(MAX_PAGE, Math.max(1, Number(c.req.query("page")) || 1)); const pageSize = Math.min(200, Math.max(1, Number(c.req.query("pageSize")) || 50)); const targetUserId = c.req.query("userId") || undefined; const adminUserId = c.req.query("adminUserId") || undefined; diff --git a/ornn-api/src/domains/analytics/routes.ts b/ornn-api/src/domains/analytics/routes.ts index 8a5eaa43..13abf3d1 100644 --- a/ornn-api/src/domains/analytics/routes.ts +++ b/ornn-api/src/domains/analytics/routes.ts @@ -21,10 +21,9 @@ import { Hono, type Context } from "hono"; import { type AuthVariables, optionalAuthMiddleware, - readUserOrgMemberships, } from "../../middleware/nyxidAuth"; import { AppError } from "../../shared/types/index"; -import { canReadSkill } from "../skills/crud/authorize"; +import { canReadSkill, buildActorContext } from "../skills/crud/authorize"; import type { SkillService } from "../skills/crud/service"; import type { AnalyticsService } from "./service"; import type { PullBucket } from "./types"; @@ -52,12 +51,7 @@ export function createAnalyticsRoutes( throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } diff --git a/ornn-api/src/domains/playground/chatService.test.ts b/ornn-api/src/domains/playground/chatService.test.ts index 9debb17f..de8e027e 100644 --- a/ornn-api/src/domains/playground/chatService.test.ts +++ b/ornn-api/src/domains/playground/chatService.test.ts @@ -33,6 +33,8 @@ import type { } from "../../clients/nyxid/llm"; import type { SkillService } from "../skills/crud/service"; import type { PlaygroundChatEvent } from "../../shared/types/index"; +import { AppError } from "../../shared/types/index"; +import { canReadSkill, type ActorContext, type SkillOwnership } from "../skills/crud/authorize"; // --------------------------------------------------------------------------- // Stubs @@ -75,8 +77,37 @@ function makeLlmClient(rounds: ResponsesApiStreamEvent[][]): NyxLlmClient { } as unknown as NyxLlmClient; } +// #806 — a private skill fixture used to exercise object-level +// authorization on both skill-load paths. The marker string below must +// appear in the package contents and ONLY in the package contents, so a +// "contents leaked?" assertion can grep the transcript for it. +const SECRET_MARKER = "SUPER_SECRET_SKILL_BODY_4f2a"; +const PRIVATE_SKILL_FIXTURE: SkillOwnership = { + createdBy: "owner-1", + isPrivate: true, + sharedWithUsers: ["shared-1"], + sharedWithOrgs: ["org-9"], +}; + +/** + * Authz-aware stub: runs the REAL `canReadSkill` against the private + * fixture and the actor it was handed. Denied → throws the same + * `skill_not_found` AppError the production service throws; allowed → + * returns a package whose files embed `SECRET_MARKER`. + */ const SKILL_SERVICE_STUB: SkillService = { - getSkillJson: async () => ({ name: "stub", description: "", metadata: {}, files: {} }), + getSkillJson: async (_idOrName: string, actor: ActorContext) => { + if (!canReadSkill(PRIVATE_SKILL_FIXTURE, actor)) { + throw AppError.notFound("skill_not_found", `Skill '${_idOrName}' not found`); + } + return { + name: "demo-skill", + description: "", + version: "1.0", + metadata: {}, + files: { "SKILL.md": `# demo\n${SECRET_MARKER}` }, + }; + }, } as unknown as SkillService; const DEFAULTS_RESOLVER = async () => ({ @@ -85,6 +116,12 @@ const DEFAULTS_RESOLVER = async () => ({ temperature: 0.5, }); +// #826 — `chat()` now requires an explicit actor (the SYSTEM_ACTOR +// fallback was dropped). `isPlatformAdmin: true` preserves the old +// SYSTEM_ACTOR-bypass intent for the session/env/timeout tests that +// don't exercise object-level authorization. +const TEST_ACTOR: ActorContext = { userId: "u1", memberships: [], isPlatformAdmin: true, membershipsResolved: true }; + interface SandboxCalls { createSession: Array<{ params: CreateSessionParams }>; sessionExecute: Array<{ sessionId: string; params: SessionExecuteParams }>; @@ -174,7 +211,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(1); expect(sandbox.calls.createSession[0]!.params).toMatchObject({ @@ -197,7 +234,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(1); expect(sandbox.calls.sessionExecute).toHaveLength(2); @@ -217,7 +254,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(2); expect(sandbox.calls.createSession.map((c) => c.params.language)).toEqual(["javascript", "python"]); @@ -235,7 +272,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.deleteSession).toEqual(["sess-1"]); }); @@ -253,7 +290,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - const events = await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + const events = await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(1); expect(sandbox.calls.sessionExecute).toHaveLength(0); @@ -277,7 +314,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(1); expect(sandbox.calls.sessionExecute).toHaveLength(1); @@ -288,7 +325,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { const sandbox = makeSandboxClient({}); const service = makeService([STOP_EVENTS], sandbox); - await drain(service.chat("u1", { messages: [{ role: "user", content: "hi" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "hi" }] }, undefined, { actor: TEST_ACTOR })); expect(sandbox.calls.createSession).toHaveLength(0); expect(sandbox.calls.sessionExecute).toHaveLength(0); @@ -314,7 +351,7 @@ describe("PlaygroundChatService — sandbox session persistence (#531)", () => { ); // Should complete without throwing despite the delete failure. - const events = await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + const events = await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); expect(events.some((e) => e.type === "finish")).toBe(true); expect(sandbox.calls.deleteSession).toEqual(["sess-1"]); // Restore in case the test runner shares state. @@ -352,6 +389,8 @@ describe("PlaygroundChatService — env value isolation (#721)", () => { messages: [{ role: "user", content: "go" }], envVars: { SECRET_TOKEN: "REAL_SECRET_FROM_UI" }, }, + undefined, + { actor: TEST_ACTOR }, ), ); @@ -376,9 +415,158 @@ describe("PlaygroundChatService — env value isolation (#721)", () => { sandbox, ); - await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] })); + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); const envSeen = sandbox.calls.sessionExecute[0]!.params.env as Record; expect(envSeen).toEqual({ MARKER: "test123" }); }); }); + +// --------------------------------------------------------------------------- +// #806 — object-level authorization (BOLA / OWASP API1) on skill loads. +// +// Two bypass paths must both be gated by the caller's actor: +// (a) the `request.skillId` developer-message injection +// (b) the `load_skill` tool call +// +// The stub runs the REAL `canReadSkill` against PRIVATE_SKILL_FIXTURE, so +// these tests exercise the production policy end-to-end through chat(). +// --------------------------------------------------------------------------- + +const STRANGER: ActorContext = { userId: "stranger", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; +const OWNER: ActorContext = { userId: "owner-1", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; +const SHARED_USER: ActorContext = { userId: "shared-1", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; +const ORG_MEMBER: ActorContext = { + userId: "someone-else", + memberships: [{ userId: "org-9", role: "member", displayName: "Org Nine" }], + isPlatformAdmin: false, + membershipsResolved: true, +}; +const ADMIN: ActorContext = { userId: "admin-1", memberships: [], isPlatformAdmin: true, membershipsResolved: true }; + +/** True if any emitted event's serialized form contains the secret marker. */ +function leaksMarker(events: PlaygroundChatEvent[]): boolean { + return events.some((e) => JSON.stringify(e).includes(SECRET_MARKER)); +} + +/** Drive a chat over the `request.skillId` injection path for `actor`. */ +async function runSkillIdPath(actor: ActorContext): Promise { + const sandbox = makeSandboxClient({}); + const service = makeService([STOP_EVENTS], sandbox); + return drain( + service.chat( + "u1", + { messages: [{ role: "user", content: "go" }], skillId: "demo-skill" }, + undefined, + { actor }, + ), + ); +} + +/** Drive a chat that issues a single `load_skill` tool call for `actor`. */ +async function runLoadSkillPath(actor: ActorContext): Promise { + const sandbox = makeSandboxClient({}); + const service = makeService( + [makeToolCallEvents({ name: "load_skill", args: { skill_id: "demo-skill" } }), STOP_EVENTS], + sandbox, + ); + return drain( + service.chat("u1", { messages: [{ role: "user", content: "load it" }] }, undefined, { actor }), + ); +} + +describe("PlaygroundChatService — skill-load authorization (#806)", () => { + it("(1) stranger via skillId path → error event, NO skill contents injected", async () => { + const events = await runSkillIdPath(STRANGER); + const err = events.find((e) => e.type === "error"); + expect(err).toBeDefined(); + expect((err as { message: string }).message).toContain("Failed to load skill"); + expect(events.some((e) => e.type === "finish" && (e as { finishReason?: string }).finishReason === "error")).toBe(true); + expect(leaksMarker(events)).toBe(false); + }); + + it("(2) stranger via load_skill tool → access-denied tool result, NO contents", async () => { + const events = await runLoadSkillPath(STRANGER); + const toolResult = events.find((e) => e.type === "tool-result") as { result: string } | undefined; + expect(toolResult).toBeDefined(); + expect(toolResult!.result).toContain("Failed to load skill"); + expect(toolResult!.result).not.toContain(SECRET_MARKER); + expect(leaksMarker(events)).toBe(false); + }); + + it("(3) owner-1 succeeds on both paths", async () => { + // skillId path: no error event, finishes normally. + const skillIdEvents = await runSkillIdPath(OWNER); + expect(skillIdEvents.some((e) => e.type === "error")).toBe(false); + expect(skillIdEvents.some((e) => e.type === "finish")).toBe(true); + // load_skill path: tool result carries the package contents. + const loadEvents = await runLoadSkillPath(OWNER); + const toolResult = loadEvents.find((e) => e.type === "tool-result") as { result: string } | undefined; + expect(toolResult!.result).toContain(SECRET_MARKER); + }); + + it("(4) shared-1 (per-user grant) succeeds on both paths", async () => { + const skillIdEvents = await runSkillIdPath(SHARED_USER); + expect(skillIdEvents.some((e) => e.type === "error")).toBe(false); + const loadEvents = await runLoadSkillPath(SHARED_USER); + const toolResult = loadEvents.find((e) => e.type === "tool-result") as { result: string } | undefined; + expect(toolResult!.result).toContain(SECRET_MARKER); + }); + + it("(5) org-9 member (per-org grant) succeeds on both paths", async () => { + const skillIdEvents = await runSkillIdPath(ORG_MEMBER); + expect(skillIdEvents.some((e) => e.type === "error")).toBe(false); + const loadEvents = await runLoadSkillPath(ORG_MEMBER); + const toolResult = loadEvents.find((e) => e.type === "tool-result") as { result: string } | undefined; + expect(toolResult!.result).toContain(SECRET_MARKER); + }); + + it("(6) platform admin succeeds on both paths", async () => { + const skillIdEvents = await runSkillIdPath(ADMIN); + expect(skillIdEvents.some((e) => e.type === "error")).toBe(false); + const loadEvents = await runLoadSkillPath(ADMIN); + const toolResult = loadEvents.find((e) => e.type === "tool-result") as { result: string } | undefined; + expect(toolResult!.result).toContain(SECRET_MARKER); + }); +}); + +// --------------------------------------------------------------------------- +// #819 — model-supplied timeout_secs is coerced + clamped to 1-600 before it +// reaches the sandbox client. The clamp lives at the single chokepoint in +// runSandboxToolCall, so asserting on the session-execute path is sufficient. +// --------------------------------------------------------------------------- + +describe("PlaygroundChatService — sandbox timeout clamp (#819)", () => { + const cases: Array<{ label: string; input: unknown; expected: number }> = [ + { label: "0 clamps up to the 1s floor", input: 0, expected: 1 }, + { label: "-5 clamps up to the 1s floor", input: -5, expected: 1 }, + // The tool-call args are JSON-serialized by makeToolCallEvents exactly as + // the real LLM stream emits them, and JSON.stringify coerces a literal NaN + // to null (-> Number(null) === 0, not NaN). A non-numeric *string* is the + // value that actually survives that boundary and reaches the service as a + // NaN under Number(...), so it is the faithful trigger for the 60s fallback. + { label: "non-numeric input falls back to the 60s default", input: "not-a-number", expected: 60 }, + { label: "10_000 clamps down to the 600s ceiling", input: 10_000, expected: 600 }, + ]; + + for (const { label, input, expected } of cases) { + it(label, async () => { + const sandbox = makeSandboxClient({}); + const service = makeService( + [ + makeToolCallEvents({ + name: "execute_in_sandbox", + args: { script: "x", language: "python", timeout_secs: input as never }, + }), + STOP_EVENTS, + ], + sandbox, + ); + + await drain(service.chat("u1", { messages: [{ role: "user", content: "go" }] }, undefined, { actor: TEST_ACTOR })); + + expect(sandbox.calls.sessionExecute).toHaveLength(1); + expect(sandbox.calls.sessionExecute[0]!.params.timeoutSecs).toBe(expected); + }); + } +}); diff --git a/ornn-api/src/domains/playground/chatService.ts b/ornn-api/src/domains/playground/chatService.ts index d0e22582..dacce498 100644 --- a/ornn-api/src/domains/playground/chatService.ts +++ b/ornn-api/src/domains/playground/chatService.ts @@ -17,6 +17,7 @@ import type { } from "../../clients/sandboxClient"; import type { SkillService } from "../skills/crud/service"; import type { PlaygroundChatEvent } from "../../shared/types/index"; +import type { ActorContext } from "../skills/crud/authorize"; import { createLogger } from "../../shared/logger"; import { z } from "zod"; @@ -173,7 +174,7 @@ When calling execute_in_sandbox: - env: the user-provided environment variables (already in context) - output_type: from metadata (text or file) - retrieve_files: glob patterns for output files (e.g. ["*.png", "*.jpg"]) -- timeout_secs: 120 (default) +- timeout_secs: 60 (default, clamped to 1-600) Be concise. Act, don't explain.`; @@ -309,9 +310,14 @@ export class PlaygroundChatService { async *chat( userId: string, request: PlaygroundChatRequest, - abortSignal?: AbortSignal, - options?: { modelId?: string }, + abortSignal: AbortSignal | undefined, + options: { modelId?: string; actor: ActorContext }, ): AsyncGenerator { + // #806/#826 — object-level authorization for skill loads. The actor + // is REQUIRED: every caller (routes + tests) must supply the real + // caller context. This single actor gates BOTH skill bypass paths + // below (skillId injection + the load_skill tool). + const actor = options.actor; // Resolve LLM defaults from admin settings on every call so // operator updates land without a pod restart. let defaults: PlaygroundLlmDefaults; @@ -335,7 +341,7 @@ export class PlaygroundChatService { // Resolved model id (already validated by the route). Falls back // to settings default for tests / internal callers that don't // go through the model picker. - const model = options?.modelId ?? defaults.model; + const model = options.modelId ?? defaults.model; const input = this.buildInput(request); // Inject system prompt as developer message (instructions field is ignored by upstream LLM) @@ -347,7 +353,7 @@ export class PlaygroundChatService { // Auto-inject skill context if skillId is provided if (request.skillId) { try { - const skillJson = await this.skillService.getSkillJson(request.skillId); + const skillJson = await this.skillService.getSkillJson(request.skillId, actor); const skillContext = this.buildSkillContext(skillJson, request.envVars); input.unshift({ role: "developer" as const, @@ -456,6 +462,7 @@ export class PlaygroundChatService { sandboxSessions, createdSessionIds, request.envVars, + actor, ); yield { type: "tool-result", toolCallId: pendingToolCall.id, result: toolResult.text }; @@ -517,6 +524,7 @@ export class PlaygroundChatService { sandboxSessions: Map, createdSessionIds: string[], userEnvVars: Record | undefined, + actor: ActorContext, ): Promise { const { name, args } = toolCall; @@ -531,7 +539,10 @@ export class PlaygroundChatService { if (name === "load_skill") { try { - const skillJson = await this.skillService.getSkillJson((args.skill_id as string) ?? ""); + // #806 — gate the load through the caller's actor. A skill the + // caller can't read throws skill_not_found here, so the tool + // result below carries the denial string, never the contents. + const skillJson = await this.skillService.getSkillJson((args.skill_id as string) ?? "", actor); return { text: JSON.stringify(skillJson, null, 2), files: [] }; } catch (err) { return { text: `Failed to load skill: ${err instanceof Error ? err.message : String(err)}`, files: [] }; @@ -583,7 +594,14 @@ export class PlaygroundChatService { const dependencies = (args.dependencies as string[]) ?? []; const retrieveFiles = (args.retrieve_files as string[]) ?? []; const inputFiles = (args.input_files as Array<{ path: string; content: string }>) ?? []; - const timeoutSecs = (args.timeout_secs as number) ?? 120; + // #819 — coerce + clamp the model-supplied timeout before it reaches the + // sandbox client. Computed once here; flows into the session path and both + // one-shot fallbacks, so this is the single chokepoint for this value. + // Non-numeric / NaN falls back to the documented 60s default. + const rawTimeout = Number(args.timeout_secs); + const timeoutSecs = Number.isFinite(rawTimeout) + ? Math.min(600, Math.max(1, Math.trunc(rawTimeout))) + : 60; let sessionId = sandboxSessions.get(language); diff --git a/ornn-api/src/domains/playground/routes.test.ts b/ornn-api/src/domains/playground/routes.test.ts new file mode 100644 index 00000000..45af781a --- /dev/null +++ b/ornn-api/src/domains/playground/routes.test.ts @@ -0,0 +1,85 @@ +/** + * Route-level wiring test for the playground chat rate limit (#809). + * + * Pins that `POST /playground/chat` carries the per-user 20/min limiter + * (same cap + class as `/skills/generate`). The limiter's own behaviour + * (RFC 9239 headers, per-user keying, window reset) is owned by + * `middleware/rateLimit.test.ts` — this test only asserts the limiter is + * mounted on this route, ahead of `validateBody`, so the 21st request + * 429s before Zod and before any LLM cost. + * + * @module domains/playground/routes.test + */ + +import { beforeEach, describe, expect, test } from "bun:test"; +import { Hono } from "hono"; +import { AppError, buildProblemJsonBody } from "../../shared/types/index"; +import { __resetRateLimitForTests } from "../../middleware/rateLimit"; +import { createPlaygroundRoutes, type PlaygroundRoutesConfig } from "./routes"; + +function makeApp() { + const app = new Hono(); + // Upstream auth stub. `nyxidAuthMiddleware` + `requirePermission` only + // READ `c.get("auth")`; the real middleware never overwrites it, so this + // stub survives into the route chain and satisfies both. + app.use("*", async (c, next) => { + c.set( + "auth" as never, + { userId: "u1", permissions: ["ornn:playground:use"] } as never, + ); + await next(); + }); + + // Mount the real route. Services are never reached: the 429 fires inside + // rateLimit, ahead of validateBody, so the empty config is never touched. + app.route("/", createPlaygroundRoutes({} as unknown as PlaygroundRoutesConfig)); + + // Translate AppError → problem+json like the global handler does. + 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("POST /playground/chat rate limit (#809)", () => { + beforeEach(() => __resetRateLimitForTests()); + + test("429s the 21st per-user request before validateBody", async () => { + const app = makeApp(); + const req = () => + app.request("/playground/chat", { + method: "POST", + headers: { "Content-Type": "application/json" }, + // Invalid body — but rateLimit trips before Zod, so the cap is + // reached on raw request count regardless of body shape. + body: "{}", + }); + + // Exhaust the 20/min cap. + for (let i = 0; i < 20; i++) { + await req(); + } + + // 21st request is denied by the limiter. + const denied = await req(); + expect(denied.status).toBe(429); + expect(denied.headers.get("RateLimit-Limit")).toBe("20"); + expect(denied.headers.get("RateLimit-Remaining")).toBe("0"); + expect(denied.headers.get("Retry-After")).not.toBeNull(); + const body = (await denied.json()) as { code: string }; + expect(body.code).toBe("rate_limited"); + }); +}); diff --git a/ornn-api/src/domains/playground/routes.ts b/ornn-api/src/domains/playground/routes.ts index c11dfa7a..ab5f16c0 100644 --- a/ornn-api/src/domains/playground/routes.ts +++ b/ornn-api/src/domains/playground/routes.ts @@ -20,7 +20,9 @@ import { requirePermission, getAuth, } from "../../middleware/nyxidAuth"; +import { buildActorContext } from "../skills/crud/authorize"; import { validateBody, getValidatedBody } from "../../middleware/validate"; +import { rateLimit } from "../../middleware/rateLimit"; import { createLogger } from "../../shared/logger"; const logger = createLogger("playgroundRoutes"); @@ -97,6 +99,11 @@ export function createPlaygroundRoutes(config: PlaygroundRoutesConfig): Hono<{ V app.post( "/playground/chat", requirePermission("ornn:playground:use"), + // Rate limit (#809): per-user 20/min, same cap as skills-generate + // (generation/routes.ts) — playground chat runs an LLM call per request, + // same cost class. Mounted before validateBody so a flood of malformed + // bodies still 429s before Zod (and before any LLM cost). + rateLimit({ windowMs: 60_000, max: 20, label: "playground-chat" }), validateBody(chatRequestSchema, "VALIDATION_ERROR"), async (c) => { const authCtx = getAuth(c); @@ -104,18 +111,13 @@ export function createPlaygroundRoutes(config: PlaygroundRoutesConfig): Hono<{ V logger.info({ userId: authCtx.userId, messageCount: parsed.messages.length }, "Chat request"); - // Quota check — rejects with 429 BEFORE any LLM cost is incurred. - // Admins bypass via permission inside the service. - const decision = await quotaService.checkAllowed({ - userId: authCtx.userId, - permissions: authCtx.permissions, - surface: "playground", - }); - if (!decision.allowed) throwQuotaError(decision); - // Resolve the model — explicit `modelId` (validated against the // surface's enabled list) or the admin-set default. 503 when no - // models are enabled for the playground surface. + // models are enabled for the playground surface. This (and the + // org-membership read below) run BEFORE the quota reserve so a + // model/auth failure can't strand a reserved slot (#808) — both + // are pure reads that touch no LLM, so "429 before LLM cost" still + // holds. const resolution = await llmProvidersService.resolveModel({ surface: "playground", // exactOptionalPropertyTypes (#657) @@ -124,6 +126,36 @@ export function createPlaygroundRoutes(config: PlaygroundRoutesConfig): Hono<{ V if (resolution.kind !== "ok") throwModelResolutionError(resolution); const resolvedModelId = resolution.modelId; + // #806 — build the caller's object-level authorization actor and + // thread it into the chat service. Depends on nyxidOrgLookupMiddleware + // being mounted in bootstrap.ts ahead of the playground routes so the + // helper can resolve the caller's org memberships. The chat service + // uses this single actor to gate BOTH skill bypass paths (the + // `skillId` injection and the `load_skill` tool). + const actor = await buildActorContext(c); + + // Quota reserve — atomically claims a slot under the cap guard and + // rejects with 429 BEFORE any LLM cost is incurred (#808). Runs + // last among the pre-stream awaits: from here to the producer's + // `finally` (which always calls chargeOnCompletion) nothing can + // throw, so a reserved slot is always reconciled (committed on + // success, released on system_error/abort). Admins bypass inside + // the service and take no reservation. + // Capture the reservation instant so the eventual charge lands in + // the SAME calendar-month bucket the slot was reserved against + // (#827). Without this, a run that starts at 23:59:59 on the last + // day of a month and finishes after the UTC rollover would commit + // its per-model tally to the next month's bucket while `used` was + // bumped in the previous one — a benign but confusing straddle. + const reservedAt = new Date(); + const decision = await quotaService.checkAllowed({ + userId: authCtx.userId, + permissions: authCtx.permissions, + surface: "playground", + now: reservedAt, + }); + if (!decision.allowed) throwQuotaError(decision); + // Record a `playground` pull if the chat is bound to a skill. The // chat service loads the skill internally; we duplicate the lookup // here so analytics doesn't change the chat-service contract. Cost @@ -227,6 +259,7 @@ export function createPlaygroundRoutes(config: PlaygroundRoutesConfig): Hono<{ V try { for await (const event of chatService.chat(authCtx.userId, chatRequest, signal, { modelId: resolvedModelId, + actor, })) { await writeEvent(event); if (event.type === "tool-result") outcome = "skill_error"; @@ -250,6 +283,10 @@ export function createPlaygroundRoutes(config: PlaygroundRoutesConfig): Hono<{ V surface: "playground", outcome, modelId: resolvedModelId, + // Reconcile against the reserved month bucket (#827), not + // wall-clock-now, so a month-boundary straddle commits/ + // releases the slot it actually reserved. + now: reservedAt, }) .catch((err) => { logger.warn( diff --git a/ornn-api/src/domains/quota/repository.test.ts b/ornn-api/src/domains/quota/repository.test.ts index 26ba2cab..cca36df5 100644 --- a/ornn-api/src/domains/quota/repository.test.ts +++ b/ornn-api/src/domains/quota/repository.test.ts @@ -1,8 +1,9 @@ /** - * QuotaRepository UT-QUOTAREPO-001..007 against an in-memory Mongo. + * QuotaRepository UT-QUOTAREPO-001..010 against an in-memory Mongo. * - * Exercises the persistence layer (atomic $inc, upsert idempotence, - * chronological lifetime, append-only audit, indexes). + * Exercises the persistence layer: the atomic cap-guarded reserve + * (#808 TOCTOU fix), per-model commit, slot release, upsert idempotence, + * chronological lifetime, append-only audit, indexes. * * @module domains/quota/repository.test */ @@ -11,7 +12,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:tes import { MongoMemoryServer } from "mongodb-memory-server"; import { MongoClient, type Db } from "mongodb"; import { QuotaRepository } from "./repository"; -import { bucketId } from "./types"; +import { bucketId, type QuotaBucketDoc } from "./types"; let mongo: MongoMemoryServer; let client: MongoClient; @@ -37,127 +38,205 @@ beforeEach(async () => { await db.collection("quota_grants_audit").deleteMany({}); }); -describe("UT-QUOTAREPO-001 incrementUsed creates new bucket", () => { - test("upsert creates doc with monthStart/monthEnd, defaults, used=1", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); - const b = await repo.incrementUsed({ +const NOW = new Date(Date.UTC(2026, 4, 15)); +const MARKER = "2026-05"; + +/** Seed a bucket directly so a test can start from a precise counter. */ +async function seedBucket(overrides: Partial): Promise { + const _id = bucketId("u1", "playground", MARKER); + await db.collection("quota_buckets").insertOne({ + _id, + userId: "u1", + surface: "playground", + monthMarker: MARKER, + monthStart: new Date(Date.UTC(2026, 4, 1)), + monthEnd: new Date(Date.UTC(2026, 5, 1)), + defaultAllotment: 100, + adminGrant: 0, + used: 0, + usedByModel: {}, + createdAt: NOW, + updatedAt: NOW, + ...overrides, + }); +} + +describe("UT-QUOTAREPO-001 reserveSlot creates new bucket on first touch", () => { + test("first reserve inserts doc with monthStart/monthEnd, defaults, used=1", async () => { + const ok = await repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "gpt-4o", - defaultAllotment: 100, - now, + effectiveDefault: 100, + now: NOW, + }); + expect(ok).toBe(true); + const b = await repo.findBucket("u1", "playground", MARKER); + expect(b?.userId).toBe("u1"); + expect(b?.surface).toBe("playground"); + expect(b?.monthMarker).toBe(MARKER); + expect(b?.monthStart.toISOString()).toBe("2026-05-01T00:00:00.000Z"); + expect(b?.monthEnd.toISOString()).toBe("2026-06-01T00:00:00.000Z"); + expect(b?.defaultAllotment).toBe(100); + expect(b?.adminGrant).toBe(0); + expect(b?.used).toBe(1); + expect(b?.usedByModel).toEqual({}); + }); + + test("effectiveDefault < 1 denies and creates nothing", async () => { + const ok = await repo.reserveSlot({ + userId: "u1", + surface: "playground", + effectiveDefault: 0, + now: NOW, }); - expect(b.userId).toBe("u1"); - expect(b.surface).toBe("playground"); - expect(b.monthMarker).toBe("2026-05"); - expect(b.monthStart.toISOString()).toBe("2026-05-01T00:00:00.000Z"); - expect(b.monthEnd.toISOString()).toBe("2026-06-01T00:00:00.000Z"); - expect(b.defaultAllotment).toBe(100); - expect(b.adminGrant).toBe(0); - expect(b.used).toBe(1); - expect(b.usedByModel["gpt-4o"]).toBe(1); + expect(ok).toBe(false); + expect(await repo.findBucket("u1", "playground", MARKER)).toBeNull(); }); }); -describe("UT-QUOTAREPO-002 upsert is idempotent on _id", () => { - test("two increments share the same _id; second hit increments used", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); - const id = bucketId("u1", "playground", "2026-05"); - await repo.incrementUsed({ +describe("UT-QUOTAREPO-002 reserveSlot honours the cap on an existing bucket", () => { + test("reserve allowed below cap, denied at cap; used never exceeds cap", async () => { + // cap = effectiveDefault(100) + adminGrant(0) = 100. + await seedBucket({ used: 99 }); + const first = await repo.reserveSlot({ + userId: "u1", + surface: "playground", + effectiveDefault: 100, + now: NOW, + }); + expect(first).toBe(true); // used 99 -> 100 + const second = await repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "m", - defaultAllotment: 100, - now, + effectiveDefault: 100, + now: NOW, }); - await repo.incrementUsed({ + expect(second).toBe(false); // 100 == cap, denied + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(100); + }); + + test("adminGrant lifts the cap", async () => { + await seedBucket({ used: 100, adminGrant: 5 }); + const ok = await repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "m", - defaultAllotment: 100, - now, + effectiveDefault: 100, + now: NOW, }); - const docs = await db - .collection("quota_buckets") - .find({ userId: "u1", surface: "playground" }) - .toArray(); - expect(docs.length).toBe(1); - expect(docs[0]!._id as unknown as string).toBe(id); - expect((docs[0]! as unknown as { used: number }).used).toBe(2); + expect(ok).toBe(true); // cap now 105, used 100 -> 101 + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(101); }); }); -describe("UT-QUOTAREPO-003 incrementUsed atomic under concurrency", () => { - test("50 parallel increments → used==50", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); - await Promise.all( - Array.from({ length: 50 }, () => - repo.incrementUsed({ +describe("UT-QUOTAREPO-003 reserveSlot is atomic under concurrency (TOCTOU)", () => { + test("K=20 parallel reserves at used=cap-1 → exactly 1 wins, used==cap", async () => { + const cap = 100; + await seedBucket({ used: cap - 1, defaultAllotment: cap }); + + const results = await Promise.all( + Array.from({ length: 20 }, () => + repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "gpt-4o", - defaultAllotment: 1000, - now, + effectiveDefault: cap, + now: NOW, }), ), ); - const b = await repo.findBucket("u1", "playground", "2026-05"); - expect(b?.used).toBe(50); - expect(b?.usedByModel["gpt-4o"]).toBe(50); + + const granted = results.filter((r) => r === true).length; + expect(granted).toBe(1); + expect(results.filter((r) => r === false).length).toBe(19); + const b = await repo.findBucket("u1", "playground", MARKER); + expect(b?.used).toBe(cap); // never cap+1 + }); + + test("K=20 parallel reserves on first touch (no bucket) → 1 creates, rest race the cap", async () => { + // effectiveDefault 1 means exactly one slot total. The insert race + // (E11000) must collapse to a single winner. + const results = await Promise.all( + Array.from({ length: 20 }, () => + repo.reserveSlot({ + userId: "u1", + surface: "playground", + effectiveDefault: 1, + now: NOW, + }), + ), + ); + expect(results.filter((r) => r === true).length).toBe(1); + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(1); }); }); -describe("UT-QUOTAREPO-004 incrementUsed updates used + usedByModel together", () => { - test("single op writes both fields atomically", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); - await repo.incrementUsed({ +describe("UT-QUOTAREPO-004 releaseSlot refunds a reservation", () => { + test("reserve then release returns used to baseline", async () => { + await seedBucket({ used: 50 }); + await repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "gpt-4o", - defaultAllotment: 100, - now, + effectiveDefault: 100, + now: NOW, }); - await repo.incrementUsed({ + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(51); + await repo.releaseSlot({ userId: "u1", surface: "playground", now: NOW }); + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(50); + }); + + test("releaseSlot at used=0 is a no-op (floored, never negative)", async () => { + await seedBucket({ used: 0 }); + await repo.releaseSlot({ userId: "u1", surface: "playground", now: NOW }); + expect((await repo.findBucket("u1", "playground", MARKER))?.used).toBe(0); + }); + + test("releaseSlot on a missing bucket is a no-op", async () => { + await repo.releaseSlot({ userId: "ghost", surface: "playground", now: NOW }); + expect(await repo.findBucket("ghost", "playground", MARKER)).toBeNull(); + }); +}); + +describe("UT-QUOTAREPO-005 commitModel records usedByModel without touching used", () => { + test("commit increments usedByModel.; used unchanged", async () => { + await seedBucket({ used: 1 }); + await repo.commitModel({ userId: "u1", surface: "playground", - modelId: "claude-sonnet", - defaultAllotment: 100, - now, + modelId: "gpt-4o", + now: NOW, }); - const b = await repo.findBucket("u1", "playground", "2026-05"); + const b = await repo.findBucket("u1", "playground", MARKER); + expect(b?.used).toBe(1); // reserve already bumped this — commit must not + expect(b?.usedByModel["gpt-4o"]).toBe(1); + }); + + test("two commits with different models tally separately", async () => { + await seedBucket({ used: 2 }); + await repo.commitModel({ userId: "u1", surface: "playground", modelId: "gpt-4o", now: NOW }); + await repo.commitModel({ userId: "u1", surface: "playground", modelId: "claude-sonnet", now: NOW }); + const b = await repo.findBucket("u1", "playground", MARKER); expect(b?.used).toBe(2); expect(b?.usedByModel["gpt-4o"]).toBe(1); expect(b?.usedByModel["claude-sonnet"]).toBe(1); }); + + test("null modelId routes to __unknown__ sentinel", async () => { + await seedBucket({ used: 1 }); + await repo.commitModel({ userId: "u1", surface: "playground", modelId: null, now: NOW }); + expect((await repo.findBucket("u1", "playground", MARKER))?.usedByModel["__unknown__"]).toBe(1); + }); }); -describe("UT-QUOTAREPO-005 appendGrantAudit inserts row with all spec fields", () => { - test("audit row carries admin + target + month details", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); - const id = await repo.appendGrantAudit({ - adminUserId: "a1", - adminEmail: "a@x.test", - adminDisplayName: "Admin", - targetUserId: "u1", - surface: "playground", - amount: 5, - note: "initial", - monthMarker: "2026-05", - createdAt: now, - }); - const row = await db - .collection("quota_grants_audit") - .findOne({ _id: id as unknown as never }); - expect(row?.adminUserId).toBe("a1"); - expect(row?.targetUserId).toBe("u1"); - expect(row?.surface).toBe("playground"); - expect(row?.amount).toBe(5); - expect(row?.note).toBe("initial"); - expect(row?.monthMarker).toBe("2026-05"); +describe("UT-QUOTAREPO-006 reserve + commit models the full charge lifecycle", () => { + test("reserve then commit → used=1, usedByModel.=1 (matches legacy increment)", async () => { + await repo.reserveSlot({ userId: "u1", surface: "playground", effectiveDefault: 100, now: NOW }); + await repo.commitModel({ userId: "u1", surface: "playground", modelId: "gpt-4o", now: NOW }); + const b = await repo.findBucket("u1", "playground", MARKER); + expect(b?.used).toBe(1); + expect(b?.usedByModel["gpt-4o"]).toBe(1); }); }); -describe("UT-QUOTAREPO-006 findLifetime sorts asc by monthMarker", () => { +describe("UT-QUOTAREPO-007 findLifetime sorts asc by monthMarker", () => { test("3 buckets seeded out of order → returned chronologically", async () => { const months = [ { marker: "2026-04", date: new Date(Date.UTC(2026, 3, 15)) }, @@ -165,11 +244,10 @@ describe("UT-QUOTAREPO-006 findLifetime sorts asc by monthMarker", () => { { marker: "2026-05", date: new Date(Date.UTC(2026, 4, 15)) }, ]; for (const { date } of months) { - await repo.incrementUsed({ + await repo.reserveSlot({ userId: "u1", surface: "playground", - modelId: "m", - defaultAllotment: 100, + effectiveDefault: 100, now: date, }); } @@ -178,7 +256,7 @@ describe("UT-QUOTAREPO-006 findLifetime sorts asc by monthMarker", () => { }); }); -describe("UT-QUOTAREPO-007 findLifetime returns [] for new user", () => { +describe("UT-QUOTAREPO-008 findLifetime returns [] for new user", () => { test("empty array, not null", async () => { const lifetime = await repo.findLifetime("nobody", "playground"); expect(lifetime).toEqual([]); @@ -187,28 +265,52 @@ describe("UT-QUOTAREPO-007 findLifetime returns [] for new user", () => { describe("incrementAdminGrant is atomic", () => { test("two parallel grants accumulate to sum", async () => { - const now = new Date(Date.UTC(2026, 4, 15)); await Promise.all([ repo.incrementAdminGrant({ userId: "u1", surface: "playground", amount: 5, defaultAllotment: 100, - now, + now: NOW, }), repo.incrementAdminGrant({ userId: "u1", surface: "playground", amount: 7, defaultAllotment: 100, - now, + now: NOW, }), ]); - const b = await repo.findBucket("u1", "playground", "2026-05"); + const b = await repo.findBucket("u1", "playground", MARKER); expect(b?.adminGrant).toBe(12); }); }); +describe("UT-QUOTAREPO-005a appendGrantAudit inserts row with all spec fields", () => { + test("audit row carries admin + target + month details", async () => { + const id = await repo.appendGrantAudit({ + adminUserId: "a1", + adminEmail: "a@x.test", + adminDisplayName: "Admin", + targetUserId: "u1", + surface: "playground", + amount: 5, + note: "initial", + monthMarker: MARKER, + createdAt: NOW, + }); + const row = await db + .collection("quota_grants_audit") + .findOne({ _id: id as unknown as never }); + expect(row?.adminUserId).toBe("a1"); + expect(row?.targetUserId).toBe("u1"); + expect(row?.surface).toBe("playground"); + expect(row?.amount).toBe(5); + expect(row?.note).toBe("initial"); + expect(row?.monthMarker).toBe(MARKER); + }); +}); + describe("listGrantAudit pagination + filter", () => { test("page=2 pageSize=5 returns rows 6..10", async () => { const base = new Date(Date.UTC(2026, 4, 1)).getTime(); @@ -220,7 +322,7 @@ describe("listGrantAudit pagination + filter", () => { targetUserId: `u${i}`, surface: "playground", amount: 1, - monthMarker: "2026-05", + monthMarker: MARKER, createdAt: new Date(base + i * 1000), }); } @@ -237,7 +339,7 @@ describe("listGrantAudit pagination + filter", () => { targetUserId: "uA", surface: "playground", amount: 1, - monthMarker: "2026-05", + monthMarker: MARKER, createdAt: new Date(), }); await repo.appendGrantAudit({ @@ -247,7 +349,7 @@ describe("listGrantAudit pagination + filter", () => { targetUserId: "uB", surface: "playground", amount: 1, - monthMarker: "2026-05", + monthMarker: MARKER, createdAt: new Date(), }); const res = await repo.listGrantAudit({ page: 1, pageSize: 50, targetUserId: "uA" }); diff --git a/ornn-api/src/domains/quota/repository.ts b/ornn-api/src/domains/quota/repository.ts index 50923473..cb3c6604 100644 --- a/ornn-api/src/domains/quota/repository.ts +++ b/ornn-api/src/domains/quota/repository.ts @@ -2,14 +2,19 @@ * Mongo persistence for the calendar-month quota bucket model. * * `quota_buckets` — one document per (userId, surface, - * monthMarker). Atomic `$inc` on `used` / - * `usedByModel` / `adminGrant`. + * monthMarker). `used` doubles as the + * reservation counter: `reserveSlot` does a + * cap-guarded atomic `$inc` (the TOCTOU fix, + * #808), `commitModel` records the per-model + * tally, `releaseSlot` refunds on failure. + * `adminGrant` has its own atomic `$inc`. * `quota_grants_audit` — append-only history of admin grants * (replaces the old drainable ledger). * * @module domains/quota/repository */ +import { MongoServerError } from "mongodb"; import type { Collection, Db } from "mongodb"; import { randomUUID } from "node:crypto"; import { createLogger } from "../../shared/logger"; @@ -24,11 +29,31 @@ import { const logger = createLogger("quotaRepository"); -export interface UpsertChargeParams { +/** Mongo duplicate-key error code (E11000). */ +const DUPLICATE_KEY_CODE = 11000; + +export interface ReserveSlotParams { + userId: string; + surface: Surface; + /** + * The runtime-effective default (`max(stored, currentSettingsDefault)`). + * Combined with the bucket's `adminGrant` this forms the cap the + * reservation is guarded against: `used < adminGrant + effectiveDefault`. + */ + effectiveDefault: number; + now?: Date; +} + +export interface CommitModelParams { userId: string; surface: Surface; modelId: string | null | undefined; - defaultAllotment: number; + now?: Date; +} + +export interface ReleaseSlotParams { + userId: string; + surface: Surface; now?: Date; } @@ -98,39 +123,101 @@ export class QuotaRepository { } /** - * Atomically increment `used` and `usedByModel.`. Upserts a fresh - * bucket on first touch with the spec defaults — `$setOnInsert` keeps - * existing buckets untouched. Returns the after-state for callers - * who want to log resulting counters. + * Atomically reserve one slot by bumping `used`, guarded by the cap + * (`used < adminGrant + effectiveDefault`). This is the time-of-check = + * time-of-use fix (#808): N concurrent requests at `used = cap-1` race + * on the same conditional `$inc`, so at most one wins — the rest see + * the guard fail and are denied. `used` doubles as the reservation + * counter; the per-model tally is recorded later by `commitModel` and + * a failed run refunds via `releaseSlot`. + * + * Returns `true` when a slot was reserved, `false` when the cap is hit. + * + * First-touch (no bucket yet) is handled by an insert with `used = 1`; + * a lost insert race (E11000) retries the guarded update. */ - async incrementUsed(params: UpsertChargeParams): Promise { + async reserveSlot(params: ReserveSlotParams): Promise { const now = params.now ?? new Date(); const { monthMarker, monthStart, monthEnd } = monthBounds(now); const id = bucketId(params.userId, params.surface, monthMarker); - const modelKey = escapeModelKey(params.modelId); - const result = await this.buckets.findOneAndUpdate( - { _id: id }, - { - $inc: { used: 1, [`usedByModel.${modelKey}`]: 1 }, - $set: { updatedAt: now }, - $setOnInsert: { - userId: params.userId, - surface: params.surface, - monthMarker, - monthStart, - monthEnd, - defaultAllotment: params.defaultAllotment, - adminGrant: 0, - createdAt: now, + // Cap-guarded conditional increment on an existing bucket. `$expr` + // lets the cap reference the doc's own `adminGrant`, so an admin + // grant applied mid-month is respected without re-reading first. + const guardedUpdate = () => + this.buckets.findOneAndUpdate( + { + _id: id, + $expr: { $lt: ["$used", { $add: ["$adminGrant", params.effectiveDefault] }] }, }, - }, - { upsert: true, returnDocument: "after" }, - ); - if (!result) { - throw new Error(`incrementUsed: upsert returned null for ${id}`); + { $inc: { used: 1 }, $set: { updatedAt: now } }, + { returnDocument: "after" }, + ); + + const updated = await guardedUpdate(); + if (updated) return true; + + // No match: either the bucket exists and is at/over cap, or it + // doesn't exist yet. A zero (or negative) cap can never admit the + // first request, so don't bother creating a bucket. + if (params.effectiveDefault < 1) return false; + + try { + await this.buckets.insertOne({ + _id: id, + userId: params.userId, + surface: params.surface, + monthMarker, + monthStart, + monthEnd, + defaultAllotment: params.effectiveDefault, + adminGrant: 0, + used: 1, + usedByModel: {}, + createdAt: now, + updatedAt: now, + }); + return true; + } catch (err) { + // Lost the insert race — another concurrent reserve created the + // bucket first. Fall back to the guarded update and honour the cap. + if (err instanceof MongoServerError && err.code === DUPLICATE_KEY_CODE) { + const retry = await guardedUpdate(); + return !!retry; + } + throw err; } - return result; + } + + /** + * Record the per-model tally for a committed run. The slot's `used` + * counter was already bumped by `reserveSlot`, so this only touches + * `usedByModel.` — it must NOT bump `used` again. + */ + async commitModel(params: CommitModelParams): Promise { + const now = params.now ?? new Date(); + const { monthMarker } = monthBounds(now); + const id = bucketId(params.userId, params.surface, monthMarker); + const modelKey = escapeModelKey(params.modelId); + await this.buckets.updateOne( + { _id: id }, + { $inc: { [`usedByModel.${modelKey}`]: 1 }, $set: { updatedAt: now } }, + ); + } + + /** + * Refund a reserved slot (e.g. the LLM call failed with a system + * error or the client aborted). Decrements `used`, floored at 0 via + * the `used > 0` guard so a double-release can never drive it negative. + */ + async releaseSlot(params: ReleaseSlotParams): Promise { + const now = params.now ?? new Date(); + const { monthMarker } = monthBounds(now); + const id = bucketId(params.userId, params.surface, monthMarker); + await this.buckets.updateOne( + { _id: id, used: { $gt: 0 } }, + { $inc: { used: -1 }, $set: { updatedAt: now } }, + ); } /** diff --git a/ornn-api/src/domains/quota/routes.ts b/ornn-api/src/domains/quota/routes.ts index d7691a0c..21d4470b 100644 --- a/ornn-api/src/domains/quota/routes.ts +++ b/ornn-api/src/domains/quota/routes.ts @@ -30,6 +30,9 @@ export function createQuotaRoutes( const auth = nyxidAuthMiddleware(); app.get("/me/quota", auth, async (c) => { + // `remaining` here reflects in-flight reservations: `used` is bumped + // at reserve time (before the LLM call) and refunded on + // system-error/abort. See `SurfaceSnapshot.remaining` in types.ts. const authCtx = getAuth(c); const snapshot = await quotaService.getSnapshot({ userId: authCtx.userId, diff --git a/ornn-api/src/domains/quota/service.test.ts b/ornn-api/src/domains/quota/service.test.ts index 923ccfde..630a26bd 100644 --- a/ornn-api/src/domains/quota/service.test.ts +++ b/ornn-api/src/domains/quota/service.test.ts @@ -32,44 +32,76 @@ class FakeRepo { .sort((a, b) => a.monthMarker.localeCompare(b.monthMarker)); } - async incrementUsed(p: { + /** + * In-memory mirror of the atomic cap-guarded reserve. JS runs this + * body to completion without interleaving (no `await` between the + * read and the write), so it is serialized the same way Mongo's + * single-document `findOneAndUpdate` is — at most one reserve per + * slot wins. + */ + async reserveSlot(p: { userId: string; surface: Surface; - modelId: string | null | undefined; - defaultAllotment: number; + effectiveDefault: number; now?: Date; }) { const now = p.now ?? new Date(); const { monthMarker, monthStart, monthEnd } = monthBounds(now); const id = bucketId(p.userId, p.surface, monthMarker); const existing = this.buckets.get(id); + if (existing) { + const cap = existing.adminGrant + p.effectiveDefault; + if (existing.used >= cap) return false; + this.buckets.set(id, { ...existing, used: existing.used + 1, updatedAt: now }); + return true; + } + if (p.effectiveDefault < 1) return false; + this.buckets.set(id, { + _id: id, + userId: p.userId, + surface: p.surface, + monthMarker, + monthStart, + monthEnd, + defaultAllotment: p.effectiveDefault, + adminGrant: 0, + used: 1, + usedByModel: {}, + createdAt: now, + updatedAt: now, + }); + return true; + } + + async commitModel(p: { + userId: string; + surface: Surface; + modelId: string | null | undefined; + now?: Date; + }) { + const now = p.now ?? new Date(); + const { monthMarker } = monthBounds(now); + const id = bucketId(p.userId, p.surface, monthMarker); + const existing = this.buckets.get(id); + if (!existing) return; const modelKey = p.modelId && p.modelId.length > 0 ? p.modelId : "__unknown__"; - const next: QuotaBucketDoc = existing - ? { - ...existing, - used: existing.used + 1, - usedByModel: { - ...existing.usedByModel, - [modelKey]: (existing.usedByModel[modelKey] ?? 0) + 1, - }, - updatedAt: now, - } - : { - _id: id, - userId: p.userId, - surface: p.surface, - monthMarker, - monthStart, - monthEnd, - defaultAllotment: p.defaultAllotment, - adminGrant: 0, - used: 1, - usedByModel: { [modelKey]: 1 }, - createdAt: now, - updatedAt: now, - }; - this.buckets.set(id, next); - return next; + this.buckets.set(id, { + ...existing, + usedByModel: { + ...existing.usedByModel, + [modelKey]: (existing.usedByModel[modelKey] ?? 0) + 1, + }, + updatedAt: now, + }); + } + + async releaseSlot(p: { userId: string; surface: Surface; now?: Date }) { + const now = p.now ?? new Date(); + const { monthMarker } = monthBounds(now); + const id = bucketId(p.userId, p.surface, monthMarker); + const existing = this.buckets.get(id); + if (!existing || existing.used <= 0) return; + this.buckets.set(id, { ...existing, used: existing.used - 1, updatedAt: now }); } async incrementAdminGrant(p: { @@ -180,8 +212,8 @@ describe("UT-QUOTA-002 first-call shows clean snapshot", () => { }); }); -describe("UT-QUOTA-003 allowed when under cap", () => { - test("used < default+grant → allowed", async () => { +describe("UT-QUOTA-003 allowed when under cap (reserves the slot)", () => { + test("used < default+grant → allowed; checkAllowed reserves (used++)", async () => { const { service, repo } = build(); const now = new Date(Date.UTC(2026, 4, 15)); repo.buckets.set("u1:playground:2026-05", { @@ -205,6 +237,8 @@ describe("UT-QUOTA-003 allowed when under cap", () => { now, }); expect(d.allowed).toBe(true); + // The check IS the reserve now (#808): the slot is claimed up front. + expect(repo.buckets.get("u1:playground:2026-05")?.used).toBe(51); }); }); @@ -234,6 +268,8 @@ describe("UT-QUOTA-004 blocked when at cap", () => { }); expect(d.allowed).toBe(false); if (!d.allowed) expect(d.message).toContain("playground"); + // A denied reserve must not over-count past the cap. + expect(repo.buckets.get("u1:playground:2026-05")?.used).toBe(100); }); }); @@ -265,10 +301,11 @@ describe("UT-QUOTA-005 admin grant lifts ceiling", () => { }); }); -describe("UT-QUOTA-006 charge success increments used+usedByModel", () => { - test("used+=1 and usedByModel.+=1", async () => { +describe("UT-QUOTA-006 reserve+success commits used+usedByModel", () => { + test("reserve bumps used to 1; success commit records usedByModel.", async () => { const { service, repo } = build(); const now = new Date(Date.UTC(2026, 4, 15)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now }); await service.chargeOnCompletion({ userId: "u1", permissions: [], @@ -283,9 +320,10 @@ describe("UT-QUOTA-006 charge success increments used+usedByModel", () => { }); }); -describe("UT-QUOTA-007 skill_error charges 1", () => { - test("used+=1 on skill_error", async () => { +describe("UT-QUOTA-007 reserve+skill_error keeps the slot consumed", () => { + test("used stays 1 on skill_error (chargeable — ran the skill)", async () => { const { service, repo } = build(); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground" }); await service.chargeOnCompletion({ userId: "u1", permissions: [], @@ -296,22 +334,25 @@ describe("UT-QUOTA-007 skill_error charges 1", () => { }); }); -describe("UT-QUOTA-008 system_error charges 0", () => { - test("DB unchanged on system_error", async () => { +describe("UT-QUOTA-008 reserve+system_error releases the slot", () => { + test("used returns to 0 on system_error (refund — never reached skill)", async () => { const { service, repo } = build(); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground" }); + expect([...repo.buckets.values()][0]?.used).toBe(1); await service.chargeOnCompletion({ userId: "u1", permissions: [], surface: "playground", outcome: "system_error", }); - expect(repo.buckets.size).toBe(0); + expect([...repo.buckets.values()][0]?.used).toBe(0); }); }); describe("UT-QUOTA-009 unknown modelId routes to __unknown__", () => { - test("missing modelId → usedByModel.__unknown__: 1", async () => { + test("missing modelId on commit → usedByModel.__unknown__: 1", async () => { const { service, repo } = build(); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground" }); await service.chargeOnCompletion({ userId: "u1", permissions: [], @@ -322,25 +363,70 @@ describe("UT-QUOTA-009 unknown modelId routes to __unknown__", () => { }); }); -describe("UT-QUOTA-011 boundary millisecond rollover", () => { - test("Jun 1 00:00:00.000Z creates new June bucket; May untouched", async () => { +describe("UT-QUOTA-009a checkAllowed denies once the cap is reached", () => { + test("repeated reserves on a cap-1 default exhaust then deny", async () => { + const { service, repo } = build({ defaultPg: 2 }); + const now = new Date(Date.UTC(2026, 4, 15)); + const d1 = await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now }); + const d2 = await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now }); + const d3 = await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now }); + expect(d1.allowed).toBe(true); + expect(d2.allowed).toBe(true); + expect(d3.allowed).toBe(false); + // Cap is 2 — the third reserve must not push used to 3. + expect(repo.buckets.get("u1:playground:2026-05")?.used).toBe(2); + }); +}); + +describe("UT-QUOTA-009b admin bypass reserves and releases nothing", () => { + test("admin checkAllowed + chargeOnCompletion (both outcomes) leave no bucket", async () => { const { service, repo } = build(); - const may = new Date(Date.UTC(2026, 4, 31, 23, 59, 59, 999)); + const d = await service.checkAllowed({ + userId: "admin", + permissions: [ADMIN_PERM], + surface: "playground", + }); + expect(d.allowed).toBe(true); + if (d.allowed) expect(d.isAdminBypass).toBe(true); await service.chargeOnCompletion({ - userId: "u1", - permissions: [], + userId: "admin", + permissions: [ADMIN_PERM], surface: "playground", - outcome: "success", - now: may, + outcome: "system_error", }); - const jun = new Date(Date.UTC(2026, 5, 1, 0, 0, 0, 0)); await service.chargeOnCompletion({ - userId: "u1", - permissions: [], + userId: "admin", + permissions: [ADMIN_PERM], surface: "playground", outcome: "success", - now: jun, }); + expect(repo.buckets.size).toBe(0); + }); +}); + +describe("UT-QUOTA-010 release then reserve frees the slot again", () => { + test("a system_error refund lets a later request through at the cap", async () => { + const { service, repo } = build({ defaultPg: 1 }); + const now = new Date(Date.UTC(2026, 4, 15)); + // Consume the single slot. + expect((await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now })).allowed).toBe(true); + // Second is denied at the cap. + expect((await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now })).allowed).toBe(false); + // First run fails with a system error → slot released. + await service.chargeOnCompletion({ userId: "u1", permissions: [], surface: "playground", outcome: "system_error", now }); + expect(repo.buckets.get("u1:playground:2026-05")?.used).toBe(0); + // Now a fresh request is admitted again. + expect((await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now })).allowed).toBe(true); + }); +}); + +describe("UT-QUOTA-011 boundary millisecond rollover", () => { + test("Jun 1 00:00:00.000Z creates new June bucket; May untouched", async () => { + const { service, repo } = build(); + const may = new Date(Date.UTC(2026, 4, 31, 23, 59, 59, 999)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now: may }); + const jun = new Date(Date.UTC(2026, 5, 1, 0, 0, 0, 0)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now: jun }); const mayBucket = repo.buckets.get("u1:playground:2026-05"); const junBucket = repo.buckets.get("u1:playground:2026-06"); expect(mayBucket?.used).toBe(1); @@ -352,13 +438,7 @@ describe("UT-QUOTA-012 last millisecond of May → 2026-05", () => { test("monthMarker stays May at 23:59:59.999", async () => { const { service, repo } = build(); const may = new Date(Date.UTC(2026, 4, 31, 23, 59, 59, 999)); - await service.chargeOnCompletion({ - userId: "u1", - permissions: [], - surface: "playground", - outcome: "success", - now: may, - }); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now: may }); expect(repo.buckets.has("u1:playground:2026-05")).toBe(true); }); }); @@ -532,25 +612,48 @@ describe("UT-QUOTA-021 snapshot has no daily/expiresAt", () => { }); }); -describe("UT-QUOTA-023 year boundary rollover", () => { - test("Dec 31 23:59:59.999Z → 2026; Jan 1 00:00Z → 2027", async () => { - const { service, repo } = build(); - const dec = new Date(Date.UTC(2026, 11, 31, 23, 59, 59, 999)); - await service.chargeOnCompletion({ +describe("UT-QUOTA-024 charge reconciles against the reserved month bucket", () => { + test("reserve in May, charge with the May reservedAt (wall-clock June) → May bucket charged, no June bucket", async () => { + const { service, repo } = build({ defaultPg: 5 }); + // Reservation taken at the very last second of May. + const reservedAt = new Date(Date.UTC(2026, 4, 31, 23, 59, 59, 0)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", - outcome: "success", - now: dec, + now: reservedAt, }); - const jan = new Date(Date.UTC(2027, 0, 1, 0, 0, 0, 0)); + // The May bucket now holds the reservation. + expect(repo.buckets.get("u1:playground:2026-05")?.used).toBe(1); + + // The run finishes after the UTC month rollover. #827: the route + // threads the reservation instant (`reservedAt`, still May) into the + // charge so the per-model commit lands in the SAME bucket the slot + // was reserved against — NOT a fresh June bucket keyed by wall-clock. await service.chargeOnCompletion({ userId: "u1", permissions: [], surface: "playground", outcome: "success", - now: jan, + modelId: "gpt-4o", + now: reservedAt, }); + + const mayBucket = repo.buckets.get("u1:playground:2026-05"); + expect(mayBucket?.used).toBe(1); + expect(mayBucket?.usedByModel["gpt-4o"]).toBe(1); + // No June bucket was created — the charge did not straddle the boundary. + expect(repo.buckets.has("u1:playground:2026-06")).toBe(false); + }); +}); + +describe("UT-QUOTA-023 year boundary rollover", () => { + test("Dec 31 23:59:59.999Z → 2026; Jan 1 00:00Z → 2027", async () => { + const { service, repo } = build(); + const dec = new Date(Date.UTC(2026, 11, 31, 23, 59, 59, 999)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now: dec }); + const jan = new Date(Date.UTC(2027, 0, 1, 0, 0, 0, 0)); + await service.checkAllowed({ userId: "u1", permissions: [], surface: "playground", now: jan }); expect(repo.buckets.has("u1:playground:2026-12")).toBe(true); expect(repo.buckets.has("u1:playground:2027-01")).toBe(true); }); diff --git a/ornn-api/src/domains/quota/service.ts b/ornn-api/src/domains/quota/service.ts index 3bf9e568..498d8ffb 100644 --- a/ornn-api/src/domains/quota/service.ts +++ b/ornn-api/src/domains/quota/service.ts @@ -2,9 +2,13 @@ * Quota policy on the calendar-month bucket model. * * - Admins (`ornn:admin:skill`) bypass entirely. - * - `checkAllowed` reads the current-month bucket. Pure read; no upsert. - * - `chargeOnCompletion` upserts the bucket and atomically increments - * `used` and `usedByModel.`. `system_error` outcomes are no-ops. + * - `checkAllowed` atomically *reserves* a slot (#808): a cap-guarded + * `$inc` on `used`, so concurrent requests at the cap boundary can't + * all pass the check and over-spend. Returns the allow/deny decision. + * - `chargeOnCompletion` reconciles the reservation by outcome: + * `system_error` (incl. abort) → release (refund the slot); + * `success` / `skill_error` → commit the per-model tally. `used` + * was already bumped at reserve time and is not touched again. * - `grant` adds to `adminGrant` for the current month and appends an * audit row. Negative grants rejected (out of scope for v1). * - Default allotment is settings-driven; runtime computes @@ -76,6 +80,16 @@ export class QuotaService { : def.defaultSkillGenMonthly; } + /** + * Atomically reserve a slot before the LLM call. The reservation IS + * the charge — it bumps `used` under a cap guard so concurrent + * requests at the boundary can't all be admitted (#808, CWE-367). + * A reservation that doesn't reach a chargeable outcome is refunded + * by `chargeOnCompletion` (`system_error`/abort → release). + * + * Admins bypass entirely: no reservation is taken and none is + * released, so their runs never touch a bucket. + */ async checkAllowed(params: { userId: string; permissions: readonly string[] | undefined; @@ -86,18 +100,38 @@ export class QuotaService { return { allowed: true, isAdminBypass: true }; } const now = params.now ?? new Date(); - const { monthMarker } = monthBounds(now); - const [bucket, currentDefault] = await Promise.all([ - this.repo.findBucket(params.userId, params.surface, monthMarker), - this.resolveDefault(params.surface), - ]); - const stored = bucket?.defaultAllotment ?? 0; + // The bucket stores the first-touch default; the runtime-effective + // default may be higher if the admin raised it mid-month. The repo + // guards against `adminGrant + effectiveDefault`, reading the bucket's + // own `adminGrant` inside the atomic update. + const currentDefault = await this.resolveDefault(params.surface); + const existing = await this.repo.findBucket( + params.userId, + params.surface, + monthBounds(now).monthMarker, + ); + const stored = existing?.defaultAllotment ?? 0; const effectiveDefault = Math.max(stored, currentDefault); - const adminGrant = bucket?.adminGrant ?? 0; - const used = bucket?.used ?? 0; - if (used < effectiveDefault + adminGrant) { + + const reserved = await this.repo.reserveSlot({ + userId: params.userId, + surface: params.surface, + effectiveDefault, + now, + }); + + if (reserved) { + logger.info( + { userId: params.userId, surface: params.surface }, + "Quota slot reserved", + ); return { allowed: true, isAdminBypass: false }; } + + logger.info( + { userId: params.userId, surface: params.surface }, + "Quota denied — cap reached", + ); return { allowed: false, isAdminBypass: false, @@ -106,6 +140,15 @@ export class QuotaService { }; } + /** + * Reconcile a reservation once the run terminates. The slot was + * already taken at `checkAllowed` time, so this never bumps `used`: + * - `system_error` (LLM timeout, infra 5xx, client abort) → release + * the slot (refund), since the request never reached the skill. + * - `success` / `skill_error` → commit the per-model tally; the slot + * stays consumed. + * Admins took no reservation, so this is a no-op for them. + */ async chargeOnCompletion(params: { userId: string; permissions: readonly string[] | undefined; @@ -115,14 +158,25 @@ export class QuotaService { now?: Date; }): Promise { if (this.isAdmin(params.permissions)) return; - if (params.outcome === "system_error") return; const now = params.now ?? new Date(); - const def = await this.resolveDefault(params.surface); - await this.repo.incrementUsed({ + + if (params.outcome === "system_error") { + await this.repo.releaseSlot({ + userId: params.userId, + surface: params.surface, + now, + }); + logger.info( + { userId: params.userId, surface: params.surface, outcome: params.outcome }, + "Quota reservation released", + ); + return; + } + + await this.repo.commitModel({ userId: params.userId, surface: params.surface, modelId: params.modelId, - defaultAllotment: def, now, }); logger.debug( diff --git a/ornn-api/src/domains/quota/types.ts b/ornn-api/src/domains/quota/types.ts index c97eb94a..23d3b939 100644 --- a/ornn-api/src/domains/quota/types.ts +++ b/ornn-api/src/domains/quota/types.ts @@ -77,6 +77,12 @@ export interface SurfaceSnapshot { defaultAllotment: number; adminGrant: number; used: number; + /** + * Reflects in-flight reservations — `used` is bumped at reserve time + * (before the LLM call), so `remaining` already accounts for runs + * currently streaming. A run that ends in system_error/abort releases + * its slot, raising `remaining` back. + */ remaining: number; warningThreshold: number; warning: boolean; diff --git a/ornn-api/src/domains/redemption-codes/me-routes.ts b/ornn-api/src/domains/redemption-codes/me-routes.ts index 5c18caf5..ffd43637 100644 --- a/ornn-api/src/domains/redemption-codes/me-routes.ts +++ b/ornn-api/src/domains/redemption-codes/me-routes.ts @@ -24,6 +24,7 @@ import { validateBody, getValidatedBody } from "../../middleware/validate"; import { AppError } from "../../shared/types/index"; import type { RedemptionCodeService } from "./service"; import { redeemSchema, type RedeemInput, type RedemptionCodeDoc } from "./types"; +import { MAX_PAGE } from "../../shared/cursor"; const logger = createLogger("meRedemptionCodeRoutes"); @@ -130,7 +131,9 @@ export function createMeRedemptionCodesRoutes( // GET /me/redemption-codes/history app.get("/me/redemption-codes/history", auth, async (c) => { const authCtx = getAuth(c); - const page = Math.max(1, Number(c.req.query("page")) || 1); + // Clamp to MAX_PAGE so a huge ?page= can't drive an unbounded + // `.skip()` scan in the redeemed-code history query (CWE-770, #810). + const page = Math.min(MAX_PAGE, Math.max(1, Number(c.req.query("page")) || 1)); const pageSize = Math.min( HISTORY_PAGE_SIZE_MAX, Math.max(1, Number(c.req.query("pageSize")) || HISTORY_PAGE_SIZE_DEFAULT), diff --git a/ornn-api/src/domains/settings/sections/mirror.ts b/ornn-api/src/domains/settings/sections/mirror.ts index 847541fe..386c5f1e 100644 --- a/ornn-api/src/domains/settings/sections/mirror.ts +++ b/ornn-api/src/domains/settings/sections/mirror.ts @@ -15,6 +15,7 @@ */ import { z } from "zod"; import { CronExpressionParser } from "cron-parser"; +import { OWNER_RE, REPO_RE } from "../../../shared/githubNaming"; import type { SectionMeta } from "./index"; /** @@ -44,8 +45,12 @@ const cronSchedule = z export const mirrorSchema = z.object({ enabled: z.boolean(), - owner: z.string(), - repo: z.string(), + owner: z.string().refine((v) => v === "" || OWNER_RE.test(v), { + message: "must be a valid GitHub owner or empty", + }), + repo: z.string().refine((v) => v === "" || REPO_RE.test(v), { + message: "must be a valid GitHub repo name or empty", + }), branch: z.string(), appId: z.string(), installationId: z.string(), diff --git a/ornn-api/src/domains/settings/sections/sections.test.ts b/ornn-api/src/domains/settings/sections/sections.test.ts index 3394de94..b97eb35f 100644 --- a/ornn-api/src/domains/settings/sections/sections.test.ts +++ b/ornn-api/src/domains/settings/sections/sections.test.ts @@ -335,6 +335,35 @@ describe("section schemas", () => { expect(mirrorSection.defaults.reconcileSchedule).toBe("0 2 * * *"); }); + it("UT-SCHEMA-MIRROR-005: owner/repo enforce GitHub naming, empty stays valid (#818)", () => { + // Path-traversal / slash-bearing repo rejected by REPO_RE. + expect( + mirrorSection.schema.safeParse({ + ...mirrorSection.defaults, + owner: "ChronoAIProject", + repo: "a/../b", + }).success, + ).toBe(false); + // Empty owner + empty repo = unset state, still valid. + expect( + mirrorSection.schema.safeParse({ + ...mirrorSection.defaults, + owner: "", + repo: "", + }).success, + ).toBe(true); + // Dotted + dashed repo names are valid GitHub repos. + for (const repo of ["repo.name", "repo-1"]) { + expect( + mirrorSection.schema.safeParse({ + ...mirrorSection.defaults, + owner: "ChronoAIProject", + repo, + }).success, + ).toBe(true); + } + }); + // -------- telemetry -------- it("telemetry schema accepts placeholder defaults", () => { expect( diff --git a/ornn-api/src/domains/skills/audit/routes.ts b/ornn-api/src/domains/skills/audit/routes.ts index d530eae8..63ec2c83 100644 --- a/ornn-api/src/domains/skills/audit/routes.ts +++ b/ornn-api/src/domains/skills/audit/routes.ts @@ -25,12 +25,11 @@ import { getAuth, nyxidAuthMiddleware, optionalAuthMiddleware, - readUserOrgMemberships, requirePermission, } from "../../../middleware/nyxidAuth"; import { AppError } from "../../../shared/types/index"; import { validateBody, getValidatedBody } from "../../../middleware/validate"; -import { canReadSkill } from "../crud/authorize"; +import { canReadSkill, buildActorContext } from "../crud/authorize"; /** Body schema for POST /skills/:idOrName/audit + admin variant (#438). */ const auditTriggerSchema = z.object({ @@ -72,12 +71,7 @@ export function createAuditRoutes(config: AuditRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -112,12 +106,7 @@ export function createAuditRoutes(config: AuditRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -149,12 +138,7 @@ export function createAuditRoutes(config: AuditRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } diff --git a/ornn-api/src/domains/skills/crud/authorize.test.ts b/ornn-api/src/domains/skills/crud/authorize.test.ts new file mode 100644 index 00000000..4eb9ae8a --- /dev/null +++ b/ornn-api/src/domains/skills/crud/authorize.test.ts @@ -0,0 +1,128 @@ +/** + * Unit tests for `buildActorContext` — the single source that derives the + * caller's object-level authorization actor from a request (#826). + * + * `buildActorContext(c)` consumes exactly two context keys: + * - `c.get("auth")` (via `getAuth`) → throws 401 when absent + * - `c.get("getUserOrgMembershipResolution")` (lazy) → memberships + + * resolved/unresolved discriminant passthrough (#842) + * + * Each case stubs a minimal fake Hono context — a plain `{ get }` cast to + * the Context type — wiring only those two keys. No env mutation, no DB, + * no NyxID round-trip: the lazy getter is a plain resolved promise. + */ + +import { describe, expect, it } from "bun:test"; +import type { Context } from "hono"; +import { buildActorContext } from "./authorize"; +import { + type AuthContext, + type AuthVariables, + type OrgMembershipFact, + type OrgMembershipResolution, +} from "../../../middleware/nyxidAuth"; +import { AppError } from "../../../shared/types/index"; + +/** + * Build a fake Hono context exposing only the keys buildActorContext reads. + * + * `resolution` mirrors what `nyxidOrgLookupMiddleware` would memoize. When + * omitted the resolution getter is unmounted, so `buildActorContext` falls + * back to the helper's `{ status: "resolved", memberships: [] }` default + * (the "middleware not mounted" path). + */ +function fakeContext(opts: { + auth?: AuthContext; + resolution?: OrgMembershipResolution; +}): Context<{ Variables: AuthVariables }> { + const getter = + opts.resolution !== undefined + ? async () => opts.resolution as OrgMembershipResolution + : undefined; + const store: Record = { + auth: opts.auth, + getUserOrgMembershipResolution: getter, + }; + return { + get: (key: string) => store[key], + } as unknown as Context<{ Variables: AuthVariables }>; +} + +/** Convenience: a resolved resolution carrying the given memberships. */ +function resolved(memberships: OrgMembershipFact[]): OrgMembershipResolution { + return { status: "resolved", memberships }; +} + +function authWith(permissions: string[]): AuthContext { + return { + userId: "user-1", + email: "u@example.com", + displayName: "User One", + roles: [], + permissions, + }; +} + +describe("buildActorContext", () => { + it("(a) flags platform admin when caller holds ornn:admin:skill", async () => { + const c = fakeContext({ auth: authWith(["ornn:admin:skill"]), resolution: resolved([]) }); + const actor = await buildActorContext(c); + expect(actor.isPlatformAdmin).toBe(true); + expect(actor.userId).toBe("user-1"); + }); + + it("(b) does not flag platform admin without ornn:admin:skill", async () => { + const c = fakeContext({ auth: authWith(["ornn:skill:read"]), resolution: resolved([]) }); + const actor = await buildActorContext(c); + expect(actor.isPlatformAdmin).toBe(false); + }); + + it("(c) throws 401 AppError when unauthenticated (no auth on context)", async () => { + const c = fakeContext({ resolution: resolved([]) }); + let thrown: unknown; + try { + await buildActorContext(c); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).statusCode).toBe(401); + }); + + it("(d) passes the lazy org-membership getter's array through verbatim", async () => { + const memberships: OrgMembershipFact[] = [ + { userId: "org-a", role: "admin", displayName: "Org A" }, + { userId: "org-b", role: "member", displayName: "Org B" }, + ]; + const c = fakeContext({ auth: authWith([]), resolution: resolved(memberships) }); + const actor = await buildActorContext(c); + expect(actor.memberships).toEqual(memberships); + }); + + it("(e) marks membershipsResolved true on a resolved lookup (#842)", async () => { + const c = fakeContext({ auth: authWith([]), resolution: resolved([]) }); + const actor = await buildActorContext(c); + // Resolved-empty is still resolved: caller is a member of nothing. + expect(actor.membershipsResolved).toBe(true); + expect(actor.memberships).toEqual([]); + }); + + it("(f) marks membershipsResolved false + memberships [] on an unresolved lookup (#842)", async () => { + const c = fakeContext({ + auth: authWith([]), + resolution: { status: "unresolved", reason: "lookup_failed", memberships: [] }, + }); + const actor = await buildActorContext(c); + expect(actor.membershipsResolved).toBe(false); + expect(actor.memberships).toEqual([]); + }); + + it("(g) defaults to resolved-empty when the resolution getter is unmounted (#842)", async () => { + // No `resolution` wired → middleware-not-mounted path. Must NOT flip to + // unresolved, so unrelated tests don't suddenly 503 on a share write. + const c = fakeContext({ auth: authWith([]) }); + const actor = await buildActorContext(c); + expect(actor.membershipsResolved).toBe(true); + expect(actor.memberships).toEqual([]); + }); +}); diff --git a/ornn-api/src/domains/skills/crud/authorize.ts b/ornn-api/src/domains/skills/crud/authorize.ts index 654a6019..3093ae50 100644 --- a/ornn-api/src/domains/skills/crud/authorize.ts +++ b/ornn-api/src/domains/skills/crud/authorize.ts @@ -26,7 +26,13 @@ * @module domains/skills/crud/authorize */ -import type { OrgMembershipFact } from "../../../middleware/nyxidAuth"; +import type { Context } from "hono"; +import { + getAuth, + readUserOrgMembershipResolution, + type AuthVariables, + type OrgMembershipFact, +} from "../../../middleware/nyxidAuth"; export interface SkillOwnership { /** Author (person user_id). Always present. */ @@ -42,6 +48,51 @@ export interface ActorContext { userId: string; memberships: OrgMembershipFact[]; isPlatformAdmin: boolean; + /** + * Whether the org-membership lookup resolved authoritatively (#842). + * `true` when NyxID answered (including a 200-empty list); `false` when + * the lookup was unresolved (no forwarded token / NyxID unreachable). + * Write gates that share a skill into orgs must distinguish an empty + * `memberships` that means "member of nothing" (resolved) from one that + * means "we couldn't ask" (unresolved) — the latter is a retryable 503, + * not a 403. Read gates ignore this field (they fail soft regardless). + */ + membershipsResolved: boolean; +} + +/** + * Internal/system caller — bypasses visibility (mirror, server-side jobs). + * `membershipsResolved: true` so system callers are never treated as having + * an unresolved org lookup. + */ +export const SYSTEM_ACTOR: ActorContext = { + userId: "__system__", + memberships: [], + isPlatformAdmin: true, + membershipsResolved: true, +}; + +/** + * Build the caller's object-level authorization actor from the request. + * Single source so the ~19 route-level builds cannot drift (#826). + * Throws 401 (via getAuth) when unauthenticated. Resolves org memberships + * via the lazy getter mounted by nyxidOrgLookupMiddleware. + */ +export async function buildActorContext( + c: Context<{ Variables: AuthVariables }>, +): Promise { + const auth = getAuth(c); + // Resolution-aware read (#842): carry whether NyxID answered so the write + // gate can tell "member of nothing" (resolved) from "couldn't ask" + // (unresolved). `.memberships` is the same fail-soft `[]` the read path + // already used for both cases, so read callers are unaffected. + const resolution = await readUserOrgMembershipResolution(c); + return { + userId: auth.userId, + memberships: resolution.memberships, + isPlatformAdmin: auth.permissions.includes("ornn:admin:skill"), + membershipsResolved: resolution.status === "resolved", + }; } /** Returns true when `actor` is allowed to read the skill. */ diff --git a/ornn-api/src/domains/skills/crud/repository.ts b/ornn-api/src/domains/skills/crud/repository.ts index 3765b85c..d8c91ab0 100644 --- a/ornn-api/src/domains/skills/crud/repository.ts +++ b/ornn-api/src/domains/skills/crud/repository.ts @@ -129,6 +129,14 @@ export interface ExtraFilters { export class SkillRepository { private readonly collection: Collection; + /** + * Server-side cap on paginated reads. Bounds the worst-case + * `.skip()` scan + its paired `countDocuments` so a deep page (even + * within the clamped page ceiling) can't tie up a Mongo worker for + * an unbounded stretch (CWE-770, #810). Mirrors admin/routes.ts. + */ + private static readonly MAX_QUERY_MS = 5_000; + constructor(db: Db) { this.collection = db.collection("skills"); } @@ -337,13 +345,16 @@ export class SkillRepository { applyScope(matchStage, scope, currentUserId, userOrgIds); applyExtraFilters(matchStage, { nyxidServiceId: serviceId }); - const total = await this.collection.countDocuments(matchStage); + const total = await this.collection.countDocuments(matchStage, { + maxTimeMS: SkillRepository.MAX_QUERY_MS, + }); const offset = (page - 1) * pageSize; const docs = await this.collection .find(matchStage) .sort({ createdOn: -1 }) .skip(offset) .limit(pageSize) + .maxTimeMS(SkillRepository.MAX_QUERY_MS) .toArray(); return { skills: docs.map((d) => mapDoc(d)!), total }; } @@ -381,9 +392,17 @@ export class SkillRepository { applyExtraFilters(matchStage, extraFilters); - const total = await this.collection.countDocuments(matchStage); + const total = await this.collection.countDocuments(matchStage, { + maxTimeMS: SkillRepository.MAX_QUERY_MS, + }); const offset = (page - 1) * pageSize; - const docs = await this.collection.find(matchStage).sort({ createdOn: -1 }).skip(offset).limit(pageSize).toArray(); + const docs = await this.collection + .find(matchStage) + .sort({ createdOn: -1 }) + .skip(offset) + .limit(pageSize) + .maxTimeMS(SkillRepository.MAX_QUERY_MS) + .toArray(); return { skills: docs.map((d) => mapDoc(d)!), total }; } @@ -408,9 +427,17 @@ export class SkillRepository { applyExtraFilters(matchStage, extraFilters); - const total = await this.collection.countDocuments(matchStage); + const total = await this.collection.countDocuments(matchStage, { + maxTimeMS: SkillRepository.MAX_QUERY_MS, + }); const offset = (page - 1) * pageSize; - const docs = await this.collection.find(matchStage).sort({ createdOn: -1 }).skip(offset).limit(pageSize).toArray(); + const docs = await this.collection + .find(matchStage) + .sort({ createdOn: -1 }) + .skip(offset) + .limit(pageSize) + .maxTimeMS(SkillRepository.MAX_QUERY_MS) + .toArray(); return { skills: docs.map((d) => mapDoc(d)!), total }; } diff --git a/ornn-api/src/domains/skills/crud/routes.ts b/ornn-api/src/domains/skills/crud/routes.ts index 5409d451..e44445c1 100644 --- a/ornn-api/src/domains/skills/crud/routes.ts +++ b/ornn-api/src/domains/skills/crud/routes.ts @@ -20,12 +20,11 @@ import { optionalAuthMiddleware, requirePermission, getAuth, - readUserOrgMemberships, readUserOrgIds, } from "../../../middleware/nyxidAuth"; import { validateBody, getValidatedBody } from "../../../middleware/validate"; import { AppError } from "../../../shared/types/index"; -import { canReadSkill, canManageSkill } from "./authorize"; +import { canReadSkill, canManageSkill, buildActorContext } from "./authorize"; import { parseGithubUrl } from "./utils/githubPull"; import { enforceZipLimits } from "../../../shared/utils/zipLimits"; import { rateLimit } from "../../../middleware/rateLimit"; @@ -565,29 +564,22 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: const version = c.req.query("version"); logger.info({ idOrName, version: version ?? null }, "Skill jsonize request"); + // Build the caller's actor once — `requirePermission` above + // guarantees authCtx is set. Used both for the defense-in-depth + // pre-check below and threaded into the service call (#806). + const actor = await buildActorContext(c); + // Visibility check (#567) — the package contents endpoint must // not be more permissive than the metadata endpoint. Load the // skill first and reject inaccessible private skills with the - // same `SKILL_NOT_FOUND` shape `/skills/:idOrName` uses. + // same `SKILL_NOT_FOUND` shape `/skills/:idOrName` uses. Kept as + // defense-in-depth on top of the service-level gate (#806). const skill = await skillService.getSkill(idOrName); - if (skill.isPrivate) { - const authCtx = c.get("auth"); - // `requirePermission` above guarantees authCtx is set. - if (!authCtx) { - throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); - } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; - if (!canReadSkill(skill, actor)) { - throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); - } + if (skill.isPrivate && !canReadSkill(skill, actor)) { + throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } - const result = await skillService.getSkillJson(idOrName, version); + const result = await skillService.getSkillJson(idOrName, actor, version); // Programmatic pull — closest signal to the north-star metric. // Fire-and-forget; the analytics service swallows its own errors. const authCtx = c.get("auth"); @@ -637,12 +629,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -677,12 +664,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -716,12 +698,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: // Authenticated users: apply the full ownership/org-visibility rules. if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -796,12 +773,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${id}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -860,12 +832,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } if (authCtx && skill.isPrivate) { - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canReadSkill(skill, actor)) { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } @@ -891,18 +858,13 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: async (c) => { const id = c.req.param("id"); const tag = c.req.param("tag"); - const authCtx = getAuth(c); const existing = await skillRepo.findByGuid(id); if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${id}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + // buildActorContext throws 401 when unauthenticated. + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -929,18 +891,13 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: async (c) => { const id = c.req.param("id"); const tag = c.req.param("tag"); - const authCtx = getAuth(c); const existing = await skillRepo.findByGuid(id); if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${id}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + // buildActorContext throws 401 when unauthenticated. + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -972,12 +929,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -1101,12 +1053,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -1120,7 +1067,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: isPrivate: body.isPrivate, sharedWithUsers: body.sharedWithUsers, sharedWithOrgs: body.sharedWithOrgs, - }); + }, actor); const updated = await skillService.getSkill(guid); @@ -1169,9 +1116,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!existing) { throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } - const memberships = await readUserOrgMemberships(c); - const isPlatformAdmin = authCtx.permissions.includes("ornn:admin:skill"); - const actor = { userId: authCtx.userId, memberships, isPlatformAdmin }; + const actor = await buildActorContext(c); if (!canManageSkill(existing, actor)) { throw AppError.forbidden( "forbidden", @@ -1185,7 +1130,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: const updated = await skillService.tieToNyxidService( guid, body.nyxidServiceId, - { userId: authCtx.userId, isPlatformAdmin }, + { userId: authCtx.userId, isPlatformAdmin: actor.isPlatformAdmin }, async (id) => { // Synthetic ids (`synthetic:`) come from // `EXTRA_NYXID_SERVICES` config — short-circuit before the @@ -1341,12 +1286,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!skill) { throw AppError.notFound("skill_not_found", `Skill '${guid}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canManageSkill(skill, actor)) { throw AppError.forbidden( "forbidden", @@ -1393,12 +1333,7 @@ export function createSkillRoutes(config: SkillRoutesConfig): Hono<{ Variables: if (!skill) { throw AppError.notFound("skill_not_found", `Skill '${id}' not found`); } - const memberships = await readUserOrgMemberships(c); - const actor = { - userId: authCtx.userId, - memberships, - isPlatformAdmin: authCtx.permissions.includes("ornn:admin:skill"), - }; + const actor = await buildActorContext(c); if (!canManageSkill(skill, actor)) { throw AppError.forbidden( "forbidden", diff --git a/ornn-api/src/domains/skills/crud/service.test.ts b/ornn-api/src/domains/skills/crud/service.test.ts new file mode 100644 index 00000000..f238c0f3 --- /dev/null +++ b/ornn-api/src/domains/skills/crud/service.test.ts @@ -0,0 +1,461 @@ +/** + * #806 — object-level authorization (BOLA / OWASP API1) for + * `SkillService.getSkillJson`. + * + * The package-contents path used to download + return any skill's full + * source (incl. embedded secrets) with no visibility check. These tests + * lock the gate in at the service layer so every caller — playground, + * mirror, the `/json` route — is protected by one policy: + * + * - private skill + stranger actor → `skill_not_found`, NO download + * - SYSTEM_ACTOR / owner / platform admin → succeed + * - public skill → succeeds for any actor + * + * Run as a Bun unit test: the only repo method reached before the + * visibility gate is `findByGuid`, so the deny cases need no storage. + * The allow cases use an in-memory zip served via a `data:` URL so the + * download + extraction runs hermetically (no MinIO, no network). + */ + +import { describe, expect, it } from "bun:test"; +import JSZip from "jszip"; +import { SkillService } from "./service"; +import { SYSTEM_ACTOR, canReadSkill, type ActorContext } from "./authorize"; +import type { SkillRepository } from "./repository"; +import type { SkillVersionRepository } from "./skillVersionRepository"; +import type { IStorageClient } from "../../../clients/storageClient"; +import type { SkillDocument } from "../../../shared/types/index"; +import { AppError } from "../../../shared/types/index"; + +const SECRET_BODY = "SECRET_FROM_PACKAGE_b91c"; + +/** Build a one-file zip and hand it back as a fetchable `data:` URL. */ +async function zipDataUrl(): Promise { + const zip = new JSZip(); + zip.file("SKILL.md", `# demo\n${SECRET_BODY}`); + const buf = await zip.generateAsync({ type: "uint8array" }); + return `data:application/zip;base64,${Buffer.from(buf).toString("base64")}`; +} + +/** Minimal SkillDocument carrying just the fields getSkillJson reads. */ +function makeSkillDoc(overrides: Partial = {}): SkillDocument { + return { + guid: "guid-1", + name: "demo-skill", + description: "A demo skill.", + metadata: {} as SkillDocument["metadata"], + storageKey: "skills/guid-1/1.0.zip", + latestVersion: "1.0", + createdBy: "owner-1", + isPrivate: true, + sharedWithUsers: [], + sharedWithOrgs: [], + ...overrides, + } as SkillDocument; +} + +function makeService(skill: SkillDocument, presignedUrl: string): SkillService { + const skillRepo = { + findByGuid: async (guid: string) => (guid === skill.guid ? skill : null), + findByName: async (name: string) => (name === skill.name ? skill : null), + } as unknown as SkillRepository; + const skillVersionRepo = {} as unknown as SkillVersionRepository; + const storageClient = { + getPresignedUrl: async () => ({ presignedUrl, expiresAt: new Date().toISOString() }), + } as unknown as IStorageClient; + return new SkillService({ + skillRepo, + skillVersionRepo, + storageClient, + storageBucketResolver: async () => "test-bucket", + }); +} + +const STRANGER: ActorContext = { userId: "stranger", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; +const OWNER: ActorContext = { userId: "owner-1", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; +const ADMIN: ActorContext = { userId: "admin-1", memberships: [], isPlatformAdmin: true, membershipsResolved: true }; + +describe("SkillService.getSkillJson — object-level authorization (#806)", () => { + it("private skill + stranger actor throws skill_not_found before any download", async () => { + let downloaded = false; + const storageClient = { + getPresignedUrl: async () => { + downloaded = true; + return { presignedUrl: "http://unused", expiresAt: "" }; + }, + } as unknown as IStorageClient; + const skill = makeSkillDoc({ isPrivate: true }); + const service = new SkillService({ + skillRepo: { + findByGuid: async () => skill, + findByName: async () => null, + } as unknown as SkillRepository, + skillVersionRepo: {} as unknown as SkillVersionRepository, + storageClient, + storageBucketResolver: async () => "test-bucket", + }); + + let thrown: unknown; + try { + await service.getSkillJson("guid-1", STRANGER); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("skill_not_found"); + // Gate runs before storage — no presigned URL was ever requested. + expect(downloaded).toBe(false); + }); + + it("private skill succeeds for SYSTEM_ACTOR and returns package contents", async () => { + const service = makeService(makeSkillDoc({ isPrivate: true }), await zipDataUrl()); + const result = await service.getSkillJson("guid-1", SYSTEM_ACTOR); + expect(result.files["SKILL.md"]).toContain(SECRET_BODY); + }); + + it("private skill succeeds for the author", async () => { + const service = makeService(makeSkillDoc({ isPrivate: true, createdBy: "owner-1" }), await zipDataUrl()); + const result = await service.getSkillJson("guid-1", OWNER); + expect(result.files["SKILL.md"]).toContain(SECRET_BODY); + }); + + it("private skill succeeds for a platform admin", async () => { + const service = makeService(makeSkillDoc({ isPrivate: true }), await zipDataUrl()); + const result = await service.getSkillJson("guid-1", ADMIN); + expect(result.files["SKILL.md"]).toContain(SECRET_BODY); + }); + + it("public skill succeeds for any actor (including a stranger)", async () => { + const service = makeService(makeSkillDoc({ isPrivate: false }), await zipDataUrl()); + const result = await service.getSkillJson("guid-1", STRANGER); + expect(result.files["SKILL.md"]).toContain(SECRET_BODY); + }); +}); + +/** + * #807 (CWE-22) — `extractSkillInfoLenient` is the `skip_validation=true` + * import path. It used to accept ANY non-empty `name`, including + * `../evil` / `/etc/passwd` / `..`, which then flowed into the public- + * mirror blob paths and could escape the skill's own `/` subtree. + * The lenient path must now enforce the SAME kebab-case rule the strict + * Zod schema enforces. + * + * The method is private; we reach it via a cast (the file already casts + * deps to stubs elsewhere). It's a pure transform — no storage / repo + * call happens before the name guard — so empty stubs are sufficient. + */ +describe("SkillService.extractSkillInfoLenient — kebab-case name guard (#807)", () => { + function makeLenientService(): SkillService { + return new SkillService({ + skillRepo: {} as unknown as SkillRepository, + skillVersionRepo: {} as unknown as SkillVersionRepository, + storageClient: {} as unknown as IStorageClient, + storageBucketResolver: async () => "test-bucket", + }); + } + + /** Call the private method through a typed cast. */ + function callLenient(raw: Record): { name: string } { + const svc = makeLenientService() as unknown as { + extractSkillInfoLenient: (r: Record) => { name: string }; + }; + return svc.extractSkillInfoLenient(raw); + } + + const TRAVERSAL_NAMES = [ + "../evil", + "/etc/passwd", + "..", + "a/b", + "a\\b", + "Foo", + "a.b", + "-lead", + "a".repeat(65), + ]; + + for (const name of TRAVERSAL_NAMES) { + it(`rejects ${JSON.stringify(name)} with frontmatter_validation_failed`, () => { + let thrown: unknown; + try { + callLenient({ name }); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("frontmatter_validation_failed"); + expect((thrown as AppError).statusCode).toBe(400); + }); + } + + it("accepts a valid kebab-case name and returns it unchanged", () => { + const result = callLenient({ name: "valid-skill-1" }); + expect(result.name).toBe("valid-skill-1"); + }); +}); + +/** + * #815 (CWE-862) — `setSkillPermissions` org-membership gate. + * + * `PUT /skills/:id/permissions` let an owner share a skill into any org id, + * including ones they're not a member of. The service now intersects every + * deduped `sharedWithOrgs` id against the caller's memberships and rejects + * non-member ids with `not_org_member` (403), before the persistence write. + * Platform admins are exempt. + * + * The repo `update` stub echoes the patched doc so the success path resolves + * through `buildDetailResponse`. A capture flag on `update` proves the deny + * case short-circuits before any write. + */ +describe("SkillService.setSkillPermissions — org-membership gate (#815)", () => { + /** + * Build a service whose repo `findByGuid` returns a fixed skill doc and + * whose `update` echoes the patched fields back onto that doc while + * recording that it was called (via the returned `state.updateCalled`). + * `skillVersionRepo.findLatestBySkill` returns null so `buildDetailResponse` + * stays on the no-overlay path; storage just mints a dummy presigned URL. + */ + function makePermissionsService(skill: SkillDocument): { + service: SkillService; + state: { updateCalled: boolean }; + } { + const state = { updateCalled: false }; + const skillRepo = { + findByGuid: async (guid: string) => (guid === skill.guid ? skill : null), + findByName: async () => null, + update: async (_guid: string, patch: Partial) => { + state.updateCalled = true; + return { ...skill, ...patch } as SkillDocument; + }, + } as unknown as SkillRepository; + const skillVersionRepo = { + findLatestBySkill: async () => null, + } as unknown as SkillVersionRepository; + const storageClient = { + getPresignedUrl: async () => ({ presignedUrl: "http://unused", expiresAt: "" }), + } as unknown as IStorageClient; + const service = new SkillService({ + skillRepo, + skillVersionRepo, + storageClient, + storageBucketResolver: async () => "test-bucket", + }); + return { service, state }; + } + + it("rejects sharing into an org the caller is not a member of", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Caller owns the skill (passes the route gate) but is a member of no org. + const actor: ActorContext = { userId: "owner-1", memberships: [], isPlatformAdmin: false, membershipsResolved: true }; + + let thrown: unknown; + try { + await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-X"] }, + actor, + ); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("not_org_member"); + expect((thrown as AppError).statusCode).toBe(403); + // Rejection happens before persistence — the write was never attempted. + expect(state.updateCalled).toBe(false); + }); + + it("allows sharing into an org the caller belongs to", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + const actor: ActorContext = { + userId: "owner-1", + memberships: [{ userId: "org-A", role: "member", displayName: "" }], + isPlatformAdmin: false, + membershipsResolved: true, + }; + + const result = await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-A"] }, + actor, + ); + expect(state.updateCalled).toBe(true); + expect(result.sharedWithOrgs).toContain("org-A"); + }); + + it("platform admin may share into any org", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Admin has zero memberships but the bypass lets them share into org-A. + const actor: ActorContext = { userId: "admin-1", memberships: [], isPlatformAdmin: true, membershipsResolved: true }; + + const result = await service.setSkillPermissions( + "guid-1", + "admin-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-A"] }, + actor, + ); + expect(state.updateCalled).toBe(true); + expect(result.sharedWithOrgs).toContain("org-A"); + }); + + it("a rejected share does not leak read access (AC#2)", () => { + // The rejected org id was never persisted, so the stored ACL has an + // empty `sharedWithOrgs`. A member of that org must still be denied + // read — proving the failed share leaked nothing into the read gate. + const skill = { + createdBy: "owner-1", + isPrivate: true, + sharedWithUsers: [], + sharedWithOrgs: [], + }; + const orgMember: ActorContext = { + userId: "u2", + memberships: [{ userId: "org-X", role: "member", displayName: "" }], + isPlatformAdmin: false, + membershipsResolved: true, + }; + expect(canReadSkill(skill, orgMember)).toBe(false); + }); + + /** + * #842 — unresolved org-membership read. + * + * The #815 gate above intersects `sharedWithOrgs` against the caller's + * `memberships`. When the org-membership lookup is UNRESOLVED (forwarded + * token absent or NyxID unreachable) `memberships` is empty for a + * non-membership reason — so the #815 intersection would 403 a legitimate + * member. `setSkillPermissions` instead fails closed with a retryable 503 + * `org_membership_unavailable`, but only when the caller actually shares + * into an org. A public / user-only change needs no membership data and + * succeeds even while the lookup is unresolved. A resolved-not-member share + * still gets the #815 403, naming only the offending org id(s). + */ + it("(a) unresolved lookup + sharing into an org → 503 org_membership_unavailable, no write", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Owner passes the route gate, but the org lookup never resolved — empty + // memberships here means "couldn't ask", not "member of nothing". + const actor: ActorContext = { + userId: "owner-1", + memberships: [], + isPlatformAdmin: false, + membershipsResolved: false, + }; + + let thrown: unknown; + try { + await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-X"] }, + actor, + ); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("org_membership_unavailable"); + expect((thrown as AppError).statusCode).toBe(503); + // Fails closed before persistence — nothing was written. + expect(state.updateCalled).toBe(false); + }); + + it("(b) unresolved lookup + NOT sharing into any org → succeeds", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Lookup unresolved, but the caller only flips visibility / shares with a + // user — no org data is needed, so the write proceeds. + const actor: ActorContext = { + userId: "owner-1", + memberships: [], + isPlatformAdmin: false, + membershipsResolved: false, + }; + + const result = await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: false, sharedWithUsers: ["friend-1"], sharedWithOrgs: [] }, + actor, + ); + expect(state.updateCalled).toBe(true); + expect(result.isPrivate).toBe(false); + expect(result.sharedWithOrgs).toEqual([]); + }); + + it("(c) resolved-not-member + sharing into an org → 403 not_org_member naming the org", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Lookup resolved authoritatively: caller belongs to no org. Sharing into + // org-X is a genuine non-membership → the #815 403, not a 503. + const actor: ActorContext = { + userId: "owner-1", + memberships: [], + isPlatformAdmin: false, + membershipsResolved: true, + }; + + let thrown: unknown; + try { + await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-X"] }, + actor, + ); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("not_org_member"); + expect((thrown as AppError).statusCode).toBe(403); + expect((thrown as AppError).message).toContain("org-X"); + expect(state.updateCalled).toBe(false); + }); + + it("(d) resolved partial membership → atomic 403 naming ONLY the non-member org", async () => { + const { service, state } = makePermissionsService( + makeSkillDoc({ guid: "guid-1", createdBy: "owner-1", isPrivate: true }), + ); + // Member of org-A only; tries to share into [org-A, org-B]. The whole + // write is rejected atomically (org-A is NOT persisted) and the message + // names only the offending org-B, not org-A. + const actor: ActorContext = { + userId: "owner-1", + memberships: [{ userId: "org-A", role: "member", displayName: "" }], + isPlatformAdmin: false, + membershipsResolved: true, + }; + + let thrown: unknown; + try { + await service.setSkillPermissions( + "guid-1", + "owner-1", + { isPrivate: true, sharedWithUsers: [], sharedWithOrgs: ["org-A", "org-B"] }, + actor, + ); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(AppError); + expect((thrown as AppError).code).toBe("not_org_member"); + expect((thrown as AppError).statusCode).toBe(403); + const msg = (thrown as AppError).message; + expect(msg).toContain("org-B"); + expect(msg).not.toContain("org-A"); + // Atomic — no partial persistence of the member org. + expect(state.updateCalled).toBe(false); + }); +}); diff --git a/ornn-api/src/domains/skills/crud/service.ts b/ornn-api/src/domains/skills/crud/service.ts index 0cd1bee5..92dbd803 100644 --- a/ornn-api/src/domains/skills/crud/service.ts +++ b/ornn-api/src/domains/skills/crud/service.ts @@ -14,6 +14,7 @@ import { AppError } from "../../../shared/types/index"; import { fetchSkillFromGitHub, parseGithubUrl, type GitHubPullInput } from "./utils/githubPull"; import { computeVersionDiff, type VersionDiffResult } from "./utils/versionDiff"; import { isReservedVerb } from "../../../shared/reservedVerbs"; +import { canReadSkill, isMemberOfOrg, type ActorContext } from "./authorize"; /** * Convert the stored hex `skillHash` into npm-style Subresource Integrity @@ -29,7 +30,11 @@ function hexToIntegrity(hex: string): string { } return `sha256-${Buffer.from(bytes).toString("base64")}`; } -import { validateSkillFrontmatter } from "../../../shared/schemas/skillFrontmatter"; +import { + validateSkillFrontmatter, + SKILL_NAME_REGEX, + SKILL_NAME_MAX, +} from "../../../shared/schemas/skillFrontmatter"; import { resolveZipRoot } from "../../../shared/utils/zip"; import { parseVersion, isGreater } from "./version"; import { diffSkillInterface, type InterfaceChange } from "./interfaceDiff"; @@ -459,6 +464,13 @@ export class SkillService { * * Ownership (`createdBy`) is left untouched — permissions don't * change who wrote the skill, they just widen who can read it. + * + * CWE-862 (#815): in addition to the route write gate, this method now + * enforces that the caller may only share a skill into orgs they are a + * member of — every `sharedWithOrgs` id is intersected against the + * caller's memberships (platform admins exempt). Defense-in-depth: the + * check lives here so any future caller of the service is covered, not + * just the current route. */ async setSkillPermissions( guid: string, @@ -468,6 +480,7 @@ export class SkillService { sharedWithUsers: string[]; sharedWithOrgs: string[]; }, + actor: ActorContext, ): Promise { const existing = await this.skillRepo.findByGuid(guid); if (!existing) { @@ -495,6 +508,37 @@ export class SkillService { new Set(permissions.sharedWithOrgs.filter((id) => !!id)), ); + // CWE-862 (#815): an owner may only share into orgs they belong to. + // isMemberOfOrg is membership-only (does not consider platform admin), so + // gate the whole check behind the admin bypass. + if (!actor.isPlatformAdmin) { + // #842: an unresolved org-membership lookup (forwarded token absent or + // NyxID unreachable) leaves `memberships` empty for a non-membership + // reason. Validating a share into orgs against that empty list would + // wrongly 403 a legitimate member, so fail closed with a retryable 503 + // instead — but only when the caller actually asked to share into an + // org. A public / user-only change (empty sharedWithOrgs) needs no + // membership data and proceeds even while the lookup is unresolved. + if (sharedWithOrgs.length > 0 && !actor.membershipsResolved) { + logger.warn( + { guid, userId }, + "Org membership unresolved; cannot validate share into orgs (#842)", + ); + throw AppError.serviceUnavailable( + "org_membership_unavailable", + "Could not verify your organization memberships right now. Retry shortly.", + ); + } + const nonMember = sharedWithOrgs.filter((orgId) => !isMemberOfOrg(actor, orgId)); + if (nonMember.length > 0) { + logger.warn({ guid, userId, nonMember }, "Rejected skill share into non-member org(s) (#815)"); + throw AppError.forbidden( + "not_org_member", + `Cannot share skill into org(s) you are not a member of: ${nonMember.join(", ")}`, + ); + } + } + const updated = await this.skillRepo.update(guid, { isPrivate: permissions.isPrivate, sharedWithUsers, @@ -1140,9 +1184,16 @@ export class SkillService { * * When `version` is omitted the response is the latest package — same * as before. + * + * #806 — object-level authorization (BOLA / OWASP API1). `actor` is a + * REQUIRED arg: the loaded skill doc is run through `canReadSkill` + * before any storage download, so a private skill the actor cannot + * read surfaces as `skill_not_found` instead of leaking its package + * (incl. embedded secrets). Trusted server jobs pass `SYSTEM_ACTOR`. */ async getSkillJson( idOrName: string, + actor: ActorContext, version?: string, ): Promise<{ name: string; @@ -1160,6 +1211,17 @@ export class SkillService { throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); } + // 1.5 Object-level authorization (#806, BOLA / OWASP API1). The + // package-contents path must not be more permissive than the + // metadata path: a private skill the actor cannot read is + // denied here, before any storage download, with the same + // `skill_not_found` shape so existence isn't leaked. Never log + // file contents or secrets — only ids. + if (!canReadSkill(skill, actor)) { + logger.info({ idOrName, actorUserId: actor.userId }, "getSkillJson visibility denied"); + throw AppError.notFound("skill_not_found", `Skill '${idOrName}' not found`); + } + // 1a. Resolve the requested version (literal or dist-tag, #463). // When unset OR empty-string, fall through to the skill doc's // latest storage. `resolveDistTag` returns `undefined` for @@ -1517,6 +1579,19 @@ export class SkillService { "name: required (cannot be derived under skipValidation either)", ); } + // #807 (CWE-22): the strict path rejects non-kebab-case names via the + // Zod schema; the lenient path bypassed it, so a crafted name (`../`, + // `/etc/passwd`, `..`) flowed straight into the public-mirror blob + // paths and could escape the skill's own `/` subtree. Enforce + // the SAME kebab-case rule here so `skipValidation` can never widen + // the name surface beyond the strict path. + if (name.length > SKILL_NAME_MAX || !SKILL_NAME_REGEX.test(name)) { + logger.warn({ name }, "skipValidation: rejecting non-kebab-case skill name"); + throw AppError.badRequest( + "frontmatter_validation_failed", + `name: must be kebab-case (^[a-z0-9][a-z0-9-]*$, <= ${SKILL_NAME_MAX} chars)`, + ); + } const description = typeof raw.description === "string" ? raw.description : ""; const version = @@ -1680,10 +1755,12 @@ export class SkillService { const allPaths = Object.keys(zip.files); const { rootFolderName, rootEntries, getFile } = resolveZipRoot(zip, allPaths); - const KEBAB_RE = /^[a-z0-9][a-z0-9-]*$/; + // #807: reuse the canonical kebab-case rule (was a duplicate local + // regex) so folder-name validation can never drift from the name rule + // enforced on the create/import path. const ALLOWED_ROOT = new Set(["SKILL.md", "scripts", "references", "assets"]); - if (rootFolderName && !KEBAB_RE.test(rootFolderName)) { + if (rootFolderName && !SKILL_NAME_REGEX.test(rootFolderName)) { violations.push({ rule: "folder-name-kebab-case", message: `Package folder name "${rootFolderName}" must be kebab-case.`, diff --git a/ornn-api/src/domains/skills/crud/utils/githubPull.test.ts b/ornn-api/src/domains/skills/crud/utils/githubPull.test.ts index 84b20100..c45d9a33 100644 --- a/ornn-api/src/domains/skills/crud/utils/githubPull.test.ts +++ b/ornn-api/src/domains/skills/crud/utils/githubPull.test.ts @@ -92,6 +92,18 @@ describe("normalizeRepoIdentifier", () => { expect(() => normalizeRepoIdentifier("acme/s kill")).toThrow(); expect(() => normalizeRepoIdentifier("acme/skill#branch")).toThrow(); }); + test("rejects path-traversal segments (#818)", () => { + expect(() => normalizeRepoIdentifier("owner/..")).toThrow( + /Invalid GitHub repo/, + ); + expect(() => normalizeRepoIdentifier("../repo")).toThrow( + /Invalid GitHub repo/, + ); + }); + test("accepts dotted / dashed repo names", () => { + expect(normalizeRepoIdentifier("owner/repo.name")).toBe("owner/repo.name"); + expect(normalizeRepoIdentifier("owner/repo-1")).toBe("owner/repo-1"); + }); }); describe("normalizePath", () => { diff --git a/ornn-api/src/domains/skills/crud/utils/githubPull.ts b/ornn-api/src/domains/skills/crud/utils/githubPull.ts index 9a222ed7..f3460596 100644 --- a/ornn-api/src/domains/skills/crud/utils/githubPull.ts +++ b/ornn-api/src/domains/skills/crud/utils/githubPull.ts @@ -16,6 +16,7 @@ import JSZip from "jszip"; import { createLogger } from "../../../../shared/logger"; +import { hasUnsafeSegment } from "../../../../shared/githubNaming"; const logger = createLogger("githubSkillPull"); export interface GitHubPullInput { @@ -63,7 +64,10 @@ const DEFAULT_MAX_BYTES = 10 * 1024 * 1024; /** Validates `owner/name` and strips any trailing whitespace. */ export function normalizeRepoIdentifier(repo: string): string { const trimmed = repo.trim(); - if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(trimmed)) { + if ( + !/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(trimmed) || + hasUnsafeSegment(trimmed) + ) { throw new Error( `Invalid GitHub repo identifier '${repo}'. Expected 'owner/name'.`, ); diff --git a/ornn-api/src/domains/skills/generation/routes.ts b/ornn-api/src/domains/skills/generation/routes.ts index 3e80640a..bfe688df 100644 --- a/ornn-api/src/domains/skills/generation/routes.ts +++ b/ornn-api/src/domains/skills/generation/routes.ts @@ -69,23 +69,30 @@ async function resolveKeepAliveMs( } /** - * Run quota check + model resolution for a skill-gen request. Returns + * Run model resolution + quota reserve for a skill-gen request. Returns * the resolved model id; throws the appropriate AppError when either - * gate fails (quota → 429, models → 503/4xx). + * gate fails (models → 503/4xx, quota → 429). + * + * Order is load-bearing (#808): model resolution runs FIRST so a + * resolution failure can't strand a reserved quota slot. `resolveModel` + * is a pure catalog read (no LLM), so reserving last still keeps the + * "429 before any LLM cost" guarantee. Once `checkAllowed` reserves, + * every caller threads the result straight into `streamGenerationEvents`, + * whose `finally` always reconciles the reservation (commit on success, + * release on system_error/abort). */ async function preflight( c: Context<{ Variables: AuthVariables }>, quotaService: QuotaService, llmProvidersService: LlmProvidersService, requestedModelId: string | undefined, -): Promise<{ modelId: string; userId: string; permissions: readonly string[] | undefined }> { +): Promise<{ + modelId: string; + userId: string; + permissions: readonly string[] | undefined; + reservedAt: Date; +}> { const authCtx = getAuth(c); - const decision = await quotaService.checkAllowed({ - userId: authCtx.userId, - permissions: authCtx.permissions, - surface: "skillGen", - }); - if (!decision.allowed) throwQuotaError(decision); const resolution = await llmProvidersService.resolveModel({ surface: "skillGen", @@ -93,7 +100,25 @@ async function preflight( ...(requestedModelId !== undefined ? { requested: requestedModelId } : {}), }); if (resolution.kind !== "ok") throwModelResolutionError(resolution); - return { modelId: resolution.modelId, userId: authCtx.userId, permissions: authCtx.permissions }; + + // Capture the reservation instant so the charge lands in the SAME + // month bucket the slot was reserved against (#827) — see the + // playground route for the boundary-straddle rationale. + const reservedAt = new Date(); + const decision = await quotaService.checkAllowed({ + userId: authCtx.userId, + permissions: authCtx.permissions, + surface: "skillGen", + now: reservedAt, + }); + if (!decision.allowed) throwQuotaError(decision); + + return { + modelId: resolution.modelId, + userId: authCtx.userId, + permissions: authCtx.permissions, + reservedAt, + }; } /** @@ -114,6 +139,12 @@ async function streamGenerationEvents( permissions: readonly string[] | undefined; /** Resolved model id used for the LLM call — flows into `usedByModel`. */ modelId: string; + /** + * Reservation instant captured at `preflight` time (#827). Threaded + * into `chargeOnCompletion` as `now` so the commit/release reconciles + * against the month bucket the slot was reserved in, not wall-clock. + */ + reservedAt: Date; }, ) { c.header("Cache-Control", "no-cache"); @@ -148,6 +179,8 @@ async function streamGenerationEvents( surface: "skillGen", outcome, modelId: chargeAfter.modelId, + // Reconcile against the reserved month bucket (#827). + now: chargeAfter.reservedAt, }) .catch((err) => { logger.warn( @@ -301,7 +334,7 @@ export function createGenerationRoutes(config: GenerationRoutesConfig): Hono<{ V pf.modelId, ), keepAliveMs, - { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId }, + { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId, reservedAt: pf.reservedAt }, ); } @@ -334,7 +367,7 @@ export function createGenerationRoutes(config: GenerationRoutesConfig): Hono<{ V c, generationService.generateStream(query, signal, pf.modelId), keepAliveMs, - { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId }, + { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId, reservedAt: pf.reservedAt }, ); }, ); @@ -451,7 +484,7 @@ export function createGenerationRoutes(config: GenerationRoutesConfig): Hono<{ V pf.modelId, ), keepAliveMs, - { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId }, + { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId, reservedAt: pf.reservedAt }, ); }, ); @@ -506,7 +539,7 @@ export function createGenerationRoutes(config: GenerationRoutesConfig): Hono<{ V pf.modelId, ), keepAliveMs, - { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId }, + { quotaService, userId: pf.userId, permissions: pf.permissions, modelId: pf.modelId, reservedAt: pf.reservedAt }, ); }, ); diff --git a/ornn-api/src/domains/skills/mirror/mirrorService.test.ts b/ornn-api/src/domains/skills/mirror/mirrorService.test.ts index 43fe46a9..2f321980 100644 --- a/ornn-api/src/domains/skills/mirror/mirrorService.test.ts +++ b/ornn-api/src/domains/skills/mirror/mirrorService.test.ts @@ -22,6 +22,7 @@ import { MirrorService, type MirrorSettingsReader } from "./mirrorService"; import type { GitHubMirrorClient, TreeEntry } from "./githubMirrorClient"; import type { SkillRepository } from "../crud/repository"; import type { SkillService } from "../crud/service"; +import type { ActorContext } from "../crud/authorize"; import type { SkillDocument } from "../../../shared/types/index"; import type { MirrorSection } from "../../settings/sections/mirror"; @@ -156,9 +157,11 @@ function makeFakeSkillService( filesByGuid: Record>, ): SkillService { return { - getSkillJson: mock(async (idOrName: string) => ({ + // Signature mirrors the #806 service change: (idOrName, actor, version?). + getSkillJson: mock(async (idOrName: string, _actor: ActorContext, _version?: string) => ({ name: "demo-skill", description: "A test skill.", + version: "1.0", metadata: { category: "plain" }, files: filesByGuid[idOrName] ?? {}, })), @@ -337,3 +340,63 @@ describe("MirrorService idempotency", () => { expect(result.unchanged).toBeGreaterThanOrEqual(1); }); }); + +describe("MirrorService path-traversal guard (#807, CWE-22)", () => { + it("removeSkill throws on a traversal name before touching the mirror", async () => { + const { github, calls } = makeFakeGithub({ + currentTree: [ + { path: "../evil/SKILL.md", mode: "100644", type: "blob", sha: "x" }, + ], + }); + const svc = new MirrorService({ + githubClientForTest: github, + skillRepo: makeFakeRepo([]), + skillService: makeFakeSkillService({}), + ornnPublicOrigin: "https://example", + settingsService: makeFakeSettings(), + }); + // `removeSkill(name)` reaches `commitSkillFolderChange` directly with + // the raw name — the guard at the top must reject it. + await expect(svc.removeSkill("../evil")).rejects.toThrow(/unsafe mirror skill folder name/i); + // Nothing was written to the mirror. + expect(calls.trees.length).toBe(0); + expect(calls.commits.length).toBe(0); + expect(calls.tags.length).toBe(0); + }); + + it("reconcileAll skips a malformed-name skill but still mirrors the good one", async () => { + const goodSkill = makeSkill({ guid: "g-good", name: "good-skill", isPrivate: false }); + // Malformed name that would escape its subtree if interpolated. + const badSkill = makeSkill({ guid: "g-bad", name: "../escape", isPrivate: false }); + const { github, calls } = makeFakeGithub(); + const svc = new MirrorService({ + githubClientForTest: github, + skillRepo: makeFakeRepo([goodSkill, badSkill]), + skillService: makeFakeSkillService({ + "g-good": { "SKILL.md": "# good" }, + "g-bad": { "SKILL.md": "# bad" }, + }), + ornnPublicOrigin: "https://example", + settingsService: makeFakeSettings(), + }); + // The whole sweep must NOT abort because of one poisoned row. + const result = await svc.reconcileAll(); + + // Every path written stays inside a safe `/` prefix — nothing + // escapes via `../`, and the bad skill's name appears in no path. + for (const tree of calls.trees) { + for (const entry of tree.entries) { + expect(entry.path).not.toContain(".."); + expect(entry.path.startsWith("../escape")).toBe(false); + } + } + // The good skill WAS mirrored (its SKILL.md got uploaded). + const goodMirrored = calls.blobs.some((b) => b.content === "# good"); + expect(goodMirrored).toBe(true); + // The bad skill's content never reached the mirror. + const badMirrored = calls.blobs.some((b) => b.content === "# bad"); + expect(badMirrored).toBe(false); + // The sweep produced a commit (the good skill is a real add). + expect(result.added).toBeGreaterThanOrEqual(1); + }); +}); diff --git a/ornn-api/src/domains/skills/mirror/mirrorService.ts b/ornn-api/src/domains/skills/mirror/mirrorService.ts index f723e6e0..6f992429 100644 --- a/ornn-api/src/domains/skills/mirror/mirrorService.ts +++ b/ornn-api/src/domains/skills/mirror/mirrorService.ts @@ -28,8 +28,10 @@ import { GitHubAppAuth } from "./githubAppAuth"; import { GitHubMirrorClient, type TreeEntry } from "./githubMirrorClient"; import type { SkillRepository } from "../crud/repository"; import type { SkillService } from "../crud/service"; +import { SYSTEM_ACTOR } from "../crud/authorize"; import type { SkillDocument } from "../../../shared/types/index"; import type { MirrorSection } from "../../settings/sections/mirror"; +import { SKILL_NAME_REGEX, SKILL_NAME_MAX } from "../../../shared/schemas/skillFrontmatter"; const logger = createLogger("mirrorService"); @@ -259,6 +261,16 @@ export class MirrorService { const desired = new Map(); // path → content const skillByName = new Map(); for (const skill of eligible) { + // #807 (CWE-22): skip — do NOT abort — a row whose name would + // escape its `/` subtree. One poisoned row must not stop the + // whole sweep from mirroring every other (safe) skill. + if (!SKILL_NAME_REGEX.test(skill.name) || skill.name.length > SKILL_NAME_MAX) { + logger.error( + { guid: skill.guid, name: skill.name }, + "reconcileAll: skipping skill with unsafe folder name", + ); + continue; + } skillByName.set(skill.name, { guid: skill.guid, version: skill.latestVersion }); const folder = await this.buildSkillFolder(skill); for (const [relPath, content] of folder) { @@ -353,8 +365,10 @@ export class MirrorService { */ private async buildSkillFolder(skill: SkillDocument): Promise> { // Reuse the existing `/skills/:id/json` extraction so we don't - // duplicate ZIP logic. Pulls latest version. - const json = await this.deps.skillService.getSkillJson(skill.guid); + // duplicate ZIP logic. Pulls latest version. Mirror is a trusted + // server job, so it reads with SYSTEM_ACTOR (#806) — the eligibility + // filter already guarantees only fully-public skills reach here. + const json = await this.deps.skillService.getSkillJson(skill.guid, SYSTEM_ACTOR); const out = new Map(); for (const [path, content] of Object.entries(json.files)) { // Drop README.md from the package if any, since we're writing @@ -470,12 +484,31 @@ export class MirrorService { * reconcile (first-push bootstrap). Callers use the return value to * decide whether to stamp `mirrorSync` on the DB. */ + /** + * Defense-in-depth (#807, CWE-22): the create/import path already + * rejects non-kebab-case names, but a row that predates that fix (or + * one written by some future path) could still carry a name like + * `../evil` or `a/b`. Every site that interpolates `skill.name` into a + * mirror blob path runs this guard first so a poisoned name can never + * escape its own `/` subtree in the public mirror repo. + */ + private assertSafeSkillFolder(name: string): void { + if (!SKILL_NAME_REGEX.test(name) || name.length > SKILL_NAME_MAX) { + logger.error({ name }, "mirror: refusing unsafe skill folder name"); + throw new Error(`Unsafe mirror skill folder name: ${name}`); + } + } + private async commitSkillFolderChange( client: GitHubMirrorClient, skillName: string, desired: Map | null, op: "publish" | "remove", ): Promise<{ sha: string; committedAt: Date } | null> { + // Guard before any path interpolation. Throwing is correct here: the + // call is scoped to a single skill (publishSkill / removeSkill), so a + // bad name fails just that operation, never a batch. + this.assertSafeSkillFolder(skillName); const headCommit = await client.getDefaultBranchHead(); if (!headCommit) { // First-ever push — bootstrap requires an initial commit. Defer diff --git a/ornn-api/src/domains/skills/mirror/routes.ts b/ornn-api/src/domains/skills/mirror/routes.ts index 47122727..fcb1aaaf 100644 --- a/ornn-api/src/domains/skills/mirror/routes.ts +++ b/ornn-api/src/domains/skills/mirror/routes.ts @@ -40,6 +40,7 @@ import { } from "../../../middleware/nyxidAuth"; import { AppError } from "../../../shared/types/index"; import { isMidMaskSentinel, midMaskSecret } from "../../../infra/crypto"; +import { OWNER_RE, REPO_RE } from "../../../shared/githubNaming"; import type { MirrorService, ReconcileResult } from "./mirrorService"; import type { MirrorScheduler, ScheduledRunStatus } from "./scheduler"; import type { SettingsService, SettingsActor } from "../../settings/types"; @@ -52,13 +53,12 @@ const logger = createLogger("mirrorRoutes"); /** * GitHub naming validation. * - * owner: alphanumeric + dashes; can't start or end with a dash; 1–39 chars. - * repo: alphanumeric + dot/dash/underscore; 1–100 chars. + * owner / repo: see `shared/githubNaming` (OWNER_RE / REPO_RE) — the + * single source of truth shared with the mirror settings section + * and the repo-pull path. * branch: any non-empty string up to 250 chars (git's actual limit is * looser but we want a sane upper bound and disallow control chars). */ -const OWNER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/; -const REPO_RE = /^[A-Za-z0-9._-]{1,100}$/; // eslint-disable-next-line no-control-regex -- intentional: rejects branch names containing C0 control chars or DEL. const BRANCH_RE = /^[^\x00-\x1f\x7f]{1,250}$/; const APP_ID_RE = /^[0-9]{1,15}$/; diff --git a/ornn-api/src/domains/skills/search/routes.test.ts b/ornn-api/src/domains/skills/search/routes.test.ts new file mode 100644 index 00000000..0a68aea7 --- /dev/null +++ b/ornn-api/src/domains/skills/search/routes.test.ts @@ -0,0 +1,99 @@ +/** + * Tests for the skill-search route's pagination bounds (CWE-770, #810). + * + * The `?page=` query param feeds an offset-based `.skip()` underneath. + * Without an upper bound a caller could request `?page=999999999` and + * drive a multi-second collection scan. `searchQuerySchema.page` now + * carries `.max(MAX_PAGE)`, so the request is rejected at the + * `validateQuery` layer with a 400 `invalid_query` BEFORE the handler + * (and the service) ever runs. + * + * The stub services here are intentionally inert: the 400 case never + * reaches them, and the positive-control 200 case only exercises + * `searchService.search` (which returns an empty result set). + * + * @module domains/skills/search/routes.test + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { Hono } from "hono"; +import { createSearchRoutes } from "./routes"; +import type { SearchService } from "./service"; +import type { SkillRepository } from "../crud/repository"; +import { + AppError, + buildProblemJsonBody, + type SkillSearchResponse, +} from "../../../shared/types/index"; +import { __resetRateLimitForTests } from "../../../middleware/rateLimit"; + +/** + * Mount the real search routes with stub deps under a Hono app whose + * onError mirrors the global handler (AppError → problem+json). Only + * `searchService.search` is ever called (positive-control path); the + * 400 path short-circuits in `validateQuery`. + */ +function makeApp(searchImpl?: SearchService["search"]) { + const searchService = { + search: + searchImpl ?? + (async (): Promise => { + throw new Error("search() should not be called when validation rejects"); + }), + } as unknown as SearchService; + + // skillRepo is never touched on the /skill-search path — the route + // only calls searchService.search. A bare cast keeps the wiring light. + const skillRepo = {} as unknown as SkillRepository; + + const app = new Hono(); + app.route("/", createSearchRoutes({ searchService, skillRepo })); + 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 /skill-search — page bound (CWE-770, #810)", () => { + beforeEach(() => __resetRateLimitForTests()); + afterEach(() => __resetRateLimitForTests()); + + test("rejects ?page=999999999 with 400 invalid_query", async () => { + const app = makeApp(); + const res = await app.request("/skill-search?page=999999999"); + expect(res.status).toBe(400); + const body = (await res.json()) as { code: string }; + expect(body.code).toBe("invalid_query"); + }); + + test("accepts ?page=10000 (the ceiling) — does not 400 at validation", async () => { + const emptyResult: SkillSearchResponse = { + searchMode: "keyword", + searchScope: "public", + total: 0, + totalPages: 0, + page: 10_000, + pageSize: 9, + items: [], + }; + const app = makeApp(async () => emptyResult); + const res = await app.request("/skill-search?page=10000"); + expect(res.status).toBe(200); + expect(res.status).not.toBe(400); + const body = (await res.json()) as { data: { items: unknown[] }; error: unknown }; + expect(body.error).toBeNull(); + expect(body.data.items).toEqual([]); + }); +}); diff --git a/ornn-api/src/domains/skills/search/routes.ts b/ornn-api/src/domains/skills/search/routes.ts index aaea9591..6bc813bb 100644 --- a/ornn-api/src/domains/skills/search/routes.ts +++ b/ornn-api/src/domains/skills/search/routes.ts @@ -19,7 +19,7 @@ import { import { validateQuery, getValidatedQuery } from "../../../middleware/validate"; import { AppError } from "../../../shared/types/index"; import { createLogger } from "../../../shared/logger"; -import { decodeCursor, buildNextCursor } from "../../../shared/cursor"; +import { decodeCursor, buildNextCursor, MAX_PAGE } from "../../../shared/cursor"; import { rateLimit } from "../../../middleware/rateLimit"; const logger = createLogger("skillSearchRoutes"); @@ -32,7 +32,7 @@ const searchQuerySchema = z.object({ query: z.string().max(2000).optional(), mode: z.enum(["keyword", "semantic"]).optional().default("keyword"), scope: z.enum(["public", "private", "mixed", "shared-with-me", "mine"]).optional().default("private"), - page: z.coerce.number().int().min(1).optional().default(1), + page: z.coerce.number().int().min(1).max(MAX_PAGE).optional().default(1), pageSize: z.coerce.number().int().min(1).max(100).optional().default(9), /** * Cursor-based pagination per CONVENTIONS.md §4.3 (#457). Opaque diff --git a/ornn-api/src/index.ts b/ornn-api/src/index.ts index af73c6bd..e2909c34 100644 --- a/ornn-api/src/index.ts +++ b/ornn-api/src/index.ts @@ -5,6 +5,7 @@ import { loadConfig, ConfigError, type SkillConfig } from "./infra/config"; import { bootstrap } from "./bootstrap"; +import { REDACT_PATHS } from "./shared/logger"; import pino from "pino"; let config: SkillConfig; @@ -23,6 +24,7 @@ try { const logger = pino({ level: config.logLevel, ...(config.logPretty ? { transport: { target: "pino-pretty" } } : {}), + redact: { paths: REDACT_PATHS }, }).child({ service: "ornn-api" }); logger.info({ port: config.port }, "ornn-api starting"); diff --git a/ornn-api/src/infra/config.test.ts b/ornn-api/src/infra/config.test.ts new file mode 100644 index 00000000..71ae87c8 --- /dev/null +++ b/ornn-api/src/infra/config.test.ts @@ -0,0 +1,107 @@ +/** + * loadConfig() env-validation tests (#821). + * + * Locks the ENCRYPTION_KEY contract the interface + schema JSDoc now + * claim: mandatory, ≥32 chars, NO dev fallback, fail-fast with a + * structured ConfigError. Asserts on ConfigError identity + the + * ZodIssue path (not message text, which is brittle). + * + * @module infra/config.test + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { ConfigError, loadConfig } from "./config"; + +const VALID_KEY = "test-encryption-key-32-chars-min-12345"; + +// Per-key save/restore (the #816 class). Never rebind process.env — that +// leaks set-but-not-snapshotted state into sibling test files. Capture the +// ORIGINAL value of exactly the keys this file touches (undefined vs string) +// once, mutate via direct assignment per case, then restore each key +// individually in afterEach. Same idiom as safeFetch.test.ts. +const ORIGINAL_ENCRYPTION_KEY = process.env.ENCRYPTION_KEY; +const ORIGINAL_MONGODB_URI = process.env.MONGODB_URI; + +beforeEach(() => { + // Seed every required var EXCEPT ENCRYPTION_KEY so failures attribute + // to the key under test, not to an unrelated missing var. + process.env.MONGODB_URI = "mongodb://localhost:27017"; +}); + +afterEach(() => { + if (ORIGINAL_ENCRYPTION_KEY === undefined) delete process.env.ENCRYPTION_KEY; + else process.env.ENCRYPTION_KEY = ORIGINAL_ENCRYPTION_KEY; + + if (ORIGINAL_MONGODB_URI === undefined) delete process.env.MONGODB_URI; + else process.env.MONGODB_URI = ORIGINAL_MONGODB_URI; +}); + +describe("loadConfig — ENCRYPTION_KEY", () => { + test("throws ConfigError when ENCRYPTION_KEY is unset", () => { + delete process.env.ENCRYPTION_KEY; + try { + loadConfig(); + throw new Error("expected loadConfig to throw"); + } catch (err) { + expect(err).toBeInstanceOf(ConfigError); + expect( + (err as ConfigError).issues.some((i) => i.path.includes("ENCRYPTION_KEY")), + ).toBe(true); + } + }); + + test("throws ConfigError when ENCRYPTION_KEY is shorter than 32 chars", () => { + process.env.ENCRYPTION_KEY = "too-short"; + try { + loadConfig(); + throw new Error("expected loadConfig to throw"); + } catch (err) { + expect(err).toBeInstanceOf(ConfigError); + expect( + (err as ConfigError).issues.some((i) => i.path.includes("ENCRYPTION_KEY")), + ).toBe(true); + } + }); + + test("throws ConfigError when ENCRYPTION_KEY is exactly 31 chars (boundary)", () => { + const KEY_31 = "a".repeat(31); + expect(KEY_31.length).toBe(31); + process.env.ENCRYPTION_KEY = KEY_31; + try { + loadConfig(); + throw new Error("expected loadConfig to throw"); + } catch (err) { + expect(err).toBeInstanceOf(ConfigError); + expect( + (err as ConfigError).issues.some((i) => i.path.includes("ENCRYPTION_KEY")), + ).toBe(true); + } + }); + + test("throws ConfigError when ENCRYPTION_KEY is the empty string (envsubst footgun)", () => { + process.env.ENCRYPTION_KEY = ""; + try { + loadConfig(); + throw new Error("expected loadConfig to throw"); + } catch (err) { + expect(err).toBeInstanceOf(ConfigError); + expect( + (err as ConfigError).issues.some((i) => i.path.includes("ENCRYPTION_KEY")), + ).toBe(true); + } + }); + + test("loads config when ENCRYPTION_KEY is exactly 32 chars (boundary)", () => { + const KEY_32 = "a".repeat(32); + expect(KEY_32.length).toBe(32); + process.env.ENCRYPTION_KEY = KEY_32; + const config = loadConfig(); + expect(config.encryptionKey).toBe(KEY_32); + }); + + test("loads config when ENCRYPTION_KEY is ≥32 chars", () => { + process.env.ENCRYPTION_KEY = VALID_KEY; + const config = loadConfig(); + expect(config.encryptionKey).toBe(VALID_KEY); + }); +}); diff --git a/ornn-api/src/infra/config.ts b/ornn-api/src/infra/config.ts index 6d2415df..6a5ad423 100644 --- a/ornn-api/src/infra/config.ts +++ b/ornn-api/src/infra/config.ts @@ -74,12 +74,7 @@ export interface SkillConfig { */ readonly ornnPublicOrigin: string; - /** - * Master passphrase for AES-256-GCM at-rest secret encryption (LLM - * provider apiKey, future operator-pasted secrets). Falls back to a - * dev sentinel when `ENCRYPTION_KEY` is unset — production deployments - * MUST override. - */ + /** Master passphrase for AES-256-GCM at-rest secret encryption. Required, ≥32 chars; boot fails with ConfigError if missing/short. See ENCRYPTION_KEY in envSchema for full rationale. */ readonly encryptionKey: string; } diff --git a/ornn-api/src/infra/safeFetch.test.ts b/ornn-api/src/infra/safeFetch.test.ts new file mode 100644 index 00000000..c6b909e8 --- /dev/null +++ b/ornn-api/src/infra/safeFetch.test.ts @@ -0,0 +1,215 @@ +/** + * Tests for #811 (SSRF-preflight) + #832 (redirect-hop hardening) for + * the shared `safeFetch` primitive. + * + * `safeFetch` re-resolves the outbound host at fetch time via + * `assertPublicResolvedAddress` (which calls `dns.lookup`) and refuses + * private/loopback/link-local resolutions BEFORE the real `fetch` — + * the DNS-rebind defense. We stub `node:dns/promises` so a public + * hostname resolves to the cloud-metadata address `169.254.169.254` + * and assert the wrapper rejects without ever issuing the network call. + * + * #832 forces `redirect: "manual"` and follows 3xx ourselves in a + * bounded loop, re-validating EACH hop's host. The redirect tests use a + * scripted per-URL fetch queue (keyed by URL, not call order) so a host + * can return a 302 once and a 200 next, without order-fragility. + * + * `url.ts` binds `dns` at module load (`import * as dns from + * "node:dns/promises"`), so the `mock.module` MUST be installed before + * the modules under test are imported — hence the top-level mock + + * dynamic `import()` (same idiom as mirror/scheduler.test.ts). + */ + +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test"; + +// Install the dns mock BEFORE importing the module under test so +// `url.ts` binds the stub at load time. `mock.module` is process-global +// and `url.ts` binds the dns namespace once, so the stub cannot be +// cleanly torn down per-file — we therefore make it host-aware: ONLY +// the rebind hostnames resolve to the cloud-metadata address; every +// other host resolves to a benign public IP, so the leaked stub is +// harmless to sibling tests that hit real public hostnames. +const REBIND_HOST = "evil.example.com"; +const METADATA_HOST = "169.254.169.254"; +mock.module("node:dns/promises", () => ({ + lookup: async (host: string) => + host === REBIND_HOST + ? [{ address: "169.254.169.254", family: 4 }] + : [{ address: "93.184.216.34", family: 4 }], +})); + +const { safeFetch } = await import("./safeFetch"); +const { SsrfRefusalError } = await import("./url"); + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; +const originalFetch = globalThis.fetch; +const originalAllowlist = process.env[ALLOWLIST_ENV]; + +let fetchCalls: string[]; + +/** + * Scripted per-URL response queue. Map a URL to an array of Responses; + * each call to that URL shifts the next one (the last one sticks once + * the queue is drained). Keyed by URL — no order-fragility across hosts. + */ +let scriptedResponses: Map; + +function urlOf(input: RequestInfo | URL): string { + return typeof input === "string" + ? input + : input instanceof URL + ? input.toString() + : input.url; +} + +/** A 302 redirect Response pointing at `location`. */ +function redirect302(location: string): Response { + return new Response(null, { status: 302, headers: { location } }); +} + +beforeEach(() => { + fetchCalls = []; + scriptedResponses = new Map(); + delete process.env[ALLOWLIST_ENV]; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = urlOf(input); + fetchCalls.push(url); + const queue = scriptedResponses.get(url); + if (queue && queue.length > 0) { + return queue.length > 1 ? queue.shift()! : queue[0]!; + } + return new Response("ok", { status: 200 }); + }) as typeof fetch; +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (originalAllowlist === undefined) delete process.env[ALLOWLIST_ENV]; + else process.env[ALLOWLIST_ENV] = originalAllowlist; +}); + +describe("safeFetch — DNS-rebind preflight (#811)", () => { + it("rejects with SsrfRefusalError and never calls fetch when the host resolves to a private address", async () => { + await expect(safeFetch("http://evil.example.com/x")).rejects.toBeInstanceOf( + SsrfRefusalError, + ); + expect(fetchCalls).toHaveLength(0); + }); + + it("lets the request through (fetch IS called) when the host is operator-allowlisted — DNS resolution skipped", async () => { + process.env[ALLOWLIST_ENV] = "evil.example.com"; + const res = await safeFetch("http://evil.example.com/x"); + expect(res.status).toBe(200); + expect(fetchCalls).toEqual(["http://evil.example.com/x"]); + }); + + it("lets the request through for a literal public IP (upstream already vetted the literal — no re-resolution)", async () => { + const res = await safeFetch("http://93.184.216.34/x"); + expect(res.status).toBe(200); + expect(fetchCalls).toEqual(["http://93.184.216.34/x"]); + }); + + it("forwards init structurally and sets redirect:manual (#832)", async () => { + // #832 spreads init to inject `redirect: "manual"`, so the forwarded + // object is no longer reference-identical. Assert the load-bearing + // fields survive the spread AND that manual-redirect is forced on. + let capturedInit: RequestInit | undefined; + globalThis.fetch = (async (_input: RequestInfo | URL, init?: RequestInit) => { + capturedInit = init; + return new Response("ok", { status: 200 }); + }) as typeof fetch; + const init: RequestInit = { method: "POST", headers: { "X-Test": "1" }, body: "payload" }; + await safeFetch("http://93.184.216.34/x", init); + expect(capturedInit).toMatchObject({ + method: "POST", + headers: { "X-Test": "1" }, + body: "payload", + }); + expect(capturedInit?.redirect).toBe("manual"); + }); + + it("throws on an invalid URL before resolving or fetching", async () => { + await expect(safeFetch("not a url")).rejects.toThrow(/Invalid URL/); + expect(fetchCalls).toHaveLength(0); + }); +}); + +describe("safeFetch — bounded manual redirect follow (#832)", () => { + it("refuses a first-hop 302 → metadata host; second fetch never fires", async () => { + // hop1 (public host) returns a 302 pointing at the cloud-metadata + // address. The loop re-validates the redirect target before its + // fetch — `evil.example.com` resolves to 169.254.169.254 so the + // SECOND fetch never fires. + const hop1 = "http://public-a.example.com/start"; + scriptedResponses.set(hop1, [redirect302("http://evil.example.com/")]); + + await expect(safeFetch(hop1)).rejects.toBeInstanceOf(SsrfRefusalError); + // Only the first hop's fetch fired; the metadata target was refused + // by the preflight before its network call. + expect(fetchCalls).toEqual([hop1]); + }); + + it("refuses a 302 to a literal metadata IP; second fetch never fires", async () => { + // A redirect straight to the literal 169.254.169.254 is caught by + // the per-hop URL guard (private IP literal) before its fetch. + const hop1 = "http://public-a.example.com/start"; + scriptedResponses.set(hop1, [redirect302(`http://${METADATA_HOST}/latest/meta-data/`)]); + + await expect(safeFetch(hop1)).rejects.toBeInstanceOf(SsrfRefusalError); + expect(fetchCalls).toEqual([hop1]); + }); + + it("follows a same-host PUBLIC 302 and returns the final 200", async () => { + // Criterion 2: a legitimate public redirect still succeeds. Same + // host, so credentials (if any) are preserved. + const hop1 = "http://public-a.example.com/start"; + const hop2 = "http://public-a.example.com/final"; + scriptedResponses.set(hop1, [redirect302(hop2)]); + scriptedResponses.set(hop2, [new Response("done", { status: 200 })]); + + const res = await safeFetch(hop1); + expect(res.status).toBe(200); + expect(await res.text()).toBe("done"); + expect(fetchCalls).toEqual([hop1, hop2]); + }); + + it("strips Authorization on a cross-host PUBLIC redirect", async () => { + // hop1 (hostA) carries an Authorization header and 302s to hostB — + // both public. The follow must NOT carry the bearer token to hostB. + const hop1 = "http://public-a.example.com/start"; + const hop2 = "http://public-b.example.com/final"; + scriptedResponses.set(hop1, [redirect302(hop2)]); + scriptedResponses.set(hop2, [new Response("done", { status: 200 })]); + + let hop2Init: RequestInit | undefined; + const baseFetch = globalThis.fetch; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + if (urlOf(input) === hop2) hop2Init = init; + return baseFetch(input, init); + }) as typeof fetch; + + const res = await safeFetch(hop1, { + headers: { Authorization: "Bearer secret-token", "X-Keep": "1" }, + }); + expect(res.status).toBe(200); + expect(fetchCalls).toEqual([hop1, hop2]); + + // The Authorization header must be gone on the cross-host hop; + // non-sensitive headers are preserved. + const hop2Headers = new Headers(hop2Init?.headers as HeadersInit); + expect(hop2Headers.has("authorization")).toBe(false); + expect(hop2Headers.get("x-keep")).toBe("1"); + }); + + it("throws after MAX_REDIRECTS on a self-redirect loop", async () => { + // Self-redirect loop on a public host — every hop returns a 302 to + // itself. The loop caps at MAX_REDIRECTS (5) + the initial attempt, + // so at most 6 fetch calls before the limit error. + const loopUrl = "http://public-loop.example.com/spin"; + scriptedResponses.set(loopUrl, [redirect302(loopUrl)]); + + await expect(safeFetch(loopUrl)).rejects.toThrow(/Too many redirects/); + expect(fetchCalls.length).toBeLessThanOrEqual(6); + expect(fetchCalls.every((u) => u === loopUrl)).toBe(true); + }); +}); diff --git a/ornn-api/src/infra/safeFetch.ts b/ornn-api/src/infra/safeFetch.ts new file mode 100644 index 00000000..678762fe --- /dev/null +++ b/ornn-api/src/infra/safeFetch.ts @@ -0,0 +1,172 @@ +/** + * SSRF-preflight fetch wrapper (#811, redirect-hop hardening #832). + * + * The single shared primitive that re-resolves an outbound host at + * fetch time and refuses private/loopback/link-local targets — the + * DNS-rebind defense that complements write-time `validatePublicUrl`. + * Every outbound client (chrono-storage, chrono-sandbox, NyxID, + * LLM gateway, model-list) routes through this so a public host that + * later flips its DNS to 169.254.169.254 / RFC1918 is caught before + * the bearer/SA token is sent. + * + * Redirect handling (#832): the underlying `fetch` default is + * `redirect: "follow"`, which transparently chases 3xx `Location` + * headers to whatever host the upstream names — including + * `http://169.254.169.254/`. That re-opens the exact rebind hole the + * preflight closes: we validate the FIRST hop, then `fetch` follows a + * 302 to the metadata host without any further check. We close this by + * forcing `redirect: "manual"` and following redirects ourselves in a + * bounded loop, re-running `assertPublicResolvedAddress` against EACH + * hop's host before issuing its request. A redirect to a private / + * metadata address therefore throws `SsrfRefusalError` on the hop that + * names it, exactly like a first-hop rebind. + * + * Cross-host credential stripping: when a redirect crosses to a + * different host, sensitive request headers (`authorization`, `cookie`, + * `proxy-authorization`) are dropped before following — a 302 from a + * legitimate gateway to a third-party host must not leak the caller's + * bearer token. Same-host redirects keep the headers (they're staying + * within the trust boundary the caller already authorized). + * + * Non-goals (deliberate): + * - NO RFC 7231 method downgrade on 301/302/303. The method and body + * are re-passed as-is on every hop. No current caller relies on a + * POST→GET downgrade, and re-issuing the original method is the + * safer default for our API-to-API traffic (a redirected POST that + * silently became a GET would drop the body and confuse callers). + * - NO timeout here (callers that need one pass their own + * `init.signal`); credential redaction / response parsing stay in + * the individual clients. + * + * @module infra/safeFetch + */ +import { createLogger } from "../shared/logger"; +import { assertPublicResolvedAddress, isPrivateHost, SsrfRefusalError } from "./url"; + +const logger = createLogger("safeFetch"); + +/** Cap on followed redirects before we give up (a loop or a chain). */ +const MAX_REDIRECTS = 5; + +/** + * Request headers stripped when a redirect crosses to a different host. + * Lowercase — comparison is case-insensitive. These carry the caller's + * credentials and must never follow a 3xx to an unrelated origin. + */ +const SENSITIVE_HEADERS = ["authorization", "cookie", "proxy-authorization"]; + +const ALLOWLIST_ENV = "ORNN_URL_ALLOWLIST_CIDR"; + +/** + * True if `host` is on the operator allowlist (exact hostname match). + * Mirrors the hostname fast-path in `assertPublicResolvedAddress` so a + * redirect-hop literal-IP guard honours the same operator override. + */ +function isAllowlistedHost(host: string): boolean { + const raw = process.env[ALLOWLIST_ENV] ?? ""; + const lower = host.toLowerCase(); + return raw + .split(",") + .map((s) => s.trim()) + .some((entry) => entry.length > 0 && entry === lower); +} + +/** + * Normalize `init.headers` (Headers | Record | [k,v][] | undefined) into + * a plain object, delete `keys` case-insensitively, and return a new + * `RequestInit` carrying the cleaned headers. Does not mutate the input. + */ +function stripSensitiveHeaders(init: RequestInit, keys: ReadonlyArray): RequestInit { + const lowerKeys = new Set(keys.map((k) => k.toLowerCase())); + const cleaned: Record = {}; + const source = init.headers; + if (source instanceof Headers) { + source.forEach((value, name) => { + if (!lowerKeys.has(name.toLowerCase())) cleaned[name] = value; + }); + } else if (Array.isArray(source)) { + for (const [name, value] of source) { + if (!lowerKeys.has(name.toLowerCase())) cleaned[name] = value; + } + } else if (source) { + for (const [name, value] of Object.entries(source)) { + if (!lowerKeys.has(name.toLowerCase())) cleaned[name] = value; + } + } + return { ...init, headers: cleaned }; +} + +export async function safeFetch(url: string, init?: RequestInit): Promise { + let currentUrl = url; + // Force manual redirect handling — we re-validate each hop ourselves + // instead of letting `fetch` chase 3xx to unvalidated hosts (#832). + let currentInit: RequestInit = { ...init, redirect: "manual" }; + let prevHost: string | null = null; + + for (let hop = 0; hop <= MAX_REDIRECTS; hop++) { + let parsed: URL; + try { + parsed = new URL(currentUrl); + } catch { + throw new Error(`Invalid URL: '${currentUrl}'`); + } + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + throw new Error(`Refusing non-http(s) URL: '${currentUrl}'`); + } + + logger.debug({ hop, host: parsed.hostname }, "safeFetch hop"); + + // Redirect targets (hop > 0) never passed write-time + // `validatePublicUrl`, so a `Location: http://169.254.169.254/` + // would slip past `assertPublicResolvedAddress`'s literal-IP + // fast-path (which assumes the literal was already vetted upstream). + // Guard literal private/loopback/link-local IPs explicitly on each + // redirect hop, honouring the operator allowlist. The first hop + // keeps relying on the write-time guard so literal PUBLIC IPs (an + // already-vetted upstream target) still work. + if (hop > 0 && isPrivateHost(parsed.hostname) && !isAllowlistedHost(parsed.hostname)) { + throw new SsrfRefusalError(parsed.hostname, parsed.hostname); + } + + // Re-resolve and re-check EACH hop's host — first hop AND every + // redirect target. Throws SsrfRefusalError on a private resolution; + // we intentionally do NOT catch it so the refusal propagates to the + // caller exactly as a first-hop rebind would. + await assertPublicResolvedAddress(parsed.hostname); + + // A cross-host redirect must not carry the caller's credentials to a + // host they did not authorize. `prevHost === null` is the first hop + // (no redirect yet) — leave headers untouched there. + if (prevHost !== null && parsed.hostname !== prevHost) { + currentInit = stripSensitiveHeaders(currentInit, SENSITIVE_HEADERS); + } + + const res = await fetch(currentUrl, currentInit); + + // Not a redirect (or a redirect with no Location) — this is the + // final response. + if (res.status < 300 || res.status >= 400) return res; + const loc = res.headers.get("location"); + if (!loc) return res; + + // Drain the redirect response body so the connection can be reused + // and we don't leak a dangling stream. + res.body?.cancel(); + + const nextUrl = new URL(loc, currentUrl).toString(); + const nextHost = new URL(nextUrl).hostname; + if (nextHost !== parsed.hostname && !isPrivateHost(nextHost)) { + // Cross-host follow is the only redirect class worth an info log + // (a same-host redirect is routine). Suppress the log for a target + // we're about to refuse on the next hop (private/metadata) so the + // record reads "refused", not "following". NEVER log header / + // token values — only the from/to hosts. + logger.info({ from: parsed.hostname, to: nextHost }, "safeFetch following cross-host redirect"); + } + prevHost = parsed.hostname; + currentUrl = nextUrl; + } + + logger.warn({ url, max: MAX_REDIRECTS }, "safeFetch exceeded redirect limit"); + throw new Error(`Too many redirects (>${MAX_REDIRECTS}) for '${url}'`); +} diff --git a/ornn-api/src/middleware/idempotency.test.ts b/ornn-api/src/middleware/idempotency.test.ts index ed3ed45a..2327b20f 100644 --- a/ornn-api/src/middleware/idempotency.test.ts +++ b/ornn-api/src/middleware/idempotency.test.ts @@ -100,6 +100,31 @@ describe("idempotency middleware (#459)", () => { expect(second.headers.get("Location")).toBe("/v1/skills/abc"); }); + test("SSE responses are passed through unbuffered and never persisted (#812, CWE-770)", async () => { + const { app, counter } = makeApp( + () => + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("data: one\n\n")); + controller.enqueue(new TextEncoder().encode("data: two\n\n")); + controller.close(); + }, + }), + { status: 200, headers: { "Content-Type": "text/event-stream; charset=utf-8" } }, + ), + ); + const headers = { "Idempotency-Key": "sse-key" }; + const first = await app.request("/api/v1/skills", { method: "POST", headers }); + expect(first.status).toBe(200); + expect(first.headers.get("content-type")).toBe("text/event-stream; charset=utf-8"); + expect(await first.text()).toBe("data: one\n\ndata: two\n\n"); + expect(await db.collection("idempotency_keys").countDocuments()).toBe(0); + const second = await app.request("/api/v1/skills", { method: "POST", headers }); + expect(counter.value).toBe(2); // handler re-ran, not replayed + expect(second.headers.get("Idempotency-Replay")).toBe(null); + }); + test("different users with the same key DON'T collide (per-user scoping)", async () => { const { app, counter } = makeApp((c) => new Response(JSON.stringify({ id: c.value }), { diff --git a/ornn-api/src/middleware/idempotency.ts b/ornn-api/src/middleware/idempotency.ts index 9998134d..61fef458 100644 --- a/ornn-api/src/middleware/idempotency.ts +++ b/ornn-api/src/middleware/idempotency.ts @@ -192,6 +192,15 @@ export function idempotencyMiddleware(config: IdempotencyConfig): MiddlewareHand try { const responseHeaders = captureHeaders(res); const contentType = responseHeaders["content-type"] ?? ""; + // CWE-770 (#812): never buffer/persist a streaming body. SSE responses + // are unbounded; clone().text() would tee + drain the live stream into + // memory and persist a frozen blob that replays incorrectly. Pass the + // live stream through untouched. startsWith — prod emits + // "text/event-stream; charset=utf-8" (playground/routes.ts), so === misses it. + if (contentType.startsWith("text/event-stream")) { + logger.debug({ id, path }, "Idempotency skip: streaming response not cached"); + return; + } let body: string; let encoding: "utf-8" | "base64"; if (contentType.startsWith("application/") || contentType.startsWith("text/")) { diff --git a/ornn-api/src/middleware/nyxidAuth.test.ts b/ornn-api/src/middleware/nyxidAuth.test.ts new file mode 100644 index 00000000..e1aaef42 --- /dev/null +++ b/ornn-api/src/middleware/nyxidAuth.test.ts @@ -0,0 +1,171 @@ +/** + * Getter-level tests for the org-membership lookup middleware (#842). + * + * `nyxidOrgLookupMiddleware` attaches two lazy getters that share ONE memo + * cell and ONE NyxID round-trip: + * - `getUserOrgMemberships` → fail-soft `OrgMembershipFact[]` + * - `getUserOrgMembershipResolution` → resolved/unresolved discriminant + * + * These tests pin the discriminant branches, the single-round-trip sharing, + * and the read-path fail-soft regression guard (read still `[]`-returns in + * every unresolved case). No env mutation, no import-order coupling: each + * case builds a fresh Hono app and a fresh fake `OrgMembershipSource`, so the + * suite is order-independent (the #816 lesson). + * + * @module middleware/nyxidAuth.test + */ + +import { describe, expect, test } from "bun:test"; +import { Hono } from "hono"; +import { + nyxidOrgLookupMiddleware, + readUserOrgMemberships, + readUserOrgMembershipResolution, + type AuthContext, + type AuthVariables, + type OrgMembershipResolution, + type OrgMembershipFact, + type OrgMembershipSource, +} from "./nyxidAuth"; + +/** An auth context with an optional forwarded access token. */ +function authWith(token?: string): AuthContext { + return { + userId: "user-1", + email: "u@example.com", + displayName: "User One", + roles: [], + permissions: [], + ...(token !== undefined ? { userAccessToken: token } : {}), + }; +} + +/** + * Build a fake org source. `listUserOrgs` either returns the supplied rows or + * throws. `calls` counts how many times NyxID was actually hit so we can + * assert the single-round-trip memo. + */ +function fakeOrgs( + behaviour: + | { kind: "ok"; rows: Array<{ userId: string; role: string; displayName: string }> } + | { kind: "throw" }, +): { source: OrgMembershipSource; calls: () => number } { + let count = 0; + const source: OrgMembershipSource = { + listUserOrgs: async () => { + count += 1; + if (behaviour.kind === "throw") { + throw new Error("nyxid down"); + } + return behaviour.rows; + }, + }; + return { source, calls: () => count }; +} + +/** + * Run the lookup middleware against a single request whose auth context is + * `auth` (or absent when undefined), then hand the live Hono context to + * `inspect` so the test can call the getters with the real memo wiring. + */ +async function withContext( + orgs: OrgMembershipSource, + auth: AuthContext | undefined, + inspect: (c: import("hono").Context<{ Variables: AuthVariables }>) => Promise, +): Promise { + const app = new Hono<{ Variables: AuthVariables }>(); + if (auth) { + app.use("*", async (c, next) => { + c.set("auth", auth); + await next(); + }); + } + app.use("*", nyxidOrgLookupMiddleware(orgs)); + app.get("/", async (c) => { + await inspect(c); + return c.json({ ok: true }); + }); + const res = await app.request("/"); + expect(res.status).toBe(200); +} + +describe("nyxidOrgLookupMiddleware getters (#842)", () => { + test("no forwarded token → unresolved/no_token, memberships []", async () => { + const { source, calls } = fakeOrgs({ kind: "ok", rows: [] }); + await withContext(source, authWith(undefined), async (c) => { + const resolution = await readUserOrgMembershipResolution(c); + expect(resolution.status).toBe("unresolved"); + expect((resolution as Extract).reason).toBe( + "no_token", + ); + expect(resolution.memberships).toEqual([]); + // No token → NyxID was never asked. + expect(calls()).toBe(0); + // Read fail-soft regression guard. + expect(await readUserOrgMemberships(c)).toEqual([]); + }); + }); + + test("listUserOrgs throws → unresolved/lookup_failed, memberships []", async () => { + const { source } = fakeOrgs({ kind: "throw" }); + await withContext(source, authWith("tok-1"), async (c) => { + const resolution = await readUserOrgMembershipResolution(c); + expect(resolution.status).toBe("unresolved"); + expect((resolution as Extract).reason).toBe( + "lookup_failed", + ); + expect(resolution.memberships).toEqual([]); + // Read fail-soft regression guard: still [] on a failed lookup. + expect(await readUserOrgMemberships(c)).toEqual([]); + }); + }); + + test("200-empty → resolved with empty memberships (member of nothing)", async () => { + const { source } = fakeOrgs({ kind: "ok", rows: [] }); + await withContext(source, authWith("tok-1"), async (c) => { + const resolution = await readUserOrgMembershipResolution(c); + expect(resolution.status).toBe("resolved"); + expect(resolution.memberships).toEqual([]); + // Read path agrees. + expect(await readUserOrgMemberships(c)).toEqual([]); + }); + }); + + test("200-with-orgs → resolved, filtered to admin + member roles", async () => { + const { source } = fakeOrgs({ + kind: "ok", + rows: [ + { userId: "org-a", role: "admin", displayName: "Org A" }, + { userId: "org-b", role: "member", displayName: "Org B" }, + // Viewers are non-members for Ornn — filtered out. + { userId: "org-c", role: "viewer", displayName: "Org C" }, + ], + }); + await withContext(source, authWith("tok-1"), async (c) => { + const resolution = await readUserOrgMembershipResolution(c); + expect(resolution.status).toBe("resolved"); + const expected: OrgMembershipFact[] = [ + { userId: "org-a", role: "admin", displayName: "Org A" }, + { userId: "org-b", role: "member", displayName: "Org B" }, + ]; + expect(resolution.memberships).toEqual(expected); + expect(await readUserOrgMemberships(c)).toEqual(expected); + }); + }); + + test("both getters share ONE NyxID round-trip", async () => { + const { source, calls } = fakeOrgs({ + kind: "ok", + rows: [{ userId: "org-a", role: "admin", displayName: "Org A" }], + }); + await withContext(source, authWith("tok-1"), async (c) => { + // Hit both getters, twice each, in mixed order. + await readUserOrgMemberships(c); + await readUserOrgMembershipResolution(c); + await readUserOrgMembershipResolution(c); + await readUserOrgMemberships(c); + // Memoized — NyxID was asked exactly once. + expect(calls()).toBe(1); + }); + }); +}); diff --git a/ornn-api/src/middleware/nyxidAuth.ts b/ornn-api/src/middleware/nyxidAuth.ts index ee4c7bda..e1ecd24a 100644 --- a/ornn-api/src/middleware/nyxidAuth.ts +++ b/ornn-api/src/middleware/nyxidAuth.ts @@ -60,6 +60,30 @@ export interface OrgMembershipFact { displayName: string; } +/** + * First-class resolved/unresolved signal for the org-membership lookup (#842). + * + * The plain `getUserOrgMemberships` getter fails soft to `[]` for both + * "caller genuinely has no orgs" and "we couldn't ask NyxID" — fine for + * read visibility (a missed share just hides a skill), dangerous for the + * write gate (a missed lookup would look like "you're a member of nothing" + * and reject a legitimate share with a misleading 403). This discriminated + * union lets write paths distinguish the two: + * + * - `resolved` — NyxID answered (including a 200-empty list). The + * `memberships` array is authoritative. + * - `unresolved` — we never got an authoritative answer: either no + * forwarded token (`no_token`) or the lookup threw + * (`lookup_failed`). `memberships` is always `[]` and + * MUST NOT be treated as "member of nothing". + * + * Note: a successful lookup that returns zero orgs is `resolved`, not + * `unresolved` — the caller really is a member of no org. + */ +export type OrgMembershipResolution = + | { status: "resolved"; memberships: OrgMembershipFact[] } + | { status: "unresolved"; reason: "no_token" | "lookup_failed"; memberships: [] }; + export type AuthVariables = { auth: AuthContext; /** @@ -75,6 +99,14 @@ export type AuthVariables = { * of a specific org. */ getUserOrgMemberships?: () => Promise; + /** + * Lazy getter for the same lookup as `getUserOrgMemberships`, but carrying + * the resolved/unresolved signal (#842). Shares the SAME memo cell and SAME + * single NyxID round-trip as `getUserOrgMemberships` — calling either getter + * (or both) hits NyxID at most once per request. Write gates that must not + * mistake an unresolved lookup for "member of nothing" read this one. + */ + getUserOrgMembershipResolution?: () => Promise; }; // --------------------------------------------------------------------------- @@ -261,25 +293,6 @@ export function requirePermission(...required: string[]) { }; } -/** - * Allows access if user owns the resource or has ornn:admin:skill permission. - */ -export function requireOwnerOrAdmin(getResourceOwnerId: (c: Context) => Promise) { - return async (c: Context<{ Variables: AuthVariables }>, next: Next) => { - const auth = c.get("auth"); - if (!auth) { - throw new AppError(401, "auth_missing", "Not authenticated"); - } - - const ownerId = await getResourceOwnerId(c); - if (auth.userId !== ownerId && !auth.permissions.includes("ornn:admin:skill")) { - throw new AppError(403, "forbidden", "You can only operate on your own resources"); - } - - await next(); - }; -} - /** * Helper to get auth context from request. Throws if not authenticated. */ @@ -307,52 +320,84 @@ export interface OrgMembershipSource { } /** - * Build a middleware that attaches a lazy `getUserOrgMemberships` getter to - * the request context. The getter: - * - Memoizes within a single request so multiple downstream calls share - * one NyxID round-trip. - * - Returns `[]` for anonymous callers, callers without a forwarded - * access token, or when the NyxID call errors out (fail-soft). - * - Filters to admin + member roles only (viewers are non-members for Ornn). + * Build a middleware that attaches the lazy org-membership getters to the + * request context. Two getters share ONE memo cell and ONE NyxID round-trip: + * - `getUserOrgMemberships` → fail-soft `OrgMembershipFact[]` (`[]` for + * anonymous callers, no forwarded token, or a lookup error). The legacy + * read API — unchanged signature, unchanged fail-soft behaviour. + * - `getUserOrgMembershipResolution` → the same lookup carrying the + * resolved/unresolved discriminant (#842) for write gates that must not + * mistake an unresolved lookup for "member of nothing". + * + * The single resolve runs at most once per request and both getters await it. + * - no forwarded token → unresolved / `no_token` / `[]` + * - `listUserOrgs` throws → unresolved / `lookup_failed`/ `[]` + * (warn-logged, same as before) + * - success (incl. 200-empty) → resolved (filtered to admin + member) * * Mount this AFTER `proxyAuthSetup()` so `auth.userAccessToken` is populated. */ export function nyxidOrgLookupMiddleware(orgs: OrgMembershipSource) { return createMiddleware<{ Variables: AuthVariables }>(async (c, next) => { - let cache: OrgMembershipFact[] | null = null; - c.set("getUserOrgMemberships", async () => { + // Single shared memo cell: the first getter to run resolves it, the + // second reuses it — so at most one NyxID round-trip per request even + // when both the read path and the write gate ask. + let cache: OrgMembershipResolution | null = null; + const resolve = async (): Promise => { if (cache) return cache; const auth = c.get("auth"); const token = auth?.userAccessToken; if (!token) { - cache = []; + cache = { status: "unresolved", reason: "no_token", memberships: [] }; return cache; } try { const raw = await orgs.listUserOrgs(token); - cache = raw + const memberships = raw .filter((m) => m.role === "admin" || m.role === "member") .map((m) => ({ userId: m.userId, role: m.role as "admin" | "member", displayName: m.displayName, })); + // Resolved even when the filtered list is empty: NyxID answered + // authoritatively that the caller belongs to no (admin/member) org. + cache = { status: "resolved", memberships }; } catch (err) { logger.warn( { err: (err as Error).message, userId: auth.userId }, "Org lookup failed; treating user as no-org", ); - cache = []; + cache = { status: "unresolved", reason: "lookup_failed", memberships: [] }; } return cache; - }); + }; + c.set("getUserOrgMembershipResolution", resolve); + c.set("getUserOrgMemberships", async () => (await resolve()).memberships); await next(); }); } +/** + * Resolution convenience helper (#842). Returns the resolved/unresolved + * signal. When the middleware wasn't mounted (tests that don't wire the + * getter) this returns `{ status: "resolved", memberships: [] }` so unrelated + * tests behave as a caller-with-no-orgs rather than flipping into the + * unresolved (503) branch. + */ +export async function readUserOrgMembershipResolution( + c: Context<{ Variables: AuthVariables }>, +): Promise { + const getter = c.get("getUserOrgMembershipResolution"); + if (!getter) return { status: "resolved", memberships: [] }; + return getter(); +} + /** * Memberships convenience helper — returns an empty array when the middleware - * wasn't mounted (tests) or the caller has none. + * wasn't mounted (tests) or the caller has none. Fail-soft to `[]` in every + * case, including an unresolved lookup: the read path stays exactly as before + * (#842 only adds a separate resolution-aware helper for write gates). */ export async function readUserOrgMemberships( c: Context<{ Variables: AuthVariables }>, diff --git a/ornn-api/src/middleware/rateLimit.test.ts b/ornn-api/src/middleware/rateLimit.test.ts index c20a3d26..082c9caf 100644 --- a/ornn-api/src/middleware/rateLimit.test.ts +++ b/ornn-api/src/middleware/rateLimit.test.ts @@ -144,4 +144,128 @@ describe("rateLimit middleware (#439 + #460)", () => { const r3 = await app.request("/"); expect(r3.status).toBe(429); }); + + test("per-pod isolation is by-design: independent limiter instances do NOT share a budget (shared store tracked in #837)", async () => { + // This is the asserted-by-design DUAL of the issue's shared-store AC + // (#814): because the deployment is pinned single-replica (no HPA), + // per-pod budgets are the intended behaviour, NOT a bug. We simulate + // two pods with two independent limiter instances. The module-level + // `buckets` Map is shared in-process but NAMESPACED by `label`, so we + // MUST give each instance a DISTINCT label ("iso-a" vs "iso-b") to + // genuinely prove separate budgets — a shared label would let B's + // first hit reuse A's bucket and make this test pass vacuously. + function makePodApp(label: string) { + const pod = new Hono(); + pod.use("*", rateLimit({ windowMs: 60_000, max: 2, label })); + pod.get("/", (c) => c.json({ ok: true })); + pod.onError((err, c) => { + if (err instanceof AppError) return c.json({ code: err.code }, err.statusCode as never); + return c.json({ code: "internal_error" }, 500); + }); + return pod; + } + const podA = makePodApp("iso-a"); + const podB = makePodApp("iso-b"); + + // Drive instance A to its cap (max=2): two 200s then a 429 on the 3rd. + expect((await podA.request("/")).status).toBe(200); + expect((await podA.request("/")).status).toBe(200); + expect((await podA.request("/")).status).toBe(429); + + // Instance B's budget is untouched by A exhausting A's — its first two + // requests still succeed. Combined throughput is 2×max (4), NOT a + // single shared max of 2. This is the per-pod multiplication that the + // shared-store backend (#837) would eliminate once replicas > 1. + expect((await podB.request("/")).status).toBe(200); + expect((await podB.request("/")).status).toBe(200); + expect((await podB.request("/")).status).toBe(429); + }); +}); + +/** + * Anonymous (no-auth) keying — the spoofable-XFF surface (#813, CWE-348). + * + * The leftmost X-Forwarded-For token is client-controlled: an attacker + * can rotate it per request to shard the per-IP bucket and bypass the + * cap. The fix keys on a trusted token counted from the RIGHT (proxies + * append), configurable via ORNN_TRUSTED_PROXY_HOPS. These tests build + * a bare app with NO auth stub so `defaultKeyBy` falls through to the + * IP branch. + */ +describe("rateLimit middleware — anonymous XFF keying (#813)", () => { + beforeEach(() => __resetRateLimitForTests()); + + // Bare app: no auth stub, so defaultKeyBy hits the x-forwarded-for path. + function makeAnonApp(opts: { windowMs?: number; max?: number; label?: string }) { + const app = new Hono(); + app.use( + "*", + rateLimit({ + windowMs: opts.windowMs ?? 60_000, + max: opts.max ?? 2, + label: opts.label ?? "ip", + }), + ); + app.get("/", (c) => c.json({ ok: true })); + 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; + } + + test("anonymous: distinct spoofed leftmost XFF + same trusted hop share one bucket", async () => { + // Default hops=0 → trusted token is the rightmost "9.9.9.9", shared by + // all three. The distinct leftmost tokens are client-prepended spoofs + // and MUST NOT shard the bucket. + const app = makeAnonApp({ windowMs: 60_000, max: 2, label: "ip" }); + const r1 = await app.request("/", { headers: { "x-forwarded-for": "1.1.1.1, 9.9.9.9" } }); + const r2 = await app.request("/", { headers: { "x-forwarded-for": "2.2.2.2, 9.9.9.9" } }); + const r3 = await app.request("/", { headers: { "x-forwarded-for": "3.3.3.3, 9.9.9.9" } }); + expect(r1.status).toBe(200); + expect(r2.status).toBe(200); + expect(r3.status).toBe(429); // shared bucket exhausted at max=2 + }); + + test("anonymous: distinct trusted hop → separate buckets", async () => { + // Different rightmost (trusted) token → genuinely different clients → + // independent buckets. max=1 means each is allowed exactly once. + const app = makeAnonApp({ windowMs: 60_000, max: 1, label: "ip" }); + const a = await app.request("/", { headers: { "x-forwarded-for": "1.1.1.1, 8.8.8.8" } }); + const b = await app.request("/", { headers: { "x-forwarded-for": "1.1.1.1, 9.9.9.9" } }); + expect(a.status).toBe(200); + expect(b.status).toBe(200); // different trusted token → not the same bucket + }); + + test("respects ORNN_TRUSTED_PROXY_HOPS (counts from the right)", async () => { + // With hops=1 the trusted token is len-2 = "7.7.7.7" for all three + // requests, even though both the leftmost spoof AND the rightmost peer + // token differ per request. Proves the index is computed from the right. + const prev = process.env.ORNN_TRUSTED_PROXY_HOPS; + process.env.ORNN_TRUSTED_PROXY_HOPS = "1"; + try { + const app = makeAnonApp({ windowMs: 60_000, max: 2, label: "ip" }); + const r1 = await app.request("/", { headers: { "x-forwarded-for": "a, 7.7.7.7, peer1" } }); + const r2 = await app.request("/", { headers: { "x-forwarded-for": "b, 7.7.7.7, peer2" } }); + const r3 = await app.request("/", { headers: { "x-forwarded-for": "c, 7.7.7.7, peer3" } }); + expect(r1.status).toBe(200); + expect(r2.status).toBe(200); + expect(r3.status).toBe(429); // all keyed on "7.7.7.7" → bucket exhausted + } finally { + // Restore so the env var doesn't leak into sibling tests. + if (prev === undefined) delete process.env.ORNN_TRUSTED_PROXY_HOPS; + else process.env.ORNN_TRUSTED_PROXY_HOPS = prev; + } + }); }); diff --git a/ornn-api/src/middleware/rateLimit.ts b/ornn-api/src/middleware/rateLimit.ts index 6d118206..3f6de05a 100644 --- a/ornn-api/src/middleware/rateLimit.ts +++ b/ornn-api/src/middleware/rateLimit.ts @@ -13,17 +13,25 @@ * style SDKs, etc.) read these and self-throttle. Without them, * clients can't tell the difference between "go slower" and "go away". * - * Storage is in-memory per process — fine for the single-replica dev / - * staging cluster; multi-pod prod will need a Redis backend before - * this can be tightened. The middleware behind the scenes uses a + * Storage is INTENTIONALLY process-local. The bucket budget is + * PER-POD, and this is correct ONLY while ornn-api runs single-replica + * with no HPA — the current, asserted deployment topology + * (`deployment/ornn-api/deployment.yaml` pins `replicas: 1`). Raising + * `replicas > 1` or adding an HPA WITHOUT a shared-store backend first + * (tracked in #837) is a correctness bug: per-pod buckets multiply the + * effective limit by the replica count. The shared store is therefore a + * hard PREREQUISITE for any horizontal scale-out, not a later + * optimisation. The middleware behind the scenes uses a * `Map` with periodic-on-access cleanup so * idle keys don't accumulate forever. * * Mount order: must run AFTER `proxyAuthSetup` so `auth.userId` is * available for keying. When auth is missing (public endpoints), the - * key falls back to the proxy-forwarded IP. Without IP either, - * everyone shares the "anonymous" bucket — a fail-safe for the rare - * truly-public path. + * key falls back to a trusted-position `x-forwarded-for` token — + * counted from the right per `ORNN_TRUSTED_PROXY_HOPS` so a client- + * prepended leftmost token can't shard the bucket (CWE-348, #813). + * Without a usable token, everyone shares the "anonymous" bucket — a + * fail-safe for the rare truly-public path. * * @module middleware/rateLimit */ @@ -34,6 +42,19 @@ import { createLogger } from "../shared/logger"; const logger = createLogger("rateLimit"); +const TRUSTED_HOPS_ENV = "ORNN_TRUSTED_PROXY_HOPS"; +/** + * Number of trusted proxies that append to X-Forwarded-For in front of + * ornn-api, beyond the immediate connecting peer. Default 0 = key on the + * peer-appended (rightmost) token. The real client IP sits `hops` tokens + * from the right; counting from the right defeats client-prepended spoof + * tokens. Operators set this to match their ingress topology (#813). + */ +function readTrustedProxyHops(): number { + const raw = Number(process.env[TRUSTED_HOPS_ENV]); + return Number.isInteger(raw) && raw >= 0 ? raw : 0; // default/clamp on unset/NaN/negative/non-int +} + export interface RateLimitConfig { /** Window length in milliseconds. */ readonly windowMs: number; @@ -85,10 +106,23 @@ function cleanupExpired(now: number): void { function defaultKeyBy(c: Context): string { const auth = (c.get as unknown as (k: "auth") => { userId?: string } | undefined)("auth"); if (auth?.userId) return `user:${auth.userId}`; - // Trust proxy-forwarded IP — production traffic comes through - // NyxID's reverse proxy which sets `x-forwarded-for`. - const ip = c.req.header("x-forwarded-for")?.split(",")[0]?.trim(); - if (ip) return `ip:${ip}`; + // XFF arrives as `client, …prepended…, realClient, proxy1, …` (proxies + // APPEND). Key on the trusted hop counted from the RIGHT so a client- + // prepended leftmost token cannot shard the bucket (CWE-348, #813). + const xff = c.req.header("x-forwarded-for"); + if (xff) { + const tokens = xff.split(",").map((t) => t.trim()).filter((t) => t.length > 0); + const idx = tokens.length - 1 - readTrustedProxyHops(); + // idx < 0 means the chain is shorter than the configured trusted-hop + // count (misconfig or a request that skipped the proxy chain): we cannot + // identify a trusted token, so bucket together rather than trust a + // client-controlled token. CRITICAL: do NOT clamp to tokens[0] — that is + // the spoofable leftmost token (the exact bug). Fail safe to a shared bucket. + if (idx >= 0) { + const ip = tokens[idx]; + if (ip) return `ip:${ip}`; + } + } return "anonymous"; } diff --git a/ornn-api/src/shared/cursor.test.ts b/ornn-api/src/shared/cursor.test.ts index 013dbd09..610abebd 100644 --- a/ornn-api/src/shared/cursor.test.ts +++ b/ornn-api/src/shared/cursor.test.ts @@ -9,7 +9,7 @@ */ import { describe, expect, test } from "bun:test"; -import { encodeCursor, decodeCursor, buildNextCursor } from "./cursor"; +import { encodeCursor, decodeCursor, buildNextCursor, MAX_PAGE } from "./cursor"; describe("encodeCursor + decodeCursor round-trip", () => { test("encodes + decodes a page payload losslessly", () => { @@ -39,6 +39,12 @@ describe("encodeCursor + decodeCursor round-trip", () => { expect(decodeCursor(Buffer.from('{"page":-1}', "utf-8").toString("base64url"))).toBeNull(); // page is 0 expect(decodeCursor(Buffer.from('{"page":0}', "utf-8").toString("base64url"))).toBeNull(); + // page at the ceiling is still valid (CWE-770 #810) + expect(decodeCursor(encodeCursor({ page: MAX_PAGE }))).toEqual({ page: MAX_PAGE }); + // page one past the ceiling → null + expect(decodeCursor(encodeCursor({ page: MAX_PAGE + 1 }))).toBeNull(); + // forged huge page → null (the unbounded-`.skip()` DoS vector) + expect(decodeCursor(Buffer.from('{"page":999999999}', "utf-8").toString("base64url"))).toBeNull(); }); }); diff --git a/ornn-api/src/shared/cursor.ts b/ornn-api/src/shared/cursor.ts index bc625cfb..15918ad9 100644 --- a/ornn-api/src/shared/cursor.ts +++ b/ornn-api/src/shared/cursor.ts @@ -16,6 +16,10 @@ * @module shared/cursor */ +// Ceiling bounds `.skip()` so a forged cursor can't drive a multi-second +// collection scan (CWE-770, #810). +export const MAX_PAGE = 10_000; + export interface CursorPayload { /** 1-indexed page number — what the underlying offset query reads. */ page: number; @@ -29,7 +33,9 @@ export function encodeCursor(payload: CursorPayload): string { /** * Decode an opaque cursor. Returns `null` for any malformed input — * the route layer then surfaces a 400 `invalid_cursor` so an old - * client doesn't end up paginating from page 1 silently. + * client doesn't end up paginating from page 1 silently. A page above + * `MAX_PAGE` is also rejected so a forged cursor can't drive an + * unbounded `.skip()` (CWE-770, #810). */ export function decodeCursor(raw: string | undefined): CursorPayload | null { if (!raw || typeof raw !== "string" || raw.length === 0) return null; @@ -38,7 +44,7 @@ export function decodeCursor(raw: string | undefined): CursorPayload | null { const parsed = JSON.parse(json) as unknown; if (!parsed || typeof parsed !== "object") return null; const page = (parsed as { page?: unknown }).page; - if (typeof page !== "number" || !Number.isInteger(page) || page < 1) return null; + if (typeof page !== "number" || !Number.isInteger(page) || page < 1 || page > MAX_PAGE) return null; return { page }; } catch { return null; diff --git a/ornn-api/src/shared/githubNaming.test.ts b/ornn-api/src/shared/githubNaming.test.ts new file mode 100644 index 00000000..60591a7b --- /dev/null +++ b/ornn-api/src/shared/githubNaming.test.ts @@ -0,0 +1,62 @@ +/** + * Unit tests for shared GitHub naming validation. + * + * @module shared/githubNaming.test + */ + +import { describe, expect, it } from "bun:test"; +import { OWNER_RE, REPO_RE, hasUnsafeSegment } from "./githubNaming"; + +describe("OWNER_RE", () => { + it("accepts a plain owner", () => { + expect(OWNER_RE.test("owner")).toBe(true); + }); + + it("rejects the empty string", () => { + expect(OWNER_RE.test("")).toBe(false); + }); + + it("rejects a leading-hyphen owner", () => { + expect(OWNER_RE.test("-owner")).toBe(false); + }); +}); + +describe("REPO_RE", () => { + it("accepts repo.name with a dot", () => { + expect(REPO_RE.test("repo.name")).toBe(true); + }); + + it("accepts repo-1 with a dash", () => { + expect(REPO_RE.test("repo-1")).toBe(true); + }); + + it("rejects a value containing a slash", () => { + expect(REPO_RE.test("a/b")).toBe(false); + }); + + it("rejects a value over 100 chars", () => { + expect(REPO_RE.test("a".repeat(101))).toBe(false); + }); +}); + +describe("hasUnsafeSegment", () => { + it("flags a trailing .. segment", () => { + expect(hasUnsafeSegment("owner/..")).toBe(true); + }); + + it("flags a leading .. segment", () => { + expect(hasUnsafeSegment("../repo")).toBe(true); + }); + + it("flags a dot-prefixed (hidden) segment", () => { + expect(hasUnsafeSegment(".hidden")).toBe(true); + }); + + it("flags a dot-suffixed (trailing-dot) segment", () => { + expect(hasUnsafeSegment("trail.")).toBe(true); + }); + + it("allows a normal owner/repo.name identifier", () => { + expect(hasUnsafeSegment("owner/repo.name")).toBe(false); + }); +}); diff --git a/ornn-api/src/shared/githubNaming.ts b/ornn-api/src/shared/githubNaming.ts new file mode 100644 index 00000000..7a0f7f08 --- /dev/null +++ b/ornn-api/src/shared/githubNaming.ts @@ -0,0 +1,39 @@ +/** + * Shared GitHub owner/repo naming validation. + * + * Single source of truth for the GitHub identifier charset + length rules + * so the mirror routes, the mirror settings section, and the repo-pull + * path all enforce the same shape. + * + * owner: alphanumeric + dashes; can't start or end with a dash; 1–39 chars. + * repo: alphanumeric + dot/dash/underscore; 1–100 chars. + * + * Note: passing the charset/length regexes does NOT mean an identifier is + * safe to splice into a filesystem path or URL — a `repo` segment can be + * `..` or `.` and still match `REPO_RE`. Callers that build paths must + * additionally reject path-traversal segments via {@link hasUnsafeSegment}. + * + * @module shared/githubNaming + */ + +export const OWNER_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$/; +export const REPO_RE = /^[A-Za-z0-9._-]{1,100}$/; + +/** + * True when any slash-delimited segment of `s` is a path-traversal or + * hidden-segment hazard: exactly `.`/`..`, or starts/ends with a dot + * (e.g. `.hidden`, `trail.`). Use this to reject identifiers that would + * otherwise pass the charset regexes but are unsafe to splice into a + * filesystem path. + */ +export function hasUnsafeSegment(s: string): boolean { + return s + .split("/") + .some( + (seg) => + seg === "." || + seg === ".." || + seg.startsWith(".") || + seg.endsWith("."), + ); +} diff --git a/ornn-api/src/shared/logger.test.ts b/ornn-api/src/shared/logger.test.ts index 19f374e4..d55ff7a5 100644 --- a/ornn-api/src/shared/logger.test.ts +++ b/ornn-api/src/shared/logger.test.ts @@ -13,7 +13,9 @@ */ import { describe, expect, test } from "bun:test"; -import { createLogger } from "./logger"; +import { Writable } from "node:stream"; +import pino from "pino"; +import { createLogger, REDACT_PATHS } from "./logger"; describe("createLogger (#575)", () => { test("returns a logger bound to the module name", () => { @@ -62,4 +64,60 @@ describe("createLogger (#575)", () => { expect(child).toBeDefined(); expect(typeof child.info).toBe("function"); }); + + test("REDACT_PATHS censors token-family secrets at depth-2 (#817)", () => { + // Build our OWN pino instance from the exported constant + a + // minimal capture stream, deliberately avoiding pino-pretty and any + // env/import-order coupling. This makes the assertion fully + // order-independent — it behaves identically in a scoped `bun test + // src/shared/` run and in the single-process full suite (the trap + // that broke #816's CI). + // + // The `*.field` redaction wildcard matches EXACTLY one level, so we + // log a depth-2 object (`user.token`, ...). A depth-1 or depth-3 + // payload would NOT match and the test would silently pass without + // proving anything. + const chunks: string[] = []; + const captureStream = new Writable({ + write(chunk, _enc, cb) { + chunks.push(chunk.toString()); + cb(); + }, + }); + const logger = pino({ redact: { paths: REDACT_PATHS } }, captureStream); + + logger.info( + { + user: { + token: "RAW_T", + accessToken: "RAW_AT", + userAccessToken: "RAW_UAT", + clientSecret: "RAW_CS", + privateKey: "RAW_PK", + }, + }, + "redaction check", + ); + + const serialized = chunks.join(""); + const parsed = JSON.parse(serialized); + + expect(parsed.user.token).toBe("[Redacted]"); + expect(parsed.user.accessToken).toBe("[Redacted]"); + expect(parsed.user.userAccessToken).toBe("[Redacted]"); + expect(parsed.user.clientSecret).toBe("[Redacted]"); + expect(parsed.user.privateKey).toBe("[Redacted]"); + + // Belt-and-braces: no raw secret value survives anywhere in the + // serialized line, not just under the expected keys. + for (const sentinel of [ + "RAW_T", + "RAW_AT", + "RAW_UAT", + "RAW_CS", + "RAW_PK", + ]) { + expect(serialized).not.toContain(sentinel); + } + }); }); diff --git a/ornn-api/src/shared/logger.ts b/ornn-api/src/shared/logger.ts index d5c643be..cb310105 100644 --- a/ornn-api/src/shared/logger.ts +++ b/ornn-api/src/shared/logger.ts @@ -13,6 +13,12 @@ * all of that — a misconfigured handler logging req headers * would leak bearer tokens. * + * `REDACT_PATHS` (exported below) is the single source of truth for + * the redaction list. Both other pino roots — `bootstrap.ts`'s + * request/response logger and `index.ts`'s startup logger — import it + * rather than re-declaring their own list, so adding a sensitive field + * here covers every logger in the process at once. + * * `createLogger(moduleName)` returns a child of a single process-wide * pino root configured with both — every consumer inherits both knobs * for free. @@ -51,16 +57,23 @@ export type { Logger }; const LEVEL = process.env.LOG_LEVEL ?? "info"; /** - * Default redaction rules. Mirrors the bootstrap pino exactly so - * module loggers can't accidentally log a bearer token. Adding new - * sensitive fields here updates every logger in the codebase. + * Default redaction rules — the single source of truth for log + * redaction across the whole service. Imported by `bootstrap.ts` and + * `index.ts` so every pino root shares one list; module loggers + * created via `createLogger()` inherit it too. Adding a sensitive + * field here updates every logger in the codebase. */ -const REDACT_PATHS = [ +export const REDACT_PATHS = [ "req.headers.authorization", 'req.headers["x-api-key"]', "*.password", "*.secret", "*.apiKey", + "*.token", + "*.accessToken", + "*.userAccessToken", + "*.clientSecret", + "*.privateKey", ]; /** diff --git a/ornn-api/src/shared/schemas/skillFrontmatter.ts b/ornn-api/src/shared/schemas/skillFrontmatter.ts index de8adf46..a6d1d617 100644 --- a/ornn-api/src/shared/schemas/skillFrontmatter.ts +++ b/ornn-api/src/shared/schemas/skillFrontmatter.ts @@ -161,9 +161,21 @@ export const refinedMetadataSchema = metadataSchema.superRefine((data, ctx) => { */ export const SKILL_VERSION_REGEX = /^(0|[1-9]\d*)\.(0|[1-9]\d*)$/; +/** + * Canonical skill-name shape: kebab-case, must start with a lowercase + * letter or digit. Single source of truth — consumed by the strict + * frontmatter schema, the lenient (`skipValidation`) extractor, the ZIP + * folder-name check, and the GitHub-mirror folder-name guard. This regex + * already rejects `/`, `\`, `.`, `..`, a leading `-`, and uppercase, so + * a skill name can never escape its `/` subtree on the public + * mirror (CWE-22 path traversal, #807). + */ +export const SKILL_NAME_REGEX = /^[a-z0-9][a-z0-9-]*$/; +export const SKILL_NAME_MAX = 64; + // Full frontmatter schema export const skillFrontmatterSchema = z.object({ - name: z.string().min(1).max(64).regex(/^[a-z0-9][a-z0-9-]*$/, "Name must be kebab-case"), + name: z.string().min(1).max(SKILL_NAME_MAX).regex(SKILL_NAME_REGEX, "Name must be kebab-case"), description: z.string().min(1).max(1024), // YAML parses `version: 0.1` (unquoted) as a float and `1.0` as an // integer `1`, which both lose the intended two-digit shape. We require diff --git a/ornn-api/tests/integration/harness.ts b/ornn-api/tests/integration/harness.ts index 0221ce9d..89f3a311 100644 --- a/ornn-api/tests/integration/harness.ts +++ b/ornn-api/tests/integration/harness.ts @@ -18,6 +18,7 @@ import { MongoMemoryServer } from "mongodb-memory-server"; import { MongoClient, type Db } from "mongodb"; import { bootstrap } from "../../src/bootstrap"; import type { SkillConfig } from "../../src/infra/config"; +import type { NyxLlmClient } from "../../src/clients/nyxid/llm"; import type { Hono } from "hono"; export interface Harness { @@ -65,6 +66,17 @@ export function authHeaders(auth: SimAuth): Record { }; } +/** + * Optional per-harness dependency overrides. Mirrors `BootstrapOverrides` + * — the only knob today is swapping the shared `NyxLlmClient` for an + * in-process fake (`tests/mocks/llmGateway.ts`) so charge-path tests run + * the real route → service → quota wiring without touching the network. + */ +export interface StartHarnessOptions { + /** Substitute the shared LLM gateway client. */ + llmClient?: NyxLlmClient; +} + let cached: Harness | null = null; /** @@ -74,9 +86,18 @@ let cached: Harness | null = null; * Mongo takes ~2s each time, so sharing one instance per `bun test` run * is worth the isolation trade-off. Individual tests MUST clean up any * state they seed via the `db` handle. + * + * When `opts.llmClient` is set the cache is SKIPPED and a fresh, + * non-shared harness is built — an override harness wires a different LLM + * client, so handing back the shared default-harness instance would be + * wrong. Override harnesses are not shared; the caller owns its lifecycle + * and MUST `cleanup()` it. */ -export async function startHarness(): Promise { - if (cached) return cached; +export async function startHarness( + opts?: StartHarnessOptions, +): Promise { + const useOverride = !!opts?.llmClient; + if (cached && !useOverride) return cached; const mongo = await MongoMemoryServer.create(); const mongoUri = mongo.getUri(); @@ -107,7 +128,11 @@ export async function startHarness(): Promise { agentsealEnabled: false, }; - const { app, shutdown } = await bootstrap(config); + const { app, shutdown } = await bootstrap( + config, + // exactOptionalPropertyTypes (#657): only attach the key when set. + opts?.llmClient ? { llmClient: opts.llmClient } : undefined, + ); // Separate client for test-side seeding. Bootstrap holds its own // internal client; closing ours does not affect it. @@ -123,10 +148,12 @@ export async function startHarness(): Promise { await client.close().catch(() => {}); await shutdown().catch(() => {}); await mongo.stop().catch(() => {}); - cached = null; + // Only the shared default harness lives in `cached`; an override + // harness was never stored there, so don't clobber the shared one. + if (!useOverride) cached = null; }, }; - cached = harness; + if (!useOverride) cached = harness; return harness; } diff --git a/ornn-api/tests/integration/playground_charge.test.ts b/ornn-api/tests/integration/playground_charge.test.ts index aaeeed1c..19cc5901 100644 --- a/ornn-api/tests/integration/playground_charge.test.ts +++ b/ornn-api/tests/integration/playground_charge.test.ts @@ -3,27 +3,34 @@ * * End-to-end flow tests for the playground surface charging path: * - At-cap user → 429 with QUOTA_EXCEEDED + * - Model-resolution failure (no models enabled) → 503, no bucket row * - Successful stream → bucket.used += 1, usedByModel[model] += 1 * - skill_error stream → bucket.used += 1 - * - system_error stream → bucket unchanged + * - system_error stream → bucket released back to baseline + * - Abort mid-stream → bucket released back to baseline * - Admin caller → no quota check, no charge * * The test seeds the `quota_buckets` collection directly (bypasses the - * quota service constructor) so we can pin the start state. The mock - * LLM gateway controls outcome via `installLlmGatewayMock`. - * - * NOTE: Round 1 implementation may still wire through the legacy - * `user_quotas` collection while backend-engineer's quota service - * rewrite lands. These tests assert against the *new* `quota_buckets` - * shape; if they fail with collection-not-found until Round 2, that's - * the expected blocker — not a test bug. + * quota service constructor) so we can pin the start state. The charge- + * path cases inject the in-process `installLlmGatewayMock()` via the + * harness `llmClient` seam so the real route → chat service → quota + * wiring runs without touching the network, and the mock controls the + * outcome (success / skill_error / system_error). * * @module tests/integration/playground_charge.test */ import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; +import { Hono } from "hono"; import { startHarness, type Harness, authHeaders } from "./harness"; import { resetCollections } from "./cleanup"; +import { installLlmGatewayMock } from "../mocks/llmGateway"; +import type { NyxLlmClient } from "../../src/clients/nyxid/llm"; +import { + createPlaygroundRoutes, + type PlaygroundRoutesConfig, +} from "../../src/domains/playground/routes"; +import type { PlaygroundChatEvent } from "../../src/shared/types/index"; let h: Harness; @@ -36,7 +43,7 @@ afterAll(async () => { }); beforeEach(async () => { - await resetCollections(h.db, ["quota_buckets", "platform_settings"]); + await resetCollections(h.db, ["quota_buckets", "platform_settings", "llm_providers"]); }); const userAuth = (userId: string, isAdmin = false) => @@ -48,19 +55,94 @@ const userAuth = (userId: string, isAdmin = false) => : ["ornn:playground:use"], }); +/** + * Seed one provider with a single model enabled for the playground + * surface so `resolveModel({ surface: "playground" })` returns `ok`. + * The injected mock LLM client ignores gateway/auth, so the seed only + * needs to satisfy the catalog resolver, not a real upstream. + */ +async function seedPlaygroundModel(db: Harness["db"], modelId: string): Promise { + const now = new Date(); + await db.collection("llm_providers").insertOne({ + _id: "prov-playground", + name: "test-provider", + gatewayUrl: "https://gw.test.invalid", + modelListUrl: "https://gw.test.invalid/models", + apiFormat: "responses", + auth: { kind: "apiKey", apiKeyEnc: "" }, + models: [ + { + id: modelId, + displayName: modelId, + enabledForPlayground: true, + enabledForSkillGen: false, + defaultForPlayground: true, + defaultForSkillGen: false, + removed: false, + firstSeenAt: now, + lastSyncedAt: now, + }, + ], + maxOutputTokens: 8192, + defaultTemperature: 0.7, + createdAt: now, + updatedAt: now, + updatedBy: "test", + }); +} + +const monthMarker = () => new Date().toISOString().slice(0, 7); + +/** Drain an SSE response body to completion so the producer task ends. */ +async function drainBody(res: Response): Promise { + const reader = res.body?.getReader(); + if (!reader) return; + for (let done = false; !done; ) { + ({ done } = await reader.read()); + } +} + +/** + * Poll the bucket until `predicate` holds or the bounded retry budget is + * exhausted. The producer's `finally` fires `chargeOnCompletion` after + * the response stream closes; it is not awaitable from the route, so we + * poll rather than race a fixed sleep — keeps the assertion deterministic. + */ +async function waitForBucket( + db: Harness["db"], + filter: Record, + predicate: (doc: Record | null) => boolean, + { tries = 100, intervalMs = 10 } = {}, +): Promise | null> { + for (let i = 0; i < tries; i++) { + const doc = (await db.collection("quota_buckets").findOne(filter)) as + | Record + | null; + if (predicate(doc)) return doc; + await new Promise((r) => setTimeout(r, intervalMs)); + } + return (await db.collection("quota_buckets").findOne(filter)) as + | Record + | null; +} + describe("IT-PLAYGROUND-QUOTA-DENY", () => { test("user at cap → 429 QUOTA_EXCEEDED", async () => { - const monthMarker = new Date().toISOString().slice(0, 7); + const mm = monthMarker(); + await seedPlaygroundModel(h.db, "gpt-test"); + // Pin used == cap. `effectiveDefault = max(stored, settingsDefault)`; + // the playground settings default is 200, so the stored allotment + // must be >= 200 for `used` to be at the effective cap. await h.db.collection("quota_buckets").insertOne({ - _id: `u-cap:playground:${monthMarker}`, + _id: `u-cap:playground:${mm}`, userId: "u-cap", surface: "playground", - monthMarker, + monthMarker: mm, monthStart: new Date(), monthEnd: new Date(), - defaultAllotment: 5, + defaultAllotment: 200, adminGrant: 0, - used: 5, + used: 200, usedByModel: {}, createdAt: new Date(), updatedAt: new Date(), @@ -71,45 +153,300 @@ describe("IT-PLAYGROUND-QUOTA-DENY", () => { headers: { ...userAuth("u-cap"), "Content-Type": "application/json" }, body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), }); - // The user is at cap. The route must reject pre-charge. Accept - // any non-2xx — 429 is the spec-correct quota surface; 503 is - // the fail-closed "no LLM provider configured" surface (which - // fires before quota when settings aren't seeded). A 2xx here - // would mean the call actually ran the LLM despite a full bucket. - expect(res.status).toBeGreaterThanOrEqual(400); + // The user is at cap; the route must reject pre-charge with 429 + // (model resolution succeeds here because a model is seeded). + expect(res.status).toBe(429); + }); +}); + +describe("IT-PLAYGROUND-MODEL-RESOLUTION-FAIL", () => { + test("no models enabled → 503 and NO bucket row for the user", async () => { + // No provider seeded → resolveModel returns no-models-enabled → 503, + // before quota reserve, so the user's bucket must never be created. + const res = await h.app.request("/api/v1/playground/chat", { + method: "POST", + headers: { ...userAuth("u-nomodels"), "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + }); + expect(res.status).toBe(503); + const buckets = await h.db + .collection("quota_buckets") + .find({ userId: "u-nomodels", surface: "playground" }) + .toArray(); + expect(buckets.length).toBe(0); }); }); describe("IT-ADMIN-BYPASS-PLAYGROUND", () => { test("admin caller skips quota check (no bucket created)", async () => { + await seedPlaygroundModel(h.db, "gpt-test"); const res = await h.app.request("/api/v1/playground/chat", { method: "POST", headers: { ...userAuth("u-admin", true), "Content-Type": "application/json" }, body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), }); - // The chat call may fail because no LLM provider is configured in - // the test settings — acceptable: we're asserting the QUOTA gate - // didn't reject the admin pre-LLM (no 429), and no bucket row - // got upserted on this surface. + // Admins bypass the quota gate (no 429) and no bucket is created. expect(res.status).not.toBe(429); const buckets = await h.db .collection("quota_buckets") .find({ userId: "u-admin", surface: "playground" }) .toArray(); - // Admins bypass; no bucket should be created on their behalf. expect(buckets.length).toBe(0); }); }); -describe("IT-PLAYGROUND-CHARGE-* (smoke)", () => { - test.todo("success outcome charges 1 + usedByModel[model] += 1", () => { - // Requires the LlmGateway mock to be installed in bootstrap. The - // round-1 harness doesn't yet expose a mock-injection seam — this - // test ships as a TODO so the assertion exists in the suite and a - // later round can flip the .todo to .test once the seam is wired. +describe("IT-PLAYGROUND-CHARGE-* (via injected LLM mock)", () => { + test("success outcome charges 1 + usedByModel[model] += 1", async () => { + const { client, handle } = installLlmGatewayMock({ + outcome: "success", + modelId: "gpt-test", + text: "hello", + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedPlaygroundModel(oh.db, "gpt-test"); + + const res = await oh.app.request("/api/v1/playground/chat", { + method: "POST", + headers: { ...userAuth("u-ok"), "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + }); + expect(res.status).toBe(200); + await drainBody(res); + expect(handle.callCount()).toBeGreaterThan(0); + + const mm = monthMarker(); + const bucket = await waitForBucket( + oh.db, + { _id: `u-ok:playground:${mm}` }, + (d) => !!d && (d.usedByModel as Record)?.["gpt-test"] === 1, + ); + expect(bucket?.used).toBe(1); + expect((bucket?.usedByModel as Record)["gpt-test"]).toBe(1); + } finally { + await oh.cleanup(); + } + }); + + test("completed run with dropped unknown error event still charges 1 (playground has no skill_error mapping — see follow-up)", async () => { + // NOTE: the mock's `skill_error` outcome yields a `response.error` + + // `response.completed{finishReason:"skill_error"}`, but the playground + // chat service's event union does not recognize those shapes — they + // are dropped as unknown events. The run therefore terminates on the + // service's own `finish{finishReason:"stop"}`, so the route records a + // SUCCESS-shaped completion. There is no skill_error event path on the + // playground surface today; this case asserts that a completed run + // (with the error frame silently dropped) still commits a +1 charge — + // identical to the success case. Wiring a real playground skill_error + // outcome is a separate product follow-up, intentionally out of scope. + const { client } = installLlmGatewayMock({ + outcome: "skill_error", + modelId: "gpt-test", + text: "partial", + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedPlaygroundModel(oh.db, "gpt-test"); + + const res = await oh.app.request("/api/v1/playground/chat", { + method: "POST", + headers: { ...userAuth("u-skillerr"), "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + }); + expect(res.status).toBe(200); + await drainBody(res); + + const mm = monthMarker(); + const bucket = await waitForBucket( + oh.db, + { _id: `u-skillerr:playground:${mm}` }, + (d) => !!d && (d.used as number) === 1, + ); + expect(bucket?.used).toBe(1); + } finally { + await oh.cleanup(); + } + }); + + test("system_error outcome leaves used unchanged (slot released)", async () => { + const { client } = installLlmGatewayMock({ + outcome: "system_error", + modelId: "gpt-test", + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedPlaygroundModel(oh.db, "gpt-test"); + + const res = await oh.app.request("/api/v1/playground/chat", { + method: "POST", + headers: { ...userAuth("u-syserr"), "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + }); + expect(res.status).toBe(200); + await drainBody(res); + + const mm = monthMarker(); + // Reserve bumped used to 1; system_error releases it back to 0. + const bucket = await waitForBucket( + oh.db, + { _id: `u-syserr:playground:${mm}` }, + (d) => !!d && (d.used as number) === 0, + ); + expect(bucket?.used).toBe(0); + } finally { + await oh.cleanup(); + } + }); +}); + +describe("IT-PLAYGROUND-ABORT", () => { + test("abort mid-stream releases the reserved slot (used back to baseline)", async () => { + // `delayMs` parks the mock producer on a real timer between stream + // events (after `response.created`, before `response.completed`), so + // the abort below lands deterministically while the generator is + // suspended mid-stream — by control flow, not back-pressure luck. + // Outcome would be success, but the client aborts before `finish`. + const { client } = installLlmGatewayMock({ + outcome: "success", + modelId: "gpt-test", + text: "streaming...", + delayMs: 50, + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedPlaygroundModel(oh.db, "gpt-test"); + + const controller = new AbortController(); + const resPromise = oh.app.request("/api/v1/playground/chat", { + method: "POST", + headers: { ...userAuth("u-abort"), "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + signal: controller.signal, + }); + + const res = await resPromise; + expect(res.status).toBe(200); + // Begin draining, then abort mid-stream. + const reader = res.body?.getReader(); + // Read the first chunk (the stream-open pad frame) then abort. + await reader?.read().catch(() => {}); + controller.abort(); + await reader?.cancel().catch(() => {}); + + const mm = monthMarker(); + // The producer's abort handler closes the writer; its `finally` + // charges with outcome=system_error → release → used back to 0. + const bucket = await waitForBucket( + oh.db, + { _id: `u-abort:playground:${mm}` }, + (d) => !!d && (d.used as number) === 0, + ); + expect(bucket?.used).toBe(0); + } finally { + await oh.cleanup(); + } }); +}); - test.todo("skill_error outcome charges 1"); +/** + * IT-PLAYGROUND-RESERVEDAT-THREADING — route-wiring guard (#827). + * + * The route captures a single `reservedAt = new Date()` and MUST thread + * that SAME instant into BOTH `quotaService.checkAllowed({ now })` (reserve) + * AND `quotaService.chargeOnCompletion({ now })` (commit/release). Without + * it, a run that straddles a UTC month boundary reserves in one month's + * bucket and reconciles against another. The route calls `new Date()` + * internally, so the instant can't be pinned from outside — instead this + * test stubs the quota service and captures the `now` arg of each call, + * then asserts they are the SAME `Date` instance. Deleting `now:` from + * either call site makes that call default to its own `new Date()`, so the + * two captured instants diverge and this test fails. + * + * This is a pure route-wiring unit test (no DB harness): it mounts the real + * `createPlaygroundRoutes` with stubbed collaborators, mirroring the auth- + * stub technique in `src/domains/playground/routes.test.ts`. Playground-only + * is acceptable here — the generation routes have no comparable stub harness + * (their routes.test mounts the full app), so a mirror is out of scope. + */ +describe("IT-PLAYGROUND-RESERVEDAT-THREADING (route wiring #827)", () => { + test("checkAllowed and chargeOnCompletion receive the SAME reserve instant", async () => { + let reserveNow: Date | undefined; + let chargeNow: Date | undefined; + + const quotaService = { + checkAllowed: async (params: { now?: Date }) => { + reserveNow = params.now; + return { allowed: true, isAdminBypass: false } as const; + }, + chargeOnCompletion: async (params: { now?: Date }) => { + chargeNow = params.now; + }, + }; + + const chatService = { + async *chat(): AsyncGenerator { + // Complete cleanly so the route flips outcome to "success" and + // reaches the `finally` that calls chargeOnCompletion. + yield { type: "finish", finishReason: "stop" }; + }, + }; + + const llmProvidersService = { + resolveModel: async () => ({ + kind: "ok" as const, + modelId: "gpt-test", + displayName: "gpt-test", + providerId: "prov-test", + }), + }; + + const config = { + chatService, + quotaService, + llmProvidersService, + keepAliveIntervalMsResolver: async () => 15_000, + } as unknown as PlaygroundRoutesConfig; + + const app = new Hono(); + // Upstream auth stub — the real `nyxidAuthMiddleware` only READS + // `c.get("auth")`, so this survives into the route chain and satisfies + // both it and `requirePermission`. + app.use("*", async (c, next) => { + c.set( + "auth" as never, + { userId: "u-thread", permissions: ["ornn:playground:use"] } as never, + ); + await next(); + }); + app.route("/", createPlaygroundRoutes(config)); - test.todo("system_error outcome charges 0"); + // Route module mounts at `/playground/chat`; the `/api/v1` prefix is + // added in bootstrap, not here, so request the bare path. + const res = await app.request("/playground/chat", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ messages: [{ role: "user", content: "hi" }] }), + }); + expect(res.status).toBe(200); + // Drain so the producer's `finally` (chargeOnCompletion) runs. + await drainBody(res); + + // The charge fires from the producer's `finally` after the stream + // closes; poll until it's observed rather than racing a fixed sleep. + for (let i = 0; i < 200 && chargeNow === undefined; i++) { + await new Promise((r) => setTimeout(r, 5)); + } + + expect(reserveNow).toBeInstanceOf(Date); + expect(chargeNow).toBeInstanceOf(Date); + // Load-bearing: same instant, threaded from the route's single + // `reservedAt`. Strict identity — dropping `now:` from either call + // makes one default to its own `new Date()` and breaks this. + expect(chargeNow).toBe(reserveNow as Date); + expect((chargeNow as Date).getTime()).toBe((reserveNow as Date).getTime()); + }); }); diff --git a/ornn-api/tests/integration/skillgen_charge.test.ts b/ornn-api/tests/integration/skillgen_charge.test.ts index a21573a6..203f83bb 100644 --- a/ornn-api/tests/integration/skillgen_charge.test.ts +++ b/ornn-api/tests/integration/skillgen_charge.test.ts @@ -1,7 +1,17 @@ /** - * IT-SKILLGEN-QUOTA-DENY + IT-SKILLGEN-* charge path mirrors of the - * playground charge tests. Same contract — different surface and - * different route prefix. + * IT-SKILLGEN-QUOTA-DENY + IT-SKILLGEN-* charge-path mirrors of the + * playground charge tests. Same contract — different surface and route + * prefix. + * + * - At-cap user → 429 + * - Model-resolution failure (no models enabled) → 503, no bucket row + * - Successful generation → bucket.used += 1 + * - system_error generation → bucket released back to baseline + * - Abort mid-stream → bucket released back to baseline + * + * Charge-path cases inject the in-process `installLlmGatewayMock()` via + * the harness `llmClient` seam so the route → generation service → quota + * wiring runs without network IO. * * @module tests/integration/skillgen_charge.test */ @@ -9,6 +19,8 @@ import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; import { startHarness, type Harness, authHeaders } from "./harness"; import { resetCollections } from "./cleanup"; +import { installLlmGatewayMock } from "../mocks/llmGateway"; +import type { NyxLlmClient } from "../../src/clients/nyxid/llm"; let h: Harness; @@ -21,22 +33,105 @@ afterAll(async () => { }); beforeEach(async () => { - await resetCollections(h.db, ["quota_buckets", "platform_settings"]); + await resetCollections(h.db, ["quota_buckets", "platform_settings", "llm_providers"]); +}); + +const buildAuth = (userId: string) => + authHeaders({ + userId, + email: `${userId}@test.invalid`, + permissions: ["ornn:skill:build"], + }); + +/** + * Valid generated-skill JSON — passes `generatedSkillSchema` so the + * service emits `generation_complete` (success outcome) on the first + * stream pass, with no retry round-trip. + */ +const VALID_SKILL_JSON = JSON.stringify({ + name: "test-skill", + description: "A test skill generated for the integration charge test.", + category: "plain", + tags: ["test", "integration"], + readmeBody: + "This is a generated test skill body long enough to clear the fifty character minimum readme length requirement.", }); +/** Seed one provider with a model enabled for the skillGen surface. */ +async function seedSkillGenModel(db: Harness["db"], modelId: string): Promise { + const now = new Date(); + await db.collection("llm_providers").insertOne({ + _id: "prov-skillgen", + name: "test-provider", + gatewayUrl: "https://gw.test.invalid", + modelListUrl: "https://gw.test.invalid/models", + apiFormat: "responses", + auth: { kind: "apiKey", apiKeyEnc: "" }, + models: [ + { + id: modelId, + displayName: modelId, + enabledForPlayground: false, + enabledForSkillGen: true, + defaultForPlayground: false, + defaultForSkillGen: true, + removed: false, + firstSeenAt: now, + lastSyncedAt: now, + }, + ], + maxOutputTokens: 8192, + defaultTemperature: 0.7, + createdAt: now, + updatedAt: now, + updatedBy: "test", + }); +} + +const monthMarker = () => new Date().toISOString().slice(0, 7); + +async function drainBody(res: Response): Promise { + const reader = res.body?.getReader(); + if (!reader) return; + for (let done = false; !done; ) { + ({ done } = await reader.read()); + } +} + +async function waitForBucket( + db: Harness["db"], + filter: Record, + predicate: (doc: Record | null) => boolean, + { tries = 100, intervalMs = 10 } = {}, +): Promise | null> { + for (let i = 0; i < tries; i++) { + const doc = (await db.collection("quota_buckets").findOne(filter)) as + | Record + | null; + if (predicate(doc)) return doc; + await new Promise((r) => setTimeout(r, intervalMs)); + } + return (await db.collection("quota_buckets").findOne(filter)) as + | Record + | null; +} + describe("IT-SKILLGEN-QUOTA-DENY", () => { test("user at skillGen cap → 429", async () => { - const monthMarker = new Date().toISOString().slice(0, 7); + const mm = monthMarker(); + await seedSkillGenModel(h.db, "gpt-test"); + // skillGen settings default is 50 (see sections/skillGen.ts); pin + // stored allotment >= that so `used` sits at the effective cap. await h.db.collection("quota_buckets").insertOne({ - _id: `u-cap:skillGen:${monthMarker}`, + _id: `u-cap:skillGen:${mm}`, userId: "u-cap", surface: "skillGen", - monthMarker, + monthMarker: mm, monthStart: new Date(), monthEnd: new Date(), - defaultAllotment: 3, + defaultAllotment: 1000, adminGrant: 0, - used: 3, + used: 1000, usedByModel: {}, createdAt: new Date(), updatedAt: new Date(), @@ -44,20 +139,134 @@ describe("IT-SKILLGEN-QUOTA-DENY", () => { const res = await h.app.request("/api/v1/skills/generate", { method: "POST", - headers: { - ...authHeaders({ - userId: "u-cap", - email: "u-cap@test.invalid", - permissions: ["ornn:skill:build"], - }), - "Content-Type": "application/json", - }, + headers: { ...buildAuth("u-cap"), "Content-Type": "application/json" }, + body: JSON.stringify({ prompt: "hello" }), + }); + expect(res.status).toBe(429); + }); +}); + +describe("IT-SKILLGEN-MODEL-RESOLUTION-FAIL", () => { + test("no models enabled → 503 and NO bucket row for the user", async () => { + const res = await h.app.request("/api/v1/skills/generate", { + method: "POST", + headers: { ...buildAuth("u-nomodels"), "Content-Type": "application/json" }, body: JSON.stringify({ prompt: "hello" }), }); - // Accept any non-2xx — see playground_charge.test for rationale. - expect(res.status).toBeGreaterThanOrEqual(400); + expect(res.status).toBe(503); + const buckets = await h.db + .collection("quota_buckets") + .find({ userId: "u-nomodels", surface: "skillGen" }) + .toArray(); + expect(buckets.length).toBe(0); + }); +}); + +describe("IT-SKILLGEN-CHARGE-* (via injected LLM mock)", () => { + test("successful skill-gen run charges +1 on bucket.used", async () => { + const { client } = installLlmGatewayMock({ + outcome: "success", + modelId: "gpt-test", + text: VALID_SKILL_JSON, + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedSkillGenModel(oh.db, "gpt-test"); + + const res = await oh.app.request("/api/v1/skills/generate", { + method: "POST", + headers: { ...buildAuth("u-ok"), "Content-Type": "application/json" }, + body: JSON.stringify({ prompt: "build me a skill" }), + }); + expect(res.status).toBe(200); + await drainBody(res); + + const mm = monthMarker(); + const bucket = await waitForBucket( + oh.db, + { _id: `u-ok:skillGen:${mm}` }, + (d) => !!d && (d.used as number) === 1, + ); + expect(bucket?.used).toBe(1); + expect((bucket?.usedByModel as Record)["gpt-test"]).toBe(1); + } finally { + await oh.cleanup(); + } }); - test.todo("successful skill-gen run charges +1 on bucket.used"); - test.todo("system_error skill-gen run charges 0"); + test("system_error skill-gen run charges 0 (slot released)", async () => { + const { client } = installLlmGatewayMock({ + outcome: "system_error", + modelId: "gpt-test", + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedSkillGenModel(oh.db, "gpt-test"); + + const res = await oh.app.request("/api/v1/skills/generate", { + method: "POST", + headers: { ...buildAuth("u-syserr"), "Content-Type": "application/json" }, + body: JSON.stringify({ prompt: "build me a skill" }), + }); + expect(res.status).toBe(200); + await drainBody(res); + + const mm = monthMarker(); + // Reserve bumped used to 1; system_error releases it back to 0. + const bucket = await waitForBucket( + oh.db, + { _id: `u-syserr:skillGen:${mm}` }, + (d) => !!d && (d.used as number) === 0, + ); + expect(bucket?.used).toBe(0); + } finally { + await oh.cleanup(); + } + }); +}); + +describe("IT-SKILLGEN-ABORT", () => { + test("abort mid-stream releases the reserved slot (used back to baseline)", async () => { + // `delayMs` parks the mock producer on a real timer between stream + // events so the abort lands deterministically while the generator is + // suspended mid-stream — by control flow, not back-pressure luck. + const { client } = installLlmGatewayMock({ + outcome: "success", + modelId: "gpt-test", + text: VALID_SKILL_JSON, + delayMs: 50, + }); + const oh = await startHarness({ llmClient: client as NyxLlmClient }); + try { + await resetCollections(oh.db, ["quota_buckets", "platform_settings", "llm_providers"]); + await seedSkillGenModel(oh.db, "gpt-test"); + + const controller = new AbortController(); + const res = await oh.app.request("/api/v1/skills/generate", { + method: "POST", + headers: { ...buildAuth("u-abort"), "Content-Type": "application/json" }, + body: JSON.stringify({ prompt: "build me a skill" }), + signal: controller.signal, + }); + expect(res.status).toBe(200); + const reader = res.body?.getReader(); + await reader?.read().catch(() => {}); + controller.abort(); + await reader?.cancel().catch(() => {}); + + const mm = monthMarker(); + // Aborted stream → generation service never emits generation_complete + // → outcome stays system_error → slot released → used back to 0. + const bucket = await waitForBucket( + oh.db, + { _id: `u-abort:skillGen:${mm}` }, + (d) => !!d && (d.used as number) === 0, + ); + expect(bucket?.used).toBe(0); + } finally { + await oh.cleanup(); + } + }); }); diff --git a/ornn-api/tests/mocks/llmGateway.ts b/ornn-api/tests/mocks/llmGateway.ts index 0d901489..3c1d44c0 100644 --- a/ornn-api/tests/mocks/llmGateway.ts +++ b/ornn-api/tests/mocks/llmGateway.ts @@ -31,6 +31,16 @@ export interface LlmGatewayMockOptions { modelId?: string; /** Optional text to emit as a single delta before the finish event. */ text?: string; + /** + * Per-yield pause (ms) injected into `stream()` AFTER the opening + * `response.created` event and before each subsequent event. Gives the + * abort tests a deterministic window to cancel mid-stream by control + * flow rather than back-pressure luck: the producer parks on a real + * timer, so a `controller.abort()` fired after the first chunk always + * lands while the generator is suspended inside the stream. `0` (the + * default) still yields a macrotask boundary via `setTimeout(_, 0)`. + */ + delayMs?: number; } export interface LlmGatewayMockHandle { @@ -62,21 +72,31 @@ export function installLlmGatewayMock( if (cfg.outcome === "system_error") { throw new Error("LLM_TRANSPORT_FAIL: simulated upstream 5xx"); } + // Awaitable inter-event pause. Defaults to a bare macrotask boundary + // (`setTimeout(_, 0)`) so every yield cooperatively suspends the + // generator; abort tests bump `delayMs` so the producer is reliably + // parked between `response.created` and `response.completed` when + // `controller.abort()` fires. + const pause = () => + new Promise((resolve) => setTimeout(resolve, cfg.delayMs ?? 0)); yield { type: "response.created", response: { model: cfg.modelId ?? params.model }, } as ResponsesApiStreamEvent; + await pause(); if (cfg.text) { yield { type: "response.output_text.delta", delta: cfg.text, } as ResponsesApiStreamEvent; + await pause(); } if (cfg.outcome === "skill_error") { yield { type: "response.error", error: { message: "simulated skill-side failure" }, } as ResponsesApiStreamEvent; + await pause(); } yield { type: "response.completed", diff --git a/ornn-web/public/ornn-intro-poster.jpg b/ornn-web/public/ornn-intro-poster.jpg new file mode 100644 index 00000000..663fd2ae Binary files /dev/null and b/ornn-web/public/ornn-intro-poster.jpg differ diff --git a/ornn-web/public/ornn-intro.mp4 b/ornn-web/public/ornn-intro.mp4 new file mode 100644 index 00000000..023f4295 Binary files /dev/null and b/ornn-web/public/ornn-intro.mp4 differ diff --git a/ornn-web/src/pages/LandingPage.tsx b/ornn-web/src/pages/LandingPage.tsx index 61182532..161853f1 100644 --- a/ornn-web/src/pages/LandingPage.tsx +++ b/ornn-web/src/pages/LandingPage.tsx @@ -4,9 +4,13 @@ * Composition root for the editorial-forge landing. Owns the page-level * background and font; everything inside is driven by Tailwind utilities * backed by the `--color-*` and `--font-display` tokens declared in - * styles/neon.css. The hero scroll-scrub lives in `HeroStage`. + * styles/neon.css. * - * Routed bare (no RootLayout) so the 820vh sticky hero owns its scroll. + * Hero (#840): the landing now leads with `HeroVideo` — a full-bleed, + * autoplaying, muted, looping background intro video (static poster under + * reduced-motion) with a top-left CTA pair. The prior scroll-scrub + * `HeroStage` is retained in `pages/landing/` (and stays compilable) but is + * no longer mounted; it can be remounted if we revisit the scrub narrative. * * Pre-launch trim (#324): the lower marketing sections (Why / Install / * Featured / VS / Publish) are unmounted in favor of a hero-only @@ -15,7 +19,7 @@ * deleted. */ import { Navbar } from "@/components/layout/Navbar"; -import { HeroStage } from "@/pages/landing/HeroStage"; +import { HeroVideo } from "@/pages/landing/HeroVideo"; import { LandingFooter } from "@/pages/landing/LandingFooter"; import { LandingChrome } from "@/pages/landing/LandingChrome"; import { AnnouncementPopup } from "@/pages/landing/AnnouncementPopup"; @@ -32,7 +36,7 @@ export function LandingPage() { the app root in App.tsx so app-shell pages share it. */}
- +
diff --git a/ornn-web/src/pages/landing/HeroVideo.test.tsx b/ornn-web/src/pages/landing/HeroVideo.test.tsx new file mode 100644 index 00000000..7320be8c --- /dev/null +++ b/ornn-web/src/pages/landing/HeroVideo.test.tsx @@ -0,0 +1,102 @@ +/** + * HeroVideo tests (#840). + * + * Covers: + * - Motion (prefers-reduced-motion: NOT set): renders the autoplaying, + * muted, looping background