fix(cli): stop uninstall and kill --port destroying what they do not own - #3361
fix(cli): stop uninstall and kill --port destroying what they do not own#3361kovtcharov wants to merge 4 commits into
Conversation
Two commands could destroy things outside GAIA with no attacker involved —
one misconfigured env var, or one mistyped port.
`GAIA_HOME=$HOME` turned `gaia uninstall --purge` into a plan targeting
`$HOME/venv` and `$HOME/documents`, and `documents` resolves case-insensitively
onto the real `Documents` folder on Windows and APFS. The containment guard in
`_remove_path` could not catch it: `_safe_roots()` returns the GAIA home itself,
so the check passed by construction. `build_plan` now refuses a home that is,
or contains, the user's home directory or a filesystem root, and `--purge`
additionally requires an existing home to look like GAIA's. `venv/` is
deliberately not proof of ownership, so a `GAIA_HOME` aimed at a project
checkout keeps its virtualenv.
A relocated `~/.gaia/documents` also aborted the purge half-way: the
containment check resolved the link *target*, raising an uncaught RuntimeError
after `venv` and `chat` were already gone. Links are now deleted as links.
Junctions need their own detection — `Path.is_symlink()` returns False for one,
and a junction is the only way to relocate a directory on Windows without
Developer Mode.
`kill_process_by_port` substring-matched `f":{port}"` against whole `netstat`
lines, so `--port 80` also matched `:8009` foreign addresses and TIME_WAIT
rows; on a dev box it selected 2 rows for `:80` and 164 for `:443`, then killed
the first. The Unix path was worse: `lsof -ti:PORT` returns both ends of every
connection and every pid got `kill -9`. Now the columns are parsed, only
LISTENING sockets whose local port matches exactly are considered, lsof is
restricted with `-sTCP:LISTEN`, and the owning process must be GAIA's or
Lemonade's before it is terminated — the identity check
`LemonadeEmbedded._daemon_alive` already makes. Subprocess output decodes with
`errors="replace"`, so OEM-codepage Windows no longer reports a UTF-8 decode
error in place of a port result.
Closes amd#3355
Request changesThis PR fixes two genuinely dangerous behaviours — The new symlink/junction detection is inverted on Python 3.10 and 3.11. It uses a helper that only exists from Python 3.12 onward, and the fallback path for older versions ends up reporting that every path is a link. On Linux and macOS under 3.10/3.11 that turns Second, smaller: the "does this directory actually belong to GAIA?" check only runs for Real-world evidenceNo 🔍 Technical details🔴 Critical
(Verified in this checkout: Failure path on Linux/macOS + 3.10/3.11,
Worth a test that pins the pre-3.12 branch, e.g. 🟡 ImportantIdentity check skips Tier 2 —
🟢 Minor
Strengths
|
The first commit fixed one of three copies of the port matcher, which left
`docs/reference/cli.mdx` claiming targeting that two other call sites did not
have.
`stop_server` (reached by `python -m gaia.api.app stop`) carried the same
defect: `f":{port}" in line` selected a listener on `:8080` for `--port 80`,
matched the foreign-address column, and taskkill'd whatever it found with no
check on the owner. On a machine where port 4001 is in use, `--port 400` picked
its pid out of 10 substring hits.
Rather than patch a second copy, the targeting rules move to `gaia.ports` and
both `kill_process_by_port` and `stop_server` call it. `stop_server` keeps its
own signalling — SIGTERM on POSIX so the server can wind its workers down —
because only the selection was wrong.
`src/gaia/util.py` held the third and worst copy: it also killed on
"ESTABLISHED" in line, so it terminated processes merely *connected* to the
port, and it had no allowlist. Nothing imports it (`lemonade_client` defines
its own `kill_process_on_port` that shadows the name), so it is deleted rather
than left as a trap for the next person who greps for "kill port".
`lemonade_client.kill_process_on_port` is the fourth site and is left alone: it
compares `conn.laddr.port == port` exactly via psutil, so it never had the
substring bug.
Refs amd#3355
|
🔴 The The fix is a one-liner: guard the fallback to Windows-only before touching reparse tags, or use 🔍 Technical details
# current — still broken on pre-3.12 POSIX
return reparse_tag == getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None)
# both sides are None on Linux → True for every plain directoryOr as a one-liner in the return: (The Simplest and clearest: A test for the pre-3.12 path would be: def test_is_link_returns_false_for_plain_dir_when_isjunction_absent(self, tmp_path, monkeypatch):
monkeypatch.delattr(os.path, "isjunction", raising=False)
assert not uc._is_link(tmp_path) |
`_is_link`'s pre-3.12 fallback compared two absent attributes:
getattr(path.lstat(), "st_reparse_tag", None)
== getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None)
On POSIX neither exists, so it evaluated `None == None` and reported every
file and directory as a link. `_unlink_link` then tried `unlink()` on a
populated `~/.gaia/venv`, fell through to `rmdir`, and failed with
"[Errno 39] Directory not empty" — every removal in the suite errored and
`gaia uninstall` returned exit 2 instead of 0. Linux CI caught it on py3.10
and py3.11; py3.12+ and Windows take the `os.path.isjunction` branch and never
reach the comparison, which is why it passed locally.
Junctions are a Windows concept, so the fallback now returns False off Windows
before touching lstat, and refuses to compare against a missing tag constant.
The regression tests simulate the pre-3.12 POSIX environment on any
interpreter — no `os.path.isjunction`, no `IO_REPARSE_TAG_MOUNT_POINT`, and an
lstat result without `st_reparse_tag` — so this branch is covered on the
Windows and 3.12+ lanes too rather than only where it happens to run. All
three fail against the previous commit.
Refs amd#3355
The identity check was gated on `require_marker=purge`, so it only ran for Tier 3. `gaia uninstall --venv` with GAIA_HOME pointed at a project checkout still deleted that project's `venv/` — the exact outcome the guard exists to prevent, and `venv` is deliberately not a marker precisely because it is the one deletion target that routinely belongs to someone else. Both tiers now run the check, and the error names the flag that was actually used. The structural guard (home directory, drive root) already covered both. Nothing legitimate is refused by extending it. Both installers shadow the environment variable with a hardcoded path -- `installer/scripts/install.sh` sets `GAIA_HOME="$HOME/.gaia"` and `install.ps1` sets `"$env:USERPROFILE\.gaia"` -- so a venv is only ever created under `~/.gaia`, which passes on its name alone. A custom GAIA_HOME that GAIA has actually used carries a marker; one that does not exist yet skips the check as a no-op. Refs amd#3355
|
Both items are now closed on the branch — The pre-3.12 link detection is fixed. You were right about the mechanism and about how it would have escaped notice. The platform guard now runs before any reparse-tag read, and a missing tag constant is treated as "not a link" rather than compared against. Linux CI caught it on py3.10 and py3.11 exactly as you predicted; both lanes are green now. The pre-3.12 regression test exists, and it deliberately runs on every lane rather than only where that branch happens to execute. The ownership check now covers Thanks — the first point was a real defect that my own testing could not have surfaced. 🔍 Technical details
|
|
This 🔴 was written against The diagnosis was correct, and it's fixed the way you suggested: the platform guard now runs before any reparse-tag read. The pre-3.12 test you asked for exists too — 🔍 Technical detailsCurrent isjunction = getattr(os.path, "isjunction", None) # 3.12+
if isjunction is not None:
return bool(isjunction(path))
if not sys.platform.startswith("win"): # :572 — your suggested guard
return False
mount_point_tag = getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None)
if mount_point_tag is None: # :576 — second, independent close
return False
try:
return getattr(path.lstat(), "st_reparse_tag", None) == mount_point_tag
except OSError:
return FalseI kept the named constant rather than hardcoding On the test — your proposed version would not have caught this. monkeypatch.delattr(os.path, "isjunction", raising=False)
assert not uc._is_link(tmp_path)That's what I wrote first, and it passed against the broken code. On Windows
CI on |
Summary
Two
gaiacommands stop destroying things they do not own:gaia uninstall --purgeno longer builds a plan that targets yourDocumentsfolder whenGAIA_HOMEis misconfigured, and every "stop what's on this port" path no longer terminates a process that merely has a connection involving that number.Why
Neither of these needs an attacker.
GAIA_HOMEis already the documented state root for the security store and the embedded Lemonade daemon, so pointing it at~is a plausible misconfiguration — and doing so turned--purgeintorm -rf ~/venv ~/documents, wheredocumentsresolves case-insensitively onto the realDocumentson Windows and macOS. The containment guard that exists to prevent exactly this was vacuous, because the only root it allowed was the misconfigured home.The port matcher had the same shape of problem:
":80" in lineover a wholenetstatline also matches a:8009foreign address, so on a normal machinegaia kill --port 80force-killed a browser. On Unix it was worse —lsof -ti:PORTlists both ends of every connection and the codekill -9'd all of them, which is how a single mistyped port took out the Agent UI backend, the daemon, and anygaia chatconnected to Lemonade. #789 reported theshell=Truehalf of this code and was closed by switching to list arguments; the substring match survived the rewrite untouched.That matcher had been copy-pasted four times. Fixing one copy and documenting the new behaviour would have shipped a false claim, so this PR consolidates them.
Linked issue
Closes #3355
Changes
gaia uninstall64, with an error namingGAIA_HOMEand the resolved path. The resolved root is printed above the plan and in the--purgeconfirmation prompt.--venvand--purgeadditionally require an existing home to be GAIA's (named.gaia, or holding something GAIA created). Applying the ownership check to Tier 2 as well was a deliberate choice over documenting it as purge-only:--venvdeletes$GAIA_HOME/venv, so guarding only--purgewould still eat a project checkout's virtualenv — the outcome this PR claims to prevent.venv/is deliberately not a marker, since it is the one deletion target that routinely belongs to someone else. Nothing legitimate is refused: both installers shadow the env var with a hardcoded path (install.sh:11,install.ps1:7), so a venv is only ever created under~/.gaia, which passes on its name alone; a home that does not exist yet is a no-op.~/.gaiais deleted as a link, leaving its target alone. Previously the containment check resolved the link target, so a relocated~/.gaia/documentsraised an uncaughtRuntimeError— aftervenvandchatwere already gone — exiting 1 instead of the documentedEXIT_FS_ERROR. Junctions need their own detection:Path.is_symlink()returns False for one, and a junction is the only way to relocate a directory on Windows without Developer Mode. A containment refusal is now reported and downgrades the exit code without aborting the rest of the plan.Port targeting — one implementation instead of four
New
src/gaia/ports.pyholds the rules: only a socket in theLISTENINGstate whose local port matches exactly,lsof -nP -iTCP:N -sTCP:LISTEN -t, and an owning-process check before anything is signalled (the same identity checkLemonadeEmbedded._daemon_alivealready makes). Output decodes witherrors="replace", so OEM-codepage Windows no longer reports a UTF-8 decode error in place of a port result. Both call sites now use it:cli.kill_process_by_portgaia kill --port,gaia kill --lemonade,gaia api stopkill -9'd both ends of every connectionapi.app.stop_serverpython -m gaia.api.app stopstop_serverkeeps its own signalling (SIGTERM on POSIX, so the server winds its workers down) — only the selection was wrong.Deleted
src/gaia/util.py— the third and worst copy. It matched the port by substring and killed on"ESTABLISHED" in line, so it terminated processes merely connected to the port, with no allowlist. Nothing imports it:lemonade_clientdefines its ownkill_process_on_portthat shadows the name, andgrep -rn "gaia\.util\b\|from gaia import util" --include=*.py src/ tests/ hub/is empty before and after. Deleted rather than left as a trap for the next person who greps for "kill port".llm/lemonade_client.py:kill_process_on_portis deliberately left alone — the fourth site. It comparesconn.laddr.port == portexactly through psutil, so it never had the substring bug. It filters no connection state and has no allowlist, but a server's own established sockets share its local port, and the default 13305 sits outside the ephemeral range. Worth revisiting if that port ever becomes freely configurable into ephemeral territory; out of scope here.Test plan
pytest tests/unit/cli/test_kill_process_by_port.py -q— 35 passed. Fixturenetstat/lsofoutput covers the:80-matches-:8009case, foreign-address andTIME_WAITrows, UDP rows, IPv6, a localized state column, lsof's exit-1-means-no-match, and the-sTCP:LISTENcall shape.pytest tests/unit/api/test_stop_server.py -q— 8 passed. Covers the:8080-listener-vs---port 80case, the owner refusal, and missing tooling.pytest tests/unit/installer/test_uninstall_command.py -q— 61 passed, 6 failed, 3 errors. The junction tests build a real junction withmklink /Jon the real filesystem (pyfakefs only models POSIX symlinks) and skip off Windows. All 6 failures and 3 errors are pre-existing on Windows (pyfakefsanchors a rootless fake path onto the real drive letter); the same set fails onupstream/mainatabe87edc, where 38 passed.pytest tests/unit/cli/ tests/unit/api/ tests/unit/installer/ tests/unit/test_check_security_gates.py -q— 459 passed, 15 skipped, plus the pre-existing set above andtest_sh_parses_under_dash(needsdash).GAIA_HOME=$HOME gaia uninstall --purge --dry-run --yes— refuses, exit 64.gaia kill --port 400on a machine with a listener on:4001— reports nothing listening instead of killing it.python -m gaia.api.app stop --port 80with a listener on:8080— leaves it alone.python util/lint.py --black --isortandflake8 --select=Fon the changed files — clean.util/lint.py --allexits 1 on 9 pre-existing pylintno-membererrors for POSIX-onlyos.killpg/os.geteuidindaemon/sidecars/andinstaller/lemonade_installer.py, reproduced identically on a pristine tree;uvx pylinton the four files this PR changes reports nothing.Evidence
gaia uninstall, on the sameGAIA_HOME:Before the fix, the first command produced a seven-line plan whose
documentsentry resolved to the realDocumentsdirectory and passed the containment guard.Port targeting, against the live
netstaton the test machine — which happens to have a service on:4001:stop_server, with a real listener on:8080:Checklist
Closes #N/Fixes #N/Refs #N).python util/lint.py --all,pytest tests/unit/).docs/reference/cli.mdxdocuments theGAIA_HOMErequirement and the--porttargeting rules for both stop paths.No LLM-affecting surface is touched (no prompts, tool registration, tool docstrings, error classification, or model selection), so no
gaia eval agentrun applies.