mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat: log additional known non-sensitive query param fields in the httpmw logger (#19532)
Blink helped here but it's suggestion was to have a set map of sensitive fields based on predefined constants in various files, such as the api token string names. For now we'll add additional query param logging for fields we know are safe/that we want to log, such as query pagination/limit fields and ID list counts which may help identify P99 DB query latencies. --------- Signed-off-by: Callum Styan <callumstyan@gmail.com>
This commit is contained in:
@@ -4,6 +4,9 @@ import (
|
||||
"context"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
@@ -15,6 +18,59 @@ import (
|
||||
"github.com/coder/coder/v2/coderd/tracing"
|
||||
)
|
||||
|
||||
var (
|
||||
safeParams = []string{"page", "limit", "offset"}
|
||||
countParams = []string{"ids", "template_ids"}
|
||||
)
|
||||
|
||||
func safeQueryParams(params url.Values) []slog.Field {
|
||||
if len(params) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
fields := make([]slog.Field, 0, len(params))
|
||||
for key, values := range params {
|
||||
// Check if this parameter should be included
|
||||
for _, pattern := range safeParams {
|
||||
if strings.EqualFold(key, pattern) {
|
||||
// Prepend query parameters in the log line to ensure we don't have issues with collisions
|
||||
// in case any other internal logging fields already log fields with similar names
|
||||
fieldName := "query_" + key
|
||||
|
||||
// Log the actual values for non-sensitive parameters
|
||||
if len(values) == 1 {
|
||||
fields = append(fields, slog.F(fieldName, values[0]))
|
||||
continue
|
||||
}
|
||||
fields = append(fields, slog.F(fieldName, values))
|
||||
}
|
||||
}
|
||||
// Some query params we just want to log the count of the params length
|
||||
for _, pattern := range countParams {
|
||||
if !strings.EqualFold(key, pattern) {
|
||||
continue
|
||||
}
|
||||
count := 0
|
||||
|
||||
// Prepend query parameters in the log line to ensure we don't have issues with collisions
|
||||
// in case any other internal logging fields already log fields with similar names
|
||||
fieldName := "query_" + key
|
||||
|
||||
// Count comma-separated values for CSV format
|
||||
for _, v := range values {
|
||||
if strings.Contains(v, ",") {
|
||||
count += len(strings.Split(v, ","))
|
||||
continue
|
||||
}
|
||||
count++
|
||||
}
|
||||
// For logging we always want strings
|
||||
fields = append(fields, slog.F(fieldName+"_count", strconv.Itoa(count)))
|
||||
}
|
||||
}
|
||||
return fields
|
||||
}
|
||||
|
||||
func Logger(log slog.Logger) func(next http.Handler) http.Handler {
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
|
||||
@@ -39,6 +95,11 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
|
||||
slog.F("start", start),
|
||||
)
|
||||
|
||||
// Add safe query parameters to the log
|
||||
if queryFields := safeQueryParams(r.URL.Query()); len(queryFields) > 0 {
|
||||
httplog = httplog.With(queryFields...)
|
||||
}
|
||||
|
||||
logContext := NewRequestLogger(httplog, r.Method, start)
|
||||
|
||||
ctx := WithRequestLogger(r.Context(), logContext)
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"slices"
|
||||
"strings"
|
||||
"sync"
|
||||
@@ -292,6 +293,76 @@ func TestRequestLogger_RouteParamsLogging(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestSafeQueryParams(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
params url.Values
|
||||
expected map[string]interface{}
|
||||
}{
|
||||
{
|
||||
name: "safe parameters",
|
||||
params: url.Values{
|
||||
"page": []string{"1"},
|
||||
"limit": []string{"10"},
|
||||
"filter": []string{"active"},
|
||||
"sort": []string{"name"},
|
||||
"offset": []string{"2"},
|
||||
"ids": []string{"some-id,another-id", "second-param"},
|
||||
"template_ids": []string{"some-id,another-id", "second-param"},
|
||||
},
|
||||
expected: map[string]interface{}{
|
||||
"query_page": "1",
|
||||
"query_limit": "10",
|
||||
"query_offset": "2",
|
||||
"query_ids_count": "3",
|
||||
"query_template_ids_count": "3",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "unknown/sensitive parameters",
|
||||
params: url.Values{
|
||||
"token": []string{"secret-token"},
|
||||
"api_key": []string{"secret-key"},
|
||||
"coder_signed_app_token": []string{"jwt-token"},
|
||||
"coder_application_connect_api_key": []string{"encrypted-key"},
|
||||
"client_secret": []string{"oauth-secret"},
|
||||
"code": []string{"auth-code"},
|
||||
},
|
||||
expected: map[string]interface{}{},
|
||||
},
|
||||
{
|
||||
name: "mixed parameters",
|
||||
params: url.Values{
|
||||
"page": []string{"1"},
|
||||
"token": []string{"secret"},
|
||||
"filter": []string{"active"},
|
||||
},
|
||||
expected: map[string]interface{}{
|
||||
"query_page": "1",
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
tt := tt
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
fields := safeQueryParams(tt.params)
|
||||
|
||||
// Convert fields to map for easier comparison
|
||||
result := make(map[string]interface{})
|
||||
for _, field := range fields {
|
||||
result[field.Name] = field.Value
|
||||
}
|
||||
|
||||
require.Equal(t, tt.expected, result)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
type fakeSink struct {
|
||||
entries []slog.SinkEntry
|
||||
newEntries chan slog.SinkEntry
|
||||
|
||||
Reference in New Issue
Block a user