Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/new-pigs-live.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'grafana-github-datasource': patch
---

Fixed a panic when GitHub REST API returns error responses (e.g. 404, 401, 403)
22 changes: 22 additions & 0 deletions pkg/github/client/errorsourcehandling.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package githubclient

import (
"errors"
"fmt"
"regexp"
"strconv"
"strings"
Expand All @@ -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)
}
Expand Down
93 changes: 93 additions & 0 deletions pkg/github/client/errorsourcehandling_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package githubclient
import (
"context"
"errors"
"fmt"
"net"
"net/http"
"net/url"
"os"
"syscall"
"testing"
Expand Down Expand Up @@ -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
Expand Down
Loading