From be5c4818f51a9eb426dae121cd3dc13218be947b Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Fri, 11 Aug 2023 05:19:44 -0700 Subject: [PATCH] lib/httpserver: properly quote the returned address from GetQuotedRemoteAddr() for requests with X-Forwarded-For header Make sure that the quoted address can be used as JSON string. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4676#issuecomment-1663203424 This is a follow up for 252643d10069efddcd133aa8a58111227caf6034 and ac0b7e042152572f47e72d70de5293ed81149f8b Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4676 --- docs/CHANGELOG.md | 1 + lib/httpserver/httpserver.go | 7 +++--- lib/httpserver/httpserver_test.go | 36 +++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 lib/httpserver/httpserver_test.go diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index eb8996277..8c651089d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -47,6 +47,7 @@ The following `tip` changes can be tested by building VictoriaMetrics components * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert.html): fix `vmalert_remotewrite_send_duration_seconds_total` value, before it didn't count in the real time spending on remote write requests. See [this pr](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4801) for details. * BUGFIX: [vmbackupmanager](https://docs.victoriametrics.com/vmbackupmanager.html): fix panic when creating a backup to a local filesystem on Windows. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4704). * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): keep unmatched series when `remoteWrite.streamAggr.dropInput` is set to `false` to match intended behaviour introduced at [v1.92.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.92.0). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4804). +* BUGFIX: [vmui](https://docs.victoriametrics.com/#vmui): properly handle client address with `X-Forwarded-For` part. See [this comment](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4676#issuecomment-1663203424). ## [v1.92.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.92.1) diff --git a/lib/httpserver/httpserver.go b/lib/httpserver/httpserver.go index 9a9675baf..75b5940e0 100644 --- a/lib/httpserver/httpserver.go +++ b/lib/httpserver/httpserver.go @@ -436,11 +436,12 @@ var ( // GetQuotedRemoteAddr returns quoted remote address. func GetQuotedRemoteAddr(r *http.Request) string { - remoteAddr := strconv.Quote(r.RemoteAddr) // quote remoteAddr and X-Forwarded-For, since they may contain untrusted input + remoteAddr := r.RemoteAddr if addr := r.Header.Get("X-Forwarded-For"); addr != "" { - remoteAddr += ", X-Forwarded-For: " + strconv.Quote(addr) + remoteAddr += ", X-Forwarded-For: " + addr } - return remoteAddr + // quote remoteAddr and X-Forwarded-For, since they may contain untrusted input + return strconv.Quote(remoteAddr) } // Errorf writes formatted error message to w and to logger. diff --git a/lib/httpserver/httpserver_test.go b/lib/httpserver/httpserver_test.go new file mode 100644 index 000000000..9311cbfcc --- /dev/null +++ b/lib/httpserver/httpserver_test.go @@ -0,0 +1,36 @@ +package httpserver + +import ( + "encoding/json" + "net/http" + "testing" +) + +func TestGetQuotedRemoteAddr(t *testing.T) { + f := func(remoteAddr, xForwardedFor, expectedAddr string) { + t.Helper() + + req := &http.Request{ + RemoteAddr: remoteAddr, + } + if xForwardedFor != "" { + req.Header = map[string][]string{ + "X-Forwarded-For": {xForwardedFor}, + } + } + addr := GetQuotedRemoteAddr(req) + if addr != expectedAddr { + t.Fatalf("unexpected remote addr;\ngot\n%s\nwant\n%s", addr, expectedAddr) + } + + // Verify that the addr can be unmarshaled as JSON string + var s string + if err := json.Unmarshal([]byte(addr), &s); err != nil { + t.Fatalf("cannot unmarshal addr: %s", err) + } + } + + f("1.2.3.4", "", `"1.2.3.4"`) + f("1.2.3.4", "foo.bar", `"1.2.3.4, X-Forwarded-For: foo.bar"`) + f("1.2\n\"3.4", "foo\nb\"ar", `"1.2\n\"3.4, X-Forwarded-For: foo\nb\"ar"`) +}