Merge pull request #1586 from slingamn/issue1577_no_truncation.2

deprecate message truncation

default.yaml

@@ -235,6 +235,13 @@ server:
         # this works around that bug, allowing them to use SASL.
         send-unprefixed-sasl: true
 
+        # traditionally, IRC servers will truncate and send messages that are
+        # too long to be relayed intact. this behavior can be disabled by setting
+        # allow-truncation to false, in which case Oragono will reject the message
+        # and return an error to the client. (note that this option defaults to true
+        # when unset.)
+        allow-truncation: false
+
     # IP-based DoS protection
     ip-limits:
         # whether to limit the total number of concurrent connections per IP/CIDR

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/go-sql-driver/mysql v1.5.0
 	github.com/go-test/deep v1.0.6 // indirect
 	github.com/gorilla/websocket v1.4.2
-	github.com/goshuirc/irc-go v0.0.0-20210301225436-2c4b83d64847
+	github.com/goshuirc/irc-go v0.0.0-20210304031553-cf78e9176f96
 	github.com/onsi/ginkgo v1.12.0 // indirect
 	github.com/onsi/gomega v1.9.0 // indirect
 	github.com/oragono/confusables v0.0.0-20201108231250-4ab98ab61fb1
@@ -24,4 +24,4 @@ require (
 	gopkg.in/yaml.v2 v2.3.0
 )
 
-replace github.com/gorilla/websocket  => github.com/oragono/websocket v1.4.2-oragono1
+replace github.com/gorilla/websocket => github.com/oragono/websocket v1.4.2-oragono1

go.sum

@@ -40,6 +40,8 @@ github.com/goshuirc/irc-go v0.0.0-20210223005429-8d38e43fc6ed h1:cwwqHrmLafgEucS
 github.com/goshuirc/irc-go v0.0.0-20210223005429-8d38e43fc6ed/go.mod h1:q/JhvvKLmif3y9q8MDQM+gRCnjEKnu5ClF298TTXJug=
 github.com/goshuirc/irc-go v0.0.0-20210301225436-2c4b83d64847 h1:MmsZRpAsMxyw0P5/SFn2L6edhmIXRlolgXvOF+fgEiQ=
 github.com/goshuirc/irc-go v0.0.0-20210301225436-2c4b83d64847/go.mod h1:q/JhvvKLmif3y9q8MDQM+gRCnjEKnu5ClF298TTXJug=
+github.com/goshuirc/irc-go v0.0.0-20210304031553-cf78e9176f96 h1:sihI3HsrJWyS4MtBmxh5W4gDZD34SWodkWyUvJltswY=
+github.com/goshuirc/irc-go v0.0.0-20210304031553-cf78e9176f96/go.mod h1:q/JhvvKLmif3y9q8MDQM+gRCnjEKnu5ClF298TTXJug=
 github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
 github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
 github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=

irc/channel.go

@@ -1361,6 +1361,15 @@ func (channel *Channel) SendSplitMessage(command string, minPrefixMode modes.Mod
 	details := client.Details()
 	chname := channel.Name()
 
+	if !client.server.Config().Server.Compatibility.allowTruncation {
+		if !validateSplitMessageLen(histType, details.nickMask, chname, message) {
+			rb.Add(nil, client.server.name, ERR_INPUTTOOLONG, details.nick, client.t("Line too long to be relayed without truncation"))
+			// TODO(#1577) remove this logline:
+			client.server.logger.Debug("internal", "rejected truncation-requiring DM from client", details.nick)
+			return
+		}
+	}
+
 	// STATUSMSG targets are prefixed with the supplied min-prefix, e.g., @#channel
 	if minPrefixMode != modes.Mode(0) {
 		chname = fmt.Sprintf("%s%s", modes.ChannelModePrefixes[minPrefixMode], chname)

irc/client.go

@@ -740,8 +740,13 @@ func (client *Client) run(session *Session) {
 		msg, err := ircmsg.ParseLineStrict(line, true, MaxLineLen)
 		if err == ircmsg.ErrorLineIsEmpty {
 			continue
-		} else if err == ircmsg.ErrorLineTooLong {
+		} else if err == ircmsg.ErrorTagsTooLong {
+			session.Send(nil, client.server.name, ERR_INPUTTOOLONG, client.Nick(), client.t("Input line contained excess tag data"))
+			continue
+		} else if err == ircmsg.ErrorBodyTooLong && !client.server.Config().Server.Compatibility.allowTruncation {
 			session.Send(nil, client.server.name, ERR_INPUTTOOLONG, client.Nick(), client.t("Input line too long"))
+			// TODO(#1577) remove this logline:
+			client.server.logger.Debug("internal", "rejected MaxLineLen-exceeding line from client", client.Nick())
 			continue
 		} else if err != nil {
 			client.Quit(client.t("Received malformed line"), session)
@@ -1711,7 +1716,7 @@ func (session *Session) SendRawMessage(message ircmsg.IRCMessage, blocking bool)
 
 	// assemble message
 	line, err := message.LineBytesStrict(false, MaxLineLen)
-	if err != nil {
+	if !(err == nil || err == ircmsg.ErrorBodyTooLong) {
 		errorParams := []string{"Error assembling message for sending", err.Error(), message.Command}
 		errorParams = append(errorParams, message.Params...)
 		session.client.server.logger.Error("internal", errorParams...)

irc/config.go

@@ -569,7 +569,9 @@ type Config struct {
 		Compatibility        struct {
 			ForceTrailing      *bool `yaml:"force-trailing"`
 			forceTrailing      bool
-			SendUnprefixedSasl bool `yaml:"send-unprefixed-sasl"`
+			SendUnprefixedSasl bool  `yaml:"send-unprefixed-sasl"`
+			AllowTruncation    *bool `yaml:"allow-truncation"`
+			allowTruncation    bool
 		}
 		isupport                 isupport.List
 		IPLimits                 connection_limits.LimiterConfig `yaml:"ip-limits"`
@@ -1378,6 +1380,7 @@ func LoadConfig(filename string) (config *Config, err error) {
 	}
 
 	config.Server.Compatibility.forceTrailing = utils.BoolDefaultTrue(config.Server.Compatibility.ForceTrailing)
+	config.Server.Compatibility.allowTruncation = utils.BoolDefaultTrue(config.Server.Compatibility.AllowTruncation)
 
 	config.loadMOTD()
 

irc/handlers.go

@@ -1981,6 +1981,43 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IRCMessage, rb *Resp
 	return false
 }
 
+// check whether a PRIVMSG or NOTICE is too long to be relayed without truncation
+func validateLineLen(msgType history.ItemType, source, target, payload string) (ok bool) {
+	// :source PRIVMSG #target :payload\r\n
+	// 1: initial colon on prefix
+	// 1: space between prefix and command
+	// 1: space between command and target (first parameter)
+	// 1: space between target and payload (second parameter)
+	// 1: colon to send the payload as a trailing (we force trailing for PRIVMSG and NOTICE)
+	// 2: final \r\n
+	limit := MaxLineLen - 7
+	limit -= len(source)
+	switch msgType {
+	case history.Privmsg:
+		limit -= 7
+	case history.Notice:
+		limit -= 6
+	default:
+		return true
+	}
+	limit -= len(payload)
+	return limit >= 0
+}
+
+// check validateLineLen for an entire SplitMessage (which may consist of multiple lines)
+func validateSplitMessageLen(msgType history.ItemType, source, target string, message utils.SplitMessage) (ok bool) {
+	if message.Is512() {
+		return validateLineLen(msgType, source, target, message.Message)
+	} else {
+		for _, messagePair := range message.Split {
+			if !validateLineLen(msgType, source, target, messagePair.Message) {
+				return false
+			}
+		}
+		return true
+	}
+}
+
 // helper to store a batched PRIVMSG in the session object
 func absorbBatchedMessage(server *Server, client *Client, msg ircmsg.IRCMessage, batchTag string, histType history.ItemType, rb *ResponseBuffer) {
 	var errorCode, errorMessage string
@@ -2033,7 +2070,6 @@ func messageHandler(server *Server, client *Client, msg ircmsg.IRCMessage, rb *R
 		return false
 	}
 
-	cnick := client.Nick()
 	clientOnlyTags := msg.ClientOnlyTags()
 	if histType == history.Tagmsg && len(clientOnlyTags) == 0 {
 		// nothing to do
@@ -2046,7 +2082,7 @@ func messageHandler(server *Server, client *Client, msg ircmsg.IRCMessage, rb *R
 		message = msg.Params[1]
 	}
 	if histType != history.Tagmsg && message == "" {
-		rb.Add(nil, server.name, ERR_NOTEXTTOSEND, cnick, client.t("No text to send"))
+		rb.Add(nil, server.name, ERR_NOTEXTTOSEND, client.Nick(), client.t("No text to send"))
 		return false
 	}
 
@@ -2147,6 +2183,14 @@ func dispatchMessageToTarget(client *Client, tags map[string]string, histType hi
 			rb.Add(nil, server.name, ERR_NEEDREGGEDNICK, client.Nick(), tnick, client.t("You must be registered to send a direct message to this user"))
 			return
 		}
+		if !client.server.Config().Server.Compatibility.allowTruncation {
+			if !validateSplitMessageLen(histType, client.NickMaskString(), tnick, message) {
+				rb.Add(nil, server.name, ERR_INPUTTOOLONG, client.Nick(), client.t("Line too long to be relayed without truncation"))
+				// TODO(#1577) remove this logline:
+				client.server.logger.Debug("internal", "rejected truncation-requiring channel message from client", details.nick)
+				return
+			}
+		}
 		nickMaskString := details.nickMask
 		accountName := details.accountName
 		var deliverySessions []*Session

irc/message_cache.go

@@ -54,7 +54,7 @@ func addAllTags(msg *ircmsg.IRCMessage, tags map[string]string, serverTime time.
 }
 
 func (m *MessageCache) handleErr(server *Server, err error) bool {
-	if err != nil {
+	if !(err == nil || err == ircmsg.ErrorBodyTooLong) {
 		server.logger.Error("internal", "Error assembling message for sending", err.Error())
 		// blank these out so Send will be a no-op
 		m.fullTags = nil

traditional.yaml

@@ -208,6 +208,13 @@ server:
         # this works around that bug, allowing them to use SASL.
         send-unprefixed-sasl: true
 
+        # traditionally, IRC servers will truncate and send messages that are
+        # too long to be relayed intact. this behavior can be disabled by setting
+        # allow-truncation to false, in which case Oragono will reject the message
+        # and return an error to the client. (note that this option defaults to true
+        # when unset.)
+        allow-truncation: true
+
     # IP-based DoS protection
     ip-limits:
         # whether to limit the total number of concurrent connections per IP/CIDR

vendor/github.com/goshuirc/irc-go/ircmsg/message.go

@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"errors"
 	"strings"
+	"unicode/utf8"
 )
 
 const (
@@ -34,17 +35,30 @@ const (
 var (
 	// ErrorLineIsEmpty indicates that the given IRC line was empty.
 	ErrorLineIsEmpty = errors.New("Line is empty")
+
 	// ErrorLineContainsBadChar indicates that the line contained invalid characters
 	ErrorLineContainsBadChar = errors.New("Line contains invalid characters")
-	// ErrorLineTooLong indicates that the message exceeded the maximum tag length
-	// (the name references 417 ERR_INPUTTOOLONG; we reserve the right to return it
-	// for messages that exceed the non-tag length limit)
-	ErrorLineTooLong = errors.New("Line could not be parsed because a specified length limit was exceeded")
+
+	// ErrorBodyTooLong indicates that the message body exceeded the specified
+	// length limit (typically 512 bytes). This error is non-fatal; if encountered
+	// when parsing a message, the message is parsed up to the length limit, and
+	// if encountered when serializing a message, the message is truncated to the limit.
+	ErrorBodyTooLong = errors.New("Line body exceeded the specified length limit; outgoing messages will be truncated")
+
+	// ErrorTagsTooLong indicates that the message exceeded the maximum tag length
+	// (the specified response on the server side is 417 ERR_INPUTTOOLONG).
+	ErrorTagsTooLong = errors.New("Line could not be processed because its tag data exceeded the length limit")
+
 	// ErrorInvalidTagContent indicates that a tag name or value was invalid
 	ErrorInvalidTagContent = errors.New("Line could not be processed because it contained an invalid tag name or value")
 
+	// ErrorCommandMissing indicates that an IRC message was invalid because it lacked a command.
 	ErrorCommandMissing = errors.New("IRC messages MUST have a command")
-	ErrorBadParam       = errors.New("Cannot have an empty param, a param with spaces, or a param that starts with ':' before the last parameter")
+
+	// ErrorBadParam indicates that an IRC message could not be serialized because
+	// its parameters violated the syntactic constraints on IRC parameters:
+	// non-final parameters cannot be empty, contain a space, or start with `:`.
+	ErrorBadParam = errors.New("Cannot have an empty param, a param with spaces, or a param that starts with ':' before the last parameter")
 )
 
 // IRCMessage represents an IRC message, as defined by the RFCs and as
@@ -161,7 +175,7 @@ func ParseLineStrict(line string, fromClient bool, truncateLen int) (ircmsg IRCM
 // slice off any amount of ' ' from the front of the string
 func trimInitialSpaces(str string) string {
 	var i int
-	for i = 0; i < len(str) && str[i] == ' '; i += 1 {
+	for i = 0; i < len(str) && str[i] == ' '; i++ {
 	}
 	return str[i:]
 }
@@ -170,6 +184,14 @@ func parseLine(line string, maxTagDataLength int, truncateLen int) (ircmsg IRCMe
 	// remove either \n or \r\n from the end of the line:
 	line = strings.TrimSuffix(line, "\n")
 	line = strings.TrimSuffix(line, "\r")
+	// whether we removed them ourselves, or whether they were removed previously,
+	// they count against the line limit:
+	if truncateLen != 0 {
+		if truncateLen <= 2 {
+			return ircmsg, ErrorLineIsEmpty
+		}
+		truncateLen -= 2
+	}
 	// now validate for the 3 forbidden bytes:
 	if strings.IndexByte(line, '\x00') != -1 || strings.IndexByte(line, '\n') != -1 || strings.IndexByte(line, '\r') != -1 {
 		return ircmsg, ErrorLineContainsBadChar
@@ -187,7 +209,7 @@ func parseLine(line string, maxTagDataLength int, truncateLen int) (ircmsg IRCMe
 		}
 		tags := line[1:tagEnd]
 		if 0 < maxTagDataLength && maxTagDataLength < len(tags) {
-			return ircmsg, ErrorLineTooLong
+			return ircmsg, ErrorTagsTooLong
 		}
 		err = ircmsg.parseTags(tags)
 		if err != nil {
@@ -198,7 +220,8 @@ func parseLine(line string, maxTagDataLength int, truncateLen int) (ircmsg IRCMe
 	}
 
 	// truncate if desired
-	if 0 < truncateLen && truncateLen < len(line) {
+	if truncateLen != 0 && truncateLen < len(line) {
+		err = ErrorBodyTooLong
 		line = line[:truncateLen]
 	}
 
@@ -252,7 +275,7 @@ func parseLine(line string, maxTagDataLength int, truncateLen int) (ircmsg IRCMe
 		line = line[paramEnd+1:]
 	}
 
-	return ircmsg, nil
+	return ircmsg, err
 }
 
 // helper to parse tags
@@ -337,8 +360,8 @@ func paramRequiresTrailing(param string) bool {
 }
 
 // line returns a sendable line created from an IRCMessage.
-func (ircmsg *IRCMessage) line(tagLimit, clientOnlyTagDataLimit, serverAddedTagDataLimit, truncateLen int) ([]byte, error) {
-	if len(ircmsg.Command) < 1 {
+func (ircmsg *IRCMessage) line(tagLimit, clientOnlyTagDataLimit, serverAddedTagDataLimit, truncateLen int) (result []byte, err error) {
+	if len(ircmsg.Command) == 0 {
 		return nil, ErrorCommandMissing
 	}
 
@@ -382,10 +405,10 @@ func (ircmsg *IRCMessage) line(tagLimit, clientOnlyTagDataLimit, serverAddedTagD
 	lenTags = buf.Len()
 
 	if 0 < tagLimit && tagLimit < buf.Len() {
-		return nil, ErrorLineTooLong
+		return nil, ErrorTagsTooLong
 	}
 	if (0 < clientOnlyTagDataLimit && clientOnlyTagDataLimit < lenClientOnlyTags) || (0 < serverAddedTagDataLimit && serverAddedTagDataLimit < lenRegularTags) {
-		return nil, ErrorLineTooLong
+		return nil, ErrorTagsTooLong
 	}
 
 	if len(ircmsg.Prefix) > 0 {
@@ -408,18 +431,33 @@ func (ircmsg *IRCMessage) line(tagLimit, clientOnlyTagDataLimit, serverAddedTagD
 		buf.WriteString(param)
 	}
 
-	// truncate if desired
-	// -2 for \r\n
-	restLen := buf.Len() - lenTags
-	if 0 < truncateLen && (truncateLen-2) < restLen {
-		buf.Truncate(lenTags + (truncateLen - 2))
+	// truncate if desired; leave 2 bytes over for \r\n:
+	if truncateLen != 0 && (truncateLen-2) < (buf.Len()-lenTags) {
+		err = ErrorBodyTooLong
+		newBufLen := lenTags + (truncateLen - 2)
+		buf.Truncate(newBufLen)
+		// XXX: we may have truncated in the middle of a UTF8-encoded codepoint;
+		// if so, remove additional bytes, stopping when the sequence either
+		// ends in a valid codepoint, or we have removed 3 bytes (the maximum
+		// length of the remnant of a once-valid, truncated codepoint; we don't
+		// want to truncate the entire message if it wasn't UTF8 in the first
+		// place).
+		for i := 0; i < (utf8.UTFMax - 1); i++ {
+			r, n := utf8.DecodeLastRune(buf.Bytes())
+			if r == utf8.RuneError && n <= 1 {
+				newBufLen--
+				buf.Truncate(newBufLen)
+			} else {
+				break
+			}
+		}
 	}
 	buf.WriteString("\r\n")
 
-	result := buf.Bytes()
+	result = buf.Bytes()
 	toValidate := result[:len(result)-2]
 	if bytes.IndexByte(toValidate, '\x00') != -1 || bytes.IndexByte(toValidate, '\r') != -1 || bytes.IndexByte(toValidate, '\n') != -1 {
 		return nil, ErrorLineContainsBadChar
 	}
-	return result, nil
+	return result, err
 }

vendor/github.com/goshuirc/irc-go/ircreader/ircreader.go

@@ -63,6 +63,10 @@ func (cc *IRCReader) ReadLine() ([]byte, error) {
 			line := cc.buf[cc.start : cc.searchFrom+nlidx]
 			cc.start = cc.searchFrom + nlidx + 1
 			cc.searchFrom = cc.start
+			// treat \r\n as the line terminator if it was present
+			if 0 < len(line) && line[len(line)-1] == '\r' {
+				line = line[:len(line)-1]
+			}
 			return line, nil
 		}
 

vendor/modules.txt

@@ -21,7 +21,7 @@ github.com/go-sql-driver/mysql
 # github.com/gorilla/websocket v1.4.2 => github.com/oragono/websocket v1.4.2-oragono1
 ## explicit
 github.com/gorilla/websocket
-# github.com/goshuirc/irc-go v0.0.0-20210301225436-2c4b83d64847
+# github.com/goshuirc/irc-go v0.0.0-20210304031553-cf78e9176f96
 ## explicit
 github.com/goshuirc/irc-go/ircfmt
 github.com/goshuirc/irc-go/ircmsg