feat(uploads): ticket-gated uploads without base64 through the model (supersedes #34) - #38
Merged
Merged
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.
…/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>
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.
Correctness: - buildCurlCommand always pins Content-Type: a valid token-shaped mimeType is carried; otherwise `-H 'Content-Type:'` strips curl's --data-binary default (application/x-www-form-urlencoded), which silently won over the server's documented ticket-mimeType/octet-stream fallback for every headerless upload. Locked by an end-to-end test that executes the emitted command with real curl. - TicketStore.complete() re-arms the TTL, so get-upload-result can read the file id for a full 15 minutes after the upload FINISHED (a minute-14 upload previously left a sub-minute window and invited a duplicate re-upload), and in-flight entries are never expiry-evicted by racing claim()/peek()/sweep(). Hardening: - One buffering body per ticket at a time (429 for a concurrent second POST, slot released on response close): claim() only runs after express.raw() has buffered the body, so N parallel POSTs on one leaked ticket could each buffer up to 20 MB before N-1 lost the race. - A Lexware 401/403 (the OPERATOR's API key rejected) maps to a generic 502 instead of forwarding upstream status and wording to the unauthenticated ticket holder; a status-0 transport failure now says the outcome is unknown and warns against blind retries (duplicate risk). Polish: - get-upload-result annotated LOCAL_RO (it only peeks the store and is meant to be polled; WRITE made clients confirm every poll). - Loopback link fallback follows __PORT (skybridge dev binds it, not PORT). - Startup warning when OAUTH_RESOURCE is set outside OAuth mode (it silently steers upload links; the stale-after-migration footgun). - Degraded-mode warning no longer overclaims: only JSON-content-typed ticket uploads are size-capped by the global parser, other types work. - Upload body forwarded without a redundant full-size Uint8Array copy. - CHANGELOG stops claiming the single-use lock prevents ALL duplicates: after an unknown-outcome transport failure a blind retry can still duplicate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Integrates the base64-free upload path contributed by @gutencoder in #34, scoped to the ticket flow:
create-upload-ticket/get-upload-result(drafts tier) plus the single-useGET/POST /upload/:ticketroute and its dependency-free drag-and-drop page. Bytes go client → server → Lexware; the model only ever sees the resulting file id.main, not squashed), so the history credits him directly. This PR supersedes feat(uploads): ticket-gated and URL uploads without base64 in the model context #34.What was deliberately left out
upload-file-from-url(the server-side URL fetcher) is deferred, not rejected. A server-side fetcher is SSRF surface by construction, and the #34 implementation — though carefully guarded (host allow-list first, per-hop re-validation, fail-closed address parsing) — carries a DNS-rebinding TOCTOU between the address check and the connection's own re-resolution. That is moot for the built-in Microsoft allow-list, but becomes live the moment an operator configures a customLEXWARE_UPLOAD_ALLOWED_HOSTS. It will be reconsidered separately with connection-level IP pinning, disabled by default. The CHANGELOG Security note records this.Hardening added on top
/uploadroutes mount only when the drafts capability is enabled — a read-only deployment exposes no write route.Cache-Control: no-store,X-Content-Type-Options: nosniff,X-Frame-Options: DENY.get-upload-resulta sub-minute window and invited a duplicate re-upload); in-flight entries are never expiry-evicted.curlcommand always pinsContent-Type—--data-binaryotherwise silently declaresapplication/x-www-form-urlencodedand defeats the documented fallback chain.get-upload-resultannotated read-only (it is designed to be polled);__PORT-aware loopback links underskybridge dev; startup warning for a staleOAUTH_RESOURCEoutside OAuth mode.Verification
curlcommand against the live routes and a client-abort slot-release test.Ships as v0.1.11. Thanks @gutencoder — the body-parsing deferral, the
X-Filename-B64design, and the measured, reproduce-first commit messages made this one a pleasure to review.