diff --git a/.env.example b/.env.example index d1d95b1..38e5922 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 # diff --git a/CHANGELOG.md b/CHANGELOG.md index 62236f5..cba754e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,70 @@ 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). +## [0.1.11] + +Upload a receipt without pushing its bytes through the model context. Based on the +contribution by [@gutencoder](https://github.com/gutencoder) ([#34]); the server-side +URL-fetch tool from that PR was intentionally held back (see **Security** below). + +### Added +- **`create-upload-ticket` / `get-upload-result` (drafts tier).** The existing `upload-file` / + `upload-voucher-file` tools 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 travels through the model even though the model only needs the resulting file id. + `create-upload-ticket` issues a short-lived (15 min), single-use ticket and returns a browser URL for + drag-and-drop plus a ready-to-run `curl` command; the bytes go client → server → Lexware and the + model only ever sees the file id. `get-upload-result` reads that id back after a browser upload (the + `curl` path prints it directly). `filename` / `mimeType` travel as `X-Filename-B64` (base64url of the + UTF-8 bytes), so names with an en dash, typographic quotes or an emoji survive a header layer that is + Latin-1 on the wire. +- **`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 `curl` command from it; + a static-token deployment behind a real domain previously had it ignored and handed out loopback + links. Unset, the loopback fallback still applies, on the port actually bound (`__PORT` under + `skybridge dev`, else `PORT`). + +### Changed +- The existing base64 upload tools are unchanged and remain available — the ticket route is additive. +- **Body parsing now also defers `/upload` paths** from the pre-applied global JSON parser, alongside + `/mcp`: the upload route reads the raw body itself, and letting the JSON parser run first turned a + JSON-content-typed upload into an empty file. The route additionally rejects gzip framing + (`inflate: false`), rejects an invalid/expired/used ticket before reading any body, buffers at most + one request body per ticket at a time, and holds a synchronous single-use lock across the upload so + **concurrent or duplicated requests** cannot file a second voucher while an attempt is in flight. + One case is deliberately weaker: after a transport failure whose outcome is unknown (the upload may + or may not have reached Lexware), the ticket is released so a retry stays possible, and the error + says to check for the file before re-uploading — blind retries after such a failure can still + duplicate a receipt, which no client-side lock can prevent without upstream idempotency support. + +A post-integration review pass hardened the details: the upload result stays readable via +`get-upload-result` for a full 15 minutes **after the upload completed** (previously it expired on the +ticket's creation-time clock, so a minute-14 upload left a sub-minute read window and invited a +duplicate); the generated `curl` command pins `Content-Type` explicitly (curl's `--data-binary` +otherwise silently declared `application/x-www-form-urlencoded` and bypassed the documented fallback +chain); a 401/403 from Lexware — the operator's API key being rejected — is answered as a generic 502 +instead of forwarding the upstream status and wording to the unauthenticated uploader; +`get-upload-result` is annotated read-only so polling it doesn't trigger write-tool confirmations; the +loopback link fallback follows `__PORT` under `skybridge dev`; and `OAUTH_RESOURCE` outside OAuth +mode still takes precedence over `SERVER_URL` for upload links, but now announces itself with a +startup warning instead of doing so silently. + +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. The upload route is mounted **only when the drafts capability is enabled** — a read-only +deployment never exposes it — and the ticket page is served `Cache-Control: no-store`. + +### Security +- **The server-side URL-fetch tool (`upload-file-from-url`) from #34 was deliberately NOT included.** + A server-side fetcher is SSRF surface by construction; the version in #34 is guarded by a host + allow-list and per-hop private-address checks but carries a DNS-rebinding TOCTOU — the resolved + address is validated, then the connection re-resolves independently — which is moot for the built-in + Microsoft defaults but live for any custom allow-list. It will be reconsidered separately, with + connection-level IP pinning and disabled by default. The ticket flow above carries no such surface. + +[#34]: https://github.com/marselsel/lexware-mcp/pull/34 + ## [0.1.10] ### Fixed diff --git a/README.md b/README.md index d4e9731..0b84e13 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: +62 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`); 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_AUDIENCE` | — | Comma-separated **additional** accepted `aud` values, on top of `OAUTH_RESOURCE`. For IdPs that ignore the Resource Indicator: Microsoft Entra always puts the API's client ID (a GUID) in `aud`, never the Application ID URI, so without this every token is rejected. Prefer this over `OAUTH_VERIFY_AUDIENCE=false` — the check stays on, just against a value your IdP actually issues. Values are matched **exactly**: they are opaque identifiers, so no normalisation is applied (unlike `OAUTH_RESOURCE`, which also accepts its trailing-slash form) | diff --git a/package-lock.json b/package-lock.json index 04b1b2c..a0a6948 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "lexware-mcp", - "version": "0.1.10", + "version": "0.1.11", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "lexware-mcp", - "version": "0.1.10", + "version": "0.1.11", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.29.0", diff --git a/package.json b/package.json index 1a83441..3124daa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "lexware-mcp", - "version": "0.1.10", + "version": "0.1.11", "private": false, "license": "MIT", "description": "Open-source, self-hostable MCP server for the Lexware Office API", diff --git a/src/config.ts b/src/config.ts index 1391315..e7b15ce 100644 --- a/src/config.ts +++ b/src/config.ts @@ -87,6 +87,17 @@ 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; port: number; debugLogging: boolean; capabilities: Capabilities; @@ -166,6 +177,37 @@ 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"); + // The loopback fallback must name the port the server actually LISTENS on. + // Under `skybridge dev` that is `__PORT` — skybridge picks it itself (~3000) + // and plain PORT is never consulted; using `port` (default 8080) there handed + // out links refusing connections on the very machine the fallback exists for. + // `npm start` is unaffected: server.ts copies config.port into __PORT only + // AFTER config is loaded, so __PORT is present here only when something else + // (skybridge dev, or an operator) chose the bound port explicitly. + const bound = env.__PORT?.trim(); + const boundPort = bound && /^\d+$/.test(bound) ? Number(bound) : NaN; + return `http://127.0.0.1:${boundPort >= 1 && boundPort <= 65535 ? boundPort : port}`; +} + /** Resolve how `/mcp` is authenticated, failing closed if nothing is configured. */ function resolveAuth(env: NodeJS.ProcessEnv): AuthConfig { const issuerRaw = env.OAUTH_ISSUER?.trim(); @@ -286,6 +328,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. @@ -303,6 +346,19 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { "finalize tier issues binding versions of draft documents and cannot run without the drafts tier.", ); } + // Outside OAuth mode, OAUTH_RESOURCE has exactly one remaining effect — it wins + // over SERVER_URL as the base for upload links (resolvePublicBaseUrl). That is + // easy to hit by accident: migrate from OAuth to a static token, remove + // OAUTH_ISSUER, update SERVER_URL — and a stale OAUTH_RESOURCE left in the + // environment silently keeps every ticket link pointing at the old host, with + // nothing anywhere saying why. Say so at startup. + if (auth.mode !== "oauth" && env.OAUTH_RESOURCE?.trim()) { + warnings.push( + "OAUTH_RESOURCE is set but OAuth mode is not enabled (no OAUTH_ISSUER). It still takes precedence " + + "over SERVER_URL when building upload links — if that is stale, links point at the wrong host. " + + "Unset OAUTH_RESOURCE or use SERVER_URL alone outside OAuth mode.", + ); + } return { lexwareApiKey, @@ -313,7 +369,8 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { "LEXWARE_APP_BASE_URL", ), auth, - port: parsePort(env.PORT), + publicBaseUrl: resolvePublicBaseUrl(env, port), + 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 44622ba..b1e201c 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 { advertisedScopes, 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; @@ -70,14 +42,18 @@ const client = new LexwareClient({ const server = new McpServer( { name: "lexware-office", - version: "0.1.10", + version: "0.1.11", }, { capabilities: {} }, ); // 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). @@ -121,7 +97,34 @@ 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(); +// The ticket-gated upload routes are a drafts-tier WRITE path (they push a file into the +// Lexware file store), so mount them only when the drafts capability is enabled. Without +// this, a read-only deployment (LEXWARE_READ_ONLY, or drafts explicitly off) would still +// expose the unauthenticated POST /upload/:ticket route wired to Lexware's write API — +// unreachable, since no ticket can be issued without the drafts-only create-upload-ticket +// tool, but a write route has no business existing on a server configured not to write. +if (config.capabilities.drafts) { + 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)"}`, @@ -132,7 +135,11 @@ 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 partially affected too: the global ~100 KB JSON parser stays in " + + "front of them instead of being skipped, so a JSON-content-typed ticket upload over ~100 KB fails " + + "there as well — other content types (PDFs, images) pass that parser untouched and still work up to " + + "the full limit (routes.ts's Buffer.isBuffer guard prevents a silent empty upload for the JSON case).", ); } if (config.auth.mode === "oauth" && config.auth.allowedEmailDomains.length === 0) { diff --git a/src/tools/index.ts b/src/tools/index.ts index c1c1bb9..b456cef 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, uploadTickets, publicBaseUrl); } // 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..2ac2286 --- /dev/null +++ b/src/tools/uploads.ts @@ -0,0 +1,169 @@ +import type { McpServer } from "skybridge/server"; +import { z } from "zod"; +import type { TicketState, TicketStore } from "../uploads/tickets.js"; +import { LOCAL_RO, 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` the X-Filename-B64 header is OMITTED entirely rather than + * guessed; the server then falls back to the ticket's own value and finally to + * `upload.bin`. Content-Type is different: curl is never allowed to decide it. + * With a known, token-shaped mimeType the header carries it; otherwise the + * command pins `-H 'Content-Type:'` — curl's documented syntax for REMOVING an + * internally generated header — because `--data-binary` otherwise defaults to + * `Content-Type: application/x-www-form-urlencoded`, a truthy declared value that + * wins over the server's fallback chain (see resolveContentType) and filed every + * headerless upload under a false type. With the header stripped, nothing is + * declared and the server's ticket-mimeType / `application/octet-stream` fallback + * actually decides, as designed. + */ +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}'`); + } else { + headers.push(` -H 'Content-Type:'`); + } + 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, + store: TicketStore, + publicBaseUrl: string, +): 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.") }, + // LOCAL_RO, not WRITE: the handler only reads the in-memory store (peek), and + // this tool is DESIGNED to be polled after a browser upload — a WRITE hint made + // approval-prompting clients confirm every poll iteration, and a client + // enforcing a read-only policy blocked the one tool that retrieves the fileId. + annotations: LOCAL_RO, + }, + 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).`), + }; + }, + ); +} diff --git a/src/uploads/filename.ts b/src/uploads/filename.ts new file mode 100644 index 0000000..83dbce3 --- /dev/null +++ b/src/uploads/filename.ts @@ -0,0 +1,39 @@ +/** + * 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 is a trust + * boundary: the name arrives from a request header (`X-Filename` / `X-Filename-B64`) or + * the ticket's own fallback 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 fallback chain + * (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(); +} 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..fa9bb1a --- /dev/null +++ b/src/uploads/routes.ts @@ -0,0 +1,358 @@ +import express from "express"; +import { LexwareApiError } from "../lexware/errors.js"; +import { sanitizeFilename } from "./filename.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(); + }; +} + +/** + * Bounds concurrent body buffering to ONE in-progress request per ticket, closing + * the gap the non-claiming pre-check leaves open: `claim()` runs only AFTER + * `express.raw()` has buffered the whole body, so N simultaneous POSTs naming the + * same valid ticket all passed `requireClaimableTicket` and each buffered up to + * `maxBytes` before N−1 lost the claim race — unbounded memory amplification from + * a single leaked ticket URL. This is NOT the single-use lock (`claim()` stays + * that); the slot is released when the RESPONSE closes — success, failure, or + * client abort, `close` fires in every case — so a failed attempt frees it for a + * sequential retry and it can never be stranded. 429, not 410: the losing request + * did nothing wrong and may legitimately retry once the winner resolves. + */ +function limitConcurrentBodyReads(store: TicketStore): express.RequestHandler { + return (req, res, next) => { + const ticket = ticketParam(req); + if (!store.beginBodyRead(ticket)) { + res.status(429).json({ error: "Another upload with this ticket is already in progress." }); + return; + } + res.once("close", () => store.endBodyRead(ticket)); + 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; + } + // Lock down the page response: it embeds the ticket (a bearer capability) in its HTML + // and URL. no-store keeps it out of any shared/proxy or browser disk cache; nosniff + // stops a client re-interpreting it as anything but the declared text/html; DENY + // framing refuses the page inside a cross-origin frame, where it has no business. + res + .type("text/html") + .set("Cache-Control", "no-store") + .set("X-Content-Type-Options", "nosniff") + .set("X-Frame-Options", "DENY") + .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), + // One buffering body per ticket at a time — must also run before the body is + // read. See limitConcurrentBodyReads' doc comment. + limitConcurrentBodyReads(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); + // `bytes` is passed as-is: a Buffer IS a Uint8Array, and the multipart + // sink copies it into a Blob anyway — a defensive `new Uint8Array(bytes)` + // here just held a third full-size copy of a 20 MB upload for nothing. + const created = await upload({ + 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; + } + // EXCEPT auth failures: a 401/403 from Lexware means the OPERATOR's API + // key was rejected, not anything the ticket holder did. Forwarding it + // verbatim answers "unauthorized" on a route that has no caller auth at + // all, and hands Lexware's own error wording to an unauthenticated + // audience. It is a server configuration problem — fixed message, 502. + if (err instanceof LexwareApiError && err.kind === "auth") { + res.status(502).json({ + error: + "Upload failed: the server could not authenticate with Lexware Office. " + + "This is a server configuration problem — contact the operator.", + }); + 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). + if (err instanceof LexwareApiError && err.status >= 400 && err.status < 600) { + res.status(err.status).json({ error: err.message }); + return; + } + // `status === 0` means a network/transport failure reaching Lexware: the + // POST is deliberately not retried by the client (non-idempotent), so the + // upload MAY have landed even though no answer came back. The ticket was + // released above (retries must stay possible), which means a blind re-run + // CAN file the receipt twice — say so, instead of leaving the caller to + // guess. Anything else non-HTTP stays a plain 502. + if (err instanceof LexwareApiError && err.status === 0) { + res.status(502).json({ + error: + `${err.message} The upload may or may not have reached Lexware — ` + + "check whether the file already exists before retrying, or it can be filed twice.", + }); + 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 filename.ts, the shared trust-boundary helper) + * 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..add930a --- /dev/null +++ b/src/uploads/tickets.ts @@ -0,0 +1,176 @@ +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() && !state.inFlight)) { + // 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. + // + // EXCEPT an in-flight entry: a claim at 14:59 whose Lexware call is still + // running at 15:01 must not be evicted by a SECOND request racing the same + // ticket — deleting it here would orphan the first request's complete() + // (its result silently lost) even though the upload succeeded. The racing + // request falls through to the "already used" rejection below instead. + 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; + // Re-arm the TTL from the moment of COMPLETION. Without this, the result was + // readable only until the ticket's original creation-time expiry — an upload + // dropped onto the page at minute 14 left get-upload-result a sub-minute + // window, after which the model was told "unknown or expired, issue a new + // one" and the user uploaded the SAME receipt again (duplicate voucher, + // first file id orphaned). The result now stays readable for a full TTL + // after the upload finished; the entry is evicted after that as before. + state.expiresAt = this.now() + this.ttlMs; + } + } + + /** + * 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; + // An in-flight entry is never expired-evicted (same reasoning as claim()): the + // upload is happening RIGHT NOW, and callers need the truthful answers — the + // GET page and the POST pre-check see "already used", get-upload-result sees + // "pending" — not a false "expired" that races the in-progress complete(). + if (state.expiresAt <= this.now() && !state.inFlight) { + this.tickets.delete(ticket); + return undefined; + } + return state; + } + + /** + * True when this call takes the one body-read slot for `ticket`; false when a + * request already holds it. NOT the single-use lock (claim() stays that): this + * bounds how many request BODIES can be buffering for one ticket at a time. + * Without it, N simultaneous POSTs naming the same valid ticket all passed the + * (deliberately non-claiming) pre-check and each buffered up to maxBytes before + * the first claim() won — unbounded memory amplification from one leaked ticket + * URL. Deliberately keyed in a separate Set rather than on TicketState, so the + * slot survives the entry being evicted mid-read and is always released by the + * route's response-close hook, never leaked. + */ + beginBodyRead(ticket: string): boolean { + if (this.readingBody.has(ticket)) return false; + this.readingBody.add(ticket); + return true; + } + + /** Releases the body-read slot. Idempotent; called from the response-close hook. */ + endBodyRead(ticket: string): void { + this.readingBody.delete(ticket); + } + + private readonly readingBody = new Set(); + + /** 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) { + // Never evict an in-flight entry (see claim()/peek()); release() clears the + // flag on failure, so a failed expired entry is collected on the next pass. + if (state.expiresAt <= t && !state.inFlight) this.tickets.delete(key); + } + } +} diff --git a/tests/config.test.ts b/tests/config.test.ts index 63fe103..2f84fed 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -226,6 +226,94 @@ describe("loadConfig", () => { expect(c.warnings).toEqual([]); }); + 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("prefers __PORT for the loopback fallback — that is the port the listener actually binds", () => { + // Under `skybridge dev` the listener binds __PORT (skybridge picks it itself, + // ~3000) and plain PORT is never consulted — links built from PORT refused + // connections on the very machine the fallback exists for. `npm start` is + // unaffected: server.ts copies config.port into __PORT only AFTER loadConfig. + expect(loadConfig({ ...base(), __PORT: "3000" } as NodeJS.ProcessEnv).publicBaseUrl).toBe("http://127.0.0.1:3000"); + // Both set: __PORT is what is actually bound. + expect( + loadConfig({ ...base(), PORT: "9443", __PORT: "3000" } as NodeJS.ProcessEnv).publicBaseUrl, + ).toBe("http://127.0.0.1:3000"); + // Garbage __PORT is ignored rather than pasted into links. + expect(loadConfig({ ...base(), __PORT: "nope" } as NodeJS.ProcessEnv).publicBaseUrl).toBe("http://127.0.0.1:8080"); + // An explicit public URL always wins over any loopback fallback. + expect( + loadConfig({ ...base(), __PORT: "3000", SERVER_URL: "https://mcp.example.com" } as NodeJS.ProcessEnv) + .publicBaseUrl, + ).toBe("https://mcp.example.com"); + }); + + it("warns when OAUTH_RESOURCE is set outside OAuth mode (it silently steers upload links)", () => { + // The migration footgun: switch from OAuth to a static token, remove + // OAUTH_ISSUER, update SERVER_URL — a stale OAUTH_RESOURCE left behind keeps + // every ticket link pointing at the old host, previously with no notice. + const c = loadConfig({ + ...base(), + OAUTH_RESOURCE: "https://old.example.com", + SERVER_URL: "https://new.example.com", + } as NodeJS.ProcessEnv); + expect(c.auth.mode).toBe("static"); + expect(c.publicBaseUrl).toBe("https://old.example.com"); // documented precedence still applies… + expect(c.warnings.some((w) => w.includes("OAUTH_RESOURCE") && w.includes("SERVER_URL"))).toBe(true); // …but loudly + // No warning in OAuth mode — there the resource IS the resource… + const oauth = loadConfig({ + LEXWARE_API_KEY: "k", + OAUTH_ISSUER: "https://auth.example.com", + OAUTH_RESOURCE: "https://mcp.example.com", + } as NodeJS.ProcessEnv); + expect(oauth.warnings).toEqual([]); + // …and none when the variable is simply absent. + expect(loadConfig(base()).warnings).toEqual([]); + }); + 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..e9f145b 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,9 @@ const DRAFT_TOOLS = [ "update-voucher", "upload-voucher-file", "upload-file", + // expansion: ticket-gated upload, no base64 through the model context + "create-upload-ticket", + "get-upload-result", ]; const FINALIZE_TOOLS = [ "create-finalized-invoice", @@ -85,7 +89,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-filename.test.ts b/tests/uploads-filename.test.ts new file mode 100644 index 0000000..7c73ebe --- /dev/null +++ b/tests/uploads-filename.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { sanitizeFilename } from "../src/uploads/filename.js"; + +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..3555fa9 --- /dev/null +++ b/tests/uploads-routes.test.ts @@ -0,0 +1,1102 @@ +import express from "express"; +import { execFile, execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import http from "node:http"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { promisify } from "node:util"; +import { describe, expect, it } from "vitest"; +import { LexwareApiError } from "../src/lexware/errors.js"; +import { buildTicketResponse } from "../src/tools/uploads.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 }); + // The body buffer is forwarded AS-IS (a Buffer is a Uint8Array; the multipart + // sink copies it into a Blob anyway) — so assert the CONTENT, not the concrete + // constructor, which is deliberately Buffer rather than a defensive copy. + const call = uploaded[0] as { bytes: Uint8Array; filename: string; contentType: string; type: string }; + expect(call.bytes).toBeInstanceOf(Uint8Array); + expect(Array.from(call.bytes)).toEqual([37, 80, 68, 70]); + expect(call.filename).toBe("beleg.pdf"); + expect(call.contentType).toBe("application/pdf"); + expect(call.type).toBe("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(Array.from((uploaded[0] as { bytes: Uint8Array }).bytes)).toEqual(Array.from(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(); + }); + + // --- One buffering body per ticket at a time ---------------------------------- + + it("answers 429 to a second POST while the first is still STREAMING its body (pre-claim window)", async () => { + // This is the window claim() cannot cover: claim runs only after express.raw + // has buffered the whole body, so without the body-read slot N simultaneous + // POSTs on one valid ticket each buffered up to maxBytes before N−1 lost the + // race — unbounded memory amplification from a single leaked ticket URL. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + + // First request: a streamed body that stalls after its first chunk, holding + // the slot open mid-buffer. + let releaseFirst!: () => void; + const gate = new Promise((r) => { + releaseFirst = r; + }); + const stalledBody = new ReadableStream({ + async start(controller) { + controller.enqueue(new Uint8Array([1, 2, 3])); + await gate; + controller.enqueue(new Uint8Array([4])); + controller.close(); + }, + }); + const first = fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf" }, + body: stalledBody, + // Node's fetch requires the half-duplex opt-in for streamed request bodies. + duplex: "half", + } as RequestInit & { duplex: "half" }); + // Give the first request time to pass the pre-checks and reach express.raw. + await new Promise((r) => setTimeout(r, 150)); + + const second = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf" }, + body: new Uint8Array([9]), + }); + expect(second.status).toBe(429); + + releaseFirst(); + const firstRes = await first; + expect(firstRes.status).toBe(200); + + // NOTE: this 410 comes from requireClaimableTicket, which runs BEFORE the + // slot check — it proves the ticket was consumed, not that the slot was + // freed. Slot release is proven elsewhere: the sequential-retry tests cover + // the finished-response path, and the abort test below drives the + // premature-termination path. + const third = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf" }, + body: new Uint8Array([9]), + }); + expect(third.status).toBe(410); + await s.close(); + }); + + it("releases the body-read slot when the client ABORTS mid-stream, so the ticket is not 429-locked", async () => { + // The slot is released on response 'close', which fires on premature + // termination too — not only on a finished response. If that hook ever + // regressed to 'finish' (which does NOT fire on abort), an aborted upload + // would keep the slot for a still-valid ticket and every retry would answer + // 429 until the socket timeout. This test drives exactly the abort path. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeApp(store); + const s = await listen(app); + + const aborter = new AbortController(); + const neverEnding = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([1, 2, 3])); // then stall forever + }, + }); + const aborted = fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf" }, + body: neverEnding, + duplex: "half", + signal: aborter.signal, + } as RequestInit & { duplex: "half" }).catch((e) => e); + await new Promise((r) => setTimeout(r, 150)); // let it reach the slot middleware + aborter.abort(); + await aborted; // fetch rejects; the server sees the connection die + + // 'close' propagates asynchronously — poll briefly rather than guessing one + // magic delay. The retry must stop answering 429 once the slot frees, and + // must then SUCCEED: the aborted request never completed its body, so claim() + // never ran and the ticket is still unconsumed. + let retryStatus = 429; + for (let i = 0; i < 40 && retryStatus === 429; i++) { + await new Promise((r) => setTimeout(r, 50)); + const retry = await fetch(`${s.url}/upload/${t.ticket}`, { + method: "POST", + headers: { "content-type": "application/pdf" }, + body: new Uint8Array([9]), + }); + retryStatus = retry.status; + } + expect(retryStatus).toBe(200); + 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) and names the duplicate risk", 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. And because the POST is non-idempotent and not + // retried, the upload MAY have landed anyway; the ticket is released for a + // retry, so the message must say the outcome is unknown — a blind re-run can + // file the receipt twice. + 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); + expect((await res.json()).error).toMatch(/may or may not have reached/i); + expect(store.peek(t.ticket)?.inFlight).toBe(false); // released — a retry stays possible + await s.close(); + }); + + it("answers a Lexware auth failure (the OPERATOR's API key rejected) as a generic 502, never the upstream status", async () => { + for (const status of [401, 403]) { + // Forwarding these verbatim tells an unauthenticated ticket holder + // "unauthorized" on a route that has no caller auth at all, and hands + // Lexware's own error wording to strangers. It is a server configuration + // problem, so it belongs in the 502 bucket with a fixed message. + const store = new TicketStore(); + const t = store.create({ type: "voucher" }); + const app = makeFailingApp(store, new LexwareApiError(status, `Lexware API ${status}: invalid or expired token`)); + 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, String(status)).toBe(502); + const body = await res.json(); + expect(body.error).not.toContain(String(status)); + expect(body.error).not.toContain("invalid or expired token"); + expect(body.error).toMatch(/server configuration|operator/i); + // Released: once the operator fixes the key, the same ticket still works. + expect(store.peek(t.ticket)?.inFlight).toBe(false); + 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'); + }); +}); + +// --- The emitted curl command, run for real -------------------------------------- + +const hasCurl = (() => { + try { + execFileSync("sh", ["-c", "command -v curl"], { stdio: "ignore" }); + return true; + } catch { + return false; + } +})(); + +describe("the emitted curl command, executed against the real routes", () => { + it.skipIf(!hasCurl)( + "never lets curl invent the Content-Type: the server's fallback decides, not application/x-www-form-urlencoded", + async () => { + // The exact precondition of the bug: the model passed no mimeType, so the + // ticket carries none and the command has no valued Content-Type header. + // Measured before the fix: curl's --data-binary default then declared + // 'application/x-www-form-urlencoded' — a truthy value that won in + // resolveContentType, so the documented ticket-mimeType/octet-stream + // fallback chain was dead code for every headerless curl upload. + const store = new TicketStore(); + const uploaded: unknown[] = []; + const t = store.create({ type: "voucher" }); + const app = makeApp(store, uploaded); + const s = await listen(app); + const { curlCommand } = buildTicketResponse(t, s.url); + const dir = mkdtempSync(path.join(tmpdir(), "lexware-curl-")); + const file = path.join(dir, "beleg.pdf"); + writeFileSync(file, Buffer.from([37, 80, 68, 70])); + try { + // Exactly the edit a user is told to make: replace the FILE path, run it. + // ASYNC exec, not execFileSync: curl talks to a server living in THIS + // process — a sync child-process wait freezes the event loop and deadlocks + // the test against its own server. + const { stdout: out } = await promisify(execFile)("sh", ["-c", curlCommand.replace("/path/to/file.pdf", file)], { + encoding: "utf8", + }); + expect(JSON.parse(out).fileId).toBe("file-123"); // curl prints the server's JSON + const call = uploaded[0] as { contentType: string; filename: string }; + expect(call.contentType).toBe("application/octet-stream"); + expect(call.contentType).not.toBe("application/x-www-form-urlencoded"); + expect(call.filename).toBe("upload.bin"); // no name known anywhere → the fixed default + } finally { + rmSync(dir, { recursive: true, force: true }); + await s.close(); + } + }, + ); +}); diff --git a/tests/uploads-tickets.test.ts b/tests/uploads-tickets.test.ts new file mode 100644 index 0000000..5446f06 --- /dev/null +++ b/tests/uploads-tickets.test.ts @@ -0,0 +1,196 @@ +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); + }); + + // --- complete() re-arms the TTL: the result outlives the CREATION clock -------- + + it("keeps the result readable for a full TTL after COMPLETION, not after creation", () => { + // The bug this pins down: an upload dropped onto the page at minute 14 left + // get-upload-result a sub-minute window before the creation-time expiry evicted + // the entry — the model was told "unknown or expired, issue a new one" for an + // upload that SUCCEEDED, and the user filed the same receipt twice. + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); // creation clock runs out at 60_000 + now = 59_000; // upload lands just before that + store.claim(t.ticket); + store.complete(t.ticket, { fileId: "f1", filename: "x.pdf", byteLength: 3 }); + now = 100_000; // creation clock long past — the result must still be readable + expect(store.peek(t.ticket)?.result?.fileId).toBe("f1"); + now = 119_001; // one full TTL after completion (59_000 + 60_000) — now it may go + expect(store.peek(t.ticket)).toBeUndefined(); + }); + + it("never expiry-evicts an IN-FLIGHT entry: a racing claim reads 'already used' and the result survives", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + now = 59_999; + store.claim(t.ticket); // the upload starts just before expiry… + now = 61_000; // …and its Lexware call is still running past it + // A second request racing the same ticket must NOT delete the in-flight entry — + // that would orphan the first request's complete() (result silently lost even + // though Lexware filed the receipt). It reads "already used", not "expired". + expect(() => store.claim(t.ticket)).toThrow(/already used/); + // get-upload-result during that window reads "pending", never "expired". + expect(store.peek(t.ticket)?.inFlight).toBe(true); + expect(store.peek(t.ticket)?.result).toBeUndefined(); + // create() → sweep() must not collect it either. + store.create({ type: "voucher" }); + store.complete(t.ticket, { fileId: "f2", filename: "y.pdf", byteLength: 1 }); + expect(store.peek(t.ticket)?.result?.fileId).toBe("f2"); + }); + + it("still collects an expired entry once release() clears the in-flight shield", () => { + let now = 0; + const store = new TicketStore(60_000, () => now); + const t = store.create({ type: "voucher" }); + store.claim(t.ticket); + now = 61_000; + store.release(t.ticket); // the upload failed, past expiry + expect(store.peek(t.ticket)).toBeUndefined(); // expired + not in flight → evicted + }); + + // --- the body-read slot (bounds buffering; NOT the single-use lock) ------------ + + it("beginBodyRead grants one slot per ticket until endBodyRead releases it", () => { + const store = new TicketStore(60_000, () => 0); + expect(store.beginBodyRead("t1")).toBe(true); + expect(store.beginBodyRead("t1")).toBe(false); // a second concurrent reader is refused + expect(store.beginBodyRead("t2")).toBe(true); // slots are per ticket + store.endBodyRead("t1"); + expect(store.beginBodyRead("t1")).toBe(true); // a sequential retry gets the slot back + store.endBodyRead("t1"); + store.endBodyRead("t1"); // idempotent — the response-close hook may fire late + expect(store.beginBodyRead("t1")).toBe(true); + }); +}); diff --git a/tests/uploads-tools.test.ts b/tests/uploads-tools.test.ts new file mode 100644 index 0000000..b483adb --- /dev/null +++ b/tests/uploads-tools.test.ts @@ -0,0 +1,220 @@ +import type { McpServer } from "skybridge/server"; +import { execFileSync } from "node:child_process"; +import { describe, expect, it } from "vitest"; +import { loadConfig } from "../src/config.js"; +import { buildCurlCommand, buildTicketResponse, registerUploadTools } from "../src/tools/uploads.js"; +import { TicketStore } from "../src/uploads/tickets.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 the filename header when unknown, but ALWAYS pins Content-Type (real value or explicit unset)", () => { + const nameOnly = buildCurlCommand(URL_, { filename: "beleg.pdf" }); + expect(headerValue(nameOnly, "X-Filename-B64")).toBeDefined(); + // No VALUED Content-Type — but the unset form must be present (see below). + expect(nameOnly).not.toMatch(/Content-Type: \S/); + expect(nameOnly).toContain("-H '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: the command still pins `-H 'Content-Type:'` — curl's syntax for + // REMOVING an internally generated header. Measured: without it, --data-binary + // silently declares 'application/x-www-form-urlencoded', a truthy value that wins + // over the server's fallback chain and files every headerless upload under a + // false type. With the header stripped, nothing is declared and the server's + // ticket-mimeType / application/octet-stream fallback actually decides. + const bare = buildCurlCommand(URL_); + expect(bare).toBe(`FILE='/path/to/file.pdf'; curl -sS -X POST '${URL_}' -H 'Content-Type:' --data-binary @"$FILE"`); + expect(headerValue(bare, "Content-Type")).toBeUndefined(); // unset form carries no value + }); + + 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 + // to the fixed unset form is the safe outcome, the server's fallback applies. + const hostile = buildCurlCommand(URL_, { mimeType: "application/pdf'; rm -rf ~; echo '" }); + expect(hostile).not.toContain("rm -rf"); + expect(hostile).not.toMatch(/Content-Type: \S/); // nothing of the hostile value survives + expect(hostile).toBe(`FILE='/path/to/file.pdf'; curl -sS -X POST '${URL_}' -H 'Content-Type:' --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.toMatch(/Content-Type: \S/); + // 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("X-Filename-B64"); + expect(blank).not.toMatch(/Content-Type: \S/); + expect(blank).toContain("-H 'Content-Type:'"); // absent type still pins the unset form + }); +}); + +// --- Tool annotations ------------------------------------------------------------- + +describe("upload tool annotations", () => { + it("marks get-upload-result as a local read; create-upload-ticket stays a write", () => { + const defs: { name: string; annotations: Record }[] = []; + const fake = { + registerTool(def: { name: string; annotations: Record }) { + defs.push(def); + return fake; + }, + } as unknown as McpServer; + registerUploadTools(fake, new TicketStore(), "https://mcp.example.test"); + const byName = Object.fromEntries(defs.map((d) => [d.name, d.annotations])); + // get-upload-result only peeks the in-memory store, and it is DESIGNED to be + // polled after a browser upload — a WRITE hint made approval-prompting clients + // confirm every poll iteration, and a client enforcing a read-only policy + // blocked the one tool that retrieves the fileId. + expect(byName["get-upload-result"]).toMatchObject({ readOnlyHint: true, openWorldHint: false }); + // Issuing a ticket arms an (unauthenticated, single-use) write path: WRITE. + expect(byName["create-upload-ticket"]).toMatchObject({ readOnlyHint: false }); + }); +});