Harden reliability and security across all top-level commands - #49
Merged
Conversation
Audits every top-level command plus the auth backends, and fixes the defects found. Adds 32 tests: per-defect regressions and an end-to-end smoke suite covering all 18 commands (the previous contract matrix covered 7). Security: - Reject API keys containing control characters. Gemini stores keys as GEMINI_API_KEY=<key> in a .env file the CLI sources, so a newline in a key injected extra environment variables into that file. - Reject path traversal in profile and stored-file names, and use symlink_metadata for the symlink guard so a dangling symlink cannot redirect a credential write to its target. - uninstall --remove-data purges the keyring secrets it created instead of stranding them, and refuses when AISW_HOME is the home directory. - Unit tests no longer resolve tools from the real PATH. Detection spawns what it finds to read --version, so the suite was executing the developer's real claude/codex/gemini CLIs against their live home, which could rotate and invalidate real OAuth tokens. Reliability: - remove no longer destroys credentials before checking whether a context still references the profile, which then rejected the removal. - Removing the active profile clears `active` in the same locked config mutation, so config cannot name a profile that does not exist. - status no longer panics when `active` names a missing profile. - init --json no longer aborts under a shell it has no hook for. - doctor no longer reports a false failure for every Gemini profile; it checks the files a profile actually stores. This also unblocks verify. - use --all honors --emit-env and --state-mode. --emit-env previously printed the human summary, which a shell hook would eval. - OAuth capture reaps its interactive child on every error path. - Antigravity is guarded by the shell hooks and present in workspace status --json and status --context --json. Performance: - workspace check, which the shell hook runs on every directory change, reads active profiles from config instead of probing tool binaries, credential files, and the OS keyring. Behavior change: use --all now exits non-zero when a switch fails. It previously exited 0 while reporting a failure in `warnings`, which contradicted the documented contract that zero means success. On partial failure --json now emits the standard failure envelope. Noted in CHANGELOG.
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
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.
Audits every top-level command plus the auth backends and fixes the defects found.
921 tests pass, 0 fail. Clippy and
cargo fmt --checkclean. 32 tests added: one regression per defect, plus an end-to-end smoke suite covering all 18 top-level commands (the previous contract matrix covered 7).Why this branch exists
Running the test suite could log the developer out of their real accounts. Unit tests never sandboxed
PATH, andtool_detection::detect()spawns whatever it finds to read--version:HOMEwas not overridden either, so those ran against live state. Tests run multi-threaded, and these CLIs refresh OAuth tokens on startup — concurrent refreshes race and invalidate the refresh token. This actually happened and required re-login.Fixed by applying the safety default the keyring already used: under
cfg(test), ambient detection resolves nothing unless a test opts in viaAISW_TOOL_PATH_TEST_DIR. Tests passing an explicit path are unaffected. Verified withstrace: zero real-binary executions, zero real-home access. Lib tests also got ~3x faster (29s → 10s).Security
GEMINI_API_KEY=<key>in a.envfile the CLI sources, but validation only rejected empty strings. Reproduced — this exited 0:symlink_metadata, sincePath::exists()follows links and a dangling symlink previously slipped through and let a write create the link's target.uninstall --remove-datapurges the keyring secrets it created instead of stranding them forever, and refuses whenAISW_HOMEis the home directory (rm -rf ~guard).Reliability
removedestroyed credentials it then refused to delete. It snapshotted, deleted the keyring secret, and deleted the profile directory before the config write rejected a context-referenced profile.activein the same locked mutation, so config cannot name a nonexistent profile.statuspanicked (no entry found for key, exit 101) whenactivenamed a missing profile.init --jsonaborted withunreachable!()under/bin/sh— the default in most containers.doctorfailed for every Gemini profile. It looked for one hardcoded filename per tool that Gemini never writes, soaisw doctorexited 1 on healthy installs and draggedverifydown with it.use --allignored--emit-env. It fell through to the human summary — and since the shell hook dispatches on$1 = "use", a hooked shell waseval-ing ANSI-colored text:--state-modewas dropped the same way.?returns inside the polling loop previously orphaned a liveclaude auth loginwith the terminal attached.workspace status --jsonandstatus --context --json.Performance
workspace checkruns on every directory change via the shell hook. It was doing full binary detection, credential reads, and OS keyring access; it now reads active profiles from config.Breaking-change review
Built binaries from
mainand this branch, ran both against identical fixtures, and diffed every machine-readable output. Three differences, all intentional:workspace status --jsongains"antigravity": nulldoctor --jsondetail"0600 ok"→"0600 ok (1 file(s))"doctor/verifygemini checkfail→pass(exit 1 → 0)status --jsonis byte-identical — the newactive_profile_registeredfield is internal and never serialized.aisw use --allnow exits non-zero when a tool switch fails. It previously exited0and reported the failure only in awarningsarray, contradictingdocs/automation.md: "Exit code0means success." On partial failure,--jsonnow emits the standard failure envelope instead of{"ok": true, ..., "warnings": [...]}.Called out explicitly in CHANGELOG under
Unreleased → Fixed.Not included
Cargo.tomlstays at0.3.8— version bump and release timing left to you.