Skip to content

Commit 98168bc

Browse files
avasconcelos114avasconcelos114
authored
Enhancing backwards compatibility of OAuth credential fetching (#647)
* Implementing backwards compatibility with previous versions oauth credential fetching - Making sure that errors with fetching credentials fail more gracefully * Fixing linter errors * Added write-time validation of oauth credentials for instance * Ensuring that the GitlabURL properly corresponds to the instance or plugin config * Improving validation of instance config * Reworked resolveOAuthCredentials and protected completeConnectUserToGitlab - 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 * Added missing return line * Adding check against GitlabURL - Also added test case pointed out as nitpick by ai reviewer * Addressing PR issues - Returning internal server error on KV store delete errors - Renamed validation function for better clarity - Added better URL validation in oauth credential resolving * Simplifying error messages sent to client during connection
1 parent edea524 commit 98168bc

File tree

5 files changed

+523
-36
lines changed

5 files changed

+523
-36
lines changed

‎server/api.go‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"net/http"
1111
"net/url"
1212
"path"
13+
"regexp"
1314
"runtime/debug"
1415
"strconv"
1516
"strings"
@@ -29,6 +30,8 @@ import (
2930
"github.com/mattermost/mattermost-plugin-gitlab/server/subscription"
3031
)
3132

33+
var oauthStateRegexp = regexp.MustCompile(`^[a-z0-9]{15}_[a-z0-9]{26}$`)
34+
3235
const (
3336
APIErrorIDNotConnected = "not_connected"
3437

@@ -248,8 +251,9 @@ func (p *Plugin) connectUserToGitlab(c *Context, w http.ResponseWriter, r *http.
248251
return
249252
}
250253

251-
conf := p.getOAuthConfig()
252-
if conf == nil {
254+
conf, err := p.getOAuthConfig()
255+
if err != nil {
256+
c.Log.WithError(err).Warnf("Failed to get OAuth configuration")
253257
http.Error(w, "OAuth configuration not found", http.StatusInternalServerError)
254258
return
255259
}
@@ -310,46 +314,58 @@ func (p *Plugin) completeConnectUserToGitlab(c *Context, w http.ResponseWriter,
310314

311315
config := p.getConfiguration()
312316

313-
conf := p.getOAuthConfig()
317+
conf, err := p.getOAuthConfig()
318+
if err != nil {
319+
c.Log.WithError(err).Warnf("Failed to get OAuth configuration")
320+
rErr = errors.Wrap(err, "OAuth configuration not found")
321+
http.Error(w, "OAuth configuration not found", http.StatusInternalServerError)
322+
return
323+
}
314324

315325
code := r.URL.Query().Get("code")
316326
if len(code) == 0 {
317327
rErr = errors.New("missing authorization code")
318-
http.Error(w, rErr.Error(), http.StatusBadRequest)
328+
http.Error(w, "Missing authorization code", http.StatusBadRequest)
319329
return
320330
}
321331

322332
state := r.URL.Query().Get("state")
323333

324-
var storedState []byte
325-
err := p.client.KV.Get(state, &storedState)
326-
if err != nil {
327-
c.Log.WithError(err).Warnf("Can't get state from store")
334+
if !oauthStateRegexp.MatchString(state) {
335+
rErr = errors.New("invalid state format")
336+
http.Error(w, "Invalid OAuth state", http.StatusBadRequest)
337+
return
338+
}
328339

329-
rErr = errors.Wrap(err, "missing stored state")
330-
http.Error(w, rErr.Error(), http.StatusBadRequest)
340+
userID := strings.Split(state, "_")[1]
341+
if userID != authedUserID {
342+
rErr = errors.New("not authorized, incorrect user")
343+
http.Error(w, "Not authorized", http.StatusUnauthorized)
331344
return
332345
}
333346

334-
err = p.client.KV.Delete(state)
347+
var storedState []byte
348+
err = p.client.KV.Get(state, &storedState)
335349
if err != nil {
336-
c.Log.WithError(err).Warnf("Failed to delete state token")
350+
c.Log.WithError(err).Warnf("Can't get state from store")
337351

338-
rErr = errors.Wrap(err, "error deleting stored state")
339-
http.Error(w, rErr.Error(), http.StatusBadRequest)
352+
rErr = errors.Wrap(err, "missing stored state")
353+
http.Error(w, "Missing stored OAuth state", http.StatusBadRequest)
354+
return
340355
}
341356

342357
if string(storedState) != state {
343358
rErr = errors.New("invalid state token")
344-
http.Error(w, rErr.Error(), http.StatusBadRequest)
359+
http.Error(w, "Invalid OAuth state", http.StatusBadRequest)
345360
return
346361
}
347362

348-
userID := strings.Split(state, "_")[1]
363+
err = p.client.KV.Delete(state)
364+
if err != nil {
365+
c.Log.WithError(err).Warnf("Failed to delete state token")
349366

350-
if userID != authedUserID {
351-
rErr = errors.New("not authorized, incorrect user")
352-
http.Error(w, rErr.Error(), http.StatusUnauthorized)
367+
rErr = errors.Wrap(err, "error deleting stored state")
368+
http.Error(w, "Error completing OAuth connection", http.StatusInternalServerError)
353369
return
354370
}
355371

@@ -358,7 +374,7 @@ func (p *Plugin) completeConnectUserToGitlab(c *Context, w http.ResponseWriter,
358374
c.Log.WithError(err).Warnf("Can't exchange state")
359375

360376
rErr = errors.Wrap(err, "Failed to exchange oauth code into token")
361-
http.Error(w, rErr.Error(), http.StatusInternalServerError)
377+
http.Error(w, "Error completing OAuth connection", http.StatusInternalServerError)
362378
return
363379
}
364380

@@ -367,23 +383,23 @@ func (p *Plugin) completeConnectUserToGitlab(c *Context, w http.ResponseWriter,
367383
c.Log.WithError(err).Warnf("Can't retrieve user info from gitLab API")
368384

369385
rErr = errors.Wrap(err, "unable to connect user to GitLab")
370-
http.Error(w, rErr.Error(), http.StatusInternalServerError)
386+
http.Error(w, "Unable to connect user to GitLab", http.StatusInternalServerError)
371387
return
372388
}
373389

374390
if err = p.storeGitlabUserInfo(userInfo); err != nil {
375391
c.Log.WithError(err).Warnf("Can't store user info")
376392

377393
rErr = errors.Wrap(err, "Unable to connect user to GitLab")
378-
http.Error(w, rErr.Error(), http.StatusInternalServerError)
394+
http.Error(w, "Unable to connect user to GitLab", http.StatusInternalServerError)
379395
return
380396
}
381397

382398
if err = p.storeGitlabUserToken(userInfo.UserID, tok); err != nil {
383399
c.Log.WithError(err).Warnf("Can't store user token")
384400

385401
rErr = errors.Wrap(err, "Unable to connect user to GitLab")
386-
http.Error(w, rErr.Error(), http.StatusInternalServerError)
402+
http.Error(w, "Unable to connect user to GitLab", http.StatusInternalServerError)
387403
return
388404
}
389405

‎server/api_test.go‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,201 @@ func TestCreateIssueAllowsWhenGitlabGroupEmpty(t *testing.T) {
267267
assert.Equal(t, http.StatusOK, result.StatusCode)
268268
}
269269

270+
func TestCompleteConnectUserToGitlab_StateValidation(t *testing.T) {
271+
validUserID := "abcdefghijklmnopqrstuvwxyz"
272+
273+
setupPlugin := func(t *testing.T) *Plugin {
274+
t.Helper()
275+
276+
siteURL := "https://mattermost.example.com"
277+
mmConfig := &model.Config{}
278+
mmConfig.ServiceSettings.SiteURL = &siteURL
279+
280+
config := configuration{
281+
GitlabURL: "https://gitlab.example.com",
282+
GitlabOAuthClientID: "client_id",
283+
GitlabOAuthClientSecret: "client_secret",
284+
EncryptionKey: "aaaaaaaaaaaaaaaa",
285+
}
286+
287+
p := &Plugin{configuration: &config}
288+
p.initializeAPI()
289+
290+
api := &plugintest.API{}
291+
api.On("GetConfig").Return(mmConfig)
292+
api.On("KVGet", mock.Anything).Return(nil, nil).Maybe()
293+
api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
294+
api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
295+
p.SetAPI(api)
296+
p.client = pluginapi.NewClient(api, p.Driver)
297+
p.oauthBroker = NewOAuthBroker(func(_ OAuthCompleteEvent) {})
298+
return p
299+
}
300+
301+
t.Run("rejects state with arbitrary KV key name", func(t *testing.T) {
302+
p := setupPlugin(t)
303+
304+
w := httptest.NewRecorder()
305+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state=Gitlab_Instance_Configuration_Map", nil)
306+
r.Header.Set("Mattermost-User-ID", validUserID)
307+
308+
p.ServeHTTP(nil, w, r)
309+
310+
result := w.Result()
311+
defer func() { _ = result.Body.Close() }()
312+
assert.Equal(t, http.StatusBadRequest, result.StatusCode)
313+
data, _ := io.ReadAll(result.Body)
314+
assert.Contains(t, string(data), "Invalid OAuth state")
315+
})
316+
317+
t.Run("rejects state targeting user token key", func(t *testing.T) {
318+
p := setupPlugin(t)
319+
320+
w := httptest.NewRecorder()
321+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state="+validUserID+"_usertoken", nil)
322+
r.Header.Set("Mattermost-User-ID", validUserID)
323+
324+
p.ServeHTTP(nil, w, r)
325+
326+
result := w.Result()
327+
defer func() { _ = result.Body.Close() }()
328+
assert.Equal(t, http.StatusBadRequest, result.StatusCode)
329+
data, _ := io.ReadAll(result.Body)
330+
assert.Contains(t, string(data), "Invalid OAuth state")
331+
})
332+
333+
t.Run("rejects empty state", func(t *testing.T) {
334+
p := setupPlugin(t)
335+
336+
w := httptest.NewRecorder()
337+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state=", nil)
338+
r.Header.Set("Mattermost-User-ID", validUserID)
339+
340+
p.ServeHTTP(nil, w, r)
341+
342+
result := w.Result()
343+
defer func() { _ = result.Body.Close() }()
344+
assert.Equal(t, http.StatusBadRequest, result.StatusCode)
345+
})
346+
347+
t.Run("passes validation with correctly formatted state and matching user", func(t *testing.T) {
348+
state := "abcdefghijklmno_" + validUserID
349+
350+
siteURL := "https://mattermost.example.com"
351+
mmConfig := &model.Config{}
352+
mmConfig.ServiceSettings.SiteURL = &siteURL
353+
354+
config := configuration{
355+
GitlabURL: "https://gitlab.example.com",
356+
GitlabOAuthClientID: "client_id",
357+
GitlabOAuthClientSecret: "client_secret",
358+
EncryptionKey: "aaaaaaaaaaaaaaaa",
359+
}
360+
361+
p := &Plugin{configuration: &config}
362+
p.initializeAPI()
363+
364+
api := &plugintest.API{}
365+
api.On("GetConfig").Return(mmConfig)
366+
api.On("KVGet", state).Return([]byte(state), nil)
367+
api.On("KVSetWithOptions", state, []byte(nil), mock.Anything).Return(true, nil)
368+
api.On("KVGet", instanceConfigNameListKey).Return(nil, nil)
369+
api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
370+
api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
371+
api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
372+
p.SetAPI(api)
373+
p.client = pluginapi.NewClient(api, p.Driver)
374+
p.oauthBroker = NewOAuthBroker(func(_ OAuthCompleteEvent) {})
375+
376+
w := httptest.NewRecorder()
377+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state="+state, nil)
378+
r.Header.Set("Mattermost-User-ID", validUserID)
379+
380+
p.ServeHTTP(nil, w, r)
381+
382+
result := w.Result()
383+
defer func() { _ = result.Body.Close() }()
384+
385+
// The request passes state validation and proceeds to the token exchange,
386+
// which fails because there's no real GitLab server — that's an Internal
387+
// Server Error, not a Bad Request or Unauthorized from the validation gates.
388+
assert.Equal(t, http.StatusInternalServerError, result.StatusCode)
389+
data, _ := io.ReadAll(result.Body)
390+
assert.NotContains(t, string(data), "invalid state")
391+
assert.NotContains(t, string(data), "not authorized, incorrect user")
392+
393+
api.AssertCalled(t, "KVGet", state)
394+
api.AssertCalled(t, "KVSetWithOptions", state, []byte(nil), mock.Anything)
395+
})
396+
397+
t.Run("returns error when KV delete of state token fails", func(t *testing.T) {
398+
state := "abcdefghijklmno_" + validUserID
399+
400+
siteURL := "https://mattermost.example.com"
401+
mmConfig := &model.Config{}
402+
mmConfig.ServiceSettings.SiteURL = &siteURL
403+
404+
config := configuration{
405+
GitlabURL: "https://gitlab.example.com",
406+
GitlabOAuthClientID: "client_id",
407+
GitlabOAuthClientSecret: "client_secret",
408+
EncryptionKey: "aaaaaaaaaaaaaaaa",
409+
}
410+
411+
p := &Plugin{configuration: &config}
412+
p.initializeAPI()
413+
414+
kvDeleteErr := model.NewAppError("KVDelete", "plugin.kv_delete.error", nil, "storage failure", http.StatusInternalServerError)
415+
416+
api := &plugintest.API{}
417+
api.On("GetConfig").Return(mmConfig)
418+
api.On("KVGet", state).Return([]byte(state), nil)
419+
api.On("KVSetWithOptions", state, []byte(nil), mock.Anything).Return(false, kvDeleteErr)
420+
api.On("KVGet", instanceConfigNameListKey).Return(nil, nil)
421+
api.On("LogDebug", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
422+
api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
423+
api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
424+
p.SetAPI(api)
425+
p.client = pluginapi.NewClient(api, p.Driver)
426+
p.oauthBroker = NewOAuthBroker(func(_ OAuthCompleteEvent) {})
427+
428+
w := httptest.NewRecorder()
429+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state="+state, nil)
430+
r.Header.Set("Mattermost-User-ID", validUserID)
431+
432+
p.ServeHTTP(nil, w, r)
433+
434+
result := w.Result()
435+
defer func() { _ = result.Body.Close() }()
436+
437+
assert.Equal(t, http.StatusInternalServerError, result.StatusCode)
438+
data, _ := io.ReadAll(result.Body)
439+
assert.Contains(t, string(data), "Error completing OAuth connection")
440+
441+
api.AssertCalled(t, "KVSetWithOptions", state, []byte(nil), mock.Anything)
442+
api.AssertNotCalled(t, "KVGet", "user_id_usertoken")
443+
})
444+
445+
t.Run("rejects state with wrong user ID", func(t *testing.T) {
446+
p := setupPlugin(t)
447+
448+
differentUserID := "zyxwvutsrqponmlkjihgfedcba"
449+
state := "abcdefghijklmno_" + differentUserID
450+
451+
w := httptest.NewRecorder()
452+
r := httptest.NewRequest(http.MethodGet, "/oauth/complete?code=test&state="+state, nil)
453+
r.Header.Set("Mattermost-User-ID", validUserID)
454+
455+
p.ServeHTTP(nil, w, r)
456+
457+
result := w.Result()
458+
defer func() { _ = result.Body.Close() }()
459+
assert.Equal(t, http.StatusUnauthorized, result.StatusCode)
460+
data, _ := io.ReadAll(result.Body)
461+
assert.Contains(t, string(data), "Not authorized")
462+
})
463+
}
464+
270465
func TestAttachCommentToIssueReturns403WhenNamespaceNotAllowed(t *testing.T) {
271466
fakeGitLab := fakeGitLabServer(t, "othergroup/repo")
272467
defer fakeGitLab.Close()

‎server/instance.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,26 @@ type InstanceConfiguration struct {
1616
GitlabOAuthClientSecret string `json:"gitlaboauthclientsecret"`
1717
}
1818

19+
func (c *InstanceConfiguration) Validate() error {
20+
if c == nil {
21+
return errors.New("instance configuration is nil")
22+
}
23+
24+
if err := isValidURL(c.GitlabURL); err != nil {
25+
return errors.Wrap(err, "must have a valid GitLab URL")
26+
}
27+
28+
if c.GitlabOAuthClientID == "" {
29+
return errors.New("must have a GitLab OAuth client ID")
30+
}
31+
32+
if c.GitlabOAuthClientSecret == "" {
33+
return errors.New("must have a GitLab OAuth client secret")
34+
}
35+
36+
return nil
37+
}
38+
1939
const (
2040
instanceConfigMapKey = "Gitlab_Instance_Configuration_Map"
2141
instanceConfigNameListKey = "Gitlab_Instance_Configuration_Name_List"
@@ -26,6 +46,10 @@ func (p *Plugin) installInstance(instanceName string, config *InstanceConfigurat
2646
return errors.New("config is nil")
2747
}
2848

49+
if err := config.Validate(); err != nil {
50+
return fmt.Errorf("invalid instance configuration: %w", err)
51+
}
52+
2953
var instanceNameList []string
3054
err := p.client.KV.Get(instanceConfigNameListKey, &instanceNameList)
3155
if err != nil {

0 commit comments

Comments
 (0)