Implement overall stability fixes including session store injection

2026-06-30 11:31:41 +02:00
parent f2c390f0a8
commit ae0da2d620
3 changed files with 236 additions and 10 deletions
@@ -16,14 +16,23 @@ import {createPersonasRouter} from "./routes/personas.js";
 import {createSettingsRouter} from "./routes/settings.js";
 import {createSystemRouter} from "./routes/system.js";

-function createCloseHandler(resources) {
+export function createCloseHandler(resources) {
    let closed = false;
    return async () => {
        if (closed) return;
        closed = true;

+        const errors = [];
        for (const close of resources) {
+            try {
                await close();
+            } catch (error) {
+                errors.push(error);
+            }
+        }
+
+        if (errors.length) {
+            throw new AggregateError(errors, "Failed to close one or more resources");
        }
    };
 }
@@ -82,21 +91,59 @@ function errorHandler(err, req, res, next) {
        return next(err);
    }

-    const status = Number.isInteger(err.status) ? err.status : 500;
+    if (err?.type === "entity.parse.failed") {
+        return res.status(400).json({error: "Invalid JSON"});
+    }
+
+    const status = Number.isInteger(err.status) ? err.status : Number.isInteger(err.statusCode) ? err.statusCode : 500;
    const message = status >= 500 ? "Internal server error" : err.message;
+    if (status >= 500) {
        console.warn("REQUEST ERROR", status, req.method, req.url, err);
+    } else {
+        console.warn("REQUEST ERROR", status, req.method, req.url, message);
+    }
    if (wantsJson(req)) {
        return res.status(status).json({error: message});
    }
    return res.status(status).type("text").send(message);
 }

+function sameOriginProtection(config) {
+    return (req, res, next) => {
+        if (!config.sameOriginProtectionEnabled || !["POST", "PUT", "PATCH", "DELETE"].includes(req.method)) {
+            return next();
+        }
+
+        const secFetchSite = req.get("Sec-Fetch-Site");
+        if (secFetchSite && !["same-origin", "none"].includes(secFetchSite)) {
+            return res.status(403).json({error: "Cross-origin request blocked"});
+        }
+
+        const origin = req.get("Origin");
+        if (!origin) return next();
+
+        let parsedOrigin;
+        try {
+            parsedOrigin = new URL(origin);
+        } catch {
+            return res.status(403).json({error: "Cross-origin request blocked"});
+        }
+
+        if (parsedOrigin.protocol !== `${req.protocol}:` || parsedOrigin.host !== req.get("Host")) {
+            return res.status(403).json({error: "Cross-origin request blocked"});
+        }
+
+        return next();
+    };
+}
+
 export function createApp(options = {}) {
    const config = createConfig(options.config);
    const app = express();
    app.set("trust proxy", config.trustProxy);

    if (config.dbPath !== ":memory:") {
+        fs.mkdirSync(config.dataDir, {recursive: true});
        fs.mkdirSync(config.sessionsDir, {recursive: true});
    }

@@ -116,9 +163,13 @@ export function createApp(options = {}) {
    );

    app.use(express.json({limit: "1mb"}));
-    app.use(express.static(config.publicDir));
+    app.use(express.static(config.publicDir, {
+        maxAge: config.staticMaxAge,
+        etag: config.staticEtag,
+        fallthrough: config.staticFallthrough
+    }));
    app.use(session({
-        store: config.testMode ? undefined : new FileStore({path: config.sessionsDir, reapInterval: 3600}),
+        store: options.sessionStore || (config.testMode ? undefined : new FileStore({path: config.sessionsDir, reapInterval: 3600})),
        secret: config.sessionSecret,
        resave: false,
        saveUninitialized: false,
@@ -129,6 +180,7 @@ export function createApp(options = {}) {
            maxAge: config.sessionMaxAgeMs
        }
    }));
+    app.use(sameOriginProtection(config));

    app.locals.config = config;
    app.locals.db = db;
@@ -155,8 +207,39 @@ export function startServer(options = {}) {
        console.log(`Server running at http://localhost:${port}`);
    });
    const close = async () => {
+        const errors = [];
+        try {
            await new Promise((resolve, reject) => server.close((error) => error ? reject(error) : resolve()));
+        } catch (error) {
+            errors.push(error);
+        }
+        try {
            await app.locals.close();
+        } catch (error) {
+            if (error instanceof AggregateError) {
+                errors.push(...error.errors);
+            } else {
+                errors.push(error);
+            }
+        }
+        if (errors.length) {
+            throw new AggregateError(errors, "Failed to close server cleanly");
+        }
    };
+    if (options.handleSignals !== false) {
+        let shuttingDown = false;
+        const shutdown = (signal) => {
+            if (shuttingDown) return;
+            shuttingDown = true;
+            close()
+                .then(() => process.exit(0))
+                .catch((error) => {
+                    console.error(`Failed to shut down after ${signal}`, error);
+                    process.exit(1);
+                });
+        };
+        process.once("SIGINT", shutdown);
+        process.once("SIGTERM", shutdown);
+    }
    return {app, server, close};
 }
@@ -37,6 +37,10 @@ export function createConfig(overrides = {}) {
        allowInsecureCookies: bool(process.env.ALLOW_INSECURE_COOKIES, false),
        trustProxy: trustProxy(process.env.TRUST_PROXY, false),
        sessionMaxAgeMs: int(process.env.SESSION_MAX_AGE_MS, 7 * 24 * 60 * 60 * 1000),
+        sameOriginProtectionEnabled: bool(process.env.SAME_ORIGIN_PROTECTION_ENABLED, true),
+        staticMaxAge: process.env.STATIC_MAX_AGE || (process.env.NODE_ENV === "production" ? "1h" : 0),
+        staticEtag: bool(process.env.STATIC_ETAG, true),
+        staticFallthrough: bool(process.env.STATIC_FALLTHROUGH, true),
        eventLoopLagMonitorEnabled: bool(process.env.EVENT_LOOP_LAG_MONITOR_ENABLED, !testMode),
        eventLoopLagWarnMs: int(process.env.EVENT_LOOP_LAG_WARN_MS, 50),
        slowRequestLoggingEnabled: bool(process.env.SLOW_REQUEST_LOGGING_ENABLED, !testMode),
@@ -1,6 +1,10 @@
 import test from "node:test";
 import assert from "node:assert/strict";
-import {createApp} from "../src/app.js";
+import fs from "fs";
+import os from "os";
+import path from "path";
+import session from "express-session";
+import {createApp, createCloseHandler} from "../src/app.js";

 class CookieJar {
    constructor() {
@@ -55,7 +59,8 @@ async function startTestServer(overrides = {}) {
            sessionSecret: "test-secret",
            ...(overrides.config || {})
        },
-        modelService
+        modelService,
+        sessionStore: overrides.sessionStore
    });

    const server = await new Promise((resolve) => {
@@ -118,6 +123,140 @@ test("auth flow validates inputs and persists session", async () => {
    await server.close();
 });

+test("createApp can use an injected session store", async () => {
+    class CountingStore extends session.Store {
+        constructor() {
+            super();
+            this.sessions = new Map();
+            this.setCount = 0;
+        }
+
+        get(sid, callback) {
+            callback(null, this.sessions.get(sid));
+        }
+
+        set(sid, value, callback) {
+            this.setCount += 1;
+            this.sessions.set(sid, value);
+            callback();
+        }
+
+        destroy(sid, callback) {
+            this.sessions.delete(sid);
+            callback();
+        }
+    }
+
+    const store = new CountingStore();
+    const server = await startTestServer({sessionStore: store});
+    const jar = new CookieJar();
+
+    await request(server.baseUrl, "/register", {
+        method: "POST",
+        body: {username: "alice", password: "password123"}
+    }).then(({status}) => assert.equal(status, 200));
+
+    await request(server.baseUrl, "/login", {
+        method: "POST",
+        body: {username: "alice", password: "password123"},
+        jar
+    }).then(({status}) => assert.equal(status, 200));
+
+    assert.equal(store.setCount > 0, true);
+    await server.close();
+});
+
+test("invalid JSON returns a clean 400 response", async () => {
+    const server = await startTestServer();
+
+    const response = await fetch(`${server.baseUrl}/register`, {
+        method: "POST",
+        headers: {"Content-Type": "application/json"},
+        body: "{"
+    });
+    const json = await response.json();
+
+    assert.equal(response.status, 400);
+    assert.deepEqual(json, {error: "Invalid JSON"});
+    await server.close();
+});
+
+test("cross-origin state-changing requests are blocked", async () => {
+    const server = await startTestServer();
+
+    const response = await fetch(`${server.baseUrl}/register`, {
+        method: "POST",
+        headers: {
+            "Content-Type": "application/json",
+            Origin: "https://example.test"
+        },
+        body: JSON.stringify({username: "alice", password: "password123"})
+    });
+    const json = await response.json();
+
+    assert.equal(response.status, 403);
+    assert.deepEqual(json, {error: "Cross-origin request blocked"});
+    await server.close();
+});
+
+test("file-backed startup creates data and session directories", async () => {
+    const rootDir = fs.mkdtempSync(path.join(os.tmpdir(), "krishd-app-"));
+    const dataDir = path.join(rootDir, "data");
+    const sessionsDir = path.join(rootDir, "sessions");
+    const app = createApp({
+        config: {
+            rootDir,
+            dataDir,
+            sessionsDir,
+            dbPath: path.join(dataDir, "app.db"),
+            testMode: true,
+            sessionSecret: "test-secret"
+        },
+        modelService: {
+            async generateReply() {},
+            async streamReply() {},
+            async listModels() {
+                return {models: []};
+            },
+            async checkHealth() {
+                return {ok: true, checkedAt: new Date().toISOString(), error: null};
+            },
+            mapError(error) {
+                return {status: 500, body: {error: error.message}};
+            }
+        }
+    });
+
+    assert.equal(fs.existsSync(dataDir), true);
+    assert.equal(fs.existsSync(sessionsDir), true);
+    await app.locals.close();
+});
+
+test("close handler attempts all resources and reports collected failures", async () => {
+    const closed = [];
+    const close = createCloseHandler([
+        () => {
+            closed.push("first");
+            throw new Error("first failed");
+        },
+        () => {
+            closed.push("second");
+        },
+        () => {
+            closed.push("third");
+            throw new Error("third failed");
+        }
+    ]);
+
+    await assert.rejects(close, (error) => {
+        assert.equal(error instanceof AggregateError, true);
+        assert.equal(error.errors.length, 2);
+        assert.deepEqual(error.errors.map((item) => item.message), ["first failed", "third failed"]);
+        return true;
+    });
+    assert.deepEqual(closed, ["first", "second", "third"]);
+});
+
 test("chat creation, send, and retry endpoints work with stubbed model service", async () => {
    const server = await startTestServer();
    const jar = new CookieJar();