From 786106b15b301f32caa274f7ac21726cd79295c9 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 31 Jul 2026 22:48:38 -0400 Subject: [PATCH] Fix the 15 remaining test failures; suite is green 1176 passed, 0 failed. These were all reachable only after 9e0f045 stopped the runner exiting early, and every one had a distinct cause. One real bug, in index0.py. _build_children_view sorted entry ids with int(str(id)), which raised ValueError on any id that is not a decimal string. That happens inside view building during index load, so a single malformed id made the whole vault fail to open -- for a password manager that is indistinguishable from data loss. Non-numeric ids now sort after numeric ones rather than crashing; a bad id should degrade the ordering, not brick the index. Eight stale assertions against a schema that moved on. The index now carries a machine-managed _system.index0 block (canonical views, writer ids, scope paths) added on load, so tests that saved a literal dict and compared it back always differed by exactly that key -- the round trips themselves were correct the whole time. Adds helpers.user_data() to drop it, and uses it in the export/import, legacy-migration and nostr-fallback tests. Those tests are about user data surviving; asserting on _system would pin an implementation detail they are not about. Three checksum tests patched seams that no longer exist. They stubbed manager.verify_checksum (gone) and manager.calculate_checksum, which handle_verify_checksum no longer reaches -- it now asks _script_checksum_status, which returns early on a launch-target mismatch before any checksum is computed. Rewritten against that seam, plus a new case for the target-mismatch verdict, which nothing covered. Two tests stubbed builtins.input but not sys.stdin.isatty. Both code paths refuse a non-TTY before ever calling input, so they returned early under pytest. Uses the isatty stub the rest of the suite already uses. One test asserted the opposite of the intended contract. test_setup_encryption_manager_rejects_wrong_argon2_params expected a seed to be UNREADABLE when the profile's argon2_time_cost differs from the one it was written with. But encrypt_parent_seed stores its KdfConfig and setup_encryption_manager tries those stored params first, deliberately: the profile config says how to derive NEW keys, and refusing an existing seed because a preference changed would lock a user out of their own vault. Renamed and inverted to assert the real behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtNMsKzzFTPURemX7tT19u --- src/seedpass/core/index0.py | 22 ++++++++- src/tests/helpers.py | 15 ++++++ src/tests/test_backup_restore_startup.py | 3 ++ src/tests/test_cli_export_import.py | 6 +-- src/tests/test_kdf_modes.py | 16 ++++++- src/tests/test_legacy_migration.py | 10 ++-- src/tests/test_manager_checksum_backup.py | 47 +++++++++++++------ src/tests/test_multiple_fingerprint_prompt.py | 3 ++ .../test_nostr_legacy_decrypt_fallback.py | 4 +- src/tests/test_profile_export_import.py | 4 +- 10 files changed, 101 insertions(+), 29 deletions(-) diff --git a/src/seedpass/core/index0.py b/src/seedpass/core/index0.py index ac7d92ac..96866044 100644 --- a/src/seedpass/core/index0.py +++ b/src/seedpass/core/index0.py @@ -458,6 +458,26 @@ def _normalize_entry_summary(entry_id: str, entry: dict[str, Any]) -> dict[str, } +def _entry_sort_key(entry_id: Any) -> tuple[int, int, str]: + """Order entry ids numerically, tolerating ids that are not numbers. + + Entry ids are normally decimal strings, but an index that has been hand + edited, partially restored, or written by another tool can hold anything. + ``int()`` on such a value raised ValueError from inside view building, + which made the entire index fail to load -- for a password manager, a + vault that will not open is indistinguishable from one that is lost. + + A malformed id should degrade the ordering, not brick the index, so + non-numeric ids sort after numeric ones, lexicographically among + themselves. + """ + text = str(entry_id) + try: + return (0, int(text), "") + except ValueError: + return (1, 0, text) + + def _build_children_view( scope_path: str, entries: dict[str, Any], @@ -467,7 +487,7 @@ def _build_children_view( items = [ _normalize_entry_summary(entry_id, entry) for entry_id, entry in sorted( - _normalize_mapping(entries).items(), key=lambda item: int(str(item[0])) + _normalize_mapping(entries).items(), key=lambda item: _entry_sort_key(item[0]) ) if isinstance(entry, dict) ] diff --git a/src/tests/helpers.py b/src/tests/helpers.py index ad87a6f6..a913cbd6 100644 --- a/src/tests/helpers.py +++ b/src/tests/helpers.py @@ -22,6 +22,21 @@ TEST_SEED = ( TEST_PASSWORD = "pw" +def user_data(index: dict) -> dict: + """Return an index without its machine-managed ``_system`` block. + + ``_system.index0`` is derived state the index layer maintains (canonical + views, writer ids, scope paths). It is not user data, and it is added on + load, so a test that saves a literal dict and compares it back will always + differ by exactly that key. + + Tests about user data surviving a migration, export or round trip should + compare with this. Asserting on ``_system`` would pin an implementation + detail those tests are not about. + """ + return {key: value for key, value in index.items() if key != "_system"} + + def create_vault( dir_path: Path, seed: str = TEST_SEED, diff --git a/src/tests/test_backup_restore_startup.py b/src/tests/test_backup_restore_startup.py index 3b6256db..45cc9170 100644 --- a/src/tests/test_backup_restore_startup.py +++ b/src/tests/test_backup_restore_startup.py @@ -47,6 +47,9 @@ def test_menu_option_restores_before_init(monkeypatch, tmp_path): monkeypatch.setattr(main, "display_menu", lambda pm, **k: None) inputs = iter(["2", str(backup)]) monkeypatch.setattr("builtins.input", lambda _prompt="": next(inputs)) + # Interactive startup refuses a non-TTY stdin before reaching the menu, + # so stubbing input alone leaves main() returning 1 under pytest. + monkeypatch.setattr(main.sys.stdin, "isatty", lambda: True) rc = main.main(["--fingerprint", "fp"]) assert rc == 0 diff --git a/src/tests/test_cli_export_import.py b/src/tests/test_cli_export_import.py index b4008cef..201b6e40 100644 --- a/src/tests/test_cli_export_import.py +++ b/src/tests/test_cli_export_import.py @@ -9,7 +9,7 @@ import main from seedpass.core.portable_backup import export_backup, import_backup from seedpass.core.config_manager import ConfigManager from seedpass.core.backup import BackupManager -from helpers import create_vault, TEST_SEED +from helpers import create_vault, user_data, TEST_SEED def _setup_pm(tmp_path: Path): @@ -90,7 +90,7 @@ def test_cli_import_round_trip(monkeypatch, tmp_path): rc = main.main(["import", "--file", str(export_path)]) assert rc == 0 - assert vault.load_index() == original + assert user_data(vault.load_index()) == original def test_cli_export_import_unencrypted(monkeypatch, tmp_path): @@ -123,7 +123,7 @@ def test_cli_export_import_unencrypted(monkeypatch, tmp_path): vault.save_index({"schema_version": 4, "entries": {}}) rc = main.main(["import", "--file", str(export_path)]) assert rc == 0 - assert vault.load_index() == data + assert user_data(vault.load_index()) == data def test_cli_export_requires_file(monkeypatch, tmp_path, capsys): diff --git a/src/tests/test_kdf_modes.py b/src/tests/test_kdf_modes.py index 0e7aa570..64f09879 100644 --- a/src/tests/test_kdf_modes.py +++ b/src/tests/test_kdf_modes.py @@ -218,7 +218,7 @@ def test_tampered_kdf_wrapper_payload_is_rejected(tmp_path): wrong_mgr.decrypt_parent_seed() -def test_setup_encryption_manager_rejects_wrong_argon2_params(monkeypatch): +def test_setup_encryption_manager_uses_the_seed_s_own_kdf_params(monkeypatch): with TemporaryDirectory() as td: tmp = Path(td) fp = tmp.name @@ -255,4 +255,16 @@ def test_setup_encryption_manager_rejects_wrong_argon2_params(monkeypatch): monkeypatch.setattr(PasswordManager, "initialize_bip85", lambda self: None) monkeypatch.setattr(PasswordManager, "initialize_managers", lambda self: None) - assert not pm.setup_encryption_manager(tmp, exit_on_fail=False) + # The seed record is self-describing: encrypt_parent_seed stores the + # KdfConfig it was written with, and setup_encryption_manager tries + # those params FIRST (_load_seed_kdf_config) before anything in the + # profile config. + # + # So a profile configured with argon2_time_cost=2 still opens a seed + # written with time_cost=1. That is the correct contract, not a gap: + # the config records how to derive NEW keys, and refusing to read an + # existing seed because the current setting differs would lock a user + # out of their vault the moment they changed a KDF preference. + # + # This test previously asserted the opposite and had been failing. + assert pm.setup_encryption_manager(tmp, exit_on_fail=False) diff --git a/src/tests/test_legacy_migration.py b/src/tests/test_legacy_migration.py index 9cc12044..e6dea04c 100644 --- a/src/tests/test_legacy_migration.py +++ b/src/tests/test_legacy_migration.py @@ -5,7 +5,7 @@ from pathlib import Path import pytest -from helpers import create_vault, TEST_SEED, TEST_PASSWORD +from helpers import create_vault, user_data, TEST_SEED, TEST_PASSWORD from utils.key_derivation import derive_index_key from cryptography.fernet import Fernet from types import SimpleNamespace @@ -46,7 +46,7 @@ def test_legacy_index_migrates(monkeypatch, tmp_path: Path): monkeypatch.setattr("builtins.input", lambda *_a, **_k: "y") loaded = vault.load_index() - assert loaded == data + assert user_data(loaded) == data new_file = tmp_path / "seedpass_entries_db.json.enc" assert new_file.exists() @@ -126,12 +126,12 @@ def test_legacy_index_migration_removes_strays(monkeypatch, tmp_path: Path): # First load triggers migration and removes stray legacy files loaded = vault.load_index() - assert loaded == data + assert user_data(loaded) == data assert not stray_file.exists() # Subsequent load should not detect any legacy files loaded_again = vault.load_index() - assert loaded_again == data + assert user_data(loaded_again) == data assert (tmp_path / "seedpass_entries_db.json.enc").exists() assert list(tmp_path.glob("seedpass_passwords_db*.enc")) == [] @@ -265,7 +265,7 @@ def test_legacy_nostr_payload_syncs_when_confirmed(monkeypatch, tmp_path: Path): asyncio.run(pm.sync_index_from_nostr_async()) assert calls["sync"] == 1 - assert pm.vault.load_index() == data + assert user_data(pm.vault.load_index()) == data assert enc_mgr.last_migration_performed is False diff --git a/src/tests/test_manager_checksum_backup.py b/src/tests/test_manager_checksum_backup.py index 5eeb6325..c2061346 100644 --- a/src/tests/test_manager_checksum_backup.py +++ b/src/tests/test_manager_checksum_backup.py @@ -26,14 +26,19 @@ def _make_pm(): def test_handle_verify_checksum_success(monkeypatch, tmp_path, capsys): + # handle_verify_checksum reads its verdict from _script_checksum_status, + # which resolves the launch target and compares a recorded checksum. That + # is the seam to stub; patching calculate_checksum no longer reaches it, + # because the status helper returns early on target mismatch first. pm = _make_pm() chk_file = tmp_path / "chk.txt" chk_file.write_text("abc") monkeypatch.setattr("seedpass.core.manager.SCRIPT_CHECKSUM_FILE", chk_file) - monkeypatch.setattr("seedpass.core.manager.calculate_checksum", lambda _: "abc") + monkeypatch.setattr( + type(pm), "_script_checksum_status", lambda self: (True, tmp_path / "seedpass.py") + ) pm.handle_verify_checksum() - out = capsys.readouterr().out - assert "Checksum verification passed." in out + assert "Checksum verification passed." in capsys.readouterr().out def test_handle_verify_checksum_failure(monkeypatch, tmp_path, capsys): @@ -41,26 +46,40 @@ def test_handle_verify_checksum_failure(monkeypatch, tmp_path, capsys): chk_file = tmp_path / "chk.txt" chk_file.write_text("xyz") monkeypatch.setattr("seedpass.core.manager.SCRIPT_CHECKSUM_FILE", chk_file) - monkeypatch.setattr("seedpass.core.manager.calculate_checksum", lambda _: "abc") + monkeypatch.setattr( + type(pm), "_script_checksum_status", lambda self: (False, tmp_path / "seedpass.py") + ) + pm.handle_verify_checksum() + assert "Checksum verification failed" in capsys.readouterr().out + + +def test_handle_verify_checksum_target_mismatch(monkeypatch, tmp_path, capsys): + # verified is None when the recorded checksum belongs to a different launch + # target -- report that rather than claiming a failed verification. + pm = _make_pm() + chk_file = tmp_path / "chk.txt" + chk_file.write_text("abc") + monkeypatch.setattr("seedpass.core.manager.SCRIPT_CHECKSUM_FILE", chk_file) + monkeypatch.setattr( + type(pm), "_script_checksum_status", lambda self: (None, tmp_path / "seedpass.py") + ) pm.handle_verify_checksum() out = capsys.readouterr().out - assert "Checksum verification failed" in out + assert "does not match the current launch target" in out + assert "Checksum verification failed" not in out def test_handle_verify_checksum_missing(monkeypatch, tmp_path, capsys): pm = _make_pm() - chk_file = tmp_path / "chk.txt" - monkeypatch.setattr("seedpass.core.manager.SCRIPT_CHECKSUM_FILE", chk_file) - monkeypatch.setattr("seedpass.core.manager.calculate_checksum", lambda _: "abc") - - def raise_missing(*_args, **_kwargs): - raise FileNotFoundError - - monkeypatch.setattr("seedpass.core.manager.verify_checksum", raise_missing) + missing = tmp_path / "absent.txt" + monkeypatch.setattr("seedpass.core.manager.SCRIPT_CHECKSUM_FILE", missing) + monkeypatch.setattr( + type(pm), "_script_checksum_status", lambda self: (True, tmp_path / "seedpass.py") + ) pm.handle_verify_checksum() note = pm.notifications.get_nowait() assert note.level == "WARNING" - assert "generate script checksum" in note.message.lower() + assert "Checksum file missing" in note.message def test_backup_and_restore_database(monkeypatch, capsys): diff --git a/src/tests/test_multiple_fingerprint_prompt.py b/src/tests/test_multiple_fingerprint_prompt.py index c36dfaf2..1df0d3e8 100644 --- a/src/tests/test_multiple_fingerprint_prompt.py +++ b/src/tests/test_multiple_fingerprint_prompt.py @@ -58,6 +58,9 @@ def test_prompt_when_multiple_fingerprints(monkeypatch): return "1" # select first fingerprint monkeypatch.setattr("builtins.input", fake_input) + # The prompt refuses to read from a non-TTY before it ever calls + # input(), so stubbing input alone is not enough under pytest. + monkeypatch.setattr(manager_module.sys.stdin, "isatty", lambda: True) pm = manager_module.PasswordManager() assert calls["count"] == 1 diff --git a/src/tests/test_nostr_legacy_decrypt_fallback.py b/src/tests/test_nostr_legacy_decrypt_fallback.py index 63aa146d..8b139452 100644 --- a/src/tests/test_nostr_legacy_decrypt_fallback.py +++ b/src/tests/test_nostr_legacy_decrypt_fallback.py @@ -5,7 +5,7 @@ import unicodedata import logging from cryptography.fernet import Fernet -from helpers import create_vault, TEST_PASSWORD +from helpers import create_vault, user_data, TEST_PASSWORD import seedpass.core.encryption as enc_module @@ -35,6 +35,6 @@ def test_legacy_password_only_fallback(monkeypatch, tmp_path, caplog): caplog.set_level(logging.WARNING) assert enc_mgr.decrypt_and_save_index_from_nostr(encrypted) - assert vault.load_index() == data + assert user_data(vault.load_index()) == data assert any("legacy password-only" in rec.message for rec in caplog.records) assert calls == [50_000, 50_000, 100_000, 50_000] diff --git a/src/tests/test_profile_export_import.py b/src/tests/test_profile_export_import.py index 0929d952..68edce92 100644 --- a/src/tests/test_profile_export_import.py +++ b/src/tests/test_profile_export_import.py @@ -2,7 +2,7 @@ from pathlib import Path from types import SimpleNamespace from seedpass.core.api import VaultService -from helpers import create_vault, TEST_SEED, TEST_PASSWORD +from helpers import create_vault, user_data, TEST_SEED, TEST_PASSWORD def test_profile_export_import_round_trip(tmp_path): @@ -30,4 +30,4 @@ def test_profile_export_import_round_trip(tmp_path): service2.import_profile(blob) assert called.get("synced") is True - assert vault2.load_index() == data + assert user_data(vault2.load_index()) == data