From fa0d14a223aed23b1967b0621e5f1c349445b6f1 Mon Sep 17 00:00:00 2001 From: LIghtJUNction Date: Fri, 2 Oct 2026 02:30:20 +0800 Subject: [PATCH 1/3] test: reproduce ambiguous account recovery and listing side effects --- .github/workflows/service-aliases.yml | 2 + tests/test_client_account_recovery.py | 148 ++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 tests/test_client_account_recovery.py diff --git a/.github/workflows/service-aliases.yml b/.github/workflows/service-aliases.yml index fe431be5..6e0137d1 100644 --- a/.github/workflows/service-aliases.yml +++ b/.github/workflows/service-aliases.yml @@ -56,6 +56,8 @@ jobs: run: >- uv run --locked --extra dev python -m pytest tests/test_service_aliases.py tests/test_client_connection.py + tests/test_client_accounts.py tests/test_client_account_recovery.py + tests/test_client_paths.py tests/test_hosting_alias_ingress.py tests/test_backup_daemon.py tests/test_config_safety.py tests/test_configuration_field_contracts.py tests/test_execution_composition.py tests/test_cli_cursor_output.py diff --git a/tests/test_client_account_recovery.py b/tests/test_client_account_recovery.py new file mode 100644 index 00000000..584e4fe6 --- /dev/null +++ b/tests/test_client_account_recovery.py @@ -0,0 +1,148 @@ +"""Restoring account files must not silently choose or replace an identity.""" + +from types import SimpleNamespace + +import pytest + +from msg.atomic_file import durable_write +from msg.client import ClientState +from msg.client_accounts import run_command +from msg.core.codec import canonical +from msg.core.errors import Failure +from msg.paths import ClientPaths +from msg.security.crypto import Ed25519Signer + +SERVER = 'https://example.org' + + +@pytest.fixture +def home(monkeypatch, tmp_path): + monkeypatch.setenv('HOME', str(tmp_path)) + monkeypatch.setenv('MSG_SERVER', SERVER) + for key in ('XDG_DATA_HOME', 'XDG_STATE_HOME', 'XDG_CACHE_HOME', 'XDG_CONFIG_HOME'): + monkeypatch.delenv(key, raising=False) + return tmp_path + + +def args(action='list', **extra): + return SimpleNamespace( + action=action, config_dir=None, server=SERVER, profile=None, account=None, **extra + ) + + +def registered(account): + state = ClientState(account=account) + state.save_signer(Ed25519Signer.generate()) + state.data.update(handle=account, subject_id=state.signer.key_id) + state._save() + return state + + +def snapshot(root): + return { + str(path.relative_to(root)): (path.stat().st_mode, path.read_bytes()) + for path in root.rglob('*') + if path.is_file() + } + + +@pytest.mark.parametrize('accounts', [('alice',), ('default', 'bob')]) +def test_missing_selection_requires_explicit_choice_without_writes(home, accounts): + states = [registered(account) for account in accounts] + before = snapshot(home) + with pytest.raises(Failure, match='local_account_selection_required'): + ClientState() + assert snapshot(home) == before + listing = run_command(args()) + assert {item['account'] for item in listing['accounts']} == set(accounts) + assert not any(item['selected'] for item in listing['accounts']) + assert snapshot(home) == before + run_command(args('use', name=accounts[-1])) + assert ClientState().subject == states[-1].subject + assert ClientState().signer.public_key == states[-1].signer.public_key + + +def test_account_list_on_fresh_service_creates_no_files(home): + assert run_command(args()) == {'server': SERVER, 'accounts': []} + assert list(home.iterdir()) == [] + + +def test_account_list_does_not_migrate_legacy_identity(home): + paths = ClientPaths.discover(server=SERVER) + old = ClientState(paths=paths, server=SERVER) + old.save_signer(Ed25519Signer.generate()) + old.data.update(handle='alice', subject_id=old.signer.key_id) + old._save() + before = snapshot(home) + listing = run_command(args()) + assert listing['accounts'] == [] + assert snapshot(home) == before + assert ClientState().signer.public_key == old.signer.public_key + + +def test_stale_selection_never_imports_another_legacy_identity(home): + bob = registered('bob') + run_command(args('use', name='bob')) + bob.path.unlink() + old = ClientState(ClientPaths.discover().config) + old.save_signer(Ed25519Signer.generate()) + old.data.update(handle='alice', subject_id=old.signer.key_id) + old._save() + before = snapshot(home) + with pytest.raises(Failure, match='local_account_not_found'): + ClientState() + assert snapshot(home) == before + assert old.key_path.exists() + + +def test_empty_selected_account_does_not_import_different_legacy_handle(home): + bob = ClientState(account='bob') + marker = ClientPaths.discover(server=SERVER).config / 'current-account.json' + durable_write(marker, canonical({'version': 1, 'account': 'bob'}), mode=0o600) + old = ClientState(ClientPaths.discover().config) + old.save_signer(Ed25519Signer.generate()) + old.data.update(handle='alice', subject_id=old.signer.key_id) + old._save() + before = snapshot(home) + selected = ClientState() + assert selected.account == 'bob' and selected.signer is None + assert selected.path == bob.path + assert snapshot(home) == before + + +def test_rejected_use_keeps_previous_service_and_selection(home, monkeypatch): + alice = registered('alice') + run_command(args('use', name='alice')) + other = 'https://other.example.org' + ClientState(server=other, account='empty') + ClientState(server=SERVER) + monkeypatch.delenv('MSG_SERVER') + before = snapshot(home) + request = args('use', name='empty') + request.server = other + with pytest.raises(Failure, match='local_account_not_authenticated'): + run_command(request) + assert snapshot(home) == before + assert ClientState().subject == alice.subject + + +def test_invocation_override_does_not_mark_default_as_selected(home): + registered('alice') + registered('bob') + run_command(args('use', name='alice')) + request = args() + request.account = 'bob' + before = snapshot(home) + listing = run_command(request)['accounts'] + assert [item['account'] for item in listing if item['selected']] == ['bob'] + assert snapshot(home) == before + assert ClientState().account == 'alice' + + +def test_restored_key_without_state_still_requires_account_selection(home): + state = registered('alice') + state.path.unlink() + before = snapshot(home) + with pytest.raises(Failure, match='local_account_selection_required'): + ClientState() + assert snapshot(home) == before From 6dfab5ae2c2c1d2b285a9d77280233008a9c1d03 Mon Sep 17 00:00:00 2001 From: LIghtJUNction Date: Fri, 2 Oct 2026 02:33:59 +0800 Subject: [PATCH 2/3] fix: make recovered account selection explicit and listing read-only --- docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md | 37 ++++++++++++++ docs/FILESYSTEM_LAYOUT.md | 4 ++ src/msg/client.py | 20 +++++++- src/msg/client_accounts.py | 62 +++++++++++++++--------- src/msg/paths.py | 27 +++++++++++ tests/test_client_account_recovery.py | 61 +++++++++++++++++++++++ 6 files changed, 185 insertions(+), 26 deletions(-) create mode 100644 docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md diff --git a/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md b/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md new file mode 100644 index 00000000..852da2b2 --- /dev/null +++ b/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md @@ -0,0 +1,37 @@ +# Account selection and recovery — issue #219 + +Source baseline: `4bbdbca52b54f8d2de36a7a04b5207a9b9951d61` (msgctl 0.2.11). +Scope: client account selection, metadata listing and legacy migration boundaries. + +## Reproductions and changes + +- Restored account directories without `current-account.json` silently fall back + to `default`, even when that is a different saved identity. Require an explicit + selection when saved accounts exist and no legacy service singleton remains. +- A selected account with missing state can accidentally import a different old + profile or create an empty replacement. Reject the missing selected state before + migration; apply the legacy-handle guard to saved selections as well as explicit + `--account` arguments. Explicit import remains available for recovery. +- `account list` creates state, changes selection and can move legacy credentials. + Inspect config/data/state account directories and safe metadata without creating + files, loading keys or migrating anything. Show incomplete accounts as well. +- `account use` can update the remembered service before rejecting an + unauthenticated account. Check the saved subject before constructing client + state. Failed selection preserves the previous service and account. +- The list's `selected` flag now reflects an invocation-only `--account` override, + while the persistent default remains unchanged. + +## Validation + +`tests/test_client_account_recovery.py` adds regression coverage. Existing account, +connection and path tests continue to cover singleton migration, profile aliases, +hardware/software separation, imports, keys and journals. The service-alias CI +workflow includes all these files and records source and JUnit evidence. + +The first PR commit contains only tests and workflow wiring, for baseline failure +evidence. The following implementation commit is verified separately. Exact run +results belong in PR #232; this file does not predeclare CI success. + +Issue #219 also includes server-account retirement and full recovery acceptance. +Those are separate from this code change; the issue stays open. No production +account, credential, release or deployment is changed by this work. diff --git a/docs/FILESYSTEM_LAYOUT.md b/docs/FILESYSTEM_LAYOUT.md index 716816ec..8d4c560c 100644 --- a/docs/FILESYSTEM_LAYOUT.md +++ b/docs/FILESYSTEM_LAYOUT.md @@ -30,6 +30,10 @@ Local names allow letters, digits, underscores, dots and hyphens, begin with a l The service default is stored in `~/.config/msg/services//current-account.json`. An invocation override does not change it. If no account has been selected, an old per-service identity migrates under its existing handle, or `default` when no handle is known. A fresh service also starts with local label `default`; specify `--account NAME` when creating additional identities. Existing `hardware-signer.json` is account state, not a private key. Its path is `~/.local/state/msg/services//accounts//hardware-signer.json`. +If account directories have been restored without the selection record and no old per-service identity remains, ordinary commands stop with `local_account_selection_required`. Run `msg account list`, then explicitly choose `msg account use NAME` or the invocation-only `--account NAME`. A selection pointing to a missing `client.json` stops with `local_account_not_found`; it never silently creates an empty replacement or imports a different legacy identity. An explicit account import can restore the missing state. + +`account list` only inspects local metadata. It does not create directories, select a default, load private keys, contact the service or migrate an old profile. Partially restored accounts remain visible with missing metadata set to null. Its `selected` field reflects an explicit `--account` override for this invocation; otherwise it reflects the saved service default. A rejected `account use` on an unregistered account keeps both the previous service and account selection. + Stop old clients and background listeners before upgrading or migrating. Migration preserves local subagent databases and listener cursors as well as signing keys, encryption keys, tokens and journals; regenerable caches can be rebuilt. Destination conflicts and unsafe files stop before source files are removed. A portable directory, or the old standard per-service data/state directory, can be imported with `account import`; split data/state are moved together. No `msg-hardware` namespace is used. Stop clients using the old profile before migrating: diff --git a/src/msg/client.py b/src/msg/client.py index 37ff7437..a109c625 100644 --- a/src/msg/client.py +++ b/src/msg/client.py @@ -163,6 +163,17 @@ def __init__( require(account_selection.get('version') == 1, 'unknown_client_state_version') ClientPaths.discover(server=server, account=account_selection.get('account', '')) old_service = private_client_json(service_paths.state / 'client.json') + if ( + account is None + and account_selection is None + and old_service is None + and migrate_from is None + ): + require( + not service_paths.account_names(), + 'local_account_selection_required', + details={'options': ['account list', 'account use NAME', '--account NAME']}, + ) self.account = ( account if account is not None @@ -173,16 +184,21 @@ def __init__( ) ) self.paths = ClientPaths.discover(server=server, account=self.account) + if account is None and account_selection is not None and migrate_from is None: + require( + private_client_json(self.paths.state / 'client.json') is not None, + 'local_account_not_found', + ) if ( old_service is not None and migrate_from is None and (self.account == old_service.get('handle') or self.account == 'default') ): legacy = service_paths - if account is not None and legacy is not None and migrate_from is None: + if legacy is not None and migrate_from is None: source_state = legacy.state if isinstance(legacy, ClientPaths) else legacy source_data = private_client_json(source_state / 'client.json') or {} - if account not in {'default', source_data.get('handle')}: + if self.account not in {'default', source_data.get('handle')}: legacy = None if legacy is not None and migrate_from is None: current = private_client_json(self.paths.state / 'client.json') diff --git a/src/msg/client_accounts.py b/src/msg/client_accounts.py index c541df0c..01664047 100644 --- a/src/msg/client_accounts.py +++ b/src/msg/client_accounts.py @@ -10,6 +10,21 @@ from msg.service_origin import service_origin +def selected_service(args): + previous = ClientPaths.discover(profile=args.profile) + selection = private_client_json(previous.config / 'service.json') + if selection is not None: + require(selection.get('version') == 1, 'unknown_client_state_version') + server = args.server or os.environ.get('MSG_SERVER') or (selection or {}).get('server') + if server is None: + legacy = private_client_json(previous.state / 'client.json') + if legacy is None: + legacy = private_client_json(previous.config / 'client.json') + server = (legacy or {}).get('server') + require(server is not None, 'server_required') + return service_origin(server) + + def run_command(args): require(args.config_dir is None, 'account_conflicts_with_config_dir') if args.action == 'import': @@ -30,48 +45,47 @@ def run_command(args): 'handle': state.data.get('handle'), } if args.action == 'use': - previous = ClientPaths.discover(profile=args.profile) - server = ( - args.server - or os.environ.get('MSG_SERVER') - or (private_client_json(previous.config / 'service.json') or {}).get('server') - ) - require(server is not None, 'server_required') - server = service_origin(server) + server = selected_service(args) paths = ClientPaths.discover(server=server, account=args.name) saved = private_client_json(paths.state / 'client.json') require(saved is not None, 'local_account_not_found') + require(saved.get('subject_id') is not None, 'local_account_not_authenticated') selected = ClientState(server=server, profile=args.profile, account=args.name) require(selected.subject is not None, 'local_account_not_authenticated') marker = ClientPaths.discover(server=server).config / 'current-account.json' private_directory(marker.parent) durable_write(marker, canonical({'version': 1, 'account': args.name}), mode=0o600) return {'account': args.name, 'server': server, 'handle': selected.data.get('handle')} - # Resolve the selected service and safely migrate its old singleton layout. - state = ClientState(server=args.server, profile=args.profile, account=args.account) - service = ClientPaths.discover(server=state.server) + server = selected_service(args) + service = ClientPaths.discover(server=server) marker = service.config / 'current-account.json' - selection = private_client_json(marker) or {'account': 'default'} - root = service.state / 'accounts' + selection = private_client_json(marker) + if selection is not None: + require(selection.get('version') == 1, 'unknown_client_state_version') + ClientPaths.discover(server=server, account=selection.get('account', '')) + names = service.account_names() + selected = (selection or {}).get('account') + if args.account is not None: + ClientPaths.discover(server=server, account=args.account) + require(args.account in names, 'local_account_not_found') + selected = args.account result = [] - for directory in sorted(root.iterdir()): - require(not directory.is_symlink(), 'unsafe_client_directory') - if not directory.is_dir(): - continue - # Validate each account name and directory without loading/exporting keys. - paths = ClientPaths.discover(server=state.server, account=directory.name) + for name in names: + paths = ClientPaths.discover(server=server, account=name) saved = private_client_json(paths.state / 'client.json') - if saved is None: - continue + if saved is not None: + require(saved.get('version') == 1, 'unknown_client_state_version') + require(service_origin(saved.get('server')) == server, 'client_server_mismatch') + saved = saved or {} result.append({ - 'account': directory.name, + 'account': name, 'handle': saved.get('handle'), 'subject_id': saved.get('subject_id'), - 'selected': directory.name == selection['account'], + 'selected': name == selected, 'signer': 'yubikey' if paths.file('hardware-signer.json').exists() else 'software' if paths.file('identity.key').exists() else None, }) - return {'server': state.server, 'accounts': result} + return {'server': server, 'accounts': result} diff --git a/src/msg/paths.py b/src/msg/paths.py index 3a8f753f..d2181d21 100644 --- a/src/msg/paths.py +++ b/src/msg/paths.py @@ -118,6 +118,33 @@ def prepare(self): private_directory(path.parent.parent) private_directory(path) + def account_names(self): + """Inspect a service's saved accounts without creating or migrating files.""" + names = set() + for base in {self.config, self.data, self.state}: + root = base / 'accounts' + for parent in (root, *root.parents): + require(not parent.is_symlink(), 'unsafe_client_directory') + if not root.exists(): + continue + info = root.stat() + require( + stat.S_ISDIR(info.st_mode) and info.st_uid == os.geteuid(), + 'unsafe_client_directory', + ) + for entry in root.iterdir(): + info = entry.lstat() + require(not stat.S_ISLNK(info.st_mode), 'unsafe_client_directory') + if not stat.S_ISDIR(info.st_mode): + continue + require( + info.st_uid == os.geteuid() and info.st_mode & 0o077 == 0, + 'unsafe_client_directory', + ) + self.discover(account=entry.name) + names.add(entry.name) + return sorted(names) + def temporary_parent(self): value = os.environ.get('XDG_RUNTIME_DIR') if not value or not Path(value).is_absolute(): diff --git a/tests/test_client_account_recovery.py b/tests/test_client_account_recovery.py index 584e4fe6..ae093490 100644 --- a/tests/test_client_account_recovery.py +++ b/tests/test_client_account_recovery.py @@ -146,3 +146,64 @@ def test_restored_key_without_state_still_requires_account_selection(home): with pytest.raises(Failure, match='local_account_selection_required'): ClientState() assert snapshot(home) == before + assert run_command(args())['accounts'] == [ + { + 'account': 'alice', + 'handle': None, + 'subject_id': None, + 'selected': False, + 'signer': 'software', + } + ] + assert snapshot(home) == before + + +def test_listing_another_service_does_not_change_remembered_service(home, monkeypatch): + alice = registered('alice') + run_command(args('use', name='alice')) + monkeypatch.delenv('MSG_SERVER') + before = snapshot(home) + request = args() + request.server = 'https://other.example.org' + assert run_command(request) == {'server': request.server, 'accounts': []} + assert snapshot(home) == before + assert ClientState().subject == alice.subject + + +@pytest.mark.parametrize('base', ['config', 'data', 'state']) +def test_listing_rejects_symlinked_account_roots_without_changing_target(home, base): + registered('alice') + service = ClientPaths.discover(server=SERVER) + directory = getattr(service, base) / 'accounts' + target = directory.with_name('saved-accounts') + directory.rename(target) + directory.symlink_to(target, target_is_directory=True) + before = snapshot(target) + with pytest.raises(Failure, match='unsafe_client_directory'): + run_command(args()) + assert snapshot(target) == before + + +def test_explicit_import_restores_missing_selected_state_with_same_key(home): + alice = registered('alice') + run_command(args('use', name='alice')) + backup = ClientState(home / 'backup') + backup.save_signer(alice.signer) + backup.data.update(alice.data) + backup._save() + alice.path.unlink() + run_command(args('import', name='alice', directory=backup.directory)) + restored = ClientState() + assert restored.subject == alice.subject + assert restored.signer.public_key == alice.signer.public_key + + +@pytest.mark.parametrize('version', [None, 2]) +def test_listing_rejects_unknown_selection_version_without_changes(home, version): + registered('alice') + marker = ClientPaths.discover(server=SERVER).config / 'current-account.json' + durable_write(marker, canonical({'version': version, 'account': 'alice'}), mode=0o600) + before = snapshot(home) + with pytest.raises(Failure, match='unknown_client_state_version'): + run_command(args()) + assert snapshot(home) == before From b31cc5b9c562477c089b4fc0bb598eef20121277 Mon Sep 17 00:00:00 2001 From: LIghtJUNction Date: Fri, 2 Oct 2026 02:36:50 +0800 Subject: [PATCH 3/3] test: validate the shipped 3D root page instead of retired copy --- docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md | 6 ++++++ tests/test_branding.py | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md b/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md index 852da2b2..3f982263 100644 --- a/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md +++ b/docs/CLIENT_ACCOUNT_RECOVERY_PROGRESS.md @@ -32,6 +32,12 @@ The first PR commit contains only tests and workflow wiring, for baseline failur evidence. The following implementation commit is verified separately. Exact run results belong in PR #232; this file does not predeclare CI success. +The unchanged baseline produced nine account-regression failures in service-alias +run `36907335525`. Its tenth failure is an existing branding assertion, also +present in main run `36904405959`: it requires copy from the retired landing page. +The smoke test now checks the exact bundled root-page bytes, the 3D canvas and the +accessible catalog, retaining its CSP, font, language and HTTP assertions. + Issue #219 also includes server-account retirement and full recovery acceptance. Those are separate from this code change; the issue stays open. No production account, credential, release or deployment is changed by this work. diff --git a/tests/test_branding.py b/tests/test_branding.py index d6a993a5..309a3ed7 100644 --- a/tests/test_branding.py +++ b/tests/test_branding.py @@ -9,6 +9,7 @@ import pytest from test_service import NOW, call, register +from msg.bootstrap import ROOT_WEB_SAMPLE from msg.core.codec import b64, canonical, wire from msg.core.requests import request_for from msg.transports.dictionary import build_dictionary @@ -81,7 +82,9 @@ async def test_logo_is_packaged_and_served_read_only(installed): hosted = await http.get('/@root/web/index.html') assert hosted.status_code == 200 and b'' in hosted.text - assert 'a conversation begins.' in hosted.text + assert hosted.content == ROOT_WEB_SAMPLE + assert '