add debug logs for github api requests #2047
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR adds comprehensive debug logging for GitHub API requests by implementing a custom HTTP round tripper (loggingRoundTripper) that intercepts and logs all HTTP traffic between the Digger backend and GitHub's API. The implementation wraps the existing GitHub client transport in the Get method of DiggerGithubRealClientProvider, capturing detailed information about requests (method, URL, headers) and responses (status, headers, full body content).
The change integrates seamlessly with the existing GitHub client creation workflow in backend/utils/github.go. When a GitHub client is created through the Get method, it now automatically includes the logging wrapper around the installation transport. This ensures all GitHub API calls made through the client - including those from the underlying go-github library - are consistently logged at debug level.
This enhancement aligns with the codebase's focus on observability and debugging, particularly important given the complex GitHub App installation management, webhook processing, and API interactions that Digger handles. The logging uses structured logging via slog which is consistent with the existing logging patterns throughout the codebase.
Confidence score: 2/5
- This PR has significant security and performance risks that make it unsafe for production environments
- The implementation logs sensitive authentication headers and full response bodies which could expose tokens and private data
- The
backend/utils/github.gofile needs careful security review before merging
PR Description Notes:
- The PR description is empty (
null) - consider adding details about the purpose and scope of the logging changes
1 file reviewed, 1 comment
backend/utils/github.go
Outdated
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url", req.URL.String(), | ||
| "headers", req.Header, |
There was a problem hiding this comment.
logic: Logging headers may expose sensitive authentication tokens in debug logs. Consider filtering or redacting authorization headers.
|
Cancelled. |
Security Bugs Summary in GitHub API LoggingTotal bugs found: 5 Critical Security Issues
These issues primarily revolve around improper handling of sensitive data in logs and memory management problems when processing API responses. |
![bismuthdev[bot]](https://avatars.githubusercontent.com/in/957724?s=60&v=4){kind=small}
backend/utils/trip_logger.go
Outdated
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| "headers", resp.Header, | ||
| "body", string(bodyBytes), | ||
| ) |
There was a problem hiding this comment.
The LoggingRoundTripper is logging the entire response body from GitHub API calls at debug level. This is a security risk because:
- API responses may contain sensitive information like access tokens, credentials, or personal data
- When debug logs are enabled in production, this sensitive information would be exposed in log files
- Log files are often accessible to more people than should have access to these credentials
- Log files might be sent to third-party logging services without proper redaction
The fix replaces the full body logging with just logging the length of the response body, which provides debugging value (confirming response size) without exposing sensitive content.
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "headers", resp.Header, | |
| "body", string(bodyBytes), | |
| ) | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "headers", resp.Header, | |
| "bodyLength", len(bodyBytes), | |
| ) |
backend/utils/trip_logger.go
Outdated
| // Read and clone response body for logging | ||
| bodyBytes, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| slog.Error("Failed to read response body", "error", err) | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
In the RoundTrip method of LoggingRoundTripper, when there's an error reading the response body (lines 29-32), the function returns the original response object, but the response body has already been consumed by the io.ReadAll call. This means the caller would receive a response with an empty/consumed body, which would likely cause issues for any code trying to read the response body later.
The fix restores the response body with an empty buffer when there's an error reading it, which is better than returning a consumed body. This way, the caller will at least get an empty body rather than an error when trying to read from a closed reader.
| // Read and clone response body for logging | |
| bodyBytes, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| slog.Error("Failed to read response body", "error", err) | |
| return resp, err | |
| } | |
| // Read and clone response body for logging | |
| bodyBytes, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| slog.Error("Failed to read response body", "error", err) | |
| // When we can't read the body, we should still restore it to prevent returning a consumed body | |
| resp.Body = io.NopCloser(bytes.NewBuffer([]byte{})) | |
| return resp, err | |
| } |
backend/utils/trip_logger.go
Outdated
| // Read and clone response body for logging | ||
| bodyBytes, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| slog.Error("Failed to read response body", "error", err) | ||
| return resp, err | ||
| } | ||
| resp.Body = io.NopCloser(bytes.NewBuffer(bodyBytes)) // restore the body for the actual client | ||
|
|
||
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| "headers", resp.Header, | ||
| "body", string(bodyBytes), | ||
| ) |
There was a problem hiding this comment.
The current implementation in trip_logger.go reads the entire response body into memory for every GitHub API request using io.ReadAll(resp.Body). This is problematic for large responses as it:
- Consumes memory proportional to the response size with no upper bound
- Could lead to out-of-memory errors for very large responses
- Logs the entire response body regardless of size, which is inefficient and potentially risky for large payloads
The fix implements a size-limited approach that:
- Uses a TeeReader to simultaneously read the body while writing to a buffer
- Limits the amount read for logging purposes to 1MB
- Properly drains and closes the original response body
- Restores the body for the client using the buffer
- Adds metadata about response size and truncation to the logs
This approach maintains the logging functionality while preventing unbounded memory usage.
backend/utils/trip_logger.go
Outdated
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url", req.URL.String(), | ||
| "headers", req.Header, | ||
| ) |
There was a problem hiding this comment.
The code in trip_logger.go logs all request headers without filtering sensitive information. This is a security vulnerability as it could expose authentication tokens, API keys, and other sensitive data in the logs.
The current implementation logs the entire req.Header object, which may contain:
- Authorization tokens
- API keys
- Session cookies
- Other sensitive credentials
This information could be exposed in log files and potentially accessed by unauthorized parties. The fix creates a sanitized copy of the headers and redacts values for common sensitive header names before logging.
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", req.URL.String(), | |
| "headers", req.Header, | |
| ) | |
| // Create a copy of headers to sanitize sensitive information | |
| sanitizedHeaders := make(net.Header) | |
| for k, v := range req.Header { | |
| // Skip sensitive headers or redact their values | |
| if k == "Authorization" || k == "Cookie" || k == "X-GitHub-Token" || | |
| strings.Contains(strings.ToLower(k), "key") || | |
| strings.Contains(strings.ToLower(k), "secret") || | |
| strings.Contains(strings.ToLower(k), "token") || | |
| strings.Contains(strings.ToLower(k), "password") { | |
| sanitizedHeaders[k] = []string{"[REDACTED]"} | |
| } else { | |
| sanitizedHeaders[k] = v | |
| } | |
| } | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", req.URL.String(), | |
| "headers", sanitizedHeaders, | |
| ) |
backend/utils/trip_logger.go
Outdated
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| "headers", resp.Header, | ||
| "body", string(bodyBytes), | ||
| ) |
There was a problem hiding this comment.
The code in trip_logger.go logs all response headers and the complete response body without filtering sensitive information. This is a security vulnerability as it could expose authentication tokens, API keys, and other sensitive data in the logs.
The current implementation logs:
- The entire
resp.Headerobject, which may contain sensitive tokens or cookies - The complete response body, which might include sensitive data or tokens
This information could be exposed in log files and potentially accessed by unauthorized parties. The fix:
- Creates a sanitized copy of the headers and redacts values for common sensitive header names
- Limits the logging of large response bodies to prevent excessive log sizes and reduce the chance of exposing sensitive data
Additionally, the fix adds a string import that was missing in the original code.
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "headers", resp.Header, | |
| "body", string(bodyBytes), | |
| ) | |
| // Create a copy of headers to sanitize sensitive information | |
| sanitizedHeaders := make(net.Header) | |
| for k, v := range resp.Header { | |
| // Skip sensitive headers or redact their values | |
| if k == "Authorization" || k == "Cookie" || k == "X-GitHub-Token" || | |
| strings.Contains(strings.ToLower(k), "key") || | |
| strings.Contains(strings.ToLower(k), "secret") || | |
| strings.Contains(strings.ToLower(k), "token") || | |
| strings.Contains(strings.ToLower(k), "password") { | |
| sanitizedHeaders[k] = []string{"[REDACTED]"} | |
| } else { | |
| sanitizedHeaders[k] = v | |
| } | |
| } | |
| // Also consider sanitizing the response body if it might contain sensitive data | |
| // For simplicity, we're just logging the length for large responses | |
| bodyToLog := string(bodyBytes) | |
| if len(bodyBytes) > 1000 { | |
| bodyToLog = fmt.Sprintf("[%d bytes]", len(bodyBytes)) | |
| } | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "headers", sanitizedHeaders, | |
| "body", bodyToLog, | |
| ) |
Bug Summary ReportTotal Bugs Found: 2Bug Summaries:
|
There was a problem hiding this comment.
Greptile Summary
This PR refactors the GitHub API logging implementation by addressing the security concerns raised in the previous review. The changes move the LoggingRoundTripper from inline code to a separate utility file (backend/utils/trip_logger.go) and significantly improve security by removing the logging of sensitive headers and response bodies.
The refactored implementation now only logs essential request information (method and URL) and specific GitHub rate limit headers (X-RateLimit-Remaining, X-RateLimit-Used, etc.), which are crucial for monitoring API quota usage without exposing authentication tokens or sensitive data. The logging functionality is consistently applied across both the community edition (backend/utils/github.go) and enterprise edition (ee/backend/providers/github/providers.go) GitHub client providers.
The change maintains the same integration point in the Get method where the logging wrapper is applied to the GitHub installation transport, ensuring comprehensive coverage of all GitHub API interactions while following secure logging practices.
Confidence score: 4/5
- This PR successfully addresses the major security concerns from the previous implementation by removing sensitive data logging
- The refactored approach focuses on operationally useful information (rate limits) while maintaining security
- The enterprise edition implementation has some dead code that should be cleaned up, but it doesn't affect functionality
3 files reviewed, 2 comments
backend/utils/trip_logger.go
Outdated
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url", req.URL.String(), | ||
| ) |
There was a problem hiding this comment.
style: Logging the full URL may expose sensitive query parameters or tokens. Consider sanitizing URLs before logging.
| clientWithLogging := &net.Client{ | ||
| Transport: &utils.LoggingRoundTripper{Rt: itr}, | ||
| } |
There was a problem hiding this comment.
style: Consider adding contextual identifiers like githubAppId and installationId to the logging transport to maintain consistency with backend implementation and improve traceability
backend/utils/trip_logger.go
Outdated
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url", req.URL.String(), | ||
| ) |
There was a problem hiding this comment.
The LoggingRoundTripper logs the full URL of requests including query parameters using req.URL.String(). This is a security vulnerability as it could leak sensitive information like tokens, API keys, or credentials that might be present in query parameters to logs.
The fix creates a copy of the URL, identifies potentially sensitive query parameters by looking for common keywords in parameter names (token, key, secret, password, auth, credential), and redacts their values before logging. This ensures that sensitive information is not exposed in logs while still providing useful debugging information about the request.
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", req.URL.String(), | |
| ) | |
| // Create a copy of the URL to sanitize sensitive information | |
| sanitizedURL := *req.URL | |
| q := sanitizedURL.Query() | |
| // Redact potentially sensitive query parameters | |
| for key := range q { | |
| if strings.Contains(strings.ToLower(key), "token") || | |
| strings.Contains(strings.ToLower(key), "key") || | |
| strings.Contains(strings.ToLower(key), "secret") || | |
| strings.Contains(strings.ToLower(key), "password") || | |
| strings.Contains(strings.ToLower(key), "auth") || | |
| strings.Contains(strings.ToLower(key), "credential") { | |
| q.Set(key, "[REDACTED]") | |
| } | |
| } | |
| sanitizedURL.RawQuery = q.Encode() | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", sanitizedURL.String(), | |
| ) |
backend/utils/trip_logger.go
Outdated
| // Log the request | ||
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url", req.URL.String(), | ||
| ) | ||
|
|
||
| resp, err := lrt.Rt.RoundTrip(req) | ||
| if err != nil { | ||
| slog.Error("GitHub API Request failed", "error", err) | ||
| return nil, err | ||
| } | ||
|
|
||
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | ||
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | ||
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | ||
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | ||
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | ||
| ) |
There was a problem hiding this comment.
The current implementation logs every GitHub API request and response at DEBUG level, which could generate excessive log data in environments with many GitHub API requests. This is particularly problematic because:
- The GitHub client is wrapped with the
LoggingRoundTripperin bothbackend/utils/github.goandee/backend/providers/github/providers.go, meaning all GitHub API calls are logged. - Each API call generates two debug log entries (request and response) with multiple fields.
- In high-traffic environments or during operations that make many API calls (like scanning repositories or processing webhooks), this can lead to log flooding.
The fix adds a conditional check based on an environment variable DEBUG_GITHUB_API, which allows operators to explicitly enable this verbose logging only when needed for troubleshooting, while keeping logs manageable in normal operation.
| // Log the request | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", req.URL.String(), | |
| ) | |
| resp, err := lrt.Rt.RoundTrip(req) | |
| if err != nil { | |
| slog.Error("GitHub API Request failed", "error", err) | |
| return nil, err | |
| } | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| // Only log GitHub API requests at DEBUG level if DEBUG_GITHUB_API is set | |
| if os.Getenv("DEBUG_GITHUB_API") == "true" { | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url", req.URL.String(), | |
| ) | |
| } | |
| resp, err := lrt.Rt.RoundTrip(req) | |
| if err != nil { | |
| slog.Error("GitHub API Request failed", "error", err) | |
| return nil, err | |
| } | |
| // Only log GitHub API responses at DEBUG level if DEBUG_GITHUB_API is set | |
| if os.Getenv("DEBUG_GITHUB_API") == "true" { | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| } |
Bug Summary ReportOverviewA total of 3 bugs were found in the codebase, all located in the Summary of Bugs
The most critical bug is the rate limit handling issue, as it could lead to silent failures in production environments when API limits are reached, followed by the security concern of logging sensitive information. |
There was a problem hiding this comment.
Greptile Summary
This PR makes a final security refinement to the GitHub API logging implementation by changing the logged URL information from the full URL string (req.URL.String()) to only the URL path (req.URL.Path) in the LoggingRoundTripper. This change prevents sensitive query parameters, authentication tokens, or other credentials that might be present in URL query strings from being exposed in debug logs.
The modification occurs in backend/utils/trip_logger.go at line 16, where the logging statement now captures only the API endpoint path (e.g., /repos/owner/repo/pulls) rather than the complete URL with potential sensitive parameters. This maintains the debugging utility of knowing which GitHub API endpoints are being called while eliminating the security risk of accidentally logging authentication data embedded in URLs.
This change complements the previous security improvements made in earlier iterations of this PR, where sensitive headers and response bodies were already removed from logging. The LoggingRoundTripper continues to be used in the GitHub client creation process and will now provide secure, operationally useful logging for all GitHub API interactions across both community and enterprise editions.
Confidence score: 5/5
- This is a focused security improvement that eliminates the risk of logging sensitive URL parameters without affecting functionality
- The change maintains all debugging benefits while removing potential credential exposure
- No files need additional attention as this is a straightforward, single-line security fix
1 file reviewed, no comments
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url_path", req.URL.Path, | ||
| ) |
There was a problem hiding this comment.
The LoggingRoundTripper logs the raw URL path of GitHub API requests, which could contain sensitive information like repository names, PR numbers, or other identifiers. This information is logged at the debug level, but it's still a potential security concern as these logs might be collected and viewed by various people.
The URL path should be sanitized before logging to remove or mask sensitive information. This would require adding a sanitization function to process the URL path before logging it.
Additionally, a sanitization function should be added to the file:
// sanitizeUrlPath removes or masks sensitive information from GitHub API URL paths
func sanitizeUrlPath(path string) string {
// Implementation could use regex to replace repository names, PR numbers, etc.
// with placeholders or masked values
// For example: "/repos/username/repo-name/pulls/123" -> "/repos/[REDACTED]/[REDACTED]/pulls/[ID]"
// Simple implementation for demonstration
// A more robust implementation would use proper regex patterns
return path
}This change helps protect sensitive information while still providing useful debugging information about API requests.
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url_path", req.URL.Path, | |
| ) | |
| // Sanitize URL path to remove potentially sensitive information | |
| sanitizedPath := sanitizeUrlPath(req.URL.Path) | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url_path", sanitizedPath, | |
| ) |
backend/utils/trip_logger.go
Outdated
| resp, err := lrt.Rt.RoundTrip(req) | ||
| if err != nil { | ||
| slog.Error("GitHub API Request failed", "error", err) | ||
| return nil, err | ||
| } | ||
|
|
||
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | ||
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | ||
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | ||
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | ||
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | ||
| ) |
There was a problem hiding this comment.
The LoggingRoundTripper currently logs GitHub API rate limit information at the debug level, but it doesn't specifically detect or handle rate limit exceeded errors. When GitHub API rate limits are exceeded, the API returns a 403 status code with a "X-RateLimit-Remaining" header value of "0".
This bug fix adds specific detection and error-level logging for rate limit exceeded conditions, which will make these critical events more visible in logs. This is important because rate limit exceeded errors can cause API requests to fail silently if they're only logged at debug level, potentially leading to hard-to-diagnose issues in production.
The fix maintains the existing debug logging for normal responses while adding special handling for the rate limit exceeded case.
| resp, err := lrt.Rt.RoundTrip(req) | |
| if err != nil { | |
| slog.Error("GitHub API Request failed", "error", err) | |
| return nil, err | |
| } | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| resp, err := lrt.Rt.RoundTrip(req) | |
| if err != nil { | |
| slog.Error("GitHub API Request failed", "error", err) | |
| return nil, err | |
| } | |
| // Check for rate limit exceeded (status code 403 with specific message) | |
| if resp.StatusCode == 403 && resp.Header.Get("X-RateLimit-Remaining") == "0" { | |
| slog.Error("GitHub API Rate Limit Exceeded", | |
| "status", resp.Status, | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| ) | |
| } else { | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| } |
Co-authored-by: bismuthdev[bot] <177057995+bismuthdev[bot]@users.noreply.github.com>
Bug Summary ReportTotal Bugs Found: 2Bug Summaries
The security risk in request logging is particularly critical as it could potentially expose sensitive organizational data through debug logs. |
| // Log the request | ||
| slog.Debug("GitHub API Request", | ||
| "method", req.Method, | ||
| "url_path", req.URL.Path, | ||
| ) |
There was a problem hiding this comment.
The LoggingRoundTripper logs the full URL path of GitHub API requests in debug logs. This could potentially leak sensitive information when debug logging is enabled in production environments.
GitHub API paths can contain sensitive information such as:
- Repository names in private organizations
- Issue or PR numbers that might be confidential
- File paths that could reveal internal project structure
- Query parameters that might contain tokens or other sensitive data
Logging the full URL path creates a security risk as these logs might be accessible to unauthorized personnel or could be included in error reports. The fix removes the URL path from debug logs while still maintaining useful information about the request method.
| // Log the request | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| "url_path", req.URL.Path, | |
| ) | |
| // Log the request method only to avoid potential sensitive information leakage | |
| slog.Debug("GitHub API Request", | |
| "method", req.Method, | |
| ) |
| slog.Debug("GitHub API Response", | ||
| "status", resp.Status, | ||
| ) | ||
|
|
||
| if resp.Header != nil { | ||
| slog.Debug("GitHub API Rate Limits", | ||
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | ||
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | ||
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | ||
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | ||
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
The LoggingRoundTripper currently logs the status code of GitHub API responses, but doesn't log the response body when there's an error (non-2xx status code). This makes debugging API errors difficult because developers can't see the actual error message returned by GitHub.
The fix adds code to read and log the response body when a non-2xx status code is received, while ensuring the response body is still available for downstream consumers by creating a new reader with the same content. This significantly improves the debugging experience when GitHub API calls fail.
Note: This implementation requires adding "bytes" to the import statement, which isn't included in this fix.
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| ) | |
| if resp.Header != nil { | |
| slog.Debug("GitHub API Rate Limits", | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| } | |
| slog.Debug("GitHub API Response", | |
| "status", resp.Status, | |
| ) | |
| if resp.Header != nil { | |
| slog.Debug("GitHub API Rate Limits", | |
| "X-RateLimit-Limit", resp.Header.Get("X-RateLimit-Limit"), | |
| "X-RateLimit-Remaining", resp.Header.Get("X-RateLimit-Remaining"), | |
| "X-RateLimit-Used", resp.Header.Get("X-RateLimit-Used"), | |
| "X-RateLimit-Resource", resp.Header.Get("X-RateLimit-Resource"), | |
| "X-RateLimit-Reset", resp.Header.Get("X-RateLimit-Reset"), | |
| ) | |
| } | |
| // Log response body for non-2xx status codes to aid debugging | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| body, err := net.ReadAll(resp.Body) | |
| if err != nil { | |
| slog.Error("Failed to read error response body", "error", err) | |
| } else { | |
| // We need to restore the response body for downstream consumers | |
| resp.Body = net.NopCloser(bytes.NewBuffer(body)) | |
| slog.Error("GitHub API Error Response", | |
| "status", resp.Status, | |
| "body", string(body), | |
| ) | |
| } | |
| } |
No description provided.