From 4475e2d18ec245c3190f8dd028d15818343f5a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Wed, 9 Sep 2026 11:35:46 +0200 Subject: [PATCH] fix(sdk/go): honor explicitly-set zero option values in oidc defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loginConfig.applyDefaults()` keyed off the zero value rather than set-ness, so it could not distinguish an unset field from one a caller explicitly set to its zero value: - `WithTimeout(0)` was silently replaced by the 2m default. - `WithScopes()` (explicit empty) was replaced by the default scopes, even though `WithScopes` already records `scopesSet`. Switch `applyDefaults` to consult the `*Set` sentinels, add a `timeoutSet` sentinel set by `WithTimeout`, and guard the client-credentials exchange so a zero timeout means "no deadline" instead of creating an already-expired context (matching Login and DeviceFlow). `WithTimeout(0)` now means "no timeout". Unset fields still receive their defaults; non-zero explicit values are unaffected. Signed-off-by: Roland Huß --- sdk/go/openshell/v1/oidc/credentials_auth.go | 8 ++++- sdk/go/openshell/v1/oidc/credentials_test.go | 34 ++++++++++++++++++++ sdk/go/openshell/v1/oidc/options.go | 8 +++-- sdk/go/openshell/v1/oidc/options_test.go | 20 ++++++++++++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/sdk/go/openshell/v1/oidc/credentials_auth.go b/sdk/go/openshell/v1/oidc/credentials_auth.go index 24012c8bf4..2c6d9a0e74 100644 --- a/sdk/go/openshell/v1/oidc/credentials_auth.go +++ b/sdk/go/openshell/v1/oidc/credentials_auth.go @@ -71,7 +71,13 @@ func (a *clientCredentialsAuth) accessTokenForExchange() (string, error) { return accessToken, nil } - exchangeCtx, cancel := context.WithTimeout(context.Background(), a.cfg.timeout) + // A zero timeout means "no deadline"; only bound the exchange when a + // positive timeout was configured (mirrors Login and DeviceFlow). + exchangeCtx := context.Background() + cancel := context.CancelFunc(func() {}) + if a.cfg.timeout > 0 { + exchangeCtx, cancel = context.WithTimeout(exchangeCtx, a.cfg.timeout) + } defer cancel() token, err := exchangeClientCredentials(exchangeCtx, a.cfg, true) if err != nil { diff --git a/sdk/go/openshell/v1/oidc/credentials_test.go b/sdk/go/openshell/v1/oidc/credentials_test.go index 3269c44d40..a2e11d1a80 100644 --- a/sdk/go/openshell/v1/oidc/credentials_test.go +++ b/sdk/go/openshell/v1/oidc/credentials_test.go @@ -308,6 +308,40 @@ func TestClientCredentialsAuthLateFlightReusesCachedToken(t *testing.T) { assert.Equal(t, "cached-token", accessToken) } +// A zero timeout means "no deadline". The exchange context must not be born +// expired, so the token exchange should still succeed. +func TestClientCredentialsAuthZeroTimeoutHasNoDeadline(t *testing.T) { + resetDiscoveryCache() + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": server.URL, + "authorization_endpoint": server.URL + "/authorize", + "token_endpoint": server.URL + "/token", + }) + return + } + _, _ = w.Write([]byte(tokenResponseJSON("token", "", 3600))) + })) + t.Cleanup(server.Close) + + auth, err := NewClientCredentialsAuth( + WithIssuer(server.URL), + WithClientID("client"), + WithClientSecret("secret"), + WithTimeout(0), // explicitly no timeout + ) + require.NoError(t, err) + + // The explicit zero timeout must be preserved (not replaced by the default). + require.Zero(t, auth.(*clientCredentialsAuth).cfg.timeout) + + metadata, err := auth.GetRequestMetadata(context.Background()) + require.NoError(t, err) + assert.Equal(t, "Bearer token", metadata["authorization"]) +} + func TestClientCredentialsAuthCancellationDoesNotPoisonSharedExchange(t *testing.T) { resetDiscoveryCache() var server *httptest.Server diff --git a/sdk/go/openshell/v1/oidc/options.go b/sdk/go/openshell/v1/oidc/options.go index d4d856ba4b..141e964be5 100644 --- a/sdk/go/openshell/v1/oidc/options.go +++ b/sdk/go/openshell/v1/oidc/options.go @@ -33,6 +33,7 @@ type loginConfig struct { scopesSet bool callbackPort int timeout time.Duration + timeoutSet bool keyboardFlow bool inMemory bool displayFunc func(verificationURL, userCode string) @@ -48,12 +49,14 @@ type loginConfig struct { // applyDefaults fills in default values for fields that were not set // by any option function. func (c *loginConfig) applyDefaults() { - if len(c.scopes) == 0 { + // Check set-ness, not the zero value, so an explicitly-set empty scope + // list or zero timeout is honored instead of being replaced by defaults. + if !c.scopesSet { // Deep copy to avoid callers mutating the package-level slice. c.scopes = make([]string, len(defaultScopes)) copy(c.scopes, defaultScopes) } - if c.timeout == 0 { + if !c.timeoutSet { c.timeout = defaultTimeout } } @@ -125,6 +128,7 @@ func WithCallbackPort(port int) LoginOption { func WithTimeout(d time.Duration) LoginOption { return func(c *loginConfig) { c.timeout = d + c.timeoutSet = true } } diff --git a/sdk/go/openshell/v1/oidc/options_test.go b/sdk/go/openshell/v1/oidc/options_test.go index 28d17c06b4..8af2df7ff7 100644 --- a/sdk/go/openshell/v1/oidc/options_test.go +++ b/sdk/go/openshell/v1/oidc/options_test.go @@ -82,6 +82,26 @@ func TestWithTimeout(t *testing.T) { assert.Equal(t, 5*time.Minute, cfg.timeout) } +func TestWithScopes_ExplicitEmptyNotOverridden(t *testing.T) { + var cfg loginConfig + WithScopes()(&cfg) // caller explicitly requests no scopes + cfg.applyDefaults() + + // An explicitly-set empty scope list must be honored, not replaced by defaults. + assert.True(t, cfg.scopesSet) + assert.Empty(t, cfg.scopes) +} + +func TestWithTimeout_ExplicitZeroNotOverridden(t *testing.T) { + var cfg loginConfig + WithTimeout(0)(&cfg) // caller explicitly requests no timeout + cfg.applyDefaults() + + // An explicitly-set zero timeout must be honored, not replaced by the default. + assert.True(t, cfg.timeoutSet) + assert.Zero(t, cfg.timeout) +} + func TestWithKeyboardFlow(t *testing.T) { var cfg loginConfig WithKeyboardFlow()(&cfg)