fix(api): report bad request bodies and fractional limits as client errors (#484)
A malformed JSON body, or a body over the route's size limit, made Hono throw an HTTPException that the API reported as a 500 internal error. In the companion API the same requests returned 503 "unavailable, please retry", so clients retried a request that could never succeed. These now return 400 bad_request and 413 payload_too_large in the API's usual error shape. A fractional limit or offset such as ?limit=1.5 was passed to SQLite as a float and failed with a datatype mismatch. Limits, offsets and cursors are now truncated to whole numbers, and the two private copies of the clamp helper reuse the shared one.
This commit is contained in:
1 parent
be7bbe25e6
commit
518fafa86e
7 files changed
+111
-10
No files matched your search
@@ -0,0 +1,87 @@
|
||||
import { afterEach, describe, expect, test } from "bun:test";
|
||||
import { Database } from "bun:sqlite";
|
||||
import { mkdtemp, readdir, readFile, rm } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { clampNumber } from "./entity-utils.ts";
|
||||
import { fetchEdgeEverApp } from "./index.ts";
|
||||
import { createSelfHostedStorageAdapter } from "./self-hosted-storage-adapter.ts";
|
||||
|
||||
const temporaryDirectories = [];
|
||||
const databases = [];
|
||||
afterEach(async () => {
|
||||
for (const database of databases.splice(0)) database.close();
|
||||
await Promise.all(temporaryDirectories.splice(0).map((directory) => rm(directory, { recursive: true, force: true })));
|
||||
});
|
||||
|
||||
const createEnvironment = async () => {
|
||||
const directory = await mkdtemp(`${tmpdir()}/edgeever-request-errors-`);
|
||||
temporaryDirectories.push(directory);
|
||||
const database = new Database(join(directory, "edgeever.sqlite"), { create: true });
|
||||
databases.push(database);
|
||||
database.exec("PRAGMA foreign_keys = ON");
|
||||
const migrationDirectory = new URL("../../../migrations/", import.meta.url);
|
||||
for (const name of (await readdir(migrationDirectory)).filter((value) => value.endsWith(".sql")).sort()) {
|
||||
database.exec(await readFile(new URL(name, migrationDirectory), "utf8"));
|
||||
}
|
||||
database.query("INSERT OR IGNORE INTO workspaces (id, name, is_personal) VALUES ('ws_default', 'Personal', 1)").run();
|
||||
return {
|
||||
storage: createSelfHostedStorageAdapter(database, join(directory, "resources")),
|
||||
EDGE_EVER_ALLOW_UNAUTHENTICATED: "true",
|
||||
EDGE_EVER_RUNTIME: "self-hosted-bun",
|
||||
};
|
||||
};
|
||||
|
||||
const request = (environment, path, init = {}) => fetchEdgeEverApp(
|
||||
new Request(`http://edgeever.test${path}`, init),
|
||||
environment,
|
||||
{ waitUntil: () => undefined, passThroughOnException: () => undefined },
|
||||
);
|
||||
|
||||
const jsonPost = (body) => ({
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body,
|
||||
});
|
||||
|
||||
describe("API request errors", () => {
|
||||
test("reports a malformed JSON body as a client error", async () => {
|
||||
const environment = await createEnvironment();
|
||||
const response = await request(environment, "/api/v1/notebooks", jsonPost("{\"name\":"));
|
||||
|
||||
expect(response.status).toBe(400);
|
||||
expect(await response.json()).toEqual({
|
||||
error: { code: "bad_request", message: "Malformed JSON in request body" },
|
||||
});
|
||||
});
|
||||
|
||||
test("reports an oversized body as payload too large", async () => {
|
||||
const environment = await createEnvironment();
|
||||
const oversized = JSON.stringify({ prompt: "x".repeat(450_000) });
|
||||
|
||||
for (const path of ["/api/v1/plugins/ai/generate", "/api/v1/companion/turns"]) {
|
||||
const response = await request(environment, path, jsonPost(oversized));
|
||||
expect({ path, status: response.status }).toEqual({ path, status: 413 });
|
||||
expect(await response.json()).toEqual({
|
||||
error: { code: "payload_too_large", message: "The request body is too large." },
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
test("accepts a fractional list limit instead of failing in SQL", async () => {
|
||||
const environment = await createEnvironment();
|
||||
const response = await request(environment, "/api/v1/memos?limit=1.5");
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(Array.isArray((await response.json()).memos)).toBe(true);
|
||||
});
|
||||
|
||||
test("clamps limits and offsets to whole numbers", () => {
|
||||
expect(clampNumber(1.5, 1, 100)).toBe(1);
|
||||
expect(clampNumber(99.9, 1, 100)).toBe(99);
|
||||
expect(clampNumber(250, 1, 100)).toBe(100);
|
||||
expect(clampNumber(-3, 0, 100)).toBe(0);
|
||||
expect(clampNumber(Number.NaN, 1, 100)).toBe(1);
|
||||
expect(clampNumber(Number.POSITIVE_INFINITY, 0, 50)).toBe(50);
|
||||
});
|
||||
});
|
||||
@@ -7,7 +7,7 @@ import { bodyLimit } from "hono/body-limit";
|
||||
import type { AppContext, AppEnv, Bindings } from "./api-context";
|
||||
import { AppError } from "./app-error";
|
||||
import { loadDefaultAiModel, loadDefaultAiModelCredentials } from "./ai-service";
|
||||
import { apiError, forbidden, notFound } from "./http-errors";
|
||||
import { apiError, forbidden, httpExceptionError, notFound } from "./http-errors";
|
||||
import { requireUser, getWorkspaceId } from "./request-auth";
|
||||
import { beginCompanionTurn, checkpointCompanionTurn, clearCompanionHistory, companionRevision,
|
||||
forgetCompanionMemory, getCompanionTurn, listCompanionMemories, listCompanionTurns, mapCompanionTurn,
|
||||
@@ -23,6 +23,8 @@ import { acknowledgeDiscovery, rememberDiscoveryFeedback, checkDiscoveries, comp
|
||||
const scopeFor = (c: AppContext): CompanionScope => ({ workspaceId: getWorkspaceId(c), ownerId: c.get("auth").actorId! });
|
||||
const fail = (c: AppContext, error: unknown) => {
|
||||
if (error instanceof AppError) return apiError(c, error.code, error.message, error.status);
|
||||
const requestError = httpExceptionError(c, error);
|
||||
if (requestError) return requestError;
|
||||
console.error("companion_failed", error instanceof Error ? error.name : "unknown");
|
||||
return apiError(c, "companion_failed", "The companion is unavailable. Please retry later.", 503);
|
||||
};
|
||||
|
||||
@@ -2,8 +2,10 @@ export const createId = (prefix: string) => `${prefix}_${crypto.randomUUID().rep
|
||||
|
||||
export const isoNow = () => new Date().toISOString();
|
||||
|
||||
// Limits, offsets and cursors are bound to SQL integer parameters, which
|
||||
// reject fractions, so a value such as `?limit=1.5` is truncated first.
|
||||
export const clampNumber = (value: number, min: number, max: number) =>
|
||||
Number.isNaN(value) ? min : Math.min(Math.max(value, min), max);
|
||||
Number.isNaN(value) ? min : Math.min(Math.max(Math.trunc(value), min), max);
|
||||
|
||||
export const parseJsonArray = (json: string): string[] => {
|
||||
try {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { Context } from "hono";
|
||||
import { HTTPException } from "hono/http-exception";
|
||||
|
||||
export const apiError = (c: Context, code: string, message: string, status: number) =>
|
||||
c.json(
|
||||
@@ -41,3 +42,14 @@ export const unauthorized = (c: Context, message: string) =>
|
||||
|
||||
export const forbidden = (c: Context, message: string) =>
|
||||
apiError(c, "forbidden", message, 403);
|
||||
|
||||
// Hono middleware (JSON validation, body limits) rejects bad requests by
|
||||
// throwing HTTPException. Report those as client errors in the API's error
|
||||
// shape instead of letting them surface as an internal server error.
|
||||
export const httpExceptionError = (c: Context, error: unknown) => {
|
||||
if (!(error instanceof HTTPException) || error.status < 400 || error.status >= 500) return null;
|
||||
if (error.status === 413) {
|
||||
return apiError(c, "payload_too_large", "The request body is too large.", 413);
|
||||
}
|
||||
return apiError(c, "bad_request", error.message || "The request could not be read.", error.status);
|
||||
};
|
||||
@@ -58,6 +58,7 @@ import {
|
||||
apiError,
|
||||
authNotConfigured,
|
||||
databaseNotReady,
|
||||
httpExceptionError,
|
||||
unauthorized,
|
||||
} from "./http-errors";
|
||||
import { audit } from "./audit";
|
||||
@@ -478,6 +479,9 @@ app.onError((error, c) => {
|
||||
return apiError(c, error.code, error.message, error.status);
|
||||
}
|
||||
|
||||
const requestError = httpExceptionError(c, error);
|
||||
if (requestError) return requestError;
|
||||
|
||||
if (isDatabaseNotReadyError(error)) {
|
||||
console.error("EdgeEver database readiness check failed", error);
|
||||
return databaseNotReady(c);
|
||||
|
||||
@@ -8,7 +8,7 @@ import {
|
||||
getTableSummary,
|
||||
type MemoSummary,
|
||||
} from "@edgeever/shared";
|
||||
import { parseJsonArray } from "./entity-utils";
|
||||
import { clampNumber, parseJsonArray } from "./entity-utils";
|
||||
import type { DatabaseAdapter } from "./storage-contract";
|
||||
|
||||
export type MemoSummaryRow = {
|
||||
@@ -95,10 +95,6 @@ const normalizeMemoListSort = (value: string | undefined): MemoListSortMode =>
|
||||
const normalizeMemoListFilter = (value: string | undefined): MemoListFilterMode =>
|
||||
value === "tagged" || value === "untagged" || value === "pinned" ? value : "all";
|
||||
|
||||
const clampNumber = (value: number, min: number, max: number) => {
|
||||
if (Number.isNaN(value)) return min;
|
||||
return Math.min(Math.max(value, min), max);
|
||||
};
|
||||
|
||||
const encodeMemoListCursor = (
|
||||
memo: MemoSummaryRow,
|
||||
|
||||
@@ -22,7 +22,7 @@ import {
|
||||
import { auditStatement } from "./audit";
|
||||
import type { AppContext, AuditActor, AuthContext, Bindings } from "./api-context";
|
||||
import { AppError } from "./app-error";
|
||||
import { createId, isoNow, parseJsonArray } from "./entity-utils";
|
||||
import { clampNumber, createId, isoNow, parseJsonArray } from "./entity-utils";
|
||||
import { workspaceInboxId } from "./notebook-service";
|
||||
import { sha256 } from "./hash-utils";
|
||||
import { getRequiredString } from "./mcp-json-rpc";
|
||||
@@ -53,8 +53,6 @@ export type MemoMutationCommit = {
|
||||
after: (memoId: string) => PreparedStatementAdapter[];
|
||||
};
|
||||
|
||||
const clampNumber = (value: number, min: number, max: number) =>
|
||||
Number.isNaN(value) ? min : Math.min(Math.max(value, min), max);
|
||||
|
||||
export type MemoDetailRow = MemoSummaryRow & {
|
||||
content_json: string;
|
||||
|
||||
Reference in new issue
Block a user