feat(uploads): ticket-gated and URL uploads without base64 in the model context - #34
feat(uploads): ticket-gated and URL uploads without base64 in the model context#34gutencoder wants to merge 2 commits into
Conversation
…el context upload-file / upload-voucher-file carry the file as base64 inline in the JSON-RPC body: every byte is billed as tokens, lands in the transcript, and a ~8 MB receipt runs into the 12 MB body limit — even though the model has no use for the contents. Adds three drafts-tier tools that route the bytes around the model: create-upload-ticket (short-lived single-use ticket -> browser page or a curl one-liner), get-upload-result, and upload-file-from-url (server-side fetch). Supporting pieces: an in-memory ticket store, GET/POST /upload/:ticket with a self-contained page, and an SSRF-hardened URL fetcher (host allowlist, redirect re-validation, private/loopback/link-local rejection after DNS resolution). Filenames travel as X-Filename-B64 because a raw header value is Latin-1 on the wire and fetch() rejects anything above U+00FF. LEXWARE_UPLOAD_ALLOWED_HOSTS configures which hosts upload-file-from-url may download from. It replaces the built-in defaults rather than extending them so they can be opted out of, and an empty value blocks every host rather than allowing all. The default is the previous hard-coded list, so behaviour is unchanged when it is unset. /upload paths are deferred from the global JSON parser alongside /mcp: the routes read the raw body themselves, and letting the JSON parser run first turned a JSON-content-typed upload into an empty file. The base64 tools are unchanged; this is additive.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a ticket-gated upload flow and SSRF-hardened URL uploads so files can reach Lexware without sending raw bytes through the model context.
Changes:
- Introduces
/upload/:ticketGET/POST routes with an in-memoryTicketStore, plus a self-contained browser upload page. - Adds MCP tools for ticket issuance/result polling and for URL-based uploads with host allowlisting + address-blocking.
- Updates body parsing to defer global JSON parsing for
/upload(and makes path checks case-insensitive), plus extensive tests and documentation updates.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/uploads-tools.test.ts | Adds unit tests for ticket response + curl command construction (headers, quoting, base64url filename). |
| tests/uploads-tickets.test.ts | Adds coverage for TicketStore lifecycle (create/claim/release/complete/peek, expiry). |
| tests/uploads-routes.test.ts | Adds end-to-end tests for /upload/:ticket behavior, body parsing deferral, error mapping, and filename encoding. |
| tests/uploads-fetch-url.test.ts | Adds tests for SSRF protections: allowed-host matching, blocked IP ranges, redirects, limits, timeout, content-disposition parsing. |
| tests/tools.test.ts | Updates tool-registration test to include new upload tools + new registerTools signature. |
| tests/server-body-parsing.test.ts | Adds regression tests for case-insensitive /mcp and /upload path detection. |
| tests/config.test.ts | Adds tests for LEXWARE_UPLOAD_ALLOWED_HOSTS defaulting/replacement/empty semantics. |
| src/uploads/tickets.ts | Implements in-memory ticket store with single-use + in-flight locking + expiry. |
| src/uploads/routes.ts | Implements /upload/:ticket endpoints, raw-body handling, error mapping, X-Filename-B64 decoding, header normalization. |
| src/uploads/page.ts | Adds dependency-free browser upload page and exported filename base64url encoder source for tests. |
| src/uploads/fetch-url.ts | Adds SSRF-hardened remote fetching: https-only, host allowlist, DNS resolution + blocked-address checks, redirect revalidation, size/time limits, filename parsing. |
| src/tools/uploads.ts | Adds upload-related MCP tools and curl command builder; integrates default allowlist. |
| src/tools/index.ts | Extends registerTools to include upload tools with shared ticket store + base URL. |
| src/server.ts | Wires body-parsing deferral for /mcp and /upload, mounts upload routes, creates shared ticket store, passes new args to registerTools. |
| src/server-body-parsing.ts | Extracts and generalizes global-json-parser deferral + adds case-insensitive path match helpers. |
| src/config.ts | Adds uploadAllowedHosts to config and parsing for LEXWARE_UPLOAD_ALLOWED_HOSTS. |
| README.md | Documents new tools and LEXWARE_UPLOAD_ALLOWED_HOSTS; updates tool count. |
| CHANGELOG.md | Documents new ticket-based upload flow, URL upload tool, SSRF hardening, and /upload body parsing deferral. |
| .env.example | Documents LEXWARE_UPLOAD_ALLOWED_HOSTS usage and defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…/filename paths Four review findings on the ticket-gated upload path. The public base URL the upload links are built from was derived from the auth mode — the OAuth resource, otherwise http://127.0.0.1:$PORT. SERVER_URL was read only inside the OAuth branch, so a static-token deployment behind a real domain (a documented, supported mode) set it, had it ignored, and handed the model a browser URL and a curl command pointing at the container's own loopback interface. It is now resolved once in loadConfig from OAUTH_RESOURCE or SERVER_URL, independently of the auth mode, and lives on Config rather than AuthConfig — where the server is reachable is a deployment fact, not an auth one. It goes through the same normalizeUrl validation as every other configured URL, so a typo fails at startup instead of being pasted into a command an operator runs. Unset, the loopback fallback still applies on the configured port; OAuth deployments are unaffected (the value is the same string the token audience is built from). The upload page built its success message with innerHTML, interpolating a file id that comes back from the Lexware API. It is now a text node inside a <code> element: same rendering, no parsing. The error path already did this. sanitizeFilename only stripped path separators and trimmed the ends, so anything in the middle survived: filename*=UTF-8''evil%0D%0Ainjected.pdf arrived as "evil\r\ninjected.pdf" and went on into the multipart field and into anything that logs the name. C0 controls and DEL are now removed wherever they appear, and the result is capped at 255 characters (the per-component limit of ext4/XFS/APFS/NTFS) without splitting a surrogate pair. A name left empty by this returns undefined, which the existing fallback chain already handles. Legitimate non-ASCII names are untouched. Expired tickets were only removed by the sweep in create(), so an instance that issued tickets and then went quiet held them for as long as it stayed up. claim() and peek() now evict what they find expired. Externally unchanged: still 410, still undefined. Tests: 241 -> 255. Each fix was mutation-probed — reverted, the covering test fails, restored, the suite is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Short note to tie these three together, since they landed at once and one of them is One practical thing you may not have noticed: the CI runs on all three are sitting at In the meantime I ran what
On ordering: #32 and #33 both append an On #34 specifically: it is by far the largest and the most opinionated of the three, Finally: on #32 I left two of the Copilot threads open rather than resolving them, |
|
Thanks for this one too — and particularly for naming the trade-offs yourself rather than leaving them to be found. The section on why I want to set expectations honestly: this one is going to take me longer than #32 and #33. Those were ~95 and ~140 lines against code paths I already knew. This is ~3,300 lines, six new modules, and a new unauthenticated HTTP surface — and it's on a server that fronts real accounting data. I'd rather review it properly than merge it quickly. I'm merging #32 and #33 first. Before I get into this one in depth, two questions, because the answers change how much of the rest matters: 1. Can the upload endpoints be opt-in? You offered this ("the endpoints disabled unless explicitly enabled") and I think I'd take it. My reasoning: the capability-URL model you describe is sound, and the ticket really is a credential — but it changes this server from "one gate, everything behind it" to "two gates with different properties". For an operator who never uses the feature, an env flag means the surface simply isn't there. Something like 2. How much do you need On the in-process store: for my deployment it's a non-issue ( Give me a bit of time on the full read-through. The volume of tests here is reassuring, but I want to actually trace the SSRF guards and the ticket lifecycle myself rather than trust the test names. |
Integrate the base64-free upload path from #34, scoped to the ticket flow (create-upload-ticket / get-upload-result + the /upload/:ticket route) and hold back upload-file-from-url. - Remove upload-file-from-url, the SSRF URL fetcher (fetch-url.ts), the host allow-list config (LEXWARE_UPLOAD_ALLOWED_HOSTS) and their tests. Keep the shared sanitizeFilename helper as src/uploads/filename.ts. - Mount the /upload routes only when the drafts capability is enabled, so a read-only deployment never exposes the unauthenticated write route. - Serve the ticket page Cache-Control: no-store + X-Content-Type-Options: nosniff. - Bump to 0.1.11; CHANGELOG records the deferral and its reason. The URL fetcher carries a DNS-rebinding TOCTOU that is moot for the built-in Microsoft defaults but live for any custom allow-list; it will be reconsidered separately with connection-level IP pinning, disabled by default.
|
Merged — thank you! The ticket flow from this PR is now on
Review of the ticket flow surfaced a few things now fixed on top of your commits (details in #38): curl's The measured, reproduce-first writeups in your commits and PR description made this genuinely enjoyable to review — that's now three solid contributions (#32, #33, and this). Closing as superseded by #38. |
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.
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.
….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.
Problem
upload-fileandupload-voucher-filetake the file as base64 inline in the JSON-RPCbody. That makes the file's bytes part of the conversation:
the conversation history;
JSON_BODY_LIMITof 12 MB (src/server.ts), which a ~8 MB sourcefile already brushes against once base64-expanded.
All of that to move bytes the model never needs to look at. The model needs the resulting
file id; it does not need the PDF.
Solution
Three new tools in the drafts tier, plus the endpoints they depend on. The base64
tools are untouched and stay available — this is purely additive.
create-upload-ticket— issues a short-lived single-use ticket and returns both abrowser URL (drag-and-drop page) and a ready-to-run
curlcommand. Bytes goclient → server → Lexware; the model only ever sees the file id.
get-upload-result— reads back the file id once the transfer happened.upload-file-from-url— fetches the file server-side from a URL.Supporting pieces: an in-memory ticket store (
src/uploads/tickets.ts), theGET/POST /upload/:ticketendpoints and their dependency-free page(
src/uploads/routes.ts,page.ts), and an SSRF-hardened URL fetcher(
src/uploads/fetch-url.ts).Two details worth calling out because they were both found the hard way:
/uploadpaths must be deferred from the pre-applied globalexpress.json()alongside/mcp. The upload routes read the raw body themselves; ifthe JSON parser runs first, a JSON-content-typed upload arrives as a parsed object
instead of a Buffer and silently became an empty file. The refactor of the existing
swap into
src/server-body-parsing.tsis what makes that expressible; there is aBuffer.isBufferguard in the route as a second line of defence.filenametravels asX-Filename-B64(base64url of the UTF-8 bytes),never as a raw header value. A header value is Latin-1 on the wire, and
fetch()throws a
TypeErrorbefore sending for any character above U+00FF — so an ordinaryGerman filename with an en dash or typographic quotes broke the entire upload, and a
raw-byte header turned
Rechnung Müller.pdfintoRechnung Müller.pdfin the books.Why the upload endpoints sit outside the OAuth gate — deliberately
/upload/:ticketis mounted onserver.express, i.e. not behind the/mcpbearergate. That is the central design decision in this PR and it is intentional, so please
review it on its merits rather than as an oversight:
The uploader is not the MCP client. The whole point is that the bytes come from
somewhere the MCP client isn't: a browser tab the user opens, or a
curlon themachine where the file actually lives. Neither holds an MCP access token, and neither
can obtain one — the OAuth flow issues tokens to the registered MCP client, not to an
arbitrary browser or shell. Putting the endpoint behind the same gate would make the
feature unimplementable.
The ticket is the credential. It is 24 bytes from
randomBytesrenderedbase64url (192 bits), so it is not guessable, and it can only be obtained by calling
create-upload-ticket— a drafts-tier tool reachable only through the authenticated/mcpendpoint. Possession of an unguessable, authenticated-issued, short-livedtoken is the same capability-URL model as a pre-signed upload URL.
The capability is narrow and bounded. A ticket permits exactly one action: POST
one file, of at most 20 MB, into the Lexware file store under the
typethat wasfixed when the ticket was issued. It grants no read access, no enumeration, and no
second use. It expires after 15 minutes.
Reuse is blocked, not merely discouraged.
claim()sets aninFlightflagsynchronously and holds it across the async body read and Lexware call, so a retried
request, a duplicated proxy call or a leaked ticket cannot file a second voucher. A
ticket that is unknown, expired, in flight or already completed gets a flat
410.Failures release the lock through a single
catchso a ticket is never stranded.The honest trade-off: a ticket travels in a URL, and URLs leak — browser history,
shoulder-surfing, a copy-pasted link. The mitigations are the short TTL, single use, and
the narrowness of what the capability permits (write-only, one file, pre-fixed type).
If you would rather this were gated differently — a separate shared secret on the
endpoint, a configurable TTL, or the endpoints disabled unless explicitly enabled — I am
happy to change it; it is a small change and I would rather match your preference than
argue for mine.
The URL allowlist is configurable
upload-file-from-urlonly fetches from hosts on an allow-list, matched on a dotboundary so
evilsharepoint.comcannot pass assharepoint.com. The list is set withLEXWARE_UPLOAD_ALLOWED_HOSTS(comma-separated).Three properties worth reviewing explicitly:
(
sharepoint.com,onedrive.live.com,1drv.ms,graph.microsoft.com), so leavingthe variable unset behaves exactly as if it did not exist.
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.
emptied cannot be used to switch the feature off, and "empty means allow everything"
would turn a typo into an open SSRF surface.
fetchRemoteFileuses??rather than||for exactly this reason, and there is a test pinning it.Naming:
LEXWARE_*rather thanOAUTH_*/MCP_*. In this projectOAUTH_*is strictlythe IdP/token layer and
MCP_*is how/mcpitself is protected; server behaviour andcapability settings are
LEXWARE_*(LEXWARE_READ_ONLY,LEXWARE_ENABLE_DRAFTS, …).The
ALLOWED_<thing>noun follows the existingOAUTH_ALLOWED_EMAIL_DOMAINS.The SSRF hardening is independent of the allow-list and applies regardless: DNS
resolution is re-checked against loopback, link-local, private, CGNAT, benchmarking and
multicast ranges, and redirects are re-validated rather than followed blindly.
Limitation: the ticket store is in-process
Stating this plainly rather than leaving it to be discovered, because the README
documents Cloud Run and autoscaling is the normal case there:
Mapin the server process. A restart drops open tickets. They thenanswer
410 — ticket unknown or expired, which is a clear error rather than a hang, andthe client can simply ask for a new ticket.
instance than the one that issued the ticket, and gets the same
410. In thattopology the feature is unreliable as built.
Why it is still built this way: the alternative is an external store (Redis, a database,
a bucket), which would be the first infrastructure dependency this server has — today it
is a single container with an API key. That is a real cost to impose on every operator to
serve the multi-instance case. The 15-minute TTL bounds the exposure, and the failure mode
is a clean 410 rather than data loss or a wrong voucher.
If you would rather have it pluggable, the store is already behind a small class
(
TicketStore:create/peek/claim/release/complete), so a secondimplementation would not disturb the routes. Happy to add the seam — I did not want to
design an extension point into your codebase uninvited.
What changes for existing users
Nothing changes for anyone not using the new tools.
upload-fileandupload-voucher-fileare unmodified. The three new tools are in the drafts tier, soLEXWARE_READ_ONLY=trueorLEXWARE_ENABLE_DRAFTS=falseexcludes them exactly like theother write tools. The
/upload/:ticketroutes are registered unconditionally, butwithout a ticket every request to them is a
410.LEXWARE_UPLOAD_ALLOWED_HOSTSis new but defaults to the same host list the fetcherwould use without it, so an existing deployment sees no difference.
One behavioural note: the body-parsing deferral now also covers
/upload. For/mcpandall other paths the behaviour is unchanged.
Tests
npm testpasses (241 tests / 15 files, up from 135 / 10). The new suites cover theticket store lifecycle (issue, claim, single-use, expiry, in-flight lock), the routes
(body-parser ordering, rejection before the body is read, empty body releasing the
ticket, clean JSON errors with no stack traces, case-insensitive routing, filename
sanitization, the full non-ASCII filename matrix), the SSRF guards (each blocked range,
redirect re-validation,
Content-Dispositionparsing including the RFC 5987charset'language'valueform), and the emittedcurlcommand (single replacement point,no command substitution, quote-injection resistance — one test asks a real shell to
confirm the quoting property).
For the allow-list specifically: the default applies with the variable unset; a
configured list replaces the defaults (a Microsoft host is then rejected); an empty value
blocks everything at both the config and the
fetchRemoteFilelayer; and the dot-boundaryproperty (
evilsharepoint.commust not matchsharepoint.com, norsharepoint.com.evil.com) is covered, along with case-insensitivity and a trailingroot-label dot.
npm run buildanddocker buildpass.npm audit --omit=dev --audit-level=highisunchanged from
main(no dependency changes; the branch adds no runtime dependency).Docs
CHANGELOG.mdunder[Unreleased](including the in-process ticket-store limitation);README.mdtool count 60 → 63, the Drafts tier row describes the ticket route, and theconfig table plus
.env.exampledocumentLEXWARE_UPLOAD_ALLOWED_HOSTS.