mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-13 22:32:04 +00:00
Refuse what these four routes used to absorb in silence
Four surfaces answered a bad request by quietly substituting a good one. Each fix comes with the test that catches it, and each test was confirmed red against the old behaviour before the fix went back in. `PATCH /api/user/ai-provider` inspected its body key by key with `typeof` guards, so a numeric `model` or a boolean `baseUrl` failed every guard, produced no entry in the update, and the write went ahead without it — a 200 that says "saved" for a field that was discarded. A body of nothing but such keys came back as "No valid fields" naming none of them. The real schema now lives in `src/lib/validations/ai-provider.ts`, the route runs one `safeParse`, and a mismatch is the multi-issue 422 every sibling answers with. The accepted field set is unchanged, and the object is deliberately not strict: an unknown key has always been ignored here and still is. The one thing the schema has to carry that no other does is that this body holds plaintext credentials, so no rule may interpolate the value it rejected — noted where the next `.refine()` would go. `?slice=` on `/api/analytics` was compared against one literal and fell through to the DEFAULT body on anything else, so `?slice=summary` answered 200 after running the heaviest chain on the surface. Invisible and expensive at once. Closed enum, 422 on anything outside it. `?metricType=` and `?limit=` on `/api/personal-records` were parsed defensively, which read as robustness and behaved as silence — and in one direction it was worse than silence: dropping an unrecognised `metricType` WIDENS the read to every metric, so a typo returned more of the record than was asked for. Both refuse now, and the 500 ceiling refuses rather than clamps, because serving a different number of rows than were requested is the same quiet substitution. No caller in the tree sends a value the new rules reject. `POST /api/ingest/medication` bucketed on the client address, so a household running a chat bridge, a button and a home-automation rule — the normal deployment for this route — put all three in one 60/min cell and let the chattiest starve the rest. It already resolves a per-caller identity two lines further down and threw it away; the bucket is that token now. A request that resolves no token keeps the per-IP bucket it always had, through the shared anonymous helper, so a flood is still capped and a broken proxy chain collapses into the tight global cell rather than a shared "unknown" one. Its 429 goes through `apiError` and carries `meta` like every other refusal. Two older tests pinned the defects by name — "drops an unknown metricType silently" and "falls back to default on garbage ?limit" — and one pinned a throw where the rest of the surface returns an envelope. All three are rewritten with the reason beside them. The generated YAML is deliberately not in this commit.
This commit is contained in:
1 parent
49da15feaa
commit
bb9bf0ba68
15 files changed
+682
-255
No files matched your search
@@ -178,10 +178,22 @@ describe("PATCH /api/user/ai-provider — v1.22 fields", () => {
|
||||
);
|
||||
});
|
||||
|
||||
// Was `.rejects.toThrow(/between 10 and 600/)`. The route threw an
|
||||
// HttpError for this one field while every other bad value was skipped in
|
||||
// silence; it runs one Zod parse now, so the refusal is the standard
|
||||
// multi-issue 422 that every sibling route answers with.
|
||||
it("rejects an out-of-range response timeout", async () => {
|
||||
await expect(
|
||||
patch(patchRequest({ responseTimeoutSeconds: 5 })),
|
||||
).rejects.toThrow(/between 10 and 600/);
|
||||
const res = await patch(patchRequest({ responseTimeoutSeconds: 5 }));
|
||||
expect(res.status).toBe(422);
|
||||
const body = (await res.json()) as {
|
||||
meta?: { errorCode?: string };
|
||||
details?: { issues?: Array<{ path: string; message?: string }> };
|
||||
};
|
||||
expect(body.meta?.errorCode).toBe("ai_provider.invalid");
|
||||
const issue = body.details?.issues?.find(
|
||||
(i) => i.path === "responseTimeoutSeconds",
|
||||
);
|
||||
expect(issue?.message).toMatch(/between 10 and 600/);
|
||||
expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -268,6 +280,98 @@ describe("PATCH /api/user/ai-provider — the gateway's own columns (#470)", ()
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* A wrongly-typed field must be refused, not skipped.
|
||||
*
|
||||
* The route inspected the body key by key with `typeof` guards. A value of
|
||||
* the wrong type failed every guard and simply produced no entry in the
|
||||
* update object, so the write went ahead WITHOUT it and answered 200 — the
|
||||
* caller was told the save succeeded while the field they set was discarded.
|
||||
* A body of nothing but such keys fell through to "No valid fields", naming
|
||||
* none of them, which is the same silence with a worse message.
|
||||
*/
|
||||
describe("PATCH /api/user/ai-provider — wrongly-typed fields are named", () => {
|
||||
const patch = PATCH as (req: Request) => Promise<Response>;
|
||||
|
||||
it.each([
|
||||
["model", 42],
|
||||
["baseUrl", true],
|
||||
["compatModel", { nested: "object" }],
|
||||
["anthropicKey", 12345],
|
||||
["openaiKey", ["array"]],
|
||||
["responseTimeoutSeconds", "120"],
|
||||
])("refuses a %s of the wrong type and names its path", async (key, bad) => {
|
||||
const res = await patch(patchRequest({ [key]: bad }));
|
||||
expect(res.status).toBe(422);
|
||||
const body = (await res.json()) as {
|
||||
meta?: { errorCode?: string };
|
||||
details?: { issues?: Array<{ path: string }> };
|
||||
};
|
||||
expect(body.meta?.errorCode).toBe("ai_provider.invalid");
|
||||
expect(body.details?.issues?.map((i) => i.path)).toContain(key);
|
||||
expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("refuses the whole write when one field of several is wrongly typed", async () => {
|
||||
vi.mocked(isPublicUrl).mockReturnValue(true);
|
||||
const res = await patch(
|
||||
patchRequest({ provider: "OPENAI", model: 42, openaiKey: "sk-real" }),
|
||||
);
|
||||
expect(res.status).toBe(422);
|
||||
// Partial application would have persisted the provider and dropped the
|
||||
// model, leaving the account in a state the caller never asked for.
|
||||
expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("refuses an unrecognised provider and names its path", async () => {
|
||||
const res = await patch(patchRequest({ provider: "GEMINI" }));
|
||||
expect(res.status).toBe(422);
|
||||
const body = (await res.json()) as {
|
||||
details?: { issues?: Array<{ path: string }> };
|
||||
};
|
||||
expect(body.details?.issues?.map((i) => i.path)).toContain("provider");
|
||||
});
|
||||
|
||||
it("never echoes a rejected credential back in the error body", async () => {
|
||||
const res = await patch(
|
||||
patchRequest({ anthropicKey: 12345, openaiKey: "sk-super-secret" }),
|
||||
);
|
||||
expect(res.status).toBe(422);
|
||||
expect(JSON.stringify(await res.json())).not.toContain("sk-super-secret");
|
||||
});
|
||||
|
||||
it("still answers 'No valid fields' for a body of nothing it knows", async () => {
|
||||
const res = await patch(patchRequest({ somethingElse: "value" }));
|
||||
expect(res.status).toBe(422);
|
||||
const body = (await res.json()) as {
|
||||
error: string;
|
||||
meta?: { errorCode?: string };
|
||||
};
|
||||
expect(body.error).toBe("No valid fields");
|
||||
expect(body.meta?.errorCode).toBe("ai_provider.no_fields");
|
||||
expect(vi.mocked(prisma.user.update)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps accepting the three-state contract on a known field", async () => {
|
||||
vi.mocked(isPublicUrl).mockReturnValue(true);
|
||||
await patch(patchRequest({ model: " gpt-4o ", anthropicKey: "" }));
|
||||
expect(vi.mocked(prisma.user.update)).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
data: expect.objectContaining({
|
||||
aiModel: "gpt-4o",
|
||||
aiAnthropicKeyEncrypted: null,
|
||||
}),
|
||||
}),
|
||||
);
|
||||
// An omitted key is untouched, not cleared.
|
||||
const data = vi.mocked(prisma.user.update).mock.calls[0][0].data as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
expect(data).not.toHaveProperty("aiOpenaiKeyEncrypted");
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /api/user/ai-provider — the gateway's fields (#470)", () => {
|
||||
it("reports the base URL and model, and the key as presence only", async () => {
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
|
||||
@@ -1,25 +1,21 @@
|
||||
import { NextRequest } from "next/server";
|
||||
import { apiHandler, requireAuth, HttpError } from "@/lib/api-handler";
|
||||
import { apiHandler, requireAuth } from "@/lib/api-handler";
|
||||
import { prisma } from "@/lib/db";
|
||||
import { apiSuccess, apiError, safeJson } from "@/lib/api-response";
|
||||
import {
|
||||
apiSuccess,
|
||||
apiError,
|
||||
returnAllZodIssues,
|
||||
safeJson,
|
||||
} from "@/lib/api-response";
|
||||
import { isPublicUrl } from "@/lib/validations/notifications";
|
||||
import { isLocalAiHostAllowed } from "@/lib/ai/local-host-allowlist";
|
||||
import { encrypt, decrypt } from "@/lib/crypto";
|
||||
import { resolveProviderAvailability } from "@/lib/ai/provider";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { aiProviderPatchSchema } from "@/lib/validations/ai-provider";
|
||||
|
||||
export const dynamic = "force-dynamic";
|
||||
|
||||
const ALLOWED = new Set([
|
||||
"OPENAI",
|
||||
"ANTHROPIC",
|
||||
"LOCAL",
|
||||
// v1.33.1 (#470) — the OpenAI-compatible gateway (LiteLLM / OpenRouter /
|
||||
// vLLM). Its base URL, key and model live in their own three columns.
|
||||
"OPENAI_COMPATIBLE",
|
||||
"CHATGPT_OAUTH",
|
||||
]);
|
||||
|
||||
export const GET = apiHandler(async () => {
|
||||
const { user } = await requireAuth();
|
||||
annotate({ action: { name: "user.ai-provider.get" } });
|
||||
@@ -78,39 +74,47 @@ export const PATCH = apiHandler(async (request: NextRequest) => {
|
||||
const { user } = await requireAuth();
|
||||
annotate({ action: { name: "user.ai-provider.update" } });
|
||||
|
||||
const { data: body, error } = await safeJson<Record<string, unknown>>(
|
||||
request,
|
||||
{ maxBytes: 64 * 1024 },
|
||||
);
|
||||
const { data: rawBody, error } = await safeJson<unknown>(request, {
|
||||
maxBytes: 64 * 1024,
|
||||
});
|
||||
if (error) return error;
|
||||
|
||||
// Every accepted field states its type in the schema, so a wrongly-typed
|
||||
// value is a named issue rather than a key quietly dropped on the floor.
|
||||
// The multi-issue envelope is safe to return even though this body carries
|
||||
// plaintext credentials: Zod's type and range messages name the received
|
||||
// TYPE, never the received value. See the note in the validations module
|
||||
// before adding a `.refine()` here.
|
||||
const parsed = aiProviderPatchSchema.safeParse(rawBody);
|
||||
if (!parsed.success) {
|
||||
annotate({
|
||||
action: { name: "user.ai-provider.update" },
|
||||
meta: {
|
||||
outcome: "validation_failed",
|
||||
issue_count: parsed.error.issues.length,
|
||||
},
|
||||
});
|
||||
return returnAllZodIssues(parsed.error, 422, {
|
||||
errorCode: "ai_provider.invalid",
|
||||
});
|
||||
}
|
||||
const body = parsed.data;
|
||||
|
||||
const updates: Record<string, unknown> = {};
|
||||
|
||||
if (body.provider !== undefined) {
|
||||
if (body.provider === null || body.provider === "") {
|
||||
updates.aiProvider = null;
|
||||
} else if (
|
||||
typeof body.provider === "string" &&
|
||||
ALLOWED.has(body.provider)
|
||||
) {
|
||||
updates.aiProvider = body.provider;
|
||||
} else {
|
||||
throw new HttpError(422, "Invalid provider");
|
||||
}
|
||||
updates.aiProvider =
|
||||
body.provider === null || body.provider === "" ? null : body.provider;
|
||||
}
|
||||
|
||||
if (body.model !== undefined) {
|
||||
if (body.model === null || body.model === "") {
|
||||
updates.aiModel = null;
|
||||
} else if (typeof body.model === "string") {
|
||||
updates.aiModel = body.model.trim() || null;
|
||||
}
|
||||
updates.aiModel = body.model === null ? null : body.model.trim() || null;
|
||||
}
|
||||
|
||||
if (body.baseUrl !== undefined) {
|
||||
if (body.baseUrl === null || body.baseUrl === "") {
|
||||
updates.aiBaseUrl = null;
|
||||
} else if (typeof body.baseUrl === "string") {
|
||||
} else {
|
||||
const trimmed = body.baseUrl.trim();
|
||||
// SSRF guard: by default reject private/internal hostnames so a
|
||||
// compromised user account cannot point the server at the cloud
|
||||
@@ -137,7 +141,7 @@ export const PATCH = apiHandler(async (request: NextRequest) => {
|
||||
if (body.compatBaseUrl !== undefined) {
|
||||
if (body.compatBaseUrl === null || body.compatBaseUrl === "") {
|
||||
updates.aiCompatBaseUrl = null;
|
||||
} else if (typeof body.compatBaseUrl === "string") {
|
||||
} else {
|
||||
const trimmed = body.compatBaseUrl.trim();
|
||||
// Same SSRF floor as the Local provider: a public host always, a
|
||||
// private one only when the operator allowlisted it. Gateways on a LAN
|
||||
@@ -154,72 +158,49 @@ export const PATCH = apiHandler(async (request: NextRequest) => {
|
||||
}
|
||||
|
||||
if (body.compatModel !== undefined) {
|
||||
if (body.compatModel === null || body.compatModel === "") {
|
||||
updates.aiCompatModel = null;
|
||||
} else if (typeof body.compatModel === "string") {
|
||||
updates.aiCompatModel = body.compatModel.trim() || null;
|
||||
}
|
||||
updates.aiCompatModel =
|
||||
body.compatModel === null ? null : body.compatModel.trim() || null;
|
||||
}
|
||||
|
||||
if (body.compatKey !== undefined) {
|
||||
if (body.compatKey === null || body.compatKey === "") {
|
||||
updates.aiCompatKeyEncrypted = null;
|
||||
} else if (typeof body.compatKey === "string") {
|
||||
updates.aiCompatKeyEncrypted = encrypt(body.compatKey.trim());
|
||||
}
|
||||
updates.aiCompatKeyEncrypted =
|
||||
body.compatKey === null || body.compatKey === ""
|
||||
? null
|
||||
: encrypt(body.compatKey.trim());
|
||||
}
|
||||
|
||||
if (body.anthropicKey !== undefined) {
|
||||
if (body.anthropicKey === null || body.anthropicKey === "") {
|
||||
updates.aiAnthropicKeyEncrypted = null;
|
||||
} else if (typeof body.anthropicKey === "string") {
|
||||
updates.aiAnthropicKeyEncrypted = encrypt(body.anthropicKey.trim());
|
||||
}
|
||||
updates.aiAnthropicKeyEncrypted =
|
||||
body.anthropicKey === null || body.anthropicKey === ""
|
||||
? null
|
||||
: encrypt(body.anthropicKey.trim());
|
||||
}
|
||||
|
||||
if (body.localKey !== undefined) {
|
||||
if (body.localKey === null || body.localKey === "") {
|
||||
updates.aiLocalKeyEncrypted = null;
|
||||
} else if (typeof body.localKey === "string") {
|
||||
updates.aiLocalKeyEncrypted = encrypt(body.localKey.trim());
|
||||
}
|
||||
updates.aiLocalKeyEncrypted =
|
||||
body.localKey === null || body.localKey === ""
|
||||
? null
|
||||
: encrypt(body.localKey.trim());
|
||||
}
|
||||
|
||||
if (body.openaiKey !== undefined) {
|
||||
if (body.openaiKey === null || body.openaiKey === "") {
|
||||
updates.aiOpenaiKeyEncrypted = null;
|
||||
} else if (typeof body.openaiKey === "string") {
|
||||
updates.aiOpenaiKeyEncrypted = encrypt(body.openaiKey.trim());
|
||||
}
|
||||
updates.aiOpenaiKeyEncrypted =
|
||||
body.openaiKey === null || body.openaiKey === ""
|
||||
? null
|
||||
: encrypt(body.openaiKey.trim());
|
||||
}
|
||||
|
||||
// ── v1.22 (#89) — response timeout (seconds) ──────────────────
|
||||
// The 10–600 bounds are the schema's; an out-of-range value never reaches
|
||||
// here.
|
||||
if (body.responseTimeoutSeconds !== undefined) {
|
||||
if (body.responseTimeoutSeconds === null) {
|
||||
updates.aiResponseTimeoutSeconds = null;
|
||||
} else if (
|
||||
typeof body.responseTimeoutSeconds === "number" &&
|
||||
Number.isInteger(body.responseTimeoutSeconds)
|
||||
) {
|
||||
// Sane bounds: 10 s floor (a stricter value just trips the timeout) and
|
||||
// 600 s ceiling (a generous cap for a slow self-hosted backend).
|
||||
if (
|
||||
body.responseTimeoutSeconds < 10 ||
|
||||
body.responseTimeoutSeconds > 600
|
||||
) {
|
||||
throw new HttpError(
|
||||
422,
|
||||
"Response timeout must be between 10 and 600 seconds",
|
||||
);
|
||||
}
|
||||
updates.aiResponseTimeoutSeconds = body.responseTimeoutSeconds;
|
||||
} else {
|
||||
throw new HttpError(422, "Invalid response timeout");
|
||||
}
|
||||
updates.aiResponseTimeoutSeconds = body.responseTimeoutSeconds;
|
||||
}
|
||||
|
||||
if (Object.keys(updates).length === 0) {
|
||||
throw new HttpError(422, "No valid fields");
|
||||
return apiError("No valid fields", 422, {
|
||||
errorCode: "ai_provider.no_fields",
|
||||
});
|
||||
}
|
||||
|
||||
// When the provider switches away from LOCAL, drop any stored
|
||||
|
||||
Reference in new issue
Block a user