Repository navigation
fix: multiple minor bugs - #194
Merged
Merged
Conversation
`detectRunningApp(udid)` interpolated `params.device_id` directly into
two execSync template strings:
execSync(`xcrun simctl spawn ${udid} launchctl list`)
execSync(`xcrun simctl listapps ${udid} | plutil -convert json -o - -`)
A device_id like `x;touch /tmp/pwn;#` was parsed by /bin/sh as additional
commands, giving any caller (including a cross-origin browser POST, since
the tool-server has no auth and serves wildcard CORS) arbitrary command
execution as the user running the tool-server.
Switched both calls to execFileSync with array args, which never invokes
a shell. The piped `xcrun listapps | plutil` is now two stages with the
plist piped via stdin (input option) instead of through /bin/sh.
The unsanitized `name` parameter was joined into <project_root>/.argent/flows/<name>.yaml so a name like "../../../tmp/x" landed the YAML file at an attacker-chosen path. Combined with the no-auth/wildcard-CORS HTTP surface, a cross-origin browser POST could write attacker-controlled YAML anywhere the user can write — .github/workflows/*.yaml, docker-compose.yml, etc. Adds assertSafeFlowName() in flow-utils.ts. getFlowPath() now requires ^[A-Za-z0-9_\-]+$ (letters, digits, underscore, hyphen — no separators, no "..", no spaces) and additionally checks that the resolved path stays inside the flows directory via path.relative. Centralizing the check in getFlowPath() means every caller (flow-start-recording, flow-add-step, flow-insert-echo, flow-finish-recording, flow-run, flow-read-prerequisite) is covered without per-tool churn.
The tool-server binds to 127.0.0.1, but a public attacker page that DNS-rebinds its hostname to 127.0.0.1 still reaches us — the Host header is the only signal that distinguishes that traffic from a legitimate same-origin request. Adds an early middleware (runs before CORS) that 403s any request whose Host header is not a loopback name (127.0.0.1, localhost, ::1). IPv6 literals in brackets are normalised before the check. Without this, an attacker can pair a DNS-rebinding flow with the existing wildcard CORS to read /tools and POST to /tools/:name from any public origin.
…xtensions
readSourceFragment() previously accepted any `file` field on the
SourceLocation it was given:
- if absolute, it read it directly (anywhere on disk);
- if relative, it joined with projectRoot but never verified the
resolved path stayed inside (so "../../../etc/passwd" worked).
The `file` field originates in a React fiber's _debugSource.fileName,
which a malicious app — or a cross-origin browser tab leveraging Vuln 1
to call debugger-evaluate — can mutate to any string. Combined with the
no-auth HTTP surface, this was an arbitrary file-read primitive
(~/.gitconfig, ~/.aws/credentials, /etc/passwd, ...).
Adds two gates inside readSourceFragment:
1. Containment: path.resolve the absolute candidate, then
path.relative(projectRoot, absPath) — null if it starts with `..`
or is itself absolute (escaped projectRoot).
2. Extension allowlist: only .js/.jsx/.ts/.tsx/.mjs/.cjs/.json. Even
.env files inside the project are out of scope here.
Both checks return null silently; the fiber-source-fragment path is
opportunistic (the inspector never required it to succeed) so attackers
get no signal about whether a file existed.
doRegister() fetched the sourceMapURL from a Debugger.scriptParsed CDP event with no URL validation. Combined with the no-auth tool-server HTTP surface, a malicious app — or any caller of debugger-evaluate / debugger-connect — could inject a script whose //# sourceMappingURL pointed at an attacker-chosen URL. The tool-server would then dutifully fetch it, turning it into a blind SSRF probe of the host network (internal services, AWS metadata at 169.254.169.254, intranet pages, etc.). Adds isAllowedSourceMapURL(): only http/https URLs whose hostname is localhost, 127.0.0.1, or ::1 (IPv6 brackets stripped). Anything else, including malformed URLs and file:// schemes, is rejected and the map is silently skipped. The data: URI branch is untouched — those are inline and don't fetch.
The tool-server exposed every registered tool over HTTP with no
authentication and Access-Control-Allow-Origin: * on every response, so
any web origin (Firefox unconditionally; Chrome via PNA-disabled or
loopback-served) could read /tools and invoke /tools/:name from a
cross-origin browser tab.
Adds a per-process auth token:
- launcher.ts generates a 32-byte hex token at server start, passes it
to the spawned tool-server via ARGENT_AUTH_TOKEN, and persists it in
~/.argent/tool-server.json with mode 0600.
- http.ts requires Authorization: Bearer <token> on every route except
/preview (which is the in-process UI and was not gated before;
tightening it is a follow-up). Constant-time compare prevents
byte-by-byte timing recovery. Wrong/missing token returns 401.
- argent-tools-client and argent-mcp/mcp-server thread the token
through every fetch (fetchTools, callTool, healthcheck, /shutdown).
- argent-cli/server.ts reads the token for `argent server status`
healthcheck. The token is hidden from --json output (replaced with
`hasToken: bool`).
- ensureToolsServer now returns { url, token } instead of just url.
Also drops the global Access-Control-Allow-Origin: * middleware. The
tool-server is in-process stdio for MCP and same-origin for /preview;
no browser needs cross-origin access. Without CORS, the browser cannot
even read response bodies for hostile cross-origin probes.
Backward compat: when ARGENT_AUTH_TOKEN is unset (e.g., `npm run dev`)
the server logs a one-shot warning and skips the gate, so existing local
dev workflows keep working. Stale state files lacking a token are
treated as a launcher-generation mismatch and the server is respawned.
latekvo
changed the title
…gnature - Run prettier --write on source-resolver.ts and source-resolver.test.ts (collapsed extension allowlist + readSourceFragment call sites onto single lines, per repo style). - Update launcher.test.ts to match the new buildToolsServerEnv signature (token is now the third positional arg, baseEnv moved to fourth). Adds an assertion that the auth token is forwarded into the spawned tool-server's env.
…ection-bundle # Conflicts: # packages/tool-server/src/http.ts
- dev.cjs: set ARGENT_TOOLS_URL on the Claude AND Cursor dev MCP entries so `npm run dev` reuses the running tokenless dev tool-server instead of ensureToolsServer SIGTERM-ing it and spawning the throwing stub.
- source-maps: fetch sourceMapURL with { redirect: "error" } so a loopback URL that passes the allowlist cannot 302 onto an internal/metadata host (the redirect target was never re-validated).
- launcher writeState: unlink the state file before writeFile so mode 0600 is always applied to a freshly created file (the bearer token could previously be written into a pre-existing 0644 file).
- source-resolver: realpath both projectRoot and the candidate before the containment + extension checks, closing the in-project symlink escape (e.g. an in-project *.js symlink to /etc/passwd).
- source-resolver: drop .json from the source-extension allowlist so an attacker-controlled location.file cannot read in-project credential JSON.
Adds regression tests for the SSRF-redirect and symlink/.json vectors (all 5 reproduced E2E before fixing, re-verified closed after).
…sinks
Deep review of the bundle found the per-tool zod regexes were only enforced at the HTTP layer: registry.invokeTool() passed raw params straight to execute(), so flow-execute / flow-add-step / run-sequence bypassed every injection guard. E2E-proven: a flow YAML reached stop-metro's execSync with a string port and ran a host command (RCE), and launch-app's adb-shell with an unescaped bundleId.
- registry.invokeTool: validate params against the tool's zodSchema on EVERY dispatch path (params ?? {} mirrors the HTTP layer so no-arg internal invokes still work); pass the parsed data to services()/execute(). Closes the host-RCE and the adb command-injection at the root, plus every other zod-only guard.
- stop-metro: execSync -> execFileSync (no shell), defence-in-depth.
- adb: add shellQuote(); single-quote-escape bundleId/activity/component in launch-app + restart-app android adbShell calls (same pattern open-url already uses), defence-in-depth.
- preview /simulator-server/:udid: this endpoint is reachable without the auth token (browser UI); bind the spawn to a currently-listed device so an unauthenticated caller can't spawn unbounded simulator-server processes with arbitrary ids (DoS) or inject argv.
- http: tighten the /preview auth exemption to an exact /preview or /preview/-prefixed match (closes the /previewX footgun).
Tests: +4 registry tests guarding invokeTool zod enforcement, +2 http-auth tests for the /preview boundary; updated 12 network/metro integration invokeTool calls to pass the now-enforced required device_id (a real HTTP/MCP caller already had to). All reproduced E2E before fixing and re-verified closed; build + 523 tool-server + 61 registry + 1 tools-client green.
Follow-up hardening from the deep review (all reproduced, then re-verified closed): - registry source-map SSRF twin (react-profiler metro/source-map.ts): the CPU-hotspot resolver fetched an app-controlled sourceMapURL with no allowlist and follows redirects — structurally identical to the debugger registry hole already fixed. Now gated by isAllowedSourceMapURL + redirect:"error". - isAllowedSourceMapURL: additionally require a .map pathname, so an attacker-set sourceMapURL can at most GET a loopback *.map path, not an arbitrary local endpoint (query strings unaffected — they live in .search). - readCappedJson: cap source-map response bodies (content-length + streamed bytes, 64 MiB) before JSON.parse to prevent a malicious loopback responder OOM-ing the tool-server; used by both source-map fetch sites. - profiler-load: session_id is interpolated into on-disk paths; constrain it (zod .regex + assertSafeSessionId in loadReactSession/loadNativeSession) so it can't traverse out of the debug dir. - flows setActiveProjectRoot: reject project_root containing ".." segments (path.join collapses them and would relocate the validated flows dir/file). - launcher writeState: publish atomically (tmp + chmod 0600 + rename) instead of unlink+writeFile, removing the brief absent-file window a concurrent reader could hit while still guaranteeing the token is never written world-readable. - simulator-server spawn: reject device ids not matching a safe charset (rejects leading '-' / shell-ish chars) — defence-in-depth at the spawn sink, covering every caller (preview + gesture tools) independent of zod. Tests: +5 source-maps-ssrf (.map requirement + readCappedJson cap), +3 flow-utils (setActiveProjectRoot). Full build + 531 tool-server + 61 registry + 1 tools-client green; B/C/D/E E2E-proven via the live HTTP API + an ensureToolsServer harness; verification swarm PASS (no regression, real Metro/.map URLs, timestamp session ids, iOS UDIDs/Android serials all still accepted).
Resolve import conflict in argent-mcp/src/mcp-server.ts by taking the union of both sides: AUTH_TOKEN_ENV from this branch plus the screenshot-diff content helpers from main. main also added a typecheck:tests gate that now covers this branch's new tool-server tests, surfacing two latent type errors which are fixed here: http-auth/http-host-validation now use a top-level 'import supertest' (the typeof import(...).default form does not resolve under the gate), and source-resolver-symlink passes the required 'column' field on SourceLocation.
latekvo
marked this pull request as ready for review
June 3, 2026 13:56
latekvo
added a commit
that referenced
this pull request
Jun 3, 2026
…#296) ## Summary Five issues found in the post-merge code review of #249 (Linux support). All confirmed present on main; none fixed by #194. ### H1 — `awaitFirstRealFrame` timeout leaves emulator running `awaitFirstRealFrame` is called after the cold-boot `try/catch` closes. On a SurfaceFlinger timeout the emulator process stayed alive: `inFlightBoots.finally` cleared the map entry, so the next `boot-device` call found the stale serial via the `alreadyRunning` fast path, had to run `assertScreencapAlive`, killed it there, and then fell through to another full cold boot — doubling latency and surfacing a confusing "wedged framebuffer" error instead of the original timeout. **Fix:** wrap the `awaitFirstRealFrame` call in a try/catch that calls `killEmulatorQuietly` before re-throwing. ### H2 — `awaitFirstRealFrame` budget not clamped against `overallDeadline` Every `attemptBoot` stage clamps via `Math.min(params.attemptDeadline, …)`. `awaitFirstRealFrame` received the fixed `STAGE_BUDGET.firstRealFrame` (90 s) with no knowledge of `overallDeadline`, allowing total wall time to silently exceed `bootTimeoutMs` by up to 90 s — violating the documented contract and the Zod schema's `max(900_000)`. **Fix:** pass `Math.min(STAGE_BUDGET.firstRealFrame, Math.max(0, overallDeadline - Date.now()))`. ### H3 — `ARGENT_SIMULATOR_SERVER_DIR` semantic changed without migration hint The env var previously pointed directly at the directory containing `simulator-server`; it now points at the root of the per-platform tree. A user with the old flat layout gets a bare "binary not found" with no explanation of what changed. **Fix:** detect the old flat layout and include a migration hint in the error message; update the env var's doc comment. ### M4 — `ARGENT_EMULATOR_NO_WINDOW=false` silently enables headless mode `selectExtraEmulatorArgs` used a deny-list (`!== "0"`) so `"false"` and `"no"` — natural shell idioms for "disabled" — both mapped to `-no-window`. Test suite covered `""`, `" "`, `"0"` but not `"false"`. **Fix:** switch to an allowlist `["1", "true", "yes"]`; add `"false"` and `"no"` to the parameterized off-test. ### M5 — `bootstrapDylibPathTcp` / `nativeDevtoolsDylibPathTcp` missing `requireDarwin` All six non-TCP dylib/binary exports gained explicit `requireDarwin()` guards in #249. The two TCP dylib exports did not. A Linux caller gets "dylib not found" instead of "requires macOS host". **Fix:** add `requireDarwin()` as the first statement in both TCP dylib functions; add a test. ### S7 (bonus) — `selectGpuMode` validation fires mid-function after slow I/O `selectGpuMode()` and `selectExtraEmulatorArgs()` were called inside deep nested code after several seconds of I/O. A typo in `ARGENT_EMULATOR_GPU_MODE` surfaced late with a spurious "emulator has been terminated" suffix appended. **Fix:** call both once at the top of `bootAndroidImpl`, just after `ensureDep`, and reuse the captured values — eliminates the late error and the redundant double-call. ## Test plan - `npm run test -w @argent/tool-server` — **847 passed** (was 845; +2 for `false`/`no` NO_WINDOW cases) - `npm run test -w @argent/native-devtools-ios` — **10 passed** (was 8; +2 for TCP dylib darwin gate and migration hint)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human Summary
This PR aims to fix bugs that are benign for now, could become serious in the future:
tool-serverMCP/CLI <-> tool-serverpath (bug details discussed in person). After the other fixes this is somewhat redundant, but doesn't hurt and protects against theoretical access attempts from within other processes running on the user's device.AI Summary
Details
Bundles six independent fixes that together close a CSRF cluster on the local tool-server. The vulnerabilities chain because the HTTP API has no auth and serves wildcard CORS, so any web origin (Firefox unconditionally; Chrome via PNA-disabled or loopback-served) can drive the tool-server from a cross-origin tab. Each individual fix lives on its own branch (linked below) and stands alone — this PR exists so the cluster can be reviewed and shipped as one piece.Fixes
fix(http): require auth token + drop wildcard CORS— per-process 32-byte hex Bearer token, generated by the launcher, persisted in~/.argent/tool-server.jsonwith mode0600, required on every route except/preview. Constant-time compare. Wildcard CORS dropped. Threaded through MCP server + CLI + tools-client. Backwards-compat: whenARGENT_AUTH_TOKENis unset (e.g.npm run dev), the server logs a one-shot warning and skips the gate.fix(http): reject non-loopback Host headers— middleware (before the auth gate) returns 403 for any request whose Host is not127.0.0.1,localhost, or::1. Closes DNS-rebinding bypass.fix(ios-profiler-start): prevent shell injection via device_id—detectRunningApp(udid)no longer interpolatesparams.device_idintoexecSynctemplate strings. Bothxcrun simctl spawn ${udid} …and the pipedxcrun simctl listapps ${udid} | plutil …are rewritten toexecFileSyncarray form, with the pipe toplutilhappening in JS (input option) rather than the shell.fix(flows): validate name to close path-traversal in getFlowPath—getFlowPath()requires^[A-Za-z0-9_\\-]+$and verifiespath.relative(getFlowsDir(), filePath)does not start with... Centralizing the check ingetFlowPathcovers every caller (start-recording, add-step, insert-echo, finish-recording, run, read-prerequisite) without per-tool churn.fix(source-resolver): reject paths outside projectRoot + non-source extensions—readSourceFragment()previously accepted anyfilefield on the SourceLocation it was given (the field traces back to a React fiber's_debugSource.fileName, which is attacker-controllable). Now: containment check viapath.relative(projectRoot, abs)plus an extension allowlist (.js/.jsx/.ts/.tsx/.mjs/.cjs/.json).fix(source-maps): reject non-loopback URLs (close SSRF)—doRegister()validatessourceMapURLbefore fetching: onlyhttp:///https://URLs whose hostname is loopback. Closes a blind-SSRF probe of the host network (cloud metadata at169.254.169.254, intranet pages, etc.).Individual branches
fix/auth-token-drop-corsfix/host-header-validationfix/profiler-command-injectionfix/flow-name-validationfix/source-resolver-traversalfix/source-map-ssrfIf you want to land them as separate PRs, the integration branch can be dropped after each lands.
Test plan
Unit tests (already in CI):
npm run buildpasses (workspace tsc clean)npm run test -w @argent/tool-server— 510 passed (was 473 onmain), +37 new tests across:test/http-auth.test.ts— 10 tests (auth, CORS removal, dev-mode escape hatch)test/http-host-validation.test.ts— 7 tests (loopback allow, DNS-rebinding reject)test/flows/flow-utils.test.ts— +5 tests (path-traversal name rejection)test/metro/source-resolver.test.ts— +5 tests (containment + extension allowlist)test/metro/source-maps-ssrf.test.ts— 10 tests (loopback-only allow)Live end-to-end (run locally against this branch's bundled artifacts):
~/.argent/tool-server.jsoncontains a 64-char hex token and is mode0600GET /toolswithoutAuthorization→ 401GET /toolswith wrong token → 401GET /toolswith correct token → 200 (61 tools)Host: attacker.example→ 403Access-Control-Allow-Originheader on responsesdevice_id=\"x;touch /tmp/marker;#\"does NOT create the marker file (Vuln 4 closed)flow-start-recordingwithname=\"../../../tmp/escape\"rejected withInvalid flow nameflow-start-recordingwith a valid name still workstools-client.fetchTools()andtools-client.callTool('list-simulators')succeed via the auth-token threading (the path the MCP server uses internally)startMcpServer) end-to-end:initialize→tools/list(61 tools) →tools/call('list-simulators')returns content. Confirms the launcher → token → fetch → tool-server flow is intact for the productionargent mcppath.Notes for reviewers
/previewis intentionally exempt from the auth gate (it was not gated before; tightening it deserves a separate PR).ARGENT_AUTH_TOKEN-unset escape hatch keepsnpm run devworkable; productionargent mcpalways passes a token via env, so the warning never fires there.ensureToolsServerfromPromise<string>toPromise<{ url, token }>. The two in-tree callers (argent-mcp/mcp-server.ts,argent-tools-client/tools-client.ts) were updated; external consumers usingARGENT_TOOLS_URLshould also setARGENT_AUTH_TOKENto match the server.