Skip to content

Commit afcc6d6

Browse files
authored
Sanitize go-github ErrorResponse to prevent panic in SDK error handling (#629)
## Summary Fixes a nil pointer dereference panic caused by the interaction between go-github v81's `ErrorResponse.Is` and the SDK's `guessErrorStatus` function. go-github v76 (bumped from v72 to v81 in #574) changed `ErrorResponse.Is` from a simple type assertion to calling `errors.As(target, &v)` on the target. See PR: google/go-github#3739 The SDK's `guessErrorStatus` (`backend/status.go:112`) passes typed nil targets (`(*url.Error)(nil)`) to `errors.Is`. When `errors.Is` walks the error chain and reaches `*github.ErrorResponse`, the v81 `ErrorResponse.Is` calls `errors.As` on that typed nil, which tries to call `Unwrap()` on the nil receiver — causing a nil pointer dereference panic. The fix adds `sanitizeGitHubError` which converts `*github.ErrorResponse` to a plain error at the `addErrorSourceToError` boundary, preserving the error message while stripping the problematic `Is` method from the error chain. ## Manual reproduction steps 1. Run Grafana with the github-datasource plugin (v2.5.0) 2. Configure the GitHub datasource with a valid personal access token 3. Open Explore (or create a panel) and select the GitHub datasource 4. Set query type to **Workflows** (or **Code Scanning**) 5. Set **Owner** to `grafana`, **Repository** to `this-repo-does-not-exist` 6. Run the query 7. Observe the panic in Grafana server logs: `panic triggered error="runtime error: invalid memory address or nil pointer dereference"` With this fix applied, the query returns a proper error response instead of panicking. Fixes grafana/oss-big-tent-squad#183
1 parent 1193b6a commit afcc6d6

3 files changed

Lines changed: 120 additions & 0 deletions

File tree

.changeset/new-pigs-live.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'grafana-github-datasource': patch
3+
---
4+
5+
Fixed a panic when GitHub REST API returns error responses (e.g. 404, 401, 403)

pkg/github/client/errorsourcehandling.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package githubclient
22

33
import (
44
"errors"
5+
"fmt"
56
"regexp"
67
"strconv"
78
"strings"
@@ -25,12 +26,33 @@ var (
2526
}
2627
)
2728

29+
// sanitizeGitHubError converts a *github.ErrorResponse into a plain error to
30+
// prevent a nil pointer dereference panic in the SDK's error handling.
31+
//
32+
// go-github v76 (https://github.com/google/go-github/pull/3739) changed ErrorResponse.Is to call errors.As(target, &v)
33+
// on the target, which panics when the SDK's guessErrorStatus passes typed nil targets
34+
// (e.g. (*url.Error)(nil)) to errors.Is. Converting to a plain error preserves
35+
// the message while removing the problematic Is method from the error chain.
36+
//
37+
// See TestGitHubErrorResponseWithTypedNilErrorsIs for a reproduction of the panic.
38+
func sanitizeGitHubError(err error) error {
39+
var ghErr *googlegithub.ErrorResponse
40+
if errors.As(err, &ghErr) {
41+
return fmt.Errorf("%s", err.Error())
42+
}
43+
return err
44+
}
45+
2846
func addErrorSourceToError(err error, resp *googlegithub.Response) error {
2947
// If there is no error then return nil
3048
if err == nil {
3149
return nil
3250
}
3351

52+
// Sanitize *github.ErrorResponse before any further processing or wrapping.
53+
// This prevents a panic in the SDK's error handling pipeline.
54+
err = sanitizeGitHubError(err)
55+
3456
if backend.IsDownstreamHTTPError(err) {
3557
return backend.DownstreamError(err)
3658
}

pkg/github/client/errorsourcehandling_test.go

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@ package githubclient
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"net"
78
"net/http"
9+
"net/url"
810
"os"
911
"syscall"
1012
"testing"
@@ -103,6 +105,97 @@ func TestAddErrorSourceToError(t *testing.T) {
103105
}
104106
}
105107

108+
// TestAddErrorSourceToError_SanitizesGitHubErrorResponse verifies that addErrorSourceToError
109+
// strips *github.ErrorResponse from the error chain before returning.
110+
//
111+
// Background: go-github v76 changed ErrorResponse.Is to call errors.As(target, &v) on the
112+
// target error. https://github.com/google/go-github/pull/3739
113+
// The SDK's guessErrorStatus (backend/status.go:112) passes typed nil targets
114+
// (e.g. (*url.Error)(nil)) to errors.Is. When errors.Is walks the chain and reaches a
115+
// *github.ErrorResponse, ErrorResponse.Is calls errors.As on that typed nil, which tries
116+
// to call Unwrap() on the nil receiver and panics with a nil pointer dereference.
117+
//
118+
// The fix: sanitizeGitHubError converts *github.ErrorResponse to a plain error before it
119+
// can reach the SDK. This test ensures that guarantee holds for all REST API error scenarios.
120+
//
121+
// Real-world trigger: any query type that uses the GitHub REST API (Workflows, Code Scanning,
122+
// etc.) against a repo that returns a non-2xx response (e.g. 404 for a non-existent repo,
123+
// 401 for an expired token, 403 for insufficient permissions).
124+
func TestAddErrorSourceToError_SanitizesGitHubErrorResponse(t *testing.T) {
125+
gitHubErrorResponses := []struct {
126+
name string
127+
statusCode int
128+
message string
129+
}{
130+
{name: "404 Not Found (non-existent repo)", statusCode: http.StatusNotFound, message: "Not Found"},
131+
{name: "401 Unauthorized (expired token)", statusCode: http.StatusUnauthorized, message: "Bad credentials"},
132+
{name: "403 Forbidden (insufficient scopes)", statusCode: http.StatusForbidden, message: "Resource not accessible by integration"},
133+
{name: "500 Internal Server Error", statusCode: http.StatusInternalServerError, message: "Internal Server Error"},
134+
}
135+
136+
for _, tc := range gitHubErrorResponses {
137+
t.Run(tc.name, func(t *testing.T) {
138+
ghErr := &googlegithub.ErrorResponse{
139+
Response: &http.Response{StatusCode: tc.statusCode},
140+
Message: tc.message,
141+
}
142+
resp := &googlegithub.Response{Response: &http.Response{StatusCode: tc.statusCode}}
143+
144+
result := addErrorSourceToError(ghErr, resp)
145+
146+
// Guard: *github.ErrorResponse must NOT be present in the returned error chain.
147+
// If this fails, a future dependency bump or code change has broken the sanitization
148+
// and the SDK will panic when processing this error.
149+
var leaked *googlegithub.ErrorResponse
150+
require.False(t, errors.As(result, &leaked),
151+
"addErrorSourceToError must not leak *github.ErrorResponse into the error chain; "+
152+
"the SDK's guessErrorStatus will panic due to go-github v81's ErrorResponse.Is")
153+
154+
// Verify the error message is preserved so user-facing messages are not lost.
155+
require.Contains(t, result.Error(), tc.message,
156+
"the sanitized error should preserve the original error message")
157+
158+
// Verify the SDK's guessErrorStatus code path does not panic.
159+
// This replicates the exact calls the SDK makes at backend/status.go:108-112:
160+
// var connErr *url.Error // typed nil
161+
// var netErr *net.OpError // typed nil
162+
// errors.Is(err, connErr) || errors.Is(err, netErr)
163+
var connErr *url.Error
164+
var netErr *net.OpError
165+
require.NotPanics(t, func() {
166+
errors.Is(result, connErr)
167+
errors.Is(result, netErr)
168+
}, "errors.Is with typed nil targets must not panic after sanitization")
169+
})
170+
}
171+
}
172+
173+
// TestSanitizeGitHubError_WrappedError verifies that sanitizeGitHubError also handles
174+
// *github.ErrorResponse that is wrapped inside another error (e.g. via fmt.Errorf("%w", err)),
175+
// which matches how some client methods pass errors to addErrorSourceToError.
176+
func TestSanitizeGitHubError_WrappedError(t *testing.T) {
177+
ghErr := &googlegithub.ErrorResponse{
178+
Response: &http.Response{StatusCode: http.StatusNotFound},
179+
Message: "Not Found",
180+
}
181+
wrapped := fmt.Errorf("fetching workflow usage: %w", ghErr)
182+
183+
result := sanitizeGitHubError(wrapped)
184+
185+
var leaked *googlegithub.ErrorResponse
186+
require.False(t, errors.As(result, &leaked),
187+
"sanitizeGitHubError must strip *github.ErrorResponse even when wrapped")
188+
require.Contains(t, result.Error(), "Not Found")
189+
}
190+
191+
// TestSanitizeGitHubError_NonGitHubError verifies that sanitizeGitHubError is a no-op
192+
// for errors that are not *github.ErrorResponse.
193+
func TestSanitizeGitHubError_NonGitHubError(t *testing.T) {
194+
original := errors.New("some other error")
195+
result := sanitizeGitHubError(original)
196+
require.Equal(t, original, result, "non-github errors should pass through unchanged")
197+
}
198+
106199
func TestExtractStatusCode(t *testing.T) {
107200
tests := []struct {
108201
name string

0 commit comments

Comments
 (0)