From ee420bef24642ef6c4ec4508f938f25b5a2ed9ad Mon Sep 17 00:00:00 2001 From: Deltaman-MWI <281890303+Deltaman-MWI@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:05:08 +0200 Subject: [PATCH 1/3] Allow the DMARC pct field to be left empty in bind8 RFC 9989 (DMARCbis) removes the pct tag, but the DMARC form validated the percentage as mandatory and always assigned it, so every record written through Webmin contained pct=. Treat the field like the sp field directly below it: when it is empty, delete the tag. Values 0-100 are still accepted and validated for anyone who deliberately uses pct during a rollout. join_dmarc() already skips tags with an empty value, so no change was needed there. Fixes #2843 Co-Authored-By: Claude Opus 5 (1M context) --- bind8/save_record.cgi | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/bind8/save_record.cgi b/bind8/save_record.cgi index 406eb71a6..240fcea4a 100755 --- a/bind8/save_record.cgi +++ b/bind8/save_record.cgi @@ -394,9 +394,14 @@ else { my $dmarc = $r ? &parse_dmarc(@{$r->{'values'}}) : { }; $dmarc->{'p'} = $in{'dmarcp'}; - $in{'dmarcpct'} =~ /^\d+$/ && $in{'dmarcpct'} >= 0 && - $in{'dmarcpct'} <= 100 || &error($text{'edit_edmarcpct'}); - $dmarc->{'pct'} = $in{'dmarcpct'}; + if ($in{'dmarcpct'} ne '') { + $in{'dmarcpct'} =~ /^\d+$/ && $in{'dmarcpct'} >= 0 && + $in{'dmarcpct'} <= 100 || &error($text{'edit_edmarcpct'}); + $dmarc->{'pct'} = $in{'dmarcpct'}; + } + else { + delete($dmarc->{'pct'}); + } if ($in{'dmarcsp'}) { $dmarc->{'sp'} = $in{'dmarcsp'}; From 3d27aca04a7e1c578d68bf7ad2cf709694aa3886 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Wed, 16 Sep 2026 23:00:38 +0200 Subject: [PATCH 2/3] Fix nested Apache directive rewrites This PR fixes incorrect line numbers when Apache directives contain nested blocks. It preserves comments during rewrites and correctly tracks every nested line. This prevents Virtualmin from corrupting custom Apache configurations. Fixes: https://github.com/virtualmin/virtualmin-gpl/issues/1153 --- apache/apache-lib.pl | 36 +++++++-- apache/t/directive-lines.t | 145 +++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+), 8 deletions(-) create mode 100644 apache/t/directive-lines.t diff --git a/apache/apache-lib.pl b/apache/apache-lib.pl index 59d3303e0..77371dab6 100755 --- a/apache/apache-lib.pl +++ b/apache/apache-lib.pl @@ -126,6 +126,7 @@ if (&read_file($site_file, \%site)) { # value - Value (possibly with spaces) # members - For type 1, a reference to the array of members # indent - Number of spaces before the name +# comment - Full text for a comment stored as a dummy directive sub parse_config_file { local($fh, @rv, $line, %dummy); @@ -146,8 +147,22 @@ foreach my $d (&get_httpd_defines()) { } while($line = <$fh>) { $line =~ s/\r|\n//g; - $line =~ s/^\s*#.*$//g; - if ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { + if ($line =~ /^(\s*)(#.*)$/) { + # Keep comments in the structure so block rewrites preserve them + local(%dir); + %dir = ('line', $_[1], + 'eline', $_[1], + 'file', $_[2], + 'type', 0, + 'name', 'dummy', + 'comment', $2); + local $indent = $1; + $indent =~ s/\t/ /g; + $dir{'indent'} = length($indent); + push(@rv, \%dir); + $_[1]++; + } + elsif ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { # end of a container directive. This can only happen in a # recursive call to this function $_[1]++; @@ -779,13 +794,12 @@ foreach my $dir (@$dirs) { $dir->{'line'} = $line; $dir->{'file'} = $file; if ($dir->{'type'}) { - # Do sub-members too - &recursive_set_lines_files($dir->{'members'}, $line+1, $file); - $line += scalar(grep { $_->{'name'} ne 'dummy' } - @{$dir->{'members'}})+1; + # Continue after every line used by nested members + $line = &recursive_set_lines_files($dir->{'members'}, + $line+1, $file); } $dir->{'eline'} = $line; - $line++ if ($dir->{'name'} ne 'dummy'); + $line++ if ($dir->{'name'} ne 'dummy' || defined($dir->{'comment'})); } return $line; } @@ -1931,7 +1945,13 @@ sub directive_lines { my @rv; foreach my $d (@_) { - next if ($d->{'name'} eq 'dummy'); + if ($d->{'name'} eq 'dummy') { + if (defined($d->{'comment'})) { + my $indent = (" " x $d->{'indent'}); + push(@rv, $indent.$d->{'comment'}); + } + next; + } my $indent = (" " x $d->{'indent'}); if ($d->{'type'}) { push(@rv, $indent."<$d->{'name'} $d->{'value'}>"); diff --git a/apache/t/directive-lines.t b/apache/t/directive-lines.t new file mode 100644 index 000000000..152b3aacc --- /dev/null +++ b/apache/t/directive-lines.t @@ -0,0 +1,145 @@ +#!/usr/bin/perl +# Apache directive line numbers must match their serialized positions. + +use strict; +use warnings; +use Test::More; +use File::Basename qw(dirname); +use File::Path qw(make_path); +use File::Spec; +use File::Temp qw(tempdir); +use Cwd qw(abs_path); + +my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..', '..')); +my $tmp = abs_path(tempdir(CLEANUP => 1)); +my $webmin_config = File::Spec->catdir($tmp, 'webmin-config'); +my $webmin_var = File::Spec->catdir($tmp, 'webmin-var'); +my $apache_root = File::Spec->catdir($tmp, 'apache2'); +my $apache_conf = File::Spec->catfile($apache_root, 'apache2.conf'); + +make_path($webmin_config, $webmin_var, "$webmin_config/apache", + "$webmin_var/apache", $apache_root); + +sub write_text +{ +my ($file, $text) = @_; +open(my $fh, '>', $file) || die "Failed to write $file: $!"; +print $fh $text; +close($fh) || die "Failed to close $file: $!"; +} + +sub read_text +{ +my ($file) = @_; +open(my $fh, '<', $file) || die "Failed to read $file: $!"; +local $/ = undef; +my $text = <$fh>; +close($fh) || die "Failed to close $file: $!"; +return $text; +} + +# Load the Apache module with an isolated Webmin configuration. +write_text(File::Spec->catfile($webmin_config, 'config'), + "os_type=debian-linux\n". + "os_version=12\n"); +write_text(File::Spec->catfile($webmin_config, 'miniserv.conf'), + "root=$root\n"); +write_text(File::Spec->catfile($webmin_config, 'apache', 'config'), + "httpd_dir=$apache_root\n". + "httpd_path=/bin/true\n". + "httpd_conf=$apache_conf\n". + "apachectl_path=/bin/true\n". + "httpd_version=2.4.57\n"); +write_text($apache_conf, "Listen 80\n"); + +$ENV{'WEBMIN_CONFIG'} = $webmin_config; +$ENV{'WEBMIN_VAR'} = $webmin_var; +$ENV{'FOREIGN_MODULE_NAME'} = 'apache'; +$ENV{'FOREIGN_ROOT_DIRECTORY'} = $root; +$ENV{'REMOTE_USER'} = 'root'; + +unshift(@INC, $root); +require File::Spec->catfile($root, 'apache', 'apache-lib.pl'); + +# Model a template containing inside , followed by a +# directive whose position must include every line in both nested blocks. +my $inner_require = { + 'name' => 'Require', 'value' => 'all denied', 'indent' => 8, + }; +my $files = { + 'name' => 'Files', 'value' => '*.php', 'type' => 1, 'indent' => 4, + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + $inner_require, + ], + }; +my $directory = { + 'name' => 'Directory', 'value' => '/srv/example', 'type' => 1, + 'indent' => 0, + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + { 'name' => 'Require', 'value' => 'all granted', 'indent' => 4 }, + $files, + ], + }; +my $alias = { + 'name' => 'ScriptAlias', 'value' => '/cgi-bin/ /srv/cgi-bin/', + 'indent' => 0, + }; +my @directives = ($directory, $alias); +my @lines = main::directive_lines(@directives); +my $next = main::recursive_set_lines_files(\@directives, 10, '/tmp/test.conf'); + +is($directory->{'line'}, 10, 'outer block starts at the first line'); +is($files->{'line'}, 12, 'nested block starts after the outer directive'); +is($inner_require->{'line'}, 13, 'nested member has its serialized line'); +is($files->{'eline'}, 14, 'nested block ends after all of its members'); +is($directory->{'eline'}, 15, 'outer block includes the nested closing line'); +is($alias->{'line'}, 16, 'following directive starts after the outer block'); +is($next, 10 + scalar(@lines), 'returned line follows serialized output'); + +# Parsed comments are not Apache directives, but block rewrites must retain +# their text and indentation around nested containers. +my $roundtrip = File::Spec->catfile($tmp, 'comments.conf'); +my $roundtrip_text = + "# outer comment\n". + "\n". + " # nested comment\n". + " \n". + " Require all denied\n". + " \n". + "\n"; +write_text($roundtrip, $roundtrip_text); +open(my $fh, '<', $roundtrip) || die "Failed to read $roundtrip: $!"; +my $line = 0; +my @parsed = main::parse_config_file($fh, $line, $roundtrip); +close($fh) || die "Failed to close $roundtrip: $!"; +is(join("\n", main::directive_lines(@parsed))."\n", $roundtrip_text, + 'comments survive parsing and serialization'); + +# Removing one of two parsed virtual hosts must use spans that include their +# comments, or remnants of the removed block will make Apache invalid. +my $vhosts_file = File::Spec->catfile($tmp, 'vhosts.conf'); +my $first_vhost = + "\n". + " # first comment\n". + " ServerName first.example\n". + "\n"; +my $second_vhost = + "\n". + " # second comment\n". + " ServerName second.example\n". + "\n"; +write_text($vhosts_file, $first_vhost.$second_vhost); +open($fh, '<', $vhosts_file) || die "Failed to read $vhosts_file: $!"; +$line = 0; +my @vhost_config = main::parse_config_file($fh, $line, $vhosts_file); +close($fh) || die "Failed to close $vhosts_file: $!"; +my @vhosts = main::find_directive_struct('VirtualHost', \@vhost_config); +main::save_directive_struct($vhosts[1], undef, + \@vhost_config, \@vhost_config); +main::flush_file_lines($vhosts_file); +is(read_text($vhosts_file), $first_vhost, + 'removing a block also removes all of its comments'); + +done_testing(); From faaef530f2506627c3e5bc1a51eb4087ccd544df Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Thu, 17 Sep 2026 00:01:01 +0200 Subject: [PATCH 3/3] Drop Apache comment preservation from this PR --- apache/apache-lib.pl | 29 +++----------------- apache/t/directive-lines.t | 54 -------------------------------------- 2 files changed, 4 insertions(+), 79 deletions(-) diff --git a/apache/apache-lib.pl b/apache/apache-lib.pl index 77371dab6..19033cc3c 100755 --- a/apache/apache-lib.pl +++ b/apache/apache-lib.pl @@ -126,7 +126,6 @@ if (&read_file($site_file, \%site)) { # value - Value (possibly with spaces) # members - For type 1, a reference to the array of members # indent - Number of spaces before the name -# comment - Full text for a comment stored as a dummy directive sub parse_config_file { local($fh, @rv, $line, %dummy); @@ -147,22 +146,8 @@ foreach my $d (&get_httpd_defines()) { } while($line = <$fh>) { $line =~ s/\r|\n//g; - if ($line =~ /^(\s*)(#.*)$/) { - # Keep comments in the structure so block rewrites preserve them - local(%dir); - %dir = ('line', $_[1], - 'eline', $_[1], - 'file', $_[2], - 'type', 0, - 'name', 'dummy', - 'comment', $2); - local $indent = $1; - $indent =~ s/\t/ /g; - $dir{'indent'} = length($indent); - push(@rv, \%dir); - $_[1]++; - } - elsif ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { + $line =~ s/^\s*#.*$//g; + if ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { # end of a container directive. This can only happen in a # recursive call to this function $_[1]++; @@ -799,7 +784,7 @@ foreach my $dir (@$dirs) { $line+1, $file); } $dir->{'eline'} = $line; - $line++ if ($dir->{'name'} ne 'dummy' || defined($dir->{'comment'})); + $line++ if ($dir->{'name'} ne 'dummy'); } return $line; } @@ -1945,13 +1930,7 @@ sub directive_lines { my @rv; foreach my $d (@_) { - if ($d->{'name'} eq 'dummy') { - if (defined($d->{'comment'})) { - my $indent = (" " x $d->{'indent'}); - push(@rv, $indent.$d->{'comment'}); - } - next; - } + next if ($d->{'name'} eq 'dummy'); my $indent = (" " x $d->{'indent'}); if ($d->{'type'}) { push(@rv, $indent."<$d->{'name'} $d->{'value'}>"); diff --git a/apache/t/directive-lines.t b/apache/t/directive-lines.t index 152b3aacc..3b9277b22 100644 --- a/apache/t/directive-lines.t +++ b/apache/t/directive-lines.t @@ -28,16 +28,6 @@ print $fh $text; close($fh) || die "Failed to close $file: $!"; } -sub read_text -{ -my ($file) = @_; -open(my $fh, '<', $file) || die "Failed to read $file: $!"; -local $/ = undef; -my $text = <$fh>; -close($fh) || die "Failed to close $file: $!"; -return $text; -} - # Load the Apache module with an isolated Webmin configuration. write_text(File::Spec->catfile($webmin_config, 'config'), "os_type=debian-linux\n". @@ -98,48 +88,4 @@ is($directory->{'eline'}, 15, 'outer block includes the nested closing line'); is($alias->{'line'}, 16, 'following directive starts after the outer block'); is($next, 10 + scalar(@lines), 'returned line follows serialized output'); -# Parsed comments are not Apache directives, but block rewrites must retain -# their text and indentation around nested containers. -my $roundtrip = File::Spec->catfile($tmp, 'comments.conf'); -my $roundtrip_text = - "# outer comment\n". - "\n". - " # nested comment\n". - " \n". - " Require all denied\n". - " \n". - "\n"; -write_text($roundtrip, $roundtrip_text); -open(my $fh, '<', $roundtrip) || die "Failed to read $roundtrip: $!"; -my $line = 0; -my @parsed = main::parse_config_file($fh, $line, $roundtrip); -close($fh) || die "Failed to close $roundtrip: $!"; -is(join("\n", main::directive_lines(@parsed))."\n", $roundtrip_text, - 'comments survive parsing and serialization'); - -# Removing one of two parsed virtual hosts must use spans that include their -# comments, or remnants of the removed block will make Apache invalid. -my $vhosts_file = File::Spec->catfile($tmp, 'vhosts.conf'); -my $first_vhost = - "\n". - " # first comment\n". - " ServerName first.example\n". - "\n"; -my $second_vhost = - "\n". - " # second comment\n". - " ServerName second.example\n". - "\n"; -write_text($vhosts_file, $first_vhost.$second_vhost); -open($fh, '<', $vhosts_file) || die "Failed to read $vhosts_file: $!"; -$line = 0; -my @vhost_config = main::parse_config_file($fh, $line, $vhosts_file); -close($fh) || die "Failed to close $vhosts_file: $!"; -my @vhosts = main::find_directive_struct('VirtualHost', \@vhost_config); -main::save_directive_struct($vhosts[1], undef, - \@vhost_config, \@vhost_config); -main::flush_file_lines($vhosts_file); -is(read_text($vhosts_file), $first_vhost, - 'removing a block also removes all of its comments'); - done_testing();