aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorvapatel2 <149737089+vapatel2@users.noreply.github.com>2023-11-08 03:10:23 -0800
committerGitHub <noreply@github.com>2023-11-08 11:10:23 +0000
commit854499382e20e39a43b1643939b5fa9f167537f5 (patch)
tree9eacdd9763e12424a19c88d057f354a11dcf737e
parentcb4c1f9ad28c3f8fc44ac9abed37f1f860072335 (diff)
downloadsyncthing-854499382e20e39a43b1643939b5fa9f167537f5.tar.gz
syncthing-854499382e20e39a43b1643939b5fa9f167537f5.zip
cmd/stdiscosrv: Prevent nil IPs from X-Forwarded-For (fixes #9189) (#9190)
### Purpose Treat X-Forwarded-For as a comma-separated string to prevent nil IP being returned by the Discovery Server ### Testing Unit Tests implemented Testing with a Discovery Client can be done as follows: ``` A simple example to replicate this entails running Discovery with HTTP, use Nginx as a reverse proxy and hardcode (as an example) a list of IPs in the X-Forwarded-For header. 1. Send an Announcement with tcp://0.0.0.0:<some-port> 2. Query the DeviceID 3. Observe the returned IP Address is no longer nil; i.e. `tcp://<nil>:<some-port>` ```
-rw-r--r--cmd/stdiscosrv/apisrv.go28
-rw-r--r--cmd/stdiscosrv/apisrv_test.go8
2 files changed, 30 insertions, 6 deletions
diff --git a/cmd/stdiscosrv/apisrv.go b/cmd/stdiscosrv/apisrv.go
index c9c8344b3..d4bfd490b 100644
--- a/cmd/stdiscosrv/apisrv.go
+++ b/cmd/stdiscosrv/apisrv.go
@@ -136,7 +136,12 @@ func (s *apiSrv) handler(w http.ResponseWriter, req *http.Request) {
}
if s.useHTTP {
- remoteAddr.IP = net.ParseIP(req.Header.Get("X-Forwarded-For"))
+ // X-Forwarded-For can have multiple client IPs; split using the comma separator
+ forwardIP, _, _ := strings.Cut(req.Header.Get("X-Forwarded-For"), ",")
+
+ // net.ParseIP will return nil if leading/trailing whitespace exists; use strings.TrimSpace()
+ remoteAddr.IP = net.ParseIP(strings.TrimSpace(forwardIP))
+
if parsedPort, err := strconv.ParseInt(req.Header.Get("X-Client-Port"), 10, 0); err == nil {
remoteAddr.Port = int(parsedPort)
}
@@ -410,13 +415,13 @@ func fixupAddresses(remote *net.TCPAddr, addresses []string) []string {
continue
}
- if remote != nil {
- if host == "" || ip.IsUnspecified() {
+ if host == "" || ip.IsUnspecified() {
+ if remote != nil {
// Replace the unspecified IP with the request source.
// ... unless the request source is the loopback address or
// multicast/unspecified (can't happen, really).
- if remote.IP.IsLoopback() || remote.IP.IsMulticast() || remote.IP.IsUnspecified() {
+ if remote.IP == nil || remote.IP.IsLoopback() || remote.IP.IsMulticast() || remote.IP.IsUnspecified() {
continue
}
@@ -432,11 +437,22 @@ func fixupAddresses(remote *net.TCPAddr, addresses []string) []string {
}
host = remote.IP.String()
+
+ } else {
+ // remote is nil, unable to determine host IP
+ continue
}
- // If zero port was specified, use remote port.
- if port == "0" && remote.Port > 0 {
+ }
+
+ // If zero port was specified, use remote port.
+ if port == "0" {
+ if remote != nil && remote.Port > 0 {
+ // use remote port
port = strconv.Itoa(remote.Port)
+ } else {
+ // unable to determine remote port
+ continue
}
}
diff --git a/cmd/stdiscosrv/apisrv_test.go b/cmd/stdiscosrv/apisrv_test.go
index 3514994a3..457ed404f 100644
--- a/cmd/stdiscosrv/apisrv_test.go
+++ b/cmd/stdiscosrv/apisrv_test.go
@@ -69,6 +69,14 @@ func TestFixupAddresses(t *testing.T) {
remote: addr("123.123.123.123", 9000),
in: []string{"tcp://44.44.44.44:0"},
out: []string{"tcp://44.44.44.44:9000"},
+ }, { // remote ip nil
+ remote: addr("", 9000),
+ in: []string{"tcp://:22000", "tcp://44.44.44.44:9000"},
+ out: []string{"tcp://44.44.44.44:9000"},
+ }, { // remote port 0
+ remote: addr("123.123.123.123", 0),
+ in: []string{"tcp://:22000", "tcp://44.44.44.44"},
+ out: []string{"tcp://123.123.123.123:22000"},
},
}