diff --git a/apache/apache-lib.pl b/apache/apache-lib.pl index c71d50a43..ec8d8804e 100755 --- a/apache/apache-lib.pl +++ b/apache/apache-lib.pl @@ -126,9 +126,10 @@ 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 +# comments - Full comment lines immediately before this directive sub parse_config_file { -local($fh, @rv, $line, %dummy); +local($fh, @rv, $line, %dummy, @comments); $fh = $_[0]; $dummy{'line'} = $dummy{'eline'} = $_[1]-1; $dummy{'file'} = $_[2]; @@ -146,8 +147,13 @@ 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*#/) { + # Attach comments to the next directive without reusing the + # dummy entry that marks the start of this block + push(@comments, $line); + $_[1]++; + } + elsif ($line =~ /^\s*<\/(\S+)\s*(.*)>/) { # end of a container directive. This can only happen in a # recursive call to this function $_[1]++; @@ -178,10 +184,13 @@ while($line = <$fh>) { $not && !$httpd_modules{$mpmmod} ) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -199,10 +208,13 @@ while($line = <$fh>) { if (!$not && defined($defs{$def}) || $not && !defined($defs{$def})) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -247,10 +259,13 @@ while($line = <$fh>) { $match = !$match if ($not); if ($match) { # use the directives.. - push(@rv, { 'line', $oldline, - 'eline', $oldline, - 'file', $_[2], - 'name', "" }); + local %open = ( 'line', $oldline, + 'eline', $oldline, + 'file', $_[2], + 'name', "" ); + $open{'comments'} = [ @comments ] if (@comments); + @comments = ( ); + push(@rv, \%open); push(@rv, @dirs); push(@rv, { 'line', $_[1]-1, 'eline', $_[1]-1, @@ -276,6 +291,8 @@ while($line = <$fh>) { $dir{'eline'} = $_[1]-1; $indent =~ s/\t/ /g; $dir{'indent'} = length($indent); + $dir{'comments'} = [ @comments ] if (@comments); + @comments = ( ); push(@rv, \%dir); } elsif ($line =~ /^(\s*)(\S+)\s*(.*)$/) { @@ -309,11 +326,15 @@ while($line = <$fh>) { } } $dir{'words'} = &wsplit($dir{'value'}); + $dir{'comments'} = [ @comments ] if (@comments); + @comments = ( ); push(@rv, \%dir); $_[1]++; } else { - # blank or comment line + # Keep blank lines that separate a pending comment block from + # the directive it describes + push(@comments, $line) if (@comments); $_[1]++; } } @@ -636,9 +657,12 @@ for($i=0; $i<@old || $i<@{$_[1]}; $i++) { push(@files, $old[$i]->{'file'}); $idx = &indexof($old[$i], @{$_[2]}); splice(@{$_[2]}, $idx, 1); - $len = $old[$i]->{'eline'} - $old[$i]->{'line'} + 1; - splice(@$lref, $old[$i]->{'line'}, $len); - &renumber($_[3], $old[$i]->{'line'}, $old[$i]->{'file'}, -$len); + local $comments = $old[$i]->{'comments'} ? + scalar(@{$old[$i]->{'comments'}}) : 0; + local $start = $old[$i]->{'line'}-$comments; + $len = $old[$i]->{'eline'}-$start+1; + splice(@$lref, $start, $len); + &renumber($_[3], $start, $old[$i]->{'file'}, -$len); } else { # just changing the value @@ -673,9 +697,16 @@ return if (!$olddir && !$newdir); # Nothing to do local $file = $olddir ? $olddir->{'file'} : $newdir->{'file'} ? $newdir->{'file'} : $pconf->[0]->{'file'}; local $lref = &read_file_lines($file); -local $oldlen = $olddir ? $olddir->{'eline'}-$olddir->{'line'}+1 : undef; +local $oldcomments = $olddir && $olddir->{'comments'} ? + scalar(@{$olddir->{'comments'}}) : 0; +local $oldline = $olddir ? $olddir->{'line'}-$oldcomments : undef; +local $oldlen = $olddir ? $olddir->{'eline'}-$oldline+1 : undef; local @newlines; if ($newdir) { + if ($olddir && !exists($newdir->{'comments'}) && $oldcomments) { + # A replacement inherits the comments attached to the old block + $newdir->{'comments'} = [ @{$olddir->{'comments'}} ]; + } my $isrc = $olddir ? $olddir : @$pconf ? $pconf->[0] : undef; if ($isrc) { @@ -689,7 +720,9 @@ if ($olddir && $newdir) { $newdir->{'words'} = &wsplit($newdir->{'value'}); if ($first) { # Just changing first and last line, like virtualhost IP - $lref->[$olddir->{'line'}] = $newlines[0]; + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $lref->[$olddir->{'line'}] = $newlines[$comments]; $lref->[$olddir->{'eline'}] = $newlines[$#newlines]; $olddir->{'name'} = $newdir->{'name'}; $olddir->{'value'} = $newdir->{'value'}; @@ -702,9 +735,11 @@ if ($olddir && $newdir) { local $idx = &indexof($olddir, @$pconf); $pconf->[$idx] = $newdir if ($idx >= 0); $newdir->{'file'} = $olddir->{'file'}; - $newdir->{'line'} = $olddir->{'line'}; - $newdir->{'eline'} = $olddir->{'line'}+scalar(@newlines)-1; - splice(@$lref, $olddir->{'line'}, $oldlen, @newlines); + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $newdir->{'line'} = $oldline+$comments; + $newdir->{'eline'} = $oldline+scalar(@newlines)-1; + splice(@$lref, $oldline, $oldlen, @newlines); # Update sub-directive lines and files too if ($newdir->{'type'}) { @@ -716,10 +751,10 @@ if ($olddir && $newdir) { } elsif ($olddir && !$newdir) { # Remove - splice(@$lref, $olddir->{'line'}, $oldlen); + splice(@$lref, $oldline, $oldlen); local $idx = &indexof($olddir, @$pconf); splice(@$pconf, $idx, 1) if ($idx >= 0); - &renumber($conf, $olddir->{'line'}, $olddir->{'file'}, -$oldlen); + &renumber($conf, $oldline, $olddir->{'file'}, -$oldlen); } elsif (!$olddir && $newdir) { # Add to file, at end of specific file or parent section @@ -739,7 +774,9 @@ elsif (!$olddir && $newdir) { } $newdir->{'words'} = &wsplit($newdir->{'value'}); $newdir->{'file'} = $file; - $newdir->{'line'} = $addline; + local $comments = $newdir->{'comments'} ? + scalar(@{$newdir->{'comments'}}) : 0; + $newdir->{'line'} = $addline+$comments; $newdir->{'eline'} = $addline + scalar(@newlines) - 1; &renumber($conf, $addline, $file, scalar(@newlines)); splice(@$pconf, $addpos, 0, $newdir); @@ -776,6 +813,8 @@ sub recursive_set_lines_files { my ($dirs, $line, $file) = @_; foreach my $dir (@$dirs) { + # A directive's line follows any comments attached to it + $line += scalar(@{$dir->{'comments'}}) if ($dir->{'comments'}); $dir->{'line'} = $line; $dir->{'file'} = $file; if ($dir->{'type'}) { @@ -1930,6 +1969,7 @@ sub directive_lines my @rv; foreach my $d (@_) { next if ($d->{'name'} eq 'dummy'); + push(@rv, @{$d->{'comments'}}) if ($d->{'comments'}); my $indent = (" " x $d->{'indent'}); if ($d->{'type'}) { push(@rv, $indent."<$d->{'name'} $d->{'value'}>"); diff --git a/apache/t/directive-comments.t b/apache/t/directive-comments.t new file mode 100644 index 000000000..2deebd9d7 --- /dev/null +++ b/apache/t/directive-comments.t @@ -0,0 +1,214 @@ +#!/usr/bin/perl +# Full-line comments must survive Apache block rewrites. + +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'); + +# Comments at multiple indentation levels must round-trip with the directives. +my $roundtrip = File::Spec->catfile($tmp, 'comments.conf'); +my $roundtrip_text = + "# outer comment\n". + "\n". + " # nested comment\n". + "\n". + " Require all granted\n". + "\n"; +write_text($roundtrip, $roundtrip_text); +open(my $fh, '<', $roundtrip) || die "Failed to read $roundtrip: $!"; +my $line = 0; +my @roundtrip_config = main::parse_config_file($fh, $line, $roundtrip); +close($fh) || die "Failed to close $roundtrip: $!"; +my ($directory) = + main::find_directive_struct('Directory', \@roundtrip_config); +my ($require) = + main::find_directive_struct('Require', $directory->{'members'}); +is_deeply($directory->{'comments'}, [ '# outer comment' ], + 'outer comment is attached to the directory'); +is_deeply($require->{'comments'}, [ ' # nested comment', '' ], + 'nested comment block is attached to the following directive'); +ok(!$roundtrip_config[0]->{'comments'} && + !$directory->{'members'}->[0]->{'comments'}, + 'dummy block markers do not store comments'); +is(join("\n", main::directive_lines(@roundtrip_config))."\n", + $roundtrip_text, 'comments survive parsing and serialization'); + +# Rewriting and removing virtual hosts must handle attached comments with them. +my $vhosts_file = File::Spec->catfile($tmp, 'vhosts.conf'); +my $first_vhost = + "# first virtual host\n". + "\n". + " # first comment\n". + "\n". + " ServerName first.example\n". + "\n"; +my $second_vhost = + "# second virtual host\n". + "\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::recursive_set_lines_files(\@vhost_config, 0, $vhosts_file); +is($vhosts[0]->{'line'}, 1, 'block line follows its attached comment'); +is($vhosts[0]->{'eline'}, 5, 'member comment is included in block line count'); +is($vhosts[1]->{'line'}, 7, 'next block follows its attached comment'); + +my %replacement = %{$vhosts[0]}; +delete($replacement{'comments'}); +$replacement{'value'} = '*:8080'; +my $rewritten_first = $first_vhost; +$rewritten_first =~ s/\*:80>/*:8080>/; +main::save_directive_struct($vhosts[0], \%replacement, + \@vhost_config, \@vhost_config); +main::flush_file_lines($vhosts_file); +is(read_text($vhosts_file), $rewritten_first.$second_vhost, + 'replacement block preserves surrounding comments'); +is_deeply($replacement{'comments'}, [ '# first virtual host' ], + 'replacement block inherits attached comments'); + +main::save_directive_struct($vhosts[1], undef, + \@vhost_config, \@vhost_config); +main::flush_file_lines($vhosts_file); +is(read_text($vhosts_file), + $rewritten_first, + 'deleting a block removes its attached comment'); + +# Normal directive edits preserve comments, while deletion removes their owner. +my $directives_file = File::Spec->catfile($tmp, 'directives.conf'); +my $directives_text = + "# listen comment\n". + "Listen 80\n". + "# server name comment\n". + "ServerName old.example\n"; +write_text($directives_file, $directives_text); +open($fh, '<', $directives_file) || + die "Failed to read $directives_file: $!"; +$line = 0; +my @directive_config = + main::parse_config_file($fh, $line, $directives_file); +close($fh) || die "Failed to close $directives_file: $!"; + +main::save_directive('ServerName', [ 'new.example' ], + \@directive_config, \@directive_config); +main::flush_file_lines($directives_file); +my $updated_directives = + "# listen comment\n". + "Listen 80\n". + "# server name comment\n". + "ServerName new.example\n"; +is(read_text($directives_file), $updated_directives, + 'comments survive a normal directive edit'); + +main::save_directive('Listen', [ ], \@directive_config, \@directive_config); +main::flush_file_lines($directives_file); +my $deleted_directive = + "# server name comment\n". + "ServerName new.example\n"; +is(read_text($directives_file), $deleted_directive, + 'deleting a directive removes its attached comment'); + +open($fh, '<', $directives_file) || + die "Failed to read $directives_file: $!"; +$line = 0; +@directive_config = main::parse_config_file($fh, $line, $directives_file); +close($fh) || die "Failed to close $directives_file: $!"; +my ($servername) = + main::find_directive_struct('ServerName', \@directive_config); +is_deeply($servername->{'comments'}, + [ '# server name comment' ], + 'remaining comment stays attached to its directive'); + +# Nested block positions must include comments at every level. +my $inner = { + 'name' => 'Files', 'value' => '*.php', 'type' => 1, + 'comments' => [ ' # files comment' ], + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + { 'name' => 'Require', 'value' => 'all denied', + 'comments' => [ ' # access comment' ] }, + ], + }; +my $outer = { + 'name' => 'Directory', 'value' => '/srv/example', 'type' => 1, + 'members' => [ + { 'name' => 'dummy', 'type' => 0 }, + $inner, + { 'name' => 'Options', 'value' => 'Indexes' }, + ], + }; +my @nested = ($outer); +my @nested_lines = main::directive_lines(@nested); +my $next_line = main::recursive_set_lines_files(\@nested, 20, $vhosts_file); +is($inner->{'line'}, 22, 'nested block line follows its comment'); +is($inner->{'members'}->[1]->{'line'}, 24, + 'nested member line follows its comment'); +is($outer->{'members'}->[2]->{'line'}, 26, + 'directive after nested block has the correct line'); +is($outer->{'eline'}, 27, 'outer block ends after all nested lines'); +is($next_line, 20 + scalar(@nested_lines), + 'line count matches nested serialized output'); + +done_testing();