review fixes; add submatch support to glob

conventional.yaml

@@ -89,10 +89,11 @@ server:
         preload: false
 
     websockets:
-        # sets the Origin headers that will be accepted for websocket connections.
-        # an empty list means any value (or no value) is allowed. the main use of this
-        # is to prevent malicious third-party Javascript from co-opting non-malicious
-        # clients (i.e., mainstream browsers) to DDoS your server.
+        # Restrict the origin of WebSocket connections by matching the "Origin" HTTP
+        # header. This settings makes oragono reject every WebSocket connection,
+        # except when it originates from one of the hosts in this list. Use this to
+        # prevent malicious websites from making their visitors connect to oragono
+        # without their knowledge. An empty list means that there are no restrictions.
         allowed-origins:
             # - "https://oragono.io"
             # - "https://*.oragono.io"

irc/client.go

@@ -277,7 +277,7 @@ func (server *Server) RunClient(conn IRCConn) {
 	if isBanned {
 		// this might not show up properly on some clients,
 		// but our objective here is just to close the connection out before it has a load impact on us
-		conn.Write([]byte(fmt.Sprintf(errorMsg, banMsg)))
+		conn.WriteLine([]byte(fmt.Sprintf(errorMsg, banMsg)))
 		conn.Close()
 		return
 	}

irc/config.go

@@ -772,7 +772,7 @@ func (conf *Config) prepareListeners() (err error) {
 	conf.Server.trueListeners = make(map[string]utils.ListenerConfig)
 	for addr, block := range conf.Server.Listeners {
 		var lconf utils.ListenerConfig
-		lconf.ProxyDeadline = time.Minute
+		lconf.ProxyDeadline = RegisterTimeout
 		lconf.Tor = block.Tor
 		lconf.STSOnly = block.STSOnly
 		if lconf.STSOnly && !conf.Server.STS.Enabled {
@@ -1217,20 +1217,19 @@ func (config *Config) Diff(oldConfig *Config) (addedCaps, removedCaps *caps.Set)
 }
 
 func compileGuestRegexp(guestFormat string, casemapping Casemapping) (standard, folded *regexp.Regexp, err error) {
+	if strings.Count(guestFormat, "?") != 0 || strings.Count(guestFormat, "*") != 1 {
+		err = errors.New("guest format must contain 1 '*' and no '?'s")
+		return
+	}
+
 	standard, err = utils.CompileGlob(guestFormat)
 	if err != nil {
 		return
 	}
 
 	starIndex := strings.IndexByte(guestFormat, '*')
-	if starIndex == -1 {
-		return nil, nil, errors.New("guest format must contain exactly one *")
-	}
 	initial := guestFormat[:starIndex]
 	final := guestFormat[starIndex+1:]
-	if strings.IndexByte(final, '*') != -1 {
-		return nil, nil, errors.New("guest format must contain exactly one *")
-	}
 	initialFolded, err := casefoldWithSetting(initial, casemapping)
 	if err != nil {
 		return

irc/ircconn.go

@@ -22,13 +22,16 @@ var (
 
 // IRCConn abstracts away the distinction between a regular
 // net.Conn (which includes both raw TCP and TLS) and a websocket.
-// it doesn't expose Read and Write because websockets are message-oriented,
-// not stream-oriented.
+// it doesn't expose the net.Conn, io.Reader, or io.Writer interfaces
+// because websockets are message-oriented, not stream-oriented, and
+// therefore this abstraction is message-oriented as well.
 type IRCConn interface {
-	UnderlyingConn() *utils.ProxiedConnection
+	UnderlyingConn() *utils.WrappedConn
 
-	Write([]byte) error
-	WriteBuffers([][]byte) error
+	// these take an IRC line or lines, correctly terminated with CRLF:
+	WriteLine([]byte) error
+	WriteLines([][]byte) error
+	// this returns an IRC line without the terminating CRLF:
 	ReadLine() (line []byte, err error)
 
 	Close() error
@@ -36,26 +39,26 @@ type IRCConn interface {
 
 // IRCStreamConn is an IRCConn over a regular stream connection.
 type IRCStreamConn struct {
-	conn   *utils.ProxiedConnection
+	conn   *utils.WrappedConn
 	reader *bufio.Reader
 }
 
-func NewIRCStreamConn(conn *utils.ProxiedConnection) *IRCStreamConn {
+func NewIRCStreamConn(conn *utils.WrappedConn) *IRCStreamConn {
 	return &IRCStreamConn{
 		conn: conn,
 	}
 }
 
-func (cc *IRCStreamConn) UnderlyingConn() *utils.ProxiedConnection {
+func (cc *IRCStreamConn) UnderlyingConn() *utils.WrappedConn {
 	return cc.conn
 }
 
-func (cc *IRCStreamConn) Write(buf []byte) (err error) {
+func (cc *IRCStreamConn) WriteLine(buf []byte) (err error) {
 	_, err = cc.conn.Write(buf)
 	return
 }
 
-func (cc *IRCStreamConn) WriteBuffers(buffers [][]byte) (err error) {
+func (cc *IRCStreamConn) WriteLines(buffers [][]byte) (err error) {
 	// on Linux, with a plaintext TCP or Unix domain socket,
 	// the Go runtime will optimize this into a single writev(2) call:
 	_, err = (*net.Buffers)(&buffers).WriteTo(cc.conn)
@@ -90,17 +93,13 @@ func NewIRCWSConn(conn *websocket.Conn) IRCWSConn {
 	return IRCWSConn{conn: conn}
 }
 
-func (wc IRCWSConn) UnderlyingConn() *utils.ProxiedConnection {
-	pConn, ok := wc.conn.UnderlyingConn().(*utils.ProxiedConnection)
-	if ok {
-		return pConn
-	} else {
-		// this can't happen
-		return nil
-	}
+func (wc IRCWSConn) UnderlyingConn() *utils.WrappedConn {
+	// just assume that the type is OK
+	wConn, _ := wc.conn.UnderlyingConn().(*utils.WrappedConn)
+	return wConn
 }
 
-func (wc IRCWSConn) Write(buf []byte) (err error) {
+func (wc IRCWSConn) WriteLine(buf []byte) (err error) {
 	buf = bytes.TrimSuffix(buf, crlf)
 	// there's not much we can do about this;
 	// silently drop the message
@@ -110,9 +109,9 @@ func (wc IRCWSConn) Write(buf []byte) (err error) {
 	return wc.conn.WriteMessage(websocket.TextMessage, buf)
 }
 
-func (wc IRCWSConn) WriteBuffers(buffers [][]byte) (err error) {
+func (wc IRCWSConn) WriteLines(buffers [][]byte) (err error) {
 	for _, buf := range buffers {
-		err = wc.Write(buf)
+		err = wc.WriteLine(buf)
 		if err != nil {
 			return
 		}

irc/listeners.go

@@ -89,7 +89,7 @@ func (nl *NetListener) Stop() error {
 }
 
 // ensure that any IP we got from the PROXY line is trustworthy (otherwise, clear it)
-func validateProxiedIP(conn *utils.ProxiedConnection, config *Config) {
+func validateProxiedIP(conn *utils.WrappedConn, config *Config) {
 	if !utils.IPInNets(utils.AddrToIP(conn.RemoteAddr()), config.Server.proxyAllowedFromNets) {
 		conn.ProxiedIP = nil
 	}
@@ -101,12 +101,12 @@ func (nl *NetListener) serve() {
 
 		if err == nil {
 			// hand off the connection
-			pConn, ok := conn.(*utils.ProxiedConnection)
+			wConn, ok := conn.(*utils.WrappedConn)
 			if ok {
-				if pConn.ProxiedIP != nil {
-					validateProxiedIP(pConn, nl.server.Config())
+				if wConn.ProxiedIP != nil {
+					validateProxiedIP(wConn, nl.server.Config())
 				}
-				go nl.server.RunClient(NewIRCStreamConn(pConn))
+				go nl.server.RunClient(NewIRCStreamConn(wConn))
 			} else {
 				nl.server.logger.Error("internal", "invalid connection type", nl.addr)
 			}
@@ -186,19 +186,19 @@ func (wl *WSListener) handle(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	pConn, ok := conn.UnderlyingConn().(*utils.ProxiedConnection)
+	wConn, ok := conn.UnderlyingConn().(*utils.WrappedConn)
 	if !ok {
 		wl.server.logger.Error("internal", "non-proxied connection on websocket", wl.addr)
 		conn.Close()
 		return
 	}
-	if pConn.ProxiedIP != nil {
-		validateProxiedIP(pConn, config)
+	if wConn.ProxiedIP != nil {
+		validateProxiedIP(wConn, config)
 	} else {
 		// if there was no PROXY protocol IP, use the validated X-Forwarded-For IP instead,
 		// unless it is redundant
-		if proxiedIP != nil && !proxiedIP.Equal(utils.AddrToIP(pConn.RemoteAddr())) {
-			pConn.ProxiedIP = proxiedIP
+		if proxiedIP != nil && !proxiedIP.Equal(utils.AddrToIP(wConn.RemoteAddr())) {
+			wConn.ProxiedIP = proxiedIP
 		}
 	}
 

irc/server.go

@@ -736,16 +736,16 @@ func (server *Server) setupListeners(config *Config) (err error) {
 		newConfig, stillConfigured := config.Server.trueListeners[addr]
 
 		if stillConfigured {
-			err := currentListener.Reload(newConfig)
+			reloadErr := currentListener.Reload(newConfig)
 			// attempt to stop and replace the listener if the reload failed
-			if err != nil {
+			if reloadErr != nil {
 				currentListener.Stop()
-				newListener, err := NewListener(server, addr, newConfig, config.Server.UnixBindMode)
-				if err != nil {
-					delete(server.listeners, addr)
-					return err
-				} else {
+				newListener, newErr := NewListener(server, addr, newConfig, config.Server.UnixBindMode)
+				if newErr == nil {
 					server.listeners[addr] = newListener
+				} else {
+					delete(server.listeners, addr)
+					return newErr
 				}
 			}
 			logListener(addr, newConfig)
@@ -765,13 +765,13 @@ func (server *Server) setupListeners(config *Config) (err error) {
 		_, exists := server.listeners[newAddr]
 		if !exists {
 			// make a new listener
-			newListener, listenerErr := NewListener(server, newAddr, newConfig, config.Server.UnixBindMode)
-			if listenerErr != nil {
-				server.logger.Error("server", "couldn't listen on", newAddr, listenerErr.Error())
-				err = listenerErr
-			} else {
+			newListener, newErr := NewListener(server, newAddr, newConfig, config.Server.UnixBindMode)
+			if newErr == nil {
 				server.listeners[newAddr] = newListener
 				logListener(newAddr, newConfig)
+			} else {
+				server.logger.Error("server", "couldn't listen on", newAddr, newErr.Error())
+				err = newErr
 			}
 		}
 	}

irc/socket.go

@@ -144,7 +144,7 @@ func (socket *Socket) BlockingWrite(data []byte) (err error) {
 		return io.EOF
 	}
 
-	err = socket.conn.Write(data)
+	err = socket.conn.WriteLine(data)
 	if err != nil {
 		socket.finalize()
 	}
@@ -216,7 +216,7 @@ func (socket *Socket) performWrite() (closed bool) {
 
 	var err error
 	if 0 < len(buffers) {
-		err = socket.conn.WriteBuffers(buffers)
+		err = socket.conn.WriteLines(buffers)
 	}
 
 	closed = closed || err != nil
@@ -244,7 +244,7 @@ func (socket *Socket) finalize() {
 	}
 
 	if len(finalData) != 0 {
-		socket.conn.Write(finalData)
+		socket.conn.WriteLine(finalData)
 	}
 
 	// close the connection

irc/utils/glob.go

@@ -6,7 +6,7 @@ package utils
 import (
 	"bytes"
 	"regexp"
-	"strings"
+	"regexp/syntax"
 )
 
 // yet another glob implementation in Go
@@ -14,15 +14,16 @@ import (
 func CompileGlob(glob string) (result *regexp.Regexp, err error) {
 	var buf bytes.Buffer
 	buf.WriteByte('^')
-	for {
-		i := strings.IndexByte(glob, '*')
-		if i == -1 {
-			buf.WriteString(regexp.QuoteMeta(glob))
-			break
-		} else {
-			buf.WriteString(regexp.QuoteMeta(glob[:i]))
-			buf.WriteString(".*")
-			glob = glob[i+1:]
+	for _, r := range glob {
+		switch r {
+		case '*':
+			buf.WriteString("(.*)")
+		case '?':
+			buf.WriteString("(.)")
+		case 0xFFFD:
+			return nil, &syntax.Error{Code: syntax.ErrInvalidUTF8, Expr: glob}
+		default:
+			buf.WriteString(regexp.QuoteMeta(string(r)))
 		}
 	}
 	buf.WriteByte('$')

irc/utils/glob_test.go

@@ -29,9 +29,20 @@ func TestGlob(t *testing.T) {
 	assertMatches("*://*.oragono.io", "https://testnet.oragono.io", true, t)
 	assertMatches("*://*.oragono.io", "https://oragono.io", false, t)
 	assertMatches("*://*.oragono.io", "https://githubusercontent.com", false, t)
+	assertMatches("*://*.oragono.io", "https://testnet.oragono.io.example.com", false, t)
 
 	assertMatches("", "", true, t)
 	assertMatches("", "x", false, t)
 	assertMatches("*", "", true, t)
 	assertMatches("*", "x", true, t)
+
+	assertMatches("c?b", "cab", true, t)
+	assertMatches("c?b", "cub", true, t)
+	assertMatches("c?b", "cb", false, t)
+	assertMatches("c?b", "cube", false, t)
+	assertMatches("?*", "cube", true, t)
+	assertMatches("?*", "", false, t)
+
+	assertMatches("S*e", "Skåne", true, t)
+	assertMatches("Sk?ne", "Skåne", true, t)
 }

irc/utils/net_test.go

@@ -190,8 +190,18 @@ func TestXForwardedFor(t *testing.T) {
 	checkXFF("10.0.0.4:28432", "10.0.0.3", "10.0.0.3", t)
 
 	checkXFF("10.0.0.4:28432", "1.1.1.1, 8.8.4.4", "8.8.4.4", t)
+	checkXFF("10.0.0.4:28432", "1.1.1.1,8.8.4.4", "8.8.4.4", t)
 	checkXFF("10.0.0.4:28432", "8.8.4.4, 1.1.1.1, 10.0.0.3", "1.1.1.1", t)
 	checkXFF("10.0.0.4:28432", "10.0.0.1, 10.0.0.2, 10.0.0.3", "10.0.0.1", t)
+	checkXFF("10.0.0.4:28432", "10.0.0.1, 10.0.0.2,10.0.0.3", "10.0.0.1", t)
 
 	checkXFF("@", "8.8.4.4, 1.1.1.1, 10.0.0.3", "1.1.1.1", t)
+
+	// invalid IP tests:
+	checkXFF("8.8.4.4:9999", "not_an_ip", "8.8.4.4", t)
+	checkXFF("10.0.0.4:28432", "not_an_ip", "10.0.0.4", t)
+	checkXFF("10.0.0.4:28432", "not_an_ip, 1.1.1.1, 10.0.0.3", "1.1.1.1", t)
+
+	checkXFF("10.0.0.4:28432", "8.8.4.4, not_an_ip, 10.0.0.3", "10.0.0.3", t)
+	checkXFF("10.0.0.4:28432", "8.8.4.4, not_an_ip", "10.0.0.4", t)
 }

irc/utils/proxy.go

@@ -86,10 +86,10 @@ func ParseProxyLine(line string) (ip net.IP, err error) {
 	return ip.To16(), nil
 }
 
-/// ProxiedConnection is a net.Conn with some additional data stapled to it;
+/// WrappedConn is a net.Conn with some additional data stapled to it;
 // the proxied IP, if one was read via the PROXY protocol, and the listener
 // configuration.
-type ProxiedConnection struct {
+type WrappedConn struct {
 	net.Conn
 	ProxiedIP net.IP
 	Config    ListenerConfig
@@ -154,7 +154,7 @@ func (rl *ReloadableListener) Accept() (conn net.Conn, err error) {
 		conn = tls.Server(conn, config.TLSConfig)
 	}
 
-	return &ProxiedConnection{
+	return &WrappedConn{
 		Conn:      conn,
 		ProxiedIP: proxiedIP,
 		Config:    config,

oragono.yaml

@@ -110,10 +110,11 @@ server:
         preload: false
 
     websockets:
-        # sets the Origin headers that will be accepted for websocket connections.
-        # an empty list means any value (or no value) is allowed. the main use of this
-        # is to prevent malicious third-party Javascript from co-opting non-malicious
-        # clients (i.e., mainstream browsers) to DDoS your server.
+        # Restrict the origin of WebSocket connections by matching the "Origin" HTTP
+        # header. This settings makes oragono reject every WebSocket connection,
+        # except when it originates from one of the hosts in this list. Use this to
+        # prevent malicious websites from making their visitors connect to oragono
+        # without their knowledge. An empty list means that there are no restrictions.
         allowed-origins:
             # - "https://oragono.io"
             # - "https://*.oragono.io"