Skip to content

Commit deb3981

Browse files
committed
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 <zashaikh@redhat.com>
1 parent e7a2b7b commit deb3981

File tree

2 files changed

+35
-11
lines changed

2 files changed

+35
-11
lines changed

pkg/provider/github/github.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,26 +258,33 @@ func parseTS(headerTS string) (time.Time, error) {
258258
// the issue was.
259259
func (v *Provider) checkWebhookSecretValidity(ctx context.Context, cw clockwork.Clock) error {
260260
rl, resp, err := v.Client().RateLimit.Get(ctx)
261-
if resp.StatusCode == http.StatusNotFound {
261+
if resp != nil && resp.StatusCode == http.StatusNotFound {
262262
v.Logger.Info("skipping checking if token has expired, rate_limit api is not enabled on token")
263263
return nil
264264
}
265265

266266
if err != nil {
267267
return fmt.Errorf("error making request to the GitHub API checking rate limit: %w", err)
268268
}
269-
if resp.Header.Get("GitHub-Authentication-Token-Expiration") != "" {
269+
270+
if resp != nil && resp.Header.Get("GitHub-Authentication-Token-Expiration") != "" {
270271
ts, err := parseTS(resp.Header.Get("GitHub-Authentication-Token-Expiration"))
271272
if err != nil {
272273
return fmt.Errorf("error parsing token expiration date: %w", err)
273274
}
274275

275276
if cw.Now().After(ts) {
276-
errm := fmt.Sprintf("token has expired at %s", resp.TokenExpiration.Format(time.RFC1123))
277-
return fmt.Errorf("%s", errm)
277+
errMsg := fmt.Sprintf("token has expired at %s", resp.TokenExpiration.Format(time.RFC1123))
278+
return fmt.Errorf("%s", errMsg)
278279
}
279280
}
280281

282+
// Guard against nil rl or rl.SCIM which could lead to a panic.
283+
if rl == nil || rl.SCIM == nil {
284+
v.Logger.Info("skipping token expiration check, SCIM rate limit API is not available for this token")
285+
return nil
286+
}
287+
281288
if rl.SCIM.Remaining == 0 {
282289
return fmt.Errorf("api rate limit exceeded. Access will be restored at %s", rl.SCIM.Reset.Format(time.RFC1123))
283290
}

pkg/provider/github/github_test.go

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -963,7 +963,8 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
963963
expHeaderSet bool
964964
apiNotEnabled bool
965965
wantLogSnippet string
966-
report500 bool
966+
statusCode int
967+
wantSCIMNil bool
967968
}{
968969
{
969970
name: "remaining scim calls",
@@ -988,6 +989,16 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
988989
name: "no header mean unlimited",
989990
remaining: 5,
990991
},
992+
{
993+
name: "skipping api rate limit is not enabled",
994+
remaining: 0,
995+
statusCode: http.StatusNotFound,
996+
},
997+
{
998+
name: "skipping because scim is not available",
999+
remaining: 0,
1000+
wantSCIMNil: true,
1001+
},
9911002
{
9921003
name: "no header but no remaining scim calls",
9931004
remaining: 0,
@@ -996,7 +1007,12 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
9961007
{
9971008
name: "api error",
9981009
wantSubErr: "error making request to the GitHub API checking rate limit",
999-
report500: true,
1010+
statusCode: http.StatusInternalServerError,
1011+
},
1012+
{
1013+
name: "not enabled",
1014+
apiNotEnabled: true,
1015+
wantLogSnippet: "skipping checking",
10001016
},
10011017
{
10021018
name: "not enabled",
@@ -1012,14 +1028,15 @@ func TestProvider_checkWebhookSecretValidity(t *testing.T) {
10121028

10131029
if !tt.apiNotEnabled {
10141030
mux.HandleFunc("/rate_limit", func(rw http.ResponseWriter, _ *http.Request) {
1015-
if tt.report500 {
1016-
rw.WriteHeader(http.StatusInternalServerError)
1031+
if tt.statusCode != 0 {
1032+
rw.WriteHeader(tt.statusCode)
10171033
return
10181034
}
1019-
s := &github.RateLimits{
1020-
SCIM: &github.Rate{
1035+
s := &github.RateLimits{}
1036+
if !tt.wantSCIMNil {
1037+
s.SCIM = &github.Rate{
10211038
Remaining: tt.remaining,
1022-
},
1039+
}
10231040
}
10241041
st := new(struct {
10251042
Resources *github.RateLimits `json:"resources"`

0 commit comments

Comments
 (0)