From 9b6210497dec57cff653e85b453dc18b6a8ee8cf Mon Sep 17 00:00:00 2001 From: gutencoder Date: Thu, 13 Aug 2026 12:14:43 +0000 Subject: [PATCH 1/4] feat(uploads): a transport that connects to the address that was checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every check-then-fetch SSRF filter has the same hole, and 0.1.11's CHANGELOG named it as the reason upload-file-from-url was held back: the guard resolves the host and validates the addresses, then hands the *name* to fetch, which resolves it again when it opens the socket. Two lookups, and only the first one was checked. A DNS answer that differs between them — two records on a short TTL, or deliberate rebinding — gets connected to without ever having been looked at. createPinnedFetch removes the second lookup rather than trying to make it agree with the first: net.connect's `lookup` hook is fed the addresses the caller already vetted, and consults DNS for nothing. No window is left, because there is no second resolution. Built on node:https rather than the undici dispatcher suggested in #34, for one reason: undici is not a dependency here — `fetch` is Node's built-in copy, not reachable as a module — so the dispatcher route means adding a network stack to a server that fronts accounting data, and carrying two copies of undici in one process. node:https already exposes the hook, so the guarantee is identical and the dependency count does not move. Happy to switch if you would rather have the dispatcher. What pinning does NOT touch is certificate validation: SNI and checkServerIdentity still come from the hostname, only the dialled address comes from the pin. The test for that reads the SNI out of the raw ClientHello on the wire rather than asking the client to report on itself. The connection tests use a `.invalid` host (RFC 6761 guarantees it cannot resolve), so a socket arriving at the listener can only have come from the pin — an unpinned client fails with ENOTFOUND and never opens one. Sockets are never pooled (fresh agent, keepAlive: false), so a connection opened for a differently vetted request cannot be reused here. --- src/uploads/pinned-fetch.ts | 199 ++++++++++++++++++++ tests/uploads-pinned-fetch.test.ts | 292 +++++++++++++++++++++++++++++ 2 files changed, 491 insertions(+) create mode 100644 src/uploads/pinned-fetch.ts create mode 100644 tests/uploads-pinned-fetch.test.ts diff --git a/src/uploads/pinned-fetch.ts b/src/uploads/pinned-fetch.ts new file mode 100644 index 0000000..926cd22 --- /dev/null +++ b/src/uploads/pinned-fetch.ts @@ -0,0 +1,199 @@ +import type { IncomingMessage } from "node:http"; +import { Agent as HttpsAgent, request as httpsRequest } from "node:https"; +import { isIPv6 } from "node:net"; +import { Readable } from "node:stream"; + +/** + * The subset of `fetch` this module needs and provides. `typeof fetch` is assignable + * to it (a function taking a wider input type satisfies a narrower one), so a caller + * can still inject the global fetch or a test double. + */ +export type FetchLike = ( + url: URL, + init: { redirect: "manual"; signal: AbortSignal }, +) => Promise; + +/** + * Statuses the `Response` constructor refuses to pair with a body (WHATWG "null body + * status"). 1xx is absent on purpose: Node reports informational responses through the + * `information` event, never as `statusCode`, and the `Response` constructor rejects any + * status below 200 outright — so a 1xx here would be a RangeError, not a null body. + */ +const NULL_BODY_STATUSES = new Set([204, 205, 304]); + +/** + * The shape `net.connect` expects of a custom `lookup`. `family` is typed as Node types + * it — the numeric form is what the connect path passes, but `dns.lookup` also accepts + * the string spellings, and a lookup that only understood numbers would silently treat + * `"IPv6"` as "no preference". + */ +export type PinnedLookup = ( + hostname: string, + options: { family?: number | "IPv4" | "IPv6"; all?: boolean }, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | { address: string; family: number }[], + family?: number, + ) => void, +) => void; + +/** Normalizes a hostname for comparison: case-folded, trailing root-label dot removed. */ +function normalizeHost(host: string): string { + return host.trim().toLowerCase().replace(/\.$/, ""); +} + +/** + * The pin itself: a `lookup` for `net.connect` that never consults DNS and only ever + * yields `addresses` — the ones the caller already vetted. + * + * Exported separately from {@link createPinnedFetch} because this is the whole security + * property in one pure function, and it is worth testing on its own: every path out of + * here either hands back a vetted address or an error. There is no path that resolves + * anything. + */ +export function createPinnedLookup(hostname: string, addresses: string[]): PinnedLookup { + const pinnedHost = normalizeHost(hostname); + const pinned = addresses.map((address) => ({ address, family: isIPv6(address) ? 6 : 4 })); + + return (lookupHost, options, callback) => { + // Fails closed rather than falling back to a real lookup: being asked for a + // different host means the connection is not the one that was vetted. + if (normalizeHost(lookupHost) !== pinnedHost) { + callback(new Error(`Refusing to resolve ${lookupHost}: only ${pinnedHost} was vetted.`), "", 0); + return; + } + // `family` is 0 (or absent) when either family is acceptable. Filtering rather than + // ignoring it keeps Node's happy-eyeballs retries inside the vetted set. + const requested = options.family; + const wanted = + requested === 4 || requested === "IPv4" ? 4 : requested === 6 || requested === "IPv6" ? 6 : 0; + const matching = wanted === 0 ? pinned : pinned.filter((a) => a.family === wanted); + if (matching.length === 0) { + const what = wanted === 0 ? "address" : `IPv${wanted} address`; + callback(new Error(`No vetted ${what} for ${pinnedHost}.`), "", 0); + return; + } + if (options.all) callback(null, matching); + else callback(null, matching[0].address, matching[0].family); + }; +} + +/** + * Builds a `fetch`-shaped function that connects ONLY to `addresses` — the addresses + * the caller already vetted — instead of resolving `hostname` again at connect time. + * + * ## Why this exists + * + * The SSRF guard in `fetch-url.ts` resolves the host, checks every returned address + * against the blocked ranges, and then calls `fetch`. `fetch` does its own DNS + * resolution when it opens the socket, so the address that was *checked* and the + * address that is *connected to* come from two separate lookups. A DNS server that + * answers differently between them — a short TTL and two records, or a deliberate + * rebinding attack — makes the check describe a different host than the one the bytes + * come from. That is the classic TOCTOU in every check-then-fetch SSRF filter, and no + * amount of care in the checking half closes it. + * + * Pinning closes it structurally: the check and the connection consume the SAME lookup + * result, so there is no second resolution to disagree with the first. + * + * ## Why `node:https` and not an undici dispatcher + * + * An undici `Agent` with a custom `connect` is the other way to do this, but `undici` + * is not a dependency of this project — `fetch` is Node's built-in copy, which cannot + * be reached as a module. Taking the npm package on would add a network stack to the + * dependency set of a server that fronts accounting data, and would leave two copies + * of undici in the process. `node:https` already exposes the exact hook needed + * (`lookup`, passed down to `net.connect`), so the guarantee is the same and the + * dependency count is unchanged. + * + * ## What is NOT weakened + * + * TLS still validates the certificate against the **hostname**, not the pinned address: + * `https.request` derives SNI and `checkServerIdentity` from the URL's host, and only + * the address the socket dials comes from here. Pinning therefore cannot be used to + * accept a certificate that plain `fetch` would have rejected. + * + * Connections are never pooled: a fresh agent with `keepAlive: false` per request, so a + * socket opened for an earlier (differently vetted) request can never be reused here. + */ +export function createPinnedFetch(hostname: string, addresses: string[]): FetchLike { + const pinnedHost = normalizeHost(hostname); + const lookup = createPinnedLookup(hostname, addresses); + + return async function pinnedFetch(url, init) { + if (addresses.length === 0) { + throw new Error(`No vetted address to connect to for ${pinnedHost}.`); + } + + const agent = new HttpsAgent({ keepAlive: false, maxSockets: 1 }); + + try { + const res = await new Promise((resolve, reject) => { + const req = httpsRequest( + url, + { + method: "GET", + agent, + signal: init.signal, + headers: { + accept: "*/*", + // Deliberately no accept-encoding: without it the peer sends identity, so + // the byte count the size cap is enforced on is the byte count that ends up + // in memory. With transparent decompression a small compressed body can + // expand past the cap after the check. + "user-agent": "lexware-mcp", + }, + // The whole point. Node hands this to net.connect, so the socket dials an + // address from the list that was already checked — no second resolution. + lookup, + }, + resolve, + ); + req.on("error", reject); + req.end(); + }); + + // Destroy the agent once the response is done with the socket. Doing it earlier + // would tear down the connection the body is still arriving on. The `closed` check + // covers the response that finished during the await above, whose `close` event has + // already been emitted and would never reach a listener attached now. + if (res.closed) agent.destroy(); + else res.once("close", () => agent.destroy()); + + return responseFromIncoming(res); + } catch (err) { + agent.destroy(); + throw err; + } + }; +} + +/** + * Translates Node's `IncomingMessage` into the `Response` the rest of the upload path + * already speaks, so swapping the transport did not force the caller to change. + * + * Exported for tests: this is the one part of the transport a unit test can exercise + * end-to-end without a TLS peer, and getting a header or a null-body status wrong here + * would fail as a puzzling download error far away from the cause. + */ +export function responseFromIncoming(res: IncomingMessage): Response { + const headers = new Headers(); + for (const [key, value] of Object.entries(res.headers)) { + // set-cookie is the array case; appending keeps each value its own header line. + if (Array.isArray(value)) for (const item of value) headers.append(key, item); + else if (value !== undefined) headers.set(key, value); + } + + const status = res.statusCode ?? 502; + // `Response` accepts 200–599 only, and throws a bare RangeError outside it. Say what + // actually happened instead of letting that surface as the download's error. + if (status < 200 || status > 599) { + res.resume(); + throw new Error(`Unexpected HTTP status ${status}.`); + } + if (NULL_BODY_STATUSES.has(status)) { + res.resume(); + return new Response(null, { status, headers }); + } + return new Response(Readable.toWeb(res) as ReadableStream, { status, headers }); +} diff --git a/tests/uploads-pinned-fetch.test.ts b/tests/uploads-pinned-fetch.test.ts new file mode 100644 index 0000000..fb1bc8d --- /dev/null +++ b/tests/uploads-pinned-fetch.test.ts @@ -0,0 +1,292 @@ +import type { IncomingMessage } from "node:http"; +import { createServer, type Server, type Socket } from "node:net"; +import { Readable } from "node:stream"; +import { afterEach, describe, expect, it } from "vitest"; +import { createPinnedFetch, createPinnedLookup, responseFromIncoming } from "../src/uploads/pinned-fetch.js"; + +/** + * A hostname reserved by RFC 6761 §6.4 to be guaranteed NOT resolvable. That is what + * makes the connection tests proofs rather than coincidences: if a connection arrives at + * the listener, DNS cannot have produced the address — only the pin can have. + */ +const UNRESOLVABLE = "no-such-host.invalid"; + +/** + * A TCP listener that records what reaches it and then hangs up. Deliberately NOT a TLS + * server: these tests assert where the socket goes and what it announces, and the + * handshake is expected to fail right afterwards. Standing up a real TLS peer would need + * a certificate, and X.509 issuance is not something Node can do without a dependency. + */ +function recordingListener(): Promise<{ + server: Server; + port: number; + connections: Socket[]; + /** Peer addresses, captured on connect — `socket.remoteAddress` is cleared on destroy. */ + peers: string[]; + firstBytes: Promise; +}> { + const connections: Socket[] = []; + const peers: string[] = []; + let resolveBytes: (b: Buffer) => void; + const firstBytes = new Promise((r) => (resolveBytes = r)); + + const server = createServer((socket) => { + connections.push(socket); + peers.push(socket.remoteAddress ?? ""); + socket.once("data", (chunk: Buffer) => { + resolveBytes(chunk); + socket.destroy(); + }); + // A peer that connects but sends nothing must not wedge the test. + socket.setTimeout(2_000, () => socket.destroy()); + }); + + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resolve({ server, port, connections, peers, firstBytes }); + }); + }); +} + +/** + * Extracts the SNI host from a TLS ClientHello. Hand-rolled because the point is to read + * what actually went on the wire, and asking Node's TLS stack would just be asking the + * code under test to grade itself. + * + * Layout walked: record header (5) → handshake header (4) → client_version (2) → + * random (32) → session_id → cipher_suites → compression_methods → extensions, then the + * server_name extension (type 0x0000) → server_name_list → host_name entry (type 0x00). + */ +function sniFromClientHello(buf: Buffer): string | undefined { + let p = 5 + 4 + 2 + 32; // record header, handshake header, version, random + if (buf.length < p + 1) return undefined; + p += 1 + buf[p]; // session_id + if (buf.length < p + 2) return undefined; + p += 2 + buf.readUInt16BE(p); // cipher_suites + if (buf.length < p + 1) return undefined; + p += 1 + buf[p]; // compression_methods + if (buf.length < p + 2) return undefined; + const extensionsEnd = p + 2 + buf.readUInt16BE(p); + p += 2; + + while (p + 4 <= extensionsEnd && p + 4 <= buf.length) { + const type = buf.readUInt16BE(p); + const length = buf.readUInt16BE(p + 2); + const body = p + 4; + if (type === 0x0000) { + // server_name_list length (2), then entries of: type (1) + length (2) + host. + let q = body + 2; + while (q + 3 <= body + length) { + const nameType = buf[q]; + const nameLength = buf.readUInt16BE(q + 1); + if (nameType === 0x00) return buf.subarray(q + 3, q + 3 + nameLength).toString("ascii"); + q += 3 + nameLength; + } + return undefined; + } + p = body + length; + } + return undefined; +} + +describe("createPinnedLookup", () => { + /** Collects one invocation of the lookup into a plain value the assertions can read. */ + function invoke( + lookup: ReturnType, + host: string, + options: { family?: number | "IPv4" | "IPv6"; all?: boolean }, + ) { + return new Promise<{ err: Error | null; address: unknown; family?: number }>((resolve) => { + lookup(host, options, (err, address, family) => resolve({ err, address, family })); + }); + } + + it("returns every vetted address for an all:true lookup, in order", async () => { + const lookup = createPinnedLookup("files.example.com", ["203.0.113.7", "2001:db8::1"]); + const { err, address } = await invoke(lookup, "files.example.com", { all: true }); + expect(err).toBeNull(); + expect(address).toEqual([ + { address: "203.0.113.7", family: 4 }, + { address: "2001:db8::1", family: 6 }, + ]); + }); + + it("returns the first vetted address for an all:false lookup, with its family", async () => { + const lookup = createPinnedLookup("files.example.com", ["2001:db8::1", "203.0.113.7"]); + const { err, address, family } = await invoke(lookup, "files.example.com", { all: false }); + expect(err).toBeNull(); + expect(address).toBe("2001:db8::1"); + expect(family).toBe(6); + }); + + it("honours a requested family instead of handing back an address of the wrong one", async () => { + const lookup = createPinnedLookup("files.example.com", ["203.0.113.7", "2001:db8::1"]); + const v6 = await invoke(lookup, "files.example.com", { family: 6, all: true }); + expect(v6.address).toEqual([{ address: "2001:db8::1", family: 6 }]); + const v4 = await invoke(lookup, "files.example.com", { family: 4, all: false }); + expect(v4.address).toBe("203.0.113.7"); + }); + + it("understands the string spelling of family, not only the numeric one", async () => { + // dns.lookup accepts "IPv4"/"IPv6"; a lookup that only matched 4 and 6 would read + // those as "no preference" and hand back an address of the wrong family. + const lookup = createPinnedLookup("files.example.com", ["203.0.113.7", "2001:db8::1"]); + const v6 = await invoke(lookup, "files.example.com", { family: "IPv6", all: true }); + expect(v6.address).toEqual([{ address: "2001:db8::1", family: 6 }]); + const v4 = await invoke(lookup, "files.example.com", { family: "IPv4", all: true }); + expect(v4.address).toEqual([{ address: "203.0.113.7", family: 4 }]); + }); + + it("errors rather than falling back when the requested family has no vetted address", async () => { + const lookup = createPinnedLookup("files.example.com", ["203.0.113.7"]); + const { err, address } = await invoke(lookup, "files.example.com", { family: 6, all: true }); + expect(err?.message).toMatch(/No vetted IPv6 address/); + expect(address).toBe(""); + }); + + it("refuses a hostname other than the vetted one — the pin is not a general resolver", async () => { + const lookup = createPinnedLookup("files.example.com", ["203.0.113.7"]); + const { err } = await invoke(lookup, "attacker.example.net", { all: true }); + expect(err?.message).toMatch(/only files\.example\.com was vetted/); + }); + + it("treats a case difference and a trailing root-label dot as the same host", async () => { + const lookup = createPinnedLookup("Files.Example.com.", ["203.0.113.7"]); + const { err, address } = await invoke(lookup, "FILES.example.COM", { all: true }); + expect(err).toBeNull(); + expect(address).toEqual([{ address: "203.0.113.7", family: 4 }]); + }); + + it("errors when nothing was vetted, instead of resolving the name", async () => { + const lookup = createPinnedLookup("files.example.com", []); + const { err } = await invoke(lookup, "files.example.com", { all: true }); + expect(err?.message).toMatch(/No vetted address/); + }); +}); + +describe("createPinnedFetch", () => { + let open: Awaited> | undefined; + + afterEach(async () => { + if (!open) return; + for (const socket of open.connections) socket.destroy(); + await new Promise((resolve) => open!.server.close(resolve)); + open = undefined; + }); + + it("connects to the pinned address for a host DNS cannot resolve at all", async () => { + open = await recordingListener(); + const pinnedFetch = createPinnedFetch(UNRESOLVABLE, ["127.0.0.1"]); + + // Rejects, because the listener is not a TLS peer — that is expected and beside the + // point. What matters is that the connection ARRIVED: `no-such-host.invalid` has no + // DNS answer, so an unpinned client fails with ENOTFOUND and never opens a socket. + await expect( + pinnedFetch(new URL(`https://${UNRESOLVABLE}:${open.port}/file.pdf`), { + redirect: "manual", + signal: AbortSignal.timeout(5_000), + }), + ).rejects.toThrow(); + + expect(open.connections.length).toBe(1); + expect(open.peers[0]).toBe("127.0.0.1"); + }); + + it("still announces the HOSTNAME in the TLS handshake, so certificate validation is unchanged", async () => { + open = await recordingListener(); + const pinnedFetch = createPinnedFetch(UNRESOLVABLE, ["127.0.0.1"]); + + const attempt = pinnedFetch(new URL(`https://${UNRESOLVABLE}:${open.port}/file.pdf`), { + redirect: "manual", + signal: AbortSignal.timeout(5_000), + }); + const hello = await open.firstBytes; + await expect(attempt).rejects.toThrow(); + + // 0x16 = TLS handshake record. Pinning changes the address dialled, nothing else: + // the SNI is the hostname, so the peer must still present a certificate valid for + // it. Were the IP substituted into the request instead, this would read "127.0.0.1" + // and any certificate for the pinned host would be rejected — or worse, accepted. + expect(hello[0]).toBe(0x16); + expect(sniFromClientHello(hello)).toBe(UNRESOLVABLE); + }); + + it("refuses to issue a request for a host other than the vetted one", async () => { + open = await recordingListener(); + const pinnedFetch = createPinnedFetch(UNRESOLVABLE, ["127.0.0.1"]); + + await expect( + pinnedFetch(new URL(`https://other-host.invalid:${open.port}/file.pdf`), { + redirect: "manual", + signal: AbortSignal.timeout(5_000), + }), + ).rejects.toThrow(/only no-such-host\.invalid was vetted/); + + expect(open.connections.length).toBe(0); + }); + + it("refuses when no address was vetted, without touching the network", async () => { + open = await recordingListener(); + const pinnedFetch = createPinnedFetch(UNRESOLVABLE, []); + + await expect( + pinnedFetch(new URL(`https://${UNRESOLVABLE}:${open.port}/file.pdf`), { + redirect: "manual", + signal: AbortSignal.timeout(5_000), + }), + ).rejects.toThrow(/No vetted address/); + + expect(open.connections.length).toBe(0); + }); +}); + +describe("responseFromIncoming", () => { + /** A stand-in for what `https.request` hands back, so the translation can be read directly. */ + function incoming(statusCode: number, headers: Record, body?: string) { + const stream = Readable.from(body === undefined ? [] : [Buffer.from(body, "utf8")]); + return Object.assign(stream, { statusCode, headers }) as unknown as IncomingMessage; + } + + it("carries status, headers and body through unchanged", async () => { + const res = responseFromIncoming( + incoming( + 200, + { + "content-type": "application/pdf", + "content-length": "5", + "content-disposition": `attachment; filename*=UTF-8'de'Rechnung.pdf`, + }, + "hello", + ), + ); + expect(res.status).toBe(200); + expect(res.headers.get("content-type")).toBe("application/pdf"); + expect(res.headers.get("content-disposition")).toBe(`attachment; filename*=UTF-8'de'Rechnung.pdf`); + expect(await res.text()).toBe("hello"); + }); + + it("keeps a redirect's location readable, since redirects are followed by hand", async () => { + const res = responseFromIncoming(incoming(302, { location: "https://elsewhere.example.com/f.pdf" })); + expect(res.status).toBe(302); + expect(res.headers.get("location")).toBe("https://elsewhere.example.com/f.pdf"); + }); + + it("gives a null body to statuses that cannot carry one, instead of throwing", async () => { + for (const status of [204, 205, 304]) { + const res = responseFromIncoming(incoming(status, {})); + expect(res.status, String(status)).toBe(status); + expect(res.body, String(status)).toBeNull(); + } + }); + + it("keeps repeated headers as separate values rather than losing all but one", () => { + const res = responseFromIncoming(incoming(200, { "set-cookie": ["a=1", "b=2"] }, "x")); + expect(res.headers.getSetCookie()).toEqual(["a=1", "b=2"]); + }); + + it("reports a status outside 200-599 as itself, not as a RangeError from Response", () => { + expect(() => responseFromIncoming(incoming(100, {}))).toThrow(/Unexpected HTTP status 100/); + }); +}); From 1c6651a01153f2034413ffdb3eb1166a4e75405a Mon Sep 17 00:00:00 2001 From: gutencoder Date: Thu, 13 Aug 2026 12:14:56 +0000 Subject: [PATCH 2/4] feat(uploads): upload-file-from-url, opt-in, on the pinned transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings back the tool held out of #34, now that the TOCTOU its deferral named is closed by the transport in the previous commit. It fetches a file from a share link server-side and stores it in Lexware, so a receipt already sitting in OneDrive/SharePoint reaches the books without its bytes crossing the model context. Off by default (LEXWARE_ENABLE_URL_UPLOAD), and in a file of its own rather than beside the ticket flow. The ticket flow only ever receives bytes; this is the only tool that makes the server originate an outbound request to a destination the model chose. Different risk class, own switch — so enabling drafts cannot hand an operator an outbound fetcher as a side effect. It needs the drafts tier but deliberately does not pull it up the way finalize does, and warns rather than ignoring the flag in silence. LEXWARE_UPLOAD_ALLOWED_HOSTS configures the allow-list. Setting it replaces the Microsoft defaults instead of extending them, so those domains can be opted out of; an empty value blocks every host, which is how the fetcher is switched off without unregistering it. `??` and not `||` for exactly that, with a test pinning the behaviour and a startup warning so a typo is not mistaken for an open door. The guards from #34 are unchanged and still run first, at every hop: allow-list matched on a dot boundary, then the resolved-address range check. Pinning is a third layer, not a replacement for either — and the wiring between the check and the connection has its own test, because that seam is exactly what a refactor could quietly unhook with every other test still green. npm run build, npm test (312 tests / 18 files, up from 251 / 15) and docker build all pass. --- .env.example | 14 + CHANGELOG.md | 37 ++ README.md | 34 +- src/config.ts | 64 +++- src/tools/index.ts | 8 + src/tools/url-upload.ts | 56 +++ src/uploads/fetch-url.ts | 365 ++++++++++++++++++ tests/config.test.ts | 60 ++- tests/tools.test.ts | 26 ++ tests/uploads-fetch-url-pinning.test.ts | 83 +++++ tests/uploads-fetch-url.test.ts | 474 ++++++++++++++++++++++++ 11 files changed, 1217 insertions(+), 4 deletions(-) create mode 100644 src/tools/url-upload.ts create mode 100644 src/uploads/fetch-url.ts create mode 100644 tests/uploads-fetch-url-pinning.test.ts create mode 100644 tests/uploads-fetch-url.test.ts diff --git a/.env.example b/.env.example index 38e5922..e3e8839 100644 --- a/.env.example +++ b/.env.example @@ -84,6 +84,20 @@ MCP_AUTH_TOKEN= # Finalize / legally-binding write tools (issue invoices). IRREVERSIBLE. Default: false # LEXWARE_ENABLE_FINALIZE=false +# upload-file-from-url: fetch a file from a share link server-side and store it in +# Lexware. OFF by default, and the only tool that makes this server originate an outbound +# request to an address the model chose. Requires the drafts tier (it will not turn it on). +# The connection is pinned to the address that passed the checks, so DNS cannot answer +# differently between the check and the connection. Default: false +# LEXWARE_ENABLE_URL_UPLOAD=false + +# Hosts upload-file-from-url may fetch from, comma-separated. Unset = the Microsoft +# file-sharing defaults below. Setting it REPLACES those defaults (so they can be opted +# out of); an EMPTY value blocks every host, which is how you switch the fetcher off +# without unregistering it. Matched on a dot boundary: evilsharepoint.com never passes +# as sharepoint.com. +# LEXWARE_UPLOAD_ALLOWED_HOSTS=sharepoint.com,onedrive.live.com,1drv.ms,graph.microsoft.com + # ---- Server ---- # Listen port. Cloud Run injects this automatically. Default: 8080 # PORT=8080 diff --git a/CHANGELOG.md b/CHANGELOG.md index cba754e..2f216c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,43 @@ All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **`upload-file-from-url`, with the DNS-rebinding TOCTOU closed** — the tool held back from + [#34] in 0.1.11, returning with the connection-level IP pinning that release asked for. It fetches a + file from a share link server-side and stores it in Lexware, so a receipt already sitting in + OneDrive/SharePoint reaches the books without its bytes passing through the model context. + **Off by default** (`LEXWARE_ENABLE_URL_UPLOAD=false`) and gated separately from the drafts tier it + writes through: it is the only tool that makes this server originate an outbound request to a + destination the model chose, and that is not something to acquire as a side effect of enabling + drafts. Setting it without the drafts tier warns rather than silently doing nothing. +- **`LEXWARE_UPLOAD_ALLOWED_HOSTS`** — the hosts that tool may fetch from, comma-separated, matched on + a dot boundary (`evilsharepoint.com` never passes as `sharepoint.com`). Unset means the built-in + Microsoft file-sharing list; a configured value **replaces** it rather than extending, so those + domains can be opted out of; an **empty** value blocks every host, which is how the fetcher is + switched off without unregistering it (and warns, so a typo is not mistaken for an open door). + +### Security +- **The connection is pinned to the address that was checked.** The 0.1.11 note recorded why the + fetcher was withheld: its guards resolved the host, validated every returned address, and then let + `fetch` resolve the name a second time when it opened the socket — so the address that was + *approved* and the address that was *connected to* came from two different lookups, and a DNS + answer that changed in between (short TTL, or deliberate rebinding) walked past a check that + looked correct. `src/uploads/pinned-fetch.ts` removes the second lookup: the socket is given the + addresses the check just approved and never resolves anything. There is no longer a window between + the two, because there is no longer a second lookup to disagree with the first. + - Implemented on `node:https`'s `lookup` hook rather than an undici dispatcher, so **no runtime + dependency is added** to a server that fronts accounting data — and no second copy of undici + enters the process beside the one backing `fetch`. + - **TLS is untouched.** SNI and certificate validation still bind to the hostname; only the address + dialled comes from the pin. A test reads the SNI out of the raw ClientHello on the wire to hold + that property, rather than trusting the client to report on itself. + - Sockets are never pooled across requests (`keepAlive: false`, a fresh agent per request), so a + connection opened for a differently vetted request cannot be reused. +- The allow-list and per-hop address checks from #34 are unchanged and still apply first; pinning is a + third layer, not a replacement for either. + ## [0.1.11] Upload a receipt without pushing its bytes through the model context. Based on the diff --git a/README.md b/README.md index 7796335..0af3805 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,8 @@ Related projects — local (stdio) Lexware MCP servers: ## Capabilities -62 tools across three tiers you enable via environment variables: +62 tools across three tiers you enable via environment variables, plus one opt-in tool +outside them (`upload-file-from-url`, see below): | Tier | Default | What it covers | |------|---------|----------------| @@ -42,6 +43,35 @@ Related projects — local (stdio) Lexware MCP servers: Set `LEXWARE_READ_ONLY=true` to force read-only (overrides the flags above). +#### `upload-file-from-url` — outside the tiers, off by default + +One tool sits outside this table: `upload-file-from-url` fetches a file from a share link +**server-side** and stores it in Lexware, which is how a receipt already sitting in +OneDrive/SharePoint gets into the books without a round trip through the model. It is +enabled with `LEXWARE_ENABLE_URL_UPLOAD=true` and needs the drafts tier (it will not turn +that tier on for you). + +It has its own switch because it has its own risk: it is the only tool that makes this +server originate an outbound request to a destination the model chose — server-side +request forgery, in the general case. Three things bound it, applied at **every** redirect +hop: + +1. **A host allow-list**, matched on a dot boundary so `evilsharepoint.com` cannot pass as + `sharepoint.com`. Configure with `LEXWARE_UPLOAD_ALLOWED_HOSTS`; unset means the + Microsoft file-sharing defaults, a set value replaces them, an empty value blocks + everything. +2. **A resolved-address check** rejecting loopback, private, link-local (including the + `169.254.169.254` metadata endpoint), CGNAT, multicast and reserved space, in every + IPv4, IPv6 and IPv4-in-IPv6 spelling. +3. **Connection pinning.** The socket connects to an address from the very lookup that + step 2 approved, rather than letting the HTTP client resolve the name again. Without + this, steps 1 and 2 describe one lookup and the connection uses another, and a DNS + answer that changes in between (rebinding) slips past a check that looked correct. + TLS is unaffected: the certificate is still validated against the hostname. + +Only `https` is accepted, the download is capped at 20 MB and 30 s, and redirects are +followed manually — at most three — so no hop skips the checks. + ### What that looks like in practice > *"Summarize my open invoices for this quarter — who still owes what?"* @@ -151,6 +181,8 @@ LEXWARE_API_KEY=... MCP_AUTH_TOKEN=... npm start | `LEXWARE_READ_ONLY` | `false` | Register only read tools (hard override) | | `LEXWARE_ENABLE_DRAFTS` | `true` | Enable create-draft tools | | `LEXWARE_ENABLE_FINALIZE` | `false` | Enable finalize / legally-binding tools (also enables Drafts) | +| `LEXWARE_ENABLE_URL_UPLOAD` | `false` | Enable `upload-file-from-url` (server-side fetch). Requires the Drafts tier; does not enable it | +| `LEXWARE_UPLOAD_ALLOWED_HOSTS` | Microsoft file-sharing hosts | Hosts `upload-file-from-url` may fetch from, comma-separated. Replaces the defaults; empty blocks everything | | `LEXWARE_API_BASE_URL` | `https://api.lexware.io` | API base URL | | `LEXWARE_APP_BASE_URL` | `https://app.lexware.de` | Web-app base for document deeplinks | | `PORT` | `8080` | Listen port (your platform may inject this) | diff --git a/src/config.ts b/src/config.ts index e7b15ce..414ffc3 100644 --- a/src/config.ts +++ b/src/config.ts @@ -5,6 +5,10 @@ * any Skybridge/Express imports so it can be unit-tested in isolation. */ +// The allow-list default lives with the fetcher that enforces it, so the documented +// default and the applied default cannot drift apart. +import { DEFAULT_ALLOWED_HOSTS } from "./uploads/fetch-url.js"; + /** Minimum length for `MCP_AUTH_TOKEN`. A 32-hex-char token is 32 chars. */ export const MIN_TOKEN_LENGTH = 16; @@ -24,6 +28,14 @@ export interface Capabilities { drafts: boolean; /** Finalize / legally-binding write tools. */ finalize: boolean; + /** + * `upload-file-from-url`, the server-side URL fetcher. Off by default and gated + * separately from the rest of the drafts tier, because it is the one tool that makes + * this server originate outbound requests to a location the model chose — a class of + * risk (SSRF) the other write tools simply do not have. An operator who wants drafts + * should not silently get an outbound fetcher along with them. + */ + urlUpload: boolean; } /** @@ -98,6 +110,13 @@ export interface Config { * that resolves nowhere but inside the container. */ publicBaseUrl: string; + /** + * Hosts `upload-file-from-url` may fetch from (`LEXWARE_UPLOAD_ALLOWED_HOSTS`, + * comma-separated). Unset means the built-in Microsoft file-sharing list; see + * {@link resolveUploadAllowedHosts} for why setting it REPLACES rather than extends, + * and why an empty value blocks everything. + */ + uploadAllowedHosts: string[]; port: number; debugLogging: boolean; capabilities: Capabilities; @@ -208,6 +227,29 @@ function resolvePublicBaseUrl(env: NodeJS.ProcessEnv, port: number): string { return `http://127.0.0.1:${boundPort >= 1 && boundPort <= 65535 ? boundPort : port}`; } +/** + * Hosts `upload-file-from-url` may fetch from. + * + * Three properties, each chosen deliberately: + * + * - **Unset means the built-in Microsoft file-sharing list**, so an operator who never + * touches the variable behaves exactly as if it did not exist. + * - **A configured list REPLACES the defaults, it does not extend them.** Extending + * would make Microsoft's domains impossible to opt out of, which is the wrong default + * for a self-hosted server that may have nothing to do with M365. + * - **An empty value blocks every host**, disabling the tool. An allow-list that cannot + * be emptied cannot be used to switch the feature off, and "empty means allow + * everything" would turn a typo into an open SSRF surface. Hence `??` and not `||`. + */ +function resolveUploadAllowedHosts(env: NodeJS.ProcessEnv): string[] { + const raw = env.LEXWARE_UPLOAD_ALLOWED_HOSTS; + if (raw === undefined) return DEFAULT_ALLOWED_HOSTS; + return raw + .split(",") + .map((h) => h.trim().toLowerCase()) + .filter(Boolean); +} + /** Resolve how `/mcp` is authenticated, failing closed if nothing is configured. */ function resolveAuth(env: NodeJS.ProcessEnv): AuthConfig { const issuerRaw = env.OAUTH_ISSUER?.trim(); @@ -338,8 +380,26 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { // irreversible create-finalized-* tools (no safe draft path). Never allow that. const draftsRequested = parseBool(env.LEXWARE_ENABLE_DRAFTS, true); const enableDrafts = readOnly ? false : draftsRequested || enableFinalize; + // Opt-in, and only meaningful inside the drafts tier (it writes a file to Lexware). + // Unlike finalize→drafts, this one does NOT pull drafts up: an outbound fetcher is + // not something to enable as a side effect of a flag about uploads. + const urlUploadRequested = parseBool(env.LEXWARE_ENABLE_URL_UPLOAD, false); + const enableUrlUpload = enableDrafts && urlUploadRequested; + const uploadAllowedHosts = resolveUploadAllowedHosts(env); const warnings: string[] = []; + if (urlUploadRequested && !enableDrafts) { + warnings.push( + "LEXWARE_ENABLE_URL_UPLOAD=true has no effect: upload-file-from-url writes a file to Lexware and " + + "lives in the drafts tier, which is disabled (LEXWARE_READ_ONLY / LEXWARE_ENABLE_DRAFTS).", + ); + } + if (enableUrlUpload && uploadAllowedHosts.length === 0) { + warnings.push( + "LEXWARE_ENABLE_URL_UPLOAD=true but LEXWARE_UPLOAD_ALLOWED_HOSTS is empty — every host is blocked, " + + "so upload-file-from-url is registered but will refuse every URL.", + ); + } if (!readOnly && !draftsRequested && enableFinalize) { warnings.push( "LEXWARE_ENABLE_DRAFTS=false was overridden to true because LEXWARE_ENABLE_FINALIZE=true — the " + @@ -370,9 +430,10 @@ export function loadConfig(env: NodeJS.ProcessEnv = process.env): Config { ), auth, publicBaseUrl: resolvePublicBaseUrl(env, port), + uploadAllowedHosts, port, debugLogging: parseBool(env.LEXWARE_DEBUG_LOGGING, false), - capabilities: { read: true, drafts: enableDrafts, finalize: enableFinalize }, + capabilities: { read: true, drafts: enableDrafts, finalize: enableFinalize, urlUpload: enableUrlUpload }, warnings, }; } @@ -382,6 +443,7 @@ export function describeCapabilities(config: Config): string { const tiers = ["read"]; if (config.capabilities.drafts) tiers.push("drafts"); if (config.capabilities.finalize) tiers.push("finalize"); + if (config.capabilities.urlUpload) tiers.push("url-upload"); const auth = config.auth.mode === "oauth" ? "oauth" diff --git a/src/tools/index.ts b/src/tools/index.ts index 5da5c38..d2b0045 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -22,6 +22,7 @@ import { registerFileReadTools, registerFileWriteTools } from "./files.js"; import { registerProfileTools } from "./profile.js"; import { registerReferenceReadTools } from "./reference.js"; import { registerUploadTools } from "./uploads.js"; +import { registerUrlUploadTool } from "./url-upload.js"; import { registerVoucherWriteTools } from "./vouchers.js"; /** @@ -55,6 +56,13 @@ export function registerTools( registerUploadTools(server, uploadTickets, config.publicBaseUrl); } + // Gated separately from the drafts tier, not nested inside it: config.capabilities + // already resolves urlUpload to false whenever drafts are off, so the flat check + // states the actual precondition instead of restating it in two places. + if (capabilities.urlUpload) { + registerUrlUploadTool(server, client, config.uploadAllowedHosts); + } + // Finalize / sensitive & irreversible tier (off by default). if (capabilities.finalize) { registerDocumentFinalizeTools(server, client); diff --git a/src/tools/url-upload.ts b/src/tools/url-upload.ts new file mode 100644 index 0000000..fd8ffa3 --- /dev/null +++ b/src/tools/url-upload.ts @@ -0,0 +1,56 @@ +import type { McpServer } from "skybridge/server"; +import { z } from "zod"; +import type { LexwareClient } from "../lexware/client.js"; +import { fetchRemoteFile } from "../uploads/fetch-url.js"; +import { text, WRITE } from "./shared.js"; + +/** + * `upload-file-from-url` — the server-side URL fetcher. + * + * Deliberately its own file and its own capability gate (`LEXWARE_ENABLE_URL_UPLOAD`, + * off by default) rather than part of the ticket flow in `uploads.ts`. The ticket flow + * only ever *receives* bytes; this tool makes the server *originate* an outbound request + * to a location the model picked, which is a different risk class (SSRF) and deserves a + * switch of its own. Keeping the two apart means an operator can read the file tree and + * see which one is which. + * + * The guards live in `fetch-url.ts`: allow-list, per-hop address checks, and pinning the + * connection to the address that was checked. + */ +export function registerUrlUploadTool( + server: McpServer, + client: LexwareClient, + allowedHosts: string[], +): void { + server.registerTool( + { + name: "upload-file-from-url", + description: + "Download a file from a pre-authenticated share link and store it in Lexware, without the bytes " + + "passing through the model context — e.g. an email attachment saved to OneDrive/SharePoint via the " + + "Microsoft 365 tools. Only https URLs on an allow-listed host are accepted, re-checked after every " + + `redirect; arbitrary URLs are refused by design. Allow-listed here: ${allowedHosts.join(", ") || "(none — every URL is refused)"}. ` + + "Limit 20 MB. If the file is on this machine rather than behind a link, use create-upload-ticket.", + inputSchema: { + url: z.string().describe("Public https URL of the file (allow-listed hosts only)."), + filename: z.string().optional().describe("Overrides the name derived from the response."), + mimeType: z.string().optional().describe("Overrides the content type from the response."), + type: z.string().default("voucher").describe('Lexware file category. "voucher" for bookkeeping receipts.'), + }, + annotations: WRITE, + }, + async ({ url, filename, mimeType, type }: { url: string; filename?: string; mimeType?: string; type: string }) => { + const fetched = await fetchRemoteFile(url, { allowedHosts }); + const name = filename ?? fetched.filename ?? new URL(url).pathname.split("/").pop() ?? "download.bin"; + const created = await client.postMultipart<{ id: string }>( + "/v1/files", + { bytes: fetched.bytes, filename: name, contentType: mimeType ?? fetched.contentType }, + { type }, + ); + return { + structuredContent: { fileId: created.id, filename: name, byteLength: fetched.bytes.byteLength }, + content: text(`Uploaded file ${created.id} (${name}, ${fetched.bytes.byteLength} bytes) from ${url}.`), + }; + }, + ); +} diff --git a/src/uploads/fetch-url.ts b/src/uploads/fetch-url.ts new file mode 100644 index 0000000..972bd5d --- /dev/null +++ b/src/uploads/fetch-url.ts @@ -0,0 +1,365 @@ +import { lookup as dnsLookup } from "node:dns/promises"; +import { isIPv4, isIPv6 } from "node:net"; +import { sanitizeFilename } from "./filename.js"; +import { createPinnedFetch, type FetchLike } from "./pinned-fetch.js"; + +const DEFAULT_MAX_BYTES = 20 * 1024 * 1024; +const DEFAULT_TIMEOUT_MS = 30_000; +const DEFAULT_MAX_REDIRECTS = 3; + +/** + * Hosts a pre-authenticated download link is allowed to point at. The real use case is + * OneDrive/SharePoint share links. Callers may override via `fetchRemoteFile`'s + * `allowedHosts` option (`LEXWARE_UPLOAD_ALLOWED_HOSTS` in the environment). + */ +export const DEFAULT_ALLOWED_HOSTS = ["sharepoint.com", "onedrive.live.com", "1drv.ms", "graph.microsoft.com"]; + +/** + * True when `hostname` is exactly one of `allowed`, or a subdomain of one of them + * (matched on a dot boundary — "evilsharepoint.com" must NOT match "sharepoint.com"). + * Case-insensitive; a trailing dot on `hostname` (a valid DNS root-label terminator) is + * stripped before comparison. + */ +export function isAllowedHost(hostname: string, allowed: string[]): boolean { + let host = hostname.trim().toLowerCase(); + if (host.endsWith(".")) host = host.slice(0, -1); + for (const entry of allowed) { + const suffix = entry.trim().toLowerCase(); + if (!suffix) continue; + if (host === suffix || host.endsWith(`.${suffix}`)) return true; + } + return false; +} + +/** Parses a dotted-decimal IPv4 string into its four octets. */ +function parseIPv4Octets(ip: string): [number, number, number, number] | null { + const parts = ip.split("."); + if (parts.length !== 4) return null; + const nums = parts.map((p) => Number(p)); + if (nums.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) return null; + return nums as [number, number, number, number]; +} + +/** + * Range checks shared between plain IPv4 addresses and IPv4 addresses embedded in an + * IPv6 literal (mapped, compatible, or NAT64). Covers loopback, the RFC 1918 private + * ranges, link-local (incl. the 169.254.169.254 cloud metadata endpoint), carrier-grade + * NAT (100.64.0.0/10), IETF protocol assignments, benchmarking, multicast and the + * reserved/broadcast block. + */ +function isBlockedIPv4Bytes(a: number, b: number, c: number, _d: number): boolean { + if (a === 0 || a === 127) return true; + if (a === 10) return true; + if (a === 172 && b >= 16 && b <= 31) return true; + if (a === 192 && b === 168) return true; + if (a === 169 && b === 254) return true; + if (a === 100 && b >= 64 && b <= 127) return true; + if (a === 192 && b === 0 && c === 0) return true; + if (a === 198 && (b === 18 || b === 19)) return true; + if (a >= 224 && a <= 239) return true; + if (a >= 240) return true; // 240.0.0.0/4 reserved, includes 255.255.255.255 + return false; +} + +/** + * Expands an IPv6 literal (RFC 4291 text form, including `::` compression and an + * embedded trailing IPv4 dotted-quad group) into its 16 bytes. Returns null for + * anything that doesn't parse as exactly 8 groups — callers must treat null as blocked, + * not as "not an address". + */ +function parseIPv6ToBytes(ip: string): number[] | null { + const percentIdx = ip.indexOf("%"); + const text = percentIdx === -1 ? ip : ip.slice(0, percentIdx); + + const dcIdx = text.indexOf("::"); + const hasDoubleColon = dcIdx !== -1; + const leftPart = hasDoubleColon ? text.slice(0, dcIdx) : text; + const rightPart = hasDoubleColon ? text.slice(dcIdx + 2) : ""; + + const leftGroups = leftPart === "" ? [] : leftPart.split(":"); + const rightGroups = rightPart === "" ? [] : rightPart.split(":"); + + // An embedded IPv4 dotted-quad, if present, is always the final group. + const target = rightGroups.length > 0 ? rightGroups : leftGroups; + if (target.length > 0 && target[target.length - 1].includes(".")) { + const v4 = target.pop()!; + const octets = parseIPv4Octets(v4); + if (!octets) return null; + const [o0, o1, o2, o3] = octets; + target.push(((o0 << 8) | o1).toString(16), ((o2 << 8) | o3).toString(16)); + } + + let allGroups: string[]; + if (!hasDoubleColon) { + if (leftGroups.length !== 8) return null; + allGroups = leftGroups; + } else { + const known = leftGroups.length + rightGroups.length; + if (known > 8) return null; + allGroups = [...leftGroups, ...new Array(8 - known).fill("0"), ...rightGroups]; + } + if (allGroups.length !== 8) return null; + + const bytes: number[] = []; + for (const g of allGroups) { + if (!/^[0-9a-fA-F]{1,4}$/.test(g)) return null; + const n = parseInt(g, 16); + bytes.push((n >> 8) & 0xff, n & 0xff); + } + return bytes; +} + +/** + * Range checks over the 16-byte form of an IPv6 address. Any address whose first 96 bits + * (mapped: first 80 bits + ffff; compatible: first 96 bits) are zero-with-ffff-marker or + * fully zero is really an IPv4 address in disguise and is delegated to the IPv4 check — + * this is what makes `::ffff:127.0.0.1`, `::ffff:a9fe:a9fe`, `::127.0.0.1`, `::1` and `::` + * (in any of their hex/dotted/expanded spellings) fold onto the same, already-correct + * IPv4 logic instead of needing to be special-cased by string pattern. + */ +function isBlockedIPv6Bytes(bytes: number[]): boolean { + const first10Zero = bytes.slice(0, 10).every((b) => b === 0); + if (first10Zero && bytes[10] === 0xff && bytes[11] === 0xff) { + return isBlockedIPv4Bytes(bytes[12], bytes[13], bytes[14], bytes[15]); // ::ffff:a.b.c.d + } + const first12Zero = bytes.slice(0, 12).every((b) => b === 0); + if (first12Zero) { + return isBlockedIPv4Bytes(bytes[12], bytes[13], bytes[14], bytes[15]); // ::a.b.c.d, ::1, :: + } + // 64:ff9b::/96 — well-known NAT64 prefix; block wholesale rather than trusting + // whatever IPv4 address is embedded in the low 32 bits. + if ( + bytes[0] === 0x00 && + bytes[1] === 0x64 && + bytes[2] === 0xff && + bytes[3] === 0x9b && + bytes.slice(4, 12).every((b) => b === 0) + ) { + return true; + } + if (bytes[0] === 0x20 && bytes[1] === 0x02) return true; // 2002::/16 (6to4) + if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0x80) return true; // fe80::/10 link-local + if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0xc0) return true; // fec0::/10 deprecated site-local + if ((bytes[0] & 0xfe) === 0xfc) return true; // fc00::/7 unique local + return false; +} + +/** + * True for addresses a server-side fetch must never reach: loopback, private, + * link-local (including the 169.254.169.254 cloud metadata endpoint), carrier-grade + * NAT, multicast/reserved, and their IPv6 equivalents — including every IPv4-in-IPv6 + * spelling (mapped, compatible, NAT64) and non-canonical hex/expanded forms. Checked + * for EVERY hop, not just the first — a public URL is free to redirect somewhere + * internal. + * + * Fails closed: anything that is not a syntactically valid IPv4 or IPv6 address + * (including the empty string) counts as blocked rather than as "not an address" — + * an allow-by-default parser is exactly the kind of gap DNS rebinding and exotic + * address spellings exploit. + */ +export function isBlockedAddress(rawIp: string): boolean { + const trimmed = rawIp.trim(); + if (!trimmed) return true; + if (isIPv4(trimmed)) { + const octets = parseIPv4Octets(trimmed); + if (!octets) return true; + return isBlockedIPv4Bytes(...octets); + } + if (isIPv6(trimmed)) { + const bytes = parseIPv6ToBytes(trimmed); + if (!bytes) return true; + return isBlockedIPv6Bytes(bytes); + } + return true; +} + +export function assertFetchableUrl(raw: string): URL { + let url: URL; + try { + url = new URL(raw); + } catch { + throw new Error(`Not a valid URL: ${raw}`); + } + if (url.protocol !== "https:") { + throw new Error(`URL scheme ${url.protocol} is not allowed — only https.`); + } + return url; +} + +async function defaultLookup(host: string): Promise { + const records = await dnsLookup(host, { all: true }); + return records.map((r) => r.address); +} + +/** + * Parses a Content-Disposition header for a filename. Per RFC 6266, `filename*` + * (percent-encoded, RFC 5987 extended value syntax) takes precedence over plain + * `filename` when both are present. Only the `filename*` form is percent-decoded — + * applying `decodeURIComponent` to the plain form as well was the bug: a completely + * ordinary name like `100% Rabatt.pdf` would throw a URIError on the lone `%` and fail + * the whole download *after* it had already succeeded. Decoding is wrapped in try/catch + * so a malformed extended value degrades to the raw string instead of failing the call. + * + * The extended value's full grammar is `charset'language'percent-encoded-value` + * (RFC 5987 §3.2.1) and the language part is OPTIONAL BUT COMMONLY SET — German + * servers routinely send `filename*=UTF-8'de'Rechnung.pdf`. Matching only the + * literal `UTF-8''` prefix, as this did, left `UTF-8'de'` glued to the front and + * filed the receipt as `UTF-8'de'Rechnung.pdf`. Both delimiters are therefore + * split off generically; a value with no apostrophes at all (malformed, but seen + * in the wild) is still taken verbatim rather than dropped. Percent-decoding + * always assumes UTF-8: a non-UTF-8 charset label is rare enough that the + * try/catch fallback to the raw string is the better trade against carrying a + * transcoder. + */ +function filenameFromDisposition(value: string | null): string | undefined { + if (!value) return undefined; + + const extMatch = /filename\*\s*=\s*([^;]+)/i.exec(value); + if (extMatch) { + let raw = extMatch[1].trim().replace(/^"|"$/g, ""); + const parts = /^([^']*)'([^']*)'([\s\S]*)$/.exec(raw); + if (parts) raw = parts[3]; + try { + raw = decodeURIComponent(raw); + } catch { + // Malformed percent-encoding: keep the raw value rather than failing the download. + } + return sanitizeFilename(raw); + } + + const plainMatch = /filename\s*=\s*"?([^";]+)"?/i.exec(value); + if (plainMatch) return sanitizeFilename(plainMatch[1].trim()); + + return undefined; +} + +/** Discards a response body we're not going to use so the socket can be released promptly. */ +async function drain(res: Response): Promise { + try { + await res.body?.cancel(); + } catch { + // Best-effort: a body already errored/closed is fine to ignore. + } +} + +/** + * Fetch a remote file with SSRF guards. Redirects are followed manually so every + * hop's host and address can be re-validated before the request is made. + * + * Three layers, applied at EVERY hop: + * + * 1. **Host allowlist** (`allowedHosts`, default: Microsoft file-sharing domains) — + * the primary control. Nothing off the list is ever contacted. + * 2. **Resolved-address check** (`isBlockedAddress`) — rejects a host that resolves + * into loopback, private, link-local or other non-routable space. + * 3. **Connection pinning** (`createPinnedFetch`) — the connection is made to an + * address from the very lookup that layer 2 just approved, instead of letting the + * HTTP client resolve the name a second time. Without this, layers 1 and 2 describe + * a lookup that the socket is free to disagree with (DNS rebinding); with it there + * is only one lookup, so there is nothing to disagree about. + * + * `fetchImpl` bypasses layer 3 and exists for tests. Production passes nothing. + */ +export async function fetchRemoteFile( + rawUrl: string, + opts: { + maxBytes?: number; + timeoutMs?: number; + maxRedirects?: number; + lookup?: (host: string) => Promise; + fetchImpl?: FetchLike; + allowedHosts?: string[]; + } = {}, +): Promise<{ bytes: Uint8Array; contentType: string; filename?: string }> { + const maxBytes = opts.maxBytes ?? DEFAULT_MAX_BYTES; + const timeoutMs = opts.timeoutMs ?? DEFAULT_TIMEOUT_MS; + const maxRedirects = opts.maxRedirects ?? DEFAULT_MAX_REDIRECTS; + const lookup = opts.lookup ?? defaultLookup; + const allowedHosts = opts.allowedHosts ?? DEFAULT_ALLOWED_HOSTS; + + let url = assertFetchableUrl(rawUrl); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), timeoutMs); + + try { + for (let hop = 0; hop <= maxRedirects; hop++) { + if (!isAllowedHost(url.hostname, allowedHosts)) { + throw new Error(`Host ${url.hostname} is not allowed — not on the configured allowlist.`); + } + + const addresses = await lookup(url.hostname); + if (addresses.length === 0) throw new Error(`Host ${url.hostname} did not resolve.`); + // Deliberately no detail about which address: do not leak internal topology. + if (addresses.some(isBlockedAddress)) { + throw new Error(`Target address is not allowed (private, loopback or link-local).`); + } + + // The addresses just checked are the addresses the socket dials — see layer 3 above. + const doFetch = opts.fetchImpl ?? createPinnedFetch(url.hostname, addresses); + const res = await doFetch(url, { redirect: "manual", signal: controller.signal }); + + if (res.status >= 300 && res.status < 400) { + const location = res.headers.get("location"); + if (!location) throw new Error(`Redirect without a location header.`); + await drain(res); + url = assertFetchableUrl(new URL(location, url).toString()); + continue; + } + + if (!res.ok) { + await drain(res); + throw new Error(`Download failed with HTTP ${res.status}.`); + } + + const declared = Number(res.headers.get("content-length") ?? "0"); + if (declared > maxBytes) { + await drain(res); + throw new Error(`File is too large (${declared} bytes, limit ${maxBytes}).`); + } + + // Stream the body and cut it off as soon as maxBytes is exceeded, instead of + // buffering the whole thing via res.arrayBuffer() first — a lying (or absent) + // content-length must not be able to force an unbounded read into memory. + const chunks: Uint8Array[] = []; + let total = 0; + if (res.body) { + const reader = res.body.getReader(); + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + if (!value) continue; + total += value.byteLength; + if (total > maxBytes) { + controller.abort(); + throw new Error(`File is too large (limit ${maxBytes} bytes).`); + } + chunks.push(value); + } + } finally { + try { + await reader.cancel(); + } catch { + // Best-effort. + } + } + } + + const buf = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + buf.set(chunk, offset); + offset += chunk.byteLength; + } + + return { + bytes: buf, + contentType: res.headers.get("content-type")?.split(";")[0].trim() || "application/octet-stream", + filename: filenameFromDisposition(res.headers.get("content-disposition")), + }; + } + throw new Error(`Too many redirects (limit ${maxRedirects}).`); + } finally { + clearTimeout(timer); + } +} diff --git a/tests/config.test.ts b/tests/config.test.ts index 2f84fed..45606de 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -11,7 +11,7 @@ describe("loadConfig", () => { expect(c.lexwareApiBaseUrl).toBe("https://api.lexware.io"); expect(c.lexwareAppBaseUrl).toBe("https://app.lexware.de"); expect(c.port).toBe(8080); - expect(c.capabilities).toEqual({ read: true, drafts: true, finalize: false }); + expect(c.capabilities).toEqual({ read: true, drafts: true, finalize: false, urlUpload: false }); }); it("requires LEXWARE_API_KEY", () => { @@ -198,7 +198,7 @@ describe("loadConfig", () => { LEXWARE_ENABLE_DRAFTS: "true", LEXWARE_ENABLE_FINALIZE: "true", } as NodeJS.ProcessEnv); - expect(c.capabilities).toEqual({ read: true, drafts: false, finalize: false }); + expect(c.capabilities).toEqual({ read: true, drafts: false, finalize: false, urlUpload: false }); }); it("enables finalize when requested", () => { @@ -339,6 +339,62 @@ describe("loadConfig", () => { expect(c.lexwareApiBaseUrl).toBe("http://localhost:9000"); }); + it("LEXWARE_ENABLE_URL_UPLOAD is off by default and on only when asked for", () => { + expect(loadConfig(base()).capabilities.urlUpload).toBe(false); + expect( + loadConfig({ ...base(), LEXWARE_ENABLE_URL_UPLOAD: "true" } as NodeJS.ProcessEnv).capabilities.urlUpload, + ).toBe(true); + }); + + it("URL upload cannot outlive the drafts tier it writes through, and says so", () => { + for (const off of [{ LEXWARE_READ_ONLY: "true" }, { LEXWARE_ENABLE_DRAFTS: "false" }]) { + const c = loadConfig({ ...base(), ...off, LEXWARE_ENABLE_URL_UPLOAD: "true" } as NodeJS.ProcessEnv); + expect(c.capabilities.urlUpload, JSON.stringify(off)).toBe(false); + // Silently ignoring the flag would leave an operator believing the tool is live. + expect(c.warnings.join(" ")).toMatch(/LEXWARE_ENABLE_URL_UPLOAD=true has no effect/); + } + }); + + it("unlike finalize, URL upload does NOT pull the drafts tier up with it", () => { + const c = loadConfig({ + ...base(), + LEXWARE_ENABLE_DRAFTS: "false", + LEXWARE_ENABLE_URL_UPLOAD: "true", + } as NodeJS.ProcessEnv); + expect(c.capabilities.drafts).toBe(false); + }); + + it("the upload allowlist defaults to the Microsoft file-sharing hosts when unset", () => { + expect(loadConfig(base()).uploadAllowedHosts).toEqual([ + "sharepoint.com", + "onedrive.live.com", + "1drv.ms", + "graph.microsoft.com", + ]); + }); + + it("a configured allowlist REPLACES the defaults rather than extending them", () => { + const c = loadConfig({ + ...base(), + LEXWARE_UPLOAD_ALLOWED_HOSTS: "files.example.com, CDN.Example.org ", + } as NodeJS.ProcessEnv); + expect(c.uploadAllowedHosts).toEqual(["files.example.com", "cdn.example.org"]); + // The point of replacing: Microsoft's domains must be opt-out-able. + expect(c.uploadAllowedHosts).not.toContain("sharepoint.com"); + }); + + it("an EMPTY allowlist blocks every host instead of meaning 'allow everything'", () => { + const c = loadConfig({ + ...base(), + LEXWARE_UPLOAD_ALLOWED_HOSTS: "", + LEXWARE_ENABLE_URL_UPLOAD: "true", + } as NodeJS.ProcessEnv); + expect(c.uploadAllowedHosts).toEqual([]); + // A typo that empties the list must not silently become an open SSRF surface, so the + // fail-closed reading is paired with a warning rather than left to be discovered. + expect(c.warnings.join(" ")).toMatch(/every host is blocked/); + }); + it("describeCapabilities is secret-free and informative", () => { const c = loadConfig(base()); const s = describeCapabilities(c); diff --git a/tests/tools.test.ts b/tests/tools.test.ts index 1502174..0ab5daa 100644 --- a/tests/tools.test.ts +++ b/tests/tools.test.ts @@ -119,6 +119,32 @@ describe("registerTools (tiered registration)", () => { expect(names).toEqual([...READ_TOOLS].sort()); }); + it("does NOT register upload-file-from-url by default — the outbound fetcher is opt-in", () => { + const names = registeredNames(loadConfig(env())); + expect(names).not.toContain("upload-file-from-url"); + }); + + it("registers upload-file-from-url only when LEXWARE_ENABLE_URL_UPLOAD is on", () => { + const names = registeredNames(loadConfig(env({ LEXWARE_ENABLE_URL_UPLOAD: "true" }))); + expect(names).toEqual([...READ_TOOLS, ...DRAFT_TOOLS, "upload-file-from-url"].sort()); + }); + + it("does not register it in read-only mode, even when explicitly enabled", () => { + // The flag says "yes" and the tier says "no". A tool that writes a file into the + // bookkeeping must never win that argument. + const names = registeredNames( + loadConfig(env({ LEXWARE_READ_ONLY: "true", LEXWARE_ENABLE_URL_UPLOAD: "true" })), + ); + expect(names).toEqual([...READ_TOOLS].sort()); + }); + + it("does not register it when the drafts tier is off, even when explicitly enabled", () => { + const names = registeredNames( + loadConfig(env({ LEXWARE_ENABLE_DRAFTS: "false", LEXWARE_ENABLE_URL_UPLOAD: "true" })), + ); + expect(names).toEqual([...READ_TOOLS].sort()); + }); + it("finalize implies drafts: enabling finalize with drafts off still registers drafts", () => { // Guards against a config that exposes ONLY the irreversible create-finalized-* // tools (no safe draft path). diff --git a/tests/uploads-fetch-url-pinning.test.ts b/tests/uploads-fetch-url-pinning.test.ts new file mode 100644 index 0000000..0f89715 --- /dev/null +++ b/tests/uploads-fetch-url-pinning.test.ts @@ -0,0 +1,83 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * The wiring test for the third SSRF layer: proves `fetchRemoteFile` hands the addresses + * it just vetted to the connection, instead of letting the HTTP client resolve the host + * a second time. + * + * In its own file because it mocks the transport module, and the rest of the fetch-url + * suite deliberately exercises the real guards. The guarantee inside the transport is + * tested for real in `uploads-pinned-fetch.test.ts`; what is checked here is only that + * the two halves are connected — the exact seam a refactor could quietly unhook, + * restoring the DNS-rebinding TOCTOU with every other test still green. + */ +const createPinnedFetch = vi.hoisted(() => vi.fn()); + +vi.mock("../src/uploads/pinned-fetch.js", () => ({ createPinnedFetch })); + +const { fetchRemoteFile } = await import("../src/uploads/fetch-url.js"); + +/** A body-less 200 the fetcher will accept. */ +function okResponse(): Response { + return new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { "content-type": "application/pdf", "content-length": "3" }, + }); +} + +describe("fetchRemoteFile connection pinning", () => { + beforeEach(() => { + createPinnedFetch.mockReset(); + createPinnedFetch.mockReturnValue(async () => okResponse()); + }); + + it("builds the connection from the SAME lookup result the address check approved", async () => { + const lookup = vi.fn(async () => ["203.0.113.7", "2001:db8::1"]); + + await fetchRemoteFile("https://acme.sharepoint.com/x.pdf", { lookup }); + + // One lookup for the hop, and its result — not the hostname alone — is what the + // transport is built from. A second resolution is what this design removes. + expect(lookup).toHaveBeenCalledTimes(1); + expect(createPinnedFetch).toHaveBeenCalledTimes(1); + expect(createPinnedFetch).toHaveBeenCalledWith("acme.sharepoint.com", [ + "203.0.113.7", + "2001:db8::1", + ]); + }); + + it("re-pins on a redirect, to the new host's own vetted addresses", async () => { + const lookup = vi.fn(async (host: string) => + host === "acme.sharepoint.com" ? ["203.0.113.7"] : ["198.51.100.9"], + ); + createPinnedFetch + .mockReturnValueOnce(async () => new Response(null, { status: 302, headers: { location: "https://acme-my.sharepoint.com/y.pdf" } })) + .mockReturnValueOnce(async () => okResponse()); + + await fetchRemoteFile("https://acme.sharepoint.com/x.pdf", { lookup }); + + expect(createPinnedFetch.mock.calls).toEqual([ + ["acme.sharepoint.com", ["203.0.113.7"]], + ["acme-my.sharepoint.com", ["198.51.100.9"]], + ]); + }); + + it("never reaches the transport when an address fails the range check", async () => { + const lookup = vi.fn(async () => ["203.0.113.7", "169.254.169.254"]); + + await expect(fetchRemoteFile("https://acme.sharepoint.com/x.pdf", { lookup })).rejects.toThrow( + /Target address is not allowed/, + ); + expect(createPinnedFetch).not.toHaveBeenCalled(); + }); + + it("never reaches the transport when the host is off the allowlist", async () => { + const lookup = vi.fn(async () => ["203.0.113.7"]); + + await expect(fetchRemoteFile("https://evil.example.com/x.pdf", { lookup })).rejects.toThrow( + /is not allowed/, + ); + expect(lookup).not.toHaveBeenCalled(); + expect(createPinnedFetch).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/uploads-fetch-url.test.ts b/tests/uploads-fetch-url.test.ts new file mode 100644 index 0000000..6443d29 --- /dev/null +++ b/tests/uploads-fetch-url.test.ts @@ -0,0 +1,474 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_ALLOWED_HOSTS, + fetchRemoteFile, + isAllowedHost, + isBlockedAddress, +} from "../src/uploads/fetch-url.js"; + +describe("isBlockedAddress", () => { + it("blocks loopback, private and link-local ranges", () => { + for (const ip of [ + "127.0.0.1", "127.53.1.9", "::1", + "10.0.0.5", "172.16.4.2", "172.31.255.255", "192.168.1.1", + "169.254.169.254", "fe80::1", "fc00::1", "fd12::9", + "0.0.0.0", + ]) { + expect(isBlockedAddress(ip), ip).toBe(true); + } + }); + + it("allows public addresses", () => { + for (const ip of ["8.8.8.8", "1.1.1.1", "93.184.216.34", "172.32.0.1", "2606:4700::1111"]) { + expect(isBlockedAddress(ip), ip).toBe(false); + } + }); + + // Regression coverage for Critical review finding: isBlockedAddress previously failed + // OPEN on anything it could not parse as a plain dotted-quad or a small set of string + // prefixes. All of these are real, routable spellings of loopback/link-local addresses + // that were measured as NOT blocked before this fix. + it("blocks non-canonical IPv4-in-IPv6 spellings (hex-form mapped, expanded, deprecated-compatible) and fails closed on unparseable input", () => { + for (const ip of [ + "::ffff:7f00:1", // 127.0.0.1, mapped, hex form (not dotted) + "::FFFF:7F00:1", // same, uppercase + "0:0:0:0:0:ffff:127.0.0.1", // 127.0.0.1, mapped, fully expanded + dotted tail + "::ffff:a9fe:a9fe", // 169.254.169.254, mapped, hex form + "::127.0.0.1", // 127.0.0.1, deprecated IPv4-compatible form + "0:0:0:0:0:0:0:1", // ::1 fully expanded + "", // empty string must fail closed, not fail open + ]) { + expect(isBlockedAddress(ip), JSON.stringify(ip)).toBe(true); + } + }); + + it("blocks the additionally required ranges: CGNAT, IETF protocol assignment, benchmarking, multicast, reserved/broadcast, 6to4, NAT64 and deprecated site-local", () => { + for (const ip of [ + "100.64.0.1", "100.127.255.255", // 100.64.0.0/10 (CGNAT) + "192.0.0.5", // 192.0.0.0/24 (IETF protocol assignments) + "198.18.0.1", "198.19.255.255", // 198.18.0.0/15 (benchmarking) + "224.0.0.1", "239.255.255.255", // 224.0.0.0/4 (multicast) + "240.0.0.1", "255.255.255.255", // 240.0.0.0/4 (reserved, incl. broadcast) + "fec0::1", // fec0::/10 (deprecated site-local) + "64:ff9b::808:808", // 64:ff9b::/96 (NAT64 well-known prefix) + "2002:c000:0204::", // 2002::/16 (6to4) + ]) { + expect(isBlockedAddress(ip), ip).toBe(true); + } + }); +}); + +describe("isAllowedHost", () => { + it("matches an exact configured host and a subdomain of it", () => { + expect(isAllowedHost("sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("foo.sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("contoso.sharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(true); + }); + + it("does not match a lookalike domain that merely shares a suffix without a dot boundary", () => { + expect(isAllowedHost("evilsharepoint.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + expect(isAllowedHost("sharepoint.com.evil.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + expect(isAllowedHost("not-allowed.example.com", DEFAULT_ALLOWED_HOSTS)).toBe(false); + }); + + it("is case-insensitive and strips a trailing root-label dot", () => { + expect(isAllowedHost("FOO.SharePoint.COM", DEFAULT_ALLOWED_HOSTS)).toBe(true); + expect(isAllowedHost("sharepoint.com.", DEFAULT_ALLOWED_HOSTS)).toBe(true); + }); +}); + +describe("fetchRemoteFile", () => { + const publicLookup = async () => ["93.184.216.34"]; + + it("rejects non-https schemes", async () => { + await expect(fetchRemoteFile("file:///etc/passwd", { lookup: publicLookup })).rejects.toThrow(/scheme/i); + }); + + it("rejects plain http, even on an allow-listed host", async () => { + await expect(fetchRemoteFile("http://foo.sharepoint.com/x.pdf", { lookup: publicLookup })).rejects.toThrow( + /scheme/i, + ); + }); + + it("rejects a redirect from https down to http", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + seen.push(String(url)); + return new Response(null, { status: 302, headers: { location: "http://foo.sharepoint.com/downgraded.pdf" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://foo.sharepoint.com/start.pdf", { lookup: publicLookup, fetchImpl }), + ).rejects.toThrow(/scheme/i); + expect(seen).toHaveLength(1); + }); + + it("rejects a host that is not on the allowlist", async () => { + await expect( + fetchRemoteFile("https://not-allowed.example.com/x.pdf", { lookup: publicLookup }), + ).rejects.toThrow(/not allowed/i); + }); + + it("allows a host on the default allowlist through", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { "content-type": "application/pdf" }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://foo.sharepoint.com/x.pdf", { lookup: publicLookup, fetchImpl }); + expect(Array.from(out.bytes)).toEqual([1, 2, 3]); + }); + + // Same scenario as the original brief test, adapted for the allowlist-first model: + // the allowlist is now checked first, so a host must be explicitly permitted for this + // test to actually exercise the IP layer (defense in depth) rather than being rejected + // one layer earlier for an unrelated reason. + it("rejects a host that resolves to a private address, even when it is allowlisted (defense in depth)", async () => { + await expect( + fetchRemoteFile("https://internal.example.com/x.pdf", { + lookup: async () => ["10.1.2.3"], + allowedHosts: ["internal.example.com"], + }), + ).rejects.toThrow(/not allowed/i); + }); + + // Same DNS-rebinding scenario as the original brief test. ok.example.com and the literal + // metadata IP are explicitly allowlisted here so the test isolates the IP-layer re-check + // (this is what stops the rebinding attack), rather than the redirect being rejected one + // layer earlier by the host allowlist. + // Both hops use https and both hosts are allowlisted, so neither the scheme check nor + // the host allowlist can be what stops this — only the per-hop IP re-check can. The + // assertion targets the IP layer's specific message (not the generic /not allowed/i, + // which also matches the scheme-rejection and host-allowlist-rejection strings and so + // would pass even with the IP check deleted — see the mutation note in the task report). + it("re-checks the address after a redirect, even when the redirect target is https and allowlisted", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + const u = String(url); + seen.push(u); + if (u.endsWith("/start.pdf")) { + return new Response(null, { + status: 302, + headers: { location: "https://internal.example.com/latest/meta-data" }, + }); + } + return new Response("should never be reached", { status: 200 }); + }) as unknown as typeof fetch; + const lookup = async (host: string) => + host === "internal.example.com" ? ["169.254.169.254"] : ["93.184.216.34"]; + await expect( + fetchRemoteFile("https://ok.example.com/start.pdf", { + lookup, + fetchImpl, + allowedHosts: ["ok.example.com", "internal.example.com"], + }), + ).rejects.toThrow(/private, loopback or link-local/i); + expect(seen).toHaveLength(1); + }); + + it("rejects a redirect from an allowlisted host to a host that is not allowlisted, before the second request goes out", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + seen.push(String(url)); + return new Response(null, { status: 302, headers: { location: "https://evil.example.com/payload" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://foo.sharepoint.com/start.pdf", { lookup: publicLookup, fetchImpl }), + ).rejects.toThrow(/not allowed/i); + expect(seen).toHaveLength(1); + }); + + it("stops after the redirect limit", async () => { + const fetchImpl = (async (url: string | URL) => + new Response(null, { status: 302, headers: { location: `https://ok.example.com/${Math.random()}` } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/a", { + lookup: publicLookup, + fetchImpl, + maxRedirects: 3, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/redirect/i); + }); + + it("enforces the default redirect limit of 3 when maxRedirects is not given", async () => { + let calls = 0; + const fetchImpl = (async () => { + calls++; + return new Response(null, { status: 302, headers: { location: "https://ok.example.com/next" } }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/a", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/redirect/i); + // hops 0..3 inclusive = DEFAULT_MAX_REDIRECTS (3) + 1 attempts. + expect(calls).toBe(4); + }); + + it("rejects a body larger than maxBytes", async () => { + const big = new Uint8Array(1024); + const fetchImpl = (async () => + new Response(big, { status: 200, headers: { "content-type": "application/pdf" } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/big.pdf", { + lookup: publicLookup, + fetchImpl, + maxBytes: 100, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + }); + + it("enforces the default maxBytes of 20 MiB via content-length when maxBytes is not given", async () => { + const tooLarge = 20 * 1024 * 1024 + 1; + const fetchImpl = (async () => + new Response(new Uint8Array(0), { + status: 200, + headers: { "content-type": "application/pdf", "content-length": String(tooLarge) }, + })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/huge.pdf", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + }); + + it("aborts a streamed body without a truthful content-length once maxBytes is exceeded, without reading it fully", async () => { + const chunkSize = 1024; + const totalChunks = 100; // 100 * 1024 = 102400 bytes if the whole stream were drained + let pulled = 0; + const stream = new ReadableStream({ + pull(controller) { + if (pulled >= totalChunks) { + controller.close(); + return; + } + pulled++; + controller.enqueue(new Uint8Array(chunkSize)); + }, + }); + const fetchImpl = (async () => + new Response(stream, { status: 200, headers: { "content-type": "application/pdf" } })) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://ok.example.com/big.pdf", { + lookup: publicLookup, + fetchImpl, + maxBytes: 2048, + allowedHosts: ["ok.example.com"], + }), + ).rejects.toThrow(/too large/i); + // Proof it stopped streaming early rather than buffering the whole body first. + expect(pulled).toBeLessThan(totalChunks); + }); + + it("applies the timeout globally across multiple redirects, not per hop", async () => { + let hopCount = 0; + const fetchImpl = ((_url: string | URL, init?: { signal?: AbortSignal }) => { + hopCount++; + return new Promise((resolve, reject) => { + const signal = init?.signal; + if (signal?.aborted) { + reject(Object.assign(new Error("aborted"), { name: "AbortError" })); + return; + } + const t = setTimeout(() => { + resolve( + new Response(null, { + status: 302, + headers: { location: `https://ok.example.com/hop-${hopCount}` }, + }), + ); + }, 8); + signal?.addEventListener("abort", () => { + clearTimeout(t); + reject(Object.assign(new Error("aborted"), { name: "AbortError" })); + }); + }); + }) as unknown as typeof fetch; + + await expect( + fetchRemoteFile("https://ok.example.com/start", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + timeoutMs: 40, + maxRedirects: 100, + }), + ).rejects.toThrow(/abort/i); + // If the timeout were reset on every hop instead of being global, all 101 attempts + // (maxRedirects=100 + 1) would run to completion and the call would fail with "too + // many redirects" instead of aborting on elapsed time. + expect(hopCount).toBeLessThan(100); + }); + + it("returns bytes, content type and filename from content-disposition", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="beleg-2026.pdf"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(Array.from(out.bytes)).toEqual([1, 2, 3]); + expect(out.contentType).toBe("application/pdf"); + expect(out.filename).toBe("beleg-2026.pdf"); + }); + + it("does not fail on a plain filename containing a literal percent sign", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="100% Rabatt.pdf"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("100% Rabatt.pdf"); + }); + + it("prefers the RFC 5987 extended filename* over the plain filename and decodes it", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename=\"fallback.pdf\"; filename*=UTF-8''beleg%20zwei.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("beleg zwei.pdf"); + }); + + it("strips the RFC 5987 language tag from filename* instead of gluing it onto the name", async () => { + // Minor 4: the extended-value grammar is charset'language'value, and the + // language part is optional but routinely set by German servers. Matching + // only the literal `UTF-8''` prefix left `UTF-8'de'` in front of the name and + // filed the receipt as `UTF-8'de'Rechnung.pdf` — wrong data in the books, + // with nothing failing visibly. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8'de'Rechnung.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung.pdf"); + }); + + it("decodes a percent-encoded filename* carrying a language tag", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8'de'Rechnung%20M%C3%BCller.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung Müller.pdf"); + }); + + it("still decodes filename* with an EMPTY language tag (the no-language form)", async () => { + // The other half of "with and without a language tag": splitting on the two + // apostrophes generically must not break the plain `UTF-8''…` case. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8''Rechnung%20M%C3%BCller.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung Müller.pdf"); + }); + + it("keeps a malformed filename* with no apostrophes at all verbatim", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=Rechnung.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("Rechnung.pdf"); + }); + + it("reduces a filename to its basename and strips path separators", async () => { + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": 'attachment; filename="../../etc/passwd"', + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("passwd"); + }); + + it("strips control characters out of a filename* instead of forwarding them", async () => { + // Measured: `filename*=UTF-8''evil%0D%0Ainjected.pdf` decodes to + // "evil\r\ninjected.pdf", and trimming leaves a CRLF sitting in the MIDDLE of the + // name — which then goes into the multipart field and into every log line built + // from it. + const fetchImpl = (async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { + "content-type": "application/pdf", + "content-disposition": "attachment; filename*=UTF-8''evil%0D%0Ainjected.pdf", + }, + })) as unknown as typeof fetch; + const out = await fetchRemoteFile("https://ok.example.com/x", { + lookup: publicLookup, + fetchImpl, + allowedHosts: ["ok.example.com"], + }); + expect(out.filename).toBe("evilinjected.pdf"); + expect(out.filename).not.toMatch(/[\u0000-\u001f\u007f]/); + }); +}); From ae6a2df3b709b0907c093a885fdf007b5bb087f4 Mon Sep 17 00:00:00 2001 From: Marcel Date: Thu, 13 Aug 2026 17:12:00 +0000 Subject: [PATCH 3/4] fix(uploads): close the review findings on the URL fetcher; release 0.1.12 Integrates #39 (gutencoder's pinned-transport upload-file-from-url) with the findings from the review applied on top. His two commits are cherry-picked as authored; this commit is the fixes. - Filename handling now matches the ticket flow: the model-supplied `filename` override and the URL basename run through sanitizeFilename (the same trust boundary), so `../../etc/passwd`, an embedded CRLF or an over-long name can't reach Lexware/logs, and a trailing-slash URL no longer submits an EMPTY filename (the `"" ?? default` bug, reintroduced from the #34 fetcher). The URL basename is percent-decoded first. resolveDownloadName is exported + unit-tested. - URLs with embedded credentials are refused (first URL and every redirect hop): node:https would otherwise send them as Authorization: Basic on the wire, where the fetch it replaced refused such URLs outright. - A leading dot on an allow-list entry (`.sharepoint.com`) is stripped instead of silently matching nothing (subdomain matching is already dot-boundary). - Drain the response body before the "redirect without location" throw, the one post-fetch error path that skipped it. The pinning itself was verified sound under adversarial review (real-cert TLS tests prove validation still binds to the hostname, not the pinned address; no SSRF escape, TOCTOU, decompression-cap, or multipart-injection path found), so it is unchanged. Ships as 0.1.12. 320 tests. --- CHANGELOG.md | 24 +++++++++++- package-lock.json | 4 +- package.json | 2 +- src/config.ts | 5 ++- src/server.ts | 2 +- src/tools/url-upload.ts | 47 ++++++++++++++++++++++- src/uploads/fetch-url.ts | 13 ++++++- tests/config.test.ts | 10 +++++ tests/uploads-fetch-url.test.ts | 32 ++++++++++++++++ tests/uploads-url-upload.test.ts | 65 ++++++++++++++++++++++++++++++++ 10 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 tests/uploads-url-upload.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f216c2..043dec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,12 @@ All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [0.1.12] + +The server-side URL fetcher held back in 0.1.11 — now with its DNS-rebinding TOCTOU closed by +connection-level IP pinning. Based on the contribution by +[@gutencoder](https://github.com/gutencoder) ([#39]); a review pass on top tightened the filename +handling and a few edges (see **Fixed** below). ### Added - **`upload-file-from-url`, with the DNS-rebinding TOCTOU closed** — the tool held back from @@ -41,6 +46,23 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - The allow-list and per-hop address checks from #34 are unchanged and still apply first; pinning is a third layer, not a replacement for either. +### Fixed +- **The stored filename now goes through the same sanitizer the ticket flow uses.** The model-supplied + `filename` override and the URL's own basename previously reached Lexware and the logs unsanitized — + only the `Content-Disposition` name was cleaned. A trailing-slash URL (`…/x/`) with no other name + produced an **empty** filename (`"" ?? "download.bin"` keeps the empty string); a `filename` of + `../../etc/passwd`, an embedded CRLF, or a 300-character string passed straight through. All three + candidates now run through `sanitizeFilename` — empty degrades to `download.bin`, and the URL + basename is percent-decoded first. +- **URLs carrying embedded credentials are refused** (`https://user:pass@host/…`), on the first URL and + every redirect hop. The `node:https` transport would otherwise turn userinfo into an + `Authorization: Basic` header on the wire; the `fetch` it replaced refused such URLs, and that + refusal is restored. +- **A leading dot on an allow-list entry (`.sharepoint.com`) no longer silently blocks everything** — + it is stripped, since subdomain matching is already on a dot boundary. + +[#39]: https://github.com/marselsel/lexware-mcp/pull/39 + ## [0.1.11] Upload a receipt without pushing its bytes through the model context. Based on the diff --git a/package-lock.json b/package-lock.json index a0a6948..a348052 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "lexware-mcp", - "version": "0.1.11", + "version": "0.1.12", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "lexware-mcp", - "version": "0.1.11", + "version": "0.1.12", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.29.0", diff --git a/package.json b/package.json index 3124daa..122c1ef 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "lexware-mcp", - "version": "0.1.11", + "version": "0.1.12", "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 414ffc3..efa2475 100644 --- a/src/config.ts +++ b/src/config.ts @@ -246,7 +246,10 @@ function resolveUploadAllowedHosts(env: NodeJS.ProcessEnv): string[] { if (raw === undefined) return DEFAULT_ALLOWED_HOSTS; return raw .split(",") - .map((h) => h.trim().toLowerCase()) + // Strip a leading dot (the cookie/Java `.sharepoint.com` convention): isAllowedHost + // already matches subdomains on a dot boundary, so a `.`-prefixed entry would match + // NOTHING and silently block the very host the operator meant to allow. + .map((h) => h.trim().toLowerCase().replace(/^\.+/, "")) .filter(Boolean); } diff --git a/src/server.ts b/src/server.ts index 230a47a..0c04820 100644 --- a/src/server.ts +++ b/src/server.ts @@ -42,7 +42,7 @@ const client = new LexwareClient({ const server = new McpServer( { name: "lexware-office", - version: "0.1.11", + version: "0.1.12", }, { capabilities: {} }, ); diff --git a/src/tools/url-upload.ts b/src/tools/url-upload.ts index fd8ffa3..d9a96f3 100644 --- a/src/tools/url-upload.ts +++ b/src/tools/url-upload.ts @@ -2,8 +2,53 @@ import type { McpServer } from "skybridge/server"; import { z } from "zod"; import type { LexwareClient } from "../lexware/client.js"; import { fetchRemoteFile } from "../uploads/fetch-url.js"; +import { sanitizeFilename } from "../uploads/filename.js"; import { text, WRITE } from "./shared.js"; +/** + * Last path segment of a URL, percent-decoded. Total by contract: a malformed URL or a + * lone `%` yields `""` (which `sanitizeFilename` then maps to `undefined`) rather than + * throwing — the caller relies on this to fall through to the fixed default. + */ +function urlBasename(rawUrl: string): string { + let last: string; + try { + last = new URL(rawUrl).pathname.split("/").pop() ?? ""; + } catch { + return ""; + } + try { + return decodeURIComponent(last); + } catch { + return last; + } +} + +/** + * The filename to store the download under, decided the same way as the ticket flow + * (see `resolveFilename` in routes.ts): a sanitized model override, then the sanitized + * `Content-Disposition` name the fetch already resolved, then the URL's own basename + * (percent-decoded, sanitized), then a fixed default. EVERY candidate goes through + * `sanitizeFilename` — the same trust-boundary helper the ticket flow uses — so a name + * like `../../etc/passwd`, an embedded CRLF, or an over-long string never reaches the + * multipart field, the logs, or the tool result. Because `sanitizeFilename` never returns + * `""` (an empty/unusable name comes back `undefined`), each candidate is either a real + * name or falls through — so a trailing-slash URL (`.../x/` → basename `""`) lands on + * `download.bin`, never an empty filename. + * + * The URL basename is the LAST resort, so it is only parsed when the override and the + * response name both came back empty — no `new URL()`/decode on the common path. + * `resolveDownloadName` never throws: `urlBasename` is total (see above). + */ +export function resolveDownloadName(override: string | undefined, fromResponse: string | undefined, url: string): string { + const fromOverride = override !== undefined ? sanitizeFilename(override) : undefined; + if (fromOverride) return fromOverride; + // fromResponse (fetched.filename) was already sanitized in filenameFromDisposition and + // is never `""`, so it wins here without a second sanitize pass. + if (fromResponse) return fromResponse; + return sanitizeFilename(urlBasename(url)) ?? "download.bin"; +} + /** * `upload-file-from-url` — the server-side URL fetcher. * @@ -41,7 +86,7 @@ export function registerUrlUploadTool( }, async ({ url, filename, mimeType, type }: { url: string; filename?: string; mimeType?: string; type: string }) => { const fetched = await fetchRemoteFile(url, { allowedHosts }); - const name = filename ?? fetched.filename ?? new URL(url).pathname.split("/").pop() ?? "download.bin"; + const name = resolveDownloadName(filename, fetched.filename, url); const created = await client.postMultipart<{ id: string }>( "/v1/files", { bytes: fetched.bytes, filename: name, contentType: mimeType ?? fetched.contentType }, diff --git a/src/uploads/fetch-url.ts b/src/uploads/fetch-url.ts index 972bd5d..8ae4834 100644 --- a/src/uploads/fetch-url.ts +++ b/src/uploads/fetch-url.ts @@ -183,6 +183,14 @@ export function assertFetchableUrl(raw: string): URL { if (url.protocol !== "https:") { throw new Error(`URL scheme ${url.protocol} is not allowed — only https.`); } + // Node's `https.request` turns userinfo in the URL into an `Authorization: Basic` + // header and sends it to the (allow-listed) host; the `fetch` this transport replaced + // refused such URLs outright. Restore that refusal rather than silently putting + // caller-supplied credentials on the wire and into the peer's access log. Runs on + // every redirect hop too, since each hop re-enters here. + if (url.username || url.password) { + throw new Error("URLs with embedded credentials are not allowed."); + } return url; } @@ -300,7 +308,10 @@ export async function fetchRemoteFile( if (res.status >= 300 && res.status < 400) { const location = res.headers.get("location"); - if (!location) throw new Error(`Redirect without a location header.`); + if (!location) { + await drain(res); + throw new Error(`Redirect without a location header.`); + } await drain(res); url = assertFetchableUrl(new URL(location, url).toString()); continue; diff --git a/tests/config.test.ts b/tests/config.test.ts index 45606de..89301cc 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -383,6 +383,16 @@ describe("loadConfig", () => { expect(c.uploadAllowedHosts).not.toContain("sharepoint.com"); }); + it("strips a leading dot from an allowlist entry so `.sharepoint.com` is not silently dead", () => { + // isAllowedHost matches subdomains on a dot boundary already, so a `.`-prefixed entry + // (the cookie/Java convention) would match NOTHING and quietly block the intended host. + const c = loadConfig({ + ...base(), + LEXWARE_UPLOAD_ALLOWED_HOSTS: ".files.example.com, ..cdn.example.org", + } as NodeJS.ProcessEnv); + expect(c.uploadAllowedHosts).toEqual(["files.example.com", "cdn.example.org"]); + }); + it("an EMPTY allowlist blocks every host instead of meaning 'allow everything'", () => { const c = loadConfig({ ...base(), diff --git a/tests/uploads-fetch-url.test.ts b/tests/uploads-fetch-url.test.ts index 6443d29..36c330e 100644 --- a/tests/uploads-fetch-url.test.ts +++ b/tests/uploads-fetch-url.test.ts @@ -102,6 +102,38 @@ describe("fetchRemoteFile", () => { expect(seen).toHaveLength(1); }); + it("rejects a URL with embedded credentials, before any lookup or connection", async () => { + // The node:https transport turns userinfo into an Authorization: Basic header and + // sends it to the (allow-listed) host — the fetch it replaced refused such URLs. The + // guard restores that, and rejects at the URL stage so nothing is resolved or dialled. + let lookups = 0; + const lookup = async () => { + lookups++; + return ["93.184.216.34"]; + }; + await expect( + fetchRemoteFile("https://user:secret@foo.sharepoint.com/x.pdf", { lookup }), + ).rejects.toThrow(/credentials/i); + expect(lookups).toBe(0); + }); + + it("rejects a redirect whose Location carries credentials, before the next request goes out", async () => { + const seen: string[] = []; + const fetchImpl = (async (url: string | URL) => { + seen.push(String(url)); + // A same-host relative Location is the one form that would preserve userinfo; force + // the absolute credentialed form to prove the hop is re-checked either way. + return new Response(null, { + status: 302, + headers: { location: "https://user:secret@foo.sharepoint.com/next.pdf" }, + }); + }) as unknown as typeof fetch; + await expect( + fetchRemoteFile("https://foo.sharepoint.com/start.pdf", { lookup: publicLookup, fetchImpl }), + ).rejects.toThrow(/credentials/i); + expect(seen).toHaveLength(1); // stopped at the redirect, never issued the second request + }); + it("rejects a host that is not on the allowlist", async () => { await expect( fetchRemoteFile("https://not-allowed.example.com/x.pdf", { lookup: publicLookup }), diff --git a/tests/uploads-url-upload.test.ts b/tests/uploads-url-upload.test.ts new file mode 100644 index 0000000..caec34e --- /dev/null +++ b/tests/uploads-url-upload.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { resolveDownloadName } from "../src/tools/url-upload.js"; + +const URL_ = "https://acme.sharepoint.com/personal/beleg.pdf"; + +describe("resolveDownloadName", () => { + it("prefers a sanitized model override over everything else", () => { + expect(resolveDownloadName("Rechnung.pdf", "from-response.pdf", URL_)).toBe("Rechnung.pdf"); + }); + + it("falls back to the response filename, then the URL basename, then a fixed default", () => { + expect(resolveDownloadName(undefined, "from-response.pdf", URL_)).toBe("from-response.pdf"); + expect(resolveDownloadName(undefined, undefined, URL_)).toBe("beleg.pdf"); + // No name anywhere resolvable → the fixed default, never an empty string. + expect(resolveDownloadName(undefined, undefined, "https://acme.sharepoint.com/")).toBe("download.bin"); + }); + + // The bug this pins down (reintroduced from the #34 fetcher, fixed again here): a URL + // whose path ends in "/" has basename "" — and "" ?? "download.bin" KEEPS the empty + // string. sanitizeFilename maps "" to undefined, so the ?? chain lands on the default. + it("never submits an empty filename for a trailing-slash URL", () => { + for (const u of [ + "https://acme.sharepoint.com/personal/x/", + "https://acme.sharepoint.com/", + "https://acme.sharepoint.com", + ]) { + const name = resolveDownloadName(undefined, undefined, u); + expect(name, u).not.toBe(""); + expect(name, u).toBe("download.bin"); + } + }); + + it("runs the model override through sanitizeFilename — path traversal, control chars, over-long", () => { + // Not exploitable for multipart injection (undici escapes it), but the override must + // be treated as the trust boundary the ticket flow already treats it as. + expect(resolveDownloadName("../../../../etc/cron.d/evil.sh", undefined, URL_)).toBe("evil.sh"); + expect(resolveDownloadName("evil\r\ninjected.pdf", undefined, URL_)).toBe("evilinjected.pdf"); + const long = resolveDownloadName(`${"a".repeat(400)}.pdf`, undefined, URL_); + expect(long.length).toBeLessThanOrEqual(255); + // An override that sanitizes to nothing falls through to the next candidate. + expect(resolveDownloadName(" ", "kept.pdf", URL_)).toBe("kept.pdf"); + expect(resolveDownloadName(" ", undefined, URL_)).toBe("beleg.pdf"); + }); + + it("percent-decodes and sanitizes the URL basename", () => { + expect(resolveDownloadName(undefined, undefined, "https://acme.sharepoint.com/Rechnung%20M%C3%BCller.pdf")).toBe( + "Rechnung Müller.pdf", + ); + // A path segment that is itself a traversal is reduced to its basename. + expect(resolveDownloadName(undefined, undefined, "https://acme.sharepoint.com/a/b/..%2F..%2Fpasswd")).toBe("passwd"); + // A lone percent is not valid percent-encoding: degrade to the raw segment, don't throw. + expect(resolveDownloadName(undefined, undefined, "https://acme.sharepoint.com/100%.pdf")).toBe("100%.pdf"); + }); + + it("is total: a malformed URL falls through to the default instead of throwing", () => { + // The exported contract is "always returns a string, never throws". urlBasename parses + // the URL only as the last resort, so this can't fire from the tool (url is pre-validated), + // but the helper must honour its contract for any direct caller. + expect(() => resolveDownloadName(undefined, undefined, "not a valid url")).not.toThrow(); + expect(resolveDownloadName(undefined, undefined, "not a valid url")).toBe("download.bin"); + // An earlier candidate still wins without the URL being parsed at all. + expect(resolveDownloadName("real.pdf", undefined, "not a valid url")).toBe("real.pdf"); + expect(resolveDownloadName(undefined, "fromresp.pdf", "://also-bad")).toBe("fromresp.pdf"); + }); +}); From c2b078a9d1c091e201c297596b422a4bfc113fea Mon Sep 17 00:00:00 2001 From: Marcel Date: Thu, 13 Aug 2026 19:15:06 +0000 Subject: [PATCH 4/4] =?UTF-8?q?chore(deps):=20npm=20audit=20fix=20?= =?UTF-8?q?=E2=80=94=20clear=20nanoid=20GHSA-2v37-7h3g-55p8=20(lockfile=20?= =?UTF-8?q?only)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New high-severity advisory (nanoid <3.3.18, disclosed after 0.1.11's green CI) tripped the audit gate. Transitive prod dep; npm audit fix bumps it to 3.3.18 in the lockfile only, package.json untouched. 321 tests green. --- package-lock.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index a348052..55900b5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4101,9 +4101,9 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.17", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.17.tgz", - "integrity": "sha512-xQLf0A3HOMlgHq0n247/LRuAOYmB7dXJ/DvAxGvsSBij45XtBSmQycu+F8ODbHwns/XyFZagyL1+J0Offw1E0g==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "funding": [ { "type": "github",