Repository navigation
docs(compliance): audit posthog-react-native, posthog-flutter and posthog-android (64 rows each) - #120
docs(compliance): audit posthog-react-native, posthog-flutter and posthog-android (64 rows each)#120posthog[bot] wants to merge 5 commits into
Conversation
posthog
Bot
changed the title
…ach) Weekly bounded SDK compliance audit. Audited this run (priority rule 1 + rule 2): posthog-python at 509c8525 (from 95c7f6e0) and posthog-node at 4f590b36 (from fbdb6c7b). Both were the previous queue's joint #1, both are named by the server-side spec changes merged since the last baseline, and both were short the two newest contract rows, which this run adds — so both files now carry all 64 contracts. posthog-python: 13 ✅ · 16 🟡 · 5 ❌ · 30 ➖ · 0 ❓ (was 14/12/7/29/0 on 62) posthog-node: 9 ✅ · 21 🟡 · 5 ❌ · 29 ➖ · 0 ❓ (was 10/16/8/28/0 on 62) Only two of the three available slots were used; the reasoning and the resulting rotation-slippage warning are recorded in the index. Co-Authored-By: Claude Opus 5 <[email protected]> Generated-By: PostHog Desktop Task-Id: 229000da-306e-48a3-a8c2-cf9021906a6c
…thog-android (64 rows each) Three-slot run following the queue the previous run left. No spec change merged since 2026-10-12, so selection fell to the staleness rules. - posthog-react-native at 3ddaa654 (802 commits of drift): 28/28/3/5/0. Capture Exception cleared on the PR #103 withdrawal; Session Replay Privacy resolved from Unknown. Four rows downgraded purely on new spec text (projectToken, and malformed-payload decoding on three flag getters). Session Replay Debug Properties scores Fail on first audit. - posthog-flutter at 1032c102: 21/29/6/8/0. The repo now ships its own Dart core driving a Windows/Linux desktop platform, so all six of its Unknown cells are resolvable from the repo — five to Partial, Retry Queue to Pass. Exception Event Metadata scores Fail on first audit. - posthog-android at c5e788bd (targeted re-audit): 35/18/5/6/0, the largest net improvement so far. Screen, Shutdown, HTTP Client and Capture Exception all cleared; Autocapture improved. Matrix re-sorted alphabetically, so its note numbers are reassigned. Also corrects posthog-node's Traces row from Fail to Unknown: the pipeline it was recorded as missing had merged four weeks before that audit, and node is the package that wires it. Not re-scored, because scoring it properly needs a read this run did not have budget for. The matrix now has ten of twelve SDKs at the full 64 rows and one Unknown cell left, which is that correction. posthog-java is the only SDK outside the four-week window and the only one still at 59 rows. Co-Authored-By: Claude Opus 5 <[email protected]> Generated-By: PostHog Desktop Task-Id: a169426b-4e39-41b1-99bb-816a0ff89e14
…indings - Downgrade posthog-flutter Get Feature Flag Payload and Get Feature Flag Result to partial: only the Dart core (Windows/Linux) drops malformed payloads; iOS/macOS go through posthog-ios, which returns the raw string. Flutter summary and README roll-up move from 21/29 to 19/31. - Restore the 2026-08-06 findings that were reduced to "as previously recorded", re-verified at 1032c10 with fixed and retired items marked. Notes are renumbered for the two new payload notes. - Point flutter's cross-references at posthog-android's new note numbers. - Reconcile README: Shutdown pattern, Unknown-cell claims, applicable-row ratios for python/node, and per-SDK open-gap counts. - Restore the #110 refused-connection wording in posthog-android's HTTP Client entry. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…Person Properties to pass Their recorded gaps were fixed (#614) or retired (sdk-specs #103), and the empty setPersonProperties call no-ops on every downstream path. Flutter: 22/28/6/8. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
884ab8c to
9cae3c9
Compare
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
| - **SDK currently:** Multi-hook support is real (`PostHogBeforeSend.kt:8-16`, | ||
| `PostHogConfig.kt:484-516` stores hooks in a lock-guarded `MutableList`). Two bugs reconfirmed by | ||
| direct read this cycle in `posthog/src/main/java/com/posthog/PostHogStateless.kt`, method | ||
| `buildEvent` (lines 289-322): (1) **line 310** — `eventChecked = beforeSend.run(postHogEvent)` |
There was a problem hiding this comment.
blocking: Refresh the Android hook evidence without reversing fail-closed safety — The refreshed audit says every hook receives the original event and recommends keeping the last-good event after an exception, but at the newly advertised c5e788bd, PostHogStateless.kt:322-335 calls beforeSend.run(eventChecked), assigns its result, and logs/drops on an exception. Both alleged defects are therefore wrong for this revision; moreover, before-send-hook/spec.md:89 explicitly requires stopping the chain and dropping after a throw to prevent privacy-filter leaks. Re-scoring against the old implementation and recommending fail-open would undo a compliant safety fix. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_android_hook_chain_matches_pinned_source fails on the reviewed head, and the compiled AndroidHookProbe.kt confirms replacement chaining and throw-driven drop/stop.
There was a problem hiding this comment.
Fixed in d89492a: re-scored ✅, removed fail-open advice.
| - **Backwards compatibility:** Backward-compatible — a net-new, additive feature; adding it cannot break any existing integration. | ||
| - **Remediation:** Implement the shared span/queue/OTLP-JSON logic once in `@posthog/core`, mirroring the architecture the `logs` pipeline already established there (buffered queue, batching, `AppState`-aware mobile flush triggers), so node, browser, and react-native can all consume it from one place. | ||
| - **Spec requires:** Dedupe `$feature_flag_called` per (flag, value), re-fire on value change, clear on `reset()` and shutdown, and honour the minimal-envelope allowlist. | ||
| - **SDK currently:** Re-read at this commit. The tracker (`flagCallReported`, `packages/core/src/posthog-core.ts:62`, read at `:1047`, written at `:1079-1080`) dedupes per `(flag, value)` correctly and is cleared on both `reset()` (`:151`) and `_shutdown()` (`:209`, in a `finally`). The minimal-envelope path is implemented in the stateless base (`minimizeFlagCalledEventProperties`, applied at `posthog-core-stateless.ts:569-575`). **Gap, unchanged:** the client never persists the server's `minimalFlagCalledEvents` gate, so `isMinimalFlagCalledEventsEnabled()` (`posthog-core-stateless.ts:645-647`) returns the fail-safe `false` for react-native and the minimal envelope is never produced, even when the project is gated into it. |
There was a problem hiding this comment.
blocking: Remove the nonexistent React Native minimal-gate gap — The newly stated remaining gap says the client never persists minimalFlagCalledEvents and inherits the stateless fallback, but the pinned packages/core/src/posthog-core.ts:894-900 stores res.minimalFlagCalledEvents === true, and :967-968 overrides isMinimalFlagCalledEventsEnabled() to read that persisted field. Failed/quota-limited loads also preserve the previous gate. Thus the advertised source already implements the proposed remediation; this replacement finding and the corresponding index narrative direct maintainers to duplicate existing functionality rather than report the actual remaining tracker gaps. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_react_native_persists_minimal_gate fails on the reviewed head, while node RNMinimalGateProbe.ts demonstrates the pinned storage/reader path enabling minimization.
|
|
||
| ### n10 — Feature Flag Called Tracker (🟡 Partial) | ||
| - **Spec requires:** Dedupe `$feature_flag_called` per (flag, value), re-fire on value change, clear on `reset()`, and honour the minimal-envelope allowlist. | ||
| - **SDK currently:** **Resolved from ❓ Unknown.** The Dart core implements the tracker directly: `_flagCallReported` is a `Map<String, Set<PostHogFeatureFlagValue?>>` (`posthog_core.dart:25-26`), the dedup check is `!(_flagCallReported[key]?.contains(flagValue) ?? false)` (`:780`) — keyed on the value, so a changed value correctly re-fires — the set is populated after reporting (`:815`), `$feature_flag_called` is captured at `:838`, and the whole map is cleared on reset (`:82`). Failure cases are recorded for the event (`:888`). **Why 🟡:** desktop only; the mobile path is the embedded native tracker, which `compliance/posthog-android.md` n8 records as 🟡 (posthog-ios is ✅), and no minimal-envelope allowlist handling is visible on the Dart path. |
There was a problem hiding this comment.
blocking: Account for Flutter's existing desktop minimal-envelope implementation — The resolved tracker audit claims no minimal-envelope allowlist exists in Dart and asks for one to be added. At 1032c102, core/feature_flag_utils.dart parses the server gate and defines minimalFeatureFlagCalledProperties; posthog_core.dart:652 persists it and :120-121 reads it, while posthog_core_stateless.dart:229-236 applies the allowlist specifically to non-experiment flag-called events. The same false absence is used in the Remote Config note and index. Any mobile gap may still justify Partial, but the desktop evidence and remediation need correcting rather than prescribing an already-shipped feature. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_flutter_minimal_allowlist_exists fails on the reviewed head, and dart FlutterMinimalProbe.dart confirms the pinned parser/filter removes custom properties while retaining attribution.
There was a problem hiding this comment.
Fixed in d89492a: credited the Dart allowlist; stays 🟡 only on the embedded Android tracker.
| | 45 | Reset | ✅ | | | ||
| | 46 | Reset Group Properties For Flags | ✅ | | | ||
| | 47 | Reset Person Properties For Flags | ✅ | | | ||
| | 48 | Retry Queue | ✅ | | |
There was a problem hiding this comment.
blocking: Do not award Flutter an all-platform Retry Queue pass from desktop evidence alone — The file explicitly grades every row across native, desktop and web, but this new Pass is justified only by Dart _doFlush. Flutter Web forwards captures to window.posthog; the browser implementation at the matrix's recorded 6538babc uses its separate RetryQueue, whose retry-queue.ts:94 excludes HTTP 429 from retries. A Flutter Web event rejected with 429 is therefore discarded rather than retained, contradicting the retry-queue contract even though the desktop 503 path is correct. The all-platform row and roll-up should retain the inherited web gap. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_flutter_retry_status_accounts_for_web fails on the reviewed head, and node BrowserRetryProbe.ts produces zero retry entries for 429 with one entry for its 503 control.
There was a problem hiding this comment.
Fixed in d89492a: Retry Queue now 🟡 for the Flutter Web 429 gap (n38).
| - **Spec requires:** Walk element ancestry on a touch/click, build a sanitized `$elements` array, emit `$autocapture` with `$event_type` and screen context; respect no-capture markers; never capture sensitive control values. | ||
| - **SDK currently:** `packages/react-native/src/autocapture.tsx` (273 lines), dispatched from `PostHogProvider`'s `onTouchEndCapture` into `posthog.autocapture(eventType, elements, …)` (`:267`). `ph-no-capture` is correctly honoured per ancestor (`:177`, configurable via `noCaptureProp`). **Improved since the last audit:** `eventType` is no longer a hardcoded literal — it is now a typed `'touch' | 'click'` parameter (`:127`, `:143`), though it still **defaults to `'touch'`** rather than the cross-SDK `'click'` vocabulary, so the default-path deviation stands. **Unchanged gap:** no defence-in-depth for secure/password inputs — grep for `secure`/`password` across `autocapture.tsx` still returns zero hits; only the generic `propsToCapture` allowlist (`:150`) gates capture, with no explicit exclusion for a `TextInput` with `secureTextEntry`. | ||
| - **Backwards compatibility:** Backward-compatible — the parameter already exists, so changing the default is a value change warranting a changelog note; a `secureTextEntry` skip is purely additive. | ||
| - **Remediation:** Consider flipping the `eventType` default to `'click'` with a changelog note; add an unconditional skip for `secureTextEntry`/password-like props regardless of `propsToCapture`. |
There was a problem hiding this comment.
blocking: Keep the mandated mobile autocapture event type — The refreshed note still treats the mobile default touch as noncompliant and recommends changing it to click. Both the advertised spec snapshot and the reviewed autocapture/spec.md:98 explicitly mandate $event_type: touch for Android/iOS/React Native mobile taps and reserve click for browser interactions, including React Native Web. Following this remediation would introduce the contract violation the audit is supposed to prevent; remove the default-type gap and distinguish mobile from web when describing the remaining privacy issue. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_react_native_touch_remediation_obeys_contract fails on the reviewed head because the proposed default contradicts the explicit mobile requirement.
There was a problem hiding this comment.
Fixed in d89492a: removed the 'click' remediation.
| ### n4 — Get Feature Flags (❌ Fail) | ||
| ### n2 — Autocapture (🟡 Partial) | ||
| - **Spec requires:** Walk element ancestry on a UI interaction, build a sanitized `$elements` array, emit `$autocapture` with an `$event_type` and screen context; respect no-capture markers; never capture sensitive control values. | ||
| - **SDK currently:** **Improved from ❌ Fail — `$autocapture` now genuinely exists.** The previous audit found no generic UI-interaction autocapture at all. `PostHogElementInteractionIntegration` (`posthog-android/src/main/java/com/posthog/android/internal/PostHogElementInteractionIntegration.kt:186`) now captures `$autocapture` for actionable View and Compose taps, carries `$touch_x`/`$touch_y`, and additionally detects and emits `$rageclick` (`:178-182`). A no-capture marker is honoured per subtree via the `com.posthog.android.R.id.posthog_autocapture_no_capture` tag (`internal/InteractionTarget.kt:179`), documented at `PostHogAndroidConfig.kt:77-82`. **Why 🟡 rather than ✅:** the feature is **off by default** — `captureElementInteractions` defaults to `false` (`PostHogAndroidConfig.kt:77`, "Default: false"), so an app that does nothing gets no autocapture, unlike the browser SDK where it is the default-on behaviour the contract describes. |
There was a problem hiding this comment.
blocking: Do not invent a default-on Android autocapture requirement — This new note identifies opt-in as the sole reason the implemented Android feature is Partial and asks maintainers to enable it by default or seek a carve-out. The canonical autocapture/spec.md:15 already says option names and defaults vary by platform, its behavior applies “when enabled,” and its acceptance scenarios explicitly initialize autocapture enabled. Default-off is not a contract gap; this invented requirement would encourage an unnecessary collection/volume change while leaving actual shape/privacy validation unfinished. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_android_default_is_not_a_contract_gap fails on the reviewed head against the contract's explicit platform-default allowance.
There was a problem hiding this comment.
Fixed in d89492a: default-off isn't a gap, re-scored ✅.
| - **Spec requires:** `client`. Four required keys on **every** captured event except `$snapshot` — `$recording_status`, `$sdk_debug_replay_event_trigger_status`, `$sdk_debug_replay_linked_flag_trigger_status`, `$sdk_debug_replay_internal_buffer_length`. Everything else is an **optional bundle** that goes **only on SDK events** (names starting with `$`, excluding `$feature_flag_called`, `$snapshot`, and the browser's `$$heatmap`) and **at most once every 30 seconds per SDK instance**. The queue-depth key sits outside both tiers and stays on every non-`$snapshot` event. `$snapshot` carries none of them. An SDK with no replay integration produces only `$recording_status: disabled`. **On mobile, a crash-time `$exception` reported on a later launch is stamped from a crash-context snapshot that always carries the full bundle.** | ||
| - **SDK currently:** Audited from scratch, and this is the most complete implementation of the contract outside `posthog-js`. `PostHogReplayIntegration.debugProperties()` (`posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt:2591-2643`) produces all three cross-platform `$sdk_debug_replay_*` required keys plus a genuine tri-state `$recording_status` (`disabled`/`buffering`/`active`) derived from a real buffering snapshot, and adds a thoughtful optional set: `$sdk_debug_replay_flush_hold_reason` (distinguishing `awaiting_remote_config` from `below_minimum_duration`), `$sdk_debug_replay_pending_trigger_conditions`, `$sdk_debug_replay_capture_mode`, and `$sdk_debug_replay_throttle_delay_ms`. The trigger statuses use a shared `triggerStatus()` helper with a correct `trigger_disabled`/`trigger_pending`/`trigger_activated` vocabulary. The wiring in `PostHog.sdkDebugProperties()` (`PostHog.kt:782-810`) is careful: it is applied only inside the `appendSharedProps` block, which `$snapshot` events skip (`:918-919`), so **the `$snapshot` exclusion is correct**; it is merged **after** the caller's properties so SDK-computed values win, with a comment citing posthog-js's `extend(properties, sdkDebugProperties)` as the model; the replay handler's contribution is guarded separately from the session and queue keys so a throwing handler cannot suppress them, surfacing `$sdk_debug_error_capturing_properties` instead; and it falls back to `$recording_status: disabled` when no replay handler is installed, exactly as the spec's no-integration case requires. `$sdk_debug_pending_queue_size` is attached as the queue-depth key. | ||
| **Two real deviations.** (1) **No tiering and no throttle.** The spec puts everything beyond the four required keys into an optional bundle restricted to SDK events and rate-limited to once per 30 seconds per instance. Android attaches the *entire* set — `$sdk_debug_replay_capture_mode`, `$sdk_debug_replay_throttle_delay_ms`, `$sdk_debug_replay_pending_trigger_conditions`, `$sdk_debug_replay_flush_hold_reason`, `$sdk_debug_session_start`, `$sdk_debug_current_session_duration` — to **every** event with shared props, including custom (non-`$`) events, with no throttle window anywhere. There is no `REQUIRED_REPLAY_PROPERTIES`-equivalent split. That is a per-event payload cost on every customer event, which is the cost the tiering exists to avoid. | ||
| (2) **The crash-context case does the opposite of what the spec says.** `sdkDebugProperties()` opens with an early return of an **empty** map when an explicit timestamp predates the current session start (`PostHog.kt:787-791`, "An explicit timestamp from before this session started (a previous-process crash reported on this launch) did not occur here"). That is precisely the mobile crash-time `$exception` the spec singles out — and the spec says it SHALL be stamped from a crash-context snapshot carrying the full bundle, not left bare. The code's reasoning is sound in isolation (today's live state is not the crash's state), but the spec's answer is to persist the snapshot at crash time and replay it, not to omit the keys. |
There was a problem hiding this comment.
blocking: Respect the previous-process crash exception to debug-property attachment — The new audit labels the empty debug map for a previous-process crash “the opposite of what the spec says.” The cited contract actually says at session-replay-debug-properties/spec.md:973-977 to attach a persisted crash-time snapshot if one was captured, otherwise attach none, and explicitly gives this rule precedence over required every-event keys; its scenario at :1012-1018 expects precisely that omission when no snapshot exists. Android's early return prevents attaching today's live state and does not itself prove the alleged violation. Remove this deviation unless evidence demonstrates that an available persisted snapshot is being lost; the separate tiering/throttling gap is unaffected. Reproduction: reproduced — python3 review_checks.py AuditTruth.test_android_previous_process_omission_is_permitted fails on the reviewed head because the audit condemns an outcome the normative scenario requires.
There was a problem hiding this comment.
Fixed in d89492a: removed that deviation; stays 🟡 on tiering/throttling.
Android: Before Send Hook and Autocapture re-scored to pass (hooks chain and drop on throw as the spec requires; default-off autocapture is allowed), and the previous-process crash omission in Session Replay Debug Properties is recorded as conforming. React Native: Feature Flag Called Tracker re-scored to pass (PostHogCore persists and reads the minimal gate); the 'touch' autocapture default is correct for mobile taps. Flutter: the Dart core already implements the minimal flag-called envelope; Retry Queue moves to partial because Flutter Web inherits posthog-js's 429 gap. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
2 participants
Why
Weekly bounded audit of the PostHog SDKs against the cross-SDK contracts in
openspec/specs/, so the matrix reflects both new SDK code and the spec changes merged since the last run. ≤3 SDKs are deeply re-verified per run. This is the accumulating branch for that work — it carries two runs so far and should be read as one baseline.Audited across this PR
e1efa57→3ddaa65405b53dc→1032c1028659a7b4→c5e788bd95c7f6e0→509c8525fbdb6c7b→4f590b36Ten of twelve SDKs now carry all 64 contracts. posthog-php and posthog-ruby sit at 63 (needing only the MCP Analytics row, N/A for both). posthog-java is the last SDK at 59 rows.
Notable movement — 2026-10-19 run
projectToken, and malformed-payload decoding on three flag getters).Needs a human decision
exception-event-metadataline 300 forbids a caller's generic property map from overriding SDK-owned$exception_list/$exception_level/$exception_source. Six of twelve SDKs violate it — this run added react-native, flutter and android, and android's source documents it as intended. All six carry a "Needs deprecation path" verdict for the same reason: a typed override channel has to ship before the generic key stops winning. That sequencing is one cross-SDK decision, not six.@posthog/mcpunder posthog-js, but the package declares posthog-node as its workspace and peer dependency. Exactly one should own the row.Rotation
All three slots used — the first full run since the rotation began slipping, and it clears the three oldest pointers at once. Six SDKs are queued; posthog-java is the clear #1 and the only SDK outside the ~4-week window. Because its
posthog-server/lives in the posthog-android monorepo this run just read, much of its evidence is already written. The previous run's contingency (a shallow row-completion pass over the four 59-row files) is no longer needed — three of those four were completed properly here.Scope: documentation under
compliance/only — no spec content, acceptance files, or SDK code touched.Created with PostHog Desktop
🤖 Generated with Claude Code