diff --git a/docs/typescript_port_compatibility_matrix.md b/docs/typescript_port_compatibility_matrix.md index da625f01..1704df56 100644 --- a/docs/typescript_port_compatibility_matrix.md +++ b/docs/typescript_port_compatibility_matrix.md @@ -87,24 +87,22 @@ nostr, ssh. ## Known intentional divergences -1. **TOTP codes for entries with a non-default period/digits.** Python's - `TotpManager.current_code_from_secret` constructs `pyotp.TOTP(secret)` - with library defaults (6 digits, 30s) and ignores the entry's stored - `period`/`digits`; TS honors them. For an imported 8-digit/45s secret the - two produce different codes — and Python's would be rejected by the - issuing service. **TS behavior is believed correct; awaiting a product - decision on whether Python should be fixed to match.** Recorded by the - cross-impl suite as a `[DIVERGE]` line so it can never become silent. - -2. **`kind` backfill on legacy entries** (see the migrations row): TS fills +1. **`kind` backfill on legacy entries** (see the migrations row): TS fills `kind` from `type` at parse time for pre-v2 entries; Python leaves them `type`-only and falls back on read. The filled shape matches what Python writes for new entries, and Python reads it unchanged. -3. **Index/config file envelope**: Python wraps ciphertext in a kdf/ct JSON +2. **Index/config file envelope**: Python wraps ciphertext in a kdf/ct JSON envelope; TS writes bare ciphertext. Both implementations read both forms (Python via its legacy fallback, TS via `parseEncryptedFile`), verified in cross-impl phases A/B/F/J. +**Resolved:** TOTP codes for entries with a non-default period/digits used +to differ — Python built `pyotp.TOTP(secret)` with library defaults and +ignored the entry's recorded values, so an imported 8-digit/45s secret +produced codes the issuing service would reject. Python was fixed to honor +the entry (`src/seedpass/core/totp.py` and every call site); the cross-impl +suite now asserts equality with no divergence allowance. + Any future divergence must carry a compatibility version, a migration/fallback path, release notes, and tests (plan §6). diff --git a/js/packages/test-vectors/fixtures/manifest.json b/js/packages/test-vectors/fixtures/manifest.json index 8ae2da1f..706849d9 100644 --- a/js/packages/test-vectors/fixtures/manifest.json +++ b/js/packages/test-vectors/fixtures/manifest.json @@ -27,6 +27,6 @@ ], "fixture_version": 1, "generator": "scripts/generate_ts_port_fixtures.py", - "python_commit": "7407d65294493ed096b6b419513984a248f8f087", + "python_commit": "b4f20d417a678a3b26ef173167a44c961f0b6c6c", "redaction_policy": "All fixtures derive from public BIP-39 test mnemonics (abandon.../legal.../zoo...). Secret-shaped outputs are generated test material, never from a live profile." } diff --git a/scripts/cross_impl_check.py b/scripts/cross_impl_check.py index 4b8d9228..e2d55337 100644 --- a/scripts/cross_impl_check.py +++ b/scripts/cross_impl_check.py @@ -260,7 +260,12 @@ def py_secrets_for(index: dict, seed: str) -> dict[str, str]: secret = entry["secret"] else: secret = derive_totp_secret(seed, int(entry.get("index", 0))) - out[label] = TotpManager.current_code_from_secret(secret, FIXED_TS) + out[label] = TotpManager.current_code_from_secret( + secret, + FIXED_TS, + period=int(entry.get("period", 30)), + digits=int(entry.get("digits", 6)), + ) elif kind == "key_value": out[label] = entry["value"] elif kind == "document": @@ -332,21 +337,6 @@ def phase_a(tmp: Path) -> None: for k in py_secrets if py_secrets[k].rstrip("\n") != (ts_secrets.get(k) or "").rstrip("\n") ] - # Known divergence: Python generates TOTP codes with pyotp defaults - # (6 digits / 30s) even when the entry records different values, so an - # imported 8-digit/45s code differs. Tracked, not silently accepted. - custom_totp = { - e["label"] - for e in py_index.get("entries", {}).values() - if e.get("kind", e.get("type")) == "totp" - and (int(e.get("period", 30)) != 30 or int(e.get("digits", 6)) != 6) - } - for label in sorted(custom_totp & set(mismatches)): - note_divergence( - f"TOTP code for {label!r}", - "Python ignores the entry's period/digits (pyotp defaults); TS honors them", - ) - mismatches = [k for k in mismatches if k not in custom_totp] check( "every secret derives identically", not mismatches, @@ -417,21 +407,6 @@ def phase_b(tmp: Path) -> None: for k in py_secrets if py_secrets[k].rstrip("\n") != (ts_secrets.get(k) or "").rstrip("\n") ] - # Known divergence: Python generates TOTP codes with pyotp defaults - # (6 digits / 30s) even when the entry records different values, so an - # imported 8-digit/45s code differs. Tracked, not silently accepted. - custom_totp = { - e["label"] - for e in py_index.get("entries", {}).values() - if e.get("kind", e.get("type")) == "totp" - and (int(e.get("period", 30)) != 30 or int(e.get("digits", 6)) != 6) - } - for label in sorted(custom_totp & set(mismatches)): - note_divergence( - f"TOTP code for {label!r}", - "Python ignores the entry's period/digits (pyotp defaults); TS honors them", - ) - mismatches = [k for k in mismatches if k not in custom_totp] check("every secret derives identically", not mismatches, f"mismatched: {mismatches}") # Python's schema validation should accept the TS-written index diff --git a/src/seedpass/core/display_service.py b/src/seedpass/core/display_service.py index 52ccbbd2..669cefd5 100644 --- a/src/seedpass/core/display_service.py +++ b/src/seedpass/core/display_service.py @@ -87,8 +87,11 @@ class DisplayService: self.manager, "parent_seed", None ) secret = self.manager.entry_manager.get_totp_secret(index, key) + digits = int(entry.get("digits", 6)) while True: - code = TotpManager.current_code_from_secret(secret) + code = TotpManager.current_code_from_secret( + secret, period=period, digits=digits + ) if self.manager.secret_mode_enabled: if self._copy_to_clipboard(code): print( diff --git a/src/seedpass/core/entry_management.py b/src/seedpass/core/entry_management.py index 41dbea14..6984fdef 100644 --- a/src/seedpass/core/entry_management.py +++ b/src/seedpass/core/entry_management.py @@ -1059,12 +1059,20 @@ class EntryManager: etype != EntryType.TOTP.value and kind != EntryType.TOTP.value ): raise ValueError("Entry is not a TOTP entry") + # The entry's own period/digits govern the code; the issuing service + # was configured from those values, not from pyotp's defaults. + period = int(entry.get("period", 30)) + digits = int(entry.get("digits", 6)) if entry.get("deterministic", False) or "secret" not in entry: if parent_seed is None: raise ValueError("Seed required for derived TOTP") totp_index = int(entry.get("index", 0)) - return TotpManager.current_code(parent_seed, totp_index, timestamp) - return TotpManager.current_code_from_secret(entry["secret"], timestamp) + return TotpManager.current_code( + parent_seed, totp_index, timestamp, period=period, digits=digits + ) + return TotpManager.current_code_from_secret( + entry["secret"], timestamp, period=period, digits=digits + ) def get_totp_time_remaining(self, index: int) -> int: """Return seconds remaining in the TOTP period for the given entry.""" diff --git a/src/seedpass/core/menu_handler.py b/src/seedpass/core/menu_handler.py index ed50d68f..53c1db5d 100644 --- a/src/seedpass/core/menu_handler.py +++ b/src/seedpass/core/menu_handler.py @@ -157,15 +157,18 @@ class MenuHandler: ) data = pm.entry_manager.vault.load_index() entries = data.get("entries", {}) - totp_list: list[tuple[str, int, int, bool]] = [] + # digits travels with period: both govern the code the issuing + # service expects, and neither may fall back to pyotp defaults. + totp_list: list[tuple[str, int, int, bool, int]] = [] for idx_str, entry in entries.items(): if pm._entry_type_str(entry) == EntryType.TOTP.value and not entry.get( "archived", entry.get("blacklisted", False) ): label = entry.get("label", "") period = int(entry.get("period", 30)) + digits = int(entry.get("digits", 6)) imported = "secret" in entry - totp_list.append((label, int(idx_str), period, imported)) + totp_list.append((label, int(idx_str), period, imported, digits)) if not totp_list: pm.notify("No 2FA entries found.", level="WARNING") @@ -175,7 +178,7 @@ class MenuHandler: totp_secrets: dict[int, str] = {} key = getattr(pm, "KEY_TOTP_DET", None) or getattr(pm, "parent_seed", None) - for _, idx, _, _ in totp_list: + for _, idx, _, _, _ in totp_list: try: totp_secrets[idx] = pm.entry_manager.get_totp_secret(idx, key) except Exception: @@ -196,10 +199,12 @@ class MenuHandler: imported_list = [t for t in totp_list if t[3]] if generated: print(colored("\nGenerated 2FA Codes:", "green")) - for label, idx, period, _ in generated: + for label, idx, period, _, digits in generated: secret = totp_secrets.get(idx) code = ( - TotpManager.current_code_from_secret(secret) + TotpManager.current_code_from_secret( + secret, period=period, digits=digits + ) if secret else "ERROR" ) @@ -219,10 +224,12 @@ class MenuHandler: ) if imported_list: print(colored("\nImported 2FA Codes:", "green")) - for label, idx, period, _ in imported_list: + for label, idx, period, _, digits in imported_list: secret = totp_secrets.get(idx) code = ( - TotpManager.current_code_from_secret(secret) + TotpManager.current_code_from_secret( + secret, period=period, digits=digits + ) if secret else "ERROR" ) diff --git a/src/seedpass/core/totp.py b/src/seedpass/core/totp.py index 7164d650..0455477f 100644 --- a/src/seedpass/core/totp.py +++ b/src/seedpass/core/totp.py @@ -32,19 +32,35 @@ class TotpManager: @classmethod def current_code( - cls, seed: Union[str, bytes], index: int, timestamp: int | None = None + cls, + seed: Union[str, bytes], + index: int, + timestamp: int | None = None, + *, + period: int = 30, + digits: int = 6, ) -> str: - """Return the TOTP code for the given seed/key and index.""" + """Return the TOTP code for the given seed/key and index. + + ``period`` and ``digits`` must be the values recorded on the entry. + Falling back to pyotp's defaults produces codes the issuing service + rejects whenever an entry uses anything else. + """ secret = cls.derive_secret(seed, index) - totp = pyotp.TOTP(secret) - if timestamp is None: - return totp.now() - return totp.at(timestamp) + return cls.current_code_from_secret( + secret, timestamp, period=period, digits=digits + ) @staticmethod - def current_code_from_secret(secret: str, timestamp: int | None = None) -> str: + def current_code_from_secret( + secret: str, + timestamp: int | None = None, + *, + period: int = 30, + digits: int = 6, + ) -> str: """Return the TOTP code for a raw secret.""" - totp = pyotp.TOTP(secret) + totp = pyotp.TOTP(secret, interval=int(period), digits=int(digits)) return totp.now() if timestamp is None else totp.at(timestamp) @staticmethod diff --git a/src/seedpass_gui/app.py b/src/seedpass_gui/app.py index 414ec3ed..9e5fbc1c 100644 --- a/src/seedpass_gui/app.py +++ b/src/seedpass_gui/app.py @@ -403,6 +403,7 @@ class TotpViewerWindow(toga.Window): entry = self.entries.retrieve_entry(idx) period = int(entry.get("period", 30)) if entry else 30 + digits = int(entry.get("digits", 6)) if entry else 6 remaining = TotpManager.time_remaining(period) if hasattr(self.entries, "get_totp_secret"): if idx in self._secret_cache: @@ -414,7 +415,9 @@ class TotpViewerWindow(toga.Window): except Exception: # Skip entries where secret cannot be derived (e.g. no seed) continue - code = TotpManager.current_code_from_secret(secret) + code = TotpManager.current_code_from_secret( + secret, period=period, digits=digits + ) else: # Compatibility path for services exposing code retrieval directly. code = self.entries.get_totp_code(idx) diff --git a/src/tests/test_totp_entry.py b/src/tests/test_totp_entry.py index 92e9bf7c..6b43b66b 100644 --- a/src/tests/test_totp_entry.py +++ b/src/tests/test_totp_entry.py @@ -99,3 +99,63 @@ def test_legacy_deterministic_entry(tmp_path): exported = em.export_totp_entries(TEST_SEED) assert exported["entries"][0]["secret"] == TotpManager.derive_secret(TEST_SEED, 0) + + +def test_totp_code_honors_entry_period_and_digits(tmp_path): + """An entry's recorded period/digits govern its code. + + Regression: codes were generated with pyotp's defaults (6 digits, 30s) + regardless of what the entry recorded, so any imported 8-digit or + 45-second secret produced codes the issuing service rejects. Found by + scripts/cross_impl_check.py comparing against the TypeScript port. + """ + import pyotp + + from helpers import TEST_PASSWORD, TEST_SEED, create_vault + from seedpass.core.backup import BackupManager + from seedpass.core.config_manager import ConfigManager + from seedpass.core.entry_management import EntryManager + + vault, _enc = create_vault(tmp_path, TEST_SEED, TEST_PASSWORD) + cfg = ConfigManager(vault, tmp_path) + entry_mgr = EntryManager(vault, BackupManager(tmp_path, cfg)) + + secret = "JBSWY3DPEHPK3PXP" + timestamp = 1700000000 + + idx = entry_mgr.get_next_index() + entry_mgr.add_totp("custom", secret=secret, period=45, digits=8) + code = entry_mgr.get_totp_code(idx, timestamp=timestamp) + expected = pyotp.TOTP(secret, interval=45, digits=8).at(timestamp) + assert code == expected + assert len(code) == 8 + + # Defaults still behave exactly as before + idx_default = entry_mgr.get_next_index() + entry_mgr.add_totp("plain", secret=secret) + default_code = entry_mgr.get_totp_code(idx_default, timestamp=timestamp) + assert default_code == pyotp.TOTP(secret).at(timestamp) + assert len(default_code) == 6 + + +def test_deterministic_totp_code_honors_entry_period_and_digits(tmp_path): + """The same rule applies to derived (deterministic) TOTP entries.""" + import pyotp + + from helpers import TEST_PASSWORD, TEST_SEED, create_vault + from seedpass.core.backup import BackupManager + from seedpass.core.config_manager import ConfigManager + from seedpass.core.entry_management import EntryManager + from seedpass.core.totp import TotpManager + + vault, _enc = create_vault(tmp_path, TEST_SEED, TEST_PASSWORD) + cfg = ConfigManager(vault, tmp_path) + entry_mgr = EntryManager(vault, BackupManager(tmp_path, cfg)) + + idx = entry_mgr.get_next_index() + entry_mgr.add_totp("derived", TEST_SEED, deterministic=True, period=60, digits=8) + timestamp = 1700000000 + code = entry_mgr.get_totp_code(idx, TEST_SEED, timestamp=timestamp) + derived_secret = TotpManager.derive_secret(TEST_SEED, 0) + assert code == pyotp.TOTP(derived_secret, interval=60, digits=8).at(timestamp) + assert len(code) == 8