fix: final-review wave (CSRF guard, fail-closed owner bootstrap, streamed zips, e2e visibility suite)
- Critical 1 (CSRF): core wrap refuses every POST/PUT/PATCH/DELETE whose Origin (Referer as fallback) is not publicUrl's origin or STC_ALLOWED_ORIGINS, and any body that is not application/json (multipart only on declared upload routes). Better Auth routes go through the same guard; only declared identity-provider callbacks (SAML ACS/SLO, OAuth callbacks) are exempt. Better Auth trustedOrigins use the same list. Session cookie stays Lax (Strict breaks OAuth/OIDC/SAML returns); documented. - Important 4 / learner minor 4: the first-owner claim and the role row are one transaction guarded by "no owner yet"; only a unique violation means learner; any other error fails the sign-up with 500 and removes the half-created user. Serialised per DB handle (Bun's SQLite connection). - Important 2 / learner minor 12: code zips stream entry by entry (large files stored in 256 KB chunks with a data descriptor and incremental CRC-32), so memory stays at one chunk; the cap is now 1 GiB (ZIP32). - Important 3 / learner minor 10: the e2e runs the production wiring (productFeatures order, AI drafts through the real sink) against a restricted course and asserts 404/401 for learner and anonymous on course, lesson, media, cover, code, zip, notes, progress, attempts, AI chat, library, search and AI status; a real Range request (206, 416); forged writes (foreign Origin, text/plain, no Origin) are 403 and the MCP test command never runs. - Minors 7-9: compose passes every documented variable; the health check uses STC_PORT; productFeatures checks the studio-before-AI order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
69aaedcb43
commit
8045c80d93
@@ -22,6 +22,47 @@ export function canView(user: AuthUser | null, course: LoadedCourse, acl: { prin
|
||||
|
||||
const json = (body: unknown, status = 200) => Response.json(body, { status });
|
||||
|
||||
const WRITES = new Set(["POST", "PUT", "PATCH", "DELETE"]);
|
||||
|
||||
/** The request's origin from `Origin`, else from `Referer`; null when neither names one ("null" counts as none). */
|
||||
function requestOrigin(req: Request): string | null {
|
||||
const o = req.headers.get("origin");
|
||||
if (o && o !== "null") return o.toLowerCase();
|
||||
const r = req.headers.get("referer");
|
||||
if (r) { try { const u = new URL(r).origin; return u === "null" ? null : u; } catch { return null; } }
|
||||
return null;
|
||||
}
|
||||
|
||||
const hasBody = (req: Request) => {
|
||||
const len = req.headers.get("content-length");
|
||||
if (len !== null) return Number(len) > 0;
|
||||
return req.headers.has("transfer-encoding") || req.body !== null;
|
||||
};
|
||||
|
||||
/**
|
||||
* Cross-site request guard for every state-changing request (POST, PUT, PATCH, DELETE), applied once here for all
|
||||
* features, Better Auth included:
|
||||
* - the `Origin` (or, without it, the `Referer`) must be one of `config.allowedOrigins` (publicUrl's origin plus
|
||||
* STC_ALLOWED_ORIGINS); missing or foreign gives 403;
|
||||
* - a request with a body must be `application/json`, or `multipart/form-data` on the feature's `multipartRoutes`.
|
||||
* Forms and text/plain bodies (what a hostile page can send without a preflight) are refused with 403.
|
||||
* Paths in a feature's `externalCallbacks` (identity-provider posts) are exempt and verify themselves.
|
||||
* Returns the refusal, or null to proceed.
|
||||
*/
|
||||
export function crossSiteRefusal(req: Request, allowedOrigins: string[], opts: { multipart: boolean; external: boolean }): Response | null {
|
||||
if (!WRITES.has(req.method) || opts.external) return null;
|
||||
const origin = requestOrigin(req);
|
||||
if (!origin || !allowedOrigins.includes(origin)) {
|
||||
return json({ error: "cross-site request refused: the Origin must be this server's public URL (STC_PUBLIC_URL or STC_ALLOWED_ORIGINS)" }, 403);
|
||||
}
|
||||
if (hasBody(req)) {
|
||||
const type = (req.headers.get("content-type") ?? "").split(";")[0]!.trim().toLowerCase();
|
||||
const ok = type === "application/json" || (opts.multipart && type === "multipart/form-data");
|
||||
if (!ok) return json({ error: opts.multipart ? "send multipart/form-data or application/json" : "send application/json" }, 403);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
export function createApp(ctx: ServerContext, features: Feature[]): { routes: Routes; fetch(req: Request): Promise<Response> } {
|
||||
const acl = aclRepo(ctx.db);
|
||||
|
||||
@@ -48,19 +89,25 @@ export function createApp(ctx: ServerContext, features: Feature[]): { routes: Ro
|
||||
},
|
||||
};
|
||||
|
||||
const wrap = (h: RouteHandler) => async (req: Request & { params?: Record<string, string> }): Promise<Response> => {
|
||||
// No configuration means no allowed origin: every write is refused (fail closed).
|
||||
const allowed = ctx.config?.allowedOrigins ?? [];
|
||||
const external = features.flatMap((f) => f.externalCallbacks ?? []);
|
||||
const wrap = (h: RouteHandler, multipart: boolean) => async (req: Request & { params?: Record<string, string> }): Promise<Response> => {
|
||||
const url = new URL(req.url);
|
||||
const refused = crossSiteRefusal(req, allowed, { multipart, external: external.some((p) => url.pathname.startsWith(p)) });
|
||||
if (refused) return refused;
|
||||
const rc: RequestContext = { user: await ctx.auth.userFromRequest(req), params: req.params ?? {}, url };
|
||||
return h(req, rc);
|
||||
};
|
||||
|
||||
const routes: Routes = {};
|
||||
for (const f of [core, ...features]) {
|
||||
const multipart = new Set(f.multipartRoutes ?? []);
|
||||
for (const [path, methods] of Object.entries(f.routes)) {
|
||||
const slot = (routes[path] ??= {});
|
||||
for (const [m, h] of Object.entries(methods) as [Method, RouteHandler][]) {
|
||||
if (slot[m]) throw new Error(`Duplicate route ${m} ${path} (feature "${f.name}")`);
|
||||
slot[m] = wrap(h);
|
||||
slot[m] = wrap(h, multipart.has(path));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -8,6 +8,15 @@ export type ConfigInput = Pick<AppConfig, "dataDir"> & Partial<Omit<AppConfig, "
|
||||
|
||||
// Path lists are split on ":" only (paths may contain spaces); URL lists are trimmed.
|
||||
const paths = (raw: string | undefined) => (raw ?? "").split(":").filter(Boolean);
|
||||
/** `scheme://host[:port]` of a URL, lower-cased by URL parsing; null when it does not parse. */
|
||||
export function originOf(u: string): string | null {
|
||||
try {
|
||||
const o = new URL(u).origin;
|
||||
return o === "null" ? null : o;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
const urls = (raw: string | undefined) => (raw ?? "").split(",").map((s) => s.trim()).filter(Boolean);
|
||||
|
||||
/**
|
||||
@@ -24,13 +33,20 @@ export function resolveConfig(input: ConfigInput): AppConfig {
|
||||
if (!courseDirs.some((c) => resolve(c) === resolve(d))) courseDirs.push(d);
|
||||
}
|
||||
const port = input.port ?? 3000;
|
||||
const publicUrl = input.publicUrl ?? `http://localhost:${port}`;
|
||||
const origins: string[] = [];
|
||||
for (const o of [publicUrl, ...(input.allowedOrigins ?? [])]) {
|
||||
const n = originOf(o);
|
||||
if (n && !origins.includes(n)) origins.push(n);
|
||||
}
|
||||
return {
|
||||
port,
|
||||
databaseUrl: input.databaseUrl ?? "sqlite://./data/stcourses.db",
|
||||
dataDir,
|
||||
courseDirs,
|
||||
secret: input.secret ?? DEV_SECRET,
|
||||
publicUrl: input.publicUrl ?? `http://localhost:${port}`,
|
||||
publicUrl,
|
||||
allowedOrigins: origins,
|
||||
importedCoursesDir,
|
||||
studioReposDir: input.studioReposDir ?? resolve(dataDir, "studio", "repos"),
|
||||
studioWorktreesDir,
|
||||
@@ -71,6 +87,7 @@ export function loadConfig(env: Record<string, string | undefined> = process.env
|
||||
aiAllowedBaseUrls: urls(env.STC_AI_ALLOWED_BASE_URLS),
|
||||
transcribeUrl: env.STC_TRANSCRIBE_URL || null,
|
||||
python: env.STC_PYTHON || "python3",
|
||||
allowedOrigins: [...urls(env.STC_ALLOWED_ORIGINS), ...urls(env.STC_TRUSTED_ORIGINS)],
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
export { loadConfig, resolveConfig, cachePaths, type ConfigInput } from "./config";
|
||||
export { loadConfig, resolveConfig, cachePaths, originOf, type ConfigInput } from "./config";
|
||||
export { createJobQueue } from "./jobs";
|
||||
export { serveMedia, contentType, contentDisposition, mediaSecurityHeaders, MEDIA_SANDBOX_CSP } from "./media";
|
||||
export { createApp, canView, type Routes } from "./app";
|
||||
export { createApp, canView, crossSiteRefusal, type Routes } from "./app";
|
||||
export { composeServer, type ComposeOptions, type ComposedServer } from "./compose";
|
||||
|
||||
Reference in New Issue
Block a user