diff --git a/packages/plugin-sdk/src/backend.ts b/packages/plugin-sdk/src/backend.ts index 92e153caa..6ea70c0f8 100644 --- a/packages/plugin-sdk/src/backend.ts +++ b/packages/plugin-sdk/src/backend.ts @@ -339,6 +339,9 @@ export interface PluginRouterOptions { * and matched exactly ("/callback", not "/plugin-api//callback"). A * ":name" segment matches one segment, and a trailing "/*" matches any * number of further segments, for a proxied page ("/chooser/:id/*"). + * An entry only opens the route registered with that same path, for the + * methods it was registered with, so a literal route beside a ":param" + * ("/webhook/config" next to "/webhook/:token") still needs a login. * * For the handful of routes an unauthenticated third party has to reach: an * OIDC callback, an inbound webhook. Every entry is audited when the router @@ -1859,8 +1862,9 @@ export interface PluginBinarySpec { /** Hex SHA-256 the file must have. */ sha256: string; /** - * Paths checked before downloading, such as a copy baked into the Docker - * image. One whose checksum matches is used in place. + * Absolute paths checked before downloading, such as a copy baked into the + * Docker image. Only a regular file named `name` is considered, and one + * whose checksum matches is used in place. */ prebuilt?: readonly string[]; } diff --git a/packages/plugin-sdk/src/manifest.ts b/packages/plugin-sdk/src/manifest.ts index 351839ed3..010783100 100644 --- a/packages/plugin-sdk/src/manifest.ts +++ b/packages/plugin-sdk/src/manifest.ts @@ -434,7 +434,8 @@ export interface PluginProtocolContribution { defaultPort?: number; /** * What the owner's login falls back to from the host's own SSH login - * when the protocol login leaves it empty. + * when the protocol login leaves it empty. The password only falls back + * while the host signs in to SSH with a password. */ hostLoginFallback?: Array<"username" | "password">; } diff --git a/src/backend/database/routes/plugins.ts b/src/backend/database/routes/plugins.ts index cfa19f433..675077d38 100644 --- a/src/backend/database/routes/plugins.ts +++ b/src/backend/database/routes/plugins.ts @@ -87,7 +87,8 @@ router.get("/public", async (_req: Request, res: Response) => { { id: record.id, name: record.name, - version: record.version, + // Not before login: it tells a scanner which known bugs apply. + version: "", enabled: true, state: loaded?.state ?? record.state, contributes: { @@ -121,7 +122,8 @@ router.get("/public", async (_req: Request, res: Response) => { * that plugins provide, before anyone has signed in. Returns only * enabled plugins that contribute login methods or second factors, with * the fields the browser loader needs and the ids they contribute. - * Nothing operational: no capabilities, grants, settings or errors. + * Nothing operational: no versions, capabilities, grants, settings or + * errors. * tags: * - Plugins * responses: @@ -165,7 +167,7 @@ export async function listPreLoginPlugins(): Promise< { id: record.id, name: record.name, - version: record.version, + version: "", enabled: true, state: loaded.state, contributes: { auth: { loginMethods, secondFactors } }, diff --git a/src/backend/hosts/protocol-auth/protocol-auth.ts b/src/backend/hosts/protocol-auth/protocol-auth.ts index 2d16ab80f..f0c845c17 100644 --- a/src/backend/hosts/protocol-auth/protocol-auth.ts +++ b/src/backend/hosts/protocol-auth/protocol-auth.ts @@ -511,7 +511,10 @@ export async function resolveOwnerProtocolLogin( const fallback = declared.hostLoginFallback ?? []; if (fallback.includes("username")) username ||= str(host.username); - if (fallback.includes("password")) password ||= str(host.password); + // A key or credential host can still carry an old password; never send it. + if (fallback.includes("password") && host.authType === "password") { + password ||= str(host.password); + } const secretFields: Record = {}; for (const [key, value] of Object.entries(login?.secretFields ?? {})) { diff --git a/src/backend/plugins/ctx-process.ts b/src/backend/plugins/ctx-process.ts index c72e015c1..8256f2978 100644 --- a/src/backend/plugins/ctx-process.ts +++ b/src/backend/plugins/ctx-process.ts @@ -41,6 +41,30 @@ interface Deps { const SHA256_PATTERN = /^[a-f0-9]{64}$/; const SAFE_NAME = /^[A-Za-z0-9._-]+$/; +/** Larger than any CLI a plugin ships, small enough not to stall a boot. */ +const MAX_PREBUILT_BYTES = 512 * 1024 * 1024; + +/** + * Only a regular file carrying the binary's own name is hashed, so a plugin + * cannot use prebuilt to fingerprint arbitrary files on the server. + */ +async function isPrebuiltCandidate( + candidate: string, + name: string, +): Promise { + if (!path.isAbsolute(candidate) || path.basename(candidate) !== name) { + return false; + } + try { + const real = await fs.realpath(candidate); + if (path.basename(real) !== name) return false; + const stat = await fs.stat(real); + return stat.isFile() && stat.size <= MAX_PREBUILT_BYTES; + } catch { + return false; + } +} + async function sha256Of(file: string): Promise { try { const hash = createHash("sha256"); @@ -217,6 +241,7 @@ export function createPluginProcess(deps: Deps): PluginProcess { } for (const candidate of spec.prebuilt ?? []) { + if (!(await isPrebuiltCandidate(candidate, spec.name))) continue; if ((await sha256Of(candidate)) === expected) return candidate; } diff --git a/src/backend/plugins/http.ts b/src/backend/plugins/http.ts index b7a2196aa..695a4791b 100644 --- a/src/backend/plugins/http.ts +++ b/src/backend/plugins/http.ts @@ -162,17 +162,81 @@ function compilePublicPath(path: string): RegExp { return new RegExp(prefix ? `^${escaped}(?:/.*)?$` : `^${escaped}$`); } +interface PublicPath { + declared: string; + pattern: RegExp; +} + +interface RouterLayer { + route?: { + path?: unknown; + _handlesMethod?: (method: string) => boolean; + }; + match?: (path: string) => boolean; +} + +function segments(path: string): string[] { + return normalizePublicPath(path).split("/").slice(1); +} + /** - * Matches the request path against the declared public paths. - * - * Full-path match only. A prefix match would turn one declared route into a - * hole covering everything below it, which is not what declaring a single - * public path should mean. + * Whether a declared public path names this registered route: literal + * segments must be equal and a ":param" must sit where the route has one, so + * "/webhook/:token" never covers a sibling "/webhook/config". */ -function isPublicRequest(req: Request, publicPaths: RegExp[]): boolean { +function declaredCoversRoute(declared: string, routePath: string): boolean { + const prefix = declared.endsWith("/*"); + const want = segments(prefix ? declared.slice(0, -2) : declared); + const have = segments(routePath); + if (prefix ? have.length < want.length : have.length !== want.length) { + return false; + } + return want.every((segment, index) => + segment.startsWith(":") + ? have[index].startsWith(":") + : segment.toLowerCase() === have[index].toLowerCase(), + ); +} + +/** + * Whether the request is for a declared public path. + * + * Full-path match only, and only for the route that will actually answer: + * the first route in the plugin's router that matches the path and method + * has to be the one the declaration names. So a public "/webhook/:token" + * registered for POST does not open GET, and does not open a literal sibling + * route registered next to it. A trailing "/*" may also be served by + * router.use(), which has no route to compare against. + */ +function isPublicRequest( + req: Request, + publicPaths: PublicPath[], + inner: Router, +): boolean { if (publicPaths.length === 0) return false; const path = normalizePublicPath(req.path || "/"); - return publicPaths.some((pattern) => pattern.test(path)); + const candidates = publicPaths.filter(({ pattern }) => pattern.test(path)); + if (candidates.length === 0) return false; + + const stack = (inner as unknown as { stack?: RouterLayer[] }).stack ?? []; + for (const layer of stack) { + if (!layer.match?.(req.path || "/")) continue; + if (!layer.route) { + if (candidates.some(({ declared }) => declared.endsWith("/*"))) { + return true; + } + continue; + } + if (!layer.route._handlesMethod?.(req.method)) continue; + const routePath = layer.route.path; + return ( + typeof routePath === "string" && + candidates.some(({ declared }) => + declaredCoversRoute(declared, routePath), + ) + ); + } + return false; } export interface CreatePluginRouterArgs { @@ -196,7 +260,10 @@ export function createPluginRouter({ const declaredPublic = (options?.public ?? []).map((path) => normalizePublicPath(path), ); - const publicPaths = declaredPublic.map((path) => compilePublicPath(path)); + const publicPaths = declaredPublic.map((declared) => ({ + declared, + pattern: compilePublicPath(declared), + })); publicRoutes.set(pluginId, declaredPublic); if (declaredPublic.length > 0) { @@ -250,7 +317,7 @@ export function createPluginRouter({ const authenticate = AuthManager.getInstance().createAuthMiddleware(); outer.use((req: Request, res: Response, next: NextFunction) => { - if (isPublicRequest(req, publicPaths)) { + if (isPublicRequest(req, publicPaths, inner)) { const ip = req.ip || req.socket?.remoteAddress || "unknown"; if (overPublicRateLimit(pluginId, ip)) { res.status(429).json({ error: "Too many requests" }); diff --git a/src/backend/plugins/ws.ts b/src/backend/plugins/ws.ts index 5ca9bc768..5a17dc964 100644 --- a/src/backend/plugins/ws.ts +++ b/src/backend/plugins/ws.ts @@ -28,7 +28,10 @@ import type { } from "@termix-ssh/plugin-sdk/backend"; import { pluginLogger } from "../utils/logger.js"; import { recordConflict } from "./conflicts.js"; -import { extractWebSocketToken } from "../utils/ws-auth.js"; +import { + extractWebSocketToken, + isCookieOriginAllowed, +} from "../utils/ws-auth.js"; import { runAsActor } from "./actor.js"; import { isPluginInstalled } from "./http.js"; @@ -163,6 +166,12 @@ async function handlePluginUpgrade( return true; } + // A page on another site gets the browser's cookies attached for free. + // Core already ignores them for auth; plugin code must not see them either. + if (request.headers.cookie && !isCookieOriginAllowed(request)) { + delete request.headers.cookie; + } + let userId = ""; if (!route.options.public) { const resolved = await verifyToken(request); diff --git a/src/backend/tests/database/plugin-settings-cascade.test.ts b/src/backend/tests/database/plugin-settings-cascade.test.ts index f9f7ddbd7..c5dde0e73 100644 --- a/src/backend/tests/database/plugin-settings-cascade.test.ts +++ b/src/backend/tests/database/plugin-settings-cascade.test.ts @@ -9,7 +9,10 @@ import { asc } from "drizzle-orm"; import { afterEach, describe, expect, it } from "vitest"; -import { TestSqliteDatabase } from "./repositories/test-support.js"; +import { + TestSqliteDatabase, + testDialect, +} from "./repositories/test-support.js"; import { HostRepository } from "../../database/repositories/host-repository.js"; import { UserRepository } from "../../database/repositories/user-repository.js"; import type { DatabaseContext } from "../../database/repositories/database-context.js"; @@ -48,13 +51,16 @@ async function seed(): Promise { return context; } +// key is reserved in MySQL +const KEY_COLUMN = testDialect() === "mysql" ? "`key`" : "key"; + async function addSetting( scope: string, scopeId: string | null, key = "k", ): Promise { await adapter!.exec( - `INSERT INTO plugin_settings (plugin_id, scope, scope_id, key, value) + `INSERT INTO plugin_settings (plugin_id, scope, scope_id, ${KEY_COLUMN}, value) VALUES ('sample', '${scope}', ${scopeId === null ? "NULL" : `'${scopeId}'`}, '${key}', '"v"')`, ); } diff --git a/src/backend/tests/database/routes/plugins.test.ts b/src/backend/tests/database/routes/plugins.test.ts index e0acfda86..d2a73c11f 100644 --- a/src/backend/tests/database/routes/plugins.test.ts +++ b/src/backend/tests/database/routes/plugins.test.ts @@ -239,6 +239,7 @@ describe("plugins route", () => { }); expect(body[0]).not.toHaveProperty("capabilities"); expect(body[0]).not.toHaveProperty("lastError"); + expect(body[0].version).toBe(""); }); it("skips a plugin whose manifest does not parse", async () => { @@ -314,6 +315,9 @@ describe("plugins route", () => { expect(body[0].contributes).toEqual({ auth: { loginMethods: ["corp-sso"], secondFactors: [] }, }); + expect(body.map((plugin: { version: string }) => plugin.version)).toEqual( + ["", ""], + ); for (const plugin of body) { expect(Object.keys(plugin).sort()).toEqual( [ diff --git a/src/backend/tests/plugins/ctx-credentials.test.ts b/src/backend/tests/plugins/ctx-credentials.test.ts index 7615d9f41..3eb8bc505 100644 --- a/src/backend/tests/plugins/ctx-credentials.test.ts +++ b/src/backend/tests/plugins/ctx-credentials.test.ts @@ -156,6 +156,7 @@ const baseHost = { ip: "10.0.0.7", port: 22, username: "sshuser", + authType: "password", password: "ssh-secret", jumpHosts: JSON.stringify([{ hostId: 3 }]), }; @@ -285,6 +286,26 @@ describe("ctx.credentials.resolveHostProtocol", () => { }); }); + it("does not fall back to an old password on a key host", async () => { + grants.set("demo", ["credentials:read"]); + state.hosts.set(7, { ...baseHost, authType: "key" }); + state.logins.set("7:spice", { + protocol: "spice", + authType: "direct", + credentialId: null, + username: null, + password: null, + fields: {}, + secretFields: {}, + }); + const ctx = spiceContext(["credentials:read"]); + + const target = await runAsActor("owner", "request", () => + ctx.credentials.resolveHostProtocol(7, "spice"), + ); + expect(target?.auth.password).toBe(""); + }); + it("uses the stored credential in credential mode", async () => { grants.set("demo", ["credentials:read"]); state.logins.set("7:spice", { diff --git a/src/backend/tests/plugins/ctx-process.test.ts b/src/backend/tests/plugins/ctx-process.test.ts index 29acdc4b8..c50a14645 100644 --- a/src/backend/tests/plugins/ctx-process.test.ts +++ b/src/backend/tests/plugins/ctx-process.test.ts @@ -113,7 +113,8 @@ describe("ctx.process.ensureBinary", () => { const contents = "#!/bin/sh\necho hi\n"; it("uses a prebuilt copy whose checksum matches, without downloading", async () => { - const prebuilt = path.join(dir, "prebuilt-tool"); + await fs.mkdir(path.join(dir, "prebuilt"), { recursive: true }); + const prebuilt = path.join(dir, "prebuilt", "tool"); await fs.writeFile(prebuilt, contents); const fetch = vi.fn(); const { process: proc } = create({ fetch: fetch as never }); @@ -129,6 +130,23 @@ describe("ctx.process.ensureBinary", () => { expect(fetch).not.toHaveBeenCalled(); }); + it("never hashes a prebuilt path that is not the binary itself", async () => { + const other = path.join(dir, "secrets.db"); + await fs.writeFile(other, contents); + await fs.mkdir(path.join(dir, "tool-dir", "tool"), { recursive: true }); + const fetch = vi.fn(async () => new Response(contents)); + const { process: proc } = create({ fetch: fetch as never }); + const result = await proc.ensureBinary({ + name: "tool", + version: "1", + url: "https://example.test/tool", + sha256: sha256(contents), + prebuilt: [other, "relative/tool", path.join(dir, "tool-dir", "tool")], + }); + expect(result).toBe(path.join(dir, "bin", "tool")); + expect(fetch).toHaveBeenCalledOnce(); + }); + it("downloads once, verifies it and reuses the copy", async () => { const fetch = vi.fn(async () => new Response(contents)); const { process: proc } = create({ fetch: fetch as never }); diff --git a/src/backend/tests/plugins/disable-isolation.test.ts b/src/backend/tests/plugins/disable-isolation.test.ts index ae4262541..5fad15c06 100644 --- a/src/backend/tests/plugins/disable-isolation.test.ts +++ b/src/backend/tests/plugins/disable-isolation.test.ts @@ -33,7 +33,19 @@ vi.mock("../../plugins/permissions.js", () => ({ invalidatePluginPermissionCache: vi.fn(), })); -const TEST_PORT = 39102; +// A fixed port can land in a range Windows reserves for Hyper-V or Docker. +function freePort(): Promise { + return new Promise((resolve, reject) => { + const server = net.createServer(); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const { port } = server.address() as net.AddressInfo; + server.close(() => resolve(port)); + }); + }); +} + +const TEST_PORT = await freePort(); function isListening(port: number): Promise { return new Promise((resolve) => { diff --git a/src/backend/tests/plugins/http.test.ts b/src/backend/tests/plugins/http.test.ts index d38a7dfd2..a947192bb 100644 --- a/src/backend/tests/plugins/http.test.ts +++ b/src/backend/tests/plugins/http.test.ts @@ -234,6 +234,48 @@ describe("ctx.http authentication", () => { expect(response.status).toBe(401); }); + it("only opens the methods the public route was registered for", async () => { + const router = http.createPluginRouter({ + manifest: manifest(), + options: { public: ["/webhook/:token"] }, + reportError: () => {}, + }); + router.post("/webhook/:token", (_req, res) => res.json({ ok: true })); + router.delete("/webhook/:token", (_req, res) => res.json({ ok: true })); + + const base = await startServer(); + const url = `${base}/plugin-api/sample-plugin/webhook/abc`; + + expect((await fetch(url, { method: "POST" })).status).toBe(200); + expect((await fetch(url)).status).toBe(401); + expect((await fetch(url, { method: "PUT" })).status).toBe(401); + }); + + it("does not open a literal route beside a public parameter", async () => { + const router = http.createPluginRouter({ + manifest: manifest(), + options: { public: ["/webhook/:token"] }, + reportError: () => {}, + }); + router.get("/webhook/config", (_req, res) => res.json({ secret: true })); + router.post("/webhook/config", (_req, res) => res.json({ secret: true })); + router.get("/webhook/:token", (_req, res) => res.json({ ok: true })); + router.post("/webhook/:token", (_req, res) => res.json({ ok: true })); + + const base = await startServer(); + const root = `${base}/plugin-api/sample-plugin/webhook`; + + expect((await fetch(`${root}/config`)).status).toBe(401); + expect((await fetch(`${root}/config`, { method: "POST" })).status).toBe( + 401, + ); + expect((await fetch(`${root}/abc`)).status).toBe(200); + expect((await fetch(`${root}/abc`, { method: "HEAD" })).status).toBe(200); + expect((await fetch(`${root}/config`, { headers: authed })).status).toBe( + 200, + ); + }); + it("opens everything below a trailing /* and nothing beside it", async () => { const router = http.createPluginRouter({ manifest: manifest(), diff --git a/src/backend/tests/plugins/shutdown.test.ts b/src/backend/tests/plugins/shutdown.test.ts index 82c84d1e6..c0a4ae243 100644 --- a/src/backend/tests/plugins/shutdown.test.ts +++ b/src/backend/tests/plugins/shutdown.test.ts @@ -34,7 +34,19 @@ vi.mock("../../plugins/permissions.js", () => ({ invalidatePluginPermissionCache: vi.fn(), })); -const CONSOLE_PORT = 39109; +// A fixed port can land in a range Windows reserves for Hyper-V or Docker. +function freePort(): Promise { + return new Promise((resolve, reject) => { + const server = net.createServer(); + server.once("error", reject); + server.listen(0, "127.0.0.1", () => { + const { port } = server.address() as net.AddressInfo; + server.close(() => resolve(port)); + }); + }); +} + +const CONSOLE_PORT = await freePort(); const cleanups: Array<() => void> = []; function tempRoot(prefix: string): string { diff --git a/src/backend/tests/plugins/ws.test.ts b/src/backend/tests/plugins/ws.test.ts index ea3a2275b..84b7ca851 100644 --- a/src/backend/tests/plugins/ws.test.ts +++ b/src/backend/tests/plugins/ws.test.ts @@ -91,6 +91,20 @@ function connect( }); } +function connectWithHeaders( + url: string, + headers: Record, +): Promise<{ outcome: "open"; socket: WebSocket } | { outcome: number }> { + return new Promise((resolve) => { + const socket = new WebSocket(url, { headers }); + socket.once("open", () => resolve({ outcome: "open", socket })); + socket.once("unexpected-response", (_req, res) => + resolve({ outcome: res.statusCode ?? 0 }), + ); + socket.once("error", () => resolve({ outcome: 0 })); + }); +} + function authProtocols(token: string): string[] { return [`termix.jwt.${token}`]; } @@ -159,6 +173,61 @@ describe("ctx.ws routing", () => { }); describe("ctx.ws authentication", () => { + it("ignores the jwt cookie on an upgrade from another site", async () => { + ws.registerPluginWsRoute("sample-plugin", "/socket", () => {}, DECLARED); + + const base = await startServer(); + const result = await connectWithHeaders( + `${base}/plugin-ws/sample-plugin/socket`, + { Cookie: `jwt=${state.validToken}`, Origin: "https://evil.example" }, + ); + + expect(result.outcome).toBe(401); + }); + + it("accepts the jwt cookie from this server's own page", async () => { + ws.registerPluginWsRoute("sample-plugin", "/socket", () => {}, DECLARED); + + const base = await startServer(); + const result = await connectWithHeaders( + `${base}/plugin-ws/sample-plugin/socket`, + { + Cookie: `jwt=${state.validToken}`, + Origin: base.replace(/^ws:/, "http:"), + }, + ); + + expect(result.outcome).toBe("open"); + if (result.outcome === "open") result.socket.close(); + }); + + it("hides cross-site cookies from a public socket's handler", async () => { + const cookies: Array = []; + const users: string[] = []; + ws.registerPluginWsRoute( + "sample-plugin", + "/public", + ({ request, userId, socket }) => { + cookies.push(request.headers.cookie); + users.push(userId); + (socket as WebSocket).close(); + }, + DECLARED, + { public: true, optionalAuth: true }, + ); + + const base = await startServer(); + const result = await connectWithHeaders( + `${base}/plugin-ws/sample-plugin/public`, + { Cookie: `jwt=${state.validToken}`, Origin: "https://evil.example" }, + ); + + expect(result.outcome).toBe("open"); + await vi.waitFor(() => expect(users).toHaveLength(1)); + expect(users).toEqual([""]); + expect(cookies).toEqual([undefined]); + }); + it("rejects an upgrade with no token", async () => { ws.registerPluginWsRoute("sample-plugin", "/socket", () => {}, DECLARED); diff --git a/src/backend/tests/upgrade/protocol-auth-migration.test.ts b/src/backend/tests/upgrade/protocol-auth-migration.test.ts index b33e0737d..9811418ac 100644 --- a/src/backend/tests/upgrade/protocol-auth-migration.test.ts +++ b/src/backend/tests/upgrade/protocol-auth-migration.test.ts @@ -195,7 +195,7 @@ describe("runProtocolAuthMigration", () => { username: "waits", }); const [marker] = await database!.query<{ value: string }>( - sql`SELECT value FROM settings WHERE key = ${copiedMarker("locked")}`, + sql`SELECT value FROM settings WHERE ${sql.identifier("key")} = ${copiedMarker("locked")}`, ); expect(marker.value).toBe("done"); });