diff --git a/pkg/github/client/errorsourcehandling.go b/pkg/github/client/errorsourcehandling.go index d48c7144..a8fa90d7 100644 --- a/pkg/github/client/errorsourcehandling.go +++ b/pkg/github/client/errorsourcehandling.go @@ -36,8 +36,20 @@ var ( // // See TestGitHubErrorResponseWithTypedNilErrorsIs for a reproduction of the panic. func sanitizeGitHubError(err error) error { - var ghErr *googlegithub.ErrorResponse - if errors.As(err, &ghErr) { + // Flatten to a plain error to drop the unsafe Is method while keeping the + // message. These are every go-github error type that defines such an Is (github.go). + var ( + errResp *googlegithub.ErrorResponse + rateErr *googlegithub.RateLimitError + abuseErr *googlegithub.AbuseRateLimitError + acceptErr *googlegithub.AcceptedError + redirectErr *googlegithub.RedirectionError + ) + if errors.As(err, &errResp) || + errors.As(err, &rateErr) || + errors.As(err, &abuseErr) || + errors.As(err, &acceptErr) || + errors.As(err, &redirectErr) { return fmt.Errorf("%s", err.Error()) } return err @@ -74,8 +86,10 @@ func addErrorSourceToError(err error, resp *googlegithub.Response) error { return backend.PluginError(err) } } - // If we have response we can use the status code from it - if resp != nil { + // If there is a response, use the status code from it. + // resp.StatusCode reads go-github's embedded *http.Response. That pointer can + // be nil even when resp is not nil. Guard it to avoid a nil-pointer panic. + if resp != nil && resp.Response != nil { if resp.StatusCode/100 != 2 { if backend.ErrorSourceFromHTTPStatus(resp.StatusCode) == backend.ErrorSourceDownstream { return backend.DownstreamError(err) diff --git a/pkg/github/client/errorsourcehandling_test.go b/pkg/github/client/errorsourcehandling_test.go index e2f65d0f..ece45673 100644 --- a/pkg/github/client/errorsourcehandling_test.go +++ b/pkg/github/client/errorsourcehandling_test.go @@ -95,6 +95,15 @@ func TestAddErrorSourceToError(t *testing.T) { resp: nil, expected: backend.DownstreamError(errors.New("Resource not accessible by integration")), }, + { + // go-github's Response embeds a *http.Response that can be nil, even + // when the Response wrapper is not nil. Then resp.StatusCode panics. + // The function must return the error as-is. It must not crash. + name: "response wrapper with nil inner http.Response does not panic", + err: errors.New("some transport error"), + resp: &googlegithub.Response{}, + expected: errors.New("some transport error"), + }, } for _, tt := range tests { @@ -242,3 +251,27 @@ func TestExtractStatusCode(t *testing.T) { }) } } + +// TestSanitizeGitHubError_RateLimitErrorNoPanic checks that a rate-limit error does +// not crash the SDK's error classification. go-github's *RateLimitError has an Is +// method that calls errors.As(target, &v). The SDK classifies errors with +// errors.Is(err, (*url.Error)(nil)), where the target is a typed nil. That makes the +// errors.As call (*url.Error).Unwrap on a nil receiver and panic. sanitizeGitHubError +// flattens the error so this cannot happen. +func TestSanitizeGitHubError_RateLimitErrorNoPanic(t *testing.T) { + req, _ := http.NewRequest("GET", "https://api.github.com/x", nil) + ghResp := &http.Response{StatusCode: 403, Request: req} + rle := &googlegithub.RateLimitError{ + Response: ghResp, + Message: "API rate limit exceeded", + } + + // What the plugin returns up the chain for a rate-limit error. + classified := addErrorSourceToError(rle, &googlegithub.Response{Response: ghResp}) + + // Exactly what the SDK's guessErrorStatus does (status.go:112). + var connErr *url.Error + require.NotPanics(t, func() { + _ = errors.Is(classified, connErr) + }, "errors.Is with a typed-nil *url.Error target must not panic") +}