diff --git a/CLAUDE.md b/CLAUDE.md index fdb1198..b165b28 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,16 +90,15 @@ pnpm db:migrate:remote # Apply migrations to remote D1 | `/api/auth/*` | auth-catchall.tsx | Better Auth | | `/api/search` | api.search.ts | None | | `/api/skills/:slug` | api.skill-detail.ts | None | -| `/api/skills/:slug/references` | api.skill-references.ts | None (GET), Session/Key (POST) | -| `/api/skills/:slug/rate` | api.skill-rate.ts | Session/Key | -| `/api/skills/:slug/review` | api.skill-review.ts | Session/Key | -| `/api/skills/:slug/favorite` | api.skill-favorite.ts | Session/Key | +| `/api/skills/:slug/rate` | api.skill-rate.ts | Session, same-origin only | +| `/api/skills/:slug/review` | api.skill-review.ts | None (GET), Session same-origin only (POST) | +| `/api/skills/:slug/favorite` | api.skill-favorite.ts | Session, same-origin only | | `/api/skills/:slug/install` | api.skill-install.ts | Optional (API key or X-Device-Id) | -| `/api/skills/:slug/vote` | api.skill-vote.ts | Session | -| `/api/report` | api.usage-report.ts | Session/Key | +| `/api/skills/:slug/vote` | api.skill-vote.ts | Session, same-origin only | +| `/api/report` | api.usage-report.ts | API key | | `/api/user/api-keys` | api.user-api-keys.ts | Session | | `/api/user/interactions` | api.user-interactions.ts | Session (graceful fallback) | -| `/api/skills/register` | api.skill-register.ts | Session/Key | +| `/api/skills/register` | api.skill-register.ts | Session (same-origin only)/Key | | `/api/admin/seed` | api.admin.seed.ts | Admin secret | | `/api/v1/packages/:scope/:slug/releases` | api.v1.package-releases.ts (GET list); action re-exported from api.v1.release-publish.ts (POST publish) | GET: optional Key/Session; POST: scoped key `releases:publish:@handle` (404 unless `SKILLX_RELEASES_API` on) | | `/api/v1/packages/:scope/:slug/releases/:version`, `/api/v1/releases/:digest/manifest` | api.v1.release-manifest.ts | Optional Key/Session (404 unless `SKILLX_RELEASES_API` on) | @@ -140,7 +139,7 @@ Search algorithm: `./docs/search-algorithm.md` **References & Scripts**: Stored separately — `skill_references` table for external docs/links/examples (title, filename, url, type enum, content). Scripts stored as JSON in `skills.scripts` (name, description, language, url). CLI flags: `skillx use --include-refs --include-scripts` to display. -**Vote API**: POST `/api/skills/:slug/vote` with `{ direction: 'up'|'down' }`. Rate limited 10 votes/min per user. Atomic count update via SQL subquery. Bidirectional: same vote direction toggles off. +**Vote API**: POST `/api/skills/:slug/vote` with `{ type: 'up'|'down'|'none' }` (`none` clears the vote). Rate limited 10 votes/min per user. Atomic count update via SQL subquery. Returns `{ vote_type, net_votes }`. **CLI `skillx use` resolution**: `author/skill` (two-part → DB slug `author-skill`) | `org/repo/skill` (three-part → DB slug `org-skill`, fallback register from GitHub) | `slug` (direct lookup, fallback search) | `"keywords"` (search mode). Flags: `--include-refs`, `--include-scripts` for extended content. Display logic split into `use-display.ts` module. @@ -158,6 +157,7 @@ Search algorithm: `./docs/search-algorithm.md` - **Seed data** in `scripts/seed-data.json` (30 real skills from skills.sh). Seed via `ADMIN_SECRET=... pnpm seed`. - **Scoped API keys** — `api_keys.scopes` NULL = legacy key; scoped keys (`releases:publish:@handle`) work only on the release publish API. They are stored under `hashScopedApiKey` (domain-separated), so legacy routes (`authenticateApiKey`, by plain key hash) never accept them. Apply migration 0012 to remote D1 before deploying (`docs/deployment-guide.md`). - **Releases are byte-exact** — the release publish API (`app/lib/releases/`) never sanitizes; the scan only picks `published` vs `quarantined`. R2 writes are create-only, the D1 row is written last. See `docs/api-reference.md#publishing-releases`. +- **Same-origin on cookie mutations** — `SameSite=Lax` alone lets a sibling subdomain ride the session cookie, so a mutation a session can authenticate must refuse a foreign origin with `crossOriginRefusal` (`app/lib/auth/same-origin.ts`) before it writes. Session-only routes (rate, review, favorite, vote, user API keys) check first; routes that also take a Bearer key check only on the session path (register after `authenticateRequest`; every `resolveLimitedActor` caller — release state, promote, review, collections, personal kits —, 403 `cross_origin`). Add the check to any new cookie-authenticated mutation. - **Content security scanning** — all SKILL.md content scanned for prompt injection, invisible chars, ANSI escapes, shell injection. `risk_label` column stores result ("safe"/"caution"/"danger"/"unknown"). Sanitization strips zero-width Unicode + ANSI escapes before storage. ## Deployment @@ -179,3 +179,12 @@ Read here: `./docs/deployment-guide.md` ## Documentation All docs in `./docs/` — see design-guidelines.md for full UI/UX spec, code-standards.md for patterns, system-architecture.md for diagrams. + +## Issue Workflow + +Claim an issue before working on it so parallel contributors and agents do not +collide: assign yourself and move it through the labels `in progress` → `in review` +(code review running) → `done` (PR open) → `shipped` (merged to `main`). Search open +and closed issues first; comment on a related or duplicate issue with a reference +instead of starting parallel work. File an issue for an unrelated bug or gap you +find instead of widening the current PR silently. diff --git a/apps/web/app/lib/docs/api-endpoints-data.ts b/apps/web/app/lib/docs/api-endpoints-data.ts index adc4ba7..edffab4 100644 --- a/apps/web/app/lib/docs/api-endpoints-data.ts +++ b/apps/web/app/lib/docs/api-endpoints-data.ts @@ -98,9 +98,29 @@ export const SKILL_ENDPOINTS: EndpointProps[] = [ errors: [ { status: 400, body: "Score must be a number between 0 and 10", cause: "Invalid score value" }, { status: 401, body: "Authentication required", cause: "Not logged in" }, + { status: 403, body: "Cross-origin request refused", cause: "Origin is not this site" }, { status: 404, body: "Skill not found", cause: "Invalid slug" }, ], }, + { + id: "skill-vote", + method: "POST", + path: "/api/skills/:slug/vote", + auth: "Session required", + description: "Upvote, downvote, or clear your vote. Rate limited to 10 votes per minute.", + params: [ + { name: "type", type: "string", required: true, description: '"up", "down", or "none" (clears the vote)' }, + ], + requestExample: `{ "type": "up" }`, + responseExample: `{ "vote_type": "up", "net_votes": 39 }`, + errors: [ + { status: 400, body: "type must be 'up', 'down', or 'none'", cause: "Invalid type" }, + { status: 401, body: "Authentication required", cause: "Not logged in" }, + { status: 403, body: "Cross-origin request refused", cause: "Origin is not this site" }, + { status: 404, body: "Skill not found", cause: "Invalid slug" }, + { status: 429, body: "Rate limit exceeded. Max 10 votes per minute.", cause: "Too many votes" }, + ], + }, { id: "skill-review-get", method: "GET", @@ -134,6 +154,7 @@ export const SKILL_ENDPOINTS: EndpointProps[] = [ { status: 400, body: "Review content cannot be empty", cause: "Empty content" }, { status: 400, body: "Cannot exceed 2000 characters", cause: "Too long" }, { status: 401, body: "Authentication required", cause: "Not logged in" }, + { status: 403, body: "Cross-origin request refused", cause: "Origin is not this site" }, { status: 404, body: "Skill not found", cause: "Invalid slug" }, ], }, @@ -147,6 +168,7 @@ export const SKILL_ENDPOINTS: EndpointProps[] = [ notes: ["favorited: true = added, false = removed. No request body needed."], errors: [ { status: 401, body: "Authentication required", cause: "Not logged in" }, + { status: 403, body: "Cross-origin request refused", cause: "Origin is not this site" }, { status: 404, body: "Skill not found", cause: "Invalid slug" }, ], }, diff --git a/apps/web/app/lib/releases/release-json-body.ts b/apps/web/app/lib/releases/release-json-body.ts new file mode 100644 index 0000000..984c83e --- /dev/null +++ b/apps/web/app/lib/releases/release-json-body.ts @@ -0,0 +1,61 @@ +/** + * Capped JSON object bodies of the release write APIs (state, provisioning, + * personal kits). A body must be `application/json`: a cross-origin page can + * send that only after a CORS preflight, which these routes never grant. + */ +import { ReleaseHttpError, releaseError } from "./release-http"; + +/** Largest accepted request body; a reason is at most 512 UTF-8 bytes. */ +export const MAX_STATE_BODY_BYTES = 4096; + +export const invalidRequest = (message: string) => releaseError(400, "invalid_request", message); + +async function readCappedText(request: Request, maxBytes: number): Promise { + const type = (request.headers.get("Content-Type") ?? "").split(";")[0]!.trim().toLowerCase(); + if (type !== "application/json") throw releaseError(415, "unsupported_media_type", "body must be application/json"); + const tooLarge = () => releaseError(413, "request_too_large", `body exceeds ${maxBytes} bytes`); + if (Number(request.headers.get("Content-Length") ?? "0") > maxBytes) throw tooLarge(); + if (request.body === null) throw invalidRequest("body is required"); + const reader = request.body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (let next = await reader.read(); !next.done; next = await reader.read()) { + total += next.value.length; + if (total > maxBytes) { + await reader.cancel().catch(() => undefined); + throw tooLarge(); + } + chunks.push(next.value); + } + const bytes = new Uint8Array(total); + let at = 0; + for (const chunk of chunks) { + bytes.set(chunk, at); + at += chunk.length; + } + try { + return new TextDecoder("utf-8", { fatal: true }).decode(bytes); + } catch { + throw invalidRequest("body must be UTF-8 JSON"); + } +} + +/** + * Reads a capped `application/json` body that must be a JSON object holding + * only `fields` (an unknown field is 400). Values are not checked here. + * `maxBytes` defaults to the state API cap; other write APIs pass their own. + */ +export async function readJsonObject(request: Request, fields: ReadonlySet, maxBytes = MAX_STATE_BODY_BYTES): Promise> { + let body: unknown; + try { + body = JSON.parse(await readCappedText(request, maxBytes)); + } catch (error) { + if (error instanceof ReleaseHttpError) throw error; + throw invalidRequest("body must be a JSON object"); + } + if (typeof body !== "object" || body === null || Array.isArray(body)) throw invalidRequest("body must be a JSON object"); + const record = body as Record; + const unknown = Object.keys(record).filter((key) => !fields.has(key)); + if (unknown.length > 0) throw invalidRequest(`unknown field: ${unknown.join(", ")}`); + return record; +} diff --git a/apps/web/app/lib/releases/release-state-http.ts b/apps/web/app/lib/releases/release-state-http.ts index a37efda..a147ce8 100644 --- a/apps/web/app/lib/releases/release-state-http.ts +++ b/apps/web/app/lib/releases/release-state-http.ts @@ -25,14 +25,14 @@ import { ReleaseHttpError, errorResponse, notFoundResponse, releaseError } from import { releasesApiEnabled } from "./release-limits"; import { RELEASE_WRITE_RATE_WINDOW_SECONDS, checkReleaseRateLimit, rateLimitCaller } from "./release-rate-limit"; import { resolveViewer } from "./release-read"; +import { invalidRequest, readJsonObject } from "./release-json-body"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; import { PUBLISHER_STATES, transitionPublisher, transitionRelease } from "./release-transitions"; import type { PublisherState, TransitionActor } from "./release-transitions"; /** `ADMIN_SECRET` is typed as set (workers/app.ts) but may be missing at runtime; then no admin exists. */ export type ReleaseStateEnv = Env; -/** Largest accepted request body; a reason is at most 512 UTF-8 bytes. */ -export const MAX_STATE_BODY_BYTES = 4096; const BODY_FIELDS: ReadonlySet = new Set(["from", "to", "reason"]); /** A publisher handle as stored in `publishers.handle`: lowercase, without the `@`. */ const HANDLE_SOURCE = "[a-z0-9][a-z0-9._-]{0,63}"; @@ -42,8 +42,7 @@ export const BARE_PUBLISHER_HANDLE = new RegExp(`^${HANDLE_SOURCE}$`); /** Control, format (bidi included), line/paragraph separator and lone surrogate characters. */ const FORBIDDEN_REASON = /[\p{Cc}\p{Cf}\p{Zl}\p{Zp}\p{Cs}]/u; export const NO_STORE = { "Cache-Control": "no-store" }; - -export const invalidRequest = (message: string) => releaseError(400, "invalid_request", message); +export { MAX_STATE_BODY_BYTES, invalidRequest, readJsonObject } from "./release-json-body"; async function sha256(text: string): Promise { return new Uint8Array(await crypto.subtle.digest("SHA-256", new TextEncoder().encode(text))); @@ -69,36 +68,6 @@ export async function resolveActor(request: Request, env: ReleaseStateEnv): Prom return { kind: "user", id: userId }; } -async function readCappedText(request: Request, maxBytes: number): Promise { - const type = (request.headers.get("Content-Type") ?? "").split(";")[0]!.trim().toLowerCase(); - if (type !== "application/json") throw releaseError(415, "unsupported_media_type", "body must be application/json"); - const tooLarge = () => releaseError(413, "request_too_large", `body exceeds ${maxBytes} bytes`); - if (Number(request.headers.get("Content-Length") ?? "0") > maxBytes) throw tooLarge(); - if (request.body === null) throw invalidRequest("body is required"); - const reader = request.body.getReader(); - const chunks: Uint8Array[] = []; - let total = 0; - for (let next = await reader.read(); !next.done; next = await reader.read()) { - total += next.value.length; - if (total > maxBytes) { - await reader.cancel().catch(() => undefined); - throw tooLarge(); - } - chunks.push(next.value); - } - const bytes = new Uint8Array(total); - let at = 0; - for (const chunk of chunks) { - bytes.set(chunk, at); - at += chunk.length; - } - try { - return new TextDecoder("utf-8", { fatal: true }).decode(bytes); - } catch { - throw invalidRequest("body must be UTF-8 JSON"); - } -} - /** An optional audit reason: 1 to 512 UTF-8 bytes without control, format or separator characters. */ export function parseReason(raw: unknown): string | null { if (raw === undefined || raw === null) return null; @@ -109,26 +78,6 @@ export function parseReason(raw: unknown): string | null { return raw; } -/** - * Reads a capped `application/json` body that must be a JSON object holding - * only `fields` (an unknown field is 400). Values are not checked here. - * `maxBytes` defaults to the state API cap; other write APIs pass their own. - */ -export async function readJsonObject(request: Request, fields: ReadonlySet, maxBytes = MAX_STATE_BODY_BYTES): Promise> { - let body: unknown; - try { - body = JSON.parse(await readCappedText(request, maxBytes)); - } catch (error) { - if (error instanceof ReleaseHttpError) throw error; - throw invalidRequest("body must be a JSON object"); - } - if (typeof body !== "object" || body === null || Array.isArray(body)) throw invalidRequest("body must be a JSON object"); - const record = body as Record; - const unknown = Object.keys(record).filter((key) => !fields.has(key)); - if (unknown.length > 0) throw invalidRequest(`unknown field: ${unknown.join(", ")}`); - return record; -} - /** Reads `{from, to, reason?}`; `from` and `to` must be states of the subject. */ export async function readStateChange( request: Request, @@ -184,7 +133,12 @@ export async function enforceWriteLimit(env: ReleaseStateEnv, caller: string, fa * than the limit allows. */ export async function resolveLimitedActor(request: Request, env: ReleaseStateEnv): Promise> { - if (request.headers.get("X-Admin-Secret") !== null) await enforceWriteLimit(env, rateLimitCaller(request, null), false); + const admin = request.headers.get("X-Admin-Secret") !== null; + // Without a key or secret the session cookie decides, and it rides along ambiently. + if (!admin && request.headers.get("Authorization") === null && crossOriginRefusal(request) !== null) { + throw releaseError(403, "cross_origin", "cross-origin request refused"); + } + if (admin) await enforceWriteLimit(env, rateLimitCaller(request, null), false); const actor = await resolveActor(request, env); await enforceWriteLimit(env, rateLimitCaller(request, actor.id), actor.kind !== "admin"); return actor; diff --git a/apps/web/app/routes/api.cookie-mutation-origin.test.ts b/apps/web/app/routes/api.cookie-mutation-origin.test.ts new file mode 100644 index 0000000..ac3a54c --- /dev/null +++ b/apps/web/app/routes/api.cookie-mutation-origin.test.ts @@ -0,0 +1,159 @@ +/** + * Cookie-authenticated skill mutations (favorite, vote, rate, review, and register + * when it falls back to the session) must come from this site's origin: + * `SameSite=Lax` alone still lets a sibling subdomain send the session cookie. A + * foreign `Origin` (or a cross-site `Sec-Fetch-Site`) is refused before anything is + * written (and, on the session-only routes, before the session is read); same-origin + * and header-less requests pass, and an API-key request is not subject to the check. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const h = vi.hoisted(() => { + const state = { + selectQueue: [] as unknown[], + sessionReads: 0, + writes: 0, + authMethod: "session" as "session" | "api-key", + imports: 0, + }; + + function chain(result: unknown) { + const promise = Promise.resolve(result); + const c: Record = { + from: () => c, + where: () => c, + orderBy: () => c, + limit: () => c, + values: () => c, + set: () => c, + returning: () => c, + onConflictDoUpdate: () => c, + get: () => promise, + then: (a: never, b: never) => promise.then(a, b), + }; + return c; + } + const write = (result: unknown) => { + state.writes++; + return chain(result); + }; + return { state, chain, write }; +}); + +vi.mock("~/lib/db", () => ({ + getDb: () => ({ + select: () => h.chain(h.state.selectQueue.shift() ?? []), + insert: () => h.write([{ id: "row_1" }]), + update: () => h.write(undefined), + delete: () => h.write(undefined), + run: async () => { + h.state.writes++; + }, + }), +})); +vi.mock("~/lib/auth/session-helpers", () => ({ + getSession: async () => { + h.state.sessionReads++; + return { user: { id: "u1" } }; + }, +})); +vi.mock("~/lib/auth/authenticate-request", () => ({ + authenticateRequest: async () => ({ userId: "u1", method: h.state.authMethod }), +})); +vi.mock("~/lib/github/validate-repo-ownership", () => ({ validateRepoOwnership: async () => ({ valid: true }) })); +vi.mock("~/lib/github/skill-import", () => ({ + runImport: async () => { + h.state.imports++; + return Response.json({ imported: 1 }); + }, + handleRegisterError: () => Response.json({ error: "failed" }, { status: 500 }), +})); +vi.mock("~/lib/leaderboard/recompute-skill-scores", () => ({ recomputeSkillScores: async () => {} })); +vi.mock("~/lib/db/skill-aliases", () => ({ + resolveSkillBySlug: async () => ({ id: "skill_1", slug: "s" }), +})); + +import { action as favorite } from "./api.skill-favorite"; +import { action as vote } from "./api.skill-vote"; +import { action as rate } from "./api.skill-rate"; +import { action as review } from "./api.skill-review"; +import { action as register } from "./api.skill-register"; + +type Action = (args: never) => Promise; + +const routes: Array<{ name: string; action: Action; body?: unknown; selects: unknown[] }> = [ + { name: "favorite", action: favorite as Action, selects: [[]] }, + { name: "vote", action: vote as Action, body: { type: "up" }, selects: [[{ count: 0 }], [], [{ net_votes: 1 }]] }, + { name: "rate", action: rate as Action, body: { score: 8 }, selects: [{ avgRating: 8, ratingCount: 1 }] }, + { name: "review", action: review as Action, body: { content: "Great" }, selects: [] }, +]; + +const context = { cloudflare: { env: { DB: {} } } } as never; + +async function send(route: (typeof routes)[number], headers: Record) { + h.state.selectQueue = [...route.selects]; + const request = new Request(`https://skillx.test/api/skills/s/${route.name}`, { + method: "POST", + headers: { "Content-Type": "application/json", ...headers }, + body: route.body === undefined ? undefined : JSON.stringify(route.body), + }); + const res = (await route.action({ request, params: { slug: "s" }, context } as never)) as Response; + return res.status; +} + +beforeEach(() => { + h.state.sessionReads = 0; + h.state.writes = 0; + h.state.imports = 0; + h.state.authMethod = "session"; +}); + +describe.each(routes)("POST /api/skills/:slug/$name", (route) => { + it("refuses a foreign or sibling-subdomain Origin before reading the session or writing", async () => { + for (const origin of ["https://evil.example", "https://sub.skillx.test", "http://skillx.test"]) { + expect(await send(route, { Origin: origin })).toBe(403); + } + expect(await send(route, { "Sec-Fetch-Site": "same-site" })).toBe(403); + expect(await send(route, { "Sec-Fetch-Site": "cross-site" })).toBe(403); + expect(h.state.sessionReads).toBe(0); + expect(h.state.writes).toBe(0); + }); + + it("accepts a same-origin browser request and a header-less non-browser client", async () => { + expect(await send(route, { Origin: "https://skillx.test" })).toBe(200); + expect(await send(route, { "Sec-Fetch-Site": "same-origin" })).toBe(200); + expect(await send(route, { "Sec-Fetch-Site": "none" })).toBe(200); + expect(await send(route, {})).toBe(200); + expect(h.state.writes).toBeGreaterThan(0); + }); +}); + +describe("POST /api/skills/register", () => { + async function registerWith(headers: Record) { + const request = new Request("https://skillx.test/api/skills/register", { + method: "POST", + headers: { "Content-Type": "application/json", ...headers }, + body: JSON.stringify({ owner: "org", repo: "repo" }), + }); + return ((await register({ request, params: {}, context } as never)) as Response).status; + } + + it("refuses a cookie-authenticated request from a foreign Origin before importing", async () => { + expect(await registerWith({ Origin: "https://evil.example" })).toBe(403); + expect(await registerWith({ "Sec-Fetch-Site": "cross-site" })).toBe(403); + expect(h.state.imports).toBe(0); + }); + + it("accepts a same-origin or header-less session request", async () => { + expect(await registerWith({ Origin: "https://skillx.test" })).toBe(200); + expect(await registerWith({})).toBe(200); + expect(h.state.imports).toBe(2); + }); + + it("does not apply the check to an API-key request", async () => { + h.state.authMethod = "api-key"; + expect(await registerWith({ Origin: "https://evil.example" })).toBe(200); + expect(h.state.imports).toBe(1); + }); +}); diff --git a/apps/web/app/routes/api.skill-favorite.ts b/apps/web/app/routes/api.skill-favorite.ts index 792d765..ecca7ee 100644 --- a/apps/web/app/routes/api.skill-favorite.ts +++ b/apps/web/app/routes/api.skill-favorite.ts @@ -3,6 +3,7 @@ import { getDb } from "~/lib/db"; import { favorites } from "~/lib/db/schema"; import { eq, and } from "drizzle-orm"; import { getSession } from "~/lib/auth/session-helpers"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; import { recomputeSkillScores } from "~/lib/leaderboard/recompute-skill-scores"; import { resolveSkillBySlug } from "~/lib/db/skill-aliases"; @@ -16,6 +17,9 @@ export async function action({ request, params, context }: ActionFunctionArgs) { const env = context.cloudflare.env as Env; const db = getDb(env.DB); + const crossOrigin = crossOriginRefusal(request); + if (crossOrigin) return crossOrigin; + // Require authentication const session = await getSession(request, env); if (!session?.user?.id) { diff --git a/apps/web/app/routes/api.skill-rate.ts b/apps/web/app/routes/api.skill-rate.ts index 3224130..fa3589e 100644 --- a/apps/web/app/routes/api.skill-rate.ts +++ b/apps/web/app/routes/api.skill-rate.ts @@ -4,6 +4,7 @@ import { resolveSkillBySlug } from "~/lib/db/skill-aliases"; import { skills, ratings } from "~/lib/db/schema"; import { eq, avg, count } from "drizzle-orm"; import { getSession } from "~/lib/auth/session-helpers"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; import { recomputeSkillScores } from "~/lib/leaderboard/recompute-skill-scores"; export async function action({ request, params, context }: ActionFunctionArgs) { @@ -16,6 +17,9 @@ export async function action({ request, params, context }: ActionFunctionArgs) { const env = context.cloudflare.env as Env; const db = getDb(env.DB); + const crossOrigin = crossOriginRefusal(request); + if (crossOrigin) return crossOrigin; + // Require authentication const session = await getSession(request, env); if (!session?.user?.id) { diff --git a/apps/web/app/routes/api.skill-register.ts b/apps/web/app/routes/api.skill-register.ts index 19c6194..04cfe25 100644 --- a/apps/web/app/routes/api.skill-register.ts +++ b/apps/web/app/routes/api.skill-register.ts @@ -16,6 +16,7 @@ import type { ActionFunctionArgs } from "react-router"; import { getDb } from "~/lib/db"; import { authenticateRequest } from "~/lib/auth/authenticate-request"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; import { validateRepoOwnership } from "~/lib/github/validate-repo-ownership"; import { handleRegisterError, runImport } from "~/lib/github/skill-import"; import type { RegisterMode } from "~/lib/github/skill-import"; @@ -60,6 +61,11 @@ export async function action({ request, context }: ActionFunctionArgs) { { status: 401 }, ); } + // Only a session cookie rides along ambiently; API-key clients send no Origin. + if (auth.method === "session") { + const crossOrigin = crossOriginRefusal(request); + if (crossOrigin) return crossOrigin; + } // SAFETY: request bodies are untrusted; every field is validated below. const body = (await request.json()) as RegisterBody; diff --git a/apps/web/app/routes/api.skill-review.ts b/apps/web/app/routes/api.skill-review.ts index a66e6b1..829a111 100644 --- a/apps/web/app/routes/api.skill-review.ts +++ b/apps/web/app/routes/api.skill-review.ts @@ -4,6 +4,7 @@ import { resolveSkillBySlug } from "~/lib/db/skill-aliases"; import { reviews } from "~/lib/db/schema"; import { eq, desc } from "drizzle-orm"; import { getSession } from "~/lib/auth/session-helpers"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; export async function loader({ params, context }: LoaderFunctionArgs) { try { @@ -47,6 +48,9 @@ export async function action({ request, params, context }: ActionFunctionArgs) { const env = context.cloudflare.env as Env; const db = getDb(env.DB); + const crossOrigin = crossOriginRefusal(request); + if (crossOrigin) return crossOrigin; + // Require authentication const session = await getSession(request, env); if (!session?.user?.id) { diff --git a/apps/web/app/routes/api.skill-vote.ts b/apps/web/app/routes/api.skill-vote.ts index 39b4c39..a170e97 100644 --- a/apps/web/app/routes/api.skill-vote.ts +++ b/apps/web/app/routes/api.skill-vote.ts @@ -4,6 +4,7 @@ import { resolveSkillBySlug } from "~/lib/db/skill-aliases"; import { skills, votes } from "~/lib/db/schema"; import { eq, and, sql, gt } from "drizzle-orm"; import { getSession } from "~/lib/auth/session-helpers"; +import { crossOriginRefusal } from "~/lib/auth/same-origin"; import { recomputeSkillScores } from "~/lib/leaderboard/recompute-skill-scores"; const RATE_LIMIT_WINDOW_MS = 60_000; @@ -17,6 +18,9 @@ export async function action({ request, params, context }: ActionFunctionArgs) { const env = context.cloudflare.env as Env; const db = getDb(env.DB); + const crossOrigin = crossOriginRefusal(request); + if (crossOrigin) return crossOrigin; + // Auth required const session = await getSession(request, env); if (!session?.user?.id) { diff --git a/apps/web/tests/personal-kits/kit-revisions.test.ts b/apps/web/tests/personal-kits/kit-revisions.test.ts index 8ef0f7e..da08c71 100644 --- a/apps/web/tests/personal-kits/kit-revisions.test.ts +++ b/apps/web/tests/personal-kits/kit-revisions.test.ts @@ -53,6 +53,19 @@ describe("personal kit revisions", () => { expect(lock.body.documents).toHaveLength(1); }); + it("refuses a cross-origin web-session write, then accepts the same write from this origin", async () => { + session.user = { id: "user-a" }; + const write = async (headers: Record) => + k.call("revisions", { expectedGeneration: 0, idempotencyKey: k.idem(), document: k.doc([await k.entry("@alpha/demo", "1.0.0", MINE)]) }, { key: null, headers }); + for (const headers of [{ Origin: "https://evil.example" }, { "Sec-Fetch-Site": "same-site" }]) { + const refused = await write(headers); + expect(refused.status).toBe(403); + expect(refused.body.error.code).toBe("cross_origin"); + } + // Nothing was written: generation 0 is still the current one. + expect((await write({ Origin: "https://skillx.test" })).status).toBe(201); + }); + it("refuses a client compositionDigest that the pinned bytes do not give, writing nothing", async () => { const bad = { ...(await k.entry("@alpha/demo", "1.0.0", MINE)), compositionDigest: `sha256:${"1".repeat(64)}` }; const res = await k.call("revisions", { expectedGeneration: 0, idempotencyKey: k.idem(), document: k.doc([bad]) }); diff --git a/apps/web/tests/releases/release-state-origin.test.ts b/apps/web/tests/releases/release-state-origin.test.ts new file mode 100644 index 0000000..f0c6f1b --- /dev/null +++ b/apps/web/tests/releases/release-state-origin.test.ts @@ -0,0 +1,41 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ADMIN, audit, change, expectError, publish, useStateHarness } from "./release-state-helpers"; + +/** + * A state change in a web session (no `Authorization`, no `X-Admin-Secret`) rides + * the ambient cookie, so it must come from this origin; a key or the admin secret + * is never ambient and is not checked. + */ +const session = vi.hoisted(() => ({ user: null as { id: string } | null })); +vi.mock("~/lib/auth/session-helpers", () => ({ + getSession: async () => (session.user === null ? null : { user: session.user }), +})); + +let ownerKey: string; + +beforeEach(async () => { + const h = await useStateHarness(); + session.user = null; + ownerKey = await h.createKey("user-a", null); +}); + +describe("release state: same-origin web sessions", () => { + it("refuses a cross-origin web-session request before the session or limiter decides", async () => { + await publish(); + session.user = { id: "user-a" }; + const yank = { from: "published", to: "yanked" }; + for (const headers of [{ Origin: "https://evil.example" }, { Origin: "https://sub.skillx.test" }, { "Sec-Fetch-Site": "cross-site" }]) { + expectError(await change("@alpha/demo", "1.0.0", yank, { headers }), 403, "cross_origin"); + } + expect(audit()).toEqual([]); + // An API key or the admin secret is not ambient, so a foreign Origin does not matter there. + expect((await change("@alpha/demo", "1.0.0", yank, { key: ownerKey, headers: { Origin: "https://evil.example" } })).status).toBe(200); + expect((await change("@alpha/demo", "1.0.0", { from: "yanked", to: "quarantined" }, { admin: ADMIN, headers: { Origin: "https://evil.example" } })).status).toBe(200); + }); + + it("accepts a same-origin web-session request", async () => { + await publish(); + session.user = { id: "user-a" }; + expect((await change("@alpha/demo", "1.0.0", { from: "published", to: "yanked" }, { headers: { Origin: "https://skillx.test" } })).status).toBe(200); + }); +}); diff --git a/docs/api-reference.md b/docs/api-reference.md index 2e45aca..1e47890 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -179,7 +179,9 @@ Fetch full skill details with reviews and rating summary. Rate a skill (0-10 scale). Upserts — re-rating updates existing score. -**Auth**: Required (session) +**Auth**: Required (session). Same-origin only: a request whose `Origin` differs +from the site (or, without `Origin`, whose `Sec-Fetch-Site` is not +`same-origin`/`none`) gets `403` before the session is read. **URL Params**: `slug` — skill URL slug @@ -211,6 +213,7 @@ Rate a skill (0-10 scale). Upserts — re-rating updates existing score. |--------|------|-------| | 400 | `{ "error": "Score must be a number between 0 and 10" }` | Invalid score | | 401 | `{ "error": "Authentication required" }` | Not logged in | +| 403 | `{ "error": "Cross-origin request refused" }` | `Origin` is not this site | | 404 | `{ "error": "Skill not found" }` | Invalid slug | --- @@ -219,9 +222,11 @@ Rate a skill (0-10 scale). Upserts — re-rating updates existing score. #### `POST /api/skills/:slug/vote` -Upvote or downvote a skill. Vote is toggled off if same direction already exists. +Upvote, downvote, or clear your vote on a skill. Sending the same type again keeps the vote; send `"none"` to remove it. -**Auth**: Required (session) +**Auth**: Required (session). Same-origin only: a request whose `Origin` differs +from the site (or, without `Origin`, whose `Sec-Fetch-Site` is not +`same-origin`/`none`) gets `403` before the session is read. **URL Params**: `slug` — skill URL slug @@ -229,33 +234,34 @@ Upvote or downvote a skill. Vote is toggled off if same direction already exists ```json { - "direction": "up" + "type": "up" } ``` | Field | Type | Required | Validation | |-------|------|----------|------------| -| `direction` | string | Yes | `"up"` or `"down"` | +| `type` | string | Yes | `"up"`, `"down"`, or `"none"` (clears the vote) | **Response** `200`: ```json { - "success": true, - "direction": "up", - "upvote_count": 42, - "downvote_count": 3, + "vote_type": "up", "net_votes": 39 } ``` +`vote_type` is `null` after `"none"`. `net_votes` is upvotes minus downvotes. + **Errors**: | Status | Body | Cause | |--------|------|-------| -| 400 | `{ "error": "Invalid direction" }` | Direction not `up` or `down` | +| 400 | `{ "error": "type must be 'up', 'down', or 'none'" }` | Invalid `type` | | 401 | `{ "error": "Authentication required" }` | Not logged in | +| 403 | `{ "error": "Cross-origin request refused" }` | `Origin` is not this site | | 404 | `{ "error": "Skill not found" }` | Invalid slug | +| 429 | `{ "error": "Rate limit exceeded. Max 10 votes per minute." }` | More than 10 votes in 60 s | --- @@ -288,7 +294,9 @@ List reviews for a skill, newest first (max 100). Submit a text review for a skill. -**Auth**: Required (session) +**Auth**: Required (session). Same-origin only: a request whose `Origin` differs +from the site (or, without `Origin`, whose `Sec-Fetch-Site` is not +`same-origin`/`none`) gets `403` before the session is read. **Request Body**: @@ -325,6 +333,7 @@ Submit a text review for a skill. | 400 | `{ "error": "Review content cannot be empty" }` | Empty content | | 400 | `{ "error": "Review content cannot exceed 2000 characters" }` | Too long | | 401 | `{ "error": "Authentication required" }` | Not logged in | +| 403 | `{ "error": "Cross-origin request refused" }` | `Origin` is not this site | | 404 | `{ "error": "Skill not found" }` | Invalid slug | --- @@ -335,7 +344,9 @@ Submit a text review for a skill. Toggle favorite status for a skill. Adds if not favorited, removes if already favorited. -**Auth**: Required (session) +**Auth**: Required (session). Same-origin only: a request whose `Origin` differs +from the site (or, without `Origin`, whose `Sec-Fetch-Site` is not +`same-origin`/`none`) gets `403` before the session is read. **Request Body**: None (empty body OK) @@ -354,6 +365,7 @@ Toggle favorite status for a skill. Adds if not favorited, removes if already fa | Status | Body | Cause | |--------|------|-------| | 401 | `{ "error": "Authentication required" }` | Not logged in | +| 403 | `{ "error": "Cross-origin request refused" }` | `Origin` is not this site | | 404 | `{ "error": "Skill not found" }` | Invalid slug | --- @@ -599,7 +611,9 @@ Revoke an API key (soft delete — sets `revoked_at` timestamp). Register and publish skills from a GitHub repository. Scans for SKILL.md files, validates ownership, and stores with content security scanning. -**Auth**: Required (session or API key) +**Auth**: Required (session or API key). A session-cookie request must come from +the site's own origin (same rule as the rate/vote/review/favorite routes) or it +gets `403`; API-key requests are not checked. **Request Body**: @@ -1383,7 +1397,8 @@ numbered revisions; per-user overlays and activation stay with personal kits. | POST | `/:id/clone` | Reader | `{slug?, name?, visibility?, revisionNumber?}` → 201 new private collection of the caller | - **Auth**: owner writes need a session or an unscoped account API key; scoped - keys are refused. Writes use `RELEASE_WRITE_LIMITER`, reads + keys are refused. A session write from another origin is `403 cross_origin` + before the session is read. Writes use `RELEASE_WRITE_LIMITER`, reads `RELEASE_READ_LIMITER`. - **Visibility**: `private` (default; non-owners get 404), `unlisted` and `public` (anyone may read and clone). @@ -1423,7 +1438,9 @@ pattern; a present but wrong or unconfigured secret is `401 invalid_admin_secret`, never a fall back). Otherwise the caller is the read API viewer: `Authorization: Bearer sx_...` with an unscoped account key, or the web session. Scoped `releases:publish:@handle` keys are publish-only and -answer `401 invalid_api_key`, so a leaked CI publish key cannot yank. +answer `401 invalid_api_key`, so a leaked CI publish key cannot yank. A +web-session request (no `Authorization`, no `X-Admin-Secret`) must come from the +site's own origin, or it is `403 cross_origin` before the session is read. **Rate limit**: writes count against their own binding, `RELEASE_WRITE_LIMITER`, 30 per 60 seconds per caller. A request carrying @@ -1528,7 +1545,7 @@ The publisher route answers `{"handle", "from", "to", "state", "changedAt"}`. |--------|-------| | 400 | `invalid_request` (body, states, reason), `identity.invalid`, `invalid_version` | | 401 | `auth.required`, `invalid_api_key`, `invalid_admin_secret` (with `WWW-Authenticate: Bearer`) | -| 403 | `transition.not_owner` (public package), `transition.admin_only`, `publisher_revoked` | +| 403 | `cross_origin` (web session from another origin), `transition.not_owner` (public package), `transition.admin_only`, `publisher_revoked` | | 404 | `not_found` (unknown release or publisher; a private package of another owner; API disabled) | | 409 | `state_conflict` | | 413 | `request_too_large` | @@ -1545,7 +1562,9 @@ not_found` while off; other methods `405 method_not_allowed`), same admin secret check and same `RELEASE_WRITE_LIMITER` counting as a state change (client IP first, then `user:admin`, before the body is read). Without `X-Admin-Secret` an account (unscoped key or session) is `403 -publisher.admin_only`; no credentials at all is `401 auth.required`. +publisher.admin_only`; no credentials at all is `401 auth.required`. A +request with neither `Authorization` nor `X-Admin-Secret` from another origin +is `403 cross_origin` before either check. **Request**: `Content-Type: application/json`, at most 4 KiB: @@ -1579,7 +1598,7 @@ the same batch: `subject_kind` `publisher`, actor `admin`, `from_state` `none`, `to_state` `active`, outcome `applied`. **Errors**: `400 invalid_request`, `401 auth.required` / `invalid_api_key` / -`invalid_admin_secret`, `403 publisher.admin_only`, `404 not_found` (API +`invalid_admin_secret`, `403 publisher.admin_only` / `cross_origin`, `404 not_found` (API disabled) / `user.not_found`, `409 publisher.conflict`, `413 request_too_large`, `415 unsupported_media_type`, `429 rate_limited`, `500 release.internal_error`, `503 rate_limit_unavailable` (an account request @@ -1635,8 +1654,8 @@ release writes a `release_state_audit` row (`subject_kind` `release_channel`, `{"identity", "version", "releaseDigest", "from", "to", "channel", "changedAt"}`. **Errors**: `400 invalid_request`, `401`, `403 transition.not_owner` / -`promote.demotion_forbidden` / `publisher_revoked` / `auth.scope_missing`, -`404 not_found` (unknown, private and not owned, or API disabled), `409 +`promote.demotion_forbidden` / `publisher_revoked` / `auth.scope_missing` / +`cross_origin` (web session from another origin), `404 not_found` (unknown, private and not owned, or API disabled), `409 promote.not_published` / `state_conflict`, `422 promote.same_channel`, `429 rate_limited`. diff --git a/docs/codebase-summary.md b/docs/codebase-summary.md index 8e8ec07..d9a5922 100644 --- a/docs/codebase-summary.md +++ b/docs/codebase-summary.md @@ -115,7 +115,7 @@ skillx/ | `ratings` | id, skill_id, user_id, score, is_agent, timestamps | 0-10 scores | | `reviews` | id, skill_id, user_id, content, is_agent, created_at | Text feedback | | `favorites` | user_id, skill_id, created_at | Many-to-many bookmarks | -| `votes` | id, skill_id, user_id, direction (up/down), created_at, updated_at | Upvote/downvote tracking (deduplicated per user) | +| `votes` | id, skill_id, user_id, vote_type (up/down), created_at, updated_at | Upvote/downvote tracking (deduplicated per user) | | `installs` | id, skill_id, user_id, device_id, created_at | Install tracking (deduplicated per user/device) | | `usageStats` | id, skill_id, user_id, model, outcome, duration_ms, created_at | Execution tracking | | `apiKeys` | id, user_id, name, key_hash, key_prefix, last_used_at, revoked_at, created_at | API authentication | diff --git a/docs/personal-kits-api.md b/docs/personal-kits-api.md index 9f748ef..a7272fc 100644 --- a/docs/personal-kits-api.md +++ b/docs/personal-kits-api.md @@ -21,7 +21,9 @@ without an explicit user approval. - Callers authenticate with a session or an **unscoped** API key. Scoped keys, such as `releases:publish:@h`, get 401. The admin secret is not accepted - (404). + (404). A write in a web session (no `Authorization` header) must come from + the site's own origin; otherwise it gets 403 `cross_origin` before the + session is read. - A kit is visible only to its owner. Every other caller gets the JSON 404, never 403, so a kit's existence is not revealed. - Every write (create, store, rebase, resolve, activate, reject) requires @@ -534,7 +536,7 @@ or file content. Its rollback SQL is in its header. | `kit.generation_conflict`, `kit.idempotency_conflict`, `kit.revision_exists`, `kit.composition_digest_mismatch`, `overlay.conflict`, `kit.approval_required`, `kit.review_required`, `kit.revision_unusable`, `kit.revision_effective`, `kit.discard_unconfirmed`, `kit.conflict_closed`, `kit.locked_incompatible`, `kit.not_a_proposal`, `proposal.private_kit_required` | 409 | | `kit.generation_conflict` when the stale generation came with `If-Match` | 412 | | `proposal.invalid`, `proposal.forbidden_field`, `proposal.private_only`, `proposal.evidence_too_large`, `proposal.unknown_skill`, `proposal.base_mismatch`, `proposal.content_refused` (`findings`), `proposal.undeclared_permission` (`required`, `missing`), `proposal.excess_permission` (`required`, `extra`) | 422 | -| `kit.admin_only`, `kit.interactive_approval_required` | 403 | +| `kit.admin_only`, `kit.interactive_approval_required`, `cross_origin` | 403 | | `kit.invalid`, `overlay.invalid`, `schema.unsupported_version`, `kit.duplicate_identity`, `kit.mode_violation`, `kit.alias_collision`, `overlay.shared_alias`, `identity.reserved_alias`, `kit.request_too_large`, `kit.cycle`, `kit.multi_parent`, `kit.public_derivative_forbidden`, `kit.base_unavailable` (409 when resolving a conflict) | 422 | | `kit.publisher_not_owned` | 403 | | `invalid_request` (malformed body, ids, updates or `If-Match`) | 400 | diff --git a/docs/system-architecture.md b/docs/system-architecture.md index c379ff5..d59a3d2 100644 --- a/docs/system-architecture.md +++ b/docs/system-architecture.md @@ -324,20 +324,22 @@ Response: | GET | `/api/search` | Session/Key | Search skills | | GET | `/api/leaderboard` | None | Leaderboard with sorting & category filter | | GET | `/api/skills/:slug` | None | Skill detail + references + scripts (public) | -| GET | `/api/skills/:slug/references` | None | List skill references (NEW) | -### Protected Endpoints (Session or API Key) +### Protected Endpoints -| Method | Path | Purpose | -|--------|------|---------| -| POST | `/api/skills/:slug/rate` | Submit rating | -| POST | `/api/skills/:slug/review` | Write review | -| POST | `/api/skills/:slug/favorite` | Add/remove favorite | -| POST | `/api/skills/:slug/vote` | Upvote/downvote skill | -| POST | `/api/skills/:slug/install` | Track install (fire-and-forget) | -| POST | `/api/skills/:slug/references` | Add reference to skill (NEW) | -| POST | `/api/skills/register` | Register/publish skills from GitHub repo (validates write access) | -| POST | `/api/report` | Report usage | +Session-cookie mutations must come from the site's own origin +(`crossOriginRefusal` in `app/lib/auth/same-origin.ts`); a foreign `Origin` +gets `403` before the session is read. API-key requests are not checked. + +| Method | Path | Auth | Purpose | +|--------|------|------|---------| +| POST | `/api/skills/:slug/rate` | Session | Submit rating | +| POST | `/api/skills/:slug/review` | Session | Write review | +| POST | `/api/skills/:slug/favorite` | Session | Add/remove favorite | +| POST | `/api/skills/:slug/vote` | Session | Upvote/downvote/clear vote | +| POST | `/api/skills/:slug/install` | Optional key or `X-Device-Id` | Track install (fire-and-forget) | +| POST | `/api/skills/register` | Session or key | Register/publish skills from GitHub repo (validates write access) | +| POST | `/api/report` | API key | Report usage | ### User Endpoints (Session Only) @@ -365,22 +367,22 @@ Response: ``` User clicks up/down arrow on skill card/row ↓ -POST /api/skills/:slug/vote { direction: "up" | "down" } +POST /api/skills/:slug/vote { type: "up" | "down" | "none" } ↓ -[Authenticate via session] +[Same-origin check, then authenticate via session; 10 votes/min per user] ↓ Check existing vote: SELECT * FROM votes WHERE user_id = ? AND skill_id = ? ↓ - ├─ If exists & same direction → DELETE (toggle off) + ├─ type "none" → DELETE the vote if any │ - └─ Otherwise → INSERT OR REPLACE vote + └─ Otherwise → UPDATE the existing vote_type, or INSERT a new vote ↓ UPDATE skills SET - upvote_count = (SELECT COUNT(*) FROM votes WHERE skill_id = ? AND direction = 'up'), - downvote_count = (SELECT COUNT(*) FROM votes WHERE skill_id = ? AND direction = 'down'), - net_votes = upvote_count - downvote_count + upvote_count = (SELECT COUNT(*) FROM votes WHERE skill_id = ? AND vote_type = 'up'), + downvote_count = (SELECT COUNT(*) FROM votes WHERE skill_id = ? AND vote_type = 'down'), + net_votes = upvotes - downvotes ↓ - Response: { direction, upvote_count, downvote_count, net_votes } + Response: { vote_type, net_votes } ``` ### Scoring Impact @@ -395,30 +397,9 @@ Check existing vote: SELECT * FROM votes WHERE user_id = ? AND skill_id = ? ### References Management (NEW) -**Adding a Reference:** - -``` -POST /api/skills/:slug/references -{ title, filename, url, type: "doc"|"link"|"example", content? } - ↓ -[Authenticate: session or API key] - ↓ -INSERT INTO skill_references (skill_id, title, filename, url, type, content, ...) - ↓ -[Optionally: index in Vectorize (title + first 100 tokens of content)] - ↓ -Response: { id, skill_id, title, url, type, created_at } -``` - -**Retrieving References:** - -``` -GET /api/skills/:slug/references - ↓ -SELECT * FROM skill_references WHERE skill_id = ? ORDER BY created_at DESC - ↓ -Response: [{ id, title, filename, url, type, created_at }, ...] -``` +There is no references endpoint. References are written by admin seeding +(`skill_references` rows) and read through `GET /api/skills/:slug`, whose +response carries the `references` array (title, url, type). **On Skill Detail Page:**