diff --git a/backup.pl b/backup.pl index 74d0e399f..7cb2a13eb 100755 --- a/backup.pl +++ b/backup.pl @@ -2,6 +2,9 @@ # Do a scheduled virtual server backup package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + $main::no_acl_check++; require './virtual-server-lib.pl'; $host = &get_system_hostname(); @@ -338,6 +341,8 @@ package virtual_server; 'failed' => !$ok, 'sched' => $id, }); +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # Override print functions to capture output sub first_save_print { diff --git a/bwgraph.cgi b/bwgraph.cgi index f45b8bc1d..14771f7cd 100755 --- a/bwgraph.cgi +++ b/bwgraph.cgi @@ -2,6 +2,8 @@ # bwgraph.cgi # Show current bandwidth usage graphs +unless (caller) { + require './virtual-server-lib.pl'; &ReadParse(); @@ -404,6 +406,8 @@ if (&can_edit_templates() && $in{'dom'}) { push(@rets, "", $text{'index_return'}); &ui_print_footer(@rets); +} # end of unless (caller) + # usage_colours(&domain, &usage) sub usage_colours { diff --git a/check-scripts.pl b/check-scripts.pl index d15054ee4..47baa5906 100755 --- a/check-scripts.pl +++ b/check-scripts.pl @@ -11,6 +11,9 @@ =head1 check-scripts.pl =cut package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + if (!$module_name) { $main::no_acl_check++; $ENV{'WEBMIN_CONFIG'} ||= "/etc/webmin"; @@ -246,6 +249,8 @@ package virtual_server; } } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + sub patch_file { my ($script_file, $script_name, $ver_curr, $ver_new) = @_; diff --git a/downgrade-licence.pl b/downgrade-licence.pl index 336247489..95886221e 100755 --- a/downgrade-licence.pl +++ b/downgrade-licence.pl @@ -11,6 +11,9 @@ =head1 downgrade-licence.pl =cut package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + if (!$module_name) { $main::no_acl_check++; $ENV{'WEBMIN_CONFIG'} ||= "/etc/webmin"; @@ -167,6 +170,8 @@ package virtual_server; &$first_print($text{'downgrade_gpl_all_done'}); } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # lock_all_resellers() # Lock all reseller accounts sub lock_all_resellers diff --git a/functional-test.pl b/functional-test.pl index e96149279..b6dab33fe 100755 --- a/functional-test.pl +++ b/functional-test.pl @@ -2,6 +2,9 @@ # Runs all Virtualmin tests package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + if (!$module_name) { $main::no_acl_check++; $ENV{'WEBMIN_CONFIG'} ||= "/etc/webmin"; @@ -13320,6 +13323,8 @@ package virtual_server; } exit($total_failed); +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + sub run_test { local ($t) = @_; diff --git a/info.pl b/info.pl index 5e4b951cc..b00ca5873 100755 --- a/info.pl +++ b/info.pl @@ -25,6 +25,9 @@ =head1 info.pl =cut package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + if (!$module_name) { $main::no_acl_check++; $ENV{'WEBMIN_CONFIG'} ||= "/etc/webmin"; @@ -90,6 +93,8 @@ package virtual_server; } &recursive_info_dump($info, ""); +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + sub recursive_info_dump { local ($info, $indent) = @_; diff --git a/link.cgi b/link.cgi index d9909b0ef..faa00ebe5 100755 --- a/link.cgi +++ b/link.cgi @@ -321,6 +321,8 @@ if (!$$injected_ref) { return $chunk; } +unless (caller) { + &init_config(); delete($ENV{'HTTP_REFERER'}); $| = 1; @@ -369,3 +371,5 @@ else { } &close_http_connection($con); +} # end of unless (caller) + diff --git a/list-config-revisions.pl b/list-config-revisions.pl index 323512582..cabbc87cc 100755 --- a/list-config-revisions.pl +++ b/list-config-revisions.pl @@ -71,6 +71,13 @@ =head2 Options package virtual_server; +# File-scope lexicals shared between the main body and the subs below. +# Kept outside the VIRTUALMIN_NO_MAIN guard so subs see them when the +# script is `require`d for testing without the main body running. +my ($etcdir, $depth, $git_repo); + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + # If not loaded by Webmin, do standard Virtualmin environment prep if (!$module_name) { $main::no_acl_check++; @@ -88,7 +95,7 @@ package virtual_server; } # Get /etc from environment -my $etcdir = $ENV{'WEBMIN_CONFIG'}; +$etcdir = $ENV{'WEBMIN_CONFIG'}; $etcdir =~ s/\/[^\/]+$//; # Disable HTML output @@ -97,10 +104,10 @@ package virtual_server; # Parse command-line args &parse_common_cli_flags(\@ARGV); -my $depth = 1; +$depth = 1; my @module_files; my $module; -my $git_repo = "$etcdir/.git"; +$git_repo = "$etcdir/.git"; while(@ARGV > 0) { my $a = shift(@ARGV); @@ -162,6 +169,8 @@ package virtual_server; &do_list(\@source_paths, $depth, $git_repo); exit(0); +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # usage(msg) # Print usage message and exit sub usage diff --git a/lookup-domain-daemon.pl b/lookup-domain-daemon.pl index ecd8b1f93..265f1f183 100755 --- a/lookup-domain-daemon.pl +++ b/lookup-domain-daemon.pl @@ -20,6 +20,11 @@ =head1 lookup-domain-daemon.pl =cut package virtual_server; +use POSIX; +use Socket; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + $main::no_acl_check++; $ENV{'WEBMIN_CONFIG'} ||= "/etc/webmin"; $ENV{'WEBMIN_VAR'} ||= "/var/webmin"; @@ -33,8 +38,6 @@ package virtual_server; $no_virtualmin_plugins = 1; require './virtual-server-lib.pl'; $< == 0 || die "lookup-domain-daemon.pl must be run as root"; -use POSIX; -use Socket; # Parse command line $port = $config{'lookup_domain_port'} || $lookup_domain_port; @@ -136,6 +139,8 @@ package virtual_server; } } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + sub handle_one_request { # Read the username diff --git a/quotas.pl b/quotas.pl index 5490ead6c..cb1666824 100755 --- a/quotas.pl +++ b/quotas.pl @@ -3,6 +3,9 @@ # the admin for those that are over. package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + $main::no_acl_check++; $no_virtualmin_plugins = 1; require './virtual-server-lib.pl'; @@ -111,6 +114,8 @@ package virtual_server; } } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # send_domain_quota_email(&message, address) # Converts a list of domain over-quota notifications into a message, and send it sub send_domain_quota_email diff --git a/restore-config-revision.pl b/restore-config-revision.pl index cb76723f3..cc5549a08 100755 --- a/restore-config-revision.pl +++ b/restore-config-revision.pl @@ -93,6 +93,13 @@ =head2 Options package virtual_server; +# File-scope lexical shared between the main body and the subs below. +# Kept outside the VIRTUALMIN_NO_MAIN guard so subs see it when the +# script is `require`d for testing without the main body running. +my ($etcdir); + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + # If not loaded by Webmin, do standard Virtualmin environment prep if (!$module_name) { $main::no_acl_check++; @@ -110,7 +117,7 @@ package virtual_server; } # Get /etc from environment -my $etcdir = $ENV{'WEBMIN_CONFIG'}; +$etcdir = $ENV{'WEBMIN_CONFIG'}; $etcdir =~ s/\/[^\/]+$//; # Disable HTML output @@ -119,12 +126,10 @@ package virtual_server; # Parse command-line args &parse_common_cli_flags(\@ARGV); -my $target_dir; -my $dry_run; -my $depth = 1; +$depth = 1; my @module_files; my $module; -my $git_repo = "$etcdir/.git"; +$git_repo = "$etcdir/.git"; while(@ARGV > 0) { my $a = shift(@ARGV); @@ -199,6 +204,8 @@ package virtual_server; &do_restore(\@source_paths, $depth, $git_repo, $target_dir, $dry_run); exit(0); +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # usage(msg) # Print usage message and exit sub usage diff --git a/spamtrap.pl b/spamtrap.pl index 1ba509a8d..066948c02 100755 --- a/spamtrap.pl +++ b/spamtrap.pl @@ -3,6 +3,9 @@ # domains with spam emailed. package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + $main::no_acl_check++; $no_virtualmin_plugins = 1; require './virtual-server-lib.pl'; @@ -246,6 +249,8 @@ package virtual_server; &clear_index_file($hamf->{'file'}); } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # find_user_by_email(email, &users, &aliases) sub find_user_by_email { diff --git a/t/README.md b/t/README.md new file mode 100644 index 000000000..f1ce47fd1 --- /dev/null +++ b/t/README.md @@ -0,0 +1,176 @@ +# Virtualmin test suite + +This `t/` tree holds tests for virtualmin-gpl. Companion infrastructure +lives in the Webmin repo (`webmin/t/`); the patterns here mirror it. + +`functional-test.pl` already provides significant integration testing, but +likely needs some work to make it usable via `prove` and in CI. + +## Running tests + +```sh +prove -lr t # everything under t/ +prove t/compile.t # one test file +VIRTUALMIN_COMPILE_T_FILTER='^\./backup' prove t/compile.t # one area +``` + +`prove` and `Test::More` are core; on RPM-based distros, install +`perl-Test-Harness`. + +## What's here + +| File | What it checks | +| --- | --- | +| `compile.t` | Every `.pl` and `.cgi` parses cleanly (`perl -c`). Catches breakage from bulk refactors without exercising every page. ~1s for the full tree (537 files). | + +## The require-and-stub pattern + +Most Virtualmin scripts mix sub definitions with a main body that opens +the Webmin config, reads `/etc/webmin/virtual-server/*`, talks to MySQL, +or runs CLI work. To test individual subs in isolation we `require` the +script as a library without running the main body. + +Virtualmin's idiom — script body runs at file scope, helper subs are +defined alongside or below — calls for the **block-wrap** form. The +guard differs between CLI `.pl` scripts and `.cgi` files: + +```perl +#!/usr/local/bin/perl +package virtual_server; + +unless ($ENV{VIRTUALMIN_NO_MAIN}) { # CLI .pl files + +# main body: arg parsing, the actual work +require './virtual-server-lib.pl'; +while(@ARGV > 0) { ... } +... + +} # end of guard + +sub helper { ... } +``` + +```perl +#!/usr/local/bin/perl +# A .cgi file +unless (caller) { # .cgi files only + +require './virtual-server-lib.pl'; +&ReadParse(); +... + +} # end of guard + +sub helper { ... } +``` + +**Why two guards.** Virtualmin's `execute_webmin_script` (in +`json-lib.pl`) runs CLI scripts as `do $cmd` inside an `eval "..."` +block in a forked child — this is the path Webmin Cron uses to run +scheduled jobs like `backup.pl`. Under that path, `caller` is defined +(the eval frame), so `unless (caller)` would wrongly skip the main body +and the cron job would silently no-op. `$ENV{VIRTUALMIN_NO_MAIN}` is +unaffected by `do` and is explicitly cleared by +`execute_webmin_script`'s `clean_environment()` call, so production +always runs the main body and only tests skip it. + +`.cgi` files are never loaded via `do`/`require` in production — +miniserv fork+execs them — so `unless (caller)` is sufficient there and +keeps the test mechanism identical to Webmin's. + +The `!caller(0)` one-liner form used by Webmin's `bin/` tools (`exit +main(\@ARGV) if !caller(0);`) doesn't fit either kind of file here — +Virtualmin scripts don't have a `sub main` convention. + +**In tests**: set the env var (for CLI `.pl`) before `require`: + +```perl +BEGIN { $ENV{VIRTUALMIN_NO_MAIN} = 1; } +require './backup.pl'; +# now backup.pl's subs are defined; its main body did not run +``` + +For `.cgi` files, the bare `require` is enough — `caller` is defined +inside a `.t` file and the guard skips the body. + +**Which scripts are wrapped.** The vast majority of `.pl` and `.cgi` +files have no helper subs beyond `sub usage`, so wrapping them buys +nothing — there is nothing to call from a test. The scripts currently +wrapped are the ones with non-trivial helper subs we'd plausibly want to +exercise in isolation: + +| File | Subs available to tests | +| --- | --- | +| `link.cgi` | preview/proxy logic: `parse_preview_request`, `open_target_connection`, `send_target_request`, `read_target_headers`, `read_browser_request_body`, `is_basic_auth_challenge`, `preflight_preview_request`, `print_preview_wrapper`, `rewrite_proxy_redirect`, `sanitize_proxy_headers`, `preview_blocker_markup`, `rewrite_html_chunk`, `require_local_preview_ip` | +| `bwgraph.cgi` | `usage_colours`, `minimum_day`, `usage_for_days` | +| `backup.pl` | `first_save_print`, `second_save_print`, `indent_save_print`, `outdent_save_print`, `backup_cbfunc` | +| `functional-test.pl` | `run_test`, `run_test_command`, `postgresql_login_commands`, `convert_to_encrypted`, `convert_to_dnscloud`, `convert_to_location`, `convert_to_atmail` | +| `quotas.pl` | `send_domain_quota_email`, `send_user_quota_email`, `send_single_user_quota_email`, `check_quota_threshold`, `check_quota_interval` | +| `upload-api-docs.pl` | `convert_to_html`, `extract_html_title`, `unique`, `indexof` | +| `check-scripts.pl` | `patch_file`, `ftp_size` | +| `lookup-domain-daemon.pl` | `handle_one_request`, `send_response` | +| `info.pl` | `recursive_info_dump`, `info_search_match` | +| `restore-config-revision.pl` | `normalize_paths`, `do_restore` | +| `list-config-revisions.pl` | `normalize_paths`, `do_list` | +| `spamtrap.pl` | `find_user_by_email`, `parse_received_header`, `clear_index_file` | +| `downgrade-licence.pl` (and the `downgrade-license.pl` symlink) | `lock_all_resellers`, `execute_command_error`, `revert_virtualmin_license_file` | + +Any future script that grows a testable sub should add the same guard at +the same time. + +A few of the wrapped files (`restore-config-revision.pl`, +`list-config-revisions.pl`) declare lexical `my` variables at file scope +that the subs below reference (`$etcdir` for path normalization). Those +declarations are pulled above the `unless (caller)` line so the subs +still see them when the script is `require`d for testing. Initialization +remains inside the guard, so tests can mock the values themselves. + +## Sub-stubbing in tests + +The canonical example lives in the Webmin repo: +`webmin/t/miniserv.t`. The pattern: + +1. `require` the script. The `unless (caller)` guard skips its main body. +2. Replace side-effecting subs (disk I/O, `backquote_command`, network, + logging) with capture-buffer overrides under `no warnings 'redefine'`. +3. Populate package globals (`%config`, `%text`, `%access`, etc.) + directly instead of running `init_config()` and friends. +4. Call the sub under test. Assert on contract — return values, + side-effect captures, structural properties — not on cosmetics like + exact wording or HTML class names. + +Tying tests to contract rather than rendering lets the UI evolve without +breaking the test, while still catching real regressions. + +## Tiered coverage policy + +- **Tier 1 — security-critical paths.** ACL checks (`acl_security.pl`, + `can_*` predicates in `virtual-server-lib-funcs.pl`), backup + encryption/key handling, command execution wrappers, user/password + handling. Comprehensive contract tests as scripts come under audit. +- **Tier 2 — active refactor surface.** New code, code changing in + response to ongoing audit. perlcritic and strict/warnings for all + code. +- **Tier 3 — everything else.** Covered by `compile.t`. Don't chase line + coverage on stable parsers. + +The goal is not coverage-as-a-number. It's: + +- Every parser round-trips its serializer. +- Every privilege boundary has a test. +- Every external-command call has a mock-driven test for its output + parser. + +## Caveats + +- `VIRTUALMIN_COMPILE_T_STRICT=1` turns missing-CPAN-module skips into + failures. Use this in CI on a fully-provisioned image; leave it off on + dev boxes where optional deps may be missing. +- `.pl` is also the Polish translation suffix. `compile.t` skips + `.pl` when a sibling `` (no extension) exists, so + `module.info.pl` and similar data files are excluded without a + hardcoded list. +- Virtualmin scripts expect to be invoked from the module directory + with Webmin's environment (`WEBMIN_CONFIG`, `WEBMIN_VAR`). Tests that + go beyond `perl -c` will need to either set those up in a tmpdir or + stub the loaders. diff --git a/t/compile.t b/t/compile.t new file mode 100644 index 000000000..8a8bb9fa9 --- /dev/null +++ b/t/compile.t @@ -0,0 +1,77 @@ +#!/usr/bin/perl +# Verify every .pl and .cgi in the tree parses (perl -c). +# +# Catches syntax and `use` breakage from bulk refactors without having +# to load every page in a browser. The test is the first line of defence +# for the "we changed thousands of files mechanically, did anything +# break" question. +# +# Skipped: +# - $file.pl when a sibling $file (no .pl) exists. .pl is also the +# Polish translation suffix, so module.info.pl, config.info.pl, etc. +# are data files, not Perl. +# - Files that fail only because of a missing CPAN module. The file +# itself parses, but `use Foo::Bar` can't resolve at compile time. +# Treated as a skip so missing optional deps don't gate the suite. +# Set VIRTUALMIN_COMPILE_T_STRICT=1 to turn these into failures. +# +# Narrow with VIRTUALMIN_COMPILE_T_FILTER= when iterating on a +# specific area. + +use strict; +use warnings; +use Test::More; +use File::Find; +use File::Basename qw(dirname); +use File::Spec; +use Cwd qw(abs_path); + +my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..')); +chdir($root) or die "chdir($root): $!"; + +my $filter = $ENV{VIRTUALMIN_COMPILE_T_FILTER}; +my $strict = $ENV{VIRTUALMIN_COMPILE_T_STRICT}; + +my @files; +find({ + no_chdir => 1, + wanted => sub { + return if -d; + my $name = $File::Find::name; + return unless $name =~ /\.(pl|cgi)\z/; + # Skip the Polish translations that share the .pl suffix. + if ($name =~ m{(.+)\.pl\z}) { + my $base = $1; + return if -f "$base"; + } + push(@files, $name); + }, + }, '.'); + +@files = sort @files; +@files or BAIL_OUT("found no .pl/.cgi scripts under $root"); + +if ($filter) { + @files = grep { /$filter/ } @files; + @files or do { diag("filter '$filter' matched zero files"); plan skip_all => "no files match filter"; }; + } + +diag("compile-checking " . scalar(@files) . " files"); + +for my $f (@files) { + my $rel = $f; + $rel =~ s{^\./}{}; + my $out = qx{perl -I. -c -- "$rel" 2>&1}; + if ($out =~ /\bsyntax OK\b/) { + pass("$rel compiles"); + } + elsif (!$strict && $out =~ /Can't locate (\S+\.pm) in \@INC/) { + SKIP: { skip("$rel: missing optional CPAN module $1", 1); } + } + else { + fail("$rel compiles"); + diag($out); + } + } + +done_testing(); diff --git a/upload-api-docs.pl b/upload-api-docs.pl index 3d306f529..94ecd8c13 100755 --- a/upload-api-docs.pl +++ b/upload-api-docs.pl @@ -4,6 +4,8 @@ use Pod::Simple::HTML; +unless ($ENV{VIRTUALMIN_NO_MAIN}) { + $wiki_pages_host = "virtualmin.com"; $wiki_pages_user = "virtualmin"; $wiki_pages_dir = "/home/virtualmin/virtualmin-api"; @@ -192,6 +194,8 @@ system("su $wiki_pages_su -c 'scp $tempdir/* $wiki_pages_user\@$wiki_pages_host:$wiki_pages_dir/'"); } +} # end of unless ($ENV{VIRTUALMIN_NO_MAIN}) + # convert_to_html(pod-text) # Converts a POD-format text into HTML format, and returns that and # the program summary line.