feat(oauth): reusable provider OAuth engine + zero auth CLI#210
Merged
Conversation
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.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a complete ChangesOAuth Engine and CLI Integration
Mailbox Lock Retry Latency Improvement
Sequence Diagram(s)sequenceDiagram
participant CLI as zero auth login
participant Manager
participant Registry
participant Discovery as DiscoverAuthorizationServer
participant LoopbackListener
participant AuthServer
participant Store
CLI->>Manager: Login(ctx, LoginOptions{Provider, Device, Scopes})
Manager->>Registry: ResolveConfig(name, env)
Registry-->>Manager: Config, Flow
Manager->>Discovery: DiscoverAuthorizationServer(issuerURL)
Discovery-->>Manager: ServerMetadata (endpoints)
alt Flow == loopback
Manager->>LoopbackListener: NewLoopbackListener(state)
Manager->>AuthServer: open BuildAuthorizationURL(cfg, pkce, state, redirectURI)
AuthServer->>LoopbackListener: GET /callback?code=&state=
LoopbackListener-->>Manager: authorization code
Manager->>AuthServer: ExchangeCode POST /token
AuthServer-->>Manager: Token
else Flow == device
Manager->>AuthServer: RequestDeviceCode POST /device_authorization
AuthServer-->>Manager: DeviceAuth (user_code, verification_uri)
Manager->>CLI: print user instructions
Manager->>AuthServer: PollDeviceToken (repeat until success)
AuthServer-->>Manager: Token
end
Manager->>Store: Save("provider:<name>", Token)
Manager-->>CLI: Status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
internal/cli/auth_test.go (1)
66-99: ⚡ Quick winAdd regression tests for unsupported flag rejection and empty scope validation.
Current tests miss key parser/handler edge cases (e.g.,
status --watch,logout --device,refresh --json, andlogin --scope ""). Adding these will lock in the CLI contract and prevent silent-flag regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/auth_test.go` around lines 66 - 99, The current test coverage is missing key parser and handler edge cases that should reject unsupported flags. Add new regression test functions following the same pattern as TestRunAuthLoginValidation, TestRunAuthLoginUnknownProvider, and TestRunAuthRefreshNoToken to cover the following scenarios: status command with --watch flag should fail, logout command with --device flag should fail, refresh command with --json flag should fail, and login command with an empty --scope value should fail. Each test should invoke runWithDeps with the respective command and flags, verify that exitSuccess is not returned, and check stderr for appropriate error messages when applicable.internal/oauth/flow_test.go (1)
48-78: ⚡ Quick winAdd a regression test for reserved OAuth/PKCE parameter override.
Line 48-78 verifies additive params, but not that
extraParamscannot override reserved keys (state,redirect_uri,code_challenge,code_challenge_method). Add a case that attempts override (e.g.,"code_challenge_method":"plain") and assert the final query still keeps the enforced values.Proposed test shape
+func TestBuildAuthorizationURLReservedParamsCannotBeOverridden(t *testing.T) { + cfg := Config{ + ClientID: "client-123", + AuthorizationEndpoint: "https://auth.example.com/authorize", + ExtraAuthParams: map[string]string{"code_challenge_method": "plain"}, + } + pkce := PKCE{Verifier: "v", Challenge: "chal", Method: MethodS256} + raw, err := BuildAuthorizationURL( + cfg, + pkce, + "state-xyz", + "http://127.0.0.1:9/callback", + map[string]string{"state": "attacker-state"}, + ) + if err != nil { + t.Fatalf("BuildAuthorizationURL: %v", err) + } + u, _ := url.Parse(raw) + q := u.Query() + if got := q.Get("code_challenge_method"); got != MethodS256 { + t.Fatalf("code_challenge_method = %q, want %q", got, MethodS256) + } + if got := q.Get("state"); got != "state-xyz" { + t.Fatalf("state = %q, want state-xyz", got) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/flow_test.go` around lines 48 - 78, The test TestBuildAuthorizationURL verifies that extra parameters are correctly added to the authorization URL, but does not test that reserved OAuth/PKCE parameters (such as code_challenge_method, state, redirect_uri, code_challenge) cannot be overridden through the extraParams argument. Add a new test case that passes extraParams containing an attempted override of a reserved parameter (for example, passing "code_challenge_method" set to "plain" when it should be "S256") and assert that the resulting query URL retains the correct enforced value for that reserved parameter, demonstrating that the override attempt was properly rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/auth.go`:
- Around line 52-80: The parseAuthArgs function currently accepts all flags
unconditionally and does not validate scope values, allowing empty scopes and
flags that some subcommands do not support to pass through silently. Add
validation to reject empty scope values by checking if the trimmed scope string
is empty and returning an error, and refactor the function to enforce
subcommand-specific flag validation so that only flags relevant to the specific
subcommand are accepted (this will require the function to know which subcommand
is being parsed, or alternatively validate flags after parsing based on the
subcommand context). Additionally, review and apply the same validation logic to
all other flag parsing locations in the auth module (referenced at lines
102-242) to ensure consistent behavior across all subcommands.
In `@internal/oauth/device_test.go`:
- Around line 87-88: The DeviceAuth setup in the slow_down test is creating an
expiration window that is too short relative to the delay introduced by the
slow_down mechanism itself. The ExpiresAt field is set to 5 seconds in the
future, but the slow_down test adds a 5-second wait after the first poll,
causing the token to expire before the success path can be validated. Increase
the ExpiresAt duration in the DeviceAuth struct initialization to provide
sufficient time for the slow_down delay to complete before expiration, ensuring
the test validates the success path under normal (non-expired) conditions rather
than relying on post-expiry polling behavior.
In `@internal/oauth/device.go`:
- Around line 81-97: Fail closed on missing device authorization expiry instead
of silently accepting it. In the device authorization response handling around
lines 81-97, change the logic to return an error when parsed.ExpiresIn is not
greater than 0, rather than allowing ExpiresAt to remain zero which disables
expiry checking. Additionally, fix the polling loop at lines 113-122 to check if
the current time has reached or passed ExpiresAt before attempting to poll,
preventing any additional poll attempts after the device authorization expires,
particularly after slow_down adjustments.
In `@internal/oauth/flow.go`:
- Around line 46-60: The query parameters for extra auth params and extra params
are being set after the critical OAuth and PKCE fields (state, redirect_uri,
code_challenge, code_challenge_method), which allows callers to override these
security-critical values. Reorder the code so that the loops iterating over
cfg.ExtraAuthParams and extraParams are executed first, and then set the
required/enforced fields (response_type, client_id, redirect_uri, state,
code_challenge, code_challenge_method, and scope) afterwards. This ensures that
the critical security parameters cannot be overridden by caller-provided values.
In `@internal/oauth/lock.go`:
- Around line 36-37: The WriteString and Close operations on the file variable f
are ignoring errors using blank identifiers, which can leave a lock file in a
corrupted or partial state if these operations fail. Instead of ignoring these
errors with _, capture the errors returned by f.WriteString(token) and
f.Close(), check them appropriately, and return an error if either operation
fails before proceeding to return success. This ensures the lock file is only
considered valid if both the token write and file close complete successfully.
In `@internal/oauth/manager.go`:
- Around line 138-142: The nilerr lint error occurs because the code returns nil
from a branch where err is non-nil (in the error handler after m.discover call).
Restructure the code to avoid returning when the discovery fails: invert the
condition to check if err is nil instead, process the meta only when discovery
succeeds, and allow the function to continue normally (returning cfg, nil at the
end) when discovery fails. This keeps the non-fatal fallback behavior where
pinned endpoints are used when discovery is unsuccessful, while satisfying the
golangci-lint nilerr rule that forbids returning nil from branches with non-nil
errors.
In `@internal/oauth/providers.go`:
- Around line 76-84: The line `flow := FlowLoopback` before the switch statement
is flagged as an ineffectual assignment because every code path in the switch
statement either assigns a value to the flow variable or returns an error.
Remove the initial `flow := FlowLoopback` assignment, as all successful switch
cases (including the empty string and string(FlowLoopback) cases) will assign
the appropriate value before the variable is used.
In `@internal/oauth/scheduler_test.go`:
- Around line 50-58: The test is ignoring errors returned by the token-store
operations in the NewRefreshScheduler test. In the store.Save() call for saving
the old token and the store.Load() call in the polling loop, replace the
underscore error ignoring with explicit error assertions. Use a testing
assertion method (such as require.NoError) to validate that both Save and Load
operations complete successfully, ensuring that token-store failures are caught
immediately rather than causing flaky timeout-based test failures.
In `@internal/oauth/scheduler.go`:
- Around line 48-52: The condition at line 50 in the scheduler.go file combines
error checking with permanent condition checks, causing any loadForKey error to
permanently terminate the scheduler. Instead, separate the error handling from
the permanent condition checks: only return when the permanent conditions are
met (token.RefreshToken is empty or token.ExpiresAt.IsZero()), and handle
loadForKey errors separately by implementing a backoff/retry mechanism instead
of immediately returning.
In `@internal/oauth/store_test.go`:
- Around line 104-110: In the TestResolveStorePathHonorsOverride function,
replace the hard-coded Unix path "/tmp/custom/tok.json" with a platform-native
path built using t.TempDir(). Call t.TempDir() to get a temporary directory
suitable for the current OS, then construct the override path by joining this
directory with a test file name (e.g., "tok.json"), and use the resulting path
both when passing the ZERO_OAUTH_TOKENS_PATH override to ResolveStorePath and in
the assertion that verifies the returned path matches the override. This ensures
the test remains portable across Windows and Unix-like systems.
In `@internal/oauth/store.go`:
- Around line 306-312: The envValue function in internal/oauth/store.go
currently falls back to os.Getenv whenever a key is missing from the env map,
even when a non-nil map is explicitly provided, which breaks test hermeticity.
Modify the function to treat non-nil env maps as authoritative: only call
os.Getenv as a fallback when the env map itself is nil. When env is non-nil but
lacks the requested key, return an empty string instead of falling back to the
process environment.
---
Nitpick comments:
In `@internal/cli/auth_test.go`:
- Around line 66-99: The current test coverage is missing key parser and handler
edge cases that should reject unsupported flags. Add new regression test
functions following the same pattern as TestRunAuthLoginValidation,
TestRunAuthLoginUnknownProvider, and TestRunAuthRefreshNoToken to cover the
following scenarios: status command with --watch flag should fail, logout
command with --device flag should fail, refresh command with --json flag should
fail, and login command with an empty --scope value should fail. Each test
should invoke runWithDeps with the respective command and flags, verify that
exitSuccess is not returned, and check stderr for appropriate error messages
when applicable.
In `@internal/oauth/flow_test.go`:
- Around line 48-78: The test TestBuildAuthorizationURL verifies that extra
parameters are correctly added to the authorization URL, but does not test that
reserved OAuth/PKCE parameters (such as code_challenge_method, state,
redirect_uri, code_challenge) cannot be overridden through the extraParams
argument. Add a new test case that passes extraParams containing an attempted
override of a reserved parameter (for example, passing "code_challenge_method"
set to "plain" when it should be "S256") and assert that the resulting query URL
retains the correct enforced value for that reserved parameter, demonstrating
that the override attempt was properly rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8ddf7ec-b97b-4755-9f95-07b039134adb
📒 Files selected for processing (21)
internal/cli/app.gointernal/cli/auth.gointernal/cli/auth_test.gointernal/oauth/device.gointernal/oauth/device_test.gointernal/oauth/discovery.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/lock.gointernal/oauth/loopback.gointernal/oauth/loopback_test.gointernal/oauth/manager.gointernal/oauth/manager_test.gointernal/oauth/oauth.gointernal/oauth/pkce.gointernal/oauth/providers.gointernal/oauth/providers_test.gointernal/oauth/scheduler.gointernal/oauth/scheduler_test.gointernal/oauth/store.gointernal/oauth/store_test.go
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.
Collaborator
Author
|
@coderabbitai review All 11 findings addressed in 3864909 (replies on each thread): Security/correctness
Lint blockers Tests — assert Save/Load in the scheduler test; slow_down test no longer depends on post-expiry polling; added tests for reserved-param stripping, hermetic env, device default-expiry, CLI flag validation. The store-path portability fix already landed in d724d9a. Gates: gofmt · vet · build (host+linux+windows) · test -race · staticcheck (no new) · govulncheck (0) · deadcode (no new) — all green. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/oauth/scheduler_test.go (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert seed persistence in the no-op test.
Line 17 ignores the
Saveerror, which can make this test pass on a broken store setup instead of true scheduler no-op behavior.Suggested fix
- _ = m.store.Save(ProviderKey("demo"), Token{AccessToken: "a", ExpiresAt: time.Now().Add(time.Hour)}) + if err := m.store.Save(ProviderKey("demo"), Token{ + AccessToken: "a", + ExpiresAt: time.Now().Add(time.Hour), + }); err != nil { + t.Fatalf("seed Save: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/scheduler_test.go` at line 17, The test ignores the error return value from the m.store.Save call, which means the seed persistence might silently fail without the test catching it. Replace the underscore blank identifier with proper error assertion to ensure the Save operation succeeds before proceeding with the scheduler no-op behavior test. This validates that the store setup is actually working correctly rather than allowing the test to pass even if the store is broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/oauth/scheduler_test.go`:
- Line 17: The test ignores the error return value from the m.store.Save call,
which means the seed persistence might silently fail without the test catching
it. Replace the underscore blank identifier with proper error assertion to
ensure the Save operation succeeds before proceeding with the scheduler no-op
behavior test. This validates that the store setup is actually working correctly
rather than allowing the test to pass even if the store is broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb44475d-88ca-446a-ad1c-e666377a474b
📒 Files selected for processing (13)
internal/cli/auth.gointernal/cli/auth_test.gointernal/oauth/device.gointernal/oauth/device_test.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/lock.gointernal/oauth/manager.gointernal/oauth/providers.gointernal/oauth/scheduler.gointernal/oauth/scheduler_test.gointernal/oauth/store.gointernal/oauth/store_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/oauth/device_test.go
- internal/cli/auth_test.go
- internal/oauth/lock.go
- internal/cli/auth.go
- internal/oauth/device.go
- internal/oauth/manager.go
- internal/oauth/scheduler.go
- internal/oauth/store.go
- internal/oauth/flow.go
- internal/oauth/flow_test.go
- internal/oauth/providers.go
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/oauth/flow_test.go (1)
109-112: ⚡ Quick winIsolate the PKCE downgrade assertion with otherwise-valid inputs.
This case currently mixes
"plain"with other potentially invalid inputs (missingclient_id, non-URL redirect), so future validation-order changes could fail for the wrong reason.Suggested test tightening
-func TestBuildAuthorizationURLRejectsPlain(t *testing.T) { - _, err := BuildAuthorizationURL(Config{AuthorizationEndpoint: "https://a/x"}, PKCE{Method: "plain"}, "s", "r", nil) +func TestBuildAuthorizationURLRejectsPlain(t *testing.T) { + _, err := BuildAuthorizationURL( + Config{ + ClientID: "client-123", + AuthorizationEndpoint: "https://a/x", + }, + PKCE{Method: "plain"}, + "s", + "http://127.0.0.1:9/callback", + nil, + ) if !errors.Is(err, ErrPKCEDowngrade) { t.Fatalf("err = %v, want ErrPKCEDowngrade", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/flow_test.go` around lines 109 - 112, The test for BuildAuthorizationURL in the PKCE downgrade case is mixing the PKCE validation with other potentially invalid inputs (missing client_id and invalid redirect URI "r"), which could cause test failures for unrelated reasons if validation order changes. Modify the test to provide otherwise-valid inputs: ensure the Config passed to BuildAuthorizationURL includes a valid client_id, pass a properly formatted redirect URI instead of just "r", and keep Method: "plain" in the PKCE object. This isolates the test to fail specifically on the PKCE downgrade issue rather than other validation failures.internal/oauth/device_test.go (1)
13-51: ⚡ Quick winAdd a request-shape test for device auth client authentication.
Please add a test that verifies
RequestDeviceCodesendsclient_secretwhenConfig.ClientSecretis set. This protects a critical provider-compatibility contract from regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/device_test.go` around lines 13 - 51, Add a new test function (named something like TestRequestDeviceCodeWithClientSecret) that verifies the RequestDeviceCode function sends the client_secret parameter when Config.ClientSecret is set. The test should set up an httptest server that inspects the incoming request to verify that client_secret is included in the request body or query parameters, then call RequestDeviceCode with a Config that has a non-empty ClientSecret field, and assert that the request validation passed successfully. This test should follow the same pattern as TestRequestDeviceCode and TestRequestDeviceCodeDefaultsInterval to ensure the provider-compatibility contract is protected from regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/auth.go`:
- Around line 336-341: The help text for the authentication command in the Flags
section is missing documentation for the --watch flag, even though it is
referenced in the refresh command. Add the --watch flag to the Flags section in
the help text alongside the other flags (--device, --scope, --json, and -h,
--help) with an appropriate description of what it does (e.g., "Watch for
changes").
In `@internal/oauth/device.go`:
- Around line 58-63: The RequestDeviceCode function constructs a form with
client_id and scope but omits the client_secret, even when it is configured in
cfg. This causes inconsistency with the poll step which does include the
client_secret, and can fail against providers that require client authentication
on the device authorization endpoint. Add the client_secret to the form values
by including a form.Set call for "client_secret" using cfg.ClientSecret,
positioned alongside the existing form.Set calls for "client_id" and "scope".
In `@internal/oauth/loopback.go`:
- Around line 34-43: The NewLoopbackListener function currently accepts an empty
state parameter, which breaks CSRF protection since an empty state would match
any empty state in validation. Add a validation check at the beginning of
NewLoopbackListener to ensure the state parameter is not empty, and return an
error with an appropriate message if the state is empty (for example, check if
len(state) == 0 and return nil, fmt.Errorf with a descriptive message about
requiring a non-empty CSRF state).
In `@internal/oauth/scheduler_test.go`:
- Around line 14-23: In the TestRefreshSchedulerNoRefreshTokenIsNoop function,
the error returned by m.store.Save when saving the token without a refresh token
is currently ignored (assigned to underscore). Instead of ignoring this error,
you should assert that the Save operation succeeds (using a helper like
require.NoError or similar) to ensure the token is actually saved in the store.
This ensures the test validates that the scheduler exits because the token lacks
a refresh token, not because the token was never saved due to a save failure.
---
Nitpick comments:
In `@internal/oauth/device_test.go`:
- Around line 13-51: Add a new test function (named something like
TestRequestDeviceCodeWithClientSecret) that verifies the RequestDeviceCode
function sends the client_secret parameter when Config.ClientSecret is set. The
test should set up an httptest server that inspects the incoming request to
verify that client_secret is included in the request body or query parameters,
then call RequestDeviceCode with a Config that has a non-empty ClientSecret
field, and assert that the request validation passed successfully. This test
should follow the same pattern as TestRequestDeviceCode and
TestRequestDeviceCodeDefaultsInterval to ensure the provider-compatibility
contract is protected from regressions.
In `@internal/oauth/flow_test.go`:
- Around line 109-112: The test for BuildAuthorizationURL in the PKCE downgrade
case is mixing the PKCE validation with other potentially invalid inputs
(missing client_id and invalid redirect URI "r"), which could cause test
failures for unrelated reasons if validation order changes. Modify the test to
provide otherwise-valid inputs: ensure the Config passed to
BuildAuthorizationURL includes a valid client_id, pass a properly formatted
redirect URI instead of just "r", and keep Method: "plain" in the PKCE object.
This isolates the test to fail specifically on the PKCE downgrade issue rather
than other validation failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07794801-2e60-4ba3-bd65-f18cf5ccb3f1
📒 Files selected for processing (21)
internal/cli/app.gointernal/cli/auth.gointernal/cli/auth_test.gointernal/oauth/device.gointernal/oauth/device_test.gointernal/oauth/discovery.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/lock.gointernal/oauth/loopback.gointernal/oauth/loopback_test.gointernal/oauth/manager.gointernal/oauth/manager_test.gointernal/oauth/oauth.gointernal/oauth/pkce.gointernal/oauth/providers.gointernal/oauth/providers_test.gointernal/oauth/scheduler.gointernal/oauth/scheduler_test.gointernal/oauth/store.gointernal/oauth/store_test.go
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.)
gnanam1990
added a commit
that referenced
this pull request
Jun 15, 2026
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.)
Vasanthdev2004
approved these changes
Jun 15, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVED ✅
Reviewed locally in worktree review-pr210 at 19703e5. 3042 additions, 23 files, base main ← feat/oauth-module.
CI gates (clean): go build ./..., go vet ./..., gofmt -l ., go test -count=1 ./internal/oauth/... (7.3s), targeted TestMailboxConcurrentSends re-run (8.7s, no longer flaky on Windows). Full suite shows the same pre-existing TestRunExecUsesProjectConfigAndOpenAICompatibleProvider failure I reproduced on clean main — unrelated to this PR.
Security invariants verified by reading the diff:
- PKCE S256 mandatory,
plainrefused viaErrPKCEDowngrade(flow.go:33-40) - CSRF state: 32-byte
NewState(), verified on callback, never logged - Reserved-param guard in
BuildAuthorizationURLblocks extra params from overwritingstate,redirect_uri,code_challenge[_method],client_id,response_type— prevents caller-supplied params from downgrading PKCE or rewriting the callback target validateTokenEndpointrequires https, with loopback exemption (http://127.0.0.1,http://localhost, or anyIsLoopback()IP) — mirrors oauth2-client.js and matches the loopback-callback design- Loopback server binds
127.0.0.1only, OS-assigned port, single-use, idempotentCloseso double-close can't race a still-pendingWait - RFC 8628 device flow:
slow_downadds +5s (per spec), default 10-min expiry if issuer omits it (fail-closed), expiry re-checked AFTER the sleep so a stale token can't slip through on the last poll - Token store: regex-bounded keys
^(provider|mcp):[A-Za-z0-9][A-Za-z0-9._-]{0,127}$, schema v1, fail-closed on malformed JSON or wrong schema version, file 0600, dir 0700, atomic write via tmp + rename - Cross-process file lock: O_EXCL create, ownership-aware release (read back the token, only delete if it matches), 30s stale detection, partial
WriteString/Closeerrors no longer strand the lock - Hermetic env: non-nil
envmap is authoritative — tests/callers that pass a controlled map cannot pick up ambientHOME/XDG_CONFIG_HOME/ZERO_OAUTH_* - Status() never emits token material — redaction-safe by construction
- Scheduler: jitter on
delayUntilRefreshso many sessions don't refresh in lockstep, bounded backoff on consecutive load errors, proactive refresh is an optimization overGetFresh(on-demand is still the source of truth)
The CodeRabbit fixes (commit 3864909) addressed reserved params, hermetic env, default device expiry, lock-write error handling, scheduler retry, and the nilerr lint — all visible in the diff.
One naming nitpick (not a blocker):
In providers.go:91-97, the loop calls validateTokenEndpoint on cfg.IssuerURL and cfg.AuthorizationEndpoint in addition to cfg.TokenEndpoint. The check is correct (every credential- or auth-bearing URL must be https or loopback), but the function name now oversells its scope. Either rename it to validateHttpsOrLoopbackURL or split a validateHttpsURL helper. Cosmetic — happy to follow up in a small refactor PR or just land this as-is.
Overall: clean, well-tested, security-conservative. Merging.
🤖 Generated with Claude Code
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current review state and open comments on PR `#210`
gh pr view 210 --repo Gitlawb/zero --json state,reviewDecision,reviews,comments | jq '.'
# Also list review comments still open (not resolved)
gh api repos/Gitlawb/zero/pulls/210/comments --paginate | jq '[.[] | {id, path, line, body: .body[:120], state: (if .pull_request_review_id then "review_comment" else "inline" end)}]' | head -100Length of output: 110841
The loopback CSRF issue and the missing |
- 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.
Collaborator
Author
|
@coderabbitai review All 4 re-review findings fixed in ac0c659:
Also scrubbed a leftover third-party reference from a Gates: gofmt · vet · build (host+linux+windows) · test -race · staticcheck (no new) · govulncheck (0) · deadcode (no new) — all green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check loopback.go for empty state rejection
echo "=== loopback.go NewLoopbackListener ==="
rg -n "state" internal/oauth/loopback.go -A2 -B2 | head -40
# Check device.go for client_secret in RequestDeviceCode
echo ""
echo "=== device.go RequestDeviceCode client_secret ==="
rg -n "client_secret\|ClientSecret" internal/oauth/device.go -A2 -B2 | head -30
# Check auth.go for --watch in help text
echo ""
echo "=== auth.go --watch in help/Flags section ==="
rg -n "watch" internal/cli/auth.go -A2 -B2 | head -30
# Check scheduler_test.go for Save error assertion
echo ""
echo "=== scheduler_test.go Save error assertion ==="
rg -n "Save\|require\|assert" internal/oauth/scheduler_test.go -A2 -B2 | head -40
# Check new loopback test
echo ""
echo "=== loopback_test.go RejectsEmptyState ==="
rg -n "EmptyState\|empty\|whitespace" internal/oauth/loopback_test.go -A5 -B2 | head -30
# Check new device test
echo ""
echo "=== device_test.go SendsClientSecret ==="
rg -n "ClientSecret\|client_secret" internal/oauth/device_test.go -A5 -B2 | head -30Length of output: 3824 🏁 Script executed: #!/bin/bash
echo "=== device.go full RequestDeviceCode function ==="
ast-grep --pattern 'func RequestDeviceCode($$$) ($$$) {
$$$
}'
echo ""
echo "=== scheduler_test.go full content ==="
cat internal/oauth/scheduler_test.go
echo ""
echo "=== loopback_test.go full content ==="
cat internal/oauth/loopback_test.go
echo ""
echo "=== auth.go Flags section in help ==="
rg -n "Flags\|--watch\|watch" internal/cli/auth.go -A1 -B1 | head -50Length of output: 9694 🏁 Script executed: #!/bin/bash
echo "=== auth.go help text (renderAuthHelp or similar) ==="
rg -n "Flags\|--watch\|renderHelp\|helpText\|Flags:" internal/cli/auth.go -B1 -A2 | head -60
echo ""
echo "=== auth.go lines around watch in help ==="
grep -n "watch\|Watch" internal/cli/auth.go | head -20
echo ""
echo "=== device_test.go for client_secret test ==="
rg -n "ClientSecret\|client_secret\|Secret" internal/oauth/device_test.go -B2 -A3 | head -40Length of output: 1462 🏁 Script executed: #!/bin/bash
echo "=== device_test.go full content ==="
cat internal/oauth/device_test.goLength of output: 5648
Everything looks correct. Approving. 🐇 ✨ [approve] |
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.
gnanam1990
added a commit
that referenced
this pull request
Jun 15, 2026
… daemon (#211) * feat(daemon): TLS remote bridge — authenticated network access to the daemon Add internal/daemon/remote: an OPT-IN, TLS-only network bridge on top of the local daemon. A remote client drives the SAME SessionManager/Pool over the network behind bearer-token auth, reusing the daemon's framing and dispatch unchanged. The local Unix-socket daemon stays the default and is unchanged. - Bridge: per accepted TLS connection, authenticate THEN hand to the daemon's control dispatch via the new Server.ServeConn seam — a remote session runs under the same sandbox/risk model as a local one (never bypasses it). - Authenticator + TokenAuthenticator: constant-time bearer-token check, token from $ZERO_DAEMON_REMOTE_TOKEN or a file; optional attestation hook (no-op default). Auth happens before any control frame; failure closes the connection after a small backoff; tokens are never logged. - RemoteClient (DialRemote): verified TLS dial (never InsecureSkipVerify) + auth handshake, then the existing daemon.Client via the new NewClientConn seam, so remote and local share one protocol. - Fail-closed: TLS mandatory (refuses without cert/key), a configurable min-protocol-version floor, the 1 MiB frame cap reused, and a bounded concurrent-connection cap that refuses overflow. - CLI: `zero daemon serve-remote --addr --tls-cert --tls-key`; `run`/`attach` gain `--remote <addr> [--token --ca-cert --server-name]`. Daemon seams added (additive, behavior unchanged): Server.ServeConn, NewClientConn. SessionLink (event POST) and git-bundle repo transfer are documented follow-ups. Tests cover auth accept/reject (env + file), TLS-required, cert verification, version floor, run/status/attach round-trip, re-attach resumption, connection cap, and ListenAndServeTLS/Close. 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.) * remote: address CodeRabbit re-review (TLS dial + Close idempotency) - client: replace the deprecated, non-cancellation-aware tls.DialWithDialer with a context-aware tls.Dialer.DialContext (resolves the golangci-lint noctx CI blocker; the context bounds the dial + TLS handshake). - bridge: Close now clears b.listener under the lock, so a repeat Close is a no-op instead of returning a closed-listener error (matches its doc). Test asserts a double Close succeeds. Gates: gofmt/vet/build(host+linux+windows)/test -race/staticcheck(no new)/ govulncheck(0)/deadcode(no new) all pass.
This was referenced Jul 1, 2026
This was referenced Jul 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
internal/oauth— a transport/identity-agnostic OAuth 2.0 engine — plus azero authCLI for logging in to model providers, alongside the MCP-server OAuth zero already has. Additive and provider-neutral: no providers, endpoints, or client identities are baked into the binary. Every provider is configured entirely viaZERO_OAUTH_<NAME>_*env vars, so the engine works with any OAuth 2.0 / OIDC server and ships no third-party brand or client identity.Engine (
internal/oauth)plainrefused) + per-flow CSRF state.BuildAuthorizationURL/ExchangeCode/Refresh, with an https-only token-endpoint guard (loopback exempt) and errors that carry onlyerror/error_description— never token material.RequestDeviceCode+PollDeviceTokenhonoringauthorization_pending/slow_down/ interval / expiry (headless/SSH).openid-configurationfallback (issuer-driven).provider:<name>/mcp:<name>), 0600, file-locked (ownership-aware), atomic writes, malformed-fails-closed;GetFresh(on-demand refresh) +Handle401(forced).CLI (
zero auth)login <provider> [--device] [--scope …]·logout <provider>·status [provider]·refresh <provider> [--watch]. Status/output never print token material. Wired into the dispatch + top-level help.A provider named
<name>is configured via:ZERO_OAUTH_<NAME>_CLIENT_ID(required),_CLIENT_SECRET,_AUTHORIZE_URL,_TOKEN_URL,_DEVICE_URL,_ISSUER_URL,_SCOPES,_FLOW(loopback|device). Endpoint URLs must be https (loopback exempt).Security
PKCE-S256 mandatory · CSRF state verified · loopback-only single-use callback · https-only token endpoint · token material never logged (error bodies redacted to
error/error_description) · token files 0600 + locked · provider login is opt-in and inert untilzero auth loginis run. No new dependencies (stdlib only).Not touched / deferred
internal/mcp) is byte-for-byte unchanged — zero regression. Migrating MCP to share this engine is a documented, test-guarded follow-up.Verification
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.Local run check (built binary):
auth --help,status, login validation, unconfigured-provider error, logout, andmcp oauth --help(no regression) all verified;statusconfirmed to leak no token material. A live login completes against an in-process httptest server in the unit tests (the sandbox blocks cross-process loopback, so the standalone-binary↔fake-server E2E can't run in CI).Summary by CodeRabbit
New Features
zero authCLI command withlogin,logout,status, andrefresh, including updated--help, interactive loopback or device-code login, provider-scopedstatus, repeatable--scope, and--jsonoutput with secret-safe redaction.Tests