mirror of
https://github.com/PR0M3TH3AN/SeedPass.git
synced 2026-11-06 12:52:04 +00:00
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01HtNMsKzzFTPURemX7tT19u
This commit is contained in:
1 parent
e873f7f5ec
commit
786106b15b
10 files changed
+101
-29
No files matched your search
@@ -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)
|
||||
]
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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)
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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]
|
||||
@@ -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
|
||||
Reference in new issue
Block a user