fix: complete the malformed-input guards, including the two paths still live (#3663)

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.
This commit is contained in:
Daniel Salazar
2026-08-28 11:14:30 -07:00
committed by GitHub
parent f3a46a9be4
commit 9fab4742c9
12 changed files with 247 additions and 47 deletions
@@ -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(
+17 -14
View File
@@ -3779,7 +3779,7 @@ export class AuthController extends PuterController {
rateLimit: CREDENTIAL_MINT_LIMIT,
})
async handleCreateAccessToken(req: Request, res: Response): Promise<void> {
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
+7 -4
View File
@@ -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<T>(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',
});
@@ -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;
@@ -17,6 +17,8 @@
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
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,
@@ -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', () => {
@@ -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,
@@ -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', () => {
@@ -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<string, unknown> | undefined;
| Record<string, unknown>
| 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<string, unknown> | undefined;
| Record<string, unknown>
| undefined;
const byteplusSharedCfg = providers['byteplus'] as
Record<string, unknown> | undefined;
| Record<string, unknown>
| 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,
);
@@ -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();
+30 -7
View File
@@ -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<IGenerateParams, 'input_image' | 'input_images'>,
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
-6
View File
@@ -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());