feat(uploads): upload-file-from-url on a pinned transport (supersedes #39) - #41
Merged
Conversation
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.
…le only) 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.
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 @gutencoder's #39 —
upload-file-from-url, the server-side URL fetcher held back from #34, now with its DNS-rebinding TOCTOU closed by connection-level IP pinning. His two commits are cherry-picked as authored; this PR supersedes #39.A receipt already sitting in OneDrive/SharePoint reaches the books without its bytes crossing the model context. Off by default (
LEXWARE_ENABLE_URL_UPLOAD), its own capability, requires the drafts tier (does not pull it up), and lives in its own file — the only tool that makes this server originate an outbound request to a destination the model chose, so it gets its own switch rather than riding in on drafts.How the TOCTOU is closed
Every check-then-fetch SSRF filter resolves the host, validates the addresses, then hands the name to
fetch, which resolves it again at connect time — two lookups, one checked.src/uploads/pinned-fetch.tsremoves the second lookup:net.connect'slookuphook is fed the exact addresses the guard just vetted, so there is no second resolution to disagree with the first. Built onnode:https(not an undici dispatcher) so no runtime dependency is added. TLS is untouched — SNI and cert validation still bind to the hostname; only the dialled address comes from the pin.Three layers run at every redirect hop: host allow-list (dot-boundary) → resolved-address range check (loopback/private/link-local/metadata/CGNAT/… in every IPv4/IPv6/IPv4-in-IPv6 spelling, fail-closed) → pinning.
Review fixes applied on top of #39
filenameoverride and the URL basename now run throughsanitizeFilename, so../../etc/passwd, an embedded CRLF, or an over-long name can't reach Lexware or the logs — and a trailing-slash URL no longer submits an empty filename. URL basename is percent-decoded;resolveDownloadNameis total (never throws) and unit-tested.node:httpswould otherwise put them on the wire asAuthorization: Basic, where thefetchit replaced refused them)..sharepoint.com) are stripped instead of silently matching nothing; response body drained before the one redirect-error path that skipped it.Verification
.invalid-host connection proof./code-review high(3 LOW — 2 fixed, 1 declined as pre-existing/marginal), and/security-review high(no findings).Ships as v0.1.12. Thanks @gutencoder — the node:https-over-dispatcher call (zero new deps) and the on-the-wire SNI test were the right instincts.