From 01e454414b136965da26993a5dfd50ef6bd956a6 Mon Sep 17 00:00:00 2001 From: Ilia Ross Date: Tue, 13 Oct 2026 19:02:54 +0200 Subject: [PATCH] Fix to remove Cron env check when moving jobs https://github.com/webmin/webmin/commit/c23b6b5b484a644ef9ed5e55a84f0bb8607ecd59#commitcomment-204659847 --- cron/cron-lib.pl | 21 --------------------- cron/index.cgi | 47 +++++++++------------------------------------- cron/move.cgi | 3 --- cron/t/run-tests.t | 23 +++-------------------- 4 files changed, 12 insertions(+), 82 deletions(-) diff --git a/cron/cron-lib.pl b/cron/cron-lib.pl index 1587b698f..e6e46417f 100755 --- a/cron/cron-lib.pl +++ b/cron/cron-lib.pl @@ -1357,27 +1357,6 @@ elsif ($direction eq "bottom") { return; } -=head2 cron_move_affects_env(\@jobs, &job, &target) - -Returns 1 if moving a Cron entry to the target crosses an environment -variable, or 0 otherwise. - -=cut -sub cron_move_affects_env -{ -my ($jobs, $job, $target) = @_; -return 0 if (!$job || !$target); -my ($first, $last) = sort { $a <=> $b } - ($job->{'index'}, $target->{'index'}); -for(my $i=$first; $i<=$last; $i++) { - my $entry = $jobs->[$i]; - return 1 if ($entry && - $entry->{'file'} eq $job->{'file'} && - $entry->{'name'}); - } -return 0; -} - =head2 swap_cron_jobs(&job1, &job2) Swaps two Cron jobs, which must be in the same file, identified by their diff --git a/cron/index.cgi b/cron/index.cgi index 38c56b9e5..41b60cb29 100755 --- a/cron/index.cgi +++ b/cron/index.cgi @@ -193,44 +193,15 @@ foreach $u (@ulist) { } # Show mover buttons + local $prv = $i > 0 ? $plist[$i-1]->[0] : undef; + local $nxt = $i != $#plist ? $plist[$i+1]->[0] : undef; if ($access{'move'}) { - # Hide moves that would cross a protected variable - local $upswap = &get_cron_move_target(\@jobs, $idx, "up"); - local $downswap = &get_cron_move_target(\@jobs, $idx, "down"); - local $topswap = &get_cron_move_target(\@jobs, $idx, "top"); - local $bottomswap = &get_cron_move_target(\@jobs, $idx, "bottom"); - local $movable = $job->{'name'} || - $job->{'type'} == 0 || - $job->{'type'} == 3; - local $canup = $upswap && $movable && - $upswap->{'index'} != $idx && - $upswap->{'file'} eq $job->{'file'} && - ($upswap->{'name'} || $upswap->{'type'} == 0 || - $upswap->{'type'} == 3) && - ($access{'env'} || - !&cron_move_affects_env(\@jobs, $job, $upswap)); - local $candown = $downswap && $movable && - $downswap->{'index'} != $idx && - $downswap->{'file'} eq $job->{'file'} && - ($downswap->{'name'} || $downswap->{'type'} == 0 || - $downswap->{'type'} == 3) && - ($access{'env'} || - !&cron_move_affects_env(\@jobs, $job, $downswap)); - local $cantop = $topswap && $movable && - $topswap->{'index'} != $idx && - $topswap->{'file'} eq $job->{'file'} && - ($topswap->{'name'} || $topswap->{'type'} == 0 || - $topswap->{'type'} == 3) && - ($access{'env'} || - !&cron_move_affects_env(\@jobs, $job, $topswap)); - local $canbottom = $bottomswap && $movable && - $bottomswap->{'index'} != $idx && - $bottomswap->{'file'} eq $job->{'file'} && - ($bottomswap->{'name'} || - $bottomswap->{'type'} == 0 || - $bottomswap->{'type'} == 3) && - ($access{'env'} || - !&cron_move_affects_env(\@jobs, $job, $bottomswap)); + local $canup = $prv && + $prv->{'file'} eq $job->{'file'} && + ($job->{'type'} == 0 || $job->{'type'} == 3); + local $candown = $nxt && + $nxt->{'file'} eq $job->{'file'} && + ($job->{'type'} == 0 || $job->{'type'} == 3); local $mover = "move.cgi?search=". &urlize($in{'search'})."&idx=$idx"; push(@cols, &ui_up_down_arrows( @@ -241,7 +212,7 @@ foreach $u (@ulist) { &ui_up_down_arrows( "$mover&top=1", "$mover&bottom=1", - $cantop, $canbottom, + $canup, $candown, "images/top.gif", "images/bottom.gif", )); diff --git a/cron/move.cgi b/cron/move.cgi index d1c3014d2..e9d533be8 100755 --- a/cron/move.cgi +++ b/cron/move.cgi @@ -34,9 +34,6 @@ $job->{'file'} eq $swap->{'file'} && ($job->{'name'} || $job->{'type'} == 0 || $job->{'type'} == 3) && ($swap->{'name'} || $swap->{'type'} == 0 || $swap->{'type'} == 3) || &error($text{'move_etype'}); -# Crossing a variable changes which jobs it applies to -$access{'env'} || !&cron_move_affects_env(\@jobs, $job, $swap) || - &error($text{'env_ecannot'}); &lock_file($job->{'file'}); &swap_cron_jobs($job, $swap); &unlock_file($job->{'file'}); diff --git a/cron/t/run-tests.t b/cron/t/run-tests.t index 0c3a68ab1..47943316c 100644 --- a/cron/t/run-tests.t +++ b/cron/t/run-tests.t @@ -67,17 +67,6 @@ is(get_cron_move_target(\@jobs, 0, "top"), undef, is(get_cron_move_target(\@jobs, 5, "bottom"), undef, "bottom has no target for the last entry in a file"); -ok(cron_move_affects_env(\@jobs, $jobs[2], $jobs[3]), - "an adjacent variable affects a move"); -ok(cron_move_affects_env(\@jobs, $jobs[5], $jobs[1]), - "an intermediate variable affects a top move"); -ok(cron_move_affects_env(\@jobs, $jobs[1], $jobs[5]), - "an intermediate variable affects a bottom move"); -ok(!cron_move_affects_env(\@jobs, $jobs[1], $jobs[2]), - "ordinary jobs can move without a variable"); -ok(!cron_move_affects_env(\@jobs, $jobs[0], $jobs[6]), - "variables in another file do not affect a move"); - my $cronfile = "$vardir/crontab"; my $crondir = "$vardir/crontabs"; make_path($crondir); @@ -204,13 +193,7 @@ my %full_acl = ( write_crontab(); my $original = read_crontab_fixture(); seed_acl(%full_acl, 'env' => 0); -my $result = run_cgi("move.cgi", { 'idx' => 4, 'top' => 1 }); -like($result->{'out'}, qr/not allowed to edit environment variables/i, - "top move cannot cross a protected variable"); -is(read_crontab_fixture(), $original, - "denied top move leaves the crontab unchanged"); - -$result = run_cgi("delete_cron.cgi", { 'idx' => 2 }); +my $result = run_cgi("delete_cron.cgi", { 'idx' => 2 }); like($result->{'out'}, qr/job does not exist/i, "job delete endpoint rejects a variable row"); $result = run_cgi("save_cron.cgi", { 'idx' => 2 }); @@ -287,10 +270,10 @@ foreach my $case ( "rejected $desc leaves the crontab unchanged"); } -seed_acl(%full_acl); +seed_acl(%full_acl, 'env' => 0); $result = run_cgi("move.cgi", { 'idx' => 4, 'top' => 1 }); like($result->{'out'}, qr/^Location:/m, - "a user with env permission can move a job across a variable"); + "a job can move across a variable without env permission"); isnt(read_crontab_fixture(), $original, "the permitted top move changes the crontab");