mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 21:10:59 +00:00
fix(mcp): validate the write preview so it matches the commit outcome
The confirm:false preview echoed the would-be record without running the type / range / instant validation, so a preview could show a value the commit then refused. Run the same shared validation in the preview and surface wouldFail with the error + reason the commit would return, so a preview and its commit never disagree. Also note at the REST read-method allowlist that the MCP-audience guard assumes GET/HEAD/OPTIONS are side-effect-free — a future side-effecting GET must revisit that audience binding.
This commit is contained in:
1 parent
b9c164c4bb
commit
9bc3b0b101
3 files changed
+56
-1
No files matched your search
@@ -16,6 +16,11 @@ import { SCOPE_HEALTH_READ, SCOPE_HEALTH_WRITE } from "./mcp/oauth/config";
|
||||
/**
|
||||
* HTTP methods a read-only credential may use on the REST surface. A request
|
||||
* with any other method (POST / PUT / PATCH / DELETE) is a write.
|
||||
*
|
||||
* The MCP-audience guard below assumes these methods are side-effect-free:
|
||||
* an MCP-bound token is admitted on them. A future side-effecting GET (or HEAD)
|
||||
* would silently widen that token's reach over REST — do NOT add one without
|
||||
* revisiting the MCP audience binding here.
|
||||
*/
|
||||
const READ_HTTP_METHODS = new Set(["GET", "HEAD", "OPTIONS"]);
|
||||
|
||||
|
||||
@@ -3,10 +3,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
const logMcpMeasurement = vi.fn();
|
||||
const logMcpMood = vi.fn();
|
||||
const logMcpBloodPressure = vi.fn();
|
||||
const checkMcpMeasurement = vi.fn();
|
||||
const checkMcpBloodPressure = vi.fn();
|
||||
vi.mock("../writes", () => ({
|
||||
logMcpMeasurement: (...a: unknown[]) => logMcpMeasurement(...a),
|
||||
logMcpMood: (...a: unknown[]) => logMcpMood(...a),
|
||||
logMcpBloodPressure: (...a: unknown[]) => logMcpBloodPressure(...a),
|
||||
checkMcpMeasurement: (...a: unknown[]) => checkMcpMeasurement(...a),
|
||||
checkMcpBloodPressure: (...a: unknown[]) => checkMcpBloodPressure(...a),
|
||||
}));
|
||||
|
||||
const checkMcpWriteRateLimit = vi.fn();
|
||||
@@ -41,6 +45,10 @@ beforeEach(() => {
|
||||
remaining: 59,
|
||||
resetAt: Date.now() + 1000,
|
||||
});
|
||||
// Default: the pre-write validation passes, so a preview echoes a clean
|
||||
// would-be record. Individual tests override to assert wouldFail.
|
||||
checkMcpMeasurement.mockReturnValue({ ok: true });
|
||||
checkMcpBloodPressure.mockReturnValue({ ok: true });
|
||||
});
|
||||
|
||||
describe("write-tool surface", () => {
|
||||
@@ -167,6 +175,27 @@ describe("confirm gate — log_measurement", () => {
|
||||
expect(result.written).toBe(false);
|
||||
expect(logMcpMeasurement).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("a preview that would be refused on commit carries wouldFail + reason", async () => {
|
||||
checkMcpMeasurement.mockReturnValue({
|
||||
ok: false,
|
||||
error: "out_of_range",
|
||||
reason: "Timestamp must not be in the future",
|
||||
});
|
||||
const result = (await tool("log_measurement").run(CTX, {
|
||||
type: "WEIGHT",
|
||||
value: 80,
|
||||
measuredAt: "9999-01-01T00:00:00Z",
|
||||
idempotencyKey: "k-1",
|
||||
})) as Record<string, unknown>;
|
||||
// Still a preview — nothing written — but the commit verdict is surfaced.
|
||||
expect(result.requiresConfirmation).toBe(true);
|
||||
expect(result.written).toBe(false);
|
||||
expect(result.wouldFail).toBe(true);
|
||||
expect(result.error).toBe("out_of_range");
|
||||
expect(result.reason).toBe("Timestamp must not be in the future");
|
||||
expect(logMcpMeasurement).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("confirm gate — log_mood", () => {
|
||||
|
||||
@@ -35,7 +35,13 @@ import type { MeasurementType } from "@/generated/prisma/client";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { checkMcpWriteRateLimit } from "@/lib/rate-limit";
|
||||
import type { McpToolDefinition } from "./tools";
|
||||
import { logMcpMeasurement, logMcpMood, logMcpBloodPressure } from "./writes";
|
||||
import {
|
||||
logMcpMeasurement,
|
||||
logMcpMood,
|
||||
logMcpBloodPressure,
|
||||
checkMcpMeasurement,
|
||||
checkMcpBloodPressure,
|
||||
} from "./writes";
|
||||
|
||||
/**
|
||||
* Write-tool annotations (MCP 2025-11-25). NOT read-only and NOT destructive
|
||||
@@ -66,6 +72,10 @@ const writeOutput: z.ZodRawShape = {
|
||||
instruction: z.string().optional(),
|
||||
preview: z.unknown().optional(),
|
||||
record: z.unknown().optional(),
|
||||
// Set on a preview that would be REFUSED on commit (type / range / instant),
|
||||
// with `error` + `reason` carrying the same verdict the commit would return —
|
||||
// so a preview and its commit never disagree.
|
||||
wouldFail: z.boolean().optional(),
|
||||
};
|
||||
|
||||
/** Shared confirm-gate inputs every write tool carries. */
|
||||
@@ -156,6 +166,9 @@ export const MCP_WRITE_TOOLS: McpToolDefinition[] = [
|
||||
}
|
||||
|
||||
if (!confirm) {
|
||||
// Run the SAME validation the commit core runs so the preview cannot
|
||||
// show a value that would then be refused on commit.
|
||||
const check = checkMcpMeasurement(type, value, measuredAt);
|
||||
annotate({
|
||||
action: { name: "mcp.tool.write" },
|
||||
meta: { tool: "log_measurement", status: "preview" },
|
||||
@@ -171,6 +184,9 @@ export const MCP_WRITE_TOOLS: McpToolDefinition[] = [
|
||||
source: "MCP",
|
||||
},
|
||||
instruction: CONFIRM_INSTRUCTION,
|
||||
...(check.ok
|
||||
? {}
|
||||
: { wouldFail: true, error: check.error, reason: check.reason }),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -310,6 +326,8 @@ export const MCP_WRITE_TOOLS: McpToolDefinition[] = [
|
||||
}
|
||||
|
||||
if (!confirm) {
|
||||
// Same validation the commit core runs — preview == commit verdict.
|
||||
const check = checkMcpBloodPressure(systolic, diastolic, measuredAt);
|
||||
annotate({
|
||||
action: { name: "mcp.tool.write" },
|
||||
meta: { tool: "log_blood_pressure", status: "preview" },
|
||||
@@ -325,6 +343,9 @@ export const MCP_WRITE_TOOLS: McpToolDefinition[] = [
|
||||
source: "MCP",
|
||||
},
|
||||
instruction: CONFIRM_INSTRUCTION,
|
||||
...(check.ok
|
||||
? {}
|
||||
: { wouldFail: true, error: check.error, reason: check.reason }),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user