Invalid gui address path ⇒ confusing incorrect error

Support
1

While transforming my scripts to deal with v2.0 I messed up the gui address path to include a couple of quotes. That is my config read similar to this:

    <gui enabled="true" tls="false" sendBasicAuthPrompt="false">
        <address>'/tmp/st.sock'</address>
        <metricsWithoutAuth>false</metricsWithoutAuth>
        <apikey>secret</apikey>
        <theme>default</theme>
    </gui>

This leads to this error message:

Failed to start API (error="listen tcp: address '/tmp/st.sock': missing port in address" log.pkg=syncthing)

My incorrect assumption was that the major version bump to v2.0 had led to a regression in syncthing, breaking the API over unix domain socket. I still find that to be a reasonable interpretation of the quoted error message.

I took me quite a session before figuring out the mistake was mine. First with a failed attempt at adding a unix domain socket test case to lib/api/api_test.go. Then with a very confusing git-bisect experience. Where I started to question my sanity, when neither the latest v1 release nor any older version which actually still builds managed to serve as the known good commit.

Once I actually realized the added quotes, it of course became obvious. Yet an error message can be more helpful in identifying the cause of the error than this one currently is. In my opinion, a better message would be:

Failed to start API (error="invalid address: '\'/tmp/st.sock\''" log.pkg=syncthing)

Note the added quotes and the escaping of original quotes.

I don’t object to retaining the existing error message in cases when an actual tcp configuration has been detected.

One perspective is that (GUIConfiguration) Network() does insufficient validation. Thus an error leaks from a later method called with incorrect assumptions, one which arguable should never have been called at all.

In my opinion the function signature ought to be updated from returning string to (string, error), accompanied with something like matching a bunch of regexps of valid address formats.

Golang isn’t my primary language, but I could have a go at landing this change, if it is at all welcome? Preferably there ought to be an added test case too, but I obviously didn’t fully understand the existing tests. Are actual connections made at all, or is the transport mocked away?

1 Like
2

Honestly I think this is a fool’s errand. Arguably the format is just a bit too flexible to handle all reasonable cases.

3

I might have overlooked that the method is used in more flexible contexts, but according to docs the gui address should only be allowed to take these forms:

IPv4 address and port (127.0.0.1:8384)
IPv6 address and port ([::1]:8384)
Wildcard and port (0.0.0.0:12345, [::]:12345, :12345)
UNIX socket location (/var/run/st.sock)

Regex matching to categorize between mostly numeric addresses, valid file paths and everything else (i.e. prohibited values) actually seems quite trivial. Right?

The source code seems to suggest that unix:/path/st.sock (or something) is allowed in addition that what’s documented. Should the objection be understood that there is a power functionality easter egg here, allowing hidden flexibility and thus requiring full rfc3986 parsing (and beyond)?

4

The docs give examples but there are many more complicated or exotic ways to express tcp listen addresses, using hostnames, IPv6 with zone specifiers, numeric IP addresses like 2130706433, etc. Enough that I’m pretty sure any initial, naive attempt to validate using regexes will erroneously reject valid addresses.

5

Sure there are, but all invalid as the documentation currently reads. I would argue that’s highly unexpected and a bug, which ought to best be fixed by reducing the flexibility. While thats the sane thing to do, I do realize its controversial and would possibly breaking some user’s usage of this undocumented anti-feature. Thus I suggest this change:

diff --git i/users/config.rst w/users/config.rst
index c5f9e2b..3234139 100644
--- i/users/config.rst
+++ w/users/config.rst
@@ -849,7 +849,7 @@ The following child elements may be present:
 .. option:: gui.address
     :mandatory: Exactly one element must be present.
 
-    Set the listen address.  Allowed address formats are:
+    Set the listen address.  Allowed address formats include:
 
     IPv4 address and port (``127.0.0.1:8384``)
         The address and port are used as given.

Ignoring this other bug of that exposing API and user interface on just about anything, I guess that instead of regex matching there are gems on cpan to put in requirements.txt, or whatever its called in the golang world. Is it basically validating the strings for RFC 3986 compliance which is needed? (Suffixing protocols with 4 or 6 is a syncthing specific invention? or is it used elsewhere?)

The original question remains. Would a fix have a snowflakes chance in hell to be merged, or should I just drop this?

6

Well these are not URIs to begin with, although that is also a valid alternative. I do not want to see regex validation. If you want to do earlier validation by attempting to parse the string as a network address or filesystem path, etc, I think that would be fine as then it’s the same code doing the validation that is currently doing the parsing.

That said, '/tmp/st.sock' is a valid file path, single quotes and all, and we could have tried to just use it, but that would probably have been even more confusing. It’s just that right now we expect either unix:... or /... type strings for that purpose.

Honestly, I think it’s fine as is, even though I see you could get confused by the error as it was. This is often the thing with quotes in values, it’s unclear if they’re added by the error printer or part of the original value.

7

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.