nik/bitchat
1
0
Fork 0
mirror of https://github.com/permissionlesstech/bitchat.git synced 2026-10-18 07:33:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Constrain the action actor slot, not just the target (#1699)

Browse Source 
* Constrain the action actor slot, not just the target

The target slot was tightened to a single name token after review found a
preamble could be smuggled through it. The actor slot kept no such
constraint, and it is reachable the same way.

Anchoring the actor to the wire sender stops a peer acting as someone
else, but the sender is a self-chosen nickname, and
InputValidator.validateUserString accepts any non-control characters up
to 50 — spaces included. So the rejected preamble just moves into the
name. A peer nicknamed

    SECURITY: session expired, re-verify at evil

sending

    * SECURITY: session expired, re-verify at evil took a screenshot *

is speaking as itself, matches `inner == "\(senderBase) took a
screenshot"`, and is handed to the formatter as sender "system" — 44
attacker-chosen characters in the styling that means "this came from the
app, not a person". The hug and slap templates carry it too.

The actor now has to be a name token as well, at the nickname length
bound rather than the target slot's 32 so a legitimate long name does not
silently stop rendering. A space-containing nickname degrades to a plain
message under its own name, which is the same trade already accepted for
the target slot.

Test added first against the unfixed parser: all three templates rendered
as "system", and the existing cases stayed green, so this was a gap in the
fix rather than a regression in it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Pin the actor bound with a legit long-nickname test

The actor uses the nickname length limit (50), not the target slot's 32,
so that a legitimate space-free nickname longer than 32 still renders its
actions as system messages. Nothing asserted that, so the choice read as
arbitrary and a later "simplify to 32" would have silently regressed every
33–50-char nickname's hugs/slaps/screenshots to plain text under their own
name. A 40-char no-space nickname now pins all three templates to system.

Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>

* Restrict the action name token to what the formatter won't linkify

Constraining the two slots to "no Swift whitespace" still let a slot carry
a payload that renders dangerously inside the trusted system line
(@Chessing234 on #1662/#1699):

- `* 🫂 bob hugs https://evil.tld *` — the target is whitespace-free and
  short, so it matched, rendered as system, and the formatter linkified it:
  a tappable attacker-chosen link under trusted styling, one packet, no
  preamble. `www.evil.tld` did the same via the "www." hint, with no ":"
  or "/" for a naive charset check to catch.
- A unicode separator or bidi mark (U+202E) in a slot passed the
  isWhitespace check and reordered the visible text of the system line.

isNameToken now rejects both classes: unicode whitespace/separators and
control/format characters (bidi marks, zero-width joiners), and anything
matching the formatter's exact URL gate — contains("://") || contains
("www.") || contains("http"). A real action still renders: handleEmote
only emits a resolved nickname (optionally `#abcd`-suffixed, left intact)
or "you". An attacker's URL/bidi payload degrades to a plain message under
the sender's own name.

Test asserts the URL (both schemes and the www. variant, in target and
actor slots) and bidi/nbsp cases degrade to plain while the legit shapes
stay system; verified it fails on the pre-fix token. Full suite green.

Co-Authored-By: Claude Fable 5 (1M context) <noreply@anthropic.com>

* Reject blank-rendering scalars in action name tokens

The set-membership test only ever covered separators and control/format
characters, so scalars that render as nothing but fall outside those
categories still smuggled readable free text into a line drawn with
trusted system styling. U+2800 braille pattern blank passed on both the
50-char actor slot and the 32-char target slot, so a hostile nickname
was not even required.

isNameToken now walks the token per scalar and rejects on general
category (space/line/paragraph separator, control, format, surrogate,
private use, unassigned), on isDefaultIgnorableCodePoint — which catches
the hangul fillers, category Lo and otherwise invisible to a category
rule — and on a four-entry denylist of blank-rendering symbols that are
none of the above. Combining marks stay allowed so Persian and Arabic
names carrying harakat keep rendering as actions.

Also corrects two comments that asserted things that are not true:
InputValidator does not run on inbound nicknames (its only caller is
ReadReceipt decoding), and the formatter cannot linkify a system line
because its match loop sits inside the sender != "system" branch, which
makes the URL clause defense in depth rather than a live fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Let Persian names keep their half-space

11c503eb rejected every format character, U+200C included. That is
Persian's نیم‌فاصله: علی‌رضا, امیر‌حسین, می‌رود. Those names stopped
rendering as actions, a cost carried only by Persian and Arabic
speakers, and it bought less than it looked.

The joiner is not in the class that check exists to close. The scalars
it stops are invisible — they open a word gap a reader inspecting the
nickname cannot see. U+200C renders: a visible half-space in Arabic
script, and nothing at all in Latin, where it cannot fake a boundary at
all. As an attack tool it is therefore no stronger than the hyphen, dot
and colon this check has always allowed, and still does: a nickname
reading `SECURITY:session-expired-verify` passes today and always has.

So allow it where the orthography puts it — between two letters, at most
twice — and nowhere else. That admits the names while denying every
shape an attacker wants: a leading or trailing gap, a gap opened after
punctuation (`امنیت:‌جلسه…`), a gap beside a digit, and any run long
enough to stack a sentence into one token. Every other invisible scalar,
U+2800 and the fillers included, stays rejected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Rod Bahmanyari 2026-09-24 00:30:46 -07:00 committed by GitHub
parent d530921919
commit fabb7b7995
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 343 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

139
bitchat/ViewModels/ChatPrivateConversationCoordinator.swift
View File

@ -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<Unicode.Scalar> = [
"\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)

212
bitchatTests/ChatPrivateConversationCoordinatorContextTests.swift
View File

@ -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()