feat: add optional logging for HTTP requests and responses #191
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: narthur <[email protected]>
Co-authored-by: narthur <[email protected]>
Co-authored-by: narthur <[email protected]>
Co-authored-by: narthur <[email protected]>
🚀 Pre-release Build ReadyTest builds are ready! Install directly using the # Install the pre-release
bin install https://github.com/PinePeakDigital/buzz/releases/tag/pr-191-latest buzz-pr-191# Run the pre-release
buzz-pr-191# Uninstall the pre-release
bin remove buzz-pr-191Direct Download LinksOr download binaries directly from the pre-release page:
|
There was a problem hiding this comment.
Pull request overview
This PR adds optional HTTP request/response logging functionality to help users debug API interactions with Beeminder. The feature is opt-in and disabled by default, requiring users to explicitly configure a log file path in their ~/.buzzrc configuration.
Key changes:
- Added
log_fileconfiguration field to enable optional logging - Implemented logging functions that write timestamped HTTP request/response details
- Integrated logging calls into all 9 HTTP operations in the Beeminder API client
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| config.go | Added LogFile field to Config struct and implemented logToFile(), LogRequest(), and LogResponse() functions for opt-in HTTP logging |
| beeminder.go | Integrated logging calls before/after all HTTP operations (FetchGoals, CreateDatapoint, CreateCharge, FetchGoal, FetchGoalWithDatapoints, FetchGoalRawJSON, CreateGoal, RefreshGoal, GetLastDatapointValue) |
| config_test.go | Added comprehensive tests for logging functionality covering enabled/disabled states, file operations, and JSON marshaling of log_file field |
| README.md | Added documentation section explaining the optional logging feature, configuration instructions, and example log output format |
Comments suppressed due to low confidence (1)
README.md:90
- Documentation inconsistency: The example shows auth tokens as
auth_token=..., implying they are redacted in logs. However, the actual implementation logs complete URLs including full auth tokens in plain text.
This documentation should either:
- Accurately show that full tokens are logged (if that's the intended behavior), OR
- Be updated after implementing URL sanitization to redact sensitive tokens (recommended)
Given the security implications, consider updating the documentation to warn users that sensitive credentials will be stored in log files:
**Security Warning**: Log files will contain your Beeminder auth token in plain text. Ensure log files are stored securely with appropriate file permissions and are not shared or committed to version control.
[2025-12-09 12:34:56] REQUEST: GET https://www.beeminder.com/api/v1/users/alice/goals.json?auth_token=...
[2025-12-09 12:34:57] RESPONSE: 200 https://www.beeminder.com/api/v1/users/alice/goals.json?auth_token=...
</details>
| t.Run("LogRequest writes to file when LogFile is set", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log.txt") |
There was a problem hiding this comment.
Inconsistent indentation: This line has extra indentation (two tabs instead of one). Should align with the previous line tmpDir := t.TempDir().
| logFile := filepath.Join(tmpDir, "buzz_test_log.txt") | |
| logFile := filepath.Join(tmpDir, "buzz_test_log.txt") |
| t.Run("LogResponse writes to file when LogFile is set", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log_response.txt") |
There was a problem hiding this comment.
Inconsistent indentation: This line has extra indentation (two tabs instead of one). Should align with the previous line tmpDir := t.TempDir().
| logFile := filepath.Join(tmpDir, "buzz_test_log_response.txt") | |
| logFile := filepath.Join(tmpDir, "buzz_test_log_response.txt") |
| t.Run("Multiple log entries are appended", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log_multiple.txt") |
There was a problem hiding this comment.
Inconsistent indentation: This line has extra indentation (two tabs instead of one). Should align with the previous line tmpDir := t.TempDir().
| logFile := filepath.Join(tmpDir, "buzz_test_log_multiple.txt") | |
| logFile := filepath.Join(tmpDir, "buzz_test_log_multiple.txt") |
| return // Logging disabled | ||
| } | ||
|
|
||
| f, err := os.OpenFile(config.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) |
There was a problem hiding this comment.
Security Issue: Log file is created with 0644 permissions, making it readable by all users on the system. Given that logs will contain sensitive auth tokens (see related security comments), this is a security vulnerability.
Recommendation: Use more restrictive permissions similar to the config file (0600):
f, err := os.OpenFile(config.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)This ensures only the file owner can read the log file.
| f, err := os.OpenFile(config.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | |
| f, err := os.OpenFile(config.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) |
| func TestLoggingFunctionality(t *testing.T) { | ||
| t.Run("LogRequest does nothing when LogFile is empty", func(t *testing.T) { | ||
| config := &Config{ | ||
| Username: "test", | ||
| AuthToken: "token", | ||
| LogFile: "", // Empty means disabled | ||
| } | ||
| // Should not panic or error | ||
| LogRequest(config, "GET", "http://example.com") | ||
| LogResponse(config, 200, "http://example.com") | ||
| }) | ||
|
|
||
| t.Run("LogRequest does nothing when config is nil", func(t *testing.T) { | ||
| // Should not panic or error | ||
| LogRequest(nil, "GET", "http://example.com") | ||
| LogResponse(nil, 200, "http://example.com") | ||
| }) | ||
|
|
||
| t.Run("LogRequest writes to file when LogFile is set", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log.txt") | ||
|
|
||
| config := &Config{ | ||
| Username: "test", | ||
| AuthToken: "token", | ||
| LogFile: logFile, | ||
| } | ||
|
|
||
| // Log a request | ||
| LogRequest(config, "GET", "http://example.com/api") | ||
|
|
||
| // Verify file exists | ||
| if _, err := os.Stat(logFile); os.IsNotExist(err) { | ||
| t.Error("Log file should exist after LogRequest") | ||
| } | ||
|
|
||
| // Read and verify content | ||
| data, err := os.ReadFile(logFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read log file: %v", err) | ||
| } | ||
|
|
||
| content := string(data) | ||
| if !strings.Contains(content, "REQUEST: GET http://example.com/api") { | ||
| t.Errorf("Log content should contain request details, got: %s", content) | ||
| } | ||
| if !strings.Contains(content, "[20") { // Check for timestamp format [20XX-XX-XX ...] | ||
| t.Errorf("Log content should contain timestamp, got: %s", content) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("LogResponse writes to file when LogFile is set", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log_response.txt") | ||
|
|
||
| config := &Config{ | ||
| Username: "test", | ||
| AuthToken: "token", | ||
| LogFile: logFile, | ||
| } | ||
|
|
||
| // Log a response | ||
| LogResponse(config, 200, "http://example.com/api") | ||
|
|
||
| // Verify file exists | ||
| if _, err := os.Stat(logFile); os.IsNotExist(err) { | ||
| t.Error("Log file should exist after LogResponse") | ||
| } | ||
|
|
||
| // Read and verify content | ||
| data, err := os.ReadFile(logFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read log file: %v", err) | ||
| } | ||
|
|
||
| content := string(data) | ||
| if !strings.Contains(content, "RESPONSE: 200 http://example.com/api") { | ||
| t.Errorf("Log content should contain response details, got: %s", content) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Multiple log entries are appended", func(t *testing.T) { | ||
| // Create a temp directory for testing | ||
| tmpDir := t.TempDir() | ||
| logFile := filepath.Join(tmpDir, "buzz_test_log_multiple.txt") | ||
|
|
||
| config := &Config{ | ||
| Username: "test", | ||
| AuthToken: "token", | ||
| LogFile: logFile, | ||
| } | ||
|
|
||
| // Log multiple entries | ||
| LogRequest(config, "GET", "http://example.com/api/1") | ||
| LogResponse(config, 200, "http://example.com/api/1") | ||
| LogRequest(config, "POST", "http://example.com/api/2") | ||
| LogResponse(config, 201, "http://example.com/api/2") | ||
|
|
||
| // Read and verify content | ||
| data, err := os.ReadFile(logFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read log file: %v", err) | ||
| } | ||
|
|
||
| content := string(data) | ||
| if !strings.Contains(content, "REQUEST: GET http://example.com/api/1") { | ||
| t.Error("Log should contain first request") | ||
| } | ||
| if !strings.Contains(content, "RESPONSE: 200 http://example.com/api/1") { | ||
| t.Error("Log should contain first response") | ||
| } | ||
| if !strings.Contains(content, "REQUEST: POST http://example.com/api/2") { | ||
| t.Error("Log should contain second request") | ||
| } | ||
| if !strings.Contains(content, "RESPONSE: 201 http://example.com/api/2") { | ||
| t.Error("Log should contain second response") | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for URL sanitization: The tests verify that logging works when enabled/disabled and that log content is written correctly, but there's no test to verify that sensitive data (auth tokens) are properly handled in logs.
Recommendation: Add a test case to verify auth token sanitization:
t.Run("LogRequest sanitizes auth_token from URL", func(t *testing.T) {
tmpDir := t.TempDir()
logFile := filepath.Join(tmpDir, "buzz_test_sanitized.txt")
config := &Config{
Username: "test",
AuthToken: "secret_token_123",
LogFile: logFile,
}
// Log a request with auth_token in URL
LogRequest(config, "GET", "https://api.example.com/goals?auth_token=secret_token_123")
// Verify token is redacted
data, _ := os.ReadFile(logFile)
content := string(data)
if strings.Contains(content, "secret_token_123") {
t.Error("Log should not contain actual auth token")
}
if !strings.Contains(content, "[REDACTED]") || !strings.Contains(content, "auth_token=") {
t.Error("Log should show auth_token as redacted")
}
})| // LogRequest logs HTTP request details to the configured log file | ||
| func LogRequest(config *Config, method, url string) { | ||
| logToFile(config, fmt.Sprintf("REQUEST: %s %s", method, url)) | ||
| } |
There was a problem hiding this comment.
Critical Security Issue: The logging implementation logs complete URLs including authentication tokens. In Beeminder API calls, auth tokens are passed as query parameters (e.g., ?auth_token=abc123), which means sensitive credentials will be written to the log file in plain text.
Risk: If the log file is compromised, attackers gain access to the user's Beeminder account credentials.
Recommendation: Before logging URLs, sanitize them to redact sensitive query parameters:
func sanitizeURL(urlStr string) string {
u, err := url.Parse(urlStr)
if err != nil {
return urlStr
}
q := u.Query()
if q.Get("auth_token") != "" {
q.Set("auth_token", "[REDACTED]")
u.RawQuery = q.Encode()
}
return u.String()
}
// LogRequest logs HTTP request details to the configured log file
func LogRequest(config *Config, method, url string) {
logToFile(config, fmt.Sprintf("REQUEST: %s %s", method, sanitizeURL(url)))
}| // LogResponse logs HTTP response details to the configured log file | ||
| func LogResponse(config *Config, statusCode int, url string) { | ||
| logToFile(config, fmt.Sprintf("RESPONSE: %d %s", statusCode, url)) | ||
| } |
There was a problem hiding this comment.
Critical Security Issue: The LogResponse function also logs URLs containing authentication tokens. When logging responses, the same URLs with auth_token query parameters are exposed.
Recommendation: Apply the same URL sanitization as suggested for LogRequest to prevent auth token exposure.
| defer f.Close() | ||
|
|
||
| timestamp := time.Now().Format("2006-01-02 15:04:05") | ||
| logEntry := fmt.Sprintf("[%s] %s\n", timestamp, message) | ||
| // Intentionally ignore write errors to fail silently and not disrupt normal operations | ||
| f.WriteString(logEntry) |
There was a problem hiding this comment.
File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.
| defer f.Close() | |
| timestamp := time.Now().Format("2006-01-02 15:04:05") | |
| logEntry := fmt.Sprintf("[%s] %s\n", timestamp, message) | |
| // Intentionally ignore write errors to fail silently and not disrupt normal operations | |
| f.WriteString(logEntry) | |
| // Do not defer f.Close(); instead, close explicitly after writing and check error | |
| timestamp := time.Now().Format("2006-01-02 15:04:05") | |
| logEntry := fmt.Sprintf("[%s] %s\n", timestamp, message) | |
| // Intentionally ignore write errors to fail silently and not disrupt normal operations | |
| f.WriteString(logEntry) | |
| // Explicitly close the file and ignore the error to match fail-silently policy | |
| _ = f.Close() |
| When logging is enabled, buzz will append timestamped entries for each HTTP request and response to the specified log file: | ||
|
|
||
| ``` | ||
| [2025-12-09 12:34:56] REQUEST: GET https://www.beeminder.com/api/v1/users/alice/goals.json?auth_token=... | ||
| [2025-12-09 12:34:57] RESPONSE: 200 https://www.beeminder.com/api/v1/users/alice/goals.json?auth_token=... | ||
| ``` |
There was a problem hiding this comment.
These examples show request/response logs including the full Beeminder URL with the auth_token query parameter (?auth_token=...), which means the logging feature records API tokens in a plaintext log file. Anyone with access to the log (including other local users if the file is world-readable or systems ingesting the logs) can recover this token and impersonate the user against the Beeminder API. To avoid leaking credentials, ensure the implementation strips auth_token (and other secrets) from URLs before logging and recommend/ enforce restrictive permissions (e.g., 0600) on the log file path.
Adds opt-in logging capability to record HTTP request/response details for debugging API interactions. Disabled by default.
Changes
Config
log_filefield toConfigstruct (optional, omitempty)Logging implementation (
config.go)logToFile()- writes timestamped entries, fails silently on errorsLogRequest()- logs HTTP method and URLLogResponse()- logs status code and URLIntegration (
beeminder.go)Tests (
config_test.go)t.TempDir()for cross-platform compatibilityUsage
Edit
~/.buzzrc:{ "username": "alice", "auth_token": "abc123", "log_file": "/var/log/buzz.log" }Output format:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.