From e460e9034c736c888b3d9933b90d80b2d866093a Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Fri, 28 Aug 2026 10:22:51 -0700 Subject: [PATCH] fix: answer 400 instead of 500 on four malformed-input paths (#3662) Each of these read a field off caller input that wasn't the shape the code assumed, threw a TypeError, and was served as a 500 with a critical page. - POST /fs/write and /startWrite: a request whose body never parsed left `req.body` undefined, and the first read of `fileMetadata` threw. - POST /drivers/call, image generation: `input_image` / `input_images` entries are documented as strings but nothing checked, so a number or an object reached `.startsWith`. Type confusion on caller input, so reachable on demand rather than by accident. - POST /auth/create-access-token: `expiresIn` went to the expiry parser unvalidated, where anything but a string or a number has no `.trim`. - POST /login/wait: destructuring `session` out of an absent body threw. Same class as the POST /login body ticket; that one covers /login itself. --- .../controllers/auth/AuthController.test.ts | 42 ++++++++++++ .../controllers/auth/AuthController.ts | 22 ++++++- src/backend/controllers/fs/FSController.ts | 32 ++++++++-- .../controllers/fs/FSController.write.test.ts | 48 +++++++++++--- .../drivers/ai-image/inputImage.test.ts | 64 ++++++++++++++++--- src/backend/drivers/ai-image/inputImage.ts | 26 +++++++- .../providers/xai/XAIImageProvider.ts | 2 + src/backend/services/auth/AuthService.ts | 6 ++ 8 files changed, 217 insertions(+), 25 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index bc642b830..2cc1607ab 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -2549,6 +2549,27 @@ describe('AuthController.handleCreateAccessToken + handleRevokeAccessToken', () ).rejects.toMatchObject({ statusCode: 400 }); }); + it.each([ + ['an object', {}], + ['an array', ['30d']], + ['a boolean', true], + ])('rejects %s as expiresIn with 400', async (_label, expiresIn) => { + // Only seconds or a duration string are valid; anything else reached + // the expiry parser and 500'd on `.trim`. + await expect( + controller.handleCreateAccessToken( + makeReq( + { + permissions: ['fs:read'], + expiresIn: expiresIn as unknown as string, + }, + { actor }, + ), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + it('rejects a permission spec that is neither a string nor a tuple with 400', async () => { await expect( controller.handleCreateAccessToken( @@ -7334,6 +7355,27 @@ describe('AuthController.loginWait audience binding', () => { ).rejects.toMatchObject({ statusCode: 403 }); }); + it('answers 400 when the request has no parsable body', async () => { + // Destructuring an absent body is a TypeError, which surfaced as a 500. + const req = makeReq({}, { headers: { origin: OPENER } }); + (req as { body?: unknown }).body = undefined; + await expect( + controller.loginWait(req, makeRes()), + ).rejects.toMatchObject({ statusCode: 400, legacyCode: 'bad_request' }); + }); + + it('rejects a non-string session id with 400', async () => { + await expect( + controller.loginWait( + makeReq( + { session: { nested: true } as unknown as string }, + { headers: { origin: OPENER } }, + ), + makeRes(), + ), + ).rejects.toMatchObject({ statusCode: 400 }); + }); + it('still rejects a malformed session id before looking at Origin', async () => { await expect( controller.loginWait( diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index d65351e4f..5bb1e74fc 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -239,9 +239,12 @@ export class AuthController extends PuterController { ], }) async loginWait(req: Request, res: Response) { - const { session } = req.body; + // Destructuring an absent body is a TypeError, not the 400 a request + // with no session id deserves. + const session = (req.body as { session?: unknown } | undefined) + ?.session; // validate uuid to prevent ultra long key or listening on pubsub.login.* - if (!session || !validateUuid(session)) { + if (typeof session !== 'string' || !validateUuid(session)) { throw new HttpError(400, 'session is required.', { legacyCode: 'bad_request', }); @@ -3795,6 +3798,21 @@ export class AuthController extends PuterController { normalizedLabel = label.trim().slice(0, 64) || null; } + // jsonwebtoken takes seconds or a duration string ('30d'); anything + // else reaches the expiry parser as something with no `.trim`. + if ( + expiresIn !== undefined && + expiresIn !== null && + typeof expiresIn !== 'string' && + typeof expiresIn !== 'number' + ) { + throw new HttpError( + 400, + '`expiresIn` must be a number of seconds or a duration string', + { legacyCode: 'bad_request' }, + ); + } + // Normalize specs: string → [string], [string] → [string, {}], [string, extra] → as-is const normalized = permissions.map((spec) => { if (typeof spec === 'string') return [spec]; diff --git a/src/backend/controllers/fs/FSController.ts b/src/backend/controllers/fs/FSController.ts index a42c6a25f..1efd1c2d6 100644 --- a/src/backend/controllers/fs/FSController.ts +++ b/src/backend/controllers/fs/FSController.ts @@ -152,7 +152,10 @@ export class FSController extends PuterController { ) { const userId = this.#getActorUserId(req); const storageAllowanceMax = this.#getStorageAllowanceMaxOverride(req); - const requestBody = this.#withGuiMetadata(req.body, req.body); + const requestBody = this.#withGuiMetadata( + this.#requireObjectBody(req.body), + req.body, + ); requestBody.fileMetadata = await this.#normalizeFileMetadataPath( req, requestBody.fileMetadata, @@ -319,7 +322,10 @@ export class FSController extends PuterController { res: Response, ) { const userId = this.#getActorUserId(req); - const requestBody = this.#withGuiMetadata(req.body, req.body); + const requestBody = this.#withGuiMetadata( + this.#requireObjectBody(req.body), + req.body, + ); this.#assertNoInlineSignedThumbnailData(requestBody.thumbnailData); const response = await this.services.fs.completeUrlWrite( @@ -443,7 +449,10 @@ export class FSController extends PuterController { ) { const userId = this.#getActorUserId(req); const storageAllowanceMax = this.#getStorageAllowanceMaxOverride(req); - const requestBody = this.#withGuiMetadata(req.body, req.body); + const requestBody = this.#withGuiMetadata( + this.#requireObjectBody(req.body), + req.body, + ); requestBody.fileMetadata = await this.#normalizeFileMetadataPath( req, requestBody.fileMetadata, @@ -2213,7 +2222,8 @@ export class FSController extends PuterController { // the ActorUser type. Access via the escape hatch until a proper // storage-quota mechanism is in place. const actorUser = req.actor?.user as - Record | undefined; + | Record + | undefined; const candidates = [ this.#toStorageCapacityCandidate(actorUser?.free_storage), @@ -2361,6 +2371,20 @@ export class FSController extends PuterController { return guiMetadata; } + /** + * The write handlers build on `req.body` being an object. A request whose + * body never parsed leaves it undefined, and the first field read after + * that is a 500 where the request deserves a 400. + */ + #requireObjectBody(body: T | undefined): T { + if (!body || typeof body !== 'object') { + throw new HttpError(400, 'A request body is required', { + legacyCode: 'bad_request', + }); + } + return body; + } + #withGuiMetadata( value: T, fallbackSource: unknown, diff --git a/src/backend/controllers/fs/FSController.write.test.ts b/src/backend/controllers/fs/FSController.write.test.ts index 7ef0bf0cb..675e34852 100644 --- a/src/backend/controllers/fs/FSController.write.test.ts +++ b/src/backend/controllers/fs/FSController.write.test.ts @@ -3,18 +3,19 @@ * * This file is part of Puter. * - * Puter is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published - * by the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. + * Puter is free software: you can redistribute it and/or modify it under the + * terms of the GNU Affero General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) any + * later version. * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. * * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . + * along with this program. If not, see + * [https://www.gnu.org/licenses/](https://www.gnu.org/licenses/). */ import type { Request, Response } from 'express'; @@ -542,6 +543,35 @@ describe('FSController.write', () => { // false makes the ceiling free disk space), so this group runs its own // server with the limit switched on. +describe('write handlers with no parsable body', () => { + // A request whose body never parsed leaves `req.body` undefined; reading + // fileMetadata off it used to 500 instead of answering 400. + it.each([ + [ + 'write', + (c: typeof controller, r: Request, res: Response) => + c.write(r as never, res as never), + ], + [ + 'startWrite', + (c: typeof controller, r: Request, res: Response) => + c.startWrite(r as never, res as never), + ], + ])('%s answers 400', async (_label, call) => { + const { actor } = await makeUser(); + const req = makeReq({ actor }); + (req as { body?: unknown }).body = undefined; + const { res } = makeRes(); + + await expect( + withActor(actor, () => call(controller, req, res)), + ).rejects.toMatchObject({ + statusCode: 400, + legacyCode: 'bad_request', + }); + }); +}); + describe('FSController.write storage allowance', () => { let limitedServer: PuterServer; let limitedController: FSController; diff --git a/src/backend/drivers/ai-image/inputImage.test.ts b/src/backend/drivers/ai-image/inputImage.test.ts index 0071fa5e5..bd45e8bbd 100644 --- a/src/backend/drivers/ai-image/inputImage.test.ts +++ b/src/backend/drivers/ai-image/inputImage.test.ts @@ -3,18 +3,19 @@ * * This file is part of Puter. * - * Puter is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published - * by the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. + * Puter is free software: you can redistribute it and/or modify it under the + * terms of the GNU Affero General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) any + * later version. * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. * * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . + * along with this program. If not, see + * [https://www.gnu.org/licenses/](https://www.gnu.org/licenses/). */ /** @@ -101,6 +102,44 @@ describe('resolveSingleInputImage', () => { ).toBeUndefined(); }); + it.each([ + ['a number', 42], + ['an object', {}], + ['a nested array', ['inner']], + ])('throws 400 for %s in input_image', (_label, img) => { + // The field comes straight off the driver call; a non-string used to + // reach `.startsWith` and 500. + expect(() => + resolveSingleInputImage( + { input_image: img } as never, + 'TestProvider', + ), + ).toThrowError( + expect.objectContaining({ + statusCode: 400, + legacyCode: 'bad_request', + }), + ); + }); + + it('treats a null input_image as absent, not as a bad request', () => { + expect( + resolveSingleInputImage( + { input_image: null } as never, + 'TestProvider', + ), + ).toBeUndefined(); + }); + + it('throws 400 for a non-string input_images entry', () => { + expect(() => + resolveSingleInputImage( + { input_images: [{ url: 'x' }] } as never, + 'TestProvider', + ), + ).toThrowError(expect.objectContaining({ statusCode: 400 })); + }); + it('throws 400 naming the provider when more than one image is supplied', () => { try { resolveSingleInputImage( @@ -233,6 +272,13 @@ describe('toBase64DataUri', () => { ); }); + it('rejects a non-string with 400 rather than a 500 from startsWith', async () => { + await expect(toBase64DataUri(42 as never)).rejects.toMatchObject({ + statusCode: 400, + }); + expect(secureFetchMock).not.toHaveBeenCalled(); + }); + it('propagates a failed remote fetch rather than producing an empty image', async () => { secureFetchMock.mockResolvedValueOnce( fetchResponse(Buffer.alloc(0), { ok: false, status: 500 }), diff --git a/src/backend/drivers/ai-image/inputImage.ts b/src/backend/drivers/ai-image/inputImage.ts index e31ab7863..a177c2359 100644 --- a/src/backend/drivers/ai-image/inputImage.ts +++ b/src/backend/drivers/ai-image/inputImage.ts @@ -40,11 +40,31 @@ export function isHttpUrl(s: string): boolean { * `mimeHint` (default image/png). */ export function toUrlOrDataUri(img: string, mimeHint?: string): string { + assertInputImageString(img, 'input image'); return isHttpUrl(img) || img.startsWith('data:') ? img : `data:${mimeHint ?? 'image/png'};base64,${img}`; } +/** + * An input image is a URL, a data-URI or raw base64 — always a string. The + * field comes straight off the driver call, so the type has to be checked + * before the helpers below reach for `.startsWith`. + */ +export function assertInputImageString( + img: unknown, + providerLabel: string, +): string { + if (typeof img !== 'string') { + throw new HttpError( + 400, + `${providerLabel}: each input image must be a URL, data-URI, or base64 string.`, + { legacyCode: 'bad_request' }, + ); + } + return img; +} + /** * Resolve the single input image for providers that only support one. Throws * 400 if `input_images` carries more than one entry. Returns the chosen image @@ -62,7 +82,10 @@ export function resolveSingleInputImage( { legacyCode: 'bad_request' }, ); } - return params.input_image ?? imgs?.[0]; + const chosen = params.input_image ?? imgs?.[0]; + return chosen === undefined + ? undefined + : assertInputImageString(chosen, providerLabel); } const DATA_URI_PATTERN = /^data:([^;,]+)?(?:;base64)?,(.*)$/s; @@ -103,6 +126,7 @@ export async function toBase64DataUri( img: string, mimeHint?: string, ): Promise { + assertInputImageString(img, 'input image'); if (img.startsWith('data:')) return img; if (isHttpUrl(img)) { const { base64, mime } = await fetchImageAsBase64(img); diff --git a/src/backend/drivers/ai-image/providers/xai/XAIImageProvider.ts b/src/backend/drivers/ai-image/providers/xai/XAIImageProvider.ts index a68b3db9f..10dc29299 100644 --- a/src/backend/drivers/ai-image/providers/xai/XAIImageProvider.ts +++ b/src/backend/drivers/ai-image/providers/xai/XAIImageProvider.ts @@ -27,6 +27,7 @@ import type { } from '../../types.js'; import { XAI_IMAGE_GENERATION_MODELS } from './models.js'; import { HttpError } from '../../../../core/http/HttpError.js'; +import { assertInputImageString } from '../../inputImage.js'; const DEFAULT_MODEL = 'grok-imagine-image'; // xAI's Grok Imagine edit endpoint accepts up to 3 source images per request. @@ -193,6 +194,7 @@ export class XAIImageProvider implements IImageProvider { // xAI accepts a public URL or a base64 data URI for input images. #toImageRef(img: string, mimeHint?: string) { + assertInputImageString(img, 'xAI'); const url = img.startsWith('http://') || img.startsWith('https://') || diff --git a/src/backend/services/auth/AuthService.ts b/src/backend/services/auth/AuthService.ts index b35528fb0..19fb2d83a 100644 --- a/src/backend/services/auth/AuthService.ts +++ b/src/backend/services/auth/AuthService.ts @@ -464,6 +464,12 @@ export class AuthService extends PuterService { expiresIn: string | number | undefined, ): number | null { if (expiresIn === undefined) return null; + // Route handlers validate this, but the parser is reachable from + // internal callers too — a wrong type reads as "no hard expiry" + // rather than throwing halfway through a mint. + if (typeof expiresIn !== 'string' && typeof expiresIn !== 'number') { + return null; + } const now = nowSeconds(); if (typeof expiresIn === 'number') return now + Math.floor(expiresIn); const match = /^(\d+)\s*([smhdwy])?$/.exec(expiresIn.trim());