diff --git a/.changeset/linear-trims-168.md b/.changeset/linear-trims-168.md new file mode 100644 index 00000000..bcaa22a6 --- /dev/null +++ b/.changeset/linear-trims-168.md @@ -0,0 +1,11 @@ +--- +"@cloudhal/core-ts": patch +"@cloudhal/analytics": patch +"@cloudhal/astro-renderer": patch +"@cloudhal/cli": patch +"@cloudhal/comments": patch +--- + +Slug and name trims run in one linear pass instead of `/^-+|-+$/g` (CodeQL js/polynomial-redos, #168). This covers `slugify`, `foldName` and `slugifyFilename` in core-ts (the new `trimChar` is exported from `@cloudhal/core-ts/names`), heading ids in astro-renderer, and Studio's option, media and entry slugs. Results are unchanged. These trims were already linear in practice, because a run of other characters was collapsed to one `-` first. These inputs were quadratic and are linear now: analytics' `datasetName` on a long run of `_` inside a deployment name; json-render component keys in `fromJsonRender` (`./interop`); trailing whitespace in directive blocks and on comment lines; trailing `/` in request paths (the playground guard and route matching in api, analytics paths, the renderer's link localizing and redirect splats, the docs Worker). The new `trimEndChar` is exported from `@cloudhal/core-ts/names` too. + +Also in 0.6.3, with no changeset at the time (#161): Studio's "Not published" badges and its "unknown" scheduler state show their status dot again. They used a Badge variant without a dot colour and rendered no dot. diff --git a/.changeset/mcp-step-up-168.md b/.changeset/mcp-step-up-168.md new file mode 100644 index 00000000..b0ffe187 --- /dev/null +++ b/.changeset/mcp-step-up-168.md @@ -0,0 +1,6 @@ +--- +"@cloudhal/agent-runtime": patch +"@cloudhal/api": patch +--- + +MCP OAuth step-up follow-ups (RFC-0044 phase 1, #168). These stay latent until phase 2 gives connections a `grantId`. A policy denial for missing scopes now carries `missing`, the required scopes that neither the caller's scopes nor an own variant satisfy. The new `stepUpScopes(policy, principal, tool, args)` turns that into the scopes a token should ask for, using the same rules as the handler: a propose call's `proposeScopes`, the own variant the member's role holds (`content:write:own` for an Author, `content:draft:own` for a Contributor, `site:publish:own` for an Author's publish) and the member's role on the call's site (RFC-0034 A2), capped by their installation role as consent caps a token (an installation Contributor who is an Editor on one site is asked for `content:draft:own` there). A call that would still be denied with those scopes, such as a write in plan mode or a tool outside the declared skill, is not challenged. The `/mcp` step-up 403 asks for exactly that set and now leaves an audit row, like other denials. `createApi` throws when `mcp.oauth.resource` is not the public URL of `/mcp`. A playground refuses both OAuth protected-resource documents (`403 {error: "playground", block: "mcp"}`), as its route table already said. diff --git a/.changeset/rate-limits-168.md b/.changeset/rate-limits-168.md new file mode 100644 index 00000000..9c2e7eb5 --- /dev/null +++ b/.changeset/rate-limits-168.md @@ -0,0 +1,6 @@ +--- +"@cloudhal/storage": patch +"@cloudhal/api": patch +--- + +Cloudhal's own rate-limit counters move out of Better Auth's `rateLimit` table into `rate_limits` (migration `0030_rate_limits`, #168). This covers the hourly cap of 3 reset mails per address, the per-minute buckets of the public token endpoints and each agent key's window. Better Auth prunes `rateLimit` by its own 60 s window, so the hourly reset-mail cap used to restart about a minute after any sign-in from the same address. Each hit counts in one statement (an upsert with `RETURNING`). Because the keys come from client addresses and submitted email addresses, about one hit in 32 also deletes up to 500 expired counters through an `expires_at` index. The migration deletes the old `cloudhal:` rows from `rateLimit`, so each counter starts over once at upgrade. diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 88561a51..755bd1aa 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -21,9 +21,12 @@ on: permissions: {} +# A new push to a pull request cancels that pull request's running analysis. A push to main and +# the weekly run each get a group of their own (the run id): a concurrency group also cancels a +# *pending* run when another one queues, so sharing `refs/heads/main` would drop analyses of main. concurrency: - group: codeql-${{ github.ref }} - cancel-in-progress: true + group: codeql-${{ github.event_name == 'pull_request' && github.ref || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: analyze: diff --git a/apps/docs/src/worker/paths.ts b/apps/docs/src/worker/paths.ts index fa526857..fa4e7c9b 100644 --- a/apps/docs/src/worker/paths.ts +++ b/apps/docs/src/worker/paths.ts @@ -22,13 +22,15 @@ export function pageId(input: string, selfHost?: string): string | undefined { if (!HOSTS.has(url.hostname) && url.host !== selfHost) return undefined; value = url.pathname; } - const path = value + const stem = value .replace(/[?#].*$/, "") .replace(/^\/+/, "") .replace(/(^|\/)index\.md$/, "") - .replace(/\.md$/, "") - .replace(/\/+$/, "") - .toLowerCase(); + .replace(/\.md$/, ""); + // A linear trim of trailing "/" (`/\/+$/` backtracks on a long run of "/" inside the path). + let end = stem.length; + while (end > 0 && stem[end - 1] === "/") end--; + const path = stem.slice(0, end).toLowerCase(); if (path === "" || path === "index") return "index"; if (!ID.test(path) || path.split("/").some((s) => s === "." || s === "..")) return undefined; return path; diff --git a/apps/studio/src/lib/field-form.ts b/apps/studio/src/lib/field-form.ts index 08591405..e14017ff 100644 --- a/apps/studio/src/lib/field-form.ts +++ b/apps/studio/src/lib/field-form.ts @@ -5,6 +5,7 @@ */ import type { FieldDefinition } from "@cloudhal/core-contracts"; import { FIELD_LIMITS } from "@cloudhal/core-ts/fields"; +import { trimChar } from "@cloudhal/core-ts/names"; import { type FieldType, fieldKeyFrom, @@ -120,14 +121,15 @@ export function withOptionLabel( if (j !== i) return o; const value = isDefault && !o.valueEdited - ? label - .toLowerCase() - .normalize("NFKD") - .replace(/[\u0300-\u036f]/g, "") - .replace(/ı/g, "i") - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 128) + ? trimChar( + label + .toLowerCase() + .normalize("NFKD") + .replace(/[\u0300-\u036f]/g, "") + .replace(/ı/g, "i") + .replace(/[^a-z0-9]+/g, "-"), + "-", + ).slice(0, 128) : o.value; return { ...o, value, labels: { ...o.labels, [locale]: label } }; }); diff --git a/apps/studio/src/lib/media.ts b/apps/studio/src/lib/media.ts index 8ae148e3..2e58cdc0 100644 --- a/apps/studio/src/lib/media.ts +++ b/apps/studio/src/lib/media.ts @@ -15,6 +15,7 @@ import { normalizeMediaType, RASTER_TYPES, } from "@cloudhal/core-ts/media"; +import { trimChar } from "@cloudhal/core-ts/names"; import { numberFormat } from "@cloudhal/messages/format"; import { type AssetListItem, type AssetPatch, api } from "./api.ts"; import { type ImageRef, previewImageUrl } from "./seo.ts"; @@ -389,12 +390,11 @@ export function assetPatch(form: AssetForm, kind: AssetKind): AssetPatch { /** A URL slug from a file name (lower-case words joined by hyphens), for the slug field. */ export function slugify(input: string): string { - return input + const dashed = input .toLowerCase() .replace(/ı/g, "i") .normalize("NFKD") .replace(/[\u0300-\u036f]/g, "") - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, ASSET_LIMITS.slug); + .replace(/[^a-z0-9]+/g, "-"); + return trimChar(dashed, "-").slice(0, ASSET_LIMITS.slug); } diff --git a/apps/studio/src/lib/workflow.ts b/apps/studio/src/lib/workflow.ts index 8c196ce8..78d5a167 100644 --- a/apps/studio/src/lib/workflow.ts +++ b/apps/studio/src/lib/workflow.ts @@ -1,4 +1,5 @@ import type { PublishStatus } from "@cloudhal/core-contracts"; +import { trimChar } from "@cloudhal/core-ts/names"; /** Allowed editorial transitions (RFC-0001 lifecycle). */ export const TRANSITIONS: Record = { @@ -18,13 +19,10 @@ export function needsApproval(to: PublishStatus): boolean { } export function slugify(input: string): string { - return ( - input - .toLowerCase() - .normalize("NFKD") - .replace(/[̀-ͯ]/g, "") - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 128) || "untitled" - ); + const dashed = input + .toLowerCase() + .normalize("NFKD") + .replace(/[̀-ͯ]/g, "") + .replace(/[^a-z0-9]+/g, "-"); + return trimChar(dashed, "-").slice(0, 128) || "untitled"; } diff --git a/docs/adr/0012-cloudflare-deployment.md b/docs/adr/0012-cloudflare-deployment.md index a4a169d3..a3bfe8d7 100644 --- a/docs/adr/0012-cloudflare-deployment.md +++ b/docs/adr/0012-cloudflare-deployment.md @@ -50,6 +50,6 @@ Decision 6 pins the direct `miniflare` devDependency (the D1, R2 and Durable Obj - **sharp.** Every Miniflare release pins sharp exactly. 4.20260730.0, still Miniflare's npm `latest`, pins 0.35.2, and the 5.20261006.0 alpha pins 0.35.4. Both are below 0.35.5, the fix for GHSA-wq5f-xc86-pv6w (librsvg), and 0.35.2 is also below 0.35.4, the fix for GHSA-rgj7-g3m4-5g8c (libheif). Miniflare loads sharp only for its Images binding and its `cf.image` fetch mock. Cloudhal configures no Images binding and makes no `cf.image` fetches: the `/cdn-cgi/image/` URLs from `@cloudhal/astro-renderer` are resolved at the edge. `pnpm-workspace.yaml` therefore overrides `sharp@<0.35.5` to exactly 0.35.5. That version was already in the lockfile through Astro, and the calls Miniflare makes behave the same on 0.35.2, 0.35.4 and 0.35.5 (checked locally on 2026-10-25). Overrides are not published. A project that uses `@cloudhal/storage/testing` with Miniflare 4 keeps Miniflare's sharp unless it adds the same override. A new project resolves the starter's `wrangler ^4.148.0` to a release whose Miniflare pins 0.35.5. Remove the override when `pnpm why -r sharp` shows nothing below 0.35.5 without it. - **wrangler** stays at 4.148.0 in this change. 4.152 is a minor move: it changes the Miniflare under `createTestHarness` (5.20261019.2-beta), needs `@cloudflare/vite-plugin` 1.65 and `@cloudflare/workers-types` ≥ 5.20261019.1, and uploads source maps on deploy by default (4.150). It gets its own change, verified with the CLI's Cloudflare suites. (wrangler 4.152.0 landed 2026-11-08; wrangler 4.158.0 and @astrojs/cloudflare 14.4.4 follow together, because 14.4 resolves @cloudflare/vite-plugin 1.67, whose peer range is wrangler ^4.156.1.) -- **undici.** The undici versions Miniflare pins (7.28.0 in 4.20260730.0, 7.29.1 in the 5.x alpha) are accepted for local development and tests: Miniflare uses undici to reach its own workerd on localhost and, outside tests, to fetch Cloudflare's `cf.json` from workers.cloudflare.com, a trusted endpoint (set `CLOUDFLARE_CF_FETCH_ENABLED=false` to turn that off). The fixed 7.30.1 is a minor move inside `dispatchFetch`, which the D1 harness and Miniflare's binding proxy depend on (the Durable Object harness bypasses it since #116), so an override waits for its own serial test run. +- **undici.** The undici versions Miniflare pins (7.28.0 in 4.20260730.0, 7.29.1 in the 5.x alpha) are accepted for local development and tests: Miniflare uses undici to reach its own workerd on localhost and, outside tests, to fetch Cloudflare's `cf.json` from workers.cloudflare.com, a trusted endpoint (set `CLOUDFLARE_CF_FETCH_ENABLED=false` to turn that off). The fixed 7.30.1 is a minor move inside `dispatchFetch`, which the D1 harness and Miniflare's binding proxy depend on (the Durable Object harness bypasses it since #116), so an override waits for its own serial test run. *Update 2026-11-08:* that run happened. #159 (Dependabot alerts #5–#32) added the override `"undici@<7.30.1": 7.30.2` to `pnpm-workspace.yaml`, so Miniflare's pinned 7.28.0 resolves to 7.30.2 in monorepo installs; the D1 and Durable Object harnesses passed on it. Like the sharp override it is not published, and it goes when Miniflare 4 is no longer pinned. Sources: https://github.com/advisories/GHSA-wq5f-xc86-pv6w · https://github.com/advisories/GHSA-rgj7-g3m4-5g8c · https://pnpm.io/settings#overrides · `npm view miniflare dist-tags`, `npm view sharp time`, `npm view wrangler time` (checked 2026-10-25) · Miniflare 4.20260730.0 `dist/src/index.js` (`imagesLocalFetcher`, `cfImageLocalFetcher`, `setupCf`). diff --git a/docs/rfcs/0044-mcp-oauth-authorization.md b/docs/rfcs/0044-mcp-oauth-authorization.md index 776827f6..f8101ce7 100644 --- a/docs/rfcs/0044-mcp-oauth-authorization.md +++ b/docs/rfcs/0044-mcp-oauth-authorization.md @@ -100,6 +100,13 @@ not apply, cacheable for an hour: - `/.well-known/oauth-protected-resource` (root form, the fallback the 2025-11-25 and later revisions require clients to probe; Claude's docs probe it too). +Both are served without CORS headers. That matches `/mcp`, which refuses any request whose +`Origin` is not its own (DNS-rebinding protection): the clients this RFC serves are native apps +and servers, which do not need CORS, and a browser page on another origin that could read the +document still could not call `/mcp`. `McpMountOptions.oauth.resource` must be the public URL of +the mounted `/mcp` (its path ends in `/mcp`; a proxy may add a prefix): `createApi` throws +otherwise, since every token's audience is bound to it. + ```json { "resource": "https://studio.example.com/mcp", @@ -150,9 +157,19 @@ WWW-Authenticate: Bearer error="insufficient_scope", scope="site:publish", ``` This is possible without a second policy engine because each tool's scopes are static -(`toolPolicies(defineTools())`, ADR-0011): the route reads the JSON-RPC method and tool name of -the single message a stateless request carries and compares the tool's scopes with the token's. -Every other denial (role, mode, `:own`, approvals) stays a JSON-RPC result as today. Clients then +(`toolPolicies(defineTools())`, ADR-0011): the route reads the JSON-RPC method, tool name and +arguments of the single message a stateless request carries and asks the policy engine +(`stepUpScopes` in `@cloudhal/agent-runtime`) which scopes the denial lacks. The answer follows +the same rules as the handler's decision: a propose call needs its `proposeScopes`, an own +variant satisfies a tool that allows one (an Author is asked for `site:publish:own`, a +Contributor for `content:draft:own`), and the member's role is their role on the call's site +(RFC-0034 A2). Only scopes both that site role and the installation role give are asked for, +because consent caps a token by the installation role and `forSite` then caps it by the site +role: an installation Contributor who is an Editor on one site is asked for `content:draft:own` +there, not `content:write`. No challenge is sent when the call would still be denied once the +token has those scopes (plan mode, a skill that does not cover the tool). Every other denial +(role, mode, `:own`, approvals) stays a JSON-RPC result as today. The 403 leaves an audit row +like every other denial. Clients then re-authorize with the union of their scopes (the SDK's `computeScopeUnion`; the draft revision makes the union normative). A scope the role does not hold is never challenged: the client would loop. The challenge is emitted together for all scopes the tool needs ("servers SHOULD include all @@ -409,7 +426,7 @@ attestation, per-grant IP allow-lists. `better-call` 1.4.5), because the three plugins peer-depend on `^1.7.18`; `test/better-auth-pins.test.ts` keeps the family consistent. New catalog entries: `@better-auth/mcp`, `@better-auth/oauth-provider`, `@better-auth/cimd`, `jose`. -- **`packages/storage/migrations/0030_oauth.ts`**: the DDL for `oauthClient`, +- **`packages/storage/migrations/0031_oauth.ts`**: the DDL for `oauthClient`, `oauthAccessToken`, `oauthRefreshToken`, `oauthConsent`, `oauthClientAssertion`, `oauthResource`, `oauthClientResource` and `jwks`, generated with `getMigrations(auth.options).compileMigrations()` on 1.7.18 and checked in, as `0002_auth` and @@ -470,7 +487,7 @@ CREATE INDEX agent_grants_user ON agent_grants (user_id); | Phase | Ships | Amends | |---|---|---| | 1 · Discovery | `WWW-Authenticate` with `resource_metadata` and `scope` on `/mcp` 401; both PRM documents; the 403 step-up challenge; `ApiDeps.mcp.oauth` option (issuer, resource) that turns them on; the suite's discovery cases. Until Phase 2 the issuer answers 404, so clients fail at "authorization server metadata", a diagnosable error | ADR-0004 addendum: "No OAuth provider" gains "superseded by ADR-00NN (RFC-0044)" | -| 2 · Authorization server | Better Auth bump; `mcp()` + `jwt()` in `createInstance`; migration `0030_oauth`; `agent_grants`; token → `Principal`; consent screen; `/api/agent-grants`; `cloudhal studio` issuer from `--base-url` (#143); shared suite on sqlite + D1 | ADR-0010 "MCP authorization (Sprint 5)" paragraph (DCR fallback, no DPoP in v1); ADR-0011 §5–6 row "OAuth 2.1 … Sprint 6" → done; new ADR-00NN | +| 2 · Authorization server | Better Auth bump; `mcp()` + `jwt()` in `createInstance`; migration `0031_oauth`; `agent_grants`; token → `Principal`; consent screen; `/api/agent-grants`; `cloudhal studio` issuer from `--base-url` (#143); shared suite on sqlite + D1 | ADR-0010 "MCP authorization (Sprint 5)" paragraph (DCR fallback, no DPoP in v1); ADR-0011 §5–6 row "OAuth 2.1 … Sprint 6" → done; new ADR-00NN | | 3 · Registration and Studio | `cimd()` with the two transports; DCR with limits and pruning; "Connected agents"; `McpClientSetup` tab; `docs/guides/mcp-oauth.md` and the docs site's clients page; hardening guide (secret rotation, allow-list, `CLOUDHAL_MCP_DCR`); e2e test; changeset | `packages/mcp-server/README.md` auth section; `docs/guides/cloudflare-hardening.md` | Each phase is one PR; Phase 1 is independent of the Better Auth bump and can land first. @@ -577,8 +594,10 @@ sequenceDiagram `agent-runtime`, not a contract) gains the optional `grantId`. - **Agent keys:** unchanged, `x-api-key` and `Authorization: Bearer ` keep working; `resolvePrincipal` distinguishes a JWT by shape. The agent-keys suite stays green unchanged. -- **Data:** additive tables (§7). Existing deployments get the challenge on `/mcp` after Phase 1 - with no data change; an installation that never enables the AS loses nothing. +- **Data:** additive tables (§7). Phase 1 ships the challenge behind `McpMountOptions.oauth`, + and no runtime sets that option until Phase 2 gives it an issuer; existing deployments get the + challenge on `/mcp` once a runtime sets it (Phase 2), with no data change. An installation that + never enables the AS loses nothing. - **API:** `/api/agent-grants` is new; `/api/instance` `features` gains `mcpOauth: true|false` so Studio shows the "Connect by URL" tab only where it works. - **CLI:** `cloudhal studio --base-url` (#143) is the dependency; `cloudhal mcp clients create` diff --git a/packages/agent-runtime/src/index.ts b/packages/agent-runtime/src/index.ts index df4115d6..26ee5f1b 100644 --- a/packages/agent-runtime/src/index.ts +++ b/packages/agent-runtime/src/index.ts @@ -10,4 +10,5 @@ export * from "./policy.ts"; export * from "./roles.ts"; export * from "./site-access.ts"; export * from "./skills.ts"; +export * from "./step-up.ts"; export * from "./taxonomy.ts"; diff --git a/packages/agent-runtime/src/policy.ts b/packages/agent-runtime/src/policy.ts index c0feb2bf..c45dea97 100644 --- a/packages/agent-runtime/src/policy.ts +++ b/packages/agent-runtime/src/policy.ts @@ -188,6 +188,11 @@ const OWN_VARIANTS: Partial v.scope) : []; +} + /** * What a call does, decided from its arguments (RFC-0007 §10.2), for tools whose policy depends * on them (`moderate_comments` may hide in `edits` mode but never approve): @@ -307,7 +312,16 @@ export type ApprovalCause = "tool" | "destructive" | "skill" | "manual-mode" | " export type Decision = | { kind: "allow"; limit?: OwnLimit } - | { kind: "deny"; reason: string; code?: "missing_scope" | "mode" | "skill" } + | { + kind: "deny"; + reason: string; + code?: "missing_scope" | "mode" | "skill"; + /** + * `missing_scope` only: the required scopes (`proposeScopes` for a propose call) that + * neither the caller's scopes nor an own variant of them satisfy (RFC-0044 §1 step-up). + */ + missing?: Scope[]; + } | { kind: "approval-required"; reason: string; @@ -379,7 +393,12 @@ export class PolicyEngine { missing.push(s); } if (missing.length > 0) - return { kind: "deny", reason: `missing scope ${missing.join(", ")}`, code: "missing_scope" }; + return { + kind: "deny", + reason: `missing scope ${missing.join(", ")}`, + code: "missing_scope", + missing, + }; if (principal.skill) { // A skill may only call tools inside the scopes it declared (own variants count). const declared = principal.skill.scopes; diff --git a/packages/agent-runtime/src/step-up.ts b/packages/agent-runtime/src/step-up.ts new file mode 100644 index 00000000..e6c7c637 --- /dev/null +++ b/packages/agent-runtime/src/step-up.ts @@ -0,0 +1,64 @@ +/** + * RFC-0044 §1 step-up: which scopes an OAuth connection must add for a call it was denied. The + * answer comes from the same decision the MCP handler makes (`PolicyEngine.evaluate`), so it + * follows the own-variant lattice, a propose call's `proposeScopes`, and the member's role on the + * call's site (RFC-0034 A2), not their installation role. + */ +import { + holds, + isKeyScope, + ownVariantsOf, + type PolicyEngine, + type Principal, + type Scope, +} from "./policy.ts"; +import { narrow, roleScopes } from "./roles.ts"; +import { forInstallation, forSite, INSTALLATION_SCOPES, isSiteLimited } from "./site-access.ts"; + +/** + * The scopes to ask for so that `principal` may make the call `toolName(args)`, each the + * strongest one a token can hold on the call's site: given by the member's role there and by + * their installation role, which caps what consent issues (`site:publish:own` for an Author, not + * `site:publish`). Undefined when the call is not denied for a missing scope, when the principal + * has no person behind it, when the roles cannot give every missing scope, or when the call + * would still be denied with them (plan mode, a skill that does not cover it): a challenge would + * then make the client re-authorize for nothing, or loop. + */ +export function stepUpScopes( + policy: PolicyEngine, + principal: Principal, + toolName: string, + args: Record = {}, +): Scope[] | undefined { + const tool = policy.tool(toolName); + if (!tool || !principal.person) return undefined; + // The handler judges the call as the principal acts on its site (`handleToolCall`). + const siteId = typeof args.siteId === "string" ? args.siteId : undefined; + const onSite = (p: Principal) => (siteId ? forSite(p, siteId) : forInstallation(p)); + const sited = onSite(principal); + const person = sited?.person; + if (!sited || !person) return undefined; + const decision = policy.evaluate(sited, toolName, { args }); + if (decision.kind !== "deny" || decision.code !== "missing_scope" || !decision.missing?.length) + return undefined; + // Consent caps a token by the installation role, as key creation does; `forSite` then caps it + // by the site role. Only scopes both give survive, so only those are asked for. + const installation = roleScopes(principal.person.role); + const both = narrow(roleScopes(person.role), installation); + const grantable = + !siteId && isSiteLimited(person) ? both.filter((s) => !INSTALLATION_SCOPES.includes(s)) : both; + const asked: Scope[] = []; + for (const scope of decision.missing) { + const pick = [scope, ...ownVariantsOf(tool, scope)].find( + (s) => isKeyScope(s) && holds(grantable, s), + ); + if (!pick) return undefined; + if (!asked.includes(pick)) asked.push(pick); + } + // The call as it would run after consent must not be denied for any reason (scope, mode, + // skill): otherwise the client would re-authorize for nothing, or loop. + const granted = narrow([...(principal.keyScopes ?? principal.scopes), ...asked], installation); + const after = onSite({ ...principal, scopes: granted, keyScopes: granted }); + if (!after) return undefined; + return policy.evaluate(after, toolName, { args }).kind === "deny" ? undefined : asked; +} diff --git a/packages/agent-runtime/test/policy.test.ts b/packages/agent-runtime/test/policy.test.ts index ed1da778..be9f04a1 100644 --- a/packages/agent-runtime/test/policy.test.ts +++ b/packages/agent-runtime/test/policy.test.ts @@ -217,7 +217,12 @@ describe("PolicyEngine", () => { expect(implies("content:write", "content:propose")).toBe(true); expect( body.evaluate({ actor: agent, scopes: ["content:read"] }, "edit_body", propose), - ).toEqual({ kind: "deny", reason: "missing scope content:propose", code: "missing_scope" }); + ).toEqual({ + kind: "deny", + reason: "missing scope content:propose", + code: "missing_scope", + missing: ["content:propose"], + }); // A skill must declare what the call needs. const skilled = { ...planKey, diff --git a/packages/agent-runtime/test/step-up.test.ts b/packages/agent-runtime/test/step-up.test.ts new file mode 100644 index 00000000..28f22002 --- /dev/null +++ b/packages/agent-runtime/test/step-up.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it } from "vitest"; +import { scopesForMode } from "../src/modes.ts"; +import { type PersonRef, PolicyEngine, type Principal, type Scope } from "../src/policy.ts"; +import { narrow, type Role, roleScopes } from "../src/roles.ts"; +import { forSite } from "../src/site-access.ts"; +import { stepUpScopes } from "../src/step-up.ts"; + +const engine = new PolicyEngine([ + { + name: "update_entry", + scopes: ["content:write"], + annotations: { readOnlyHint: false }, + own: "entry", + }, + { + name: "update_page", + scopes: ["content:write"], + annotations: { readOnlyHint: false }, + }, + { + name: "publish", + scopes: ["site:publish"], + annotations: { destructiveHint: true }, + requiresApproval: true, + own: "changeset", + }, + { + name: "edit_body", + scopes: ["content:write"], + annotations: { readOnlyHint: false }, + own: "entry", + classify: (a) => (a.mode === "propose" ? "propose" : undefined), + proposeScopes: ["content:read", "content:propose"], + }, + { name: "audit_log", scopes: ["audit:read"], annotations: { readOnlyHint: true } }, +]); + +/** A token principal of RFC-0044 phase 2: an agent with a grant, its scopes the token's. */ +const grant = (role: Role, scopes: Scope[], sites?: PersonRef["sites"]): Principal => ({ + actor: { kind: "agent", id: "agent_grant0001" }, + scopes, + keyScopes: scopes, + mode: "edits", + grantId: "grant_0001", + person: { id: "user_aaaaaaaa", userId: "u1", role, ...(sites ? { sites } : {}) }, +}); +const READ: Scope[] = ["site:read", "content:read"]; + +describe("stepUpScopes (RFC-0044 §1)", () => { + it("reports what the denial lacks, from the policy engine", () => { + expect(engine.evaluate(grant("editor", READ), "update_entry")).toMatchObject({ + kind: "deny", + code: "missing_scope", + missing: ["content:write"], + }); + expect(stepUpScopes(engine, grant("editor", READ), "update_entry")).toEqual(["content:write"]); + expect(stepUpScopes(engine, grant("editor", READ), "publish")).toEqual(["site:publish"]); + }); + + it("asks an Author for the own variant their role holds", () => { + expect(stepUpScopes(engine, grant("author", READ), "update_entry")).toEqual([ + "content:write:own", + ]); + expect(stepUpScopes(engine, grant("author", READ), "publish")).toEqual(["site:publish:own"]); + // A tool without `own` is not satisfied by an own variant: no challenge, no loop. + expect(stepUpScopes(engine, grant("author", READ), "update_page")).toBeUndefined(); + }); + + it("asks a Contributor for the draft variant, and nothing their role lacks", () => { + expect(stepUpScopes(engine, grant("contributor", READ), "update_entry")).toEqual([ + "content:draft:own", + ]); + expect(stepUpScopes(engine, grant("contributor", READ), "publish")).toBeUndefined(); + }); + + it("asks a propose call for proposeScopes, not the write it proposes", () => { + const propose = { mode: "propose" }; + expect(stepUpScopes(engine, grant("editor", ["site:read"]), "edit_body", propose)).toEqual([ + "content:read", + "content:propose", + ]); + // The same tool applying the change needs the write. + expect(stepUpScopes(engine, grant("editor", READ), "edit_body", {})).toEqual(["content:write"]); + }); + + it("judges the member's role on the call's site, not the installation role", () => { + const limited = grant("editor", READ, { site_a: "contributor", site_b: "editor" }); + expect(stepUpScopes(engine, limited, "update_entry", { siteId: "site_a" })).toEqual([ + "content:draft:own", + ]); + expect(stepUpScopes(engine, limited, "update_entry", { siteId: "site_b" })).toEqual([ + "content:write", + ]); + // The installation role (Editor) holds site:publish; the Contributor on site_a does not. + expect(stepUpScopes(engine, limited, "publish", { siteId: "site_a" })).toBeUndefined(); + // A site the member cannot open: the handler answers not_found, never a challenge. + expect(stepUpScopes(engine, limited, "update_entry", { siteId: "site_c" })).toBeUndefined(); + }); + + it("asks only for what both the site role and the installation role give (no loop)", () => { + // An installation Contributor who is an Editor on site_b: consent caps the token at the + // Contributor's scopes, so asking for content:write would come back as content:draft:own. + const contributor = grant("contributor", READ, { site_b: "editor" }); + const args = { siteId: "site_b" }; + const asked = stepUpScopes(engine, contributor, "update_entry", args); + expect(asked).toEqual(["content:draft:own"]); + // After consent (capped by the installation role) the call runs and nothing is asked again. + const granted = narrow([...READ, ...(asked ?? [])], roleScopes("contributor")); + const after = { ...contributor, scopes: granted, keyScopes: granted }; + expect(stepUpScopes(engine, after, "update_entry", args)).toBeUndefined(); + const onSite = forSite(after, "site_b"); + expect(onSite && engine.evaluate(onSite, "update_entry", { args })).toMatchObject({ + kind: "allow", + limit: { level: "draft" }, + }); + // Publishing is neither role's to give on that site through a Contributor's consent. + expect(stepUpScopes(engine, contributor, "publish", args)).toBeUndefined(); + }); + + it("never challenges a call that would still be denied after consent", () => { + // Plan mode: the token lacks content:write, but plan mode refuses the write anyway. + const plan = { ...grant("editor", scopesForMode("plan")), mode: "plan" as const }; + expect(engine.evaluate(plan, "update_entry")).toMatchObject({ code: "missing_scope" }); + expect(stepUpScopes(engine, plan, "update_entry")).toBeUndefined(); + // ... while a propose call, which plan mode may make, is still challenged when it lacks scopes. + expect( + stepUpScopes( + engine, + { ...plan, scopes: ["site:read"], keyScopes: ["site:read"] }, + "edit_body", + { + mode: "propose", + }, + ), + ).toEqual(["content:read", "content:propose"]); + // A declared skill that does not cover the tool's scope: the skill check would refuse it. + const skilled = { + ...grant("editor", READ), + skill: { name: "reader", scopes: ["site:read", "content:read"], approval: [] }, + }; + expect(stepUpScopes(engine, skilled, "update_entry")).toBeUndefined(); + const covering = { ...skilled, skill: { ...skilled.skill, scopes: ["content:write"] } }; + expect(stepUpScopes(engine, covering, "update_entry")).toEqual(["content:write"]); + }); + + it("never asks for a person-only scope, and never for a call that is not a scope denial", () => { + expect(stepUpScopes(engine, grant("administrator", READ), "audit_log")).toBeUndefined(); + const writer = grant("editor", [...READ, "content:write"]); + expect(stepUpScopes(engine, writer, "update_entry")).toBeUndefined(); + expect(stepUpScopes(engine, { ...writer, mode: "plan" }, "update_entry")).toBeUndefined(); + expect(stepUpScopes(engine, grant("editor", READ), "no_such_tool")).toBeUndefined(); + const { person: _, ...anonymous } = grant("editor", READ); + expect(stepUpScopes(engine, anonymous, "update_entry")).toBeUndefined(); + }); +}); diff --git a/packages/analytics/src/paths.ts b/packages/analytics/src/paths.ts index 4d08a145..f2a773f0 100644 --- a/packages/analytics/src/paths.ts +++ b/packages/analytics/src/paths.ts @@ -27,7 +27,10 @@ export function normaliseBeaconPath(raw: string): string | undefined { return undefined; } // no query split after decoding: an encoded "?" is part of the path, and the route pattern refuses it - const trimmed = decoded.length > 1 ? decoded.replace(/\/+$/, "") || "/" : decoded; + // A linear trim of trailing "/" (`/\/+$/` backtracks on a long run of "/" inside the path). + let end = decoded.length; + while (end > 1 && decoded[end - 1] === "/") end--; + const trimmed = decoded.slice(0, end); const path = trimmed.toLowerCase(); return ROUTE_PATH_RE.test(path) ? path : undefined; } diff --git a/packages/analytics/src/wae/index.ts b/packages/analytics/src/wae/index.ts index 68239f54..ddfa9d01 100644 --- a/packages/analytics/src/wae/index.ts +++ b/packages/analytics/src/wae/index.ts @@ -24,9 +24,13 @@ export { WaeWriter } from "./writer.ts"; /** The dataset name for a deployment: `_events`, `-` → `_`, lower case (RFC-0006 §4). */ export function datasetName(deployment: string): string { - const base = deployment - .toLowerCase() - .replace(/[^a-z0-9_]+/g, "_") - .replace(/^_+|_+$/g, ""); + const underscored = deployment.toLowerCase().replace(/[^a-z0-9_]+/g, "_"); + // Linear trim: `/^_+|_+$/g` backtracks on a long run of `_` inside the name (CodeQL + // js/polynomial-redos), and the replace above keeps runs the name already had. + let start = 0; + let end = underscored.length; + while (start < end && underscored[start] === "_") start++; + while (end > start && underscored[end - 1] === "_") end--; + const base = underscored.slice(start, end); return `${base || "cloudhal"}_events`; } diff --git a/packages/analytics/test/wae-reader.test.ts b/packages/analytics/test/wae-reader.test.ts index e6f3a32e..7f936ec0 100644 --- a/packages/analytics/test/wae-reader.test.ts +++ b/packages/analytics/test/wae-reader.test.ts @@ -167,5 +167,16 @@ describe("WaeReader", () => { it("names datasets after the deployment", () => { expect(datasetName("newsroom")).toBe("newsroom_events"); expect(datasetName("My-Site 2")).toBe("my_site_2_events"); + expect(datasetName("__news__room__")).toBe("news__room_events"); + expect(datasetName("___")).toBe("cloudhal_events"); + }); + + it("trims a dataset name in linear time (#168, CodeQL js/polynomial-redos)", () => { + // `/^_+|_+$/g` took about 4 s on a 100k run of `_` inside the name (the replace before it + // keeps such runs); the loop takes well under a millisecond. + const name = `a${"_".repeat(200_000)}b`; + const start = performance.now(); + expect(datasetName(name)).toBe(`${name}_events`); + expect(performance.now() - start).toBeLessThan(1500); }); }); diff --git a/packages/api/src/auth/principal.ts b/packages/api/src/auth/principal.ts index 7d3e9b2a..1ea7b4c6 100644 --- a/packages/api/src/auth/principal.ts +++ b/packages/api/src/auth/principal.ts @@ -31,7 +31,7 @@ export function createPrincipalResolver(d: AuthDeps): PrincipalResolver { if (key) { const verified = await api.verifyApiKey({ body: { key } }); if (!verified.valid || !verified.key) return undefined; - // A fixed window per key (`rateLimit` table): it resets every window however busy the key is. + // A fixed window per key (`rate_limits` table): it resets every window however busy the key is. if ( !(await staffStore.hit(`cloudhal:key:${verified.key.id}`, keyLimit.windowMs, keyLimit.max)) ) diff --git a/packages/api/src/mcp-mount.ts b/packages/api/src/mcp-mount.ts index c7cd7fcc..5ca2600d 100644 --- a/packages/api/src/mcp-mount.ts +++ b/packages/api/src/mcp-mount.ts @@ -10,6 +10,8 @@ import type { BuildHub } from "./build.ts"; import type { ApiDeps, Env } from "./context.ts"; import { unauthorized } from "./errors.ts"; import { + assertMcpOauth, + MCP_PATH, mountProtectedResourceMetadata, stepUpChallenge, unauthorizedChallenge, @@ -38,8 +40,11 @@ export function mountMcp( schedules: parts.schedules, }); const oauth = deps.mcp.oauth; - if (oauth) mountProtectedResourceMetadata(app, oauth); - app.all("/mcp", async (c) => { + if (oauth) { + assertMcpOauth(oauth); + mountProtectedResourceMetadata(app, oauth); + } + app.all(MCP_PATH, async (c) => { // DNS-rebinding / cross-site protection: browsers send Origin; it must be this origin. const origin = c.req.header("origin"); if (origin && origin !== new URL(c.req.url).origin) @@ -59,7 +64,14 @@ export function mountMcp( 403, ); if (oauth) { - const stepUp = await stepUpChallenge(c.req.raw, principal, c.get("policy")(), oauth); + const stepUp = await stepUpChallenge({ + request: c.req.raw, + principal, + policy: c.get("policy")(), + oauth, + audit: deps.mcp.audit, + now: deps.now, + }); if (stepUp) return stepUp; } return mcp.fetch(c.req.raw, principal); diff --git a/packages/api/src/mcp-oauth.ts b/packages/api/src/mcp-oauth.ts index ea826282..1acd4430 100644 --- a/packages/api/src/mcp-oauth.ts +++ b/packages/api/src/mcp-oauth.ts @@ -10,13 +10,13 @@ * exist, so clients fail at "authorization server metadata", a diagnosable error (RFC-0044 §9). */ import { - holds, - isKeyScope, + type AuditLog, + effectiveMode, type PolicyEngine, type Principal, - roleScopes, type Scope, scopesForMode, + stepUpScopes, } from "@cloudhal/agent-runtime"; import type { Context, Hono } from "hono"; import type { Env } from "./context.ts"; @@ -53,6 +53,22 @@ export function canonicalResource(resource: string): string { return url.href.replace(/\/$/, ""); } +/** The path `createApi` mounts the MCP endpoint at (ADR-0011). */ +export const MCP_PATH = "/mcp"; + +/** + * Fails `createApi` when `resource` cannot name this endpoint: tokens are bound to it (RFC 8707), + * so a resource that does not end in `/mcp` would make every token's audience wrong. A proxy may + * put a prefix in front (`https://example.com/studio/mcp`). + */ +export function assertMcpOauth(oauth: McpOauthOptions): void { + const resource = new URL(canonicalResource(oauth.resource)); + if (!resource.pathname.endsWith(MCP_PATH) || resource.search) + throw new Error( + `mcp.oauth.resource must be the public URL of ${MCP_PATH} (e.g. https://studio.example.com${MCP_PATH}), got ${oauth.resource}`, + ); +} + /** The URL of the path-aware metadata document, named in every challenge. */ export function resourceMetadataUrl(oauth: McpOauthOptions): string { const url = new URL(canonicalResource(oauth.resource)); @@ -156,30 +172,42 @@ async function toolCallOf( return { name, args: args && typeof args === "object" ? (args as Record) : {} }; } +export interface StepUpOptions { + readonly request: Request; + readonly principal: Principal; + readonly policy: PolicyEngine; + readonly oauth: McpOauthOptions; + /** Where the step-up 403 is recorded, like every other denial (`runTool`). */ + readonly audit?: AuditLog | undefined; + readonly now?: (() => Date) | undefined; +} + /** * RFC-0044 §1 step-up: a token principal (`grantId`) calling a tool whose scopes the token lacks - * but the member's role holds gets `403 insufficient_scope` naming every missing scope at once, - * so the client re-authorizes with the union (MCP "Scope Challenge Handling"). Every other denial - * (role, mode, `:own`, approvals) stays a JSON-RPC result from the handler, and a key principal - * is never challenged: a key cannot re-authorize. + * but the member's role on the call's site gives gets `403 insufficient_scope` naming every + * missing scope at once, so the client re-authorizes with the union (MCP "Scope Challenge + * Handling"). The set is the policy engine's (`stepUpScopes`): own variants, a propose call's + * scopes, the site role. Every other denial (role, mode, `:own`, approvals) stays a JSON-RPC + * result from the handler, and a key principal is never challenged: a key cannot re-authorize. */ -export async function stepUpChallenge( - request: Request, - principal: Principal, - policy: PolicyEngine, - oauth: McpOauthOptions, -): Promise { +export async function stepUpChallenge(options: StepUpOptions): Promise { + const { request, principal, policy, oauth } = options; if (!principal.grantId || !principal.person) return undefined; const call = await toolCallOf(request); if (!call) return undefined; - const tool = policy.tool(call.name); - if (!tool) return undefined; - const decision = policy.evaluate(principal, call.name, { args: call.args }); - if (decision.kind !== "deny" || decision.code !== "missing_scope") return undefined; - const missing = tool.scopes.filter((s) => !holds(principal.scopes, s)); - const role = roleScopes(principal.person.role); - // Only scopes a token can carry and the role holds: anything else would make the client loop. - if (!missing.length || !missing.every((s) => isKeyScope(s) && holds(role, s))) return undefined; + const missing = stepUpScopes(policy, principal, call.name, call.args); + if (!missing) return undefined; + const reason = `insufficient_scope: this connection needs ${missing.join(", ")}`; + await options.audit?.record({ + at: (options.now?.() ?? new Date()).toISOString(), + actor: principal.actor, + siteId: typeof call.args.siteId === "string" ? call.args.siteId : undefined, + tool: call.name, + decision: "deny", + reason, + mode: effectiveMode(principal), + outcome: "error", + }); return Response.json( { error: "insufficient_scope", diff --git a/packages/api/src/playground.ts b/packages/api/src/playground.ts index 17e033f1..79779865 100644 --- a/packages/api/src/playground.ts +++ b/packages/api/src/playground.ts @@ -8,6 +8,7 @@ * refused as `unclassified`, so a new route is never silently open in a playground. */ import { type Principal, ROLE_SCOPES, type Scope } from "@cloudhal/agent-runtime"; +import { trimEndChar } from "@cloudhal/core-ts/names"; import { createMiddleware } from "hono/factory"; import type { Env } from "./context.ts"; import { matchesRoute, routeFor } from "./scopes.ts"; @@ -211,14 +212,19 @@ export function playgroundDenyRule(method: string, pattern: string): PlaygroundB return undefined; } +/** The API's well-known documents (RFC-0044 §1): judged like `/api/*`, so their rows apply. */ +const WELL_KNOWN_API = "/.well-known/oauth-protected-resource"; + /** * Why `method path` is refused in playground mode, or undefined when it is allowed. Only API - * paths (`/api/*`, `/mcp`) are judged; anything else (Studio, the rendered site) is not the API's. + * paths (`/api/*`, `/mcp`, the API's well-known documents) are judged; anything else (Studio, + * the rendered site) is not the API's. */ export function isBlockedInPlayground(method: string, path: string): PlaygroundBlock | undefined { - const clean = path.replace(/\/+$/, "") || "/"; + const clean = trimEndChar(path, "/") || "/"; if (clean === "/mcp" || clean.startsWith("/mcp/")) return "mcp"; - if (!clean.startsWith("/api/")) return undefined; + const wellKnown = clean === WELL_KNOWN_API || clean.startsWith(`${WELL_KNOWN_API}/`); + if (!clean.startsWith("/api/") && !wellKnown) return undefined; const m = method.toUpperCase() === "HEAD" ? "GET" : method.toUpperCase(); // Prefix rules first, so an undeclared path under a blocked area names its area. const route = routeFor(m, clean); diff --git a/packages/api/src/rate-limits.ts b/packages/api/src/rate-limits.ts new file mode 100644 index 00000000..950ae685 --- /dev/null +++ b/packages/api/src/rate-limits.ts @@ -0,0 +1,45 @@ +/** + * Cloudhal's own fixed-window counters (#168 item 8), in `rate_limits` (migration 0030) rather + * than Better Auth's `rateLimit`, which Better Auth prunes by its own 60 s window. + * + * Counting is one statement (an upsert with `RETURNING`), so two hits never read each other's + * half-done window. The table is pruned here, because nothing else does and its keys come from + * client addresses and submitted email addresses: about one hit in `PRUNE_EVERY` also deletes up + * to `PRUNE_LIMIT` expired rows through the `expires_at` index. The prune is a statement of its + * own (a `batch()` returns no rows, so it cannot carry the count), and it may run before or after + * any hit: it only removes rows whose window has ended, which the next hit on that key would + * reset to 1 anyway. + */ +import type { SqlDriver } from "@cloudhal/storage"; + +/** On average one hit in this many prunes expired counters. */ +export const PRUNE_EVERY = 32; +/** The most expired rows one prune deletes (D1 prefers short statements). */ +export const PRUNE_LIMIT = 500; + +/** The default prune decision: random, so per-request instances (Workers) share the cost. */ +export const randomPrune = (): boolean => Math.random() < 1 / PRUNE_EVERY; + +/** Counts one hit on `key`; the result is the count in the current window, this hit included. */ +export async function countHit( + sql: SqlDriver, + hit: { key: string; windowMs: number; now: number }, +): Promise { + const { key, windowMs, now } = hit; + // In an UPDATE every column reference is the row before the update (SQLite), so the three + // CASEs agree on whether the window has ended. + const row = await sql.get<{ count: number }>( + "INSERT INTO rate_limits (key, count, window_start, expires_at) VALUES (?, 1, ?, ?) ON CONFLICT (key) DO UPDATE SET count = CASE WHEN rate_limits.expires_at < ? THEN 1 ELSE rate_limits.count + 1 END, window_start = CASE WHEN rate_limits.expires_at < ? THEN excluded.window_start ELSE rate_limits.window_start END, expires_at = CASE WHEN rate_limits.expires_at < ? THEN excluded.expires_at ELSE rate_limits.expires_at END RETURNING count", + [key, now, now + windowMs, now, now, now], + ); + return Number(row?.count ?? 0); +} + +/** Deletes up to `PRUNE_LIMIT` counters whose window ended before `now`; returns how many. */ +export async function pruneCounters(sql: SqlDriver, now: number): Promise { + const r = await sql.run( + "DELETE FROM rate_limits WHERE rowid IN (SELECT rowid FROM rate_limits WHERE expires_at < ? LIMIT ?)", + [now, PRUNE_LIMIT], + ); + return r.changes; +} diff --git a/packages/api/src/scopes.ts b/packages/api/src/scopes.ts index 33848264..c850b4f1 100644 --- a/packages/api/src/scopes.ts +++ b/packages/api/src/scopes.ts @@ -14,6 +14,7 @@ * Agents (API keys) are refused everywhere except `public`/`signed-in`: they work through MCP. */ import { forInstallation, forSite, holds, type Scope } from "@cloudhal/agent-runtime"; +import { trimEndChar } from "@cloudhal/core-ts/names"; import { createMiddleware } from "hono/factory"; import type { Env } from "./context.ts"; import { apiError, missingScope, unauthorized } from "./errors.ts"; @@ -342,7 +343,7 @@ export const ROUTE_SCOPES: readonly (readonly [Method, string, RouteRequirement] /** Whether `path` fits a `ROUTE_SCOPES` pattern (`:param` one segment, `*` the rest). */ export function matchesRoute(pattern: string, path: string): boolean { const p = pattern.split("/"); - const s = path.replace(/\/+$/, "").split("/"); + const s = trimEndChar(path, "/").split("/"); for (let i = 0; i < p.length; i++) { const part = p[i] as string; if (part === "*") return s.length > i; diff --git a/packages/api/src/staff.ts b/packages/api/src/staff.ts index 6cdf50d0..89b81470 100644 --- a/packages/api/src/staff.ts +++ b/packages/api/src/staff.ts @@ -9,6 +9,7 @@ */ import { isRole, type Role } from "@cloudhal/agent-runtime"; import type { SqlDriver } from "@cloudhal/storage"; +import { countHit, pruneCounters, randomPrune } from "./rate-limits.ts"; import { allSiteRoles, inviteSiteRoles, @@ -91,10 +92,13 @@ function toMember(r: Row): MemberRow | undefined { export class StaffStore { readonly #sql: SqlDriver; readonly #now: () => Date; + readonly #prune: () => boolean; - constructor(sql: SqlDriver, now: () => Date = () => new Date()) { + /** `prune` decides per hit whether to delete expired rate-limit counters (tests pin it). */ + constructor(sql: SqlDriver, now: () => Date = () => new Date(), prune = randomPrune) { this.#sql = sql; this.#now = now; + this.#prune = prune; } // ───────────────────────────── members ───────────────────────────── @@ -534,21 +538,15 @@ export class StaffStore { // ───────────────────────────── rate limits ───────────────────────────── /** - * Fixed-window counter in Better Auth's `rateLimit` table (migration 0005), under keys of our own - * (`cloudhal:…`). Returns false once `max` requests were seen within `windowMs`. + * Fixed-window counter under keys of our own (`cloudhal:…`) in `rate_limits` (`./rate-limits.ts`, + * #168): one atomic upsert per hit, and now and then a prune of expired counters. Returns false + * once `max` requests were seen within `windowMs`. */ async hit(key: string, windowMs: number, max: number): Promise { const now = this.#now().getTime(); - const start = now - windowMs; - await this.#sql.run( - `INSERT INTO "rateLimit" ("id", "key", "count", "lastRequest") VALUES (?, ?, 1, ?) ON CONFLICT ("key") DO UPDATE SET "count" = CASE WHEN "rateLimit"."lastRequest" < ? THEN 1 ELSE "rateLimit"."count" + 1 END, "lastRequest" = CASE WHEN "rateLimit"."lastRequest" < ? THEN excluded."lastRequest" ELSE "rateLimit"."lastRequest" END`, - [newRowId(), key, now, start, start], - ); - const row = await this.#sql.get<{ count: number }>( - `SELECT "count" FROM "rateLimit" WHERE "key" = ?`, - [key], - ); - return Number(row?.count ?? 0) <= max; + const count = await countHit(this.#sql, { key, windowMs, now }); + if (this.#prune()) await pruneCounters(this.#sql, now); + return count <= max; } // ───────────────────────────── helpers ───────────────────────────── diff --git a/packages/api/src/testing-mcp-oauth.ts b/packages/api/src/testing-mcp-oauth.ts index 1eccd049..6dfddbfc 100644 --- a/packages/api/src/testing-mcp-oauth.ts +++ b/packages/api/src/testing-mcp-oauth.ts @@ -5,7 +5,7 @@ * better-sqlite3 and Miniflare D1 (the step-up's key-principal case reads the key from the * database). */ -import { type Principal, scopesForMode } from "@cloudhal/agent-runtime"; +import { AuditLog, type PersonRef, type Principal, scopesForMode } from "@cloudhal/agent-runtime"; import { seedStarterSite } from "@cloudhal/core-ts"; import { afterAll, describe, expect, it } from "vitest"; import { createApi } from "./app.ts"; @@ -41,6 +41,8 @@ function toolCall(name: string, args: Json): RequestInit { }; } +const entryArgs = (siteId: string): Json => ({ siteId, slug: "x", title: "X", body: "x" }); + export function describeMcpOauthDiscovery(label: string, backend: RolesBackend): void { describe(`MCP OAuth discovery: ${label}`, { timeout: 180_000 }, () => { afterAll(async () => { @@ -84,29 +86,52 @@ export function describeMcpOauthDiscovery(label: string, backend: RolesBackend): }; /** A deployment whose principal is injected: a token principal of Phase 2, before Phase 2. */ - const setupWithPrincipal = async (principal: Principal | undefined) => { + const setupWithPrincipal = async ( + principal: Principal | undefined | ((siteId: string) => Principal), + ) => { const b = await backend.create(); const site = await seedStarterSite(b.store, { name: "Blog", slug: "blog" }); + const audit = new AuditLog(b.sql); + const resolved = typeof principal === "function" ? principal(site.id) : principal; const deps: ApiDeps = { store: b.store, - resolvePrincipal: async () => principal, - mcp: { stateKey: "mcp-oauth-state-key-0123456789abcdefghij", oauth: OAUTH }, + resolvePrincipal: async () => resolved, + mcp: { stateKey: "mcp-oauth-state-key-0123456789abcdefghij", oauth: OAUTH, audit }, }; - return { app: createApi(deps), siteId: site.id }; + return { app: createApi(deps), siteId: site.id, audit }; }; const grantPrincipal = ( - role: "editor" | "contributor", + role: PersonRef["role"], scopes: Principal["scopes"], + sites?: PersonRef["sites"], ): Principal => ({ actor: { kind: "agent", id: "agent_grant0001", name: "Claude" }, scopes, mode: "edits", grantId: "grant_0001", keyScopes: scopes, - person: { id: "user_owner1", userId: "owner1", role, name: "Olive Owner" }, + person: { + id: "user_owner1", + userId: "owner1", + role, + name: "Olive Owner", + ...(sites ? { sites } : {}), + }, }); + /** The scopes a step-up 403 names, or undefined when the call was not challenged. */ + const challenged = async ( + principal: Principal | ((siteId: string) => Principal), + tool: string, + args: (siteId: string) => Json, + ) => { + const { app, siteId } = await setupWithPrincipal(principal); + const res = await app.request(`${ORIGIN}/mcp`, toolCall(tool, args(siteId))); + if (res.status !== 403) return undefined; + return ((await res.json()) as { details: { scope: string[] } }).details.scope; + }; + it("answers 401 with the RFC 9728 challenge and the scopes to request", async () => { const { app } = await setup(OAUTH); const res = await app.request(`${ORIGIN}/mcp`, toolCall("get_mode", {})); @@ -187,6 +212,98 @@ export function describeMcpOauthDiscovery(label: string, backend: RolesBackend): expect(reached.headers.get("www-authenticate")).toBeNull(); }); + it("asks for the own variant an Author's or Contributor's role holds", async () => { + const read: Principal["scopes"] = ["site:read", "content:read"]; + expect(await challenged(grantPrincipal("author", read), "create_entry", entryArgs)).toEqual([ + "content:write:own", + ]); + expect( + await challenged(grantPrincipal("contributor", read), "create_entry", entryArgs), + ).toEqual(["content:draft:own"]); + expect( + await challenged(grantPrincipal("author", read), "publish", (siteId) => ({ siteId })), + ).toEqual(["site:publish:own"]); + }); + + it("asks a propose call for the propose scopes, not the write", async () => { + const propose = (siteId: string) => ({ siteId, entryId: "e1", ops: [], mode: "propose" }); + expect( + await challenged(grantPrincipal("editor", ["site:read"]), "edit_body", propose), + ).toEqual(["content:read", "content:propose"]); + }); + + it("judges the member's role on the call's site, not the installation role", async () => { + // An Editor of the installation who is a Contributor on this site: no publish to ask for, + // and the draft variant (not content:write) for a new entry. + const read: Principal["scopes"] = ["site:read", "content:read"]; + const limited = (siteId: string) => + grantPrincipal("editor", read, { [siteId]: "contributor" }); + const { app, siteId } = await setupWithPrincipal(limited); + const res = await app.request(`${ORIGIN}/mcp`, toolCall("publish", { siteId })); + expect(res.status).not.toBe(403); + expect(res.headers.get("www-authenticate")).toBeNull(); + const entry = await app.request(`${ORIGIN}/mcp`, toolCall("create_entry", entryArgs(siteId))); + expect(entry.status).toBe(403); + expect(await entry.json()).toMatchObject({ details: { scope: ["content:draft:own"] } }); + // An installation Contributor who is an Editor here: consent caps the token at the + // Contributor's scopes, so the challenge names what it can issue (content:draft:own), not + // content:write, which would come back narrowed and loop. With it the call goes through. + const editorOn = (siteId: string) => + grantPrincipal("contributor", read, { [siteId]: "editor" }); + expect(await challenged(editorOn, "create_entry", entryArgs)).toEqual(["content:draft:own"]); + const consented = (siteId: string) => + grantPrincipal("contributor", [...read, "content:draft:own"], { [siteId]: "editor" }); + expect(await challenged(consented, "create_entry", entryArgs)).toBeUndefined(); + // A site the member cannot open answers not_found from the handler, never a challenge. + const elsewhere = grantPrincipal("editor", read, { site_other: "editor" }); + expect(await challenged(elsewhere, "create_entry", entryArgs)).toBeUndefined(); + }); + + it("never challenges a plan-mode token for a write plan mode refuses anyway", async () => { + const plan: Principal = { + ...grantPrincipal("editor", scopesForMode("plan")), + mode: "plan", + }; + expect(await challenged(plan, "create_entry", entryArgs)).toBeUndefined(); + }); + + it("records the step-up 403 in the audit log, like any other denial", async () => { + const { app, siteId, audit } = await setupWithPrincipal( + grantPrincipal("editor", ["site:read", "content:read"]), + ); + const res = await app.request(`${ORIGIN}/mcp`, toolCall("publish", { siteId })); + expect(res.status).toBe(403); + const rows = await audit.list({ tool: "publish" }); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ + actor: { kind: "agent", id: "agent_grant0001" }, + siteId, + tool: "publish", + decision: "deny", + outcome: "error", + reason: "insufficient_scope: this connection needs site:publish", + }); + }); + + it("refuses at createApi time an oauth.resource that is not the /mcp endpoint", async () => { + const b = await backend.create(); + const make = (resource: string) => + createApi({ + store: b.store, + mcp: { + stateKey: "mcp-oauth-state-key-0123456789abcdefghij", + oauth: { issuer: `${ORIGIN}/api/auth`, resource }, + }, + }); + expect(() => make(`${ORIGIN}/`)).toThrow( + /mcp\.oauth\.resource must be the public URL of \/mcp/, + ); + expect(() => make(`${ORIGIN}/api/mcp-tools`)).toThrow(/mcp\.oauth\.resource/); + expect(() => make(`${ORIGIN}/mcp?x=1`)).toThrow(/mcp\.oauth\.resource/); + expect(() => make(`${ORIGIN}/mcp/`)).not.toThrow(); + expect(() => make("https://example.com/studio/mcp")).not.toThrow(); + }); + it("never challenges for a scope the role lacks, and never challenges a key", async () => { // A Contributor's role has no site:publish: a challenge would make the client loop. const contributor = await setupWithPrincipal( diff --git a/packages/api/src/testing-rate-limits.ts b/packages/api/src/testing-rate-limits.ts new file mode 100644 index 00000000..d4111509 --- /dev/null +++ b/packages/api/src/testing-rate-limits.ts @@ -0,0 +1,144 @@ +/** + * Shared suite for #168 item 8: Cloudhal's own long-window counters survive Better Auth's + * rate-limit pruning. Better Auth deletes every `rateLimit` row older than its longest window + * (60 s) whenever one of its own windows lapses; the hourly reset-mail cap used to live there and + * restarted every minute. Runs the real API and Better Auth (with database rate limits, as + * production does) on any backend; `Date` is faked so Better Auth's clock moves too. + */ +import { seedStarterSite } from "@cloudhal/core-ts"; +import { afterAll, afterEach, describe, expect, it, vi } from "vitest"; +import { createApi } from "./app.ts"; +import { createAuth } from "./auth.ts"; +import type { MailMessage } from "./mail.ts"; +import { PRUNE_LIMIT } from "./rate-limits.ts"; +import { StaffStore } from "./staff.ts"; +import type { RolesBackend } from "./testing-roles.ts"; + +const ORIGIN = "http://limits.test"; +const IP = "203.0.113.7"; +const T0 = Date.parse("2026-11-08T12:00:00.000Z"); + +export function describeRateLimitCounters(label: string, backend: RolesBackend): void { + describe(`Cloudhal rate-limit counters: ${label}`, { timeout: 180_000 }, () => { + afterEach(() => { + vi.useRealTimers(); + }); + afterAll(async () => { + await backend.dispose?.(); + }); + + it("keeps the hourly reset-mail cap when Better Auth prunes its 60 s window", async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(T0); + const b = await backend.create(); + await seedStarterSite(b.store, { name: "Limits", slug: "limits" }); + const auth = createAuth({ + database: b.database, + sql: b.sql, + baseURL: ORIGIN, + secret: "rate-limit-test-secret-0123456789abcdefg", + signUp: "first-user", + clientAddressHeader: "cf-connecting-ip", + }); + const mailbox: MailMessage[] = []; + const pending: Promise[] = []; + const app = createApi({ + store: b.store, + resolvePrincipal: auth.resolvePrincipal, + authHandler: auth.handler, + staff: auth.staff, + signUp: auth.signUp, + mail: { id: "test", send: async (m) => void mailbox.push(m) }, + defer: (task) => void pending.push(task), + }); + const post = (path: string, body: unknown) => + app.request(`${ORIGIN}${path}`, { + method: "POST", + headers: { "content-type": "application/json", "cf-connecting-ip": IP }, + body: JSON.stringify(body), + }); + const email = "owner@example.com"; + const signedUp = await post("/api/auth/sign-up/email", { + name: "Olive Owner", + email, + password: "correct horse battery staple", + }); + expect(signedUp.status).toBe(200); + const signIn = () => + post("/api/auth/sign-in/email", { email, password: "not the password at all" }); + expect((await signIn()).status).toBe(401); + const askForResets = async (n: number) => { + for (let i = 0; i < n; i++) + expect((await post("/api/password-reset/request", { email })).status).toBe(202); + while (pending.length > 0) await Promise.all(pending.splice(0)); + return mailbox.filter((m) => m.to === email).length; + }; + + // The first burst reaches the cap of 3 mails per address per hour. + expect(await askForResets(4)).toBe(3); + + // 61 s later Better Auth's own sign-in window for this address has lapsed: its next sign-in + // prunes every `rateLimit` row older than 60 s (the sentinel proves the prune ran). + await b.sql.run( + `INSERT INTO "rateLimit" ("id", "key", "count", "lastRequest") VALUES ('sentinel', 'sentinel', 1, ?)`, + [T0], + ); + vi.setSystemTime(T0 + 61_000); + expect((await signIn()).status).toBe(401); + expect(await b.sql.get(`SELECT "key" FROM "rateLimit" WHERE "key" = 'sentinel'`)).toBe( + undefined, + ); + + // The second burst, inside the same hour, still sees the cap. + expect(await askForResets(3)).toBe(3); + + // An hour after the first mail the address can be mailed again. + vi.setSystemTime(T0 + 61 * 60_000); + expect(await askForResets(1)).toBe(4); + }); + + it("prunes expired counters, which nothing else deletes", async () => { + const b = await backend.create(); + let now = T0; + let prune = false; + const staff = new StaffStore( + b.sql, + () => new Date(now), + () => prune, + ); + const keys = async () => + (await b.sql.all<{ key: string }>("SELECT key FROM rate_limits ORDER BY key")).map( + (r) => r.key, + ); + expect(await staff.hit("cloudhal:minute", 60_000, 3)).toBe(true); + expect(await staff.hit("cloudhal:hour", 3_600_000, 2)).toBe(true); + now = T0 + 61_000; + // A hit that does not prune leaves the stale row; a pruning one removes only expired rows. + expect(await staff.hit("cloudhal:other", 60_000, 3)).toBe(true); + expect(await keys()).toEqual(["cloudhal:hour", "cloudhal:minute", "cloudhal:other"]); + prune = true; + expect(await staff.hit("cloudhal:hour", 3_600_000, 2)).toBe(true); + expect(await keys()).toEqual(["cloudhal:hour", "cloudhal:other"]); + // The hourly counter kept its window: the third hit within the hour is over its max of 2. + expect(await staff.hit("cloudhal:hour", 3_600_000, 2)).toBe(false); + // A counter whose row was pruned starts again at 1. + expect(await staff.hit("cloudhal:minute", 60_000, 1)).toBe(true); + // One prune deletes at most PRUNE_LIMIT expired rows; later hits take the rest. + await b.sql.batch( + Array.from({ length: PRUNE_LIMIT + 5 }, (_, i) => ({ + sql: "INSERT INTO rate_limits (key, count, window_start, expires_at) VALUES (?, 1, 0, 1)", + params: [`cloudhal:stale:${i}`], + })), + ); + await staff.hit("cloudhal:other", 60_000, 3); + const left = await b.sql.get<{ n: number }>( + "SELECT count(*) AS n FROM rate_limits WHERE key LIKE 'cloudhal:stale:%'", + ); + expect(Number(left?.n)).toBe(5); + const index = await b.sql.get<{ name: string }>( + "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'rate_limits' AND name = 'rate_limits_expires'", + ); + expect(index?.name).toBe("rate_limits_expires"); + }); + }); +} diff --git a/packages/api/src/tokens.ts b/packages/api/src/tokens.ts index 5c114065..0f325ce7 100644 --- a/packages/api/src/tokens.ts +++ b/packages/api/src/tokens.ts @@ -70,8 +70,8 @@ export function clientIp(c: Context): string { } /** - * 10 requests per minute per IP on a public token endpoint (RFC-0005 §7.1), counted in Better - * Auth's `rateLimit` table. + * 10 requests per minute per IP on a public token endpoint (RFC-0005 §7.1), counted in Cloudhal's + * `rate_limits` table (migration 0030; not Better Auth's `rateLimit`, which it prunes by 60 s). */ export async function limitPublic(c: Context, bucket: string): Promise { const staff = c.get("staff"); diff --git a/packages/api/test/playground.test.ts b/packages/api/test/playground.test.ts index d94d361d..08202fa1 100644 --- a/packages/api/test/playground.test.ts +++ b/packages/api/test/playground.test.ts @@ -79,6 +79,10 @@ describe("the playground route table", () => { ["POST", "/api/agent-keys", "keys"], ["POST", "/mcp", "mcp"], ["GET", "/mcp/", "mcp"], + // RFC-0044 decision 10: no OAuth discovery in a playground, even if `mcp.oauth` were set. + ["GET", "/.well-known/oauth-protected-resource", "mcp"], + ["HEAD", "/.well-known/oauth-protected-resource/mcp", "mcp"], + ["GET", "/.well-known/oauth-protected-resource/other", "unclassified"], ["POST", "/api/sites/s1/assets/uploads", "uploads"], ["PUT", "/api/sites/s1/assets/uploads/u1", "uploads"], ["DELETE", "/api/sites/s1/assets/a1", "uploads"], @@ -113,6 +117,7 @@ describe("the playground route table", () => { // not the API's to judge: Studio and the rendered site ["GET", "/studio/members"], ["GET", "/s/01J0000000/blog/"], + ["GET", "/.well-known/security.txt"], ])("allows %s %s", (method, path) => { expect(isBlockedInPlayground(method, path)).toBeUndefined(); }); @@ -162,6 +167,36 @@ describe("createApi({ playground })", () => { expect(instance.features.mcp).toBe(false); }); + it("judges a path with a long run of slashes in linear time (#168)", () => { + // `/\/+$/` on the request path took about 150 ms per 16k slashes, on every request. + const start = performance.now(); + expect(isBlockedInPlayground("GET", `/api/${"/".repeat(200_000)}x`)).toBe("unclassified"); + expect(isBlockedInPlayground("GET", "/api/me///")).toBeUndefined(); + expect(performance.now() - start).toBeLessThan(1500); + }); + + it("refuses the OAuth discovery documents even when mcp.oauth is configured", async () => { + const sql = await BetterSqlite3Driver.open(":memory:"); + const store = new SiteGraphStore({ sql, blobs: new MemoryBlobStore() }); + await store.init(); + const app = createApi({ + store, + playground: { info: () => info }, + mcp: { + stateKey: "playground-state-key-0123456789abcdefghij", + oauth: { issuer: "http://try.test/api/auth", resource: "http://try.test/mcp" }, + }, + }); + for (const path of [ + "/.well-known/oauth-protected-resource", + "/.well-known/oauth-protected-resource/mcp", + ]) { + const res = await app.request(`http://try.test${path}`); + expect(res.status, path).toBe(403); + expect(await res.json()).toMatchObject({ error: "playground", block: "mcp" }); + } + }); + it("edits, snapshots and publishes", async () => { const { app, site, store } = await setup(); expect((await app.request(`/api/sites/${site.id}`)).status).toBe(200); diff --git a/packages/api/test/rate-limits.test.ts b/packages/api/test/rate-limits.test.ts new file mode 100644 index 00000000..80cecbf9 --- /dev/null +++ b/packages/api/test/rate-limits.test.ts @@ -0,0 +1,34 @@ +import { SiteGraphStore } from "@cloudhal/core-ts"; +import { MemoryBlobStore } from "@cloudhal/storage"; +import { D1Driver } from "@cloudhal/storage/d1"; +import { BetterSqlite3Driver } from "@cloudhal/storage/sqlite"; +import { createMiniflareD1, createTableReset } from "@cloudhal/storage/testing"; +import Database from "better-sqlite3"; +import { describeRateLimitCounters } from "../src/testing-rate-limits.ts"; + +describeRateLimitCounters("better-sqlite3", { + async create() { + const database = new Database(":memory:"); + const sql = new BetterSqlite3Driver(database); + const blobs = new MemoryBlobStore(); + const store = new SiteGraphStore({ sql, blobs }); + await store.init(); + return { store, sql, blobs, database }; + }, +}); + +let d1: Awaited> | undefined; +const reset = createTableReset(); +describeRateLimitCounters("Cloudflare D1 (Miniflare)", { + async create() { + d1 ??= await createMiniflareD1(); + const sql = new D1Driver(d1.db); + const blobs = new MemoryBlobStore(); + const store = new SiteGraphStore({ sql, blobs }); + await reset(sql, () => store.init()); + return { store, sql, blobs, database: d1.db }; + }, + async dispose() { + await d1?.dispose(); + }, +}); diff --git a/packages/astro-renderer/src/feeds.ts b/packages/astro-renderer/src/feeds.ts index b07b9135..26e32c51 100644 --- a/packages/astro-renderer/src/feeds.ts +++ b/packages/astro-renderer/src/feeds.ts @@ -5,6 +5,7 @@ */ import type { ContentEntry, Page, Redirect, Site } from "@cloudhal/core-contracts"; import { collectionSettings, entryPath } from "@cloudhal/core-ts"; +import { trimEndChar } from "@cloudhal/core-ts/names"; import type { ArchiveSummary, ArchiveView } from "./archive.ts"; import { markdownText, renderBody } from "./markdown.ts"; import type { AssetIndex } from "./media.ts"; @@ -214,7 +215,7 @@ export function redirectsFile(redirects: readonly Redirect[]): string { .sort((a, b) => b.from.length - a.from.length || a.from.localeCompare(b.from)); const splat = (to: string) => { const i = to.search(/[?#]/); - const head = (i < 0 ? to : to.slice(0, i)).replace(/\/+$/, ""); + const head = trimEndChar(i < 0 ? to : to.slice(0, i), "/"); return `${head}/:splat${i < 0 ? "" : to.slice(i)}`; }; const lines = [ diff --git a/packages/astro-renderer/src/locales.ts b/packages/astro-renderer/src/locales.ts index 1fe942ae..f44204ff 100644 --- a/packages/astro-renderer/src/locales.ts +++ b/packages/astro-renderer/src/locales.ts @@ -9,6 +9,7 @@ import type { ContentEntry, Page, Site, Taxonomy, Term } from "@cloudhal/core-contracts"; import { entryPath, type PermalinkSite, termSlug } from "@cloudhal/core-ts"; import { localeRouting, localeUrl, localizedPath } from "@cloudhal/core-ts/locale-paths"; +import { trimEndChar } from "@cloudhal/core-ts/names"; import { routableEntries } from "./entry-views.ts"; import type { HeadMeta } from "./seo.ts"; import type { GraphSource } from "./source.ts"; @@ -247,7 +248,7 @@ export function localizeLinks(html: string, localize: (path: string) => string): const cut = url.search(/[?#]/); const path = cut < 0 ? url : url.slice(0, cut); const suffix = cut < 0 ? "" : url.slice(cut); - return `${head}${localize(path.length > 1 ? path.replace(/\/+$/, "") : path)}${suffix}${tail}`; + return `${head}${localize(path.length > 1 ? trimEndChar(path, "/") : path)}${suffix}${tail}`; }; return html .replace(/(<(?:a|form)\b[^>]*?\s(?:href|action)=")(\/[^"]*)(")/gi, swap) diff --git a/packages/astro-renderer/src/markdown.ts b/packages/astro-renderer/src/markdown.ts index a6dea292..4fdb2f38 100644 --- a/packages/astro-renderer/src/markdown.ts +++ b/packages/astro-renderer/src/markdown.ts @@ -3,6 +3,7 @@ import { type BlockRegistry, createBlockRegistry } from "@cloudhal/core-ts/block import { type DirectiveToken, directiveExtension } from "@cloudhal/core-ts/body-blocks"; import { checkDirective, directiveProps } from "@cloudhal/core-ts/directives"; import { ASSET_HREF, assetUrl, mediaPath } from "@cloudhal/core-ts/media"; +import { trimChar } from "@cloudhal/core-ts/names"; import { parseSurfaceFence } from "@cloudhal/core-ts/surfaces"; import { Marked } from "marked"; import { builtInBlockHtml, isBuiltInBlock, missingBlockHtml } from "./block-html.ts"; @@ -48,17 +49,14 @@ const RESERVED_ID = /^(?:comment|cloudhal|ch-)/; * Latin letters or digits falls back to `section`; repeats get `-2`, `-3`, …. */ export function headingSlug(text: string, taken?: Map): string { - const base = - text - .replace(/[ıİşŞğĞ]/g, (c) => TURKISH[c] ?? c) - .normalize("NFKD") - .replace(/[̀-ͯ]/g, "") - .toLowerCase() - .replace(/ß/g, "ss") - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 64) - .replace(/-+$/, "") || "section"; + const dashed = text + .replace(/[ıİşŞğĞ]/g, (c) => TURKISH[c] ?? c) + .normalize("NFKD") + .replace(/[̀-ͯ]/g, "") + .toLowerCase() + .replace(/ß/g, "ss") + .replace(/[^a-z0-9]+/g, "-"); + const base = trimChar(trimChar(dashed, "-").slice(0, 64), "-") || "section"; const slug = RESERVED_ID.test(base) || /^\d/.test(base) ? `h-${base}` : base; if (!taken) return slug; const seen = taken.get(slug) ?? 0; diff --git a/packages/astro-renderer/test/kit.test.ts b/packages/astro-renderer/test/kit.test.ts index e6391282..40996351 100644 --- a/packages/astro-renderer/test/kit.test.ts +++ b/packages/astro-renderer/test/kit.test.ts @@ -24,6 +24,12 @@ describe("heading ids (RFC-0018 §3)", () => { expect(headingSlug("Café façade — Straße")).toBe("cafe-facade-strasse"); }); + it("trims dashes at both ends and at the 64-character cut", () => { + expect(headingSlug(" — Notes — ")).toBe("notes"); + expect(headingSlug(`${"a".repeat(63)} b`)).toBe("a".repeat(63)); + expect(headingSlug("-".repeat(100_000))).toBe("section"); + }); + it("never takes an id Cloudhal's own markup uses, nor starts with a digit", () => { expect(headingSlug("Comments")).toBe("h-comments"); expect(headingSlug("Cloudhal setup")).toBe("h-cloudhal-setup"); diff --git a/packages/cli/test/studio-selfhost.test.ts b/packages/cli/test/studio-selfhost.test.ts index 823d6665..45b05b4f 100644 --- a/packages/cli/test/studio-selfhost.test.ts +++ b/packages/cli/test/studio-selfhost.test.ts @@ -257,14 +257,21 @@ describe("studio auth options (#165)", () => { expect(() => assertProxyHasBaseUrl({ ...server, hostname: "0.0.0.0" })).not.toThrow(); }); - it("makes slugs and setup lines in linear time (CodeQL js/polynomial-redos)", () => { + it("makes package slugs with no dash at either end or at the 64-character cut", () => { expect(slugify(" Hello, World! ")).toBe("hello-world"); expect(slugify("---")).toBe("site"); expect(slugify("a".repeat(70))).toBe("a".repeat(64)); + // The regex version this replaced cut after the dash and returned 63 a's and a "-". expect(slugify(`${"a".repeat(63)} b`)).toBe("a".repeat(63)); - const start = performance.now(); + // Long separator runs: behaviour only. The regex version was linear here too (its first + // replace collapsed every run before the trim), so no input tells the two apart by time + // (#168 item 10); the loop is for CodeQL js/polynomial-redos. expect(slugify("-".repeat(100_000))).toBe("site"); expect(slugify(`x${" -".repeat(100_000)}y`)).toBe("x-y"); + }); + + it("finds the setup fragment in linear time (CodeQL js/polynomial-redos)", () => { + const start = performance.now(); const long = describeSetup({ url: `http://localhost:8787/login#setup=${"#setup=".repeat(100_000)}`, tokenSource: "env", diff --git a/packages/comments/src/normalize.ts b/packages/comments/src/normalize.ts index b4779a0e..55702e7b 100644 --- a/packages/comments/src/normalize.ts +++ b/packages/comments/src/normalize.ts @@ -57,17 +57,20 @@ export function cutCodePoints(s: string, max: number): string { } export function normalizeText(raw: string): string { - return raw - .normalize("NFC") - .replace(/\r\n?/g, "\n") - .replace(UNSAFE_IN_TEXT, "") - .replace(INVISIBLE_RE, "") - .replace(LONE_ZWJ_RE, "") - .split("\n") - .map((line) => line.replace(/[^\S\n]+$/u, "")) - .join("\n") - .replace(/\n{4,}/g, "\n\n\n") - .trim(); + return ( + raw + .normalize("NFC") + .replace(/\r\n?/g, "\n") + .replace(UNSAFE_IN_TEXT, "") + .replace(INVISIBLE_RE, "") + .replace(LONE_ZWJ_RE, "") + .split("\n") + // `trimEnd`, not `/[^\S\n]+$/u`: the regex backtracks on a long run of spaces inside a line. + .map((line) => line.trimEnd()) + .join("\n") + .replace(/\n{4,}/g, "\n\n\n") + .trim() + ); } export function normalizeName(raw: string): string { diff --git a/packages/comments/test/text.test.ts b/packages/comments/test/text.test.ts index 2f3b1e62..e02d691c 100644 --- a/packages/comments/test/text.test.ts +++ b/packages/comments/test/text.test.ts @@ -40,6 +40,14 @@ describe("normalising (§6.4)", () => { ); }); + it("trims line ends in linear time (#168, CodeQL js/polynomial-redos)", () => { + // `/[^\S\n]+$/u` took seconds on a long run of spaces inside one line. + const line = `a${" ".repeat(200_000)}b`; + const start = performance.now(); + expect(normalizeText(`${line} \nnext`)).toBe(`${line}\nnext`); + expect(performance.now() - start).toBeLessThan(1500); + }); + it("removes control characters but tab and line feed, C1 controls and bidi overrides", () => { const hostile = "a\u0000b\u0007c\u001Bd\u007Fe\u0085f" + diff --git a/packages/core-ts/src/directives.ts b/packages/core-ts/src/directives.ts index b9285aa8..217d1dcb 100644 --- a/packages/core-ts/src/directives.ts +++ b/packages/core-ts/src/directives.ts @@ -156,7 +156,7 @@ export function matchDirective(src: string): { directive: Directive; raw: string /** A whole block's source as a directive, or `undefined` when it is not exactly one. */ export function parseDirective(markdown: string): Directive | undefined { - const source = markdown.replace(/\s+$/, ""); + const source = markdown.trimEnd(); // not `/\s+$/`: it backtracks on inner whitespace runs const m = matchDirective(source); return m && m.raw === source ? m.directive : undefined; } @@ -199,7 +199,7 @@ export function writeDirective( ): string { const attrs = directiveAttrsText(props); if (content === undefined) return `::${name}${attrs}`; - const inner = content.replace(/\s+$/, ""); + const inner = content.trimEnd(); let deepest = 2; for (const m of inner.matchAll(/^(:{3,})/gm)) deepest = Math.max(deepest, (m[1] as string).length); diff --git a/packages/core-ts/src/media/types.ts b/packages/core-ts/src/media/types.ts index b1f0d159..30432cd7 100644 --- a/packages/core-ts/src/media/types.ts +++ b/packages/core-ts/src/media/types.ts @@ -4,6 +4,7 @@ * so a client can never pick a kind or an extension (invariant 4). Worker-safe. */ import type { Asset } from "@cloudhal/core-contracts"; +import { trimChar } from "../names.ts"; export type AssetKind = Asset["kind"]; @@ -248,14 +249,12 @@ const SPECIAL: Record = { /** A URL slug from a file name or title: `Boğaz at Dawn (1).JPG` → `bogaz-at-dawn-1`. */ export function slugifyFilename(name: string | undefined, fallback = "file"): string { const base = (name ?? "").replace(/\.[A-Za-z0-9]{1,10}$/, ""); - const slug = base + const dashed = base .toLowerCase() .replace(/[ıßæœøđðłþ]/g, (c) => SPECIAL[c] ?? c) .normalize("NFKD") .replace(/[̀-ͯ]/g, "") - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 96) - .replace(/-+$/g, ""); + .replace(/[^a-z0-9]+/g, "-"); + const slug = trimChar(trimChar(dashed, "-").slice(0, 96), "-"); return slug || fallback; } diff --git a/packages/core-ts/src/names.ts b/packages/core-ts/src/names.ts index c42054b1..0c0d6e76 100644 --- a/packages/core-ts/src/names.ts +++ b/packages/core-ts/src/names.ts @@ -27,13 +27,33 @@ const SPECIAL: Record = { ð: "d", }; +/** + * `text` without `ch` (one UTF-16 unit, e.g. `-`) at either end, in one linear pass. Use it in + * place of a `/^-+|-+$/g` trim: CodeQL reports that pattern as polynomial backtracking + * (js/polynomial-redos) on long runs of `ch`. + */ +export function trimChar(text: string, ch: string): string { + let start = 0; + let end = text.length; + while (start < end && text[start] === ch) start++; + while (end > start && text[end - 1] === ch) end--; + return start === 0 && end === text.length ? text : text.slice(start, end); +} + +/** `text` without `ch` at its end, linearly: the `/\/+$/` kind of trim, for paths and the like. */ +export function trimEndChar(text: string, ch: string): string { + let end = text.length; + while (end > 0 && text[end - 1] === ch) end--; + return end === text.length ? text : text.slice(0, end); +} + /** * Folds a name for comparison: NFKC, Turkish-aware lower-casing (İ, I and ı all become i), * diacritics removed, apostrophes and dots dropped (`A.I.` → `ai`), every other run of * non-letters/digits turned into a single `-`. `Yapay Zekâ` → `yapay-zeka`; a folded slug is itself. */ export function foldName(input: string): string { - return input + const dashed = input .normalize("NFKC") .replace(/[İIı]/g, "i") .toLowerCase() @@ -41,8 +61,8 @@ export function foldName(input: string): string { .replace(/\p{M}+/gu, "") .replace(/[øßæœđłþð]/g, (c) => SPECIAL[c] ?? c) .replace(/['’‘`ʼ.]/g, "") - .replace(/[^\p{L}\p{N}]+/gu, "-") - .replace(/^-+|-+$/g, ""); + .replace(/[^\p{L}\p{N}]+/gu, "-"); + return trimChar(dashed, "-"); } /** @@ -61,11 +81,8 @@ export function pluralForms(folded: string): string[] { /** A slug for a free-text name (`Machine Learning` → `machine-learning`); empty when nothing is left. */ export function slugify(name: string): string { - return foldName(name) - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 128) - .replace(/-+$/, ""); + const dashed = foldName(name).replace(/[^a-z0-9]+/g, "-"); + return trimChar(trimChar(dashed, "-").slice(0, 128), "-"); } /** `machine-learning` → `Machine learning`. */ diff --git a/packages/core-ts/src/surface-interop.ts b/packages/core-ts/src/surface-interop.ts index 1aa20715..a67ad64f 100644 --- a/packages/core-ts/src/surface-interop.ts +++ b/packages/core-ts/src/surface-interop.ts @@ -13,6 +13,7 @@ */ import type { SurfaceComponent, SurfaceSection } from "@cloudhal/core-contracts"; import { nearestIcon } from "./icon-names.ts"; +import { trimChar } from "./names.ts"; export const A2UI_VERSION = "v0.9"; export const A2UI_BASIC_CATALOG = "https://a2ui.org/specification/v0_9/catalogs/basic/catalog.json"; @@ -50,15 +51,12 @@ class Ids { get(raw: string): string { const known = this.#map.get(raw); if (known) return known; - let base = - raw === "root" - ? "root" - : raw - .toLowerCase() - .replace(/[^a-z0-9-]+/g, "-") - .replace(/^[^a-z]+/, "") - .replace(/-+$/, "") - .slice(0, 36) || "c"; + // `trimChar`, not `/-+$/`, which backtracks on the "-" runs kept here (js/polynomial-redos). + const dashed = raw + .toLowerCase() + .replace(/[^a-z0-9-]+/g, "-") + .replace(/^[^a-z]+/, ""); + let base = raw === "root" ? "root" : trimChar(dashed, "-").slice(0, 36) || "c"; if (!ID.test(base)) base = `c-${base}`.slice(0, 36); let id = base; for (let n = 2; this.#taken.has(id); n++) id = `${base}-${n}`; diff --git a/packages/core-ts/test/directives.test.ts b/packages/core-ts/test/directives.test.ts index 1a51e6d4..7dc7efa6 100644 --- a/packages/core-ts/test/directives.test.ts +++ b/packages/core-ts/test/directives.test.ts @@ -31,6 +31,14 @@ describe("directive syntax", () => { expect(parseDirective("::embed")).toEqual({ name: "embed", attrs: {}, fence: 2 }); }); + it("trims trailing whitespace in linear time (#168, CodeQL js/polynomial-redos)", () => { + // `/\s+$/` took about 6 s on a 100k run of spaces inside an agent's block. + const start = performance.now(); + expect(parseDirective(`::embed${" ".repeat(200_000)}x`)).toBeUndefined(); + expect(parseDirective("::embed \n\t ")).toEqual({ name: "embed", attrs: {}, fence: 2 }); + expect(performance.now() - start).toBeLessThan(1500); + }); + it("rejects anything that does not parse exactly", () => { for (const bad of [ ":::callout{tone=warning}\nx\n:::", // unquoted value diff --git a/packages/core-ts/test/interop.test.ts b/packages/core-ts/test/interop.test.ts index f3cebdde..c7166d9a 100644 --- a/packages/core-ts/test/interop.test.ts +++ b/packages/core-ts/test/interop.test.ts @@ -527,4 +527,23 @@ describe("json-render", () => { fromJsonRender({ elements: {} }, { sectionId: "sec_x00000001", catalog: catalog.id }), ).toThrow(InteropError); }); + + it("turns an agent's key with a long run of dashes into an id in linear time (#168)", () => { + // The old `/-+$/` trim took about 16 s on this key (the replace before it keeps "-" runs). + const key = `a${"-".repeat(200_000)}b-`; + const start = performance.now(); + const r = fromJsonRender( + { + root: "page", + elements: { + page: { type: "Stack", children: [key] }, + [key]: { type: "Heading", props: { text: "Hi", level: 2 } }, + }, + }, + { sectionId: "sec_jsonrend02", catalog: catalog.id }, + ); + expect(performance.now() - start).toBeLessThan(1500); + expect(r.surface.components[1]?.id).toBe(`a${"-".repeat(35)}`); + expect(checkSurface(r.surface, catalog).issues).toEqual([]); + }); }); diff --git a/packages/core-ts/test/media.test.ts b/packages/core-ts/test/media.test.ts index bb577bb8..dc483cfe 100644 --- a/packages/core-ts/test/media.test.ts +++ b/packages/core-ts/test/media.test.ts @@ -559,6 +559,10 @@ describe("references and URLs", () => { expect(slugifyFilename("Boğaz at Dawn (1).JPG")).toBe("bogaz-at-dawn-1"); expect(slugifyFilename("İstanbul ışıkları.png")).toBe("istanbul-isiklari"); expect(slugifyFilename("...")).toBe("file"); + // #168: linear trims, and a cut at 96 characters that lands on a dash drops it. + expect(slugifyFilename(`--${"-".repeat(100_000)}--.png`)).toBe("file"); + expect(slugifyFilename(`${"a".repeat(95)} b.jpg`)).toBe("a".repeat(95)); + expect(slugifyFilename(" -Hello- .txt")).toBe("hello"); }); }); diff --git a/packages/core-ts/test/names.test.ts b/packages/core-ts/test/names.test.ts index 51c9deea..c786f767 100644 --- a/packages/core-ts/test/names.test.ts +++ b/packages/core-ts/test/names.test.ts @@ -7,6 +7,8 @@ import { type ResolvableTerm, resolveTerm, slugify, + trimChar, + trimEndChar, } from "../src/names.ts"; const term = ( @@ -49,6 +51,43 @@ describe("foldName", () => { expect(foldName("machine-learning")).toBe("machine-learning"); }); + it("trimChar trims one character at both ends, linearly", () => { + expect(trimChar("--a-b--", "-")).toBe("a-b"); + expect(trimChar("a-b", "-")).toBe("a-b"); + expect(trimChar("----", "-")).toBe(""); + expect(trimChar("", "-")).toBe(""); + expect(trimChar("__x_", "_")).toBe("x"); + expect(trimChar(`a${"-".repeat(200_000)}b`, "-")).toBe(`a${"-".repeat(200_000)}b`); + }); + + it("trimEndChar trims one character at the end only, linearly", () => { + expect(trimEndChar("/a/b//", "/")).toBe("/a/b"); + expect(trimEndChar("///", "/")).toBe(""); + expect(trimEndChar("/a", "/")).toBe("/a"); + const inner = `/a${"/".repeat(200_000)}b`; + const start = performance.now(); + expect(trimEndChar(`${inner}/`, "/")).toBe(inner); + expect(performance.now() - start).toBeLessThan(1500); + }); + + it("slugify trims dashes at both ends and at the 128-character cut (#168)", () => { + expect(slugify(" --Hello, World!-- ")).toBe("hello-world"); + expect(slugify("---")).toBe(""); + expect(slugify("-".repeat(100_000))).toBe(""); + // A cut that lands on a dash drops it; the slug never ends in "-". + expect(slugify(`${"a".repeat(127)} b`)).toBe("a".repeat(127)); + expect(slugify("a".repeat(200))).toBe("a".repeat(128)); + // The same answers as the regex version it replaced (`/^-+|-+$/g`, then `/-+$/`). + const regex = (name: string) => + foldName(name) + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") + .slice(0, 128) + .replace(/-+$/, ""); + for (const input of ["", "A.I.", " x ", "a--b", "Çağrı & Öz", `${"ab ".repeat(60)}`, "--"]) + expect(slugify(input), input).toBe(regex(input)); + }); + it("slugify, humanizeSlug, pluralForms and editDistance", () => { expect(slugify("Yapay Zekâ & Robotlar")).toBe("yapay-zeka-robotlar"); expect(humanizeSlug("machine-learning")).toBe("Machine learning"); diff --git a/packages/storage/migrations/0030_rate_limits.ts b/packages/storage/migrations/0030_rate_limits.ts new file mode 100644 index 00000000..a1a3faae --- /dev/null +++ b/packages/storage/migrations/0030_rate_limits.ts @@ -0,0 +1,28 @@ +import type { Migration } from "../src/migrate.ts"; + +/** + * #168 item 8: Cloudhal's own fixed-window counters (`StaffStore.hit`: the hourly per-address + * reset-mail cap, the per-minute public token buckets, each agent key's window) leave Better + * Auth's `rateLimit` table (0005). Better Auth prunes that table by its own 60 s window and so + * deleted the hourly counters too, which let the reset-mail cap restart every minute. + * + * `rate_limits`: one row per counter key (`cloudhal:…`), its `count` in the window that began at + * `window_start` and ends at `expires_at` (epoch ms; each counter has its own window length). + * Keys come from client addresses and submitted email addresses, so `StaffStore.hit` deletes + * expired rows itself, a bounded number at a time, through the `expires_at` index. The old + * `cloudhal:` rows in `rateLimit` are deleted (Better Auth would prune them anyway), so a counter + * restarts once at upgrade. + */ +export const migration0030RateLimits: Migration = { + name: "0030_rate_limits", + sql: ` +CREATE TABLE rate_limits ( + key TEXT NOT NULL PRIMARY KEY, + count INTEGER NOT NULL, + window_start INTEGER NOT NULL, + expires_at INTEGER NOT NULL +); +CREATE INDEX rate_limits_expires ON rate_limits (expires_at); +DELETE FROM "rateLimit" WHERE "key" LIKE 'cloudhal:%'; +`, +}; diff --git a/packages/storage/migrations/index.ts b/packages/storage/migrations/index.ts index 0104e059..765dc640 100644 --- a/packages/storage/migrations/index.ts +++ b/packages/storage/migrations/index.ts @@ -28,6 +28,7 @@ import { migration0026SearchCjk } from "./0026_search_cjk.ts"; import { migration0027MultiSite } from "./0027_multi_site.ts"; import { migration0028SiteMembers } from "./0028_site_members.ts"; import { migration0029EntryListValues } from "./0029_entry_list_values.ts"; +import { migration0030RateLimits } from "./0030_rate_limits.ts"; /** All Cloudhal storage migrations, in order. */ export const migrations: readonly Migration[] = [ @@ -60,4 +61,5 @@ export const migrations: readonly Migration[] = [ migration0027MultiSite, migration0028SiteMembers, migration0029EntryListValues, + migration0030RateLimits, ]; diff --git a/packages/storage/src/testing.ts b/packages/storage/src/testing.ts index 2ef846d5..84938df8 100644 --- a/packages/storage/src/testing.ts +++ b/packages/storage/src/testing.ts @@ -65,6 +65,17 @@ export function describeSqlDriverConformance(label: string, factory: SqlDriverFa expect(await driver.get("SELECT v FROM t WHERE id = 999")).toBeUndefined(); }); + it("get returns the row of a write's RETURNING clause (an upsert's count in one statement)", async () => { + const upsert = (v: string) => + driver.get<{ n: number }>( + "INSERT INTO t (id, v, n) VALUES (1, ?, 1) ON CONFLICT (id) DO UPDATE SET n = t.n + 1, v = excluded.v RETURNING n", + [v], + ); + expect(await upsert("a")).toEqual({ n: 1 }); + expect(await upsert("b")).toEqual({ n: 2 }); + expect(await driver.get("SELECT v, n FROM t WHERE id = 1")).toEqual({ v: "b", n: 2 }); + }); + it("batch is atomic: a failing statement rolls back the whole batch", async () => { await expect( driver.batch([ diff --git a/packages/storage/test/migrate.test.ts b/packages/storage/test/migrate.test.ts index e83454e7..1c029ba8 100644 --- a/packages/storage/test/migrate.test.ts +++ b/packages/storage/test/migrate.test.ts @@ -47,6 +47,7 @@ describe("migrate", () => { "0027_multi_site", "0028_site_members", "0029_entry_list_values", + "0030_rate_limits", ]); expect((await migrate(driver, migrations)).applied).toEqual([]); expect(await appliedMigrations(driver)).toEqual([ @@ -79,6 +80,7 @@ describe("migrate", () => { "0027_multi_site", "0028_site_members", "0029_entry_list_values", + "0030_rate_limits", ]); const tables = await driver.all<{ name: string }>( "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%' ORDER BY name", @@ -124,6 +126,7 @@ describe("migrate", () => { "password_reset", "projects", "rateLimit", + "rate_limits", "release_items", "releases", "schedule_runs", @@ -190,6 +193,7 @@ describe("migrate", () => { "0027_multi_site", "0028_site_members", "0029_entry_list_values", + "0030_rate_limits", ]); expect(await appliedMigrations(driver)).toEqual([ "0001_init", @@ -221,6 +225,7 @@ describe("migrate", () => { "0027_multi_site", "0028_site_members", "0029_entry_list_values", + "0030_rate_limits", ]); await h.dispose(); }); @@ -259,6 +264,7 @@ describe("migrate", () => { "0027_multi_site", "0028_site_members", "0029_entry_list_values", + "0030_rate_limits", ]); const ok = await driver.get("SELECT name FROM sqlite_master WHERE name = 'ok'"); expect(ok).toBeUndefined(); diff --git a/scripts/size-baseline.json b/scripts/size-baseline.json index 8d97ee22..db95a8a3 100644 --- a/scripts/size-baseline.json +++ b/scripts/size-baseline.json @@ -22,7 +22,7 @@ "packages/core-ts/src/menu.ts": 928, "packages/core-ts/src/releases.ts": 996, "packages/core-ts/src/store.ts": 1212, - "packages/core-ts/src/surface-interop.ts": 1235, + "packages/core-ts/src/surface-interop.ts": 1233, "packages/core-ts/src/taxonomy.ts": 1718, "packages/mcp-server/src/menu-tools.ts": 1058, "packages/mcp-server/src/schedule-tools.ts": 1596,