From 94f9ed45c999a8bdc12b70018223c1539eb010d6 Mon Sep 17 00:00:00 2001 From: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:37:31 +0100 Subject: [PATCH] =?UTF-8?q?fix(security):=20Track=20C=20cleanup=20?= =?UTF-8?q?=E2=80=94=20closes=20idaptik#99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the 23 Critical/High panic-attack findings tracked at idaptik#99. Mix of real refactors, intentional-use classifications, and stale-doc cleanup. ## What changed ### UnsafeDeserialization × 7 → 1 refactor + 3 source classifications The one real fix: - `src/app/screens/BalanceAnalyserModel.res:217` — was raw `JSON.parseExn(jsonStr)->JSON.Classify.classify`; now wraps in `SafeJson.parse` (`Result`-returning) with an `Error(_) => empty` arm. Malformed report JSON now degrades to the empty record instead of throwing into the host (escape-hatch / PanLL panel). The three intentional uses, classified in `audits/assail-classifications.a2ml`: - `src/app/proven/SafeJson.res:32` — IS the canonical safe-parse wrapper (try/catch around `parseExn`; returns `Result`). The finding here is what the fix above consumes. - `vm/lib/ocaml/benchmark.res:139` — benchmark-fixture measuring raw parser throughput against a hardcoded valid JSON string. SafeJson would measure try/catch overhead, wrong instrument. - `lib/ocaml/SafeJson.res` — build-mirror of the source-side SafeJson. The remaining lib/ mirrors of BalanceAnalyserModel.res auto-regenerate on the next ReScript compile (the BS- and lib-mirror copies pick up the new `SafeJson.parse` shape). ### DynamicCodeExecution × 2 → 2 classifications Vite-emitted bundles in `main-game/dist/assets/`: - `index-Cdt-JTFK.js` (SPA bootstrap) - `webworkerAll-DNs-UuZS.js` (web-worker bundle) Both classified as `compiled-output`. DOM manipulation / dynamic-script patterns are inherent to SPA + web-worker initialisation, not user-controlled execution. Filenames are content-hashed → entries will need refreshing when the bundles rotate. Tracked as a follow-up. ### ExcessivePermissions × 1 → tightened invocation + doc cleanup - `Justfile`: `just run` and `just run-full` recipes now use the scoped permission set instead of `--allow-all`: `deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js` - `run.js`: header comment + `--help` output rewritten to document the scoped invocation as canonical; `--allow-all` is now explicitly NOT recommended in the header. - The residual `--allow-all` mention in the discouragement text is classified as `documentation-mention-only` in `audits/assail-classifications.a2ml` so the static analyzer doesn't flag the warning text itself. Per-API audit captured inline in the run.js header: - `--allow-read=.`: Deno.readTextFile + Deno.stat on tree-relative paths - `--allow-env`: Deno.env.get + Deno.env.toObject for child-env passthru (narrowing requires rewriting the passthrough to a whitelist — tracked) - `--allow-run=...`: spawns deno/git/which + platform browser opener - `--allow-net=127.0.0.1`: Deno.listen({port}) probing only (loopback) ### HardcodedSecret × 12 → no change All 12 are already classified `game-content-fixture` in `assail-classifications.a2ml` from PR #112's S-expr→TOML conversion. Re-verified during this PR; no additions needed. ## Out of scope (follow-ups noted) - Refresh `main-game/dist/assets/` exact-hash classifications on each release rebuild, OR gitignore `main-game/dist/` and let CI regenerate. - Narrow `--allow-env` in run.js by rewriting `Deno.env.toObject()` child-env passthrough to a whitelist (currently passes the whole env). ## Closes closes hyperpolymath/idaptik#99 Signed-off-by: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com> --- Justfile | 17 ++++-- audits/assail-classifications.a2ml | 70 ++++++++++++++++++++++++ deno.lock | 34 +++++++++++- run.js | 34 ++++++++++-- src/app/screens/BalanceAnalyserModel.res | 14 ++++- 5 files changed, 156 insertions(+), 13 deletions(-) diff --git a/Justfile b/Justfile index bc26393e..8985ffd2 100644 --- a/Justfile +++ b/Justfile @@ -8,13 +8,22 @@ import? "contractile.just" default: @just --list --unsorted -# Launch the game (platform-detect, self-heal, git cycle) +# Launch the game (platform-detect, self-heal, git cycle). +# Permission set is intentionally scoped (no --allow-all) — see run.js header +# for the rationale + per-API audit. --allow-env is broad because run.js calls +# Deno.env.toObject() to propagate the user's environment into spawned child +# processes (deno task dev, deno task dev:all); narrowing this requires +# rewriting the child-env passthrough to whitelist only the vars the children +# actually consume. run: - deno run --allow-all run.js + deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js -# Launch with Elixir sync server (multiplayer/co-op) +# Launch with Elixir sync server (multiplayer/co-op). +# Same permission set as `just run` — the --full flag changes what child +# command run.js spawns (deno task dev:all instead of deno task dev), not the +# parent permission surface. run-full: - deno run --allow-all run.js --full + deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js --full # Start full development environment dev: diff --git a/audits/assail-classifications.a2ml b/audits/assail-classifications.a2ml index 06e818cf..1317acc7 100644 --- a/audits/assail-classifications.a2ml +++ b/audits/assail-classifications.a2ml @@ -95,3 +95,73 @@ category = "HardcodedSecret" classification = "game-content-fixture" audit = "audits/audit-game-content-fixture-2026-05-26.md" rationale = "Fictional in-game passwords / credentials for the hacker-themed gameplay; not real secrets. GlobalNetworkData.res carries the explicit SECURITY NOTE." + +# ─────────────────────────────────────────────────────────────────────────── +# UnsafeDeserialization — Track C #99 cleanup 2026-06-01 +# ─────────────────────────────────────────────────────────────────────────── +# Three remaining JSON.parseExn call sites after the BalanceAnalyserModel +# refactor (src/app/screens/BalanceAnalyserModel.res:217 → SafeJson.parse). +# Two are intentional infrastructure; one is a benchmark fixture. + +[[classification]] +file = "src/app/proven/SafeJson.res" +category = "UnsafeDeserialization" +classification = "intentional-safe-wrapper" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "Canonical safe-parse wrapper for the estate. JSON.parseExn at line 32 is wrapped in try/catch and returns Result — never throws. This file IS the fix that other call sites consume. ProvenSafeJson is the dependently-typed Proven equivalent (formal proof of totality)." + +[[classification]] +file = "lib/ocaml/SafeJson.res" +category = "UnsafeDeserialization" +classification = "intentional-safe-wrapper" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "Build mirror of src/app/proven/SafeJson.res — see source-side rationale. Auto-regenerated by ReScript compile; do not edit by hand." + +[[classification]] +file = "vm/lib/ocaml/benchmark.res" +category = "UnsafeDeserialization" +classification = "benchmark-fixture" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "VM-serialisation benchmark. JSON.parseExn at line 139 is intentionally measured against a hardcoded valid JSON string ({\"playerX\":100,...}) inside benchmark(\"Game state JSON deserialize\", 100000, ...). Using SafeJson.parse would measure the try/catch overhead instead of the raw parser — wrong instrument for this benchmark." + +# ─────────────────────────────────────────────────────────────────────────── +# DynamicCodeExecution — Track C #99 cleanup 2026-06-01 +# ─────────────────────────────────────────────────────────────────────────── +# Vite-emitted build artifacts in main-game/dist/assets/. DOM manipulation +# (innerHTML / document.write equivalents) is inherent to SPA bootstrap + +# web-worker initialisation. Filenames are content-hashed and rotate every +# build; the suppression below covers the current snapshot. Followup: either +# gitignore main-game/dist/ and regenerate per CI, or refresh these entries +# at each release. Tracked separately. + +[[classification]] +file = "main-game/dist/assets/index-Cdt-JTFK.js" +category = "DynamicCodeExecution" +classification = "compiled-output" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "Vite-generated SPA bootstrap. innerHTML/document.write is standard SPA initialisation pattern, not user-controlled code execution. Content-hashed filename rotates per build; this entry will need refreshing if the bundle changes." + +[[classification]] +file = "main-game/dist/assets/webworkerAll-DNs-UuZS.js" +category = "DynamicCodeExecution" +classification = "compiled-output" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "Vite-generated web-worker bundle. DOM/script-dynamic operations are inherent to web-worker initialisation. Content-hashed filename rotates per build." + +# ─────────────────────────────────────────────────────────────────────────── +# ExcessivePermissions — Track C #99 cleanup 2026-06-01 +# ─────────────────────────────────────────────────────────────────────────── +# run.js previously documented "deno run --allow-all" as its canonical +# invocation. The Justfile recipes (`just run` / `just run-full`) + the +# updated run.js header now carry the scoped permission set +# (--allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start +# --allow-net=127.0.0.1). The header retains a "NOT recommended" mention of +# --allow-all so static analyzers don't dredge up a fresh finding from old +# documentation drift. Suppression below covers that residual text. + +[[classification]] +file = "run.js" +category = "ExcessivePermissions" +classification = "documentation-mention-only" +audit = "audits/audit-game-content-fixture-2026-05-26.md" +rationale = "The 'deno -A' string surviving in run.js after Track C cleanup appears only inside the header comment that explicitly DISCOURAGES its use ('--allow-all is NOT recommended'). The canonical invocation via `just run` carries the scoped permission set. No runtime --allow-all behaviour is invoked." diff --git a/deno.lock b/deno.lock index 78b773bb..c2a77d90 100644 --- a/deno.lock +++ b/deno.lock @@ -11,6 +11,8 @@ "npm:@pixi/sound@^6.0.1": "6.0.1_pixi.js@8.8.1", "npm:@pixi/ui@2.2.2": "2.2.2_pixi.js@8.8.1", "npm:@pixi/ui@^2.2.2": "2.2.2_pixi.js@8.8.1", + "npm:@playwright/test@1.60.0": "1.60.0", + "npm:@playwright/test@^1.60.0": "1.60.0", "npm:@rescript/core@1.6.1": "1.6.1_rescript@12.2.0", "npm:@rescript/core@^1.6.1": "1.6.1_rescript@12.2.0", "npm:@rescript/runtime@12.2.0": "12.2.0", @@ -1071,6 +1073,13 @@ "typed-signals" ] }, + "@playwright/test@1.60.0": { + "integrity": "sha512-O71yZIbAh/PxDMNGns37GHBIfrVkEVyn+AXyIa5dOTfb4/xNvRWV+Vv/NMbNCtODB/pO7vLlF2OTmMVLhmr7Ag==", + "dependencies": [ + "playwright" + ], + "bin": true + }, "@rescript/core@1.6.1_rescript@12.2.0": { "integrity": "sha512-vyb5k90ck+65Fgui+5vCja/mUfzKaK3kOPT4Z6aAJdHLH1eljEi1zKhXroCiCtpNLSWp8k4ulh1bdB5WS0hvqA==", "dependencies": [ @@ -1668,6 +1677,11 @@ "universalify" ] }, + "fsevents@2.3.2": { + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "os": ["darwin"], + "scripts": true + }, "fsevents@2.3.3": { "integrity": "sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw==", "os": ["darwin"], @@ -2180,6 +2194,20 @@ "parse-svg-path" ] }, + "playwright-core@1.60.0": { + "integrity": "sha512-9bW6zvX/m0lEbgTKJ6YppOKx8H3VOPBMOCFh2irXFOT4BbHgrx5hPjwJYLT40Lu+4qtD36qKc/Hn56StUW57IA==", + "bin": true + }, + "playwright@1.60.0": { + "integrity": "sha512-hheHdokM8cdqCb0lcE3s+zT4t4W+vvjpGxsZlDnikarzx8tSzMebh3UiFtgqwFwnTnjYQcsyMF8ei2mCO/tpeA==", + "dependencies": [ + "playwright-core" + ], + "optionalDependencies": [ + "fsevents@2.3.2" + ], + "bin": true + }, "pngjs@6.0.0": { "integrity": "sha512-TRzzuFRRmEoSW/p1KVAmiOgPco2Irlah+bGFCeNfJXxxYGwSw7YwAOAcd7X28K/m5bjBWKsC29KyoMfHbypayg==" }, @@ -2265,7 +2293,7 @@ "@rollup/rollup-win32-ia32-msvc", "@rollup/rollup-win32-x64-gnu", "@rollup/rollup-win32-x64-msvc", - "fsevents" + "fsevents@2.3.3" ], "bin": true }, @@ -2549,7 +2577,7 @@ "rollup" ], "optionalDependencies": [ - "fsevents" + "fsevents@2.3.3" ], "bin": true }, @@ -2659,6 +2687,7 @@ "npm:@esotericsoftware/spine-pixi-v8@4.2.74", "npm:@pixi/sound@6.0.1", "npm:@pixi/ui@2.2.2", + "npm:@playwright/test@1.60.0", "npm:@rescript/core@1.6.1", "npm:@rescript/runtime@12.2.0", "npm:motion@12.4.7", @@ -2672,6 +2701,7 @@ "npm:@esotericsoftware/spine-pixi-v8@^4.2.74", "npm:@pixi/sound@^6.0.1", "npm:@pixi/ui@^2.2.2", + "npm:@playwright/test@^1.60.0", "npm:@rescript/core@^1.6.1", "npm:motion@^12.4.7", "npm:pixi.js@^8.8.1", diff --git a/run.js b/run.js index 9dddfbeb..f2bff1c9 100644 --- a/run.js +++ b/run.js @@ -4,9 +4,34 @@ // run.js — Homoiconic, fault-tolerant, platform-independent run script // for IDApTIK (asymmetric co-op stealth puzzle-platformer) // -// Usage: -// deno run --allow-all run.js # auto-detect and launch -// deno run --allow-all run.js --help # show usage +// Usage (scoped permissions — preferred): +// deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js +// deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js --help +// +// Or via the Justfile recipe (which carries the canonical permission set): +// just run +// just run-full +// +// Permission audit (panic-attack ExcessivePermissions Track C, idaptik#99): +// --allow-read=. : Deno.readTextFile(...) + Deno.stat(...) on +// tree-relative paths (node_modules, lib/bs, dist). +// --allow-env : Deno.env.get(...) for WAYLAND_DISPLAY/DISPLAY + +// Deno.env.toObject() to propagate env into child +// processes. Narrowing this requires rewriting the +// child-env passthrough to a whitelist — tracked +// as follow-up to this PR. +// --allow-run=... : Deno.Command spawns of `deno`, `git`, `which`, +// and the platform's browser opener (`xdg-open` on +// Linux/Wayland/X11, `open` on macOS, `start` on +// Windows). +// --allow-net=127.0.0.1 : Deno.listen({port}) port-probing for the Vite +// dev-server port (1984) and the fallback range +// 1985-1987. Loopback only — no outbound network +// access required. +// +// --allow-all is NOT recommended; see `just run` / `just run-full` for the +// canonical scoped invocation. The previous --allow-all advice in this +// header was the trigger for the Track C ExcessivePermissions finding. // ───────────────────────────────────────────────────────────────────────────── // REGISTRY — the script IS the data; reflect() reads this at runtime @@ -354,7 +379,8 @@ if (import.meta.main) { ${c.bold}${REGISTRY.identity.display} — run.js${c.reset} ${REGISTRY.identity.license} | ${REGISTRY.identity.repo} -Usage: deno run --allow-all run.js [OPTIONS] +Usage: deno run --allow-read=. --allow-env --allow-run=deno,git,which,xdg-open,open,start --allow-net=127.0.0.1 run.js [OPTIONS] + or: just run [OPTIONS] (carries the canonical permission set) Options: --help, -h Show this help diff --git a/src/app/screens/BalanceAnalyserModel.res b/src/app/screens/BalanceAnalyserModel.res index 3a1577f8..0040b3b6 100644 --- a/src/app/screens/BalanceAnalyserModel.res +++ b/src/app/screens/BalanceAnalyserModel.res @@ -213,9 +213,16 @@ let empty: report = { /// Parse a full balance report from a JSON string. /// Returns `empty` if the string is not valid JSON or the structure is wrong. +/// Uses SafeJson.parse (Result-returning) instead of JSON.parseExn so a +/// malformed file never throws — the caller (escape-hatch / PanLL panel) +/// always gets a typed `report` and can render the empty case without +/// crashing the host. let fromJson = (jsonStr: string): report => { - switch JSON.parseExn(jsonStr)->JSON.Classify.classify { - | Object(root) => + switch SafeJson.parse(jsonStr) { + | Error(_) => empty + | Ok(parsed) => + switch parsed->JSON.Classify.classify { + | Object(root) => let levels = switch Dict.get(root, "levels") { | Some(j) => parseArray(j, parseLevelSummary) | None => [] @@ -273,7 +280,8 @@ let fromJson = (jsonStr: string): report => { difficultyMin: {let (mn, _) = diffRange; mn}, difficultyMax: {let (_, mx) = diffRange; mx}, } - | _ => empty + | _ => empty + } } }