Actually having a comma-separated allowedNetworks

Development
1

A direct quote from the documentation of allowedNetwork reads (my emphasis):

By setting this to a comma separated list of networks, connections to the given device will be limited to those networks.

Are we sure that really is the case?

I’m no golang developer, but the following patch sure seems to both make multiple allowed networks work and add some debug output to clarify why it fails unpatched. Where the added loop is for the actual functionality.

diff --git a/lib/connections/service.go b/lib/connections/service.go
index 1aaf3633c..75b8a5739 100644
--- a/lib/connections/service.go
+++ b/lib/connections/service.go
@@ -1119,7 +1119,10 @@ func IsAllowedNetwork(host string, allowed []string) bool {
 		return false
 	}
 
-	for _, n := range allowed {
+	for _, p := range allowed {
+		l.Warnf("p: ", p)
+		for _, n := range strings.Split(p, ",") {
+			l.Warnf("n: ", p)
 			result := true
 			if strings.HasPrefix(n, "!") {
 				result = false
@@ -1133,6 +1136,7 @@ func IsAllowedNetwork(host string, allowed []string) bool {
 				return result
 			}
 		}
+	}
 
 	return false
 }

Without the split+loop the range allowed appears to return a string still containing the comma. E.g. “0.0.0.0/0,192.168.1.0/24”. I fail to see how the code in that function is supposed to work with such a string.

Maybe there is a more appropriate place to fix this bug. It seems from the existing loop that the input ought to already have been split. The datatype probably gets constructed from proto/lib/config/deviceconfiguration.proto, but how that works and where the actual parsed XML gets transformed into protobuf is beyond my understanding.

Do we agree this is a bug? Would someone be keen on either fixing it properly, or be holding my hand through the likely long journey of getting me up to speed with golang?

2

In the config file, they are separate elements. However, in the Advanced Configuration in the GUI, they are all placed together in a single line and divided with commas.

2 Likes
3

Not in mine they aren’t. I constructed the XML according to the apparently buggy documentation.

How would the team feel about applying a patch such as the following one:

From 9ecf6fe8db59518c3a7a196140de4d2a616171a6 Mon Sep 17 00:00:00 2001
From: cos <cos>
Date: Wed, 4 Dec 2024 20:43:57 +0100
Subject: [PATCH] Fix repeats of allowedNetworks in XML context

---
 users/config.rst | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/users/config.rst b/users/config.rst
index 2d42b3c..f968868 100644
--- a/users/config.rst
+++ b/users/config.rst
@@ -644,6 +644,7 @@ Device Element
         <address>tcp://192.0.2.1:22001</address>
         <paused>true</paused>
         <allowedNetwork>192.168.0.0/16</allowedNetwork>
+        <allowedNetwork>172.16.0.0/16</allowedNetwork>
         <autoAcceptFolders>false</autoAcceptFolders>
         <maxSendKbps>100</maxSendKbps>
         <maxRecvKbps>100</maxRecvKbps>
@@ -764,8 +765,10 @@ From the following child elements at least one ``address`` child must exist.
 .. option:: device.allowedNetwork
     :aliases: device.allowedNetworks
 
-    If given, this restricts connections to this device to only this network.
-    The mechanism is described in detail in a :doc:`separate chapter
+    If given at least once, this restricts connections to this device to only
+    this network. In the GUI a comma separated list is given, while in XML this
+    attribute needs to be repeated for each network. The mechanism is described
+    in detail in a :doc:`separate chapter
     </advanced/device-allowednetworks>`).
 
 .. option:: device.autoAcceptFolders
-- 
2.46.1

Most repeatable tags appears to already be properly documented. It might be worth clarifying the syntax for alwaysLocalNet, stunServer and unackedNotificationID. Please let me know if you’d wish me to make an attempt at that as a new version of the above patch, or if it’s better handled separately.

4

The config page that shows xml is uses singular allowedNetwork and nothing about comma separation. That separate page on allowedNetworks is written from the point of view of the web UI, where it is represented as a comma separated list. Granted that’s not entirely self evident, so adjusting that to either entirely leave out the “comma separated” and just say “a list of networks” seems reasonable. Maybe with an addendum that in the UI/json that corresponds to a comma separated list on allowedNetworks while in xml it’s multiple allowedNetwork elements, though that feels a bit pedantic.

1 Like
5

A very late response, but just now I realized this got fixed in f3165de.

Timing and content makes me believe this thread had at least some influence on the fix, even if #832 does tell that the issue was known for at least a year prior to me getting bitten by this.

Thanks to everyone involved for acting on feedback through this forum! It is much appreciated. …and thanks for syncthing itself, of course!!