f20ba795c0
Phase 2.4, which I deferred during Phase 2 on a wrong assumption.
An unterminated CAD script hung the evaluator permanently. vm.eval is a
single blocking call, so `while true { s = s.translate(..) }` never
returns: the rebuild worker blocks forever, every later edit queues behind
it, and the UI sits on "Computing 3D model..." until the app is killed.
The source is user- and AI-authored, and an LLM will emit a loop like that
without much provocation.
Reproduced before fixing. A probe test was still running at 60 seconds,
and again at 90 with the budget removed.
I deferred this originally saying it needed "a cooperative check inside
the Makepad script VM or a watchdog that can kill and respawn the worker
thread", and called it a VM design decision. That was wrong, and I should
have read the VM before concluding it. makepad_script already provides
ScriptRunBudget::from_durations(soft, hard, sample_interval), checked in
run_core against a sampled Instant. The CAD module simply never set it.
The fix is six lines in eval_cad_script_in_vm. PHASE2_STATUS.md now
records the bad call rather than quietly marking the item done.
Only the hard deadline is used. A soft hit sets TimeBudgetYield, which
expects a host that drives the VM back to completion; there is no such
driver here, so a soft deadline would present as a script that silently
produced nothing at all. Soft is set equal to hard so the budget can only
ever produce a real, reported error. The previous budget is saved and
restored, so one evaluation cannot starve the next.
5 seconds, sampled every 4096 instructions. Generous deliberately: a real
document is a few hundred CSG ops and finishes in milliseconds, so
anything still running is a runaway rather than a slow model.
Result: the same script now returns "script time budget exceeded" in 5.2s.
Three tests. The runaway case is #[ignore]d because it burns the whole
budget by design; the other two guard the ways a timeout typically breaks
things -- that a normal script is unaffected across repeated evaluations
(catching a budget that is not reset), and that a syntax error still
reports itself rather than being misattributed to the timeout.
CI gate added and negative-tested: losing the budget line reintroduces a
hang that no test failure announces, only a frozen app.
617 lib + 154 integration, 0 failed.
189 lines
7.4 KiB
Markdown
189 lines
7.4 KiB
Markdown
# Phase 2 — Security — status
|
||
|
||
Phase 2 of `CAD_ASSESSMENT_AND_PLAN.md`. Every change is covered by a test,
|
||
and the two most repeatable defect classes are now blocked in CI.
|
||
|
||
**Test suite: 702 → 720 passing, 0 failing.**
|
||
|
||
---
|
||
|
||
## 2.1 Build-time paths used at runtime — DONE
|
||
|
||
```rust
|
||
pub fn cad_manifest_path() -> PathBuf {
|
||
Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf() // build machine!
|
||
}
|
||
```
|
||
|
||
`CARGO_MANIFEST_DIR` is resolved **at compile time**. Every CAD save,
|
||
export and script load resolved against the *build machine's* source tree:
|
||
|
||
- On a user's install that path does not exist, so saves failed silently —
|
||
the only signal was a status label.
|
||
- It leaks the developer's directory layout into the shipped binary.
|
||
- On a developer machine or CI runner, where the path *does* exist, the app
|
||
wrote into its own checkout.
|
||
|
||
Worst instance: `SessionConfig { cwd: env!("CARGO_MANIFEST_DIR") }` set the
|
||
**AI agent's working directory to the source tree**.
|
||
|
||
Replaced with `persistence::cad_data_dir()` → `app_data_dir()/nigig_build_store`,
|
||
the convention `cad_store::cad_projects_dir` already used. The agent now
|
||
gets a scratch dir at `<app-data>/nigig_build_store/agent`.
|
||
|
||
`cad_manifest_path()` is kept as a `#[deprecated]` alias so out-of-tree
|
||
callers keep compiling while pointing somewhere safe.
|
||
|
||
**Related find:** `system_prompt.md` — the file the AI path tried to read —
|
||
**does not exist in the repository**. Every session silently fell back to
|
||
the one-line string `"You are a CAD script generator."`. The real prompt
|
||
(API surface, degree convention, finite-number requirement) is now compiled
|
||
in, with an optional override at
|
||
`<app-data>/nigig_build_store/system_prompt.md`.
|
||
|
||
---
|
||
|
||
## 2.2 Hardcoded LLM endpoint — DONE
|
||
|
||
```rust
|
||
pub const LOCAL_OPENAI_URL: &str = "http://10.0.0.168:8080/v1/chat/completions";
|
||
```
|
||
|
||
A specific RFC1918 address, shipped in the binary, selectable from a
|
||
user-facing dropdown whose status label **advertised the address**
|
||
(`"Ready: Local OpenAI stream at 10.0.0.168:8080"`).
|
||
|
||
On any network other than the developer's, `10.0.0.168` is somebody else's
|
||
machine. Selecting that backend POSTed the user's CAD script — and any
|
||
attached reference image — in plaintext to an arbitrary LAN host.
|
||
|
||
Now read from the environment and **off by default**:
|
||
|
||
| Variable | Purpose |
|
||
|---|---|
|
||
| `NIGIG_CAD_LOCAL_OPENAI_URL` | Endpoint. Rejected unless `https://` **or** loopback `http://`. |
|
||
| `NIGIG_CAD_LOCAL_OPENAI_MODEL` | Model name. |
|
||
|
||
Plaintext HTTP to a non-loopback host is refused with a logged error:
|
||
sending a design document unencrypted to a LAN peer should be deliberate.
|
||
The status label no longer names any address, and when unconfigured it
|
||
tells the operator which variables to set.
|
||
|
||
5 tests, including one asserting the previously-hardcoded IP is now
|
||
rejected.
|
||
|
||
---
|
||
|
||
## 2.3 Unbounded image attachment — DONE
|
||
|
||
```rust
|
||
std::fs::read(local.path()).ok().map(|bytes| { /* base64, no checks */ })
|
||
```
|
||
|
||
No size limit, and the MIME type was inferred from the **filename
|
||
extension**. A file was read whole, expanded ~1.37× into base64, stored in
|
||
a process-global, interpolated into a prompt (another copy) and uploaded.
|
||
|
||
- **Size cap** — 8 MB, checked with `metadata()` *before* reading, so an
|
||
oversized file is never loaded into memory.
|
||
- **Magic-byte sniffing** — PNG/JPEG/GIF/WebP/BMP identified by content.
|
||
Renaming a shell script to `.png` no longer gets its bytes uploaded.
|
||
- The sniffed MIME is carried through to the prompt instead of being
|
||
re-guessed from the extension.
|
||
|
||
Extracted into a testable `read_attachment_as_base64` rather than left
|
||
inline in the file-picker callback. 5 tests.
|
||
|
||
---
|
||
|
||
## 2.5 Output escaping and the CDN script — DONE
|
||
|
||
**SVG.** Added `escape_xml` and routed the two non-numeric attributes
|
||
through it. This is not fixing a live injection — the collector currently
|
||
emits only numbers and colour literals — but there was no escaping helper
|
||
in the module at all, and adding a `<text>` label to a floor plan (the
|
||
obvious next feature) would have made it a sink.
|
||
|
||
**Generated HTML viewer.** `model_filename` was interpolated raw into both
|
||
a `src="..."` attribute and the page text. Now escaped; a test asserts the
|
||
attribute-breakout payload `x" onerror="alert(1)` cannot survive.
|
||
|
||
**The `<script>` tag.** Was an unpinned, unverified CDN load. Now carries
|
||
`integrity` + `crossorigin`, so a substituted response is rejected by the
|
||
browser rather than executed — an exported floor plan is an artifact a
|
||
contractor may open on-site months later.
|
||
|
||
> **Found while computing the SRI hash:** the pinned URL referenced
|
||
> `@google/model-viewer@3.5.1`, **a version that does not exist**. unpkg
|
||
> returns 404, so *every exported viewer has been silently broken* — the
|
||
> web component never loaded and the page showed nothing. Now pinned to
|
||
> 4.0.0 with a hash computed from the actual bytes, plus a test that the
|
||
> URL and hash reference the same version and a `<noscript>` fallback.
|
||
|
||
---
|
||
|
||
## 2.6 / 2.7 Hygiene — DONE
|
||
|
||
- **`$USER` in PDF metadata.** Every exported PDF embedded the OS username
|
||
in a document that typically gets emailed to a client. Now blank unless
|
||
`NIGIG_CAD_PDF_AUTHOR` is set.
|
||
- **Document content in logs.** `request_rebuild` and `eval_cad_script_in_vm`
|
||
logged prefixes of the user's script unconditionally. Content is now
|
||
`#[cfg(debug_assertions)]`; release builds log the length only.
|
||
|
||
---
|
||
|
||
## CI enforcement
|
||
|
||
Both classes above are easy to reintroduce by copy-paste and invisible in
|
||
review, so `supply-chain` now fails the build on either:
|
||
|
||
1. `env!("CARGO_MANIFEST_DIR")` in CAD runtime code (comments excluded).
|
||
2. An RFC1918 URL literal in CAD code (comments and the tests that assert
|
||
such addresses are *rejected* excluded).
|
||
|
||
Both were verified in **both directions** — they pass on the current tree,
|
||
and reintroducing each defect makes them fail.
|
||
|
||
---
|
||
|
||
## Deferred
|
||
|
||
**2.4 — script execution budget. RESOLVED** (see below).
|
||
|
||
~~`eval_cad_script` still has no timeout or instruction cap, so
|
||
`while true { s = s.merge(cube(...)) }` hangs the rebuild worker
|
||
forever.~~
|
||
|
||
I deferred this on the assumption that it needed a cooperative check
|
||
added to the Makepad script VM, or a watchdog to kill and respawn the
|
||
worker. **That assumption was wrong, and I should have checked the VM
|
||
before concluding it.** `makepad_script` already ships
|
||
`ScriptRunBudget::from_durations(soft, hard, sample_interval)`, checked
|
||
inside `run_core`; the CAD module simply never set it. The fix is six
|
||
lines in `eval_cad_script_in_vm`, not a VM design change.
|
||
|
||
Reproduced the hang first (still running at 60s, then at 90s with the
|
||
budget removed), then fixed it: the same script now returns
|
||
`script time budget exceeded` in 5.2s. Only the hard deadline is used —
|
||
a soft hit yields with `TimeBudgetYield` and needs a host that resumes
|
||
the VM, which does not exist here, so soft is set equal to hard.
|
||
|
||
Covered by `script_bindings::script_budget_tests` and a CI gate.
|
||
|
||
**2.6 (file picker for exports).** Exports still write to fixed paths under
|
||
the app data dir. That is no longer a *security* problem now that the path
|
||
is not the source tree — it is a UX gap, better handled alongside the
|
||
Phase 4 exporter consolidation, since all five export paths duplicate the
|
||
same prologue today.
|
||
|
||
---
|
||
|
||
## Verification
|
||
|
||
```
|
||
cargo test --locked -p nigig-build --lib # 720 passed; 0 failed; 7 ignored
|
||
```
|
||
|
||
19 new tests: 5 endpoint, 5 attachment, 5 viewer-HTML, 3 SVG escaping,
|
||
plus coverage folded into existing modules.
|