diff --git a/README.md b/README.md index a61c310..5c8abfb 100644 --- a/README.md +++ b/README.md @@ -390,14 +390,45 @@ Filtros para las cuatro preguntas que no se pueden responder en ningún otro sitio: qué está **sin procesar**, qué **falló**, qué **no usa nadie** (lo mismo que se llevaría `content:prune --media`) y qué **ya no está en disco**. -No se sube desde ahí: los archivos llegan al subirlos en una ficha. Y **no se -puede borrar uno que algo esté usando** — dejaría una imagen rota en la web, que +No se sube desde ahí: los archivos llegan al subirlos en una ficha (o por la +API, abajo). Y **no se puede borrar uno que algo esté usando** — dejaría una imagen rota en la web, que es justo de lo que se queja la página de salud. El botón solo aparece en los huérfanos. «Reprocesar todo» es la respuesta a «he cambiado los anchos, ¿y ahora?»: la tabla es derivada, así que reconstruirla siempre es seguro. +### Subir por la API + +`POST /api/v1/media` es el campo de subida de una ficha, sin la ficha: guarda la +imagen donde el panel pone las de las páginas y contesta con el `path` que se +manda como `featured_image`. Existe para el [MCP](#servidor-mcp), que sin él +podía crear una página pero nunca ponerle una foto. Pasa por `Media::for()`, +así que se procesa, se sirve y se poda igual que una subida desde el panel. + +Dos formas de entrada, una por petición: **multipart** con `file`, o **JSON** +con una `url` que la API descarga ella misma. Solo JPEG, PNG, WebP, GIF y AVIF, +decidido por los bytes y no por el nombre ni el tipo declarado. **Sin SVG**: el +panel lo acepta de quien lo administra, pero esto también descarga de URLs +arbitrarias, y un SVG es un documento que puede llevar script servido desde +este dominio. 20 MB como mucho (`MEDIA_UPLOAD_MAX_KB`). + +**La URL es la parte delicada**, porque la petición la hace este servidor desde +dentro de la red del stack: sin control, sería una forma de pedirle +`http://postgres:5432` o el servicio de metadatos de la nube. +[`RemoteImage`](app/Services/Media/RemoteImage.php) exige HTTPS, resuelve el +dominio y **rechaza si una sola de sus direcciones no es pública** (privadas, +reservadas, loopback, link-local, 100.64/10, IPv4 mapeada en IPv6), **fija la +conexión a la dirección comprobada** con `CURLOPT_RESOLVE` para que un DNS que +cambie entre la comprobación y la petición no cuele otra, **no sigue +redirecciones** y deja de leer al pasar del límite, diga lo que diga +`Content-Length`. + +El permiso es escribir páginas (`pages.create` o `pages.update`), no ninguno de +`media.*`: esos siempre describieron cuidar ficheros que ya existen, y +`MediaPolicy::create` sigue siendo `false` para que el panel no gane un botón de +«nuevo fichero» suelto. + ### Galerías Un proyecto tiene además una galería —capturas, el antes y el después— que se @@ -1225,6 +1256,7 @@ POST /api/v1/pages/{page}/restore GET /api/v1/users PATCH para roles, is_active, locale GET /api/v1/roles solo lectura GET /api/v1/roles/catalogue catálogo de permisos agrupado +POST /api/v1/media subir una imagen (fichero o URL) ``` `GET /v1/me` devuelve el array `permissions`: úsalo para construir la UI del @@ -1419,6 +1451,12 @@ salvo `LANDING_MCP_HOST`/`LANDING_MCP_PORT`), para probarlo sin Docker. avisa a gente real. - Las cabeceras no se exponen como parámetros: son secretos, y salen de la configuración, no del modelo. +- **Subir imágenes** (`admin_media_store`): en local el modelo pasa una ruta de + tu disco (`file_path`) y el servidor la lee y la sube como multipart —en + base64 una foto normal serían cientos de miles de tokens—. **Desplegado no hay + `file_path`**: el servidor leería su propio disco a petición de cualquiera, así + que solo se ofrece `body.url`, que descarga la API con las protecciones de + [Subir por la API](#subir-por-la-api). `npm test` (en `mcp/`, y en CI) genera las herramientas desde el `openapi.yaml` de este repo —un cambio en el documento que haga chocar dos diff --git a/app/Http/Controllers/Api/V1/MediaController.php b/app/Http/Controllers/Api/V1/MediaController.php new file mode 100644 index 0000000..5cf5999 --- /dev/null +++ b/app/Http/Controllers/Api/V1/MediaController.php @@ -0,0 +1,85 @@ + + */ + protected const EXTENSIONS = [ + 'image/jpeg' => 'jpg', + 'image/png' => 'png', + 'image/webp' => 'webp', + 'image/gif' => 'gif', + 'image/avif' => 'avif', + ]; + + public function store(MediaRequest $request, RemoteImage $remote): JsonResponse + { + $this->authorize('upload', Media::class); + + $field = $request->hasFile('file') ? 'file' : 'url'; + + $contents = $field === 'file' + ? (string) $request->file('file')->get() + : $remote->fetch((string) $request->string('url')); + + $mime = (new finfo(FILEINFO_MIME_TYPE))->buffer($contents) ?: null; + $size = @getimagesizefromstring($contents); + + // Both, because each catches what the other misses: the type is read + // from the magic bytes, and a file that has them but does not decode + // is not an image anybody can render. + if ($size === false || ! in_array($mime, (array) config('media.upload.mimes'), true)) { + throw ValidationException::withMessages([$field => [__('rules.media.not_an_image')]]); + } + + $path = config('media.upload.directory').'/'.Str::random(40).'.'.self::EXTENSIONS[$mime]; + + Storage::disk((string) config('media.disk'))->put($path, $contents); + + $media = Media::for($path); + + // What is already known, so the answer carries dimensions straight + // away rather than after the queue has run. The processor writes the + // same values, and the rest, when it does. + if (! $media->isProcessed()) { + $media->forceFill([ + 'mime' => $mime, + 'size' => strlen($contents), + 'width' => $size[0], + 'height' => $size[1], + ])->save(); + } + + return MediaResource::make($media->fresh())->response()->setStatusCode(201); + } +} diff --git a/app/Http/Requests/Api/V1/MediaRequest.php b/app/Http/Requests/Api/V1/MediaRequest.php new file mode 100644 index 0000000..324e23e --- /dev/null +++ b/app/Http/Requests/Api/V1/MediaRequest.php @@ -0,0 +1,45 @@ + + */ + public function rules(): array + { + return [ + 'file' => [ + 'required_without:url', + 'prohibits:url', + 'file', + 'max:'.(int) config('media.upload.max_kb'), + 'mimetypes:'.implode(',', (array) config('media.upload.mimes')), + ], + 'url' => ['required_without:file', 'string', 'url:https', 'max:2048'], + ]; + } +} diff --git a/app/Http/Resources/MediaResource.php b/app/Http/Resources/MediaResource.php new file mode 100644 index 0000000..916c13c --- /dev/null +++ b/app/Http/Resources/MediaResource.php @@ -0,0 +1,35 @@ + + */ + public function toArray(Request $request): array + { + return [ + // What a record stores — `featured_image` on a page — and so the one + // field a caller has to keep. + 'path' => $this->path, + 'url' => $this->url(), + 'mime' => $this->mime, + 'size' => $this->size, + 'width' => $this->width, + 'height' => $this->height, + // False until the queue has derived the smaller versions and the + // placeholder. The image serves either way. + 'processed' => $this->isProcessed(), + ]; + } +} diff --git a/app/Policies/MediaPolicy.php b/app/Policies/MediaPolicy.php index 37b39f7..8b22962 100644 --- a/app/Policies/MediaPolicy.php +++ b/app/Policies/MediaPolicy.php @@ -21,6 +21,19 @@ class MediaPolicy extends ResourcePolicy return false; } + /** + * Uploading through the API, which has no form to attach a file to. + * + * Still true to the rule above in spirit: through the API the one record + * that holds an image is a page, so whoever may write pages may upload the + * image for one — and nobody else, whatever their `media.*` permissions, + * which only ever described looking after files that already exist. + */ + public function upload(User $user): bool + { + return $user->can('pages.create') || $user->can('pages.update'); + } + /** * A file something still points at cannot be deleted from here: doing so * would leave a broken image on the public site, which is precisely what diff --git a/app/Services/Media/HostResolver.php b/app/Services/Media/HostResolver.php new file mode 100644 index 0000000..79f90a0 --- /dev/null +++ b/app/Services/Media/HostResolver.php @@ -0,0 +1,26 @@ + + */ + public function resolve(string $host): array + { + $v4 = gethostbynamel($host) ?: []; + $v6 = array_column(@dns_get_record($host, DNS_AAAA) ?: [], 'ipv6'); + + return array_values(array_unique([...$v4, ...$v6])); + } +} diff --git a/app/Services/Media/RemoteImage.php b/app/Services/Media/RemoteImage.php new file mode 100644 index 0000000..6353df5 --- /dev/null +++ b/app/Services/Media/RemoteImage.php @@ -0,0 +1,130 @@ +fail('https_only'); + } + + $host = trim((string) $host, '[]'); + $port = (int) ($parts['port'] ?? 443); + $address = $this->publicAddressFor($host); + $maxBytes = (int) config('media.upload.max_kb') * 1024; + + try { + $response = Http::withOptions([ + 'allow_redirects' => false, + 'stream' => true, + 'connect_timeout' => 5, + 'timeout' => (int) config('media.upload.remote_timeout', 15), + 'curl' => [CURLOPT_RESOLVE => [sprintf( + '%s:%d:%s', + $host, + $port, + str_contains($address, ':') ? "[{$address}]" : $address, + )]], + ]) + ->withHeaders(['Accept' => 'image/*']) + ->get($url); + } catch (ConnectionException) { + $this->fail('unreachable'); + } + + if ($response->redirect()) { + $this->fail('redirect'); + } + + if ($response->status() !== 200) { + $this->fail('status', ['status' => $response->status()]); + } + + if ((int) $response->header('Content-Length') > $maxBytes) { + $this->fail('too_large', ['max' => (int) config('media.upload.max_kb')]); + } + + $body = $response->toPsrResponse()->getBody(); + $contents = ''; + + while (! $body->eof()) { + $contents .= $body->read(65536); + + if (strlen($contents) > $maxBytes) { + $this->fail('too_large', ['max' => (int) config('media.upload.max_kb')]); + } + } + + return $contents; + } + + /** + * The address to connect to, once every address the host has is known to + * be public. + */ + protected function publicAddressFor(string $host): string + { + $addresses = filter_var($host, FILTER_VALIDATE_IP) ? [$host] : $this->resolver->resolve($host); + + if ($addresses === []) { + $this->fail('unresolvable'); + } + + foreach ($addresses as $address) { + // GLOBAL_RANGE is the strict one: besides the private and reserved + // blocks it refuses shared (100.64/10), link-local and the + // IPv4-mapped IPv6 spelling of any of them. + if (! filter_var($address, FILTER_VALIDATE_IP, FILTER_FLAG_GLOBAL_RANGE)) { + $this->fail('private'); + } + } + + return $addresses[0]; + } + + /** + * @param array $replace + */ + protected function fail(string $reason, array $replace = []): never + { + throw ValidationException::withMessages([ + 'url' => [__("rules.media.remote.{$reason}", $replace)], + ]); + } +} diff --git a/config/media.php b/config/media.php index 765c4fe..9e2458f 100644 --- a/config/media.php +++ b/config/media.php @@ -76,4 +76,30 @@ return [ // Formats that are copied through untouched. Resampling a vector loses the // point of it. 'passthrough_mimes' => ['image/svg+xml', 'image/gif'], + + /* + |-------------------------------------------------------------------------- + | Uploads through the API + |-------------------------------------------------------------------------- + | + | POST /api/v1/media, which the MCP server uses to put an image on a page. + | + | Raster formats only. The panel's own upload accepts SVG from the people + | who run it; this endpoint also fetches from arbitrary URLs, and an SVG is + | a document that can carry script, served from this application's domain. + | + | The size ceiling sits under php.ini's `upload_max_filesize` (24M) so the + | answer is a readable 422 rather than PHP silently dropping the file. + | + */ + + 'upload' => [ + 'mimes' => ['image/jpeg', 'image/png', 'image/webp', 'image/gif', 'image/avif'], + 'max_kb' => (int) env('MEDIA_UPLOAD_MAX_KB', 20 * 1024), + 'directory' => 'pages', + + // Fetching from a URL: how long to wait, in seconds. The request that + // triggered it is waiting too. + 'remote_timeout' => (int) env('MEDIA_REMOTE_TIMEOUT', 15), + ], ]; diff --git a/lang/ca/rules.php b/lang/ca/rules.php index 9e3428c..869bdfe 100644 --- a/lang/ca/rules.php +++ b/lang/ca/rules.php @@ -28,4 +28,17 @@ return [ 'pages' => [ 'cannot_publish' => 'No tens permís per publicar pàgines.', ], + + 'media' => [ + 'not_an_image' => 'El fitxer no és una imatge JPEG, PNG, WebP, GIF o AVIF.', + 'remote' => [ + 'https_only' => 'La URL de la imatge ha de començar per https://.', + 'unresolvable' => 'No s\'ha pogut resoldre el domini de la URL.', + 'private' => 'La URL apunta a una adreça privada o reservada: només es descarreguen imatges d\'adreces públiques.', + 'unreachable' => 'No s\'ha pogut connectar amb la URL.', + 'redirect' => 'La URL redirigeix a una altra adreça. Fes servir l\'adreça final de la imatge.', + 'status' => 'La URL ha respost :status en lloc de la imatge.', + 'too_large' => 'La imatge supera el màxim de :max KB.', + ], + ], ]; diff --git a/lang/en/rules.php b/lang/en/rules.php index eb042da..62ae1fe 100644 --- a/lang/en/rules.php +++ b/lang/en/rules.php @@ -28,4 +28,17 @@ return [ 'pages' => [ 'cannot_publish' => 'You do not have permission to publish pages.', ], + + 'media' => [ + 'not_an_image' => 'The file is not a JPEG, PNG, WebP, GIF or AVIF image.', + 'remote' => [ + 'https_only' => 'The image URL must start with https://.', + 'unresolvable' => 'The URL\'s domain could not be resolved.', + 'private' => 'The URL points at a private or reserved address: images are only downloaded from public ones.', + 'unreachable' => 'Could not connect to the URL.', + 'redirect' => 'The URL redirects elsewhere. Use the image\'s final address.', + 'status' => 'The URL answered :status instead of the image.', + 'too_large' => 'The image is larger than the :max KB limit.', + ], + ], ]; diff --git a/lang/es/rules.php b/lang/es/rules.php index 4359bb5..87cbe32 100644 --- a/lang/es/rules.php +++ b/lang/es/rules.php @@ -28,4 +28,17 @@ return [ 'pages' => [ 'cannot_publish' => 'No tienes permiso para publicar páginas.', ], + + 'media' => [ + 'not_an_image' => 'El fichero no es una imagen JPEG, PNG, WebP, GIF o AVIF.', + 'remote' => [ + 'https_only' => 'La URL de la imagen tiene que empezar por https://.', + 'unresolvable' => 'No se ha podido resolver el dominio de la URL.', + 'private' => 'La URL apunta a una dirección privada o reservada: solo se descargan imágenes de direcciones públicas.', + 'unreachable' => 'No se ha podido conectar con la URL.', + 'redirect' => 'La URL redirige a otra dirección. Usa la dirección final de la imagen.', + 'status' => 'La URL ha respondido :status en lugar de la imagen.', + 'too_large' => 'La imagen supera el máximo de :max KB.', + ], + ], ]; diff --git a/mcp/server.mjs b/mcp/server.mjs index 8f61019..3b4464b 100644 --- a/mcp/server.mjs +++ b/mcp/server.mjs @@ -49,6 +49,8 @@ const catalogue = new Catalogue({ // A deployment outlives many deploys of the application; a stdio process // lives for one editor session and describes its tools once anyway. ttlMs: http ? 60_000 : Infinity, + // Over stdio the disk the server reads is the caller's own. + localFiles: !http, }); if (http) { diff --git a/mcp/src/catalogue.mjs b/mcp/src/catalogue.mjs index 97c74b2..a06cf88 100644 --- a/mcp/src/catalogue.mjs +++ b/mcp/src/catalogue.mjs @@ -16,11 +16,13 @@ export class Catalogue { * @param {string} options.source Path or URL of openapi.yaml. * @param {string} options.baseUrl Application root the document's server URL is relative to. * @param {number} [options.ttlMs] How long a loaded document is trusted. Infinity: load once. + * @param {boolean} [options.localFiles] Offer uploads from a local path; see `buildTools`. */ - constructor({ source, baseUrl, ttlMs = Infinity }) { + constructor({ source, baseUrl, ttlMs = Infinity, localFiles = false }) { this.source = source; this.baseUrl = baseUrl.replace(/\/+$/, ''); this.ttlMs = ttlMs; + this.localFiles = localFiles; this.current = null; this.loadedAt = 0; this.pending = null; @@ -58,7 +60,7 @@ export class Catalogue { this.current = { spec, - tools: buildTools(spec), + tools: buildTools(spec, { localFiles: this.localFiles }), apiBase: /^https?:\/\//.test(serverUrl) ? serverUrl : `${this.baseUrl}${serverUrl}`, }; this.loadedAt = Date.now(); diff --git a/mcp/src/client.mjs b/mcp/src/client.mjs index e558125..f8fb2a2 100644 --- a/mcp/src/client.mjs +++ b/mcp/src/client.mjs @@ -8,12 +8,19 @@ * expired upstream costs one extra round trip, not a broken session. */ +import { readFile, stat } from 'node:fs/promises'; +import { basename } from 'node:path'; + // Refresh this long before the stated expiry, so a token never expires between // the check and the request. const EXPIRY_MARGIN_MS = 60_000; const TIMEOUT_MS = 30_000; +// Above the API's own 20 MB so its answer, not this one, is what a caller +// usually sees; this only stops something absurd being read into memory. +const MAX_UPLOAD_BYTES = 25 * 1024 * 1024; + export class LandingClient { /** * @param {object} options @@ -55,7 +62,7 @@ export class LandingClient { * @returns {Promise<{status: number, body: unknown}>} */ async call(operation, args = {}) { - const request = this.buildRequest(operation, args); + const request = await this.buildRequest(operation, args); if (!operation.requiresAuth) { if (this.previewToken) request.headers['X-Preview-Token'] = this.previewToken; @@ -77,7 +84,7 @@ export class LandingClient { return response; } - buildRequest(operation, args) { + async buildRequest(operation, args) { let path = operation.path; const query = new URLSearchParams(); @@ -100,7 +107,20 @@ export class LandingClient { const headers = { Accept: 'application/json' }; let body; - if (operation.hasBody && args.body !== undefined) { + const uploads = (operation.files ?? []).filter((field) => args[`${field}_path`]); + + if (uploads.length > 0) { + // No Content-Type: fetch writes it, boundary included, from the form. + body = new FormData(); + + for (const [key, value] of Object.entries(args.body ?? {})) { + body.append(key, typeof value === 'object' ? JSON.stringify(value) : String(value)); + } + + for (const field of uploads) { + body.append(field, ...(await this.readUpload(args[`${field}_path`]))); + } + } else if (operation.hasBody && args.body !== undefined) { headers['Content-Type'] = 'application/json'; body = JSON.stringify(args.body); } @@ -115,6 +135,22 @@ export class LandingClient { }; } + /** @returns {Promise<[Blob, string]>} */ + async readUpload(path) { + let info; + + try { + info = await stat(path); + } catch { + throw new Error(`Cannot read ${path}: no such file`); + } + + if (!info.isFile()) throw new Error(`${path} is not a file`); + if (info.size > MAX_UPLOAD_BYTES) throw new Error(`${path} is ${Math.round(info.size / 1048576)} MB; the limit is 20 MB`); + + return [new Blob([await readFile(path)]), basename(path)]; + } + async send({ url, method, headers, body }) { const response = await this.fetch(url, { method, diff --git a/mcp/src/tools.mjs b/mcp/src/tools.mjs index 442f837..b13b358 100644 --- a/mcp/src/tools.mjs +++ b/mcp/src/tools.mjs @@ -30,6 +30,7 @@ const DROPPED_KEYWORDS = new Set([ * @property {string} path Path as documented, e.g. /v1/pages/{page}. * @property {{name: string, in: string}[]} params * @property {boolean} hasBody + * @property {string[]} files Multipart fields filled from a local file, read from `_path`. * @property {boolean} requiresAuth * * @typedef {object} Tool @@ -42,9 +43,13 @@ const DROPPED_KEYWORDS = new Set([ /** * @param {object} spec The parsed OpenAPI document. + * @param {object} [options] + * @param {boolean} [options.localFiles] Offer uploads from a path on this + * machine. Only true over stdio, where this machine is the caller's own; a + * deployed server would be reading its own disk on a stranger's say-so. * @returns {Tool[]} */ -export function buildTools(spec) { +export function buildTools(spec, { localFiles = false } = {}) { const paths = Object.keys(spec.paths ?? {}); const tools = []; @@ -58,7 +63,9 @@ export function buildTools(spec) { if (EXCLUDED_OPERATIONS.has(`${method.toUpperCase()} ${path}`)) continue; if ((op.tags ?? []).some((tag) => EXCLUDED_TAGS.has(tag))) continue; - tools.push(buildTool(spec, paths, path, method, item, op)); + const tool = buildTool(spec, paths, path, method, item, op, localFiles); + + if (tool) tools.push(tool); } } @@ -79,7 +86,7 @@ export function buildTools(spec) { return tools; } -function buildTool(spec, paths, path, method, item, op) { +function buildTool(spec, paths, path, method, item, op, localFiles) { const params = mergeParameters(spec, item.parameters, op.parameters) // Headers are credentials here (preview and ingest secrets), which come // from the server's configuration, never from the model. @@ -99,6 +106,32 @@ function buildTool(spec, paths, path, method, item, op) { } const bodySchema = op.requestBody?.content?.['application/json']?.schema; + const multipartSchema = op.requestBody?.content?.['multipart/form-data']?.schema; + + // A file in a multipart body cannot travel as JSON — as base64 an ordinary + // photo costs hundreds of thousands of tokens — so the model names a path + // and the server reads it. Only where that path is on the caller's machine. + const files = localFiles && multipartSchema + ? Object.entries(toJsonSchema(spec, multipartSchema).properties ?? {}) + .filter(([, property]) => property.format === 'binary') + .map(([name]) => name) + : []; + + // Multipart only, and no way to send a file from here: nothing to offer. + if (!bodySchema && multipartSchema && files.length === 0) return null; + + for (const field of files) { + const name = `${field}_path`; + + if (name in properties) { + throw new Error(`${method.toUpperCase()} ${path} has a parameter named "${name}", which the file upload needs`); + } + + properties[name] = { + type: 'string', + description: `Absolute path to a file on this machine, uploaded as \`${field}\`. Read by the MCP server; use instead of \`body\`.`, + }; + } if (bodySchema) { if ('body' in properties) { @@ -114,7 +147,9 @@ function buildTool(spec, paths, path, method, item, op) { properties.body = { ...body, description: 'JSON request body.' }; - if (op.requestBody.required) required.push('body'); + // With a file to send instead, the JSON body is the other way in rather + // than a requirement; the API says which one it missed if neither comes. + if (op.requestBody.required && files.length === 0) required.push('body'); } const security = op.security ?? spec.security ?? []; @@ -139,6 +174,7 @@ function buildTool(spec, paths, path, method, item, op) { path, params: params.map(({ name, in: location }) => ({ name, in: location })), hasBody: Boolean(bodySchema), + files, requiresAuth: security.length > 0, }, }; diff --git a/mcp/test/client.test.mjs b/mcp/test/client.test.mjs index f1a1fbe..556d80e 100644 --- a/mcp/test/client.test.mjs +++ b/mcp/test/client.test.mjs @@ -1,4 +1,7 @@ import assert from 'node:assert/strict'; +import { mkdtemp, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { test } from 'node:test'; import { LandingClient } from '../src/client.mjs'; @@ -149,3 +152,47 @@ test('refuses a call missing a path parameter', async () => { await assert.rejects(client.call(showPage, {}), /"page"/); }); + +const uploadMedia = { + method: 'POST', + path: '/v1/media', + params: [], + hasBody: true, + files: ['file'], + requiresAuth: true, +}; + +test('uploads a local file as multipart, never as JSON', async () => { + const dir = await mkdtemp(join(tmpdir(), 'landing-mcp-')); + const file = join(dir, 'hero.png'); + await writeFile(file, Buffer.from([0x89, 0x50, 0x4e, 0x47])); + + const { fetch, calls } = fakeFetch(() => ({ status: 201, body: { data: { path: 'pages/x.png' } } })); + const client = new LandingClient({ apiBase: 'http://app.test/api', token: 't', fetch }); + + const response = await client.call(uploadMedia, { file_path: file }); + const sent = calls[0].body; + + assert.equal(response.status, 201); + assert.ok(sent instanceof FormData); + assert.equal(sent.get('file').name, 'hero.png'); + assert.equal(sent.get('file').size, 4); + // fetch has to write it, boundary and all. + assert.equal(calls[0].headers['Content-Type'], undefined); +}); + +test('sends the JSON body when there is no file', async () => { + const { fetch, calls } = fakeFetch(() => ({ status: 201, body: {} })); + const client = new LandingClient({ apiBase: 'http://app.test/api', token: 't', fetch }); + + await client.call(uploadMedia, { body: { url: 'https://images.example.org/a.png' } }); + + assert.equal(calls[0].headers['Content-Type'], 'application/json'); + assert.equal(JSON.parse(calls[0].body).url, 'https://images.example.org/a.png'); +}); + +test('says which file it could not read', async () => { + const client = new LandingClient({ apiBase: 'http://app.test/api', token: 't', fetch: async () => assert.fail('no request') }); + + await assert.rejects(client.call(uploadMedia, { file_path: '/nope/missing.png' }), /\/nope\/missing\.png/); +}); diff --git a/mcp/test/tools.test.mjs b/mcp/test/tools.test.mjs index f096f9b..c8eb8d0 100644 --- a/mcp/test/tools.test.mjs +++ b/mcp/test/tools.test.mjs @@ -79,3 +79,20 @@ test('only the admin API is marked as needing a token', () => { assert.equal(tool.operation.requiresAuth, tool.name.startsWith('admin_'), tool.name); } }); + +test('over stdio an upload takes a local path, and the JSON body becomes optional', () => { + const local = buildTools(spec, { localFiles: true }).find((tool) => tool.name === 'admin_media_store'); + + assert.deepEqual(local.operation.files, ['file']); + assert.equal(local.inputSchema.properties.file_path.type, 'string'); + assert.ok(local.inputSchema.properties.body.properties.url); + assert.equal(local.inputSchema.required, undefined); +}); + +test('over HTTP there is no local path to offer: only the URL', () => { + const remote = byName.get('admin_media_store'); + + assert.deepEqual(remote.operation.files, []); + assert.ok(!('file_path' in remote.inputSchema.properties)); + assert.deepEqual(remote.inputSchema.required, ['body']); +}); diff --git a/public/openapi.yaml b/public/openapi.yaml index d1c0acc..1810313 100644 --- a/public/openapi.yaml +++ b/public/openapi.yaml @@ -108,6 +108,8 @@ tags: description: Translatable page categories - name: Users description: Local user administration + - name: Media + description: Images uploaded for a record to point at - name: Roles description: Local authorisation model (read-only) @@ -1172,6 +1174,61 @@ paths: "204": { description: Deleted } "403": { $ref: "#/components/responses/Forbidden" } + /v1/media: + post: + tags: [Media] + summary: Upload an image + description: | + Stores an image and answers with the `path` a record points at — send + it as a page's `featured_image`. Two ways in, exactly one per request: + + - **`multipart/form-data`** with the image as `file`. + - **`application/json`** with a `url`, which the API downloads itself. + HTTPS only, from a public address — anything resolving to a private, + reserved or loopback address is refused — and redirects are not + followed: send the image's final address. + + JPEG, PNG, WebP, GIF or AVIF, decided from the file's bytes rather than + its name or declared type; SVG is not accepted here. 20 MB at most. + + The smaller versions and the placeholder are derived afterwards, on the + queue: `processed` says whether that has happened, and the image serves + either way. Requires permission to create or edit pages. + requestBody: + required: true + content: + multipart/form-data: + schema: + type: object + required: [file] + properties: + file: + type: string + format: binary + description: The image. + application/json: + schema: + type: object + required: [url] + properties: + url: + type: string + format: uri + description: An `https://` address the API downloads the image from. + example: https://images.example.org/hero.jpg + responses: + "201": + description: Image stored + content: + application/json: + schema: + type: object + properties: + data: { $ref: "#/components/schemas/Media" } + "401": { $ref: "#/components/responses/Unauthorized" } + "403": { $ref: "#/components/responses/Forbidden" } + "422": { $ref: "#/components/responses/ValidationError" } + /v1/users: get: tags: [Users] @@ -2124,6 +2181,24 @@ components: nullable: true description: Must be after `published_at`. When it arrives the record leaves the site on its own. + Media: + type: object + properties: + path: + type: string + description: What a record stores, e.g. as a page's `featured_image`. + example: pages/Xk3fQ9sLw2PmN8vR1tY6uZ0aB4cD7eF5gH2jK9mN.jpg + url: { type: string, format: uri, description: Absolute. } + mime: { type: string, nullable: true, example: image/jpeg } + size: { type: integer, nullable: true, description: Bytes. } + width: { type: integer, nullable: true } + height: { type: integer, nullable: true } + processed: + type: boolean + description: | + Whether the queue has derived the smaller versions and the + placeholder yet. The image serves either way. + Category: type: object properties: diff --git a/routes/api.php b/routes/api.php index de9e49d..2ab798d 100644 --- a/routes/api.php +++ b/routes/api.php @@ -12,6 +12,7 @@ use App\Http\Controllers\Api\PublicApi\V1\SiteController; use App\Http\Controllers\Api\PublicApi\V1\TelemetryController; use App\Http\Controllers\Api\V1\CategoryController; use App\Http\Controllers\Api\V1\MeController; +use App\Http\Controllers\Api\V1\MediaController; use App\Http\Controllers\Api\V1\PageController; use App\Http\Controllers\Api\V1\RoleController; use App\Http\Controllers\Api\V1\TokenController; @@ -165,6 +166,9 @@ Route::prefix('v1')->name('api.v1.')->group(function () { Route::apiResource('categories', CategoryController::class); + // Upload only: the file is then referenced from a record by its path. + Route::post('media', [MediaController::class, 'store'])->name('media.store'); + Route::apiResource('users', UserController::class)->only(['index', 'show', 'update']); // Declared before the resource route so "catalogue" is not swallowed diff --git a/tests/Feature/Api/AdminApiContractTest.php b/tests/Feature/Api/AdminApiContractTest.php index 692885d..fae19b6 100644 --- a/tests/Feature/Api/AdminApiContractTest.php +++ b/tests/Feature/Api/AdminApiContractTest.php @@ -9,6 +9,8 @@ use App\Models\Page; use App\Models\User; use Database\Seeders\RolePermissionSeeder; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Storage; use PHPUnit\Framework\Attributes\Test; use Tests\FakesPireon; use Tests\TestCase; @@ -120,6 +122,22 @@ class AdminApiContractTest extends TestCase $this->assertSame([], $problems, "The admin API and its specification disagree:\n".implode("\n", $problems)); } + #[Test] + public function an_upload_answers_in_the_shape_it_documents(): void + { + Storage::fake((string) config('media.disk')); + + $response = $this->post('/api/v1/media', [ + 'file' => UploadedFile::fake()->image('hero.jpg', 64, 48), + ], ['Accept' => 'application/json'])->assertCreated(); + + $record = $response->json('data'); + $schema = $this->recordSchema('/v1/media', 'post', 201); + + $this->assertSame([], array_values(array_diff(array_keys($record), $this->documentedKeys($schema))), 'Sent but not documented'); + $this->assertSame([], array_values(array_diff($this->alwaysPresent($schema), array_keys($record))), 'Documented but not sent'); + } + /** * The pagination envelope every listing here uses — `data`, `links`, * `meta` — checked the same way the record inside it is: nothing sent diff --git a/tests/Feature/Api/MediaUploadTest.php b/tests/Feature/Api/MediaUploadTest.php new file mode 100644 index 0000000..0950ceb --- /dev/null +++ b/tests/Feature/Api/MediaUploadTest.php @@ -0,0 +1,323 @@ +seed(RolePermissionSeeder::class); + + Storage::fake((string) config('media.disk')); + } + + protected function actingWithRole(string $role): void + { + $user = User::factory()->create(); + $user->syncRoles([$role]); + + $this->withHeaders($this->apiHeadersFor($user)); + } + + /** + * Answers DNS for the hosts the tests invent, so nothing here depends on + * the network — and so a test can make a name resolve somewhere private. + * + * @param array> $answers + */ + protected function resolving(array $answers): void + { + $this->app->instance(HostResolver::class, new class($answers) extends HostResolver + { + /** @param array> $answers */ + public function __construct(private array $answers) {} + + public function resolve(string $host): array + { + return $this->answers[$host] ?? []; + } + }); + } + + /** + * `Http::assertNothingSent()` would also count the token check against the + * identity server; what matters is that the image host was never asked. + */ + protected function assertNoImageWasFetched(): void + { + Http::assertNotSent(fn ($request) => ! str_starts_with($request->url(), $this->pireonServer)); + } + + protected function png(int $width = 12, int $height = 8): string + { + return UploadedFile::fake()->image('x.png', $width, $height)->get(); + } + + #[Test] + public function it_stores_an_uploaded_image_where_the_panel_puts_a_pages_images(): void + { + $this->actingWithRole('editor'); + + $response = $this->post('/api/v1/media', [ + 'file' => UploadedFile::fake()->image('Foto de portada.JPG', 1200, 800), + ], ['Accept' => 'application/json']); + + $response->assertCreated() + ->assertJsonPath('data.mime', 'image/jpeg') + ->assertJsonPath('data.width', 1200) + ->assertJsonPath('data.height', 800); + + $path = $response->json('data.path'); + + // Named by the server, never after what the client sent. + $this->assertMatchesRegularExpression('#^pages/[A-Za-z0-9]{40}\.jpg$#', $path); + Storage::disk((string) config('media.disk'))->assertExists($path); + $this->assertDatabaseHas('media', ['path' => $path]); + $this->assertStringEndsWith($path, $response->json('data.url')); + } + + #[Test] + public function the_path_it_answers_with_is_what_a_page_points_at(): void + { + $this->actingWithRole('editor'); + + $path = $this->post('/api/v1/media', [ + 'file' => UploadedFile::fake()->image('hero.png', 800, 600), + ], ['Accept' => 'application/json'])->json('data.path'); + + $page = Page::factory()->create(); + + $this->patchJson("/api/v1/pages/{$page->id}", ['featured_image' => $path]) + ->assertOk() + ->assertJsonPath('data.featured_image', $path) + ->assertJsonPath('data.featured_image_media.width', 800); + + // One upload, one row: attaching it to the page reuses what the upload + // already created instead of starting over. + $this->assertSame(1, Media::query()->where('path', $path)->count()); + } + + #[Test] + public function it_downloads_an_image_from_a_public_url(): void + { + $this->actingWithRole('editor'); + $this->resolving(['images.example.test' => ['93.184.216.34']]); + + Http::fake([ + 'https://images.example.test/*' => Http::response($this->png(30, 20), 200, ['Content-Type' => 'image/png']), + ]); + + $this->postJson('/api/v1/media', ['url' => 'https://images.example.test/cover.png']) + ->assertCreated() + ->assertJsonPath('data.mime', 'image/png') + ->assertJsonPath('data.width', 30); + } + + /** + * @return array>, 1: string}> + */ + public static function unsafeDestinations(): array + { + return [ + 'a name resolving to a private address' => [['intranet.example.test' => ['10.0.0.5']], 'https://intranet.example.test/a.png'], + 'one private address among public ones' => [['mixed.example.test' => ['93.184.216.34', '192.168.1.10']], 'https://mixed.example.test/a.png'], + 'loopback, literally' => [[], 'https://127.0.0.1/a.png'], + 'the cloud metadata service' => [[], 'https://169.254.169.254/latest/meta-data'], + 'shared address space' => [[], 'https://100.64.0.1/a.png'], + 'IPv6 loopback' => [[], 'https://[::1]/a.png'], + 'IPv4-mapped loopback' => [[], 'https://[::ffff:127.0.0.1]/a.png'], + 'a name that does not resolve' => [[], 'https://nowhere.example.test/a.png'], + ]; + } + + /** + * @param array> $dns + */ + #[Test] + #[DataProvider('unsafeDestinations')] + public function it_never_fetches_from_somewhere_private(array $dns, string $url): void + { + $this->actingWithRole('editor'); + $this->resolving($dns); + + Http::fake(); + + $this->postJson('/api/v1/media', ['url' => $url]) + ->assertUnprocessable() + ->assertJsonValidationErrors('url'); + + $this->assertNoImageWasFetched(); + } + + #[Test] + public function it_refuses_plain_http(): void + { + $this->actingWithRole('editor'); + Http::fake(); + + $this->postJson('/api/v1/media', ['url' => 'http://images.example.test/a.png']) + ->assertUnprocessable() + ->assertJsonValidationErrors('url'); + + $this->assertNoImageWasFetched(); + } + + #[Test] + public function it_does_not_follow_a_redirect(): void + { + $this->actingWithRole('editor'); + $this->resolving(['images.example.test' => ['93.184.216.34']]); + + Http::fake([ + 'https://images.example.test/*' => Http::response('', 302, ['Location' => 'https://10.0.0.5/a.png']), + ]); + + $this->postJson('/api/v1/media', ['url' => 'https://images.example.test/a.png']) + ->assertUnprocessable() + ->assertJsonPath('errors.url.0', __('rules.media.remote.redirect')); + } + + #[Test] + public function it_stops_reading_past_the_size_limit(): void + { + config()->set('media.upload.max_kb', 1); + + $this->actingWithRole('editor'); + $this->resolving(['images.example.test' => ['93.184.216.34']]); + + // No Content-Length to go by: the limit has to hold while reading. + Http::fake([ + 'https://images.example.test/*' => Http::response(str_repeat('x', 4096), 200), + ]); + + $this->postJson('/api/v1/media', ['url' => 'https://images.example.test/a.png']) + ->assertUnprocessable() + ->assertJsonPath('errors.url.0', __('rules.media.remote.too_large', ['max' => 1])); + } + + #[Test] + public function a_url_that_answers_with_something_else_is_not_stored(): void + { + $this->actingWithRole('editor'); + $this->resolving(['images.example.test' => ['93.184.216.34']]); + + // Claims to be an image; the bytes say otherwise. + Http::fake([ + 'https://images.example.test/*' => Http::response('nope', 200, ['Content-Type' => 'image/png']), + ]); + + $this->postJson('/api/v1/media', ['url' => 'https://images.example.test/a.png']) + ->assertUnprocessable() + ->assertJsonPath('errors.url.0', __('rules.media.not_an_image')); + + $this->assertDatabaseCount('media', 0); + } + + /** + * @return array + */ + public static function notImages(): array + { + return [ + 'text renamed to .jpg' => ['photo.jpg', 'not an image at all'], + // Served from this domain, an SVG is a document that can run script. + 'an SVG' => ['logo.svg', ''], + ]; + } + + #[Test] + #[DataProvider('notImages')] + public function it_refuses_an_upload_that_is_not_a_raster_image(string $name, string $contents): void + { + $this->actingWithRole('editor'); + + $this->post('/api/v1/media', [ + 'file' => UploadedFile::fake()->createWithContent($name, $contents), + ], ['Accept' => 'application/json']) + ->assertUnprocessable() + ->assertJsonValidationErrors('file'); + + $this->assertDatabaseCount('media', 0); + } + + #[Test] + public function it_wants_exactly_one_source(): void + { + $this->actingWithRole('editor'); + + $this->postJson('/api/v1/media', []) + ->assertUnprocessable() + ->assertJsonValidationErrors(['file', 'url']); + + $this->post('/api/v1/media', [ + 'file' => UploadedFile::fake()->image('a.png'), + 'url' => 'https://images.example.test/a.png', + ], ['Accept' => 'application/json']) + ->assertUnprocessable() + ->assertJsonValidationErrors('file'); + } + + #[Test] + public function whoever_writes_pages_may_upload(): void + { + $this->actingWithRole('writer'); + + $this->post('/api/v1/media', ['file' => UploadedFile::fake()->image('a.png')], ['Accept' => 'application/json']) + ->assertCreated(); + } + + #[Test] + public function whoever_only_reads_may_not(): void + { + $this->actingWithRole('viewer'); + + $this->post('/api/v1/media', ['file' => UploadedFile::fake()->image('b.png')], ['Accept' => 'application/json']) + ->assertForbidden(); + + $this->assertDatabaseCount('media', 0); + } + + #[Test] + public function it_refuses_a_url_before_fetching_it_for_someone_who_may_not_upload(): void + { + $this->actingWithRole('viewer'); + $this->resolving(['images.example.test' => ['93.184.216.34']]); + + Http::fake(); + + $this->postJson('/api/v1/media', ['url' => 'https://images.example.test/a.png']) + ->assertForbidden(); + + $this->assertNoImageWasFetched(); + } + + #[Test] + public function the_panel_still_has_no_way_to_create_a_file_on_its_own(): void + { + $editor = User::factory()->create(); + $editor->syncRoles(['editor']); + + $this->assertTrue($editor->can('upload', Media::class)); + $this->assertFalse($editor->can('create', Media::class)); + } +}