# Security Hardening Review

Review date: 2026-06-14. Reviewer: Claude (Opus 4.8), for OpenAI Codex to implement.

Final submission note, 2026-06-15: this review was implemented before the Space
became public under `build-small-hackathon/Snap2Sim`.

This review answers three questions:

1. Are the endpoints secure?
2. Is the UI/UX good?
3. Should we prioritize Three.js or A-Frame for animation rendering?

Two product decisions were confirmed with the owner and shape the priorities below:

- **The Space is public now.** Findings that were only mitigated by private
  visibility were treated as blockers and were resolved before publication.
- **Screen-only viewing; no VR/AR goal.** WebXR is not required, which removes
  A-Frame's main structural advantage.

File references use `path:line` from the state of the repo at review time.

---

## 1. Endpoint Security

### Architecture summary

- `app.py` is the public Hugging Face Space (`gradio.Server`). It serves
  `index.html` at `/` and exposes `POST /analyze_image` and
  `POST /generate_scene`. The browser calls these **same-origin**, and the
  server attaches the Modal bearer token when it forwards to Modal. This split
  (token stays server-side) is correct and matches `SECURITY.md`.
- `modal_app.py` exposes the GPU/inference web endpoints, each guarded by a
  bearer token via `require_authorization`.

The Modal layer is in good shape. The Space layer is the exposure.

### HIGH — The Space's own `/analyze_image` and `/generate_scene` are unauthenticated (`app.py:64`, `app.py:74`)

These endpoints have no auth of their own. At review time, they were gated only
by the Space being private. **Because the Space is now public, anyone on the
internet can POST to them unless the implemented abuse controls remain active.**

- With `INFERENCE_BACKEND=local` (current default), they return placeholder data
  — low impact but a free compute/bandwidth sink (base64 decode + PIL decode +
  scene build per request).
- With `INFERENCE_BACKEND=modal`, every public request is proxied to Modal
  **using the server's own bearer token**, turning the Space into an open proxy
  that spends GPU credits on behalf of any anonymous caller. The Modal bearer
  token protects Modal from direct callers, but the Space is a trusted caller,
  so the token does not help here. This directly contradicts the stated intent
  in `SECURITY.md` that the token is "the real protection against
  credit-spending spam."

**Recommendation (implemented before publication):**

- Add abuse controls at the Space layer that do not require leaking a secret to
  the browser. Options, roughly in order of preference:
  1. **Server-side rate limiting / quota** per client IP and a global ceiling
     (e.g. `slowapi`/`limits`, or a small in-process token bucket). This is the
     most important mitigation since the Space is intentionally token-free on
     the client side.
  2. A short-lived, server-issued session/CSRF-style token handed to the page on
     `GET /` and required on the POST endpoints, to stop trivial scripted abuse
     (not a strong control, but raises the bar).
  3. Hard caps: max upload size, max requests/min, and a circuit breaker that
     falls back to `local` placeholder mode if a Modal spend threshold is hit.
- Consider HF Space-level protections too (HF supports gating), but do not rely
  on them as the only layer once public.

### HIGH — Untrusted scene HTML is injected via `innerHTML` (`index.html:545`)

`renderAframe()` does `viewport.innerHTML = sceneHtml;` where `sceneHtml` comes
from `/generate_scene`. In the deterministic path the HTML is built server-side
with `html.escape` (`snap2sim/aframe_scene.py:14-16`, safe). But the
model-generation path (`generate_scene_llamacpp` in `modal_app.py:462`) returns
**raw model output**, extracted by `parse_scene_response`
(`snap2sim/model_io.py:49`) which does *no sanitization* — it slices everything
between `<a-scene>` and `</a-scene>` verbatim.

Because the scene is derived from a **user-uploaded image**, an attacker can
prompt-inject the vision/scene model (text in the photo, adversarial content)
into emitting markup such as `<a-entity onloaded="...">` or
`<img src=x onerror=...>`. `innerHTML` will not execute injected `<script>`
tags, but it *does* run inline event-handler attributes and A-Frame can execute
component JS — so this is a realistic stored/reflected XSS vector that becomes
internet-reachable when the Space is public.

The validation in `validate_analysis` protects the *JSON analysis* path but does
nothing for the free-form HTML scene path.

**Recommendation:** Do not inject free-form model HTML into the DOM. See §3 —
the cleanest fix is to make scene rendering deterministic from the validated
JSON (which is already escaped/validated) and stop returning model-authored
HTML. If any model-authored HTML is ever rendered, sanitize it with an
allowlist (e.g. DOMPurify with an A-Frame-aware tag/attribute allowlist) before
`innerHTML`, and strip all `on*` attributes.

### MEDIUM — No upload size limit or decompression-bomb hardening (`app.py:88`, `modal_app.py:327`)

`_decode_image` and `write_payload_image` do `base64.b64decode(...)` then
`Image.open(BytesIO(...))` with no cap on the base64 string length or decoded
pixel count. A large or crafted image (decompression bomb) can exhaust memory on
`cpu-basic`. PIL's default `DecompressionBombWarning` only warns; it does not
block at typical sizes.

**Recommendation:**

- Reject requests whose base64 body exceeds a sane limit (e.g. ~8–12 MB) before
  decoding.
- Set `Image.MAX_IMAGE_PIXELS` to an explicit ceiling and catch
  `Image.DecompressionBombError`.
- Wrap decode failures and return a clean `400` instead of a 500/stack trace.

### MEDIUM — Fixed temp file paths cause cross-request collisions (`modal_app.py:199`, `modal_app.py:342`)

Both the smoke test and `write_payload_image` write to fixed paths
(`/tmp/snap2sim-request-image.jpg`). Concurrent requests on the same container
overwrite each other's input image, so one user could be analyzed against
another user's photo. This is both a correctness and a privacy concern.

**Recommendation:** Use `tempfile.NamedTemporaryFile`/`mkstemp` (unique per
request) and clean up in a `finally`.

### LOW — Third-party scripts loaded without Subresource Integrity (`index.html:7-13`)

A-Frame (`aframe.io`), the Gradio client (`jsdelivr`), and fonts
(`bunny.net`) are loaded from CDNs with no `integrity`/SRI hashes. A CDN
compromise would run arbitrary JS in the (soon public) page.

**Recommendation:** Pin versions and add SRI hashes where the CDN supports it.
Note: if A-Frame is dropped (see §3) and Three.js is bundled/self-hosted, this
surface shrinks substantially.

### LOW — Unused third-party dependency loaded (`index.html:10-13`)

The Gradio JS client is imported into `window.snap2simGradioClient` but never
used; all calls go through a plain `fetch` (`postJson`, `index.html:765`).
Remove the import to cut an unnecessary CDN dependency and load.

### Positives (keep these)

- Modal bearer check uses constant-time comparison
  (`token_secrets.compare_digest`, `modal_app.py:113`) and returns `503` when
  the token is unconfigured, `401` on mismatch (`modal_app.py:105-115`).
  Unauthenticated `401` was verified per AGENTS.md.
- Token never reaches the browser; server-side attaches it
  (`snap2sim/backend.py:62-68`). Matches `SECURITY.md` guidance.
- The deterministic scene builders escape text with `html.escape`
  (`snap2sim/aframe_scene.py:14-16`) and the analysis readout uses `textContent`
  rather than `innerHTML` (`index.html:513-536`) — no XSS on that path.
- GitHub→HF workflow uses least-privilege `permissions: contents: read` and
  keeps `HF_TOKEN` in secrets (`.github/workflows/sync_to_hf.yml:19-21`).
- Schema validation (`snap2sim/schema.py:176`) runs before scene generation.

---

## 2. UI/UX

The visual direction is strong and cohesive: a "technical cutaway / field
manual" aesthetic (amber `#E8A33D` + cyan `#5FD4D0` on dark, blueprint grid,
Chakra Petch / Fira Code). The two-pane layout (viewport + readout) is clear and
the loading choreography (progress bar, scan-line reveal, staggered mesh
reveal, cold-start "WAKING THE WORKSHOP" message after 6.5 s) is a genuinely
nice touch for HF cold starts. Drag-and-drop plus a Load button, and a
play/pause control, cover the core interactions. The fallback chain
(A-Frame → deterministic Three.js → "3D runtime unavailable") is thoughtful.

### Issues, roughly by impact

**Accessibility (MEDIUM — matters more once public):**

- Status messages (`index.html:392`) are not announced to screen readers. Add
  `aria-live="polite"` (and `role="status"`) so analysis progress/errors are
  read out.
- `#viewport` injected 3D content has no text alternative. Provide an
  `aria-label`/visually-hidden description, or rely on the readout panel as the
  accessible representation and mark the viewport `aria-hidden`.
- Low-contrast text: `--text-muted #6B7280` on the dark panel is below WCAG AA
  for body text (`trigger`, raw JSON, drop-zone hint). Nudge it lighter.
- The file input is triggered via a `<label>` drop zone (`index.html:382`); make
  sure keyboard focus + Enter works to open the picker, and the Load button is
  already focusable (good).

**Functional gaps:**

- **No preview of the uploaded photo.** Users lose their reference image once
  the cutaway renders. Show a thumbnail of the source photo in the panel — it
  also reinforces the "this came from *my* object" story for a demo.
- **`playButton` initial state is confusing** (`index.html:379`): it reads
  "Pause" while disabled, before any scene exists. Start as disabled "Play" or
  hide until a scene is ready.
- **Error UX is thin:** errors render as uppercased status text only
  (`index.html:495`). Add a visible retry affordance and keep the drop zone
  reachable so the user can try another photo without hunting for the Load
  button.
- **No client-side file validation:** `accept="image/*"` only. Reject
  non-images and oversized files before base64 inflation (~33% size increase)
  and surface a friendly message. (Pairs with the server-side size cap in §1.)
- **Mobile:** the layout has a breakpoint at 860 px (good), but A-Frame
  `look-controls` can capture touch/scroll. If Three.js becomes the primary
  renderer (see §3), use OrbitControls tuned for touch and ensure the page still
  scrolls. Also consider `capture="environment"` to offer "take a photo" on
  mobile.
- **No confidence visualization:** confidence is shown as a number
  (`index.html:516`); a small bar/gauge would read faster and fit the
  instrument aesthetic.
- **First-run onboarding** is just "Drop component photo." A one-line example
  ("try a lock, a gear, a ratchet…") lowers the cold-start barrier for judges.

**Verdict:** UI/UX is above-average for a hackathon and the aesthetic is a real
asset. The gaps are accessibility, an uploaded-image preview, and error/retry
polish — none are large, and they raise the demo from "looks great" to "feels
finished."

---

## 3. Three.js vs A-Frame for Rendering

**Recommendation: consolidate on Three.js as the primary (and only) renderer,
driven deterministically from the validated JSON analysis. Drop the A-Frame
dependency and stop returning model-authored scene HTML.**

### Reasoning

1. **No VR/AR goal (confirmed).** A-Frame's defining advantage is declarative
   WebXR. Screen-only viewing removes the main reason to carry it. Without VR,
   A-Frame is mostly a ~1.2 MB declarative wrapper over Three.js.

2. **The Three.js path is already the more capable implementation.** The
   "fallback" in `index.html:565-755` has OrbitControls, DOM-projected part
   labels, staggered reveal, gear extrusion, fog/lighting, and motion handling.
   It is richer than the A-Frame output (`snap2sim/aframe_scene.py`). You'd be
   promoting your best renderer, not rewriting one.

3. **True "cutaway" cross-sections need clipping planes.** The product is an
   *annotated technical cutaway*. Three.js exposes
   `renderer.localClippingEnabled` + `material.clippingPlanes` natively, which is
   the clean way to slice a housing and reveal internals. A-Frame has no
   first-class clipping-plane support. If "cutaway" is to be more than parts
   floating in space, Three.js is required anyway.

4. **It removes the §1 HIGH XSS vector.** Rendering deterministically from
   validated JSON means no `innerHTML` of untrusted model HTML. The model
   already produces the structured JSON (`snap2sim/schema.py`), which is
   validated and safe; let a deterministic Three.js builder turn that JSON into
   geometry. This is both safer and more *reliable* than asking an LLM to emit
   well-formed scene markup.

5. **Smaller, self-hostable footprint.** Three.js can be pinned/bundled and
   served same-origin, shrinking the CDN/SRI surface from §1.

### What this means concretely for implementation

- **Keep the model's job as the JSON `analysis` contract** (it already is,
  `snap2sim/schema.py`). The model should *not* author HTML/JS scenes.
- **Make `generate_scene` deterministic** from that JSON. The browser already
  has `buildDeterministicScene()` — promote it from fallback to the main path.
  You can drop the server returning scene HTML entirely, or keep the server
  returning the validated analysis and let the browser build the scene.
- **Remove A-Frame** (`index.html:9`, the `<a-scene>` injection in
  `renderAframe`, and the A-Frame branch of `generate_scene_llamacpp`) once the
  Three.js path is the default. Delete `snap2sim/aframe_scene.py` and the
  A-Frame prompt template after migration, or keep them only if you want a
  server-rendered static preview.
- **If you keep any model-authored HTML path**, it must be sanitized
  (DOMPurify, strip `on*`) per §1.

This also simplifies the mental model: one renderer, one safe data contract,
no dual A-Frame/Three.js maintenance.

---

## Suggested Implementation Order (for Codex)

Blockers resolved before the Space went public:

1. Add server-side rate limiting / quota + max upload size to `app.py`
   endpoints (§1 HIGH).
2. Eliminate the untrusted-HTML `innerHTML` path: move to deterministic Three.js
   rendering from validated JSON, or sanitize (§1 HIGH + §3).
3. Add decompression-bomb guards and unique temp files (§1 MEDIUM).

Then:

4. Consolidate rendering on Three.js; remove A-Frame and the dead Gradio client
   import; add SRI/self-hosting (§3, §1 LOW).
5. UI/UX polish: uploaded-image preview, `aria-live` status, contrast fix,
   error/retry affordance, play-button initial state (§2).

Out of scope / leave as-is: Modal bearer auth (already correct), the
GitHub→HF sync workflow, the schema contract.