diff --git a/packages/plugin-sdk/ARCHITECTURE.md b/packages/plugin-sdk/ARCHITECTURE.md index 828370774..016db9380 100644 --- a/packages/plugin-sdk/ARCHITECTURE.md +++ b/packages/plugin-sdk/ARCHITECTURE.md @@ -252,7 +252,10 @@ handler)` for a socket. Core runs the same middleware in front of every plugin: 1. **Auth**, unless the path is listed in `options.public`. A public path is audited when the router is registered and logged on every request, because - "this plugin opened a hole in auth" should be findable later. It matches the + "this plugin opened a hole in auth" should be findable later. **D2** lists + a running plugin's public HTTP paths and public socket paths in the admin + plugin dialog (`publicRoutes` on `GET /plugins`), and audits a public + socket when it is registered. It matches the full path, with `:param` segments allowed, so declaring `/webhook/:token` public cannot open `/webhook/:token/anything`. A trailing `/*` is the one deliberate prefix (**C3**): `/chooser/:id/*` opens that path and everything @@ -260,12 +263,19 @@ handler)` for a socket. Core runs the same middleware in front of every plugin: 2. **The actor**, from the user core authenticated, never from the request body. 3. **An enabled check** returning 503 while the plugin is disabled. That is a different fact from 404 ("no such plugin") and a caller can act on it. + A disabled plugin has no router left, so the dispatcher answers 503 for any + installed id and 404 only for an unknown one; sockets do the same on the + upgrade (**D2**). 4. **Body limits**, defaulting to the 2mb core itself accepts. `database.ts` skips its own global parser for `/plugin-api` so a plugin's `bodyLimit` actually applies; `rawBody` turns the parsers off for a router that parses its own (multer, a raw body for signature checks). 5. **An error wrapper** that logs against the plugin, never leaks a stack, and - feeds the error budget. + feeds the error budget. A 4xx (a body that does not parse, one over the + limit) answers its own status and does not count, and nothing an + unauthenticated request causes counts either, so five junk requests to a + public route cannot switch a plugin off (**D2**). A public socket's guest + runs with no actor at all, the same as a public HTTP route. `ctx.rbac.require(permission)` adds a per-route RBAC gate, and a plugin may only require a permission it declares itself. @@ -1111,6 +1121,11 @@ is disposed automatically on deactivate. See [Lifecycle rules](#lifecycle-rules) ### 11. Tests +The security regressions for the runtime live in +`src/backend/tests/plugins/security/` (one file per area of the D2 review) and +`src/ui/tests/plugin-host/security/`. A fix to plugin code gets its test in +that plugin's own `tests/`, like everything else. + Each plugin has `plugins//tests/backend` (node) and `plugins//tests/frontend` (jsdom), run by its own vitest config: @@ -1725,6 +1740,22 @@ fields are rejected at every level: a typo'd `contribute` used to validate clean and be silently dropped, which made a manifest describe something it did not do. +**D2** added the discovery rules: + +- `RESERVED_PLUGIN_IDS` (`admin`, `hosts`, `credentials`, `core`, `termix`, + `plugin`, `plugins`, `users`, `system`, `sdk`) are refused. A plugin's + permissions are registered as `.`, so a plugin called `admin` + would otherwise mint `admin.*` permissions. +- `backend`, `frontend` and `locales` must be relative paths inside the + plugin: no absolute path, no `..`, no backslash. The loader also realpaths + the backend entry and every migration file and refuses one that resolves + outside the plugin, so a symlink cannot point elsewhere. +- A plugin whose table prefix overlaps a loaded plugin's (`foo` and + `foo-bar` both own `p_foo_bar_*`) is refused, bundled first. +- A user plugin can never take the id of a bundled plugin folder, even when + the bundled one failed to load. +- A `uiPresets` key must be a plain property name. + ```jsonc { "id": "example", // ^[a-z][a-z0-9-]{1,39}$, equals the directory name @@ -1979,48 +2010,49 @@ Built per plugin in `src/backend/plugins/ctx.ts` and passed to `activate`. Every member is listed; the types in `@termix/plugin-sdk/backend` are the authority for signatures. -| Member | Capability | What it does | -| -------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | -| `pluginId`, `manifest` | none | The plugin's id and parsed manifest | -| `log.debug/info/warn/error` | none | Lines in core's log, tagged with the plugin | -| `events.emit/on` | `events:core` for core topics | Topics under `plugin..*`, or core's (`user.deleted`, `user.data_wiped`, host events) | -| `kv.get/set/delete/list` | `kv:own` | Small key/value state | -| `files.dataDir()` | `files:own` | A per-plugin folder under `DATA_DIR` | -| `db.define/client/refs/persist/dialect` | `db:own` (not `dialect`) | The plugin's own tables through Drizzle; `persist` flushes SQLite to disk | -| `sync.registerEntity/recordTombstone` | none | Adds a table to remote sync, and records a delete for it | -| `registry.provide/consume/revoke` | none | The low-level named registry services sit on | -| `services.provide/get/providers` | per-service RBAC | Typed, versioned cross-plugin services, checked per call | -| `secrets.get/set/delete/seal/unseal` | `secrets:own` | The plugin's own encrypted values | -| `secrets.offer/withdraw/getShared` | per-secret RBAC | Secrets one plugin lends another | -| `http.router(options)`, `http.baseUrl(req)` | `network:serve` (router) | The router mounted at `/plugin-api//`, and the public origin for callback URLs | -| `ws.route/upgrade` | `network:serve` | Sockets at `/plugin-ws//` | -| `rbac.has/hasFor/require` | own permissions (require) | The acting user's role permissions; `require` is route middleware | -| `settings.get/set/getUser/setUser/getHost/setHost/listHostValues/getAll/onChange/onValidate` | `hosts:read` (listHostValues) | The plugin's admin, user and host settings from its manifest | -| `settings.readCore(key)` | `settings:read-core` | One of the core keys in `CORE_SETTINGS_ALLOWLIST` | -| `disposables.add` | none | Extra cleanup on deactivate | -| `capabilities.has/require` | the named capability | A generic check for privileged code no ctx member wraps | -| `hosts.list/get/checkAccess` | `hosts:read` | Hosts the acting user can see, without secrets | -| `hosts.create/update/delete/listOwned/share/listUsers/listRoles` | `hosts:write` | Host changes and sharing on the user's behalf | -| `hosts.trackSession/recordActivity` | `hosts:read` | The online indicator and a recent activity entry | -| `hosts.status.get/check/reportLogin/registerPort` | `hosts:read` | Core's reachability status | -| `ssh.connect/withConnection/jumpChain` | `ssh:connect`, `credentials:use` | Connections through core's one connect pipeline | -| `ssh.resolveHost/prepare/openTransport/classifyKeyboardInteractive/autoResponses/startInteraction/cancelInteraction` | `ssh:connect`, `credentials:use` | The lower level, for a transport with its own prompt flow | -| `ssh.poolKey/dropPooled/requiresSecret/supportsBackground` | `ssh:connect` | Pool housekeeping and questions for an auth type's provider | -| `auth.registerLoginMethod/registerSecondFactor/recordEnrollment/removeEnrollment` | `auth:provide` | Login methods and second factors | -| `auth.completeRedirectLogin/revokeSessions/loginRateLimit/countLinkedUsers` | `auth:provide` | What a login method needs from core's session and provisioning code | -| `auth.registerSshAuthProvider/registerKeyboardInteractiveHandler` | `auth:provide` | SSH auth types and keyboard-interactive handlers | -| `credentials.listSshKeys` / `.createSshKey` | `credentials:use` / `credentials:write` | The user's saved SSH keys | -| `credentials.resolveHostProtocol` | `credentials:read` | A host's RDP, VNC or Telnet login | -| `credentials.registerSecretResolver` | `auth:provide` | Resolves external secret references in host and credential fields | -| `desktop.openIsolatedWindow/launchNativeRdp/available` | `desktop:window` (not `available`) | Electron windows and the native RDP client | -| `audit.record` | none | An audit line under the plugin's own action name | -| `schedule.every/after` | none | Timers that stop on deactivate and never overlap | -| `notify.channels/send` | `notify:send` | Core's notification channels, as the acting user | -| `fetch(url, init)` | `network:outbound` | Outbound HTTP through core's SSRF guard; `allowPrivateHosts` hosts also go through the configured proxy | -| `process.run/ensureBinary` | `process:spawn` (+ `network:outbound` to download) | Programs on the server | -| `system.tlsStatus/writeTlsCertificate/reloadTls/publishHttpChallenge/registerTlsRenewer` | `system:tls` | The server certificate | -| `asUser(userId, fn)` | none, always audited | Background work as a user | -| `currentActor()` | none | The acting user for the current call | +| Member | Capability | What it does | +| ------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | +| `pluginId`, `manifest` | none | The plugin's id and parsed manifest | +| `log.debug/info/warn/error` | none | Lines in core's log, tagged with the plugin | +| `events.emit/on` | `events:core` for core topics | Topics under `plugin..*`, or core's (`user.deleted`, `user.data_wiped`, host events) | +| `kv.get/set/delete/list` | `kv:own` | Small key/value state | +| `files.dataDir()` | `files:own` | A per-plugin folder under `DATA_DIR` | +| `db.define/client/refs/persist/dialect` | `db:own` (not `dialect`) | The plugin's own tables through Drizzle; `persist` flushes SQLite to disk | +| `sync.registerEntity/recordTombstone` | none | Adds a table to remote sync, and records a delete for it | +| `registry.provide/consume/revoke` | none | The low-level named registry services sit on | +| `services.provide/get/providers` | per-service RBAC | Typed, versioned cross-plugin services, checked per call | +| `secrets.get/set/delete/seal/unseal` | `secrets:own` | The plugin's own encrypted values | +| `secrets.offer/withdraw/getShared` | per-secret RBAC | Secrets one plugin lends another | +| `http.router(options)`, `http.baseUrl(req)` | `network:serve` (router) | The router mounted at `/plugin-api//`, and the public origin for callback URLs | +| `ws.route/upgrade` | `network:serve` | Sockets at `/plugin-ws//` | +| `rbac.has/hasFor/require` | own permissions (require) | The acting user's role permissions; `require` is route middleware | +| `settings.get/set/getUser/setUser/getHost/setHost/listHostValues/getAll/onChange/onValidate` | `hosts:read` (listHostValues) | The plugin's admin, user and host settings from its manifest | +| `settings.readCore(key)` | `settings:read-core` | One of the core keys in `CORE_SETTINGS_ALLOWLIST` | +| `disposables.add` | none | Extra cleanup on deactivate | +| `capabilities.has/require` | the named capability | A generic check for privileged code no ctx member wraps | +| `hosts.list/get/checkAccess` | `hosts:read` | Hosts the acting user can see, without secrets | +| `hosts.create/update/delete/listOwned/share/listUsers/listRoles` | `hosts:write` | Host changes and sharing on the user's behalf | +| `hosts.trackSession/recordActivity` | `hosts:read` | The online indicator and a recent activity entry | +| `hosts.status.get/check/reportLogin/registerPort` | `hosts:read` | Core's reachability status (`get` is async since **D2**) | +| `ssh.connect/withConnection/jumpChain` | `ssh:connect`, `credentials:use` | Connections through core's one connect pipeline | +| `ssh.resolveHost/openTransport/classifyKeyboardInteractive/autoResponses/startInteraction/cancelInteraction` | `ssh:connect`, `credentials:use` | The lower level, for a transport with its own prompt flow; `resolveHost` redacts secrets without `credentials:read` | +| `ssh.prepare` | `ssh:connect`, `credentials:use`, `credentials:read` | The ssh2 config for a transport that connects its own client; it carries the password and key | +| `ssh.poolKey/dropPooled/requiresSecret/supportsBackground` | `ssh:connect` | Pool housekeeping and questions for an auth type's provider | +| `auth.registerLoginMethod/registerSecondFactor/recordEnrollment/removeEnrollment` | `auth:provide` | Login methods and second factors | +| `auth.completeRedirectLogin/revokeSessions/loginRateLimit/countLinkedUsers` | `auth:provide` | What a login method needs from core's session and provisioning code | +| `auth.registerSshAuthProvider/registerKeyboardInteractiveHandler` | `auth:provide` | SSH auth types and keyboard-interactive handlers | +| `credentials.listSshKeys` / `.createSshKey` | `credentials:use` / `credentials:write` | The user's saved SSH keys | +| `credentials.resolveHostProtocol` | `credentials:read` | A host's RDP, VNC or Telnet login | +| `credentials.registerSecretResolver` | `auth:provide` | Resolves external secret references in host and credential fields | +| `desktop.openIsolatedWindow/launchNativeRdp/available` | `desktop:window` (not `available`) | Electron windows and the native RDP client | +| `audit.record` | none | An audit line under the plugin's own action name | +| `schedule.every/after` | none | Timers that stop on deactivate and never overlap | +| `notify.channels/send` | `notify:send` | Core's notification channels, as the acting user | +| `fetch(url, init)` | `network:outbound` | Outbound HTTP through core's SSRF guard; `allowPrivateHosts` hosts also go through the configured proxy | +| `process.run/ensureBinary` | `process:spawn` (+ `network:outbound` to download) | Programs on the server | +| `system.tlsStatus/writeTlsCertificate/reloadTls/publishHttpChallenge/registerTlsRenewer` | `system:tls` | The server certificate | +| `asUser(userId, fn)` | none, always audited | Background work as a user | +| `currentActor()` | none | The acting user for the current call | **B11** added `ctx.capabilities.has(capability)` / `.require(capability)`, a generic check for a capability no other ctx member wraps. Unlike every other @@ -2341,7 +2373,7 @@ Windows guest as RDP) without core naming either. ### The actor Never comes from plugin code. It is held in `AsyncLocalStorage` and set two -ways, with no third: +ways, with no third (a public socket's guest has none): - **A request.** A4's HTTP and WebSocket middleware runs the handler as the user core already authenticated. @@ -2352,6 +2384,68 @@ A plugin cannot pass a user id to a guarded method and have it believed. This is what closes the forged-caller bug the old worker broker had, where a worker could set `callerUserId` on its own messages. +Two calls do take a user id, for background work that reaches another +plugin: `ctx.services.get(name, { userId })` (and a handle's +`asUser(userId)`) and `ctx.secrets.getShared(..., { userId })`. Naming anyone +but the current actor there is the same act as `ctx.asUser`, so since **D2** +it writes the same `plugin_as_user` audit line. + +`ctx.ssh` never believes a host object's own `userId` either. Before **D2** +a call with no actor ran as whatever `userId` the plugin put on the host it +passed, which reached that user's jump hosts with that user's credentials. +Now every `ctx.ssh` call needs an actor, except for a host core itself +resolved earlier (from `resolveHost`, `connect` or `withConnection`), which +keeps the user it was resolved for. Any other object host runs as the actor. + +**D2** also put the per-user access check on `hosts.status.*`, +`hosts.trackSession`, `hosts.recordActivity` and `ssh.startInteraction`: with +an actor, a host the actor cannot connect to answers nothing. With no actor +they are the plugin's own background work (a poller, a guacd session opening) +and only the capability applies. + +### Credentials + +Only a plugin holding `credentials:read` sees plaintext credential material +(`HOST_SECRET_FIELDS` in `ctx-hosts.ts`: passwords, keys, passphrases, sudo +and SOCKS passwords, the autostart ones and `terminalConfig.sudoPassword`). +Since **D2**: + +- `ctx.hosts` always returns hosts with those removed, `listOwned` included, + and `listOwned` is audited. `update` never moves `id`, `userId` or + `syncId`. +- `ctx.ssh.connect().host` is always redacted. +- `ctx.ssh.resolveHost` returns the full host, audited as + `plugin_credentials_read`, only to a plugin with `credentials:read`. Any + other plugin gets the redacted host, and core remembers the real one, so + handing that object back to `connect`, `withConnection` or `prepare` + connects with the real secrets. +- `ctx.ssh.prepare` needs `credentials:read`, because the ssh2 config it + returns carries them. + +ssh-terminal, file-manager, host-metrics and tailscale declare +`credentials:read`: they run `prepare` or read a sudo password. +`credentials.resolveHostProtocol` was already behind it. + +### Refusals + +`assertCapability` writes the refusal's audit line itself, named after the +call (`plugin_kv_set`), so no guard can forget to. A synchronous declaration +check writes it in the background through `capabilityRefused`. Successes are +audited for every privileged call except the hot-path reads a plugin makes on +every request or sample: `kv.get`, `secrets.get/seal/unseal`, `db.persist`, +`hosts.list/get/checkAccess`, `hosts.status.*` and `hosts.trackSession`. +`src/backend/tests/plugins/security/ctx-guards.test.ts` walks the whole ctx +object and fails for any member that runs without a capability and is not on +its `UNGATED` list with a reason. + +Hooks ssh2 calls synchronously (a provider's `onBanner` and `onAuthFailed`, +keyboard-interactive `detect` and `autoAnswerPasswords`) answer from the +grant cache and do nothing until the grant is confirmed. + +`ctx.process.run` gives the program a small base environment (path, home, +temp, locale, proxy) plus the plugin's own `env`, never the server's, which +holds the JWT secret and database keys. + --- ## Lifecycle rules @@ -2421,7 +2515,17 @@ architecture did. `ctx.db` is a clear example. It hands back a handle scoped to the plugin's own tables, and the `p__` prefix, the CLI check and the runner's check all say -a plugin may only touch its own. None of that is enforced by the engine: a +a plugin may only touch its own. Since **D2** both checks are one function, +`findUnownedTableWrites` in `@termix/plugin-sdk/ddl`, which reads each +statement the way the engine does: comments and every quoting style are +seen through, every name in a `DROP TABLE a, b` list is checked, a rename +target counts as a write, reads (`FROM`, `JOIN`, `REFERENCES`) are allowed, +and `ATTACH`, `PRAGMA`, `GRANT`, triggers, functions and other statement kinds +are refused outright. The legacy-table exemption applies only to the bundled +plugin that adopts the table. `removePluginData` finds a plugin's tables in +the database's own catalog by prefix, leaves a longer prefix another plugin +owns, and takes the plugin's settings and secrets with it; role permissions +stay. None of that is enforced by the engine: a plugin holding that handle can write any table in the database, and `ctx.db.refs` hands it `users`, `ssh_data`, `roles` and `user_roles` outright (the last two added in **B2**, for a plugin that shares its own rows with a role and needs @@ -2442,6 +2546,90 @@ until one is, this document does not pretend otherwise. --- +## Threat model + +Written in **D2**, after a security review of the whole plugin system. The +short version: a plugin is code you run on your server, with the same reach +as Termix itself. Everything below follows from that. + +### What a malicious plugin can do + +A plugin runs in the Termix server process and in the browser tab of anyone +who uses Termix. In the server it can do everything the server's OS user can: + +- read `DATA_DIR`, which holds the database, the system key, the JWT secret + and every user's encrypted data, and import core modules to decrypt it; +- read and write any table, whatever `ctx.db` says it owns; +- open sockets, listen on ports, spawn programs and read the environment; +- replace or patch core modules in memory, including the auth middleware and + the capability check itself; +- log in as any user, because `ctx.auth` identities and `ctx.asUser` are + trusted by design. + +In the browser it runs in the shell's own JavaScript context, so it can read +anything the page can, including the session cookie's effects and every +other plugin's UI. + +None of the checks in this document stop that. They are not a sandbox. + +### What the system does about it + +- **Signing and review.** Official plugins live in this repo and ship in the + Termix image; they are reviewed like core. Signed community plugins and + their review process arrive with the install UI in 3.0.0. +- **The kill list (3.0.0).** A signed list of plugin versions known to be bad, + which the server refuses to load, so a bad plugin can be switched off on + every install without waiting for each admin. +- **Capability consent.** A manifest names every privileged thing its code + does through `ctx`, with a risk level. In 3.0.0 an admin grants each one + when installing; in 2.9.0 bundled plugins get what they declare. +- **Audit.** Every privileged `ctx` call, allowed or refused, writes a line + with the plugin id and the acting user, and so does every switch of the + actor (`ctx.asUser`, or a service handle or shared secret named for another + user). "What did this plugin do, as whom" has an answer. +- **Honest plugins stay honest.** The checks D2 added or confirmed stop the + accidental and the confused-deputy cases, which are the ones that actually + happen: + - every privileged member is guarded, and a test walks `ctx` so a new + member without a guard fails CI; + - plaintext credentials only reach a plugin holding `credentials:read`; + - a host object's `userId` is never believed, so a plugin cannot borrow + another user's jump hosts or credentials by naming them; + - `ctx.hosts` and `ctx.ssh` apply the actor's RBAC to every host; + - routes need login unless declared public, public routes are listed in + the admin plugin dialog, a disabled plugin answers 503, errors hide their + stack, and an anonymous caller cannot spend a plugin's error budget; + - `/plugin-assets` serves only a running plugin's browser files, with no + way out of the plugin folder, symlinks included; + - a user plugin cannot take a bundled plugin's id, a reserved id (`admin`, + `hosts`, `credentials` and a few more) or an id whose table prefix + overlaps another plugin's; + - migrations are parsed, not pattern-matched, and may only write the + plugin's own tables; + - a plugin's programs get a small environment without the server's + secrets. + +### Before installing a community plugin + +- **Know where it came from.** Install from a source you would trust with + root on the server, and check the signature once 3.0.0 has them. +- **Read the capabilities.** Anything critical or high (`credentials:read`, + `system:tls`, `ssh:connect`, `process:spawn`, `users:write`, + `auth:provide`, `device:serial`) means the plugin can reach your hosts, + your users or the server itself. Ask why it needs each one. +- **Read the public routes.** Admin > Plugins > Permissions lists every path + it serves without login. Each one is attack surface on your server. +- **Check its dependencies.** A plugin's npm dependencies run with the same + reach as the plugin. +- **Watch the audit log after installing.** Filter on `plugin:` and look + for calls you did not expect, especially `plugin_as_user` and + `plugin_credentials_read`. +- **Run Termix as an unprivileged user**, in its container or under its own + OS account, so "everything the server user can do" is as little as + possible. + +--- + ## Building a plugin `termix-plugin`, the CLI the SDK ships, is the only build a plugin needs. It diff --git a/packages/plugin-sdk/FINISH-LIST.md b/packages/plugin-sdk/FINISH-LIST.md index 6dc1f908f..99e36f0db 100644 --- a/packages/plugin-sdk/FINISH-LIST.md +++ b/packages/plugin-sdk/FINISH-LIST.md @@ -167,6 +167,18 @@ with a real install, a real device or a repo outside this one. import that carries file manager bookmarks; the Simple preset hiding plugin rail items except Snippets. +- Termix-Mobile: after a redirect login (SSO) with the admin setting + "second factor after external login" on, the server answers with the + second-factor step (pending cookie and `temp_token`) instead of a session. + The mobile app has to handle that step; that change is in the mobile repo. +- D2 changes to check by hand: a remote desktop session still opens, and a + touch mode switch within five minutes reuses its token (after that the app + mints a new one); the terminal, file manager and host metrics still connect + and sudo still works (they now declare `credentials:read`); Docker and + automations connect with a redacted host handed back to core; Admin > + Plugins > Permissions lists a plugin's public routes; a disabled plugin's + `/plugin-api` URL answers 503; a share-link guest who leaves cannot type. + ## Left for later - 3.0.0: drop the migrated 2.8 settings rows (`guac_url`, `step_ca_url`, @@ -183,3 +195,13 @@ with a real install, a real device or a repo outside this one. rule: `src/backend/hosts/host-session-status.test.ts`, `jump-host-proxy.test.ts` and `ssh-keepalive.test.ts`. Move them under `src/backend/tests/hosts/`. +- D2 low findings, not fixed: plugin sockets have no Origin check (a + SameSite=lax cookie is not sent on a cross-site WebSocket, so only a + sibling subdomain could try); a public `:param` path matches every HTTP + method and would also match a sibling literal route; `/plugins/public-manifest` + shows plugin versions before login; `ctx.process.ensureBinary` hashes any + `prebuilt` path on disk; `credentials.resolveHostProtocol` falls back to the + SSH password for an RDP login (2.8 behaviour, behind `credentials:read`); + a remote desktop display token can be replayed within its five minute TTL + (strict single use would break the touch mode switch and reconnect, which + reuse it). Revisit with signing and the install UI in 3.0.0. diff --git a/packages/plugin-sdk/cli/commands/validate.mjs b/packages/plugin-sdk/cli/commands/validate.mjs index 04e6eb687..a1b86454e 100644 --- a/packages/plugin-sdk/cli/commands/validate.mjs +++ b/packages/plugin-sdk/cli/commands/validate.mjs @@ -3,6 +3,7 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { readManifest } from "../lib/plugin-dir.mjs"; import { LEGACY_TABLE_OWNERS as LEGACY_TABLES } from "../../dist/db.js"; +import { findUnownedTableWrites } from "../../dist/ddl.js"; /** * The manifest rules live in src/manifest.ts, which the server uses too, so @@ -76,9 +77,6 @@ function validateNativeDependencies(cwd, raw) { const DIALECTS = ["sqlite", "postgres", "mysql"]; -const CREATE_OR_ALTER = - /(?:CREATE\s+(?:TEMP\s+|TEMPORARY\s+)?TABLE(?:\s+IF\s+NOT\s+EXISTS)?|ALTER\s+TABLE|DROP\s+TABLE(?:\s+IF\s+EXISTS)?|TRUNCATE\s+TABLE)\s+([`"']?)([a-zA-Z0-9_]+)\1/gi; - /** * Checks that a plugin's migrations only touch tables it owns. * @@ -88,7 +86,11 @@ const CREATE_OR_ALTER = */ function validateMigrations(cwd, pluginId) { const problems = []; - const prefix = `p_${String(pluginId).replace(/-/g, "_")}_`; + const legacy = new Set( + Object.entries(LEGACY_TABLES) + .filter(([, owner]) => owner === pluginId) + .map(([table]) => table), + ); const root = path.join(cwd, "migrations"); if (!fs.existsSync(root)) return problems; @@ -113,15 +115,8 @@ function validateMigrations(cwd, pluginId) { } const sql = fs.readFileSync(path.join(dir, file), "utf8"); - CREATE_OR_ALTER.lastIndex = 0; - let match; - while ((match = CREATE_OR_ALTER.exec(sql)) !== null) { - const table = match[2]; - if (table.startsWith(prefix)) continue; - if (LEGACY_TABLES[table] === pluginId) continue; - problems.push( - `migrations/${dialect}/${file} touches "${table}", which is not prefixed "${prefix}"`, - ); + for (const problem of findUnownedTableWrites(pluginId, sql, legacy)) { + problems.push(`migrations/${dialect}/${file} ${problem}`); } } } diff --git a/packages/plugin-sdk/src/backend.ts b/packages/plugin-sdk/src/backend.ts index d7fdf41e3..6580117e5 100644 --- a/packages/plugin-sdk/src/backend.ts +++ b/packages/plugin-sdk/src/backend.ts @@ -699,8 +699,11 @@ export interface PluginHostStatusEntry { * turns "reachable" into "online". */ export interface PluginHostStatusApi { - /** The last known status, or null when core has not checked the host yet. */ - get: (hostId: number) => PluginHostStatusEntry | null; + /** + * The last known status, or null when core has not checked the host yet or + * the acting user cannot reach it. + */ + get: (hostId: number) => Promise; /** Checks the host now unless core checked it recently. */ check: (hostId: number) => Promise; /** Reports a login attempt. Not audited: pollers call it every sample. */ @@ -1588,7 +1591,7 @@ export interface PluginCapabilities { } export interface PluginProcessOptions { - /** Added to the server's environment. */ + /** Added to a small base environment (PATH, HOME, temp, locale, proxy), never the whole server one. */ env?: Record; cwd?: string; /** Kills the program with SIGKILL after this long. */ diff --git a/packages/plugin-sdk/src/ddl.ts b/packages/plugin-sdk/src/ddl.ts index 8a93f55a7..18247a7f2 100644 --- a/packages/plugin-sdk/src/ddl.ts +++ b/packages/plugin-sdk/src/ddl.ts @@ -10,7 +10,7 @@ * core's schema, kept deliberately in step with it. */ -import { KEY_LENGTH, prefixedTableName } from "./db.js"; +import { KEY_LENGTH, prefixedTableName, tablePrefix } from "./db.js"; import type { PluginColumn, PluginTableDefinition, @@ -346,3 +346,253 @@ export function addColumnSql( return `ALTER TABLE ${quote(dialect, physical)} ADD COLUMN ${line};`; } + +interface SqlToken { + /** As written: a bare word keeps its case, a quoted name loses its quotes. */ + value: string; + kind: "word" | "quoted" | "punct"; +} + +/** + * Words and identifiers in one statement, with comments and string literals + * dropped. `"x"`, `` `x` `` and `[x]` all come out as the identifier x, so no + * quoting style hides a table name from the ownership check. + */ +function tokenize(source: string): SqlToken[] { + const tokens: SqlToken[] = []; + let i = 0; + while (i < source.length) { + const char = source[i]; + const next = source[i + 1]; + if (/\s/.test(char)) { + i++; + } else if (char === "-" && next === "-") { + while (i < source.length && source[i] !== "\n") i++; + } else if (char === "/" && next === "*") { + const end = source.indexOf("*/", i + 2); + i = end < 0 ? source.length : end + 2; + } else if (char === "'") { + i++; + while (i < source.length) { + if (source[i] === "'" && source[i + 1] === "'") i += 2; + else if (source[i] === "'") break; + else i++; + } + i++; + tokens.push({ value: "'", kind: "punct" }); + } else if (char === '"' || char === "`" || char === "[") { + const close = char === "[" ? "]" : char; + let value = ""; + i++; + while (i < source.length) { + if (source[i] === close && source[i + 1] === close && close !== "]") { + value += close; + i += 2; + } else if (source[i] === close) { + break; + } else { + value += source[i++]; + } + } + i++; + tokens.push({ value, kind: "quoted" }); + } else if (/[A-Za-z0-9_$]/.test(char)) { + let value = ""; + while (i < source.length && /[A-Za-z0-9_$]/.test(source[i])) { + value += source[i++]; + } + tokens.push({ value, kind: "word" }); + } else { + tokens.push({ value: char, kind: "punct" }); + i++; + } + } + return tokens; +} + +/** Statements a plugin migration may never contain. */ +const REFUSED_STATEMENTS = new Set([ + "ATTACH", + "DETACH", + "PRAGMA", + "GRANT", + "REVOKE", + "COPY", + "LOAD", + "DO", + "CALL", + "HANDLER", + "WITH", + "SET", + "VACUUM", +]); + +/** What a migration may CREATE, ALTER or DROP. Anything else is refused. */ +const TABLE_OBJECTS = new Set(["TABLE", "VIEW"]); +const HARMLESS_OBJECTS = new Set(["SEQUENCE", "TYPE"]); + +const CREATE_MODIFIERS = [ + "OR", + "REPLACE", + "TEMP", + "TEMPORARY", + "UNLOGGED", + "GLOBAL", + "LOCAL", + "VIRTUAL", + "UNIQUE", + "CONCURRENTLY", +]; + +/** + * Every problem with the tables a migration writes to. + * + * The check has to hold against a plugin that is trying, not only one that + * made a typo, so it reads each statement the way the engine does rather + * than matching one pattern: a comment between a keyword and a name, bracket + * quoting, a second name in a DROP list, a schema-qualified name and + * `ALTER ... RENAME TO users` are all seen. Reads (FROM, JOIN, REFERENCES) + * are allowed; writes outside the plugin's prefix are not. + * + * `ownedLegacy` lists the legacy core tables this plugin adopts. + */ +export function findUnownedTableWrites( + pluginId: string, + sql: string, + ownedLegacy: ReadonlySet = new Set(), +): string[] { + const prefix = tablePrefix(pluginId); + const problems: string[] = []; + + for (const statement of splitStatements(sql)) { + const tokens = tokenize(statement); + if (tokens.length === 0) continue; + const upper = (at: number) => + tokens[at]?.kind === "word" ? tokens[at].value.toUpperCase() : ""; + + /** Checks the table name at `at`, returns the index after it. */ + const table = (at: number): number => { + const name = tokens[at]; + if (!name || name.kind === "punct") { + problems.push("a statement writes to a table with no name"); + return at + 1; + } + if (tokens[at + 1]?.value === ".") { + problems.push( + `"${name.value}.${tokens[at + 2]?.value ?? ""}" names a schema; a plugin may only write its own tables`, + ); + return at + 3; + } + if (!name.value.startsWith(prefix) && !ownedLegacy.has(name.value)) { + problems.push( + `writes to "${name.value}", which is not prefixed "${prefix}"`, + ); + } + return at + 1; + }; + const skip = (at: number, words: string[]) => { + while (words.includes(upper(at))) at++; + return at; + }; + const find = (word: string, from = 0) => { + for (let at = from; at < tokens.length; at++) { + if (upper(at) === word) return at; + } + return -1; + }; + + const first = upper(0); + if (REFUSED_STATEMENTS.has(first)) { + problems.push(`${first} is not allowed in a plugin migration`); + continue; + } + + if (first === "CREATE" || first === "ALTER" || first === "DROP") { + let at = skip(1, CREATE_MODIFIERS); + const object = upper(at); + if (object === "INDEX") { + // The index name is free; the table after ON is what it writes to. + const on = find("ON", at); + if (on > 0) table(skip(on + 1, ["ONLY"])); + continue; + } + if (HARMLESS_OBJECTS.has(object)) continue; + if (!TABLE_OBJECTS.has(object)) { + problems.push( + `${first} ${object || tokens[at]?.value || ""} is not allowed in a plugin migration`.trim(), + ); + continue; + } + at = table(skip(at + 1, ["IF", "NOT", "EXISTS", "ONLY"])); + // DROP TABLE a, b drops both. + while (first === "DROP" && tokens[at]?.value === ",") { + at = table(at + 1); + } + // ALTER TABLE p_x RENAME TO users moves a table out of the namespace. + // RENAME [COLUMN] a TO b renames a column and is fine. + for (let rename = find("RENAME", at); rename > 0;) { + const next = upper(rename + 1); + if (next === "TO" || next === "AS") table(rename + 2); + rename = find("RENAME", rename + 1); + } + continue; + } + + if (first === "RENAME") { + // RENAME TABLE a TO b, c TO d + let at = skip(1, ["TABLE"]); + while (at < tokens.length) { + at = table(at); + if (upper(at) !== "TO") break; + at = table(at + 1); + if (tokens[at]?.value !== ",") break; + at++; + } + continue; + } + + if (first === "TRUNCATE") { + let at = table(skip(1, ["TABLE", "ONLY"])); + while (tokens[at]?.value === ",") at = table(at + 1); + continue; + } + + if (first === "INSERT" || first === "REPLACE") { + const into = find("INTO"); + if (into > 0) table(into + 1); + else problems.push(`${first} without INTO is not allowed`); + continue; + } + + if (first === "UPDATE") { + table( + skip(1, [ + "OR", + "ROLLBACK", + "ABORT", + "REPLACE", + "FAIL", + "IGNORE", + "ONLY", + "LOW_PRIORITY", + ]), + ); + continue; + } + + if (first === "DELETE") { + const from = find("FROM"); + if (from > 0) table(skip(from + 1, ["ONLY"])); + else problems.push("DELETE without FROM is not allowed"); + continue; + } + + if (first !== "SELECT" && first !== "COMMENT") { + problems.push( + `"${tokens[0].value}" statements are not allowed in a plugin migration`, + ); + } + } + + return problems; +} diff --git a/packages/plugin-sdk/src/manifest.ts b/packages/plugin-sdk/src/manifest.ts index 2503de298..2349923d7 100644 --- a/packages/plugin-sdk/src/manifest.ts +++ b/packages/plugin-sdk/src/manifest.ts @@ -37,6 +37,27 @@ export type ActionContributionKind = (typeof ACTION_CONTRIBUTION_KINDS)[number]; const OPEN_FROM_VALUES = ["rail", "host-context-menu", "palette"] as const; const ID_PATTERN = /^[a-z][a-z0-9-]{1,39}$/; + +/** + * Ids no plugin may take. A permission is registered as ., so a + * plugin called "admin" would mint admin.* permissions and could default them + * onto the user role; the rest are names core or the runtime already uses. + */ +export const RESERVED_PLUGIN_IDS: readonly string[] = [ + "admin", + "hosts", + "credentials", + "core", + "termix", + "plugin", + "plugins", + "users", + "system", + "sdk", +]; + +/** A preset key is a plain property name, never a prototype one. */ +const PRESET_KEY_PATTERN = /^[a-zA-Z][a-zA-Z0-9_]{0,63}$/; const SEMVER_PATTERN = /^\d+\.\d+\.\d+(-[0-9A-Za-z-.]+)?(\+[0-9A-Za-z-.]+)?$/; const API_VERSION_PATTERN = /^[0-9]+$/; /** Service names are dotted, e.g. "ssh.transport". */ @@ -476,6 +497,30 @@ function rejectUnknown( } } +/** + * A path inside the plugin folder. Absolute paths, ".." and backslashes are + * refused so an entry point or asset folder cannot point outside the plugin. + */ +function requireRelativePath( + value: string, + where: string, + errors: string[], +): void { + const segments = value.split("/"); + if ( + value.startsWith("/") || + /^[A-Za-z]:/.test(value) || + value.includes("\\") || + value.includes("\0") || + segments.includes("..") || + segments.every((segment) => segment === "" || segment === ".") + ) { + errors.push( + `Field "${where}" must be a relative path inside the plugin, got: ${JSON.stringify(value)}`, + ); + } +} + function requireString( value: unknown, where: string, @@ -507,6 +552,8 @@ export function validateManifest(manifest: unknown): string[] { errors.push( `Field "id" must match ${ID_PATTERN} (lowercase, starts with a letter, 2-40 chars), got: ${JSON.stringify(m.id)}`, ); + } else if (RESERVED_PLUGIN_IDS.includes(m.id)) { + errors.push(`Field "id" may not be "${m.id}": that name is reserved`); } } @@ -524,9 +571,11 @@ export function validateManifest(manifest: unknown): string[] { if ("repository" in m) requireString(m.repository, "repository", errors); if ("icon" in m) requireString(m.icon, "icon", errors); - if ("backend" in m) requireString(m.backend, "backend", errors); - if ("frontend" in m) requireString(m.frontend, "frontend", errors); - if ("locales" in m) requireString(m.locales, "locales", errors); + for (const field of ["backend", "frontend", "locales"] as const) { + if (field in m && requireString(m[field], field, errors)) { + requireRelativePath(m[field] as string, field, errors); + } + } validateAuthor(m.author, errors); validateEngine(m.engine, errors); @@ -870,6 +919,11 @@ function validateUiPresets(value: unknown, errors: string[]): void { continue; } for (const [key, entry] of Object.entries(preset)) { + if (!PRESET_KEY_PATTERN.test(key)) { + errors.push( + `${where}.${level} has an invalid key ${JSON.stringify(key)}`, + ); + } if (!isPresetValue(entry)) { errors.push( `${where}.${level}.${key} must be a boolean, number, string or string array`, diff --git a/packages/plugin-sdk/src/testing.ts b/packages/plugin-sdk/src/testing.ts index ef59f73ff..2142830df 100644 --- a/packages/plugin-sdk/src/testing.ts +++ b/packages/plugin-sdk/src/testing.ts @@ -885,7 +885,7 @@ export function createFakeContext( activities.push({ hostId, type, hostName }); }, status: { - get: (hostId) => hostStatuses.get(hostId) ?? null, + get: async (hostId) => hostStatuses.get(hostId) ?? null, check: async (hostId) => hostStatuses.get(hostId) ?? null, reportLogin: (hostId, outcome) => { statusReports.push({ hostId, ...outcome }); @@ -1546,7 +1546,7 @@ export function createMockCtx( return ctx.hosts.recordActivity(hostId, type, hostName); }, status: { - get: (hostId) => { + get: async (hostId) => { require("hosts:read"); return ctx.hosts.status.get(hostId); }, diff --git a/plugins/file-manager/manifest.json b/plugins/file-manager/manifest.json index e80f53160..1cdef990f 100644 --- a/plugins/file-manager/manifest.json +++ b/plugins/file-manager/manifest.json @@ -17,6 +17,7 @@ "capabilities": [ "hosts:read", "credentials:use", + "credentials:read", "ssh:connect", "db:own", "network:serve", diff --git a/plugins/host-metrics/manifest.json b/plugins/host-metrics/manifest.json index 34e2ad36a..7e29bcfa7 100644 --- a/plugins/host-metrics/manifest.json +++ b/plugins/host-metrics/manifest.json @@ -17,6 +17,7 @@ "capabilities": [ "hosts:read", "credentials:use", + "credentials:read", "ssh:connect", "db:own", "network:serve", diff --git a/plugins/host-metrics/src/backend/poller.ts b/plugins/host-metrics/src/backend/poller.ts index f1715c6e3..fd61c400f 100644 --- a/plugins/host-metrics/src/backend/poller.ts +++ b/plugins/host-metrics/src/backend/poller.ts @@ -300,9 +300,8 @@ export class MetricsPoller { this.initialRequested.add(host.id); void this.state.initialLimiter .run(async () => { - let status: PluginHostStatusEntry | null = this.ctx.hosts.status.get( - host.id, - ); + let status: PluginHostStatusEntry | null = + await this.ctx.hosts.status.get(host.id); if (!status) status = await this.ctx.hosts.status.check(host.id); if (!canStartInitialMetrics(status?.status, this.hasViewers(host.id))) { return; diff --git a/plugins/remote-desktop/src/backend/guacamole-server.ts b/plugins/remote-desktop/src/backend/guacamole-server.ts index 4cf585dea..2d2365f6f 100644 --- a/plugins/remote-desktop/src/backend/guacamole-server.ts +++ b/plugins/remote-desktop/src/backend/guacamole-server.ts @@ -264,6 +264,23 @@ export function createGuacamoleServer( return { handleUpgrade(request, socket, head) { + // The route is public, so the token is the only credential: refuse + // anything forged, edited or expired before guacamole-lite reads it. + const token = new URL( + request.url ?? "/", + "http://localhost", + ).searchParams.get("token"); + if (!token || !deps.tokens.verifyToken(token)) { + try { + socket.write( + "HTTP/1.1 401 Unauthorized\r\nConnection: close\r\n\r\n", + ); + } catch { + // The peer may already be gone. + } + socket.destroy(); + return; + } const wss = ( server as unknown as { webSocketServer?: { diff --git a/plugins/remote-desktop/src/backend/routes.ts b/plugins/remote-desktop/src/backend/routes.ts index 66a8dd036..4fce49175 100644 --- a/plugins/remote-desktop/src/backend/routes.ts +++ b/plugins/remote-desktop/src/backend/routes.ts @@ -3,7 +3,10 @@ import net from "net"; import path from "path"; import type { Request, Response, Router } from "express"; import type { PluginContext } from "@termix/plugin-sdk/backend"; -import type { GuacamoleTokenService } from "./token-service.js"; +import { + isServerOwnedSetting, + type GuacamoleTokenService, +} from "./token-service.js"; import type { GuacdOptions } from "./guacd-config.js"; import type { RemoteSessions } from "./sessions.js"; import type { RecordingsWriter } from "./guacamole-server.js"; @@ -188,6 +191,9 @@ export function registerRoutes(router: Router, deps: RouteDeps): void { } const options: Record = {}; for (const [key, value] of Object.entries(raw)) { + // A quick connect names a host, never where guacd is or what paths + // it writes to: those would let any user aim guacd anywhere. + if (isServerOwnedSetting(key)) continue; if (value !== "auto") options[key] = value; } const host = String(hostname); @@ -199,6 +205,7 @@ export function registerRoutes(router: Router, deps: RouteDeps): void { for (const [key, value] of Object.entries( await readUserDefaults(ctx, userId), )) { + if (isServerOwnedSetting(key)) continue; if (options[key] === undefined) options[key] = value; } } diff --git a/plugins/remote-desktop/src/backend/token-service.ts b/plugins/remote-desktop/src/backend/token-service.ts index 4e6d96afa..27bfd82e2 100644 --- a/plugins/remote-desktop/src/backend/token-service.ts +++ b/plugins/remote-desktop/src/backend/token-service.ts @@ -47,6 +47,8 @@ export interface GuacamoleToken { connection: GuacamoleConnectionSettings; recording?: GuacamoleRecordingMetadata; termixMeta?: TermixGuacMeta; + /** When the token stops opening displays, in ms since the epoch. */ + exp?: number; } export interface GuacamoleRecordingMetadata { @@ -63,6 +65,26 @@ export interface GuacamoleRecordingMetadata { const CIPHER = "aes-256-cbc"; const KEY_LENGTH = 32; +/** + * How long a token opens a display for. Short, because a token for a saved + * host carries its password and ends up in the display URL. + */ +export const TOKEN_TTL_MS = 5 * 60 * 1000; + +/** + * Settings the server decides, never the caller: where guacd is, and every + * path guacd writes or reads on its own filesystem. + */ +export function isServerOwnedSetting(key: string): boolean { + return ( + key.startsWith("guacd") || + key.endsWith("-path") || + key.startsWith("create-") || + key.startsWith("recording-") || + key.startsWith("typescript-") + ); +} + const silentLogger: TokenLogger = { warn: () => {}, error: () => {} }; /** @@ -71,9 +93,17 @@ const silentLogger: TokenLogger = { warn: () => {}, error: () => {} }; */ export class GuacamoleTokenService { private encryptionKey: Buffer; + private macKey: Buffer; - constructor(private readonly log: TokenLogger = silentLogger) { + constructor( + private readonly log: TokenLogger = silentLogger, + private readonly now: () => number = Date.now, + ) { this.encryptionKey = this.initializeKey(); + this.macKey = crypto + .createHmac("sha256", this.encryptionKey) + .update("termix-guacamole-token-mac") + .digest(); } private initializeKey(): Buffer { @@ -105,12 +135,17 @@ export class GuacamoleTokenService { return this.encryptionKey; } + /** + * guacamole-lite only knows AES-CBC, which does not notice tampering, so + * the ciphertext also carries an HMAC that verifyToken checks before + * guacamole-lite ever sees the token. + */ encryptToken(tokenObject: GuacamoleToken): string { const iv = crypto.randomBytes(16); const cipher = crypto.createCipheriv(CIPHER, this.encryptionKey, iv); let encrypted = cipher.update( - JSON.stringify(tokenObject), + JSON.stringify({ ...tokenObject, exp: this.now() + TOKEN_TTL_MS }), "utf8", "base64", ); @@ -119,11 +154,50 @@ export class GuacamoleTokenService { const data = { iv: iv.toString("base64"), value: encrypted, + mac: this.mac(iv.toString("base64"), encrypted), }; return Buffer.from(JSON.stringify(data)).toString("base64"); } + private mac(iv: string, value: string): string { + return crypto + .createHmac("sha256", this.macKey) + .update(`${iv}.${value}`) + .digest("base64"); + } + + /** + * The token if it is one this server minted, untouched and not expired, + * else null. The gate in front of guacamole-lite. + */ + verifyToken(token: string): GuacamoleToken | null { + let data: { iv?: unknown; value?: unknown; mac?: unknown }; + try { + data = JSON.parse(Buffer.from(token, "base64").toString("utf8")); + } catch { + return null; + } + if ( + typeof data?.iv !== "string" || + typeof data.value !== "string" || + typeof data.mac !== "string" + ) { + return null; + } + const expected = Buffer.from(this.mac(data.iv, data.value), "base64"); + const given = Buffer.from(data.mac, "base64"); + if ( + expected.length !== given.length || + !crypto.timingSafeEqual(expected, given) + ) { + return null; + } + const decoded = this.decryptToken(token); + if (!decoded || typeof decoded.exp !== "number") return null; + return decoded.exp > this.now() ? decoded : null; + } + decryptToken(token: string): GuacamoleToken | null { try { const data = JSON.parse(Buffer.from(token, "base64").toString("utf8")); diff --git a/plugins/remote-desktop/tests/backend/routes.test.ts b/plugins/remote-desktop/tests/backend/routes.test.ts index 278e8eb04..f1cc4d141 100644 --- a/plugins/remote-desktop/tests/backend/routes.test.ts +++ b/plugins/remote-desktop/tests/backend/routes.test.ts @@ -261,6 +261,26 @@ describe("the rest of the routes", () => { ).toMatchObject({ hostname: "10.0.0.9", port: 3389, "color-depth": 24 }); }); + it("ignores guacd and path settings a quick connect tries to set", async () => { + server = await start(); + const response = await server.request("POST", "/token", { + type: "rdp", + hostname: "10.0.0.9", + guacdHost: "attacker.example", + guacdPort: 4822, + "drive-path": "/etc", + "recording-path": "/tmp/x", + width: 800, + }); + expect(response.status).toBe(200); + const token = tokens.decryptToken(response.body.token); + expect(token?.connection.guacdHost).toBeUndefined(); + expect(token?.connection.guacdPort).toBeUndefined(); + expect(token?.connection.settings["drive-path"]).toBeUndefined(); + expect(token?.connection.settings["recording-path"]).toBeUndefined(); + expect(token?.connection.settings.width).toBe(800); + }); + it("offers native RDP only in the desktop app", async () => { server = await start(); expect((await server.request("GET", "/native-rdp")).body).toEqual({ diff --git a/plugins/remote-desktop/tests/backend/token-security.test.ts b/plugins/remote-desktop/tests/backend/token-security.test.ts new file mode 100644 index 000000000..7a033d99e --- /dev/null +++ b/plugins/remote-desktop/tests/backend/token-security.test.ts @@ -0,0 +1,93 @@ +/** + * A display token is the only credential on the public /display socket, so + * it has to be one this server minted, unedited and recent. + */ + +import { describe, expect, it } from "vitest"; +import { + GuacamoleTokenService, + TOKEN_TTL_MS, + isServerOwnedSetting, +} from "../../src/backend/token-service.js"; + +process.env.JWT_SECRET = "remote-desktop-test-secret"; + +function decode(token: string) { + return JSON.parse(Buffer.from(token, "base64").toString("utf8")) as { + iv: string; + value: string; + mac: string; + }; +} + +function encode(data: unknown): string { + return Buffer.from(JSON.stringify(data)).toString("base64"); +} + +describe("verifyToken", () => { + it("accepts a fresh token it minted", () => { + const tokens = new GuacamoleTokenService(); + const token = tokens.createRdpToken("10.0.0.7", "admin", "secret"); + expect(tokens.verifyToken(token)?.connection.settings.hostname).toBe( + "10.0.0.7", + ); + }); + + it("refuses an edited ciphertext or IV", () => { + const tokens = new GuacamoleTokenService(); + const data = decode(tokens.createRdpToken("10.0.0.7", "admin", "secret")); + const iv = Buffer.from(data.iv, "base64"); + iv[0] ^= 0xff; + expect( + tokens.verifyToken(encode({ ...data, iv: iv.toString("base64") })), + ).toBeNull(); + expect( + tokens.verifyToken(encode({ ...data, value: `A${data.value.slice(1)}` })), + ).toBeNull(); + }); + + it("refuses a token with no MAC, as guacamole-lite alone would accept", () => { + const tokens = new GuacamoleTokenService(); + const { iv, value } = decode(tokens.createRdpToken("h", "u", "p")); + expect(tokens.verifyToken(encode({ iv, value }))).toBeNull(); + }); + + it("refuses a token once it is older than the TTL", () => { + let now = 1_000_000; + const tokens = new GuacamoleTokenService(undefined, () => now); + const token = tokens.createJoinToken("session-1", true); + now += TOKEN_TTL_MS - 1; + expect(tokens.verifyToken(token)).not.toBeNull(); + now += 2; + expect(tokens.verifyToken(token)).toBeNull(); + }); + + it("refuses garbage", () => { + const tokens = new GuacamoleTokenService(); + expect(tokens.verifyToken("not-a-token")).toBeNull(); + expect(tokens.verifyToken(encode({ iv: 1 }))).toBeNull(); + }); +}); + +describe("isServerOwnedSetting", () => { + it.each([ + "guacdHost", + "guacdPort", + "drive-path", + "create-drive-path", + "recording-path", + "recording-name", + "create-recording-path", + "typescript-path", + "sftp-private-key-path", + ])("keeps %s for the server", (key) => { + expect(isServerOwnedSetting(key)).toBe(true); + }); + + it.each(["width", "security", "ignore-cert", "color-depth", "enable-drive"])( + "lets a caller set %s", + (key) => { + expect(isServerOwnedSetting(key)).toBe(false); + }, + ); +}); diff --git a/plugins/session-sharing/src/backend/services.ts b/plugins/session-sharing/src/backend/services.ts index bb3ddbcc8..6d379404c 100644 --- a/plugins/session-sharing/src/backend/services.ts +++ b/plugins/session-sharing/src/backend/services.ts @@ -128,6 +128,10 @@ export function createSessionGuests(deps: SharingDeps): SessionGuestsV1 { return { async resolve({ shareToken, roomGuestToken, clientIp }) { if (shareToken) { + // Guessing link tokens is limited the same way room tokens are. + if (guestLimiter.isLimited(clientIp)) { + return { ok: false, reason: "Too many requests" }; + } const share = await shares.findByLinkToken(shareToken); if (!share || share.shareType !== "link") { return { ok: false, reason: "Invalid or expired share link" }; diff --git a/plugins/ssh-terminal/manifest.json b/plugins/ssh-terminal/manifest.json index 9cc44fe8e..fc21ac1b1 100644 --- a/plugins/ssh-terminal/manifest.json +++ b/plugins/ssh-terminal/manifest.json @@ -17,6 +17,7 @@ "capabilities": [ "hosts:read", "credentials:use", + "credentials:read", "ssh:connect", "network:serve", "db:own", diff --git a/plugins/ssh-terminal/src/backend/terminal-socket.ts b/plugins/ssh-terminal/src/backend/terminal-socket.ts index 56c895a07..03c2671be 100644 --- a/plugins/ssh-terminal/src/backend/terminal-socket.ts +++ b/plugins/ssh-terminal/src/backend/terminal-socket.ts @@ -214,7 +214,10 @@ export function createTerminalSocket(deps: TerminalSocketDeps) { const participant = liveSession ? sessionManager.getParticipantForWs(liveSession, ws) : null; - if (!isMessageAllowedForParticipant(participant, type)) { + // A guest is only ever a participant. Once it has left (or was never + // added) it may send nothing, or a read-only guest could "disconnect" + // and then type into the owner's shell as nobody in particular. + if (!participant || !isMessageAllowedForParticipant(participant, type)) { return; } @@ -238,6 +241,7 @@ export function createTerminalSocket(deps: TerminalSocketDeps) { break; case "disconnect": sessionManager.removeParticipant(currentSessionId, ws); + ws.close(1000, "Left the session"); break; default: break; diff --git a/plugins/ssh-terminal/tests/backend/guest-socket.test.ts b/plugins/ssh-terminal/tests/backend/guest-socket.test.ts new file mode 100644 index 000000000..a8bae3557 --- /dev/null +++ b/plugins/ssh-terminal/tests/backend/guest-socket.test.ts @@ -0,0 +1,126 @@ +/** + * A share-link guest only ever acts as a participant. Once it has left the + * session it can send nothing, so a read-only guest cannot "disconnect" and + * then type into the owner's shell. + */ + +import { EventEmitter } from "node:events"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createTerminalSocket } from "../../src/backend/terminal-socket.js"; + +class FakeSocket extends EventEmitter { + readyState = 1; + sent: string[] = []; + closedWith: number | null = null; + send(message: string) { + this.sent.push(message); + } + close(code = 1000) { + this.closedWith = code; + this.readyState = 3; + this.emit("close"); + } + ping() {} + terminate() { + this.close(1006); + } + message(type: string, data?: unknown) { + this.emit("message", Buffer.from(JSON.stringify({ type, data }))); + } +} + +function fakeSessions(permissionLevel: "read-only" | "read-write") { + const written: string[] = []; + const participants = new Map< + unknown, + { isOwner: boolean; permissionLevel: string } + >(); + const session = { + isConnected: true, + sshStream: { write: (chunk: Buffer) => written.push(chunk.toString()) }, + }; + const manager = { + getSession: (id: string) => (id === "s1" ? session : null), + joinAsParticipant: (_id: string, ws: unknown) => { + participants.set(ws, { isOwner: false, permissionLevel }); + return "s1"; + }, + getBuffer: () => "", + getParticipantForWs: (_session: unknown, ws: unknown) => + participants.get(ws) ?? null, + removeParticipant: (_id: string, ws: unknown) => { + participants.delete(ws); + }, + bufferInput: () => {}, + }; + return { manager, written }; +} + +function socketFor(permissionLevel: "read-only" | "read-write") { + const sessions = fakeSessions(permissionLevel); + const log = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + const terminal = createTerminalSocket({ + ctx: {} as never, + log: log as never, + sessionManager: sessions.manager as never, + getTmux: () => null, + getSharing: () => null, + getGuests: () => ({ + resolve: async () => ({ + ok: true, + share: { id: "share-1", sessionId: "s1", permissionLevel }, + }), + recordJoin: async () => {}, + }), + } as never); + return { terminal, written: sessions.written }; +} + +async function joinAsGuest(terminal: ReturnType["terminal"]) { + const ws = new FakeSocket(); + await terminal.handleConnection({ + socket: ws, + request: { url: "/terminal?shareToken=abc", headers: {} }, + userId: "", + clientIp: "10.0.0.1", + requestOrigin: "", + isDataUnlocked: () => false, + } as never); + return ws; +} + +let cleanup: Array<() => void> = []; +afterEach(() => { + cleanup.forEach((fn) => fn()); + cleanup = []; +}); + +describe("share-link guests", () => { + it("cannot type into a read-only session", async () => { + const { terminal, written } = socketFor("read-only"); + const ws = await joinAsGuest(terminal); + cleanup.push(() => terminal.closeAll()); + ws.message("input", "rm -rf ~\n"); + expect(written).toEqual([]); + }); + + it("cannot type after leaving the session", async () => { + const { terminal, written } = socketFor("read-only"); + const ws = await joinAsGuest(terminal); + cleanup.push(() => terminal.closeAll()); + ws.message("disconnect"); + ws.message("input", "rm -rf ~\n"); + expect(written).toEqual([]); + expect(ws.closedWith).toBe(1000); + }); + + it("a read-write guest types until it leaves", async () => { + const { terminal, written } = socketFor("read-write"); + const ws = await joinAsGuest(terminal); + cleanup.push(() => terminal.closeAll()); + ws.message("input", "ls\n"); + ws.message("disconnect"); + ws.message("input", "whoami\n"); + expect(written).toEqual(["ls\n"]); + }); +}); diff --git a/plugins/sso/src/backend/login.ts b/plugins/sso/src/backend/login.ts index fa7390b0a..1212cadbf 100644 --- a/plugins/sso/src/backend/login.ts +++ b/plugins/sso/src/backend/login.ts @@ -418,7 +418,12 @@ export function createSsoLogin(ctx: PluginContext, store: ProviderStore) { kind: "external", provider: providerKey(provider), subject: String(identifier), - email: (userInfo.email as string | undefined) ?? null, + // An address the provider says it has not verified does not count + // toward the allowed list. + email: + userInfo.email_verified === false || userInfo.email_verified === "false" + ? null + : ((userInfo.email as string | undefined) ?? null), name, groups, isAdmin, diff --git a/plugins/tailscale/manifest.json b/plugins/tailscale/manifest.json index 7e3a5ca30..e5e046cb6 100644 --- a/plugins/tailscale/manifest.json +++ b/plugins/tailscale/manifest.json @@ -22,6 +22,7 @@ "auth:provide", "ssh:connect", "credentials:use", + "credentials:read", "ui:surface" ], "optionalDependencies": { diff --git a/src/backend/auth/login-pipeline.ts b/src/backend/auth/login-pipeline.ts index 079f7934c..b2d5417cd 100644 --- a/src/backend/auth/login-pipeline.ts +++ b/src/backend/auth/login-pipeline.ts @@ -105,6 +105,9 @@ export async function evaluateSecondFactors( try { if (await factor.isEnrolled(userId)) required.set(key, factor); } catch (error) { + // Unknown is not "not enrolled": the login stops instead of skipping + // a factor the user may well have. + blocked.push({ pluginId: factor.pluginId, factorId: factor.id }); authLogger.warn("Second factor enrolment check failed", { operation: "second_factor_enrolment_check", factorId: factor.id, diff --git a/src/backend/auth/provisioning.ts b/src/backend/auth/provisioning.ts index 3b07c45a6..d6677da2c 100644 --- a/src/backend/auth/provisioning.ts +++ b/src/backend/auth/provisioning.ts @@ -163,10 +163,12 @@ async function findLinkedUser( } // Accounts from before user_external_identities matched on the raw - // identifier column; link them the first time they sign in. + // identifier column; link them the first time they sign in. The column is + // not scoped to a provider, so only an SSO account that has no link yet, + // from the same provider when both say which, can be claimed this way. if (identity.legacyIdentifier) { const legacy = await users.findByOidcIdentifier(identity.legacyIdentifier); - if (legacy) { + if (legacy && (await mayClaimLegacyAccount(legacy, identity))) { await identities.linkIdentity({ userId: legacy.id, providerId: identity.provider, @@ -179,6 +181,42 @@ async function findLinkedUser( return null; } +async function mayClaimLegacyAccount( + legacy: UserRecord, + identity: ExternalIdentity, +): Promise { + if (!legacy.isOidc) return false; + if ( + legacy.ssoProviderId != null && + identity.ssoProviderId != null && + legacy.ssoProviderId !== identity.ssoProviderId + ) { + return false; + } + const links = await createCurrentUserAuthRepository().listIdentitiesForUser( + legacy.id, + ); + return links.length === 0; +} + +/** + * A username nobody else has. An identity provider's display name must never + * give an SSO account the same username as a local one, such as "admin". + */ +async function freeUsername( + wanted: string, + selfId: string | null, +): Promise { + const users = createCurrentUserRepository(); + let candidate = wanted; + for (let attempt = 0; attempt < 5; attempt++) { + const holder = await users.findByUsername(candidate); + if (!holder || holder.id === selfId) return candidate; + candidate = `${wanted}-${nanoid(4)}`; + } + return `${wanted}-${nanoid(10)}`; +} + async function createUser( identity: ExternalIdentity, deviceType: string, @@ -187,7 +225,7 @@ async function createUser( const id = nanoid(); const created = await users.createFirstSsoUser({ id, - username: identity.name || identity.subject, + username: await freeUsername(identity.name || identity.subject, null), passwordHash: "", isAdmin: !!identity.isAdmin, isOidc: true, @@ -303,10 +341,16 @@ export async function findOrProvisionExternalUser( // A user who also has a password picked their own username; leave it. const isDualAuth = !!user.passwordHash && user.passwordHash.trim() !== ""; if (!isDualAuth && identity.name && user.username !== identity.name) { - user = - (await createCurrentUserRepository().update(user.id, { - username: identity.name, - })) ?? user; + // Taken by someone else: keep the name the account already has. + const holder = await createCurrentUserRepository().findByUsername( + identity.name, + ); + if (!holder || holder.id === user.id) { + user = + (await createCurrentUserRepository().update(user.id, { + username: identity.name, + })) ?? user; + } } if (identity.isAdmin !== undefined) { diff --git a/src/backend/database/routes/host-plugin-settings.ts b/src/backend/database/routes/host-plugin-settings.ts index abac2eba4..4f2454024 100644 --- a/src/backend/database/routes/host-plugin-settings.ts +++ b/src/backend/database/routes/host-plugin-settings.ts @@ -225,21 +225,27 @@ export async function withHostPluginSettings( } /** - * Writes a set of host-scope plugin settings in one call, for a core route - * that still accepts a plugin's fields inline on the host create/update body - * (the fields haven't grown their own editor UI flow yet). Values are - * JSON-stringified the same way ctx.settings.setHost stores them. + * Writes a set of host-scope plugin settings in one call, through the same + * path as every other settings write: an undeclared key is refused, a value + * is validated, and a secret field is encrypted. */ export async function writeHostPluginSettings( - pluginId: string, + manifest: PluginManifest, hostId: number, values: Record, ): Promise { - const repository = createCurrentPluginSettingsRepository(); - const scopeId = String(hostId); for (const [key, value] of Object.entries(values)) { if (value === undefined) continue; - await repository.set(pluginId, "host", scopeId, key, JSON.stringify(value)); + const error = await setSetting(manifest, "host", hostId, key, value); + if (error) { + sshLogger.warn("Refused a plugin host setting on import", { + operation: "host_import_plugin_settings", + pluginId: manifest.id, + hostId, + key, + error, + }); + } } } @@ -296,7 +302,7 @@ export async function applyPluginHostImportSettings( `${manifest.id}.hostImportNormalizer`, ); const values = normalizer?.(raw); - if (values) await writeHostPluginSettings(manifest.id, hostId, values); + if (values) await writeHostPluginSettings(manifest, hostId, values); } catch (error) { sshLogger.warn("Plugin host import normalizer failed", { operation: "host_import_plugin_settings", diff --git a/src/backend/database/routes/plugin-api-routes.ts b/src/backend/database/routes/plugin-api-routes.ts index e5dfe69bb..cffc554e6 100644 --- a/src/backend/database/routes/plugin-api-routes.ts +++ b/src/backend/database/routes/plugin-api-routes.ts @@ -16,7 +16,7 @@ import express, { type Request, type Response } from "express"; import { databaseLogger } from "../../utils/logger.js"; -import { getPluginRouter } from "../../plugins/http.js"; +import { getPluginRouter, isPluginInstalled } from "../../plugins/http.js"; import { getRequestBasePath } from "../../utils/request-origin.js"; import type { PluginLegacyRedirect } from "@termix/plugin-sdk/manifest"; @@ -51,7 +51,7 @@ const router = express.Router(); * 401: * description: Authentication required. * 404: - * description: Plugin not installed or serves no routes. + * description: No plugin with this id is installed. * 503: * description: Plugin is installed but not running. */ @@ -60,17 +60,26 @@ router.use("/:pluginId", (req: Request, res: Response, next) => { const pluginRouter = getPluginRouter(pluginId); if (!pluginRouter) { - databaseLogger.warn("Plugin API request for unregistered plugin", { - operation: "plugin_api_dispatch", - pluginId, - }); - res.status(404).json({ error: "Plugin not installed or not running" }); + notServing(res, pluginId); return; } pluginRouter(req, res, next); }); +/** 503 for an installed plugin that is off, 404 for an unknown id. */ +function notServing(res: Response, pluginId: string): void { + if (isPluginInstalled(pluginId)) { + res.status(503).json({ error: "Plugin is not running", pluginId }); + return; + } + databaseLogger.warn("Plugin API request for unregistered plugin", { + operation: "plugin_api_dispatch", + pluginId, + }); + res.status(404).json({ error: "Plugin not installed" }); +} + /** * Mounts the dispatcher. No auth in front of it: each plugin router runs * core auth itself and skips it only for the paths it declared public, so a @@ -120,7 +129,7 @@ export function mountPluginLegacyPaths( if (!plugin.legacyPaths.some((path) => under(req.path, path))) continue; const pluginRouter = getPluginRouter(plugin.id); if (!pluginRouter) { - res.status(404).json({ error: "Plugin not installed or not running" }); + notServing(res, plugin.id); return; } req.url = req.url.slice(plugin.id.length + 1) || "/"; diff --git a/src/backend/database/routes/plugins.ts b/src/backend/database/routes/plugins.ts index 54d37f62b..cf842f76b 100644 --- a/src/backend/database/routes/plugins.ts +++ b/src/backend/database/routes/plugins.ts @@ -16,6 +16,8 @@ import { getPluginRuntime } from "../../plugins/index.js"; import { describePluginFrontend } from "../../plugins/assets.js"; import { getSetting } from "../../plugins/settings.js"; import { invalidatePluginPermissionCache } from "../../plugins/permissions.js"; +import { getPluginPublicHttpRoutes } from "../../plugins/http.js"; +import { getPluginPublicWsRoutes } from "../../plugins/ws.js"; import { findField, getAllSettings, @@ -191,8 +193,10 @@ export async function listPreLoginPlugins(): Promise< * * Non-admins get only what the shell needs. What a plugin is allowed to * do, what has been granted to it and why it failed are operational - * details, so capabilities, grants and lastError are included only for - * holders of admin.plugins.manage. + * details, so capabilities, grants, lastError and publicRoutes are + * included only for holders of admin.plugins.manage. publicRoutes lists + * the HTTP paths and socket paths a running plugin serves without + * core's login check. * * The frontend fields drive the browser's plugin loader: `frontend` * says there is a bundle at /plugin-assets//frontend.js, @@ -283,6 +287,10 @@ router.get("/", authenticateJWT, async (req: Request, res: Response) => { capabilities, grantedCapabilities: grantsByPlugin.get(record.id) ?? [], lastError: loaded?.lastError ?? record.lastError ?? null, + publicRoutes: { + http: getPluginPublicHttpRoutes(record.id), + ws: getPluginPublicWsRoutes(record.id), + }, }; }), ); @@ -640,7 +648,7 @@ router.delete( * /plugins/{id}/data: * delete: * summary: Delete everything a plugin stores - * description: Drops the plugin's own tables and clears its key/value state, capability grants and migration ledger. Disabling a plugin never touches its data; this is the explicit uninstall path, and it cannot be undone. The plugin is deactivated first so nothing is writing while its tables go. + * description: Drops every table under the plugin's prefix and clears its key/value state, settings and secrets, capability grants and migration ledger. Role permissions stay. Disabling a plugin never touches its data; this is the explicit uninstall path, and it cannot be undone. The plugin is deactivated first so nothing is writing while its tables go. * tags: * - Plugins * parameters: @@ -680,7 +688,11 @@ router.delete( } const { removePluginData } = await import("../../plugins/data.js"); - const removed = await removePluginData(pluginId); + const removed = await removePluginData(pluginId, { + knownPluginIds: getPluginRuntime() + .loader.list() + .map((plugin) => plugin.id), + }); databaseLogger.warn(`Removed all data for plugin ${pluginId}`, { operation: "plugin_remove_data", diff --git a/src/backend/plugins/assets.ts b/src/backend/plugins/assets.ts index a50b8c3c6..af05b73f4 100644 --- a/src/backend/plugins/assets.ts +++ b/src/backend/plugins/assets.ts @@ -3,20 +3,23 @@ // Unauthenticated on purpose, the same as /assets: import() cannot carry the // bearer header, and nothing here is secret. It only ever serves files from // the two directories a plugin ships for the browser, so a request can never -// reach the plugin's backend bundle, migrations or anything outside it. +// reach the plugin's backend bundle, migrations or anything outside it, and +// only for a plugin that is running. import crypto from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import express, { type Request, type Response } from "express"; import { + DEFAULT_BACKEND_ENTRY, DEFAULT_FRONTEND_ENTRY, DEFAULT_LOCALES_DIR, type PluginManifest, } from "@termix/plugin-sdk/manifest"; import type { LoadedPlugin } from "./loader.js"; -type AssetPlugin = Pick; +type AssetPlugin = Pick & + Partial>; export interface PluginFrontendInfo { frontend: boolean; @@ -135,9 +138,41 @@ export function resolvePluginAsset( const file = path.resolve(root, rest); if (!isInside(root, file) || !fileExists(file)) return null; + + // The text check above cannot see a symlink, so the real paths have to + // agree too: a link inside dist/ must not reach a file outside the plugin. + let real: string; + try { + real = fs.realpathSync(file); + if (!isInside(fs.realpathSync(plugin.dir), real)) return null; + } catch { + return null; + } + if (isServerOnly(plugin, real)) return null; return file; } +/** The backend entry and the migrations, wherever the manifest put them. */ +function isServerOnly(plugin: AssetPlugin, real: string): boolean { + const manifest = plugin.manifest as PluginManifest; + const backend = path.resolve( + plugin.dir, + manifest.backend ?? DEFAULT_BACKEND_ENTRY, + ); + const candidates = [backend, `${backend}.map`]; + for (const candidate of candidates) { + try { + if (fs.realpathSync(candidate) === real) return true; + } catch { + // Not there. + } + } + const segments = path + .relative(fs.realpathSync(plugin.dir), real) + .split(path.sep); + return segments.includes("migrations"); +} + const CONTENT_TYPES: Record = { ".js": "text/javascript; charset=utf-8", ".css": "text/css; charset=utf-8", @@ -182,7 +217,7 @@ export function createPluginAssetsRouter( * 200: * description: The file. * 404: - * description: Unknown plugin or file. + * description: Unknown or stopped plugin, or no such file. */ router.get( /^\/([a-z][a-z0-9-]{1,39})\/(.+)$/, @@ -190,7 +225,11 @@ export function createPluginAssetsRouter( const params = req.params as unknown as Record; const pluginId = params[0]; const plugin = getPlugin(pluginId); - const file = plugin ? resolvePluginAsset(plugin, params[1]) : null; + // A disabled or failed plugin serves nothing, so its bundle cannot be + // loaded around the shell's own enabled check. + const running = !!plugin && (!plugin.state || plugin.state === "active"); + const file = + plugin && running ? resolvePluginAsset(plugin, params[1]) : null; // Loaded as a module from Electron's file:// page and the Vite dev // server, so any origin may read it. No credentials are involved. diff --git a/src/backend/plugins/conflicts.ts b/src/backend/plugins/conflicts.ts index 419ce7ccf..c171a376c 100644 --- a/src/backend/plugins/conflicts.ts +++ b/src/backend/plugins/conflicts.ts @@ -6,7 +6,7 @@ */ export interface RegistrationConflict { - kind: "service" | "registry" | "permission" | "ws"; + kind: "service" | "registry" | "permission" | "ws" | "id"; pluginId: string; /** Who already held it, when known. */ heldBy?: string; diff --git a/src/backend/plugins/ctx-credentials.ts b/src/backend/plugins/ctx-credentials.ts index b3c133e31..497ed5299 100644 --- a/src/backend/plugins/ctx-credentials.ts +++ b/src/backend/plugins/ctx-credentials.ts @@ -26,9 +26,8 @@ import type { PluginSshKeyCredential, } from "@termix/plugin-sdk/backend"; import ssh2 from "ssh2"; -import { PluginCapabilityError } from "@termix/plugin-sdk/backend"; import type { PluginManifest } from "@termix/plugin-sdk/manifest"; -import { assertCapability } from "./permissions.js"; +import { assertCapability, capabilityRefused } from "./permissions.js"; import { getActor } from "./actor.js"; import type { DisposableBag } from "./disposables.js"; import { registerSecretResolver } from "../hosts/connect/secret-resolver-registry.js"; @@ -293,7 +292,7 @@ export function createPluginCredentials({ registerSecretResolver: (scheme, resolve) => { if (!declared.includes("auth:provide")) { - throw new PluginCapabilityError(pluginId, "auth:provide"); + throw capabilityRefused(pluginId, "auth:provide"); } if (!manifest.contributes?.auth?.secretSchemes?.includes(scheme)) { throw new Error( diff --git a/src/backend/plugins/ctx-hosts.ts b/src/backend/plugins/ctx-hosts.ts index c19739c5a..7a85b2036 100644 --- a/src/backend/plugins/ctx-hosts.ts +++ b/src/backend/plugins/ctx-hosts.ts @@ -21,8 +21,7 @@ import type { PluginShareableRole, } from "@termix/plugin-sdk/backend"; import type { PluginManifest } from "@termix/plugin-sdk/manifest"; -import { PluginCapabilityError } from "@termix/plugin-sdk/backend"; -import { assertCapability } from "./permissions.js"; +import { assertCapability, capabilityRefused } from "./permissions.js"; import { hostSessionStatus } from "../hosts/host-session-status.js"; import { getActor } from "./actor.js"; import { hostStatusService } from "../hosts/status/host-status-service.js"; @@ -65,8 +64,56 @@ function toSummary(host: { }; } +/** + * Host fields that carry secret material. Stripped from anything ctx.hosts or + * ctx.ssh hands a plugin, unless the plugin holds credentials:read. + */ +export const HOST_SECRET_FIELDS = [ + "password", + "key", + "keyPassword", + "privateKey", + "passphrase", + "sudoPassword", + "socks5Password", + "rdpPassword", + "vncPassword", + "telnetPassword", + "autostartPassword", + "autostartKey", + "autostartKeyPassword", + "vaultToken", +] as const; + +/** A copy of a host with every secret field removed, nested ones included. */ +export function redactHostSecrets>( + host: T, +): T { + const copy: Record = { ...host }; + for (const field of HOST_SECRET_FIELDS) delete copy[field]; + const terminalConfig = copy.terminalConfig; + if (terminalConfig && typeof terminalConfig === "object") { + const { sudoPassword: _sudo, ...rest } = terminalConfig as Record< + string, + unknown + >; + copy.terminalConfig = rest; + } + if (Array.isArray(copy.socks5ProxyChain)) { + copy.socks5ProxyChain = copy.socks5ProxyChain.map((hop) => + hop && typeof hop === "object" + ? redactHostSecrets(hop as Record) + : hop, + ); + } + return copy as T; +} + +/** Columns that say who a host belongs to. ctx.hosts.update never moves them. */ +const PROTECTED_HOST_FIELDS = new Set(["id", "userId", "syncId"]); + function toRecord(host: Record): PluginHostRecord { - return { + return redactHostSecrets({ id: host.id as number, userId: host.userId as string, name: (host.name as string | null) ?? null, @@ -85,7 +132,7 @@ function toRecord(host: Record): PluginHostRecord { createdAt: (host.createdAt as string | null) ?? null, updatedAt: (host.updatedAt as string | null) ?? null, ...host, - }; + }); } function actingUser(): string { @@ -105,12 +152,27 @@ export function createPluginHosts({ manifest, bag, audit }: Deps): PluginHosts { const requireRead = () => assertCapability(pluginId, "hosts:read", declared); const requireReadSync = () => { if (!declared.includes("hosts:read")) { - throw new PluginCapabilityError(pluginId, "hosts:read"); + throw capabilityRefused(pluginId, "hosts:read"); } }; const requireWrite = () => assertCapability(pluginId, "hosts:write", declared); + // With an actor, a host the actor cannot reach is out of bounds. With none, + // it is the plugin's own background work (a poller, a guacd session). + const actorMayReach = async (hostId: number): Promise => { + const actor = getActor(); + if (!actor) return true; + const { PermissionManager } = + await import("../utils/permission-manager.js"); + const access = await PermissionManager.getInstance().canAccessHost( + actor, + hostId, + "connect", + ); + return access.hasAccess; + }; + return { list: async () => { await requireRead(); @@ -235,13 +297,18 @@ export function createPluginHosts({ manifest, bag, audit }: Deps): PluginHosts { throw error; } const userId = actingUser(); + const allowed = Object.fromEntries( + Object.entries(patch as Record).filter( + ([field]) => !PROTECTED_HOST_FIELDS.has(field), + ), + ) as PluginHostUpdateInput; const { createCurrentHostRepository } = await import("../database/repositories/factory.js"); const updated = await createCurrentHostRepository().updateEncryptedForUser( userId, hostId, - patch, + allowed, ); await audit("hosts_update", `host ${hostId}`, { success: updated !== null, @@ -287,6 +354,9 @@ export function createPluginHosts({ manifest, bag, audit }: Deps): PluginHosts { await import("../database/repositories/factory.js"); const rows = await createCurrentHostRepository().listDecryptedByUserId(userId); + await audit("hosts_list_owned", `${rows.length} host(s)`, { + success: true, + }); return rows.map((row) => toRecord(row as unknown as Record), ); @@ -437,36 +507,55 @@ export function createPluginHosts({ manifest, bag, audit }: Deps): PluginHosts { }, // Synchronous so a transport can call it from an ssh2 "ready" handler, so - // only the declaration is checked here, like ctx.http.router. + // only the declaration is checked up front, like ctx.http.router. The + // session only counts once the actor's access to the host is confirmed. trackSession: (hostId: number) => { if (!declared.includes("hosts:read")) { - throw new PluginCapabilityError(pluginId, "hosts:read"); + throw capabilityRefused(pluginId, "hosts:read"); } - return hostSessionStatus.register(hostId); + let release: (() => void) | null = null; + let stopped = false; + void actorMayReach(hostId) + .then((allowed) => { + if (allowed && !stopped) release = hostSessionStatus.register(hostId); + }) + .catch(() => {}); + return () => { + stopped = true; + release?.(); + release = null; + }; }, recordActivity: async (hostId, type, hostName) => { await requireRead(); const userId = actingUser(); + if (!(await actorMayReach(hostId))) return; const { recordRecentActivity } = await import("../services/recent-activity.js"); await recordRecentActivity(userId, { type, hostId, hostName }); }, - // Synchronous and not audited, like trackSession: pollers call these on - // every sample. + // Not audited: pollers call these on every sample. status: { - get: (hostId) => { - requireReadSync(); + get: async (hostId) => { + await requireRead(); + if (!(await actorMayReach(hostId))) return null; return hostStatusService.get(hostId); }, check: async (hostId) => { await requireRead(); + if (!(await actorMayReach(hostId))) return null; return hostStatusService.check(hostId); }, + // Synchronous for the same reason as trackSession. reportLogin: (hostId, outcome) => { requireReadSync(); - hostStatusService.reportLogin(hostId, outcome); + void actorMayReach(hostId) + .then((allowed) => { + if (allowed) hostStatusService.reportLogin(hostId, outcome); + }) + .catch(() => {}); }, registerPort: (connectionType, resolve) => { requireReadSync(); diff --git a/src/backend/plugins/ctx-process.ts b/src/backend/plugins/ctx-process.ts index 9c5cbf395..02351ebf8 100644 --- a/src/backend/plugins/ctx-process.ts +++ b/src/backend/plugins/ctx-process.ts @@ -88,10 +88,10 @@ export function createPluginProcess(deps: Deps): PluginProcess { }; const run: PluginProcess["run"] = (file, args, options = {}) => - audited("process_run", `ran ${path.basename(file)}`, async () => { + audited("process_run", `ran ${path.basename(String(file))}`, async () => { const child = spawn(file, [...args], { cwd: options.cwd, - env: { ...process.env, ...(options.env ?? {}) }, + env: { ...baseEnvironment(), ...(options.env ?? {}) }, stdio: ["ignore", "pipe", "pipe"], shell: false, }); @@ -210,3 +210,46 @@ export function createPluginProcess(deps: Deps): PluginProcess { return { run, ensureBinary }; } + +/** + * What a plugin's program inherits from the server: enough to find binaries, + * a home and temp folder, the locale and the outbound proxy. Never the rest, + * which holds the JWT secret, database keys and OIDC client secrets. + */ +const INHERITED_ENV = [ + "PATH", + "PATHEXT", + "HOME", + "USERPROFILE", + "APPDATA", + "LOCALAPPDATA", + "XDG_CONFIG_HOME", + "XDG_CACHE_HOME", + "TMP", + "TEMP", + "TMPDIR", + "SystemRoot", + "windir", + "ComSpec", + "LANG", + "LC_ALL", + "TZ", + "HTTP_PROXY", + "HTTPS_PROXY", + "NO_PROXY", + "http_proxy", + "https_proxy", + "no_proxy", + "SSL_CERT_FILE", + "SSL_CERT_DIR", +]; + +export function baseEnvironment( + env: NodeJS.ProcessEnv = process.env, +): NodeJS.ProcessEnv { + const inherited: NodeJS.ProcessEnv = {}; + for (const name of INHERITED_ENV) { + if (env[name] !== undefined) inherited[name] = env[name]; + } + return inherited; +} diff --git a/src/backend/plugins/ctx-ssh-auth.ts b/src/backend/plugins/ctx-ssh-auth.ts index ebbbd0192..1d9f6785a 100644 --- a/src/backend/plugins/ctx-ssh-auth.ts +++ b/src/backend/plugins/ctx-ssh-auth.ts @@ -14,13 +14,17 @@ import type { PluginSshHost, } from "@termix/plugin-sdk/backend"; import type { PluginManifest } from "@termix/plugin-sdk/manifest"; +import { PluginSshInteractionError } from "@termix/plugin-sdk/backend"; import { - PluginCapabilityError, - PluginSshInteractionError, -} from "@termix/plugin-sdk/backend"; -import { assertCapability } from "./permissions.js"; + assertCapability, + capabilityRefused, + hasCachedCapability, + hasCapability, + warmPluginGrants, +} from "./permissions.js"; import { getActor, getActorSessionId } from "./actor.js"; import type { DisposableBag } from "./disposables.js"; +import { redactHostSecrets } from "./ctx-hosts.js"; import { getSshAuthProvider, listSshAuthProviders, @@ -74,16 +78,16 @@ function profileOf(options?: { profile?: string }): SshConnectProfile { } /** - * The user a connection runs as: the request's actor, or for background work - * on a resolved host, that host's owner. A bare host id with no actor has - * nobody to resolve it for. + * The user a connection runs as: the actor, or for a host core resolved + * earlier, the user it was resolved for. A host object's own userId is plugin + * data and is never believed, or a plugin could borrow another user's jump + * hosts and stored credentials by naming them. */ -function actingUser(host: number | PluginSshHost): string { - const actor = getActor(); +function actingUser(resolvedFor?: string): string { + const actor = getActor() ?? resolvedFor; if (actor) return actor; - if (typeof host === "object" && host.userId) return host.userId; throw new Error( - "ctx.ssh needs an acting user: call it inside a request, ctx.asUser, or pass a resolved host", + "ctx.ssh needs an acting user: call it inside a request or ctx.asUser", ); } @@ -119,6 +123,47 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { } }; + // Hosts core resolved, keyed by the redacted copy the plugin was given, so + // handing that copy back to connect still connects with the real secrets. + const resolvedHosts = new WeakMap< + object, + { host: SshConnectHost; userId: string } + >(); + + const remember = (key: object, host: SshConnectHost, userId: string) => + resolvedHosts.set(key, { host, userId }); + + const handOut = (host: SshConnectHost, userId: string): PluginSshHost => { + const redacted = redactHostSecrets( + host as unknown as Record, + ) as unknown as PluginSshHost; + remember(redacted, host, userId); + return redacted; + }; + + /** The acting user for a call about this host. */ + const userFor = (host: number | PluginSshHost | undefined): string => + actingUser( + host && typeof host === "object" + ? resolvedHosts.get(host)?.userId + : undefined, + ); + + /** + * A host for core's pipeline. An id stays an id (core resolves it with an + * access check). An object core handed out maps back to what core resolved; + * any other object is the plugin's own, so it runs as the actor. + */ + const toConnectHost = ( + host: number | PluginSshHost, + userId: string, + ): number | SshConnectHost => { + if (typeof host === "number") return host; + const known = resolvedHosts.get(host); + if (known) return known.host; + return { ...(host as unknown as SshConnectHost), userId }; + }; + /** Checks, connects and audits one new connection. */ const connectOnce = async ( host: number | PluginSshHost, @@ -134,10 +179,19 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { throw error; } - const userId = actingUser(host); + let userId: string; + try { + userId = userFor(host); + } catch (error) { + await audit("ssh_connect", describeHost(host), { + success: false, + errorMessage: error instanceof Error ? error.message : String(error), + }); + throw error; + } const { connectHost } = await import("../hosts/connect/connect-host.js"); try { - const connection = await connectHost(host as number | SshConnectHost, { + const connection = await connectHost(toConnectHost(host, userId), { userId, purpose: purposeOf(options), profile: profileOf(options), @@ -156,7 +210,7 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { connection.client.once("close", () => open.delete(dispose)); return { ...connection, - host: connection.host as unknown as PluginSshHost, + host: handOut(connection.host as SshConnectHost, userId), dispose, }; } catch (error) { @@ -188,10 +242,12 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { await checkSsh(true); const { resolveConnectHost } = await import("../hosts/connect/connect-host.js"); + const userId = userFor(host); const resolved = await resolveConnectHost( - host as number | SshConnectHost, - actingUser(host), + toConnectHost(host, userId), + userId, ); + remember(resolved, resolved, userId); const key = poolKey(options.pool, resolved as PluginSshHost); poolKeys.set(key, resolved.id); const { withConnection } = @@ -206,7 +262,7 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { jumpChain: async (jumpHosts, chainOptions) => { await checkSsh(true); - const userId = actingUser(chainOptions?.forHost ?? 0); + const userId = userFor(chainOptions?.forHost); const { createJumpHostChain } = await import("../hosts/jump-host-chain.js"); const client = await createJumpHostChain(jumpHosts, userId); @@ -227,12 +283,16 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { client.once("close", () => open.delete(dispose)); // A jump chain ends at a forwarding client, not a single resolved SSH // host: report the caller's own host if it gave one, else the last hop. - const host: PluginSshHost = chainOptions?.forHost ?? { - id: jumpHosts[jumpHosts.length - 1]?.hostId ?? 0, - ip: "", - port: 22, - username: "", - }; + const host: PluginSshHost = chainOptions?.forHost + ? (redactHostSecrets( + chainOptions.forHost as unknown as Record, + ) as unknown as PluginSshHost) + : { + id: jumpHosts[jumpHosts.length - 1]?.hostId ?? 0, + ip: "", + port: 22, + username: "", + }; return { client: client as never, jumpClient: null, host, dispose }; }, @@ -253,7 +313,7 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { resolveHost: async (hostId, options) => { await checkSsh(true); - const userId = actingUser(hostId); + const userId = actingUser(); const { resolveHostById, resolveHostBySyncId } = await import("../hosts/host-resolver.js"); const resolved = options?.syncId @@ -263,15 +323,30 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { success: !!resolved, errorMessage: resolved ? undefined : "Host not found", }); - return (resolved as unknown as PluginSshHost | null) ?? null; + if (!resolved) return null; + // The secrets only go to a plugin that declared it reads them. + if (await hasCapability(pluginId, "credentials:read", declared)) { + await audit("credentials_read", describeHost(hostId), { + success: true, + }); + const full = resolved as unknown as SshConnectHost; + remember(full, full, userId); + return full as unknown as PluginSshHost; + } + return handOut(resolved as unknown as SshConnectHost, userId); }, + // The config it returns carries the password and private key, so it + // needs credentials:read on top of the connect pair. prepare: async (host, options) => { await checkSsh(true); + await assertCapability(pluginId, "credentials:read", declared); + const userId = userFor(host); + const target = toConnectHost(host, userId) as SshConnectHost; const { buildConnectConfig } = await import("../hosts/connect/build-connect-config.js"); - const built = await buildConnectConfig(host as SshConnectHost, { - userId: actingUser(host), + const built = await buildConnectConfig(target, { + userId, purpose: purposeOf(options), profile: profileOf(options), client: options.client as never, @@ -286,7 +361,6 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { built.outcome.status === "ready" ? undefined : built.outcome.message, }); const provider = built.provider; - const target = host as SshConnectHost; return { config: built.config, outcome: built.outcome, @@ -302,14 +376,25 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { }, openTransport: async (host, config, transportOptions) => { - await checkSsh(false); + await checkSsh(true); + const target = toConnectHost(host, userFor(host)) as SshConnectHost; const { openSshTransport } = await import("../hosts/connect/transport.js"); - const opened = await openSshTransport( - host as SshConnectHost, - config as MutableConnectConfig, - transportOptions, - ); + let opened: Awaited>; + try { + opened = await openSshTransport( + target, + config as MutableConnectConfig, + transportOptions, + ); + } catch (error) { + await audit("ssh_open_transport", describeHost(host), { + success: false, + errorMessage: error instanceof Error ? error.message : String(error), + }); + throw error; + } + await audit("ssh_open_transport", describeHost(host), { success: true }); if (opened.jumpClient) { const jumpClient = opened.jumpClient; const dispose = () => { @@ -324,8 +409,22 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { startInteraction: async (interaction, request) => { await checkSsh(true); - const userId = actingUser(request.hostId); + const userId = actingUser(); ensureCoreSshAuthProviders(); + const { PermissionManager } = + await import("../utils/permission-manager.js"); + const access = await PermissionManager.getInstance().canAccessHost( + userId, + request.hostId, + "connect", + ); + if (!access.hasAccess) { + await audit("ssh_start_interaction", describeHost(request.hostId), { + success: false, + errorMessage: "Host not found", + }); + throw new PluginSshInteractionError("Host not found"); + } const { createCurrentHostResolutionRepository } = await import("../database/repositories/factory.js"); const host = await createCurrentHostResolutionRepository().findHostById( @@ -367,7 +466,7 @@ export function createPluginSsh({ manifest, bag, audit }: Deps): PluginSsh { cancelInteraction: async (interaction, request) => { await checkSsh(false); - const userId = actingUser(request.hostId ?? 0); + const userId = actingUser(); ensureCoreSshAuthProviders(); for (const provider of listSshAuthProviders()) { if (provider.interaction !== interaction) continue; @@ -467,7 +566,7 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { field: string, ) => { if (!declared.includes("auth:provide")) { - throw new PluginCapabilityError(pluginId, "auth:provide"); + throw capabilityRefused(pluginId, "auth:provide"); } if (!list?.includes(id)) { throw new Error( @@ -477,6 +576,10 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { }; const granted = () => assertCapability(pluginId, "auth:provide", declared); + // ssh2 calls some hooks synchronously, so those answer from the grant + // cache and do nothing until the grant is confirmed. + const grantedNow = () => + hasCachedCapability(pluginId, "auth:provide", declared); const record = (action: string, id: string) => void audit(action, `${pluginId} registered ${id}`, { success: true }); @@ -499,8 +602,22 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { if (provider.onBanner) { const onBanner = provider.onBanner; wrapped.onBanner = (banner, host, env) => - onBanner(banner, host as PluginSshHost, env as never) as never; + grantedNow() + ? (onBanner(banner, host as PluginSshHost, env as never) as never) + : (undefined as never); } + if (provider.onAuthFailed) { + const onAuthFailed = provider.onAuthFailed; + wrapped.onAuthFailed = (host, env, context) => + grantedNow() + ? (onAuthFailed( + host as PluginSshHost, + env as never, + context as never, + ) as never) + : undefined; + } + warmPluginGrants(pluginId); ensureCoreSshAuthProviders(); const disposers = [registerSshAuthProvider(wrapped)]; if (provider.onKeyboardInteractive) { @@ -510,7 +627,7 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { id: `${pluginId}:${provider.type}`, pluginId, detect: (round, host) => - host.authType === provider.type + host.authType === provider.type && grantedNow() ? (detect(round, host as PluginSshHost) as never) : null, }), @@ -529,8 +646,9 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { handler.id, "keyboardInteractive", ); - // Synchronous hooks inside ssh2's callback, so only the declaration is - // checked, like ctx.http.router. Deactivate removes the handler. + // Synchronous hooks inside ssh2's callback, so they answer from the + // grant cache. Deactivate removes the handler. + warmPluginGrants(pluginId); const settingsFor = (host: SshConnectHost): Record => host.pluginSettings?.[pluginId] ?? {}; ensureCoreSshAuthProviders(); @@ -538,6 +656,7 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { id: handler.id, pluginId, detect: (round, host) => { + if (!grantedNow()) return null; const detected = handler.detect( round, host as PluginSshHost, @@ -551,6 +670,7 @@ export function createPluginAuth({ manifest, bag, audit }: Deps): PluginAuth { }, autoAnswerPasswords: handler.autoAnswerPasswords ? (host) => + grantedNow() && handler.autoAnswerPasswords!( host as PluginSshHost, settingsFor(host), diff --git a/src/backend/plugins/ctx.ts b/src/backend/plugins/ctx.ts index 020978cbb..55aaeb470 100644 --- a/src/backend/plugins/ctx.ts +++ b/src/backend/plugins/ctx.ts @@ -27,7 +27,11 @@ import * as serviceRegistry from "./service-registry.js"; import type { ServiceRegistration } from "./service-registry.js"; import * as secretRegistry from "./secret-registry.js"; import type { SecretRegistration } from "./secret-registry.js"; -import { assertCapability, hasCapability } from "./permissions.js"; +import { + assertCapability, + capabilityRefused, + hasCapability, +} from "./permissions.js"; import { getActor, runAsActor } from "./actor.js"; import { DisposableBag } from "./disposables.js"; import { @@ -40,7 +44,6 @@ import { resolvePermission } from "./rbac.js"; import * as pluginSettings from "./settings.js"; import type { PluginManifest } from "@termix/plugin-sdk/manifest"; import { - PluginCapabilityError, type PluginContext, type PluginModule, type PluginOpenIsolatedWindowRequest, @@ -110,8 +113,9 @@ interface AuditOptions { /** * Wraps a privileged function in the capability check and the audit line. * - * The check runs before the call and the audit after, so a denied call is - * still recorded: "tried and was refused" is exactly the thing worth seeing. + * The check runs before the call and the audit after. A denied call is still + * recorded, by assertCapability: "tried and was refused" is exactly the thing + * worth seeing. */ function guarded( manifest: PluginManifest, @@ -120,17 +124,13 @@ function guarded( options: AuditOptions, ): (...args: Args) => Promise { return async (...args: Args): Promise => { - const pluginId = manifest.id; - - try { - await assertCapability(pluginId, capability, manifest.capabilities); - } catch (error) { - await writeAudit(manifest, options, { - success: false, - errorMessage: error instanceof Error ? error.message : String(error), - }); - throw error; - } + // A refusal is audited inside assertCapability, under this action. + await assertCapability( + manifest.id, + capability, + manifest.capabilities, + options.action, + ); try { const result = await fn(...args); @@ -191,6 +191,26 @@ export function createPluginContext( await import("../database/repositories/factory.js"); return createCurrentPluginSettingsRepository(); }; + // A user named by the plugin instead of the ambient actor. Honoured, like + // ctx.asUser, and audited the same way. + const namedUser = ( + userId: string | undefined, + via: string, + ): string | undefined => { + if (!userId) return getActor(); + if (userId !== getActor()) { + void writeAudit( + manifest, + { + action: "as_user", + details: () => `${pluginId} acted as ${userId} through ${via}`, + }, + { success: true }, + ); + } + return userId; + }; + const secretOwner = (): string => { const actor = getActor(); if (!actor) { @@ -384,19 +404,12 @@ export function createPluginContext( ); const capabilitiesRequire = async (capability: string) => { - try { - await assertCapability(pluginId, capability, declared); - } catch (error) { - await writeAudit( - manifest, - { action: "capability_require", details: () => capability }, - { - success: false, - errorMessage: error instanceof Error ? error.message : String(error), - }, - ); - throw error; - } + await assertCapability( + pluginId, + capability, + declared, + "capability_require", + ); await writeAudit( manifest, { action: "capability_require", details: () => capability }, @@ -551,10 +564,11 @@ export function createPluginContext( service, pluginId, { - // A caller-supplied userId is only honoured when it matches the - // actor; otherwise the ambient actor wins. A plugin cannot widen - // its reach by naming someone else here. - resolveUserId: () => options?.userId ?? getActor(), + // Naming a user other than the actor is the same thing as + // ctx.asUser, so it gets the same audit line. + resolveUserId: () => + namedUser(options?.userId, `service ${service}`), + nameUser: (userId) => namedUser(userId, `service ${service}`), hasPermission: checkPermission, audit: (entry) => writeServiceAudit(entry), }, @@ -667,7 +681,8 @@ export function createPluginContext( getShared: (providerPluginId, key, options) => secretRegistry.readSharedSecret(manifest, providerPluginId, key, { - resolveUserId: () => options?.userId ?? getActor(), + resolveUserId: () => + namedUser(options?.userId, `shared secret ${providerPluginId}`), hasPermission: checkPermission, audit: (entry) => writeSecretAudit(entry), }), @@ -679,7 +694,7 @@ export function createPluginContext( // and router() has to be synchronous so a plugin can register routes // inline in activate. http.ts re-checks on every request. if (!declared.includes("network:serve")) { - throw new PluginCapabilityError(pluginId, "network:serve"); + throw capabilityRefused(pluginId, "network:serve"); } const router = createPluginRouter({ @@ -703,7 +718,7 @@ export function createPluginContext( ws: { route: (path, wsHandler, options) => { if (!declared.includes("network:serve")) { - throw new PluginCapabilityError(pluginId, "network:serve"); + throw capabilityRefused(pluginId, "network:serve"); } const dispose = registerPluginWsRoute( pluginId, @@ -717,7 +732,7 @@ export function createPluginContext( upgrade: (path, upgradeHandler, options) => { if (!declared.includes("network:serve")) { - throw new PluginCapabilityError(pluginId, "network:serve"); + throw capabilityRefused(pluginId, "network:serve"); } const dispose = registerPluginWsUpgrade( pluginId, diff --git a/src/backend/plugins/data.ts b/src/backend/plugins/data.ts index 6512bd1c4..91867c3f8 100644 --- a/src/backend/plugins/data.ts +++ b/src/backend/plugins/data.ts @@ -12,8 +12,8 @@ import { pluginLogger } from "../utils/logger.js"; import { resolveDatabaseDialect } from "../database/db/dialect.js"; import type { DatabaseDialect } from "../database/db/dialect.js"; import type { PluginTableDefinition } from "@termix/plugin-sdk/db"; -import { prefixedTableName } from "@termix/plugin-sdk/db"; -import { buildTable, dropTableSql } from "./table-builder.js"; +import { prefixedTableName, tablePrefix } from "@termix/plugin-sdk/db"; +import { buildTable } from "./table-builder.js"; import { applyPluginMigrations, readMigrations, @@ -88,10 +88,9 @@ async function execute(statements: string[]): Promise { /** * Builds the runner that applies migrations against the live database. * - * SQLite gets a transaction; the client-server engines do not, because MySQL - * commits implicitly on DDL so a "transaction" there would only be a promise - * the engine does not keep. A half-applied migration is recorded statement by - * statement either way, so a retry resumes rather than repeating. + * No transaction on any engine: MySQL commits implicitly on DDL, and each + * migration is recorded once it has run, so a retry after a failure starts + * at the migration that failed. */ async function createMigrationRunner( dialect: DatabaseDialect = resolveDatabaseDialect(), @@ -126,37 +125,59 @@ async function createMigrationRunner( export async function migratePlugin( pluginId: string, pluginDir: string, - dialect: DatabaseDialect = resolveDatabaseDialect(), + options: { bundled?: boolean; dialect?: DatabaseDialect } = {}, ): Promise { + const dialect = options.dialect ?? resolveDatabaseDialect(); if (readMigrations(pluginDir, dialect).length === 0) return []; const runner = await createMigrationRunner(dialect); - return applyPluginMigrations(pluginId, pluginDir, runner); + return applyPluginMigrations(pluginId, pluginDir, runner, { + bundled: options.bundled, + }); } /** * Removes everything a plugin owns. * - * Tables go in reverse registration order so a table referenced by a later one - * is dropped last. Disabling a plugin never calls this: data outlives being - * turned off, and only an explicit uninstall throws it away. + * Tables are found in the database's own catalog by the plugin's prefix, not + * from what it registered this boot, so a plugin that never activated since + * the restart still loses its tables. A longer prefix that belongs to another + * known plugin is left alone. Disabling a plugin never calls this: data + * outlives being turned off, and only an explicit uninstall throws it away. + * Role permissions stay, so a role keeps them if the plugin comes back. */ export async function removePluginData( pluginId: string, - dialect: DatabaseDialect = resolveDatabaseDialect(), + options: { knownPluginIds?: string[]; dialect?: DatabaseDialect } = {}, ): Promise<{ tables: string[]; kvKeys: number; migrations: number }> { - const definitions = listTables(pluginId); - const dropped: string[] = []; + const dialect = options.dialect ?? resolveDatabaseDialect(); + const tables = ownedTableNames( + pluginId, + await listDatabaseTables(dialect), + options.knownPluginIds ?? [], + ); - for (const definition of [...definitions].reverse()) { - await execute([dropTableSql(dialect, pluginId, definition)]); - dropped.push(prefixedTableName(pluginId, definition.name)); + // Registered order reversed first, so a referencing table goes before the + // one it points at; anything else after. + const registeredOrder = listTables(pluginId) + .map((definition) => prefixedTableName(pluginId, definition.name)) + .reverse(); + const ordered = [ + ...registeredOrder.filter((table) => tables.includes(table)), + ...tables.filter((table) => !registeredOrder.includes(table)), + ]; + + const dropped: string[] = []; + for (const table of ordered) { + await execute([dropTableByName(dialect, table)]); + dropped.push(table); } const { createCurrentPluginStorageRepository, createCurrentPluginMigrationRepository, createCurrentPluginPermissionGrantRepository, + createCurrentPluginSettingsRepository, } = await import("../database/repositories/factory.js"); const kvKeys = @@ -164,6 +185,8 @@ export async function removePluginData( const migrations = await createCurrentPluginMigrationRepository().deleteByPlugin(pluginId); await createCurrentPluginPermissionGrantRepository().deleteByPlugin(pluginId); + // Settings and the plugin's encrypted secrets go with it. + await createCurrentPluginSettingsRepository().deleteByPlugin(pluginId); forgetTables(pluginId); @@ -174,3 +197,53 @@ export async function removePluginData( return { tables: dropped, kvKeys, migrations }; } + +/** + * The tables in `all` that belong to this plugin: its prefix, minus any that + * sit under a longer prefix another plugin owns. + */ +export function ownedTableNames( + pluginId: string, + all: string[], + knownPluginIds: string[], +): string[] { + const prefix = tablePrefix(pluginId); + const longer = knownPluginIds + .filter((id) => id !== pluginId) + .map((id) => tablePrefix(id)) + .filter((other) => other.startsWith(prefix)); + return all.filter( + (table) => + table.startsWith(prefix) && + !longer.some((other) => table.startsWith(other)), + ); +} + +function dropTableByName(dialect: DatabaseDialect, table: string): string { + const quoted = dialect === "mysql" ? "`" + table + "`" : `"${table}"`; + return `DROP TABLE IF EXISTS ${quoted};`; +} + +async function listDatabaseTables(dialect: DatabaseDialect): Promise { + const { getDb } = await import("../database/db/index.js"); + const db = getDb() as unknown as { + all?: (query: unknown) => Promise>>; + execute?: (query: unknown) => Promise; + }; + if (dialect === "sqlite") { + const rows = await db.all!( + sql`SELECT name FROM sqlite_master WHERE type = 'table'`, + ); + return rows.map((row) => String(row.name)); + } + const query = + dialect === "postgres" + ? sql`SELECT table_name AS name FROM information_schema.tables WHERE table_schema = current_schema()` + : sql`SELECT table_name AS name FROM information_schema.tables WHERE table_schema = DATABASE()`; + const result = await db.execute!(query); + // node-postgres answers { rows }, mysql2 answers [rows, fields]. + const rows = ( + Array.isArray(result) ? result[0] : (result as { rows?: unknown[] }).rows + ) as Array>; + return rows.map((row) => String(row.name ?? row.NAME)); +} diff --git a/src/backend/plugins/http.ts b/src/backend/plugins/http.ts index 841a03e29..ef9ed2abd 100644 --- a/src/backend/plugins/http.ts +++ b/src/backend/plugins/http.ts @@ -38,11 +38,19 @@ const DEFAULT_BODY_LIMIT = "2mb"; /** Routers by plugin id, read by the dispatcher in plugin-api-routes.ts. */ const routers = new Map(); +/** Declared public paths by plugin id, for the admin plugin details. */ +const publicRoutes = new Map(); + +export function getPluginPublicHttpRoutes(pluginId: string): string[] { + return [...(publicRoutes.get(pluginId) ?? [])]; +} + export function getPluginRouter(pluginId: string): Router | undefined { return routers.get(pluginId); } export function unregisterPluginHttp(pluginId: string): void { + publicRoutes.delete(pluginId); if (!routers.delete(pluginId)) return; pluginLogger.info(`Unmounted /plugin-api/${pluginId}`, { operation: "plugin_http_unmount", @@ -63,10 +71,27 @@ export function setPluginEnabledCheck(check: IsEnabled): void { isPluginEnabled = check; } +/** + * Whether a plugin with this id is installed at all, running or not. A + * disabled plugin has no router or socket routes left, so this is what tells + * "disabled" (503) apart from "no such plugin" (404). + */ +let isInstalled: IsEnabled = () => false; + +export function setPluginInstalledCheck(check: IsEnabled): void { + isInstalled = check; +} + +export function isPluginInstalled(pluginId: string): boolean { + return isInstalled(pluginId); +} + /** Test seam, so one test's stub does not leak into the next. */ export function resetPluginHttp(): void { routers.clear(); + publicRoutes.clear(); isPluginEnabled = () => true; + isInstalled = () => false; } function normalizePublicPath(path: string): string { @@ -130,6 +155,7 @@ export function createPluginRouter({ normalizePublicPath(path), ); const publicPaths = declaredPublic.map((path) => compilePublicPath(path)); + publicRoutes.set(pluginId, declaredPublic); if (declaredPublic.length > 0) { // Audited once at registration: the set of unauthenticated routes a plugin @@ -225,7 +251,20 @@ export function createPluginRouter({ _next: NextFunction, ): void => { void _next; - reportError(error); + const status = clientErrorStatus(error); + const authenticated = !!(req as Request & { userId?: string }).userId; + // A bad body or a 4xx is the caller's fault, and an anonymous caller + // must never be able to spend the plugin's error budget: five junk + // requests to a public route would otherwise switch the plugin off. + if (status === null && authenticated) reportError(error); + if (status !== null) { + if (!res.headersSent) { + res.status(status).json({ error: "Invalid request" }); + } else { + res.end(); + } + return; + } pluginLogger.error( `Plugin ${pluginId} route ${req.method} ${req.path} failed`, error instanceof Error ? error : new Error(String(error)), @@ -248,6 +287,16 @@ export function createPluginRouter({ return inner; } +/** The status of an error that is the caller's fault, or null. */ +function clientErrorStatus(error: unknown): number | null { + const status = + (error as { status?: unknown; statusCode?: unknown } | null)?.status ?? + (error as { statusCode?: unknown } | null)?.statusCode; + return typeof status === "number" && status >= 400 && status < 500 + ? status + : null; +} + async function writePublicRouteAudit( manifest: PluginManifest, paths: string[], diff --git a/src/backend/plugins/index.ts b/src/backend/plugins/index.ts index 7092850ac..d6ec75b05 100644 --- a/src/backend/plugins/index.ts +++ b/src/backend/plugins/index.ts @@ -10,7 +10,11 @@ */ import { pluginLogger } from "../utils/logger.js"; -import { setPluginEnabledCheck, unregisterPluginHttp } from "./http.js"; +import { + setPluginEnabledCheck, + setPluginInstalledCheck, + unregisterPluginHttp, +} from "./http.js"; import { PluginLoader, type LoadedPlugin } from "./loader.js"; import type { PluginPermissionContribution } from "./manifest.js"; import { invalidatePluginPermissionCache } from "./permissions.js"; @@ -29,6 +33,7 @@ export function getPluginRuntime(): { loader: PluginLoader } { setPluginEnabledCheck( (pluginId) => loader?.get(pluginId)?.state === "active", ); + setPluginInstalledCheck((pluginId) => !!loader?.get(pluginId)); // Lets a host whose auth type belongs to a disabled plugin name it. setSshAuthTypeOwnerSource(() => (loader?.list() ?? []).flatMap((plugin) => @@ -256,6 +261,24 @@ async function registerPluginPermissions(plugin: LoadedPlugin): Promise { ].filter((id) => id !== plugin.id), ); + // The group is the plugin id, so a plugin named after a core group would + // register that group's permissions. The manifest validator refuses the + // id too; this holds for a manifest that skipped it. + if (coreGroups.has(plugin.id)) { + recordConflict({ + kind: "permission", + pluginId: plugin.id, + heldBy: "core", + name: plugin.id, + }); + pluginLogger.error( + `Ignoring every ${plugin.id} permission: its id is a core permission group`, + undefined, + { operation: "plugin_permissions" }, + ); + return; + } + const accepted = declared.filter((permission) => { const head = permission.name.split(".")[0]; if (coreGroups.has(head) || otherPluginIds.has(head)) { @@ -379,16 +402,18 @@ async function applyRoleDefaults( return !coveredBy(current as string[], permission); }); + if (missing.length > 0) { + await roleRepository.updateRole(role.id, { + permissions: JSON.stringify([...(current as string[]), ...missing]), + }); + } + // Only once the role really has them, so a failed update is retried + // on the next boot instead of being marked done. for (const permission of permissions) { recorded.push({ roleName, permission }); } - if (missing.length === 0) continue; - await roleRepository.updateRole(role.id, { - permissions: JSON.stringify([...(current as string[]), ...missing]), - }); - for (const memberId of await roleRepository.listRoleUserIds(role.id)) { PermissionManager.getInstance().invalidateUserPermissionCache(memberId); } diff --git a/src/backend/plugins/loader.ts b/src/backend/plugins/loader.ts index e728ad35b..55ac8e6d9 100644 --- a/src/backend/plugins/loader.ts +++ b/src/backend/plugins/loader.ts @@ -33,6 +33,8 @@ import { } from "./ctx.js"; import { resolveRequirements } from "./service-registry.js"; import { resolveSecretRequirements } from "./secret-registry.js"; +import { recordConflict } from "./conflicts.js"; +import { tablePrefix } from "@termix/plugin-sdk/db"; export type PluginState = | "loaded" @@ -119,6 +121,27 @@ export class PluginLoader { `Plugin ${manifest.id} backend entry ${entry} is missing`, ); } + // A symlink can point anywhere, so the real path has to stay inside too. + if (!isRealPathInside(dir, entry)) { + throw new Error( + `Plugin ${manifest.id} backend entry resolves outside the plugin directory`, + ); + } + + const overlap = [...this.plugins.values()].find((other) => + prefixesOverlap(other.id, manifest.id), + ); + if (overlap) { + recordConflict({ + kind: "id", + pluginId: manifest.id, + heldBy: overlap.id, + name: tablePrefix(manifest.id), + }); + throw new Error( + `Plugin id "${manifest.id}" overlaps "${overlap.id}": their table names would share the prefix ${tablePrefix(overlap.id)}`, + ); + } const plugin: LoadedPlugin = { id: manifest.id, @@ -144,12 +167,15 @@ export class PluginLoader { * dropping a folder with a bundled plugin's id into the data directory is visibly * refused instead of quietly ignored. * - * The check is on the parsed manifest id, not the directory name, because a - * directory can be named anything. + * The check is on the directory name, which load() then requires to equal + * the manifest id, and it covers bundled folders that failed to load too. */ async loadAll(): Promise { this.plugins.clear(); const loaded: LoadedPlugin[] = []; + // Every bundled folder name, loaded or not: a bundled plugin that fails + // to load must not leave its id free for a user plugin to take. + const bundledIds = new Set(); const roots: Array<{ root: string; source: PluginSource }> = [ { root: getBundledPluginsDir(), source: "bundled" }, @@ -160,14 +186,18 @@ export class PluginLoader { if (!fs.existsSync(root)) continue; const entries = await fs.promises.readdir(root, { withFileTypes: true }); + if (source === "bundled") { + for (const entry of entries) { + if (entry.isDirectory()) bundledIds.add(entry.name); + } + } for (const entry of entries) { if (!entry.isDirectory()) continue; const dir = path.join(root, entry.name); try { - const existing = this.plugins.get(entry.name); - if (existing && source === "user") { + if (source === "user" && bundledIds.has(entry.name)) { throw new Error( `a bundled plugin already uses the id "${entry.name}"; a plugin in the data directory cannot replace it`, ); @@ -343,7 +373,9 @@ export class PluginLoader { // than leave it half-running against a schema that is not there. A // throw here lands in the catch below, which fails this plugin only. const { migratePlugin } = await import("./data.js"); - const applied = await migratePlugin(plugin.id, plugin.dir); + const applied = await migratePlugin(plugin.id, plugin.dir, { + bundled: plugin.source === "bundled", + }); // A new table may be where core data is waiting to move. if (applied.length > 0) { const { runPluginDataMoves } = @@ -577,3 +609,28 @@ function pathToFileUrlString(filePath: string): string { const resolved = path.resolve(filePath).replace(/\\/g, "/"); return `file://${resolved.startsWith("/") ? "" : "/"}${resolved}`; } + +/** + * True when one id's table prefix starts with the other's: "foo" owns + * p_foo_*, which already covers every table "foo-bar" (p_foo_bar_*) creates. + */ +export function prefixesOverlap(a: string, b: string): boolean { + if (a === b) return false; + const left = tablePrefix(a); + const right = tablePrefix(b); + return left.startsWith(right) || right.startsWith(left); +} + +/** Whether `target` really lives under `root`, following symlinks. */ +export function isRealPathInside(root: string, target: string): boolean { + try { + const realRoot = fs.realpathSync(root); + const relative = path.relative(realRoot, fs.realpathSync(target)); + return ( + relative === "" || + (!relative.startsWith("..") && !path.isAbsolute(relative)) + ); + } catch { + return false; + } +} diff --git a/src/backend/plugins/migrations.ts b/src/backend/plugins/migrations.ts index 9f587e5d7..51f2dddee 100644 --- a/src/backend/plugins/migrations.ts +++ b/src/backend/plugins/migrations.ts @@ -18,8 +18,11 @@ import path from "node:path"; import { sql } from "drizzle-orm"; import { pluginLogger } from "../utils/logger.js"; import type { DatabaseDialect } from "../database/db/dialect.js"; -import { tablePrefix, LEGACY_TABLE_OWNERS } from "@termix/plugin-sdk/db"; -import { splitStatements } from "@termix/plugin-sdk/ddl"; +import { LEGACY_TABLE_OWNERS } from "@termix/plugin-sdk/db"; +import { + findUnownedTableWrites, + splitStatements, +} from "@termix/plugin-sdk/ddl"; // The splitter is shared with createTestDb in the SDK, so tests apply a // migration exactly the way this runner does. @@ -68,6 +71,15 @@ export function readMigrations( ): PluginMigration[] { const dir = migrationsDir(pluginDir, dialect); if (!fs.existsSync(dir)) return []; + // A symlinked folder or file could feed in SQL from anywhere on disk. + const realRoot = fs.realpathSync(pluginDir); + const inside = (target: string) => { + const relative = path.relative(realRoot, fs.realpathSync(target)); + return !relative.startsWith("..") && !path.isAbsolute(relative); + }; + if (!inside(dir)) { + throw new Error("The migrations folder resolves outside the plugin"); + } const migrations: PluginMigration[] = []; for (const file of fs.readdirSync(dir).sort()) { @@ -80,6 +92,9 @@ export function readMigrations( } continue; } + if (!inside(path.join(dir, file))) { + throw new Error(`Migration "${file}" resolves outside the plugin`); + } const contents = fs.readFileSync(path.join(dir, file), "utf8"); migrations.push({ id: file.replace(/\.sql$/, ""), @@ -95,41 +110,39 @@ export function readMigrations( } /** - * Tables a migration may create or alter. + * Checks that every table a migration writes to belongs to this plugin. * * A plugin owns the p__ namespace and nothing else. Enforced here as well * as in the CLI, because a plugin installed from a tarball never ran the CLI. - */ -const CREATE_OR_ALTER = - /\b(?:CREATE\s+(?:TEMP\s+|TEMPORARY\s+)?TABLE(?:\s+IF\s+NOT\s+EXISTS)?|ALTER\s+TABLE|DROP\s+TABLE(?:\s+IF\s+EXISTS)?|TRUNCATE\s+TABLE)\s+([`"']?)([a-zA-Z0-9_]+)\1/gi; - -/** - * Checks that every table a migration touches belongs to this plugin. - * - * A legacy table it is allowed to adopt is permitted too, because adoption - * renames it into the plugin's namespace in the same migration. + * A legacy core table is allowed only for the bundled plugin that adopts it: + * a user plugin with the same id gets no exemption. */ export function assertOwnedTables( pluginId: string, - statements: string[], + sql: string, + options: { bundled?: boolean } = {}, ): void { - const prefix = tablePrefix(pluginId); - - for (const statement of statements) { - CREATE_OR_ALTER.lastIndex = 0; - let match: RegExpExecArray | null; - while ((match = CREATE_OR_ALTER.exec(statement)) !== null) { - const table = match[2]; - if (table.startsWith(prefix)) continue; - if (LEGACY_TABLE_OWNERS[table] === pluginId) continue; - - const owner = LEGACY_TABLE_OWNERS[table]; - throw new Error( - owner - ? `Plugin "${pluginId}" may not touch "${table}": it belongs to the "${owner}" plugin.` - : `Plugin "${pluginId}" may only create or alter tables prefixed "${prefix}", not "${table}".`, - ); - } + const legacy = new Set( + options.bundled + ? Object.entries(LEGACY_TABLE_OWNERS) + .filter(([, owner]) => owner === pluginId) + .map(([table]) => table) + : [], + ); + const problems = findUnownedTableWrites(pluginId, sql, legacy).map( + (problem) => { + // Name the owner when the table is another plugin's legacy table. + const table = /writes to "([^"]+)"/.exec(problem)?.[1]; + const owner = table ? LEGACY_TABLE_OWNERS[table] : undefined; + return owner && owner !== pluginId + ? `may not touch "${table}": it belongs to the "${owner}" plugin` + : problem; + }, + ); + if (problems.length > 0) { + throw new Error( + `Plugin "${pluginId}" migration refused: ${problems.join("; ")}`, + ); } } @@ -158,6 +171,7 @@ export async function applyPluginMigrations( pluginId: string, pluginDir: string, runner: MigrationRunner, + options: { bundled?: boolean } = {}, ): Promise { const migrations = readMigrations(pluginDir, runner.dialect); if (migrations.length === 0) return []; @@ -185,8 +199,8 @@ export async function applyPluginMigrations( const run = async () => { for (const migration of pending) { + assertOwnedTables(pluginId, migration.sql, options); const statements = splitStatements(migration.sql); - assertOwnedTables(pluginId, statements); await runner.execute(statements); await runner.record(pluginId, migration); } diff --git a/src/backend/plugins/permissions.ts b/src/backend/plugins/permissions.ts index eacecf23c..00dab6ad4 100644 --- a/src/backend/plugins/permissions.ts +++ b/src/backend/plugins/permissions.ts @@ -10,10 +10,14 @@ * * The cache is invalidated explicitly on every grant change rather than by * TTL: a revoke that takes effect a minute later is not a revoke. + * + * Every refusal is audited here, so a guard added anywhere in the runtime + * leaves a trail without each call site remembering to write one. */ import { createCurrentPluginPermissionGrantRepository } from "../database/repositories/factory.js"; import { PluginCapabilityError } from "@termix/plugin-sdk/backend"; +import { getActor } from "./actor.js"; const cache = new Map>(); @@ -43,12 +47,81 @@ export async function hasCapability( return (await loadGrants(pluginId)).has(capability); } +/** + * For hooks ssh2 calls synchronously. Answers from the cache only and fails + * closed when the grants are not loaded yet, starting the load for next time. + */ +export function hasCachedCapability( + pluginId: string, + capability: string, + declared: readonly string[], +): boolean { + if (!declared.includes(capability)) return false; + const cached = cache.get(pluginId); + if (!cached) { + void loadGrants(pluginId).catch(() => {}); + return false; + } + return cached.has(capability); +} + +/** Loads a plugin's grants ahead of a synchronous hasCachedCapability. */ +export function warmPluginGrants(pluginId: string): void { + void loadGrants(pluginId).catch(() => {}); +} + +/** + * Throws PluginCapabilityError unless granted, after writing the refusal's + * audit line. `action` names the call ("kv_set"), so the line says what was + * refused and not only which capability was missing. + */ export async function assertCapability( pluginId: string, capability: string, declared: readonly string[], + action = "capability_refused", ): Promise { - if (!(await hasCapability(pluginId, capability, declared))) { - throw new PluginCapabilityError(pluginId, capability); + if (await hasCapability(pluginId, capability, declared)) return; + const error = new PluginCapabilityError(pluginId, capability); + await auditRefusal(pluginId, capability, action, error); + throw error; +} + +/** + * The error for a capability a sync call never declared, with its audit + * line written in the background. + */ +export function capabilityRefused( + pluginId: string, + capability: string, + action = "capability_refused", +): PluginCapabilityError { + const error = new PluginCapabilityError(pluginId, capability); + void auditRefusal(pluginId, capability, action, error); + return error; +} + +async function auditRefusal( + pluginId: string, + capability: string, + action: string, + error: Error, +): Promise { + try { + const { logAudit } = await import("../utils/audit-logger.js"); + await logAudit({ + // Attribution comes from the runtime, never from the plugin. + userId: getActor() ?? "system", + username: `plugin:${pluginId}`, + action: `plugin_${action}`, + resourceType: "plugin", + resourceId: pluginId, + resourceName: pluginId, + details: `refused: ${capability}`, + success: false, + errorMessage: error.message, + }); + } catch { + // Auditing must never change what the caller sees. } } diff --git a/src/backend/plugins/rbac.ts b/src/backend/plugins/rbac.ts index f5617931f..c31cc51a8 100644 --- a/src/backend/plugins/rbac.ts +++ b/src/backend/plugins/rbac.ts @@ -41,6 +41,15 @@ export function resolvePermission( if (!head) return permission; if (head === manifest.id) return permission; + // A name this plugin declares is its own, even if another plugin's id + // happens to match its first segment. + if ( + (manifest.contributes?.permissions ?? []).some( + (declared) => declared.name === permission, + ) + ) { + return `${manifest.id}.${permission}`; + } if (coreGroups().has(head)) return permission; // Another plugin's registered group, which is the cross-plugin check. diff --git a/src/backend/plugins/secret-registry.ts b/src/backend/plugins/secret-registry.ts index e324c807b..f7320ad1f 100644 --- a/src/backend/plugins/secret-registry.ts +++ b/src/backend/plugins/secret-registry.ts @@ -106,7 +106,7 @@ export class PluginSecretActorError extends Error { constructor(pluginId: string, key: string) { super( `A read of "${pluginId}:${key}" has no acting user. Pass one with ` + - `ctx.secrets.forUser(userId).getShared(...).`, + `ctx.secrets.getShared(pluginId, key, { userId }) or ctx.asUser.`, ); this.name = "PluginSecretActorError"; } diff --git a/src/backend/plugins/service-registry.ts b/src/backend/plugins/service-registry.ts index 921b61dba..edb45d446 100644 --- a/src/backend/plugins/service-registry.ts +++ b/src/backend/plugins/service-registry.ts @@ -309,6 +309,8 @@ function recordUnsatisfied( export interface ServiceCallContext { /** Resolves the user this call acts as. */ resolveUserId: () => string | undefined; + /** Called when a handle is bound to a named user with asUser(). */ + nameUser?: (userId: string) => void; /** Checks a role permission for a user. */ hasPermission: (userId: string, permission: string) => Promise; /** @@ -359,7 +361,13 @@ export function createServiceHandle( createServiceHandle( service, consumerPluginId, - { ...context, resolveUserId: () => userId }, + { + ...context, + resolveUserId: () => { + context.nameUser?.(userId); + return userId; + }, + }, name, ); } diff --git a/src/backend/plugins/ws.ts b/src/backend/plugins/ws.ts index 58c1a6497..7dde054c6 100644 --- a/src/backend/plugins/ws.ts +++ b/src/backend/plugins/ws.ts @@ -30,6 +30,7 @@ import { pluginLogger } from "../utils/logger.js"; import { recordConflict } from "./conflicts.js"; import { extractWebSocketToken } from "../utils/ws-auth.js"; import { runAsActor } from "./actor.js"; +import { isPluginInstalled } from "./http.js"; const PLUGIN_WS_PREFIX = "/plugin-ws/"; @@ -138,8 +139,13 @@ async function handlePluginUpgrade( const route = routes.get(key(parsed.pluginId, parsed.path)); if (!route) { // Claimed by the prefix but nothing is listening: answering here is more - // honest than leaving the socket hanging for a plugin that is disabled. - reject(socket, 404, "Not Found"); + // honest than leaving the socket hanging. A disabled plugin has no routes + // left, so an installed id means "off", not "missing". + if (isPluginInstalled(parsed.pluginId)) { + reject(socket, 503, "Service Unavailable"); + } else { + reject(socket, 404, "Not Found"); + } return true; } @@ -164,11 +170,14 @@ async function handlePluginUpgrade( userId = (await verifyToken(request)) ?? ""; } + // A guest on a public socket has no actor at all, the same as a public + // HTTP route, so nothing downstream mistakes a placeholder for a user. + const asCaller = (fn: () => void) => + userId ? runAsActor(userId, "request", fn) : fn(); + if (route.rawHandler) { const rawHandler = route.rawHandler; - runAsActor(userId || "anonymous", "request", () => - rawHandler(request, socket, head, userId), - ); + asCaller(() => rawHandler(request, socket, head, userId)); return true; } @@ -195,7 +204,7 @@ async function handlePluginUpgrade( ws.close(1011, "No handler"); return; } - runAsActor(userId || "anonymous", "request", () => { + asCaller(() => { void Promise.resolve(handler({ ...connection, socket: ws })).catch( (error) => { pluginLogger.error( @@ -260,6 +269,7 @@ export function registerPluginWsRoute( declared, handler, }); + if (options.public) auditPublicSocket(pluginId, normalized); servers.set(routeKey, new WebSocketServer({ noServer: true })); pluginLogger.info(`Mounted /plugin-ws/${pluginId}${normalized}`, { @@ -292,6 +302,7 @@ export function registerPluginWsUpgrade( declared, rawHandler: handler, }); + if (options.public) auditPublicSocket(pluginId, normalized); pluginLogger.info(`Mounted raw upgrade /plugin-ws/${pluginId}${normalized}`, { operation: "plugin_ws_mount", @@ -331,6 +342,35 @@ export function getRegisteredWsRoutes(): string[] { return [...routes.keys()]; } +/** The socket paths a plugin serves without core's login check. */ +export function getPluginPublicWsRoutes(pluginId: string): string[] { + return [...routes.values()] + .filter((route) => route.pluginId === pluginId && route.options.public) + .map((route) => route.path); +} + +/** Same record the HTTP side writes for its public routes. */ +function auditPublicSocket(pluginId: string, path: string): void { + pluginLogger.warn( + `Plugin ${pluginId} serves /plugin-ws/${pluginId}${path} without core authentication`, + { operation: "plugin_ws_public" }, + ); + void import("../utils/audit-logger.js") + .then(({ logAudit }) => + logAudit({ + userId: "system", + username: `plugin:${pluginId}`, + action: "plugin_ws_public_route", + resourceType: "plugin", + resourceId: pluginId, + resourceName: pluginId, + details: `unauthenticated socket: ${path}`, + success: true, + }), + ) + .catch(() => {}); +} + /** Test seam. */ export function resetPluginWebSockets(): void { for (const routeKey of [...routes.keys()]) { diff --git a/src/backend/tests/auth/auth-test-helpers.ts b/src/backend/tests/auth/auth-test-helpers.ts index ca6139baa..8681d7757 100644 --- a/src/backend/tests/auth/auth-test-helpers.ts +++ b/src/backend/tests/auth/auth-test-helpers.ts @@ -67,6 +67,8 @@ export function fakeFactory(state: AuthState) { delete: async (id: string) => state.users.delete(id), }), createCurrentUserAuthRepository: () => ({ + listIdentitiesForUser: async (userId: string) => + state.identities.filter((row) => row.userId === userId), findIdentity: async (providerId: string, subject: string) => state.identities.find( (row) => row.providerId === providerId && row.subject === subject, diff --git a/src/backend/tests/database/routes/host-plugin-settings.test.ts b/src/backend/tests/database/routes/host-plugin-settings.test.ts index 4496bf718..6f6203376 100644 --- a/src/backend/tests/database/routes/host-plugin-settings.test.ts +++ b/src/backend/tests/database/routes/host-plugin-settings.test.ts @@ -271,20 +271,25 @@ describe("withHostPluginSettings", () => { }); describe("writeHostPluginSettings", () => { - it("JSON-stringifies each value under the plugin's own namespace", async () => { - await writeHostPluginSettings("docker", 7, { + it("writes each value through the settings service, under the plugin's own namespace", async () => { + await writeHostPluginSettings(DOCKER, 7, { enableDocker: true, - socketPath: null, + socketPath: "/var/run/docker.sock", }); expect(setCalls).toEqual([ ["docker", "host", "7", "enableDocker", "true"], - ["docker", "host", "7", "socketPath", "null"], + ["docker", "host", "7", "socketPath", '"/var/run/docker.sock"'], ]); }); + it("refuses a key the manifest does not declare", async () => { + await writeHostPluginSettings(DOCKER, 7, { injected: "x" }); + expect(setCalls).toEqual([]); + }); + it("skips a field whose value is undefined", async () => { - await writeHostPluginSettings("docker", 7, { + await writeHostPluginSettings(DOCKER, 7, { enableDocker: true, socketPath: undefined, }); diff --git a/src/backend/tests/database/routes/plugin-legacy-paths.test.ts b/src/backend/tests/database/routes/plugin-legacy-paths.test.ts index 595a30f0c..aadcabc85 100644 --- a/src/backend/tests/database/routes/plugin-legacy-paths.test.ts +++ b/src/backend/tests/database/routes/plugin-legacy-paths.test.ts @@ -10,9 +10,11 @@ import request from "supertest"; import { describe, expect, it, vi } from "vitest"; const routers = vi.hoisted(() => new Map()); +const installed = vi.hoisted(() => new Set()); vi.mock("../../../plugins/http.js", () => ({ getPluginRouter: (id: string) => routers.get(id), + isPluginInstalled: (id: string) => installed.has(id), })); const { mountPluginLegacyPaths } = @@ -64,11 +66,18 @@ describe("mountPluginLegacyPaths", () => { }); }); - it("answers 404 while the plugin serves no router", async () => { + it("answers 404 for a plugin that is not installed", async () => { const app = appWith([{ id: "gone", legacyPaths: ["/gone/hook"] }]); const response = await request(app).post("/gone/hook/x"); expect(response.status).toBe(404); }); + + it("answers 503 for an installed plugin that serves no router", async () => { + installed.add("paused"); + const app = appWith([{ id: "paused", legacyPaths: ["/paused/hook"] }]); + const response = await request(app).post("/paused/hook/x"); + expect(response.status).toBe(503); + }); }); describe("legacy redirects", () => { diff --git a/src/backend/tests/plugins/ctx-hosts.test.ts b/src/backend/tests/plugins/ctx-hosts.test.ts index fdb11a1e0..1bf9b936b 100644 --- a/src/backend/tests/plugins/ctx-hosts.test.ts +++ b/src/backend/tests/plugins/ctx-hosts.test.ts @@ -47,8 +47,13 @@ vi.mock("../../plugins/permissions.js", async () => { throw new PluginCapabilityError(pluginId, capability); } }, + capabilityRefused: (pluginId: string, capability: string) => + new PluginCapabilityError(pluginId, capability), }; }); + +/** The access check behind trackSession and reportLogin is async. */ +const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); vi.mock("../../plugins/actor.js", () => ({ getActor: () => h.actor })); vi.mock("../../utils/permission-manager.js", () => ({ PermissionManager: { @@ -455,7 +460,7 @@ describe("ctx.hosts.delete", () => { }); describe("ctx.hosts session tracking and activity", () => { - it("counts a live session through core and releases it", () => { + it("counts a live session through core and releases it", async () => { h.sessions = []; h.released = []; const hosts = createPluginHosts({ @@ -463,6 +468,7 @@ describe("ctx.hosts session tracking and activity", () => { audit: vi.fn(async () => {}), }); const release = hosts.trackSession(4); + await settle(); expect(h.sessions).toEqual([4]); release(); expect(h.released).toEqual([4]); @@ -498,11 +504,11 @@ describe("ctx.hosts.status", () => { manifest: manifest(["hosts:read"]), audit: vi.fn(async () => {}), }); - expect(hosts.status.get(4)).toEqual({ + await expect(hosts.status.get(4)).resolves.toEqual({ status: "reachable", lastChecked: "t", }); - expect(hosts.status.get(5)).toBeNull(); + await expect(hosts.status.get(5)).resolves.toBeNull(); await expect(hosts.status.check(5)).resolves.toEqual({ status: "offline", lastChecked: "5", @@ -514,7 +520,9 @@ describe("ctx.hosts.status", () => { manifest: manifest([]), audit: vi.fn(async () => {}), }); - expect(() => hosts.status.get(4)).toThrow(PluginCapabilityError); + await expect(hosts.status.get(4)).rejects.toBeInstanceOf( + PluginCapabilityError, + ); expect(() => hosts.status.reportLogin(4, { ok: true })).toThrow( PluginCapabilityError, ); @@ -526,7 +534,7 @@ describe("ctx.hosts.status", () => { ); }); - it("passes login reports on to core", () => { + it("passes login reports on to core", async () => { h.statusReports = []; h.granted = new Set(["hosts:read"]); const hosts = createPluginHosts({ @@ -534,6 +542,7 @@ describe("ctx.hosts.status", () => { audit: vi.fn(async () => {}), }); hosts.status.reportLogin(4, { ok: false, hostKeyChanged: true }); + await settle(); expect(h.statusReports).toEqual([ { hostId: 4, ok: false, hostKeyChanged: true }, ]); diff --git a/src/backend/tests/plugins/ctx-ssh-auth.test.ts b/src/backend/tests/plugins/ctx-ssh-auth.test.ts index a9f8dcef5..143de5008 100644 --- a/src/backend/tests/plugins/ctx-ssh-auth.test.ts +++ b/src/backend/tests/plugins/ctx-ssh-auth.test.ts @@ -31,6 +31,19 @@ vi.mock("../../plugins/permissions.js", async () => { throw new PluginCapabilityError(pluginId, capability); } }, + hasCapability: async ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => declared.includes(capability) && h.granted.has(capability), + hasCachedCapability: ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => declared.includes(capability) && h.granted.has(capability), + warmPluginGrants: () => {}, + capabilityRefused: (pluginId: string, capability: string) => + new PluginCapabilityError(pluginId, capability), }; }); vi.mock("../../plugins/actor.js", () => ({ @@ -171,7 +184,7 @@ describe("ctx.ssh", () => { }); }); - it("runs background work on a resolved host as its owner", async () => { + it("never believes a host object's own userId", async () => { h.actor = undefined; h.granted = new Set(["ssh:connect", "credentials:use"]); const ssh = createPluginSsh({ @@ -179,15 +192,21 @@ describe("ctx.ssh", () => { bag: new DisposableBag("fixture"), audit: vi.fn(async () => {}), }); - await ssh.connect({ + const forged = { id: 3, ip: "h", port: 22, username: "u", userId: "owner", - }); - expect(h.connects[0].options).toMatchObject({ userId: "owner" }); + }; + await expect(ssh.connect(forged)).rejects.toThrow(/acting user/); await expect(ssh.connect(3)).rejects.toThrow(/acting user/); + expect(h.connects).toEqual([]); + + h.actor = "caller"; + await ssh.connect(forged); + expect(h.connects[0].options).toMatchObject({ userId: "caller" }); + expect(h.connects[0].target).toMatchObject({ userId: "caller" }); }); it("passes a given stream through to the pipeline, gated like any connect", async () => { @@ -343,7 +362,37 @@ describe("ctx.auth", () => { expect(getSshAuthProvider("corp")).toBeUndefined(); }); + it("never calls a keyboard-interactive handler whose grant is missing", async () => { + h.granted = new Set(); + const bag = new DisposableBag("fixture"); + const auth = createPluginAuth({ + manifest: manifest(["auth:provide"], { keyboardInteractive: ["gate"] }), + bag, + audit: vi.fn(async () => {}), + }); + const detect = vi.fn(() => null); + const autoAnswerPasswords = vi.fn(() => true); + auth.registerKeyboardInteractiveHandler({ + id: "gate", + label: "Gate", + detect, + autoAnswerPasswords, + }); + classifyKeyboardInteractive( + { + name: "Gate sign-in", + instructions: "", + prompts: [{ prompt: "Password:", echo: false }], + }, + { id: 1, ip: "10.0.0.1", port: 22, username: "root", password: "pw" }, + ); + expect(detect).not.toHaveBeenCalled(); + expect(autoAnswerPasswords).not.toHaveBeenCalled(); + await bag.disposeAll(); + }); + it("registers a keyboard-interactive handler that sees only its own host settings", async () => { + h.granted = new Set(["auth:provide"]); const bag = new DisposableBag("fixture"); const auth = createPluginAuth({ manifest: manifest(["auth:provide"], { keyboardInteractive: ["gate"] }), diff --git a/src/backend/tests/plugins/ctx-ssh-interactive.test.ts b/src/backend/tests/plugins/ctx-ssh-interactive.test.ts index f5e5f6c46..38f7c7635 100644 --- a/src/backend/tests/plugins/ctx-ssh-interactive.test.ts +++ b/src/backend/tests/plugins/ctx-ssh-interactive.test.ts @@ -29,9 +29,31 @@ vi.mock("../../plugins/permissions.js", async () => { throw new PluginCapabilityError(pluginId, capability); } }, + hasCapability: async ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => declared.includes(capability) && h.granted.has(capability), + hasCachedCapability: ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => declared.includes(capability) && h.granted.has(capability), + warmPluginGrants: () => {}, + capabilityRefused: (pluginId: string, capability: string) => + new PluginCapabilityError(pluginId, capability), }; }); vi.mock("../../plugins/actor.js", () => ({ getActor: () => h.actor })); +vi.mock("../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ + canAccessHost: async (_userId: string, hostId: number) => ({ + hasAccess: hostId !== 7, + }), + }), + }, +})); vi.mock("../../hosts/connect/core-providers.js", () => ({ ensureCoreSshAuthProviders: () => {}, })); @@ -90,7 +112,7 @@ const { registerSshAuthProvider, resetSshAuthRegistryForTests } = const { PluginCapabilityError, PluginSshInteractionError } = await import("@termix/plugin-sdk/backend"); -const ALL = ["ssh:connect", "credentials:use"]; +const ALL = ["ssh:connect", "credentials:use", "credentials:read"]; function sshWith(capabilities = ALL) { const audit = vi.fn(async () => {}); @@ -131,6 +153,14 @@ describe("ctx.ssh.resolveHost", () => { }); }); + it("hides the secrets from a plugin without credentials:read", async () => { + h.granted = new Set(["ssh:connect", "credentials:use"]); + const { ssh } = sshWith(["ssh:connect", "credentials:use"]); + const host = await ssh.resolveHost(3); + expect(host).toMatchObject({ id: 3, ip: "10.0.0.1" }); + expect(host).not.toHaveProperty("password"); + }); + it("answers null for a host this user cannot reach", async () => { const { ssh } = sshWith(); expect(await ssh.resolveHost(404)).toBeNull(); @@ -147,6 +177,18 @@ describe("ctx.ssh.resolveHost", () => { }); describe("ctx.ssh.prepare", () => { + it("needs credentials:read, because its config carries the secrets", async () => { + h.granted = new Set(["ssh:connect", "credentials:use"]); + const { ssh } = sshWith(["ssh:connect", "credentials:use"]); + await expect( + ssh.prepare( + { id: 1, ip: "10.0.0.1", port: 22, username: "root" }, + { client: {} }, + ), + ).rejects.toBeInstanceOf(PluginCapabilityError); + expect(h.built).toEqual([]); + }); + it("passes the terminal purpose, interactivity and host key socket through", async () => { const { ssh } = sshWith(); const socket = { send: vi.fn() }; @@ -218,6 +260,27 @@ describe("ctx.ssh browser sign-in", () => { ); }); + it("will not start one for a host the actor cannot reach", async () => { + const started = vi.fn(async () => {}); + registerSshAuthProvider({ + type: "fixture-auth", + pluginId: "core", + labelKey: "k", + interaction: "fixture", + startInteraction: started, + prepare: async () => ({ status: "ready" }), + }); + const { ssh } = sshWith(); + await expect( + ssh.startInteraction("fixture", { + hostId: 7, + socket: null, + requestOrigin: "", + }), + ).rejects.toBeInstanceOf(PluginSshInteractionError); + expect(started).not.toHaveBeenCalled(); + }); + it("says why when nothing can start it", async () => { const { ssh } = sshWith(); await expect( diff --git a/src/backend/tests/plugins/migrations.test.ts b/src/backend/tests/plugins/migrations.test.ts index f28810f3f..79afde684 100644 --- a/src/backend/tests/plugins/migrations.test.ts +++ b/src/backend/tests/plugins/migrations.test.ts @@ -146,39 +146,51 @@ describe("splitStatements", () => { describe("assertOwnedTables", () => { it("allows a table in the plugin's own namespace", () => { expect(() => - assertOwnedTables("my-plugin", [ + assertOwnedTables( + "my-plugin", "CREATE TABLE IF NOT EXISTS p_my_plugin_thing (id integer)", - ]), + ), ).not.toThrow(); }); it("rejects a table outside the plugin's namespace", () => { expect(() => - assertOwnedTables("my-plugin", ["CREATE TABLE users (id integer)"]), - ).toThrow(/may only create or alter tables prefixed "p_my_plugin_"/); + assertOwnedTables("my-plugin", "CREATE TABLE users (id integer)"), + ).toThrow(/"users", which is not prefixed "p_my_plugin_"/); }); it("rejects altering a core table", () => { expect(() => - assertOwnedTables("my-plugin", [ - "ALTER TABLE ssh_data ADD COLUMN x text", - ]), + assertOwnedTables("my-plugin", "ALTER TABLE ssh_data ADD COLUMN x text"), ).toThrow(/p_my_plugin_/); }); - it("allows the one plugin that may adopt a legacy table", () => { + it("allows the bundled plugin that adopts a legacy table", () => { expect(() => - assertOwnedTables("fleets", [ + assertOwnedTables( + "fleets", 'ALTER TABLE "fleets" RENAME TO "p_fleets_fleets"', - ]), + { bundled: true }, + ), ).not.toThrow(); }); + it("gives a user plugin with that id no legacy exemption", () => { + expect(() => + assertOwnedTables( + "fleets", + 'ALTER TABLE "fleets" RENAME TO "p_fleets_fleets"', + ), + ).toThrow(/"fleets", which is not prefixed/); + }); + it("names the owner when another plugin tries to adopt its table", () => { expect(() => - assertOwnedTables("workspaces", [ + assertOwnedTables( + "workspaces", 'ALTER TABLE "fleets" RENAME TO "p_workspaces_fleets"', - ]), + { bundled: true }, + ), ).toThrow(/belongs to the "fleets" plugin/); }); diff --git a/src/backend/tests/plugins/plugin-data.test.ts b/src/backend/tests/plugins/plugin-data.test.ts index 38d35c935..b868cd637 100644 --- a/src/backend/tests/plugins/plugin-data.test.ts +++ b/src/backend/tests/plugins/plugin-data.test.ts @@ -285,7 +285,9 @@ describe("adopting a legacy core table", () => { "0001_adopt.sql", adoptLegacyTableSql("sqlite", "fleets", "p_fleets_fleets"), ); - await applyPluginMigrations("fleets", root, createRunner()); + await applyPluginMigrations("fleets", root, createRunner(), { + bundled: true, + }); expect(tableExists("fleets")).toBe(false); const rows = sqlite @@ -319,7 +321,9 @@ describe("adopting a legacy core table", () => { ); await expect( - applyPluginMigrations("workspaces", root, createRunner()), + applyPluginMigrations("workspaces", root, createRunner(), { + bundled: true, + }), ).rejects.toThrow(/belongs to the "fleets" plugin/); // Refused before execution, so the legacy table is still there. diff --git a/src/backend/tests/plugins/security/actor.test.ts b/src/backend/tests/plugins/security/actor.test.ts new file mode 100644 index 000000000..8644fe33e --- /dev/null +++ b/src/backend/tests/plugins/security/actor.test.ts @@ -0,0 +1,215 @@ +/** + * The actor cannot be spoofed. It is set by a request or by ctx.asUser, and + * every other way of naming a user (a service handle, a shared secret read) + * is audited the same way asUser is. RBAC inside ctx.hosts and ctx.ssh is + * applied for that actor, so a user without access to a host cannot reach it + * through a plugin. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const auditEntries: Array> = []; +const h = vi.hoisted(() => ({ + reachable: new Set([1]), + resolvedFor: [] as Array<{ id: number; userId: string }>, +})); + +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: async (entry: Record) => { + auditEntries.push(entry); + }, +})); +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentPluginPermissionGrantRepository: () => ({ + listByPlugin: async () => + ["hosts:read", "ssh:connect", "credentials:use"].map((capability) => ({ + capability, + })), + }), + createCurrentHostResolutionRepository: () => ({ + findHostOwnerId: async () => "owner", + findHostById: async (id: number) => ({ + id, + userId: "owner", + name: "box", + ip: "10.0.0.1", + port: 22, + username: "root", + tags: null, + folder: null, + authType: "password", + }), + }), +})); +vi.mock("../../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ + hasPermission: async () => true, + canAccessHost: async (_userId: string, hostId: number) => ({ + hasAccess: h.reachable.has(hostId), + isOwner: false, + isShared: h.reachable.has(hostId), + }), + }), + }, +})); +vi.mock("../../../hosts/host-resolver.js", () => ({ + resolveHostById: async (id: number, userId: string) => { + h.resolvedFor.push({ id, userId }); + return h.reachable.has(id) ? { id, userId: "owner", ip: "10.0.0.1" } : null; + }, + resolveHostBySyncId: async () => null, +})); + +const { createPluginContext, createPluginHandle } = + await import("../../../plugins/ctx.js"); +const { invalidatePluginPermissionCache } = + await import("../../../plugins/permissions.js"); +const { runAsActor, getActor } = await import("../../../plugins/actor.js"); +const { clearServiceRegistry } = + await import("../../../plugins/service-registry.js"); + +function contextFor(pluginId: string, extra: Record = {}) { + const handle = createPluginHandle(pluginId, { activate: () => {} }); + return createPluginContext( + { + id: pluginId, + name: pluginId, + version: "1.0.0", + description: "", + author: { name: "test" }, + license: "MIT", + category: "Productivity", + engine: { termix: ">=2.9.0", api: "1" }, + capabilities: ["hosts:read", "ssh:connect", "credentials:use"], + ...extra, + } as never, + handle, + ); +} + +const asUserLines = () => + auditEntries.filter((entry) => entry.action === "plugin_as_user"); + +beforeEach(() => { + auditEntries.length = 0; + h.resolvedFor = []; + h.reachable = new Set([1]); + invalidatePluginPermissionCache(); +}); +afterEach(() => clearServiceRegistry()); + +describe("naming a user is audited like ctx.asUser", () => { + function provider() { + const ctx = contextFor("provider", { + provides: [ + { service: "sample.echo", version: "1.0.0", permission: "x.use" }, + ], + }); + ctx.services.provide("sample.echo", { + whoAmI: async () => getActor(), + }); + return ctx; + } + + it("ctx.asUser writes an as_user line", async () => { + const ctx = contextFor("caller"); + await ctx.asUser("victim", async () => undefined); + expect(asUserLines()).toHaveLength(1); + expect(asUserLines()[0]).toMatchObject({ resourceId: "caller" }); + }); + + it("services.get({ userId }) for someone else writes one too", async () => { + provider(); + const caller = contextFor("caller"); + const handle = caller.services.get<{ whoAmI: () => Promise }>( + "sample.echo", + { userId: "victim" }, + ); + await runAsActor("alice", "request", async () => { + expect(await handle.whoAmI()).toBe("victim"); + }); + expect(asUserLines()).toHaveLength(1); + expect(String(asUserLines()[0].details)).toMatch(/victim/); + }); + + it("a handle's asUser(userId) writes one too", async () => { + provider(); + const caller = contextFor("caller"); + const handle = caller.services.get<{ + asUser: (userId: string) => { whoAmI: () => Promise }; + }>("sample.echo"); + await runAsActor("alice", "request", async () => { + expect(await handle.asUser("victim").whoAmI()).toBe("victim"); + }); + expect(asUserLines()).toHaveLength(1); + }); + + it("naming the actor itself is not a switch and writes nothing", async () => { + provider(); + const caller = contextFor("caller"); + const handle = caller.services.get<{ whoAmI: () => Promise }>( + "sample.echo", + { userId: "alice" }, + ); + await runAsActor("alice", "request", () => handle.whoAmI()); + expect(asUserLines()).toHaveLength(0); + }); + + it("the provider sees the user the permission was checked for", async () => { + provider(); + const caller = contextFor("caller"); + const handle = caller.services.get<{ whoAmI: () => Promise }>( + "sample.echo", + ); + await runAsActor("alice", "request", async () => { + expect(await handle.whoAmI()).toBe("alice"); + }); + }); +}); + +describe("a user cannot reach a host they have no access to", () => { + it("ctx.hosts answers nothing for it", async () => { + const ctx = contextFor("reader"); + await runAsActor("alice", "request", async () => { + expect(await ctx.hosts.get(99)).toBeNull(); + expect(await ctx.hosts.status.get(99)).toBeNull(); + expect(await ctx.hosts.status.check(99)).toBeNull(); + const access = await ctx.hosts.checkAccess(99, "connect"); + expect(access.hasAccess).toBe(false); + }); + }); + + it("ctx.ssh resolves it as the actor and gets nothing", async () => { + const ctx = contextFor("reader"); + await runAsActor("alice", "request", async () => { + expect(await ctx.ssh.resolveHost(99)).toBeNull(); + }); + expect(h.resolvedFor).toEqual([{ id: 99, userId: "alice" }]); + }); + + it("ctx.ssh refuses everything without an actor, whatever the host says", async () => { + const ctx = contextFor("reader"); + await expect(ctx.ssh.resolveHost(1)).rejects.toThrow(/acting user/); + await expect( + ctx.ssh.connect({ + id: 1, + userId: "owner", + ip: "10.0.0.1", + port: 22, + username: "root", + }), + ).rejects.toThrow(/acting user/); + await expect( + ctx.ssh.jumpChain([{ hostId: 1 }], { + forHost: { + id: 1, + userId: "owner", + ip: "10.0.0.1", + port: 22, + username: "root", + }, + }), + ).rejects.toThrow(/acting user/); + }); +}); diff --git a/src/backend/tests/plugins/security/assets.test.ts b/src/backend/tests/plugins/security/assets.test.ts new file mode 100644 index 000000000..e912104e8 --- /dev/null +++ b/src/backend/tests/plugins/security/assets.test.ts @@ -0,0 +1,177 @@ +/** + * /plugin-assets serves only a running plugin's browser files: never a path + * outside its folder (however it is encoded, or through a symlink), never its + * backend or migrations, and always with a fixed content type and nosniff. + */ + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import type { AddressInfo } from "node:net"; +import type { Server } from "node:http"; +import express from "express"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + createPluginAssetsRouter, + resolvePluginAsset, +} from "../../../plugins/assets.js"; + +let root = ""; +let server: Server; +let baseUrl = ""; +const plugins = new Map>(); + +function write(relative: string, content: string) { + const file = path.join(root, relative); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, content); +} + +/** Creates a link, or returns false where the OS will not let us. */ +function link(target: string, at: string, type: "file" | "junction"): boolean { + try { + fs.symlinkSync(target, at, type); + return true; + } catch { + return false; + } +} + +let fileLinks = false; +let dirLinks = false; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), "plugin-assets-sec-")); + write("gizmo/dist/frontend.js", "export function activate() {}"); + write("gizmo/dist/backend.js", "secret()"); + write("gizmo/dist/migrations/snapshot.json", "{}"); + write("gizmo/locales/en.json", "{}"); + write("secret.js", "outside()"); + write("secrets/keys.json", "{}"); + + // A plugin whose manifest puts its frontend at the plugin root, so the + // backend and migrations sit in the served folder. + write("flat/frontend.js", "export function activate() {}"); + write("flat/server/main.js", "secret()"); + write( + "flat/migrations/sqlite/0001_init.sql", + "CREATE TABLE p_flat_x (id int);", + ); + + fileLinks = link( + path.join(root, "secret.js"), + path.join(root, "gizmo/dist/escape.js"), + "file", + ); + dirLinks = link( + path.join(root, "secrets"), + path.join(root, "gizmo/dist/linked"), + "junction", + ); + + plugins.set("gizmo", { + id: "gizmo", + dir: path.join(root, "gizmo"), + manifest: { id: "gizmo", version: "1.0.0" }, + state: "active", + }); + plugins.set("flat", { + id: "flat", + dir: path.join(root, "flat"), + manifest: { + id: "flat", + version: "1.0.0", + frontend: "frontend.js", + backend: "server/main.js", + }, + state: "active", + }); + plugins.set("stopped", { + id: "stopped", + dir: path.join(root, "gizmo"), + manifest: { id: "stopped", version: "1.0.0" }, + state: "disabled", + }); + + const app = express(); + app.use( + "/plugin-assets", + createPluginAssetsRouter((id) => plugins.get(id) as never), + ); + server = await new Promise((resolve) => { + const created = app.listen(0, "127.0.0.1", () => resolve(created)); + }); + baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + fs.rmSync(root, { recursive: true, force: true }); +}); + +const get = (url: string) => fetch(`${baseUrl}/plugin-assets/${url}`); + +describe("path traversal", () => { + it.each([ + "gizmo/../secret.js", + "gizmo/..%2fsecret.js", + "gizmo/%2e%2e/secret.js", + "gizmo/%2e%2e%2f%2e%2e%2fsecret.js", + "gizmo/..%5csecret.js", + "gizmo/%2e%2e%5c%2e%2e%5csecret.js", + "gizmo/frontend.js%00.js", + "gizmo/locales/..%2f..%2fsecret.js", + "gizmo/%2fetc%2fpasswd.js", + ])("refuses %s", async (url) => { + const response = await get(url); + expect(response.status).toBe(404); + expect(await response.text()).not.toContain("outside()"); + }); + + it("refuses a symlinked file that points outside the plugin", () => { + if (!fileLinks) return; + expect( + resolvePluginAsset(plugins.get("gizmo") as never, "escape.js"), + ).toBeNull(); + }); + + it("refuses a file inside a symlinked folder that points outside", () => { + if (!dirLinks) return; + expect( + resolvePluginAsset(plugins.get("gizmo") as never, "linked/keys.json"), + ).toBeNull(); + }); +}); + +describe("server-only files", () => { + it("never serves the backend entry, wherever the manifest put it", async () => { + expect((await get("gizmo/backend.js")).status).toBe(404); + expect((await get("flat/server/main.js")).status).toBe(404); + }); + + it("never serves anything under migrations", async () => { + expect((await get("gizmo/migrations/snapshot.json")).status).toBe(404); + }); +}); + +describe("a stopped plugin", () => { + it("serves nothing, so its bundle cannot be loaded around the shell", async () => { + expect((await get("stopped/frontend.js")).status).toBe(404); + expect((await get("gizmo/frontend.js")).status).toBe(200); + }); +}); + +describe("headers", () => { + it("sends a fixed content type and nosniff", async () => { + const script = await get("gizmo/frontend.js"); + expect(script.headers.get("content-type")).toMatch(/^text\/javascript/); + expect(script.headers.get("x-content-type-options")).toBe("nosniff"); + + const json = await get("gizmo/locales/en.json"); + expect(json.headers.get("content-type")).toMatch(/^application\/json/); + expect(json.headers.get("x-content-type-options")).toBe("nosniff"); + + const missing = await get("gizmo/nothing.js"); + expect(missing.headers.get("x-content-type-options")).toBe("nosniff"); + }); +}); diff --git a/src/backend/tests/plugins/security/auth.test.ts b/src/backend/tests/plugins/security/auth.test.ts new file mode 100644 index 000000000..17f6dc0b3 --- /dev/null +++ b/src/backend/tests/plugins/security/auth.test.ts @@ -0,0 +1,349 @@ +/** + * Auth: second factors fail closed, the public login list leaks nothing, + * external sign-in cannot claim someone else's account or username, and the + * lockout guard keeps password login on when nothing else can sign anyone in. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import bcrypt from "bcryptjs"; +import { + createAuthState, + fakeAuthManager, + fakeRequest, + type AuthState, +} from "../../auth/auth-test-helpers.js"; + +const h = vi.hoisted(() => ({ + state: null as unknown as AuthState, + manager: null as unknown as ReturnType< + typeof import("../../auth/auth-test-helpers.js").fakeAuthManager + >, + trustedProxy: false, +})); + +vi.mock("../../../database/repositories/factory.js", async () => { + const helpers = await import("../../auth/auth-test-helpers.js"); + const names = Object.keys(helpers.fakeFactory(helpers.createAuthState())); + return Object.fromEntries( + names.map((name) => [ + name, + (...args: unknown[]) => + ( + helpers.fakeFactory(h.state) as Record< + string, + (...a: unknown[]) => unknown + > + )[name](...args), + ]), + ); +}); +vi.mock("../../../utils/auth-manager.js", () => ({ + AuthManager: { getInstance: () => h.manager }, +})); +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: async (entry: Record) => { + h.state.audits.push(entry); + }, + getRequestMeta: () => ({ ipAddress: "10.0.0.1", userAgent: "test" }), +})); +vi.mock("../../../hosts/internal-events.js", () => ({ + emitInternalEvent: vi.fn(), +})); +vi.mock("../../../utils/logger.js", () => { + const log = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + debug: vi.fn(), + }; + return { + authLogger: log, + databaseLogger: log, + sshLogger: log, + logger: log, + systemLogger: log, + }; +}); +vi.mock("../../../utils/database-save-trigger.js", () => ({ + DatabaseSaveTrigger: { forceSave: vi.fn(async () => {}) }, +})); +vi.mock("../../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ invalidateUserPermissionCache: vi.fn() }), + }, +})); +vi.mock("../../../utils/trusted-proxy-auth.js", () => ({ + isTrustedProxyAuthEnabled: () => h.trustedProxy, +})); +vi.mock("../../../utils/shared-host-secrets-manager.js", () => ({ + SharedHostSecretsManager: { + getInstance: () => ({ snapshotForUserRoles: async () => {} }), + }, +})); +vi.mock("../../../utils/shared-credential-secrets-manager.js", () => ({ + SharedCredentialSecretsManager: { + getInstance: () => ({ snapshotForUserRoles: async () => {} }), + }, +})); + +const { runLogin, resetPendingLoginsForTests } = + await import("../../../auth/login-pipeline.js"); +const { verifyPasswordLogin } = + await import("../../../auth/builtin-login-methods.js"); +const { getPasswordLoginStatus } = await import("../../../auth/core-auth.js"); +const { registerLoginMethod, registerSecondFactor } = + await import("../../../auth/registry.js"); +const { listPublicLoginMethods } = + await import("../../../database/routes/auth-routes.js"); +const { loginRateLimiter } = + await import("../../../utils/login-rate-limiter.js"); + +const PASSWORD = "correct-horse"; + +function addUser(overrides: Record = {}) { + const user = { + id: "u1", + username: "alice", + passwordHash: bcrypt.hashSync(PASSWORD, 4), + isAdmin: false, + isOidc: false, + ...overrides, + }; + h.state.users.set(user.id as string, user as never); + return user; +} + +async function passwordIdentity() { + const req = fakeRequest({ body: { username: "alice", password: PASSWORD } }); + return { req, identity: await verifyPasswordLogin(req as never) }; +} + +const disposers: Array<() => void> = []; + +beforeEach(() => { + h.state = createAuthState(); + h.manager = fakeAuthManager(h.state); + h.trustedProxy = false; + resetPendingLoginsForTests(); + loginRateLimiter.resetAttempts("10.0.0.1", "alice"); + delete process.env.ALLOW_PASSWORD_LOGIN; +}); + +afterEach(() => { + while (disposers.length) disposers.pop()?.(); +}); + +describe("second factors fail closed", () => { + it("refuses the login when a factor cannot say whether the user is enrolled", async () => { + addUser(); + disposers.push( + registerSecondFactor({ + id: "flaky", + pluginId: "fixture", + labelKey: "k", + isEnrolled: async () => { + throw new Error("database is locked"); + }, + verify: async () => true, + }), + ); + const { req, identity } = await passwordIdentity(); + await expect( + runLogin(req as never, identity, { + methodId: "password", + rememberMe: false, + }), + ).rejects.toMatchObject({ status: 403 }); + expect(h.manager.generateJWTToken).not.toHaveBeenCalled(); + }); +}); + +describe("the public login list", () => { + it("names methods and enabled instances, and nothing else", async () => { + disposers.push( + registerLoginMethod({ + id: "corp-sso", + pluginId: "fixture", + labelKey: "k", + kind: "redirect", + external: true, + describe: async () => + [ + { + id: "1", + label: "Corp", + enabled: true, + clientSecret: "shh", + issuerUrl: "https://idp.internal", + }, + { id: "2", label: "Old", enabled: false }, + ] as never, + }), + ); + const methods = await listPublicLoginMethods(); + const sso = methods.find((method) => method.id === "corp-sso"); + expect(sso?.instances).toEqual([{ id: "1", label: "Corp" }]); + expect(JSON.stringify(methods)).not.toMatch(/shh|idp\.internal/); + }); +}); + +describe("external sign-in cannot claim another account", () => { + const external = { + kind: "external" as const, + provider: "3", + subject: "sub-1", + email: null, + name: "Alice", + legacyIdentifier: "shared-id", + ssoProviderId: 3, + }; + + beforeEach(() => { + disposers.push( + registerLoginMethod({ + id: "oidc", + pluginId: "sso", + labelKey: "k", + kind: "redirect", + external: true, + }), + ); + h.state.settings.set("oidc_auto_provision", "true"); + }); + + async function signIn(identity: Record = external) { + return runLogin(fakeRequest() as never, identity as never, { + methodId: "oidc", + rememberMe: false, + }); + } + + function linkedTo(): string | undefined { + return h.state.identities.find((row) => row.subject === "sub-1")?.userId; + } + + it("never links a local password account by its old identifier", async () => { + addUser({ id: "local", oidcIdentifier: "shared-id", isOidc: false }); + await signIn(); + expect(linkedTo()).not.toBe("local"); + }); + + it("never links an account that already has an identity", async () => { + addUser({ + id: "taken", + username: "bob", + passwordHash: "", + isOidc: true, + oidcIdentifier: "shared-id", + }); + h.state.identities.push({ + id: 1, + userId: "taken", + providerId: "9", + subject: "bob", + email: null, + }); + await signIn(); + expect(linkedTo()).not.toBe("taken"); + }); + + it("never links an account from a different provider", async () => { + addUser({ + id: "other", + username: "bob", + passwordHash: "", + isOidc: true, + oidcIdentifier: "shared-id", + ssoProviderId: 7, + }); + await signIn(); + expect(linkedTo()).not.toBe("other"); + }); + + it("still links a genuine 2.8 account on its first sign-in", async () => { + addUser({ + id: "legacy", + username: "Alice", + passwordHash: "", + isOidc: true, + oidcIdentifier: "shared-id", + ssoProviderId: 3, + }); + await signIn(); + expect(linkedTo()).toBe("legacy"); + }); + + it("never gives a new account a local user's username", async () => { + addUser({ id: "admin-local", username: "admin" }); + await signIn({ ...external, name: "admin", legacyIdentifier: undefined }); + const created = [...h.state.users.values()].find( + (user) => user.id !== "admin-local", + ); + expect(created?.username).not.toBe("admin"); + expect(created?.username).toMatch(/^admin-/); + }); + + it("never renames an existing account onto a local user's username", async () => { + addUser({ id: "admin-local", username: "admin" }); + addUser({ + id: "sso-user", + username: "carol", + passwordHash: "", + isOidc: true, + }); + h.state.identities.push({ + id: 1, + userId: "sso-user", + providerId: "3", + subject: "sub-1", + email: null, + }); + await signIn({ ...external, name: "admin" }); + expect(h.state.users.get("sso-user")?.username).toBe("carol"); + }); +}); + +describe("the lockout guard", () => { + it("keeps password login on when it is off and nothing else can sign in", async () => { + process.env.ALLOW_PASSWORD_LOGIN = "false"; + await expect(getPasswordLoginStatus()).resolves.toEqual({ + allowed: true, + forced: true, + }); + }); + + it("lets it go off once another method has an enabled instance", async () => { + process.env.ALLOW_PASSWORD_LOGIN = "false"; + disposers.push( + registerLoginMethod({ + id: "corp-sso", + pluginId: "fixture", + labelKey: "k", + kind: "redirect", + describe: async () => [{ id: "1", label: "Corp", enabled: true }], + }), + ); + await expect(getPasswordLoginStatus()).resolves.toEqual({ + allowed: false, + forced: false, + }); + }); + + it("does not count a method whose instances are all off", async () => { + process.env.ALLOW_PASSWORD_LOGIN = "false"; + disposers.push( + registerLoginMethod({ + id: "corp-sso", + pluginId: "fixture", + labelKey: "k", + kind: "redirect", + describe: async () => [{ id: "1", label: "Corp", enabled: false }], + }), + ); + await expect(getPasswordLoginStatus()).resolves.toMatchObject({ + allowed: true, + forced: true, + }); + }); +}); diff --git a/src/backend/tests/plugins/security/credential-shapes.test.ts b/src/backend/tests/plugins/security/credential-shapes.test.ts new file mode 100644 index 000000000..c991c2788 --- /dev/null +++ b/src/backend/tests/plugins/security/credential-shapes.test.ts @@ -0,0 +1,254 @@ +/** + * No ctx method hands a plugin plaintext credential material unless the + * plugin holds credentials:read. Every return value is scanned for secret + * keys, nested ones included, so a new field on the host row that carries a + * secret shows up here. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const SECRET = "s3cret-value"; + +const h = vi.hoisted(() => ({ + granted: new Set(), + actor: "user-1" as string | undefined, + connected: [] as unknown[], +})); + +function secretHost(id: number): Record { + return { + id, + userId: "user-1", + name: "box", + ip: "10.0.0.1", + port: 22, + username: "root", + authType: "password", + tags: null, + folder: null, + password: "s3cret-value", + key: "s3cret-value", + keyPassword: "s3cret-value", + sudoPassword: "s3cret-value", + socks5Password: "s3cret-value", + rdpPassword: "s3cret-value", + vncPassword: "s3cret-value", + telnetPassword: "s3cret-value", + autostartPassword: "s3cret-value", + autostartKey: "s3cret-value", + autostartKeyPassword: "s3cret-value", + terminalConfig: { sudoPassword: "s3cret-value", fontSize: 14 }, + socks5ProxyChain: [{ host: "p", password: "s3cret-value" }], + }; +} + +vi.mock("../../../plugins/permissions.js", async () => { + const { PluginCapabilityError } = await import("@termix/plugin-sdk/backend"); + const granted = (capability: string, declared: readonly string[]) => + declared.includes(capability) && h.granted.has(capability); + return { + assertCapability: async ( + pluginId: string, + capability: string, + declared: readonly string[], + ) => { + if (!granted(capability, declared)) { + throw new PluginCapabilityError(pluginId, capability); + } + }, + hasCapability: async ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => granted(capability, declared), + hasCachedCapability: ( + _pluginId: string, + capability: string, + declared: readonly string[], + ) => granted(capability, declared), + warmPluginGrants: () => {}, + capabilityRefused: (pluginId: string, capability: string) => + new PluginCapabilityError(pluginId, capability), + }; +}); +vi.mock("../../../plugins/actor.js", () => ({ + getActor: () => h.actor, + getActorSessionId: () => undefined, +})); +vi.mock("../../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ + canAccessHost: async () => ({ hasAccess: true, isOwner: true }), + hasPermission: async () => true, + }), + }, +})); +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentHostRepository: () => ({ + createEncryptedForUser: async () => secretHost(1), + updateEncryptedForUser: async () => secretHost(1), + listDecryptedByUserId: async () => [secretHost(1), secretHost(2)], + }), + createCurrentHostResolutionRepository: () => ({ + findHostsByUserId: async () => [secretHost(1)], + listHostRowsForAccessList: async () => [secretHost(2)], + findHostOwnerId: async () => "user-1", + findHostById: async (id: number) => secretHost(id), + }), + createCurrentRoleRepository: () => ({ listUserRoleIds: async () => [] }), + createCurrentRbacAccessRepository: () => ({ + listVisibleHostAccessEntries: async () => [], + }), +})); +vi.mock("../../../database/routes/host-plugin-settings.js", () => ({ + applyPluginHostImportSettings: async () => {}, +})); +vi.mock("../../../hosts/host-resolver.js", () => ({ + resolveHostById: async (id: number) => secretHost(id), + resolveHostBySyncId: async () => secretHost(9), +})); +vi.mock("../../../hosts/connect/connect-host.js", () => ({ + connectHost: async (target: unknown) => { + h.connected.push(target); + return { + client: { once: () => {} }, + jumpClient: null, + host: typeof target === "number" ? secretHost(target) : target, + dispose: () => {}, + }; + }, + resolveConnectHost: async (target: unknown) => + typeof target === "number" ? secretHost(target) : target, +})); + +const { createPluginHosts, redactHostSecrets, HOST_SECRET_FIELDS } = + await import("../../../plugins/ctx-hosts.js"); +const { createPluginSsh } = await import("../../../plugins/ctx-ssh-auth.js"); +const { DisposableBag } = await import("../../../plugins/disposables.js"); + +function manifest(capabilities: string[]) { + return { + id: "fixture", + name: "Fixture", + version: "1.0.0", + capabilities, + } as never; +} + +/** Every path in `value` that holds the secret. */ +function leaks(value: unknown, path = "$"): string[] { + if (value === SECRET) return [path]; + if (Array.isArray(value)) { + return value.flatMap((entry, index) => leaks(entry, `${path}[${index}]`)); + } + if (value && typeof value === "object") { + return Object.entries(value).flatMap(([key, entry]) => + leaks(entry, `${path}.${key}`), + ); + } + return []; +} + +const audit = vi.fn(async () => {}); + +beforeEach(() => { + h.actor = "user-1"; + h.granted = new Set(); + h.connected = []; + audit.mockClear(); +}); + +describe("redactHostSecrets", () => { + it("removes every secret field, nested ones too", () => { + const redacted = redactHostSecrets(secretHost(1)); + expect(leaks(redacted)).toEqual([]); + for (const field of HOST_SECRET_FIELDS) { + expect(redacted).not.toHaveProperty(field); + } + expect(redacted).toMatchObject({ id: 1, ip: "10.0.0.1" }); + expect(redacted.terminalConfig).toEqual({ fontSize: 14 }); + }); +}); + +describe("ctx.hosts never returns secrets", () => { + const all = ["hosts:read", "hosts:write"]; + + it("list, get, create, update and listOwned", async () => { + h.granted = new Set(all); + const hosts = createPluginHosts({ manifest: manifest(all), audit }); + const results = { + list: await hosts.list(), + get: await hosts.get(1), + create: await hosts.create({ + name: "n", + ip: "i", + port: 22, + username: "u", + authType: "password", + }), + update: await hosts.update(1, { name: "m" }), + listOwned: await hosts.listOwned(), + }; + expect(leaks(results)).toEqual([]); + }); + + it("audits listOwned, the widest read it has", async () => { + h.granted = new Set(all); + const hosts = createPluginHosts({ manifest: manifest(all), audit }); + await hosts.listOwned(); + expect(audit).toHaveBeenCalledWith("hosts_list_owned", "2 host(s)", { + success: true, + }); + }); +}); + +describe("ctx.ssh hands secrets only to credentials:read", () => { + const connectPair = ["ssh:connect", "credentials:use"]; + + it("connect() returns the host without its secrets", async () => { + h.granted = new Set(connectPair); + const ssh = createPluginSsh({ + manifest: manifest(connectPair), + bag: new DisposableBag("fixture"), + audit, + }); + const connection = await ssh.connect(1); + expect(leaks(connection.host)).toEqual([]); + }); + + it("resolveHost() redacts without credentials:read", async () => { + h.granted = new Set(connectPair); + const ssh = createPluginSsh({ + manifest: manifest(connectPair), + bag: new DisposableBag("fixture"), + audit, + }); + expect(leaks(await ssh.resolveHost(1))).toEqual([]); + }); + + it("resolveHost() gives the full host with credentials:read, and audits it", async () => { + const withRead = [...connectPair, "credentials:read"]; + h.granted = new Set(withRead); + const ssh = createPluginSsh({ + manifest: manifest(withRead), + bag: new DisposableBag("fixture"), + audit, + }); + expect(leaks(await ssh.resolveHost(1)).length).toBeGreaterThan(0); + expect(audit).toHaveBeenCalledWith("credentials_read", "host 1", { + success: true, + }); + }); + + it("a redacted host handed back still connects with the real secrets", async () => { + h.granted = new Set(connectPair); + const ssh = createPluginSsh({ + manifest: manifest(connectPair), + bag: new DisposableBag("fixture"), + audit, + }); + const redacted = await ssh.resolveHost(1); + await ssh.connect(redacted!); + expect(h.connected[0]).toMatchObject({ password: SECRET }); + }); +}); diff --git a/src/backend/tests/plugins/security/ctx-guards.test.ts b/src/backend/tests/plugins/security/ctx-guards.test.ts new file mode 100644 index 000000000..2696d80a2 --- /dev/null +++ b/src/backend/tests/plugins/security/ctx-guards.test.ts @@ -0,0 +1,183 @@ +/** + * Every privileged ctx method checks its capability and leaves an audit line. + * + * This walks the whole ctx object rather than listing methods by hand, so a + * member added later without a guard fails here until it is either guarded + * or added to UNGATED with a reason. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const auditEntries: Array> = []; + +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentPluginPermissionGrantRepository: () => ({ + listByPlugin: async () => [], + }), +})); + +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: async (entry: Record) => { + auditEntries.push(entry); + }, +})); + +const { createPluginContext, createPluginHandle } = + await import("../../../plugins/ctx.js"); +const { invalidatePluginPermissionCache } = + await import("../../../plugins/permissions.js"); +const { runAsActor } = await import("../../../plugins/actor.js"); +const { PluginCapabilityError } = await import("@termix/plugin-sdk/backend"); + +const PLUGIN_ID = "walker"; +const ACTOR = "user-walker"; + +/** + * Members that need no capability, and why. Anything not listed here must + * refuse a plugin that declared and was granted nothing. + */ +const UNGATED: Record = { + "log.debug": "writes to core's log under the plugin's own tag", + "log.info": "writes to core's log under the plugin's own tag", + "log.warn": "writes to core's log under the plugin's own tag", + "log.error": "writes to core's log under the plugin's own tag", + "events.emit": "namespace check: plugin..* only without events:core", + "events.on": "namespace check: plugin.* only without events:core", + "sync.registerEntity": "adds the plugin's own table to remote sync", + "sync.recordTombstone": "records a delete of the plugin's own row", + "registry.provide": "the plugin's own registry key", + "registry.consume": "reads what another plugin chose to publish", + "registry.revoke": "removes the plugin's own registry key", + "services.provide": "only services declared in manifest provides", + "services.get": "each call is checked against the service's permission", + "services.providers": "names only", + "secrets.offer": "only secrets declared in manifest providesSecret", + "secrets.withdraw": "the plugin's own offer", + "secrets.getShared": "each read is checked against the secret's permission", + "settings.get": "the plugin's own admin settings", + "settings.set": "the plugin's own admin settings", + "settings.getUser": "the plugin's own user settings", + "settings.setUser": "the plugin's own user settings", + "settings.getHost": "the plugin's own host settings", + "settings.setHost": "the plugin's own host settings", + "settings.getAll": "the plugin's own settings", + "settings.onChange": "the plugin's own settings", + "settings.onValidate": "the plugin's own settings", + "rbac.has": "answers for the actor only", + "rbac.hasFor": "a yes/no about a role permission, grants nothing", + "rbac.require": "route middleware for the plugin's own permissions", + "capabilities.has": "a yes/no about the plugin's own grants", + "disposables.add": "the plugin's own cleanup", + "schedule.every": "the plugin's own timers", + "schedule.after": "the plugin's own timers", + "audit.record": "writes under the runtime's actor, never a plugin value", + asUser: "always audited as plugin_as_user", + currentActor: "reads the actor, cannot set it", + "desktop.available": "a yes/no about the environment", + "http.baseUrl": "reads the request's public origin", + "ssh.poolKey": "a string built from a host the plugin already holds", + "ssh.dropPooled": "closes only this plugin's own pooled connections", + "ssh.classifyKeyboardInteractive": "pure classification of a prompt", + "ssh.autoResponses": "pure mapping of prompts to answers", + "ssh.requiresSecret": "a yes/no about an auth type", + "ssh.supportsBackground": "a yes/no about an auth type", +}; + +function walk( + value: unknown, + path: string[] = [], + out: Array<{ path: string; fn: (...args: unknown[]) => unknown }> = [], +) { + if (!value || typeof value !== "object") return out; + for (const [key, member] of Object.entries(value)) { + if (path.length === 0 && (key === "manifest" || key === "pluginId")) { + continue; + } + const at = [...path, key]; + if (typeof member === "function") { + out.push({ path: at.join("."), fn: member as never }); + } + if (member && typeof member === "object") walk(member, at, out); + } + return out; +} + +/** Args that get a call past argument checks and into the guard. */ +const PLACEHOLDER_ARGS = [ + 1, + { id: 1, ip: "10.0.0.1", port: 22, username: "u", type: "x" }, + { purpose: "background", pool: "p" }, + () => undefined, +]; + +function contextWithNothing() { + const handle = createPluginHandle(PLUGIN_ID, { activate: () => {} }); + return createPluginContext( + { + id: PLUGIN_ID, + name: "Walker", + version: "1.0.0", + description: "", + author: { name: "test" }, + license: "MIT", + category: "Productivity", + engine: { termix: ">=2.9.0", api: "1" }, + capabilities: [], + } as never, + handle, + ); +} + +beforeEach(() => { + auditEntries.length = 0; + invalidatePluginPermissionCache(); +}); + +describe("every privileged ctx member is guarded", () => { + const members = walk(contextWithNothing()); + + it("finds the members it walks", () => { + expect(members.length).toBeGreaterThan(60); + for (const name of Object.keys(UNGATED)) { + expect( + members.some((member) => member.path === name), + `UNGATED lists ${name}, which ctx no longer has`, + ).toBe(true); + } + }); + + const gated = members.filter((member) => !(member.path in UNGATED)); + + it.each(gated.map((member) => [member.path]))( + "%s refuses a plugin with no grants and audits it", + async (path) => { + const ctx = contextWithNothing(); + const member = walk(ctx).find((entry) => entry.path === path)!; + + let caught: unknown; + await runAsActor(ACTOR, "request", async () => { + try { + await member.fn(...PLACEHOLDER_ARGS); + } catch (error) { + caught = error; + } + }); + // Sync refusals write their audit line in the background. + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect( + caught, + `${path} ran without a capability. Guard it, or add it to UNGATED with a reason.`, + ).toBeInstanceOf(PluginCapabilityError); + expect( + auditEntries.some( + (entry) => + entry.resourceId === PLUGIN_ID && + entry.userId === ACTOR && + entry.success === false, + ), + `${path} refused without an audit line naming the plugin and actor`, + ).toBe(true); + }, + ); +}); diff --git a/src/backend/tests/plugins/security/data.test.ts b/src/backend/tests/plugins/security/data.test.ts new file mode 100644 index 000000000..da47adbe1 --- /dev/null +++ b/src/backend/tests/plugins/security/data.test.ts @@ -0,0 +1,211 @@ +/** + * Data: a plugin migration can only write tables under its own prefix, the + * CLI and the runtime share one checker, a symlink cannot feed in SQL from + * elsewhere, and removing a plugin's data drops only that plugin's tables. + */ + +import Database from "better-sqlite3"; +import { drizzle } from "drizzle-orm/better-sqlite3"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { findUnownedTableWrites } from "@termix/plugin-sdk/ddl"; +import { LEGACY_TABLE_OWNERS } from "@termix/plugin-sdk/db"; + +const state = vi.hoisted(() => ({ + db: null as unknown, + deleted: [] as string[], +})); + +vi.mock("../../../utils/logger.js", () => { + const log = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + return { pluginLogger: log }; +}); +vi.mock("../../../database/db/index.js", () => ({ + getDb: () => state.db, +})); +vi.mock("../../../database/db/dialect.js", () => ({ + resolveDatabaseDialect: () => "sqlite", +})); +vi.mock("../../../database/repositories/factory.js", () => { + const remove = (what: string) => ({ + deleteByPlugin: async (pluginId: string) => { + state.deleted.push(`${what}:${pluginId}`); + return 0; + }, + }); + return { + createCurrentPluginStorageRepository: () => remove("kv"), + createCurrentPluginMigrationRepository: () => remove("migrations"), + createCurrentPluginPermissionGrantRepository: () => remove("grants"), + createCurrentPluginSettingsRepository: () => remove("settings"), + }; +}); + +const { assertOwnedTables, readMigrations } = + await import("../../../plugins/migrations.js"); +const { removePluginData, ownedTableNames } = + await import("../../../plugins/data.js"); + +describe("the migration checker", () => { + it.each([ + ["a comment between keyword and name", "DROP TABLE/**/users"], + ["a comment with spaces", "DROP TABLE /* x */ users"], + ["bracket quoting", "DROP TABLE [users]"], + ["double quoting", 'DROP TABLE "users"'], + ["back quoting", "DROP TABLE `users`"], + ["a second name in a list", "DROP TABLE p_foo_a, users"], + ["a rename out of the namespace", "ALTER TABLE p_foo_a RENAME TO users"], + ["a MySQL rename in", "RENAME TABLE users TO p_foo_users"], + ["an insert", "INSERT INTO users (id) VALUES ('x')"], + ["an update", "UPDATE users SET is_admin = 1"], + ["a delete", "DELETE FROM sessions"], + ["a schema name", "DELETE FROM main.users"], + ["an index on a core table", "CREATE INDEX i ON users (id)"], + [ + "a trigger", + "CREATE TRIGGER t AFTER INSERT ON p_foo_a BEGIN SELECT 1; END", + ], + ["a view", "CREATE VIEW users_copy AS SELECT * FROM users"], + ["attaching a database", "ATTACH DATABASE 'x.db' AS other"], + ["a pragma", "PRAGMA writable_schema = 1"], + ["a truncate", "TRUNCATE TABLE users"], + ["an unlogged table", "CREATE UNLOGGED TABLE users (id int)"], + ["a CTE write", "WITH x AS (SELECT 1) DELETE FROM users"], + ])("refuses %s", (_label, sql) => { + expect(findUnownedTableWrites("foo", sql)).not.toEqual([]); + }); + + it.each([ + ["its own table", "CREATE TABLE IF NOT EXISTS p_foo_a (id int)"], + [ + "a reference to a core table", + "CREATE TABLE p_foo_a (u text REFERENCES users(id))", + ], + ["copying from a core table", "INSERT INTO p_foo_a SELECT id FROM users"], + ["a column rename", "ALTER TABLE p_foo_a RENAME COLUMN a TO b"], + [ + "a quoted name in a string", + "INSERT INTO p_foo_a (x) VALUES ('DROP TABLE users')", + ], + ])("allows %s", (_label, sql) => { + expect(findUnownedTableWrites("foo", sql)).toEqual([]); + }); + + it("passes every migration a bundled plugin ships", () => { + const plugins = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../plugins", + ); + for (const id of fs.readdirSync(plugins)) { + const legacy = new Set( + Object.entries(LEGACY_TABLE_OWNERS) + .filter(([, owner]) => owner === id) + .map(([table]) => table), + ); + for (const dialect of ["sqlite", "postgres", "mysql"]) { + const dir = path.join(plugins, id, "migrations", dialect); + if (!fs.existsSync(dir)) continue; + for (const file of fs.readdirSync(dir)) { + if (!file.endsWith(".sql")) continue; + const sql = fs.readFileSync(path.join(dir, file), "utf8"); + expect( + findUnownedTableWrites(id, sql, legacy), + `${id}/${dialect}/${file}`, + ).toEqual([]); + } + } + } + }); + + it("gives a user plugin no legacy exemption at runtime", () => { + const adopt = 'ALTER TABLE "fleets" RENAME TO "p_fleets_fleets"'; + expect(() => + assertOwnedTables("fleets", adopt, { bundled: true }), + ).not.toThrow(); + expect(() => assertOwnedTables("fleets", adopt)).toThrow(/fleets/); + }); +}); + +describe("reading migrations", () => { + const roots: string[] = []; + afterEach(() => { + while (roots.length) { + fs.rmSync(roots.pop()!, { recursive: true, force: true }); + } + }); + + it("refuses a migration that is a symlink to a file outside the plugin", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "plugin-mig-link-")); + roots.push(root); + const outside = path.join(root, "evil.sql"); + fs.writeFileSync(outside, "DROP TABLE users;"); + const dir = path.join(root, "plugin", "migrations", "sqlite"); + fs.mkdirSync(dir, { recursive: true }); + try { + fs.symlinkSync(outside, path.join(dir, "0001_init.sql"), "file"); + } catch { + return; + } + expect(() => readMigrations(path.join(root, "plugin"), "sqlite")).toThrow( + /outside the plugin/, + ); + }); +}); + +describe("removePluginData", () => { + let sqlite: Database.Database; + + beforeEach(() => { + sqlite = new Database(":memory:"); + for (const table of [ + "users", + "p_foo_notes", + "p_foo_items", + "p_foo_bar_x", + "p_other_notes", + ]) { + sqlite.exec(`CREATE TABLE "${table}" (id INTEGER PRIMARY KEY)`); + } + state.db = drizzle(sqlite); + state.deleted = []; + }); + + afterEach(() => sqlite.close()); + + const tables = () => + ( + sqlite + .prepare("SELECT name FROM sqlite_master WHERE type = 'table'") + .all() as Array<{ name: string }> + ) + .map((row) => row.name) + .sort(); + + it("drops only the plugin's own tables, found in the catalog", async () => { + const removed = await removePluginData("foo", { + knownPluginIds: ["foo", "foo-bar", "other"], + }); + expect(removed.tables.sort()).toEqual(["p_foo_items", "p_foo_notes"]); + expect(tables()).toEqual(["p_foo_bar_x", "p_other_notes", "users"]); + }); + + it("takes its settings and secrets with it", async () => { + await removePluginData("foo", { knownPluginIds: ["foo"] }); + expect(state.deleted).toContain("settings:foo"); + expect(state.deleted).toContain("kv:foo"); + expect(state.deleted).toContain("grants:foo"); + }); + + it("leaves a longer prefix another plugin owns", () => { + expect( + ownedTableNames( + "foo", + ["p_foo_a", "p_foo_bar_b", "p_food_c"], + ["foo-bar"], + ), + ).toEqual(["p_foo_a"]); + }); +}); diff --git a/src/backend/tests/plugins/security/discovery.test.ts b/src/backend/tests/plugins/security/discovery.test.ts new file mode 100644 index 000000000..b1c33d91c --- /dev/null +++ b/src/backend/tests/plugins/security/discovery.test.ts @@ -0,0 +1,194 @@ +/** + * Discovery: a bundled id can never be taken by a user plugin, the folder + * name is the id, nothing a manifest names (or a symlink inside the plugin) + * reaches outside the plugin folder, table prefixes cannot overlap, and the + * manifest validator refuses unknown fields, bad ids and reserved ids. + */ + +import { afterEach, describe, expect, it, vi } from "vitest"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { PluginLoader, prefixesOverlap } from "../../../plugins/loader.js"; +import { parseManifest } from "../../../plugins/manifest.js"; +import { RESERVED_PLUGIN_IDS } from "@termix/plugin-sdk/manifest"; +import { createFixturePlugin } from "../fixture-plugin.js"; + +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentPluginStorageRepository: () => ({}), +})); + +const cleanups: Array<() => void> = []; + +function tempRoot(prefix: string): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + cleanups.push(() => fs.rmSync(root, { recursive: true, force: true })); + return root; +} + +afterEach(() => { + delete process.env.TERMIX_BUNDLED_PLUGINS_DIR; + delete process.env.DATA_DIR; + while (cleanups.length) cleanups.pop()?.(); +}); + +function validManifest(overrides: Record = {}) { + const fixture = createFixturePlugin(); + cleanups.push(fixture.cleanup); + const manifest = JSON.parse( + fs.readFileSync(path.join(fixture.dir, "manifest.json"), "utf8"), + ); + return { ...manifest, ...overrides }; +} + +describe("shadowing a bundled plugin", () => { + it("is refused even when the bundled one failed to load", async () => { + const bundled = tempRoot("termix-bundled-"); + const user = tempRoot("termix-data-"); + const userPlugins = path.join(user, "plugins"); + fs.mkdirSync(userPlugins, { recursive: true }); + + // The bundled copy is broken, so it never makes it into the loader. + createFixturePlugin({ id: "ssh-terminal", root: bundled }); + fs.writeFileSync( + path.join(bundled, "ssh-terminal", "manifest.json"), + "{ broken", + ); + createFixturePlugin({ id: "ssh-terminal", root: userPlugins }); + + process.env.TERMIX_BUNDLED_PLUGINS_DIR = bundled; + process.env.DATA_DIR = user; + const loaded = await new PluginLoader().loadAll(); + + expect(loaded).toEqual([]); + }); + + it("needs the folder name to be the manifest id", async () => { + const fixture = createFixturePlugin({ + manifestOverrides: { id: "other-id" }, + }); + cleanups.push(fixture.cleanup); + await expect(new PluginLoader().load(fixture.dir, "user")).rejects.toThrow( + /does not match manifest id/, + ); + }); +}); + +describe("table prefixes", () => { + it("overlap when one id's prefix covers the other's tables", () => { + expect(prefixesOverlap("foo", "foo-bar")).toBe(true); + expect(prefixesOverlap("foo-bar", "foo")).toBe(true); + expect(prefixesOverlap("session", "session-sharing")).toBe(true); + expect(prefixesOverlap("ai", "automations")).toBe(false); + expect(prefixesOverlap("web-endpoint", "webauthn")).toBe(false); + expect(prefixesOverlap("foo", "foo")).toBe(false); + }); + + it("the loader refuses the second plugin of an overlapping pair", async () => { + const root = tempRoot("termix-overlap-"); + createFixturePlugin({ id: "foo", root }); + createFixturePlugin({ id: "foo-bar", root }); + const loader = new PluginLoader(); + await loader.load(path.join(root, "foo"), "bundled"); + await expect( + loader.load(path.join(root, "foo-bar"), "user"), + ).rejects.toThrow(/overlaps "foo"/); + }); + + it("no two bundled plugins overlap", () => { + const plugins = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../plugins", + ); + const ids = fs + .readdirSync(plugins, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + for (const a of ids) { + for (const b of ids) { + expect(prefixesOverlap(a, b), `${a} and ${b}`).toBe(false); + } + } + }); +}); + +describe("paths stay inside the plugin", () => { + it("refuses a backend entry that is a symlink to outside", async () => { + const root = tempRoot("termix-link-"); + const outside = path.join(root, "outside.mjs"); + fs.writeFileSync(outside, "export function activate() {}"); + const fixture = createFixturePlugin({ id: "linker", root }); + const entry = path.join(fixture.dir, "backend", "index.mjs"); + fs.rmSync(entry); + try { + fs.symlinkSync(outside, entry, "file"); + } catch { + return; + } + await expect(new PluginLoader().load(fixture.dir, "user")).rejects.toThrow( + /outside the plugin directory/, + ); + }); + + it.each([ + ["backend", "../../evil.js"], + ["backend", "/etc/evil.js"], + ["backend", "C:/evil.js"], + ["frontend", "dist\\..\\..\\evil.js"], + ["frontend", "."], + ["locales", "../../secrets"], + ])("the validator refuses %s: %s", (field, value) => { + const { manifest, errors } = parseManifest( + validManifest({ [field]: value }), + ); + expect(manifest).toBeUndefined(); + expect(errors.join("\n")).toMatch(/relative path inside the plugin/); + }); +}); + +describe("manifest validation", () => { + it.each(RESERVED_PLUGIN_IDS.map((id) => [id]))( + "refuses the reserved id %s", + (id) => { + const { manifest, errors } = parseManifest(validManifest({ id })); + expect(manifest).toBeUndefined(); + expect(errors.join("\n")).toMatch(/reserved/); + }, + ); + + it.each([["Bad-Id"], ["1starts-with-digit"], ["has_underscore"], ["a"]])( + "refuses the bad id %s", + (id) => { + expect(parseManifest(validManifest({ id })).manifest).toBeUndefined(); + }, + ); + + it("refuses an unknown top-level field", () => { + const { manifest, errors } = parseManifest( + validManifest({ runAsRoot: true }), + ); + expect(manifest).toBeUndefined(); + expect(errors.join("\n")).toMatch(/runAsRoot/); + }); + + it("refuses an unknown nested field", () => { + const { manifest } = parseManifest( + validManifest({ author: { name: "x", backdoor: true } }), + ); + expect(manifest).toBeUndefined(); + }); + + it("refuses a preset key that is not a plain name", () => { + const presets = { + simple: { __proto__x: true }, + balanced: { __proto__x: true }, + advanced: { __proto__x: true }, + }; + const { manifest, errors } = parseManifest( + validManifest({ contributes: { uiPresets: presets } }), + ); + expect(manifest).toBeUndefined(); + expect(errors.join("\n")).toMatch(/invalid key/); + }); +}); diff --git a/src/backend/tests/plugins/security/process-env.test.ts b/src/backend/tests/plugins/security/process-env.test.ts new file mode 100644 index 000000000..a4da2002a --- /dev/null +++ b/src/backend/tests/plugins/security/process-env.test.ts @@ -0,0 +1,38 @@ +/** + * A program a plugin starts through ctx.process.run gets a small base + * environment, never the server's own, which holds the JWT secret, database + * keys and OIDC client secrets. + */ + +import { describe, expect, it } from "vitest"; +import { baseEnvironment } from "../../../plugins/ctx-process.js"; + +describe("ctx.process environment", () => { + it("passes the path, home, temp and proxy through", () => { + const env = baseEnvironment({ + PATH: "/usr/bin", + HOME: "/home/termix", + TMPDIR: "/tmp", + HTTPS_PROXY: "http://proxy:3128", + }); + expect(env).toEqual({ + PATH: "/usr/bin", + HOME: "/home/termix", + TMPDIR: "/tmp", + HTTPS_PROXY: "http://proxy:3128", + }); + }); + + it("drops the server's secrets", () => { + const env = baseEnvironment({ + PATH: "/usr/bin", + JWT_SECRET: "jwt", + DATABASE_KEY: "db", + DATABASE_URL: "postgres://user:pass@db/termix", + OIDC_CLIENT_SECRET: "oidc", + INTERNAL_AUTH_TOKEN: "internal", + GUACAMOLE_ENCRYPTION_KEY: "guac", + }); + expect(Object.keys(env)).toEqual(["PATH"]); + }); +}); diff --git a/src/backend/tests/plugins/security/rbac.test.ts b/src/backend/tests/plugins/security/rbac.test.ts new file mode 100644 index 000000000..1ecdb2a21 --- /dev/null +++ b/src/backend/tests/plugins/security/rbac.test.ts @@ -0,0 +1,245 @@ +/** + * RBAC: a plugin's permissions live in its own namespace and nowhere else. + * A plugin cannot take a core group's name as its id, its own declared names + * resolve to itself, and a role default is recorded as applied only once the + * role really has it. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const state = vi.hoisted(() => ({ + /** roleName -> the permission array stored on that role. */ + roles: new Map(), + /** The rbac_applied_defaults rows, as ":". */ + appliedDefaults: new Set(), + /** The rbac_known_permissions rows. */ + known: new Set(), + warnings: [] as string[], + failUpdate: false, +})); + +vi.mock("../../../upgrade/boot-migrations.js", () => ({ + runPluginDataMigrations: async () => {}, +})); + +vi.mock("../../../utils/logger.js", () => ({ + pluginLogger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn((message: string) => { + state.warnings.push(message); + }), + error: vi.fn((message: string) => { + state.warnings.push(message); + }), + success: vi.fn(), + }, + databaseLogger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + }, +})); + +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: vi.fn(async () => {}), + getAuditUsername: vi.fn(async (userId: string) => `user:${userId}`), +})); + +vi.mock("../../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ + hasPermission: async () => true, + invalidateUserPermissionCache: vi.fn(), + }), + }, +})); + +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentRoleRepository: () => ({ + findRoleByName: async (name: string) => { + const role = state.roles.get(name); + if (!role) return null; + return { + id: role.id, + name, + permissions: JSON.stringify(role.permissions), + }; + }, + updateRole: async (id: number, updates: { permissions?: string }) => { + if (state.failUpdate) throw new Error("database is locked"); + for (const [name, role] of state.roles) { + if (role.id !== id) continue; + if (updates.permissions === undefined) continue; + state.roles.set(name, { + id, + permissions: JSON.parse(updates.permissions) as string[], + }); + } + }, + listRoleUserIds: async () => [], + listRoles: async () => + [...state.roles.entries()].map(([name, role]) => ({ + id: role.id, + name, + permissions: JSON.stringify(role.permissions), + })), + }), + createCurrentRbacPermissionRepository: () => ({ + listKnown: async () => + [...state.known].map((permission) => ({ permission, pluginId: null })), + recordKnown: async (rows: { permission: string }[]) => { + for (const row of rows) state.known.add(row.permission); + return rows.length; + }, + listAppliedDefaults: async () => + [...state.appliedDefaults].map((key) => { + const [roleName, ...rest] = key.split(":"); + return { roleName, permission: rest.join(":") }; + }), + recordAppliedDefaults: async ( + rows: { roleName: string; permission: string }[], + ) => { + for (const row of rows) { + state.appliedDefaults.add(`${row.roleName}:${row.permission}`); + } + return rows.length; + }, + }), +})); + +const { PluginLoader } = await import("../../../plugins/loader.js"); +const { createFixturePlugin } = await import("../fixture-plugin.js"); +const { registerPluginPermissions, resetPermissionCatalog, isValidPermission } = + await import("../../../utils/permission-catalog.js"); +const { resolvePermission } = await import("../../../plugins/rbac.js"); +const { parseManifest } = await import("../../../plugins/manifest.js"); + +function permission(name: string, defaultRoles?: string[]) { + return { + name, + titleKey: `permissions.${name}.title`, + descriptionKey: `permissions.${name}.description`, + ...(defaultRoles ? { defaultRoles } : {}), + }; +} + +beforeEach(() => { + state.roles.clear(); + state.appliedDefaults.clear(); + state.known.clear(); + state.warnings.length = 0; + state.failUpdate = false; + resetPermissionCatalog(); +}); +afterEach(() => resetPermissionCatalog()); + +describe("a plugin cannot mint core permissions", () => { + it.each([["admin"], ["hosts"], ["credentials"]])( + "a plugin called %s never loads", + async (id) => { + const fixture = createFixturePlugin({ + id, + capabilities: ["kv:own"], + manifestOverrides: { + contributes: { permissions: [permission("backdoor", ["user"])] }, + }, + }); + try { + await expect( + new PluginLoader().load(fixture.dir, "user"), + ).rejects.toThrow(/reserved/); + expect(isValidPermission(`${id}.backdoor`)).toBe(false); + } finally { + fixture.cleanup(); + } + }, + ); +}); + +describe("resolving a permission name", () => { + it("a name the plugin declares is its own, whatever its first segment", () => { + registerPluginPermissions({ + group: "billing", + pluginId: "billing", + label: "Billing", + permissions: ["billing.pay"], + }); + const parsed = parseManifest({ + id: "sample", + name: "Sample", + version: "1.0.0", + description: "A fixture.", + author: { name: "Termix" }, + license: "MIT", + category: "Productivity", + engine: { termix: ">=2.9.0", api: "1" }, + capabilities: ["kv:own"], + contributes: { permissions: [permission("billing.pay")] }, + }).manifest!; + + expect(resolvePermission(parsed, "billing.pay")).toBe("sample.billing.pay"); + }); +}); + +describe("role defaults", () => { + it("are not marked applied when the role update fails, so the next boot retries", async () => { + state.roles.set("user", { id: 2, permissions: ["hosts.*"] }); + state.failUpdate = true; + const fixture = createFixturePlugin({ + id: "docker", + capabilities: ["kv:own"], + manifestOverrides: { + category: "Infrastructure", + contributes: { permissions: [permission("run", ["user"])] }, + }, + }); + const { resetPluginRuntime, getPluginRuntime, activatePlugin } = + await import("../../../plugins/index.js"); + resetPluginRuntime(); + const loader = getPluginRuntime().loader; + try { + const plugin = await loader.load(fixture.dir); + await activatePlugin(plugin.id); + expect(state.roles.get("user")?.permissions).not.toContain("docker.run"); + expect(state.appliedDefaults.has("user:docker.run")).toBe(false); + + // Next boot, the database is fine again. + await loader.shutdown(); + state.failUpdate = false; + resetPluginRuntime(); + const again = getPluginRuntime().loader; + await activatePlugin((await again.load(fixture.dir)).id); + expect(state.roles.get("user")?.permissions).toContain("docker.run"); + expect(state.appliedDefaults.has("user:docker.run")).toBe(true); + await again.shutdown(); + } finally { + fixture.cleanup(); + } + }); + + it("only ever land in the plugin's own namespace", async () => { + state.roles.set("user", { id: 2, permissions: [] }); + const fixture = createFixturePlugin({ + id: "docker", + capabilities: ["kv:own"], + manifestOverrides: { + category: "Infrastructure", + contributes: { permissions: [permission("run", ["user"])] }, + }, + }); + const { resetPluginRuntime, getPluginRuntime, activatePlugin } = + await import("../../../plugins/index.js"); + resetPluginRuntime(); + const loader = getPluginRuntime().loader; + try { + await activatePlugin((await loader.load(fixture.dir)).id); + expect(state.roles.get("user")?.permissions).toEqual(["docker.run"]); + } finally { + await loader.shutdown(); + fixture.cleanup(); + } + }); +}); diff --git a/src/backend/tests/plugins/security/routes.test.ts b/src/backend/tests/plugins/security/routes.test.ts new file mode 100644 index 000000000..e5d51bcc9 --- /dev/null +++ b/src/backend/tests/plugins/security/routes.test.ts @@ -0,0 +1,268 @@ +/** + * /plugin-api and /plugin-ws: auth unless declared public, 503 for an + * installed plugin that is off, no stack in an error, public routes listed + * for admins, and an anonymous caller who cannot spend a plugin's error + * budget. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import express from "express"; +import httpModule from "node:http"; +import type { AddressInfo } from "node:net"; +import type { Server } from "node:http"; +import { WebSocket } from "ws"; + +const state = vi.hoisted(() => ({ + validToken: "valid-token", + userId: "user-1", + installed: new Set(), + audits: [] as Array>, +})); + +vi.mock("../../../utils/logger.js", () => { + const log = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + }; + return { pluginLogger: log, databaseLogger: log }; +}); + +vi.mock("../../../utils/auth-manager.js", () => ({ + AuthManager: { + getInstance: () => ({ + createAuthMiddleware: + () => + ( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ) => { + const header = req.headers.authorization; + if (header !== `Bearer ${state.validToken}`) { + res.status(401).json({ error: "Unauthorized" }); + return; + } + (req as express.Request & { userId?: string }).userId = state.userId; + next(); + }, + verifyJWTToken: async (token: string) => + token === state.validToken ? { userId: state.userId } : null, + }), + }, +})); +vi.mock("../../../utils/data-crypto.js", () => ({ + DataCrypto: { getUserDataKey: () => Buffer.from("key") }, +})); +vi.mock("../../../plugins/permissions.js", () => ({ + hasCapability: async () => true, +})); +vi.mock("../../../utils/audit-logger.js", () => ({ + logAudit: async (entry: Record) => { + state.audits.push(entry); + }, +})); + +const http = await import("../../../plugins/http.js"); +const ws = await import("../../../plugins/ws.js"); +const { getActor } = await import("../../../plugins/actor.js"); +const { mountPluginApi } = + await import("../../../database/routes/plugin-api-routes.js"); + +const manifest = { + id: "sample", + name: "Sample", + version: "1.0.0", + capabilities: ["network:serve"], +} as never; + +let server: Server | null = null; + +async function start(): Promise<{ httpBase: string; wsBase: string }> { + const app = express(); + mountPluginApi(app); + const instance = httpModule.createServer(app); + ws.attachPluginWebSockets(instance); + server = instance; + return new Promise((resolve) => { + instance.listen(0, "127.0.0.1", () => { + const { port } = instance.address() as AddressInfo; + resolve({ + httpBase: `http://127.0.0.1:${port}`, + wsBase: `ws://127.0.0.1:${port}`, + }); + }); + }); +} + +function upgrade( + url: string, + protocols: string[] = [], +): Promise<"open" | number> { + return new Promise((resolve) => { + const socket = new WebSocket(url, protocols); + socket.once("open", () => { + socket.close(); + resolve("open"); + }); + socket.once("unexpected-response", (_req, res) => + resolve(res.statusCode ?? 0), + ); + socket.once("error", () => resolve(0)); + }); +} + +beforeEach(() => { + http.resetPluginHttp(); + ws.resetPluginWebSockets(); + state.installed = new Set(); + state.audits = []; + http.setPluginInstalledCheck((id) => state.installed.has(id)); +}); + +afterEach(async () => { + ws.resetPluginWebSockets(); + if (server) { + await new Promise((resolve) => server!.close(() => resolve(undefined))); + server = null; + } +}); + +describe("auth by default", () => { + it("refuses an HTTP route and a socket without a login", async () => { + const router = http.createPluginRouter({ manifest, reportError: () => {} }); + router.get("/private", (_req, res) => res.json({ ok: true })); + ws.registerPluginWsRoute("sample", "/socket", () => {}, ["network:serve"]); + const { httpBase, wsBase } = await start(); + + expect((await fetch(`${httpBase}/plugin-api/sample/private`)).status).toBe( + 401, + ); + expect(await upgrade(`${wsBase}/plugin-ws/sample/socket`)).toBe(401); + }); + + it("gives a public socket's guest no actor at all", async () => { + let actor: string | undefined = "unset"; + ws.registerPluginWsRoute( + "sample", + "/guest", + ({ socket }) => { + actor = getActor(); + (socket as WebSocket).close(); + }, + ["network:serve"], + { public: true }, + ); + const { wsBase } = await start(); + await upgrade(`${wsBase}/plugin-ws/sample/guest`); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(actor).toBeUndefined(); + }); +}); + +describe("a disabled plugin", () => { + it("answers 503 on HTTP and on a socket once its routes are gone", async () => { + state.installed.add("sample"); + const { httpBase, wsBase } = await start(); + + const response = await fetch(`${httpBase}/plugin-api/sample/anything`); + expect(response.status).toBe(503); + expect(await upgrade(`${wsBase}/plugin-ws/sample/socket`)).toBe(503); + }); + + it("answers 404 for an id nobody installed", async () => { + const { httpBase, wsBase } = await start(); + expect((await fetch(`${httpBase}/plugin-api/ghost/x`)).status).toBe(404); + expect(await upgrade(`${wsBase}/plugin-ws/ghost/socket`)).toBe(404); + }); +}); + +describe("errors", () => { + it("hides the stack and the message", async () => { + const router = http.createPluginRouter({ manifest, reportError: () => {} }); + router.get("/boom", () => { + throw new Error("secret detail at /srv/termix/keys"); + }); + const { httpBase } = await start(); + const response = await fetch(`${httpBase}/plugin-api/sample/boom`, { + headers: { Authorization: `Bearer ${state.validToken}` }, + }); + const body = await response.text(); + expect(response.status).toBe(500); + expect(body).not.toMatch(/secret detail|at \//); + }); + + it("does not let an anonymous caller spend the error budget", async () => { + const reported: unknown[] = []; + const router = http.createPluginRouter({ + manifest, + options: { public: ["/hook", "/throws"] }, + reportError: (error) => reported.push(error), + }); + router.post("/hook", (_req, res) => res.json({ ok: true })); + router.get("/throws", () => { + throw new Error("bad input"); + }); + const { httpBase } = await start(); + + for (let i = 0; i < 6; i++) { + const junk = await fetch(`${httpBase}/plugin-api/sample/hook`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{not json", + }); + expect(junk.status).toBe(400); + await fetch(`${httpBase}/plugin-api/sample/throws`); + } + expect(reported).toEqual([]); + }); + + it("still counts a signed-in caller's server error", async () => { + const reported: unknown[] = []; + const router = http.createPluginRouter({ + manifest, + reportError: (error) => reported.push(error), + }); + router.get("/boom", () => { + throw new Error("real bug"); + }); + const { httpBase } = await start(); + await fetch(`${httpBase}/plugin-api/sample/boom`, { + headers: { Authorization: `Bearer ${state.validToken}` }, + }); + expect(reported).toHaveLength(1); + }); +}); + +describe("public routes are visible", () => { + it("lists them for the admin details and audits a public socket", async () => { + http.createPluginRouter({ + manifest, + options: { public: ["/callback", "/u/:handle"] }, + reportError: () => {}, + }); + ws.registerPluginWsRoute("sample", "/guest", () => {}, ["network:serve"], { + public: true, + }); + ws.registerPluginWsRoute("sample", "/private", () => {}, ["network:serve"]); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(http.getPluginPublicHttpRoutes("sample")).toEqual([ + "/callback", + "/u/:handle", + ]); + expect(ws.getPluginPublicWsRoutes("sample")).toEqual(["/guest"]); + expect( + state.audits.some( + (entry) => + entry.action === "plugin_ws_public_route" && + entry.resourceId === "sample", + ), + ).toBe(true); + + http.unregisterPluginHttp("sample"); + expect(http.getPluginPublicHttpRoutes("sample")).toEqual([]); + }); +}); diff --git a/src/backend/tests/plugins/security/settings.test.ts b/src/backend/tests/plugins/security/settings.test.ts new file mode 100644 index 000000000..82611a787 --- /dev/null +++ b/src/backend/tests/plugins/security/settings.test.ts @@ -0,0 +1,154 @@ +/** + * Settings: a secret is encrypted at rest and never read back, and every + * write path goes through the same checks, including the host import path + * that used to write rows straight into plugin_settings. + * + * The HTTP scope checks (admin needs admin.plugins.manage, a user only ever + * writes their own, a host needs edit access) are covered route by route in + * settings-routes.test.ts. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +interface Row { + pluginId: string; + scope: string; + scopeId: string | null; + key: string; + value: string | null; + encrypted: boolean; +} + +const state = vi.hoisted(() => ({ + rows: [] as Row[], + manifests: [] as unknown[], + normalizer: null as null | ((raw: Record) => unknown), +})); + +vi.mock("../../../utils/logger.js", () => { + const log = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + return { sshLogger: log, pluginLogger: log }; +}); +vi.mock("../../../database/repositories/factory.js", () => ({ + createCurrentPluginSettingsRepository: () => ({ + get: async (p: string, s: string, id: string | null, k: string) => + state.rows.find( + (row) => + row.pluginId === p && + row.scope === s && + row.scopeId === id && + row.key === k, + ) ?? null, + getAll: async (p: string, s: string, id: string | null) => + state.rows.filter( + (row) => row.pluginId === p && row.scope === s && row.scopeId === id, + ), + set: async ( + pluginId: string, + scope: string, + scopeId: string | null, + key: string, + value: string | null, + encrypted = false, + ) => { + state.rows = state.rows.filter( + (row) => + !( + row.pluginId === pluginId && + row.scope === scope && + row.scopeId === scopeId && + row.key === key + ), + ); + state.rows.push({ pluginId, scope, scopeId, key, value, encrypted }); + }, + }), +})); +vi.mock("../../../utils/system-secret-crypto.js", () => ({ + isSystemEncrypted: (value: string) => value.startsWith("sysenc:v1:"), + encryptSystemSecret: async (plaintext: string) => + `sysenc:v1:${Buffer.from(plaintext, "utf8").toString("base64")}`, + decryptSystemSecret: async (stored: string) => + Buffer.from(stored.slice("sysenc:v1:".length), "base64").toString("utf8"), +})); +vi.mock("../../../plugins/index.js", () => ({ + getPluginRuntime: () => ({ + loader: { + list: () => + state.manifests.map((manifest) => ({ state: "active", manifest })), + }, + }), +})); +vi.mock("../../../plugins/registry.js", () => ({ + consume: () => state.normalizer, +})); + +const { applyPluginHostImportSettings } = + await import("../../../database/routes/host-plugin-settings.js"); +const { getAllSettings, setSetting } = + await import("../../../plugins/settings.js"); + +const MANIFEST = { + id: "vaultish", + name: "Vaultish", + version: "1.0.0", + capabilities: [], + contributes: { + settings: { + host: { + fields: [ + { key: "token", type: "secret", labelKey: "k" }, + { key: "port", type: "number", labelKey: "k", min: 1, max: 65535 }, + ], + }, + admin: [{ key: "apiKey", type: "secret", labelKey: "k" }], + }, + }, +}; + +beforeEach(() => { + state.rows = []; + state.manifests = [MANIFEST]; + state.normalizer = null; +}); + +describe("secrets at rest", () => { + it("are stored encrypted and read back only as { set }", async () => { + await setSetting(MANIFEST as never, "admin", null, "apiKey", "hunter2"); + const row = state.rows.find((entry) => entry.key === "apiKey")!; + expect(row.encrypted).toBe(true); + expect(row.value).not.toContain("hunter2"); + + const shown = await getAllSettings(MANIFEST as never, "admin", null, { + redactSecrets: true, + }); + expect(shown.apiKey).toEqual({ set: true }); + }); +}); + +describe("the host import path", () => { + it("encrypts a secret a plugin's normalizer returns", async () => { + state.normalizer = () => ({ token: "from-import", port: 8200 }); + await applyPluginHostImportSettings(7, {}); + + const token = state.rows.find((entry) => entry.key === "token")!; + expect(token.encrypted).toBe(true); + expect(token.value).not.toContain("from-import"); + expect(state.rows.find((entry) => entry.key === "port")?.value).toBe( + "8200", + ); + }); + + it("refuses a key the manifest does not declare, and a bad value", async () => { + state.normalizer = () => ({ injected: "x", port: 999999 }); + await applyPluginHostImportSettings(7, {}); + expect(state.rows).toEqual([]); + }); + + it("never restores a secret carried in an export", async () => { + await applyPluginHostImportSettings(7, { + pluginSettings: { vaultish: { token: "leaked", port: 22 } }, + }); + expect(state.rows.map((entry) => entry.key)).toEqual(["port"]); + }); +}); diff --git a/src/ui/api/plugins-api.ts b/src/ui/api/plugins-api.ts index ee973a355..11dcdaee1 100644 --- a/src/ui/api/plugins-api.ts +++ b/src/ui/api/plugins-api.ts @@ -238,6 +238,8 @@ export interface PluginSummary { grantedCapabilities?: string[]; /** Why the plugin is blocked or failed. Admins only. */ lastError?: string | null; + /** Paths a running plugin serves without login. Admins only. */ + publicRoutes?: { http: string[]; ws: string[] }; } export async function getPlugins(): Promise { diff --git a/src/ui/locales/en.json b/src/ui/locales/en.json index c02c982ee..d6eec9e60 100644 --- a/src/ui/locales/en.json +++ b/src/ui/locales/en.json @@ -1842,6 +1842,8 @@ "pluginDisabled": "{{name}} disabled", "pluginPermissions": "Permissions", "pluginPermissionsNone": "This plugin does not declare any capabilities that need to be granted.", + "pluginPublicRoutes": "Public routes", + "pluginPublicRoutesDescription": "Anyone can reach these without signing in. The plugin checks access on its own.", "pluginPermissionGranted": "Granted", "pluginPermissionNotGranted": "Not granted", "pluginGrantFailed": "Failed to grant the capability", diff --git a/src/ui/plugin-host/app.ts b/src/ui/plugin-host/app.ts index 9d6115913..d5d7274f5 100644 --- a/src/ui/plugin-host/app.ts +++ b/src/ui/plugin-host/app.ts @@ -47,7 +47,7 @@ import { pluginHostBridge, resolvePluginPermission } from "./bridge"; import { shell, tabsApi } from "./shell-bridge"; import { onRemoteServerChange, remoteServerUrl } from "./desktop"; import { isElectron } from "@/lib/electron"; -import { withPluginScope } from "./scope"; +import { withPluginScope, withIconBoundary, guardCallback } from "./scope"; import { manifestDeclares, type ViewKind } from "./view-ownership"; import { pluginKey } from "@/lib/plugin-i18n"; import { hasPermission } from "@/hooks/use-permissions"; @@ -114,7 +114,10 @@ export function createPluginApp( return track( registerRailItem({ id: item.id, - icon: item.icon as unknown as LucideIcon, + icon: withIconBoundary( + pluginId, + item.icon as unknown as LucideIcon, + ) as unknown as LucideIcon, labelKey: key(item.titleKey), kind: item.kind === "tab" ? "tab" : undefined, hideable: item.hideable, @@ -158,7 +161,7 @@ export function createPluginApp( component: scoped( component as unknown as ComponentType, ), - icon: options.icon, + icon: withIconBoundary(pluginId, options.icon as never) as never, titleKey: options.titleKey ? key(options.titleKey) : undefined, requiresHost: options.requiresHost, noHostMessageKey: options.noHostMessageKey @@ -191,7 +194,7 @@ export function createPluginApp( pluginId, group: section.group, labelKey: key(section.titleKey), - icon: section.icon, + icon: withIconBoundary(pluginId, section.icon as never) as never, order: section.order, visible: section.visible, component: scoped( @@ -207,10 +210,20 @@ export function createPluginApp( ...action, titleKey: key(action.titleKey), pluginId, + icon: withIconBoundary(pluginId, action.icon as never) as never, when: action.when as never, run: action.run as never, - label: action.label as never, - items: action.items as never, + // Called while the host list renders, so a throw must not escape. + label: guardCallback( + pluginId, + action.label as ((...args: unknown[]) => unknown) | undefined, + undefined, + ) as never, + items: guardCallback( + pluginId, + action.items as ((...args: unknown[]) => unknown) | undefined, + undefined, + ) as never, }), ); }, diff --git a/src/ui/plugin-host/loader.ts b/src/ui/plugin-host/loader.ts index eea34bdaa..ebd49da4a 100644 --- a/src/ui/plugin-host/loader.ts +++ b/src/ui/plugin-host/loader.ts @@ -184,6 +184,20 @@ async function deactivate(pluginId: string): Promise { setFrontendState(pluginId, "inactive"); } +const ACTIVATE_TIMEOUT_MS = 15_000; + +function withTimeout( + promise: Promise, + ms: number, + message: string, +): Promise { + let timer: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(message)), ms); + }); + return Promise.race([promise, timeout]).finally(() => clearTimeout(timer)); +} + async function activate(summary: PluginSummary): Promise { const loader = currentDeps(); setFrontendState(summary.id, "loading"); @@ -202,7 +216,13 @@ async function activate(summary: PluginSummary): Promise { summary.contributes, { guest: guestMode }, ); - await module.activate(handle.app); + // A plugin whose activate never settles would hold up every plugin + // queued after it, so it fails instead. + await withTimeout( + Promise.resolve(module.activate(handle.app)), + ACTIVATE_TIMEOUT_MS, + `activate() did not finish within ${ACTIVATE_TIMEOUT_MS / 1000}s`, + ); active.set(summary.id, { id: summary.id, assetVersion: summary.assetVersion ?? null, diff --git a/src/ui/plugin-host/scope.tsx b/src/ui/plugin-host/scope.tsx index edebf7be3..53dc5e12b 100644 --- a/src/ui/plugin-host/scope.tsx +++ b/src/ui/plugin-host/scope.tsx @@ -110,3 +110,61 @@ export function withPluginScope

( Scoped.displayName = `Plugin(${pluginId}:${Inner.displayName ?? Inner.name ?? "Component"})`; return Scoped; } + +/** Renders nothing when a plugin's icon throws, instead of a crash notice. */ +class IconBoundary extends Component { + state = { error: false }; + + static getDerivedStateFromError() { + return { error: true }; + } + + componentDidCatch(error: Error) { + console.error(`[plugins] ${this.props.pluginId} icon failed`, error); + } + + render() { + return this.state.error ? null : this.props.children; + } +} + +/** + * A plugin icon the shell renders inline (rail, tab strip, host row). Small + * enough that a broken one should just disappear, not take the row with it. + */ +export function withIconBoundary

( + pluginId: string, + Icon: ComponentType

| undefined, +): ComponentType

| undefined { + if (!Icon) return undefined; + function SafeIcon(props: P) { + return ( + + + + ); + } + SafeIcon.displayName = `PluginIcon(${pluginId})`; + return SafeIcon; +} + +/** + * A plugin callback core calls while rendering, such as a host action's + * label. A throw is logged and answered with `fallback`, so one plugin + * cannot break the host list. + */ +export function guardCallback( + pluginId: string, + fn: ((...args: A) => R) | undefined, + fallback: R, +): ((...args: A) => R) | undefined { + if (!fn) return undefined; + return (...args: A) => { + try { + return fn(...args); + } catch (error) { + console.error(`[plugins] ${pluginId} callback failed`, error); + return fallback; + } + }; +} diff --git a/src/ui/sidebar/PluginPermissionsDialog.tsx b/src/ui/sidebar/PluginPermissionsDialog.tsx index 59360ce16..fc00d3f59 100644 --- a/src/ui/sidebar/PluginPermissionsDialog.tsx +++ b/src/ui/sidebar/PluginPermissionsDialog.tsx @@ -49,6 +49,14 @@ export function PluginPermissionsDialog({ declared.includes(row.capability), ); const granted = new Set(plugin.grantedCapabilities ?? []); + const publicRoutes = [ + ...(plugin.publicRoutes?.http ?? []).map( + (path) => `/plugin-api/${plugin.id}${path}`, + ), + ...(plugin.publicRoutes?.ws ?? []).map( + (path) => `/plugin-ws/${plugin.id}${path}`, + ), + ]; const toggle = async (capability: string, isGranted: boolean) => { setBusy(capability); @@ -123,6 +131,27 @@ export function PluginPermissionsDialog({ )} + + {publicRoutes.length > 0 && ( +

+ )} ); diff --git a/src/ui/tests/plugin-host/security/guards.test.tsx b/src/ui/tests/plugin-host/security/guards.test.tsx new file mode 100644 index 000000000..b29345a0b --- /dev/null +++ b/src/ui/tests/plugin-host/security/guards.test.tsx @@ -0,0 +1,144 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { Puzzle } from "lucide-react"; +import type { FrontendModule } from "@termix/plugin-sdk/frontend"; +import type { PluginSummary } from "@/api/plugins-api"; +import { + configurePluginLoader, + resetPluginLoader, + syncPlugins, +} from "@/plugin-host/loader"; +import { getPluginRecord, resetPluginStore } from "@/plugin-host/plugin-store"; +import { guardCallback, withIconBoundary } from "@/plugin-host/scope"; +import { + resetRegisteredRailItems, + visibleRailItems, +} from "@/sidebar/rail-items"; +import { + listHostActions, + resetHostContributions, +} from "@/sidebar/host-contributions"; + +function summary(id: string): PluginSummary { + return { + id, + name: id, + version: "1.0.0", + enabled: true, + state: "active", + frontend: true, + assetVersion: "v1", + contributes: { panels: [{ id: `${id}-panel`, titleKey: "nav.hosts" }] }, + }; +} + +let plugins: PluginSummary[] = []; +let modules: Record = {}; + +beforeEach(() => { + plugins = []; + modules = {}; + vi.spyOn(console, "error").mockImplementation(() => {}); + configurePluginLoader({ + fetchPlugins: async () => plugins, + importFrontend: async (entry) => modules[entry.id], + loadLocale: async () => null, + injectCss: () => null, + }); +}); + +afterEach(async () => { + vi.useRealTimers(); + await resetPluginLoader(); + resetPluginStore(); + resetRegisteredRailItems(); + resetHostContributions(); + vi.restoreAllMocks(); +}); + +describe("a plugin cannot stall or crash the shell", () => { + it("fails a plugin whose activate never finishes, and starts the rest", async () => { + vi.useFakeTimers(); + modules.stuck = { activate: () => new Promise(() => {}) }; + modules.healthy = { + activate(app) { + app.registerRailItem({ + id: "healthy-panel", + icon: Puzzle, + titleKey: "nav.hosts", + }); + }, + }; + plugins = [summary("stuck"), summary("healthy")]; + + const sync = syncPlugins(); + await vi.advanceTimersByTimeAsync(16_000); + await sync; + + expect(getPluginRecord("stuck")?.frontend).toBe("failed"); + expect(visibleRailItems().map((item) => item.id)).toContain( + "healthy-panel", + ); + }); + + it("answers a host action's throwing label and items with nothing", async () => { + modules.labels = { + activate(app) { + app.registerHostAction({ + id: "labels.open", + titleKey: "open", + label: () => { + throw new Error("label broke"); + }, + items: () => { + throw new Error("items broke"); + }, + run: () => {}, + } as never); + }, + }; + plugins = [summary("labels")]; + await syncPlugins(); + + const action = listHostActions().find( + (entry) => entry.id === "labels.open", + ); + expect(action).toBeDefined(); + expect(() => action!.label?.({} as never)).not.toThrow(); + expect(action!.label?.({} as never)).toBeUndefined(); + expect(action!.items?.({} as never)).toBeUndefined(); + }); +}); + +describe("withIconBoundary", () => { + it("renders nothing for a throwing icon and leaves its neighbours alone", () => { + const Broken = () => { + throw new Error("icon broke"); + }; + const Safe = withIconBoundary("demo", Broken)!; + render( +
+ + still here +
, + ); + expect(screen.getByText("still here")).toBeTruthy(); + }); + + it("passes undefined through", () => { + expect(withIconBoundary("demo", undefined)).toBeUndefined(); + }); +}); + +describe("guardCallback", () => { + it("returns the fallback when the plugin's callback throws", () => { + const guarded = guardCallback( + "demo", + () => { + throw new Error("nope"); + }, + "fallback", + ); + expect(guarded?.()).toBe("fallback"); + }); +});