fix #1676, take 2

Ensure the pagination window is full by making sure that every history item
gets a replay line in CHATHISTORY output, even TAGMSG.

default.yaml

@@ -962,7 +962,7 @@ history:
         # if `default` is false, store TAGMSG containing any of these tags:
         whitelist:
             - "+draft/react"
-            - "react"
+            - "+react"
 
         # if `default` is true, don't store TAGMSG containing any of these tags:
         #blacklist:

irc/channel.go

@@ -952,7 +952,7 @@ func (channel *Channel) autoReplayHistory(client *Client, rb *ResponseBuffer, sk
 		}
 	}
 	if 0 < numItems {
-		channel.replayHistoryItems(rb, items, true)
+		channel.replayHistoryItems(rb, items, false)
 		rb.Flush(true)
 	}
 }
@@ -1035,7 +1035,7 @@ func (channel *Channel) Part(client *Client, message string, rb *ResponseBuffer)
 	client.server.logger.Debug("channels", fmt.Sprintf("%s left channel %s", details.nick, chname))
 }
 
-func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.Item, autoreplay bool) {
+func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.Item, chathistoryCommand bool) {
 	// send an empty batch if necessary, as per the CHATHISTORY spec
 	chname := channel.Name()
 	client := rb.target
@@ -1043,13 +1043,15 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 	extendedJoin := rb.session.capabilities.Has(caps.ExtendedJoin)
 	var playJoinsAsPrivmsg bool
 	if !eventPlayback {
-		switch client.AccountSettings().ReplayJoins {
-		case ReplayJoinsCommandsOnly:
-			playJoinsAsPrivmsg = !autoreplay
-		case ReplayJoinsAlways:
+		if chathistoryCommand {
 			playJoinsAsPrivmsg = true
-		case ReplayJoinsNever:
-			playJoinsAsPrivmsg = false
+		} else {
+			switch client.AccountSettings().ReplayJoins {
+			case ReplayJoinsCommandsOnly:
+				playJoinsAsPrivmsg = false
+			case ReplayJoinsAlways:
+				playJoinsAsPrivmsg = true
+			}
 		}
 	}
 
@@ -1066,6 +1068,9 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 		case history.Tagmsg:
 			if eventPlayback {
 				rb.AddSplitMessageFromClient(item.Nick, item.AccountName, item.IsBot, item.Tags, "TAGMSG", chname, item.Message)
+			} else if chathistoryCommand {
+				// #1676, we have to send something here or else it breaks pagination
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, fmt.Sprintf(client.t("%s sent a TAGMSG"), nick))
 			}
 		case history.Join:
 			if eventPlayback {
@@ -1084,7 +1089,7 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 				} else {
 					message = fmt.Sprintf(client.t("%[1]s [account: %[2]s] joined the channel"), nick, item.AccountName)
 				}
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Part:
 			if eventPlayback {
@@ -1094,14 +1099,14 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 					continue // #474
 				}
 				message := fmt.Sprintf(client.t("%[1]s left the channel (%[2]s)"), nick, item.Message.Message)
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Kick:
 			if eventPlayback {
 				rb.AddFromClient(item.Message.Time, item.Message.Msgid, item.Nick, item.AccountName, item.IsBot, nil, "KICK", chname, item.Params[0], item.Message.Message)
 			} else {
 				message := fmt.Sprintf(client.t("%[1]s kicked %[2]s (%[3]s)"), nick, item.Params[0], item.Message.Message)
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Quit:
 			if eventPlayback {
@@ -1111,21 +1116,21 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 					continue // #474
 				}
 				message := fmt.Sprintf(client.t("%[1]s quit (%[2]s)"), nick, item.Message.Message)
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Nick:
 			if eventPlayback {
 				rb.AddFromClient(item.Message.Time, item.Message.Msgid, item.Nick, item.AccountName, item.IsBot, nil, "NICK", item.Params[0])
 			} else {
 				message := fmt.Sprintf(client.t("%[1]s changed nick to %[2]s"), nick, item.Params[0])
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Topic:
 			if eventPlayback {
 				rb.AddFromClient(item.Message.Time, item.Message.Msgid, item.Nick, item.AccountName, item.IsBot, nil, "TOPIC", chname, item.Message.Message)
 			} else {
 				message := fmt.Sprintf(client.t("%[1]s set the channel topic to: %[2]s"), nick, item.Message.Message)
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		case history.Mode:
 			params := make([]string, len(item.Message.Split)+1)
@@ -1137,7 +1142,7 @@ func (channel *Channel) replayHistoryItems(rb *ResponseBuffer, items []history.I
 				rb.AddFromClient(item.Message.Time, item.Message.Msgid, item.Nick, item.AccountName, item.IsBot, nil, "MODE", params...)
 			} else {
 				message := fmt.Sprintf(client.t("%[1]s set channel modes: %[2]s"), nick, strings.Join(params[1:], " "))
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", chname, message)
 			}
 		}
 	}

irc/client.go

@@ -853,7 +853,7 @@ func (session *Session) Ping() {
 	session.Send(nil, "", "PING", session.client.Nick())
 }
 
-func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.Item, target string) {
+func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.Item, target string, chathistoryCommand bool) {
 	var batchID string
 	details := client.Details()
 	nick := details.nick
@@ -883,7 +883,7 @@ func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.I
 			if hasEventPlayback {
 				rb.AddFromClient(item.Message.Time, item.Message.Msgid, item.Nick, item.AccountName, item.IsBot, nil, "INVITE", nick, item.Message.Message)
 			} else {
-				rb.AddFromClient(item.Message.Time, utils.MungeSecretToken(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", fmt.Sprintf(client.t("%[1]s invited you to channel %[2]s"), NUHToNick(item.Nick), item.Message.Message))
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", fmt.Sprintf(client.t("%[1]s invited you to channel %[2]s"), NUHToNick(item.Nick), item.Message.Message))
 			}
 			continue
 		case history.Privmsg:
@@ -893,10 +893,15 @@ func (client *Client) replayPrivmsgHistory(rb *ResponseBuffer, items []history.I
 		case history.Tagmsg:
 			if hasEventPlayback && hasTags {
 				command = "TAGMSG"
+			} else if chathistoryCommand {
+				// #1676: send something for TAGMSG; we can't discard it entirely
+				// because it'll break pagination
+				rb.AddFromClient(item.Message.Time, history.HistservMungeMsgid(item.Message.Msgid), histservService.prefix, "*", false, nil, "PRIVMSG", fmt.Sprintf(client.t("%[1]s sent you a TAGMSG"), NUHToNick(item.Nick)))
 			} else {
 				continue
 			}
 		default:
+			// see #1676, this shouldn't happen
 			continue
 		}
 		var tags map[string]string

irc/handlers.go

@@ -611,9 +611,9 @@ func chathistoryHandler(server *Server, client *Client, msg ircmsg.Message, rb *
 						target.Time.Format(IRCv3TimestampFormat))
 				}
 			} else if channel != nil {
-				channel.replayHistoryItems(rb, items, false)
+				channel.replayHistoryItems(rb, items, true)
 			} else {
-				client.replayPrivmsgHistory(rb, items, target)
+				client.replayPrivmsgHistory(rb, items, target, true)
 			}
 		}
 	}()
@@ -640,7 +640,7 @@ func chathistoryHandler(server *Server, client *Client, msg ircmsg.Message, rb *
 		}
 		identifier, value := strings.ToLower(pieces[0]), pieces[1]
 		if identifier == "msgid" {
-			msgid, err = value, nil
+			msgid, err = history.NormalizeMsgid(value), nil
 			return
 		} else if identifier == "timestamp" {
 			timestamp, err = time.Parse(IRCv3TimestampFormat, value)
@@ -1122,9 +1122,9 @@ func historyHandler(server *Server, client *Client, msg ircmsg.Message, rb *Resp
 
 	if len(items) != 0 {
 		if channel != nil {
-			channel.replayHistoryItems(rb, items, false)
+			channel.replayHistoryItems(rb, items, true)
 		} else {
-			client.replayPrivmsgHistory(rb, items, "")
+			client.replayPrivmsgHistory(rb, items, "", true)
 		}
 	}
 	return false

irc/history/queries.go

@@ -4,6 +4,7 @@
 package history
 
 import (
+	"strings"
 	"time"
 )
 
@@ -77,3 +78,16 @@ func MinMaxAsc(after, before, cutoff time.Time) (min, max time.Time, ascending b
 	}
 	return after, before, ascending
 }
+
+// maps regular msgids from JOIN, etc. to a msgid suitable for attaching
+// to a HistServ message describing the JOIN. See #491 for some history.
+func HistservMungeMsgid(msgid string) string {
+	return "_" + msgid
+}
+
+// strips munging from a msgid. future schemes may not support a well-defined
+// mapping of munged msgids to true msgids, but munged msgids should always contain
+// a _, with metadata in front and data (possibly the true msgid) after.
+func NormalizeMsgid(msgid string) string {
+	return strings.TrimPrefix(msgid, "_")
+}

irc/utils/crypto.go

@@ -42,29 +42,6 @@ func GenerateSecretToken() string {
 	return B32Encoder.EncodeToString(buf[:])
 }
 
-// "munge" a secret token to a new value. requirements:
-// 1. MUST be roughly as unlikely to collide with `GenerateSecretToken` outputs
-// as those outputs are with each other
-// 2. SHOULD be deterministic (motivation: if a JOIN line has msgid x,
-// create a deterministic msgid y for the fake HistServ PRIVMSG that "replays" it)
-// 3. SHOULD be in the same "namespace" as `GenerateSecretToken` outputs
-// (same length and character set)
-func MungeSecretToken(token string) (result string) {
-	bytes, err := B32Encoder.DecodeString(token)
-	if err != nil {
-		// this should never happen
-		return GenerateSecretToken()
-	}
-	// add 1 with carrying
-	for i := len(bytes) - 1; 0 <= i; i -= 1 {
-		bytes[i] += 1
-		if bytes[i] != 0 {
-			break
-		} // else: overflow, carry to the next place
-	}
-	return B32Encoder.EncodeToString(bytes)
-}
-
 // securely check if a supplied token matches a stored token
 func SecretTokensMatch(storedToken string, suppliedToken string) bool {
 	// XXX fix a potential gotcha: if the stored token is uninitialized,

irc/utils/crypto_test.go

@@ -47,41 +47,12 @@ func TestTokenCompare(t *testing.T) {
 	}
 }
 
-func TestMunging(t *testing.T) {
-	count := 131072
-	set := make(map[string]bool)
-	var token string
-	for i := 0; i < count; i++ {
-		token = GenerateSecretToken()
-		set[token] = true
-	}
-	// all tokens generated thus far should be unique
-	assertEqual(len(set), count, t)
-
-	// iteratively munge the last generated token an additional `count` times
-	mungedToken := token
-	for i := 0; i < count; i++ {
-		mungedToken = MungeSecretToken(mungedToken)
-		assertEqual(len(mungedToken), len(token), t)
-		set[mungedToken] = true
-	}
-	// munged tokens should not collide with generated tokens, or each other
-	assertEqual(len(set), count*2, t)
-}
-
 func BenchmarkGenerateSecretToken(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		GenerateSecretToken()
 	}
 }
 
-func BenchmarkMungeSecretToken(b *testing.B) {
-	t := GenerateSecretToken()
-	for i := 0; i < b.N; i++ {
-		t = MungeSecretToken(t)
-	}
-}
-
 func TestCertfpComparisons(t *testing.T) {
 	opensslFP := "3D:6B:11:BF:B4:05:C3:F8:4B:38:CD:30:38:FB:EC:01:71:D5:03:54:79:04:07:88:4C:A5:5D:23:41:85:66:C9"
 	oragonoFP := "3d6b11bfb405c3f84b38cd3038fbec0171d50354790407884ca55d23418566c9"

irc/znc.go

@@ -196,7 +196,7 @@ func zncPlayPrivmsgsFrom(client *Client, rb *ResponseBuffer, target string, star
 	zncMax := client.server.Config().History.ZNCMax
 	items, err := sequence.Between(history.Selector{Time: start}, history.Selector{Time: end}, zncMax)
 	if err == nil && len(items) != 0 {
-		client.replayPrivmsgHistory(rb, items, target)
+		client.replayPrivmsgHistory(rb, items, target, false)
 	}
 }
 
@@ -204,7 +204,7 @@ func zncPlayPrivmsgsFromAll(client *Client, rb *ResponseBuffer, start, end time.
 	zncMax := client.server.Config().History.ZNCMax
 	items, err := client.privmsgsBetween(start, end, maxDMTargetsForAutoplay, zncMax)
 	if err == nil && len(items) != 0 {
-		client.replayPrivmsgHistory(rb, items, "")
+		client.replayPrivmsgHistory(rb, items, "", false)
 	}
 }
 

irctest

@@ -1 +1 @@
-Subproject commit 33f0702c260ea716a4ad0f24821a50d44c91fca1
+Subproject commit 5e4ae7c99965801cd91d974637ad344a47b5414f

traditional.yaml

@@ -935,7 +935,7 @@ history:
         # if `default` is false, store TAGMSG containing any of these tags:
         whitelist:
             - "+draft/react"
-            - "react"
+            - "+react"
 
         # if `default` is true, don't store TAGMSG containing any of these tags:
         #blacklist: