Repository: syncthing/syncthing (language: go)

Issue:
https://relays.syncthing.net/endpoint contains non-valid URLs with spaces
Hi,

When I fetch the public list of relays via the url `https://relays.syncthing.net/endpoint`, some urls contains spaces, i.e.

```
relay://70.166.128.229:443/?id=SIG7FCH-EGABO3D-QSF6MBM-5O5Y6HP-YJGMHEI-QGHNOKZ-3X4JGCY-UOSMIAO&pingInterval=1m0s&networkTimeout=2m0s&sessionLimitBps=0&globalLimitBps=0&statusAddr=:22070&providedBy=Jon Penn
```

(the `providedBy=Jon Penn` piece) . However, it should not; the spaces have to be escaped. As this is non-valid urls, and they are completely rejected by 3rd party parsers, like https://github.com/uriparser/uriparser I use.

 


Discussion / hints:

Fix patch:
diff --git a/cmd/strelaypoolsrv/main.go b/cmd/strelaypoolsrv/main.go
index a0364a81c55..be54fd3b95b 100644
--- a/cmd/strelaypoolsrv/main.go
+++ b/cmd/strelaypoolsrv/main.go
@@ -368,6 +368,11 @@ func handlePostRequest(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Canonicalize the URL. In particular, parse and re-encode the query
+	// string so that it's guaranteed to be valid.
+	uri.RawQuery = uri.Query().Encode()
+	newRelay.URL = uri.String()
+
 	if relayCert != nil {
 		advertisedId := uri.Query().Get("id")
 		idFromCert := protocol.NewDeviceID(relayCert.Raw).String()
diff --git a/cmd/strelaysrv/main.go b/cmd/strelaysrv/main.go
index 507b3e6c01b..4757f90b0ee 100644
--- a/cmd/strelaysrv/main.go
+++ b/cmd/strelaysrv/main.go
@@ -230,12 +230,31 @@ func main() {
 		go statusService(statusAddr)
 	}
 
-	uri, err := url.Parse(fmt.Sprintf("relay://%s/?id=%s&pingInterval=%s&networkTimeout=%s&sessionLimitBps=%d&globalLimitBps=%d&statusAddr=%s&providedBy=%s", mapping.Address(), id, pingInterval, networkTimeout, sessionLimitBps, globalLimitBps, statusAddr, providedBy))
+	uri, err := url.Parse(fmt.Sprintf("relay://%s/", mapping.Address()))
 	if err != nil {
 		log.Fatalln("Failed to construct URI", err)
 		return
 	}
 
+	// Add properly encoded query string parameters to URL.
+	query := make(url.Values)
+	query.Set("id", id.String())
+	query.Set("pingInterval", pingInterval.String())
+	query.Set("networkTimeout", networkTimeout.String())
+	if sessionLimitBps > 0 {
+		query.Set("sessionLimitBps", fmt.Sprint(sessionLimitBps))
+	}
+	if globalLimitBps > 0 {
+		query.Set("globalLimitBps", fmt.Sprint(globalLimitBps))
+	}
+	if statusAddr != "" {
+		query.Set("statusAddr", statusAddr)
+	}
+	if providedBy != "" {
+		query.Set("providedBy", providedBy)
+	}
+	uri.RawQuery = query.Encode()
+
 	log.Println("URI:", uri.String())
 
 	if poolAddrs == defaultPoolAddrs {


Test patch:
diff --git a/cmd/strelaypoolsrv/main_test.go b/cmd/strelaypoolsrv/main_test.go
index 6b5732f124d..db75846557c 100644
--- a/cmd/strelaypoolsrv/main_test.go
+++ b/cmd/strelaypoolsrv/main_test.go
@@ -11,6 +11,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"sync"
 	"testing"
@@ -65,3 +66,29 @@ func TestHandleGetRequest(t *testing.T) {
 		}
 	}
 }
+
+func TestCanonicalizeQueryValues(t *testing.T) {
+	// This just demonstrates and validates the uri.Parse/String stuff in
+	// regards to query strings.
+
+	in := "http://example.com/?some weird= query^value"
+	exp := "http://example.com/?some+weird=+query%5Evalue"
+
+	uri, err := url.Parse(in)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	str := uri.String()
+	if str != in {
+		// Just re-encoding the URL doesn't sanitize the query string.
+		t.Errorf("expected %q, got %q", in, str)
+	}
+
+	uri.RawQuery = uri.Query().Encode()
+	str = uri.String()
+	if str != exp {
+		// The query string is now in correct format.
+		t.Errorf("expected %q, got %q", exp, str)
+	}
+}