feat: include DimSim#2081
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
| except AttributeError: | ||
| pass | ||
| sock.bind(("", MCAST_PORT)) |
| sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP) | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
| sock.bind(("", LCM_PORT)) |
| app.get("/vlm/asset-library", (req, res) => { | ||
| try { | ||
| if (existsSync(ASSET_LIBRARY_FILE)) { | ||
| const data = JSON.parse(readFileSync(ASSET_LIBRARY_FILE, "utf-8")); | ||
| if (Array.isArray(data)) return res.json({ assets: data }); | ||
| } | ||
| } catch (err) { | ||
| console.error("[asset-library] read failed:", err.message); | ||
| } | ||
| res.json({ assets: [] }); | ||
| }); |
| app.post("/vlm/asset-library", (req, res) => { | ||
| try { | ||
| const { assets } = req.body; | ||
| writeFileSync(ASSET_LIBRARY_FILE, JSON.stringify(assets), "utf-8"); | ||
| res.json({ ok: true, count: assets?.length || 0 }); | ||
| } catch (err) { | ||
| console.error("[asset-library] write failed:", err.message); | ||
| res.status(500).json({ detail: err.message }); | ||
| } | ||
| }); |
| _npcClock: any = null; // THREE.Clock | ||
|
|
||
| async _execCode(code: string, id?: string): Promise<void> { | ||
| console.log(`[sceneEditor] exec${id ? ` (${id})` : ""}:`, code.slice(0, 100)); |
| } | ||
|
|
||
| async _loadScript(url: string, id?: string): Promise<void> { | ||
| console.log(`[sceneEditor] loadScript${id ? ` (${id})` : ""}:`, url); |
| async _loadScript(url: string, id?: string): Promise<void> { | ||
| console.log(`[sceneEditor] loadScript${id ? ` (${id})` : ""}:`, url); | ||
| try { | ||
| const resp = await fetch(url); |
Greptile SummaryThis PR introduces DimSim, a 3D browser-based robotics simulator, as a new subproject under
Confidence Score: 3/5The PR introduces a large new subproject with several correctness and security issues, some unresolved from a prior round of review; merging while those issues remain open carries real risk. The scene name is inserted into the served HTML page's inline script without escaping, which will silently corrupt the page for any scene name containing a double-quote, and allows tag injection for names containing
|
| Filename | Overview |
|---|---|
| misc/DimSim/dimos-cli/bridge/server.ts | Central WebSocket bridge and static file server; scene name is injected into the served HTML without escaping, breaking the page for any scene name containing a double quote. |
| misc/DimSim/dimos-cli/eval/runner.ts | Sequential and parallel eval runners; timed-out sendAndWait calls leak message-event listeners (previously flagged), and JUnit XML output does not escape special characters in rubric fields (previously flagged). |
| misc/DimSim/server.js | Express VLM proxy server; CORS wildcard origin exposes API keys to any origin (previously flagged); otherwise straightforward request routing with retry logic. |
| misc/DimSim/dimos-cli/setup.ts | Scene download and extraction logic; extractGz uses shell interpolation of user-supplied archive/destination paths (previously flagged); core download and version tracking logic is sound. |
| dimos/simulation/dimsim/dimsim_process.py | Python wrapper that spawns the Deno CLI as a subprocess; lifecycle management is clean with daemon threads for log streaming and graceful SIGTERM/SIGKILL fallback in stop(). |
| misc/DimSim/src/dimos/evalHarness.ts | Browser-side eval orchestrator; rubric scoring is consistent with the runner's pass logic, and the ping/pong handshake and workflow lifecycle are correctly implemented. |
| .github/workflows/dimsim-check.yml | New CI workflow for DimSim; runs npm build, Deno type-check, and scene template validation on changes to misc/DimSim/; permissions are correctly scoped to contents:read. |
| misc/DimSim/dimos-cli/headless/launcher.ts | Playwright-based Chromium launcher supporting single-page and multi-page parallel eval modes; GPU/CPU render paths are cleanly separated with appropriate Chrome flags. |
Sequence Diagram
sequenceDiagram
participant PY as Python DimSimProcess
participant CLI as Deno CLI (cli.ts)
participant BRIDGE as Bridge Server (server.ts)
participant BROWSER as Playwright/Browser (engine.js)
participant LCM as LCM Multicast
participant AGENT as Python Robot Agent
PY->>CLI: spawn deno run cli.ts dev --scene apt --headless
CLI->>BRIDGE: startBridgeServer(port, distDir, scene)
CLI->>BROWSER: launchHeadless(url)
BROWSER->>BRIDGE: WebSocket connect (control ch)
BROWSER->>BRIDGE: WebSocket connect (sensor ch)
BROWSER->>BRIDGE: Rapier snapshot (physics state)
BRIDGE->>BRIDGE: initServerSystems(snapshot)
BRIDGE->>LCM: LCM.start()
AGENT->>LCM: publish /cmd_vel (velocity commands)
LCM->>BRIDGE: packet received
BRIDGE->>BROWSER: forward LCM packet via WS
BROWSER->>BRIDGE: /odom (pose) via sensor WS
BRIDGE->>LCM: republish odom
Note over CLI,BROWSER: Eval mode
CLI->>BROWSER: "sendAndWait({type:ping}, pong)"
BROWSER-->>CLI: "{type:pong}"
CLI->>BROWSER: "{type:startWorkflow, workflow}"
BROWSER->>BROWSER: run rubric scoring at timeout
BROWSER-->>CLI: "{type:workflowComplete, rubricScores}"
CLI->>CLI: toJunitXml(results)
Reviews (3): Last reviewed commit: "skip lfs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const proc = new Deno.Command("sh", { | ||
| args: ["-c", `gunzip -c "${archive}" > "${destFile}"`], | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| }).spawn(); | ||
| const status = await proc.status; | ||
| if (!status.success) throw new Error(`gunzip failed (exit ${status.code})`); | ||
| } |
There was a problem hiding this comment.
archive and destFile are derived directly from the CLI name argument and interpolated into a shell command string. A scene name containing " can escape the quoted argument and inject arbitrary shell commands — e.g. dimsim scene install 'apt"; rm -rf ~/; echo "' would execute the injected commands at the user's privilege level.
The fix is to avoid sh -c entirely and spawn gunzip with its arguments as a proper array, piping stdout to the destination file directly (e.g. Deno.Command("gunzip", { args: ["-c", archive], stdout: "piped" }) piped to a Deno.open write stream).
| for (const r of results) { | ||
| const time = (r.durationMs / 1000).toFixed(1); | ||
| xml += ` <testcase name="${r.name}" classname="${r.environment}" time="${time}"`; | ||
| if (r.pass) { | ||
| xml += ` />\n`; | ||
| } else { | ||
| xml += `>\n`; | ||
| xml += ` <failure message="${r.reason}">${JSON.stringify(r.rubricScores)}</failure>\n`; | ||
| xml += ` </testcase>\n`; | ||
| } | ||
| } |
There was a problem hiding this comment.
r.name, r.environment, or r.reason (e.g. a task description containing " or <) will produce malformed JUnit XML, which many CI parsers will reject entirely. All values interpolated into XML attributes or content should be escaped.
| for (const r of results) { | |
| const time = (r.durationMs / 1000).toFixed(1); | |
| xml += ` <testcase name="${r.name}" classname="${r.environment}" time="${time}"`; | |
| if (r.pass) { | |
| xml += ` />\n`; | |
| } else { | |
| xml += `>\n`; | |
| xml += ` <failure message="${r.reason}">${JSON.stringify(r.rubricScores)}</failure>\n`; | |
| xml += ` </testcase>\n`; | |
| } | |
| } | |
| function escXml(s: string): string { | |
| return String(s) | |
| .replace(/&/g, "&") | |
| .replace(/</g, "<") | |
| .replace(/>/g, ">") | |
| .replace(/"/g, """); | |
| } | |
| for (const r of results) { | |
| const time = (r.durationMs / 1000).toFixed(1); | |
| xml += ` <testcase name="${escXml(r.name)}" classname="${escXml(r.environment)}" time="${time}"`; | |
| if (r.pass) { | |
| xml += ` />\n`; | |
| } else { | |
| xml += `>\n`; | |
| xml += ` <failure message="${escXml(r.reason)}">${escXml(JSON.stringify(r.rubricScores))}</failure>\n`; | |
| xml += ` </testcase>\n`; | |
| } | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: Viswajit Nair <viswajitnair@gmail.com>
paul-nechifor
force-pushed
the
paul/feat/include-dimsim
branch
from
40287e9 to
bed678f
Compare
|
|
||
| // ── App ───────────────────────────────────────────────────────────────────── | ||
| const app = express(); | ||
| app.use(cors()); |
There was a problem hiding this comment.
cors() with no configuration sets Access-Control-Allow-Origin: *, which tells the browser to allow any website to make cross-origin requests to this server. Because the API keys (OPENAI_API_KEY, GEMINI_API_KEY) live server-side, a malicious web page the user happens to visit can silently send POST requests to http://127.0.0.1:8000/vlm/decision and http://127.0.0.1:8000/vlm/generate-image, triggering paid API calls on the user's behalf and reading the VLM responses. It can also overwrite the asset-library file via /vlm/asset-library.
Restrict the allowed origin to the DimSim frontend explicitly (e.g. cors({ origin: ["http://localhost:5173", "http://localhost:8080"] })) or, if the port is dynamic, validate the Origin header against a known prefix at request time.
| function sendAndWait(cmd: Record<string, unknown>, responseType: string, timeoutMs = 60000): Promise<Record<string, unknown>> { | ||
| return new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => reject(new Error(`Timeout waiting for ${responseType}`)), timeoutMs); | ||
|
|
||
| const handler = (event: MessageEvent) => { | ||
| if (typeof event.data !== "string") return; | ||
| try { | ||
| const msg = JSON.parse(event.data); | ||
| if (msg.type === responseType) { | ||
| clearTimeout(timeout); | ||
| ws.removeEventListener("message", handler); | ||
| resolve(msg); | ||
| } | ||
| } catch { /* not JSON */ } | ||
| }; | ||
|
|
||
| ws.addEventListener("message", handler); | ||
| ws.send(JSON.stringify(cmd)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
sendAndWait calls leak event listeners
When the timeout fires and the promise rejects, ws.removeEventListener("message", handler) is never called. During the ping/pong readiness loop (lines 115–124), each failed 3-second attempt registers another "pong" listener that is never cleaned up. After 60 retries (the full 60-second window), 60 stale handlers are attached to the WebSocket. They stay until a matching message finally arrives or the socket closes. In a long-running parallel eval session with repeated timeouts this grows unboundedly and could cause delayed message handling (all stale handlers fire on every incoming message).
The timeout callback should call ws.removeEventListener("message", handler) before calling reject(…) so the listener is always removed on settlement.
| const ratesJs = sensorRates ? `window.__dimosSensorRates=${JSON.stringify(sensorRates)};` : ""; | ||
| const enableJs = sensorEnable ? `window.__dimosSensorEnable=${JSON.stringify(sensorEnable)};` : ""; | ||
| const fovJs = cameraFov ? `window.__dimosCameraFov=${cameraFov};` : ""; | ||
| const inject = `<script>window.__dimosMode=true;window.__dimosScene="${scene || "apt"}";${headless ? "window.__dimosHeadless=true;" : ""}${ratesJs}${enableJs}${fovJs}</script>`; |
There was a problem hiding this comment.
scene value is interpolated into a JS string literal using double-quote delimiters but is never escaped. A scene name containing " breaks the string (e.g. --scene 'apt" yields window.__dimosScene="apt";" which is a syntax error), and a value like apt"</script><script> would silently terminate the script block and inject arbitrary HTML. The idiomatic fix is to use JSON.stringify so the value is always a well-formed JS string literal, regardless of its contents.
| const inject = `<script>window.__dimosMode=true;window.__dimosScene="${scene || "apt"}";${headless ? "window.__dimosHeadless=true;" : ""}${ratesJs}${enableJs}${fovJs}</script>`; | |
| const inject = `<script>window.__dimosMode=true;window.__dimosScene=${JSON.stringify(scene || "apt")};${headless ? "window.__dimosHeadless=true;" : ""}${ratesJs}${enableJs}${fovJs}</script>`; |
2 participants
Problem
Closes DIM-XXX
Solution
How to Test
Contributor License Agreement