fix(github): guard checkWebhookSecretValidity against nil response

Fix nil-pointer panic when /rate_limit returns success without SCIM data
or when the HTTP response itself is nil.
Add early err/resp checks and ensure rl and rl.SCIM are non-nil
before accessing them.

Jira: https://issues.redhat.com/browse/SRVKP-8075

Signed-off-by: Zaki Shaikh <[email protected]>

Author: zakisk

pkg/provider/github/github.go

@@ -258,26 +258,33 @@ func parseTS(headerTS string) (time.Time, error) {
 // the issue was.
 func (v *Provider) checkWebhookSecretValidity(ctx context.Context, cw clockwork.Clock) error {
 	rl, resp, err := v.Client().RateLimit.Get(ctx)
-	if resp.StatusCode == http.StatusNotFound {
+	if resp != nil && resp.StatusCode == http.StatusNotFound {
 		v.Logger.Info("skipping checking if token has expired, rate_limit api is not enabled on token")
 		return nil
 	}
 
 	if err != nil {
 		return fmt.Errorf("error making request to the GitHub API checking rate limit: %w", err)
 	}
-	if resp.Header.Get("GitHub-Authentication-Token-Expiration") != "" {
+
+	if resp != nil && resp.Header.Get("GitHub-Authentication-Token-Expiration") != "" {
 		ts, err := parseTS(resp.Header.Get("GitHub-Authentication-Token-Expiration"))
 		if err != nil {
 			return fmt.Errorf("error parsing token expiration date: %w", err)
 		}
 
 		if cw.Now().After(ts) {
-			errm := fmt.Sprintf("token has expired at %s", resp.TokenExpiration.Format(time.RFC1123))
-			return fmt.Errorf("%s", errm)
+			errMsg := fmt.Sprintf("token has expired at %s", resp.TokenExpiration.Format(time.RFC1123))
+			return fmt.Errorf("%s", errMsg)
 		}
 	}
 
+	// Guard against nil rl or rl.SCIM which could lead to a panic.
+	if rl == nil || rl.SCIM == nil {
+		v.Logger.Info("skipping token expiration check, SCIM rate limit API is not available for this token")
+		return nil
+	}
+
 	if rl.SCIM.Remaining == 0 {
 		return fmt.Errorf("api rate limit exceeded. Access will be restored at %s", rl.SCIM.Reset.Format(time.RFC1123))
 	}

pkg/provider/github/github_test.go

@@ -914,6 +914,12 @@ func TestGetFiles(t *testing.T) {
 	}
 }
 
+type roundTripperFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) {
+	return f(r)
+}
+
 func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 	t1 := time.Date(1999, time.February, 3, 4, 5, 6, 7, time.UTC)
 	cw := clockwork.NewFakeClockAt(t1)
@@ -925,7 +931,9 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 		expHeaderSet   bool
 		apiNotEnabled  bool
 		wantLogSnippet string
-		report500      bool
+		statusCode     int
+		wantNilSCIM    bool
+		wantNilResp    bool
 	}{
 		{
 			name:         "remaining scim calls",
@@ -950,6 +958,22 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 			name:      "no header mean unlimited",
 			remaining: 5,
 		},
+		{
+			name:       "skipping api rate limit is not enabled",
+			remaining:  0,
+			statusCode: http.StatusNotFound,
+		},
+		{
+			name:        "skipping because scim is not available",
+			remaining:   0,
+			wantNilSCIM: true,
+		},
+		{
+			name:        "resp is nil",
+			remaining:   0,
+			wantNilResp: true,
+			wantSubErr:  "error making request to the GitHub API checking rate limit",
+		},
 		{
 			name:       "no header but no remaining scim calls",
 			remaining:  0,
@@ -958,7 +982,12 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 		{
 			name:       "api error",
 			wantSubErr: "error making request to the GitHub API checking rate limit",
-			report500:  true,
+			statusCode: http.StatusInternalServerError,
+		},
+		{
+			name:           "not enabled",
+			apiNotEnabled:  true,
+			wantLogSnippet: "skipping checking",
 		},
 		{
 			name:           "not enabled",
@@ -974,14 +1003,15 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 
 			if !tt.apiNotEnabled {
 				mux.HandleFunc("/rate_limit", func(rw http.ResponseWriter, _ *http.Request) {
-					if tt.report500 {
-						rw.WriteHeader(http.StatusInternalServerError)
+					if tt.statusCode != 0 {
+						rw.WriteHeader(tt.statusCode)
 						return
 					}
-					s := &github.RateLimits{
-						SCIM: &github.Rate{
+					s := &github.RateLimits{}
+					if !tt.wantNilSCIM {
+						s.SCIM = &github.Rate{
 							Remaining: tt.remaining,
-						},
+						}
 					}
 					st := new(struct {
 						Resources *github.RateLimits `json:"resources"`
@@ -996,6 +1026,16 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
 				})
 			}
 			defer teardown()
+
+			// create bad round tripper to make response nil and test that it handles that case.
+			if tt.wantNilResp {
+				errRT := roundTripperFunc(func(*http.Request) (*http.Response, error) {
+					return nil, fmt.Errorf("network down")
+				})
+				httpClient := &http.Client{Transport: errRT}
+				fakeclient = github.NewClient(httpClient)
+			}
+
 			v := &Provider{
 				ghClient: fakeclient,
 				Logger:   logger,