Skip to content

Fix: reject invalid server IPs (CIDR) in Conditional Forwarding table - #3833

Open
amonkarsidhant wants to merge 1 commit into
pi-hole:developmentfrom
amonkarsidhant:fix/revservers-ip-validation
Open

Fix: reject invalid server IPs (CIDR) in Conditional Forwarding table#3833
amonkarsidhant wants to merge 1 commit into
pi-hole:developmentfrom
amonkarsidhant:fix/revservers-ip-validation

Conversation

@amonkarsidhant

@amonkarsidhant amonkarsidhant commented Jul 30, 2026

Copy link
Copy Markdown

Fixes #3829

What does this PR aim to accomplish?

Reject invalid server IPs (CIDR notation like 192.168.1.0/24) in the Conditional Forwarding table. Previously, the Server IP field accepted these invalid values due to two bugs.

How does this PR accomplish the above?

  1. Fix footer row save button not being disabled (settings-dns.lp): Added class=actions to the footer row's <th> containing the save button. The validation handler uses javascript\n.siblings(.actions).find(.saveRevServers).prop(disabled, true);\n but the footer row's <th> lacked the actions class, so the button was never disabled.

  2. Add server-side IP validation (scripts/js/settings-dns.js): Added IP format validation in addRevServer() and saveRecord() using existing utils.validateIPv4WithPort() and utils.validateIPv6WithPort() functions, which already reject CIDR notation.

Link documentation PRs if any are needed to support this PR

N/A


By submitting this pull request, I confirm the following:

  1. ✅ Based against the development branch
  2. ✅ Commits are DCO-signed-off
  3. ✅ Commits are GPG-signed
  4. ✅ No documentation changes needed

The Server IP field in the Conditional Forwarding table accepted
invalid values like CIDR notation (e.g., 192.168.1.0/24) due to two bugs:

1. The footer row's save button was never disabled by input validation
   because its parent <th> lacked the 'actions' class that the
   validation selector expects.

2. The addRevServer() and saveRecord() functions only checked for
   empty fields without validating the IP format.

Fix:
- Add class="actions" to the footer row's <th> so the validation
  selector can find and disable the save button.
- Add IP format validation in addRevServer() and saveRecord() using
  the existing utils.validateIPv4WithPort() and
  utils.validateIPv6WithPort() functions, which already reject CIDR
  notation.

Fixes pi-hole#3829

Signed-off-by: Sidhant Aneesh <sidhant@amonkar.dev>
@amonkarsidhant
amonkarsidhant requested a review from a team as a code owner July 30, 2026 22:00
@rdwebdesign

Copy link
Copy Markdown
Member

The original issue was actually a bad validation function, that allowed /24 in the IP.
The function was already fixed in development branch.

but the footer row's <th> lacked the actions class, so the button was never disabled.

This is not the case.

The table cell has the .actions class:
Image

The class is set using datatables className: option:

{ data: null, width: "82px", className: "actions" },

The values type on the table footer were validated, but using a broken function.


Add server-side IP validation (scripts/js/settings-dns.js): Added IP format validation in addRevServer() and saveRecord() using existing utils.validateIPv4WithPort() and utils.validateIPv6WithPort() functions, which already reject CIDR notation.

The validation function was broken. Adding the validation here (using that broken function) wouldn't change anything.

This would take effect now that the validation function has been fixed, but the values ​​will already be validated, so this is not necessary.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DL6ER

DL6ER commented Aug 9, 2026

Copy link
Copy Markdown
Member

The conflicts here are context only: settings-dns.lp was reindented during the AdminLTE 4 migration (#3809), nothing collides with your change. A rebase onto current development should be all it takes.

Worth noting that the class="actions" part fixes something on its own: without it, $(this).siblings(".actions") in settings-dns.js finds nothing in the add-row, so its save button is never disabled when a cell is invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants