Security audit: critical path-traversal delete, injection-guard bypass, and 3 more (multi-day deep dive)

[TheAmericanMaker]

Disclosure: filed by an AI coding agent operating on behalf of James Sesler (TheAmericanMaker), sharing the results of a multi-day deep-dive audit of this codebase at d75821e0747d1a6a09c277ee14d14d0446f3b7a7.
The audit was produced with CodeCartographer, James's open-source audit tool (https://github.com/HuginnIndustries/CodeCartographer): it reconstructs a codebase's intent, runs mechanical and semantic defect scans, then forces every candidate finding through claim-by-claim verification (path-join and regex simulations, type cross-references) before it is reported. Three findings were corrected or withdrawn during verification, so everything below has been re-checked against the source rather than pattern-matched.

Severity tags are the auditor's assessment.

1. [critical] DELETE /api/channel-packages/:name — path traversal to rmSync on arbitrary directories, up to /

Evidence: server/src/api/channels.ts:282 accepts decodeURIComponent(req.params.name) with no validation. server/src/channels/loader.ts:236-237 then does path.join(dir, "node_modules", packageName) → existsSync(target) → rmSync(target, { recursive: true, force: true }). The preceding npm remove <name> fails for traversal names and its error is swallowed (loader.ts:231-235), so execution always reaches rmSync. Simulation (node path.join with decoded name): ../.. → /data (portal.db, sessions, agent home); ../../.. → / — three hops to deleting the whole filesystem. Auth-gated only when PORTAL_PASSWORD is set (server/src/auth.ts:16-24 warns and disables auth otherwise); docker-compose.portainer.yml runs as root (no user: directive).

Impact: Remote deletion of the portal data volume or the entire container filesystem.

Suggested fix: validate name against a package-name regex (/^(@[a-z0-9-_.]+\/)?[a-z0-9-_.]+$/), require the resolved path to start with the node_modules dir + separator, and confirm the target exists inside that prefix before deleting. Add a test with traversal names asserting 400 + no filesystem change.

2. [high] Injection guard skips taint on error results — payload delivery without triggering blast-radius rules

Evidence: server/src/pi/guard.ts:238 — if (!untrusted || event.isError) return undefined;. Tool results from untrusted sources (bash curl/wget, any MCP tool) that carry isError: true skip the taint envelope entirely. Error bodies are attacker-controlled in practice (HTTP error pages echo URLs; MCP error strings carry remote text), so injected text reaches the model unmarked and the session never becomes tainted.

Impact: The guard's core premise ("once tainted, dangerous actions are refused") can be evaded by delivering the payload on an error path.

Suggested fix: envelope/defaced-mark error content too, or at minimum set tainted = true for untrusted error results. Add a guard test: untrusted result with isError: true still produces the envelope and taints the session.

3. [medium] Guard coverage gaps: clone/install verbs untainted, persistence-surface and --json blind spots

Evidence (regex-verified):

• UNTRUSTED_COMMAND (guard.ts:30) matches mail readers + curl|wget|lynx|w3m only — git clone, git fetch, npm install, pip install, ssh, scp all return false, so fetching an attacker's repo (a canonical injection vector) never taints the session.
• write-to-path (PATH_DIRS, guard.ts:51) covers only /data/bin | /usr/local/bin | /usr/bin | /usr/local/sbin with a trailing slash required. Two bypasses: dir-as-destination without slash (cp evil /usr/bin) and destinations outside the list (echo x > /etc/cron.d/evil). persist (guard.ts:96-102) matches crontab | systemd-run | at now commands but not file writes into /etc/cron.d, /etc/systemd/system, ~/.config/autostart, or shell rc files.
• The upload rule (guard.ts:73-78) misses curl --json (POST-body upload matches nothing). -G is not a gap — it only exfiltrates with -d/--data, already matched; -d@file/--data-binary are also already caught.
• Cosmetic note: id_[re]d?sa misses bare id_ecdsa/id_ed25519 references, but the .ssh/ alternation covers every realistic read path; hardening only.

Impact: The easy paths for the most common fetch verbs and for scheduled/boot persistence stay open, widening what taint rules were meant to close.

Suggested fix: one focused PR — extend UNTRUSTED_COMMAND with clone/fetch/install verbs; add cron/systemd/autostart/rc paths to the write rule and drop the trailing-slash requirement for destination-directory forms; add --json to the upload rule. Tests per added form.

4. [medium] Missing param validation on skills handlers (locate/enabled/update/PUT/delete)

Evidence: server/src/api/skills.ts:201-211 (locate()) and :271 (/skills/:name/enabled) join the raw URL param into path.join(skillsRoot(), name, ...) with no isValidSlug check, while POST /skills enforces it (:243). PUT/DELETE go through locate().

Impact: Constrained but real — a traversal name reaching a directory containing a SKILL.md/SKILL.md.disabled can rename, overwrite, or recursively delete files outside the skills root; inconsistent with the create path.

Suggested fix: apply isValidSlug (or resolve-and-prefix check) to req.params.name in all four handlers; tests with traversal names.

5. [medium] Open-portal default, no login throttling, no CSP

Evidence:

• server/src/auth.ts:12-24: PORTAL_PASSWORD unset ⇒ auth disabled with a warning only — every /api route open (including prompt, which runs arbitrary commands). docs/guide/deploying.md:99 says everything is optional except the password; the code disagrees.
• server/src/index.ts:68-75: POST /api/auth/login has no rate limiting/lockout; the shared password is the portal's only credential.
• No Content-Security-Policy anywhere (no CSP middleware; web/index.html has none). XSS is mitigated by React escaping + react-markdown defaults, so this is defense-in-depth.
• Not filed separately: channel secrets stored plaintext in SQLite (redacted in API, write-only from browser) — documented deployment assumption.

Suggested fix: refuse non-loopback binds when auth is disabled unless an explicit ALLOW_OPEN=1 flag is set; per-source throttle/lockout on login; CSP header for the static UI; document secrets-at-rest in the deploy docs.

---

Reproducing

All refs were verified at d75821e. The traversal is a one-liner: node -e "const p=require('path'); console.log(p.join('/data/channels','node_modules','../../..'))" prints /. The guard-regex findings were checked against the verbatim patterns in server/src/pi/guard.ts: UNTRUSTED_COMMAND matches wget but not git clone / pip install; PATH_DIRS blocks cp evil /usr/bin/ but not cp evil /usr/bin or echo x > /etc/cron.d/evil; the upload pattern misses curl --json (and -G is covered only via its mandatory -d). decodeURIComponent(req.params.name), event.isError, and the isValidSlug call sites are at the lines cited above.