Enhancing backwards compatibility of OAuth credential fetching#647
Enhancing backwards compatibility of OAuth credential fetching#647avasconcelos114 merged 10 commits intomasterfrom
Conversation
…dential fetching - Making sure that errors with fetching credentials fail more gracefully
avasconcelos114
added
2: Dev Review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughgetOAuthConfig now returns (*oauth2.Config, error) and adds resolveOAuthCredentials to prefer instance KV with legacy fallback; OAuth completion validates state format and user ownership, moves state retrieval/deletion after auth checks; InstanceConfiguration gains Validate(); tests added for config resolution, refresh errors, and state validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant HTTP as HTTP Handler
participant Plugin as Plugin
participant KV as KV Store
participant Legacy as Legacy Settings
participant OAuthLib as oauth2
Client->>HTTP: GET /oauth/complete?state=<state>&code=<code>
HTTP->>Plugin: completeConnectUserToGitlab(state, authedUserID)
Plugin->>Plugin: validate state format (regex)
alt invalid format
Plugin-->>HTTP: 400 Invalid OAuth state
else valid format
Plugin->>Plugin: extract userID from state
Plugin->>HTTP: compare authedUserID
alt user mismatch
Plugin-->>HTTP: 401 Not authorized
else authorized
Plugin->>KV: KVGet(storedStateKey)
KV-->>Plugin: storedState or error
alt stored state missing/mismatch
Plugin-->>HTTP: 400 Invalid OAuth state token
else match
Plugin->>KV: KVDelete(storedStateKey)
KV-->>Plugin: delete OK / error
Plugin->>Plugin: getOAuthConfig()
alt instance creds in KV
Plugin->>KV: read instance config
KV-->>Plugin: clientID, secret, gitlabURL
else fallback
Plugin->>Legacy: read legacy plugin settings
Legacy-->>Plugin: clientID, secret, gitlabURL
end
Plugin->>OAuthLib: exchange code using conf
OAuthLib-->>Plugin: token or error
Plugin-->>HTTP: respond (success or wrapped error)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
avasconcelos114
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/plugin.go (1)
246-278:⚠️ Potential issue | 🔴 CriticalUse the GitlabURL from the instance configuration when instance credentials are resolved
When
resolveOAuthCredentials()returns credentials from an instance (lines 284–286), those credentials are registered on that specific instance's GitLab. However,getOAuthConfig()builds the authorization and token URLs fromconfig.GitlabURL(lines 262–263), which is from the main plugin configuration. This causes a mismatch: the OAuth app is registered oninstance.GitlabURL, but the OAuth flow is directed toconfig.GitlabURL.To fix this, either:
- Have
resolveOAuthCredentials()also return the instance's GitlabURL sogetOAuthConfig()can use it, or- Call
getInstance()ingetOAuthConfig()to fetch both the credentials and the correct GitlabURL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin.go` around lines 246 - 278, getOAuthConfig currently builds AuthURL/TokenURL from the global configuration (getConfiguration().GitlabURL) even when resolveOAuthCredentials returns per-instance credentials; modify getOAuthConfig to use the correct instance GitLab URL by either (a) updating resolveOAuthCredentials to return the instance's GitlabURL along with clientID/clientSecret and then use that returned URL when constructing authURL/tokenURL, or (b) calling getInstance(...) from within getOAuthConfig to obtain the instance object and its GitlabURL after resolving credentials; ensure you reference resolveOAuthCredentials and getInstance and replace usage of config.GitlabURL when building oauth2.Endpoint so the auth/token endpoints match the instance that registered the OAuth app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin_test.go`:
- Around line 226-409: The file server/plugin_test.go has formatting issues; run
gofmt to fix them. Specifically, format the test block around TestGetOAuthConfig
and TestRefreshTokenReturnsErrorWhenOAuthConfigFails (and any surrounding new
OAuth config tests) by running `gofmt -w server/plugin_test.go` (or your
editor's Go format action), then re-run lint/CI to ensure the gofmt failure is
resolved.
In `@server/plugin.go`:
- Around line 280-298: Add a data-layer validation method on
InstanceConfiguration (e.g., InstanceConfiguration.IsValid or Validate) that
enforces non-empty/length requirements for GitlabOAuthClientID and
GitlabOAuthClientSecret (matching configuration.IsValid rules), call this
validator from installInstance() before saving, and update
resolveOAuthCredentials to check the returned instanceConfig via that validator
(or reject empty creds) so instance credentials cannot be empty even if
installInstance() is invoked directly; reference InstanceConfiguration,
installInstance(), resolveOAuthCredentials, and configuration.IsValid when
implementing these checks.
---
Outside diff comments:
In `@server/plugin.go`:
- Around line 246-278: getOAuthConfig currently builds AuthURL/TokenURL from the
global configuration (getConfiguration().GitlabURL) even when
resolveOAuthCredentials returns per-instance credentials; modify getOAuthConfig
to use the correct instance GitLab URL by either (a) updating
resolveOAuthCredentials to return the instance's GitlabURL along with
clientID/clientSecret and then use that returned URL when constructing
authURL/tokenURL, or (b) calling getInstance(...) from within getOAuthConfig to
obtain the instance object and its GitlabURL after resolving credentials; ensure
you reference resolveOAuthCredentials and getInstance and replace usage of
config.GitlabURL when building oauth2.Endpoint so the auth/token endpoints match
the instance that registered the OAuth app.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/instance.go (1)
19-27: Add a nil-receiver guard to avoid accidental panics.
IsValid()will panic if invoked on a nil*InstanceConfiguration(possible with pointer receivers). A small guard keeps behavior consistent with the nil check ininstallInstanceand protects future call sites.💡 Suggested tweak
func (c *InstanceConfiguration) IsValid() error { + if c == nil { + return errors.New("config is nil") + } if c.GitlabOAuthClientID == "" { return errors.New("must have a GitLab OAuth client ID") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/instance.go` around lines 19 - 27, IsValid currently assumes the receiver is non-nil and will panic if called on a nil *InstanceConfiguration; add a nil-receiver guard at the top of IsValid (check if c == nil) and return an appropriate error (e.g., errors.New("instance configuration is nil")) to match the nil check pattern used in installInstance and prevent panics in future call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/instance.go`:
- Around line 19-27: IsValid currently assumes the receiver is non-nil and will
panic if called on a nil *InstanceConfiguration; add a nil-receiver guard at the
top of IsValid (check if c == nil) and return an appropriate error (e.g.,
errors.New("instance configuration is nil")) to match the nil check pattern used
in installInstance and prevent panics in future call sites.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/plugin.go (1)
284-288:⚠️ Potential issue | 🟠 MajorAvoid read-time instance validation and empty-name instance lookup in credential resolution.
Line 284 still attempts instance lookup even when
DefaultInstanceNameis empty (can reintroduce noisy"instance name '' does not exist"lookup behavior), and Line 286 does read-path validation that returns early on failure, preventing legacy fallback in that case.♻️ Proposed refactor
func (p *Plugin) resolveOAuthCredentials(config *configuration) (clientID, clientSecret, gitlabURL string, err error) { - instanceConfig, instanceErr := p.getInstance(config.DefaultInstanceName) - if instanceErr == nil { - if err := instanceConfig.IsValid(); err != nil { - return "", "", "", fmt.Errorf("instance %q has invalid OAuth credentials: %w", config.DefaultInstanceName, err) - } - return instanceConfig.GitlabOAuthClientID, instanceConfig.GitlabOAuthClientSecret, instanceConfig.GitlabURL, nil - } + var instanceErr error + if config.DefaultInstanceName != "" { + instanceConfig, err := p.getInstance(config.DefaultInstanceName) + if err == nil { + return instanceConfig.GitlabOAuthClientID, instanceConfig.GitlabOAuthClientSecret, instanceConfig.GitlabURL, nil + } + instanceErr = err + }Based on learnings, "validation should only be added at write-time (e.g., in installInstance() when saving to KV store), not at read-time (e.g., in resolveOAuthCredentials()), since OAuth credentials are static values once stored and re-validating on every read would be wasteful."
Also applies to: 292-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin.go` around lines 284 - 288, The current code calls p.getInstance(config.DefaultInstanceName) and then runs instanceConfig.IsValid() during credential resolution, which causes unwanted lookups when DefaultInstanceName == "" and blocks legacy fallback; change resolveOAuthCredentials to skip instance lookup entirely when config.DefaultInstanceName is empty and remove/read-time validation calls to instanceConfig.IsValid() inside resolveOAuthCredentials (leave validation to installInstance or other write-time flows). Specifically, update the logic around p.getInstance(...) and the IsValid() check so resolveOAuthCredentials only reads stored credentials without early-return validation, preserving legacy fallback paths and deferring validation to installInstance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/plugin.go`:
- Around line 262-266: The code ignores errors from url.Parse for gitlabURL
leading to possible nil authURL/tokenURL dereference when calling path.Join;
modify the logic around url.Parse(gitlabURL) so you capture and check both
returned errors and the parsed URL values (authURL, tokenURL), and if either
parse fails handle it (return an error from the surrounding function or log and
abort initialization) before using authURL.Path/tokenURL.Path with path.Join;
update the block that currently assigns authURL, tokenURL to validate the parsed
URLs and use the parsed values only after verifying they are non-nil and err ==
nil.
---
Duplicate comments:
In `@server/plugin.go`:
- Around line 284-288: The current code calls
p.getInstance(config.DefaultInstanceName) and then runs instanceConfig.IsValid()
during credential resolution, which causes unwanted lookups when
DefaultInstanceName == "" and blocks legacy fallback; change
resolveOAuthCredentials to skip instance lookup entirely when
config.DefaultInstanceName is empty and remove/read-time validation calls to
instanceConfig.IsValid() inside resolveOAuthCredentials (leave validation to
installInstance or other write-time flows). Specifically, update the logic
around p.getInstance(...) and the IsValid() check so resolveOAuthCredentials
only reads stored credentials without early-return validation, preserving legacy
fallback paths and deferring validation to installInstance.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/instance.go (1)
24-26: Preserve URL validation cause for easier debugging.Consider wrapping the underlying URL validation error instead of replacing it with a generic message.
Suggested diff
- if err := isValidURL(c.GitlabURL); err != nil { - return errors.New("must have a valid GitLab URL") - } + if err := isValidURL(c.GitlabURL); err != nil { + return fmt.Errorf("must have a valid GitLab URL: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/instance.go` around lines 24 - 26, When validating c.GitlabURL with isValidURL, preserve the underlying error instead of returning a generic error; update the return to wrap the original err (e.g., use fmt.Errorf("must have a valid GitLab URL: %w", err) or errors.Join/Wrap equivalent) so callers see the validation cause, and ensure the package imports fmt or the chosen error-wrapping helper; locate the validation call around isValidURL(c.GitlabURL) and replace the simple errors.New with a wrapped error that includes err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/instance.go`:
- Around line 24-26: When validating c.GitlabURL with isValidURL, preserve the
underlying error instead of returning a generic error; update the return to wrap
the original err (e.g., use fmt.Errorf("must have a valid GitLab URL: %w", err)
or errors.Join/Wrap equivalent) so callers see the validation cause, and ensure
the package imports fmt or the chosen error-wrapping helper; locate the
validation call around isValidURL(c.GitlabURL) and replace the simple errors.New
with a wrapped error that includes err.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/instance.goserver/plugin.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/plugin.go
…itlab - Made sure URL parsing happens as a part of resolveOAuthCredentials - Added test case for handling of malformed URLs to ensure graceful failures - Added validation of both format and userID values when completing connections -- protecting against any requests with spoofed state query param
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api_test.go (1)
347-395: Add a state-delete failure subtest to lock in short-circuit behavior.This suite is strong, but it should also cover
KV.Delete(state)failure and assert the handler returns immediately.✅ Suggested test case
+ t.Run("returns bad request and stops when deleting state fails", func(t *testing.T) { + state := "abcdefghijklmno_" + validUserID + p := setupPlugin(t) + + // stored state exists + p.API.(*plugintest.API).On("KVGet", state).Return([]byte(state), nil).Once() + // delete fails + p.API.(*plugintest.API).On("KVSetWithOptions", state, []byte(nil), mock.Anything).Return(false, assert.AnError).Once() + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state="+state, nil) + r.Header.Set("Mattermost-User-ID", validUserID) + + p.ServeHTTP(nil, w, r) + assert.Equal(t, http.StatusBadRequest, w.Result().StatusCode) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api_test.go` around lines 347 - 395, Add a new subtest that simulates a failure when removing the stored state by having the mock API return an error for the KV delete operation (in this test the code uses KVSetWithOptions(state, []byte(nil), mock.Anything) to clear state) — e.g. api.On("KVGet", state).Return([]byte(state), nil) and api.On("KVSetWithOptions", state, []byte(nil), mock.Anything).Return(false, errors.New("kv delete failed")). Call p.ServeHTTP(nil, w, r) as in the existing test, then assert the handler returned immediately by checking the response and that the delete call was invoked (api.AssertCalled for KVSetWithOptions) and that no token-exchange path was reached (mock whatever external token-exchange hook you have — e.g., the HTTP client or oauthBroker callback — and assert it was not called).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/api.go`:
- Around line 363-369: The handler currently calls p.client.KV.Delete(state) and
on error logs and writes an http.Error but then continues executing the token
exchange; stop execution immediately after a failed Delete to avoid
double-writes and continuing the auth flow. After the existing
c.Log.WithError(err).Warnf("Failed to delete state token") and http.Error(w,
rErr.Error(), http.StatusBadRequest) lines, add an immediate return from the
handler so no further code (including the token exchange) runs; ensure this
change is applied around the p.client.KV.Delete(...) error branch in the same
function.
---
Nitpick comments:
In `@server/api_test.go`:
- Around line 347-395: Add a new subtest that simulates a failure when removing
the stored state by having the mock API return an error for the KV delete
operation (in this test the code uses KVSetWithOptions(state, []byte(nil),
mock.Anything) to clear state) — e.g. api.On("KVGet",
state).Return([]byte(state), nil) and api.On("KVSetWithOptions", state,
[]byte(nil), mock.Anything).Return(false, errors.New("kv delete failed")). Call
p.ServeHTTP(nil, w, r) as in the existing test, then assert the handler returned
immediately by checking the response and that the delete call was invoked
(api.AssertCalled for KVSetWithOptions) and that no token-exchange path was
reached (mock whatever external token-exchange hook you have — e.g., the HTTP
client or oauthBroker callback — and assert it was not called).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/api.goserver/api_test.goserver/instance.goserver/plugin.goserver/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/instance.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/api.go (1)
363-369:⚠️ Potential issue | 🔴 CriticalReturn immediately when state deletion fails.
Line 369 currently falls through after
http.Error(...), so token exchange can still run and write a second response, while also proceeding without guaranteed one-time state invalidation.🐛 Proposed fix
err = p.client.KV.Delete(state) if err != nil { c.Log.WithError(err).Warnf("Failed to delete state token") rErr = errors.Wrap(err, "error deleting stored state") http.Error(w, rErr.Error(), http.StatusBadRequest) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api.go` around lines 363 - 369, When state deletion via p.client.KV.Delete(state) fails the handler currently logs the error, sets rErr and calls http.Error(w, rErr.Error(), http.StatusBadRequest) but then continues executing (potentially performing the token exchange and writing another response). Fix by returning immediately after the http.Error call so the handler stops: locate the block around p.client.KV.Delete(state), c.Log.WithError(err).Warnf("Failed to delete state token"), rErr = errors.Wrap(...), and http.Error(...), and add an immediate return to avoid proceeding with token exchange or writing a second response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/api.go`:
- Around line 363-369: When state deletion via p.client.KV.Delete(state) fails
the handler currently logs the error, sets rErr and calls http.Error(w,
rErr.Error(), http.StatusBadRequest) but then continues executing (potentially
performing the token exchange and writing another response). Fix by returning
immediately after the http.Error call so the handler stops: locate the block
around p.client.KV.Delete(state), c.Log.WithError(err).Warnf("Failed to delete
state token"), rErr = errors.Wrap(...), and http.Error(...), and add an
immediate return to avoid proceeding with token exchange or writing a second
response.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/api.goserver/api_test.goserver/instance.goserver/plugin.goserver/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/plugin_test.go
server/plugin.go
Outdated
| clientID = instanceConfig.GitlabOAuthClientID | ||
| clientSecret = instanceConfig.GitlabOAuthClientSecret | ||
| rawURL = instanceConfig.GitlabURL | ||
| case config.GitlabOAuthClientID != "" && config.GitlabOAuthClientSecret != "": |
There was a problem hiding this comment.
the fallback condition checks GitlabOAuthClientID and GitlabOAuthClientSecret but not GitlabURL. If GitlabURL is empty url.Parse("") on line 303 silently succeeds and produces broken auth and token urls. Add config.GitlabURL != "" to the fallback condition
nevyangelova
left a comment
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114 just one minor change
- Also added test case pointed out as nitpick by ai reviewer
nevyangelova
left a comment
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114
| clientID = instanceConfig.GitlabOAuthClientID | ||
| clientSecret = instanceConfig.GitlabOAuthClientSecret | ||
| rawURL = instanceConfig.GitlabURL | ||
| case config.GitlabOAuthClientID != "" && config.GitlabOAuthClientSecret != "" && config.GitlabURL != "": |
There was a problem hiding this comment.
Should also validate with isValidURL config.GitlabURL.
There was a problem hiding this comment.
Ah fair point, I initially intended to use the URL parser as a form of validation but overlooked the fact that it's overly forgiving at times, I'll add an extra check for isValidURL
server/api.go
Outdated
| rErr = errors.New("not authorized, incorrect user") | ||
| http.Error(w, rErr.Error(), http.StatusUnauthorized) | ||
| rErr = errors.Wrap(err, "error deleting stored state") | ||
| http.Error(w, rErr.Error(), http.StatusBadRequest) |
There was a problem hiding this comment.
i think http.StatusBadRequest should be http.StatusInternalServerError.
server/instance.go
Outdated
| GitlabOAuthClientSecret string `json:"gitlaboauthclientsecret"` | ||
| } | ||
|
|
||
| func (c *InstanceConfiguration) IsValid() error { |
There was a problem hiding this comment.
Consider Validate() to match the error return type convention.
- Returning internal server error on KV store delete errors - Renamed validation function for better clarity - Added better URL validation in oauth credential resolving
|
@coderabbitai The last review seemed to be out-of-date, please review again and approve/request changes depending on the outcome |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Adding @edgarbellot for a security pass on these changes |
| if conf == nil { | ||
| conf, err := p.getOAuthConfig() | ||
| if err != nil { | ||
| c.Log.WithError(err).Warnf("Failed to get OAuth configuration") |
There was a problem hiding this comment.
I believe we explicitly didn't include the err on any oauth requests to avoid leaking configuration details. Any thoughts on this @edgarbellot ?
There was a problem hiding this comment.
@marianunez In connectUserToGitlab the err is only logged server-side and the HTTP response uses a static message, which is the safer pattern. However, in completeConnectUserToGitlab, errors.Wrap(err, ...) bundles the internal error into rErr, which then gets sent to the client via http.Error(w, rErr.Error(), ...).
Tracing the error chain, the message exposed to the client would look roughly like this:
"OAuth configuration not found: failed to resolve OAuth credentials: no OAuth credentials available: instance lookup failed (instance name '' does not exist) and no legacy credentials in plugin settings"
That said, this isn't particularly concerning from a security standpoint since it doesn't expose truly sensitive information - it's mostly internal flow details and configuration state.
I'd just suggest picking one approach and using it consistently in both places. Whether to show the detailed error or keep it generic is more of a UX/product decision at this point, since the security risk here is minimal either way.
There was a problem hiding this comment.
Ah great point, I went ahead and removed the overly descriptive parts of errors that are sent to the client while keeping them in the server logs
server/api.go
Outdated
| if err != nil { | ||
| c.Log.WithError(err).Warnf("Failed to get OAuth configuration") | ||
| rErr = errors.Wrap(err, "OAuth configuration not found") | ||
| http.Error(w, rErr.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
Same here about adding explicit the error
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
LGTM! These changes correctly address the root cause of the nil pointer dereference and a nice fallback strategy in resolveOAuthCredentials. The OAuth state validation is a nice defense-in-depth addition :)
There was a problem hiding this comment.
@avasconcelos114 Tested himself, I'll test OAuth in detail as part of RC testing.
5 participants
Summary
This PR adds better backwards compatibility with servers that relied on the plugin configurations store oauth client ID and secret values instead of utilising the newer instance management features. It accomplishes this via:
resolveOAuthCredentialsfunction that will first try to fetch credentials from an instance (current behavior)The runtime panics described in the issue were caused by the fact that passing an empty
DefaultInstanceNamewould ultimately result ingetOAuthConfigreturningnil, and the server would crash when attempting to dereference it inconf.TokenSource()Ticket Link
Fixes #646
QA Steps
Change Impact: 🟠 Medium
Reasoning: Changes touch OAuth credential resolution and the OAuth connect/complete flows (authentication/token refresh), introduce instance validation, and alter a public method signature (getOAuthConfig) — improvements with added tests but affecting critical auth paths.
Regression Risk: Moderate — authentication and token refresh flows are sensitive; signature change requires all callers to handle errors (some call sites were updated and tests added, but any missed callers or integrations could fail). Added validations and fallback logic reduce crash risk but behavior differs from prior releases in credential resolution precedence.
QA Recommendation: Exercise manual QA focused on OAuth flows: installing v1.11.0 configuration then upgrading, connecting accounts (with/without instances/default instance), token refresh scenarios, and Chimera pre-registered app paths. Verify no panics, correct error messages, and backward compatibility with legacy config. Avoid skipping manual QA due to auth blast radius.
Generated by CodeRabbitAI