fix: subnet route CIDRs may not have host bits set #89

dkaser · 2025-12-18T04:07:32Z

Closes #88

Summary by CodeRabbit

New Features
- Real-time CIDR validation in the routes UI with immediate enable/disable of the Add action.
Bug Fixes
- More robust detection of invalid CIDR formats and host-bit misconfigurations.
- Clearer inline validation messages to prevent submission of incorrect routes.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Closes #88 Signed-off-by: Derek Kaser <[email protected]>

coderabbitai · 2025-12-18T04:07:42Z

Walkthrough

Frontend and backend CIDR validation were hardened: client adds ipaddr.js and new CIDRResult enum with precise host-bits checking; server replaces manual CIDR parsing with php-ip's IPBlock to ensure subnet route CIDRs are network addresses. A UI element was added for validation feedback.

Changes

Cohort / File(s)	Change Summary
Frontend CIDR validation logic `src/usr/local/emhttp/plugins/tailscale/include/Pages/Tailscale.php`	Added `ipaddr.min.js`; introduced `CIDRResult` enum (VALID, INVALID, EMPTY, HOSTBITS_SET); replaced `isValidCIDR` with ipaddr.js-based parser that derives network address and detects host bits; refactored `validateTailscaleRoute` to switch on `CIDRResult` and update UI/button state accordingly; `showTailscaleConfig` now invokes validation after load.
Frontend UI feedback `src/usr/local/emhttp/plugins/tailscale/include/data/Config.php`	Replaced an empty table cell with a `<span id="tailscaleRouteValidation">` for route validation messages.
Backend dependency (PHP) `src/usr/local/php/unraid-tailscale-utils/composer.json`	Added `rlanvin/php-ip: "^3.0"` to `require`.
Backend CIDR validation implementation `src/usr/local/php/unraid-tailscale-utils/unraid-tailscale-utils/Utils.php`	Replaced manual CIDR parsing/validation with `PhpIP\IPBlock` usage; validates by creating an IPBlock and comparing the network address against the input CIDR; added `use PhpIP\IPBlock`.
CI configuration `.github/workflows/lint.yml`	Enabled `gmp` PHP extension in composer step via `php_extensions: gmp`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Review parity between client (ipaddr.js) and server (php-ip) validation semantics, especially host-bits detection for IPv4/IPv6.
Inspect validateTailscaleRoute UI state transitions and ensure the new span element ID is targeted correctly.
Verify composer update and CI gmp addition do not break dependency resolution or pipeline runs.

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: validating that subnet route CIDRs do not have host bits set, which is the core objective of the pull request.
Linked Issues check	✅ Passed	The PR implements the required validation logic in both frontend (Tailscale.php with CIDRResult enum and isValidCIDR refactor) and backend (Utils.php with IPBlock validation) to ensure subnet route CIDRs are network addresses without host bits set, matching issue #88 requirements.
Out of Scope Changes check	✅ Passed	All changes are directly related to CIDR validation for subnet routes: frontend validation UI/logic, backend validation library, composer dependency addition, and GMP extension enablement for the IP library. No unrelated modifications detected.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/cidr-validation

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3499fe and 720493b.

📒 Files selected for processing (1)

.github/workflows/lint.yml (1 hunks)

🔇 Additional comments (1)

.github/workflows/lint.yml (1)

29-29: GMP extension is correctly added for PHPStan dependency resolution.

The rlanvin/php-ip library requires the GMP extension, so enabling it during composer installation is necessary. The change at line 29 correctly configures this for the phpstan job.

Note: The php_extensions input includes additional PHP extensions in the action steps, which is the appropriate way to handle this dependency during CI analysis.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/usr/local/emhttp/plugins/tailscale/include/Pages/Tailscale.php (1)

174-194: Consider internationalizing the validation error messages.

The hardcoded English strings ('Invalid CIDR: Host bits may not be set' and 'Invalid CIDR') are inconsistent with the translation pattern used elsewhere in this codebase (e.g., $tr->tr("needs_login")). Consider injecting translated strings from PHP for consistency.

🔎 Suggested approach

Pass translated messages from PHP and use them in the JavaScript:

+const validationMessages = {
+    hostbits: '<?= $tr->tr("validation.hostbits_set") ?>',
+    invalid: '<?= $tr->tr("validation.invalid_cidr") ?>'
+};
+
 function validateTailscaleRoute() {
     switch(isValidCIDR($('#tailscaleRoute').val())) {
         case CIDRResult.VALID:
             $('#tailscaleRouteValidation').text('');
             $('#addTailscaleRoute').prop('disabled', false);
             break;
         case CIDRResult.HOSTBITS_SET:
-            $('#tailscaleRouteValidation').text('Invalid CIDR: Host bits may not be set').css('color', 'red');
+            $('#tailscaleRouteValidation').text(validationMessages.hostbits).css('color', 'red');
             $('#addTailscaleRoute').prop('disabled', true);
             break;
         case CIDRResult.EMPTY:
             $('#tailscaleRouteValidation').text('');
             $('#addTailscaleRoute').prop('disabled', true);
             break;
         case CIDRResult.INVALID:
         default:
-            $('#tailscaleRouteValidation').text('Invalid CIDR').css('color', 'red');
+            $('#tailscaleRouteValidation').text(validationMessages.invalid).css('color', 'red');
             $('#addTailscaleRoute').prop('disabled', true);
             break;
     }
 }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab5a1b8 and a3499fe.

⛔ Files ignored due to path filters (2)

src/usr/local/emhttp/plugins/tailscale/lib/ipaddr.min.js is excluded by !**/*.min.js
src/usr/local/php/unraid-tailscale-utils/composer.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

src/usr/local/emhttp/plugins/tailscale/include/Pages/Tailscale.php (2 hunks)
src/usr/local/emhttp/plugins/tailscale/include/data/Config.php (1 hunks)
src/usr/local/php/unraid-tailscale-utils/composer.json (1 hunks)
src/usr/local/php/unraid-tailscale-utils/unraid-tailscale-utils/Utils.php (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-11T00:22:27.117Z

Learnt from: dkaser
Repo: unraid/unraid-tailscale PR: 66
File: src/usr/local/emhttp/plugins/tailscale/include/Pages/Settings.php:218-218
Timestamp: 2025-11-11T00:22:27.117Z
Learning: In the unraid-tailscale plugin, pages with `Tabs="true"` (like Tailscale.page) and their submenu pages (like Tailscale-1-Settings.page) share the same JavaScript scope. JavaScript functions defined in Tailscale.php are accessible in Settings.php because they load in the same browser context.

Applied to files:

src/usr/local/emhttp/plugins/tailscale/include/Pages/Tailscale.php

🧬 Code graph analysis (1)

src/usr/local/emhttp/plugins/tailscale/include/data/Config.php (1)

src/usr/local/php/unraid-tailscale-utils/unraid-tailscale-utils/Info.php (1)

tr (66-69)

🔇 Additional comments (4)

src/usr/local/php/unraid-tailscale-utils/unraid-tailscale-utils/Utils.php (1)

89-99: Clean implementation using the library for CIDR validation.

The logic correctly validates that the input CIDR represents a network address by comparing it against the computed network address from IPBlock. This ensures host bits are zero.

src/usr/local/emhttp/plugins/tailscale/include/data/Config.php (1)

175-175: LGTM! Clean UI addition for validation feedback.

The <span id="tailscaleRouteValidation"> element provides a proper target for displaying client-side validation messages, and the oninput handler ensures real-time feedback as the user types.

src/usr/local/emhttp/plugins/tailscale/include/Pages/Tailscale.php (2)

139-172: Solid validation implementation with proper host bits detection.

The CIDRResult enum pattern and the validation logic correctly identify when host bits are set by comparing the input address against the computed network address. The try-catch properly handles malformed input.

48-49: LGTM! Good choice of library for client-side IP address validation.

The ipaddr.js library is a well-established JavaScript library for manipulating IP addresses in JavaScript environments that allows you to verify and parse IP addresses and match them against CIDR ranges. The ipaddr.min.js file is present in the plugin distribution at the expected location.

src/usr/local/php/unraid-tailscale-utils/composer.json

Signed-off-by: Derek Kaser <[email protected]>

fix: subnet route CIDRs may not have host bits set

a3499fe

Closes #88 Signed-off-by: Derek Kaser <[email protected]>

github-actions bot added the fix label Dec 18, 2025

coderabbitai bot reviewed Dec 18, 2025

View reviewed changes

src/usr/local/php/unraid-tailscale-utils/composer.json Show resolved Hide resolved

fix: add required extension for ci

720493b

Signed-off-by: Derek Kaser <[email protected]>

dkaser merged commit 73dcb9e into trunk Dec 18, 2025
6 checks passed

dkaser deleted the fix/cidr-validation branch December 18, 2025 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: subnet route CIDRs may not have host bits set #89

fix: subnet route CIDRs may not have host bits set #89

Uh oh!

dkaser commented Dec 18, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: subnet route CIDRs may not have host bits set #89

fix: subnet route CIDRs may not have host bits set #89

Uh oh!

Conversation

dkaser commented Dec 18, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dkaser commented Dec 18, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 18, 2025 •

edited

Loading