detached 01b6bc5

fix: address coderabbit findings on access log

- Escape \n and \r in escapeQuoted to prevent log injection via user-controlled
  fields (User-Agent, Referer, request target) in CLF/Combined formats
- Simplify AccessLogger trustedNets parameter from variadic ...[]* to plain []*net.IPNet
commit
01b6bc5268a759b9d8fc81d5d3dbee68e028b948
author
brice · · 2026-04-03 15:46 UTC
parent
be4bda7
signature
GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email
3 files · +14 -16
Unified
UnifiedSplit
M internal/adapters/in/http/middleware/accesslog.go +2 -7
@@ -26,12 +26,7 @@ type AccessLogWriter interface {
2626 //
2727 // Write failures are reported as warnings through the application logger and
2828 // never fail the HTTP response.
29-func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.Handler) http.Handler {
30- var nets []*net.IPNet
31- if len(trustedNets) > 0 {
32- nets = trustedNets[0]
33- }
34-
29+func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap.Logger, trustedNets []*net.IPNet) func(http.Handler) http.Handler {
3530 return func(next http.Handler) http.Handler {
3631 return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
3732 start := time.Now()
@@ -45,7 +40,7 @@ func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap
4540
4641 // Resolve client IP before calling the inner handler so we see the
4742 // original remote address before any mutation.
48- clientIP := GetClientIP(r, nets)
43+ clientIP := GetClientIP(r, trustedNets)
4944
5045 next.ServeHTTP(rw, r)
5146
@@ -26,12 +26,7 @@ type AccessLogWriter interface {
26 //26 //
27 // Write failures are reported as warnings through the application logger and27 // Write failures are reported as warnings through the application logger and
28 // never fail the HTTP response.28 // never fail the HTTP response.
29-func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap.Logger, trustedNets ...[]*net.IPNet) func(http.Handler) http.Handler {29+func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap.Logger, trustedNets []*net.IPNet) func(http.Handler) http.Handler {
30- var nets []*net.IPNet
31- if len(trustedNets) > 0 {
32- nets = trustedNets[0]
33- }
34-
35 return func(next http.Handler) http.Handler {30 return func(next http.Handler) http.Handler {
36 return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {31 return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
37 start := time.Now()32 start := time.Now()
@@ -45,7 +40,7 @@ func AccessLogger(writer AccessLogWriter, excludeHealthChecks bool, log zerowrap
45 40
46 // Resolve client IP before calling the inner handler so we see the41 // Resolve client IP before calling the inner handler so we see the
47 // original remote address before any mutation.42 // original remote address before any mutation.
48- clientIP := GetClientIP(r, nets)43+ clientIP := GetClientIP(r, trustedNets)
49 44
50 next.ServeHTTP(rw, r)45 next.ServeHTTP(rw, r)
51 46
M internal/adapters/in/http/middleware/accesslog_test.go +7 -7
@@ -27,7 +27,7 @@ func TestAccessLogger_EmitsEntryPerRequest(t *testing.T) {
2727 mock := &mockAccessLogWriter{}
2828 log := testLogger()
2929
30- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
30+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
3131 w.WriteHeader(http.StatusOK)
3232 _, _ = w.Write([]byte("hello"))
3333 }))
@@ -62,7 +62,7 @@ func TestAccessLogger_RequestIDReused(t *testing.T) {
6262 mock := &mockAccessLogWriter{}
6363 log := testLogger()
6464
65- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
65+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
6666 w.WriteHeader(http.StatusOK)
6767 }))
6868
@@ -80,7 +80,7 @@ func TestAccessLogger_ExcludeHealthCheckUA(t *testing.T) {
8080 mock := &mockAccessLogWriter{}
8181 log := testLogger()
8282
83- handler := AccessLogger(mock, true, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
83+ handler := AccessLogger(mock, true, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
8484 w.WriteHeader(http.StatusOK)
8585 }))
8686
@@ -97,7 +97,7 @@ func TestAccessLogger_ExcludeLoopbackIP(t *testing.T) {
9797 mock := &mockAccessLogWriter{}
9898 log := testLogger()
9999
100- handler := AccessLogger(mock, true, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
100+ handler := AccessLogger(mock, true, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
101101 w.WriteHeader(http.StatusOK)
102102 }))
103103
@@ -114,7 +114,7 @@ func TestAccessLogger_NoExcludeWhenDisabled(t *testing.T) {
114114 mock := &mockAccessLogWriter{}
115115 log := testLogger()
116116
117- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
117+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
118118 w.WriteHeader(http.StatusOK)
119119 }))
120120
@@ -131,7 +131,7 @@ func TestAccessLogger_WriteFailureDoesNotAffectResponse(t *testing.T) {
131131 mock := &mockAccessLogWriter{err: fmt.Errorf("disk full")}
132132 log := testLogger()
133133
134- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
134+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
135135 w.WriteHeader(http.StatusOK)
136136 _, _ = w.Write([]byte("ok"))
137137 }))
@@ -154,7 +154,7 @@ func TestAccessLogger_PanicRecoveryChain_Captures500(t *testing.T) {
154154 panicHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
155155 panic("test panic")
156156 })
157- chain := AccessLogger(mock, false, log)(PanicRecovery(log)(panicHandler))
157+ chain := AccessLogger(mock, false, log, nil)(PanicRecovery(log)(panicHandler))
158158
159159 req := httptest.NewRequest(http.MethodGet, "/crash", nil)
160160 rw := httptest.NewRecorder()
@@ -27,7 +27,7 @@ func TestAccessLogger_EmitsEntryPerRequest(t *testing.T) {
27 mock := &mockAccessLogWriter{}27 mock := &mockAccessLogWriter{}
28 log := testLogger()28 log := testLogger()
29 29
30- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {30+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
31 w.WriteHeader(http.StatusOK)31 w.WriteHeader(http.StatusOK)
32 _, _ = w.Write([]byte("hello"))32 _, _ = w.Write([]byte("hello"))
33 }))33 }))
@@ -62,7 +62,7 @@ func TestAccessLogger_RequestIDReused(t *testing.T) {
62 mock := &mockAccessLogWriter{}62 mock := &mockAccessLogWriter{}
63 log := testLogger()63 log := testLogger()
64 64
65- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {65+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66 w.WriteHeader(http.StatusOK)66 w.WriteHeader(http.StatusOK)
67 }))67 }))
68 68
@@ -80,7 +80,7 @@ func TestAccessLogger_ExcludeHealthCheckUA(t *testing.T) {
80 mock := &mockAccessLogWriter{}80 mock := &mockAccessLogWriter{}
81 log := testLogger()81 log := testLogger()
82 82
83- handler := AccessLogger(mock, true, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {83+ handler := AccessLogger(mock, true, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
84 w.WriteHeader(http.StatusOK)84 w.WriteHeader(http.StatusOK)
85 }))85 }))
86 86
@@ -97,7 +97,7 @@ func TestAccessLogger_ExcludeLoopbackIP(t *testing.T) {
97 mock := &mockAccessLogWriter{}97 mock := &mockAccessLogWriter{}
98 log := testLogger()98 log := testLogger()
99 99
100- handler := AccessLogger(mock, true, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {100+ handler := AccessLogger(mock, true, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
101 w.WriteHeader(http.StatusOK)101 w.WriteHeader(http.StatusOK)
102 }))102 }))
103 103
@@ -114,7 +114,7 @@ func TestAccessLogger_NoExcludeWhenDisabled(t *testing.T) {
114 mock := &mockAccessLogWriter{}114 mock := &mockAccessLogWriter{}
115 log := testLogger()115 log := testLogger()
116 116
117- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {117+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
118 w.WriteHeader(http.StatusOK)118 w.WriteHeader(http.StatusOK)
119 }))119 }))
120 120
@@ -131,7 +131,7 @@ func TestAccessLogger_WriteFailureDoesNotAffectResponse(t *testing.T) {
131 mock := &mockAccessLogWriter{err: fmt.Errorf("disk full")}131 mock := &mockAccessLogWriter{err: fmt.Errorf("disk full")}
132 log := testLogger()132 log := testLogger()
133 133
134- handler := AccessLogger(mock, false, log)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {134+ handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
135 w.WriteHeader(http.StatusOK)135 w.WriteHeader(http.StatusOK)
136 _, _ = w.Write([]byte("ok"))136 _, _ = w.Write([]byte("ok"))
137 }))137 }))
@@ -154,7 +154,7 @@ func TestAccessLogger_PanicRecoveryChain_Captures500(t *testing.T) {
154 panicHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {154 panicHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
155 panic("test panic")155 panic("test panic")
156 })156 })
157- chain := AccessLogger(mock, false, log)(PanicRecovery(log)(panicHandler))157+ chain := AccessLogger(mock, false, log, nil)(PanicRecovery(log)(panicHandler))
158 158
159 req := httptest.NewRequest(http.MethodGet, "/crash", nil)159 req := httptest.NewRequest(http.MethodGet, "/crash", nil)
160 rw := httptest.NewRecorder()160 rw := httptest.NewRecorder()
M internal/adapters/out/accesslog/formatter.go +5 -2
@@ -110,10 +110,13 @@ func buildRequestTarget(path, query string) string {
110110 return path + "?" + query
111111 }
112112
113-// escapeQuoted escapes backslashes and double-quotes inside a value that will
114-// be enclosed in double-quotes in the log line.
113+// escapeQuoted escapes backslashes, double-quotes, and newline characters inside
114+// a value that will be enclosed in double-quotes in the log line. Newline
115+// escaping prevents log-injection via user-controlled fields such as User-Agent.
115116 func escapeQuoted(s string) string {
116117 s = strings.ReplaceAll(s, `\`, `\\`)
117118 s = strings.ReplaceAll(s, `"`, `\"`)
119+ s = strings.ReplaceAll(s, "\n", `\n`)
120+ s = strings.ReplaceAll(s, "\r", `\r`)
118121 return s
119122 }
@@ -110,10 +110,13 @@ func buildRequestTarget(path, query string) string {
110 return path + "?" + query110 return path + "?" + query
111 }111 }
112 112
113-// escapeQuoted escapes backslashes and double-quotes inside a value that will113+// escapeQuoted escapes backslashes, double-quotes, and newline characters inside
114-// be enclosed in double-quotes in the log line.114+// a value that will be enclosed in double-quotes in the log line. Newline
115+// escaping prevents log-injection via user-controlled fields such as User-Agent.
115 func escapeQuoted(s string) string {116 func escapeQuoted(s string) string {
116 s = strings.ReplaceAll(s, `\`, `\\`)117 s = strings.ReplaceAll(s, `\`, `\\`)
117 s = strings.ReplaceAll(s, `"`, `\"`)118 s = strings.ReplaceAll(s, `"`, `\"`)
119+ s = strings.ReplaceAll(s, "\n", `\n`)
120+ s = strings.ReplaceAll(s, "\r", `\r`)
118 return s121 return s
119 }122 }

Keyboard shortcuts

Single-key shortcuts are on. ctrl k and esc work whatever this says.

ctrl k :
go to a section, branch, tag, file or repository
?
this list
j k
move the row cursor down / up
enter
open the row under the cursor; on Files, a directory opens or closes in place
↑ ↓ ← → home end
in the file tree (focus on a row): previous / next row, close / open a directory or go to its parent / first entry, first / last row. They never open a file
l h
on Files: open a directory or a file / close a directory or go up to its parent, then to the parent page. Elsewhere: into the side panel (on a merge request, its files) / back to the content; there j k move between its controls
g g G
first / last row
/
filter the list; on Files, find a file anywhere in the repository (esc clears, enter opens the best match)
b
switch branch or tag
g s g f g c g b g m g i g r g a
tabs: summary, files, commits, branches & tags, merge requests, issues, releases, actions (when available)
g h
home
w
wide mode: use the whole width of the screen, or not (remembered)
y
copy a permalink pinned to the commit
d u x
diff: split / unified / collapse or expand all
e E
diff: the whole file in view / every file, with the lines the hunks hide
n
merge request: next note in the diff
esc
close a dialog, leave a field

Branches 7

Tags 106

↑↓ moveenter openesc close