diff --git a/CLAUDE.md b/CLAUDE.md index 949cbe3..d08ed86 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,47 @@ 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` — 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. + 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..b6367cf 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,92 @@ 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`. 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.`); + } + + 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 +245,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'}));