feat: add parental controls (guardian) feature#569
feat: add parental controls (guardian) feature#569torlando-tech wants to merge 18 commits intomainfrom
Conversation
❌ Threading Architecture Audit FailedView Audit ReportPlease fix the dispatcher violations before merging. |
Greptile SummaryThis PR adds a parental controls system that allows parents to lock and manage messaging on children's devices using cryptographically signed commands. Key Changes
Security Fixes from Original PR #255All seven security issues mentioned in the PR description have been properly addressed:
ArchitectureThe implementation follows a clean layered architecture: Kotlin UI/ViewModels → Repository → Database layer, with Python bridge for cryptographic operations using RNS Identity. Commands flow through proper signature verification before any state changes. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Parent as Parent Device
participant Child as Child Device
participant Python as Python Layer
participant DB as Database
Note over Parent,Child: QR Code Pairing Flow
Parent->>Parent: Generate QR (dest hash + pubkey + signature)
Child->>Parent: Scan QR code
Child->>Python: Verify QR signature
Python-->>Child: Valid
Child->>DB: Store guardian config
Child->>Parent: Send PAIR_ACK
Note over Parent,Child: Lock Command Flow
Parent->>Python: Sign LOCK command
Python->>Python: Sign(cmd + nonce + timestamp + payload)
Parent->>Child: Send LXMF message with command
Child->>Child: Check sender = guardian
Child->>Python: Verify signature
Python->>Python: Reconstruct signed bytes
Python->>Python: Verify Ed25519 signature
Python-->>Child: Valid
Child->>Child: Check nonce + timestamp
Child->>DB: Set isLocked = true
Child->>Python: Sync guardian config
Child->>Child: Stop location sharing
Note over Parent,Child: Message Filtering (Locked)
Child->>Child: Receive message from non-allowed contact
Child->>DB: Check isLocked + allow list
DB-->>Child: Not allowed
Child->>Child: Silently drop message
Last reviewed commit: 535c49b |
| // Check timestamp is newer than last command | ||
| if (timestamp <= config.lastCommandTimestamp) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The timestamp check rejects commands with timestamp <= lastCommandTimestamp, which means two commands sent in the same millisecond will cause the second to be rejected even with different nonces. For parental controls this is probably acceptable (commands shouldn't need sub-millisecond delivery), but consider if this could cause issues if a parent sends multiple rapid commands (e.g., LOCK followed immediately by ALLOW_ADD).
Prompt To Fix With AI
This is a comment left during a code review.
Path: data/src/main/java/com/lxmf/messenger/data/repository/GuardianRepository.kt
Line: 304-307
Comment:
The timestamp check rejects commands with `timestamp <= lastCommandTimestamp`, which means two commands sent in the same millisecond will cause the second to be rejected even with different nonces. For parental controls this is probably acceptable (commands shouldn't need sub-millisecond delivery), but consider if this could cause issues if a parent sends multiple rapid commands (e.g., LOCK followed immediately by ALLOW_ADD).
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
app/src/main/java/com/lxmf/messenger/service/manager/EventHandler.kt
Outdated
Show resolved
Hide resolved
3308f96 to
b768950
Compare
Implements guardian/parental control system allowing a parent device to control a child's messaging through cryptographically signed commands. Features: - QR code pairing with Ed25519 signed payloads (5-minute validity) - Guardian config and allowed contacts stored in Room database - Message filtering for incoming/outgoing based on allow-list - Lock/unlock commands via signed LXMF messages (field 0x80) - Feature restrictions when locked (Announces tab, Network settings) Components added: - GuardianConfigEntity/AllowedContactEntity with DAOs - GuardianRepository for state management - GuardianViewModel for UI state - GuardianScreen for parent QR generation - GuardianQrScannerScreen for child pairing - GuardianCommandProcessor for LXMF command handling - guardian_crypto.py for Ed25519 operations - Python test suite (16 tests) Bug fixes during implementation: - Field name mismatches between Python and Kotlin layers - RNS.Identity.validate() is instance method, not class method - QR scanner bottom card padding for nav bar Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add PairedChildEntity and DAO to track paired children on parent device - Add PAIR_ACK command sent by child after successful pairing - Process PAIR_ACK on parent side to register paired children - Add parent management UI showing paired children with lock/unlock buttons - Implement guardian command signing and sending via LXMF - Move PAIR_ACK check before duplicate check in MessageCollector - Support content-based command format (__GUARDIAN_CMD__: prefix) - Add database migration 31->32 for paired_children table - Observe paired children Flow for immediate UI updates Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Fix base64/hex encoding mismatch: Python now sends source_hash and public_key as base64, EventHandler decodes correctly - Add guardian command filtering in EventHandler to skip persistence for messages with __GUARDIAN_CMD__: prefix (prevents commands from appearing in chat) - Update GuardianCommandProcessor to detect commands in message content (new format) in addition to LXMF field 0x80 (legacy format) - Fix MessageCollector to return after processing guardian commands Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Child-side restrictions (when locked): - Hide Network subtab in Contacts, show only contacts list - Disable Manage Interfaces button in NetworkCard - Disable Manage Identities button in IdentityCard - Disable Location toggle in LocationSharingCard - Hide Remove Parental Controls button in GuardianScreen Parent-side features: - Add ManageChildContactsDialog for adding/removing contacts from child's allow list - Fix ALLOW_ADD/ALLOW_REMOVE payload format to match GuardianCommandProcessor expectations - Add contacts to child's contacts list when parent sends ALLOW_ADD command Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Messages were bypassing the MessageCollector filtering by being persisted directly in the service process. Add allow-list check to ServicePersistenceManager.persistMessage() to block messages from non-allowed contacts when the device is locked. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Syncs guardian/parental control state to the Python layer on app startup and when guardian commands are processed. This enables potential link-level filtering when the device is locked. Changes: - Add updateGuardianConfig to ReticulumProtocol interface and implementations - Add AIDL method for cross-process guardian config updates - Sync guardian config on app initialization (all startup paths) - Sync after processing LOCK/UNLOCK/ALLOW_* commands - Add link_established callback with remote_identified_callback Known limitation (documented in code): RNS links don't reveal initiator identity at establishment time. The remote_identified_callback only fires if the peer explicitly calls link.identify(), which LXMF doesn't do. This means blocked contacts can still briefly show as "online" before being blocked at the message layer. Message filtering remains fully effective. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The guardian locked state (isGuardianLocked) was being reset to false
when the app was restarted, even though it was correctly stored in the
database. This caused parental control restrictions (like hiding the
Map nav button) to not apply on app launch.
Root cause: Two issues working together:
1. The StateFlow was initialized with default values before the init{}
block ran, allowing Compose to observe false before the actual value
was loaded
2. The loadSettings() combine created a new SettingsState that didn't
preserve the guardian fields, overwriting them with defaults when
isLoading became false
Fix:
- Load initial guardian config synchronously during property init using
runBlocking, before the StateFlow is created
- Initialize _state with the correct guardian values from the start
- Preserve guardian state fields (hasGuardian, isGuardianLocked,
guardianName, allowedContactCount) in the loadSettings() combine
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Documents the parental control feature for both parents and developers: - Setup guide with QR pairing flow for parents and children - Security model explaining Reticulum identity-based authentication - Child device restrictions when locked (messaging + UI) - Complete command protocol reference with JSON schemas - Known limitations and security gaps (online status leak, factory reset bypass, signature verification TODO) - Database schema and source code references Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix guardian_verify_command Python signature to match what the Kotlin binder actually sends: (commandJson, signature_bytes, pubkey_bytes) instead of the old 6-arg hex-string interface. Reconstructs payload bytes using the same json.dumps encoding used at sign time. - Implement Ed25519 signature verification in GuardianCommandProcessor. Commands are now cryptographically verified before execution; commands with missing public keys or invalid signatures are rejected. - Stop all active location sharing sessions when a LOCK command is received. Inject LocationSharingManager into GuardianCommandProcessor and call stopSharing(null) in executeLock(). - Prevent unpair while device is locked. GuardianRepository.removeGuardian() now returns Boolean and returns false when isLocked=true. GuardianViewModel surfaces a user-facing error message in this case. - Fix bare 'return' in ServicePersistenceManager.persistMessage() to 'return false' to match the function's Boolean return type. - Remove stray '=======' conflict marker left in ReticulumServiceBinder.kt. - Renumber duplicate guardian DB migrations from 30→31/31→32 to 39→40/40→41 (those versions were already used by HEAD's peer_icons migrations). Bump DB version from 39 to 41. - Update QR scanner consent dialog with clearer language indicating this feature is for parents managing a child's device, and explaining the factory reset escape route for adults. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The signature string was parsed from the JSON but replaced by signatureBytes (decoded to ByteArray) for the actual verification call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All violations are pre-existing in the PR's new code, not regressions: - Remove unused announceRepository and scope from GuardianCommandProcessor (unused private properties) - Add @Suppress annotations for TooManyFunctions on GuardianRepository, ColumbaDatabase, DatabaseModule (multiple related APIs in one class/module) - Add @Suppress ReturnCount on isGuardianCommand, processCommand, isPairAckMessage, isContactAllowed, validateCommand (early returns for clarity/security gates) - Add @Suppress CyclomaticComplexMethod on processCommand, processMessageFromJson (unavoidable when dispatching over all command/message types) - Add @Suppress NestedBlockDepth/SwallowedException on processPairAck (best-effort display name extraction is intentionally non-fatal) - Use catch (_: Exception) pattern in QR scanner and isPairAckMessage (expected no-op cases: no QR code found, not a guardian command) - Add @Suppress LongParameterList on MessageCollector (all deps required) - Add @Suppress UnusedParameter on ChildDeviceSection.lockedTimestamp (reserved for future "locked since" display) - Add @Suppress LongMethod/CyclomaticComplexMethod on processMessageFromJson (LXMF field parsing naturally covers all field types) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Python: fix reaction event source_hash encoding (hex not base64) - SettingsViewModel: remove runBlocking from property init; guardian state now loads async via existing loadGuardianSettings() Flow - Tests: add guardianRepository mock to SettingsViewModelTest, SettingsViewModelIncomingMessageLimitTest, MessagingViewModelTest, MessagingViewModelImageLoadingTest, ContactsViewModelTest - Tests: add guardianRepository + guardianCommandProcessor mocks to MessageCollectorTest Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ContactsScreenTest: stub isLocked StateFlow to prevent ClassCastException when Compose collects it via collectAsState() - ServicePersistenceManagerTest: add guardianConfigDao and allowedContactDao mocks so the guardian lock check in persistMessage() doesn't throw - EventHandlerTest: use mockkStatic(Base64) so sourceHashHex is non-empty and persistMessage is actually called; fix coEvery/coVerify to use all 13 persistMessage parameters Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add error branch tests for guardian_crypto.py functions and new reticulum_wrapper.py guardian method tests to improve codecov patch coverage. guardian_crypto.py coverage additions: - sign_data: None identity, missing sign method, signing exception - verify_signature: None/empty public key, None/empty signature, None data, identity.pub None after load_public_key, RNS exception - generate_pairing_qr_data: None identity, sign_data failure path - validate_pairing_qr: future timestamp, invalid signature - sign_command: non-string cmd triggers encode() exception reticulum_wrapper.py coverage additions: - update_guardian_config: locked with hashes, unlock, None hashes - _on_lxmf_link_established: not locked (no callback), locked (registers callback) - _on_link_remote_identified: not locked, is guardian, in allowed list, not allowed (teardown called), teardown exception handled gracefully - guardian_verify_command: valid JSON, invalid JSON - guardian_sign_command: identity not found, identity load returns None - guardian_send_command: no destination, no identity, success path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… child registration Previously, processPairAck() blindly accepted any PAIR_ACK sender as a paired child. An attacker on the network could send a crafted PAIR_ACK message and get registered as a child device without scanning the QR. The fix adds a 16-byte random pairing token to the QR code that the child must echo back in the PAIR_ACK payload, proving physical access to the QR. The token is included in the Ed25519 signature to prevent tampering. Parent stores the token via AtomicReference and consumes it on successful match (one-time use). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing, QR expiry, and direct-only delivery - Verify Ed25519 signature on PAIR_ACK using sender's LXMF identity key (HIGH) - Add per-sender rate limiting (10s throttle) to prevent brute-force token guessing (LOW) - Add parent-side QR expiry (5 min) matching child-side validation (MEDIUM) - Force guardian commands to direct-only delivery (no propagation fallback) - Refactor GuardianCard to use CollapsibleSettingsCard with expansion state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b768950 to
02d9b7e
Compare
Refactor GuardianCommandProcessor.processCommand and processPairAck by extracting helper methods (extractCommandData, validateTimestamp, validateNonce, verifyCommandSignature, decodeSignatureHex, dispatchCommand, validatePairingToken, verifyPairAckSignature) to bring both functions under the 80-line LongMethod threshold. Compact EventHandler's base64 decoding into file-level helper functions and streamline guardian command checks to resolve LargeClass violation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
unfortunately I am just hesitant about providing this as a dual child/parent role in columba. i think the design itself is sound but for one it is too easy for the child to bypass, and 2 it is too risky to apply controls maliciously if an attacker gains physical access to the device. if anything i think i will create a modified "kids mode" build or something of the app which excludes the more dangerous UI elements but uses the same cryptographic command scheme I've laid out in this PR |
Summary
Adds a parental controls system ("guardian") that lets a parent lock a child's Columba device to only communicate with pre-approved contacts. This is an app-layer feature and does not modify the Reticulum protocol.
Key capabilities:
Security model:
Anti-abuse design:
What was fixed during rebase/review
This branch is a clean cherry-pick of the original PR #255 onto current main, with several security issues fixed:
GuardianCommandProcessor.processCommand()had a TODO. Now callsreticulumProtocol.verifyGuardianCommand().guardian_verify_commandparameter mismatch — original expected 6 hex-string args; binder sends(commandJson, signature_bytes, pubkey_bytes). Fixed to parse the JSON and reconstruct signed bytes correctly.executeLock()now injectsLocationSharingManagerand callsstopSharing(null).GuardianRepository.removeGuardian()now returnsBooleanand blocks whenisLocked=true.=======left inReticulumServiceBinder.ktcaused compile error. Removed.returnvsreturn false— barereturninServicePersistenceManager.persistMessage()fixed.Test plan
🤖 Generated with Claude Code