Skip to content

fix: Migrate missing fixes from dev-1.2.7 #619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PeterJhongLinksys merged 2 commits into from
Feb 6, 2026
Merged

fix: Migrate missing fixes from dev-1.2.7 #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fde42d
fix: Migrate missing fixes from dev-1.2.7
AustinChangLinksys Feb 6, 2026
899a8b2
fix: Address Qodo review feedback
AustinChangLinksys Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/page/advanced_settings/internet_settings/utils/internet_settings_form_validator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ class InternetSettingsFormValidator {
if (value == null || value.isEmpty) {
return ValidationError.invalidSubnetMask;
}
final subnetMaskValidator = SubnetMaskValidator();
// QUALITY-439: Override default max (30) to allow /31 subnet masks for WAN Static IP
final subnetMaskValidator = SubnetMaskValidator(max: 31);
if (subnetMaskValidator.validate(value)) {
Comment on lines +46 to 48
Copy link

@qodo-code-review qodo-code-review bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. No tests for /31 masks 📘 Rule violation ⛯ Reliability

• The PR changes WAN Static IP subnet mask validation to allow /31 by constructing
  SubnetMaskValidator(max: 31) in multiple places.
• There is no automated test asserting that a /31 mask (e.g., 255.255.255.254) is accepted in
  these flows, and existing tests still only cover /24 in the PNP static IP view.
• Without coverage for this boundary change, regressions may go undetected and the intended fix may
  not be reliably validated.
Agent Prompt 
## Issue description
The PR enables `/31` subnet masks for WAN Static IP by using `SubnetMaskValidator(max: 31)`, but there are no automated tests asserting that a `/31` mask (e.g., `255.255.255.254`) is accepted in these updated flows.

## Issue Context
Existing tests for the PNP static IP view only use `255.255.255.0` as the valid subnet mask, and current `NetworkUtils.isValidSubnetMask` tests treat `255.255.255.254` as invalid under default parameters. This leaves the new boundary behavior unverified.

## Fix Focus Areas
- lib/page/advanced_settings/internet_settings/utils/internet_settings_form_validator.dart[42-53]
- lib/page/instant_setup/troubleshooter/views/isp_settings/pnp_static_ip_view.dart[34-40]
- test/page/instant_setup/troubleshooter/views/isp_settings/localizations/pnp_static_ip_view_test.dart[110-175]
- test/utils_test.dart[773-808]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

return null;
} else {
Expand Down
5 changes: 3 additions & 2 deletions lib/page/instant_safety/services/instant_safety_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ class InstantSafetyService {

// DNS Configuration Constants
static const _fortinetDns1 = '208.91.114.155';
static const _openDnsDns1 = '208.67.222.123';
static const _openDnsDns2 = '208.67.220.123';
// NOW-713: Updated OpenDNS Family Shield IPs
static const _openDnsDns1 = '208.67.222.222';
static const _openDnsDns2 = '208.67.220.220';
Comment on lines +52 to +54
Copy link

@qodo-code-review qodo-code-review bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Opendns config regression 🐞 Bug ✓ Correctness

• InstantSafetyService determines the current provider via an exact match on dnsServer1; changing
  the OpenDNS constant means routers still configured with the previously documented OpenDNS IPs will
  now be classified as off.
• This can surface as an incorrect UI state (shows “off” even though OpenDNS is configured) and can
  lead to unintended changes when the user saves settings.
• Repo specs/contracts still state OpenDNS uses the old IPs, so implementation and documented
  behavior are inconsistent, increasing the chance of future regressions.
Agent Prompt 
### Issue description
InstantSafetyService identifies OpenDNS by exact matching `dnsServer1` against `_openDnsDns1`. Updating `_openDnsDns1/_openDnsDns2` changes the detection key, so devices configured with the previously documented OpenDNS values will now be treated as `off`.

### Issue Context
Repo documentation/specs still describe OpenDNS as `208.67.222.123`/`208.67.220.123` and show detection logic matching against `208.67.222.123`. The implementation now uses `208.67.222.222`/`208.67.220.220`.

### Fix Focus Areas
- lib/page/instant_safety/services/instant_safety_service.dart[50-156]
- specs/009-instant-safety-service/spec.md[115-120]
- specs/009-instant-safety-service/contracts/instant_safety_service_contract.md[124-231]

### Suggested approach
1. Introduce legacy OpenDNS constants (or a set/list of acceptable DNS1 values).
2. Update `_determineSafeBrowsingType` to return `openDNS` when `dnsServer1` equals either legacy or new DNS1 (optionally also validate DNS2 for additional safety).
3. Decide the desired DNS values to *write* in `saveSettings` (new vs legacy) and update specs/contracts accordingly.
4. Add/extend tests to ensure fetchSettings returns `InstantSafetyType.openDNS` when LAN settings contain the legacy DNS1 value.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


/// Fetches current safe browsing configuration from router.
///
Expand Down
3 changes: 2 additions & 1 deletion lib/page/instant_setup/troubleshooter/views/isp_settings/pnp_static_ip_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class _PnpStaticIpViewState extends ConsumerState<PnpStaticIpView> {
var _hasExtraDNS = false;
bool _isLoading = false;

final subnetMaskValidator = SubnetMaskValidator();
// QUALITY-439: Override default max (30) to allow /31 subnet masks for WAN Static IP
final subnetMaskValidator = SubnetMaskValidator(max: 31);
final ipAddressValidator = IpAddressValidator();
final requiredIpAddressValidator = IpAddressRequiredValidator();
String? _ipError;
Expand Down
10 changes: 5 additions & 5 deletions specs/009-instant-safety-service/contracts/instant_safety_service_contract.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ Future<void> saveSettings(InstantSafetyType safeBrowsingType) async
1. Validates cached LAN settings exist (throws `InvalidInputError` if not)
2. Constructs DHCP settings with appropriate DNS servers based on type:
- `fortinet`: DNS1=208.91.114.155
- `openDNS`: DNS1=208.67.222.123, DNS2=208.67.220.123
- `openDNS`: DNS1=208.67.222.222, DNS2=208.67.220.220
- `off`: Clear all DNS servers
3. Calls `setLANSettings` JNAP action
4. Allows `JNAPSideEffectError` to propagate (handled by UI layer)
Expand Down Expand Up @@ -191,7 +191,7 @@ InstantSafetyType _determineSafeBrowsingType(RouterLANSettings lanSettings)
final dnsServer1 = lanSettings.dhcpSettings.dnsServer1;
if (dnsServer1 == '208.91.114.155') {
return InstantSafetyType.fortinet;
} else if (dnsServer1 == '208.67.222.123') {
} else if (dnsServer1 == '208.67.222.222') {
return InstantSafetyType.openDNS;
} else {
return InstantSafetyType.off;
Expand Down Expand Up @@ -225,9 +225,9 @@ Private constants within service:
// Fortinet Safe Browsing DNS
static const _fortinetDns1 = '208.91.114.155';

// OpenDNS Family Shield
static const _openDnsDns1 = '208.67.222.123';
static const _openDnsDns2 = '208.67.220.123';
// OpenDNS Family Shield (NOW-713: Updated IPs)
static const _openDnsDns1 = '208.67.222.222';
static const _openDnsDns2 = '208.67.220.220';
```

---
Expand Down
2 changes: 1 addition & 1 deletion specs/009-instant-safety-service/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,6 @@ The system determines whether the router hardware/firmware supports Fortinet saf
## Assumptions

- The existing `PreservableNotifierMixin` pattern will continue to be used for dirty state management
- DNS server IP addresses for Fortinet (208.91.114.155) and OpenDNS (208.67.222.123, 208.67.220.123) are fixed and do not require configuration
- DNS server IP addresses for Fortinet (208.91.114.155) and OpenDNS Family Shield (208.67.222.222, 208.67.220.220) are fixed and do not require configuration
- The compatibility map for Fortinet support (currently empty) will remain managed within the service layer
- The InstantSafety feature currently has no unit tests, so new tests will be created from scratch
5 changes: 3 additions & 2 deletions test/mocks/test_data/instant_safety_test_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import 'package:privacy_gui/core/jnap/result/jnap_result.dart';
class InstantSafetyTestData {
// DNS Configuration Constants (matching service)
static const fortinetDns1 = '208.91.114.155';
static const openDnsDns1 = '208.67.222.123';
static const openDnsDns2 = '208.67.220.123';
// NOW-713: Updated OpenDNS Family Shield IPs
static const openDnsDns1 = '208.67.222.222';
static const openDnsDns2 = '208.67.220.220';

/// Create default LAN settings response with no safe browsing configured
static JNAPSuccess createLANSettingsSuccess({
Expand Down
5 changes: 3 additions & 2 deletions test/page/instant_safety/services/instant_safety_service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,9 @@ void main() {

final data = captured.first as Map<String, dynamic>;
final dhcpSettings = data['dhcpSettings'] as Map<String, dynamic>;
expect(dhcpSettings['dnsServer1'], '208.67.222.123');
expect(dhcpSettings['dnsServer2'], '208.67.220.123');
// NOW-713: Updated OpenDNS Family Shield IPs
expect(dhcpSettings['dnsServer1'], '208.67.222.222');
expect(dhcpSettings['dnsServer2'], '208.67.220.220');
});

test('with off clears DNS servers', () async {
Expand Down
13 changes: 13 additions & 0 deletions test/validator_rules/input_validators_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,19 @@ void main() {
expect(validator.validate(invalidMask2), false);
});

// QUALITY-439: Test /31 subnet mask support for WAN Static IP
test('validate method - /31 subnet mask with max: 31', () {
final validator = SubnetMaskValidator(max: 31);
const mask31 = '255.255.255.254'; // /31 subnet mask
expect(validator.validate(mask31), true);
});

test('validate method - /31 subnet mask rejected with default max', () {
final validator = SubnetMaskValidator(); // default max is 30
const mask31 = '255.255.255.254'; // /31 subnet mask
expect(validator.validate(mask31), false);
});

test('validate method - invalid subnet mask (leading whitespace)', () {
final validator = SubnetMaskValidator();
const invalidMask = ' 255.255.255.128';
Expand Down