From d6418996dd37551e244481e0892fd9856788086b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 22 Jun 2026 20:35:21 -0500 Subject: [PATCH] Harden tests flagged in code review Strengthen weak-assertion / false-confidence tests: - test_alfcrypto: replace the tautological ctx_init determinism check with an independent golden vector, and add a golden Topaz decrypt vector that does not rely on the test's own inverse helper (so a systematic cipher bug is caught, not just round-trip symmetry). Pin the PC1 bad-key assertion to match="Bad key length", and load alfcrypto via the dedrm package so the test exercises the same module object that topazextract/mobidedrm import. - test_mobidedrm: pin the PC1 bad-key assertion to the guard message. - test_topazextract: make the path-traversal regression rely on the depth-independent positive oracle (the sanitised file must land inside outdir, which fails against the pre-fix code) plus an exact-contents check, instead of brittle parent-path negatives. - test_erdr2pml: narrow the import skip to only the missing-cgi case so a genuinely broken module fails loudly instead of silently skipping. Co-Authored-By: Claude Opus 4.8 --- tests/test_alfcrypto.py | 25 +++++++++++++++++++++---- tests/test_erdr2pml.py | 12 +++++++++--- tests/test_mobidedrm.py | 2 +- tests/test_topazextract.py | 11 ++++++++--- 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/tests/test_alfcrypto.py b/tests/test_alfcrypto.py index 2e96c4b..b725b70 100644 --- a/tests/test_alfcrypto.py +++ b/tests/test_alfcrypto.py @@ -6,7 +6,10 @@ import pytest import dedrm_test_utils as U -alf = U.load("alfcrypto") +# Load via the `dedrm` package so we exercise the same module object that +# topazextract/mobidedrm import through `from .alfcrypto import ...`, rather +# than a second top-level copy. +alf = U.load("alfcrypto", package="dedrm") def test_pc1_roundtrip(): @@ -18,7 +21,8 @@ def test_pc1_roundtrip(): def test_pc1_rejects_bad_key_length(): - with pytest.raises(Exception): + # Pin the specific guard, not just "some exception". + with pytest.raises(Exception, match="Bad key length"): alf.Pukall_Cipher().PC1(b"\x00" * 8, b"data............", decryption=False) @@ -42,8 +46,21 @@ def test_topaz_cipher_roundtrip(): assert recovered == plaintext -def test_topaz_ctx_init_is_deterministic(): - assert alf.Topaz_Cipher().ctx_init(b"abc") == alf.Topaz_Cipher().ctx_init(b"abc") +def test_topaz_ctx_init_golden(): + # Independent golden vector pins the key-schedule math; a bare determinism + # check (ctx_init(x) == ctx_init(x)) would pass for any implementation. + assert alf.Topaz_Cipher().ctx_init(b"abc") == [426181496, 3966955212] + + +def test_topaz_decrypt_golden(): + # Golden oracle independent of the test's own inverse helper: a fixed + # ciphertext + key must always decrypt to exactly these bytes. This catches + # a systematic state-update bug that the round-trip test cannot. + ctx = alf.Topaz_Cipher().ctx_init(b"topaz-key") + out = alf.Topaz_Cipher().decrypt(bytes(range(32)), list(ctx)).encode("latin-1") + assert out == bytes.fromhex( + "e64653fbb4c4ad06981b51a0e53cd17ad9c7bc72932626ed35b935791f4fd769" + ) def test_pbkdf2_matches_hashlib(): diff --git a/tests/test_erdr2pml.py b/tests/test_erdr2pml.py index 3b6c4f8..e0fea24 100644 --- a/tests/test_erdr2pml.py +++ b/tests/test_erdr2pml.py @@ -13,9 +13,15 @@ import dedrm_test_utils as U try: erdr2pml = U.load("erdr2pml", package="dedrm") _skip_reason = None -except Exception as exc: # pragma: no cover - depends on interpreter / deps - erdr2pml = None - _skip_reason = "erdr2pml not importable: {0}".format(exc) +except ImportError as exc: # pragma: no cover - depends on interpreter / deps + # Only tolerate the specific "no cgi backport" case (Python 3.13+ without + # legacy-cgi). Any other import error means the module is genuinely broken + # and must fail loudly rather than silently skip. + if getattr(exc, "name", None) == "cgi": + erdr2pml = None + _skip_reason = "erdr2pml needs the 'cgi' module (install legacy-cgi on Python 3.13+)" + else: + raise pytestmark = pytest.mark.skipif(erdr2pml is None, reason=_skip_reason or "") diff --git a/tests/test_mobidedrm.py b/tests/test_mobidedrm.py index 54eeff0..77dbb6a 100644 --- a/tests/test_mobidedrm.py +++ b/tests/test_mobidedrm.py @@ -16,7 +16,7 @@ def test_pc1_roundtrip(): def test_pc1_rejects_bad_key_length(): - with pytest.raises(Exception): + with pytest.raises(Exception, match="Bad key length"): md.PC1(b"\x00" * 8, b"data............", decryption=False) diff --git a/tests/test_topazextract.py b/tests/test_topazextract.py index cd86c15..3156ea5 100644 --- a/tests/test_topazextract.py +++ b/tests/test_topazextract.py @@ -41,11 +41,16 @@ def test_extractfiles_strips_path_traversal(tmp_path): book.extractFiles() - # The traversal sequence was stripped: nothing was written above outdir. + # Primary oracle (independent of traversal depth): with the fix, basename() + # strips every "../" so the record always lands INSIDE outdir. The pre-fix + # code did os.path.join(outdir, "../../evil0000.dat"), writing OUTSIDE outdir, + # so this assertion fails without the fix no matter how deep tmp_path nests. + assert (outdir / "evil0000.dat").read_bytes() == b"PWNED" + # outdir contains exactly that one sanitised file, nothing unexpected. + assert sorted(p.name for p in outdir.iterdir()) == ["evil0000.dat"] + # Defense in depth: nothing escaped to the dirs the "../../" payload targets. assert not (tmp_path / "evil0000.dat").exists() assert not (tmp_path.parent / "evil0000.dat").exists() - # The (sanitised) write landed safely inside outdir. - assert (outdir / "evil0000.dat").read_bytes() == b"PWNED" def test_extractfiles_writes_normal_record(tmp_path):