diff --git a/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift b/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift index 58a7335d..c46c3b00 100644 --- a/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift +++ b/bitchat/ViewModels/ChatPrivateConversationCoordinator.swift @@ -837,6 +837,21 @@ final class ChatPrivateConversationCoordinator { // nickname, so a raw-string compare would regress every received // geohash action to plain text. let senderBase = message.sender.splitSuffix().0 + // The actor slot needs the same constraint as the target slot, and for + // the same reason. Anchoring to the wire sender stops a peer acting as + // someone else, but the sender is a *self-chosen nickname* that nothing + // upstream bounds: `InputValidator.validateNickname` is only ever + // reached from ReadReceipt decoding, inbound announce nicknames are + // merely NFC-normalized, and inbound content is merely trimmed. So the + // token check has to carry the whole constraint itself — otherwise the + // preamble the target slot rejects simply moves into the name: + // nickname `SECURITY: session expired, re-verify at evil` sending + // `* SECURITY: session expired, re-verify at evil took a screenshot *` + // is a peer speaking as itself, and lands in the trusted styling. + // A space-containing nickname degrades to a plain message instead. + guard Self.isNameToken(senderBase, maxLength: InputValidator.Limits.maxNicknameLength) else { + return message + } let isActionMessage = Self.matchesActionTemplate(inner, prefix: "🫂 \(senderBase) hugs ", suffix: "") @@ -874,18 +889,126 @@ final class ChatPrivateConversationCoordinator { } /// A display name as it appears in action content: "you", or a single - /// whitespace-free token of bounded length (a nickname, optionally with - /// a `#abcd` disambiguator). A space-containing nickname degrades to a - /// plain message rather than trusted styling — the safe direction. - static func isNameToken(_ token: String) -> Bool { - guard token == "you" else { - guard !token.isEmpty, token.count <= 32, - !token.contains(where: { $0.isWhitespace }) else { return false } - return true + /// bounded token in the conservative shape handleEmote actually emits (a + /// resolved nickname, optionally with a `#abcd` disambiguator). Anything + /// else degrades to a plain message rather than trusted styling — the safe + /// direction. + /// + /// `maxLength` defaults to the target-slot bound. The actor slot passes the + /// full nickname limit, because there the token is the sender's own name + /// and truncating legitimate long names would silently stop their actions + /// rendering. + /// + /// "No Swift whitespace" is not enough (thanks @Chessing234): the token is + /// still rendered inside the trusted `system` line, where anything without + /// a visible glyph reads as a word gap and lets free text pose as one name. + /// So reject two classes: + /// + /// 1. Scalars that render as nothing — see `rendersAsBlank`. The one + /// exception is U+200C, allowed in its orthographic position only; see + /// `hasOrthographicJoinerUseOnly` for why it is not in the same class. + /// 2. Anything the formatter would linkify. Its gate is exactly + /// `contains("://") || contains("www.") || contains("http")` + /// (ChatMessageFormatter). That loop is *inside* the formatter's + /// `sender != "system"` branch and the else branch that draws system + /// lines attaches no link attributes, so no link is tappable here + /// today; matching the gate is defense in depth against a formatter + /// change that starts linkifying system content, not a live fix. + /// + /// A real action still renders — handleEmote only ever emits a resolved + /// nickname (optionally `#abcd`-suffixed, left intact) or "you", none of + /// which trip either class. An attacker's blank-scalar/URL payload falls + /// through to a plain message under the sender's own name. + static func isNameToken(_ token: String, maxLength: Int = 32) -> Bool { + if token == "you" { return true } + guard !token.isEmpty, token.count <= maxLength else { return false } + guard hasOrthographicJoinerUseOnly(token) else { return false } + guard token.unicodeScalars.allSatisfy({ $0 == Self.zeroWidthNonJoiner || !rendersAsBlank($0) }) + else { return false } + let lower = token.lowercased() + return !lower.contains("://") && !lower.contains("www.") && !lower.contains("http") + } + + /// U+200C ZERO WIDTH NON-JOINER — Persian's نیم‌فاصله (half-space). + private static let zeroWidthNonJoiner: Unicode.Scalar = "\u{200C}" + + /// Persian compounds need one, occasionally two (`علی‌رضا‌پور`). Three or + /// more is not a name, it is a sentence wearing one. + private static let maxJoinersInName = 2 + + /// U+200C is category Cf, so `rendersAsBlank` rejects it with the rest of + /// the format characters — which silently stopped Persian names written + /// with the half-space (`علی‌رضا`, `می‌رود`) from rendering as actions. + /// That is a real cost borne only by Persian and Arabic speakers, and it + /// buys less than it looks: unlike the invisible scalars this check exists + /// to stop, the joiner *renders* — as a visible half-space gap in Arabic + /// script, and as nothing at all in Latin, where it cannot fake a word + /// boundary. As an attack tool it is therefore no stronger than the hyphen, + /// dot and colon this check has always allowed. + /// + /// So allow it, but only where the orthography actually puts it: between + /// two letters, at most `maxJoinersInName` times. That admits the names + /// while denying the shapes an attacker wants — a leading or trailing gap, + /// a gap after punctuation (`امنیت:‌جلسه…`), and any run long enough to + /// stack several words into one token. + private static func hasOrthographicJoinerUseOnly(_ token: String) -> Bool { + let scalars = Array(token.unicodeScalars) + var joiners = 0 + for (index, scalar) in scalars.enumerated() where scalar == zeroWidthNonJoiner { + joiners += 1 + guard joiners <= maxJoinersInName, + index > 0, index + 1 < scalars.count, + isLetterLike(scalars[index - 1]), isLetterLike(scalars[index + 1]) + else { return false } } return true } + /// Letters, plus the combining marks that ride on them — a joiner sitting + /// after a harakat is still between two letters as far as the reader is + /// concerned. + private static func isLetterLike(_ scalar: Unicode.Scalar) -> Bool { + switch scalar.properties.generalCategory { + case .lowercaseLetter, .uppercaseLetter, .titlecaseLetter, + .modifierLetter, .otherLetter, .nonspacingMark, .spacingMark: + return true + default: + return false + } + } + + /// Blank-rendering scalars that fall in none of the rejected categories and + /// are not default-ignorable, so only naming them catches them. + private static let blankRenderingScalars: Set = [ + "\u{2800}", // braille pattern blank + "\u{FFFC}", // object replacement character + "\u{FFFD}", // replacement character + "\u{1D159}" // musical symbol null notehead + ] + + /// Why a per-scalar category grammar and not the old + /// `whitespacesAndNewlines ∪ controlCharacters` set test: set membership + /// only ever covered separators and control/format characters, and + /// blank-rendering scalars outside those categories (U+2800, U+3164) went + /// on smuggling readable free text into a line drawn with trusted system + /// styling. + /// + /// Nonspacing and enclosing combining marks are deliberately absent: + /// Persian and Arabic names carry harakat and must keep rendering. + private static func rendersAsBlank(_ scalar: Unicode.Scalar) -> Bool { + switch scalar.properties.generalCategory { + case .spaceSeparator, .lineSeparator, .paragraphSeparator, + .control, .format, .surrogate, .privateUse, .unassigned: + return true + default: + break + } + // Catches the hangul fillers (U+3164, U+115F, U+1160, U+FFA0) and the + // variation selectors, which are letters and marks by category. + if scalar.properties.isDefaultIgnorableCodePoint { return true } + return blankRenderingScalars.contains(scalar) + } + func migratePrivateChatsIfNeeded(for peerID: PeerID, senderNickname: String) { let currentFingerprint = context.getFingerprint(for: peerID) diff --git a/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift b/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift index 7b07e101..ce45f198 100644 --- a/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift +++ b/bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift @@ -373,6 +373,218 @@ struct ChatPrivateConversationCoordinatorContextTests { #expect(processed("hello there").sender == "bob") } + /// A name token is whitespace-free and bounded, but it still renders inside + /// the trusted `system` line, so a unicode separator or bidi mark reorders + /// the visible text there (@Chessing234 on #1662/#1699). The URL half of + /// the check is defense in depth, not a live tappable link: the formatter's + /// link-detection loop sits inside its `sender != "system"` branch and the + /// else branch that draws system lines attaches no link attributes — the + /// check exists so a later formatter change can't quietly make one + /// tappable. Both classes degrade to a plain message under the sender's own + /// name. + @Test @MainActor + func processActionMessage_rejectsLinkAndSeparatorPayloadsInSlots() async { + let context = MockChatPrivateConversationContext() + let coordinator = ChatPrivateConversationCoordinator(context: context) + + func processed(_ content: String, sender: String = "bob") -> BitchatMessage { + coordinator.processActionMessage( + BitchatMessage(id: UUID().uuidString, sender: sender, content: content, + timestamp: Date(), isRelay: false) + ) + } + + // URL in the target slot — the whole point of the actor-anchoring + // example, but as a tappable link rather than a preamble. + #expect(processed("* 🫂 bob hugs https://evil.tld *").sender == "bob") + // The www. variant has no ":" or "/", so a naive charset check misses + // it; the formatter linkifies it via the "www." hint just the same. + #expect(processed("* 🫂 bob hugs www.evil.tld *").sender == "bob") + #expect(processed("* bob took a screenshot http://evil.tld *", sender: "bob").sender == "bob") + // URL smuggled through the ACTOR nickname too. + #expect(processed("* https://evil.tld took a screenshot *", sender: "https://evil.tld").sender == "https://evil.tld") + // Unicode bidi mark (U+202E right-to-left override) in the slot — this + // is the case a plain `Character.isWhitespace` scan actually missed. + #expect(processed("* 🫂 bob hugs a\u{202E}b *").sender == "bob") + // Non-breaking space (U+00A0). Swift's `isWhitespace` already matched + // this one on the base; it's kept as a regression guard. + #expect(processed("* 🫂 bob hugs a\u{00A0}b *").sender == "bob") + + // Legitimate shapes still render as system actions. + #expect(processed("* 🫂 bob hugs alice *").sender == "system") + #expect(processed("* 🫂 bob hugs alice#1a2b *").sender == "system") + #expect(processed("* bob took a screenshot *").sender == "system") + } + + /// The target slot rejects free text, but the ACTOR slot is the peer's own + /// self-chosen nickname, and nothing upstream bounds what arrives on the + /// wire: `InputValidator.validateNickname` is only reached from ReadReceipt + /// decoding, announce nicknames are merely NFC-normalized, and content is + /// merely trimmed. So the preamble the target slot now refuses can simply + /// be moved into the nickname, and the line still lands in the formatter's + /// trusted "system" styling — unless the token check catches it here. + @Test @MainActor + func processActionMessage_rejectsPreambleSmuggledIntoTheActorNickname() async { + let context = MockChatPrivateConversationContext() + let coordinator = ChatPrivateConversationCoordinator(context: context) + + func processed(_ content: String, sender: String) -> BitchatMessage { + coordinator.processActionMessage( + BitchatMessage( + id: UUID().uuidString, + sender: sender, + content: content, + timestamp: Date(), + isRelay: false + ) + ) + } + + // 44 characters — comfortably inside the 50-char nickname limit. + let hostileNick = "SECURITY: session expired, re-verify at evil" + #expect(hostileNick.count <= 50) + + // Every one of these is a peer speaking as itself, so the actor check + // passes; the payload rides in the name. + #expect(processed("* \(hostileNick) took a screenshot *", sender: hostileNick).sender == hostileNick) + #expect(processed("* 🫂 \(hostileNick) hugs you *", sender: hostileNick).sender == hostileNick) + #expect( + processed( + "* 🐟 \(hostileNick) slaps you around a bit with a large trout *", + sender: hostileNick + ).sender == hostileNick + ) + + // Ordinary nicknames keep rendering as system actions. + #expect(processed("* bob took a screenshot *", sender: "bob").sender == "system") + #expect(processed("* 🫂 bob hugs you *", sender: "bob").sender == "system") + + // The reason the actor bound is the nickname limit and not the target + // slot's 32: a legitimate, space-free nickname longer than 32 must + // STILL render as a system action. If this bound is ever "simplified" + // back to 32, these regress to plain text under the peer's own name and + // their hugs/slaps/screenshots silently stop rendering as actions. + let longLegitNick = String(repeating: "a", count: 40) // > 32, ≤ 50, no spaces + #expect(longLegitNick.count > 32 && longLegitNick.count <= InputValidator.Limits.maxNicknameLength) + #expect(processed("* \(longLegitNick) took a screenshot *", sender: longLegitNick).sender == "system") + #expect(processed("* 🫂 \(longLegitNick) hugs you *", sender: longLegitNick).sender == "system") + #expect(processed("* 🐟 \(longLegitNick) slaps you around a bit with a large trout *", sender: longLegitNick).sender == "system") + } + + /// A blank-rendering scalar is neither whitespace nor a control character, + /// so the old set-membership check waved it through (Codex on #1699): every + /// gap in `SECURITY:⠀session⠀expired,⠀re-verify⠀at⠀evil` is U+2800 braille + /// pattern blank, which reads as a sentence but passes as a single token + /// and lands in the trusted "system" styling. The scalar rule is by category now, so + /// hangul fillers and private-use scalars fall the same way — while names + /// carrying combining marks keep rendering. + @Test @MainActor + func processActionMessage_rejectsBlankRenderingScalarsInSlots() async { + let context = MockChatPrivateConversationContext() + let coordinator = ChatPrivateConversationCoordinator(context: context) + + func processed(_ content: String, sender: String = "bob") -> BitchatMessage { + coordinator.processActionMessage( + BitchatMessage(id: UUID().uuidString, sender: sender, content: content, + timestamp: Date(), isRelay: false) + ) + } + + // The reported payload, verbatim: U+2800 in every gap. + let brailleBlank = "\u{2800}" + let hostileNick = [ + "SECURITY:", "session", "expired,", "re-verify", "at", "evil" + ].joined(separator: brailleBlank) + // Inside the 50-char actor bound, so the rejection has to come from the + // scalar rule rather than the length check. + #expect(hostileNick.count <= InputValidator.Limits.maxNicknameLength) + + // All three action templates, each a peer speaking as itself. + #expect(processed("* \(hostileNick) took a screenshot *", sender: hostileNick).sender == hostileNick) + #expect(processed("* 🫂 \(hostileNick) hugs you *", sender: hostileNick).sender == hostileNick) + #expect( + processed( + "* 🐟 \(hostileNick) slaps you around a bit with a large trout *", + sender: hostileNick + ).sender == hostileNick + ) + // Same scalar in the target slot. + #expect(processed("* 🫂 bob hugs SECURITY:\(brailleBlank)verify\(brailleBlank)at\(brailleBlank)evil *").sender == "bob") + + // U+3164 hangul filler is category Lo, not a separator — only its + // default-ignorable property gives it away. Well inside the 32-char + // target bound, so again the scalar rule is what rejects it. + let hangulFiller = "\u{3164}" + let fillerTarget = "SECURITY:\(hangulFiller)reset\(hangulFiller)keys" + #expect(fillerTarget.count <= 32) + #expect(processed("* 🫂 bob hugs \(fillerTarget) *").sender == "bob") + + // Private-use scalars render as whatever the font decides, including + // nothing at all. + #expect(processed("* 🫂 bob hugs a\u{E000}b *").sender == "bob") + + // Nonspacing combining marks are NOT blanks: Persian and Arabic names + // carry harakat and must keep rendering as system actions. + let harakatName = "\u{0645}\u{062D}\u{0645}\u{0651}\u{062F}" // محمّد, with a U+0651 shadda + #expect(processed("* \(harakatName) took a screenshot *", sender: harakatName).sender == "system") + #expect(processed("* 🫂 bob hugs \(harakatName) *").sender == "system") + + // A plain emoji nickname (no variation selector) is a symbol, not a + // blank. + let emojiNick = "\u{1F389}" // 🎉 + #expect(processed("* \(emojiNick) took a screenshot *", sender: emojiNick).sender == "system") + #expect(processed("* 🫂 bob hugs \(emojiNick) *").sender == "system") + } + + /// Persian writes compounds with the half-space, U+200C ZERO WIDTH + /// NON-JOINER: `علی\u{200C}رضا`, `می\u{200C}رود`. It is category Cf, so the + /// blank-scalar rule rejected it and those names stopped rendering as + /// actions — a cost carried only by Persian and Arabic speakers. Unlike the + /// invisible scalars that rule exists to stop, the joiner renders: a + /// visible half-space in Arabic script, nothing at all in Latin, where it + /// cannot fake a word boundary. That makes it no stronger than the hyphen + /// and colon this check has always allowed, so it is admitted in its + /// orthographic position — between two letters, at most twice — and + /// nowhere else. + @Test @MainActor + func processActionMessage_allowsPersianHalfSpaceButNotSentencesBuiltFromIt() async { + let context = MockChatPrivateConversationContext() + let coordinator = ChatPrivateConversationCoordinator(context: context) + + func processed(_ content: String, sender: String = "bob") -> BitchatMessage { + coordinator.processActionMessage( + BitchatMessage(id: UUID().uuidString, sender: sender, content: content, + timestamp: Date(), isRelay: false) + ) + } + + let zwnj = "\u{200C}" + + // Real names must render as system actions, in both slots. + for name in ["علی\(zwnj)رضا", "می\(zwnj)رود", "امیر\(zwnj)حسین", "علی\(zwnj)رضا\(zwnj)پور"] { + #expect(processed("* \(name) took a screenshot *", sender: name).sender == "system") + #expect(processed("* 🫂 bob hugs \(name) *").sender == "system") + } + // A joiner after a harakat is still between two letters to a reader. + let harakat = "\u{0645}\u{062D}\u{0645}\u{0651}\u{062F}\(zwnj)\u{0631}\u{0636}\u{0627}" + #expect(processed("* \(harakat) took a screenshot *", sender: harakat).sender == "system") + + // A sentence built out of joiners is not a name: three or more gaps. + let sentence = "امنیت\(zwnj)جلسه\(zwnj)منقضی\(zwnj)شد" + #expect(processed("* \(sentence) took a screenshot *", sender: sentence).sender == sentence) + #expect(processed("* 🫂 bob hugs \(sentence) *").sender == "bob") + + // Nor is a gap opened next to anything that is not a letter. + for shape in ["امنیت:\(zwnj)جلسه", "\(zwnj)امنیت", "امنیت\(zwnj)", "ab1\(zwnj)2cd"] { + #expect(processed("* 🫂 bob hugs \(shape) *").sender == "bob") + } + + // The exception is U+200C alone — every other invisible scalar stays out. + #expect(processed("* 🫂 bob hugs a\u{200B}b *").sender == "bob") + #expect(processed("* 🫂 bob hugs a\u{200D}b *").sender == "bob") + #expect(processed("* 🫂 bob hugs a\u{2800}b *").sender == "bob") + } + @Test @MainActor func addMessageToPrivateChats_upsertsByIdAndSanitizes() async { let context = MockChatPrivateConversationContext()