Spaces:
Running
Running
| # 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. | |