From 4cff803441db288cfae360fe85ba5e58af84541f Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Fri, 28 Aug 2026 10:40:08 -0700 Subject: [PATCH] fix: complete the malformed-input guards, including the two paths still live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous change found three of its claims unmet. The image-generation crash it reported fixed is still reachable. The assert was scattered across three helpers, and Gemini and OpenAI call `isHttpUrl` directly on `input_images` without going through any of them — so two of seven providers still 500 on a non-string. `isHttpUrl` now refuses a non-string itself, and the shape is settled once in `ImageGenerationDriver.generate`, where the driver call arrives, rather than per helper. That also covers `input_images` that isn't an array, which produced a different crash per provider. The sixth case in the ticket, previously unlocated, is `Messages.js` reading `tool_call.function.name` with no guard — reachable with `{"messages":[{"role":"assistant","tool_calls":[{"id":"x"}]}]}`. Guarded, along with the same shape in `make_claude_tools`: a TypeError there carries no status, so the retry loop reads it as a provider failure and marks the route unhealthy for every caller. `#hardExpiryFromExpiresIn` returning null for a bad type moved the failure past the session INSERT, leaving an orphaned non-expiring row and still answering 500. Reverted; the controller guard is the fix, now covering fractions, negatives and unparseable durations rather than only wrong types. Also: the batch write handlers check that the body is an array but not what is in it, so a null element 500s the same way; `#requireObjectBody` accepted an array despite its name; `handleCreateAccessToken` destructured a body that may be absent; and two AGPL notices had been rewrapped with a Markdown link. --- .../controllers/auth/AuthController.test.ts | 18 ++++++-- .../controllers/auth/AuthController.ts | 31 +++++++------ src/backend/controllers/fs/FSController.ts | 11 +++-- .../controllers/fs/FSController.write.test.ts | 43 ++++++++++++++++- .../drivers/ai-chat/utils/FunctionCalling.js | 11 +++++ .../ai-chat/utils/FunctionCalling.test.ts | 20 ++++++-- src/backend/drivers/ai-chat/utils/Messages.js | 9 ++++ .../drivers/ai-chat/utils/Messages.test.ts | 41 +++++++++++++++-- .../drivers/ai-image/ImageGenerationDriver.ts | 21 +++++++-- .../drivers/ai-image/inputImage.test.ts | 46 +++++++++++++++++++ src/backend/drivers/ai-image/inputImage.ts | 37 ++++++++++++--- src/backend/services/auth/AuthService.ts | 6 --- 12 files changed, 247 insertions(+), 47 deletions(-) diff --git a/src/backend/controllers/auth/AuthController.test.ts b/src/backend/controllers/auth/AuthController.test.ts index 2cc1607ab2..d8881bd31f 100644 --- a/src/backend/controllers/auth/AuthController.test.ts +++ b/src/backend/controllers/auth/AuthController.test.ts @@ -2553,9 +2553,13 @@ describe('AuthController.handleCreateAccessToken + handleRevokeAccessToken', () ['an object', {}], ['an array', ['30d']], ['a boolean', true], + ['a fraction', 1.5], + ['a negative', -60], + ['an unparseable duration', 'banana'], + ['an unknown unit', '30x'], ])('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`. + // Anything the signer can't read reaches it only after the session row + // is already inserted, so it has to be refused here. await expect( controller.handleCreateAccessToken( makeReq( @@ -2570,6 +2574,14 @@ describe('AuthController.handleCreateAccessToken + handleRevokeAccessToken', () ).rejects.toMatchObject({ statusCode: 400 }); }); + it('answers 400 when the request has no parsable body', async () => { + const req = makeReq({}, { actor }); + (req as { body?: unknown }).body = undefined; + await expect( + controller.handleCreateAccessToken(req, 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( @@ -7356,7 +7368,7 @@ describe('AuthController.loginWait audience binding', () => { }); it('answers 400 when the request has no parsable body', async () => { - // Destructuring an absent body is a TypeError, which surfaced as a 500. + // Destructuring an absent body is a TypeError, not a 400. const req = makeReq({}, { headers: { origin: OPENER } }); (req as { body?: unknown }).body = undefined; await expect( diff --git a/src/backend/controllers/auth/AuthController.ts b/src/backend/controllers/auth/AuthController.ts index 5bb1e74fc5..981fc50df1 100644 --- a/src/backend/controllers/auth/AuthController.ts +++ b/src/backend/controllers/auth/AuthController.ts @@ -3779,7 +3779,7 @@ export class AuthController extends PuterController { rateLimit: CREDENTIAL_MINT_LIMIT, }) async handleCreateAccessToken(req: Request, res: Response): Promise { - const { permissions, expiresIn, label } = req.body; + const { permissions, expiresIn, label } = req.body ?? {}; if (!Array.isArray(permissions) || permissions.length === 0) { throw new HttpError(400, 'Missing or empty `permissions` array', { legacyCode: 'bad_request', @@ -3798,19 +3798,22 @@ 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' }, - ); + // Whole seconds or a duration string ('30d'). A wrong type, a + // fraction, or an unparseable unit all reach the signer, which throws + // after the session row is already inserted. + if (expiresIn !== undefined && expiresIn !== null) { + const usable = + typeof expiresIn === 'number' + ? Number.isInteger(expiresIn) && expiresIn > 0 + : typeof expiresIn === 'string' && + /^\d+\s*[smhdwy]?$/.test(expiresIn.trim()); + if (!usable) { + throw new HttpError( + 400, + '`expiresIn` must be a whole number of seconds or a duration string like `30d`', + { legacyCode: 'bad_request' }, + ); + } } // Normalize specs: string → [string], [string] → [string, {}], [string, extra] → as-is diff --git a/src/backend/controllers/fs/FSController.ts b/src/backend/controllers/fs/FSController.ts index 1efd1c2d68..6ef864fb6d 100644 --- a/src/backend/controllers/fs/FSController.ts +++ b/src/backend/controllers/fs/FSController.ts @@ -223,7 +223,7 @@ export class FSController extends PuterController { ? await Promise.all( req.body.map(async (requestBody) => { const normalizedRequestBody = this.#withGuiMetadata( - requestBody, + this.#requireObjectBody(requestBody), req.body, ); normalizedRequestBody.fileMetadata = @@ -363,7 +363,10 @@ export class FSController extends PuterController { const userId = this.#getActorUserId(req); const requests = Array.isArray(req.body) ? req.body.map((requestBody) => { - return this.#withGuiMetadata(requestBody, req.body); + return this.#withGuiMetadata( + this.#requireObjectBody(requestBody), + req.body, + ); }) : []; for (const requestBody of requests) { @@ -850,7 +853,7 @@ export class FSController extends PuterController { ? await Promise.all( req.body.map(async (requestBody) => { const normalizedRequestBody = this.#withGuiMetadata( - requestBody, + this.#requireObjectBody(requestBody), req.body, ); normalizedRequestBody.fileMetadata = @@ -2377,7 +2380,7 @@ export class FSController extends PuterController { * that is a 500 where the request deserves a 400. */ #requireObjectBody(body: T | undefined): T { - if (!body || typeof body !== 'object') { + if (!body || typeof body !== 'object' || Array.isArray(body)) { throw new HttpError(400, 'A request body is required', { legacyCode: 'bad_request', }); diff --git a/src/backend/controllers/fs/FSController.write.test.ts b/src/backend/controllers/fs/FSController.write.test.ts index 675e348524..8ba931e2ae 100644 --- a/src/backend/controllers/fs/FSController.write.test.ts +++ b/src/backend/controllers/fs/FSController.write.test.ts @@ -544,8 +544,8 @@ describe('FSController.write', () => { // 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. + // A request whose body never parsed leaves `req.body` undefined, and the + // handlers read fileMetadata straight off it. it.each([ [ 'write', @@ -557,6 +557,11 @@ describe('write handlers with no parsable body', () => { (c: typeof controller, r: Request, res: Response) => c.startWrite(r as never, res as never), ], + [ + 'completeWrite', + (c: typeof controller, r: Request, res: Response) => + c.completeWrite(r as never, res as never), + ], ])('%s answers 400', async (_label, call) => { const { actor } = await makeUser(); const req = makeReq({ actor }); @@ -572,6 +577,40 @@ describe('write handlers with no parsable body', () => { }); }); +describe('batch write handlers with a malformed element', () => { + // The body is an array, so the array check passes; the elements are what + // the handlers then read fileMetadata / thumbnailData off. + it.each([ + [ + 'startBatchWrites', + (c: typeof controller, r: Request, res: Response) => + c.startBatchWrites(r as never, res as never), + ], + [ + 'batchWrites', + (c: typeof controller, r: Request, res: Response) => + c.batchWrites(r as never, res as never), + ], + [ + 'completeBatchWrites', + (c: typeof controller, r: Request, res: Response) => + c.completeBatchWrites(r as never, res as never), + ], + ])('%s answers 400 for a null element', async (_label, call) => { + const { actor } = await makeUser(); + const req = makeReq({ actor }); + (req as { body?: unknown }).body = [null]; + 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-chat/utils/FunctionCalling.js b/src/backend/drivers/ai-chat/utils/FunctionCalling.js index 9880d78f9b..bb9c2ff142 100644 --- a/src/backend/drivers/ai-chat/utils/FunctionCalling.js +++ b/src/backend/drivers/ai-chat/utils/FunctionCalling.js @@ -17,6 +17,8 @@ * along with this program. If not, see . */ +import { HttpError } from '@heyputer/backend/src/core/http'; + export const normalize_json_schema = (schema) => { if (!schema) return schema; @@ -137,6 +139,15 @@ export const make_openai_tools = (tools) => { export const make_claude_tools = (tools) => { if (!tools) return undefined; return tools.map((tool) => { + // A TypeError here carries no status, so it is read as a provider + // failure and marks the route unhealthy for everyone. + if (!tool?.function) { + throw new HttpError( + 400, + "each tool must have a 'function' property", + { legacyCode: 'bad_request' }, + ); + } const { name, description, parameters } = tool.function; return { name, diff --git a/src/backend/drivers/ai-chat/utils/FunctionCalling.test.ts b/src/backend/drivers/ai-chat/utils/FunctionCalling.test.ts index ae460f5498..6275ef4eca 100644 --- a/src/backend/drivers/ai-chat/utils/FunctionCalling.test.ts +++ b/src/backend/drivers/ai-chat/utils/FunctionCalling.test.ts @@ -171,9 +171,7 @@ describe('normalize_tools_object', () => { }); it('mutates the array in place and returns the same reference', () => { - const tools = [ - { name: 'lookup', input_schema: { type: 'object' } }, - ]; + const tools = [{ name: 'lookup', input_schema: { type: 'object' } }]; const out = normalize_tools_object(tools); expect(out).toBe(tools); }); @@ -196,6 +194,22 @@ describe('make_openai_tools', () => { }); }); +describe('make_claude_tools with a malformed tool', () => { + it.each([ + ['no function property', { type: 'web_search' }], + ['a null entry', null], + ])('answers 400 for a tool with %s', (_label, tool) => { + // A TypeError here has no status, so the retry loop reads it as a + // provider failure and marks the route unhealthy for every caller. + expect(() => make_claude_tools([tool] as never)).toThrowError( + expect.objectContaining({ + statusCode: 400, + legacyCode: 'bad_request', + }), + ); + }); +}); + // ── make_claude_tools ─────────────────────────────────────────────── describe('make_claude_tools', () => { diff --git a/src/backend/drivers/ai-chat/utils/Messages.js b/src/backend/drivers/ai-chat/utils/Messages.js index 941405d013..e01d1ad0a5 100644 --- a/src/backend/drivers/ai-chat/utils/Messages.js +++ b/src/backend/drivers/ai-chat/utils/Messages.js @@ -76,6 +76,15 @@ export const normalize_single_message = (message, params = {}) => { message.content = []; for (let i = 0; i < message.tool_calls.length; i++) { const tool_call = message.tool_calls[i]; + // Streaming deltas and non-OpenAI tool-call shapes both omit + // `function`, so it cannot be assumed present. + if (!tool_call?.function) { + throw new HttpError( + 400, + "each tool_call must have a 'function' property", + { legacyCode: 'bad_request' }, + ); + } message.content.push({ type: 'tool_use', id: tool_call.id, diff --git a/src/backend/drivers/ai-chat/utils/Messages.test.ts b/src/backend/drivers/ai-chat/utils/Messages.test.ts index 2f4b216078..a1de462bbe 100644 --- a/src/backend/drivers/ai-chat/utils/Messages.test.ts +++ b/src/backend/drivers/ai-chat/utils/Messages.test.ts @@ -75,9 +75,9 @@ describe('normalize_single_message', () => { }); it('throws 400 when no content + no tool_calls (and not a tool message)', () => { - expect(() => - normalize_single_message({ role: 'assistant' }), - ).toThrow(expect.objectContaining({ statusCode: 400 })); + expect(() => normalize_single_message({ role: 'assistant' })).toThrow( + expect.objectContaining({ statusCode: 400 }), + ); }); it('synthesizes content from tool_calls when content is missing', () => { @@ -192,6 +192,41 @@ describe('normalize_single_message', () => { }); }); +describe('normalize_single_message tool_calls', () => { + it.each([ + ['no function property', { id: 'x' }], + ['a type-only entry', { type: 'function' }], + ['a null entry', null], + ])('answers 400 for a tool_call with %s', (_label, toolCall) => { + // Reachable straight off a /drivers/call body, and it throws before + // the provider call, so it surfaces as a bare 500 otherwise. + expect(() => + normalize_messages([ + { role: 'assistant', tool_calls: [toolCall] }, + ] as never), + ).toThrowError( + expect.objectContaining({ + statusCode: 400, + legacyCode: 'bad_request', + }), + ); + }); + + it('still converts a well-formed tool_call', () => { + const result = normalize_messages([ + { + role: 'assistant', + tool_calls: [ + { id: 'c1', function: { name: 'lookup', arguments: '{}' } }, + ], + }, + ] as never); + expect(result[0].content).toEqual([ + { type: 'tool_use', id: 'c1', name: 'lookup', input: '{}' }, + ]); + }); +}); + // ── normalize_messages ────────────────────────────────────────────── describe('normalize_messages', () => { diff --git a/src/backend/drivers/ai-image/ImageGenerationDriver.ts b/src/backend/drivers/ai-image/ImageGenerationDriver.ts index 8ab5c1e0f8..dfd5ba3a19 100644 --- a/src/backend/drivers/ai-image/ImageGenerationDriver.ts +++ b/src/backend/drivers/ai-image/ImageGenerationDriver.ts @@ -35,6 +35,7 @@ import { ReplicateImageGenerationProvider } from './providers/replicate/Replicat import { TogetherImageProvider } from './providers/together/TogetherImageProvider.js'; import { XAIImageProvider } from './providers/xai/XAIImageProvider.js'; import type { IGenerateParams, IImageModel, IImageProvider } from './types.js'; +import { assertInputImagesShape } from './inputImage.js'; /** * Driver implementing the `puter-image-generation` interface. @@ -127,6 +128,11 @@ export class ImageGenerationDriver extends PuterDriver { legacyCode: 'unauthorized', }); + // Every provider reads these off `args`, and several index into them + // directly rather than through the shared helpers, so the shape is + // settled here — once — before any provider runs. + assertInputImagesShape(args, 'image generation'); + const puterOutputPath = args.puter_output_path; delete args.puter_output_path; @@ -293,7 +299,8 @@ export class ImageGenerationDriver extends PuterDriver { const cloudflare = (providers['cloudflare-image-generation'] ?? providers['cloudflare-workers-ai-image'] ?? providers['cloudflare-workers-ai']) as - Record | undefined; + | Record + | undefined; const cfToken = (cloudflare?.apiToken as string | undefined) ?? (cloudflare?.apiKey as string | undefined) ?? @@ -308,7 +315,8 @@ export class ImageGenerationDriver extends PuterDriver { apiToken: cfToken, accountId: cfAccount, apiBaseUrl: cloudflare?.apiBaseUrl as - string | undefined, + | string + | undefined, }, m, ); @@ -340,9 +348,11 @@ export class ImageGenerationDriver extends PuterDriver { // pair its missing apiBaseUrl with the shared block's key (or vice // versa) and point a region-scoped key at the wrong endpoint. const byteplusImageCfg = providers['byteplus-image-generation'] as - Record | undefined; + | Record + | undefined; const byteplusSharedCfg = providers['byteplus'] as - Record | undefined; + | Record + | undefined; const byteplusKey = readKey(byteplusImageCfg, byteplusSharedCfg); if (byteplusKey) { this.#providers['byteplus-image-generation'] = @@ -351,7 +361,8 @@ export class ImageGenerationDriver extends PuterDriver { apiKey: byteplusKey, apiBaseUrl: (byteplusImageCfg?.apiBaseUrl ?? byteplusSharedCfg?.apiBaseUrl) as - string | undefined, + | string + | undefined, }, m, ); diff --git a/src/backend/drivers/ai-image/inputImage.test.ts b/src/backend/drivers/ai-image/inputImage.test.ts index bd45e8bbd8..af63df80a2 100644 --- a/src/backend/drivers/ai-image/inputImage.test.ts +++ b/src/backend/drivers/ai-image/inputImage.test.ts @@ -26,6 +26,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { + assertInputImagesShape, fetchImageAsBase64, isHttpUrl, parseDataUri, @@ -76,6 +77,51 @@ describe('isHttpUrl', () => { // -- resolveSingleInputImage ----------------------------------------- +describe('isHttpUrl', () => { + // Two providers call this directly on caller data rather than going + // through the helpers below, so it cannot assume a string. + it.each([42, {}, null, undefined, ['https://x/a']])( + 'returns false for the non-string %s', + (value) => { + expect(isHttpUrl(value as never)).toBe(false); + }, + ); +}); + +describe('assertInputImagesShape', () => { + it('accepts absent, null, and well-formed fields', () => { + expect(() => assertInputImagesShape({}, 'test')).not.toThrow(); + expect(() => + assertInputImagesShape( + { input_image: null, input_images: null } as never, + 'test', + ), + ).not.toThrow(); + expect(() => + assertInputImagesShape( + { input_image: 'https://x/a', input_images: ['QUJD'] }, + 'test', + ), + ).not.toThrow(); + }); + + it.each([ + ['a non-string input_image', { input_image: 42 }], + ['a non-string input_images entry', { input_images: [{}] }], + ['a non-array input_images', { input_images: 'QUJD' }], + ['a numeric input_images', { input_images: 5 }], + ])('rejects %s with 400', (_label, params) => { + expect(() => + assertInputImagesShape(params as never, 'test'), + ).toThrowError( + expect.objectContaining({ + statusCode: 400, + legacyCode: 'bad_request', + }), + ); + }); +}); + describe('resolveSingleInputImage', () => { it('returns undefined when neither field is supplied', () => { expect(resolveSingleInputImage({}, 'TestProvider')).toBeUndefined(); diff --git a/src/backend/drivers/ai-image/inputImage.ts b/src/backend/drivers/ai-image/inputImage.ts index a177c2359b..04201b1767 100644 --- a/src/backend/drivers/ai-image/inputImage.ts +++ b/src/backend/drivers/ai-image/inputImage.ts @@ -30,8 +30,11 @@ import { HttpError } from '../../core/http/HttpError.js'; import { secureFetch } from '../../util/secureHttp.js'; import type { IGenerateParams } from './types.js'; -export function isHttpUrl(s: string): boolean { - return s.startsWith('http://') || s.startsWith('https://'); +export function isHttpUrl(s: unknown): boolean { + return ( + typeof s === 'string' && + (s.startsWith('http://') || s.startsWith('https://')) + ); } /** @@ -51,20 +54,40 @@ export function toUrlOrDataUri(img: string, mimeHint?: string): string { * 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 { +export function assertInputImageString(img: unknown, label: string): string { if (typeof img !== 'string') { throw new HttpError( 400, - `${providerLabel}: each input image must be a URL, data-URI, or base64 string.`, + `${label}: each input image must be a URL, data-URI, or base64 string.`, { legacyCode: 'bad_request' }, ); } return img; } +/** + * Validate `input_image` / `input_images` once, where the driver call arrives. + * Providers reach for `.startsWith` on these, and several do it without going + * through the helpers here, so the shape has to be settled before any of them + * runs. + */ +export function assertInputImagesShape( + params: Pick, + label: string, +): void { + if (params.input_image !== undefined && params.input_image !== null) { + assertInputImageString(params.input_image, label); + } + const imgs = params.input_images; + if (imgs === undefined || imgs === null) return; + if (!Array.isArray(imgs)) { + throw new HttpError(400, `${label}: input_images must be an array.`, { + legacyCode: 'bad_request', + }); + } + for (const img of imgs) assertInputImageString(img, label); +} + /** * 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 diff --git a/src/backend/services/auth/AuthService.ts b/src/backend/services/auth/AuthService.ts index 19fb2d83a9..b35528fb0b 100644 --- a/src/backend/services/auth/AuthService.ts +++ b/src/backend/services/auth/AuthService.ts @@ -464,12 +464,6 @@ 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());