From 3d27aca04a7e1c578d68bf7ad2cf709694aa3886 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Wed, 16 Sep 2026 23:00:38 +0200 Subject: [PATCH] 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();