diff --git a/.changeset/new-pigs-live.md b/.changeset/new-pigs-live.md new file mode 100644 index 00000000..10334f68 --- /dev/null +++ b/.changeset/new-pigs-live.md @@ -0,0 +1,5 @@ +--- +'grafana-github-datasource': patch +--- + +Fixed a panic when GitHub REST API returns error responses (e.g. 404, 401, 403) diff --git a/pkg/github/client/errorsourcehandling.go b/pkg/github/client/errorsourcehandling.go index f7aefed1..c50d655b 100644 --- a/pkg/github/client/errorsourcehandling.go +++ b/pkg/github/client/errorsourcehandling.go @@ -2,6 +2,7 @@ package githubclient import ( "errors" + "fmt" "regexp" "strconv" "strings" @@ -25,12 +26,33 @@ var ( } ) +// sanitizeGitHubError converts a *github.ErrorResponse into a plain error to +// prevent a nil pointer dereference panic in the SDK's error handling. +// +// go-github v76 (https://github.com/google/go-github/pull/3739) changed ErrorResponse.Is to call errors.As(target, &v) +// on the target, which panics when the SDK's guessErrorStatus passes typed nil targets +// (e.g. (*url.Error)(nil)) to errors.Is. Converting to a plain error preserves +// the message while removing the problematic Is method from the error chain. +// +// See TestGitHubErrorResponseWithTypedNilErrorsIs for a reproduction of the panic. +func sanitizeGitHubError(err error) error { + var ghErr *googlegithub.ErrorResponse + if errors.As(err, &ghErr) { + return fmt.Errorf("%s", err.Error()) + } + return err +} + func addErrorSourceToError(err error, resp *googlegithub.Response) error { // If there is no error then return nil if err == nil { return nil } + // Sanitize *github.ErrorResponse before any further processing or wrapping. + // This prevents a panic in the SDK's error handling pipeline. + err = sanitizeGitHubError(err) + if backend.IsDownstreamHTTPError(err) { return backend.DownstreamError(err) } diff --git a/pkg/github/client/errorsourcehandling_test.go b/pkg/github/client/errorsourcehandling_test.go index 9a6071e1..6f2c1239 100644 --- a/pkg/github/client/errorsourcehandling_test.go +++ b/pkg/github/client/errorsourcehandling_test.go @@ -3,8 +3,10 @@ package githubclient import ( "context" "errors" + "fmt" "net" "net/http" + "net/url" "os" "syscall" "testing" @@ -103,6 +105,97 @@ func TestAddErrorSourceToError(t *testing.T) { } } +// TestAddErrorSourceToError_SanitizesGitHubErrorResponse verifies that addErrorSourceToError +// strips *github.ErrorResponse from the error chain before returning. +// +// Background: go-github v76 changed ErrorResponse.Is to call errors.As(target, &v) on the +// target error. https://github.com/google/go-github/pull/3739 +// The SDK's guessErrorStatus (backend/status.go:112) passes typed nil targets +// (e.g. (*url.Error)(nil)) to errors.Is. When errors.Is walks the chain and reaches a +// *github.ErrorResponse, ErrorResponse.Is calls errors.As on that typed nil, which tries +// to call Unwrap() on the nil receiver and panics with a nil pointer dereference. +// +// The fix: sanitizeGitHubError converts *github.ErrorResponse to a plain error before it +// can reach the SDK. This test ensures that guarantee holds for all REST API error scenarios. +// +// Real-world trigger: any query type that uses the GitHub REST API (Workflows, Code Scanning, +// etc.) against a repo that returns a non-2xx response (e.g. 404 for a non-existent repo, +// 401 for an expired token, 403 for insufficient permissions). +func TestAddErrorSourceToError_SanitizesGitHubErrorResponse(t *testing.T) { + gitHubErrorResponses := []struct { + name string + statusCode int + message string + }{ + {name: "404 Not Found (non-existent repo)", statusCode: http.StatusNotFound, message: "Not Found"}, + {name: "401 Unauthorized (expired token)", statusCode: http.StatusUnauthorized, message: "Bad credentials"}, + {name: "403 Forbidden (insufficient scopes)", statusCode: http.StatusForbidden, message: "Resource not accessible by integration"}, + {name: "500 Internal Server Error", statusCode: http.StatusInternalServerError, message: "Internal Server Error"}, + } + + for _, tc := range gitHubErrorResponses { + t.Run(tc.name, func(t *testing.T) { + ghErr := &googlegithub.ErrorResponse{ + Response: &http.Response{StatusCode: tc.statusCode}, + Message: tc.message, + } + resp := &googlegithub.Response{Response: &http.Response{StatusCode: tc.statusCode}} + + result := addErrorSourceToError(ghErr, resp) + + // Guard: *github.ErrorResponse must NOT be present in the returned error chain. + // If this fails, a future dependency bump or code change has broken the sanitization + // and the SDK will panic when processing this error. + var leaked *googlegithub.ErrorResponse + require.False(t, errors.As(result, &leaked), + "addErrorSourceToError must not leak *github.ErrorResponse into the error chain; "+ + "the SDK's guessErrorStatus will panic due to go-github v81's ErrorResponse.Is") + + // Verify the error message is preserved so user-facing messages are not lost. + require.Contains(t, result.Error(), tc.message, + "the sanitized error should preserve the original error message") + + // Verify the SDK's guessErrorStatus code path does not panic. + // This replicates the exact calls the SDK makes at backend/status.go:108-112: + // var connErr *url.Error // typed nil + // var netErr *net.OpError // typed nil + // errors.Is(err, connErr) || errors.Is(err, netErr) + var connErr *url.Error + var netErr *net.OpError + require.NotPanics(t, func() { + errors.Is(result, connErr) + errors.Is(result, netErr) + }, "errors.Is with typed nil targets must not panic after sanitization") + }) + } +} + +// TestSanitizeGitHubError_WrappedError verifies that sanitizeGitHubError also handles +// *github.ErrorResponse that is wrapped inside another error (e.g. via fmt.Errorf("%w", err)), +// which matches how some client methods pass errors to addErrorSourceToError. +func TestSanitizeGitHubError_WrappedError(t *testing.T) { + ghErr := &googlegithub.ErrorResponse{ + Response: &http.Response{StatusCode: http.StatusNotFound}, + Message: "Not Found", + } + wrapped := fmt.Errorf("fetching workflow usage: %w", ghErr) + + result := sanitizeGitHubError(wrapped) + + var leaked *googlegithub.ErrorResponse + require.False(t, errors.As(result, &leaked), + "sanitizeGitHubError must strip *github.ErrorResponse even when wrapped") + require.Contains(t, result.Error(), "Not Found") +} + +// TestSanitizeGitHubError_NonGitHubError verifies that sanitizeGitHubError is a no-op +// for errors that are not *github.ErrorResponse. +func TestSanitizeGitHubError_NonGitHubError(t *testing.T) { + original := errors.New("some other error") + result := sanitizeGitHubError(original) + require.Equal(t, original, result, "non-github errors should pass through unchanged") +} + func TestExtractStatusCode(t *testing.T) { tests := []struct { name string