more test coverage improvements

testsuite/00-hello_test.py

@@ -97,3 +97,8 @@ def copy_weird(args: list, src_host: str, dst_host: str) -> None:
     )
 finally:
     os.chdir(saved)
+
+# check=True only proves a zero exit; confirm the env-var path actually copied
+# both files (as the explicit --old-args case above does).
+if not (TODIR / 'one').is_file() or not (TODIR / 'two').is_file():
+    test_fail("RSYNC_OLD_ARGS=1 copy of 'one two' failed")

testsuite/acls-default_test.py

@@ -12,7 +12,7 @@
 
 from rsyncfns import (
     SCRATCHDIR,
-    check_perms, run_rsync, test_skipped,
+    check_perms, run_rsync, test_fail, test_skipped,
 )
 
 
@@ -41,8 +41,11 @@ def testit(dirname, default_acl, file_expected, prog_expected):
     a transfer into a fresh subdir picks up the inherited perms."""
     todir = SCRATCHDIR / dirname
     todir.mkdir()
-    # Clear any inherited default ACL first.
-    subprocess.run(shlex.split(setfacl_nodef) + [str(todir)])
+    # Clear any inherited default ACL first -- and confirm it succeeded, so the
+    # no-default-ACL cases can't silently inherit the scratch dir's seeded
+    # default ACL and test the wrong base state.
+    if subprocess.run(shlex.split(setfacl_nodef) + [str(todir)]).returncode != 0:
+        test_fail(f"{dirname}: clearing the inherited default ACL failed")
     if default_acl:
         if '-k' in setfacl_nodef.split():
             opts = ['-dm', default_acl]

testsuite/alt-dest_test.py

@@ -12,7 +12,7 @@
 
 from rsyncfns import (
     CHKDIR, FROMDIR, RSYNC, SCRATCHDIR, SRCDIR, TMPDIR, TODIR,
-    checkit, hands_setup, rmtree, run_rsync,
+    checkit, hands_setup, rmtree, run_rsync, test_fail,
 )
 
 
@@ -62,6 +62,13 @@
     rmtree(TODIR)
     checkit(['-av', *maybe_inplace, f'--copy-dest={alt3dir}',
              f'{FROMDIR}/', f'{TODIR}/'], FROMDIR, TODIR)
+    # --copy-dest must COPY the unchanged candidate, not hard-link it: the
+    # result is a distinct inode from the alt-dir source (this is the property
+    # that distinguishes --copy-dest from --link-dest, which checkit's tree
+    # comparison alone does not capture).
+    if os.stat(TODIR / 'likely').st_ino == os.stat(alt3dir / 'likely').st_ino:
+        test_fail(f"--copy-dest{' --inplace' if maybe_inplace else ''} "
+                  "hard-linked 'likely' instead of copying it")
 
     for srchost in ('', 'localhost:'):
         desthost = 'localhost:' if not srchost else ''

testsuite/bare-do-open-symlink-race_test.py

@@ -92,10 +92,31 @@ def verify_outside_unchanged_or_absent(label: str, target: str) -> None:
 
 
 def run_attack(args):
-    subprocess.run(
+    """Run the (expected-to-be-contained) attack transfer and return its exit
+    code; output is discarded since the security assertion is on the filesystem."""
+    return subprocess.run(
         rsync_argv(*args),
         stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
-    )
+    ).returncode
+
+
+def positive_control():
+    """Confirm the upload module accepts an ordinary write. Without this, every
+    scenario below would also pass if the daemon refused (or failed) before the
+    vulnerable code ran -- the sentinel-unchanged check alone can't tell a
+    securely-contained attack from one that never reached the receiver path."""
+    for d in (mod, src):
+        rmtree(d)
+        d.mkdir(parents=True)
+    (src / 'legit.txt').write_text("LEGIT_IN_MODULE\n")
+    rc = run_attack(['-t', f'{src}/legit.txt', f'{daemon_url}/upload/legit.txt'])
+    if rc != 0 or not (mod / 'legit.txt').is_file() \
+            or (mod / 'legit.txt').read_text() != "LEGIT_IN_MODULE\n":
+        test_fail(f"positive control: upload module did not accept a normal "
+                  f"write (rc={rc}); attack scenarios would be vacuous")
+
+
+positive_control()
 
 
 # Scenario 3b: --inplace --backup --backup-dir=cd
@@ -105,10 +126,12 @@ def run_attack(args):
 (src / 'target.txt').write_text("NEW_DATA_FROM_SENDER\n")
 os.chmod(src / 'target.txt', 0o644)
 
-run_attack([
+rc = run_attack([
     '--inplace', '--backup', '--backup-dir=cd',
     f'{src}/target.txt', f'{daemon_url}/upload/target.txt',
 ])
+if rc >= 128:
+    test_fail(f"3b inplace+backup-dir=cd: rsync died from a signal (rc={rc})")
 verify_outside_unchanged("3b inplace+backup-dir=cd")
 
 
@@ -117,7 +140,9 @@ def run_attack(args):
 (src / 'cd').mkdir()
 os.symlink('/etc/passwd', src / 'cd' / 'sym')
 
-run_attack(['-rl', f'{src}/', f'{daemon_url}/upload_fake/'])
+rc = run_attack(['-rl', f'{src}/', f'{daemon_url}/upload_fake/'])
+if rc >= 128:
+    test_fail(f"3c-symlink: rsync died from a signal (rc={rc})")
 verify_outside_unchanged_or_absent("3c-symlink fake-super symlink push", "sym")
 
 
@@ -132,5 +157,7 @@ def run_attack(args):
 if not stat.S_ISFIFO((src / 'cd' / 'fifo').stat().st_mode):
     test_skipped("mkfifo unavailable; cannot exercise 3c-mknod")
 
-run_attack(['-rD', f'{src}/', f'{daemon_url}/upload_fake/'])
+rc = run_attack(['-rD', f'{src}/', f'{daemon_url}/upload_fake/'])
+if rc >= 128:
+    test_fail(f"3c-mknod: rsync died from a signal (rc={rc})")
 verify_outside_unchanged_or_absent("3c-mknod fake-super FIFO push", "fifo")

testsuite/chdir-symlink-race_test.py

@@ -87,13 +87,41 @@ def verify_unchanged(label: str) -> None:
 
 def run_attack(label: str, *args) -> None:
     reset_outside()
-    subprocess.run(
+    rc = subprocess.run(
         rsync_argv(*args),
         stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
-    )
+    ).returncode
+    if rc >= 128:
+        test_fail(f"{label}: rsync died from a signal (rc={rc})")
     verify_unchanged(label)
 
 
+def positive_control() -> None:
+    """Confirm the receiver writes into an ordinary in-module subdirectory, so
+    the symlink-escape scenarios below genuinely exercise the chdir path rather
+    than passing because the daemon refused (or failed) before reaching it."""
+    real = mod / 'realdir'
+    rmtree(real)
+    real.mkdir()
+    # When this test runs as root the daemon serves as 'nobody' (the module
+    # sets no uid), so make the control target world-writable; push a single
+    # file with no attribute preservation so the write never needs to own/chmod
+    # the dir -- it should land purely on the receiver's normal write path.
+    os.chmod(real, 0o777)
+    rc = subprocess.run(
+        rsync_argv(f'{src}/subdir/target.txt', f'{url}upload/realdir/'),
+        stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
+    ).returncode
+    landed = real / 'target.txt'
+    if rc != 0 or not landed.is_file() \
+            or not filecmp.cmp(landed, src / 'subdir' / 'target.txt', shallow=False):
+        test_fail(f"positive control: receiver did not write into an ordinary "
+                  f"in-module subdir (rc={rc}); attack scenarios would be vacuous")
+
+
+positive_control()
+
+
 # 1. Single file with --size-only -- receiver normally skips basis open and
 # goes straight to chmod; only the chdir-escape blocks it.
 run_attack("single-file --size-only",

testsuite/clean-fname-underflow_test.py

@@ -3,8 +3,10 @@
 #
 # Ensure clean_fname() does not read-before-buffer when collapsing "..".
 # Exercises the --server path where a crafted merge filename hits
-# clean_fname(); a non-zero exit is expected (the input is bogus), but
-# the test fails if rsync dies from a signal (status >= 128).
+# clean_fname(); the test fails if rsync dies from a signal (status >= 128)
+# AND if rsync exits 0 -- the bogus input must be rejected (clean_fname
+# collapses "a/../test" to "test", whose merge file does not exist, so rsync
+# errors out non-zero). Accepting it would mean the name was mis-collapsed.
 
 import os
 import shlex
@@ -29,5 +31,8 @@
 
 if proc.returncode >= 128:
     test_fail(f"rsync exited due to a signal (status={proc.returncode})")
+if proc.returncode == 0:
+    test_fail("rsync accepted the bogus 'a/../test' merge filter (expected a "
+              "non-zero rejection); clean_fname() may have mis-collapsed it")
 
-print("OK: clean_fname() handled 'a/../test' without crashing")
+print("OK: clean_fname() handled 'a/../test' without crashing, and rejected it")

testsuite/compress-options_test.py

@@ -9,10 +9,11 @@
 """
 
 import json
+import re
 
 from rsyncfns import (
     FROMDIR, TODIR,
-    assert_same, make_tree, rmtree, run_rsync, walk_files,
+    assert_same, make_tree, rmtree, run_rsync, test_fail, walk_files,
 )
 
 src = FROMDIR
@@ -34,14 +35,25 @@ def verify(rels, label):
 
 
 # --- --compress-choice for every advertised compressor ----------------------
+# Byte-identical output alone proves only that the option didn't corrupt data;
+# assert via --debug=NSTR (compat.c) that the requested compressor was actually
+# selected for the transfer. The trailing " (level" anchors so zlib != zlibx.
 for algo in compressors:
     rels = fresh()
-    run_rsync('-az', f'--compress-choice={algo}', f'{src}/', f'{TODIR}/')
+    proc = run_rsync('-az', f'--compress-choice={algo}', '--debug=NSTR',
+                     f'{src}/', f'{TODIR}/', capture_output=True)
+    if not re.search(rf'compress: {re.escape(algo)} \(level', proc.stdout):
+        test_fail(f"--compress-choice={algo} was not the selected compressor; "
+                  f"--debug=NSTR output:\n{proc.stdout}")
     verify(rels, f'--compress-choice={algo}')
 
-# --- --compress-level -------------------------------------------------------
+# --- --compress-level (the requested level reaches the compressor) ----------
 rels = fresh()
-run_rsync('-az', '--compress-level=9', f'{src}/', f'{TODIR}/')
+proc = run_rsync('-az', '--compress-level=9', '--debug=NSTR',
+                 f'{src}/', f'{TODIR}/', capture_output=True)
+if not re.search(r'compress: \S+ \(level 9\)', proc.stdout):
+    test_fail("--compress-level=9 was not applied; "
+              f"--debug=NSTR output:\n{proc.stdout}")
 verify(rels, '--compress-level=9')
 
 # --- --skip-compress (the file must still arrive intact) --------------------
@@ -52,9 +64,15 @@ def verify(rels, label):
             label='--skip-compress gz')
 
 # --- --checksum-choice for every advertised checksum ------------------------
+# As above: assert via --debug=NSTR (checksum.c) that the requested checksum was
+# the one negotiated, not merely that the transfer succeeded.
 for algo in checksums:
     rels = fresh()
-    run_rsync('-a', '-c', f'--checksum-choice={algo}', f'{src}/', f'{TODIR}/')
+    proc = run_rsync('-a', '-c', f'--checksum-choice={algo}', '--debug=NSTR',
+                     f'{src}/', f'{TODIR}/', capture_output=True)
+    if not re.search(rf'checksum: {re.escape(algo)}\b', proc.stdout):
+        test_fail(f"--checksum-choice={algo} was not the selected checksum; "
+                  f"--debug=NSTR output:\n{proc.stdout}")
     verify(rels, f'--checksum-choice={algo}')
 
 # --- --checksum-seed --------------------------------------------------------

testsuite/daemon-filter_test.py

@@ -40,8 +40,11 @@
 def pull(mod, dest):
     rmtree(dest)
     makepath(dest)
-    subprocess.run(rsync_argv('-a', f'{url}{mod}/', f'{dest}/'),
-                   stdout=subprocess.DEVNULL)
+    proc = subprocess.run(rsync_argv('-a', f'{url}{mod}/', f'{dest}/'),
+                          stdout=subprocess.DEVNULL, stderr=subprocess.PIPE,
+                          text=True)
+    if proc.returncode not in (0, 23):
+        test_fail(f"pull from {mod} failed (rc={proc.returncode}): {proc.stderr}")
 
 
 # --- daemon exclude hides *.secret everywhere in the module -----------------
@@ -53,8 +56,11 @@ def pull(mod, dest):
             label='daemon exclude kept others')
 
 # --- incoming chmod rewrites pushed file modes at depth ---------------------
-subprocess.run(rsync_argv('-a', f'{src}/', f'{url}inc/'),
-               stdout=subprocess.DEVNULL)
+proc = subprocess.run(rsync_argv('-a', f'{src}/', f'{url}inc/'),
+                      stdout=subprocess.DEVNULL, stderr=subprocess.PIPE,
+                      text=True)
+if proc.returncode not in (0, 23):
+    test_fail(f"incoming push failed (rc={proc.returncode}): {proc.stderr}")
 checked = 0
 for rel in rels:
     p = incdir / rel
@@ -67,10 +73,16 @@ def pull(mod, dest):
 # --- outgoing chmod rewrites pulled file modes at depth ---------------------
 op = SCRATCHDIR / 'outpull'
 pull('out', op)
+checked = 0
 for rel in rels:
     p = op / rel
-    if p.is_file() and (os.stat(p).st_mode & 0o044):
+    if not p.is_file():
+        continue
+    checked += 1
+    if os.stat(p).st_mode & 0o044:
         test_fail(f"outgoing chmod did not clear group/other read on {rel}: "
                   f"{oct(os.stat(p).st_mode & 0o777)}")
+if checked == 0:
+    test_fail("outgoing chmod test pulled no files (loop was vacuous)")
 
 print("daemon-filter: exclude / incoming chmod / outgoing chmod verified at depth")

testsuite/daemon-refuse_test.py

@@ -44,19 +44,23 @@ def refused(args, what):
     return proc.stderr
 
 
-def allowed(args, what):
+def allowed(args, what, expected=None, actual=None):
     proc = subprocess.run(rsync_argv(*args),
                           stdout=subprocess.DEVNULL, stderr=subprocess.PIPE,
                           text=True)
     if proc.returncode not in (0, 23):
         test_fail(f"{what} was unexpectedly refused: {proc.stderr}")
+    # A 0/23 exit alone doesn't prove the transfer happened; verify the data
+    # actually landed for the allowed cases.
+    if expected is not None:
+        verify_dirs(expected, actual, label=what)
 
 
 # --- a named refused option (delete) ----------------------------------------
 refused(['-a', '--delete', f'{src}/', f'{url}refuse-delete/'],
         "--delete on a refuse=delete module")
 allowed(['-a', f'{src}/', f'{url}refuse-delete/'],
-        "plain push to a refuse=delete module")
+        "plain push to a refuse=delete module", src, deldir)
 
 # --- a wildcard refused option (checksum*) ----------------------------------
 dest = SCRATCHDIR / 'wilddest'
@@ -67,7 +71,8 @@ def allowed(args, what):
 # --- the "* !a !v" allow-list: -av allowed, -z refused ----------------------
 rmtree(dest)
 makepath(dest)
-allowed(['-av', f'{url}only-av/', f'{dest}/'], "-av on an allow-list module")
+allowed(['-av', f'{url}only-av/', f'{dest}/'], "-av on an allow-list module",
+        src, dest)
 refused(['-avz', f'{url}only-av/', f'{dest}/'],
         "-z on an allow-list module")