Skip to content

Commit 9f82e4d

Browse files
authored
fix: add check for nil secret on oauthv2 (#5807)
# Description We are adding validation for a nil secret being stored in the cache, for OAuthV2. ## Linear Ticket https://linear.app/rudderstack/issue/INT-3599/fix-add-validation-for-nil-secret-being-stored-in-cache-for-oauthv2 ## Security - [ ] The code changed/added as part of this pull request won't create any security issues with how the software is being used.
1 parent d7ec6bc commit 9f82e4d

File tree

2 files changed

+94
-0
lines changed

2 files changed

+94
-0
lines changed

services/oauth/v2/oauth.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,10 @@ func (h *OAuthHandler) GetRefreshTokenErrResp(response string, accountSecret *Ac
356356
return errorType, message
357357
}
358358

359+
func isEmptySecret(accountSecret AccountSecret) bool {
360+
return accountSecret.Secret == nil || string(accountSecret.Secret) == `{}` || string(accountSecret.Secret) == "\"\"" || string(accountSecret.Secret) == "null"
361+
}
362+
359363
// This method hits the Control Plane to get the account information
360364
// As well update the account information into the destAuthInfoMap(which acts as an in-memory cache)
361365
func (h *OAuthHandler) fetchAccountInfoFromCp(refTokenParams *RefreshTokenParams, refTokenBody RefreshTokenBodyParams,
@@ -429,6 +433,15 @@ func (h *OAuthHandler) fetchAccountInfoFromCp(refTokenParams *RefreshTokenParams
429433
}
430434
return http.StatusInternalServerError, authResponse, fmt.Errorf("error occurred while fetching/refreshing account info from CP: %s", refErrMsg)
431435
}
436+
437+
if isEmptySecret(accountSecret) {
438+
errMsg := errors.New("empty secret received from CP")
439+
statsHandler.Increment("request", stats.Tags{
440+
"errorMessage": errMsg.Error(),
441+
"isCallToCpApi": "true",
442+
})
443+
return http.StatusInternalServerError, nil, errMsg
444+
}
432445
statsHandler.Increment("request", stats.Tags{
433446
"errorMessage": "",
434447
"isCallToCpApi": "true",

services/oauth/v2/oauth_test.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,87 @@ var _ = Describe("Oauth", func() {
359359
Expect(response).To(Equal(expectedResponse))
360360
Expect(err).To(MatchError(fmt.Errorf("error occurred while fetching/refreshing account info from CP: mock mock 127.0.0.1:1234->127.0.0.1:12340: read: connection timed out")))
361361
})
362+
363+
It("fetch token function call when cache is empty and cpApiCall returns empty secret", func() {
364+
fetchTokenParams := &v2.RefreshTokenParams{
365+
AccountID: "123",
366+
WorkspaceID: "456",
367+
DestDefName: "testDest",
368+
Destination: Destination,
369+
}
370+
371+
ctrl := gomock.NewController(GinkgoT())
372+
mockCpConnector := mock_oauthV2.NewMockConnector(ctrl)
373+
mockCpConnector.EXPECT().CpApiCall(gomock.Any()).Return(http.StatusOK, `{"secret":{}}`)
374+
mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl)
375+
mockTokenProvider.EXPECT().Identity().Return(nil)
376+
oauthHandler := v2.NewOAuthHandler(mockTokenProvider,
377+
v2.WithCache(v2.NewCache()),
378+
v2.WithLocker(kitsync.NewPartitionRWLocker()),
379+
v2.WithStats(stats.Default),
380+
v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")),
381+
v2.WithCpConnector(mockCpConnector),
382+
)
383+
statusCode, response, err := oauthHandler.FetchToken(fetchTokenParams)
384+
Expect(statusCode).To(Equal(http.StatusInternalServerError))
385+
var expectedResponse *v2.AuthResponse
386+
Expect(response).To(Equal(expectedResponse))
387+
Expect(err).To(MatchError(fmt.Errorf("empty secret received from CP")))
388+
})
389+
390+
It("fetch token function call when cache is empty and cpApiCall returns empty string", func() {
391+
fetchTokenParams := &v2.RefreshTokenParams{
392+
AccountID: "123",
393+
WorkspaceID: "456",
394+
DestDefName: "testDest",
395+
Destination: Destination,
396+
}
397+
398+
ctrl := gomock.NewController(GinkgoT())
399+
mockCpConnector := mock_oauthV2.NewMockConnector(ctrl)
400+
mockCpConnector.EXPECT().CpApiCall(gomock.Any()).Return(http.StatusOK, `{"secret":""}`)
401+
mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl)
402+
mockTokenProvider.EXPECT().Identity().Return(nil)
403+
oauthHandler := v2.NewOAuthHandler(mockTokenProvider,
404+
v2.WithCache(v2.NewCache()),
405+
v2.WithLocker(kitsync.NewPartitionRWLocker()),
406+
v2.WithStats(stats.Default),
407+
v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")),
408+
v2.WithCpConnector(mockCpConnector),
409+
)
410+
statusCode, response, err := oauthHandler.FetchToken(fetchTokenParams)
411+
Expect(statusCode).To(Equal(http.StatusInternalServerError))
412+
var expectedResponse *v2.AuthResponse
413+
Expect(response).To(Equal(expectedResponse))
414+
Expect(err).To(MatchError(fmt.Errorf("empty secret received from CP")))
415+
})
416+
417+
It("fetch token function call when cache is empty and cpApiCall returns nil secret", func() {
418+
fetchTokenParams := &v2.RefreshTokenParams{
419+
AccountID: "123",
420+
WorkspaceID: "456",
421+
DestDefName: "testDest",
422+
Destination: Destination,
423+
}
424+
425+
ctrl := gomock.NewController(GinkgoT())
426+
mockCpConnector := mock_oauthV2.NewMockConnector(ctrl)
427+
mockCpConnector.EXPECT().CpApiCall(gomock.Any()).Return(http.StatusOK, `{"secret": null}`)
428+
mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl)
429+
mockTokenProvider.EXPECT().Identity().Return(nil)
430+
oauthHandler := v2.NewOAuthHandler(mockTokenProvider,
431+
v2.WithCache(v2.NewCache()),
432+
v2.WithLocker(kitsync.NewPartitionRWLocker()),
433+
v2.WithStats(stats.Default),
434+
v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")),
435+
v2.WithCpConnector(mockCpConnector),
436+
)
437+
statusCode, response, err := oauthHandler.FetchToken(fetchTokenParams)
438+
Expect(statusCode).To(Equal(http.StatusInternalServerError))
439+
var expectedResponse *v2.AuthResponse
440+
Expect(response).To(Equal(expectedResponse))
441+
Expect(err).To(MatchError(fmt.Errorf("empty secret received from CP")))
442+
})
362443
})
363444

364445
Describe("Test RefreshToken function", func() {

0 commit comments

Comments
 (0)