mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
flow-visual-settle
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
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> |
||
|
Ignacy Łątka
|
c4658ac8fa | fix: invalid imports in test files | ||
|
pFornagiel
|
dc3d4ec000 | feat: rename tools package to tool-server, minor functionalities renamings |