mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
flow-visual-settle
14 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 filip131311
|
66e051fe8b |
fix(debugger): collapse a device's two ids onto one debugger instance (#528)
## The problem
A device is reached through **two id namespaces** that name the same
thing but are different strings:
- the **stable id** the caller connects with — an iOS UDID / Android
serial / Vega serial from `list-devices`;
- the **`logicalDeviceId`** Metro's inspector-proxy echoes back for that
connection (an opaque per-connection handle).
`logicalDeviceId` is not derived by Argent — it is echoed verbatim from
Metro (`js-runtime-debugger.ts`), and Metro never sees the UDID, so
there is **nothing to join on synchronously**.
The debugger service is cached by its URN, which embeds `device_id`
verbatim:
```
JsRuntimeDebugger:<port>:<device_id>
```
`debugger-connect` took the UDID, returned the logicalDeviceId, and its
description told callers to *"forward it as device_id to all subsequent
debugger-\* calls."* Following that instruction produced a **different
URN** → a **second `JsRuntimeDebugger` instance** → a second
`CDPClient`, a second console-log server, and a split log file — **all
for one device**. The second attach also kicks the first off Metro's
single-debugger-per-target proxy. Nothing deduplicated them.
It only *looked* fine because with a single device on a Metro port,
`selectTarget`'s single-device fallback quietly connects anyway; the
split surfaces as "the debugger randomly drops" and logs landing in the
wrong bucket.
Secondary damage the same split caused:
- `classifyDevice()` is shape-based and was fed the logicalDeviceId → an
iOS device's forwarded id classified as **Android** in the capability
gate + telemetry.
- The docs asserted *"iOS simulator UDID, Android logicalDeviceId"* as
if a UDID were the iOS logicalDeviceId — the repo's own test says the
opposite.
- `debugger-component-tree` printed the logicalDeviceId under a `udid:`
label — teaching callers the wrong name.
## The fix — synchronous, within the existing `services()` design
- **Learn an alias** `logicalDeviceId → connect-id` as a side effect of
a successful connect (the one place both ids are known at once; see
`utils/debugger/device-alias.ts`), and **canonicalize `device_id`
through it at every URN-building site**. A forwarded logicalDeviceId now
resolves to the already-open instance; unknown ids (the connect id
itself, Chromium ids) pass through unchanged, so it is safe to call
everywhere. **No Metro round-trip on the hot path — `services()` stays
synchronous.** The alias is 1:1 (a logicalDeviceId is unique per
connection) and is cleared on dispose.
- **Rewrite the `device_id` docs** across `debugger-*`, `network-*`, and
`react-profiler-*`: pass the **same list-devices id you connected
with**; the returned logicalDeviceId is informational (forwarding it
still resolves, via the alias). Removes the instruction that caused the
double instance and the false "UDID = logicalDeviceId" wording.
- **Fix the `debugger-component-tree` header** (`udid:` →
`logicalDeviceId:`).
Covered call sites (all that embed `device_id` into a Metro URN):
`debuggerServiceRef` (connect/evaluate/status/log-registry),
`debugger-reload-metro`, `debugger-component-tree`,
`debugger-inspect-element`, `react-profiler-start` (+ its session URN),
`react-profiler-stop`, `network-logs`, `network-request`.
## Verification
**Live**, against a real iOS sim + Metro (my-app, RN 0.81.5), driving
the branch tool-server directly:
| step | result | URNs instantiated |
|---|---|---|
| `debugger-connect { device_id: <UDID> }` | returns `logicalDeviceId:
d52ad36b…` | `JsRuntimeDebugger:8081:<UDID>` |
| `debugger-evaluate { device_id: d52ad36b… }` (forward the
logicalDeviceId) | `42` ✅ | **no new URN — reused** |
| counterfactual: `debugger-evaluate { device_id: some-unaliased-id }` |
`42` ✅ | **`JsRuntimeDebugger:8081:some-unaliased-id` — a distinct 2nd
instance** (the pre-fix behavior, reproduced) |
| `debugger-component-tree { device_id: <UDID> }` | header reads
`logicalDeviceId: d52ad36b…` ✅ | reused |
**Unit** — new `test/metro/device-alias.test.ts` covers the alias (learn
/ canonicalize / self-alias no-op / missing-id / forget) and that
`debuggerServiceRef` yields the same URN whether called with the UDID or
the forwarded logicalDeviceId. Full tool-server suite green (2726
tests).
## Not in scope
Renaming the `device_id` param to `udid` (or vice-versa) — that is a
breaking MCP wire change. This PR is additive: a forwarded
logicalDeviceId keeps working, it just no longer opens a second
connection.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ignacy Łątka
|
af8476c0a9 |
feat(vega): support the JS-runtime debugger and network inspector on Fire TV (#502)
Vega's React Native is a fork of **RN 0.72**, so it serves the *legacy* Hermes inspector rather than the post-0.73 (Fusebox) debugger. The `debugger-*` / `view-network-*` / `react-profiler-*` tools were never gated for it, so every call against a VVD was rejected before any RN code ran: ``` Tool 'view-network-logs' is not supported on vega vvd (no vega support declared). ``` That gate turned out to be the only thing in the way for most of them. The concern that prompted this was that the network inspector needs the new debugger. It does not: argent's network inspector never touches the CDP `Network` domain, it monkey-patches `fetch` over `Runtime.evaluate` and reads back a global array. `Runtime.evaluate` has worked in Hermes since long before 0.73, so the network inspector is precisely the tool that is *unaffected* by the missing new-debugger stack. ## What now works on Vega Gated in via `DEBUGGER_TOOL_CAPABILITY` (these need only `Runtime.evaluate`): `debugger-connect` · `debugger-status` · `debugger-evaluate` · `debugger-log-registry` · `view-network-logs` · `view-network-request-details` ## What stays gated off, and why `debugger-component-tree` · `debugger-inspect-element` · `debugger-reload-metro` · `react-profiler-*` · `profiler-*` Two different reasons, and it is worth keeping them apart: - **`debugger-component-tree` and `debugger-inspect-element` are hard-blocked.** They are the only two callers of `cdp.evaluateWithBinding`, i.e. they deliver their payload over `Runtime.addBinding` / `Runtime.bindingCalled`. RN 0.72's Hermes **ACKs `Runtime.addBinding` and never installs the binding** — confirmed on the VVD: after a successful `debugger-connect`, `typeof __argent_callback` is still `"undefined"`, and no `bindingCalled` ever fires. They would hang until timeout. - **`debugger-reload-metro` and the profiler tools are simply unverified here.** They do *not* use the binding (`reload-metro` is `Page.reload` plus an HTTP `/reload` fallback; the profilers ride `Runtime.evaluate` and the CDP `Profiler` domain). They share `RN_ONLY_TOOL_CAPABILITY` and stay out of scope rather than ship untested. Either way the capability gate turns a hang (or an unknown) into an immediate, explicit "not supported". `describe` remains the way to read on-screen structure on Vega. Worth noting for a follow-up: the component-tree data itself *is* reachable on RN 0.72 — walking the DevTools hook via a plain `Runtime.evaluate` returns the real fiber tree (640 fibers on the sample app). Component-tree is blocked by argent's transport choice, not by a missing runtime capability, so a non-binding fallback would unlock it. ## Two bugs this surfaced **A Vega `device_id` could bind to another device's runtime.** Vega's legacy inspector-proxy emits no `reactNative` block at all, so its targets carry no `logicalDeviceId`. `selectTarget` both matched `device_id` on `logicalDeviceId` *and* counted connected devices the same way — so a Vega device was invisible to the multi-device guard from #397: its id matched no target, the guard counted zero devices, concluded there was nothing to disambiguate, and fell through to the priority target. With a Fusebox iOS/Android app on the same Metro, that target wins the priority list, so `debugger-evaluate` would have run JS **in the wrong app** while `debugger-status` reported the other device's name and `isNewDebugger: true`. Devices are now keyed by `logicalDeviceId || deviceName` (the legacy proxy always reports a name), so the guard sees them and refuses to guess. A lone VVD still falls back to its own targets. This is not Vega-specific: any RN ≤ 0.72 device was invisible the same way. **Metro's missing project-root header.** RN 0.72's Metro never sends `X-React-Native-Project-Root`, and `discoverMetro` treated a missing header as fatal — which took down evaluate, console logs and the network inspector too. The header only feeds source-map / `file:line` resolution, which degrades cleanly to no-location, so it is now optional. Platform-agnostic, not a Vega special-case: a missing source-map hint should never kill an otherwise working debugger session. That made `projectRoot: ""` a reachable state, which two source-resolution paths mishandled. `symbolicate` stripped the root with `file.replace(projectRoot + "/", "")`, which on an empty root degrades to `.replace("/", "")` and deletes the first slash *anywhere* in the path (`src/screens/Home.tsx` → `srcscreens/Home.tsx`). And `readSourceFragment` did fail closed, but only because `fs/promises.realpath("")` happens to reject where the sync API returns the tool-server's cwd — it now bails explicitly instead of leaning on that quirk. Both are reachable from `debugger-inspect-element` on any legacy Metro (not on Vega, where it is gated off). ## Verification Driven end-to-end against a real VVD (`amazon-6b8a…`) running the `react-native-multi-tv-app-sample` Vega app (Debug `.vpkg`, Metro reachable via reverse port-forward), through a tool-server built from this branch: ``` debugger-status -> connected: true, deviceName: kepler-device, isNewDebugger: false, projectRoot: "" debugger-evaluate -> "hermes=object fetch=function global=object" debugger-log-registry -> captured console.log "PR502_E2E_MARKER" === NETWORK LOGS (page 1/1, 2 total) === {id: rn-net-1} "GET /todos/7" 200 Fetch 98 B 55 ms {id: rn-net-2} "GET /users/3" 200 Fetch 520 B 17 ms view-network-request-details -> full request/response headers, status, timing ``` And the gated tools fail fast (0.02s) rather than hanging: ``` debugger-component-tree -> not supported on vega vvd debugger-inspect-element -> not supported on vega vvd debugger-reload-metro -> not supported on vega vvd react-profiler-start -> not supported on vega vvd ``` Re-ran the whole sequence against the fixed target-selection to confirm the single-VVD path still connects. Also run locally: `tsc --build`, `tsc --noEmit -p tsconfig.test.json`, `eslint . --max-warnings 0`, `prettier --check`, `grade-skills`, and the tool-server unit tests (199 files / 1740 tests). Tests added: `vega-target-routing` (the mis-route, using a target payload captured verbatim from the live VVD), `source-resolver-empty-root` (both empty-root paths), and `vega-debugger-capability` — which now drives the real `ToolDefinition.capability` fields and `resolveDevice` instead of a hand-written name↔constant pairing, pinning all 6 enabled and all 14 gated tools. The old version passed even with `debugger-status` wired to the wrong capability constant. Scope note: VVD only — no tool declares `vega: { device: true }`, so physical Fire TV hardware is untouched. |
||
|
Ignacy Łątka
|
71344c40b9 |
fix(debugger): stop unmatched device_id silently collapsing onto one device (#397)
## Problem Opening debugger sessions with **different** `device_id`s could all resolve to the **same** physical device. `selectTarget` filters Metro's `/json/list` CDP targets by `reactNative.logicalDeviceId === device_id`, but when the provided `device_id` matched **no** target it kept *all* candidates and fell through to the first/priority target: ```ts const filtered = candidates.filter((t) => t.reactNative?.logicalDeviceId === options.deviceId); if (filtered.length) candidates = filtered; // unmatched id → silently ignored ``` This silently no-ops surprisingly often: Metro's `logicalDeviceId` is an internal hash, **not** the iOS simulator UDID. Verified live against a real device: - simulator UDID: `6DBF83B4-F341-4F8D-B48D-CD8FF312CCFB` - Metro `logicalDeviceId`: `a1956f6bd1e3a3ac3dbb505e24d79687a5ea60b1` With one device the fallback is harmless (it picks the only device). With **several** devices on one Metro, every unmatched `device_id` collapses onto whichever device Metro lists first — so two debugger sessions opened with different `device_id`s drive the same device. ## Fix In `selectTarget`, only fall back to the priority target when there is **at most one distinct device** to fall back to. When multiple distinct devices are connected and the `device_id` matches none, throw an error that lists the valid `logicalDeviceId`s so the caller can re-target instead of being silently misrouted. The single-device convenience path (pass any id — including a UDID — and get the one connected device) is preserved. ## Verification - `selectTarget` unit tests: distinct ids → distinct devices; ambiguous unmatched id throws and surfaces the valid ids; single-device and no-`logicalDeviceId` fallbacks still work. - Full-stack multi-device test driving `Registry → JsRuntimeDebugger → debugger-connect` against a two-device mock Metro: distinct `device_id`s route to distinct devices; unmatched id is rejected. - Full `tool-server` suite: **1203 passed**. - Confirmed the real contract on a live iOS sim: `debugger-connect` with the UDID still connects (single-device fallback) and returns `logicalDeviceId a1956f…`. ## Notes The three RN-only debugger tools (`debugger-component-tree`, `debugger-inspect-element`, `debugger-reload-metro`) build their Metro service ref via a raw template literal instead of `debuggerServiceRef()`. For the Metro path the resulting URN is byte-identical, so it's not part of this bug; left untouched to keep the fix scoped. |
||
|
Ignacy Łątka
|
e2e21c9ce4 |
fix(debugger): return object/array results from debugger-evaluate (#365)
## Problem `debugger-evaluate` silently returns nothing for any expression that evaluates to an object or array. `CDPClient.evaluate` calls `Runtime.evaluate` without `returnByValue`, so a non-primitive result comes back as a RemoteObject reference with no `value` field, and the tool drops it (only primitives/strings came through). ## Fix Pass `returnByValue` + `awaitPromise` (default on) so objects/arrays serialize by value and promise-returning expressions resolve to their value. `evaluateWithBinding` opts both off to keep its fire-and-forget binding path unchanged (it delivers its payload via a side binding, not the evaluate return). ## Verification - New cdp-client unit tests assert `evaluate` sends `returnByValue:true` + `awaitPromise:true` and returns an object result, and that `evaluateWithBinding` sends both `false`. - Full cdp-client + reconnect suites pass; 1254 tool-server tests pass; eslint + prettier clean. - The real-Hermes `returnByValue` path is already exercised in production by `react-profiler-fiber-tree` / `react-profiler-renders`. |
||
|
Ignacy Łątka
|
fa5af59931 |
refactor: replace hand-rolled parsers with standard packages (#373)
## What & why
Swept the repo for hand-rolled regex/string parsers that reimplement a
solved problem, and replaced the genuinely fragile ones with
battle-tested packages (or deps already in the tree). Each swap is
paired with a regression test for an input the old code got wrong.
| Site | Was | Now |
|------|-----|-----|
| `adb.ts` `resolveAvdPath`, `linux-preflight.ts` `readMb` | per-key
anchored regex over AVD `.ini` / `config.ini` | **`ini`** |
| `workspace-reader.ts` `extractEnvKeys` | split-on-first-`=` line loop
| **`dotenv`** (`parse`) |
| `uninstall.ts` `readBundledSkillName` | `^name:(.+)$` capture +
outer-quote strip | **`yaml`** (already a dep) |
| `mcp-configs.ts` `escapeRegExp` | hand-rolled char-class escape |
**`escape-string-regexp`** |
| `export.ts` xctrace version, `session-context.ts` RN minor |
`match(/(\d+)\./)` / `split(".")[1]` | **`semver`** `coerce` (already a
dep) |
| `source-resolver.ts` `parseDebugStack` | `at fn (file:l:c)` regex
(required both line+col) | **`stacktrace-parser`** |
### Bugs these fix (covered by new tests)
- **ini**: a quoted `path = "/Users/My Name/.../X.avd"` kept its quotes,
failed the `startsWith("/")` guard → AVD unresolved. Inline comments /
trailing junk after a sizing value made the anchored regex return `null`
→ sizing warning silently lost.
- **dotenv**: `export FOO=bar` produced the key `"export FOO"` (dropped
by the identifier filter); a multi-line quoted value leaked its
continuation lines (`FAKE=...`) as phantom keys.
- **yaml**: a `name:` value with a trailing `# comment` was taken
verbatim (comment included); odd quoting only had one outer pair
stripped.
- **stacktrace-parser**: a `:line`-only frame (no column, as Hermes/JSC
can emit) became an unusable `unknown` frame.
## Deliberately left alone (not "flaky", or no safe swap)
To avoid churn and regressions, I did **not** touch:
- **`uiautomator-parser.ts`** hand-rolled XML parser — a deliberate,
documented choice with a dedicated hardening suite, including a
*leniency* test for malformed dumps (stray `</node>`). A spec XML parser
would reject inputs this is built to recover from.
- **`ios-profiler/pipeline/xml-parser.ts`** (xctrace XML) — the most
tangled parser, but it has no real `.trace` fixtures and only thin
entity-decode tests. A rewrite can't be proven equivalent without
device-generated traces, so swapping it blind risks silently breaking
iOS CPU/hang/leak reports. Best done separately with a captured fixture.
- **`http.ts` `extractHostname`** — DNS-rebinding guard; `URL.hostname`
keeps the IPv6 brackets the loopback check compares without, so the
"fix" isn't simpler and the path is security-sensitive.
- **`adb.ts` `shellQuote`** — the POSIX single-quote escape is the
standard correct form.
- **`parseAdbDevices`, `android-screen.ts` `wm size`, `detect-app.ts`
dumpsys** — tool-specific CLI-output scraping with no clean package
equivalent (and documented edge-case handling).
- **`workspace-reader.ts` `extractMetroPort`** — a best-effort heuristic
over JS config; a correct replacement needs an AST parse, out of scope
here.
- **`config-parse.ts` Yarn/npm age-gate parsing** — a precise,
unit-tested domain parser; `ms` would introduce a bare-number ambiguity.
## Verification
- `tsc --build` clean across the workspace graph.
- Tests: tool-server **1205**, installer **264**, update-core **31** —
all pass (incl. the new regression tests).
- `prettier --check` clean on all changed files.
- Production esbuild bundle (`bundle-tools.cjs`) emits a valid
`tool-server.cjs`/`mcp-server.mjs` with
`ini`/`dotenv`/`stacktrace-parser` inlined (`node --check` OK). *(The
script's final step errored only on the auth-gated
`trace-processor.wasm` download, unrelated to bundling.)*
New runtime deps: `ini`, `dotenv`, `stacktrace-parser` (tool-server),
`escape-string-regexp` (installer) — all small, pure-JS. Lockfile change
is purely additive.
|
||
|
Ignacy Łątka
|
9c43da3b56 |
fix(tool-server): fall back to raw locations when inspect-element symbolication fails (#284)
## Problem
`debugger-inspect-element`, called with the default `resolveSourceMaps:
true`, returned `source: null` **and** `code: null` for **every** item
whenever Metro symbolication did not yield a usable original location.
This made the tool effectively useless in its default mode — the caller
got a list of component names with no file:line and no code fragment to
edit.
## Root cause
Two independent issues compounded:
1. **No raw fallback in the tool.** In `debugger-inspect-element.ts`,
the `resolveSourceMaps` branch only assigned `source`/`code` when
`symbolicate()` returned a result. When it returned `null`
(symbolication failed, or was filtered out — see below), the item was
left with `source: null` / `code: null`, **discarding the raw bundled
frame the tool already had** in `item.frame`.
2. **Over-broad `node_modules` rejection in the resolver.** In
`source-resolver.ts`, `symbolicateFrame` did `if
(frame.file.includes("node_modules")) return null;`. That was meant to
drop frames that symbolication couldn't map (where Metro echoes the
bundle URL back). But it also threw away **genuinely-mapped** sources
whose real path legitimately lives in `node_modules` — e.g.
`expo-router` / `react-navigation` route components — turning a
perfectly good resolved location into `null`, which then hit issue (1)
and nulled the whole item.
## Fix
Two minimal, localized changes:
- **`debugger-inspect-element.ts`** — add an `else` to the
`resolveSourceMaps` branch that falls back to the raw bundled frame `{
file: item.frame.file, line: item.frame.line, column: item.frame.col }`,
mirroring the exact shape produced by the `resolveSourceMaps: false`
branch. When symbolication can't map a frame, the caller now still gets
the bundled location instead of `null`.
- **`source-resolver.ts`** (line ~93) — replace the
`includes("node_modules")` guard with `if
(/^https?:\/\//.test(frame.file)) return null;`. A successful
symbolication yields a real file path (kept, including legitimate
`node_modules` route components); a failed one yields the original
`http(s)` bundle URL echoed back (rejected). This keeps the diff to a
single line + comment at line ~93 to stay easy to reconcile.
## Testing
`cd packages/tool-server && vitest run
test/metro/source-resolver.test.ts
test/debugger/inspect-element-filter.test.ts`
Added fail-before / pass-after coverage at two layers:
- **Resolver level** (`test/metro/source-resolver.test.ts`): mocks
`fetch` for `/symbolicate` and asserts that
- a frame mapping to a `node_modules/.../*.js` path is **kept**
(returned that mapped, project-relative location),
- an echoed-back `http(s)` bundle URL is **rejected** (`null`),
- a real app-source path is mapped relative to the project root,
- a thrown request returns `null`.
- **Tool level** (`test/debugger/inspect-element-filter.test.ts`):
drives `debuggerInspectElementTool.execute()` with a fake
`JsRuntimeDebuggerApi` (no live CDP) and asserts that when
`symbolicate()` returns `null`, the item retains the raw `{ file, line,
column }` (and reads no source fragment); when it returns a mapped
location, that location and its code fragment are used.
Verified the fail-before / pass-after cycle by checking out
`origin/main` for the two source files only (keeping the new tests): the
3 new assertions fail on `origin/main` (`source` is `null` for the
raw-fallback case; the node_modules-mapped source is `null`; the echoed
bundle URL is wrongly accepted), then pass once the fixes are restored.
All 34 tests pass with the fix; the 31 pre-existing tests are
unaffected.
`tsc --noEmit -p tsconfig.test.json` reports no errors in any changed
file (the only errors are pre-existing module-resolution artifacts from
the symlinked `node_modules`: `pngjs`,
`@argent/native-devtools-android`, `@argent/update-core`).
## Known limitation / follow-up
User-defined and route component frames that lack `_debugStack` under
**React 19** (where `_debugSource` was removed) are still `null` even in
raw mode, because there is no bundled frame to fall back to in the first
place. That is a separate, riskier change (it requires a different
source for the frame entirely) and is intentionally **out of scope**
here.
## Note for reconcilers
There are other open branches touching `source-resolver.ts`
(`fix/source-resolver-traversal`, `fix/source-map-ssrf`). This change is
deliberately confined to the single guard at line ~93 (plus an
explanatory comment) to keep it trivially mergeable alongside them.
|
||
|
Ignacy Łątka
|
acfe80ab6b |
chore: add ESLint with typescript-eslint (#349)
## What Adds type-aware linting to the workspace. Until now the only static gate on JS/TS was Prettier (formatting) and `tsc` — there was no linter, so the whole class of bugs `tsc` doesn't catch (floating promises, misused promises, throwing non-Errors, unsafe stringification, dead code) went unguarded across ~450 TS files. - **`eslint.config.mjs`** — flat config. `recommendedTypeChecked` runs on the TS sources via an explicit `project` list (each package keeps tests in a separate `tsconfig.test.json` that `projectService` can't auto-discover, so it's listed explicitly). Type-checked rules are disabled for the loosely-typed plain-JS build/dev scripts. The `argent-private` submodule and build artifacts are ignored (mirrors `.prettierignore`). - **`lint` / `lint:fix`** npm scripts. - **`.github/workflows/lint.yml`** — builds the workspace (type-aware rules need the referenced projects' declarations) then runs `eslint .`. Node 24 (ESLint 10 requires `^20.19 || ^22.13 || >=24`). ## Rule calibration This is an adoption PR, so it's deliberately calibrated rather than maximal: - **Kept on (bug-catchers):** `no-floating-promises`, `no-misused-promises` (with `checksVoidReturn: false` to drop the noisy callback variant), `only-throw-error`, `no-base-to-string`, `prefer-promise-reject-errors`, `no-unused-vars` (`_`-prefix escape hatch), plus the ESLint recommended set. - **Parked off as documented debt:** the high-volume style/`any`-family rules — `no-unsafe-*`, `require-await`, `no-unnecessary-type-assertion`, `restrict-template-expressions`, `unbound-method`, `no-explicit-any`, etc. These flag broad classes of pre-existing code and are out of scope here; each is commented as a ratchet target for follow-up passes. All findings from the kept-on rules are fixed in this PR (dead code/imports, error-cause chaining, **3 genuine floating promises**, non-Error throws, unsafe stringification). Stale unused `eslint-disable` directives left over in the tree were removed by `--fix`. ## Verification Locally, all green: `eslint .` (0 errors), `tsc --build`, `typecheck:tests`/`typecheck:scripts`, the full `vitest` suite (no behavior change from the fixes), and `prettier --check`. ## Not included (intentionally) Other static-analysis gates discussed (CodeQL, dependency-audit, ShellCheck, actionlint) are out of scope for this PR. |
||
|
Ignacy Łątka
|
74c2065ca0 |
fix: multiple minor bugs (#194)
## Human Summary
This PR aims to fix bugs that are benign for now, could become serious
in the future:
- multiple CSRF via CORS misconfiguration on `tool-server`
- multiple SSRF that were possible due to the CSRF
- path traversal via flow creator
- multiple shell injections
- lack of auth on `MCP/CLI <-> tool-server` path (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
1. **`fix(http): require auth token + drop wildcard CORS`** —
per-process 32-byte hex Bearer token, generated by the launcher,
persisted in `~/.argent/tool-server.json` with mode `0600`, required on
every route except `/preview`. Constant-time compare. Wildcard CORS
dropped. Threaded through MCP server + CLI + tools-client.
Backwards-compat: when `ARGENT_AUTH_TOKEN` is unset (e.g. `npm run
dev`), the server logs a one-shot warning and skips the gate.
2. **`fix(http): reject non-loopback Host headers`** — middleware
(before the auth gate) returns 403 for any request whose Host is not
`127.0.0.1`, `localhost`, or `::1`. Closes DNS-rebinding bypass.
3. **`fix(ios-profiler-start): prevent shell injection via device_id`**
— `detectRunningApp(udid)` no longer interpolates `params.device_id`
into `execSync` template strings. Both `xcrun simctl spawn ${udid} …`
and the piped `xcrun simctl listapps ${udid} | plutil …` are rewritten
to `execFileSync` array form, with the pipe to `plutil` happening in JS
(input option) rather than the shell.
4. **`fix(flows): validate name to close path-traversal in
getFlowPath`** — `getFlowPath()` requires `^[A-Za-z0-9_\\-]+$` and
verifies `path.relative(getFlowsDir(), filePath)` does not start with
`..`. Centralizing the check in `getFlowPath` covers every caller
(start-recording, add-step, insert-echo, finish-recording, run,
read-prerequisite) without per-tool churn.
5. **`fix(source-resolver): reject paths outside projectRoot +
non-source extensions`** — `readSourceFragment()` previously accepted
any `file` field on the SourceLocation it was given (the field traces
back to a React fiber's `_debugSource.fileName`, which is
attacker-controllable). Now: containment check via
`path.relative(projectRoot, abs)` plus an extension allowlist
(`.js/.jsx/.ts/.tsx/.mjs/.cjs/.json`).
6. **`fix(source-maps): reject non-loopback URLs (close SSRF)`** —
`doRegister()` validates `sourceMapURL` before fetching: only
`http://`/`https://` URLs whose hostname is loopback. Closes a
blind-SSRF probe of the host network (cloud metadata at
`169.254.169.254`, intranet pages, etc.).
### Individual branches
- `fix/auth-token-drop-cors`
- `fix/host-header-validation`
- `fix/profiler-command-injection`
- `fix/flow-name-validation`
- `fix/source-resolver-traversal`
- `fix/source-map-ssrf`
If you want to land them as separate PRs, the integration branch can be
dropped after each lands.
## Test plan
Unit tests (already in CI):
- [x] `npm run build` passes (workspace tsc clean)
- [x] `npm run test -w @argent/tool-server` — **510 passed (was 473 on
`main`)**, +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):
- [x] Launcher spawns tool-server; `~/.argent/tool-server.json` contains
a 64-char hex token and is mode `0600`
- [x] `GET /tools` without `Authorization` → **401**
- [x] `GET /tools` with wrong token → **401**
- [x] `GET /tools` with correct token → **200** (61 tools)
- [x] Request with `Host: attacker.example` → **403**
- [x] No `Access-Control-Allow-Origin` header on responses
- [x] Cross-origin command-injection probe via `device_id=\"x;touch
/tmp/marker;#\"` does NOT create the marker file (Vuln 4 closed)
- [x] `flow-start-recording` with `name=\"../../../tmp/escape\"`
rejected with `Invalid flow name`
- [x] `flow-start-recording` with a valid name still works
- [x] `tools-client.fetchTools()` and
`tools-client.callTool('list-simulators')` succeed via the auth-token
threading (the path the MCP server uses internally)
- [x] MCP-stdio entry point (`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 production `argent mcp` path.
## Notes for reviewers
- `/preview` is intentionally exempt from the auth gate (it was not
gated before; tightening it deserves a separate PR).
- The `ARGENT_AUTH_TOKEN`-unset escape hatch keeps `npm run dev`
workable; production `argent mcp` always passes a token via env, so the
warning never fires there.
- The auth bundle changes the public return shape of `ensureToolsServer`
from `Promise<string>` to `Promise<{ url, token }>`. The two in-tree
callers (`argent-mcp/mcp-server.ts`,
`argent-tools-client/tools-client.ts`) were updated; external consumers
using `ARGENT_TOOLS_URL` should also set `ARGENT_AUTH_TOKEN` to match
the server.
</details>
|
||
|
Filip131311
|
8c2a313c76 |
chore: format codebase with Prettier
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Paweł Fornagiel
|
330a13a2f2 | chore: remove deprecated debugger tools and update documentation (#66) | ||
|
Paweł Fornagiel
|
85ef733835 |
refactor: rename RADON references to ARGENT across the codebase (#56)
* refactor: rename RADON references to ARGENT across the codebase * Merge main into branch and complete ARGENT rename in new files Merges main's changes: modular CLI architecture, UI package deprecation, db tool removal, skills/agents prefix rename, and MCP server consolidation. Extends the RADON→ARGENT rename to cover new files introduced in main (ARGENT_MCP_LOG, ARGENT_TOOLS_URL env vars, argent-skills bin). Made-with: Cursor * fix: rename RADON_MCP_LOG in observe.cjs, regenerate lockfile for argent-skills bin Made-with: Cursor * chore: remove accidental PUBLIC_RELEASE_AUDIT.md |
||
|
Ignacy Łątka
|
c4658ac8fa | fix: invalid imports in test files | ||
|
pFornagiel
|
c86378e6c4 | feat: change radon-lite references to argent | ||
|
pFornagiel
|
dc3d4ec000 | feat: rename tools package to tool-server, minor functionalities renamings |