feat: Warning about trackers - #864
Conversation
Chessing234
left a comment
There was a problem hiding this comment.
Nice feature and the detector is careful — I checked the URL regex for catastrophic backtracking and it's fine, each label is anchored by its dot. A few things to sort out first.
The two composers now behave differently. In PrivateChatSheet you added accepted && messageText.text == submittedText before clearing, plus the LaunchedEffect(persistedDraft) sync. In ChatScreen the same onSend was converted to take submittedText but still clears unconditionally on accepted, and has no draft sync. If the guard is needed in one place it's needed in both; if it isn't, it should come out of PrivateChatSheet.
The draft LaunchedEffect eats typed whitespace. setDraft only stores when bounded.isNotBlank(), otherwise it removes the key. So in PrivateChatSheet: type a leading space → setConversationDraft(peerID, " ") → key removed → persistedDraft is "" → LaunchedEffect sees "" != " " and overwrites the field. The space is erased as you type it. The same effect can clobber a composer mid-edit when boundDrafts evicts (MAX_DRAFTS = 50, MAX_DRAFT_CHARS_TOTAL = 128_000), and it moves the caret to the end whenever it fires.
This whole draft-observation mechanism is also unrelated to tracking links — it exists only to surface the stashed notification reply — and isn't mentioned in the description. It's a real behaviour change to the private-chat composer and deserves its own PR, or at least its own section in the description.
The notification reply silently becomes a draft. From the user's side they typed a reply and hit send. If POST_NOTIFICATIONS isn't granted the notify() is skipped entirely and there is no other trace — the reply just doesn't send and nothing says so. And showTrackingReplyWarning posts under conversationID.hashCode(), the same id as the DM notification, so the warning replaces the MessagingStyle thread it came from. Is replacing intended? Either way the no-permission path needs a fallback.
The @Synchronized additions are partial. draftFor, setDraft and appendDraft get the annotation, but removeConversation, migrate and loadDrafts all mutate _drafts.value without it, so the races are still there. This also looks unrelated to tracking links — separate PR?
Detection gaps. igshid is Instagram's long-standing share parameter and the test explicitly asserts it is not flagged ("https://example.com/?igshid=value" → assertFalse). Was that deliberate? Also missing: wbraid and gbraid (Google's gclid successors, now the common case on iOS traffic), si (YouTube), mc_cid/mc_eid.
Strings. All six are tools:ignore="MissingTranslation" in an app shipping 33 locales, for user-facing security copy. And yes/no are very generic names to claim in the global resource namespace — tracking_link_confirm/tracking_link_cancel would be safer.
Smaller: mergeConversationDrafts uses appendedText.takeLast(maxChars), which truncates a URL from the front — the end you'd want to keep is the host.
Summary
igshDetection
The initial conservative catalog includes:
igshutm_*fbclidgcliddclidmsclkidtwclidttclidDetection inspects exact query parameter names and ignores matching text in paths, fragments, values, and ordinary messages.
Testing
./gradlew testDebugUnitTest lintDebug./gradlew :app:assembleDebug :wear:assembleDebugScreenshots
Sending a link:

Opening a link:

Risk
It warns without automatically removing or rewriting URL parameters. So it means it can cause some certain risks
Closes #731