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.
This commit is contained in:
Daniel Salazar
2026-08-28 10:22:51 -07:00
committed by GitHub
parent 5c8defb940
commit e460e9034c
8 changed files with 217 additions and 25 deletions
@@ -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(
+20 -2
View File
@@ -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];
+28 -4
View File
@@ -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<ClientCompleteWriteResponse>,
) {
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<string, unknown> | undefined;
| Record<string, unknown>
| 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<T>(body: T | undefined): T {
if (!body || typeof body !== 'object') {
throw new HttpError(400, 'A request body is required', {
legacyCode: 'bad_request',
});
}
return body;
}
#withGuiMetadata<T extends { guiMetadata?: WriteGuiMetadata }>(
value: T,
fallbackSource: unknown,
@@ -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 <https://www.gnu.org/licenses/>.
* 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;
@@ -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 <https://www.gnu.org/licenses/>.
* 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 }),
+25 -1
View File
@@ -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<string> {
assertInputImageString(img, 'input image');
if (img.startsWith('data:')) return img;
if (isHttpUrl(img)) {
const { base64, mime } = await fetchImageAsBase64(img);
@@ -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://') ||
+6
View File
@@ -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());