From 5b6124875db86c6969cfab7f6f3044a9c836805e Mon Sep 17 00:00:00 2001 From: Marco Moauro Date: Sun, 9 Aug 2026 18:53:57 +0200 Subject: [PATCH 1/2] Let upload_image take a local file, not only an http(s) URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `upload_image` accepted a URL and nothing else, so an image generated or edited on the machine running this server could not be uploaded at all: it had to be published somewhere public first, or the whole flow abandoned for the browser. That limit was self-imposed. `POST /api/v1/image` only ever wanted a data URI, so where the bytes came from was never its business. `path` is now the alternative to `url`, exactly one per call, backed by `readImageFileAsDataUri` in `src/api/substack/image.js` — the sibling of `fetchImageAsDataUri`, sharing its last step so the two sources cannot drift. Verified live against implementing.substack.com end to end: a PNG generated locally, uploaded by path, written to a new draft's cover_image and read back with get_draft, with cover_image_rehosted_from null proving the Substack host was recognised and the url stored unchanged. The local branch does not inherit three things from the URL one: - The content type comes from the magic bytes, never the extension. A local file carries no Content-Type, and the extension is the caller's claim rather than evidence: a PDF renamed .png would otherwise reach Substack and come back as a 400 naming neither the file nor the reason. PNG, JPEG, GIF and WebP are recognised; an ISO-BMFF ftyp box with a HEIC/HEIF brand gets the same convert-first message the URL path gives. Five bounded signatures, written out rather than taken as a dependency — the argument csv.js already makes. - The path must be absolute. A relative one resolves against this server's cwd, not the calling client's, so it would read the wrong file or none, and neither failure would say why. realpath runs first, so a symlink is followed to the file actually read and `..` cannot mean one thing at the check and another at the read. - The size cap is enforced on stat.size before the file is read, the local mirror of the Content-Length pre-check in readCapped. There is deliberately no filesystem allowlist. An env-var root was designed and dropped: a server that needs configuring before `path` works at all is a server back where it started. What it means is stated in CLAUDE.md and the README instead — the caller is an LLM, so this is a read of an arbitrary local file whose bytes then leave for Substack. The exclusivity rule is enforced twice on purpose. The `.superRefine` is the runtime half; a refinement does not survive z.toJSONSchema, so the rule is also written into both property descriptions, and a test pins that. Adding the source meant editing three descriptions, and the third was nearly missed: the tool's own `description` in the `tools` registry is what a model reads to choose which tool to call, while the property descriptions are only reached once it has already chosen. A pitch still saying "from an http(s) URL" left `path` present in the schema and invisible in the offer — exactly how a model concludes a local file cannot be uploaded and reaches for the browser. server.spec.js now asserts the pitch names both sources. 748 tests pass on Node 22 (the engines floor) and 24 (.nvmrc). Each new assertion was mutation-checked: breaking the sniff, the absolute-path guard, the size pre-check, the descriptions and the XOR rule each turns red exactly the tests that claim to cover them. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 38 ++++++++++- README.md | 29 ++++++--- src/api/substack/image.js | 94 +++++++++++++++++++++++++--- src/api/substack/image.spec.js | 111 ++++++++++++++++++++++++++++++++- src/server.js | 9 ++- src/server.spec.js | 29 ++++++++- src/tools/upload_image.js | 76 +++++++++++++++++----- src/tools/upload_image.spec.js | 97 ++++++++++++++++++++++++++++ 8 files changed, 439 insertions(+), 44 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 949cbe3..57c978b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -377,7 +377,7 @@ the wrong *thing*, not for a header detail or a Cloudflare wall — the body is `{id, url, contentType, bytes, imageWidth, imageHeight}`, `url` on `substack-post-media.s3.amazonaws.com` — the host every `image2.src` uses — and it renders through Substack's CDN (proven end to end on a real draft: upload → `captionedImage` → PUT → the editor shows a `substackcdn.com/image/fetch/…` render). -Two measured facts shape the tool: +Three measured facts shape the tool: - **Substack server-fetches only its own S3 bucket.** An external URL passed as `image` answers `400 "Failed to fetch image"`, so `upload_image` downloads the URL itself and re-encodes it. That download is where this server fetches a caller-chosen host, so it is guarded: `http(s)` only, @@ -392,6 +392,42 @@ Two measured facts shape the tool: every request body at info, so a real upload would put hundreds of KB of base64 on one line; `src/logger.js` truncates a `data:…;base64,` value to its prefix and omitted length. A post body, being prose, is still logged in full — the two are different in kind. +- **A local file is the other source, and nothing about the endpoint had to change to accept one.** + `upload_image` takes `path` as the alternative to `url` (`readImageFileAsDataUri`, the sibling of + `fetchImageAsDataUri` in the same module); the endpoint only ever wanted a data URI, so where the bytes + came from was never its business. Verified live 2026-08-09 with a real JPEG and PNG off disk — both + answered 200 with the S3 url and the true `1500x1000`. Three things this branch does *not* inherit + from the URL one, each deliberate: + - **The type comes from the magic bytes, never the extension.** A local file carries no + `Content-Type`, and the extension is the caller's claim rather than evidence: a PDF renamed `.png` + would otherwise reach Substack and come back as a 400 naming neither the file nor the reason. PNG, + JPEG, GIF and WebP are recognised; an ISO-BMFF `ftyp` box with a HEIC/HEIF brand gets the same + convert-first message the URL path gives. Written out rather than taken as a dependency — five + bounded signatures, the same argument `csv.js` makes. **SVG is refused** by that check + (`3c 73 76 67`), and whether `POST /api/v1/image` would accept `image/svg+xml` was never measured: + the dashboard builds its uploads from `canvas.toDataURL()`, which cannot produce one. + - **The path must be absolute.** A relative one resolves against *this server's* cwd, not the calling + client's, so it would read the wrong file or none — and neither failure would say why. `realpath` + runs before the checks, so a symlink is followed to the file actually read and `..` cannot mean one + thing at the check and another at the read. + - **The size cap is enforced on `stat.size` before the file is read**, the local mirror of the + `Content-Length` pre-check in `readCapped`. + + Adding the second source meant editing **three** descriptions, and the third was nearly missed: the + two property descriptions in the schema, *and* the tool's own `description` in the `tools` registry. + That last one is what a model reads to choose **which** tool to call — the property descriptions are + only reached after it has already chosen — so a pitch that still said "from an http(s) URL" left + `path` present in the schema and invisible in the offer. That is exactly how a model concludes a + local file cannot be uploaded and reaches for the browser instead. `server.spec.js` now asserts the + pitch names both sources. + + There is **no filesystem allowlist** — any absolute path the caller names is read. That was a + deliberate call, not an oversight: the env-var root was designed and dropped because a server that + needs configuring before `path` works at all is a server back where it started. Note what it means, + though — the caller is an LLM, so this is a read of an arbitrary local file whose bytes then leave for + Substack. `update_draft`'s `cover_image` deliberately does **not** take a path: the flow is + `upload_image({path})` → the S3 url → `update_draft({cover_image: })`, which recognises the + Substack host and writes it unchanged. Two doors onto one thing would be one too many. **The header token is verified now, and this is what closed it.** Every earlier live check ran on the browser session cookie, leaving `SUBSTACK_SESSION_TOKEN` in a header through `SubstackApi` as the diff --git a/README.md b/README.md index 756c185..bc7e26c 100644 --- a/README.md +++ b/README.md @@ -149,24 +149,33 @@ Three things worth knowing:
-upload_image - Re-host an external image on Substack +upload_image - Host an image on Substack, from a URL or a local file Substack's editor uploads images as base64 data URIs to `POST /api/v1/image`, which answers with a -Substack-hosted URL. `image2.src` in `set_post_body` only renders such a URL, so this tool is the -bridge: give it an http(s) image URL, it downloads the image, re-encodes it, uploads it, and returns -the hosted URL. Substack itself only re-fetches URLs already in its own storage, so the download -happens here rather than being handed off. - -**Inputs**: -- `url` (string): the http(s) URL of an image to upload +Substack-hosted URL. `image2.src` in `set_post_body` and `cover_image` in `update_draft` only render +such a URL, so this tool is the bridge. Substack itself only re-fetches URLs already in its own +storage, so the image is encoded here rather than being handed off. + +**Inputs** — exactly one of `url` or `path`: +- `url` (string): the http(s) URL of an image to download and re-host +- `path` (string): absolute path to an image file on the machine running this server, read straight + from disk with no download - `post_id` (number, optional): the post the image belongs to; its effect is unconfirmed **Returns**: `{id, url, content_type, bytes, width, height}` — put `url` into an `image2.src` when -calling `set_post_body`. +calling `set_post_body`, or into `cover_image` when calling `update_draft`. -The download is guarded: only `http`/`https`, private and loopback hosts are refused after DNS +A **download** is guarded: only `http`/`https`, private and loopback hosts are refused after DNS resolution (redirects are re-checked at every hop), the content type must be an image, HEIC is rejected with a note to convert it, and the image may not exceed 10 MB. + +A **local file** is guarded differently, because it has no `Content-Type` header to trust. The path +must be absolute — a relative one would resolve against this server's working directory, not the +calling client's — and the type is read from the file's magic bytes rather than its extension, so a +non-image with an image extension is caught here instead of at Substack. PNG, JPEG, GIF and WebP are +accepted; HEIC and SVG are not. The same 10 MB cap applies, checked against the file size before the +file is read. Note that `path` reads whatever absolute path it is given: if that matters in your +setup, do not expose this server to a client you would not trust with your filesystem.
diff --git a/src/api/substack/image.js b/src/api/substack/image.js index f0f3a34..38835c7 100644 --- a/src/api/substack/image.js +++ b/src/api/substack/image.js @@ -1,4 +1,6 @@ import dns from "node:dns"; +import fsp from "node:fs/promises"; +import nodePath from "node:path"; // Checked against a declared Content-Length before the body is read, then against the buffered // length. NOT Substack's own limit (its MAX_FILE_SIZE could not be read from the minified bundle). @@ -13,6 +15,9 @@ const FETCH_TIMEOUT_MS = 20000; // Substack instead of getting the friendlier convert-first message. const HEIC_TYPES = new Set(['image/heic', 'image/heif', 'image/heic-sequence', 'image/heif-sequence']); +// One string, so the URL path and the local-file path give the caller the identical instruction. +const HEIC_MESSAGE = 'image: HEIC is not accepted by Substack. Convert to JPG or PNG first.'; + // Resolve every address a host maps to. Injected in tests so DNS is never touched. export const defaultLookup = (hostname) => dns.promises.lookup(hostname, {all: true}); @@ -136,6 +141,85 @@ export function isSubstackHosted(rawUrl) { return SUBSTACK_IMAGE_HOSTS.has(hostname); } +// The one encoding `POST /api/v1/image` accepts, shared by both sources so they cannot drift. +const toDataUri = (buffer, contentType) => `data:${contentType};base64,${buffer.toString('base64')}`; + +// ISO-BMFF brands that mean HEIC/HEIF. `mif1`/`msf1` are the generic image and image-sequence +// brands Apple also writes; all of them are refused with the same convert-first message. +const HEIC_BRANDS = new Set(['heic', 'heix', 'hevc', 'hevx', 'heim', 'heis', 'hevm', 'hevs', 'mif1', 'msf1']); + +/** + * The content type of a local file, read off its magic bytes rather than its extension. + * + * A local file arrives with no Content-Type header, and the extension is the caller's claim, not + * evidence: a PDF renamed to `.png` would reach `POST /api/v1/image` and come back as a 400 that + * names neither the file nor the reason. Five bounded signatures, written out rather than taken as + * a dependency — the same argument `csv.js` makes about not needing a parser library. + * + * Returns null for anything unrecognised, so the caller owns the message. + */ +function sniffImageType(buffer) { + const at = (start, end) => buffer.subarray(start, end).toString('latin1'); + + if (buffer.length >= 8 && at(0, 8) === '\x89PNG\r\n\x1a\n') return 'image/png'; + if (buffer.length >= 3 && buffer[0] === 0xff && buffer[1] === 0xd8 && buffer[2] === 0xff) return 'image/jpeg'; + if (buffer.length >= 6 && (at(0, 6) === 'GIF87a' || at(0, 6) === 'GIF89a')) return 'image/gif'; + // The four bytes at offset 4 are the RIFF chunk size, part of the container rather than the tag. + if (buffer.length >= 12 && at(0, 4) === 'RIFF' && at(8, 12) === 'WEBP') return 'image/webp'; + if (buffer.length >= 12 && at(4, 8) === 'ftyp' && HEIC_BRANDS.has(at(8, 12))) return 'image/heic'; + return null; +} + +/** + * Read a local file and encode it the way `POST /api/v1/image` wants it: a data URI. + * + * The sibling of `fetchImageAsDataUri` for a path instead of a URL, and it lives here for the same + * reason that one does — `upload_image` is the only caller today, but a second copy of these checks + * would be a second place to get them wrong. + * + * The path must be **absolute**: a relative one resolves against this server process's cwd, which + * is not the calling client's, so it would read the wrong file or none, and neither failure would + * say why. It is resolved with `realpath` first, so a symlink is followed to the file actually read + * and `..` cannot mean something different at the check than at the read. + * + * Returns `{image, contentType, bytes}`. `image` is deliberately not logged by this module. + */ +export async function readImageFileAsDataUri(filePath, {maxBytes = MAX_IMAGE_BYTES} = {}) { + if (!nodePath.isAbsolute(filePath)) { + throw new Error(`image: path must be absolute, got ${JSON.stringify(filePath)}`); + } + + let resolved; + try { + resolved = await fsp.realpath(filePath); + } catch (error) { + // Raw, this surfaces as an ENOENT stack from deep in fs — useless to a model trying to repair + // its own call. The path it passed is the whole diagnosis. + if (error.code === 'ENOENT') throw new Error(`image: no such file: ${filePath}`); + throw error; + } + + const stats = await fsp.stat(resolved); + if (!stats.isFile()) throw new Error(`image: not a regular file: ${filePath}`); + // Refused on the size the filesystem reports, before the bytes are read — the local mirror of the + // Content-Length pre-check in `readCapped`. + if (stats.size > maxBytes) { + throw new Error(`image: file is ${stats.size} bytes, over the ${maxBytes}-byte limit.`); + } + + const buffer = await fsp.readFile(resolved); + const contentType = sniffImageType(buffer); + if (!contentType) { + const head = [...buffer.subarray(0, 4)].map((b) => b.toString(16).padStart(2, '0')).join(' '); + throw new Error( + `image: unrecognised image format (first bytes: ${head}). Supported: PNG, JPEG, GIF, WebP.` + ); + } + if (HEIC_TYPES.has(contentType)) throw new Error(HEIC_MESSAGE); + + return {image: toDataUri(buffer, contentType), contentType, bytes: buffer.byteLength}; +} + /** * Download a caller-chosen URL and encode it the way `POST /api/v1/image` wants it: a data URI. * Every guard lives here so both callers get the same one — there is no unguarded path. @@ -154,15 +238,9 @@ export async function fetchImageAsDataUri(url, {lookup = defaultLookup, fetchImp if (!contentType.startsWith('image/')) { throw new Error(`image: source is not an image (content-type: ${contentType || 'none'})`); } - if (HEIC_TYPES.has(contentType)) { - throw new Error('image: HEIC is not accepted by Substack. Convert to JPG or PNG first.'); - } + if (HEIC_TYPES.has(contentType)) throw new Error(HEIC_MESSAGE); const buffer = await readCapped(response, maxBytes); - return { - image: `data:${contentType};base64,${buffer.toString('base64')}`, - contentType, - bytes: buffer.byteLength, - }; + return {image: toDataUri(buffer, contentType), contentType, bytes: buffer.byteLength}; } diff --git a/src/api/substack/image.spec.js b/src/api/substack/image.spec.js index 026c161..85089fe 100644 --- a/src/api/substack/image.spec.js +++ b/src/api/substack/image.spec.js @@ -1,6 +1,9 @@ -import {test, describe} from 'node:test'; +import {test, describe, before, after} from 'node:test'; import assert from 'node:assert/strict'; -import {isSubstackHosted} from './image.js'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import {isSubstackHosted, readImageFileAsDataUri, MAX_IMAGE_BYTES} from './image.js'; // The two hosts measured live on 2026-08-08: `POST /api/v1/image` answers a url on the S3 bucket, // and `list_posts`/`get_draft` hand back covers already rewritten onto the CDN. Both are already @@ -37,3 +40,107 @@ describe('isSubstackHosted', () => { assert.equal(isSubstackHosted('not-a-url-at-all'), false); }); }); + +// A local file has no Content-Type header, so the type has to come from the bytes. These are the +// real signatures, not approximations: a `.png` that is actually a PDF must be caught here rather +// than at Substack, which answers a 400 that names neither the file nor the reason. +const PNG = Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), Buffer.alloc(16)]); +const JPEG = Buffer.concat([Buffer.from([0xff, 0xd8, 0xff, 0xe0]), Buffer.alloc(16)]); +const GIF = Buffer.concat([Buffer.from('GIF89a'), Buffer.alloc(16)]); +// RIFF....WEBP — the four size bytes at offset 4 are part of the container, not of the signature. +const WEBP = Buffer.concat([Buffer.from('RIFF'), Buffer.alloc(4), Buffer.from('WEBP'), Buffer.alloc(16)]); +// An ISO-BMFF box: size, `ftyp`, then the brand. `heic` is what an iPhone writes. +const HEIC = Buffer.concat([Buffer.alloc(4), Buffer.from('ftypheic'), Buffer.alloc(16)]); +const PDF = Buffer.concat([Buffer.from('%PDF-1.4'), Buffer.alloc(16)]); + +describe('readImageFileAsDataUri', () => { + let dir; + + before(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'substack-mcp-image-')); + }); + after(async () => { + await fs.rm(dir, {recursive: true, force: true}); + }); + + // Written with a deliberately wrong extension throughout: the type must come from the bytes. + const write = async (name, bytes) => { + const file = path.join(dir, name); + await fs.writeFile(file, bytes); + return file; + }; + + test('reads a PNG and returns it as a data URI', async () => { + const file = await write('cover.png', PNG); + const result = await readImageFileAsDataUri(file); + + assert.equal(result.contentType, 'image/png'); + assert.equal(result.bytes, PNG.byteLength); + assert.equal(result.image, `data:image/png;base64,${PNG.toString('base64')}`); + }); + + test('sniffs JPEG, GIF and WebP from their signatures, not the extension', async () => { + // Every one of these is named `.png` on purpose. + assert.equal((await readImageFileAsDataUri(await write('a.png', JPEG))).contentType, 'image/jpeg'); + assert.equal((await readImageFileAsDataUri(await write('b.png', GIF))).contentType, 'image/gif'); + assert.equal((await readImageFileAsDataUri(await write('c.png', WEBP))).contentType, 'image/webp'); + }); + + test('rejects HEIC with the same convert-first message the URL path gives', async () => { + const file = await write('photo.heic', HEIC); + await assert.rejects(readImageFileAsDataUri(file), /HEIC is not accepted/); + }); + + // The whole point of sniffing: a PDF renamed to .png would otherwise reach Substack. + test('rejects an unrecognised format and names the bytes it found', async () => { + const file = await write('fake.png', PDF); + await assert.rejects(readImageFileAsDataUri(file), /unrecognised image format.*25 50 44 46/s); + }); + + test('rejects a file too short to carry any signature', async () => { + const file = await write('tiny.png', Buffer.from([0x89, 0x50])); + await assert.rejects(readImageFileAsDataUri(file), /unrecognised image format/); + }); + + // A relative path resolves against the server process's cwd, which is not the caller's. Accepting + // one would read the wrong file, or none, and neither failure would say why. + test('rejects a relative path', async () => { + await assert.rejects(readImageFileAsDataUri('assets/cover.png'), /must be absolute/); + }); + + test('rejects a directory', async () => { + await assert.rejects(readImageFileAsDataUri(dir), /not a regular file/); + }); + + test('reports a missing file by path instead of leaking an ENOENT stack', async () => { + await assert.rejects(readImageFileAsDataUri(path.join(dir, 'nope.png')), /no such file/); + }); + + test('follows a symlink to a real image', async () => { + const target = await write('real.png', PNG); + const link = path.join(dir, 'link.png'); + await fs.symlink(target, link); + + assert.equal((await readImageFileAsDataUri(link)).contentType, 'image/png'); + }); + + test('rejects a file over the byte cap using its size, before reading it', async () => { + const file = await write('big.png', Buffer.concat([PNG, Buffer.alloc(100)])); + await assert.rejects( + readImageFileAsDataUri(file, {maxBytes: 10}), + /over the 10-byte limit/ + ); + }); + + test('accepts a file exactly at the cap', async () => { + const file = await write('atlimit.png', PNG); + const result = await readImageFileAsDataUri(file, {maxBytes: PNG.byteLength}); + assert.equal(result.bytes, PNG.byteLength); + }); + + test('defaults the cap to MAX_IMAGE_BYTES', async () => { + const file = await write('default.png', PNG); + assert.ok(MAX_IMAGE_BYTES > PNG.byteLength); + assert.equal((await readImageFileAsDataUri(file)).bytes, PNG.byteLength); + }); +}); diff --git a/src/server.js b/src/server.js index b743ae9..0694f47 100644 --- a/src/server.js +++ b/src/server.js @@ -62,9 +62,12 @@ export const tools = { }, upload_image: { description: - "Upload an image to your Substack publication from an http(s) URL. The server downloads " + - "the image and re-hosts it on Substack; the returned url is what goes into image2.src in " + - "set_post_body. Private and loopback hosts are refused, HEIC is not accepted, max 10 MB.", + "Host an image on your Substack publication and get back a Substack URL — the one thing " + + "image2.src in set_post_body and cover_image in update_draft will actually render. The " + + "source is either an http(s) URL, which the server downloads and re-hosts, or `path`, an " + + "absolute path to a local file on the machine running this server, which is read straight " + + "from disk: use that for an image you generated or edited locally, with no need to publish " + + "it anywhere first. Private and loopback hosts are refused, HEIC is not accepted, max 10 MB.", schema: uploadImageSchema, handler: uploadImageHandler, }, diff --git a/src/server.spec.js b/src/server.spec.js index 4c5737a..445e062 100644 --- a/src/server.spec.js +++ b/src/server.spec.js @@ -94,15 +94,38 @@ describe('MCP server — list_tools', () => { assert.equal(inputSchema.properties.title.type, 'string'); }); - test('upload_image advertises url + optional post_id and forbids extra keys', async () => { + test('upload_image advertises url + path + optional post_id and forbids extra keys', async () => { const {inputSchema} = (await listToolsByName()).upload_image; assert.equal(inputSchema.type, 'object'); - assert.deepEqual(Object.keys(inputSchema.properties).sort(), ['post_id', 'url']); - assert.deepEqual([...(inputSchema.required ?? [])], ['url']); + assert.deepEqual(Object.keys(inputSchema.properties).sort(), ['path', 'post_id', 'url']); + // Neither source is required on its own: the rule is exactly one of the two, and that is not + // expressible in `required`. + assert.deepEqual([...(inputSchema.required ?? [])], []); assert.equal(inputSchema.additionalProperties, false); }); + // The tool description is what a model reads when deciding WHICH tool to call; the property + // descriptions are only reached once it has already chosen this one. A description advertising + // only a URL leaves `path` discoverable in the schema but invisible in the pitch, which is how a + // model concludes a local file "cannot be uploaded" and reaches for the browser instead. + test('upload_image pitches both sources in its tool description', async () => { + const {description} = (await listToolsByName()).upload_image; + + assert.match(description, /URL/); + assert.match(description, /local file/i); + }); + + // The `.superRefine` enforcing "exactly one of url or path" does NOT survive into the published + // JSON Schema — z.toJSONSchema drops refinements. The descriptions are therefore the only place a + // model can learn the rule before it breaks it, which makes them load-bearing rather than prose. + test('upload_image states the url/path exclusivity in both descriptions', async () => { + const {properties} = (await listToolsByName()).upload_image.inputSchema; + + assert.match(properties.url.description, /exactly one of/i); + assert.match(properties.path.description, /exactly one of/i); + }); + // Regression guard for the zod 3 -> 4 migration: zod-to-json-schema silently returned a // bare `{$schema}` for a zod 4 schema instead of throwing, which would have published a // parameterless tool. Asserting the descriptions — the part an LLM actually reads to fill diff --git a/src/tools/upload_image.js b/src/tools/upload_image.js index c137573..9360223 100644 --- a/src/tools/upload_image.js +++ b/src/tools/upload_image.js @@ -1,26 +1,59 @@ import {z} from "zod"; import SubstackApi from "../api/substack/SubstackApi.js"; -import {fetchImageAsDataUri, defaultLookup, isPrivateAddress, MAX_IMAGE_BYTES} from "../api/substack/image.js"; +import { + fetchImageAsDataUri, + readImageFileAsDataUri, + defaultLookup, + isPrivateAddress, + MAX_IMAGE_BYTES, +} from "../api/substack/image.js"; import {logger} from "../logger.js"; // Re-exported, not redefined: `upload_image.spec.js` imports both from here, and the pipeline they // belong to now lives in `src/api/substack/image.js` because `update_draft` needs it too. export {isPrivateAddress, MAX_IMAGE_BYTES}; +// Two sources, exactly one per call. The `.superRefine` below is the runtime half of that rule; the +// other half is written into both descriptions, because a refinement does NOT survive into the +// published JSON Schema — a model reading tools/list would otherwise meet the rule only by breaking +// it. Same reasoning as the one-paywall rule in `document.js`. +// // strictObject: an unknown key is reported, never stripped — the only repair signal an LLM gets. -export const uploadImageSchema = z.strictObject({ - url: z - .string() - .url() - .describe( - "The http(s) URL of an image to upload. The server downloads it and re-hosts it on Substack. " + - "Private, loopback and link-local hosts are refused. Max 10 MB. HEIC is not accepted." - ), - post_id: z - .number() - .optional() - .describe("Optional id of the post the image belongs to. Its effect is unconfirmed."), -}); +export const uploadImageSchema = z + .strictObject({ + url: z + .string() + .url() + .optional() + .describe( + "The http(s) URL of an image to upload. The server downloads it and re-hosts it on Substack. " + + "Private, loopback and link-local hosts are refused. Max 10 MB. HEIC is not accepted. " + + "Provide exactly one of `url` or `path`." + ), + path: z + .string() + .optional() + .describe( + "Absolute path to an image file on the machine running this server, read directly from disk " + + "with no download. Use this for a locally generated or edited image. The path must be " + + "absolute — a relative one would resolve against the server's working directory, not the " + + "caller's. The type is detected from the file's contents, not its extension: PNG, JPEG, " + + "GIF and WebP are accepted, HEIC is not. Max 10 MB. " + + "Provide exactly one of `url` or `path`." + ), + post_id: z + .number() + .optional() + .describe("Optional id of the post the image belongs to. Its effect is unconfirmed."), + }) + .superRefine((value, ctx) => { + if ((value.url === undefined) === (value.path === undefined)) { + ctx.addIssue({ + code: "custom", + message: "Provide exactly one of `url` (to download an image) or `path` (to read a local file).", + }); + } + }); export const uploadImageHandler = async (args, {lookup = defaultLookup, fetchImpl = fetch} = {}) => { logger.debug('upload_image.start', {args}); @@ -32,10 +65,19 @@ export const uploadImageHandler = async (args, {lookup = defaultLookup, fetchImp logger.error('upload_image.args.invalid', {issues: error.issues ?? error.message}); throw error; } - const {url, post_id} = validatedArgs; + const {url, path: filePath, post_id} = validatedArgs; - logger.info('upload_image.fetching', {url, post_id: post_id ?? null}); - const {image, contentType, bytes} = await fetchImageAsDataUri(url, {lookup, fetchImpl}); + // The intent line goes out before either source is touched, so a read or download that throws + // still leaves a record of what was attempted. + let source; + if (filePath !== undefined) { + logger.info('upload_image.reading', {path: filePath, post_id: post_id ?? null}); + source = await readImageFileAsDataUri(filePath); + } else { + logger.info('upload_image.fetching', {url, post_id: post_id ?? null}); + source = await fetchImageAsDataUri(url, {lookup, fetchImpl}); + } + const {image, contentType, bytes} = source; // The data URI is deliberately NOT logged: hundreds of KB of base64 would bury the session. This // is the one exception to "post content is not truncated". diff --git a/src/tools/upload_image.spec.js b/src/tools/upload_image.spec.js index 09f31b5..e19772f 100644 --- a/src/tools/upload_image.spec.js +++ b/src/tools/upload_image.spec.js @@ -1,5 +1,8 @@ import {test, describe, before, after, afterEach} from 'node:test'; import assert from 'node:assert/strict'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import nodePath from 'node:path'; import {http, HttpResponse} from 'msw'; import {uploadImageHandler, uploadImageSchema, MAX_IMAGE_BYTES, isPrivateAddress} from './upload_image.js'; import {createMswServer, IMAGE_URL, IMAGE_UPLOAD_RESPONSE} from '../../test/helpers/msw-server.js'; @@ -58,6 +61,100 @@ describe('uploadImageHandler — happy path', () => { }); }); +describe('uploadImageHandler — local file source', () => { + const PNG = Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), Buffer.alloc(32)]); + let dir; + + before(async () => { + dir = await fs.mkdtemp(nodePath.join(os.tmpdir(), 'substack-mcp-upload-')); + }); + after(async () => fs.rm(dir, {recursive: true, force: true})); + + const writeFile = async (name, bytes) => { + const file = nodePath.join(dir, name); + await fs.writeFile(file, bytes); + return file; + }; + + test('reads a local file and uploads it as a data URI, with no outbound download', async () => { + const file = await writeFile('cover.png', PNG); + // No source handler is registered: MSW runs with onUnhandledRequest 'error', so any attempt to + // fetch a URL here would fail the test rather than pass silently. + const result = await run({path: file}); + + const upload = msw.requests.find((r) => r.url.endsWith('/api/v1/image')); + assert.equal(upload.body.image, `data:image/png;base64,${PNG.toString('base64')}`); + assert.equal(result.url, IMAGE_UPLOAD_RESPONSE.url); + }); + + test('forwards post_id for a local file too', async () => { + const file = await writeFile('with-post.png', PNG); + await run({path: file, post_id: 42}); + const upload = msw.requests.find((r) => r.url.endsWith('/api/v1/image')); + assert.equal(upload.body.postId, 42); + }); + + test('does not upload when the file is not a recognised image', async () => { + const file = await writeFile('fake.png', Buffer.from('%PDF-1.4 not an image at all')); + await assert.rejects(run({path: file}), /unrecognised image format/); + assert.equal(msw.requests.find((r) => r.url.endsWith('/api/v1/image')), undefined); + }); + + // The intent line goes out BEFORE the read, so a failing read still leaves a record of which file + // was attempted — the mirror of `upload_image.fetching` on the URL branch. Size and type are only + // known afterwards and belong to `upload_image.uploading`, which both branches share. + test('logs which file it is about to read, then the size and type, never the base64', async () => { + const body = Buffer.concat([PNG, Buffer.alloc(5000, 0xab)]); + const file = await writeFile('logged.png', body); + const payload = body.toString('base64'); + + const logs = await captureLogs(() => run({path: file})); + const reading = logs.find((l) => l.msg === 'upload_image.reading'); + const uploading = logs.find((l) => l.msg === 'upload_image.uploading'); + + assert.ok(reading, 'expected an upload_image.reading line'); + assert.equal(reading.path, file); + assert.equal(uploading.bytes, body.byteLength); + assert.equal(uploading.content_type, 'image/png'); + // Not the tool's line, not the API layer's request line either. + assert.equal(JSON.stringify(logs).includes(payload), false); + assert.ok(logs.some((l) => l.msg === 'substack.request')); + }); + + test('logs the attempted path even when the read fails', async () => { + const missing = nodePath.join(dir, 'gone.png'); + const logs = await captureLogs(() => run({path: missing}).catch(() => {})); + const reading = logs.find((l) => l.msg === 'upload_image.reading'); + + assert.equal(reading?.path, missing); + }); +}); + +// A `.refine()` does not survive into the published JSON Schema, so the rule is also stated in both +// descriptions. These pin the runtime half of that pair. +describe('uploadImageSchema — url and path are exclusive', () => { + test('rejects a call with neither', () => { + assert.throws(() => uploadImageSchema.parse({}), /exactly one of/); + }); + + test('rejects a call with both', () => { + assert.throws( + () => uploadImageSchema.parse({url: 'https://example.com/a.png', path: '/tmp/a.png'}), + /exactly one of/ + ); + }); + + test('accepts either one alone', () => { + assert.doesNotThrow(() => uploadImageSchema.parse({url: 'https://example.com/a.png'})); + assert.doesNotThrow(() => uploadImageSchema.parse({path: '/tmp/a.png'})); + }); + + // strictObject still has to report an unknown key — the only repair signal an LLM gets. + test('still reports an unrecognised key', () => { + assert.throws(() => uploadImageSchema.parse({file: '/tmp/a.png'}), /Unrecognized key/); + }); +}); + describe('uploadImageHandler — content validation', () => { test('rejects a non-image source before uploading', async () => { msw.server.use(sourceHandler({body: Buffer.from(''), type: 'text/html'})); From 49b2f2087d80e0573999f609c5ccf6e4ab870d29 Mon Sep 17 00:00:00 2001 From: Marco Moauro Date: Sun, 9 Aug 2026 19:38:26 +0200 Subject: [PATCH 2/2] Say why the local size cap is checked once, not twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review flagged the asymmetry with `readCapped`, which enforces the cap in two places and says so in a comment. The local branch checks only `stat.size`, and the comment called itself "the local mirror of the Content-Length pre-check" without noting that the other half is missing on purpose — which reads as an oversight rather than a decision. It is a decision. `readCapped` re-checks the buffered length because Content-Length is a claim made by an untrusted remote server, which may declare a small length and send more. `stat.size` is the kernel's answer about the file realpath just resolved; there is no second party to disagree with it. The window between the stat and the readFile is a real residual and is now named as one, on the same terms as the DNS-rebinding note above it: the adversary it would buy protection from already has write access to this machine's disk. No behaviour change — comment in image.js and the matching bullet in CLAUDE.md. This repo states its accepted residuals rather than leaving them implicit, and this one was the exception. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 7 ++++++- src/api/substack/image.js | 9 ++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 57c978b..d08ed86 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -411,7 +411,12 @@ Three measured facts shape the tool: runs before the checks, so a symlink is followed to the file actually read and `..` cannot mean one thing at the check and another at the read. - **The size cap is enforced on `stat.size` before the file is read**, the local mirror of the - `Content-Length` pre-check in `readCapped`. + `Content-Length` pre-check in `readCapped` — and only that half of it, deliberately. `readCapped` + re-checks the buffered length afterwards because `Content-Length` is a claim by an untrusted + remote server; `stat.size` is the kernel's answer about the file `realpath` just resolved, with + no second party to disagree. The window between the `stat` and the `readFile` is an accepted + residual, on the same terms as the DNS-rebinding note: the adversary it would buy protection from + already has write access to the disk. Adding the second source meant editing **three** descriptions, and the third was nearly missed: the two property descriptions in the schema, *and* the tool's own `description` in the `tools` registry. diff --git a/src/api/substack/image.js b/src/api/substack/image.js index 38835c7..b6367cf 100644 --- a/src/api/substack/image.js +++ b/src/api/substack/image.js @@ -202,7 +202,14 @@ export async function readImageFileAsDataUri(filePath, {maxBytes = MAX_IMAGE_BYT const stats = await fsp.stat(resolved); if (!stats.isFile()) throw new Error(`image: not a regular file: ${filePath}`); // Refused on the size the filesystem reports, before the bytes are read — the local mirror of the - // Content-Length pre-check in `readCapped`. + // Content-Length pre-check in `readCapped`. Only that half is mirrored, and deliberately: + // `readCapped` re-checks the buffered length afterwards because Content-Length is a claim made by + // an untrusted remote server, which may declare a small length and send more. `stat.size` is the + // kernel's own answer about the file `realpath` just resolved, so there is no second party to + // disagree with. What remains is the window between this `stat` and the `readFile` below — a file + // that grows in between is read whole. Accepted residual, on the same terms as the DNS-rebinding + // note above: closing it would mean reading through a bounded stream, and the adversary it would + // buy protection from already has write access to this machine's disk. if (stats.size > maxBytes) { throw new Error(`image: file is ${stats.size} bytes, over the ${maxBytes}-byte limit.`); }