fix(api): sanitize and truncate push notification content before delivery (#243)

[thomasluizon]

What

PushNotificationService sent notification title/body straight into the FCM batch/single-message payloads and the Web Push JSON payload with no sanitization and no length limit. Overlong or hostile content (control characters, multi-KB strings) could break rendering, blow past the platform payload ceilings (FCM 4KB, Web Push 4096 octets), or embed terminal/display-spoofing control sequences.

Fix

Sanitize once at the SendToUserAsync boundary, before dispatch to either transport:

• Strip control characters (Rune.IsControl → C0/C1 incl. NUL, ESC, DEL, NEL) from title and body.
• Truncate to a whole-rune UTF-8 byte budget — title 256 B, body 768 B. Iterating by Rune guarantees a multi-byte code point is never split at the boundary.
• Budgets are conservative so that even after System.Text.Json escapes non-ASCII to \uXXXX (up to ~3× byte expansion), the encrypted Web Push payload stays under the 4096-octet ceiling (RFC 8291 §4 / RFC 8030 §7.2) and FCM's 4KB message limit.

The #327 web-push retry loop and the FCM batch + per-subscription fallback paths are untouched — they now just carry the sanitized values.

Tests (Orbit.Infrastructure.Tests)

• SanitizeForDelivery_StripsControlCharacters — NUL/BEL/TAB/LF/CR/ESC/DEL/NEL removed.
• SanitizeForDelivery_TruncatesOverlongContentToByteBudget — 5000-char input capped to the byte budget.
• SanitizeForDelivery_NeverSplitsMultiByteRuneAtBoundary — a 4-byte emoji is never cut mid-rune (no U+FFFD).
• SanitizeForDelivery_PreservesValidContentUnchanged / _EmptyString_ReturnsEmpty — no regression on normal input.
• SendToUserAsync_HostileOverlongContent_DeliversPayloadUnderPlatformLimit — end-to-end: a 20 KB hostile body still produces an on-wire encrypted Web Push payload ≤ 4096 bytes and delivers.

Full Orbit.Infrastructure.Tests suite green (1540 passed).

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #341 (thomasluizon/orbit-api)

Scope: PR #341 — fix(api): sanitize and truncate push notification content before delivery
Recommendation: APPROVE

Summary

This PR adds control-character stripping and a whole-rune UTF-8 byte-budget truncation (SanitizeForDelivery) at the single entry point of PushNotificationService.SendToUserAsync, applied to both the FCM and Web Push delivery paths. The change is small, correctly scoped, well-tested (6 new unit tests including an end-to-end encrypted-payload-size assertion), and closes a real gap: habit.Title/goal.Title (user-named, unbounded for control characters) and AI-generated notification bodies flow into push payloads with no prior sanitization. The byte-budget math (256B title + 768B body, ~3x worst-case JSON-escape expansion ≈ 3072B) is sound and leaves headroom under both the FCM 4KB and Web Push 4096-octet ceilings.

One Medium-severity gap survives: the control-character filter (Rune.IsControl, Unicode category Cc) does not also strip Unicode Format-category (Cf) characters such as bidi-override controls (U+202E) or zero-width characters, which is a known display-spoofing vector the PR's own stated goal ("display-spoofing control sequences") implies it should cover. Not blocking — flagged as a fast-follow.

Findings

Critical

None.

High

None.

Medium

SanitizeForDelivery strips Cc control chars but not Cf format/bidi chars

• Location: src/Orbit.Infrastructure/Services/PushNotificationService.cs:262
• Issue: Rune.IsControl(rune) filters Unicode category Cc only (C0/DEL/C1). It does not filter category Cf ("format") characters — notably bidi-override/embedding controls (U+202E RIGHT-TO-LEFT OVERRIDE, U+202D LEFT-TO-RIGHT OVERRIDE, U+2066–U+2069 isolates) and zero-width characters (U+200B/U+200C/U+200D/U+FEFF).
• Risk: A user-named habit/goal title containing U+202E still reaches the FCM/Web Push notification title or body and can reorder rendered glyphs in the OS notification tray/lock screen — a display-spoofing vector, independent of the byte-length truncation this PR adds. Blast radius is limited to notification-UI rendering trickery (no code execution, no data loss).
• Fix: In SanitizeForDelivery, also skip runes where Rune.GetUnicodeCategory(rune) == UnicodeCategory.Format (or an explicit denylist of the bidi-control code points) alongside the existing Rune.IsControl check.

Low / Info

None (signal gate: Low/Info not surfaced on a PR review).

Subagents

Agent	Verdict
security-reviewer	PASS (1 Medium finding, folded in above)
contract-aligner	N/A — diff contains zero DTO/Controller/route/packages/shared changes; no contract surface touched

Validation

Check	Result
Build (dotnet)	N/A — skipped, CI runs Build separately
Tests (dotnet)	N/A — skipped, CI runs Unit Tests separately; PR reports full Orbit.Infrastructure.Tests suite green (1540 passed)

What's good

• Sanitization applied at the single point where all delivery paths converge (SendToUserAsync) — no bypass route through the private SendFcm/SendWebPush helpers.
• Byte-budget truncation is rune-safe (EnumerateRunes + Utf8SequenceLength check before appending), with a dedicated test proving no U+FFFD replacement-character corruption at the boundary.
• XML-doc comment on SanitizeForDelivery cites RFC 8291/8030 with a URL, explaining non-obvious byte-budget math.
• End-to-end test exercises real AES128GCM Web Push encryption to assert the actual on-wire encrypted payload stays ≤ 4096 bytes, not just the pre-encryption JSON.
• url parameter correctly left out of sanitization scope — every caller passes a hardcoded route constant, never user content.

Recommendation

Approve as-is; the one Medium finding (Cf/bidi-format character stripping) is a legitimate defense-in-depth gap worth a fast-follow but is not blocking.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud