mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
fix/opencode-session-header
24
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Pujitha PaladuguandPujitha Paladugu
|
176f5a242b |
fix(oauth): force-close loopback callback connections past the shutdown deadline (#1051)
* fix(oauth): force-close loopback callback connections past the shutdown deadline Motivation: LoopbackListener.Close shuts the callback server down with a 1-second context and discards the result. http.Server.Shutdown only closes idle connections, so a client that sends a partial request header and never finishes it counts as active; Shutdown blocks out the full deadline, times out, and Close returns while that connection and its Serve goroutine remain open indefinitely. This is a loopback-only (127.0.0.1), short-lived, single-use server, so the impact is a resource leak past the end of the flow, not a remote vulnerability or crash. Approach: - add a ReadHeaderTimeout to the server so a stalled request header is bounded on its own; - follow a timed-out or otherwise failed Shutdown with server.Close() to forcibly tear down any connection still open past the deadline; - join the Serve goroutine (via a done channel) before Close returns, so its completion is observable instead of left dangling. internal/mcp/oauth.go and internal/provideroauth/openrouter.go build their own loopback servers with the same pattern; they are left untouched here to keep this change scoped to the file the issue's reproduction demonstrates. Validation: - go build ./internal/oauth/... and go vet ./internal/oauth/...: clean. - gofmt -l internal/oauth/: clean (make fmt-check clean repo-wide). - go test ./internal/oauth/... -race -v -count=1: all pass, including three new tests (TestLoopbackCloseForceClosesSlowHeaderConnection, TestLoopbackCloseIsIdempotent, TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback). - Confirmed TestLoopbackCloseForceClosesSlowHeaderConnection fails against the pre-fix loopback.go (with the new test kept) and passes with the fix applied. - Full repo go build ./..., go vet ./..., and go test ./... pass. - Could not run make lint-static (golangci-lint) locally: it needs a very large dependency download and the local environment ran out of disk space fetching it. That check is continue-on-error (advisory) in .github/workflows/ci.yml, not a required gate. Report: https://github.com/Gitlawb/zero/issues/1028 Signed-off-by: Pujitha Paladugu <[email protected]> Assisted-by: claude-sonnet-5 (via Claude Code) * test(oauth): synchronize loopback regression tests instead of sleeping TestLoopbackCloseForceClosesSlowHeaderConnection relied on a fixed 100ms sleep to let Serve accept a connection before sending a stalled header; under load this could fail or silently skip the force-close path it's meant to exercise. Use an http.Server.ConnState hook (via a new unexported newLoopbackListener, set before Serve starts to avoid a race) to wait for the real StateNew signal instead. TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback measured Close's elapsed time without joining its http.Get goroutine or draining/closing the response body; join it first so the timing starts from a connection that's actually back to idle. Also register t.Cleanup(l.Close) in both tests so a Fatalf between construction and the existing explicit Close doesn't leak the listener's Serve goroutine. Signed-off-by: Pujitha Paladugu <[email protected]> --------- Signed-off-by: Pujitha Paladugu <[email protected]> Co-authored-by: Pujitha Paladugu <[email protected]> |
||
|
euxaristiaandeuxaristia
|
275e248669 |
fix(oauth): Split an oversized keyring token blob across entries (#1007)
* fix(oauth): split an oversized keyring token blob across entries Store every provider and MCP token in one keyring entry and the store stops working once the logins outgrow it. On macOS the secret rides inside a `security -i` command line capped at 4095 bytes, which leaves 3027 bytes of JSON for all logins combined, so a second OIDC login fails to save and every write after it fails too. Split a blob that does not fit across numbered entries and put a manifest in the anchor account. Chunks live in two alternating generations: a write fills the one that is not live, then replaces the manifest, so that single write is the commit point and a crash partway through still reads the previous generation. `zc1:` cannot prefix base64, so an entry written by an existing build is still recognised and read without a migration step. Reserve the range a write will occupy before occupying it. Without that, a write interrupted while filling a longer generation leaves chunks above the count the manifest records, and no later cleanup knows to delete them: a fragment of a token blob would stay in the keychain for good. Expose the per-entry budget from internal/keyring rather than hardcoding the macOS figure in the oauth store, sharing one line builder with Set so the budget and the boundary it describes cannot drift. Backends with no limit report so and keep the single-entry layout, so Linux is untouched. Refs #937 * fix(oauth): lock keyring reads, validate manifest digest, and harden chunk tests * test(oauth): assert Status blocks during keyring lock regression test * fix(oauth): sweep both chunk generations when a store regrows A shrink writes the blob back under the anchor, which replaces the manifest and takes the per-generation chunk counts with it. From then on nothing can name the chunks a failed cleanup left behind, so the growth branch's sweep of the target generation is the only one that will ever reach them — and it only ever targets family A. A keychain that refused one delete during the shrink therefore kept a superseded generation of access, ID and refresh tokens indefinitely, with no way for the user to know. Sweep the other generation alongside the target, and document that the reclaim waits for the next growth: a store that shrinks once and never grows again keeps the residue, which sweeping on every whole write would close at a cost of 128 `security` invocations per save on macOS. Also record why Load and Status take the cross-process lock, and give the missing-chunk error the same "log in again" advice the digest failure carries. Refs #937 * fix(oauth): serialize keyring operations under user-level lock Derive the keyring backend lock file path from the user's home directory rather than file store configuration, ensuring processes with distinct store paths share the same lock domain for the OS keychain. Refs #938 * Address review feedback on chunked keyring token store and recovery Refs #938 * Refine auth reset message and chunk format notation Refs #938 * Harden OAuth store reset lifecycle, keyring migration recovery, and completions. Refs #938 * Validate keyring cleanup markers, propagate lock resolution errors, and test reader concurrency. Refs #938 * Preserve live OAuth credentials when stale cleanup markers survive migration. Refs #937 * Verify JSON output from OAuth reset to cover its recovery response. Refs #937 * Verify that JSON OAuth reset actually removes stored logins. Refs #937 * Require confirmation before resetting shared OAuth credentials. Refs #937 --------- Co-authored-by: euxaristia <[email protected]> |
||
|
KRATOS
|
6fe0d1edcd |
fix(lockutil): use kernel-held advisory locks (#950)
* fix(lockutil): use kernel-held advisory locks * fix(lockutil): harden cross-platform lock files * test(lockutil): require one concurrent holder |
||
|
|
55fa05f0ab |
fix(sandbox): deny reads of Zero credential stores (#681)
* fix(sandbox): deny reads of zero credential stores * fix(sandbox): address PR review comments - Deny reads of mcp-oauth-tokens.json.secret in the default Zero config candidates: file-backed oauth stores keep their encryption secret in a sibling .secret file, so the default MCP token store needs the same protection as the override paths (CodeRabbit). - Clarify that explicit credential-file overrides are still filtered by on-disk existence like every other candidate (Copilot). - Align the zeroUserConfigDir doc comment with config.UserConfigDir: macOS honors XDG_CONFIG_HOME before falling back to ~/.config (Copilot). Co-Authored-By: Claude Fable 5 <[email protected]> * fix(sandbox): deny the whole Zero config dir, not itemized filenames Address code review on PR #681: - Deny credentialDenyReadPaths' Zero-config candidate as the containing directory instead of an itemized filename list. The token/credential/ config stores each publish through a randomly-named .tmp-<pid>-<nanos> sibling before their atomic rename, and the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated backup after importing it — none of those names were covered by the old itemized list, so a sandboxed command could read them under the read-all posture. - Stop dropping default candidates that don't exist yet at profile-build time. A store created later in a long-lived sandboxed session (e.g. a concurrent ) previously got no deny rule at all; every backend already treats a deny rule over a not-yet-existing path as a harmless no-op that still takes effect once the path appears. - Resolve ZERO_OAUTH_TOKENS_PATH / ZERO_MCP_OAUTH_TOKENS_PATH overrides the same way the token stores resolve them (relative-to-cwd, no ~ expansion) instead of through normalizeProfilePath, which tilde-expands and so could derive a deny path different from where the store actually writes. Adds regression coverage for the directory-wide deny (including the migrated backup and synthetic temp-file siblings), for building the profile before the store directory exists, and for the tilde-override resolution mismatch. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(sandbox): address macOS deny-path test and config parity Recompute the expected Zero config deny path after MkdirAll and resolve the temp base with EvalSymlinks so macOS /var -> /private/var does not flake TestPermissionProfileDeniesZeroCredentialFiles. Add a parity test that sandbox.zeroUserConfigDir stays aligned with config.UserConfigDir. Co-authored-by: PierrunoYT <[email protected]> * fix(sandbox): harden credential deny paths * test(cli): normalize credential deny baseline * fix(sandbox): close credential path gaps * test(sandbox): normalize command credential path * fix(sandbox): handle absent credential paths safely * fix(sandbox): protect MCP OAuth siblings * fix(sandbox): protect OAuth lock siblings * docs(sandbox): clarify Linux future-path limits * test(sandbox): probe credential store reads in the Linux smoke test The smoke test wired HOME/XDG_CONFIG_HOME at credential directories but never attempted a read through the sandbox, so it could not catch a real-bwrap regression for the paths this change protects. Write a secret into ~/.aws and $XDG_CONFIG_HOME/zero and add both to the deny-read table. * fix(sandbox): keep the credential deny usable, current, and unguessable Four problems with the credential deny baseline, all in the same mechanism. The whole-directory deny over $XDG_CONFIG_HOME/zero also hid the supported user plugin, specialist, and command roots, whose commands and scripts are executed through this sandbox, so an installed user plugin could no longer start. Narrowing the deny to itemized files was not an option: the stores publish through temporary names and a concurrent login can add a store that did not exist when the profile was built, and only a directory rule covers both. The directory deny therefore stays, and the non-secret subtrees are carved back out through a new DenyReadCarveouts field: seatbelt re-allows them after the deny (SBPL is last-match-wins) plus ancestor metadata so path resolution can reach them, and bubblewrap masks the directory with --perms 111 instead of 000 -- unlistable but traversable -- and re-binds the carveout read-only before the tmpfs is remounted read-only. A relative ZERO_OAUTH_TOKENS_PATH, ZERO_MCP_OAUTH_TOKENS_PATH, or XDG_CONFIG_HOME was resolved against the command directory only, while oauth.ResolveStorePath and mcp.ResolveTokenStorePath call filepath.Abs, i.e. resolve against the Zero process working directory. A command run from a subdirectory therefore had the wrong path denied and the real store stayed readable under the read-all posture. Relative candidates now resolve against both directories, since the process that resolves the value is not necessarily the one that writes the store. The regression test runs the command from a different directory than the process, which the previous tests did not. bubblewrap cannot mount over a path that is absent when the namespace is assembled, so a store created mid-session stayed readable through the live read-only host-root bind for the rest of a long-lived sandbox session. The directories Zero owns -- its config directory and the per-store publication directories -- are now reported as EnsureDenyReadDirs and created (0700) before the namespace is assembled, so their mask always exists. Third-party stores such as ~/.aws are still skipped when absent: Zero must not create them, and that limitation stays documented on the field. The fixed <store>.tmp publication sibling made the plaintext token, and via the same path its encryption key, observable to a same-user sandboxed process with write access to an override parent: it could wait for the known name and rename the opened inode into an allowed path. Both stores now publish through a per-store <store>.publish directory with a randomly-named file inside. The directory name is derived from the store path, so the profile can deny it up front, while the random name inside is what cannot be waited for or renamed away. A test keeps the duplicated suffix in step with oauth.PublicationDirSuffix. Also adds the seatbelt render coverage that was missing: dropping DenyReadIfExists from denyReadRules made every credential store readable again on macOS without failing a single test. * fix(sandbox): close credential deny review gaps Preserve process credential roots alongside command overrides while limiting host precreation and read carveouts to trusted process-derived paths. Canonicalize future credential roots through existing path aliases, reject symlink carveouts, and collapse automatic masks already covered by user or automatic directory denies. Normalize the policy golden and add regression coverage across profile, Seatbelt, and bubblewrap rendering. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): normalize credential carveout paths Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): canonicalize backend credential masks Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * test(sandbox): normalize backend mask expectations Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-2f41-7398-8692-75327064c6f0 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): fail closed for replaceable token stores Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): allow keyring token path overrides Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfa-8469-731e-87c0-b26b7641cf59 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): harden command credential roots Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-c723-74d9-a323-026040b94436 Co-authored-by: Pierre Bruno <[email protected]> * fix(sandbox): close credential deny review gaps Reapply credential denies after Seatbelt extension carveouts and model the legacy MCP token path as a migration input rather than an atomic writer. Retain Bubblewrap fail-closed markers for OAuth outputs exposed through readable carveouts, and add regressions for both platform policies. Amp-Thread-ID: https://ampcode.com/threads/T-019fbf55-f6b0-71fc-b384-652ba32b9645 Co-authored-by: Pierre Bruno <[email protected]> --------- Co-authored-by: Claude Fable 5 <[email protected]> Co-authored-by: Cursor Agent <[email protected]> Co-authored-by: PierrunoYT <[email protected]> Co-authored-by: Amp <[email protected]> |
||
|
KRATOSandKRATOS
|
974fc036c2 |
fix(oauth): refuse redirects on credential POSTs (#729) (#741)
Token exchange/refresh and the device authorization/poll POSTs validated only the initial endpoint, then let the http.Client follow a 307/308 that replays the form body (code, PKCE verifier, refresh_token, client_secret) to an unvalidated origin. Add withoutRedirects(): a shallow client copy whose CheckRedirect refuses every redirect (ErrUnsafeRedirect). Applied at PostToken (covers ExchangeCode, Refresh, and MCP which delegate to it), RequestDeviceCode, and pollDeviceOnce. The caller's client is never mutated; legitimate token endpoints that return 200 are unaffected. Co-authored-by: KRATOS <[email protected]> |
||
|
KRATOSandKRATOS
|
ce4a996ffa |
fix(oauth): validate discovered endpoints before merge/use (#511) (#739)
Discovery metadata was merged without the https/loopback endpoint rule that configured endpoints already pass, letting a malicious issuer downgrade the authorization/token/device/registration endpoint after config validation. - Add exported ValidateEndpointURL as the single endpoint-safety rule; validateTokenEndpoint now delegates to it. - Backstop in BuildAuthorizationURL (the choke point both the provider and MCP flows build their browser URL through). - Validate each discovered endpoint before merge in the provider resolveEndpoints and validate resolved MCP metadata in both branches; fail closed on insecure. Co-authored-by: KRATOS <[email protected]> |
||
|
euxaristia
|
da41c3a75b |
fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix (#628)
* fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix During lock reclamation, reclaiming a suspected stale lock renames the file aside and renames it back if not stale. On POSIX systems, the rename back silently overwrites any new lock file created in the gap. On Windows, it fails to rename back and leaks the stale file. Introduce a cross-platform restoreLockFile helper: - On POSIX: Use os.Link to link the file back (which atomic-fails if the target already exists), then remove the temporary link. - On Windows: Use os.Rename (which safely fails if the target exists). If restoring fails, delete the temporary file immediately to prevent leaks. * fix(lockutil): fix Windows build by using golang.org/x/sys/windows Addresses jatmn's follow-up review: the required Windows smoke check was failing because syscall.MoveFileEx is not exported by the standard library's syscall package on this platform, so internal/lockutil (and everything importing it: cron, daemon, hooks, oauth) failed to compile on Windows. Switched to golang.org/x/sys/windows, which does export MoveFileEx with the same signature, already a direct module dependency. Verified on a real Windows machine that the no-flags call still fails with ERROR_ALREADY_EXISTS when the destination exists, and that this satisfies errors.Is(err, os.ErrExist), which cron/daemon/hooks/oauth's reclaim paths all depend on to decide whether the sidelined lock was genuinely superseded (safe to discard) versus some other, unrelated failure (must be preserved). TestRestoreLockFile already covered both the successful-restore and fail-when-target-exists cases; it now passes on Windows. * fix(lockutil): route lock file deletes through RemoveLockFile with Windows retry loop * fix(lockutil): close restore split-brain and unify RemoveLockFile RestoreLockFile could fail with errors other than os.ErrExist (hardlink- incapable filesystems such as FAT or some FUSE/network mounts, ENOSPC, Windows sharing violations). Callers only clean up on os.ErrExist, so any other failure left the lock path missing with the live holder's lock stranded in the sidelined file; the next O_EXCL create then succeeded and two processes held the lock. RestoreLockFile now falls back to an O_EXCL copy in that case, which preserves the no-overwrite guarantee, and a successful restore with a failed sidelined-file cleanup reports nil instead of a failed restore. RemoveLockFile now has one cross-platform contract: removing an already missing file reports nil on POSIX too, matching Windows, and daemon release() drops its caller-side ErrNotExist filter. Tests: TestRestoreByCopy and TestRemoveLockFile run on all platforms; Windows-tagged tests pin the ERROR_ALREADY_EXISTS mapping to os.ErrExist and exercise the RemoveLockFile retry loop to success and to bounded exhaustion. The package doc comment documents the shared contract. * refactor(swarm): extract reclaimStaleLock to mirror other lock callers The stale-lock reclaim in acquireLock was inline, so its restore path (a holder rotated a fresh lock in the gap between the stale check and the rename) could not be regression-tested the way cron, daemon, hooks, and oauth test theirs. Extract it into reclaimStaleLock with the same shape as the other callers, behavior unchanged, and add the mirrored regression test. * test(hooks,oauth): add stale lock reclaim regression tests Mirror the cron reclaim test in the two packages that lacked coverage for reclaimStaleLock: a genuinely stale lock is reclaimed, a fresh lock (reacquired in the gap) is restored intact rather than stolen, and a missing lock reports no reclaim. daemon already had equivalent tests in lock_test.go. * fix(lock): fail closed and drop sidelined file when restore fails A non-os.ErrExist error from RestoreLockFile means both the no-replace restore and its O_EXCL copy fallback failed, so the lock path may be missing while the sidelined holder is live. Callers previously swallowed that error and their next O_EXCL create would succeed, breaking mutual exclusion. reclaimStaleLock in cron, daemon, hooks, oauth, and swarm now returns an error for that case and the acquire loops fail closed instead of re-acquiring. The sidelined .stale.* file is now removed on every restore failure, not only os.ErrExist: once the restore has failed it has no protocol function (release only consults the lock path), so keeping it only leaked files. Also replace the numeric errno literals in the Windows RemoveLockFile retry loop with windows.ERROR_SHARING_VIOLATION and windows.ERROR_ACCESS_DENIED. * refactor(lockutil): extract shared ReclaimStaleLock helper Consolidate the duplicated reclaimStaleLock functions from cron, hooks, oauth, and swarm into lockutil.ReclaimStaleLock. Each caller now passes an mtime-based isLive predicate to the shared helper, which owns the rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error propagation. Daemon keeps its own reclaimStaleLock because it uses PID-based liveness (different predicate shape and no staleAfter duration). The per-package reclaim regression tests are removed since the shared helper is thoroughly tested in lockutil/reclaim_test.go (dead lock reclaimed, live lock restored, missing lock, fail-closed on restore error, sidelined file dropped when new holder wins). * fix(lockutil): propagate non-contention rename failures from ReclaimStaleLock A rename-aside failure that is not a lost race previously mapped to the benign lost-race result, so acquirers spun until their deadline and reported a timeout instead of the real failure. Keep only a missing source and the Windows sharing-violation/access-denied contention errnos benign (neither Windows errno maps to os.ErrExist, and ERROR_SHARING_VIOLATION does not map to os.ErrPermission either, so the classification compares raw errnos) and return everything else so callers fail closed. Also convert daemon's private reclaimStaleLock to the shared helper so it picks up the same contract, and reword the caller error messages from "restore reclaimed" to "reclaim stale" since the error can now come from the rename aside as well. * fix(lockutil): publish restore-by-copy atomically and honor lost mailbox reclaims restoreByCopy created the destination lock file empty before copying into it, so a crash mid-copy could leave a live holder's lock permanently unrepresented at the lock path. Stage the copy under a private name and publish it with the platform's no-replace primitive instead. swarm's mailbox lock discarded the reclaimed boolean and always retried immediately, so a lost reclaim race (or Windows sharing-violation contention) could hot-spin past the lock timeout instead of falling through to the bounded wait like the other lock callers. * fix(lockutil): shrink the reclaim/restore window with a fast replacing rename Restoring a lock that turned out live went through the no-replace path (plus its copy fallback on some failures) first, which is slow enough that a fourth process's unrelated O_EXCL create could win lockPath while it was absent for the whole call. RestoreLockFile's no-replace contract would then report os.ErrExist and the caller silently dropped the sidelined lock, orphaning the still-running original holder. restoreLiveLock now tries a single replacing rename straight back first, falling back to the no-replace path only if that fails. It shrinks the window from a whole restore call down to roughly one syscall; it does not close it, since the fast path can silently overwrite a competing lock rather than detect it. A fully race-free reclaim would need an OS-level advisory lock checked non-destructively instead of by moving the file, which is a larger change than this. |
||
|
Kevin Codex
|
16550569c1 |
feat(providers): OAuth login profiles and list-first /provider manager (#560)
* feat(providers): OAuth login profiles and list-first /provider manager Make multiple providers a first-class workflow: every OAuth login is now a visible, switchable provider, and /provider manages the whole lifecycle. OAuth logins become provider profiles: - zero auth chatgpt / zero auth login <preset> and all four TUI login paths (wizard + first-run, browser + device-code) now upsert a catalog-backed profile via config.EnsureCatalogProvider after storing the token — without stealing the active provider. Previously the token was stored invisibly: the provider showed up nowhere and 'providers use' failed not-found. - Switching to an OAuth provider no longer inlines the bearer as an API key, which suppressed the runtime resolver (no token refresh, no Codex chatgpt-account-id header → guaranteed 401s). The credential gate now recognizes stored OAuth logins on keyless profiles. - providers list / current report truthful credential state: stored keys count as configured (APIKeySet honors apiKeyStored) and token logins show 'oauth login'; providers check accepts an OAuth login as a credential. List-first /provider manager (TUI): - /provider opens the saved-provider list: Enter activates via the shared switch path, 'a' adds (existing wizard), 'e' opens a field-level editor (endpoint, model, key, description, and safe rename — the stored key, activeProvider, and live ZERO_PROVIDER follow the new name), 'd' deletes with an inline confirm and stored-key cleanup. Esc walks back one level. - Credential states resolve asynchronously so keychain reads never block the render loop; 'stored key missing' surfaces broken keychain entries. - Implemented as manager steps on providerWizardState so every existing overlay gate, key route, and mouse hook covers it unchanged. CLI + config parity: - zero providers remove <name> (deletes the stored key, hands the active pointer off) and zero providers rename <old> <new>. - config.RemoveProvider / RenameProvider: read-modify-atomic-write; rename migrates the credential-store entry write-new-then-delete-old so an interruption can never lose the key. * fix(providers): address PR #560 review findings All six review findings validated as real; fixes with regression tests: - Persist the apiKeyStored marker when a key is captured onto an existing profile: mergeProfile deliberately ignores the marker (a project config must not claim the user's stored keys at resolve time), so the user-config WRITE path (UpsertProvider) now re-applies it — previously editing a key onto an env/no-key profile stranded the secret in the credential store while every ApplyStoredAPIKey gate skipped it. [jatmn P2] - Let active-pointer recovery select OAuth-login providers: firstUsableProvider now applies the same providerHasOAuthLogin check as setupRequired/usableSavedProviders, so a stale activeProvider plus a saved ChatGPT/xAI login recovers instead of forcing onboarding. [jatmn P2] - Delete stored keys from the store BESIDE the config being edited: providers remove and the TUI manager delete now use ProviderKeyStoreAt (matching setup/rename) instead of the default-path store. [jatmn P2] - Roll back the key migration when RenameProvider's config write fails, so the store and config.json can't desync (key under the new name, config still naming the old one). [CodeRabbit] - Clear provider descriptions for real: new SetProviderDescription writes the field verbatim (the upsert merge treats empty as unchanged); the manager's editor routes description changes through it. [jatmn P3] - Surface stored-key deletion failures in providers remove (stderr warning + keyError in the JSON payload) and the TUI delete status, instead of letting a lingering secret read as a clean removal. [CodeRabbit] * fix(providers): second review round — case-only rename, logout hint, test hermeticity - Skip the credential-store migration on case-only renames (groq -> Groq): the store normalizes names case-insensitively, so Set(new)+Delete(old) target ONE entry and the delete removed the key that was just "moved". The existing entry already serves the new name. [CodeRabbit] - Point the delete flow's cleanup hint at the stored login that actually exists: after a rename ({name:"codex", catalogID:"chatgpt"}) the token lives under the catalog id, and `zero auth logout codex` would delete nothing. oauthLoginName resolves the same FirstStored candidate the runtime uses. [jatmn P3] - Lock in APIKeyEnv semantics after a key edit: the env reference survives as an explicit override (a SET env var wins at resolve time; the stored key applies when it is absent). [CodeRabbit nitpick] - Make main's new firstUsableProvider tests hermetic: they read the REAL OAuth token store, so any developer with an actual xai/chatgpt login gets spurious failures (found because this machine has one). Also rebased onto main; the only conflict was comment wording in firstUsableProvider, where the audit commit (#481) independently landed the same OAuth-login fallback check this branch added. * fix(tui): read stored keys from the active config's credential store The TUI's write paths (SecureProviderProfile capture, rename migration, manager delete) all target the store beside m.userConfigPath, but the read paths still opened the DEFAULT-path store: profileWithCredential (feeding the /model and manager switch gate), the model picker's discovery credential lookup, and the manager's async credential-status probe. With a non-default config path, a provider saved with only apiKeyStored had its key in the config-adjacent store, so the gate rejected it as having no usable credential and the manager showed 'stored key missing' for a healthy profile. All TUI stored-key reads now resolve through providerKeyStoreForPath (config-adjacent, default-path fallback for ephemeral sessions), so the UI reads from the same store it writes to. Regression test covers both the credential probe and the switch path with a key seeded only beside the config. Also consolidates the OAuth test-store seeding on withAuthStore (now pins ZERO_OAUTH_STORAGE=file so an inherited keyring backend can't bypass the temp path) per review nitpick. * fix(providers): resolve self-review findings — atomic edits, key normalization, threading Findings from a multi-agent code review of this branch (8 finder angles, independently verified); all confirmed against the code, several with empirical repros. Correctness: - config.EditProvider: one atomic read-modify-write for the manager's Save (rename + fields + verbatim description + stored-key marker + key migration with write-failure rollback). Replaces the rename→upsert→ describe sequence that (a) turned a case-only rename (groq → Groq) into a DUPLICATE profile via UpsertProvider's case-sensitive merge, and (b) could fail halfway, leaving config.json renamed while savedProviders and ZERO_PROVIDER still held the old name (retry then failed 'not found'). A rename now also migrates a freshly captured replacement key (captured under the current name; the migration moves it). - savedProviders is mutated surgically on manager delete/edit instead of being replaced with the raw user-config list — the session list is seeded from the RESOLVED (project-config layered) and usability-FILTERED set, so wholesale replacement dropped project-contributed providers and resurrected filtered stubs for the rest of the session. - oauth.ProviderKey normalizes provider names to lower case at the single write/lookup choke point: `zero auth login xAI` stored provider:xAI while every candidate lookup was case-sensitive — a successful login invisible to the runtime. Map-based presence checks lowercase to match. - OAuth login persistence moved onto the Update goroutine (message handlers) — a background tea.Cmd (device-code polls land minutes later) doing an unlocked config read-modify-write raced UI-thread writes (lost updates). The write error is now surfaced and the in-memory mirror is skipped on failure, so a profile that never reached disk can't show as a working provider that vanishes on restart. UX / consistency: - Esc inside a manager-launched add flow walks back to the provider list from EVERY step (was: first step only; deeper steps destroyed the overlay and the user's place). In-flight OAuth logins are invalidated on the way out; footers say 'Esc back'. - switchProviderModel returns a structured outcome; the manager branches on it instead of strings.Contains(text, "Switched to") — UI copy is no longer a control-flow contract (a provider named 'Switched to prod' could close the manager on a refusal). - Manager delete's keychain subprocess + OAuth lookup run in a follow-up tea.Cmd (the confirm keypress no longer blocks the render loop); the credential probe takes ONE token-store snapshot instead of 2 reads per row, and resolved states carry over across reloads (no 'checking…' flash). - Credential-state reporting converges: /provider status recognizes OAuth logins (was 'api key: not set' + warning for a WORKING provider), doctor uses the shared HasConfiguredCredential definition plus OAuth awareness (its 'matching ProviderSnapshot.APIKeySet' claim is true again), and the manager's classifier falls through a stale stored-key marker to env/OAuth the way the runtime does. * Fix provider manager test formatting |
||
|
PierrunoYTandCopilot Autofix powered by AI
|
008bc9b3f3 |
fix: address bugs found in a multi-agent codebase audit (#481)
* fix: address bugs found in a multi-agent codebase audit Five parallel review agents each swept a different area (self-update, TUI keybindings, agent/session handling, sandbox/shell safety, and auth/credential storage) and surfaced nine concrete defects, all fixed here with regression tests: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant be bypassed by spelling a Windows path with different case - securefile/oauth: createSecretFile's Windows ERROR_ACCESS_DENIED handling was only applied to the oauth token store, not the shared credstore backend; both also masked the real lock-contention error behind a misleading "file does not exist" timeout - cli: firstUsableProvider ignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated user - sessions: exec prompt summarization truncated on a raw byte offset, which could split a multi-byte UTF-8 rune and embed invalid UTF-8 into a resumed session's prompt - tui: configurable keybindings had no collision detection, so a remapped chord could silently make a hardcoded shortcut (or another configured binding) permanently unreachable - tools: the Windows shell-syntax pre-flight check wasn't quote-aware and could block harmless commands whose quoted arguments merely contained operator-like text - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted; extractZip didn't reject symlink-mode entries like extractTarGz already did; replaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lock * fix: close remaining PR 481 review findings * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * fix(tools): drop stale ; test case after rebasing onto main's MSYS check Rebasing fix/bug-hunt-findings onto main pulled in #476, which replaced this file's regex-based windows_shell_syntax detection with a segment/word-anchored check and deliberately stopped treating ; as a command separator on Windows (cmd.exe doesn't split on ; the way bash does). detectShellCommandIssue itself came out of the rebase unchanged from main (our quote-masking version was fully superseded by #476's more thorough segment-based approach), but the quoted-content test this PR added still asserted the old bash-style semantics for `echo "foo" ; ls -la`, which now correctly returns no issue since it's a single echo invocation with literal arguments under cmd.exe, not a real ls invocation. --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]> |
||
|
KRATOS
|
d05e9148a7 |
fix(oauth): treat Windows ERROR_ACCESS_DENIED as lock contention in createSecretFile (#445)
createSecretFile's secret-lock loop only retried on os.ErrExist. On Windows a concurrent holder's `defer os.Remove(lockPath)` leaves the lock file in a "delete pending" state, so a racer's O_EXCL create returns ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist — which fell through to a hard "create token secret lock: Access is denied" error instead of retrying. Mirrors the identical handling already present in acquireFileLock (lock.go, from #261). Fixes the flaky Windows-CI failure TestLoadOrCreateSecretConcurrentConverges that was intermittently red-flagging every PR's Windows smoke job. Verified on macOS: build/vet/gofmt clean, oauth package green, concurrent- converge test passes under -race (5x). The Windows delete-pending path can't be reproduced off Windows, so the definitive proof is the Windows CI on this PR. |
||
|
KRATOS
|
6e0a665118 |
fix(auth): propagate credentials to every provider-build surface and pin children to the live provider (#366)
* fix(auth): propagate credentials to every provider-build surface and pin children to the live provider Sub-agents, swarm members, ACP sessions, and mid-run escalation switches could build providers without the credentials the parent was running with. This consolidates credential resolution so every surface gets the same auth: - Wrap deps.newProvider once in fillAppDeps to fold the stored API key in centrally; ACP, exec escalation, and buildProvider no longer patch it per call site (an apiKeyStored profile previously went out keyless on those paths and 401'd). - Export ZERO_PROVIDER at startup (buildProvider) and keep exporting on every runtime switch (wizard, onboarding, /model, /provider), so spawned children resolve the parent's live provider instead of whatever another process last persisted to config.json. Injected via appDeps so tests never mutate the process environment. - Share one OAuth login selection end to end: ProviderProfile. OAuthLoginCandidates (name, then catalog-ID fallback; nothing for a profile with its own configured credential) + oauth.FirstStored resolve the login ONCE, and the bound store key is threaded through providers.Options.OAuthLoginKey so the Codex account-id header always reads the same login that issued the bearer token. A renamed profile (name != catalog ID) now finds its login; an explicit API key is never silently overridden by a same-name or catalog-shared OAuth login. - Export ProviderProfile.HasConfiguredCredential as the single "profile is key-authed" predicate (cli/tui/config previously had three copies). - applyProviderWizard commits live state (provider, profile, name, env export) only after BOTH the build and the config persist succeed, so a persist failure no longer leaves the chat running on a provider the status line and children disagree with. Covered by red-green tests for each path: stored-key wrap, escalation switch, catalog-ID fallback (incl. case-variant names), explicit-key protection, same-login account selection, wizard persist-failure rollback, onboarding/startup env export. * test: isolate OAuth/config test storage and fix the flaky MCP response-id test Review + CI fixes, all test-only: - factory/cli OAuth tests: pin ZERO_OAUTH_STORAGE=file next to the temp ZERO_OAUTH_TOKENS_PATH — an inherited "keyring" value sends NewStore to the OS keychain and ignores the temp path entirely, so the tests could read (or clobber) the developer's real logins. newOAuthStoreForTest now owns both pins so a future call site can't forget them. - TestApplyProviderWizardExportsActiveProviderEnv: point m.userConfigPath at a temp config and stub newProvider, so the SUCCESS path exercises the full build+persist+commit sequence in isolation (the default empty path silently skipped persist; a changed default must never reach the real user config). - TestClientRequestWaitsForMatchingResponseID (ubuntu smoke flake): the responses were pre-loaded into a bytes.Buffer, so the client's reader goroutine could consume the matching response — and hit EOF — before request() registered its pending id, failing with "request() error = EOF" on fast runners. The fake server now sits on pipes and responds only after reading the outgoing request, matching production ordering; survives -race -count=50. |
||
|
Vasanth T
|
c4c14d62d4 |
fix(oauth): make xAI / Hugging Face OAuth actually launch from the wizard (#355)
The provider wizard offered "Sign in with OAuth" for xAI and Hugging Face, but clicking did nothing: the login managers were built without an env, so the preset opt-in (ZERO_OAUTH_ALLOW_PRESETS) read as unset, ResolveConfig errored before any browser step, and the error was never rendered on the provider list. Only ChatGPT worked, because its login path force-enables presets. Add ManagerOptions.AllowPresets, which layers the opt-in onto the manager env (snapshotting the process environment so ZERO_OAUTH_<NAME>_* overrides survive envValue's hermetic-map rule) without the operator exporting the flag. Set it on every user-initiated OAuth manager: the wizard loopback login, the device-code prepare/complete, the post-login token refresh, the runtime resolver that authenticates model calls, and `zero auth login`. This fixes xAI end to end (it ships a public client_id, so the browser now opens). Also surface a failed OAuth attempt on the provider list (renderProviderStep) so the click is no longer a silent no-op, with a Hugging Face hint pointing at app registration + ZERO_OAUTH_HUGGINGFACE_CLIENT_ID (HF ships no public client_id, so it still requires that one env var by design). |
||
|
Vasanth TandClaude Opus 4.8
|
72a4229c2a |
fix(oauth): make ChatGPT (Codex) login work end-to-end (#264)
* fix(oauth): add missing api.connectors scopes for ChatGPT OAuth preset * fix(oauth): add missing authorize params for ChatGPT OAuth (id_token_add_organizations, codex_cli_simplified_flow, originator) * fix(oauth): use localhost + /auth/callback for ChatGPT redirect URI (matches Codex CLI registration) * fix(oauth): use fixed port 1455 + localhost/auth/callback for ChatGPT (matches opencode + Codex CLI client registration) * chore: gofmt chatgpt.go * refactor(oauth): use RedirectURIWithHost for ChatGPT callback; drop dead Port() RedirectURIWithHost and Port() were added but never called — chatgpt.go hardcoded "http://localhost:1455/auth/callback" as a literal, duplicating both the port and path. Use RedirectURIWithHost("localhost", "/auth/callback") so the redirect_uri derives its port from the listener (single source of truth via the new chatgptCallbackPort constant), and remove the unused Port() method. Also make the fixed-port bind failure actionable: ChatGPT's client registration requires exactly port 1455, so we can't fall back to a random port — but if 1455 is already bound (a prior Zero/Codex login still open) the error now says so instead of surfacing a raw OS bind error. Tests: RedirectURIWithHost host/path formatting, and that the listener captures the code on the /auth/callback path (not just /callback) — both were previously uncovered. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix(oauth): restore api.connectors scopes for ChatGPT preset (review fix) The fixed-port commit accidentally reverted internal/oauth/presets.go back to only openid/profile/email/offline_access, dropping the api.connectors.read and api.connectors.invoke scopes this PR exists to add — so authorize URLs still omitted them and the authorize_hydra_invalid_request failure was not actually fixed (caught in review by anandh8x). Restore both connector scopes in the chatgpt preset and pin them with tests: - TestResolveConfigChatGPTPreset now asserts both connector scopes are present (not just allowed). - New TestChatGPTAuthorizeURLIncludesConnectorScopes inspects the generated authorize URL and asserts the scope param carries api.connectors.read and api.connectors.invoke, so a future revert is caught end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix(oauth): read chatgpt_account_id from the nested OpenAI auth claim (P0) extractChatGPTAccountID read chatgpt_account_id as a TOP-LEVEL id_token claim, but auth.openai.com namespaces it under the "https://api.openai.com/auth" claim object (the Codex CLI reads only from there). Against a real token the top-level lookup returned "", so token.Account stayed empty → the codex provider omitted the required chatgpt-account-id header on every request → Cloudflare/backend 401. The login itself succeeded (bearer minted), so the failure was silent: the user authenticated fine, then every Codex API call 401'd. The unit tests passed only because the fixture put the claim at the top level, masking the bug. Read the nested path first (id_token[ns][chatgpt_account_id]), matching codex; keep a bare top-level claim as a forward-compat fallback only. Tests now build the fixture with the nested shape a real token uses, plus regressions: a top-level decoy must not shadow the nested value, and the top-level fallback still works when the namespace is absent. Found by an adversarial audit against the codex reference (same OAuth client). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> |
||
|
KRATOS
|
a981120432 |
Audit tail: remaining medium/low correctness fixes (CLI, config, OAuth, tools, LSP, TUI) (#275)
* audit tail: fix remaining medium/low correctness findings A sweep of the remaining audit findings across CLI, config, OAuth, tools, LSP, and the TUI. Each fix has a regression test; build/vet/gofmt clean and the touched packages' suites pass. CLI - `zero -p "<prompt>"` now forwards the prompt as the inline `--prompt=<value>` form so a prompt whose first character is a dash (e.g. `zero -p "-foo"`) is taken verbatim instead of being rejected as a missing flag value (matches the cron path). Config - mergeProfile no longer drops parseThinkTags across profile merges; an explicit value carries over and a nil (unset) never clobbers an existing one. OAuth - Refresh carries the existing token_type forward when the refresh response omits it (previously silently lost). - RefreshScheduler.Stop resets started/done so the scheduler can be started again instead of being permanently inert after the first Stop. Tools - apply_patch header parsing takes the whole post-prefix value (dropping a trailing tab timestamp and undoing git's C-style quoting) instead of splitting on spaces, so a diff target whose name contains spaces is recognized. LSP - Server.Shutdown honors its context (kills immediately on cancel rather than waiting out the full grace window), and Manager.Shutdown shuts servers down concurrently so total shutdown is bounded by one grace window, not N×grace. TUI - /theme auto re-probes the terminal background (RequestBackgroundColor) instead of reusing a stale startup reading, so switching to auto re-detects light/dark. - Specialist card: wire the tool-call count from progress events (was always 0) and omit the token segment when the total is 0 rather than show "0 tokens". - Resume: extract the tool-call row id the same way as the result row and the specialist pre-pass (toolCallId first), so call/result dedup and the failed-Task visibility rule key on the same string. - Plan panel: height takes an injected clock (testable, consistent with render), and duplicate-content steps inherit distinct prior timestamps positionally. * tui: test that /theme auto re-probes the terminal background (M17) The M17 fix lives in handleSubmit's command dispatch (`return m, tea.RequestBackgroundColor` when the mode is auto), which no test exercised — only the handleThemeCommand helper was covered. Add a dispatch-level test: /theme auto must return the background re-probe command, /theme dark must not. Verified to fail if the dispatch is reverted to `return m, nil`. |
||
|
KRATOS
|
9e800b4ffc |
fix five concurrency/leak bugs (swarm, specialist, oauth, mcp, cron) (#265)
Surfaced by a full-tree audit; each ships a regression test where the behavior is deterministically testable. - swarm: deliver a handoff's mailbox note BEFORE registering the new task, so a mailbox failure can't leave a phantom pending task in the coordinator. - specialist: put a foreground sub-agent child in its own process group and group-kill on cancel/timeout (+ WaitDelay backstop), so a build/server it forked dies with it instead of orphaning. - oauth: serialize on-demand token refresh per key (keyed mutex) and re-load inside the lock, so parallel callers don't each spend the single-use refresh token (all but one would otherwise fail at the token endpoint). - mcp: treat a "source already gone" rename during legacy-token migration as success, so a concurrent migrator doesn't wrongly drop an OAuth MCP server. - cron: claim a due job atomically (advance NextRunAt under the per-job lock) before running it, so two schedulers can't double-fire the same job. |
||
|
KRATOS
|
b0a280eaf6 |
audit: fix four high-severity concurrency/correctness bugs (#261)
Surfaced by a full-tree audit; all are latent (the suite passed) and live in race windows / error edges the tests don't exercise. Each ships a regression test. - swarm: liveAgents() now counts queued specs, so AdoptOrphans can no longer re-dispatch (double-execute) a task whose member is merely waiting for a free slot over the concurrency cap. - cron/hooks/oauth: stale-lock reclaim is now atomic (rename aside, then verify-and-restore if it turns out fresh) instead of a blind Remove that let two racers both reclaim and hold the lock; also falls through to the bounded wait instead of hot-spinning when a reclaim never wins. - lsp: a dead language-server session is evicted and restarted instead of being returned forever, so one server crash no longer permanently breaks diagnostics (and spuriously fails self-correct every iteration). - config: an API-key env var no longer overwrites a same-named compatible-transport provider's kind when no base URL is supplied. |
||
|
KRATOS
|
1c30f4c117 |
audit fixes: cron lock, accounting race, O(n²) hot paths, /style wiring (#260)
* audit fixes: cron lock, accounting race, O(n^2) hot paths, /style wiring
Fixes five verified findings from a code audit. Two related findings were already
addressed upstream and three need larger/separate work (noted below).
H-1 cron store: cross-process lock for the read-modify-write.
Two schedulers seeing a job due raced the metadata read-modify-write; fireJob
even documented it ("full atomicity needs file locking"). Adds a per-job O_EXCL
lock (mirroring internal/oauth's pattern, stale-reclaim included) and a Mutate
primitive doing the re-read+modify+write atomically. Update/Remove lock too. The
lock is held only for the millisecond RMW, never across a job's exec.
M-5 specialist accounting: close the check-then-append race.
specialistEventExists (unlocked ReadEvents) + a later AppendEvent let two
finishers (TaskOutput poll vs onExit, even cross-process) both pass the check and
write duplicate stop/usage events. Adds sessions.Store.AppendEventUnlessExists
(check + append atomic under the session lock) and routes the stop and usage
paths through it.
M-2 hooks audit: O(1) sequence via tail read.
append re-read and scanned the whole audit log every call to find the max
sequence (O(n^2) over a session). Reads only the file tail now; multi-process
safe (the log is a shared global file), unlike an in-memory counter which would
collide sequences across processes.
M-4 transcript rehydration: O(n^2) -> O(n).
The three resume/rewind/compact rehydration loops called appendTranscriptRow per
event, each re-scanning every existing row for the keyed-row dedup. A bulk
appendTranscriptRowsDedup builds the key set once; semantics are identical.
M-6 /style: actually shape responses.
The TUI /style command stored a style but never threaded it anywhere. Adds
agent.Options.ResponseStyle, a system-prompt directive for concise/explanatory/
review, and wires m.responseStyle through the run. "balanced" is a no-op (prompt
byte-identical to before).
Already addressed upstream (no change):
- M-1 forked-session double-count: Store.Fork already skips EventUsage events,
with a comment naming the exact double-count it prevents.
- M-3 lineage O(N^2): Tree already does a single store.List() scan and indexes
children in memory; no path calls List per node.
Deferred (need larger/separate changes):
- M-7 escalate_model in the TUI needs a ModelSwitcher that builds a provider
mid-run; registering the tool alone yields a tool that errors on call.
- L-1 MCP SSE reconnect is a stream-lifecycle change (low-likelihood after the
8 MiB per-event cap).
- L-2 dead-code removal belongs in a dedicated cleanup sweep.
Tests: per-fix unit tests including concurrency (cron Mutate, sessions
AppendEventUnlessExists) under -race; full go test ./... green.
* fix(windows): treat ERROR_ACCESS_DENIED as lock contention (cron + oauth)
The Windows smoke run failed TestMutateIsAtomicUnderConcurrency with
"...lock: Access is denied" (FireCount 19/20). On Windows a concurrent holder's
os.Remove leaves the lock file in a delete-pending state, so an O_EXCL create
races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist — the
retry loop only treated ErrExist as contention and hard-failed.
Treat os.ErrPermission as contention (retry) in the new cron lock and in the
oauth lock it mirrors (oauth's own concurrent test exposed the same pre-existing
bug under the added load). Unix is unaffected — it returns ErrExist.
* fix: address review — cross-process atomic audit append + AppendRun guard
Addresses the review feedback on this PR.
P1 (blocking): AuditStore.append is now cross-process atomic.
The O(1) tail read observes another process's prior appends, but the read-then-
append was not atomic across processes — two could both read sequence N and write
N+1. Wraps lastSequence()+append in a per-file O_EXCL lock (hooks/lock.go, same
stale-reclaim + Windows ErrPermission handling as cron/oauth); store.mu stays as
the in-process fast path. Adds TestAuditStoreSequenceConcurrent (4 stores x 12
goroutines on one file -> unique dense sequences), which the prior sequential
test could not catch. Walked back the comment that oversold the tail read.
P2: AppendRun no longer resurrects a removed job.
fireJob's AppendRun ran after Mutate released the per-job lock; a Remove in that
window let AppendRun's MkdirAll recreate the dir with an orphaned runs.jsonl.
AppendRun now takes the per-job lock and bails if metadata.json is gone. Adds
TestAppendRunDoesNotResurrectRemovedJob.
P2: lastSequence ReadAt tolerates a short final read.
ReadAt may return io.EOF with n < len(buf) if the file shrank between Stat and
the read; treat EOF as non-fatal and use buf[:n] (matches lastEventSequence).
P3: oauth stale-reclaim no-op removed.
The double os.ReadFile "stale == data" guard compared the file to itself read
twice in a row (always true); staleness is decided by the mtime check, so the
guard is dropped (behavior-preserving).
Tests under -race; cron/hooks/oauth green. A shared filelock helper would DRY the
three near-identical locks — left as a follow-up to avoid churning the
already-green cron/oauth packages here.
|
||
|
Vasanth T
|
ab3d09d78e |
feat(oauth): Hugging Face + ChatGPT Codex as first-class built-in OAuth presets (#245)
* feat(oauth): add Hugging Face + ChatGPT Codex as first-class built-in OAuth providers
Adds two new built-in OAuth presets that round out the existing OpenRouter and
xAI surfaces, so the TUI `/provider` OAuth chooser and `zero auth login` work
out of the box for these providers.
Hugging Face
- Preset scopes, endpoints, and OIDC issuer are pre-filled; the operator
supplies a "public" OAuth client_id (no secret) via
`ZERO_OAUTH_HUGGINGFACE_CLIENT_ID` after registering an app at
huggingface.co/settings/applications/new.
- Default flow is RFC 8628 device code, so headless/SSH is the first-class
path (matches the existing xAI posture).
- The bearer works against the OpenAI-compatible Inference Providers router
at `https://router.huggingface.co/v1` for hundreds of OSS models.
ChatGPT (Codex backend)
- Uses the publicly-shipped Codex CLI client identity
(`app_EMoamEEZ73f0CkXaXp7hrann`), opt-in via `ZERO_OAUTH_ALLOW_PRESETS=1`,
overridable via `ZERO_OAUTH_CHATGPT_*`.
- The bespoke `provideroauth.ChatGPTLogin` extracts the `chatgpt_account_id`
claim from the OIDC ID token (added as `IDToken` on the `oauth.Token`
struct) and stores it on `Token.Account` so the Codex provider can inject
it as a header on every request.
- A new `internal/providers/openai/codex.go` wraps the openai provider with
the Codex-flavored request shape: `originator: codex_cli_rs`,
`chatgpt-account-id: <claim>`, and a branded User-Agent. The factory
branches off the catalog id (not the provider kind) so other
openai-compatible providers keep the standard openai path unchanged.
- The openai provider gained a `SetRequestExtra` callback so the Codex
wrapper can inject per-request headers without coupling the openai
provider to Codex.
CLI / TUI
- `zero auth chatgpt` is a one-shot login command (analog to
`zero auth openrouter`); the TUI's OAuth chooser now lists both new
providers automatically (the list is built from
`providercatalog.OAuthProviders()`).
- The provider-wizard visible-row cap was bumped from 8 to 10 so the
common providers (OpenAI, Anthropic, Google, Ollama, OpenRouter, the
new Hugging Face, the new ChatGPT, Groq, …) all fit without scrolling
the test fixtures.
Tests
- `internal/oauth/presets_test.go`: presets resolve correctly with the
expected endpoints, scopes, and flows; env overrides win; Hugging Face
preset stays inert without a client_id (the safety posture the existing
xAI tests assert).
- `internal/providercatalog/oauth_test.go` + `catalog_test.go`: catalog
now exposes 4 OAuth providers (`openrouter`, `huggingface`, `chatgpt`,
`xai`); the `chatgpt` descriptor is `OAuth=true` with
`OAuthDeviceFlow=false` (Codex is loopback-only) and `RequiresAuth=true`
via an inline IIFE that sets it after the openAICompat helper.
- `internal/provideroauth/chatgpt_test.go`: ID-token claim extraction
round-trips (happy path, missing claim, missing token, tampered JWS,
non-string claim); the full login flow runs against a stub auth server
and verifies the preset's client_id reaches the token endpoint.
- `internal/providers/openai/codex_test.go`: every request gets the three
Codex headers; the 401-refresh retry path keeps them on the second
attempt; configured baseURL wins; default originator is `codex_cli_rs`;
branded User-Agent wins over the openai default; AccountResolver is
consulted when AccountID is empty; the header is omitted when the
resolver says no.
Out of scope (separate PRs): multi-account / profiles, generic
template-driven OAuth wizard, MCP / provider OAuth store unification,
subscription-proxy autoconfig.
* fix(ci): gofmt chatgpt_test.go and provider_wizard.go
The ubuntu smoke job's "Check formatting" step lists these two files as
unformatted (struct field alignment). The local gofmt run on Windows
also lists ~170 other files, but those are CRLF-vs-LF noise from
checkout and not real gofmt issues on Linux; the CI only flags the
two struct-alignment changes below.
- internal/provideroauth/chatgpt_test.go: align chatgptTestServer fields
- internal/tui/provider_wizard.go: align ChatGPTOptions literal fields
* fix(codex): hit the /responses path and resolve account id per request
Addresses the two findings in anandh8x's CHANGES_REQUESTED review on #245.
The Codex provider previously routed the chat-completions body to
{baseURL}/chat/completions, which is not the path the Codex backend serves
(chatgpt.com/backend-api/codex/responses, the Responses API). Add an
Endpoint field to openai.Options so a flavor can target a sibling path on
the same host; the Codex constructor sets it to {baseURL}/responses and
falls back to the openai transport unchanged for everything else.
The factory was also passing the OAuth token's Account field as a static
AccountID at construction time. A refresh that rotates the token (and
its account claim) would leave a stale value on the wire. Drop the
static AccountID from the factory wiring; the per-request
AccountResolver (which reads the live store) is now the sole source, so
a refresh takes effect on the next outgoing request.
Tests:
- codex_test.go: handler mounted on /responses; expected path updated;
TestCodexProviderResolverIsConsultedOnEveryRequest pins the per-request
resolver behavior (a refresh-style sequence of calls picks up the new
account id on the second attempt).
- factory_test.go: expected request path updated to /responses.
* Addresses the Codex path shape on /responses.
The previous fix (
|
||
|
Anandan
|
2cf6fd2334 |
Add request permissions flow (#248)
* Add request permissions flow Adds a runtime-handled request_permissions tool that can ask for turn or session filesystem/network grants and return the granted profile as structured JSON. Temporary turn grants update the shared sandbox scope and effective policy, then clean up at run end; session grants persist in memory for the session. Read grants now stay read-only through tool path resolution, policy evaluation, and native sandbox profile construction. The TUI renders request_permissions with grant-specific choices, including strict review, and Esc continues without granting permissions. Tested: GOCACHE=/tmp/zero-go-cache go test ./... * Align request permission grants with prompts Display and return the normalized grant roots that the sandbox actually applies, so file-like permission requests no longer show a narrower path than the directory root being granted. Also makes the network prompt explicit about all outbound access and relabels the strict option as a model review request instead of implying enforced command review. Tested: GOCACHE=/tmp/zero-go-cache go test ./... * Retry transient OAuth secret read locks Windows can briefly deny reads of the freshly published token secret while concurrent OAuth secret creation converges. Treat Windows sharing and lock violations as transient and retry those reads before returning an error. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/oauth Tested: GOCACHE=/tmp/zero-go-cache GOOS=windows GOARCH=amd64 go test -c ./internal/oauth -o /tmp/zero-oauth-windows.test.exe Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/sandbox ./internal/agent ./internal/tui ./internal/tools ./internal/specialist ./internal/oauth Tested: GOCACHE=/tmp/zero-go-cache go test ./... |
||
|
Anandan
|
eb608741b0 |
Align permission prompt rendering (#246)
* Align permission prompt rendering Remove normal permission prompt risk labels from focused cards, transcript rows, and permission detail text so risk metadata is not presented as the permission UI contract. Rename the recoverable deny option from a redirect-style prompt to a continue-without-running label that matches its current behavior. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/tui ./internal/agent ./internal/cli Tested: GOCACHE=/tmp/zero-go-cache go test ./... Tested: git diff --check * Tighten permission prompt fallbacks Use conservative allow/deny choices when a permission request lacks backend-supplied decisions, avoiding impossible session or persistent grant options. Document that a failed session-grant recording attempt must not deny the current user-approved tool call; it only means a later matching call prompts again. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/tui ./internal/agent Tested: GOCACHE=/tmp/zero-go-cache go test ./... Tested: git diff --check * Split permission deny and cancel decisions Add an explicit cancel permission decision that aborts the active run while keeping deny as a recoverable tool refusal. Expose cancel choices for command and apply-patch approval prompts, keep apply-patch off the recoverable deny path, and stop offering persistent always approvals for command/apply-patch prompts until scoped prefix approval exists. Classify cancel decisions as permission-decision events for TUI session logs and exec JSON output. Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/agent ./internal/tui ./internal/cli Tested: GOCACHE=/tmp/zero-go-cache go test ./... Tested: git diff --check * Stabilize OAuth secret creation Publish encrypted OAuth token secrets atomically under a short-lived creation lock so concurrent first-run callers never observe a half-written secret file. This fixes the Zero Review failure in TestLoadOrCreateSecretConcurrentConverges. Tested: go test ./internal/oauth -run TestLoadOrCreateSecretConcurrentConverges -count=100 Tested: go test ./internal/oauth Tested: GOCACHE=/tmp/zero-go-cache go test ./internal/agent ./internal/tui ./internal/cli Tested: GOCACHE=/tmp/zero-go-cache go test ./... Tested: git diff --check |
||
|
KRATOS
|
a0b9876b69 |
OAuth / account login for providers (OpenRouter, xAI) + subscription-proxy support (#217)
* providers: authenticate model calls with an OAuth login when present
Foundation for account/subscription login: when a user is logged in via
`zero auth login <name>`, the OpenAI and Anthropic providers now authenticate
model requests with that OAuth bearer token, falling back to the API key when
there is no login. Auto-select, per the agreed design — no config needed.
- providerio.SendWithAuthRetry: issues a request with an OAuth bearer (from a
TokenResolver) or the API key, and retries ONCE with a force-refreshed token
after a 401. A 401 means the server rejected the request without processing a
completion, so replay-after-refresh is safe — the same no-duplicate-work basis
SendWithRetry uses for 429/503. Never sends API key and bearer together.
- openai + anthropic providers take an optional OAuthResolver and use
SendWithAuthRetry. When OAuth is active the credential goes on Authorization
(so Anthropic's x-api-key is correctly omitted). Gemini is unchanged.
- providers.New threads Options.OAuthResolver to those two providers.
- cli.oauthResolverForProfile builds the resolver from the unified oauth store:
it attaches a resolver ONLY when a login already exists for the provider
name, so API-key users incur no per-request store lookups. The resolver uses
Manager.GetFresh (refresh near expiry) and Manager.Handle401 (force refresh on
the 401 retry).
- oauth: added ErrNoToken sentinel so "not logged in" is distinguishable from a
real refresh failure (login removed mid-session → clean API-key fallback).
Tests (httptest): API-key fallback (nil resolver / ok=false), bearer wins +
never-both, 401→force-refresh→retry, stops after one retry, resolver error
surfaced. Local run check (real binary + mock OAuth + mock OpenAI-compatible LLM
requiring the bearer): with a login, `zero exec` sends Bearer <oauth-token> and
gets a completion; without a login it falls back to the (wrong) API key and the
mock returns 401 — proving the OAuth path.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/
govulncheck(0)/deadcode(no new) all pass.
* providers: add ChatGPT-subscription-via-proxy preset + OAuth/subscriptions docs
Follows verified research (cross-checked across ~30 sources) into using ChatGPT
and Claude subscriptions: a subscription OAuth token does NOT work as a standard
bearer on the public APIs. OpenAI's token only works against the Cloudflare-gated
ChatGPT backend (headless clients get cf-mitigated 403); Anthropic rejects
third-party subscription OAuth, requires Claude-Code identity spoofing, 400s on
tool calls (Max overage lane), and is contractually banned with active
enforcement. So zero does NOT call those backends or spoof those clients.
Instead, the supported pattern is a local proxy that holds the subscription
session and exposes a clean OpenAI/Anthropic-compatible endpoint on 127.0.0.1;
zero points at it (it already supports custom base URLs, and C1 supplies OAuth
bearer for gateways that issue standard tokens).
- providercatalog: add `chatgpt-proxy` ("ChatGPT (local OAuth proxy)") — an
OpenAI-compatible, local, no-API-key preset defaulting to the established
proxy port (http://localhost:10531/v1), base URL overridable. Claude uses the
existing `custom-anthropic-compatible` entry (no canonical Claude-proxy port
to hardcode — not invented).
- docs/oauth-subscriptions.md: full recipe — built-in OAuth login (C1), the
verified reason subscriptions need a proxy, copy-paste config for ChatGPT and
Claude via a local proxy, and the sanctioned alternatives (API key; spawning
the real `claude` CLI for Anthropic subscription automation).
Local run check: a config using `catalogID: chatgpt-proxy` (base URL pointed at a
mock OpenAI-compatible proxy) routes `zero exec` through the proxy with no auth
header and returns a completion.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(clean)/govulncheck(0)/
deadcode(no new) all pass. No new module deps.
* provideroauth: OpenRouter browser PKCE login (mints an API key) + `zero auth openrouter`
Adds the cleanest real OAuth login surfaced by the provider survey: OpenRouter's
loopback PKCE flow. It needs no client_id (public), uses S256, and the exchange
mints a normal OpenRouter API key — so no token-spoofing or vendor-backend gating
is involved (unlike ChatGPT/Claude subscriptions).
- internal/provideroauth.OpenRouterLogin: binds a 127.0.0.1 loopback, opens/prints
https://openrouter.ai/auth?callback_url=…&code_challenge=…&code_challenge_method=S256,
captures the code, and POSTs it + the PKCE verifier to
/api/v1/auth/keys → {key}. PKCE binds the code, so no separate CSRF state is
needed. Injectable base URL / HTTP client / browser-opener for tests.
- cli: `zero auth openrouter` runs the flow and prints the minted key with usage
guidance. ZERO_OPENROUTER_BASE_URL overrides the endpoint (self-hosted/tests).
Tests: full flow over httptest (mint, missing-code, non-2xx exchange, empty key,
browser-error). Local run check via the real binary against a mock: the command
emits the correct PKCE authorize URL, the loopback callback is captured, and the
key is minted end-to-end.
First of the in-wizard OAuth providers; the wizard "Log in with OAuth" option
(next commit) calls this same flow and saves the key to the profile.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/
govulncheck(0)/deadcode(no new) all pass. No new module deps.
* oauth: built-in provider presets + xAI (Grok) preset
Adds an overridable preset layer to the env-driven OAuth registry so well-known
providers work without per-field env configuration. ResolveConfig now seeds each
field from a baked-in preset and lets the matching ZERO_OAUTH_<NAME>_* env var
override it (env wins); flow falls back to the preset then loopback.
First preset: xAI (Grok) — standard OIDC, PUBLIC client (no secret), loopback +
device. The access token is accepted directly as a bearer on api.x.ai/v1 (an
OpenAI-compatible endpoint), so the existing C1 wiring uses it for an xai provider
profile — no header/identity spoofing. `zero auth login xai` now works out of the
box. CAVEATS (documented in code + help): the client_id is a public Grok-CLI
client not formally documented by xAI (may change; override via env), and use
requires a SuperGrok / X Premium+ subscription.
Tests: preset resolution, env-overrides-preset (per field + flow), and that a
provider with neither preset nor env still errors. Local run check: `zero auth
login xai --device` (endpoints pointed at a mock, client_id NOT overridden) logs
in and the mock confirms the preset client_id b1a00492-… was sent.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(no new)/govulncheck(0)/
deadcode(no new) all pass. No new module deps.
* tui: in-wizard "Log in with OAuth" option (OpenRouter) + browser opener
Adds the credential-step OAuth login the setup wizard was missing. For a provider
that supports a usable browser OAuth flow (currently OpenRouter), the credential
screen now offers "ctrl+o to log in with OAuth in the browser (no key needed)"
alongside pasting a key.
Flow: ctrl+o → the wizard marks itself oauthPending and dispatches an async
tea.Cmd that runs provideroauth.OpenRouterLogin (opens the browser via the new
internal/browser opener, captures the loopback callback, mints a key). While it
runs the step shows "Opening your browser… (or run: zero auth openrouter)";
Esc cancels. On success the minted key fills the credential and the wizard
advances to model selection; on failure the redacted error is shown and the user
can retry or paste a key.
- internal/browser: cross-platform OpenURL (open / xdg-open / rundll32) with a
pure, unit-tested per-OS command builder.
- provider_wizard: oauthPending/oauthErr state, providerWizardSupportsOAuth
(OpenRouter only — ChatGPT/Claude use the proxy preset, others paste a key),
the async cmd + result msg, and credential-step render/keys.
- model.Update routes the OAuth result message.
Tests: openCommand per-OS; supportsOAuth (openrouter yes / openai no); ctrl+o
starts OAuth for OpenRouter and is a no-op otherwise; apply-success advances +
applies the key; apply-error stays + surfaces the message; render shows the hint
and the pending state. The interactive browser flow itself is the same one C3
verified end-to-end against a mock.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new — the
SA1019 mouse nits are pre-existing on origin/main)/govulncheck(0)/deadcode(no new).
No new module deps.
* docs: cover the built-in OpenRouter + xAI OAuth logins
Document the two turnkey OAuth providers added in this branch (OpenRouter browser
key-mint via `zero auth openrouter` / wizard ctrl+o; xAI via `zero auth login
xai`), alongside the existing subscription-proxy and custom-provider guidance.
* tui: method-first provider setup — a prominent "Sign in with OAuth" path
Reworks `/provider` setup to lead with a connect-method chooser, modeled on Codex
/ Claude Code / gemini-cli (verified via a UX survey), so OAuth is a first-class,
separate option instead of a key buried at the credential step.
New first step "How do you want to connect?":
❯ Sign in with OAuth (default when any OAuth provider exists)
One-click browser login, no API key to copy (OpenRouter, xAI).
Paste an API key / browse providers
Any of 20+ providers, a local model, or a subscription via proxy.
- OAuth path → a dedicated list of ONLY OAuth-capable providers
(providercatalog.OAuthProviders()), each with a mode badge; selecting one
runs the browser login (OpenRouter mints a key; xAI/preset providers store a
refreshable token via the engine) and jumps straight to model selection,
skipping the key/endpoint steps. A waiting screen shows progress + a CLI
fallback; Esc cancels; errors are redacted and retryable.
- Browse path → the existing full catalog → key/endpoint/model, unchanged.
- ChatGPT/Claude are NOT offered as in-app OAuth (they can't work) — they remain
reachable as the proxy/custom providers under "browse".
- providercatalog: Descriptor gains OAuth/OAuthMintsKey/OAuthDeviceFlow; openrouter
+ xai are flagged; OAuthProviders() is the single source of truth.
- wizard: new method step + OAuth provider list + per-provider OAuth dispatch
(reuses provideroauth.OpenRouterLogin and the oauth engine login), waiting/
render/keys/step-line/retreat all updated; mouse + keys ignored while a login
is in flight.
Tests: catalog OAuth flags + OAuthProviders(); method chooser OAuth vs browse
paths; OAuth dispatch from the list; retreat→method; render shows the chooser +
provider list; existing wizard tests updated for the new first step. All tui tests
pass under -race.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(no new)/govulncheck(0)/
deadcode(no new) all pass. No new module deps.
* docs: document the /provider "Sign in with OAuth" method chooser
* tui: mirror the "Sign in with OAuth" method chooser in first-run onboarding
First-run setup (onboarding.go) was a separate flow that still showed its bare
provider list. Add the same connect-method chooser so new users get the prominent
OAuth path too, keeping /provider and first-run as one mental model.
- New setupStageMethod (after Welcome): "How do you want to connect?" with
"Sign in with OAuth" (default) and "Paste an API key / browse providers".
- OAuth path filters the provider list to OAuth-capable providers, runs the same
browser login (OpenRouter mints a key → captured as the setup credential; xAI
stores a refreshable token and the profile is named after the provider so the
runtime resolver attaches the bearer), shows a waiting screen, then jumps to
model selection (skipping endpoint/name/credentials). Esc cancels; errors are
redacted.
- Browse path is the existing flow, unchanged. setupStages()/navigation/back/
progress/footer/mouse all updated; OAuth result routed via model.Update.
Tests: method chooser OAuth path (OAuth-only list + retreat clears it) and
apply-OAuth-success advances to model with the minted key; existing onboarding
tests updated for the new step (pressSetupContinue transparently skips it). All
tui tests pass under -race.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(no new)/govulncheck(0)/
deadcode(no new) all pass. No new module deps.
* catalog+tui: show ChatGPT/Claude in the OAuth chooser as local-proxy entries
The "Sign in with OAuth" chooser listed only the real-OAuth providers (OpenRouter,
xAI), so ChatGPT and Claude looked missing. They can't do real in-app OAuth — a
subscription token is Cloudflare-gated (ChatGPT) / ToS-restricted for third-party
use (Claude) — so instead of hiding them or faking a login, list them honestly as
"subscription · needs a local proxy" entries that route into proxy setup.
- catalog: new Descriptor.OAuthProxy flag + OAuthProxyProviders(); mark
chatgpt-proxy and a new claude-proxy (local Anthropic-compatible, keyless)
entry. localAnthropic() helper. Proxy entries are not OAuth (no fake login).
- wizard + onboarding: the OAuth list now appends the proxy entries after the
real OAuth providers (catalog order keeps real ones first). Selecting a proxy
entry leaves OAuth mode and drops into the normal browse flow with the proxy
provider selected and its default URL/model pre-filled, so the user just
points Zero at the local proxy they run. No browser login is launched.
- providerWizardNeedsEndpoint now true for proxy entries (the local port
varies); faint badge "subscription · needs a local proxy".
- docs/oauth-subscriptions.md: chooser now shows the four entries and explains
the proxy routing.
Tests: catalog classification (OAuthProviders vs OAuthProxyProviders, flags);
wizard + onboarding proxy-entry routing (leaves OAuth mode, starts no login,
lands on endpoint with the URL pre-filled). Full suite green under -race;
staticcheck/govulncheck/deadcode clean. No new deps.
* oauth+tui: add xAI device-code (RFC 8628) login to the OAuth chooser
The OAuth chooser only did browser/loopback login, which can't work on a headless
or SSH box (no browser to open). Add device-code as a first-class option for
providers that support it (xAI).
- oauth.Manager: PrepareDeviceLogin (request code) + CompleteDeviceLogin (poll +
store), splitting the monolithic device flow so the UI can display the
user_code + verification URI while it waits. CLI Login(Device:true) unchanged.
- tui (shared oauth_device.go): oauthPreferDeviceFlow() picks device by default
on SSH / headless Linux (no DISPLAY) or when ZERO_OAUTH_DEVICE is set;
oauthDevicePrepare/Complete drive the two phases off the UI goroutine.
- /provider wizard + first-run onboarding: device-capable providers show
"Enter sign in · d device code" — Enter uses the browser (or device if
headless), "d" forces device. The waiting screen shows the code + URL to
enter elsewhere, then polls; success advances to model selection. Esc cancels;
errors are redacted; device state is cleared on success/cancel/back.
- docs: document the device-code path and ZERO_OAUTH_DEVICE.
Tests: manager Prepare/Complete two-phase; wizard + onboarding 'd' shortcut,
device-code display + poll dispatch, error surfacing, and success clearing device
state. Full suite green under -race; staticcheck/govulncheck/deadcode clean. No
new deps.
* tui: authenticate model discovery with the OAuth token so the live list shows
After an xAI sign-in the bearer lives in the token store, not as a pasted key, so
the post-login model step ran /models with no credential → it always fell back to
the curated list (with a discovery error). Now the user picks from the real live
model list after OAuth, like other CLIs.
- oauthStoredToken() resolves a fresh stored token for a token-login provider.
- wizard + onboarding discovery resolve and inject that token into the discovery
profile inside the async goroutine (token refresh is network I/O); OpenRouter
still uses its minted key, API-key users are unchanged. Discovery timeout
bumped 8s→12s to cover an on-demand refresh.
- The resolved token is added to the redaction secret set so it can never leak
into a surfaced discovery error. If discovery still fails (offline), the
curated per-provider fallback list is shown as before — the user always gets a
pickable list.
Tests: oauthStoredToken reads a seeded login / returns empty when absent;
wizard discovery injects the stored token into the /models profile. Hermetic via
ZERO_OAUTH_TOKENS_PATH. Full suite green under -race; staticcheck/govulncheck/
deadcode clean. No new deps.
* catalog+tui: drop ChatGPT/Claude from the OAuth chooser — OpenRouter + xAI only
Reverts the proxy-entry listing: the "Sign in with OAuth" chooser now shows only
the providers that do real OAuth (OpenRouter, xAI). ChatGPT/Claude looked
confusing there since they can't actually sign in — they remain reachable via
"Paste an API key / browse providers" + the local-proxy recipe in
docs/oauth-subscriptions.md.
- catalog: remove the Descriptor.OAuthProxy flag, OAuthProxyProviders(), the
oauthProxyProvider/localAnthropic helpers, and the claude-proxy entry.
chatgpt-proxy stays as a plain browse/config entry (its pre-existing role).
- wizard + onboarding: OAuth list = OAuthProviders() again; removed the
proxy→browse routing, the OAuthProxy badge, and the OAuthProxy endpoint rule.
- device-code + OAuth-token model discovery (the later features) are untouched.
- docs + tests updated to the two-entry chooser.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck/deadcode all
clean. No new deps.
* cli: recognize a stored OAuth login as a provider credential
A provider authenticated via OAuth (e.g. `zero auth login xai`) has no inline
key or env var, so setupRequired treated it as unconfigured and forced onboarding
on every launch — and the active provider could be passed over for a different
one. setupRequired now also counts a stored OAuth login (read from the token
store) as a credential, so an OAuth-logged-in provider stays usable without
onboarding.
Tests: providerHasOAuthLogin (by name/catalog id); setupRequired returns false
for an OAuth-logged-in provider, true for a keyless one with no login.
* Address review feedback on OAuth provider login
Resolve CodeRabbit review comments on this PR:
- PrepareDeviceLogin: self-bound the network prepare (endpoint discovery +
device-code request) with opts.Timeout (default 5m) like Login, so a caller
passing an unbounded context still gets the timeout guarantee.
- CompleteDeviceLogin: bound the poll by the device code's own ExpiresAt so an
unbounded caller context cannot leave an in-flight request hanging past the
code's lifetime (PollDeviceToken already re-checks between polls).
- OAuthProviders(): return cloned descriptors, matching All()/the other
accessors, so callers can't mutate the shared catalog backing slices.
- Onboarding applySetupOAuth / applySetupOAuthDeviceCode: ignore a stale result
for a provider the user has since switched away from, so a late login can't
capture a credential against the wrong provider.
- Onboarding: surface a failed OAuth login on the provider list the user lands
on (the waiting screen that owned the error is gone once oauthPending clears),
so the failure isn't silent and the user can retry.
- Onboarding: hide the "Sign in with OAuth" method when this setup's own
provider list has no OAuth-capable entry, so it can't be selected into an
empty provider list (setupMethodOptions).
- docs/oauth-subscriptions.md: label the bare example fences (text) and update
the Anthropic policy section for the April 4 2026 third-party cutoff.
Adds tests for the descriptor-clone isolation and the method-option filtering.
* oauth: gate built-in presets behind ZERO_OAUTH_ALLOW_PRESETS
The xAI preset baked a third-party OAuth client_id into the default
credential path, which breaks the engine's documented invariant that no
third-party OAuth client identity is used unless the operator configures
it. Rather than drop the convenience entirely, make presets opt-in:
- ResolveConfig only consults builtinOAuthPresets when ZERO_OAUTH_ALLOW_PRESETS
is truthy (presetsAllowed); by default every field must come from
ZERO_OAUTH_<NAME>_* env, so the binary's preset client_id is never used
without an explicit opt-in.
- The "not configured" error now points the user at ZERO_OAUTH_ALLOW_PRESETS
when a preset exists, so the wizard/CLI failure is self-documenting.
- Refresh the package, Registry, and preset docs to describe the opt-in;
env still overrides any preset field.
- docs/oauth-subscriptions.md: document the flag for one-click xAI sign-in.
Keeps both: invariant-clean by default, one-click xAI for opted-in users.
Tests: preset resolution now requires the opt-in; added a gate-off test
asserting xAI stays inert (and the error names the flag) without it.
* oauth: read a stored token without resolving provider config
GetFresh resolved the provider OAuth config for every read via loadForKey,
even when the stored token was still valid and no refresh would happen.
With presets gated behind ZERO_OAUTH_ALLOW_PRESETS, that made a stored xAI
login unreadable (and stalled the proactive refresh scheduler) unless the
opt-in flag was set at read time — config that is only needed to refresh.
Split loadForKey into loadToken (token only) and resolveConfigForKey
(refresh-only). GetFresh now returns a still-valid token without touching
the config; only an actual refresh (or Handle401) resolves endpoints +
client. The scheduler's load uses loadToken too, so proactive refresh
survives a gated preset and only the refresh attempt needs the config.
Fixes the Smoke failures TestOAuthStoredTokenReadsStoredLogin and
TestProviderWizardDiscoveryUsesOAuthToken (a fresh stored token must be
readable without the preset opt-in).
* Address CodeRabbit re-review round 2 on OAuth provider login
- providerio SendWithAuthRetry: resolve the auth headers BEFORE dispatching, so
a resolver error returns immediately instead of letting SendWithRetry send an
unauthenticated request (leaking the path/body).
- tui handleMouse: ignore all mouse input (clicks + wheel) while a provider-wizard
OAuth login is pending, so a stray scroll can't change the selected provider
mid-login (mirrors the keyboard pending guard; the setup mouse handler already
did this).
- cli runAuthOpenRouter: reject unexpected args/flags (e.g. --json) instead of
silently running the login, matching the other auth subcommands.
- cli auth help: align the preset wording with the opt-in gate — openrouter works
out of the box; xai needs ZERO_OAUTH_ALLOW_PRESETS=1 or ZERO_OAUTH_XAI_*.
- onboarding_test: compute selectedMethod from m.setupMethodOptions() (the
setup-filtered list) instead of providerWizardMethodOptions(), which could index
past the end when OAuth is hidden.
- docs/oauth-subscriptions.md: sharpen the Anthropic timeline — Feb 19 2026 docs
update, then the April 4 2026 enforcement that stopped subscription OAuth tokens
working in third-party harnesses; current path is an API key or pay-as-you-go.
Adds tests for the no-dispatch-on-resolver-error path and openrouter arg rejection.
* Address CodeRabbit re-review round 3 on OAuth provider login
Two findings missed in the previous round:
- oauth manager resolveConfigForKey: resolve issuer metadata before refreshing.
It returned ResolveConfig's result without calling resolveEndpoints, so a
provider configured with only ZERO_OAUTH_<NAME>_ISSUER_URL (no TOKEN_URL) had
an empty token endpoint and could not refresh on GetFresh/Handle401. Thread the
context through and run discovery; adds a test that an issuer-only provider
refreshes via the discovered token endpoint.
- provider_wizard_oauth_test: assert openrouter is actually present in the OAuth
list before selecting it, so the test can't silently pass against a different
provider if openrouter is renamed/removed.
(The re-flagged PrepareDeviceLogin timeout is already handled — it self-bounds
with opts.Timeout, same as Login.)
* Address CodeRabbit re-review round 4 (nitpicks) on OAuth provider login
- presets scopesOrPreset: return a copy of the preset scopes so a caller
appending to cfg.Scopes can't mutate the shared global preset slice.
- provideroauth/openrouter: use bytes.NewReader(payload) instead of
strings.NewReader(string(payload)) (avoid a needless []byte->string copy).
- onboarding setupCredentialSummary: show "OAuth token" on the Ready screen for
an OAuth token-login provider (e.g. xAI) instead of "env var XAI_API_KEY" — it
is authenticated by a stored token, not a key. Key-minting OAuth (OpenRouter)
still resolves to a saved key.
(The flagged "Registry doc says no built-ins" was already corrected in the
preset opt-in commit — the doc now describes the ZERO_OAUTH_ALLOW_PRESETS gate.)
Adds tests for the scope-slice copy and the OAuth-token credential summary.
|
||
|
KRATOS
|
c38444157c |
Audit backlog: tui mouse API, notify webhook, swarm scheduling, daemon-remote bundles, MCP/oauth store unification, OS keyring, dead-code prune (#216)
* tui: migrate off the deprecated Bubble Tea mouse Type API (SA1019)
The mouse classification helpers checked both the current Button/Action pair and
the deprecated msg.Type / tea.Mouse* constants. Bubble Tea's parser always
populates Button+Action and only DERIVES Type from them, so the Type fallbacks
were pure redundancy (a left-button drag is Action==Motion, already covered by
mouseMotion). Drop them — clears all 11 SA1019 findings in internal/tui with no
behavior change. The left-drag test now asserts the Action==Motion path directly.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(SA1019 0)/
deadcode(no new) all pass.
* notify: wire the webhook sink behind an opt-in env var
The WebhookSink (Slack / generic JSON POST on turn completion) was fully
implemented, redaction-safe, and tested — but never attached to a Notifier, so
deadcode flagged its entire machinery (NewWebhookSink, Emit, text, redactLinks,
log, eventType, readSnippet) as unreachable.
Wire it via a small, testable helper, MaybeAddWebhookSink(n, env, logf):
- reads ZERO_NOTIFY_WEBHOOK_URL (+ optional ZERO_NOTIFY_WEBHOOK_SUMMARY)
- no-op when the URL is unset/blank, so it is safe to call unconditionally
- sourcing the URL from the environment keeps the secret token out of any
on-disk config file
Attached at both notifier construction sites:
- headless exec: failed deliveries log to stderr (never stdout); the sink
redacts before logging
- TUI: logf=nil so a delivery failure can't corrupt the alt-screen
The sink stays gated by the existing Mode/focus policy (verified by
TestNotifierOffSuppressesSinks), so a webhook only fires when notifications are
enabled (e.g. --notify both) — no change to default behavior. Opt-in and
fail-closed: with the env var unset nothing is attached.
deadcode: 100 -> 93 unreachable funcs (the 7 WebhookSink entries become
reachable); no new dead code. gofmt/vet/build(host+linux+windows)/test -race/
staticcheck(no new)/govulncheck(0) all pass.
* swarm: add opt-in recurring-spawn scheduler (wakeup + daily)
Adds a dependency-free Scheduler to internal/swarm so a member can be spawned on
a recurring schedule, plus a swarm_schedule tool to drive it.
- Schedule is interval-based ("wakeup", every >= 1s) with an optional first
delay and a MaxRuns cap; a daily "HH:MM" ("cron") time is expressed as a 24h
interval whose first delay lands on the next occurrence.
- Each fire spawns a fresh member through the existing Swarm.Spawn path, so
members inherit the recorded policy and are bounded by the team slot cap +
queue. Non-overlapping by default: a fire is skipped while the job's previous
spawn is still running, preventing queue pile-up.
- Scheduler is lazily created (Swarm.Scheduler()), opt-in (nothing runs until a
job is added), and tied to the Swarm's base context — Swarm.Close() stops the
scheduler first so no spawn fires during shutdown.
- swarm_schedule tool: action add|list|cancel; prompt-gated like swarm_spawn;
add validates schedule + agent type up front (fail fast).
The timing loop takes an injectable ticker, so the tests drive fires
deterministically with no real time (fire/skip/cap/cancel/close, plus pure
nextDailyDelay/parseClock/swarmInt). All swarm tests pass 3x under -race.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(clean)/govulncheck(0)/
deadcode(no new) all pass.
* daemon/remote: SessionLink + git-bundle upload over the bridge
Adds an opt-in way to ship a local repo's git history to a remote daemon and
link a remote working tree to it — without a shared filesystem or a network git
remote. The daemon core (protocol/server) is untouched; all changes live in the
remote package plus a CLI hook.
Wire:
- The auth handshake gains an optional Mode field ("" / "session" => a daemon
session as before; "bundle" => a one-shot git-bundle upload). Unknown modes,
or bundle mode when uploads are disabled, are denied (fail closed).
- Bundle transfer reuses the daemon frame codec: a header frame (link id +
exact size), then the bundle bytes as KindData frames, then a result frame.
Server (Bridge):
- BridgeOptions.BundleDir enables uploads (empty => refused). A received bundle
is size-capped (default 256 MiB), staged to a temp file, `git bundle verify`-d,
then `git clone`-d into a staging dir and atomically renamed over
<BundleDir>/<link-id>. Link ids are sanitized to a single path component and
containment is re-checked, so an upload can never escape the bundle dir.
Client:
- UploadRepoBundle creates `git bundle create --all` of a work tree, hashes it
(sha256), streams it over an authenticated bundle-mode TLS connection, and
returns a SessionLink {address, link id, remote path, bundle sha}. SessionLink
persists as a 0600 atomic JSON file (token never stored) with Save/Load/Validate.
CLI:
- `daemon serve-remote --bundle-dir <d>` enables uploads.
- `daemon link --remote <addr> --repo <dir> --id <name> [--out <file>]` uploads
and prints the remote path (+ a ready run command); `daemon link --show <file>`
prints a saved link.
Tests: git bundle create/verify/clone roundtrip; link-id sanitization + path
containment; SessionLink save/load/validate (0600); and a full bridge upload E2E
over real TLS (extract + content check + re-upload-replaces + disabled + bad
token). All remote tests pass 2x under -race; CLI smoke verified via the binary.
Gates: gofmt/vet/build(host+linux+windows)/test/staticcheck(no new)/
govulncheck(0)/deadcode(no new) all pass.
* mcp: unify OAuth token storage onto the shared oauth store
MCP server tokens lived in their own mcp-oauth-tokens.json with a bespoke
codec/lock, separate from the provider-login store (internal/oauth). The oauth
store already reserved the "mcp:" key namespace for exactly this. Unify them.
- mcp.TokenStore now delegates to internal/oauth.Store, keying each server's
token as "mcp:<server>". Its public API (StoredToken/TokenStatus/Save/Load/
Delete/Status/FilePath) is unchanged, so callers (network client, CLI) are
untouched. The bespoke read/write/lock code is removed in favor of the shared,
cross-process-locked, atomic store.
- Transparent, non-destructive migration on first construction: a legacy
mcp-oauth-tokens.json is imported into the unified store (only entries not
already present — a newer unified token is never overwritten), then renamed to
a ".migrated" backup so it is imported at most once and stays recoverable.
A missing / unreadable / unknown-schema legacy file is left untouched and
never blocks startup. An explicit FilePath suppresses the default-path
migration so tests/overrides can't touch the real user config.
- Server names that can't form a valid namespaced key (oauth.ValidateKey) are
rejected on save and skipped during migration. Added oauth.Store.FilePath().
The token format was already identical (access/refresh/type/scopes/expires), so
this is a key rename, not a format change. Existing TokenStore tests pass
unchanged (they exercise the API); added migration + namespacing + newer-wins
tests. Local run check: `zero mcp oauth status` migrates a seeded legacy file
into oauth-tokens.json under mcp:demo, renames the legacy to .migrated, and leaks
no token material.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(clean)/
govulncheck(0)/deadcode(no new) all pass.
* oauth: add an opt-in OS keyring storage backend
Introduces a pluggable storage backend for the unified oauth token store and a
dependency-free OS keyring implementation, selected with
ZERO_OAUTH_STORAGE=keyring (default stays the 0600 file).
- internal/keyring: a small, third-party-dep-free secret store backed by the OS
tooling — `security` on macOS, `secret-tool` (libsecret) on Linux; other
platforms report unsupported. On Linux the secret is passed via stdin (never
the argv); on macOS `security` takes it as an argument (briefly visible in the
process list) — documented, and a reason to prefer the file backend there.
A runner seam makes the per-OS command logic fully testable without a real
keychain.
- internal/oauth: Store now persists its JSON blob through a blobStore backend.
fileBlob preserves the exact prior behavior (0600, atomic write, cross-process
lock file). keyringBlob stores the blob as one base64 entry (keeps multi-line
JSON a single control-char-free value) via a KeyringClient. NewStore resolves
the backend from StoreOptions.Storage or ZERO_OAUTH_STORAGE; unknown storage
and an unavailable keyring fail closed with a clear error.
Because both `zero auth` and the (now unified) MCP token store construct their
store through oauth.NewStore, the keyring backend is honored by both with no
call-site changes. Added oauth.Store.FilePath() returns a "keyring:" identifier
for that backend.
No new module dependency (go.mod unchanged). Tests: keyring round-trip/not-found/
unsupported/missing-binary/stdin-not-argv via an injected runner; store keyring
round-trip + base64-at-rest + storage selection; all existing file-backend tests
pass unchanged. Local run check (macOS): validated the exact `security`
add/find/delete shapes and the not-found exit code (44) against the real binary,
and `mcp oauth status` with ZERO_OAUTH_STORAGE=keyring reads the real keychain
cleanly.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/
govulncheck(0)/deadcode(no new) all pass.
* all: prune unused unexported functions (dead-code sweep)
Removes 15 unexported functions/types/consts that are unreachable even from
tests (every one flagged by staticcheck U1000), clearing the U1000 noise the v16
audit surfaced. Conservative by design: only unexported, test-unused symbols are
removed — exported internal API (which may be an intentional surface or used only
by tests) is deliberately left in place.
Removed:
- agenteval: validateExpectedChangedFiles
- providers/gemini: const providerName
- tui: stripMarkdownInline, removeTrailingAtToken,
noopModelSwitchCompactionPolicy (+ BeforeModelSwitch), deleteComposerLineBefore,
deleteComposerLineAfter, deleteCompletedFileMentionBefore, mcpViewStatus,
mcpServerLines, fitCommandOutput, (*commandPicker).clearQuery,
tuiTheme.onPanel2, indentText
No behavior change (the symbols had no callers). staticcheck U1000 is now empty;
deadcode drops accordingly with no genuinely-new unreachable functions (verified
by a name-based diff vs origin/main). gofmt/vet/build(host+linux+windows)/full
test all pass.
* Address review feedback on the audit backlog
Resolve CodeRabbit review comments on this PR:
- keyring isNotFound: match the specific not-found exit codes (macOS `security`
44 = errSecItemNotFound; `secret-tool` 1) instead of treating any non-zero
exit as "no entry", so a real failure (locked keychain, permissions) surfaces
instead of being masked as absent.
- swarm scheduler daily_at: recompute each fire as the next local HH:MM rather
than adding a fixed 24h, so a wall-clock daily schedule does not drift across
DST. Adds Daily/Hour/Minute to Schedule and a clock to the Scheduler.
- swarm scheduler: reject Add when the scheduler context is already canceled
(not just when closed); re-check cancellation after a tick so a tick+cancel
race can't spawn one extra member after Cancel/Close.
- swarm schedule_tool swarmInt: reject non-integer / non-finite JSON numbers
(max_runs=1.9 errors instead of silently truncating to 1).
- mcp migrateLegacy: resolve a relative LegacyPath to absolute before the
same-file guard so it can't be bypassed by a relative spelling.
- oauth keyring store: serialize the keyring read-modify-write with a
cross-process lock file (best-effort, beside the config dir) so concurrent
processes don't clobber each other's tokens; previously withLock was a no-op.
Adds tests for the non-not-found keyring error, the daily delay recompute, the
Add-after-cancel guard, and non-integer swarmInt.
* swarm: make nextDailyDelay roll the day, not add 24h (DST)
CodeRabbit follow-up: the run loop already recomputes a daily job's delay each
cycle, but nextDailyDelay itself rolled "passed today" to the next occurrence
with Add(24*time.Hour). On a DST changeover day (23h/25h) that fires an hour
early/late. Use time.Date(..., day+1, hour, minute, ...) so Go normalizes the
date and applies the correct local offset, holding the wall-clock HH:MM across
DST. Adds an America/New_York spring-forward test (skips if tzdata is absent).
|
||
|
KRATOS
|
75ecf8e385 |
oauth: opt-in AES-256-GCM encrypted-at-rest token storage (#213)
* oauth: opt-in AES-256-GCM encrypted-at-rest token storage
OAuth Phase 2: add an encrypted token-store backend. The default backend still
writes the 0600 plaintext JSON unchanged; setting ZERO_OAUTH_STORAGE=encrypted-file
encrypts the token file with AES-256-GCM under a per-user random 32-byte secret
persisted 0600 beside the token file (<tokens>.secret).
- internal/oauth/encrypt.go: aesGCMCrypter (nonce||ciphertext, GCM tamper
detection) + load-or-create secret (atomic 0600 write). Pure stdlib, no dep.
- Store gains an optional crypter: readState decrypts before unmarshal, writeState
encrypts after marshal. A missing secret, a short blob, or a failed auth tag all
fail closed (no silent empty/plaintext fallback).
- CLI: newAuthManager selects the backend from ZERO_OAUTH_STORAGE; `zero auth`
help documents it.
Tests: crypter round-trip + tamper, secret create/persist/0600/wrong-size,
store-through-encrypted (ciphertext on disk, round-trip, Status/Delete, tamper +
missing-secret fail-closed). Gates: gofmt/vet/build(host+linux+windows)/test
-race/staticcheck(no new)/govulncheck(0)/deadcode(no new) all pass. Local run
check: device login under ZERO_OAUTH_STORAGE=encrypted-file writes a ciphertext
file (no plaintext token), .secret is 0600, and `zero auth status` reads it back.
* Address review feedback on encrypted token storage
Resolve CodeRabbit review comments on this PR:
- newAuthManager: reject an unsupported ZERO_OAUTH_STORAGE value instead of
silently downgrading to plaintext. A mistyped non-empty value now fails fast
("invalid ZERO_OAUTH_STORAGE ... (supported: encrypted-file)") so a user who
believes encryption is on is never left writing plaintext.
- loadOrCreateSecret: fix the first-run TOCTOU race. os.Rename publishes by
clobbering on POSIX, so two concurrent processes could each generate a secret
and the loser would silently orphan every token it then encrypts. Create the
secret exclusively (O_CREATE|O_EXCL); the loser adopts the winner's on-disk
secret. Factored the read+size-validate into readSecretFile and dropped the
now-unused clock plumbing from the crypter.
- encrypt_test: handle os.Stat / os.ReadFile errors before dereferencing the
result (mode/index) so a real I/O failure surfaces instead of a panic.
Adds a test asserting an invalid ZERO_OAUTH_STORAGE fails fast.
* oauth: retry the secret read when losing the first-run create race
CodeRabbit follow-up: the O_EXCL create path read the on-disk secret
immediately after EEXIST, but the winner may not have finished writing it
yet, so a loser could observe a short file and fail transiently. Retry the
read briefly (bounded) so concurrent first-run invocations converge on the
winner's secret. Adds a -race concurrency test asserting convergence.
|
||
|
KRATOS
|
bb7de29c80 |
feat(oauth): reusable provider OAuth engine + zero auth CLI (#210)
* feat(oauth): reusable provider OAuth engine + `zero auth` CLI
Add internal/oauth: a transport/identity-agnostic OAuth 2.0 engine that powers
provider login alongside the MCP-server OAuth zero already has. It is additive
and provider-neutral — NO providers, endpoints, or client identities are baked
into the binary; every provider is configured entirely via ZERO_OAUTH_<NAME>_*
env vars, so the engine works with any OAuth 2.0 / OIDC server.
Engine (internal/oauth):
- PKCE (S256 mandatory; "plain" refused) + per-flow CSRF state.
- Authorization-code flow: BuildAuthorizationURL / ExchangeCode / Refresh, with
an https-only token-endpoint guard (loopback exempt) and error bodies that
carry only error/error_description (never token material).
- Loopback callback server: 127.0.0.1-only, OS-assigned port, single use, state
verified, then closed.
- Device-authorization grant (RFC 8628): RequestDeviceCode + PollDeviceToken
honoring authorization_pending / slow_down / interval / expiry (headless/SSH).
- RFC 8414 + OIDC discovery (issuer-driven endpoint resolution).
- Namespaced token store ("provider:<name>" / "mcp:<name>"), 0600, file-locked
(ownership-aware), atomic writes, malformed-fails-closed; GetFresh (on-demand
refresh) + Handle401 (forced refresh).
- Opt-in proactive RefreshScheduler.
CLI (zero auth):
- login <provider> [--device] [--scope ...], logout, status [provider],
refresh <provider> [--watch]. Status/output never print token material.
- Registered in app.go dispatch + top-level help.
MCP OAuth (internal/mcp) is left byte-for-byte unchanged (zero regression);
sharing the engine with MCP and encrypted-at-rest/keyring storage are documented
follow-ups. Tests cover every path (PKCE, auth URL, https-guard + redaction,
exchange/refresh, loopback capture + state-mismatch + timeout, device pending/
slow_down/expired, store namespacing/0600/locking, provider env resolution,
manager login (loopback+device)/GetFresh/Handle401, scheduler). Gates: gofmt /
vet / build (host+linux+windows) / test -race / staticcheck (no new) /
govulncheck (0) / deadcode (no new) all pass.
* oauth: fix Windows smoke — use OS-appropriate path in store-path test
TestResolveStorePathHonorsOverride hardcoded a unix "/tmp/custom/tok.json"
literal, which is not absolute on Windows (no drive), so ResolveStorePath
correctly resolves it against the current drive and the verbatim comparison
failed (D:\tmp\custom\tok.json). The production code is correct; the test now
builds the override with filepath.Join(t.TempDir(), ...) so it is absolute and
clean on every OS.
* oauth: address CodeRabbit review (11 findings)
Security / correctness:
- flow: caller-supplied ExtraAuthParams/extraParams can no longer override the
reserved OAuth/PKCE fields (state, redirect_uri, code_challenge[_method],
response_type, client_id) — e.g. forcing method=plain.
- device: a device-authorization response without expires_in now gets a bounded
default lifetime (fail closed), and the poll loop re-checks expiry AFTER the
interval sleep so it never polls past the deadline (e.g. after slow_down).
- lock: handle WriteString/Close errors on the token lock file — a partial write
no longer strands an undeletable lock.
- store: a non-nil env map is now authoritative (hermetic) — a controlled map
never falls back to ambient ZERO_OAUTH_* / HOME / XDG_CONFIG_HOME.
- scheduler: a transient loadForKey error backs off and retries (bounded) instead
of permanently stopping proactive refresh.
- cli auth: reject flags a subcommand does not accept (e.g. login --watch,
status --device) and reject empty --scope values — fail fast.
Lint blockers:
- manager: restructured the discovery fallback so the non-nil-err branch no longer
returns nil (nilerr).
- providers: dropped the ineffectual flow initializer (ineffassign).
Tests: assert Save/Load errors in the scheduler test; the slow_down device test
no longer depends on post-expiry polling. Added tests for reserved-param
stripping, hermetic env, device default-expiry, and CLI flag validation. The
store-path test was already made OS-portable in the Windows-smoke fix.
Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/
govulncheck(0)/deadcode(no new) all pass.
* swarm: de-flake TestMailboxConcurrentSends on Windows CI
The 200-way concurrent-send stress test inherited from the swarm package timed
out on Windows CI (103/200 sends hit the 10s lock timeout): the lock's 20ms
retry sleep starves waiters when Windows file ops are slow under heavy
contention. Shorten the retry to 2ms (a freed lock is re-acquired promptly) and
raise the test's lock timeout to 60s so a legitimately-slow CI never times a send
out. Passes repeatedly under -race. (CI de-flake of a test that surfaced on
#210's Windows smoke; functionally unrelated to the OAuth changes.)
* oauth: address CodeRabbit re-review (4 findings)
- loopback (security): NewLoopbackListener refuses an empty CSRF state — an empty
state would match a callback carrying no state at all, defeating the check.
- device (correctness): RequestDeviceCode now sends client_secret when set, so a
confidential client authenticates on the device-authorization endpoint too
(consistent with the token poll).
- cli auth: document --watch in the help Flags section.
- scheduler test: assert the seed Save error instead of ignoring it.
Also scrubbed a stray third-party reference from a loopback.go comment.
Tests added: empty-state rejection, device client_secret is sent. Gates:
gofmt/vet/build/test -race/staticcheck(no new)/govulncheck(0)/deadcode(no new)
all pass.
* mcp(oauth): delegate the OAuth engine to internal/oauth (dedup) (#212)
OAuth Phase 2: the MCP OAuth code now reuses the shared internal/oauth engine
instead of carrying its own copy of the transport/identity-agnostic primitives.
internal/mcp/oauth.go drops ~110 lines (526 -> 418): newPKCE, newState,
discoverAuthorizationServer, the authorize-URL builder, token exchange/refresh,
the token-response parser, and joinWellKnown now delegate to internal/oauth.
Behavior-preserving: MCP keeps its own OAuthConfig/StoredToken types,
LoginOptions/Login orchestration, loopback handling, registerClient (dynamic
client registration — not part of the shared engine), CLI output, and on-disk
token format (mcp-oauth-tokens.json, raw server-name keys). The existing mcp
oauth tests pass UNCHANGED. The only behavioral delta is a hardening inherited
from the shared engine: the token endpoint must be https (loopback exempt) —
a credential is no longer sent to a plaintext non-loopback endpoint.
Stacks on #210 (the internal/oauth package). Gates: gofmt/vet/build(host+
linux+windows)/test -race/staticcheck(no new)/govulncheck(0)/deadcode(no new
vs base) all pass; `zero mcp oauth status` output is identical.
|