feat: add unified provider account authentication - #186
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (95)
📝 WalkthroughWalkthroughThis PR adds unified managed-provider authentication for Codex, xAI, and GitHub Copilot. It adds OAuth account storage, live model catalogs, protected transports, native xAI image controls, Responses continuity, provider setup APIs, and web authentication flows. ChangesUnified managed-provider platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsView
participant ProviderAuthAPI
participant ProviderAuthManager
participant ProviderRuntime
User->>SettingsView: Select managed authentication
SettingsView->>ProviderAuthAPI: Start and poll device flow
ProviderAuthAPI->>ProviderAuthManager: Resolve account credential
ProviderAuthManager-->>ProviderAuthAPI: Return token and provider runtime
ProviderAuthAPI-->>SettingsView: Return account status and model catalog
SettingsView->>ProviderRuntime: Save binding and selected model
ProviderRuntime->>ProviderAuthManager: Resolve credential per request
ProviderRuntime-->>User: Generate chat or image response
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![jcode-cloud-app[bot]](https://avatars.githubusercontent.com/in/4395162?s=60&v=4){kind=small}
There was a problem hiding this comment.
jcode review
Clean result. The unified provider account authentication PR (managed OAuth device flows for ChatGPT/Codex, xAI, and GitHub Copilot plus a Responses-API transport and encrypted-reasoning session continuity) was reviewed for correctness, security, reliability, and data loss. The credential store uses 0700/0600 permissions, atomic temp-file+rename+fsync replacement, and cross-process file locking (flock/LockFileEx). All upstream HTTP is bounded (64KiB OAuth, 16MiB responses, 2MiB SSE events), disables redirect following to prevent bearer/POST-body leakage, and validates header names/values and verification URIs (HTTPS, pinned host, no port/userinfo). Credential tokens are json:"-" and Authorization is applied last so provider config cannot override them. Device-flow generation guards prevent a Start in flight during Logout from restoring accounts; commitMu linearizes commit vs cancel. Session persistence retains only canonical encrypted reasoning items (cleartext reasoning is never written) and reconstruction drops orphan opaque-only turns while preserving continuation data on tool-only turns. Ran go build, go vet, and go test on internal/providerauth, internal/model (incl. responsemeta), internal/session, internal/runner, internal/web, internal/config — all pass. No defects meeting the 80% confidence threshold were identified.
0 findings
Checks performed
- git diff base..head for internal/providerauth/*.go (manager, store, credential, codex, copilot, xai, http, jwt, types, filelock)
- git diff base..head for internal/model (chatmodel, responses, responses_parse, responses_convert, provider_request, registry) and responsemeta/opaque.go
- git diff base..head for internal/runner/runner.go and internal/session/{session,history}.go continuity changes
- git diff base..head for internal/web/{provider_auth,providers,server,setup}.go and internal/config/config.go
- Inspected ReconstructState/dropOrphanOpaqueAssistantMessages/RecordAssistantMessage for opaque-reasoning retention and orphan-drop correctness across both journal orderings
- Inspected Start/Logout generation race, commitFlowAccount/cancelPending commitMu linearizability, per-account refreshLocks
- Inspected mergeEndpoints parallel-array field ordering (13 endpoints matched)
- Inspected credential isolation: Credential.Token json:"-", Authorization applied last, no redirect following in cloneHTTPClient/cloneResponsesHTTPClient/managedNoRedirectClient
- Inspected validateVerificationURI/validateManagedAuthEndpoint/validCopilotRuntime host pinning
- go generate ./internal/model/... then go build ./internal/providerauth/... ./internal/model/... ./internal/session/... ./internal/runner/... ./internal/web/... (pass)
- go vet ./internal/providerauth/... ./internal/model/... ./internal/session/... ./internal/runner/... (pass)
- go test ./internal/providerauth/... ./internal/model/... ./internal/session/... ./internal/runner/... ./internal/web/... ./internal/config/... (all pass)
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
There was a problem hiding this comment.
jcode review
Clean review. This PR adds unified managed-provider account authentication (ChatGPT/Codex, xAI/Grok, GitHub Copilot device flows) plus a Responses-API transport with encrypted-reasoning continuation. The credential model is sound: durable refresh tokens are stored owner-only (0700/0600) outside config.json via atomic fsync+rename writes guarded by cross-process file locks; short-lived access tokens are memory-only; refresh is singleflight per account; logout invalidation uses a durable generation epoch that flows must recheck before committing; managed runtimes block redirects (ErrUseLastResponse) so a bearer token/POST body cannot follow a redirect; verification URIs and discovery endpoints are pinned to exact hosts/scheme/port; Copilot runtime endpoints are restricted to *.githubcopilot.com; public Flow/Account/Status/Credential types exclude secrets (Credential.Token is json:"-"); the web API never accepts or returns tokens; switching a provider to managed auth strips dormant api_key/base_url/headers/protocol/image_endpoint. Session replay persists only canonical encrypted reasoning items (cleartext reasoning and arbitrary Extra are never written), bounds them, and drops orphan reasoning-only entries; reconstruction correctly re-merges tool calls into the preceding assistant message so continuation items are replayed. Existing API-key provider behavior and legacy session files are preserved (Auth/OpaqueResponseItems are additive omitempty fields). I generated the registry, created the embed stub, built all changed packages, ran go vet (clean), and ran the providerauth/model/runner/session/web/agent/team/tools tests — all pass. No defect met the 80% confidence threshold.
0 findings
Checks performed
- git diff base..head across 63 changed files; reviewed providerauth/{manager,store,credential,http,jwt,codex,xai,copilot,types,filelock_*}.go
- Reviewed model/{chatmodel,provider_request,responses,responses_parse,responses_convert,registry,responsemeta/opaque}.go diffs
- Reviewed runner.go assistant recording, session.go RecordAssistantMessage, history.go ReconstructState opaque-item replay
- Reviewed web/{provider_auth,providers,setup,server}.go for token leakage, binding validation, field-stripping on managed switch
- Verified cloneHTTPClient/responses transport set CheckRedirect=ErrUseLastResponse for managed/OAuth paths
- Verified validateVerificationURI/validateManagedAuthEndpoint pin scheme/host/port; xAI accounts.x.ai fix in head commit
- Verified credential resolution singleflight + CAS secret rotation + requires_reauth fail-closed paths
- Generated registry_generated.go (go generate ./internal/model/...) and stub internal/web/dist/index.html
- go build ./internal/{providerauth,model,runner,session,web,command,agent,team,tools}/... — exit 0
- go vet on changed packages — exit 0
- go test ./internal/providerauth/... ./internal/model/... ./internal/session/... ./internal/runner/... ./internal/web/... — all pass
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
There was a problem hiding this comment.
jcode review
Clean result. This PR adds unified managed provider authentication (ChatGPT/Codex, xAI, GitHub Copilot OAuth) plus a Responses API transport and encrypted-reasoning session continuation. I reviewed the OAuth drivers, the owner-only secret store, the credential resolution path, the Responses SSE/JSON parser, the web auth/provider handlers, the runner + session recording of opaque reasoning, and the image-generation managed-credential wiring. The build compiles, go vet is clean, and the full test suites for internal/providerauth, internal/model (incl. responsemeta), internal/web, internal/session, internal/runner, internal/command, internal/imagegen, and internal/providertools pass, including the race detector for providerauth. Security invariants hold: durable secrets stay in a 0600 file and are never serialized (Credential.Token has json:"-"), public projections omit tokens, managed requests disable redirects and bound response bodies, verification/discovery URIs are pinned to expected HTTPS hosts, bindings fail closed when accounts are removed/expired, and the store uses atomic temp-file+fsync+rename under an advisory lock with a logout generation epoch that prevents in-flight flows from restoring deleted accounts. The opaque-reasoning path only ever persists canonical encrypted items (cleartext summary/content is stripped) and drops orphan reasoning-only entries on both record and replay. No verified defects introduced by this PR.
0 findings
Checks performed
- git diff 73567b8..0eeda9e --stat and read full diff for internal/providerauth, internal/model, internal/web, internal/session, internal/runner
- Read internal/providerauth/{manager,store,credential,codex,xai,copilot,http,jwt,types,models}.go in full
- Read internal/model/{chatmodel.go,responses.go,responses_parse.go,responses_convert.go,provider_request.go,registry.go} and responsemeta/opaque.go
- Read internal/web/{provider_auth.go,providers.go,setup.go,server.go} and provider_auth_test.go
- Read internal/session/{history.go,session.go} changes and responses_continuity_test.go
- go build ./internal/providerauth/... ./internal/model/... ./internal/web/... ./internal/session/... ./internal/runner/... ./internal/command/... ./internal/imagegen/... (clean)
- go vet on all affected packages (clean, no diagnostics)
- go test -count=1 on providerauth, model, model/responsemeta, web, session, runner, command, imagegen, providertools, config (all ok)
- go test -race -count=1 ./internal/providerauth/... (ok, no data races)
- Verified credential fail-closed paths: managedCredential BaseURL/Protocol drift, account removal invalidation, empty-token guard
- Verified opaque reasoning: CanonicalReasoningItem strips cleartext; RecordAssistantMessage drops orphan items; dropOrphanOpaqueAssistantMessages on replay
- Verified web binding validation: three-state auth_binding, managed providers clear api_key/base_url/headers/image_endpoint
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
There was a problem hiding this comment.
jcode review
Clean review. This large PR adds unified managed provider authentication (ChatGPT/Codex OAuth, xAI OAuth, GitHub Copilot device flows), a new OpenAI Responses API transport with SSE parsing, and encrypted-reasoning session continuity. The security-critical providerauth package isolates bearer/refresh tokens (Credential.Token uses json:"-"), disables HTTP redirects on all credential-bearing clients, validates verification URIs against pinned hosts (scheme/port/userinfo), validates header names and rejects CRLF injection, bounds every upstream response (64KiB OAuth, 1MiB catalogs, 2MiB SSE events, 16/2MiB opaque items), and hardens the secret store (0700 dir, 0600 files, atomic rename + fsync + flock). Concurrency is careful: durable logout epochs prevent stale cross-process flow commits, commit/cancel linearizability via commitMu, and double-checked locking on token refresh. Cleartext reasoning is never persisted; only bounded canonical encrypted items are retained and only when an assistant turn has content/tool calls. I verified go build ./..., go vet, the full test suite for all changed packages, and the race detector on providerauth/model/runner/session all pass. No defect meets the 80% confidence threshold.
0 findings
Checks performed
- git diff base..head across all 78 changed files
- Read internal/providerauth/{manager,store,credential,codex,xai,copilot,jwt,http,models,types}.go in full
- Read internal/model/{chatmodel,responses,responses_parse,responses_convert,provider_request}.go and responsemeta/opaque.go
- Read internal/web/{provider_auth,providers,server,setup,models}.go diff
- Read internal/runner/runner.go streaming/replay diff and internal/session/{session,history}.go
- go build ./... (with generated registry + dist stub) — PASS
- go vet ./internal/providerauth/... ./internal/model/... ./internal/web/... ./internal/runner/... ./internal/session/... — clean
- go test for all changed packages — PASS
- go test -race on providerauth, model, runner, session — PASS
- Verified token isolation: Credential.Token json:"-", Flow/Account/Status exclude secrets, ResponsesAPIError excludes bodies/headers
- Verified logout generation-epoch commit guard and commitMu cancel linearizability in manager.go
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
There was a problem hiding this comment.
jcode review
Large unified provider-auth feature (OAuth device flows for Codex/xAI/Copilot, Responses API transport, managed model catalogs, xAI image generation). Security posture is strong: 0600 secret store with atomic rename+fsync, file locks, HTTPS-only verification-URI allowlisting, bounded upstream responses, CRLF/header validation, no token logging, and correct linearizable logout/commit concurrency via generation epochs. One verifiable reliability defect: the web layer caches the managed-auth manager initialization error permanently, disabling provider login/catalog until restart even after the underlying cause is fixed.
1 finding
Checks performed
- Read internal/providerauth/{store,manager,credential,codex,copilot,xai,http,jwt,filelock_unix,filelock_windows,models,types}.go
- Read internal/model/{chatmodel,responses,responses_parse,responses_convert,provider_request}.go and responsemeta/opaque.go
- Read internal/web/{provider_auth,server,providers,setup,models}.go
- Read internal/providertools/manifest.go and internal/imagegen/client.go
- Read internal/tools/generate_image.go, internal/runner/runner.go, internal/session/{session,history}.go
- Read internal/command/{image_generation,provider_runtime_verifier,web_tool_overrides,interactive}.go
- Diff inspected for config.go, handler.go, team/manager.go, tools/subagent.go, approval.go
- Verified providerauth.Default (manager.go:104-107) does not cache errors while web layer does (provider_auth.go:109)
- Confirmed no code resets s.providerAuthErr (grep providerAuth/providerAuthErr assignments)
- Reviewed frontend types.ts/api.ts for token exposure (none found)
- Attempted go build (no network/module cache available; static review only)
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
| if s.providerAuth != nil || s.providerAuthErr != nil { | ||
| return s.providerAuth, s.providerAuthErr | ||
| } | ||
| s.providerAuth, s.providerAuthErr = providerauth.Default(config.ConfigDir()) | ||
| return s.providerAuth, s.providerAuthErr | ||
| } |
There was a problem hiding this comment.
P2 · Provider-auth manager init error is cached permanently, blocking recovery without restart
providerAuthService caches both the resolved service AND any initialization error in s.providerAuth/s.providerAuthErr and returns the cached values on every subsequent call once either is non-nil. The underlying providerauth.Default (manager.go:93) intentionally does NOT cache errors — it only caches successful managers so a later call can retry after a transient NewManager failure (e.g. a momentarily unwritable/unavailable config dir during secureDirectory, a chmod race, or a corrupted/Hand-edited provider-auth.json that the user later repairs). The web layer defeats that retry: once providerauth.Default returns an error once, s.providerAuthErr stays set for the lifetime of the Server process, so every /api/provider-auth/* endpoint, the managed live-model catalog browse (handleProviderCatalog -> service.Models), and setup model listing (handleSetupProviderModels) keep returning HTTP 500 with the stale error even after the root cause is resolved. There is no code path that resets providerAuthErr (grep confirms it is only assigned here). Impact: a user who hits a one-time init failure (most plausibly a corrupted store file they then fix, or a transient home-directory/filesystem hiccup on the desktop sidecar) is permanently locked out of managed login and managed model selection until they restart jcode. This is an availability regression introduced by this PR.
85% confidence
| if s.providerAuth != nil || s.providerAuthErr != nil { | |
| return s.providerAuth, s.providerAuthErr | |
| } | |
| s.providerAuth, s.providerAuthErr = providerauth.Default(config.ConfigDir()) | |
| return s.providerAuth, s.providerAuthErr | |
| } | |
| Only cache successful initialization so failures can be retried (providerauth.Default already memoizes successful managers by config dir, so this stays cheap): | |
| func (s *Server) providerAuthService() (ProviderAuthService, error) { | |
| s.providerAuthMu.Lock() | |
| defer s.providerAuthMu.Unlock() | |
| if s.providerAuth != nil { | |
| return s.providerAuth, nil | |
| } | |
| service, err := providerauth.Default(config.ConfigDir()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| s.providerAuth = service | |
| return s.providerAuth, nil | |
| } |
1 participant
Summary
grok-imagine-imageandgrok-imagine-image-qualitymodels through the independent Image Model role with per-dispatch managed credentialsgenerate_imageSecurity and lifecycle
Validation
go build ./...go vet ./...golangci-lint run --new-from-rev=origin/main ./...go test ./...via the repository pre-push hookgo test -race ./internal/providerauth ./internal/providertools ./internal/imagegen ./internal/model ./internal/command ./internal/web -count=1pnpm --dir web test(21 files / 103 tests)pnpm --dir web typecheckpnpm --dir web buildpnpm --dir site buildManual verification still needed
This PR remains draft until billable live inference smoke tests are explicitly approved and completed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation