Close the printers.metadata hole: not writable, not served
metadata was the way around the decision to hold no printer credentials. It is a free-form jsonb column that was both writable through POST /printers and listed in the public projection, so a client could stash an access code in it and the API would store it and serve it back. Nothing reads it: the frontend, the proxy and the dispatcher never touch printers.metadata (dispatch's metadata is print_jobs', a different column). It is removed from PRINTER_WRITABLE and from printerProjection. The column stays, so reopening it later is one line once something needs it, ideally with a shape to match rather than free-form JSON. The test that merely recorded the hole is replaced by one that asserts it is closed at both ends, because a field nobody can read or one nobody can write is still half of it: not in the projection, dropped from a create body by Value.Clean, and absent from PRINTER_WRITABLE. bun test 128 pass / 0 fail; e2e-authz.sh PASS with 68 checks.
This commit is contained in:
@@ -83,7 +83,6 @@ export const printerProjection = projection(printers, [
|
|||||||
"cameraStream",
|
"cameraStream",
|
||||||
"vendorState",
|
"vendorState",
|
||||||
"vendorErrorCode",
|
"vendorErrorCode",
|
||||||
"metadata",
|
|
||||||
"lastSeenAt",
|
"lastSeenAt",
|
||||||
"createdAt",
|
"createdAt",
|
||||||
"updatedAt",
|
"updatedAt",
|
||||||
|
|||||||
@@ -28,7 +28,14 @@ export const PrinterInsertSchema = createInsertSchema(printers, {
|
|||||||
* The columns a client may set on a printer, derived from the insert schema
|
* The columns a client may set on a printer, derived from the insert schema
|
||||||
* above so a renamed column still fails to compile.
|
* above so a renamed column still fails to compile.
|
||||||
*
|
*
|
||||||
* Everything left out is owned by the gateway, not by the API: `status`, the
|
* `metadata` is deliberately not here, and nor is it in the projection. It is
|
||||||
|
* a free-form jsonb column that nothing reads, so leaving it open meant a
|
||||||
|
* well-meaning client could store a printer's access code in it and the API
|
||||||
|
* would serve it back - the same credential that was removed from the schema
|
||||||
|
* on purpose. The column stays, so reopening it is a one-line change once
|
||||||
|
* something actually needs it, with a shape to match.
|
||||||
|
*
|
||||||
|
* Everything else left out is owned by the gateway, not by the API: `status`, the
|
||||||
* temperatures, layer counts, progress, `vendorState`, `lastSeenAt` and the
|
* temperatures, layer counts, progress, `vendorState`, `lastSeenAt` and the
|
||||||
* rest are written only by `persistTelemetry` in routes/ws/edge.ts. A client
|
* rest are written only by `persistTelemetry` in routes/ws/edge.ts. A client
|
||||||
* that could POST `status: "idle"` could make the scheduler send work to a
|
* that could POST `status: "idle"` could make the scheduler send work to a
|
||||||
@@ -47,7 +54,6 @@ export const PRINTER_WRITABLE = [
|
|||||||
"lanAddress",
|
"lanAddress",
|
||||||
"serialNumber",
|
"serialNumber",
|
||||||
"gatewayPrinterId",
|
"gatewayPrinterId",
|
||||||
"metadata",
|
|
||||||
] as const;
|
] as const;
|
||||||
|
|
||||||
export const PrinterCreateSchema = t.Pick(PrinterInsertSchema, PRINTER_WRITABLE);
|
export const PrinterCreateSchema = t.Pick(PrinterInsertSchema, PRINTER_WRITABLE);
|
||||||
|
|||||||
+20
-10
@@ -6,7 +6,7 @@ import { startDevAuthIssuer } from "../scripts/dev-auth";
|
|||||||
import { Value } from "@sinclair/typebox/value";
|
import { Value } from "@sinclair/typebox/value";
|
||||||
import { printers } from "../src/db/schema";
|
import { printers } from "../src/db/schema";
|
||||||
import { farmProjection, printerProjection } from "../src/db/projections";
|
import { farmProjection, printerProjection } from "../src/db/projections";
|
||||||
import { PrinterCreateSchema } from "../src/db/validators";
|
import { PRINTER_WRITABLE, PrinterCreateSchema } from "../src/db/validators";
|
||||||
|
|
||||||
const USER = "11111111-1111-1111-1111-111111111111";
|
const USER = "11111111-1111-1111-1111-111111111111";
|
||||||
|
|
||||||
@@ -297,14 +297,24 @@ describe("credentials the backend must not hold", () => {
|
|||||||
expect(Object.keys(farmProjection.columns)).not.toContain("edgeProxyToken");
|
expect(Object.keys(farmProjection.columns)).not.toContain("edgeProxyToken");
|
||||||
});
|
});
|
||||||
|
|
||||||
test("metadata is the remaining way a secret could reach the cloud", () => {
|
test("printer metadata is neither writable nor served, so it cannot carry a secret", () => {
|
||||||
// Not a bug to fix here, but a hole to know about: `metadata` is a
|
// `metadata` was the way around the access-code decision: a free-form
|
||||||
// free-form jsonb column that is both writable and in the public
|
// jsonb column that was both writable and in the projection, so a client
|
||||||
// projection, so a well-meaning client could stash an access code in it
|
// could stash a credential in it and read it back. It is now closed at
|
||||||
// and the API would store it and serve it back. Removing the dedicated
|
// both ends. Pinned as three assertions because either end reopening it
|
||||||
// column narrowed the door; it did not close it. If this assertion ever
|
// alone (a writable field nobody can read, or a readable one nobody can
|
||||||
// fails because `metadata` left the projection, that is an improvement -
|
// write) is still half of the hole.
|
||||||
// delete the test.
|
expect(Object.keys(printerProjection.columns)).not.toContain("metadata");
|
||||||
expect(Object.keys(printerProjection.columns)).toContain("metadata");
|
|
||||||
|
const cleaned = Value.Clean(PrinterCreateSchema, {
|
||||||
|
farmId: "3f2b1c5e-0000-4000-8000-000000000000",
|
||||||
|
name: "P1P",
|
||||||
|
vendor: "bambu" as const,
|
||||||
|
metadata: { accessCode: "SECRET123" },
|
||||||
|
}) as Record<string, unknown>;
|
||||||
|
expect(cleaned).not.toHaveProperty("metadata");
|
||||||
|
expect(cleaned.name).toBe("P1P");
|
||||||
|
|
||||||
|
expect(PRINTER_WRITABLE as readonly string[]).not.toContain("metadata");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user