diff --git a/docs/UNOSIM_REFACTORING_OPL.md b/docs/UNOSIM_REFACTORING_OPL.md index abb7ff311..9b6643e34 100644 --- a/docs/UNOSIM_REFACTORING_OPL.md +++ b/docs/UNOSIM_REFACTORING_OPL.md @@ -16,7 +16,7 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. |---|---|---|---|---|---|---|---|---|---|---|---| | R1 | Compile-Pfad absichern: sichere Include-Grenze vor arduino-cli (Env-Allowlist entfällt, siehe S1-ENV) | Security | S1 [Code, mit Sentinel verifiziert] | hoch | schließt den Kanal, über den eine beliebige lesbare Datei vollständig in Diagnosen erscheint | klein | – | fix/compile-include-guard | DONE | RED→GREEN: `include-guard.test.ts` (25), `arduino-compiler-include-guard.test.ts` (2), Toolchain-Sentinel-Test `compile-include-boundary.test.ts` (echte arduino-cli; vorher Sentinel in der Antwort, nachher nicht); Unit 2697 grün | PR-Merge siehe Verlauf | | R2 | `lastCompiledCode`-Fallback und Sketch-CRUD pro Identität absichern (nicht entfernen) | Isolation | S2, S3 [Code] | mittel–hoch | Nutzerisolation ohne Bruch des REST-/WS-Vertrags | klein | – | fix/isolate-legacy-global-state | DONE | RED→GREEN: `simulation-last-compiled-code.test.ts` (fremder Code wird nie ausgeführt, eigener Fallback bleibt), `last-compiled-code-store.test.ts`, `sketches.routes.test.ts` (Seed read-only, fremde Sketches 404); zwei Bestandstests an Identität angepasst (Begründung im PR) | PR-Merge siehe Verlauf | -| R6 | Einheitlicher Compile-Hash inkl. Header; kein Binary in REST-Payload/LRU | Korrektheit/Performance | A4, A5 [Code] | mittel | keine veralteten Cache-Treffer, kleinere Antworten | klein | – | fix/compile-cache-key-and-payload | OPEN | Header-only-Änderung → frischer Compile; Response ohne `binary` | – | +| R6 | Einheitlicher Compile-Hash inkl. Header (Worker-Identität); kein Binary in REST-Payload/LRU | Korrektheit/Performance/Security | A4, A5, S1-INCBIN [Code] | mittel | keine veralteten Cache-Treffer; Worker und direkter Pfad teilen Cache-Einträge; Antwort 59.653 → 562 Byte (Blink-Sketch, Worker-Pfad) | klein | – | fix/compile-cache-key-and-payload | DONE | RED→GREEN: `arduino-compiler-cache-key.test.ts` (Header-Änderung kompiliert neu; gleiche Identität wie der Worker), `compiler-binary-payload.test.ts` (frisch, gecacht, LRU ohne `binary`) | PR-Merge siehe Verlauf | | R7 | Globaler API-Limiter im Gateway-Modus nach `subject` statt IP | Skalierung | P1 [Code] | mittel (topologieabhängig) | keine klassenweiten 429 hinter NAT | klein | – | fix/api-rate-limit-identity | OPEN | Route-Tests beider Trust-Modi | – | | R3a | Lauf-Generation + Abbruch im Runner-Lifecycle | Isolation/Lifecycle | S4 [plausibel] | hoch | keine fremde Ausgabe, keine verwaisten Container | mittel | – | fix/runner-run-generation | OPEN | deterministischer Race-Test A wartet → A stoppt → B übernimmt | – | | R3b | Reset-Ownership in `runner.resetForReuse()` | Kapselung | A6 [Code] | mittel | Reset an einer Stelle | mittel | R3a | refactor/runner-reset-ownership | OPEN | Pool-/Isolationstests | – | @@ -35,7 +35,7 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. |---|---|---|---|---|---|---|---|---|---|---|---| | S1 | Absolutes/traversierendes `#include` lässt arduino-cli eine beliebige lesbare Datei lesen und in stderr ausgeben | Security | [Code] verifiziert: synthetische Sentinel-Datei erschien vollständig in der REST-Compile-Antwort | hoch | – | – | – | R1 | DONE | `compile-include-boundary.test.ts` | durch R1 geschlossen | | S1-ENV | Env-Leak über geerbte Prozessumgebung und `/proc/*/environ` | Security | g++ 12 (Sandbox-Image) und avr-g++ 7.3 (Backend-Image) lesen `/proc/self/environ` und `/proc/1/environ` per `#include` und `.incbin` als leer (Sentinel-Env-Variable nicht sichtbar) | – | – | – | – | – | FALSIFIED | Docker-Experiment mit synthetischer Variable | keine Env-Allowlist umgesetzt | -| S1-INCBIN | `.incbin` im Inline-Assembler bettet reguläre Dateien ins HEX ein; das HEX geht derzeit im REST-JSON an den Client | Security | [Code] verifiziert (Marker-Datei im Objekt) | mittel | – | – | – | R6 | OPEN | Response ohne `binary` | Kanal schließt mit R6 | +| S1-INCBIN | `.incbin` im Inline-Assembler bettet reguläre Dateien ins HEX ein; das HEX ging im REST-JSON an den Client | Security | [Code] verifiziert (Marker-Datei im Objekt) | mittel | – | – | – | R6 | DONE | Response ohne `binary` | HEX verlässt den Server nicht mehr | | S1-ASM | GAS `.include` im Inline-Assembler gibt die ersten ~10 Zeichen je Zeile einer beliebigen Datei als Fehlermeldung aus; per String-Konkatenation nicht robust textuell filterbar | Security | [Code] verifiziert (Marker-Präfix in der Assemblermeldung) | mittel (Teilinhalt; Default-Deployment hält Secrets nur in Env) | – | groß | – | – | BLOCKED_DECISION | – | Robuster Fix = REST-Compile ohne Zugriff auf Backend-Dateien (Sandbox/Namespace); Architekturentscheidung | | S2 | Globaler `lastCompiledCode` | Isolation | [Code] bestätigt | mittel–hoch | – | – | – | R2 | DONE | WS-Test zweier Subjects | Fallback jetzt pro Subject (LRU, 1000 Subjects) | | S3 | Sketch-CRUD ohne Besitzer | Isolation | [Code] bestätigt | mittel | – | – | – | R2 | DONE | Route-Test zweier Identitäten | Schreiben nur auf eigene Sketches, Seed schreibgeschützt | @@ -43,8 +43,8 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. | A1 | Überlappende Concurrency-Mechanismen, vermischte Statusmetriken | Concurrency | [Code] | mittel | – | – | – | R5b | OPEN | – | – | | A2 | Gatekeeper-TTL ohne Queue-Fortsetzung, tote Cache-Locks | Concurrency | [Code] | mittel | – | – | – | R5a | OPEN | – | – | | A3 | Fallback umgeht Lastgrenze, keine Worker-Recovery, unbegrenzte Queue | Concurrency | [Code] | mittel | – | – | – | R5b | OPEN | – | – | -| A4 | Compile-Hash ohne Header im direkten Compiler | Korrektheit | [Code] | mittel | – | – | – | R6 | OPEN | – | – | -| A5 | HEX-Binary in REST-JSON und LRU | Performance | [Code] | gering | – | – | – | R6 | OPEN | – | – | +| A4 | Compile-Hash ohne Header im direkten Compiler | Korrektheit | [Code] bestätigt | mittel | – | – | – | R6 | DONE | Header-only-Test | `libraries` bleibt außerhalb des Hashes: arduino-cli erhält sie nicht | +| A5 | HEX-Binary in REST-JSON und LRU | Performance | [Code] bestätigt, [gemessen] 59.653 statt 562 Byte | gering | – | – | – | R6 | DONE | Payload-Test | Simulation nutzt das REST-Binary nicht | | A6 | Pool setzt private Runner-Felder zurück | Kapselung | [Code] | mittel | – | – | – | R3b | OPEN | – | – | | A7 | Kein Orphan-Sweep, kein Heartbeat, keine Serialisierung, kein Message-Limit | Lifecycle | [Code] | mittel | – | – | – | R4a/R4b | OPEN | – | – | | P1 | Globaler API-Limiter pro IP (Campus-NAT) | Skalierung | [Code] | mittel | – | – | – | R7 | OPEN | – | – | @@ -79,4 +79,5 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet. |---|---|---|---| | #159 | Audit-Bericht und diese OPL | `0863ba1f` | PR-CI 5/5 grün; Post-Merge-CI siehe nächster Eintrag | | #160 | R1: Include-Grenze für den REST-Compiler | `1c2995d9` | PR-CI 5/5 grün; Post-Merge-CI von #159 grün | -| R2 | Per-Identity-Isolation von Code-Fallback und Sketch-CRUD | – | – | +| #161 | R2: Per-Identity-Isolation von Code-Fallback und Sketch-CRUD | `18c7d89b` | PR-CI 5/5 grün; Post-Merge-CI von #160 grün | +| R6 | Einheitlicher Compile-Hash, kein Binary in der REST-Antwort | – | – | diff --git a/server/routes/compiler.routes.ts b/server/routes/compiler.routes.ts index d849ef883..3663f292f 100644 --- a/server/routes/compiler.routes.ts +++ b/server/routes/compiler.routes.ts @@ -55,6 +55,12 @@ function parseCompileRequest(body: unknown): ParsedCompileRequest { return { success: true, data: parsedRequest.data }; } +/** The HEX stays on the server: the client never uses it and it inflates every response. */ +function withoutBinary(result: CompilationResult): CompilationResult { + const { binary: _binary, ...clientResult } = result; + return clientResult; +} + function isTimedOutCompileResult(result: CompilationResult): boolean { return !result.success && `${result.stderr ?? ""} ${result.errors.map((err) => err.message).join(" ")}`.toLowerCase().includes("timeout"); } @@ -183,15 +189,16 @@ export function registerCompilerRoutes(app: Express, deps: CompilerDeps) { recordCompileMetricIfNeeded(compiler, compileStartTime, result); + const clientResult = withoutBinary(result); if (result.success) { if (!cacheDisabled) { - compilationCache.set(codeHash, { result, timestamp: Date.now() }); + compilationCache.set(codeHash, { result: clientResult, timestamp: Date.now() }); logger.info(`✅ Cached compilation result for code`); } rememberCompiledCode(res, code); } - res.json(result); + res.json(clientResult); } catch (error) { recordCompileErrorIfNeeded(compiler, compileStartTime, error); logger.error(`[Compiler Route] Error during /api/compile: ${error instanceof Error ? error.message : String(error)}`); diff --git a/server/services/arduino-compiler.ts b/server/services/arduino-compiler.ts index 08d230037..c6e8ac7e8 100644 --- a/server/services/arduino-compiler.ts +++ b/server/services/arduino-compiler.ts @@ -2,7 +2,7 @@ import { mkdtemp, mkdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; -import { randomUUID, createHash } from "node:crypto"; +import { randomUUID } from "node:crypto"; import { Logger } from "@shared/logger"; import { ParserMessage, IOPinRecord } from "@shared/schema"; import { CodeParser } from "@shared/code-parser"; @@ -29,6 +29,7 @@ import { import { processHeaderIncludes } from "./compiler/header-processor"; import { findUnsafeIncludes } from "./compiler/include-guard"; import { compileWithArduinoCli, type CLICompileConfig } from "./compiler/cli-runner"; +import { buildSketchHash } from "./workers/compile-worker-utils"; // Re-export for backwards compatibility export type { CompilationError } from "./compiler/compiler-output-parser"; @@ -91,20 +92,14 @@ export class ArduinoCompiler { return instance; } + /** Same identity as the compile worker, so both paths share cache entries and headers count. */ private buildSketchHash( code: string, + headers: Array<{ name: string; content: string }> | undefined, options?: CompileRequestOptions, ): string { - if (options?.sketchHash) { - return options.sketchHash; - } - - const payload = JSON.stringify({ - code, - fqbn: options?.fqbn || this.defaultFqbn, - entryFile: options?.entryFile || "sketch.ino", - }); - return createHash("sha256").update(payload).digest("hex"); + return options?.sketchHash + ?? buildSketchHash({ code, headers, entryFile: options?.entryFile }, options?.fqbn || this.defaultFqbn); } /** @@ -194,7 +189,7 @@ export class ArduinoCompiler { const reservedNameMessages = reservedNamesValidator.validateReservedNames(code); const allParserMessages = [...parserMessages, ...reservedNameMessages]; const ioRegistry: IOPinRecord[] = []; - const sketchHash = this.buildSketchHash(code, options); + const sketchHash = this.buildSketchHash(code, headers, options); const hexCacheDir = options?.hexCacheDir || this.defaultHexCacheDir; const compileStartedAt = process.hrtime.bigint(); diff --git a/tests/server/routes/compiler-binary-payload.test.ts b/tests/server/routes/compiler-binary-payload.test.ts new file mode 100644 index 000000000..edf8a4053 --- /dev/null +++ b/tests/server/routes/compiler-binary-payload.test.ts @@ -0,0 +1,63 @@ +import express from "express"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerCompilerRoutes } from "../../../server/routes/compiler.routes"; + +const BINARY = Buffer.from(":100000000C9434000C9446000C9446000C944600A2\n"); + +describe("compile response payload", () => { + let server: http.Server; + let baseUrl: string; + let compilationCache: Map; + + beforeEach(async () => { + const app = express(); + app.use(express.json()); + app.use((_req, res, next) => { + res.locals.unosimIdentity = { subject: "student-a", roles: ["user"] }; + next(); + }); + compilationCache = new Map(); + registerCompilerRoutes(app, { + compiler: { compile: vi.fn().mockResolvedValue({ success: true, output: "ok", errors: [], arduinoCliStatus: "success", binary: BINARY }) }, + compilationCache: compilationCache as never, + hashCode: (code: string) => `hash:${code}`, + CACHE_TTL: 60_000, + setLastCompiledCode: vi.fn(), + logger: { info: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn() } as never, + }); + server = app.listen(0, "127.0.0.1"); + await new Promise((resolve) => server.once("listening", resolve)); + baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + }); + + afterEach(() => new Promise((resolve) => server.close(() => resolve()))); + + async function compile() { + const response = await fetch(`${baseUrl}/api/compile`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ code: "void setup(){} void loop(){}" }), + }); + return response.json() as Promise>; + } + + it("does not send the compiled binary to the client, fresh or cached", async () => { + const fresh = await compile(); + const cached = await compile(); + + expect(fresh).toMatchObject({ success: true, output: "ok" }); + expect(fresh).not.toHaveProperty("binary"); + expect(cached).toMatchObject({ success: true, cached: true }); + expect(cached).not.toHaveProperty("binary"); + }); + + it("does not keep the binary in the in-memory result cache", async () => { + await compile(); + + expect([...compilationCache.values()]).toEqual([ + expect.objectContaining({ result: expect.not.objectContaining({ binary: expect.anything() }) }), + ]); + }); +}); diff --git a/tests/server/services/arduino-compiler-cache-key.test.ts b/tests/server/services/arduino-compiler-cache-key.test.ts new file mode 100644 index 000000000..a29f8ee1c --- /dev/null +++ b/tests/server/services/arduino-compiler-cache-key.test.ts @@ -0,0 +1,43 @@ +import { randomUUID } from "node:crypto"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ArduinoCompiler } from "../../../server/services/arduino-compiler"; +import * as cliRunner from "../../../server/services/compiler/cli-runner"; +import { buildSketchHash } from "../../../server/services/workers/compile-worker-utils"; + +describe("ArduinoCompiler cache key", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("compiles again when only a header changed", async () => { + const cli = vi.spyOn(cliRunner, "compileWithArduinoCli").mockResolvedValue({ + success: true, + output: "Sketch uses 1 bytes.\n\nBoard: Arduino UNO", + binary: Buffer.from(":00000001FF\n"), + }); + const compiler = await ArduinoCompiler.create(); + // Unique code keeps this test independent of earlier on-disk cache entries. + const code = `#include "pins.h"\n// ${randomUUID()}\nvoid setup(){}\nvoid loop(){}\n`; + + await compiler.compile(code, [{ name: "pins.h", content: "#define PIN 5" }]); + await compiler.compile(code, [{ name: "pins.h", content: "#define PIN 6" }]); + + expect(cli).toHaveBeenCalledTimes(2); + }); + + it("uses the same sketch identity as the compile worker", async () => { + const cli = vi.spyOn(cliRunner, "compileWithArduinoCli").mockResolvedValue({ + success: true, + output: "Board: Arduino UNO", + binary: Buffer.from(":00000001FF\n"), + }); + const compiler = await ArduinoCompiler.create(); + const code = `// ${randomUUID()}\nvoid setup(){}\nvoid loop(){}\n`; + const headers = [{ name: "pins.h", content: "#define PIN 5" }]; + + await compiler.compile(code, headers, undefined, { sketchHash: buildSketchHash({ code, headers }, "arduino:avr:uno") }); + await compiler.compile(code, headers); + + expect(cli).toHaveBeenCalledOnce(); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index f414a4e7e..a055db6ed 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -19,6 +19,7 @@ const serializedHttpUnitTests = [ "tests/server/cli-label-isolation.test.ts", "tests/server/dev-entrypoint.test.ts", "tests/server/routes/compiler.routes.test.ts", + "tests/server/routes/compiler-binary-payload.test.ts", "tests/server/routes/examples.routes.test.ts", "tests/server/routes/routes-core.test.ts", "tests/server/routes/server-status-observability.test.ts",