fix(instant-privacy): warn before enabling when devices use a private WiFi address#1108
Conversation
- Detect locally-administered (private/randomized) MAC addresses among connected devices via OuiLookup - Surface a warning with the affected device list inside the enable confirmation dialog, so users can turn off "Private WiFi Address" before locking the network and avoid being blocked after MAC rotation - Add isPrivateMac flag to the device UI model, populated in the service - Add unit tests covering private vs universal MAC detection
…lling - Add privateMacWarningTitle / privateMacWarningDesc for all 25 non-English locales - Apply native-review fixes: zh_TW use 私密 (Apple's term), it use compreso, vi align setting name to OS term, nl fix spacing - Standardize "WiFi" to "Wi-Fi" across the warning strings, incl. en
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…f dialog - Move warning description to page banner (below feature desc) - Add red "Private" badge on device list items with private MAC - Simplify enable dialog to show title-only warning - Set isPrivateMac flag on allowed devices (ON list) too - Add privateMacLabel translations for all 26 locales
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · 9028ec6..22aefd4 (full)
Verdict: ✅ APPROVE — No Critical findings; 5 Warnings and 3 Suggestions noted. Safe to merge with follow-up improvements.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 🟢High | instant_privacy_service.dart:84–91 |
[both reviewers] allowedDevices() never sets isPrivateMac; enrichment only in fetchAll(), leaving public API inconsistent |
|
| 🟢High | instant_privacy_view.dart + app_en.arb |
"Before enabling" warning banner shown when feature is already enabled — factually incorrect copy | |
| 🟢High | instant_privacy_view.dart:_buildContent |
Derived hasPrivateMacInList computed in View; should be a getter on UspInstantPrivacyState |
|
| 🟢High | instant_privacy_view.dart:_onEnable |
ref.read(uspInstantPrivacyProvider) called twice in same method; state snapshot could diverge |
|
| 🟡Med | instant_privacy_view.dart |
_buildPrivateMacWarningBanner and _buildPrivateMacDialogWarning are near-duplicates — DRY violation |
|
| 💡 | 🟢High | instant_privacy_view.dart + test |
[both reviewers] _buildPrivateMacDialogWarning branch in dialog has no widget test coverage |
| 💡 | 🟡Med | oui_lookup.dart:82–88 |
isRandomizedMac and normalizeMac are independent normalization paths; doc comment recommended |
| 💡 | ⚪Low | instant_privacy_view.dart:_buildPrivateMacWarningBanner |
Raw Container for visual styling; consider AppCard or ui_kit container primitive |
Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
🔴 Critical — evidence chain
No Critical findings this round.
⚠️ Warning details
W1 — allowedDevices() never sets isPrivateMac 🟢High · [both reviewers]
lib/page/instant_privacy/services/instant_privacy_service.dart:84–91
allowedDevices() constructs InstantPrivacyDeviceUIModel without isPrivateMac, leaving it at its default false:
.map((mac) => InstantPrivacyDeviceUIModel(mac: mac, displayName: mac))
// isPrivateMac never set — defaults to falsefetchAll() then discards these objects and rebuilds them with isPrivateMac: OuiLookup.isRandomizedMac(d.mac). Any direct caller of allowedDevices() (including unit tests that invoke it in isolation) will silently receive all-false flags. activeDevices() correctly stamps the flag inline — allowedDevices() should do the same.
Fix: Move the flag assignment into allowedDevices() itself:
.map((mac) => InstantPrivacyDeviceUIModel(
mac: mac,
displayName: mac,
isPrivateMac: OuiLookup.isRandomizedMac(mac),
))Then fetchAll() only needs to update displayName, not rebuild the entire object.
W2 — "Before enabling" banner shown when feature is already enabled 🟢High · [Reviewer A]
lib/page/instant_privacy/views/instant_privacy_view.dart + lib/l10n/app_en.arb
When state.isEnabled == true, the view shows _buildPrivateMacWarningBanner (same widget as the pre-enable state) whose copy reads:
"To stay connected, turn off 'Private Wi-Fi Address' for these devices before enabling Instant Privacy."
When the feature is already ON, this instruction is factually wrong — users cannot follow it without first disabling the feature. Trigger: user enables Instant Privacy with a private-MAC device on the allowed list → navigates away → returns to the page → state.isEnabled == true, allowedDevices.any(isPrivateMac) == true → stale "before enabling" banner is displayed.
Fix: Either (a) suppress the main-screen banner when state.isEnabled == true (the dialog warning at enable-time is sufficient), or (b) add a separate localization key for the enabled-state with copy such as: "One or more allowed devices use a randomized Wi-Fi address. They may lose access when their address rotates."
W3 — Derived state hasPrivateMacInList computed in View 🟢High · [Reviewer B]
lib/page/instant_privacy/views/instant_privacy_view.dart:_buildContent
final hasPrivateMacInList = state.isEnabled
? state.allowedDevices.any((d) => d.isPrivateMac)
: state.connectedDevices.any((d) => d.isPrivateMac);UspInstantPrivacyState already exposes derived bool getters (e.g., isToggleDisabled). This computed property belongs in the state model for unit-testability and DRY.
Fix: Add to UspInstantPrivacyState:
bool get hasPrivateMacDevices =>
(isEnabled ? allowedDevices : connectedDevices).any((d) => d.isPrivateMac);View becomes: if (state.hasPrivateMacDevices) ...
W4 — ref.read(uspInstantPrivacyProvider) called twice in _onEnable() 🟢High · [Reviewer B]
lib/page/instant_privacy/views/instant_privacy_view.dart:_onEnable
// Added at top of _onEnable:
final connected = ref.read(uspInstantPrivacyProvider).valueOrNull?.connectedDevices ?? const [];
// Pre-existing inside dialog builder closure:
ref.read(uspInstantPrivacyProvider).valueOrNull?.connectedDevices.length ?? 0Two reads of the same provider snapshot in one method. Pattern is fragile and inconsistent with other methods that receive state as a parameter.
Fix: Pass UspInstantPrivacyState state into _onEnable():
Future<void> _onEnable(BuildContext context, WidgetRef ref, UspInstantPrivacyState state) async {
final privateMacDevices = state.connectedDevices.where((d) => d.isPrivateMac).toList();
// dialog body: state.connectedDevices.length (no second ref.read needed)W5 — _buildPrivateMacWarningBanner and _buildPrivateMacDialogWarning are near-duplicates 🟡Med · [Reviewer B]
lib/page/instant_privacy/views/instant_privacy_view.dart
Both methods render a warning with the same icon, same error color semantics, and largely the same content. They differ only in their outer layout container (Container vs. Padding). Any future change to the warning icon, color, or copy requires edits in two places.
Fix: Extract a single _PrivateMacWarning StatelessWidget (or one private method with a layout parameter) to eliminate duplication and make screenshot-testing easier.
✅ What looks good
- Correct IEEE 802 bit-check —
OuiLookup.isRandomizedMac()checksfirstByte & 0x02(the U/L bit), the proper IEEE standard; regex-cleaned input handles all separator variants correctly. - Three-layer dependency respected —
OuiLookupis imported only in the service layer; UI readsisPrivateMacthrough the provider/state chain with no layer violations. - All 26 locales updated —
privateMacWarningTitle,privateMacWarningDesc,privateMacLabeladded consistently across all ARB files. - Provider rules followed — No new
autoDispose, noref.watchin the provider layer, existingwithLock()mutation wrappers unchanged. - ui_kit_library components used correctly —
AppBadge,AppIcon,AppGap,AppText,AppButton,AppDialogall used correctly in new UI code. - Service-level test coverage — Two unit tests for MAC bit-check in
activeDevices()and one integration test forfetchAll()allowed-device flag; three new code paths covered. - Equatable
propscorrectly updated —isPrivateMacadded topropslist;Equatable 2.xhandlesboolequality correctly.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
Fixes #1103
Problem
Instant Privacy locks the network to a MAC whitelist snapshotted from the currently connected devices. Modern devices (iOS/macOS/Android) use private WiFi addresses (MAC randomization) by default, and these addresses rotate over time.
When a whitelisted MAC rotates, the device is blocked from its own network — including the very device the user enabled the feature from. Removing/adding entries afterwards cannot fully solve it (the rotated MAC is still not on the list, and once locked out over WiFi the user has no channel to add it), so the mitigation must happen before enabling.
Solution
Prevention-first — detect and warn, do not silently proceed:
OuiLookup.isRandomizedMac()(deterministic U/L-bit check).isPrivateMacflag, populated in the service layer.Note: the app can reliably tell that a MAC is locally administered, but cannot tell whether the device uses a fixed vs rotating private address (both are locally administered). The wording is therefore advisory ("may be blocked after it rotates"), not a hard assertion.
Localization
privateMacWarningTitle,privateMacWarningDesc, andprivateMacLabelfor all 26 locales.Tests
flutter analyzeclean.Resulting user experience