mirror of
https://github.com/permissionlesstech/bitchat.git
synced 2026-10-25 07:41:41 +00:00
fabb7b7995
* 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>
Test Harness Guide
This test suite uses an in-memory networking harness to make end-to-end and integration tests deterministic, fast, and race-free without touching production code.
In-Memory Bus
- File:
bitchatTests/Mocks/MockBLEService.swift - Registry/Adjacency: Global
registrymapspeerIDto aMockBLEServiceinstance;adjacencyrecords simulated links between peers. - Setup: Call
MockBLEService.resetTestBus()insetUp()to clear state between tests. - Topology: Use
simulateConnectedPeer(_:)andsimulateDisconnectedPeer(_:)to add/remove links.connectFullMesh()helpers in tests build larger topologies. - Handlers: Tests can observe data via
messageDeliveryHandler(decodedBitchatMessage) andpacketDeliveryHandler(rawBitchatPacket). - De‑duplication: A thread-safe
seenMessageIDsprevents duplicate deliveries during flooding/relays.
Broadcast Flooding
- Flag:
MockBLEService.autoFloodEnabled - Intent: When
true, public broadcasts propagate across the entire connected component (ignores TTL for reach) while still de‑duping to prevent loops. - Usage: Enabled in Integration tests (
setUp) to simulate large-network broadcast; disabled in E2E tests to keep routing explicit and verify TTL behavior (seePublicChatE2ETests.testZeroTTLNotRelayed).
Rehandshake Flow (Noise)
- Why: The legacy NACK recovery path was removed; recovery now relies on Noise session rehandshake after decrypt failure or desync.
- Manager:
NoiseSessionManagermanages per-peer sessions. - Pattern: On decrypt failure, proactively clear the local session and re-initiate a handshake. The peer accepts and replaces their session.
- Test:
IntegrationTests.testRehandshakeAfterDecryptionFailure- Corrupts ciphertext to induce a decrypt error.
- Calls
removeSession(for:)on the initiator’s manager beforeinitiateHandshake(with:)to avoidalreadyEstablished. - Verifies encrypt/decrypt succeeds post-rehandshake.
Tips
- Determinism: Add small async delays only where handler installation/topology changes could race the first send.
- Scoping: Keep
autoFloodEnabledtoggled only within Integration tests; always reset intearDown()to avoid cross-test contamination. - Direct vs Relay: Private messages target a specific peer when adjacent; otherwise they are surfaced to neighbors for relay and, if known, also delivered to the target.
Quick Start
- Create nodes and connect them:
let svc = MockBLEService(); svc.myPeerID = "PEER1"svc.simulateConnectedPeer("PEER2")
- Observe messages:
svc.messageDeliveryHandler = { msg in /* asserts */ }
- Enable broadcast flooding for Integration suites only:
MockBLEService.autoFloodEnabled = true