diff --git a/.env.example b/.env.example index e487b5b..95da129 100644 --- a/.env.example +++ b/.env.example @@ -27,6 +27,11 @@ MCP_AUTH_TOKEN= # # This server's public URL — used as the token audience / Resource Indicator. # Required in OAuth mode (SERVER_URL is accepted as an alias). +# +# It ALSO applies outside OAuth mode: create-upload-ticket builds its browser URL and +# its curl command from this value in every auth mode. If you run with MCP_AUTH_TOKEN +# (or unauthenticated) behind a real domain, set it here — otherwise those links fall +# back to http://127.0.0.1:$PORT, which resolves nowhere but on the server itself. # OAUTH_RESOURCE=https://your-server.example.com # SERVER_URL=https://your-server.example.com # @@ -55,6 +60,14 @@ MCP_AUTH_TOKEN= # Finalize / legally-binding write tools (issue invoices). IRREVERSIBLE. Default: false # LEXWARE_ENABLE_FINALIZE=false +# Hosts that upload-file-from-url may download from (comma-separated). Matched on the host +# itself or any subdomain, on a dot boundary — "evilsharepoint.com" does NOT match +# "sharepoint.com". Setting this REPLACES the defaults rather than adding to them, so you can +# opt out of the Microsoft file-sharing domains entirely. Setting it to an empty value blocks +# every host, which disables the tool — an empty list never means "allow everything". +# Default: sharepoint.com,onedrive.live.com,1drv.ms,graph.microsoft.com +# LEXWARE_UPLOAD_ALLOWED_HOSTS=files.example.com,storage.example.org + # ---- Server ---- # Listen port. Cloud Run injects this automatically. Default: 8080 # PORT=8080 diff --git a/CHANGELOG.md b/CHANGELOG.md index abf5732..dc24701 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,72 @@ All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **Upload files without pushing their bytes through the model context.** `upload-file` / + `upload-voucher-file` take the file as base64 inline in the JSON-RPC body, so every byte is billed as + tokens, sits in the conversation transcript, and a ~8 MB receipt runs into the 12 MB body limit — the + file has to travel through the model even though the model has no use for its contents. Three new + drafts-tier tools take a different route: + - **`create-upload-ticket`** issues a short-lived (15 min), single-use ticket and returns both a browser + URL for drag-and-drop and a ready-to-run `curl` command. The bytes go client → server → Lexware; the + model only ever sees the resulting file id. + - **`get-upload-result`** reads back the file id once the transfer has happened. + - **`upload-file-from-url`** fetches the file server-side from a URL. + + **`LEXWARE_UPLOAD_ALLOWED_HOSTS`** configures which hosts `upload-file-from-url` may download from + (comma-separated; the host itself or any subdomain, matched on a dot boundary). It **replaces** the + built-in defaults rather than extending them, so a self-hosted server can stop trusting them; setting + it empty blocks every host and disables the tool. An empty list never means "allow everything". + Default unchanged: `sharepoint.com`, `onedrive.live.com`, `1drv.ms`, `graph.microsoft.com`. + + The ticket store is **in-process**, so this is a single-instance feature: a restart drops open tickets + (they answer `410`, they do not hang), and behind a load balancer without sticky sessions an upload can + reach a different instance than the one that issued the ticket. The 15-minute lifetime bounds the + window; an external store would mean a new infrastructure dependency for what is otherwise a + self-contained server. + + Supporting pieces: an in-memory ticket store, the `GET`/`POST /upload/:ticket` endpoints with their + self-contained upload page, and a URL fetcher hardened against SSRF (host allowlist, redirect + re-validation, and rejection of loopback/link-local/private address ranges after DNS resolution). + `filename` and `mimeType` travel as `X-Filename-B64` (base64url of the UTF-8 bytes) rather than raw + header bytes, so names containing an en dash, typographic quotes or an emoji survive — a raw header + value is Latin-1 on the wire and `fetch()` refuses anything above U+00FF outright. +- **Body parsing now also defers `/upload` paths** from the pre-applied global JSON parser, alongside + `/mcp`. The upload routes read the raw body themselves; letting the JSON parser run first turned a + JSON-content-typed upload into an empty file. + +### Changed +- The existing base64 upload tools are unchanged and remain available — the ticket route is additive. +- **`SERVER_URL` (or `OAUTH_RESOURCE`) now applies in every auth mode, not only OAuth.** It is the + server's public URL, and `create-upload-ticket` builds its browser link and its `curl` command from + it. Previously that base URL was derived from the auth mode — the OAuth resource, otherwise + `http://127.0.0.1:$PORT` — so a static-token deployment behind a real domain (a documented, + supported mode) set `SERVER_URL`, had it ignored, and handed out upload links that resolve nowhere + but inside the container. The value goes through the same HTTPS validation as every other + configured URL, so a typo fails at startup instead of surfacing in a command an operator runs. + Unset, the loopback fallback still applies, now on the configured `PORT`. Nothing changes for + OAuth deployments. + +### Fixed +- **The upload page no longer builds its success message as an HTML string.** The Lexware file id + came back from the API and was interpolated into `innerHTML`; it is now a text node inside a + `` element, so it still renders as code but can never be parsed as markup. The error path + already did this. +- **Filenames from `Content-Disposition` are stripped of control characters and length-capped.** + Sanitizing only removed path separators and trimmed the ends, so + `filename*=UTF-8''evil%0D%0Ainjected.pdf` arrived as `evil\r\ninjected.pdf` — CRLF intact, because + it sits in the middle — and went on into the multipart field and into anything that logs the name. + C0 controls and DEL are now removed wherever they appear, and the result is capped at 255 + characters (the per-component limit of every common filesystem) without splitting a surrogate pair. + A name left empty by this yields the existing fallback chain instead. Legitimate non-ASCII names + (umlauts, en dash, emoji) are untouched. +- **Expired upload tickets are dropped as soon as they are seen**, in `claim()` and `peek()`, not only + by the sweep that runs on the next `create()`. An instance that issued tickets and then went quiet + used to hold the expired entries for as long as it stayed up. Externally nothing changes: still + `410`, still `undefined`. + ## [0.1.7] ### Added diff --git a/README.md b/README.md index e4caf33..b150e77 100644 --- a/README.md +++ b/README.md @@ -28,12 +28,12 @@ Related projects — local (stdio) Lexware MCP servers: ## Capabilities -60 tools across three tiers you enable via environment variables: +63 tools across three tiers you enable via environment variables: | Tier | Default | What it covers | |------|---------|----------------| | **Read** | always on | Profile; contacts & articles (list/get); the voucherlist (plus `summarize-vouchers` for server-side totals); full documents (invoices, quotations, credit notes, order confirmations, delivery notes, dunnings, down-payment invoices, vouchers); **render any document type to PDF** and **download files/receipts** (returned inline as embedded resources); batch & type-dispatched reads (get-vouchers, get-document, get-voucher-file, get-document-file); payments; reference data (countries, payment conditions, posting categories, print layouts); recurring templates (get & list); event subscriptions; document deeplinks | -| **Drafts/writes** (`LEXWARE_ENABLE_DRAFTS`) | on | Create **draft** invoices/quotations/credit-notes/order-confirmations/delivery-notes/dunnings (the Lexware API has no update endpoint for these — set every field, including payment terms, at creation); create & update contacts, articles, and **bookkeeping vouchers**; **upload files** and **attach receipts** to vouchers; create documents as **follow-ups** (`precedingSalesVoucherId`) | +| **Drafts/writes** (`LEXWARE_ENABLE_DRAFTS`) | on | Create **draft** invoices/quotations/credit-notes/order-confirmations/delivery-notes/dunnings (the Lexware API has no update endpoint for these — set every field, including payment terms, at creation); create & update contacts, articles, and **bookkeeping vouchers**; **upload files** and **attach receipts** to vouchers — inline as base64, or **without base64** via a short-lived upload ticket (`create-upload-ticket` → browser drag-and-drop or a `curl` one-liner → `get-upload-result`) or straight from a URL (`upload-file-from-url`, host-allowlisted); create documents as **follow-ups** (`precedingSalesVoucherId`) | | **Finalize** (`LEXWARE_ENABLE_FINALIZE`) | off | Issue **legally binding** finalized documents in one step via the dedicated `create-finalized-*` tools (confirmation-gated); irreversible article deletes; **manage webhook event subscriptions** (create + delete — a webhook streams financial events to an external URL, so it's opt-in). Enabling this tier also enables Drafts. | Set `LEXWARE_READ_ONLY=true` to force read-only (overrides the flags above). @@ -94,7 +94,7 @@ LEXWARE_API_KEY=... MCP_AUTH_TOKEN=... npm start |---|---|---| | `LEXWARE_API_KEY` | — (**required**) | Your Lexware API key ([create one](https://app.lexware.de/addons/public-api)) | | `OAUTH_ISSUER` | — | OAuth authorization-server issuer URL. Setting it enables OAuth mode¹ | -| `OAUTH_RESOURCE` / `SERVER_URL` | — | This server's public URL (token audience / Resource Indicator). Required in OAuth mode | +| `OAUTH_RESOURCE` / `SERVER_URL` | `http://127.0.0.1:$PORT` | This server's public URL. **Required in OAuth mode** (token audience / Resource Indicator), and used in *every* mode to build the upload links `create-upload-ticket` hands out (browser URL and `curl` command). Set it whenever the server is reachable under a real domain — without it those links point at the loopback fallback, which only works on the server itself | | `OAUTH_ALLOWED_EMAIL_DOMAINS` | — | Comma-separated allow-list of email domains (e.g. `example.com`) | | `OAUTH_VERIFY_AUDIENCE` | `true` | Verify the token `aud` matches `OAUTH_RESOURCE`. **Keep `true`.** Setting `false` accepts *any* valid token from the issuer — including one minted for a different app on the same issuer (a confused-deputy risk). Only disable for a dedicated, single-audience issuer that has no Resource Indicator | | `OAUTH_JWKS_URL` / `OAUTH_USERINFO_URL` | derived from issuer | Override the JWKS / OIDC userinfo endpoints (defaults use the WorkOS-AuthKit layout) | @@ -104,6 +104,7 @@ LEXWARE_API_KEY=... MCP_AUTH_TOKEN=... npm start | `LEXWARE_READ_ONLY` | `false` | Register only read tools (hard override) | | `LEXWARE_ENABLE_DRAFTS` | `true` | Enable create-draft tools | | `LEXWARE_ENABLE_FINALIZE` | `false` | Enable finalize / legally-binding tools (also enables Drafts) | +| `LEXWARE_UPLOAD_ALLOWED_HOSTS` | `sharepoint.com,onedrive.live.com,1drv.ms,graph.microsoft.com` | Comma-separated hosts `upload-file-from-url` may download from (the host itself or any subdomain, matched on a dot boundary). **Replaces** the defaults rather than extending them, so you can opt out of them. Set it empty to block every host and effectively disable the tool | | `LEXWARE_API_BASE_URL` | `https://api.lexware.io` | API base URL | | `LEXWARE_APP_BASE_URL` | `https://app.lexware.de` | Web-app base for document deeplinks | | `PORT` | `8080` | Listen port (your platform may inject this) | diff --git a/src/config.ts b/src/config.ts index efad733..d119fb5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -5,6 +5,8 @@ * any Skybridge/Express imports so it can be unit-tested in isolation. */ +import { DEFAULT_ALLOWED_HOSTS } from "./uploads/fetch-url.js"; + /** Minimum length for `MCP_AUTH_TOKEN`. A 32-hex-char token is 32 chars. */ export const MIN_TOKEN_LENGTH = 16; @@ -63,6 +65,24 @@ export interface Config { /** Web-app base for building document deeplinks, e.g. `https://app.lexware.de`. */ lexwareAppBaseUrl: string; auth: AuthConfig; + /** + * This server's public base URL, used to build the `/upload/:ticket` links that + * `create-upload-ticket` hands to a browser and bakes into its curl command. + * + * Deliberately NOT part of {@link AuthConfig}: where this server is reachable is a + * deployment fact, not an auth one. Deriving it from the auth mode — OAuth resource + * or else loopback — meant a static-token deployment behind a real domain (a + * supported mode, see README) handed out `http://127.0.0.1:8080/upload/…`, a link + * that resolves nowhere but inside the container. + */ + publicBaseUrl: string; + /** + * Hosts `upload-file-from-url` may download from (the host itself or any subdomain). + * Defaults to {@link DEFAULT_ALLOWED_HOSTS}; `LEXWARE_UPLOAD_ALLOWED_HOSTS` REPLACES + * that list rather than extending it, so the defaults can be opted out of. An empty + * list blocks every host — fail closed, never "allow all". + */ + uploadAllowedHosts: string[]; port: number; debugLogging: boolean; capabilities: Capabilities; @@ -74,6 +94,26 @@ const DEFAULT_BASE_URL = "https://api.lexware.io"; const DEFAULT_APP_BASE_URL = "https://app.lexware.de"; const DEFAULT_PORT = 8080; +/** + * Resolve the URL-upload host allow-list. + * + * Unset (the variable absent entirely) keeps {@link DEFAULT_ALLOWED_HOSTS}. Any other + * value REPLACES the defaults — extending them would make Microsoft's domains + * impossible to opt out of, which is the wrong default for a self-hosted server. + * + * An explicitly empty value therefore yields an empty list, which blocks every host and + * disables `upload-file-from-url` entirely. That is deliberate: an allow-list that + * cannot be emptied cannot be used to turn the feature off, and "empty means allow + * everything" would turn a typo into an open SSRF surface. + */ +function resolveUploadAllowedHosts(raw: string | undefined): string[] { + if (raw === undefined) return DEFAULT_ALLOWED_HOSTS; + return raw + .split(",") + .map((h) => h.trim().toLowerCase()) + .filter(Boolean); +} + /** Parse a boolean env value. Accepts true/1/yes/on (case-insensitive). */ function parseBool(raw: string | undefined, fallback: boolean): boolean { if (raw === undefined || raw.trim() === "") return fallback; @@ -142,6 +182,28 @@ function validateIssuerUrl(raw: string): string { return value; } +/** + * Resolve this server's public base URL — in EVERY auth mode, not just OAuth. + * + * `OAUTH_RESOURCE` first, so an OAuth deployment can never drift from the value its + * token audience is built from; then `SERVER_URL`, which the README already documents + * as "this server's public URL" and which was previously read only inside the OAuth + * branch (a static-token or unauthenticated deployment set it and it was ignored). + * Only with neither set does the loopback fallback apply: correct for a local run, + * and honest about being unusable anywhere else. + * + * Validated through {@link normalizeUrl} like every other configured URL, so a typo + * or a plain-http public URL fails at startup rather than being pasted into a curl + * command an operator then runs. + */ +function resolvePublicBaseUrl(env: NodeJS.ProcessEnv, port: number): string { + const resource = env.OAUTH_RESOURCE?.trim(); + if (resource) return normalizeUrl(resource, resource, "OAUTH_RESOURCE"); + const serverUrl = env.SERVER_URL?.trim(); + if (serverUrl) return normalizeUrl(serverUrl, serverUrl, "SERVER_URL"); + return `http://127.0.0.1:${port}`; +} + /** Resolve how `/mcp` is authenticated, failing closed if nothing is configured. */ function resolveAuth(env: NodeJS.ProcessEnv): AuthConfig { const issuerRaw = env.OAUTH_ISSUER?.trim(); @@ -239,6 +301,7 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { } const auth = resolveAuth(env); + const port = parsePort(env.PORT); const readOnly = parseBool(env.LEXWARE_READ_ONLY, false); // READ_ONLY is a hard override: it wins over the individual enable flags. @@ -266,7 +329,9 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { "LEXWARE_APP_BASE_URL", ), auth, - port: parsePort(env.PORT), + publicBaseUrl: resolvePublicBaseUrl(env, port), + uploadAllowedHosts: resolveUploadAllowedHosts(env.LEXWARE_UPLOAD_ALLOWED_HOSTS), + port, debugLogging: parseBool(env.LEXWARE_DEBUG_LOGGING, false), capabilities: { read: true, drafts: enableDrafts, finalize: enableFinalize }, warnings, diff --git a/src/server-body-parsing.ts b/src/server-body-parsing.ts new file mode 100644 index 0000000..35a92eb --- /dev/null +++ b/src/server-body-parsing.ts @@ -0,0 +1,71 @@ +import express from "express"; + +/** + * Case-insensitive prefix match on a URL path segment. Express's own routing + * is case-insensitive by default (`app.set("case sensitive routing", ...)` is + * off unless explicitly enabled, and this project never enables it) — so + * `POST /UPLOAD/` and `POST /Mcp` really do reach the same handlers as + * their lowercase spellings. An earlier version of `isMcpPath`/`isUploadPath` + * compared case-sensitively, which meant an uppercase path was routed to the + * real handler but NOT recognized by the body-parsing swap below — for + * `/UPLOAD`, that reopened the gzip-amplification path (Critical 2) up to the + * ~100 KB global-parser limit, since the pre-check + raw-body defenses in + * routes.ts never got a chance to run before the global JSON parser did. + */ +function matchesPathCaseInsensitive(p: string, exact: string, prefix: string): boolean { + const lower = p.toLowerCase(); + return lower === exact || lower.startsWith(prefix); +} + +/** MCP protocol traffic. Bodies are parsed by a raised-limit parser mounted AFTER the auth gate (see server.ts). */ +export const isMcpPath = (p: string): boolean => matchesPathCaseInsensitive(p, "/mcp", "/mcp/"); + +/** + * Ticket-gated upload endpoints (`registerUploadRoutes`). These read the request + * body themselves via `express.raw()` and must never be pre-parsed by the global + * JSON layer — if that layer ran first, `req.body` would already be a parsed + * object (not a `Buffer`) for any `Content-Type: application/json` upload, and a + * naive length guard would treat that as an empty-but-successful upload while + * still consuming the ticket. + */ +export const isUploadPath = (p: string): boolean => matchesPathCaseInsensitive(p, "/upload", "/upload/"); + +/** + * Reconfigure body parsing so large uploads (and the raw-body ticket routes) work + * WITHOUT widening the pre-auth attack surface. Skybridge pre-applies a single + * global `express.json()` (~100 KB default) at router-stack index 0 — before the + * `/mcp` auth middleware AND before the `/upload` ticket routes' own + * `express.raw()`. We swap that layer's handler, in place, so it keeps the + * ~100 KB limit for ordinary routes (e.g. `/status`) but calls `next()` + * immediately — without touching `req.body` at all — for any path `skipPath` + * accepts. + * + * In-place handler swap (no stack reordering) so it can't mis-order routes. + * Guarded: returns `false` if the internal layer can't be located, and the + * caller must treat that as "the swap did not happen" (server.ts warns loudly; + * routes.ts's own `Buffer.isBuffer` guard is the defense-in-depth backstop for + * exactly this case). + * + * Exported (rather than kept private in server.ts) so the upload routes' tests + * can exercise this exact function against a test app shaped like the real + * stack, instead of a parallel reimplementation that could silently drift from + * production — which is precisely how the original `/upload` JSON-body bug + * stayed invisible: the test app never had a global JSON parser to begin with. + */ +export function deferBodyParsingFor(app: express.Express, skipPath: (path: string) => boolean): boolean { + try { + type Layer = { handle?: express.RequestHandler & { name?: string } }; + const router = + (app as unknown as { router?: { stack: Layer[] }; _router?: { stack: Layer[] } }).router ?? + (app as unknown as { _router?: { stack: Layer[] } })._router; + const stack = router?.stack; + if (!Array.isArray(stack)) return false; + const layer = stack.find((l) => l?.handle?.name === "jsonParser"); + if (!layer) return false; + const smallJson = express.json(); // ~100 KB default — for /status and other ordinary routes + layer.handle = (req, res, next) => (skipPath(req.path) ? next() : smallJson(req, res, next)); + return true; + } catch { + return false; + } +} diff --git a/src/server.ts b/src/server.ts index 8e36217..6106df2 100644 --- a/src/server.ts +++ b/src/server.ts @@ -5,40 +5,12 @@ import { ConfigError, describeCapabilities, loadConfig } from "./config.js"; import { LexwareClient } from "./lexware/client.js"; import { buildOAuthMetadata, createAccessTokenVerifier } from "./oauth.js"; import { registerTools } from "./tools/index.js"; +import { deferBodyParsingFor, isMcpPath, isUploadPath } from "./server-body-parsing.js"; +import { registerUploadRoutes } from "./uploads/routes.js"; +import { TicketStore } from "./uploads/tickets.js"; /** Base64 file uploads (upload-file / upload-voucher-file) travel inline in the JSON-RPC body. */ const JSON_BODY_LIMIT = "12mb"; -const isMcpPath = (p: string): boolean => p === "/mcp" || p.startsWith("/mcp/"); - -/** - * Reconfigure body parsing so large uploads work WITHOUT widening the pre-auth - * attack surface. Skybridge pre-applies a single global `express.json()` (~100 KB - * default) at router-stack index 0 — before the /mcp auth middleware. We swap that - * layer's handler, in place, so it keeps the ~100 KB limit for non-/mcp routes (e.g. - * /status) but DEFERS /mcp bodies to a {@link JSON_BODY_LIMIT} parser mounted AFTER - * the auth gate (see below). Net effect: an unauthenticated request can never trigger - * a multi-MB parse, and authenticated uploads still get the raised limit. - * - * In-place handler swap (no stack reordering) so it can't mis-order routes. Guarded: - * returns false if the internal layer can't be located, and the caller warns loudly. - */ -function deferMcpBodyParsing(app: express.Express): boolean { - try { - type Layer = { handle?: express.RequestHandler & { name?: string } }; - const router = - (app as unknown as { router?: { stack: Layer[] }; _router?: { stack: Layer[] } }).router ?? - (app as unknown as { _router?: { stack: Layer[] } })._router; - const stack = router?.stack; - if (!Array.isArray(stack)) return false; - const layer = stack.find((l) => l?.handle?.name === "jsonParser"); - if (!layer) return false; - const smallJson = express.json(); // ~100 KB default — for /status and other non-/mcp routes - layer.handle = (req, res, next) => (isMcpPath(req.path) ? next() : smallJson(req, res, next)); - return true; - } catch { - return false; - } -} // Fail fast with a clear, secret-free message on any misconfiguration. let config; @@ -76,8 +48,12 @@ const server = new McpServer( ); // Defer /mcp bodies from the pre-applied ~100 KB global parser (they get the raised -// limit post-auth, below); other routes keep the small limit. -const bodyParsingConfigured = deferMcpBodyParsing(server.express); +// limit post-auth, below). Also defer /upload bodies — those are read raw by +// registerUploadRoutes' own express.raw(); letting the global JSON parser touch them +// first silently turned a JSON-content-typed upload into an empty file (see +// server-body-parsing.ts and routes.ts for the full story). Other routes keep the +// small limit. +const bodyParsingConfigured = deferBodyParsingFor(server.express, (p) => isMcpPath(p) || isUploadPath(p)); // Unauthenticated health check. Use `/status`, not `/healthz`: Google Front End // intercepts `/healthz` on Cloud Run (it never reaches the container). @@ -118,7 +94,26 @@ if (bodyParsingConfigured) { server.use("/mcp", express.json({ limit: JSON_BODY_LIMIT })); } -registerTools(server, client, config); +// Ticket-gated upload path: bytes go browser/curl -> server -> Lexware, never +// through the model context. Shared store so the MCP tools can issue and read +// tickets that these routes consume. +// +// NOTE: `server` (McpServer) has no get/post/use-as-router surface of its own — +// only a `use()` for middleware. The real Express app lives at `server.express` +// (see node_modules/skybridge/dist/server/server.d.ts: "readonly express: Express" +// with the doc example `server.express.get(...)`), which is also what's already +// used above for /status. A cast of `server` itself to `express.Express` would +// type-check (via `as unknown as`) but fail at runtime — McpServer has no `get`/ +// `post` methods to call. +export const uploadTickets = new TicketStore(); +registerUploadRoutes(server.express, uploadTickets, async ({ bytes, filename, contentType, type }) => + client.postMultipart<{ id: string }>("/v1/files", { bytes, filename, contentType }, { type }), +); + +// publicBaseUrl is resolved in config.ts from OAUTH_RESOURCE/SERVER_URL, independently +// of the auth mode — deriving it here from `config.auth` handed a static-token +// deployment behind a real domain upload links pointing at its own loopback. +registerTools(server, client, config, uploadTickets, config.publicBaseUrl); console.error( `[lexware-mcp] starting — ${describeCapabilities(config)} bodyLimit=${bodyParsingConfigured ? `${JSON_BODY_LIMIT} (/mcp, post-auth)` : "default(~100kb)"}`, @@ -129,7 +124,10 @@ for (const warning of config.warnings) { if (!bodyParsingConfigured) { console.error( "[lexware-mcp] WARNING: could not raise the JSON body limit (Skybridge/Express internals changed) — " + - "uploads over ~100 KB will be rejected. upload-file/upload-voucher-file may fail until this is fixed.", + "uploads over ~100 KB will be rejected. upload-file/upload-voucher-file may fail until this is fixed. " + + "The /upload/:ticket endpoints are ALSO affected: the global ~100 KB JSON parser stays in front of " + + "them instead of being skipped, so a ticket-gated upload over ~100 KB fails there too (routes.ts's " + + "own Buffer.isBuffer guard still prevents a silent empty upload, but the request itself will 400/413).", ); } if (config.auth.mode === "oauth" && config.auth.allowedEmailDomains.length === 0) { diff --git a/src/tools/index.ts b/src/tools/index.ts index c1c1bb9..787fb3f 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -1,6 +1,7 @@ import type { McpServer } from "skybridge/server"; import type { Config } from "../config.js"; import type { LexwareClient } from "../lexware/client.js"; +import type { TicketStore } from "../uploads/tickets.js"; import { registerArticleDeleteTools, registerArticleReadTools, @@ -20,13 +21,20 @@ import { import { registerFileReadTools, registerFileWriteTools } from "./files.js"; import { registerProfileTools } from "./profile.js"; import { registerReferenceReadTools } from "./reference.js"; +import { registerUploadTools } from "./uploads.js"; import { registerVoucherWriteTools } from "./vouchers.js"; /** * Register MCP tools according to the resolved capability tiers. Only enabled * tiers are registered — a disabled tool is never advertised to the model. */ -export function registerTools(server: McpServer, client: LexwareClient, config: Config): void { +export function registerTools( + server: McpServer, + client: LexwareClient, + config: Config, + uploadTickets: TicketStore, + publicBaseUrl: string, +): void { const { capabilities } = config; // Read tier — always on. @@ -45,6 +53,7 @@ export function registerTools(server: McpServer, client: LexwareClient, config: registerDocumentDraftTools(server, client); registerVoucherWriteTools(server, client); registerFileWriteTools(server, client); + registerUploadTools(server, client, uploadTickets, publicBaseUrl, config.uploadAllowedHosts); } // Finalize / sensitive & irreversible tier (off by default). diff --git a/src/tools/uploads.ts b/src/tools/uploads.ts new file mode 100644 index 0000000..cf54b4a --- /dev/null +++ b/src/tools/uploads.ts @@ -0,0 +1,197 @@ +import type { McpServer } from "skybridge/server"; +import { z } from "zod"; +import type { LexwareClient } from "../lexware/client.js"; +import { DEFAULT_ALLOWED_HOSTS, fetchRemoteFile } from "../uploads/fetch-url.js"; +import type { TicketState, TicketStore } from "../uploads/tickets.js"; +import { text, WRITE } from "./shared.js"; + +/** + * A media type safe to paste into a single-quoted shell argument: RFC 9110 token + * characters only, `type/subtype`. Anything else — most importantly a `'` — is + * refused rather than escaped, because the value comes from the model and the + * result is a command a human is told to run. Refusing drops the header and lets + * the server's own fallback chain decide; escaping would keep a hostile value in + * a command line, one quoting mistake away from executing. + */ +const MEDIA_TYPE_RE = /^[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]*\/[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]*$/; + +/** + * Builds the ready-to-run curl command for the local-file path. + * + * Exactly ONE thing to replace — the `FILE=` path. Name and type are NOT derived + * in the shell; they are precomputed HERE, from what the model already knew when + * it issued the ticket. Two measured defects killed the shell-derivation form: + * + * 1. `-H "X-Filename: $(basename "$FILE")"` sends raw bytes, and a header value + * is Latin-1 on the wire: `Rechnung Müller.pdf` reached Lexware as + * `Rechnung Müller.pdf`, `Beleg – Januar 2026.pdf` as `Beleg â Januar + * 2026.pdf`. Exactly the failure class already closed for the browser — so + * the same lock is used: `X-Filename-B64`, base64url of the UTF-8 bytes, + * computed server-side. base64url output is `[A-Za-z0-9_-]` and therefore + * always safe inside single quotes. + * 2. `$(file -b --mime-type "$FILE")` made `file(1)` a silent prerequisite. It + * is absent on this host (and on slim containers generally): the substitution + * printed `file: command not found`, the header went out EMPTY, and the + * receipt was filed as `application/octet-stream` — while curl still reported + * success with a file id. + * + * Without `filename`/`mimeType` the respective header is OMITTED entirely rather + * than guessed. The server then falls back to the ticket's own values and finally + * to `upload.bin` / `application/octet-stream` — the same outcome as before, but + * without a foreign dependency and without a header that promises something false. + */ +export function buildCurlCommand(uploadUrl: string, file: { filename?: string; mimeType?: string } = {}): string { + const headers: string[] = []; + const mimeType = file.mimeType?.trim(); + if (mimeType && MEDIA_TYPE_RE.test(mimeType)) { + headers.push(` -H 'Content-Type: ${mimeType}'`); + } + const filename = file.filename?.trim(); + if (filename) { + headers.push(` -H 'X-Filename-B64: ${Buffer.from(filename, "utf8").toString("base64url")}'`); + } + // The placeholder path is SINGLE-QUOTED, and that is not cosmetic: measured by + // running this command literally, `FILE=/path with spaces/file.pdf` fails at + // the ASSIGNMENT ("spaces/file.pdf: command not found") — before curl ever starts. The + // naive edit is to paste a real path over the placeholder, and real paths contain + // spaces, so the quotes have to already be there. `"$FILE"` is likewise quoted at + // every expansion, and the URL stays single-quoted so the ticket value can never + // be re-interpreted by the shell. + return `FILE='/path/to/file.pdf'; curl -sS -X POST '${uploadUrl}'${headers.join("")} --data-binary @"$FILE"`; +} + +/** Pure: builds the client-facing shape of a freshly issued ticket. Unit-tested. */ +export function buildTicketResponse( + state: Pick, + publicBaseUrl: string, +): { ticket: string; uploadUrl: string; curlCommand: string; expiresAt: string } { + const base = publicBaseUrl.replace(/\/+$/, ""); + const uploadUrl = `${base}/upload/${state.ticket}`; + return { + ticket: state.ticket, + uploadUrl, + // The ticket's own filename/mimeType are the values the model supplied — the + // command gets them baked in, so the local path is the only thing to edit. + curlCommand: buildCurlCommand(uploadUrl, { filename: state.filename, mimeType: state.mimeType }), + expiresAt: new Date(state.expiresAt).toISOString(), + }; +} + +export function registerUploadTools( + server: McpServer, + client: LexwareClient, + store: TicketStore, + publicBaseUrl: string, + /** Hosts `upload-file-from-url` may download from (see Config.uploadAllowedHosts). */ + allowedHosts: string[] = DEFAULT_ALLOWED_HOSTS, +): void { + server.registerTool( + { + name: "create-upload-ticket", + description: + "Issue a short-lived, single-use upload ticket so a file can reach Lexware WITHOUT its bytes passing " + + "through the model context (unlike upload-file, which needs base64). Returns a browser URL for " + + "drag-and-drop and a ready-to-run curl command — run the curl locally where the file lives, replacing " + + "ONLY the FILE=/path/to/file.pdf path at the front; nothing else must be edited. " + + "PASS filename AND mimeType whenever you know them: they are baked into the command as headers, so the " + + "receipt is filed under its real name (umlauts, dashes and quotes included) and its real type. Omit " + + "them and the file lands in the bookkeeping generically named 'upload.bin' as application/octet-stream. " + + "The command needs no extra tools installed. It prints the Lexware file id; in the browser case, " + + "read it afterwards with get-upload-result. Valid 15 minutes, usable once.", + inputSchema: { + filename: z + .string() + .optional() + .describe( + "The file's real name, e.g. \"Rechnung Müller.pdf\". Baked into the curl command and used as the " + + "fallback if the upload itself carries none. Without it the receipt is named upload.bin.", + ), + mimeType: z + .string() + .optional() + .describe( + 'Content type, e.g. "application/pdf" or "image/jpeg". Baked into the curl command and used as the ' + + "fallback if the upload carries none. Without it the receipt is filed as application/octet-stream.", + ), + type: z.string().default("voucher").describe('Lexware file category. "voucher" for bookkeeping receipts.'), + }, + annotations: WRITE, + }, + async ({ filename, mimeType, type }: { filename?: string; mimeType?: string; type: string }) => { + const state = store.create({ type, filename, mimeType }); + const out = buildTicketResponse(state, publicBaseUrl); + return { + structuredContent: out, + content: text( + `Upload ticket ready (valid until ${out.expiresAt}).\nBrowser: ${out.uploadUrl}\nLocal file: ${out.curlCommand}`, + ), + }; + }, + ); + + server.registerTool( + { + name: "get-upload-result", + description: + "Read the Lexware file id produced by an upload ticket. Use after a browser drag-and-drop; the curl " + + "path already prints the id itself. Returns pending=true while nothing has been uploaded yet.", + inputSchema: { ticket: z.string().describe("The ticket from create-upload-ticket.") }, + annotations: WRITE, + }, + async ({ ticket }: { ticket: string }) => { + const state = store.peek(ticket); + if (!state) { + return { + structuredContent: { pending: false, expired: true }, + content: text("Ticket is unknown or expired. Issue a new one with create-upload-ticket."), + }; + } + if (!state.result) { + return { + structuredContent: { pending: true }, + content: text("Nothing uploaded yet. The file has not arrived."), + }; + } + return { + structuredContent: { pending: false, ...state.result }, + content: text(`Uploaded file ${state.result.fileId} (${state.result.filename}, ${state.result.byteLength} bytes).`), + }; + }, + ); + + server.registerTool( + { + name: "upload-file-from-url", + description: + "Download a file from a pre-authenticated download link and store it in Lexware, without the bytes " + + "passing through the model context — e.g. a share link to a file in cloud storage. Only https URLs " + + "on a host in the server's allow-list are accepted (the host itself or a subdomain), re-checked " + + "after every redirect; arbitrary URLs are refused by design. The operator configures the list via " + + "LEXWARE_UPLOAD_ALLOWED_HOSTS; it defaults to sharepoint.com, onedrive.live.com, 1drv.ms and " + + "graph.microsoft.com. If a URL is refused, the error names the host — do not retry other hosts. " + + "Limit 20 MB.", + inputSchema: { + url: z + .string() + .describe("Public https URL of the file. Must be on the server's configured host allow-list."), + filename: z.string().optional().describe("Overrides the name derived from the response."), + mimeType: z.string().optional().describe("Overrides the content type from the response."), + type: z.string().default("voucher").describe('Lexware file category. "voucher" for bookkeeping receipts.'), + }, + annotations: WRITE, + }, + async ({ url, filename, mimeType, type }: { url: string; filename?: string; mimeType?: string; type: string }) => { + const fetched = await fetchRemoteFile(url, { allowedHosts }); + const name = filename ?? fetched.filename ?? new URL(url).pathname.split("/").pop() ?? "download.bin"; + const created = await client.postMultipart<{ id: string }>( + "/v1/files", + { bytes: fetched.bytes, filename: name, contentType: mimeType ?? fetched.contentType }, + { type }, + ); + return { + structuredContent: { fileId: created.id, filename: name, byteLength: fetched.bytes.byteLength }, + content: text(`Uploaded file ${created.id} (${name}, ${fetched.bytes.byteLength} bytes) from ${url}.`), + }; + }, + ); +} diff --git a/src/uploads/fetch-url.ts b/src/uploads/fetch-url.ts new file mode 100644 index 0000000..86b04ed --- /dev/null +++ b/src/uploads/fetch-url.ts @@ -0,0 +1,398 @@ +import { lookup as dnsLookup } from "node:dns/promises"; +import { isIPv4, isIPv6 } from "node:net"; + +const DEFAULT_MAX_BYTES = 20 * 1024 * 1024; +const DEFAULT_TIMEOUT_MS = 30_000; +const DEFAULT_MAX_REDIRECTS = 3; + +/** + * Hosts a pre-authenticated download link is allowed to point at. The real use case is + * OneDrive/SharePoint share links: restricting to Microsoft's own domains makes DNS + * rebinding irrelevant for the default configuration, because an attacker who does not + * control DNS for these domains cannot rebind them to an internal address. Callers may + * override via `fetchRemoteFile`'s `allowedHosts` option. + */ +export const DEFAULT_ALLOWED_HOSTS = ["sharepoint.com", "onedrive.live.com", "1drv.ms", "graph.microsoft.com"]; + +/** + * True when `hostname` is exactly one of `allowed`, or a subdomain of one of them + * (matched on a dot boundary — "evilsharepoint.com" must NOT match "sharepoint.com"). + * Case-insensitive; a trailing dot on `hostname` (a valid DNS root-label terminator) is + * stripped before comparison. + */ +export function isAllowedHost(hostname: string, allowed: string[]): boolean { + let host = hostname.trim().toLowerCase(); + if (host.endsWith(".")) host = host.slice(0, -1); + for (const entry of allowed) { + const suffix = entry.trim().toLowerCase(); + if (!suffix) continue; + if (host === suffix || host.endsWith(`.${suffix}`)) return true; + } + return false; +} + +/** Parses a dotted-decimal IPv4 string into its four octets. */ +function parseIPv4Octets(ip: string): [number, number, number, number] | null { + const parts = ip.split("."); + if (parts.length !== 4) return null; + const nums = parts.map((p) => Number(p)); + if (nums.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) return null; + return nums as [number, number, number, number]; +} + +/** + * Range checks shared between plain IPv4 addresses and IPv4 addresses embedded in an + * IPv6 literal (mapped, compatible, or NAT64). Covers loopback, the RFC 1918 private + * ranges, link-local (incl. the 169.254.169.254 cloud metadata endpoint), carrier-grade + * NAT (100.64.0.0/10), IETF protocol assignments, benchmarking, multicast and the + * reserved/broadcast block. + */ +function isBlockedIPv4Bytes(a: number, b: number, c: number, _d: number): boolean { + if (a === 0 || a === 127) return true; + if (a === 10) return true; + if (a === 172 && b >= 16 && b <= 31) return true; + if (a === 192 && b === 168) return true; + if (a === 169 && b === 254) return true; + if (a === 100 && b >= 64 && b <= 127) return true; + if (a === 192 && b === 0 && c === 0) return true; + if (a === 198 && (b === 18 || b === 19)) return true; + if (a >= 224 && a <= 239) return true; + if (a >= 240) return true; // 240.0.0.0/4 reserved, includes 255.255.255.255 + return false; +} + +/** + * Expands an IPv6 literal (RFC 4291 text form, including `::` compression and an + * embedded trailing IPv4 dotted-quad group) into its 16 bytes. Returns null for + * anything that doesn't parse as exactly 8 groups — callers must treat null as blocked, + * not as "not an address". + */ +function parseIPv6ToBytes(ip: string): number[] | null { + const percentIdx = ip.indexOf("%"); + const text = percentIdx === -1 ? ip : ip.slice(0, percentIdx); + + const dcIdx = text.indexOf("::"); + const hasDoubleColon = dcIdx !== -1; + const leftPart = hasDoubleColon ? text.slice(0, dcIdx) : text; + const rightPart = hasDoubleColon ? text.slice(dcIdx + 2) : ""; + + const leftGroups = leftPart === "" ? [] : leftPart.split(":"); + const rightGroups = rightPart === "" ? [] : rightPart.split(":"); + + // An embedded IPv4 dotted-quad, if present, is always the final group. + const target = rightGroups.length > 0 ? rightGroups : leftGroups; + if (target.length > 0 && target[target.length - 1].includes(".")) { + const v4 = target.pop()!; + const octets = parseIPv4Octets(v4); + if (!octets) return null; + const [o0, o1, o2, o3] = octets; + target.push(((o0 << 8) | o1).toString(16), ((o2 << 8) | o3).toString(16)); + } + + let allGroups: string[]; + if (!hasDoubleColon) { + if (leftGroups.length !== 8) return null; + allGroups = leftGroups; + } else { + const known = leftGroups.length + rightGroups.length; + if (known > 8) return null; + allGroups = [...leftGroups, ...new Array(8 - known).fill("0"), ...rightGroups]; + } + if (allGroups.length !== 8) return null; + + const bytes: number[] = []; + for (const g of allGroups) { + if (!/^[0-9a-fA-F]{1,4}$/.test(g)) return null; + const n = parseInt(g, 16); + bytes.push((n >> 8) & 0xff, n & 0xff); + } + return bytes; +} + +/** + * Range checks over the 16-byte form of an IPv6 address. Any address whose first 96 bits + * (mapped: first 80 bits + ffff; compatible: first 96 bits) are zero-with-ffff-marker or + * fully zero is really an IPv4 address in disguise and is delegated to the IPv4 check — + * this is what makes `::ffff:127.0.0.1`, `::ffff:a9fe:a9fe`, `::127.0.0.1`, `::1` and `::` + * (in any of their hex/dotted/expanded spellings) fold onto the same, already-correct + * IPv4 logic instead of needing to be special-cased by string pattern. + */ +function isBlockedIPv6Bytes(bytes: number[]): boolean { + const first10Zero = bytes.slice(0, 10).every((b) => b === 0); + if (first10Zero && bytes[10] === 0xff && bytes[11] === 0xff) { + return isBlockedIPv4Bytes(bytes[12], bytes[13], bytes[14], bytes[15]); // ::ffff:a.b.c.d + } + const first12Zero = bytes.slice(0, 12).every((b) => b === 0); + if (first12Zero) { + return isBlockedIPv4Bytes(bytes[12], bytes[13], bytes[14], bytes[15]); // ::a.b.c.d, ::1, :: + } + // 64:ff9b::/96 — well-known NAT64 prefix; block wholesale rather than trusting + // whatever IPv4 address is embedded in the low 32 bits. + if ( + bytes[0] === 0x00 && + bytes[1] === 0x64 && + bytes[2] === 0xff && + bytes[3] === 0x9b && + bytes.slice(4, 12).every((b) => b === 0) + ) { + return true; + } + if (bytes[0] === 0x20 && bytes[1] === 0x02) return true; // 2002::/16 (6to4) + if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0x80) return true; // fe80::/10 link-local + if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0xc0) return true; // fec0::/10 deprecated site-local + if ((bytes[0] & 0xfe) === 0xfc) return true; // fc00::/7 unique local + return false; +} + +/** + * True for addresses a server-side fetch must never reach: loopback, private, + * link-local (including the 169.254.169.254 cloud metadata endpoint), carrier-grade + * NAT, multicast/reserved, and their IPv6 equivalents — including every IPv4-in-IPv6 + * spelling (mapped, compatible, NAT64) and non-canonical hex/expanded forms. Checked + * for EVERY hop, not just the first — a public URL is free to redirect somewhere + * internal. + * + * Fails closed: anything that is not a syntactically valid IPv4 or IPv6 address + * (including the empty string) counts as blocked rather than as "not an address" — + * an allow-by-default parser is exactly the kind of gap DNS rebinding and exotic + * address spellings exploit. + */ +export function isBlockedAddress(rawIp: string): boolean { + const trimmed = rawIp.trim(); + if (!trimmed) return true; + if (isIPv4(trimmed)) { + const octets = parseIPv4Octets(trimmed); + if (!octets) return true; + return isBlockedIPv4Bytes(...octets); + } + if (isIPv6(trimmed)) { + const bytes = parseIPv6ToBytes(trimmed); + if (!bytes) return true; + return isBlockedIPv6Bytes(bytes); + } + return true; +} + +export function assertFetchableUrl(raw: string): URL { + let url: URL; + try { + url = new URL(raw); + } catch { + throw new Error(`Not a valid URL: ${raw}`); + } + if (url.protocol !== "https:") { + throw new Error(`URL scheme ${url.protocol} is not allowed — only https.`); + } + return url; +} + +async function defaultLookup(host: string): Promise { + const records = await dnsLookup(host, { all: true }); + return records.map((r) => r.address); +} + +/** + * Longest single path component virtually every filesystem in use accepts (ext4, XFS, + * APFS, NTFS all stop at 255). Those limits count BYTES, this counts UTF-16 code units, + * so a name of non-ASCII characters can still exceed 255 bytes downstream — deliberate: + * the point of the cap is to bound what goes into a multipart field and a log line, not + * to promise any particular filesystem will take it. Cutting at code units keeps every + * legitimate name (an umlaut is 1 unit) untouched. + */ +const MAX_FILENAME_LENGTH = 255; + +/** + * Reduces a filename to its basename and drops path separators — this module is a + * trust boundary: the name arrives from a foreign server's `Content-Disposition` (or, + * via routes.ts, from a request header) and ends up in a multipart field, in the + * bookkeeping, and in everything that logs it. + * + * Also strips C0 control characters and DEL, and only then trims. Trimming alone left + * anything in the MIDDLE of the name intact — measured: + * `filename*=UTF-8''evil%0D%0Ainjected.pdf` decodes to `evil\r\ninjected.pdf` and came + * through with its CRLF, ready to forge a line in any log that writes the name out. + * The characters are removed rather than replaced, so nothing is invented: a name that + * consists only of them comes back `undefined` and the caller's existing fallback + * chain (URL basename / ticket value / `upload.bin`) picks a real name. + * + * Length is capped at {@link MAX_FILENAME_LENGTH} by truncation rather than rejection — + * an over-long name is still a usable upload, just a shortened one. A truncation that + * would split a surrogate pair drops the orphaned half instead of leaving a lone + * surrogate that encodes as U+FFFD. + */ +export function sanitizeFilename(name: string): string | undefined { + const base = name.split(/[/\\]/).pop() ?? ""; + const cleaned = base.replace(/[\u0000-\u001f\u007f]/g, "").trim(); + if (!cleaned) return undefined; + if (cleaned.length <= MAX_FILENAME_LENGTH) return cleaned; + const cut = cleaned.slice(0, MAX_FILENAME_LENGTH); + const lastUnit = cut.charCodeAt(cut.length - 1); + const isHighSurrogate = lastUnit >= 0xd800 && lastUnit <= 0xdbff; + return (isHighSurrogate ? cut.slice(0, -1) : cut).trimEnd(); +} + +/** + * Parses a Content-Disposition header for a filename. Per RFC 6266, `filename*` + * (percent-encoded, RFC 5987 extended value syntax) takes precedence over plain + * `filename` when both are present. Only the `filename*` form is percent-decoded — + * applying `decodeURIComponent` to the plain form as well was the bug: a completely + * ordinary name like `100% Rabatt.pdf` would throw a URIError on the lone `%` and fail + * the whole download *after* it had already succeeded. Decoding is wrapped in try/catch + * so a malformed extended value degrades to the raw string instead of failing the call. + * + * The extended value's full grammar is `charset'language'percent-encoded-value` + * (RFC 5987 §3.2.1) and the language part is OPTIONAL BUT COMMONLY SET — German + * servers routinely send `filename*=UTF-8'de'Rechnung.pdf`. Matching only the + * literal `UTF-8''` prefix, as this did, left `UTF-8'de'` glued to the front and + * filed the receipt as `UTF-8'de'Rechnung.pdf`. Both delimiters are therefore + * split off generically; a value with no apostrophes at all (malformed, but seen + * in the wild) is still taken verbatim rather than dropped. Percent-decoding + * always assumes UTF-8: a non-UTF-8 charset label is rare enough that the + * try/catch fallback to the raw string is the better trade against carrying a + * transcoder. + */ +function filenameFromDisposition(value: string | null): string | undefined { + if (!value) return undefined; + + const extMatch = /filename\*\s*=\s*([^;]+)/i.exec(value); + if (extMatch) { + let raw = extMatch[1].trim().replace(/^"|"$/g, ""); + const parts = /^([^']*)'([^']*)'([\s\S]*)$/.exec(raw); + if (parts) raw = parts[3]; + try { + raw = decodeURIComponent(raw); + } catch { + // Malformed percent-encoding: keep the raw value rather than failing the download. + } + return sanitizeFilename(raw); + } + + const plainMatch = /filename\s*=\s*"?([^";]+)"?/i.exec(value); + if (plainMatch) return sanitizeFilename(plainMatch[1].trim()); + + return undefined; +} + +/** Discards a response body we're not going to use so the socket can be released promptly. */ +async function drain(res: Response): Promise { + try { + await res.body?.cancel(); + } catch { + // Best-effort: a body already errored/closed is fine to ignore. + } +} + +/** + * Fetch a remote file with SSRF guards. Redirects are followed manually so every + * hop's host and address can be re-validated before the request is made. + * + * Two layers of defense, checked at every hop: + * 1. Host allowlist (`allowedHosts`, default: Microsoft file-sharing domains). This is + * the primary control — it also makes DNS rebinding moot for the default config, + * since passing the IP check and then having `fetchImpl`/undici re-resolve to a + * different (internal) address on the actual connection is only exploitable if the + * attacker controls DNS for an allowed domain. + * 2. Resolved-address check (`isBlockedAddress`) — defense in depth, not the only lock. + */ +export async function fetchRemoteFile( + rawUrl: string, + opts: { + maxBytes?: number; + timeoutMs?: number; + maxRedirects?: number; + lookup?: (host: string) => Promise; + fetchImpl?: typeof fetch; + allowedHosts?: string[]; + } = {}, +): Promise<{ bytes: Uint8Array; contentType: string; filename?: string }> { + const maxBytes = opts.maxBytes ?? DEFAULT_MAX_BYTES; + const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const maxRedirects = opts.maxRedirects ?? DEFAULT_MAX_REDIRECTS; + const lookup = opts.lookup ?? defaultLookup; + const doFetch = opts.fetchImpl ?? fetch; + const allowedHosts = opts.allowedHosts ?? DEFAULT_ALLOWED_HOSTS; + + let url = assertFetchableUrl(rawUrl); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + + try { + for (let hop = 0; hop <= maxRedirects; hop++) { + if (!isAllowedHost(url.hostname, allowedHosts)) { + throw new Error(`Host ${url.hostname} is not allowed — not on the configured allowlist.`); + } + + const addresses = await lookup(url.hostname); + if (addresses.length === 0) throw new Error(`Host ${url.hostname} did not resolve.`); + // Deliberately no detail about which address: do not leak internal topology. + if (addresses.some(isBlockedAddress)) { + throw new Error(`Target address is not allowed (private, loopback or link-local).`); + } + + const res = await doFetch(url, { redirect: "manual", signal: controller.signal }); + + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get("location"); + if (!location) throw new Error(`Redirect without a location header.`); + await drain(res); + url = assertFetchableUrl(new URL(location, url).toString()); + continue; + } + + if (!res.ok) { + await drain(res); + throw new Error(`Download failed with HTTP ${res.status}.`); + } + + const declared = Number(res.headers.get("content-length") ?? "0"); + if (declared > maxBytes) { + await drain(res); + throw new Error(`File is too large (${declared} bytes, limit ${maxBytes}).`); + } + + // Stream the body and cut it off as soon as maxBytes is exceeded, instead of + // buffering the whole thing via res.arrayBuffer() first — a lying (or absent) + // content-length must not be able to force an unbounded read into memory. + const chunks: Uint8Array[] = []; + let total = 0; + if (res.body) { + const reader = res.body.getReader(); + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + if (!value) continue; + total += value.byteLength; + if (total > maxBytes) { + controller.abort(); + throw new Error(`File is too large (limit ${maxBytes} bytes).`); + } + chunks.push(value); + } + } finally { + try { + await reader.cancel(); + } catch { + // Best-effort. + } + } + } + + const buf = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + buf.set(chunk, offset); + offset += chunk.byteLength; + } + + return { + bytes: buf, + contentType: res.headers.get("content-type")?.split(";")[0].trim() || "application/octet-stream", + filename: filenameFromDisposition(res.headers.get("content-disposition")), + }; + } + throw new Error(`Too many redirects (limit ${maxRedirects}).`); + } finally { + clearTimeout(timer); + } +} diff --git a/src/uploads/page.ts b/src/uploads/page.ts new file mode 100644 index 0000000..cfefcc1 --- /dev/null +++ b/src/uploads/page.ts @@ -0,0 +1,104 @@ +/** + * The browser-side filename encoder, kept as its own exported source string for + * two reasons: it is the only non-trivial logic on the page, and holding it here + * lets the test suite evaluate the EXACT code the browser runs instead of a + * hand-copied twin that could drift. + * + * Why it exists: `fetch()` rejects any header value containing a character above + * U+00FF with a TypeError raised BEFORE the request is sent — so `X-Filename: + * file.name` broke the whole upload for entirely ordinary German filenames like + * `Beleg – Januar 2026.pdf` (en dash, U+2013) or `Rechnung „Mai“.pdf`, and the + * page could only show the raw TypeError. base64url of the UTF-8 bytes is pure + * ASCII, so it always survives the header layer; the server decodes it (see + * `decodeFilenameB64` in routes.ts). + * + * `btoa` takes a "binary string" — one char per byte — so the UTF-8 bytes are + * widened one at a time rather than via `String.fromCharCode(...bytes)`, whose + * spread would blow the argument limit on a pathologically long name. + */ +export const FILENAME_B64_SOURCE = `function filenameB64(name) { + const bytes = new TextEncoder().encode(name); + let bin = ""; + for (const b of bytes) bin += String.fromCharCode(b); + return btoa(bin).replace(/\\+/g, "-").replace(/\\//g, "_").replace(/=+$/, ""); + }`; + +/** + * Minimal drag-and-drop page for the browser path. Deliberately dependency-free + * and self-contained: it posts the raw File body to its own URL and shows the + * resulting Lexware file id, which the model then reads via get-upload-result. + */ +export function uploadPageHtml(ticket: string): string { + const safe = ticket.replace(/[^A-Za-z0-9_-]/g, ""); + return ` + + +Upload a file to Lexware Office + +

Upload a file to Lexware Office

+

The file goes straight to this server and on to Lexware. It never passes through the model context.

+
Drop a file here, or click to choose one
+

Ticket ${safe} · valid for 15 minutes, single use.

+ +
+ +`; +} diff --git a/src/uploads/routes.ts b/src/uploads/routes.ts new file mode 100644 index 0000000..a583d9e --- /dev/null +++ b/src/uploads/routes.ts @@ -0,0 +1,295 @@ +import express from "express"; +import { LexwareApiError } from "../lexware/errors.js"; +import { sanitizeFilename } from "./fetch-url.js"; +import { uploadPageHtml } from "./page.js"; +import { TicketError, TicketStore, type TicketState } from "./tickets.js"; + +const DEFAULT_MAX_BYTES = 20 * 1024 * 1024; + +/** Forwards bytes to storage; injected so the routes stay testable without network. */ +export type UploadFn = (args: { + bytes: Uint8Array; + filename: string; + contentType: string; + type: string; +}) => Promise<{ id: string }>; + +/** + * Raised by the route handler itself (as opposed to `TicketError`, raised by + * `store.claim()`) for a request that *did* successfully claim the ticket but is + * otherwise unusable (e.g. an empty body). Thrown rather than handled inline so + * every such failure runs through the single `catch` block below and its + * `release()` call — a `return` from inside the `try` would skip that release and + * strand the ticket `inFlight` for the rest of its TTL. + */ +class UploadRequestError extends Error { + constructor( + message: string, + public readonly status: number, + ) { + super(message); + this.name = "UploadRequestError"; + } +} + +/** + * Mirrors `claim()`'s reject conditions (unknown/expired, or already claimed/ + * completed) as a read-only check, for the two call sites that must NOT claim + * the ticket themselves: `GET` (just renders the page or a 410) and the `POST` + * pre-check below (must reject before the body is read, without taking the + * lock — see `requireClaimableTicket`). `claim()` remains the only place that + * actually sets `inFlight`. + */ +function ticketUsabilityError(state: TicketState | undefined): { status: number; message: string } | undefined { + if (!state) return { status: 410, message: "Upload ticket is unknown or expired. Create a new one." }; + if (state.result || state.inFlight) return { status: 410, message: "Upload ticket was already used." }; + return undefined; +} + +/** + * `req.params.ticket` types as `string | string[]` in some middleware-chain + * positions (Express's ParamsDictionary allows repeated-segment params to be + * arrays) even though this route only ever declares a single `:ticket` + * segment. Normalizes to the plain string in all four call sites below. + */ +function ticketParam(req: express.Request): string { + const t = req.params.ticket; + return Array.isArray(t) ? (t[0] ?? "") : t; +} + +/** + * Rejects a request for an unusable ticket BEFORE `express.raw()` runs, so an + * invalid/expired/already-used ticket never causes the body to be read at all. + * Without this, a request naming a bogus ticket could still make the server + * buffer up to `maxBytes` of attacker-supplied data — worse, WITH gzip framing + * decompressed (see `inflate: false` on the raw parser below) — purely to + * discover the ticket doesn't exist. Uses `peek()`, not `claim()`: this check + * must not itself take the lock, or an aborted/failed request for a valid + * ticket would burn it before the real handler ever ran. + */ +function requireClaimableTicket(store: TicketStore): express.RequestHandler { + return (req, res, next) => { + const err = ticketUsabilityError(store.peek(ticketParam(req))); + if (err) { + res.status(err.status).json({ error: err.message }); + return; + } + next(); + }; +} + +/** + * Ticket-gated upload endpoints. These sit OUTSIDE the OAuth gate on purpose — + * the single-use, short-lived ticket is the credential, so a browser or a plain + * curl can post bytes without holding a long-lived secret. + */ +export function registerUploadRoutes( + app: express.Express, + store: TicketStore, + upload: UploadFn, + maxBytes: number = DEFAULT_MAX_BYTES, +): void { + app.get("/upload/:ticket", (req, res) => { + const err = ticketUsabilityError(store.peek(ticketParam(req))); + if (err) { + res.status(err.status).type("text/plain").send(err.message); + return; + } + res.type("text/html").send(uploadPageHtml(ticketParam(req))); + }); + + app.post( + "/upload/:ticket", + // Cheap, non-claiming rejection for a bad ticket — must run before the body + // is ever touched. See requireClaimableTicket's doc comment. + requireClaimableTicket(store), + // `inflate: false`: refuse to transparently gunzip the body. Without this, a + // `Content-Encoding: gzip` request lets an attacker trade a small wire-size + // body for a much larger buffered one (measured amplification: ~1000x) before + // any limit or ticket check can stop it. `type: () => true` is intentional — + // the endpoint accepts arbitrary declared content-types as opaque bytes; only + // the OUTER json-vs-raw ordering (server-body-parsing.ts) determines whether + // this middleware sees the real bytes or an already-parsed object. + express.raw({ type: () => true, limit: maxBytes, inflate: false }), + async (req, res) => { + // Tracks whether this request actually took the ticket, so only a failure + // AFTER the claim releases it again — a TicketError from claim() itself + // never held it, and releasing then would hand it back to a racing request. + let claimed: string | undefined; + try { + const state = store.claim(ticketParam(req)); + claimed = state.ticket; + + const bytes = req.body; + // Defense in depth, not just a length check: if the global JSON parser + // ever ran before this middleware again (see server-body-parsing.ts), + // `req.body` would be a parsed OBJECT, not a Buffer — `!bytes || + // bytes.length === 0` does not catch that (an object has no `.length`), + // and the upload silently "succeeded" with zero bytes while still + // burning the ticket. Thrown, not returned, so `release()` below runs. + if (!Buffer.isBuffer(bytes) || bytes.length === 0) { + throw new UploadRequestError("Empty body.", 400); + } + + const filename = resolveFilename(req, state); + const contentType = resolveContentType(req, state); + const created = await upload({ + bytes: new Uint8Array(bytes), + filename, + contentType, + type: state.type, + }); + const result = { fileId: created.id, filename, byteLength: bytes.length }; + store.complete(state.ticket, result); + res.json(result); + } catch (err) { + if (claimed) store.release(claimed); + if (err instanceof TicketError || err instanceof UploadRequestError) { + res.status(err.status).json({ error: err.message }); + return; + } + // A rejection BY Lexware is not a failure OF this server. Forwarding its + // status verbatim keeps a refused file type (406) or a rejected size (413) + // out of the 502 bucket, which the operator's runbook reads as "container + // is down" — sending them hunting for an outage that never happened. The + // message already carries Lexware's own wording (see describeErrorBody). + // `status === 0` means a network/transport failure reaching Lexware — that + // IS an upstream-gateway problem and correctly stays 502, as does anything + // outside the valid HTTP error range (res.status would otherwise throw). + if (err instanceof LexwareApiError && err.status >= 400 && err.status < 600) { + res.status(err.status).json({ error: err.message }); + return; + } + res.status(502).json({ error: err instanceof Error ? err.message : String(err) }); + } + }, + ); + + // Catches every failure raised by the middleware above (currently: express.raw's + // own size/encoding errors) and closes it off with a fixed JSON shape. NEVER + // forward to `next(err)` here: that would hand off to Express' default error + // handler, which — outside NODE_ENV=production — renders a full stack trace + // (absolute file paths included) to a caller these routes deliberately leave + // unauthenticated. + app.use("/upload", (err: unknown, _req: express.Request, res: express.Response, _next: express.NextFunction) => { + const status = + (err as { status?: number; statusCode?: number })?.status ?? (err as { statusCode?: number })?.statusCode; + if (status === 413) { + // Deliberately does not name maxBytes: after the global JSON parser is + // deferred for /upload (server-body-parsing.ts), a 413 here always comes + // from THIS limit — but if that deferral ever fails (it's guarded, not + // guaranteed), the ~100 KB global parser could trip first instead, and a + // message naming the 20 MB limit would be actively wrong about what just + // happened. + res.status(413).json({ error: "Uploaded file is too large." }); + return; + } + const safeStatus = typeof status === "number" && status >= 400 && status < 600 ? status : 400; + res.status(safeStatus).json({ error: "Upload failed." }); + }); +} + +/** + * Extracts a single string from an Express/Node header value. `IncomingHttpHeaders` + * types every ordinary header as `string | string[] | undefined` even though, in + * practice, only a handful of special headers (`Set-Cookie` being the main one) + * are ever actually delivered as an array — this just takes the first element for + * those, per Node's own API contract. + * + * Deliberately does NOT split on commas. An earlier version treated a comma as + * "this is Node's folding of a duplicated header, take the first part" — but + * Node folds a genuinely REPEATED header into one comma-joined string at the + * HTTP layer, and a comma is a completely legal character IN a filename + * (`Rechnung, Mai 2026.pdf`) that the browser page sends verbatim via + * `file.name`. Splitting on "," silently truncated real filenames sent by the + * page itself — a worse outcome than the doubled-header case it was meant to + * guard against. A doubled `X-Filename` is a client bug, not an attack: the + * result still goes through `sanitizeFilename` in {@link resolveFilename} + * either way, and an odd combined name is far less harmful than a silently + * truncated one. + */ +export function headerString(value: string | string[] | undefined): string | undefined { + const raw = Array.isArray(value) ? value[0] : value; + return typeof raw === "string" ? raw : undefined; +} + +/** + * Decodes the `X-Filename-B64` header: base64url of the filename's UTF-8 bytes. + * + * Exists because an HTTP header field value is bytes, and `fetch()` in a browser + * throws a TypeError for any header character above U+00FF — so the drag-and-drop + * page could not send perfectly ordinary German filenames containing an en dash + * (`Beleg – Januar 2026.pdf`) or typographic quotes (`Rechnung „Mai“.pdf`). The + * request never left the browser at all. + * + * A SEPARATE header, not a guess: sniffing whether `X-Filename` "looks encoded" + * is ambiguous — `Rechnung 100%20 Rabatt.pdf` and a genuinely base64-looking name + * are both real filenames. The header's presence is the signal; nothing about the + * value is interpreted as one. + * + * Returns `undefined` (never throws) for anything that is not exactly a base64url + * encoding of valid UTF-8, so the caller falls back to `X-Filename`: + * - `Buffer.from(…, "base64url")` is deliberately lenient — it silently DROPS + * characters outside the alphabet and tolerates a truncated final group — so + * the alphabet regex and the re-encode round-trip below are what actually + * reject a malformed value instead of accepting a mangled name. + * - U+FFFD in the decoded string means the bytes were not valid UTF-8 (Node + * substitutes rather than throwing). + * - C0/C7F control characters cannot occur in a legitimate filename and are the + * one thing base64 could smuggle in that a plain header cannot; rejected here + * rather than sanitized, so the fallback chain produces a sane name instead. + */ +export function decodeFilenameB64(value: string): string | undefined { + const trimmed = value.trim(); + if (!/^[A-Za-z0-9_-]+={0,2}$/.test(trimmed)) return undefined; + const unpadded = trimmed.replace(/=+$/, ""); + const bytes = Buffer.from(unpadded, "base64url"); + if (bytes.length === 0) return undefined; + if (bytes.toString("base64url") !== unpadded) return undefined; + const decoded = bytes.toString("utf8"); + if (decoded.includes("\uFFFD")) return undefined; + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f\u007f]/.test(decoded)) return undefined; + return decoded; +} + +/** + * Filename precedence, decided at exactly this line: sanitized `X-Filename-B64` + * (see {@link decodeFilenameB64}) if it decodes, then sanitized `X-Filename`, + * then the ticket's own fallback, then a fixed default. `sanitizeFilename` + * (from fetch-url.ts, the same trust-boundary helper used for download links) + * reduces to a basename — so a header like `../../../../etc/cron.d/evil.sh` + * cannot escape the upload's own scope — and, importantly here, its return type + * is `string | undefined`: it NEVER returns `""` (an empty or unsanitizable name + * comes back as `undefined`). That makes `??` exactly right for this chain — + * unlike {@link resolveContentType}, where the RAW declared value genuinely can + * be `""` after stripping parameters, `fromHeader`/`fromTicket` here can only + * ever be a non-empty string or `undefined`, so there is no empty-string case + * for `||` to catch that `??` would miss. + * + * `X-Filename-B64` wins over a simultaneously sent `X-Filename` because only the + * encoded form can carry the full name — a client that sends both is either the + * page (which sends only the encoded one) or a client that deliberately added it. + * An undecodable `X-Filename-B64` yields `undefined`, so the chain simply moves + * on to `X-Filename`: a bad encoding degrades, it never fails the upload. + */ +function resolveFilename(req: express.Request, state: TicketState): string { + const b64 = headerString(req.headers["x-filename-b64"]); + const decoded = b64 !== undefined ? decodeFilenameB64(b64) : undefined; + const fromB64 = decoded !== undefined ? sanitizeFilename(decoded) : undefined; + const header = headerString(req.headers["x-filename"]); + const fromHeader = header !== undefined ? sanitizeFilename(header) : undefined; + const fromTicket = state.filename !== undefined ? sanitizeFilename(state.filename) : undefined; + return fromB64 ?? fromHeader ?? fromTicket ?? "upload.bin"; +} + +/** + * Content-type precedence: the request's own `Content-Type` (parameters + * stripped and the result TRIMMED before the emptiness check — `; charset=utf-8` + * alone must count as empty, not as a value), then the ticket's fallback, then a + * fixed default. `||`, not `??`, for the same reason as {@link resolveFilename}. + */ +function resolveContentType(req: express.Request, state: TicketState): string { + const declared = req.headers["content-type"]?.split(";")[0]?.trim(); + return declared || state.mimeType || "application/octet-stream"; +} diff --git a/src/uploads/tickets.ts b/src/uploads/tickets.ts new file mode 100644 index 0000000..3aacc8d --- /dev/null +++ b/src/uploads/tickets.ts @@ -0,0 +1,132 @@ +import { randomBytes } from "node:crypto"; + +/** Result of a completed upload, handed back to the model as a short id. */ +export type UploadResult = { fileId: string; filename: string; byteLength: number }; + +export type TicketState = { + ticket: string; + /** Lexware file category, fixed when the ticket is issued. */ + type: string; + /** Fallbacks used only when the upload itself carries no filename/content-type. */ + filename?: string; + mimeType?: string; + expiresAt: number; + /** Set once the upload succeeded; presence also marks the ticket as consumed. */ + result?: UploadResult; + /** + * Set synchronously by claim() and held for the (necessarily async) duration of + * reading the body and calling the Lexware API. Blocks a second claim() on the + * same ticket — a retried request, a duplicated proxy call, or a leaked ticket — + * from also writing a voucher. Cleared by release() on failure or by complete() + * on success. + */ + inFlight?: boolean; +}; + +/** Carries the HTTP status the route should answer with. */ +export class TicketError extends Error { + constructor( + message: string, + public readonly status: number, + ) { + super(message); + this.name = "TicketError"; + } +} + +const DEFAULT_TTL_MS = 15 * 60_000; + +/** + * In-memory, single-instance ticket store. A restart drops open tickets, which is + * acceptable at a 15-minute TTL and surfaces as a clear 410 rather than a hang. + */ +export class TicketStore { + private readonly tickets = new Map(); + + constructor( + private readonly ttlMs: number = DEFAULT_TTL_MS, + private readonly now: () => number = Date.now, + ) {} + + create(opts: { type: string; filename?: string; mimeType?: string }): TicketState { + this.sweep(); + const state: TicketState = { + ticket: randomBytes(24).toString("base64url"), + type: opts.type, + filename: opts.filename, + mimeType: opts.mimeType, + expiresAt: this.now() + this.ttlMs, + }; + this.tickets.set(state.ticket, state); + return state; + } + + /** + * Returns the ticket for an upload, or throws 410 when unusable. Marks the ticket + * as in-flight synchronously (before any `await` in the caller) so a second, + * overlapping claim() on the same ticket — retry, duplicated proxy request, a + * leaked ticket — is rejected instead of also writing a voucher. On failure the + * caller must call release() to allow a retry with the same ticket. + */ + claim(ticket: string): TicketState { + const state = this.tickets.get(ticket); + if (!state || state.expiresAt <= this.now()) { + // Drop it the moment it is seen to be expired, instead of leaving it for the + // next create(): sweep() runs only there, so an instance that issues tickets + // and then goes quiet would hold every expired entry for as long as it lives. + // Answer is unchanged either way — 410. + if (state) this.tickets.delete(ticket); + throw new TicketError("Upload ticket is unknown or expired. Create a new one.", 410); + } + if (state.result || state.inFlight) { + throw new TicketError("Upload ticket was already used.", 410); + } + state.inFlight = true; + return state; + } + + /** + * Releases the in-flight lock after a failed upload so the same ticket can be + * claimed again. No-op for an unknown ticket and for one that already completed + * (result is the lock from that point on; release() must not undo it). + */ + release(ticket: string): void { + const state = this.tickets.get(ticket); + if (state && !state.result) state.inFlight = false; + } + + complete(ticket: string, result: UploadResult): void { + // Silent no-op for an unknown ticket: intentional. This method is only ever + // called with a ticket claim() just returned, so an unknown ticket here means a + // wiring bug in the route layer, not a normal runtime path worth surfacing to + // the caller as an exception. + const state = this.tickets.get(ticket); + if (state) { + state.result = result; + state.inFlight = false; + } + } + + /** + * Lookup that keeps working after the ticket was consumed. Not entirely read-only: + * like claim(), it evicts an entry it finds expired (same reason). Return value is + * unchanged — `undefined`. + */ + peek(ticket: string): TicketState | undefined { + const state = this.tickets.get(ticket); + if (!state) return undefined; + if (state.expiresAt <= this.now()) { + this.tickets.delete(ticket); + return undefined; + } + return state; + } + + /** Bulk cleanup on create(); claim() and peek() additionally evict what they touch. */ + private sweep(): void { + const t = this.now(); + for (const [key, state] of this.tickets) { + if (state.expiresAt <= t) this.tickets.delete(key); + } + } +} diff --git a/tests/config.test.ts b/tests/config.test.ts index f12686d..ee3469a 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { ConfigError, describeCapabilities, loadConfig } from "../src/config.js"; +import { DEFAULT_ALLOWED_HOSTS, isAllowedHost } from "../src/uploads/fetch-url.js"; const TOKEN = "a".repeat(40); const base = () => ({ LEXWARE_API_KEY: "key", MCP_AUTH_TOKEN: TOKEN }) as NodeJS.ProcessEnv; @@ -133,6 +134,75 @@ describe("loadConfig", () => { expect(c.warnings).toEqual([]); }); + it("defaults the upload host allow-list to the built-in list", () => { + expect(loadConfig(base()).uploadAllowedHosts).toEqual(DEFAULT_ALLOWED_HOSTS); + }); + + it("LEXWARE_UPLOAD_ALLOWED_HOSTS replaces the defaults rather than extending them", () => { + const c = loadConfig({ + ...base(), + LEXWARE_UPLOAD_ALLOWED_HOSTS: "files.example.com, Cdn.Example.Org , ", + } as NodeJS.ProcessEnv); + // Lower-cased, trimmed, blanks dropped — and the Microsoft defaults are GONE, which + // is the point: an operator must be able to opt out of them. + expect(c.uploadAllowedHosts).toEqual(["files.example.com", "cdn.example.org"]); + expect(c.uploadAllowedHosts).not.toContain("sharepoint.com"); + }); + + it("an explicitly empty LEXWARE_UPLOAD_ALLOWED_HOSTS blocks every host (fail closed)", () => { + const c = loadConfig({ ...base(), LEXWARE_UPLOAD_ALLOWED_HOSTS: "" } as NodeJS.ProcessEnv); + expect(c.uploadAllowedHosts).toEqual([]); + // Empty must mean "nothing allowed", never "everything allowed". + expect(isAllowedHost("sharepoint.com", c.uploadAllowedHosts)).toBe(false); + expect(isAllowedHost("files.example.com", c.uploadAllowedHosts)).toBe(false); + }); + + it("resolves the public base URL from SERVER_URL in EVERY auth mode, not just OAuth", () => { + // The bug this pins down: publicBaseUrl used to be derived from the auth mode — + // the OAuth resource, else loopback — so a static-token (or unauthenticated) + // deployment behind a real domain set SERVER_URL, had it ignored, and handed out + // upload links pointing at its own container. + const staticMode = loadConfig({ ...base(), SERVER_URL: "https://mcp.example.com/lexware" } as NodeJS.ProcessEnv); + expect(staticMode.auth.mode).toBe("static"); + expect(staticMode.publicBaseUrl).toBe("https://mcp.example.com/lexware"); + + const noAuth = loadConfig({ + LEXWARE_API_KEY: "k", + MCP_ALLOW_UNAUTHENTICATED: "true", + SERVER_URL: "https://mcp.example.com", + } as NodeJS.ProcessEnv); + expect(noAuth.auth.mode).toBe("none"); + expect(noAuth.publicBaseUrl).toBe("https://mcp.example.com"); + }); + + it("prefers OAUTH_RESOURCE over SERVER_URL and matches the OAuth resource exactly", () => { + const c = loadConfig({ + LEXWARE_API_KEY: "k", + OAUTH_ISSUER: "https://auth.example.com", + OAUTH_RESOURCE: "https://mcp.example.com/", + SERVER_URL: "https://other.example.com", + } as NodeJS.ProcessEnv); + // Normalized the same way (trailing slash stripped) and never drifting from the + // value the token audience is checked against. + expect(c.publicBaseUrl).toBe("https://mcp.example.com"); + expect(c.publicBaseUrl).toBe((c.auth as { resource: string }).resource); + }); + + it("falls back to loopback on the CONFIGURED port when no public URL is set", () => { + expect(loadConfig(base()).publicBaseUrl).toBe("http://127.0.0.1:8080"); + expect(loadConfig({ ...base(), PORT: "9443" } as NodeJS.ProcessEnv).publicBaseUrl).toBe("http://127.0.0.1:9443"); + }); + + it("validates SERVER_URL like every other configured URL", () => { + // A typo must fail at startup, not end up in a curl command an operator runs. + expect(() => loadConfig({ ...base(), SERVER_URL: "not a url" } as NodeJS.ProcessEnv)).toThrow(ConfigError); + expect(() => loadConfig({ ...base(), SERVER_URL: "http://mcp.example.com" } as NodeJS.ProcessEnv)).toThrow(/https/); + // http on loopback stays allowed (local runs). + expect(loadConfig({ ...base(), SERVER_URL: "http://localhost:8080" } as NodeJS.ProcessEnv).publicBaseUrl).toBe( + "http://localhost:8080", + ); + }); + it("rejects an invalid PORT", () => { expect(() => loadConfig({ ...base(), PORT: "0" } as NodeJS.ProcessEnv)).toThrow(ConfigError); expect(() => loadConfig({ ...base(), PORT: "nope" } as NodeJS.ProcessEnv)).toThrow(ConfigError); diff --git a/tests/server-body-parsing.test.ts b/tests/server-body-parsing.test.ts new file mode 100644 index 0000000..d1f8492 --- /dev/null +++ b/tests/server-body-parsing.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { isMcpPath, isUploadPath } from "../src/server-body-parsing.js"; + +describe("isMcpPath", () => { + it("matches the lowercase path and its subpaths", () => { + expect(isMcpPath("/mcp")).toBe(true); + expect(isMcpPath("/mcp/")).toBe(true); + expect(isMcpPath("/mcp/some/nested/path")).toBe(true); + }); + + it("matches any casing, because Express routes case-insensitively by default", () => { + // Regression: an earlier version compared case-sensitively, so an uppercase + // request WAS routed to the real /mcp handler (Express's own default) but + // NOT recognized here — the global JSON parser stayed in front of it, + // silently truncating the raised /mcp body limit for that spelling. + expect(isMcpPath("/MCP")).toBe(true); + expect(isMcpPath("/Mcp/tools")).toBe(true); + expect(isMcpPath("/mCp")).toBe(true); + }); + + it("does not match an unrelated path, including one that merely starts with the same letters", () => { + expect(isMcpPath("/mcpx")).toBe(false); + expect(isMcpPath("/status")).toBe(false); + expect(isMcpPath("/")).toBe(false); + }); +}); + +describe("isUploadPath", () => { + it("matches the lowercase path and its subpaths", () => { + expect(isUploadPath("/upload")).toBe(true); + expect(isUploadPath("/upload/")).toBe(true); + expect(isUploadPath("/upload/abc123")).toBe(true); + }); + + it("matches any casing, because Express routes case-insensitively by default", () => { + // Regression (Fix-Runde 2, "Minor"): POST /UPLOAD/ WAS routed to the + // real upload handler by Express, but this predicate said "not an upload + // path" and let the global ~100 KB JSON parser run first instead of being + // skipped — reopening a bounded slice of the Critical-2 gzip-amplification + // path up to that parser's own limit. + expect(isUploadPath("/UPLOAD/abc123")).toBe(true); + expect(isUploadPath("/Upload/abc123")).toBe(true); + expect(isUploadPath("/UPLOAD")).toBe(true); + }); + + it("does not match an unrelated path, including one that merely starts with the same letters", () => { + expect(isUploadPath("/uploads")).toBe(false); + expect(isUploadPath("/status")).toBe(false); + expect(isUploadPath("/")).toBe(false); + }); +}); diff --git a/tests/tools.test.ts b/tests/tools.test.ts index bf9fe88..a4486ef 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { type Config, loadConfig } from "../src/config.js"; import type { LexwareClient } from "../src/lexware/client.js"; import { registerTools } from "../src/tools/index.js"; +import { TicketStore } from "../src/uploads/tickets.js"; const READ_TOOLS = [ "get-profile", @@ -60,6 +61,10 @@ const DRAFT_TOOLS = [ "update-voucher", "upload-voucher-file", "upload-file", + // expansion: ticket-gated / URL-based upload, no base64 through the model context + "create-upload-ticket", + "get-upload-result", + "upload-file-from-url", ]; const FINALIZE_TOOLS = [ "create-finalized-invoice", @@ -85,7 +90,7 @@ function registeredNames(config: Config): string[] { return fakeServer; }, } as unknown as McpServer; - registerTools(fakeServer, {} as unknown as LexwareClient, config); + registerTools(fakeServer, {} as unknown as LexwareClient, config, new TicketStore(), "https://mcp.example.test"); return names.sort(); } diff --git a/tests/uploads-fetch-url.test.ts b/tests/uploads-fetch-url.test.ts new file mode 100644 index 0000000..ccd6e24 --- /dev/null +++ b/tests/uploads-fetch-url.test.ts @@ -0,0 +1,553 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_ALLOWED_HOSTS, + fetchRemoteFile, + isAllowedHost, + isBlockedAddress, + sanitizeFilename, +} from "../src/uploads/fetch-url.js"; + +describe("isBlockedAddress", () => { + it("blocks loopback, private and link-local ranges", () => { + for (const ip of [ + "127.0.0.1", "127.53.1.9", "::1", + "10.0.0.5", "172.16.4.2", "172.31.255.255", "192.168.1.1", + "169.254.169.254", "fe80::1", "fc00::1", "fd12::9", + "0.0.0.0", + ]) { + expect(isBlockedAddress(ip), ip).toBe(true); + } + }); + + it("allows public addresses", () => { + for (const ip of ["8.8.8.8", "1.1.1.1", "93.184.216.34", "172.32.0.1", "2606:4700::1111"]) { + expect(isBlockedAddress(ip), ip).toBe(false); + } + }); + + // Regression coverage for Critical review finding: isBlockedAddress previously failed + // OPEN on anything it could not parse as a plain dotted-quad or a small set of string + // prefixes. All of these are real, routable spellings of loopback/link-local addresses + // that were measured as NOT blocked before this fix. + it("blocks non-canonical IPv4-in-IPv6 spellings (hex-form mapped, expanded, deprecated-compatible) and fails closed on unparseable input", () => { + for (const ip of [ + "::ffff:7f00:1", // 127.0.0.1, mapped, hex form (not dotted) + "::FFFF:7F00:1", // same, uppercase + "0:0:0:0:0:ffff:127.0.0.1", // 127.0.0.1, mapped, fully expanded + dotted tail + "::ffff:a9fe:a9fe", // 169.254.169.254, mapped, hex form + "::127.0.0.1", // 127.0.0.1, deprecated IPv4-compatible form + "0:0:0:0:0:0:0:1", // ::1 fully expanded + "", // empty string must fail closed, not fail open + ]) { + expect(isBlockedAddress(ip), JSON.stringify(ip)).toBe(true); + } + }); + + it("blocks the additionally required ranges: CGNAT, IETF protocol assignment, benchmarking, multicast, reserved/broadcast, 6to4, NAT64 and deprecated site-local", () => { + for (const ip of [ + "100.64.0.1", "100.127.255.255", // 100.64.0.0/10 (CGNAT) + "192.0.0.5", // 192.0.0.0/24 (IETF protocol assignments) + "198.18.0.1", "198.19.255.255", // 198.18.0.0/15 (benchmarking) + "224.0.0.1", "239.255.255.255", // 224.0.0.0/4 (multicast) + "240.0.0.1", "255.255.255.255", // 240.0.0.0/4 (reserved, incl. broadcast) + "fec0::1", // fec0::/10 (deprecated site-local) + "64:ff9b::808:808", // 64:ff9b::/96 (NAT64 well-known prefix) + "2002:c000:0204::", // 2002::/16 (6to4) + ]) { + expect(isBlockedAddress(ip), ip).toBe(true); + } + }); +}); + +describe("isAllowedHost", () => { + it("matches an exact configured host and a subdomain of it", () => { + expect(isAllowedHost("sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("foo.sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("contoso.sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + }); + + it("does not match a lookalike domain that merely shares a suffix without a dot boundary", () => { + expect(isAllowedHost("evilsharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + expect(isAllowedHost("sharepoint.com.evil.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + expect(isAllowedHost("not-allowed.example.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + }); + + it("is case-insensitive and strips a trailing root-label dot", () => { + expect(isAllowedHost("FOO.SharePoint.COM", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("sharepoint.com.", DEFAULT_ALLOWED_HOSTS)).toBe(true); + }); + + it("a configured list replaces the defaults instead of extending them", () => { + // LEXWARE_UPLOAD_ALLOWED_HOSTS must be able to opt OUT of the built-in hosts, + // otherwise a self-hosted server can never stop trusting Microsoft's domains. + const custom = ["files.example.com"]; + expect(isAllowedHost("files.example.com", custom)).toBe(true); + expect(isAllowedHost("sharepoint.com", custom)).toBe(false); + expect(isAllowedHost("contoso.sharepoint.com", custom)).toBe(false); + }); + + it("an empty list blocks everything (fail closed, never allow-all)", () => { + expect(isAllowedHost("sharepoint.com", [])).toBe(false); + expect(isAllowedHost("files.example.com", [])).toBe(false); + // A list of only blanks is the same as empty, not a wildcard. + expect(isAllowedHost("sharepoint.com", ["", " "])).toBe(false); + }); +}); + +describe("fetchRemoteFile", () => { + const publicLookup = async () => ["93.184.216.34"]; + + it("rejects non-https schemes", async () => { + await expect(fetchRemoteFile("file:///etc/passwd", { lookup: publicLookup })).rejects.toThrow(/scheme/i); + }); + + it("rejects plain http, even on an allow-listed host", async () => { + await expect(fetchRemoteFile("http://foo.sharepoint.com/x.pdf", { lookup: publicLookup })).rejects.toThrow( + /scheme/i, + ); + }); + + it("rejects a redirect from https down to http", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + seen.push(String(url)); + return new Response(null, { status: 302, headers: { location: "http://foo.sharepoint.com/downgraded.pdf" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://foo.sharepoint.com/start.pdf", { lookup: publicLookup, fetchImpl }), + ).rejects.toThrow(/scheme/i); + expect(seen).toHaveLength(1); + }); + + it("rejects a host that is not on the allowlist", async () => { + await expect( + fetchRemoteFile("https://not-allowed.example.com/x.pdf", { lookup: publicLookup }), + ).rejects.toThrow(/not allowed/i); + }); + + it("allows a host on the default allowlist through", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { "content-type": "application/pdf" }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://foo.sharepoint.com/x.pdf", { lookup: publicLookup, fetchImpl }); + expect(Array.from(out.bytes)).toEqual([1, 2, 3]); + }); + + it("an explicitly empty allowlist blocks a default host (`??`, not `||`)", async () => { + // Guards the operator's kill switch: `opts.allowedHosts || DEFAULT_ALLOWED_HOSTS` + // would silently fall back to the Microsoft defaults for an empty array and quietly + // re-enable a feature the operator disabled. + await expect( + fetchRemoteFile("https://foo.sharepoint.com/x.pdf", { lookup: publicLookup, allowedHosts: [] }), + ).rejects.toThrow(/not allowed/i); + }); + + // Same scenario as the original brief test, adapted for the allowlist-first model: + // the allowlist is now checked first, so a host must be explicitly permitted for this + // test to actually exercise the IP layer (defense in depth) rather than being rejected + // one layer earlier for an unrelated reason. + it("rejects a host that resolves to a private address, even when it is allowlisted (defense in depth)", async () => { + await expect( + fetchRemoteFile("https://internal.example.com/x.pdf", { + lookup: async () => ["10.1.2.3"], + allowedHosts: ["internal.example.com"], + }), + ).rejects.toThrow(/not allowed/i); + }); + + // Same DNS-rebinding scenario as the original brief test. ok.example.com and the literal + // metadata IP are explicitly allowlisted here so the test isolates the IP-layer re-check + // (this is what stops the rebinding attack), rather than the redirect being rejected one + // layer earlier by the host allowlist. + // Both hops use https and both hosts are allowlisted, so neither the scheme check nor + // the host allowlist can be what stops this — only the per-hop IP re-check can. The + // assertion targets the IP layer's specific message (not the generic /not allowed/i, + // which also matches the scheme-rejection and host-allowlist-rejection strings and so + // would pass even with the IP check deleted — see the mutation note in the task report). + it("re-checks the address after a redirect, even when the redirect target is https and allowlisted", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + const u = String(url); + seen.push(u); + if (u.endsWith("/start.pdf")) { + return new Response(null, { + status: 302, + headers: { location: "https://internal.example.com/latest/meta-data" }, + }); + } + return new Response("should never be reached", { status: 200 }); + }) as unknown as typeof fetch; + const lookup = async (host: string) => + host === "internal.example.com" ? ["169.254.169.254"] : ["93.184.216.34"]; + await expect( + fetchRemoteFile("https://ok.example.com/start.pdf", { + lookup, + fetchImpl, + allowedHosts: ["ok.example.com", "internal.example.com"], + }), + ).rejects.toThrow(/private, loopback or link-local/i); + expect(seen).toHaveLength(1); + }); + + it("rejects a redirect from an allowlisted host to a host that is not allowlisted, before the second request goes out", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + seen.push(String(url)); + return new Response(null, { status: 302, headers: { location: "https://evil.example.com/payload" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://foo.sharepoint.com/start.pdf", { lookup: publicLookup, fetchImpl }), + ).rejects.toThrow(/not allowed/i); + expect(seen).toHaveLength(1); + }); + + it("stops after the redirect limit", async () => { + const fetchImpl = (async (url: string | URL) => + new Response(null, { status: 302, headers: { location: `https://ok.example.com/${Math.random()}` } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/a", { + lookup: publicLookup, + fetchImpl, + maxRedirects: 3, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/redirect/i); + }); + + it("enforces the default redirect limit of 3 when maxRedirects is not given", async () => { + let calls = 0; + const fetchImpl = (async () => { + calls++; + return new Response(null, { status: 302, headers: { location: "https://ok.example.com/next" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/a", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/redirect/i); + // hops 0..3 inclusive = DEFAULT_MAX_REDIRECTS (3) + 1 attempts. + expect(calls).toBe(4); + }); + + it("rejects a body larger than maxBytes", async () => { + const big = new Uint8Array(1024); + const fetchImpl = (async () => + new Response(big, { status: 200, headers: { "content-type": "application/pdf" } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/big.pdf", { + lookup: publicLookup, + fetchImpl, + maxBytes: 100, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + }); + + it("enforces the default maxBytes of 20 MiB via content-length when maxBytes is not given", async () => { + const tooLarge = 20 * 1024 * 1024 + 1; + const fetchImpl = (async () => + new Response(new Uint8Array(0), { + status: 200, + headers: { "content-type": "application/pdf", "content-length": String(tooLarge) }, + })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/huge.pdf", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + }); + + it("aborts a streamed body without a truthful content-length once maxBytes is exceeded, without reading it fully", async () => { + const chunkSize = 1024; + const totalChunks = 100; // 100 * 1024 = 102400 bytes if the whole stream were drained + let pulled = 0; + const stream = new ReadableStream({ + pull(controller) { + if (pulled >= totalChunks) { + controller.close(); + return; + } + pulled++; + controller.enqueue(new Uint8Array(chunkSize)); + }, + }); + const fetchImpl = (async () => + new Response(stream, { status: 200, headers: { "content-type": "application/pdf" } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/big.pdf", { + lookup: publicLookup, + fetchImpl, + maxBytes: 2048, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + // Proof it stopped streaming early rather than buffering the whole body first. + expect(pulled).toBeLessThan(totalChunks); + }); + + it("applies the timeout globally across multiple redirects, not per hop", async () => { + let hopCount = 0; + const fetchImpl = ((_url: string | URL, init?: { signal?: AbortSignal }) => { + hopCount++; + return new Promise((resolve, reject) => { + const signal = init?.signal; + if (signal?.aborted) { + reject(Object.assign(new Error("aborted"), { name: "AbortError" })); + return; + } + const t = setTimeout(() => { + resolve( + new Response(null, { + status: 302, + headers: { location: `https://ok.example.com/hop-${hopCount}` }, + }), + ); + }, 8); + signal?.addEventListener("abort", () => { + clearTimeout(t); + reject(Object.assign(new Error("aborted"), { name: "AbortError" })); + }); + }); + }) as unknown as typeof fetch; + + await expect( + fetchRemoteFile("https://ok.example.com/start", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + timeoutMs: 40, + maxRedirects: 100, + }), + ).rejects.toThrow(/abort/i); + // If the timeout were reset on every hop instead of being global, all 101 attempts + // (maxRedirects=100 + 1) would run to completion and the call would fail with "too + // many redirects" instead of aborting on elapsed time. + expect(hopCount).toBeLessThan(100); + }); + + it("returns bytes, content type and filename from content-disposition", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="beleg-2026.pdf"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(Array.from(out.bytes)).toEqual([1, 2, 3]); + expect(out.contentType).toBe("application/pdf"); + expect(out.filename).toBe("beleg-2026.pdf"); + }); + + it("does not fail on a plain filename containing a literal percent sign", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="100% Rabatt.pdf"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("100% Rabatt.pdf"); + }); + + it("prefers the RFC 5987 extended filename* over the plain filename and decodes it", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename=\"fallback.pdf\"; filename*=UTF-8''beleg%20zwei.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("beleg zwei.pdf"); + }); + + it("strips the RFC 5987 language tag from filename* instead of gluing it onto the name", async () => { + // The extended-value grammar is charset'language'value, and the language + // part is optional but routinely set by German servers. Matching + // only the literal `UTF-8''` prefix left `UTF-8'de'` in front of the name and + // filed the receipt as `UTF-8'de'Rechnung.pdf` — wrong data in the books, + // with nothing failing visibly. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8'de'Rechnung.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung.pdf"); + }); + + it("decodes a percent-encoded filename* carrying a language tag", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8'de'Rechnung%20M%C3%BCller.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung Müller.pdf"); + }); + + it("still decodes filename* with an EMPTY language tag (the no-language form)", async () => { + // The other half of "with and without a language tag": splitting on the two + // apostrophes generically must not break the plain `UTF-8''…` case. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8''Rechnung%20M%C3%BCller.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung Müller.pdf"); + }); + + it("keeps a malformed filename* with no apostrophes at all verbatim", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=Rechnung.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung.pdf"); + }); + + it("reduces a filename to its basename and strips path separators", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="../../etc/passwd"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("passwd"); + }); + + it("strips control characters out of a filename* instead of forwarding them", async () => { + // Measured: `filename*=UTF-8''evil%0D%0Ainjected.pdf` decodes to + // "evil\r\ninjected.pdf", and trimming leaves a CRLF sitting in the MIDDLE of the + // name — which then goes into the multipart field and into every log line built + // from it. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8''evil%0D%0Ainjected.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("evilinjected.pdf"); + expect(out.filename).not.toMatch(/[\u0000-\u001f\u007f]/); + }); +}); + +describe("sanitizeFilename", () => { + it("removes C0 control characters and DEL wherever they sit, not only at the ends", () => { + expect(sanitizeFilename("evil\r\ninjected.pdf")).toBe("evilinjected.pdf"); + expect(sanitizeFilename("beleg\u0000.pdf")).toBe("beleg.pdf"); + expect(sanitizeFilename("beleg\u007f.pdf")).toBe("beleg.pdf"); + expect(sanitizeFilename("\u001bBeleg.pdf")).toBe("Beleg.pdf"); + for (const name of ["a\r\nb.pdf", "a\tb.pdf", "a\u0000b.pdf", "\u007f.pdf"]) { + expect(sanitizeFilename(name)).not.toMatch(/[\u0000-\u001f\u007f]/); + } + }); + + it("returns undefined when nothing usable is left, so the caller's fallback applies", () => { + expect(sanitizeFilename("\u0000")).toBeUndefined(); + expect(sanitizeFilename("\r\n\t")).toBeUndefined(); + expect(sanitizeFilename("\u0000\u007f")).toBeUndefined(); + expect(sanitizeFilename(" ")).toBeUndefined(); + expect(sanitizeFilename("")).toBeUndefined(); + }); + + it("caps an over-long name at 255 without splitting a surrogate pair", () => { + const long = `${"a".repeat(400)}.pdf`; + const capped = sanitizeFilename(long) as string; + expect(capped).toHaveLength(255); + expect(capped).toBe("a".repeat(255)); + + // Cut exactly between the two halves of an emoji: the orphaned half must go too, + // or it encodes as U+FFFD downstream. + const emojiAtTheCut = `${"a".repeat(254)}🧾tail.pdf`; + const cut = sanitizeFilename(emojiAtTheCut) as string; + expect(cut).toBe("a".repeat(254)); + expect(cut).not.toMatch(/[\ud800-\udfff]/); + expect(Buffer.from(cut, "utf8").toString("utf8")).toBe(cut); // no U+FFFD on the way out + }); + + it("leaves legitimate non-ASCII names completely intact", () => { + // The counterpart to the stripping above: umlauts, an en dash and an emoji are + // ordinary filename characters and must survive byte for byte. + for (const name of [ + "Rechnung Müller.pdf", + "Beleg – Januar 2026.pdf", + "Rechnung „Mai“.pdf", + "Quittung 🧾.pdf", + "100% Rabatt.pdf", + "Rechnung, Mai 2026.pdf", + ]) { + expect(sanitizeFilename(name)).toBe(name); + } + // And the pre-existing behaviour is unchanged. + expect(sanitizeFilename("../../etc/passwd")).toBe("passwd"); + expect(sanitizeFilename(" beleg.pdf ")).toBe("beleg.pdf"); + }); +}); diff --git a/tests/uploads-routes.test.ts b/tests/uploads-routes.test.ts new file mode 100644 index 0000000..87b7dc3 --- /dev/null +++ b/tests/uploads-routes.test.ts @@ -0,0 +1,901 @@ +import express from "express"; +import http from "node:http"; +import { describe, expect, it } from "vitest"; +import { LexwareApiError } from "../src/lexware/errors.js"; +import { deferBodyParsingFor, isUploadPath } from "../src/server-body-parsing.js"; +import { TicketStore } from "../src/uploads/tickets.js"; +import { FILENAME_B64_SOURCE, uploadPageHtml } from "../src/uploads/page.js"; +import { decodeFilenameB64, headerString, registerUploadRoutes } from "../src/uploads/routes.js"; + +/** + * Builds an app shaped like the real production stack: a global `express.json()` + * pre-applied at router-stack index 0 (mirroring skybridge's own setup), then the + * SAME `deferBodyParsingFor` swap `server.ts` uses to keep it off `/upload`. A + * naked `express()` — what this file used before — cannot reproduce Critical-1 + * (a JSON-content-typed upload silently becoming an empty file): with no global + * JSON parser in the stack to begin with, `express.raw()` always saw the real + * bytes, so the bug was invisible here even though it fired in production. + */ +function makeApp(store: TicketStore, uploaded: unknown[] = [], maxBytes?: number) { + const app = express(); + app.use(express.json()); + const configured = deferBodyParsingFor(app, isUploadPath); + if (!configured) { + throw new Error("deferBodyParsingFor could not locate the json layer — test setup no longer matches production"); + } + registerUploadRoutes( + app, + store, + async (args) => { + uploaded.push(args); + return { id: "file-123" }; + }, + maxBytes, + ); + return app; +} + +/** A naked app with NO global JSON parser at all — reproduces the pre-server.ts-fix + * precondition (global parser still active on /upload) so routes.ts's own + * `Buffer.isBuffer` guard can be exercised in isolation, without relying on the + * server.ts-side fix also being correct. */ +function makeAppWithoutBodyParsingFix(store: TicketStore, uploaded: unknown[] = []) { + const app = express(); + app.use(express.json()); // deliberately NOT deferred for /upload — simulates the bug's precondition + registerUploadRoutes(app, store, async (args) => { + uploaded.push(args); + return { id: "file-123" }; + }); + return app; +} + +async function listen(app: express.Express): Promise<{ url: string; close: () => Promise }> { + const server = app.listen(0); + await new Promise((r) => server.once("listening", r)); + const port = (server.address() as { port: number }).port; + return { + url: `http://127.0.0.1:${port}`, + close: () => new Promise((r) => server.close(() => r())), + }; +} + +describe("upload routes", () => { + it("serves an HTML page for an open ticket", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`); + expect(res.status).toBe(200); + expect(res.headers.get("content-type")).toMatch(/text\/html/); + expect(await res.text()).toContain(t.ticket); + await s.close(); + }); + + it("answers 410 for an unknown ticket", async () => { + const app = makeApp(new TicketStore()); + const s = await listen(app); + expect((await fetch(`${s.url}/upload/nope`)).status).toBe(410); + await s.close(); + }); + + it("accepts a raw body, forwards it and returns the file id", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "beleg.pdf" }, + body: new Uint8Array([37, 80, 68, 70]), + }); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ fileId: "file-123", filename: "beleg.pdf", byteLength: 4 }); + expect(uploaded).toEqual([ + { bytes: new Uint8Array([37, 80, 68, 70]), filename: "beleg.pdf", contentType: "application/pdf", type: "voucher" }, + ]); + await s.close(); + }); + + it("rejects a second upload on the same ticket with 410", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const send = () => + fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect((await send()).status).toBe(200); + expect((await send()).status).toBe(410); + await s.close(); + }); + + it("rejects an oversized body with 413, without naming the wrong limit", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store, [], 10); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array(64), + }); + expect(res.status).toBe(413); + const body = await res.json(); + expect(body.error).not.toMatch(/\d{2,}/); // no byte-count baked into the message + await s.close(); + }); + + it("releases the ticket when the upload fails, so a retry works", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + let attempt = 0; + // makeApp's own upload fn always succeeds, so this needs its own app wired to a + // failing-then-succeeding upload fn. + const app = express(); + app.use(express.json()); + if (!deferBodyParsingFor(app, isUploadPath)) throw new Error("setup drifted"); + registerUploadRoutes(app, store, async () => { + attempt += 1; + if (attempt === 1) throw new Error("lexware exploded"); + return { id: "file-after-retry" }; + }); + const s = await listen(app); + const send = () => + fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + const first = await send(); + expect(first.status).toBe(502); + expect(first.headers.get("content-type")).toMatch(/application\/json/); + const firstBody = await first.json(); + expect(typeof firstBody.error).toBe("string"); + const second = await send(); + expect(second.status).toBe(200); + expect((await second.json()).fileId).toBe("file-after-retry"); + await s.close(); + }); + + it("falls back to the ticket filename when the upload carries none", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher", filename: "fallback.pdf", mimeType: "application/pdf" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/octet-stream" }, + body: new Uint8Array([9]), + }); + expect((uploaded[0] as { filename: string }).filename).toBe("fallback.pdf"); + await s.close(); + }); + + // --- The global JSON parser must not touch /upload --------------------------- + + it("uploads a JSON-content-typed body byte-accurately in the production-like stack (root-cause fix)", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const payload = JSON.stringify({ hello: "world" }); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/json", "x-filename": "data.json" }, + body: payload, + }); + expect(res.status).toBe(200); + const body = await res.json(); + // Must equal the JSON text's own byte length, not 0 — the historical bug + // silently produced a 0-byte "successful" upload for exactly this request. + expect(body.byteLength).toBe(Buffer.byteLength(payload)); + expect(uploaded).toHaveLength(1); + expect((uploaded[0] as { bytes: Uint8Array }).bytes).toEqual(new Uint8Array(Buffer.from(payload))); + await s.close(); + }); + + it("rejects an already-parsed JSON object body as empty, without consuming the ticket (defense in depth)", async () => { + // Uses makeAppWithoutBodyParsingFix: the global JSON parser is deliberately + // left active on /upload, reproducing the exact precondition of the original + // bug. This proves routes.ts's own Buffer.isBuffer guard alone — independent + // of the server.ts-side fix — turns "silent empty success" into a loud, + // recoverable error. + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeAppWithoutBodyParsingFix(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/json", "x-filename": "data.json" }, + body: JSON.stringify({ hello: "world" }), + }); + expect(res.status).not.toBe(200); + expect(uploaded).toHaveLength(0); + const body = await res.json(); + expect(typeof body.error).toBe("string"); + // Ticket must still be usable — a real (non-JSON) retry succeeds. + const retry = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(retry.status).toBe(200); + await s.close(); + }); + + // --- An invalid ticket must reject before the body is read ------------------- + + it("answers 410 for an unknown ticket without reading the request body", async () => { + // A Fetch API ReadableStream body is the wrong tool here: undici drains it into + // its own internal buffer eagerly, independent of real socket backpressure, so + // a `pull()` counter mostly measures undici's buffering, not the server. Node's + // raw `http.request` doesn't have that problem — `req.write()`'s boolean return + // value IS the real Writable-stream/socket backpressure signal. If the server + // responds without ever consuming the request body, the OS receive/send buffers + // fill up and writing stalls after a small, bounded amount — regardless of how + // much more data is available to send. + const app = makeApp(new TicketStore()); + const s = await listen(app); + const url = new URL(`${s.url}/upload/nope`); + const chunk = Buffer.alloc(65536, 1); // 64 KiB + const totalAvailable = 800; // 50 MB available, if the server actually read it all + + const { status, chunksWrittenBeforeResponse } = await new Promise<{ status: number; chunksWrittenBeforeResponse: number }>( + (resolve, reject) => { + let chunksWritten = 0; + const req = http.request( + { + hostname: url.hostname, + port: url.port, + path: url.pathname, + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + }, + (res) => { + const chunksAtResponse = chunksWritten; + res.resume(); + res.on("end", () => { + // The request is still mid-write (backpressured, hundreds of chunks + // short of totalAvailable) at this point — destroy it explicitly. + // Without this, the still-open client socket keeps the server's + // `close()` (below) waiting forever for a connection that neither + // side is going to finish, and the WHOLE TEST hangs to its timeout + // even though the 410 was received correctly. + req.destroy(); + resolve({ status: res.statusCode ?? 0, chunksWrittenBeforeResponse: chunksAtResponse }); + }); + }, + ); + req.on("error", reject); + function writeNext() { + if (chunksWritten >= totalAvailable) { + req.end(); + return; + } + chunksWritten += 1; + if (req.write(chunk)) setImmediate(writeNext); + else req.once("drain", writeNext); + } + writeNext(); + }, + ); + + expect(status).toBe(410); + // Generous bound: real runs land around 40 chunks (~2.5 MB); asserting well + // below the 800 available (50 MB) is what actually distinguishes "rejected + // early" from "read the whole body first". + expect(chunksWrittenBeforeResponse).toBeLessThan(100); + await s.close(); + }); + + // --- An empty body must release the ticket ----------------------------------- + + it("releases the ticket when the body is empty, so a retry with real bytes works", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const empty = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array(0), + }); + expect(empty.status).toBe(400); + const retry = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(retry.status).toBe(200); + await s.close(); + }); + + // --- || not ??, and Content-Type is trimmed ---------------------------------- + + it("falls back to the ticket filename when X-Filename is empty", async () => { + // Exercises resolveFilename's ONE fallback decision point directly: an empty + // header sanitizes to `undefined` (sanitizeFilename never returns ""), so the + // `?? fromTicket` step is what has to fire here — not a `||`-vs-`??` distinction + // (sanitizeFilename's return type rules "" out entirely, making that distinction + // moot for filenames; see resolveFilename's doc comment). + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher", filename: "fromticket.pdf", mimeType: "image/png" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("fromticket.pdf"); + await s.close(); + }); + + it("falls back to the ticket content-type when Content-Type is empty", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher", filename: "fromticket.pdf", mimeType: "image/png" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { contentType: string }).contentType).toBe("image/png"); + await s.close(); + }); + + it("falls back to the ticket content-type when Content-Type is only a charset parameter", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher", filename: "fromticket.pdf", mimeType: "image/png" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": ";charset=utf-8", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { contentType: string }).contentType).toBe("image/png"); + await s.close(); + }); + + // --- A TicketError from claim() must not release the lock -------------------- + + it("a second concurrent claim (TicketError) does not release the ticket the first request is holding", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + let resolveUpload!: (v: { id: string }) => void; + const uploadPromise = new Promise<{ id: string }>((resolve) => { + resolveUpload = resolve; + }); + const app = express(); + app.use(express.json()); + if (!deferBodyParsingFor(app, isUploadPath)) throw new Error("setup drifted"); + registerUploadRoutes(app, store, async () => uploadPromise); + const s = await listen(app); + + const first = fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + // Give the first request time to claim() synchronously and reach the (pending) upload() call. + await new Promise((r) => setTimeout(r, 100)); + + const second = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "b.pdf" }, + body: new Uint8Array([2]), + }); + expect(second.status).toBe(410); + + // If the second request's failed claim() had wrongly released the lock, a + // third request could claim it too even though the first is still holding it. + const third = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "c.pdf" }, + body: new Uint8Array([3]), + }); + expect(third.status).toBe(410); + + resolveUpload({ id: "file-first" }); + const firstRes = await first; + expect(firstRes.status).toBe(200); + expect((await firstRes.json()).fileId).toBe("file-first"); + await s.close(); + }); + + // --- Default limit ------------------------------------------------------------ + + it("enforces the default 20 MB limit when maxBytes is not passed explicitly", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = express(); + app.use(express.json()); + if (!deferBodyParsingFor(app, isUploadPath)) throw new Error("setup drifted"); + registerUploadRoutes(app, store, async () => ({ id: "x" })); // no maxBytes -> default applies + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array(20 * 1024 * 1024 + 1), + }); + expect(res.status).toBe(413); + await s.close(); + }); + + // --- GET on a consumed ticket ------------------------------------------------- + + it("answers 410 for GET on an already-consumed ticket", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const upload = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(upload.status).toBe(200); + const page = await fetch(`${s.url}/upload/${t.ticket}`); + expect(page.status).toBe(410); + await s.close(); + }); + + // --- Every failure closes off as clean JSON, never a stack trace ------------- + + it("returns a clean JSON error, not a stack trace, when the raw body parser itself fails", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf", "content-encoding": "gzip" }, + body: new Uint8Array([1, 2, 3]), // not actually gzip-compressed; inflate:false rejects it outright + }); + expect(res.status).toBeGreaterThanOrEqual(400); + expect(res.headers.get("content-type")).toMatch(/application\/json/); + const body = await res.json(); + expect(typeof body.error).toBe("string"); + expect(body.error).not.toMatch(/node_modules|\.ts:\d+:\d+|at [A-Za-z]/); + await s.close(); + }); + + // --- X-Filename sanitization -------------------------------------------------- + + it("strips directory components from X-Filename (path traversal)", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/x-sh", "x-filename": "../../../../etc/cron.d/evil.sh" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("evil.sh"); + await s.close(); + }); + + it("passes a comma-containing filename through unmangled, including Node's own folding of a doubled header", async () => { + // Round-1 fix-round-2 regression: an earlier version split X-Filename on "," + // to "take the first of a duplicated header" — but a comma is a completely + // legal filename character (the browser page sends `file.name` verbatim), + // and Node folds a genuinely REPEATED header into exactly this shape + // ("a.pdf, b.pdf") at the HTTP layer. The old code could not tell "one + // legitimate filename containing a comma" from "two duplicated headers" + // and silently truncated the former. Both must now survive whole. + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "Rechnung, Mai 2026.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("Rechnung, Mai 2026.pdf"); + await s.close(); + }); + + it("preserves a doubled X-Filename header's Node-folded comma-joined value as-is (not the pre-comma prefix)", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const headers = new Headers(); + headers.append("content-type", "application/pdf"); + headers.append("x-filename", "a.pdf"); + headers.append("x-filename", "b.pdf"); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + // Node folds the two headers into "a.pdf, b.pdf" before Express ever sees it; + // that whole string is what routes.ts now uses (a comma-truncating "fix" + // would instead produce "a.pdf" here — this assertion is what catches it). + expect((uploaded[0] as { filename: string }).filename).toBe("a.pdf, b.pdf"); + await s.close(); + }); + + it("headerString() takes the first element when a header value arrives as an array (Node API, not comma-related)", () => { + // Node's IncomingHttpHeaders types every ordinary header as string | string[] + // | undefined, but in real HTTP traffic only a handful of special headers + // (Set-Cookie chief among them) are ever actually delivered as an array — a + // genuinely doubled X-Filename folds into ONE string (see the two tests + // above), so this branch is unreachable via a real request and is tested + // directly instead. + expect(headerString(["first.pdf", "second.pdf"])).toBe("first.pdf"); + expect(headerString("plain.pdf")).toBe("plain.pdf"); + expect(headerString(undefined)).toBeUndefined(); + }); + + // --- Case-insensitive routing vs. the body-parsing swap ---------------------- + + it("treats an uppercase /UPLOAD path the same as /upload for the body-parsing swap", async () => { + // Express itself routes /UPLOAD/:ticket to the same handler as /upload/:ticket + // (case-insensitive routing is Express's default) — but isUploadPath used to + // compare case-sensitively, so the swap in server-body-parsing.ts silently did + // NOT skip the global JSON parser for the uppercase spelling, reopening a + // bounded slice of Critical 2 on that path. This exercises the full stack + // exactly like the lowercase Critical-1 test: a JSON-content-typed body must + // still arrive as real, byte-accurate raw bytes, not get parsed-then-ignored. + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const payload = JSON.stringify({ hello: "world" }); + const res = await fetch(`${s.url}/UPLOAD/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/json", "x-filename": "data.json" }, + body: payload, + }); + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.byteLength).toBe(Buffer.byteLength(payload)); + await s.close(); + }); + + // --- inflate: false regression ------------------------------------------------ + + it("rejects a gzip-encoded body without inflating it, and leaves the ticket unclaimed for a retry", async () => { + // Regression test for `inflate: false` on the raw parser: with the default + // (inflate: true), a non-gzip body sent with Content-Encoding: gzip ALSO + // fails, just via a different path (zlib decompression error) — so a test + // that only checks "some 4xx happened" would stay green even if `inflate: + // false` were removed. Asserting the SPECIFIC 415 that body-parser's own + // `contentstream()` throws synchronously (before reading any bytes, see + // node_modules/body-parser/lib/read.js) is what actually distinguishes the + // two: remove `inflate: false` and this becomes a different status. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf", "content-encoding": "gzip" }, + body: new Uint8Array([1, 2, 3]), // not actually gzip-compressed + }); + expect(res.status).toBe(415); + // The rejection happens in express.raw(), before store.claim() ever runs — + // the ticket was never touched, so a normal retry must succeed outright. + const retry = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(retry.status).toBe(200); + await s.close(); + }); +}); + +/** + * Builds an app whose upload fn always throws `err`, so the route's error mapping + * can be exercised on its own. Mirrors makeApp's production-like body-parsing + * stack (global express.json() + deferral) so nothing else differs. + */ +function makeFailingApp(store: TicketStore, err: unknown) { + const app = express(); + app.use(express.json()); + if (!deferBodyParsingFor(app, isUploadPath)) throw new Error("setup drifted"); + registerUploadRoutes(app, store, async () => { + throw err; + }); + return app; +} + +// --- A Lexware rejection is not a server failure -------------------------------- + +describe("upload error mapping", () => { + it("forwards a LexwareApiError's own status instead of reporting 502", async () => { + // Measured before the fix: a 406 from Lexware ("unsupported file type" — a JPG + // or an oversized PDF dropped onto the page) reached the browser as 502. The + // operator's runbook reads 502 on /lexoffice as "container is not running", so + // a user error sent them hunting for an outage that did not exist. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeFailingApp(store, new LexwareApiError(406, "Lexware API 406: unsupported file type")); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "image/jpeg", "x-filename": "foto.jpg" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(406); + // The original wording must survive verbatim, not be replaced by a generic text. + expect((await res.json()).error).toBe("Lexware API 406: unsupported file type"); + await s.close(); + }); + + it("forwards other 4xx rejections too, and still releases the ticket", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeFailingApp(store, new LexwareApiError(413, "Lexware API 413: file too large")); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "gross.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(413); + // Forwarding the status must not skip the release() path: the ticket stays + // usable, so a corrected retry can still go through. + expect(store.peek(t.ticket)?.inFlight).toBe(false); + await s.close(); + }); + + it("keeps 502 for a transport failure reaching Lexware (status 0)", async () => { + // A LexwareApiError with status 0 means the request never got an HTTP answer. + // That IS a gateway problem and must stay 502 — status 0 is also not a value + // res.status() could send. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeFailingApp(store, new LexwareApiError(0, "Lexware API request failed: connect ECONNREFUSED")); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(502); + await s.close(); + }); + + it("keeps 502 for a non-Lexware error", async () => { + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeFailingApp(store, new Error("something else exploded")); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "a.pdf" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(502); + await s.close(); + }); +}); + +// --- Filenames above U+00FF via X-Filename-B64 ---------------------------------- + +/** + * Evaluates the page's OWN encoder source, so these tests drive the exact code the + * browser runs rather than a hand-copied twin that could silently drift from it. + */ +const filenameB64 = new Function(`${FILENAME_B64_SOURCE}; return filenameB64;`)() as (name: string) => string; + +/** The names measured in the review, plus an emoji: all real, everyday German filenames. */ +const NAMES = [ + "Rechnung Müller.pdf", // U+00FC — the only one that worked before + "Beleg – Januar 2026.pdf", // U+2013 en dash — TypeError, "8211 > 255" + "Rechnung „Mai“.pdf", // U+201E / U+201C German quotes — TypeError + "Quittung 🧾.pdf", // astral plane, surrogate pair +]; + +describe("X-Filename-B64", () => { + it("reproduces the failure the encoding exists for: the raw name cannot go in a header", () => { + // Node's fetch stack raises the very same TypeError the browser does ("value + // of 8211 which is greater than 255"), thrown BEFORE the request is sent — + // which is why the page showed a raw TypeError and no request ever left. + expect(() => new Headers({ "X-Filename": "Beleg – Januar 2026.pdf" })).toThrow(TypeError); + expect(() => new Headers({ "X-Filename": "Rechnung „Mai“.pdf" })).toThrow(TypeError); + // The encoded form is pure ASCII, so the same header layer accepts it. + for (const name of NAMES) { + expect(filenameB64(name)).toMatch(/^[A-Za-z0-9_-]+$/); + expect(() => new Headers({ "X-Filename-B64": filenameB64(name) })).not.toThrow(); + } + }); + + for (const name of NAMES) { + it(`round-trips ${JSON.stringify(name)} from the page encoder through the server`, async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename-b64": filenameB64(name) }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe(name); + await s.close(); + }); + } + + it("prefers X-Filename-B64 over a simultaneously sent X-Filename", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { + "content-type": "application/pdf", + "x-filename": "ascii-ersatz.pdf", + "x-filename-b64": filenameB64("Beleg – Januar 2026.pdf"), + }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("Beleg – Januar 2026.pdf"); + await s.close(); + }); + + it("falls back to X-Filename when X-Filename-B64 is not valid base64url (no throw, no 500)", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { + "content-type": "application/pdf", + "x-filename": "echt.pdf", + "x-filename-b64": "not base64!!", + }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("echt.pdf"); + await s.close(); + }); + + it("falls back to the ticket filename when X-Filename-B64 is undecodable and no X-Filename is sent", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher", filename: "fromticket.pdf" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename-b64": "=" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("fromticket.pdf"); + await s.close(); + }); + + it("strips directory components from a decoded X-Filename-B64 (path traversal)", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { + "content-type": "application/x-sh", + "x-filename-b64": filenameB64("../../../../etc/cron.d/evil.sh"), + }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + expect((uploaded[0] as { filename: string }).filename).toBe("evil.sh"); + await s.close(); + }); + + it("decodeFilenameB64 accepts the padded and unpadded forms alike", () => { + expect(decodeFilenameB64(filenameB64("Rechnung Müller.pdf"))).toBe("Rechnung Müller.pdf"); + expect(decodeFilenameB64(Buffer.from("Beleg – Januar.pdf", "utf8").toString("base64url"))).toBe( + "Beleg – Januar.pdf", + ); + // Standard-base64 padding kept by a hand-rolled client. + const padded = Buffer.from("abc.pdf", "utf8").toString("base64"); + expect(padded.endsWith("=")).toBe(true); + expect(decodeFilenameB64(padded)).toBe("abc.pdf"); + }); + + it("decodeFilenameB64 rejects a value that is pure alphabet but not a whole encoding (round-trip lock)", () => { + // "QUJDQ" is five characters, all inside the base64url alphabet, so the + // alphabet check passes — but it is not a complete encoding of anything. + // Buffer.from() silently drops the orphan fifth character and hands back + // "ABC", a name the caller never sent. ONLY the re-encode comparison catches + // this: the U+FFFD and control-character checks see nothing wrong with "ABC". + expect(Buffer.from("QUJDQ", "base64url").toString("utf8")).toBe("ABC"); // what leniency yields + expect(decodeFilenameB64("QUJDQ")).toBeUndefined(); // what we must answer + // Same shape, one character further along: also incomplete, also rejected. + expect(decodeFilenameB64("UmVjaG51bmc")).toBe("Rechnung"); // 11 chars = a whole 8-byte group + expect(decodeFilenameB64("UmVjaG51bmdz")).toBe("Rechnungs"); // 12 chars, still whole + expect(decodeFilenameB64("UmVjaG51bmdzQ")).toBeUndefined(); // 13: one orphan char + }); + + it("falls back to X-Filename for a pure-alphabet but incomplete X-Filename-B64", async () => { + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const res = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf", "x-filename": "echt.pdf", "x-filename-b64": "QUJDQ" }, + body: new Uint8Array([1]), + }); + expect(res.status).toBe(200); + // Without the round-trip lock this would silently be "ABC". + expect((uploaded[0] as { filename: string }).filename).toBe("echt.pdf"); + await s.close(); + }); + + it("decodeFilenameB64 rejects rather than mangles: bad alphabet, truncation, bad UTF-8, control chars", () => { + // Buffer.from(…, "base64url") is lenient — it silently drops stray characters + // and tolerates a truncated group — so without the alphabet check and the + // re-encode round-trip these would each yield a WRONG name, not an error. + expect(decodeFilenameB64("not base64!!")).toBeUndefined(); + expect(decodeFilenameB64("a")).toBeUndefined(); // truncated: a single base64 char is <1 byte + expect(decodeFilenameB64("")).toBeUndefined(); + expect(decodeFilenameB64(" ")).toBeUndefined(); + // 0xFF 0xFE is not valid UTF-8: Node substitutes U+FFFD instead of throwing. + expect(decodeFilenameB64(Buffer.from([0xff, 0xfe]).toString("base64url"))).toBeUndefined(); + // Control characters are the one thing base64 could smuggle past the header layer. + expect(decodeFilenameB64(Buffer.from("a\r\nb.pdf", "utf8").toString("base64url"))).toBeUndefined(); + expect(decodeFilenameB64(Buffer.from("a\u0001b.pdf", "utf8").toString("base64url"))).toBeUndefined(); + }); + + it("the served page never assigns innerHTML — the file id is built as a text node", () => { + // body.fileId is whatever the Lexware API returned; interpolating it into + // innerHTML made a value from a foreign API the source of this page's markup. + // The styling is kept, the parsing is not. + const html = uploadPageHtml("abc123"); + expect(html).not.toContain("innerHTML"); + expect(html).toContain('document.createElement("code")'); + expect(html).toContain("id.textContent = "); + // No hand-built markup around the id anywhere on the success path either. + expect(html).not.toContain("\" + body.fileId"); + }); + + it("the served page sends the encoded header and never the raw filename", () => { + const html = uploadPageHtml("abc123"); + expect(html).toContain("X-Filename-B64"); + expect(html).toContain("filenameB64(file.name)"); + // The old line `"X-Filename": file.name` is what threw in the browser. + expect(html).not.toContain('"X-Filename": file.name'); + }); +}); diff --git a/tests/uploads-tickets.test.ts b/tests/uploads-tickets.test.ts new file mode 100644 index 0000000..68772fa --- /dev/null +++ b/tests/uploads-tickets.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from "vitest"; +import { TicketError, TicketStore } from "../src/uploads/tickets.js"; + +describe("TicketStore", () => { + it("creates a ticket with a random id and the configured ttl", () => { + let now = 1_000; + const store = new TicketStore(15 * 60_000, () => now); + const a = store.create({ type: "voucher" }); + const b = store.create({ type: "voucher" }); + expect(a.ticket).not.toBe(b.ticket); + expect(a.ticket.length).toBeGreaterThanOrEqual(32); + expect(a.expiresAt).toBe(1_000 + 15 * 60_000); + }); + + it("claims an open ticket and carries its fallbacks", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher", filename: "fallback.pdf", mimeType: "application/pdf" }); + const claimed = store.claim(t.ticket); + expect(claimed.filename).toBe("fallback.pdf"); + expect(claimed.type).toBe("voucher"); + }); + + it("rejects a second claim with 410", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + store.complete(t.ticket, { fileId: "f1", filename: "x.pdf", byteLength: 3 }); + try { + store.claim(t.ticket); + throw new Error("should have thrown"); + } catch (e) { + expect(e).toBeInstanceOf(TicketError); + expect((e as TicketError).status).toBe(410); + } + }); + + it("rejects an unknown ticket with 410", () => { + const store = new TicketStore(60_000, () => 0); + expect(() => store.claim("does-not-exist")).toThrow(TicketError); + }); + + it("rejects an expired ticket with 410", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + now = 60_001; + expect(() => store.claim(t.ticket)).toThrow(TicketError); + }); + + it("still returns the result via peek after the ticket was consumed", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + store.complete(t.ticket, { fileId: "f9", filename: "beleg.pdf", byteLength: 42 }); + expect(store.peek(t.ticket)?.result).toEqual({ fileId: "f9", filename: "beleg.pdf", byteLength: 42 }); + }); + + it("peek returns undefined once the ticket expired", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + store.complete(t.ticket, { fileId: "f9", filename: "b.pdf", byteLength: 1 }); + now = 60_001; + expect(store.peek(t.ticket)).toBeUndefined(); + }); + + it("rejects a second claim while the first is still in flight (no complete yet)", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + try { + store.claim(t.ticket); + throw new Error("should have thrown"); + } catch (e) { + expect(e).toBeInstanceOf(TicketError); + expect((e as TicketError).status).toBe(410); + } + }); + + it("allows a fresh claim after release()", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + store.release(t.ticket); + const claimed = store.claim(t.ticket); + expect(claimed.ticket).toBe(t.ticket); + }); + + it("release() on an unknown ticket is a no-op", () => { + const store = new TicketStore(60_000, () => 0); + expect(() => store.release("does-not-exist")).not.toThrow(); + }); + + // --- expired entries are removed on sight, not only on the next create() ------- + // + // Retention is not directly observable through the store's public API (the Map is + // private and both an evicted and a merely hidden entry answer the same way), so + // these use the injected clock: stepping it BACK past expiresAt — what an NTP + // correction does to a real one — makes the difference visible. An entry that was + // only hidden becomes usable again; an entry that was actually deleted stays gone. + + it("peek() deletes the expired entry it reports as gone", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + now = 60_001; + expect(store.peek(t.ticket)).toBeUndefined(); + now = 0; // clock steps back + expect(store.peek(t.ticket)).toBeUndefined(); + expect(() => store.claim(t.ticket)).toThrow(TicketError); + }); + + it("claim() deletes the expired entry it rejects", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + now = 60_001; + expect(() => store.claim(t.ticket)).toThrow(TicketError); + now = 0; // clock steps back + expect(() => store.claim(t.ticket)).toThrow(TicketError); + expect(store.peek(t.ticket)).toBeUndefined(); + }); + + it("release() after complete() does not make the ticket usable again", () => { + const store = new TicketStore(60_000, () => 0); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + store.complete(t.ticket, { fileId: "f1", filename: "x.pdf", byteLength: 3 }); + store.release(t.ticket); + expect(() => store.claim(t.ticket)).toThrow(TicketError); + }); +}); diff --git a/tests/uploads-tools.test.ts b/tests/uploads-tools.test.ts new file mode 100644 index 0000000..cc85112 --- /dev/null +++ b/tests/uploads-tools.test.ts @@ -0,0 +1,187 @@ +import { execFileSync } from "node:child_process"; +import { describe, expect, it } from "vitest"; +import { loadConfig } from "../src/config.js"; +import { buildCurlCommand, buildTicketResponse } from "../src/tools/uploads.js"; + +const URL_ = "https://mcp.example.com/lexware/upload/abc123"; + +/** Reads back what the command actually declares, instead of matching on prose. */ +function headerValue(cmd: string, name: string): string | undefined { + const m = new RegExp(`-H '${name}: ([^']*)'`).exec(cmd); + return m?.[1]; +} + +describe("buildTicketResponse", () => { + it("builds the browser url and a ready-to-run curl command", () => { + const out = buildTicketResponse( + { ticket: "abc123", type: "voucher", expiresAt: 1_700_000_000_000 }, + "https://mcp.example.com/lexware", + ); + expect(out.uploadUrl).toBe(URL_); + expect(out.curlCommand).toBe(buildCurlCommand(URL_)); + expect(out.curlCommand).toContain(URL_); + expect(out.expiresAt).toBe(new Date(1_700_000_000_000).toISOString()); + }); + + it("does not double a trailing slash on the base url", () => { + const out = buildTicketResponse( + { ticket: "t1", type: "voucher", expiresAt: 0 }, + "https://mcp.example.com/lexware/", + ); + expect(out.uploadUrl).toBe("https://mcp.example.com/lexware/upload/t1"); + }); + + it("bakes the ticket's own filename and mimeType into the command", () => { + // The values the model supplied when issuing the ticket must reach the command + // — that is the whole point of asking for them. A ticket carrying them and a + // command without them would silently file the receipt as upload.bin. + const out = buildTicketResponse( + { + ticket: "abc123", + type: "voucher", + expiresAt: 0, + filename: "Rechnung Müller.pdf", + mimeType: "application/pdf", + }, + "https://mcp.example.com/lexware", + ); + expect(headerValue(out.curlCommand, "Content-Type")).toBe("application/pdf"); + const b64 = headerValue(out.curlCommand, "X-Filename-B64"); + expect(b64).toBeDefined(); + expect(Buffer.from(b64 as string, "base64url").toString("utf8")).toBe("Rechnung Müller.pdf"); + }); +}); + +// --- The URL the operator is actually handed ------------------------------------ + +describe("the public base URL reaching the issued ticket", () => { + const ticket = { ticket: "abc123", type: "voucher", expiresAt: 0 } as const; + const staticEnv = (extra: Record) => + ({ LEXWARE_API_KEY: "k", MCP_AUTH_TOKEN: "a".repeat(40), ...extra }) as NodeJS.ProcessEnv; + + it("uses SERVER_URL in static-token mode — both links, not just one of them", () => { + // The whole failure was invisible in the config: SERVER_URL was read only in the + // OAuth branch, so this deployment served a browser URL and a curl command that + // pointed at the container's own loopback interface. + const config = loadConfig(staticEnv({ SERVER_URL: "https://mcp.example.com/lexware" })); + const out = buildTicketResponse(ticket, config.publicBaseUrl); + expect(out.uploadUrl).toBe("https://mcp.example.com/lexware/upload/abc123"); + expect(out.curlCommand).toContain("https://mcp.example.com/lexware/upload/abc123"); + expect(out.uploadUrl).not.toContain("127.0.0.1"); + expect(out.curlCommand).not.toContain("127.0.0.1"); + }); + + it("falls back to loopback on the CONFIGURED port when no public URL is set", () => { + const config = loadConfig(staticEnv({ PORT: "9443" })); + const out = buildTicketResponse(ticket, config.publicBaseUrl); + expect(out.uploadUrl).toBe("http://127.0.0.1:9443/upload/abc123"); + expect(out.curlCommand).toContain("http://127.0.0.1:9443/upload/abc123"); + }); +}); + +// --- One spot to replace, no mangling, no foreign tools ------------------------- + +describe("buildCurlCommand", () => { + const full = buildCurlCommand(URL_, { filename: "Rechnung Müller.pdf", mimeType: "application/pdf" }); + + it("carries the filename as base64url of its UTF-8 bytes, not as raw bytes", () => { + // Measured with the shell-derivation form: `-H "X-Filename: $(basename + // "$FILE")"` put raw bytes in a header, which is Latin-1 on the wire — + // "Rechnung Müller.pdf" reached Lexware as "Rechnung Müller.pdf" and + // "Beleg – Januar 2026.pdf" as "Beleg â Januar 2026.pdf". Same failure class + // the browser page already solved; same lock used here. + const b64 = headerValue(full, "X-Filename-B64"); + expect(b64).toBeDefined(); + expect(b64).toMatch(/^[A-Za-z0-9_-]+$/); // pure ASCII: safe in a header AND in single quotes + expect(Buffer.from(b64 as string, "base64url").toString("utf8")).toBe("Rechnung Müller.pdf"); + // The raw-byte header must not appear at all. + expect(full).not.toContain("X-Filename: "); + expect(full).not.toContain("basename"); + }); + + it("round-trips every character class that broke the raw header", () => { + for (const name of ["Rechnung Müller.pdf", "Beleg – Januar 2026.pdf", "Rechnung „Mai“.pdf", "Quittung 🧾.pdf"]) { + const cmd = buildCurlCommand(URL_, { filename: name }); + const b64 = headerValue(cmd, "X-Filename-B64") as string; + expect(Buffer.from(b64, "base64url").toString("utf8")).toBe(name); + } + }); + + it("needs no file(1): the content type is a fixed value, never a command substitution", () => { + // Measured on this host: `command -v file` is empty, so + // `$(file -b --mime-type "$FILE")` printed "file: command not found", the + // header went out EMPTY and the receipt was filed as application/octet-stream + // — while curl still reported success with a file id. + expect(headerValue(full, "Content-Type")).toBe("application/pdf"); + expect(full).not.toContain("file -b"); + expect(full).not.toContain("--mime-type"); + // No command substitution anywhere: nothing in this command runs another program. + expect(full).not.toContain("$("); + expect(full).not.toContain("`"); + }); + + it("omits a header entirely rather than guessing when the value is unknown", () => { + const nameOnly = buildCurlCommand(URL_, { filename: "beleg.pdf" }); + expect(headerValue(nameOnly, "X-Filename-B64")).toBeDefined(); + expect(nameOnly).not.toContain("Content-Type"); + + const typeOnly = buildCurlCommand(URL_, { mimeType: "image/jpeg" }); + expect(headerValue(typeOnly, "Content-Type")).toBe("image/jpeg"); + expect(typeOnly).not.toContain("X-Filename-B64"); + + // Neither known: a bare command. The server falls back to the ticket values and + // finally to upload.bin / application/octet-stream — no false promise in between. + const bare = buildCurlCommand(URL_); + expect(bare).not.toContain("-H "); + expect(bare).toBe(`FILE='/path/to/file.pdf'; curl -sS -X POST '${URL_}' --data-binary @"$FILE"`); + }); + + it("leaves exactly ONE spot to replace: the FILE= path", () => { + for (const cmd of [full, buildCurlCommand(URL_), buildCurlCommand(URL_, { filename: "a.pdf" })]) { + expect(cmd.startsWith("FILE='/path/to/file.pdf'; curl ")).toBe(true); + expect(cmd.split("/path/to/file.pdf")).toHaveLength(2); + expect(cmd).toContain('--data-binary @"$FILE"'); + } + }); + + it("quotes the FILE placeholder so a real path with spaces survives the naive edit", () => { + // Found by running the emitted command literally: an UNQUOTED + // `FILE=/path with spaces/file.pdf` fails at the ASSIGNMENT — the shell + // splits at the space and tries to run the rest ("spaces/file.pdf: command not + // found"), before curl ever starts. The instruction is "replace the path in + // place", and real paths contain spaces, so the quotes must already be there. + const spacey = "/home/user/receipts/Rechnung Müller.pdf"; + const edited = full.replace("/path/to/file.pdf", spacey); + const assignment = edited.slice(0, edited.indexOf("; curl ")); + expect(assignment).toBe(`FILE='${spacey}'`); + // Only a real shell can prove this property, so ask one. + const seenByShell = execFileSync("sh", ["-c", `${assignment}; printf %s "$FILE"`], { encoding: "utf8" }); + expect(seenByShell).toBe(spacey); + }); + + it("keeps the url single-quoted so a ticket value can never break out", () => { + expect(full).toContain(`'${URL_}'`); + expect(full).not.toContain(`"${URL_}"`); + const other = buildCurlCommand("https://example.test/lexoffice/upload/ZmFrZS10aWNrZXQ"); + expect(other).toContain("'https://example.test/lexoffice/upload/ZmFrZS10aWNrZXQ'"); + }); + + it("refuses a mimeType that could break out of its single quotes", () => { + // The value comes from the model and the result is a command a human is told + // to run. A quote in it must not end up in the command line at all — dropping + // the header is the safe outcome, the server's fallback still applies. + const hostile = buildCurlCommand(URL_, { mimeType: "application/pdf'; rm -rf ~; echo '" }); + expect(hostile).not.toContain("rm -rf"); + expect(hostile).not.toContain("Content-Type"); + expect(hostile).toBe(`FILE='/path/to/file.pdf'; curl -sS -X POST '${URL_}' --data-binary @"$FILE"`); + // A legitimate type with parameters is not a bare token either — dropped, not mangled. + expect(buildCurlCommand(URL_, { mimeType: "text/plain; charset=utf-8" })).not.toContain("Content-Type"); + // Ordinary types still pass. + expect(headerValue(buildCurlCommand(URL_, { mimeType: "image/jpeg" }), "Content-Type")).toBe("image/jpeg"); + }); + + it("treats a blank filename or mimeType as absent", () => { + const blank = buildCurlCommand(URL_, { filename: " ", mimeType: " " }); + expect(blank).not.toContain("-H "); + }); +});