From 3dc4b570f127264212298b3c7ace6a8d4c2864bf Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:47:48 -0400 Subject: [PATCH 01/17] Fall back to Eufy's newer login host when the original is unavailable --- eufy_sync/eufy_client.py | 131 ++++++++++++++++++++++++---- tests/test_eufy_client.py | 176 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 17 deletions(-) diff --git a/eufy_sync/eufy_client.py b/eufy_sync/eufy_client.py index e82d388..df100a9 100644 --- a/eufy_sync/eufy_client.py +++ b/eufy_sync/eufy_client.py @@ -2,6 +2,7 @@ import json import logging +import re import time from dataclasses import dataclass from datetime import datetime, timezone @@ -14,6 +15,20 @@ logger = logging.getLogger(__name__) BASE_URL = "https://api.eufylife.com/v1" +LOGIN_URL = f"{BASE_URL}/user/v2/email/login" +# EufyLife 3.3.12 for Android logs in here instead. Scale data still comes +# from BASE_URL. Request shape follows m4ary/eufylife-api-hacs (MIT). +FALLBACK_LOGIN_URL = "https://home-api.eufylife.com/v1/user/v2/email/login/" +FALLBACK_APP_VERSION = "3.3.12" +DEFAULT_COUNTRY = "US" + +CLIENT_ID = "eufy-app" +CLIENT_SECRET = "8FHf22gaTKu7MZXqz5zytw" # Public app identifier from EufyLife APK, not a per-user secret +DEFAULT_TOKEN_TTL = 2592000 # 30 days + +# Error messages that mean the login endpoint itself is retired, as opposed to +# the credentials being wrong. +_ENDPOINT_RETIRED = re.compile(r"deprecat|upgrade|no longer supported|obsolete", re.I) COMMON_HEADERS = { "Accept": "*/*", @@ -87,28 +102,110 @@ def authenticate(self) -> None: self._fresh_login() def _fresh_login(self) -> None: - resp = self._client.post(f"{BASE_URL}/user/v2/email/login", json={ - "client_id": "eufy-app", - "client_secret": "8FHf22gaTKu7MZXqz5zytw", # Public app identifier from EufyLife APK, not a per-user secret - "email": self.config.email, - "password": self.config.password, - }) + data, moved_reason = self._primary_login() + host = "api.eufylife.com" + if moved_reason is not None: + logger.info("Eufy login endpoint unavailable (%s); trying home-api.eufylife.com", moved_reason) + data = self._fallback_login() + host = "home-api.eufylife.com" + + self._apply_login(data) + logger.info("Authenticated to Eufy as user %s via %s", self.user_id, host) + + def _primary_login(self) -> tuple[dict | None, str | None]: + """Log in at the original host. + + Returns (data, None) on success, or (None, reason) when the endpoint + looks moved or gone and the newer host is worth one try. Rejected + credentials and rate limits raise instead, so a bad password is never + sent twice. + """ + try: + resp = self._client.post(LOGIN_URL, json={ + "client_id": CLIENT_ID, + "client_secret": CLIENT_SECRET, + "email": self.config.email, + "password": self.config.password, + }) + except httpx.ConnectError as e: # DNS failure or connection refused + return None, type(e).__name__ + + if resp.status_code in (404, 410): + return None, f"HTTP {resp.status_code}" resp.raise_for_status() - data = resp.json() + + try: + data = resp.json() + except ValueError: + return None, "non-JSON response" + if not isinstance(data, dict): + return None, "non-JSON response" if data.get("res_code") != 1: - from eufy_sync.sync import PermanentSyncError - msg = data.get("message", "unknown error") - raise PermanentSyncError( - f"Eufy login failed: {msg}. " - "If you changed your Eufy password, run: eufy-sync --update-password" - ) + msg = str(data.get("message") or "") + if _ENDPOINT_RETIRED.search(msg): + return None, f"res_code {data.get('res_code')}" + self._raise_login_rejected(data) + return data, None + + def _fallback_login(self) -> dict: + """Log in at home-api.eufylife.com the way EufyLife 3.3.12 for Android does.""" + country = DEFAULT_COUNTRY + resp = self._client.post( + FALLBACK_LOGIN_URL, + headers={ + "User-Agent": f"EufyLife-Android-{FALLBACK_APP_VERSION}", + "Category": "Health", + "Country": country.upper(), + }, + json={ + "client_id": CLIENT_ID, + "client_Secret": CLIENT_SECRET, # the app capitalizes this key on this host + "email": self.config.email, + "password": self.config.password, + "ab": country.lower(), + "un_subscribe_flag": True, + }, + ) + resp.raise_for_status() + try: + data = resp.json() + except ValueError: + raise RuntimeError("Eufy login failed: home-api.eufylife.com returned a non-JSON response") from None + if not isinstance(data, dict): + raise RuntimeError("Eufy login failed: home-api.eufylife.com returned an unexpected response") + if data.get("res_code") != 1: + self._raise_login_rejected(data) + return data + + @staticmethod + def _raise_login_rejected(data: dict) -> None: + from eufy_sync.sync import PermanentSyncError + msg = data.get("message", "unknown error") + raise PermanentSyncError( + f"Eufy login failed: {msg}. " + "If you changed your Eufy password, run: eufy-sync --update-password" + ) + + def _apply_login(self, data: dict) -> None: + """Store the token and user id from either host's login response. + + Both hosts put access_token and user_id at the top level; a nested + `data` object is accepted too in case Eufy wraps it later. + """ + nested = data.get("data") if isinstance(data.get("data"), dict) else {} + token = data.get("access_token") or nested.get("access_token") + user_id = data.get("user_id") or nested.get("user_id") + if not token or not user_id: + raise RuntimeError("Eufy login response is missing the access token or user id") + + expires_in = data.get("expires_in", nested.get("expires_in")) + if not isinstance(expires_in, int) or expires_in <= 0: + expires_in = DEFAULT_TOKEN_TTL - self.access_token = data["access_token"] - self.user_id = data["user_id"] - expires_in = data.get("expires_in", 2592000) # default 30 days + self.access_token = str(token) + self.user_id = str(user_id) self._save_token(expires_in) - logger.info("Authenticated to Eufy as user %s", self.user_id) def token_status(self) -> dict: """Return token health without authenticating.""" diff --git a/tests/test_eufy_client.py b/tests/test_eufy_client.py index 0adc7dd..f7a5fc6 100644 --- a/tests/test_eufy_client.py +++ b/tests/test_eufy_client.py @@ -1,6 +1,7 @@ from datetime import datetime, timezone from unittest.mock import MagicMock, patch +import httpx import pytest from eufy_sync.config import EufyConfig @@ -395,3 +396,178 @@ def test_processed_measurement_is_not_weight_only(): c = _client() m = c._parse_record(_record("a", 800, 100)) assert m.weight_only is False + + +# --------------------------------------------------------------------------- +# Login host fallback: EufyLife 3.3.12 logs in at home-api.eufylife.com. Use it +# only when the original endpoint looks moved or gone, never after a rejected +# password or a rate limit. +# --------------------------------------------------------------------------- + +PRIMARY_LOGIN = "https://api.eufylife.com/v1/user/v2/email/login" +FALLBACK_LOGIN = "https://home-api.eufylife.com/v1/user/v2/email/login/" + + +def _login_client(): + c = EufyClient.__new__(EufyClient) + c.config = EufyConfig(email="e@example.com", password="pw") + c.access_token = None + c.user_id = None + c._client = MagicMock() + c._save_token = MagicMock() + return c + + +def _http_resp(status_code, json_body=None, *, url=PRIMARY_LOGIN): + request = httpx.Request("POST", url) + if json_body is None: + return httpx.Response(status_code, text="Not Found", request=request) + return httpx.Response(status_code, json=json_body, request=request) + + +def _login_ok(token="tok-1", user_id="uid-1", **extra): + return {"res_code": 1, "access_token": token, "user_id": user_id, **extra} + + +def _posted_urls(c): + return [call.args[0] for call in c._client.post.call_args_list] + + +def test_login_primary_success_does_not_fall_back(): + c = _login_client() + c._client.post.return_value = _http_resp(200, _login_ok(expires_in=86400)) + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN] + assert (c.access_token, c.user_id) == ("tok-1", "uid-1") + c._save_token.assert_called_once_with(86400) + + +@pytest.mark.parametrize("status", [404, 410]) +def test_login_primary_gone_falls_back_to_home_api(status): + c = _login_client() + c._client.post.side_effect = [ + _http_resp(status), + _http_resp(200, _login_ok("tok-2", "uid-2"), url=FALLBACK_LOGIN), + ] + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN, FALLBACK_LOGIN] + assert (c.access_token, c.user_id) == ("tok-2", "uid-2") + + fallback = c._client.post.call_args_list[1] + assert fallback.kwargs["headers"]["User-Agent"] == "EufyLife-Android-3.3.12" + assert fallback.kwargs["headers"]["Country"] == "US" + assert fallback.kwargs["headers"]["Category"] == "Health" + assert fallback.kwargs["json"]["ab"] == "us" + assert fallback.kwargs["json"]["email"] == "e@example.com" + + +def test_login_connection_failure_falls_back(): + c = _login_client() + c._client.post.side_effect = [ + httpx.ConnectError("Name or service not known"), + _http_resp(200, _login_ok(), url=FALLBACK_LOGIN), + ] + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN, FALLBACK_LOGIN] + + +def test_login_non_json_200_falls_back(): + c = _login_client() + c._client.post.side_effect = [ + _http_resp(200), # HTML body + _http_resp(200, _login_ok(), url=FALLBACK_LOGIN), + ] + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN, FALLBACK_LOGIN] + + +def test_login_deprecated_message_falls_back(): + c = _login_client() + c._client.post.side_effect = [ + _http_resp(200, {"res_code": 26050, "message": "This API is deprecated, please upgrade the app"}), + _http_resp(200, _login_ok(), url=FALLBACK_LOGIN), + ] + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN, FALLBACK_LOGIN] + + +def test_login_wrong_password_does_not_fall_back(): + from eufy_sync.sync import PermanentSyncError + c = _login_client() + c._client.post.return_value = _http_resp(200, {"res_code": 26006, "message": "Incorrect password"}) + with pytest.raises(PermanentSyncError): + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN] + c._save_token.assert_not_called() + + +@pytest.mark.parametrize("status", [401, 403]) +def test_login_auth_rejected_status_does_not_fall_back(status): + c = _login_client() + c._client.post.return_value = _http_resp(status, {"res_code": 0}) + with pytest.raises(httpx.HTTPStatusError): + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN] + + +def test_login_rate_limited_does_not_fall_back(): + c = _login_client() + c._client.post.return_value = _http_resp(429, {"res_code": 0, "message": "Too many requests"}) + with pytest.raises(httpx.HTTPStatusError): + c._fresh_login() + assert _posted_urls(c) == [PRIMARY_LOGIN] + + +def test_login_fallback_wrong_password_raises_permanent_error(): + from eufy_sync.sync import PermanentSyncError + c = _login_client() + c._client.post.side_effect = [ + _http_resp(404), + _http_resp(200, {"res_code": 26006, "message": "Incorrect password"}, url=FALLBACK_LOGIN), + ] + with pytest.raises(PermanentSyncError): + c._fresh_login() + assert len(_posted_urls(c)) == 2 + + +def test_home_api_response_shape_parses(): + """Field names from the EufyLife 3.3.12 login response, as read by + m4ary/eufylife-api-hacs and osjayaprakash/eufylife-scale-mcp.""" + c = _login_client() + home_api_body = { + "res_code": 1, + "message": "success", + "access_token": "home-tok", + "user_id": "home-uid", + "expires_in": 2592000, + "user_center_id": "center-id", + "user_center_token": "center-tok", + "device_id": "phone-id", + "customers": [{"id": "cust-a", "name": "A"}, {"id": "cust-b", "name": "B"}], + } + c._client.post.side_effect = [_http_resp(404), _http_resp(200, home_api_body, url=FALLBACK_LOGIN)] + c._fresh_login() + # The data endpoints take the login access_token and user_id, not user_center_token. + assert c.access_token == "home-tok" + assert c.user_id == "home-uid" + c._save_token.assert_called_once_with(2592000) + + +def test_login_response_nested_under_data_parses_and_defaults_ttl(): + c = _login_client() + c._client.post.return_value = _http_resp( + 200, {"res_code": 1, "data": {"access_token": "n-tok", "user_id": 12345}}, + ) + c._fresh_login() + assert (c.access_token, c.user_id) == ("n-tok", "12345") + c._save_token.assert_called_once_with(2592000) + + +def test_login_logs_host_without_secrets(caplog): + c = _login_client() + c._client.post.side_effect = [_http_resp(404), _http_resp(200, _login_ok("secret-tok"), url=FALLBACK_LOGIN)] + with caplog.at_level("DEBUG", logger="eufy_sync.eufy_client"): + c._fresh_login() + assert "home-api.eufylife.com" in caplog.text + assert "secret-tok" not in caplog.text + assert "pw" not in caplog.text.split() From 4e3d9e3882a446081467ce7a4206b8630b5f4114 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:47:58 -0400 Subject: [PATCH 02/17] Move to Strava's new API host on its published schedule --- eufy_sync/strava_client.py | 57 +++++++++++-- tests/test_strava_client.py | 166 ++++++++++++++++++++++++++++++++++-- 2 files changed, 211 insertions(+), 12 deletions(-) diff --git a/eufy_sync/strava_client.py b/eufy_sync/strava_client.py index cdd0cc8..a183c1a 100644 --- a/eufy_sync/strava_client.py +++ b/eufy_sync/strava_client.py @@ -6,7 +6,9 @@ capture the callback code via a local HTTP server, and exchange for tokens. 2. Tokens stored in keychain (file fallback). 3. Access tokens expire after 6 hours; refresh tokens are indefinite. -4. On each sync, we PUT /api/v3/athlete with the latest weight. +4. On each sync, we PUT /athlete with the latest weight. +5. The API host follows Strava's published move to api-v3.strava.com + (see _api_bases). """ from __future__ import annotations @@ -15,6 +17,8 @@ import secrets import time import webbrowser +from collections.abc import Callable +from datetime import date, datetime, timezone from http.server import BaseHTTPRequestHandler, HTTPServer from pathlib import Path from threading import Thread @@ -28,12 +32,37 @@ STRAVA_AUTH_URL = "https://www.strava.com/oauth/authorize" STRAVA_TOKEN_URL = "https://www.strava.com/oauth/token" -STRAVA_API_BASE = "https://www.strava.com/api/v3" +STRAVA_API_BASE_OLD = "https://www.strava.com/api/v3" +STRAVA_API_BASE_NEW = "https://api-v3.strava.com" +# Strava's changelog (2026-06-01) says the new base is available from this date. +NEW_API_BASE_FROM = date(2027, 1, 4) +# Third-party trackers report the old base stops working by this date. +# Strava's own docs name it only as the end of oauth/deauthorize. +OLD_API_BASE_UNTIL = date(2027, 6, 1) CALLBACK_PORT = 8089 REDIRECT_URI = f"http://localhost:{CALLBACK_PORT}/callback" REFRESH_SAFETY_MARGIN = 300 # seconds before expiry to trigger refresh +def _utc_today() -> date: + return datetime.now(timezone.utc).date() + + +def _api_bases(today: date) -> tuple[str, ...]: + """API bases to try, in order, for a request made on ``today``. + + Before the new host exists we use the old one. During the overlap we try + the new host first and keep the old one as a fallback, since users may + run an old release well past either date. Once the old host is retired, + only the new one is left. + """ + if today < NEW_API_BASE_FROM: + return (STRAVA_API_BASE_OLD,) + if today < OLD_API_BASE_UNTIL: + return (STRAVA_API_BASE_NEW, STRAVA_API_BASE_OLD) + return (STRAVA_API_BASE_NEW,) + + def _auth_url(client_id: str, state_value: str) -> str: """The authorization URL, with every query value percent-encoded.""" query = urlencode({ @@ -163,12 +192,25 @@ def _serve_until_done(): class StravaClient: - """Syncs weight to Strava via PUT /api/v3/athlete.""" + """Syncs weight to Strava via PUT /athlete.""" - def __init__(self, config: StravaConfig): + def __init__(self, config: StravaConfig, today: Callable[[], date] = _utc_today): self.config = config self._client = httpx.Client(timeout=30.0) self._tokens: dict | None = None + self._today = today + + def _api_request(self, method: str, path: str, **kwargs) -> httpx.Response: + """Send an API request, falling back to the next base only when the + connection itself fails. Any HTTP response, including 401, 403 and + 429, comes from a live host and is returned as is.""" + bases = _api_bases(self._today()) + for base in bases[:-1]: + try: + return self._client.request(method, f"{base}{path}", **kwargs) + except (httpx.ConnectError, httpx.ConnectTimeout) as e: + logger.info("Could not reach %s (%s); trying %s", base, e, bases[-1]) + return self._client.request(method, f"{bases[-1]}{path}", **kwargs) def authenticate(self) -> None: """Load tokens and refresh if needed.""" @@ -220,7 +262,7 @@ def _refresh_access_token(self) -> None: def check_connection(self) -> None: """Verify the saved session with a read-only athlete request.""" - resp = self._client.get(f"{STRAVA_API_BASE}/athlete") + resp = self._api_request("GET", "/athlete") if resp.status_code in (401, 403): from eufy_sync.sync import PermanentSyncError raise PermanentSyncError("Strava rejected the session. Run: eufy-sync --setup-strava") @@ -232,8 +274,9 @@ def update_weight(self, weight_kg: float) -> dict: Strava only accepts current weight, without a timestamp or body composition fields. The caller selects the newest valid reading. """ - resp = self._client.put( - f"{STRAVA_API_BASE}/athlete", + resp = self._api_request( + "PUT", + "/athlete", data={"weight": round(weight_kg, 2)}, ) diff --git a/tests/test_strava_client.py b/tests/test_strava_client.py index 20d338c..843732c 100644 --- a/tests/test_strava_client.py +++ b/tests/test_strava_client.py @@ -1,10 +1,18 @@ from __future__ import annotations import time +from datetime import date from unittest.mock import MagicMock, patch +import httpx +import pytest + from eufy_sync.config import StravaConfig -from eufy_sync.strava_client import StravaClient +from eufy_sync.strava_client import ( + STRAVA_API_BASE_NEW, + STRAVA_API_BASE_OLD, + StravaClient, +) def _make_config(): @@ -190,12 +198,13 @@ def test_update_weight(mock_load, mock_save): client = StravaClient(_make_config()) client.authenticate() - with patch.object(client._client, "put", return_value=mock_response) as mock_put: + with patch.object(client._client, "request", return_value=mock_response) as mock_request: result = client.update_weight(86.2) assert result == {"weight": 86.2} - call_kwargs = mock_put.call_args - assert "athlete" in call_kwargs[0][0] + method, url = mock_request.call_args[0] + assert method == "PUT" + assert url.endswith("/athlete") client.close() @@ -211,7 +220,7 @@ def test_update_weight_failure_raises(mock_load, mock_save): client = StravaClient(_make_config()) client.authenticate() - with patch.object(client._client, "put", return_value=mock_response): + with patch.object(client._client, "request", return_value=mock_response): try: client.update_weight(86.2) raise AssertionError("Should have raised") @@ -235,3 +244,150 @@ def test_auth_url_encodes_every_query_value(): assert query["scope"] == ["profile:write,profile:read_all"] assert query["state"] == ["st@te/value"] assert query["approval_prompt"] == ["force"] + + +# Strava's API base moves to api-v3.strava.com on a published schedule. + +BEFORE_NEW_HOST = date(2027, 1, 3) +NEW_HOST_DAY_ONE = date(2027, 1, 4) +LAST_OVERLAP_DAY = date(2027, 5, 31) +OLD_HOST_RETIRED = date(2027, 6, 1) + + +def _client_on(day: date) -> StravaClient: + return StravaClient(_make_config(), today=lambda: day) + + +def _ok(json_body=None): + resp = MagicMock() + resp.status_code = 200 + resp.json.return_value = json_body or {"weight": 86.2} + return resp + + +def _status(code: int): + resp = MagicMock() + resp.status_code = code + resp.text = "error" + return resp + + +def _urls(mock_request) -> list[str]: + return [c[0][1] for c in mock_request.call_args_list] + + +@pytest.mark.parametrize("day, expected", [ + (date(2026, 10, 2), STRAVA_API_BASE_OLD), + (BEFORE_NEW_HOST, STRAVA_API_BASE_OLD), + (NEW_HOST_DAY_ONE, STRAVA_API_BASE_NEW), + (LAST_OVERLAP_DAY, STRAVA_API_BASE_NEW), + (OLD_HOST_RETIRED, STRAVA_API_BASE_NEW), + (date(2028, 3, 1), STRAVA_API_BASE_NEW), +]) +def test_api_base_follows_the_published_schedule(day, expected): + client = _client_on(day) + with patch.object(client._client, "request", return_value=_ok()) as mock_request: + client.update_weight(86.2) + assert _urls(mock_request) == [f"{expected}/athlete"] + client.close() + + +def test_new_host_is_api_v3_strava_com_without_www(): + assert STRAVA_API_BASE_NEW == "https://api-v3.strava.com" + assert STRAVA_API_BASE_OLD == "https://www.strava.com/api/v3" + + +@pytest.mark.parametrize("failure", [ + httpx.ConnectError("[Errno 8] nodename nor servname provided"), # DNS + httpx.ConnectError("[Errno 61] Connection refused"), + httpx.ConnectError("[SSL: CERTIFICATE_VERIFY_FAILED]"), # TLS + httpx.ConnectTimeout("timed out connecting"), +]) +def test_overlap_falls_back_to_old_host_when_new_host_is_unreachable(failure): + client = _client_on(NEW_HOST_DAY_ONE) + with patch.object(client._client, "request", side_effect=[failure, _ok()]) as mock_request: + assert client.update_weight(86.2) == {"weight": 86.2} + assert _urls(mock_request) == [ + f"{STRAVA_API_BASE_NEW}/athlete", + f"{STRAVA_API_BASE_OLD}/athlete", + ] + client.close() + + +def test_fallback_is_per_call_and_the_next_call_tries_the_new_host_again(): + client = _client_on(NEW_HOST_DAY_ONE) + responses = [httpx.ConnectError("refused"), _ok(), _ok()] + with patch.object(client._client, "request", side_effect=responses) as mock_request: + client.update_weight(86.2) + client.update_weight(86.0) + assert _urls(mock_request) == [ + f"{STRAVA_API_BASE_NEW}/athlete", + f"{STRAVA_API_BASE_OLD}/athlete", + f"{STRAVA_API_BASE_NEW}/athlete", + ] + client.close() + + +@pytest.mark.parametrize("code", [401, 403, 429, 500]) +def test_overlap_does_not_fall_back_on_http_errors(code): + """An HTTP status means the new host answered; retrying elsewhere would + only hide a real auth, rate-limit or server problem.""" + client = _client_on(NEW_HOST_DAY_ONE) + with patch.object(client._client, "request", return_value=_status(code)) as mock_request: + with pytest.raises(RuntimeError, match=str(code)): + client.update_weight(86.2) + assert _urls(mock_request) == [f"{STRAVA_API_BASE_NEW}/athlete"] + client.close() + + +def test_overlap_does_not_fall_back_on_read_timeout(): + """A read timeout may mean the PUT already reached Strava.""" + client = _client_on(NEW_HOST_DAY_ONE) + with patch.object(client._client, "request", side_effect=httpx.ReadTimeout("slow")) as mock_request: + with pytest.raises(httpx.ReadTimeout): + client.update_weight(86.2) + assert _urls(mock_request) == [f"{STRAVA_API_BASE_NEW}/athlete"] + client.close() + + +def test_connection_failure_before_the_switch_is_not_retried(): + client = _client_on(BEFORE_NEW_HOST) + with patch.object(client._client, "request", side_effect=httpx.ConnectError("refused")) as mock_request: + with pytest.raises(httpx.ConnectError): + client.update_weight(86.2) + assert _urls(mock_request) == [f"{STRAVA_API_BASE_OLD}/athlete"] + client.close() + + +def test_connection_failure_after_old_host_retired_has_no_fallback(): + client = _client_on(OLD_HOST_RETIRED) + with patch.object(client._client, "request", side_effect=httpx.ConnectError("refused")) as mock_request: + with pytest.raises(httpx.ConnectError): + client.update_weight(86.2) + assert _urls(mock_request) == [f"{STRAVA_API_BASE_NEW}/athlete"] + client.close() + + +def test_both_hosts_unreachable_raises_the_old_host_error(): + client = _client_on(NEW_HOST_DAY_ONE) + failures = [httpx.ConnectError("new down"), httpx.ConnectError("old down")] + with patch.object(client._client, "request", side_effect=failures): + with pytest.raises(httpx.ConnectError, match="old down"): + client.update_weight(86.2) + client.close() + + +def test_check_connection_uses_the_same_fallback(): + client = _client_on(NEW_HOST_DAY_ONE) + responses = [httpx.ConnectError("refused"), _ok({"id": 1})] + with patch.object(client._client, "request", side_effect=responses) as mock_request: + client.check_connection() + assert [c[0][0] for c in mock_request.call_args_list] == ["GET", "GET"] + assert _urls(mock_request)[-1] == f"{STRAVA_API_BASE_OLD}/athlete" + client.close() + + +def test_oauth_urls_stay_on_www_strava_com_after_the_move(): + from eufy_sync.strava_client import STRAVA_AUTH_URL, STRAVA_TOKEN_URL + assert STRAVA_AUTH_URL == "https://www.strava.com/oauth/authorize" + assert STRAVA_TOKEN_URL == "https://www.strava.com/oauth/token" From 92541f081385e0fd66e2c25d86b358551b7c5f97 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:48:32 -0400 Subject: [PATCH 03/17] Cap the weight-only reach-back and skip weights Zwift cannot take --- eufy_sync/state.py | 12 +++++++- eufy_sync/sync.py | 16 ++++++++++- eufy_sync/zwift_client.py | 6 ++-- tests/test_sync.py | 59 +++++++++++++++++++++++++++++++++++++- tests/test_zwift_client.py | 15 ++++++++++ tests/test_zwift_sync.py | 28 ++++++++++++++++++ 6 files changed, 131 insertions(+), 5 deletions(-) diff --git a/eufy_sync/state.py b/eufy_sync/state.py index 1bc4b84..e5a13d4 100644 --- a/eufy_sync/state.py +++ b/eufy_sync/state.py @@ -205,12 +205,22 @@ def clear_pending_upgrade(self, user_name: str, measurement_id: str) -> None: (user_name, measurement_id), ) - def get_oldest_weight_only_timestamp(self, user_name: str, target: str) -> int | None: + def get_oldest_weight_only_timestamp( + self, user_name: str, target: str, since: int | None = None, + ) -> int | None: + """Oldest weight-only sync still waiting for its full record. With + since (unix seconds), older rows are ignored: past that age the full + record is not coming, and reaching back for it would refetch all + history since then on every run. The rows themselves stay, so a + backfill that does return the full record can still upgrade them. + Compared in Python because the stored strings mix UTC offsets.""" rows = self._conn.execute( "SELECT measurement_timestamp FROM sync_log WHERE user_name = ? AND target = ? AND weight_only = 1", (user_name, target), ) timestamps = [datetime.fromisoformat(row[0]).timestamp() for row in rows] + if since is not None: + timestamps = [ts for ts in timestamps if ts >= since] return int(min(timestamps)) if timestamps else None def get_latest_sync_timestamp(self, user_name: str, target: str | None = None) -> int | None: diff --git a/eufy_sync/sync.py b/eufy_sync/sync.py index 1adf55f..99026cd 100644 --- a/eufy_sync/sync.py +++ b/eufy_sync/sync.py @@ -24,12 +24,21 @@ # only a unique match close in both time and weight is safe to replace. UPGRADE_MAX_SECONDS = 120 UPGRADE_MAX_WEIGHT_KG = 0.1 +# How far back a run reaches for weight-only Garmin entries that still lack +# their full record. One that has not matched within two weeks never will, +# and an unbounded reach-back would refetch everything since it on every run. +UPGRADE_LOOKBACK_DAYS = 14 class PermanentSyncError(RuntimeError): """Raised for failures that retries can't fix (bad password, revoked token).""" +class UnsupportedMeasurementError(PermanentSyncError): + """A target cannot accept this one measurement (e.g. outside its weight + range). The measurement is skipped; the target itself stays healthy.""" + + def _is_permanent(exc: BaseException) -> bool: # GarminConnectTooManyRequestsError: a 429 on login/refresh won't clear by # retrying seconds later, and retrying makes the IP rate limit worse, so @@ -145,7 +154,9 @@ def sync_user(user: UserConfig, state: SyncState, backfill_days: int | None = No if ts is None: logger.info("No prior syncs to %s for %s, backfilling 7 days", name, user.name) if name == "garmin": - pending_ts = state.get_oldest_weight_only_timestamp(user.name, name) + pending_ts = state.get_oldest_weight_only_timestamp( + user.name, name, since=int(time.time()) - UPGRADE_LOOKBACK_DAYS * 86400, + ) if pending_ts is not None: # Processed data can arrive after newer weigh-ins have # advanced the cursor. Include small timestamp shifts. @@ -366,6 +377,9 @@ def sync_user(user: UserConfig, state: SyncState, backfill_days: int | None = No logger.info("Upgraded weight-only entry to full body comp for %s", m.timestamp.astimezone().date()) if target_name == "garmin" and (upgrade_row is not None or m.measurement_id in pending): state.clear_pending_upgrade(user.name, m.measurement_id) + except UnsupportedMeasurementError as e: + logger.warning("Skipping %s for %s: %s", target_name.capitalize(), user.name, e) + continue except Exception as e: logger.error("Upload to %s failed for %s: %s", target_name, user.name, e) # str(e) carries the actionable text the CLI keys its diff --git a/eufy_sync/zwift_client.py b/eufy_sync/zwift_client.py index 8c3e357..83715e6 100644 --- a/eufy_sync/zwift_client.py +++ b/eufy_sync/zwift_client.py @@ -213,12 +213,14 @@ def check_connection(self) -> None: @staticmethod def _grams(weight_kg: float) -> int: + # Retrying cannot change the weight, so these are skipped, not retried. + from eufy_sync.sync import UnsupportedMeasurementError try: value = Decimal(str(weight_kg)) except (InvalidOperation, ValueError) as exc: - raise ValueError("Zwift weight must be a finite number") from exc + raise UnsupportedMeasurementError("Zwift weight must be a finite number") from exc if not value.is_finite() or value < Decimal("30") or value > Decimal("300"): - raise ValueError("Zwift weight must be between 30 and 300 kg") + raise UnsupportedMeasurementError(f"Zwift only accepts 30 to 300 kg, got {weight_kg} kg") return int((value * 1000).quantize(Decimal("1"), rounding=ROUND_HALF_UP)) @staticmethod diff --git a/tests/test_sync.py b/tests/test_sync.py index 56bdad1..1b80e11 100644 --- a/tests/test_sync.py +++ b/tests/test_sync.py @@ -994,7 +994,8 @@ def test_second_full_record_same_day_does_not_delete(tmp_path: Path): def test_automatic_sync_revisits_older_weight_only_readings(tmp_path: Path): state = SyncState(tmp_path / "test.db") user = _garmin_user() - old_dt = datetime(2026, 5, 10, 12, tzinfo=timezone.utc) + # Inside UPGRADE_LOOKBACK_DAYS; older readings are no longer revisited. + old_dt = datetime.now(timezone.utc).replace(microsecond=0) - timedelta(days=6) old_raw = _raw_measurement(85.0, old_dt) newest = _full_measurement(84.0, old_dt + timedelta(days=3)) _run_garmin_sync(user, state, [old_raw, newest], has_weight_on_date_return=False) @@ -1016,6 +1017,62 @@ def test_automatic_sync_revisits_older_weight_only_readings(tmp_path: Path): state.close() +def test_stale_weight_only_reading_stops_widening_the_fetch_window(tmp_path: Path): + """A raw reading whose full record never matched must not pull every + run back to its date forever; past the lookback it is ignored.""" + from eufy_sync.sync import UPGRADE_LOOKBACK_DAYS + + state = SyncState(tmp_path / "test.db") + user = _garmin_user() + now = datetime.now(timezone.utc) + stale = now - timedelta(days=UPGRADE_LOOKBACK_DAYS + 30) + recent = now - timedelta(days=1) + state.record_sync(user.name, "cust_stale", stale.isoformat(), 85.0, + stale.isoformat(), target="garmin", weight_only=True) + state.record_sync(user.name, "cust_recent", recent.isoformat(), 84.0, + recent.isoformat(), target="garmin") + + source = MagicMock() + source.fetch_measurements.return_value = [] + with patch("eufy_sync.sync.EufyClient", return_value=source), \ + patch("eufy_sync.garmin_client.GarminClient", return_value=MagicMock()), \ + patch("eufy_sync.sync.time.sleep"): + _, errors = sync_user(user, state) + + assert errors == {} + after = source.fetch_measurements.call_args.kwargs["after_timestamp"] + assert after == int(recent.timestamp()) + # The row is kept, so a backfill that returns its full record can still upgrade it. + assert len(state.weight_only_syncs_on_date(user.name, "garmin", stale.astimezone().date())) == 1 + assert state.get_oldest_weight_only_timestamp(user.name, "garmin") == int(stale.timestamp()) + state.close() + + +def test_recent_weight_only_reading_still_widens_the_fetch_window(tmp_path: Path): + from eufy_sync.sync import UPGRADE_MAX_SECONDS + + state = SyncState(tmp_path / "test.db") + user = _garmin_user() + now = datetime.now(timezone.utc) + pending = now - timedelta(days=5) + recent = now - timedelta(days=1) + state.record_sync(user.name, "cust_pending", pending.isoformat(), 85.0, + pending.isoformat(), target="garmin", weight_only=True) + state.record_sync(user.name, "cust_recent", recent.isoformat(), 84.0, + recent.isoformat(), target="garmin") + + source = MagicMock() + source.fetch_measurements.return_value = [] + with patch("eufy_sync.sync.EufyClient", return_value=source), \ + patch("eufy_sync.garmin_client.GarminClient", return_value=MagicMock()), \ + patch("eufy_sync.sync.time.sleep"): + sync_user(user, state) + + after = source.fetch_measurements.call_args.kwargs["after_timestamp"] + assert after == int(pending.timestamp()) - UPGRADE_MAX_SECONDS + state.close() + + @pytest.mark.parametrize("mode,include_current", [ ({"backfill_days": 30}, True), ({"backfill_days": 30}, False), diff --git a/tests/test_zwift_client.py b/tests/test_zwift_client.py index e0f75ec..8121770 100644 --- a/tests/test_zwift_client.py +++ b/tests/test_zwift_client.py @@ -200,6 +200,21 @@ def handler(request): client.close() +@pytest.mark.parametrize("weight", [25.0, 301.0, float("nan")]) +def test_out_of_range_weight_is_skipped_without_any_request(weight): + from eufy_sync.sync import UnsupportedMeasurementError, _is_permanent + + def handler(request): + raise AssertionError("no request for a weight Zwift cannot take") + + client = _authenticated_client(handler) + with pytest.raises(UnsupportedMeasurementError) as exc: + client.update_weight(weight) + # Permanent, so _retry gives up at once instead of sleeping through retries. + assert _is_permanent(exc.value) + client.close() + + def test_204_and_wrong_readback_never_count_as_success(): def handler(request): if request.method == "PUT": diff --git a/tests/test_zwift_sync.py b/tests/test_zwift_sync.py index a993c29..5de3cf7 100644 --- a/tests/test_zwift_sync.py +++ b/tests/test_zwift_sync.py @@ -253,3 +253,31 @@ def test_invalid_target_fails_before_any_client_is_constructed(tmp_path: Path, t eufy_class.assert_not_called() state.close() + + +def test_zwift_out_of_range_weight_is_skipped_without_retry_or_error(tmp_path: Path): + from eufy_sync.sync import UnsupportedMeasurementError + + state = SyncState(tmp_path / "state.db") + user = _user(garmin=True) + # Valid for Garmin (22.7 kg floor) but under Zwift's 30 kg floor. + light = _measurement(25.0, datetime(2026, 5, 10, tzinfo=timezone.utc)) + garmin = MagicMock() + garmin.has_weight_on_date.return_value = False + garmin.upload_body_composition.return_value = {"ok": True} + zwift = MagicMock() + zwift.update_weight.side_effect = UnsupportedMeasurementError("Zwift only accepts 30 to 300 kg, got 25.0 kg") + + with patch("eufy_sync.sync.EufyClient", return_value=_source([light])), \ + patch("eufy_sync.garmin_client.GarminClient", return_value=garmin), \ + patch("eufy_sync.zwift_client.ZwiftClient", return_value=zwift), \ + patch("eufy_sync.sync.time.sleep") as sleep: + counts, errors = sync_user(user, state, backfill_days=7) + + assert errors == {} + assert counts == {"garmin": 1, "zwift": 0} + zwift.update_weight.assert_called_once_with(25.0) + assert not state.is_synced(user.name, light.measurement_id, "zwift") + # Only the inter-upload pause after Garmin; no retry backoff. + assert all(call.args[0] <= 1 for call in sleep.call_args_list) + state.close() From 0be1f19faf4306236852b845d8d73f7216267cbe Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:48:32 -0400 Subject: [PATCH 04/17] Remove the leftover Zwift probe credential path from setup --- eufy_sync/cli/setup.py | 15 +-------------- tests/test_setup_zwift.py | 20 +++++++------------- 2 files changed, 8 insertions(+), 27 deletions(-) diff --git a/eufy_sync/cli/setup.py b/eufy_sync/cli/setup.py index b7bc6f8..78248cf 100644 --- a/eufy_sync/cli/setup.py +++ b/eufy_sync/cli/setup.py @@ -217,19 +217,10 @@ def _connect_zwift(user: dict, retry_command: str = "eufy-sync --setup-zwift") - from eufy_sync import credentials vault = credentials._load_vault() - probe = vault.get("tokens", {}).get("zwift_probe") password_account = f"{user_name}:zwift" existing = user.get("zwift") or {} existing_email = existing.get("email") existing_password = vault.get("passwords", {}).get(password_account) - probe_is_current_user = ( - isinstance(probe, dict) - and probe.get("user_name") == user_name - and probe.get("password_account") == password_account - and (not existing_email or probe.get("email") == existing_email) - ) - probe_email = probe.get("email") if probe_is_current_user else None - probe_password = vault.get("passwords", {}).get(password_account) if probe_is_current_user else None print("") print(" Experimental Zwift weight sync") @@ -242,13 +233,9 @@ def _connect_zwift(user: dict, retry_command: str = "eufy-sync --setup-zwift") - email = existing_email password = existing_password print(f"Using the configured Zwift account for {email}.") - elif probe_email and probe_password: - email = probe_email - password = probe_password - print(f"Using the validated Zwift account for {email}.") else: if not sys.stdin.isatty(): - print("No validated Zwift credentials were found. Run setup in an interactive terminal.") + print("No saved Zwift credentials were found. Run setup in an interactive terminal.") sys.exit(1) email = input("Zwift email: ").strip() if not email: diff --git a/tests/test_setup_zwift.py b/tests/test_setup_zwift.py index 906d93f..1ab4a68 100644 --- a/tests/test_setup_zwift.py +++ b/tests/test_setup_zwift.py @@ -22,15 +22,13 @@ def _config(path): })) -def test_setup_zwift_reuses_probe_credentials_and_writes_no_secret(tmp_path, capsys): +def test_setup_zwift_reuses_configured_credentials_and_writes_no_secret(tmp_path, capsys): path = tmp_path / "config.yaml" _config(path) + config = yaml.safe_load(path.read_text()) + config["users"][0]["zwift"] = {"email": "z@example.com"} + path.write_text(yaml.safe_dump(config)) credentials.store_password("default:zwift", "private-value") - credentials.store_token("zwift_probe", { - "email": "z@example.com", - "user_name": "default", - "password_account": "default:zwift", - }) client = MagicMock() with patch("eufy_sync.zwift_client.ZwiftClient", return_value=client), \ @@ -93,15 +91,11 @@ def test_disconnect_zwift_preserves_other_config_and_arbitrary_probe_account(tmp assert "zwift_probe" not in vault["tokens"] -def test_setup_zwift_ignores_probe_metadata_for_another_user(tmp_path, capsys): +def test_setup_zwift_without_saved_credentials_needs_a_terminal(tmp_path, capsys): path = tmp_path / "config.yaml" _config(path) - credentials.store_password("other:zwift", "other-password") - credentials.store_token("zwift_probe", { - "email": "other@example.com", - "user_name": "other", - "password_account": "other:zwift", - }) + # A saved password alone, with no configured Zwift email, is not enough. + credentials.store_password("default:zwift", "orphaned-password") with patch("sys.stdin.isatty", return_value=False), \ patch("builtins.input", side_effect=AssertionError("must refuse before prompting")), \ From 88947c2f3a1bff7ca613e0a4c5f4fe1bf11d98d2 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:48:32 -0400 Subject: [PATCH 05/17] Label the latest weigh-in truthfully and refresh the uv index on update --- eufy_sync/cli/status.py | 9 +++++---- eufy_sync/install.py | 5 ++++- tests/test_summary.py | 4 ++-- tests/test_update_check.py | 4 ++-- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/eufy_sync/cli/status.py b/eufy_sync/cli/status.py index fa4d452..77626e5 100644 --- a/eufy_sync/cli/status.py +++ b/eufy_sync/cli/status.py @@ -48,14 +48,15 @@ def _print_summary( user = users[0] ts = state.get_latest_sync_timestamp(user.name) if ts: - last_sync = datetime.fromtimestamp(ts, tz=timezone.utc) - ago = datetime.now(timezone.utc) - last_sync + # The newest synced measurement's time, not when sync last ran. + latest = datetime.fromtimestamp(ts, tz=timezone.utc) + ago = datetime.now(timezone.utc) - latest days = ago.days hours = int(ago.total_seconds() / 3600) % 24 if days > 0: - parts.append(f"last sync: {days}d ago") + parts.append(f"latest weigh-in: {days}d ago") else: - parts.append(f"last sync: {hours}h ago") + parts.append(f"latest weigh-in: {hours}h ago") from eufy_sync.eufy_client import EufyClient eufy_status = EufyClient(user.eufy).token_status() diff --git a/eufy_sync/install.py b/eufy_sync/install.py index 96700da..e0e3bd3 100644 --- a/eufy_sync/install.py +++ b/eufy_sync/install.py @@ -31,7 +31,10 @@ def install_argv(spec: str) -> list[str]: "eufy-sync[browser]") through the installer that owns this copy.""" which = installer() if which == "uv": - return ["uv", "tool", "install", "--force", spec] + # uv trusts its cached package index, which can predate a release + # published minutes ago. pip revalidates index pages on every + # install, so the pipx and pip paths need no equivalent. + return ["uv", "tool", "install", "--force", "--refresh-package", "eufy-sync", spec] if which == "pipx": return ["pipx", "install", "--force", spec] return [sys.executable, "-m", "pip", "install", "--upgrade", spec] diff --git a/tests/test_summary.py b/tests/test_summary.py index f9b5501..1c78d69 100644 --- a/tests/test_summary.py +++ b/tests/test_summary.py @@ -63,7 +63,7 @@ def test_summary_no_new_measurements(capsys): output = capsys.readouterr().out.strip() assert "No new measurements" in output - assert "last sync: 5h ago" in output + assert "latest weigh-in: 5h ago" in output assert "Garmin connected" in output @@ -76,7 +76,7 @@ def test_summary_no_new_measurements_days_ago(capsys): _print_summary({}, [], state, [user]) output = capsys.readouterr().out.strip() - assert "last sync: 3d ago" in output + assert "latest weigh-in: 3d ago" in output def test_summary_synced_garmin_only(capsys): diff --git a/tests/test_update_check.py b/tests/test_update_check.py index 96b32bb..2533fd3 100644 --- a/tests/test_update_check.py +++ b/tests/test_update_check.py @@ -179,7 +179,7 @@ def test_self_update_uses_uv_when_installed_via_uv_tool(): patch("eufy_sync.cli.updater.subprocess.run", return_value=MagicMock(returncode=0)) as mock_run: _self_update() - assert mock_run.call_args.args[0] == ["uv", "tool", "install", "--force", "eufy-sync==9.9.9"] + assert mock_run.call_args.args[0] == ["uv", "tool", "install", "--force", "--refresh-package", "eufy-sync", "eufy-sync==9.9.9"] def test_self_update_uses_uv_for_windows_uv_tool_path(): @@ -317,4 +317,4 @@ def test_self_update_keeps_the_browser_extra_when_installed(monkeypatch): patch("eufy_sync.cli.updater.subprocess.run", return_value=MagicMock(returncode=0)) as mock_run: _self_update() - assert mock_run.call_args.args[0] == ["uv", "tool", "install", "--force", "eufy-sync[browser]==9.9.9"] + assert mock_run.call_args.args[0] == ["uv", "tool", "install", "--force", "--refresh-package", "eufy-sync", "eufy-sync[browser]==9.9.9"] From 924854806c6c7f2c68482e8f32c1c4aa9fd95b2e Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:48:32 -0400 Subject: [PATCH 06/17] Update the FitEncoder timezone note and document TZ for headless installs --- docs/headless-linux.md | 16 ++++++++++++++++ eufy_sync/transform.py | 12 +++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/headless-linux.md b/docs/headless-linux.md index abf4e87..583c9df 100644 --- a/docs/headless-linux.md +++ b/docs/headless-linux.md @@ -65,6 +65,22 @@ A user timer normally runs only while that user's systemd manager is active. If sudo loginctl enable-linger "$USER" ``` +## Set the timezone + +eufy-sync uses the machine's timezone to decide which day a weigh-in belongs to when it checks Garmin for an existing entry, and to show dates in its summaries and notifications. Servers, NAS boxes, and containers often run on UTC, which can put an evening weigh-in on the next day. Set `TZ` to your local zone for the scheduled run. + +For the systemd service, add this line under `[Service]`: + +```ini +Environment=TZ=America/New_York +``` + +For Docker, pass it when starting the container: + +```bash +docker run -e TZ=America/New_York ... +``` + ## Check a scheduled run ```bash diff --git a/eufy_sync/transform.py b/eufy_sync/transform.py index afcd671..1eb0346 100644 --- a/eufy_sync/transform.py +++ b/eufy_sync/transform.py @@ -41,11 +41,13 @@ def transform(measurement: EufyMeasurement) -> GarminBodyComposition | None: return None return GarminBodyComposition( - # garminconnect's add_body_composition encodes the FIT timestamp with - # mktime(timetuple()), which reads the wall-clock fields as LOCAL - # time. measurement.timestamp is UTC-aware, so convert to local time - # first (same instant, local wall-clock fields) or the upload lands - # shifted by the machine's UTC offset. + # Before garminconnect 0.3.17 (PR #438), add_body_composition encoded + # the FIT timestamp with mktime(timetuple()), which reads the + # wall-clock fields as LOCAL time and ignores the offset. We still + # support 0.3.10+, so convert the UTC-aware timestamp to local time + # first (same instant, local wall-clock fields); otherwise older + # versions shift the upload by the machine's UTC offset. Newer + # versions handle the offset and get the same instant either way. timestamp=measurement.timestamp.astimezone().isoformat(), weight=measurement.weight_kg, percent_fat=_clamp_or_none(measurement.body_fat_pct, 3.0, 60.0), From 2ce6f1e3c797094f3a2504f1a3a829c05a64ebf1 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:48:46 -0400 Subject: [PATCH 07/17] Keep the old Strava host as a fallback with no end date --- eufy_sync/strava_client.py | 14 ++++---------- tests/test_strava_client.py | 16 +++++++--------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/eufy_sync/strava_client.py b/eufy_sync/strava_client.py index a183c1a..cbf6f72 100644 --- a/eufy_sync/strava_client.py +++ b/eufy_sync/strava_client.py @@ -36,9 +36,6 @@ STRAVA_API_BASE_NEW = "https://api-v3.strava.com" # Strava's changelog (2026-06-01) says the new base is available from this date. NEW_API_BASE_FROM = date(2027, 1, 4) -# Third-party trackers report the old base stops working by this date. -# Strava's own docs name it only as the end of oauth/deauthorize. -OLD_API_BASE_UNTIL = date(2027, 6, 1) CALLBACK_PORT = 8089 REDIRECT_URI = f"http://localhost:{CALLBACK_PORT}/callback" REFRESH_SAFETY_MARGIN = 300 # seconds before expiry to trigger refresh @@ -51,16 +48,13 @@ def _utc_today() -> date: def _api_bases(today: date) -> tuple[str, ...]: """API bases to try, in order, for a request made on ``today``. - Before the new host exists we use the old one. During the overlap we try - the new host first and keep the old one as a fallback, since users may - run an old release well past either date. Once the old host is retired, - only the new one is left. + Before the new host exists we use the old one. After that we try the new + host first and keep the old one as a fallback. Strava hasn't published a + shutdown date for the old host, so the fallback has no end date either. """ if today < NEW_API_BASE_FROM: return (STRAVA_API_BASE_OLD,) - if today < OLD_API_BASE_UNTIL: - return (STRAVA_API_BASE_NEW, STRAVA_API_BASE_OLD) - return (STRAVA_API_BASE_NEW,) + return (STRAVA_API_BASE_NEW, STRAVA_API_BASE_OLD) def _auth_url(client_id: str, state_value: str) -> str: diff --git a/tests/test_strava_client.py b/tests/test_strava_client.py index 843732c..01c8013 100644 --- a/tests/test_strava_client.py +++ b/tests/test_strava_client.py @@ -251,7 +251,7 @@ def test_auth_url_encodes_every_query_value(): BEFORE_NEW_HOST = date(2027, 1, 3) NEW_HOST_DAY_ONE = date(2027, 1, 4) LAST_OVERLAP_DAY = date(2027, 5, 31) -OLD_HOST_RETIRED = date(2027, 6, 1) +LONG_AFTER = date(2028, 3, 1) def _client_on(day: date) -> StravaClient: @@ -281,8 +281,7 @@ def _urls(mock_request) -> list[str]: (BEFORE_NEW_HOST, STRAVA_API_BASE_OLD), (NEW_HOST_DAY_ONE, STRAVA_API_BASE_NEW), (LAST_OVERLAP_DAY, STRAVA_API_BASE_NEW), - (OLD_HOST_RETIRED, STRAVA_API_BASE_NEW), - (date(2028, 3, 1), STRAVA_API_BASE_NEW), + (LONG_AFTER, STRAVA_API_BASE_NEW), ]) def test_api_base_follows_the_published_schedule(day, expected): client = _client_on(day) @@ -359,12 +358,11 @@ def test_connection_failure_before_the_switch_is_not_retried(): client.close() -def test_connection_failure_after_old_host_retired_has_no_fallback(): - client = _client_on(OLD_HOST_RETIRED) - with patch.object(client._client, "request", side_effect=httpx.ConnectError("refused")) as mock_request: - with pytest.raises(httpx.ConnectError): - client.update_weight(86.2) - assert _urls(mock_request) == [f"{STRAVA_API_BASE_NEW}/athlete"] +def test_old_host_stays_as_fallback_with_no_end_date(): + client = _client_on(LONG_AFTER) + with patch.object(client._client, "request", side_effect=[httpx.ConnectError("refused"), _ok()]) as mock_request: + client.update_weight(86.2) + assert _urls(mock_request) == [f"{STRAVA_API_BASE_NEW}/athlete", f"{STRAVA_API_BASE_OLD}/athlete"] client.close() From a50e5804ca9dd3bc9779e859837dcbdb68806130 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:52:09 -0400 Subject: [PATCH 08/17] Keep Garmin sessions until a new login succeeds and lock credential commands --- eufy_sync/cli/app.py | 66 ++++++++------ eufy_sync/cli/maintenance.py | 163 +++++++++++++++++++++++------------ eufy_sync/garmin_auth.py | 21 +++-- eufy_sync/garmin_client.py | 45 +++++++--- tests/test_cli.py | 118 +++++++++++++++++++++++++ tests/test_garmin_auth.py | 87 +++++++++++++++++-- tests/test_garmin_client.py | 65 ++++++++++++++ tests/test_lock.py | 101 ++++++++++++++++++++++ 8 files changed, 556 insertions(+), 110 deletions(-) diff --git a/eufy_sync/cli/app.py b/eufy_sync/cli/app.py index 58efb4f..2b65438 100644 --- a/eufy_sync/cli/app.py +++ b/eufy_sync/cli/app.py @@ -1,6 +1,7 @@ """The eufy-sync command line entry point and sync driver.""" from __future__ import annotations +import contextlib import logging import sys from pathlib import Path @@ -64,6 +65,20 @@ def _password_failure_title(failures: list) -> str: return "eufy-sync: login failed" +@contextlib.contextmanager +def _credential_lock(command: str): + """Hold the sync lock for a command that changes credentials, tokens, or + config, or exit with a retry hint when a sync or another such command has + it. A lock file that cannot be opened also refuses, unlike the sync path: + an unserialized credential write can strand a token a sync just rotated.""" + from eufy_sync.cli import lock + with lock.single_instance(require_lock=True) as acquired: + if not acquired: + print(f"Another eufy-sync run is in progress. Retry {command} when it finishes.") + sys.exit(1) + yield + + def _sync_with_network_retry(user, state, **kwargs): """Give scheduled network failures one delayed retry, keeping partial counts.""" from eufy_sync.network import is_transient_network_error @@ -169,14 +184,19 @@ def _main() -> None: # Handle full uninstall if args.uninstall: - maintenance._uninstall(shared.DATA_DIR, config_path=config_path, db_path=db_path) + with _credential_lock("--uninstall"): + removed = maintenance._uninstall(shared.DATA_DIR, config_path=config_path, db_path=db_path) + if removed: + # The lock file outlives the sweep because it was held open. + maintenance._remove_lock_file(shared.DATA_DIR) return # Handle credential store mode switches if args.use_file_store: from eufy_sync import credentials try: - credentials.use_file_store() + with _credential_lock("--use-file-store"): + credentials.use_file_store() except RuntimeError as e: print(str(e)) sys.exit(1) @@ -186,7 +206,8 @@ def _main() -> None: if args.use_keychain: from eufy_sync import credentials try: - credentials.use_keychain_store() + with _credential_lock("--use-keychain"): + credentials.use_keychain_store() except RuntimeError as e: print(str(e)) sys.exit(1) @@ -209,55 +230,43 @@ def _main() -> None: # Handle self-update if args.update: - updater._self_update() + # A reinstall mid-sync swaps the code under the running process. + with _credential_lock("--update"): + updater._self_update() return # Handle Strava setup if args.setup_strava: - setup._setup_strava(config_path) + with _credential_lock("--setup-strava"): + setup._setup_strava(config_path) return if args.setup_zwift: - from eufy_sync.cli import lock - with lock.single_instance(require_lock=True) as acquired: - if not acquired: - print("Another eufy-sync run is in progress. Retry --setup-zwift when it finishes.") - sys.exit(1) + with _credential_lock("--setup-zwift"): setup._setup_zwift(config_path) return if args.disconnect_zwift: - from eufy_sync.cli import lock - with lock.single_instance(require_lock=True) as acquired: - if not acquired: - print("Another eufy-sync run is in progress. Retry --disconnect-zwift when it finishes.") - sys.exit(1) + with _credential_lock("--disconnect-zwift"): maintenance._disconnect_zwift(config_path) return # Handle profile selection if args.select_profile: - profiles._select_profile(config_path) + with _credential_lock("--select-profile"): + profiles._select_profile(config_path) return # Handle password update if args.update_password: - from eufy_sync.cli import lock - with lock.single_instance(require_lock=True) as acquired: - if not acquired: - print("Another eufy-sync run is in progress. Retry --update-password when it finishes.") - sys.exit(1) + with _credential_lock("--update-password"): maintenance._update_password(config_path) return # Handle reauth if args.reauth is not None: target = None if args.reauth == "all" else args.reauth - from eufy_sync.cli import lock - with lock.single_instance(require_lock=True) as acquired: - if not acquired: - print("Another eufy-sync run is in progress. Retry --reauth when it finishes.") - sys.exit(1) + with _credential_lock("--reauth"): maintenance._reauth(config_path, force=True, target=target) return @@ -279,7 +288,10 @@ def _main() -> None: try: if first_run: - setup._first_run_setup(config_path) + # Setup stores passwords and can log in to Garmin and Zwift. The + # lock is released before the first sync takes it again below. + with _credential_lock("eufy-sync"): + setup._first_run_setup(config_path) else: # Migrate existing plaintext passwords to keychain (one-time) setup._migrate_config_passwords(config_path) diff --git a/eufy_sync/cli/maintenance.py b/eufy_sync/cli/maintenance.py index d950313..d9f75b8 100644 --- a/eufy_sync/cli/maintenance.py +++ b/eufy_sync/cli/maintenance.py @@ -14,7 +14,7 @@ def _update_password(config_path: Path) -> None: - """Update stored passwords.""" + """Update stored passwords, each one only after it logs in.""" if not config_path.exists(): print("No config found. Run eufy-sync first to set up.") sys.exit(1) @@ -36,46 +36,77 @@ def _update_password(config_path: Path) -> None: print("No changes made.") return - from eufy_sync.credentials import delete_token, store_password - - if eufy_pw: - store_password(f"{user_name}:eufy", eufy_pw) - - if garmin_pw: - store_password(f"{user_name}:garmin", garmin_pw) - if zwift_pw: - store_password(f"{user_name}:zwift", zwift_pw) - - # Clear cached tokens for changed services - if eufy_pw: - delete_token("eufy") - eufy_token = shared.DATA_DIR / "eufy_token.json" - if eufy_token.exists(): - eufy_token.unlink() - - if garmin_pw: - delete_token("garmin") - garmin_session = shared.DATA_DIR / "session.json" - if garmin_session.exists(): - garmin_session.unlink() - if zwift_pw: - delete_token("zwift") - - changed = [] - if eufy_pw: - changed.append("Eufy") - if garmin_pw: - changed.append("Garmin") - if zwift_pw: - changed.append("Zwift") - print(f"{' and '.join(changed)} password{'s' if len(changed) > 1 else ''} updated.") - - if garmin_pw: - print("Garmin password changed - re-authenticating...") - _reauth(config_path, config, target="garmin") - if zwift_pw: - print("Zwift password changed - re-authenticating...") - _reauth(config_path, config, target="zwift") + from eufy_sync.credentials import store_password + + # Each new password logs in before it is stored. The login saves its own + # token on success; a failure (a typo, a cancelled MFA prompt, no network) + # leaves that service's stored password and token as they were, so a + # mistake here cannot end a session that was still working. + changes = [ + (label, password, verify) + for label, password, verify in ( + ("Eufy", eufy_pw, _verify_eufy_password), + ("Garmin", garmin_pw, _verify_garmin_password), + ("Zwift", zwift_pw, _verify_zwift_password), + ) + if password + ] + updated = [] + for label, password, verify in changes: + print(f"Checking the new {label} password...") + try: + verify(user, password) + except Exception as e: + print(f"{label} login with the new password failed: {e}") + print(f"The stored {label} password and login were left unchanged.") + if updated: + print(f"Already updated: {' and '.join(updated)}.") + print("Retry with: eufy-sync --update-password") + sys.exit(1) + store_password(f"{user_name}:{label.lower()}", password) + updated.append(label) + + print(f"{' and '.join(updated)} password{'s' if len(updated) > 1 else ''} updated.") + + +def _verify_eufy_password(user: dict, password: str) -> None: + """Log in to Eufy with password; the token is replaced only on success.""" + from eufy_sync.config import EufyConfig + from eufy_sync.eufy_client import EufyClient + + client = EufyClient( + EufyConfig(email=user["eufy"]["email"], password=password), + token_path=shared.DATA_DIR / "eufy_token.json", + ) + try: + # Skip the cached token on purpose: it says nothing about the password. + client._fresh_login() + finally: + client.close() + + +def _verify_garmin_password(user: dict, password: str) -> None: + """Log in to Garmin with password, prompting for MFA if Garmin asks. + force_reauth replaces the stored token only after the login succeeds.""" + from eufy_sync.garmin_auth import GarminAuth + + auth = GarminAuth(user["garmin"]["email"], password, session_path=shared.DATA_DIR / "session.json") + auth.force_reauth() + print("Done - Garmin tokens saved.") + + +def _verify_zwift_password(user: dict, password: str) -> None: + """Log in to Zwift with password. A forced login reads the profile before + it saves anything, so the cached token is replaced only on success.""" + from eufy_sync.config import ZwiftConfig + from eufy_sync.zwift_client import ZwiftClient + + client = ZwiftClient(ZwiftConfig(email=user["zwift"]["email"], password=password)) + try: + client.authenticate(force=True) + finally: + client.close() + print("Done - Zwift token saved.") def _reauth(config_path: Path, config: dict | None = None, force: bool = False, target: str | None = None) -> None: @@ -214,12 +245,14 @@ def _uninstall_launch_agent() -> None: platform_support.uninstall_agent() -def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | None = None) -> None: +def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | None = None) -> bool: """Remove all eufy-sync data: Launch Agent, config, tokens, state DB. config_path/db_path default to the standard files under data_dir, but a custom --config/--db location (outside data_dir) is also deleted so --uninstall does not leave those files behind. + + Returns True when the data was removed, False when the user cancelled. """ if not sys.stdin.isatty(): print("Error: --uninstall requires an interactive terminal.") @@ -236,7 +269,7 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | answer = input("Are you sure? [y/N] ").strip() if not answer.lower().startswith("y"): print("Cancelled.") - return + return False default_config_path = data_dir / "config.yaml" default_db_path = data_dir / "state.db" @@ -292,21 +325,24 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | # Remove data directory. A kept DB at a custom --db path lives outside # data_dir, so only the default location needs the selective sweep. + # The sync lock file is skipped: --uninstall holds it open, and Windows + # refuses to delete an open file. _remove_lock_file clears it once the + # lock is released. preserve_default_db = keep_db and db_path == default_db_path and db_path.exists() if data_dir.exists(): - if not preserve_default_db: - shutil.rmtree(data_dir) - else: - for item in data_dir.iterdir(): - if item.name == "state.db": - continue - if item.is_dir(): - shutil.rmtree(item) - else: - item.unlink() + from eufy_sync.cli.lock import LOCK_NAME + keep = {LOCK_NAME, "state.db"} if preserve_default_db else {LOCK_NAME} + for item in data_dir.iterdir(): + if item.name in keep: + continue + if item.is_dir(): + shutil.rmtree(item) + else: + item.unlink() + _remove_dir_if_empty(data_dir) # A custom --config/--db path lives outside data_dir, so it survives the - # rmtree above and must be removed explicitly. + # sweep above and must be removed explicitly. if config_path != default_config_path and config_path.exists(): config_path.unlink() if db_path != default_db_path and not keep_db and db_path.exists(): @@ -319,3 +355,22 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | print("Removed all eufy-sync data.") print(f"To remove the package itself, run: {install.uninstall_command()}") + return True + + +def _remove_dir_if_empty(path: Path) -> None: + try: + path.rmdir() + except OSError: + pass + + +def _remove_lock_file(data_dir: Path) -> None: + """Delete the sync lock file left by --uninstall, after it is released, + and the data dir with it when nothing else remains there.""" + from eufy_sync.cli.lock import LOCK_NAME + try: + (data_dir / LOCK_NAME).unlink(missing_ok=True) + except OSError: + return + _remove_dir_if_empty(data_dir) diff --git a/eufy_sync/garmin_auth.py b/eufy_sync/garmin_auth.py index f9bc421..1f34202 100644 --- a/eufy_sync/garmin_auth.py +++ b/eufy_sync/garmin_auth.py @@ -302,8 +302,12 @@ def login(self, interactive: bool = True) -> Garmin: return garmin def force_reauth(self) -> Garmin: - """Clear the stored token and do a fresh interactive login.""" - self._clear_token() + """Do a fresh interactive login and store its token. + + The login runs on a new client and the stored token is replaced only + once it succeeds. A failed attempt (a cancelled MFA prompt, a wrong + password, a passing Garmin or Cloudflare error) leaves the old token + where it was, since that session may still be good.""" garmin = Garmin(self.email, self.password, prompt_mfa=_mfa_prompt) self._fresh_login(garmin) self._save_token(garmin) @@ -311,12 +315,13 @@ def force_reauth(self) -> Garmin: return garmin def silent_reauth(self) -> Garmin: - """Clear the stored token and log in again with nobody watching. + """Log in again with nobody watching and store the new token. The scheduled counterpart to force_reauth: same recovery from a session Garmin has stopped honoring, but it never prompts and never opens a - browser.""" - self._clear_token() + browser. The old token stays stored unless this login succeeds: a 403 + that only looked like a dead session would otherwise cost an MFA user + a working session and force an interactive --reauth.""" garmin = Garmin(self.email, self.password, prompt_mfa=_headless_mfa_prompt) self._silent_login(garmin) self._save_token(garmin) @@ -453,9 +458,3 @@ def _save_token(self, garmin: Garmin) -> None: store_token("garmin", blob) if self.session_path.exists(): self.session_path.unlink() - - def _clear_token(self) -> None: - from eufy_sync.credentials import delete_token - delete_token("garmin") - if self.session_path.exists(): - self.session_path.unlink() diff --git a/eufy_sync/garmin_client.py b/eufy_sync/garmin_client.py index 28a94fb..bee63c1 100644 --- a/eufy_sync/garmin_client.py +++ b/eufy_sync/garmin_client.py @@ -68,9 +68,15 @@ def _match_uploaded_entry(entries: list[dict], uploaded_at: datetime) -> dict | def _is_garmin_auth_failure(exc: Exception) -> bool: - """True when a Garmin call failed because the session is dead. Covers the - dedicated auth error and the 401/403 that the library reports as a generic - connection error ("API Error 401 - ...").""" + """True when a Garmin call failed because the session may be dead. Covers + the dedicated auth error and the 401/403 that the library reports as a + generic connection error ("API Error 401 - ..."). + + A Cloudflare 403 matches too: the library drops the response body before + raising, so the message is the same "API Error 403" either way. That is + safe only because a relogin keeps the stored token until the new login + succeeds, so a 403 that was only a passing block costs one login attempt, + not the session.""" if isinstance(exc, GarminConnectAuthenticationError): return True return isinstance(exc, GarminConnectConnectionError) and ( @@ -84,6 +90,11 @@ def __init__(self, config: GarminConfig): self._auth = GarminAuth(config.email, config.password) self._garmin = None self._allow_interactive = True + # The error from a relogin that failed this run, if any. Later calls + # raise it again instead of trying another login: a second attempt + # minutes later meets the same MFA demand or wrong password, and every + # extra login raises the odds of a Garmin 429. + self._reauth_error: Exception | None = None def authenticate(self, allow_interactive: bool = True) -> None: self._allow_interactive = allow_interactive @@ -96,13 +107,20 @@ def _reauth(self) -> None: usually needs no input at all, so it tries once silently rather than ending the run on a re-auth nag the run could have fixed itself. When even that cannot proceed (MFA demanded, password wrong), the error - already names the command to run and travels to the caller unchanged.""" - if not self._allow_interactive: - logger.info("Garmin session expired; re-authenticating without prompts") - self._garmin = self._auth.silent_reauth() - else: - logger.info("Garmin session expired; re-authenticating") - self._garmin = self._auth.force_reauth() + already names the command to run and travels to the caller unchanged. + + Only one relogin is tried per run. After a failure, every later call + that needs one gets the same error back without contacting Garmin.""" + try: + if not self._allow_interactive: + logger.info("Garmin session expired; re-authenticating without prompts") + self._garmin = self._auth.silent_reauth() + else: + logger.info("Garmin session expired; re-authenticating") + self._garmin = self._auth.force_reauth() + except Exception as e: + self._reauth_error = e + raise def _call_with_reauth(self, call): """Run a Garmin call, re-logging in once when the session is dead. @@ -116,6 +134,11 @@ def _call_with_reauth(self, call): except (GarminConnectAuthenticationError, GarminConnectConnectionError) as e: if not _is_garmin_auth_failure(e): raise + if self._reauth_error is not None: + # This run already tried to log in again and failed. Report + # that failure, which names the fix, rather than this call's + # 401 or 403. + raise self._reauth_error from e self._reauth() return call() @@ -139,6 +162,8 @@ def read() -> bool: return self._call_with_reauth(read) except Exception as e: # Fail open: let the upload proceed; Garmin de-dupes by timestamp. + # A failed relogin is remembered, so the upload reports it with its + # fix-it hint instead of logging in a second time. logger.warning("Garmin duplicate-check failed for %s: %s", date_str, e) return False diff --git a/tests/test_cli.py b/tests/test_cli.py index 5887b2d..410eaaa 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1797,3 +1797,121 @@ def test_dunder_version_matches_pyproject(): with open(pyproject, "rb") as f: declared = tomllib.load(f)["project"]["version"] assert eufy_sync.__version__ == declared + + +# --- --update-password logs in before it stores anything --- + +_OLD_GARMIN_TOKEN = {"di_token": "old", "di_refresh_token": "old-r", "di_client_id": "cid"} +_OLD_ZWIFT_TOKEN = {"access_token": "old-zwift"} +_OLD_EUFY_TOKEN = {"access_token": "old-eufy", "user_id": "u", "expires_at": 9e12} + + +def _password_update_setup(tmp_path: Path) -> Path: + from eufy_sync.credentials import store_password, store_token + config_path = tmp_path / "config.yaml" + _write_config(config_path, {"users": [{ + "name": "default", + "eufy": {"email": "e@example.com"}, + "garmin": {"email": "g@example.com"}, + "zwift": {"email": "z@example.com"}, + }]}) + for service in ("eufy", "garmin", "zwift"): + store_password(f"default:{service}", f"old-{service}") + store_token("eufy", dict(_OLD_EUFY_TOKEN)) + store_token("garmin", dict(_OLD_GARMIN_TOKEN)) + store_token("zwift", dict(_OLD_ZWIFT_TOKEN)) + return config_path + + +def _stored(service: str) -> tuple: + from eufy_sync.credentials import get_password, get_token + return get_password(f"default:{service}"), get_token(service) + + +def _garmin_rejecting_login(): + from garminconnect import GarminConnectAuthenticationError + fake = MagicMock() + fake.login.side_effect = GarminConnectAuthenticationError("401 Unauthorized") + return patch("eufy_sync.garmin_auth.Garmin", return_value=fake) + + +def test_update_password_typo_keeps_garmin_password_and_session(tmp_path, capsys): + from eufy_sync.cli.maintenance import _update_password + config_path = _password_update_setup(tmp_path) + + with patch("getpass.getpass", side_effect=["", "typo", ""]), _garmin_rejecting_login(): + with pytest.raises(SystemExit) as exc: + _update_password(config_path) + + assert exc.value.code == 1 + assert _stored("garmin") == ("old-garmin", _OLD_GARMIN_TOKEN) + assert _stored("zwift") == ("old-zwift", _OLD_ZWIFT_TOKEN) + assert _stored("eufy") == ("old-eufy", _OLD_EUFY_TOKEN) + assert "left unchanged" in capsys.readouterr().out + + +def test_update_password_typo_keeps_zwift_password_and_session(tmp_path): + from eufy_sync.cli.maintenance import _update_password + from eufy_sync.sync import PermanentSyncError + config_path = _password_update_setup(tmp_path) + + zwift = MagicMock() + zwift.authenticate.side_effect = PermanentSyncError("Zwift login was rejected") + with patch("getpass.getpass", side_effect=["", "", "typo"]), \ + patch("eufy_sync.zwift_client.ZwiftClient", return_value=zwift): + with pytest.raises(SystemExit): + _update_password(config_path) + + zwift.authenticate.assert_called_once_with(force=True) + assert _stored("zwift") == ("old-zwift", _OLD_ZWIFT_TOKEN) + assert _stored("garmin") == ("old-garmin", _OLD_GARMIN_TOKEN) + + +def test_update_password_typo_keeps_eufy_password_and_session(tmp_path): + from eufy_sync.cli.maintenance import _update_password + from eufy_sync.sync import PermanentSyncError + config_path = _password_update_setup(tmp_path) + + with patch("getpass.getpass", side_effect=["typo", "", ""]), \ + patch("eufy_sync.eufy_client.EufyClient._fresh_login", + side_effect=PermanentSyncError("Eufy login failed")): + with pytest.raises(SystemExit): + _update_password(config_path) + + assert _stored("eufy") == ("old-eufy", _OLD_EUFY_TOKEN) + + +def test_update_password_stores_garmin_password_and_new_session_after_login(tmp_path): + import json + + from eufy_sync.cli.maintenance import _update_password + config_path = _password_update_setup(tmp_path) + + new_token = {"di_token": "new", "di_refresh_token": "new-r", "di_client_id": "cid"} + fake = MagicMock() + fake.client.dumps.return_value = json.dumps(new_token) + with patch("getpass.getpass", side_effect=["", "new-garmin", ""]), \ + patch("eufy_sync.garmin_auth.Garmin", return_value=fake) as ctor: + _update_password(config_path) + + # The login used the new password, not the stored one. + assert ctor.call_args.args[:2] == ("g@example.com", "new-garmin") + assert _stored("garmin") == ("new-garmin", new_token) + assert _stored("zwift") == ("old-zwift", _OLD_ZWIFT_TOKEN) + + +def test_update_password_keeps_earlier_updates_when_a_later_login_fails(tmp_path, capsys): + # Each service is all-or-nothing on its own: Eufy logged in and is stored, + # Garmin did not and is left exactly as it was. + from eufy_sync.cli.maintenance import _update_password + config_path = _password_update_setup(tmp_path) + + with patch("getpass.getpass", side_effect=["new-eufy", "typo", ""]), \ + patch("eufy_sync.eufy_client.EufyClient._fresh_login"), \ + _garmin_rejecting_login(): + with pytest.raises(SystemExit): + _update_password(config_path) + + assert _stored("eufy")[0] == "new-eufy" + assert _stored("garmin") == ("old-garmin", _OLD_GARMIN_TOKEN) + assert "Already updated: Eufy" in capsys.readouterr().out diff --git a/tests/test_garmin_auth.py b/tests/test_garmin_auth.py index 74bd388..f07d90b 100644 --- a/tests/test_garmin_auth.py +++ b/tests/test_garmin_auth.py @@ -73,17 +73,91 @@ def test_token_status_valid_with_blob(monkeypatch): assert auth.token_status()["state"] == "valid" -def test_force_reauth_clears_token_logs_in_and_saves(monkeypatch): +def test_force_reauth_logs_in_and_saves(monkeypatch): auth = _auth() calls = [] - monkeypatch.setattr(auth, "_clear_token", lambda: calls.append("clear")) monkeypatch.setattr(auth, "_save_token", lambda g: calls.append("save")) fake_garmin = MagicMock() with patch("eufy_sync.garmin_auth.Garmin", return_value=fake_garmin): result = auth.force_reauth() assert result is fake_garmin fake_garmin.login.assert_called_once() - assert calls == ["clear", "save"] # cleared before login, saved after + assert calls == ["save"] + + +NEW_BLOB = {"di_token": "new", "di_refresh_token": "new-r", "di_client_id": "cid"} + + +def _store_old_token(): + from eufy_sync.credentials import store_token + store_token("garmin", dict(BLOB)) + + +def _stored_token(): + from eufy_sync.credentials import get_token + return get_token("garmin") + + +def test_force_reauth_keeps_the_stored_token_when_login_fails(monkeypatch): + # A cancelled MFA prompt or a passing Garmin error must not cost the + # session that was stored before the attempt. + from eufy_sync.garmin_auth import GarminLoginCancelled + _store_old_token() + auth = _auth() + fake = MagicMock() + fake.login.side_effect = GarminLoginCancelled("no code") + with patch("eufy_sync.garmin_auth.Garmin", return_value=fake): + with pytest.raises(PermanentSyncError): + auth.force_reauth() + assert _stored_token() == BLOB + + +def test_force_reauth_replaces_the_stored_token_on_success(): + _store_old_token() + auth = _auth() + fake = MagicMock() + fake.client.dumps.return_value = json.dumps(NEW_BLOB) + with patch("eufy_sync.garmin_auth.Garmin", return_value=fake): + auth.force_reauth() + assert _stored_token() == NEW_BLOB + + +def test_silent_reauth_keeps_the_stored_token_when_garmin_wants_mfa(monkeypatch): + # The headless case from the field: a 403 looked like a dead session, the + # relogin hit an MFA demand, and the working token used to be gone. + from eufy_sync.garmin_auth import GarminLoginCancelled + _store_old_token() + _fail_on_browser(monkeypatch) + auth = _auth() + fake = MagicMock() + fake.login.side_effect = GarminLoginCancelled("mfa") + with patch("eufy_sync.garmin_auth.Garmin", return_value=fake): + with pytest.raises(PermanentSyncError): + auth.silent_reauth() + assert _stored_token() == BLOB + + +def test_silent_reauth_keeps_the_stored_token_on_a_transient_failure(monkeypatch): + _store_old_token() + _fail_on_browser(monkeypatch) + auth = _auth() + fake = MagicMock() + fake.login.side_effect = ConnectionError("Connection reset by peer") + with patch("eufy_sync.garmin_auth.Garmin", return_value=fake): + with pytest.raises(ConnectionError): + auth.silent_reauth() + assert _stored_token() == BLOB + + +def test_silent_reauth_replaces_the_stored_token_on_success(monkeypatch): + _store_old_token() + _fail_on_browser(monkeypatch) + auth = _auth() + fake = MagicMock() + fake.client.dumps.return_value = json.dumps(NEW_BLOB) + with patch("eufy_sync.garmin_auth.Garmin", return_value=fake): + assert auth.silent_reauth() is fake + assert _stored_token() == NEW_BLOB def test_login_falls_back_to_fresh_when_blob_unusable(monkeypatch): @@ -315,10 +389,9 @@ def test_headless_mfa_prompt_cancels_without_reading_input(monkeypatch): _headless_mfa_prompt() -def test_silent_reauth_clears_token_logs_in_and_saves(monkeypatch): +def test_silent_reauth_logs_in_and_saves(monkeypatch): auth = _auth() calls = [] - monkeypatch.setattr(auth, "_clear_token", lambda: calls.append("clear")) monkeypatch.setattr(auth, "_save_token", lambda g: calls.append("save")) _fail_on_browser(monkeypatch) fake = MagicMock() @@ -326,13 +399,12 @@ def test_silent_reauth_clears_token_logs_in_and_saves(monkeypatch): result = auth.silent_reauth() assert result is fake fake.login.assert_called_once() - assert calls == ["clear", "save"] + assert calls == ["save"] def test_silent_reauth_mfa_demand_names_reauth(monkeypatch): from eufy_sync.garmin_auth import GarminLoginCancelled auth = _auth() - monkeypatch.setattr(auth, "_clear_token", lambda: None) _fail_on_browser(monkeypatch) fake = MagicMock() fake.login.side_effect = GarminLoginCancelled("mfa") @@ -345,7 +417,6 @@ def test_silent_reauth_mfa_demand_names_reauth(monkeypatch): def test_force_reauth_falls_back_to_browser(monkeypatch): from garminconnect import GarminConnectTooManyRequestsError auth = _auth() - monkeypatch.setattr(auth, "_clear_token", lambda: None) monkeypatch.setattr(auth, "_save_token", lambda g: None) fake = MagicMock() fake.login.side_effect = GarminConnectTooManyRequestsError("429") diff --git a/tests/test_garmin_client.py b/tests/test_garmin_client.py index fd2d736..46b1e09 100644 --- a/tests/test_garmin_client.py +++ b/tests/test_garmin_client.py @@ -496,3 +496,68 @@ def test_delete_weight_entry_fails_open_when_the_relogin_fails(): with patch.object(client._auth, "silent_reauth", side_effect=PermanentSyncError("Garmin wants an MFA code")): assert client.delete_weight_entry(UPLOADED_AT, 85.0) is False + + +# --------------------------------------------------------------------------- +# One relogin per run +# --------------------------------------------------------------------------- + + +def test_failed_relogin_in_duplicate_check_is_not_retried_by_the_upload(): + # The duplicate check fails open after a relogin that wants MFA. The + # upload that follows must report that same failure, with its fix-it hint, + # rather than try a second login (another MFA demand, more 429 risk). + from garminconnect import GarminConnectConnectionError + + from eufy_sync.sync import PermanentSyncError + dead = MagicMock() + dead.get_body_composition.side_effect = GarminConnectConnectionError("API Error 403") + dead.add_body_composition.side_effect = GarminConnectConnectionError("API Error 403") + client = _client_with_fake_garmin(dead) + client._allow_interactive = False + failure = PermanentSyncError("Garmin wants an MFA code. Run: eufy-sync --reauth garmin") + bc = GarminBodyComposition(timestamp="2026-06-10T08:00:00+00:00", weight=80.0) + with patch.object(client._auth, "silent_reauth", side_effect=failure) as reauth: + assert client.has_weight_on_date(datetime(2026, 6, 10, tzinfo=timezone.utc)) is False + with pytest.raises(PermanentSyncError) as exc: + client.upload_body_composition(bc) + reauth.assert_called_once() + assert exc.value is failure + assert "--reauth garmin" in str(exc.value) + + +def test_failed_relogin_in_delete_is_not_retried_by_the_upload(): + from garminconnect import GarminConnectAuthenticationError + + from eufy_sync.sync import PermanentSyncError + dead = MagicMock() + dead.get_daily_weigh_ins.side_effect = GarminConnectAuthenticationError("dead") + dead.add_body_composition.side_effect = GarminConnectAuthenticationError("dead") + client = _client_with_fake_garmin(dead) + client._allow_interactive = True + failure = PermanentSyncError("Garmin login cancelled") + bc = GarminBodyComposition(timestamp="2026-06-10T08:00:00+00:00", weight=80.0) + with patch.object(client._auth, "force_reauth", side_effect=failure) as reauth: + assert client.delete_weight_entry(UPLOADED_AT, 85.0) is False + with pytest.raises(PermanentSyncError): + client.upload_body_composition(bc) + reauth.assert_called_once() + + +def test_old_session_still_serves_calls_after_a_failed_relogin(): + # A Cloudflare 403 reads like a dead session but may pass. After the + # relogin fails, the old session is kept, and a later call that goes + # through is not blocked by the remembered failure. + from garminconnect import GarminConnectConnectionError + + from eufy_sync.sync import PermanentSyncError + session = MagicMock() + session.get_body_composition.side_effect = GarminConnectConnectionError("API Error 403") + session.add_body_composition.return_value = {"ok": True} + client = _client_with_fake_garmin(session) + client._allow_interactive = False + bc = GarminBodyComposition(timestamp="2026-06-10T08:00:00+00:00", weight=80.0) + with patch.object(client._auth, "silent_reauth", side_effect=PermanentSyncError("mfa")): + assert client.has_weight_on_date(datetime(2026, 6, 10, tzinfo=timezone.utc)) is False + assert client.upload_body_composition(bc) == {"ok": True} + assert client._garmin is session diff --git a/tests/test_lock.py b/tests/test_lock.py index bc7c00d..c67c013 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -138,3 +138,104 @@ def test_sync_runs_and_releases_the_lock_for_the_next_run( assert exc.value.code == 0 with lock.single_instance() as acquired: assert acquired is True + + +# --------------------------------------------------------------------------- +# Commands that change credentials, tokens, or config wait for a running sync +# --------------------------------------------------------------------------- + +# (extra argv, function the command runs, flag named in the retry hint) +_CREDENTIAL_COMMANDS = [ + (["--uninstall"], "eufy_sync.cli.maintenance._uninstall", "--uninstall"), + (["--use-file-store"], "eufy_sync.credentials.use_file_store", "--use-file-store"), + (["--use-keychain"], "eufy_sync.credentials.use_keychain_store", "--use-keychain"), + (["--update"], "eufy_sync.cli.updater._self_update", "--update"), + (["--setup-strava"], "eufy_sync.cli.setup._setup_strava", "--setup-strava"), + (["--setup-zwift"], "eufy_sync.cli.setup._setup_zwift", "--setup-zwift"), + (["--disconnect-zwift"], "eufy_sync.cli.maintenance._disconnect_zwift", "--disconnect-zwift"), + (["--select-profile"], "eufy_sync.cli.profiles._select_profile", "--select-profile"), + (["--update-password"], "eufy_sync.cli.maintenance._update_password", "--update-password"), + (["--reauth"], "eufy_sync.cli.maintenance._reauth", "--reauth"), + (["--reauth", "garmin"], "eufy_sync.cli.maintenance._reauth", "--reauth"), + # No config: the first-run wizard, which stores passwords and logs in. + ([], "eufy_sync.cli.setup._first_run_setup", "eufy-sync"), +] + + +def _command_argv(tmp_path: Path, extra: list[str]) -> list[str]: + # The config path does not exist, so a bare run reaches first-run setup. + return ["eufy-sync", "--config", str(tmp_path / "missing.yaml"), *extra] + + +@pytest.mark.parametrize("extra, target, flag", _CREDENTIAL_COMMANDS) +def test_credential_command_refuses_while_a_sync_holds_the_lock(tmp_path, capsys, extra, target, flag): + from eufy_sync.cli.app import main + + with lock.single_instance() as held: + assert held is True + with patch(target) as command, \ + patch("sys.argv", _command_argv(tmp_path, extra)), \ + pytest.raises(SystemExit) as exc: + main() + + assert exc.value.code == 1 + command.assert_not_called() + assert f"Retry {flag} when it finishes" in capsys.readouterr().out + + +@pytest.mark.parametrize("extra, target, flag", _CREDENTIAL_COMMANDS) +def test_credential_command_runs_while_holding_the_lock(tmp_path, extra, target, flag): + from eufy_sync.cli.app import main + + seen = [] + + def check_lock(*args, **kwargs): + with lock.single_instance() as other: + seen.append(other) + + with patch(target, side_effect=check_lock), \ + patch("sys.argv", _command_argv(tmp_path, extra)): + try: + main() + except SystemExit: + pass # several of these commands exit on their own + + assert seen == [False] # a sync starting mid-command would have been kept out + with lock.single_instance() as acquired: + assert acquired is True # and the lock is free again afterwards + + +@pytest.mark.parametrize("extra, target, flag", _CREDENTIAL_COMMANDS) +def test_credential_command_refuses_when_the_lock_file_cannot_open(tmp_path, capsys, extra, target, flag): + from eufy_sync.cli.app import main + + with patch("eufy_sync.cli.lock.os.open", side_effect=OSError("read-only")), \ + patch(target) as command, \ + patch("sys.argv", _command_argv(tmp_path, extra)), \ + pytest.raises(SystemExit) as exc: + main() + + assert exc.value.code == 1 + command.assert_not_called() + + +@patch("eufy_sync.credentials._keyring_available", return_value=False) +@patch("eufy_sync.platform_support.agent_installed", return_value=False) +@patch("eufy_sync.cli.maintenance.sys.stdin") +@patch("builtins.input", return_value="y") +def test_uninstall_under_the_lock_still_removes_the_whole_data_dir( + _input, mock_stdin, _agent, _keyring, tmp_path +): + """--uninstall holds the lock file open while it sweeps the data dir, and + Windows cannot delete an open file. The sweep skips it and the command + removes it after release, so nothing is left behind.""" + from eufy_sync.cli.app import main + + mock_stdin.isatty.return_value = True + config_path = _write_synced_config(shared.DATA_DIR) + with patch("sys.argv", ["eufy-sync", "--uninstall", "--config", str(config_path)]): + main() + + assert not config_path.exists() + assert not lock.lock_path().exists() + assert not shared.DATA_DIR.exists() From e22b8f0d8277f311db40a323d3e0078c2091cdb3 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:52:31 -0400 Subject: [PATCH 09/17] Make keychain vault saves crash-safe Chunked vaults are written under a fresh generation (vault::) and the header is switched last, so a save killed at any step leaves the old or the new vault readable. The header carries a checksum, and the next save sweeps leftovers from the neighbouring generations. A damaged vault (bad JSON, missing or mismatched chunk, corrupt credentials file) now raises VaultCorruptError instead of reading as empty, so the next store can no longer save an empty vault over stored passwords. Released single-entry and vault: layouts are still read and migrate on the next save. --- eufy_sync/credentials.py | 329 +++++++++++++++++++++++-------- tests/test_credentials.py | 393 +++++++++++++++++++++++++++++++++++--- 2 files changed, 624 insertions(+), 98 deletions(-) diff --git a/eufy_sync/credentials.py b/eufy_sync/credentials.py index 62d2c09..895de54 100644 --- a/eufy_sync/credentials.py +++ b/eufy_sync/credentials.py @@ -24,7 +24,9 @@ A keychain that exists but cannot be read (locked, access denied) does raise, so a failed read can never be saved back over the real vault, and --use-file-store aborts rather than write an empty marker file that would -orphan the unread keychain secrets. +orphan the unread keychain secrets. A vault that is present but damaged +(unparseable JSON, a missing chunk) raises VaultCorruptError, a RuntimeError, +for the same reason: reading it as empty would let the next save wipe it. A lazy, one-time migration promotes secrets from the old per-item keychain layout (one keyring account per password/token) into the vault the first @@ -32,6 +34,7 @@ """ from __future__ import annotations +import hashlib import json import logging import os @@ -44,9 +47,14 @@ # Windows Credential Manager caps one entry at ~2,560 bytes stored as UTF-16, # roughly 1,280 characters. A vault larger than CHUNK_LIMIT characters is split -# across numbered "vault:i" entries so set_password never fails on Windows. -# MAX_CHUNKS bounds how far a save probes for stale leftover chunks to delete, -# so a corrupt store can never make that scan run away. +# across "vault::" entries so set_password never fails on Windows; the +# "vault" entry then holds a small header naming the generation, the chunk +# count and a checksum. Each save writes a fresh generation and switches the +# header last, so an interrupted save never splices two vaults together. +# Released versions wrote chunks as "vault:"; those are still read and are +# replaced on the next save. MAX_CHUNKS bounds the chunk count a header may +# claim and how far a save probes for leftovers to delete, so a corrupt store +# can never make either run away. CHUNK_LIMIT = 1200 MAX_CHUNKS = 40 @@ -128,96 +136,272 @@ def _normalize_vault(vault: dict | None) -> dict: return normalized -def _load_vault_from_keychain() -> dict: +_KEYCHAIN_UNREADABLE = ( + "The system keychain could not be read (it may be locked or " + "access was denied). Unlock it and retry, or run: " + "eufy-sync --use-file-store" +) + + +class VaultCorruptError(RuntimeError): + """The stored vault exists but cannot be parsed or reassembled. + + Raised instead of returning an empty vault: an empty result would be + saved back by the next store_*() call, wiping every stored secret. It is + a RuntimeError, so callers that already handle an unreadable keychain + handle this the same way.""" + + +def _keychain_corrupt(detail: str) -> VaultCorruptError: + return VaultCorruptError( + f"The credential vault in the system keychain is damaged ({detail}). " + "It was left untouched so nothing is saved over it. To start over, " + f'delete the "{VAULT_ACCOUNT}" item for "{SERVICE_NAME}" in your ' + "keychain app, then run eufy-sync to sign in again." + ) + + +def _keychain_get(account: str) -> str | None: import keyring try: - raw = keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT) + return keyring.get_password(SERVICE_NAME, account) except Exception as e: # Returning an empty vault here would let the next read-modify-write # save a near-empty vault over the real one. Raising keeps every # caller safe; sync/doctor/startup already report exceptions cleanly. - raise RuntimeError( - "The system keychain could not be read (it may be locked or " - "access was denied). Unlock it and retry, or run: " - "eufy-sync --use-file-store" - ) from e - if raw is None: - return _empty_vault() + raise RuntimeError(_KEYCHAIN_UNREADABLE) from e + + +def _valid_count(value) -> bool: + # bool is an int subclass; a header saying {"__chunks__": true} is junk. + return type(value) is int and 1 <= value <= MAX_CHUNKS + + +def _parse_header(raw: str) -> tuple: + """Classify the "vault" entry. Returns one of: + + ("single", vault) the whole vault in one entry + ("legacy", count) released-version chunks "vault:1".."vault:" + ("gen", gen, count, sha256) chunks "vault::1".."vault::" + + Raises VaultCorruptError for anything else.""" try: parsed = json.loads(raw) - # An oversized vault is stored as a header pointing at numbered chunks; - # reassemble the payload before normalizing. A missing chunk means the - # header outlived its payload, which is as unusable as malformed JSON. - if isinstance(parsed, dict) and "__chunks__" in parsed: - count = parsed["__chunks__"] - pieces = [] - for i in range(1, count + 1): - try: - piece = keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}") - except Exception as e: - # A keyring failure mid-reassembly (locked, access denied) is - # the same unreadable-keychain condition as the initial read, - # not a missing chunk. Translate it so a partial read can - # never be saved back over the real vault. A chunk that - # returns None (below) is a genuinely missing chunk and keeps - # its malformed-vault handling. - raise RuntimeError( - "The system keychain could not be read (it may be locked or " - "access was denied). Unlock it and retry, or run: " - "eufy-sync --use-file-store" - ) from e - if piece is None: - raise ValueError("missing vault chunk") - pieces.append(piece) - parsed = json.loads("".join(pieces)) - return _normalize_vault(parsed) except (ValueError, TypeError): - logger.warning("Keychain vault contained malformed JSON; treating as empty") - return _empty_vault() + raise _keychain_corrupt("the vault entry is not valid JSON") from None + if not isinstance(parsed, dict): + raise _keychain_corrupt("the vault entry is not a JSON object") + if "__vault__" in parsed: + meta = parsed["__vault__"] + if ( + isinstance(meta, dict) + and type(meta.get("gen")) is int + and meta["gen"] >= 1 + and _valid_count(meta.get("chunks")) + and isinstance(meta.get("sha256"), str) + ): + return ("gen", meta["gen"], meta["chunks"], meta["sha256"]) + raise _keychain_corrupt("the chunk header is malformed") + if "__chunks__" in parsed: + if _valid_count(parsed["__chunks__"]): + return ("legacy", parsed["__chunks__"]) + raise _keychain_corrupt("the chunk header is malformed") + return ("single", parsed) + + +def _chunk_account(gen: int | None, i: int) -> str: + # gen None is the layout released versions wrote ("vault:1", "vault:2"). + if gen is None: + return f"{VAULT_ACCOUNT}:{i}" + return f"{VAULT_ACCOUNT}:{gen}:{i}" + + +def _assemble(layout: tuple) -> dict: + """Turn a parsed header into a vault dict, reading chunks as needed.""" + if layout[0] == "single": + return _normalize_vault(layout[1]) + if layout[0] == "legacy": + gen, count, digest = None, layout[1], None + else: + _, gen, count, digest = layout + pieces = [] + for i in range(1, count + 1): + # A keyring failure here (locked, access denied) raises the + # unreadable-keychain RuntimeError, same as the header read. A chunk + # that comes back None is genuinely missing: the vault is damaged. + piece = _keychain_get(_chunk_account(gen, i)) + if piece is None: + raise _keychain_corrupt(f"chunk {i} of {count} is missing") + pieces.append(piece) + payload = "".join(pieces) + if digest is not None and hashlib.sha256(payload.encode()).hexdigest() != digest: + raise _keychain_corrupt("the chunks do not match their header") + try: + parsed = json.loads(payload) + except (ValueError, TypeError): + raise _keychain_corrupt("the reassembled vault is not valid JSON") from None + if not isinstance(parsed, dict): + raise _keychain_corrupt("the reassembled vault is not a JSON object") + return _normalize_vault(parsed) -def _delete_stale_chunks(start: int) -> None: - # A previous save may have used more chunks than this one. Delete numbered - # entries from `start` upward until the first gap, so a later read can never - # reassemble a stale tail. Bounded by MAX_CHUNKS. +def _load_vault_from_keychain() -> dict: + raw = _keychain_get(VAULT_ACCOUNT) + if raw is None: + return _empty_vault() + try: + return _assemble(_parse_header(raw)) + except VaultCorruptError: + # A concurrent save may have switched the header and deleted the + # chunks this read was partway through. If the header moved on, read + # the new vault once; if it did not, the vault really is damaged. + fresh = _keychain_get(VAULT_ACCOUNT) + if fresh is None or fresh == raw: + raise + return _assemble(_parse_header(fresh)) + + +def _delete_chunk_run(gen: int | None, start: int) -> None: + # Delete one generation's chunk entries from `start` up to the first gap. + # Chunks are always written 1..N in order, so leftovers from an + # interrupted save form an unbroken run. Deleting from the top down keeps + # it unbroken if this sweep is itself cut short, so the next sweep still + # finds the rest. Bounded by MAX_CHUNKS. import keyring + run = [] for i in range(start, MAX_CHUNKS + 1): - account = f"{VAULT_ACCOUNT}:{i}" + account = _chunk_account(gen, i) if keyring.get_password(SERVICE_NAME, account) is None: break + run.append(account) + for account in reversed(run): try: keyring.delete_password(SERVICE_NAME, account) except Exception: pass +def _sweep_chunks(prev_gen: int, keep_gen: int | None = None, keep_count: int = 0) -> None: + """Best-effort removal of chunk entries that no header points at. + + prev_gen is the generation the header named before this change (0 when + it named none). Its neighbours are swept too: prev_gen - 1 catches a + cleanup a crash cut short, and prev_gen + 1 catches chunks a crashed save + wrote before it could switch the header. keep_gen/keep_count protect the + chunks the header now references. The released-version "vault:i" names + are probed until a save after generation 1 (the first one written after + them) has swept them.""" + try: + if prev_gen <= 1: + _delete_chunk_run(None, 1) + for gen in (prev_gen - 1, prev_gen, prev_gen + 1): + if gen < 1: + continue + _delete_chunk_run(gen, keep_count + 1 if gen == keep_gen else 1) + except Exception: + # The new vault is already in place; leftovers cost tidiness, not + # data, so a failed cleanup must not fail the save. + pass + + +def _current_gen() -> int: + """The generation the stored header names, or 0 for none. A header this + module cannot parse counts as 0: saves only ever follow a successful + load, so the next sweep still covers whatever it can identify.""" + raw = _keychain_get(VAULT_ACCOUNT) + if raw is None: + return 0 + try: + layout = _parse_header(raw) + except VaultCorruptError: + return 0 + if layout[0] == "gen": + return layout[1] + if layout[0] == "single": + gen = layout[1].get("__gen__") + return gen if type(gen) is int and gen >= 1 else 0 + return 0 + + def _save_vault_to_keychain(vault: dict) -> None: import keyring + prev_gen = _current_gen() + # json.dumps escapes non-ASCII by default, so each character is one + # UTF-16 unit and a CHUNK_LIMIT-character entry stays under the Windows cap. payload = json.dumps(vault) - if len(payload) <= CHUNK_LIMIT: - keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, payload) - _delete_stale_chunks(1) + # A vault that shrinks back into one entry keeps the last chunk generation + # as "__gen__" (dropped on load), so a sweep that a crash cuts short can + # still find that generation's leftovers on the next save. + single = json.dumps({**vault, "__gen__": prev_gen}) if prev_gen else payload + if len(single) <= CHUNK_LIMIT: + keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, single) + _sweep_chunks(prev_gen) return + # The new chunks go under a generation no header references yet, and the + # single-entry header write is the commit point. A save killed before it + # leaves the old vault readable; one killed after it leaves the new vault + # readable plus leftovers that the next save sweeps. + gen = prev_gen + 1 chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] - # Chunks first, header last: a reader that races the write sees either the - # old vault or a complete new one, never a header pointing at a chunk that - # has not been written yet. for i, chunk in enumerate(chunks, start=1): - keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}", chunk) - keyring.set_password( - SERVICE_NAME, VAULT_ACCOUNT, json.dumps({"__chunks__": len(chunks)}) - ) - _delete_stale_chunks(len(chunks) + 1) + keyring.set_password(SERVICE_NAME, _chunk_account(gen, i), chunk) + header = { + "__vault__": { + "gen": gen, + "chunks": len(chunks), + "sha256": hashlib.sha256(payload.encode()).hexdigest(), + } + } + keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps(header)) + _sweep_chunks(prev_gen, keep_gen=gen, keep_count=len(chunks)) + + +def _delete_keychain_vault() -> None: + """Best-effort removal of the vault header and every chunk entry.""" + import keyring + try: + prev_gen = _current_gen() + except Exception: + prev_gen = 0 + try: + keyring.delete_password(SERVICE_NAME, VAULT_ACCOUNT) + except Exception: + pass + _sweep_chunks(prev_gen) + if prev_gen: + # Released-version chunks an earlier install left behind. + try: + _delete_chunk_run(None, 1) + except Exception: + pass def _load_vault_from_file() -> dict: - if not CRED_FILE.exists(): - return _empty_vault() try: - return _normalize_vault(json.loads(CRED_FILE.read_text())) - except (ValueError, TypeError, OSError): - logger.warning("Credentials file contained malformed JSON; treating as empty") + text = CRED_FILE.read_text() + except FileNotFoundError: return _empty_vault() + except ValueError: + parsed = None # non-UTF-8 bytes + except OSError as e: + raise RuntimeError( + f"The credentials file {CRED_FILE} could not be read " + f"({e.strerror or e}). Check its permissions and retry." + ) from e + else: + try: + parsed = json.loads(text) + except ValueError: + parsed = None + if not isinstance(parsed, dict): + # Treating this as empty would let the next save replace the file + # with an empty vault, destroying whatever is still recoverable in it. + raise VaultCorruptError( + f"The credentials file {CRED_FILE} is damaged (not a JSON object). " + "It was left untouched so nothing is saved over it. Repair it or " + "move it aside, then run eufy-sync to sign in again." + ) + return _normalize_vault(parsed) def _save_vault_to_file(vault: dict) -> None: @@ -388,6 +572,9 @@ def use_file_store() -> None: if _keyring_available(): try: keychain_vault = _load_vault_from_keychain() + except VaultCorruptError: + # Already says what is damaged and that nothing was changed. + raise except Exception as e: raise RuntimeError( "The system keychain could not be read (it may be locked or " @@ -414,20 +601,12 @@ def use_file_store() -> None: _save_vault_to_file(merged) if _keyring_available(): - try: - import keyring - keyring.delete_password(SERVICE_NAME, VAULT_ACCOUNT) - except Exception: - pass - # An oversized vault also left numbered "vault:i" entries behind, each - # holding a slice of the same plaintext secrets. Deleting only the - # header hides them from every reader but leaves full copies in the - # keychain the user just opted out of. Best-effort: the file already + # An oversized vault also has chunk entries, each holding a slice of + # the same plaintext secrets. Deleting only the header hides them from + # every reader but leaves full copies in the keychain the user just + # opted out of, so the chunks go too. Best-effort: the file already # holds the merged vault, so a failure here must not fail the switch. - try: - _delete_stale_chunks(1) - except Exception: - pass + _delete_keychain_vault() def use_keychain_store() -> None: diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 03fd70e..419bcae 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -328,25 +328,47 @@ def test_auto_fallback_creates_file_and_persists_token_with_no_keychain(no_keyri # --- 7. malformed vault JSON -------------------------------------------------- -def test_malformed_file_vault_json_returns_none_not_crash(no_keyring, cred_file): - from eufy_sync.credentials import get_password, get_token +def test_malformed_file_vault_raises_and_is_never_overwritten(no_keyring, cred_file): + """Reading a damaged file as empty would let the next store save an + empty vault over it. The read raises instead, and the file is untouched.""" + from eufy_sync.credentials import VaultCorruptError, get_password, get_token cred_file.parent.mkdir(parents=True, exist_ok=True) cred_file.write_text("{not valid json::") - assert get_password("default:eufy") is None - assert get_token("eufy") is None + with pytest.raises(VaultCorruptError, match="damaged"): + get_password("default:eufy") + with pytest.raises(RuntimeError): + get_token("eufy") + with pytest.raises(RuntimeError): + store_token("eufy", {"access_token": "new"}) + + assert cred_file.read_text() == "{not valid json::" + +def test_non_object_file_vault_raises(no_keyring, cred_file): + from eufy_sync.credentials import VaultCorruptError -def test_malformed_keychain_vault_json_returns_none_not_crash(fake_keyring, cred_file): + cred_file.parent.mkdir(parents=True, exist_ok=True) + cred_file.write_text("[1, 2]") + + with pytest.raises(VaultCorruptError): + get_token("eufy") + + +def test_malformed_keychain_vault_raises_and_is_never_overwritten(fake_keyring, cred_file): import keyring - from eufy_sync.credentials import SERVICE_NAME, get_password, get_token + from eufy_sync.credentials import SERVICE_NAME, VaultCorruptError, get_password keyring.set_password(SERVICE_NAME, "vault", "{not valid json::") - assert get_password("default:eufy") is None - assert get_token("eufy") is None + with pytest.raises(VaultCorruptError, match="damaged"): + get_password("default:eufy") + with pytest.raises(VaultCorruptError): + store_token("garmin", {"di_token": "new"}) + + assert fake_keyring.get_password(SERVICE_NAME, "vault") == "{not valid json::" # --- 8. backward compat: legacy config.yaml inline password ----------------- @@ -771,31 +793,359 @@ def test_small_vault_keeps_single_entry_shape(fake_keyring): assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:1") is None +def _vault_accounts(store: _FakeKeyringStore) -> set[str]: + """Every vault-related entry currently stored (header and chunks).""" + return { + account for account in store.accounts_written() + if account == VAULT_ACCOUNT or account.startswith(f"{VAULT_ACCOUNT}:") + } + + +def _header(store: _FakeKeyringStore) -> dict: + return json.loads(store.get_password(SERVICE_NAME, VAULT_ACCOUNT))["__vault__"] + + +def _chunk_accounts(gen: int, count: int) -> set[str]: + return {f"{VAULT_ACCOUNT}:{gen}:{i}" for i in range(1, count + 1)} + + def test_oversized_vault_chunks_and_round_trips(fake_keyring): store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - header = json.loads(fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT)) - n = header["__chunks__"] + header = _header(fake_keyring) + n = header["chunks"] assert n >= 3 - for i in range(1, n + 1): - chunk = fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}") + for account in _chunk_accounts(header["gen"], n): + chunk = fake_keyring.get_password(SERVICE_NAME, account) assert chunk is not None assert len(chunk) <= CHUNK_LIMIT + # Windows caps one entry at 2,560 bytes of UTF-16. + assert len(chunk.encode("utf-16-le")) <= 2560 assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], n) + + +def test_non_ascii_vault_respects_the_windows_entry_cap(fake_keyring): + """json.dumps escapes non-ASCII, so a vault full of multi-byte characters + still splits into entries under the per-entry byte cap.""" + store_token("garmin", {"access_token": "é\U0001f600" * CHUNK_LIMIT}) + header = _header(fake_keyring) + for account in _chunk_accounts(header["gen"], header["chunks"]): + assert len(fake_keyring.get_password(SERVICE_NAME, account).encode("utf-16-le")) <= 2560 + assert get_token("garmin") == {"access_token": "é\U0001f600" * CHUNK_LIMIT} + + +def test_each_save_uses_a_new_generation_and_deletes_the_old_one(fake_keyring): + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + first = _header(fake_keyring) + store_token("garmin", _big_token(4 * CHUNK_LIMIT)) + second = _header(fake_keyring) + + assert second["gen"] == first["gen"] + 1 + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(second["gen"], second["chunks"]) + ) + assert get_token("garmin") == _big_token(4 * CHUNK_LIMIT) def test_shrinking_vault_deletes_stale_chunks(fake_keyring): store_token("garmin", _big_token(3 * CHUNK_LIMIT)) store_token("garmin", {"a": 1}) # replaces the big token; vault fits again assert get_token("garmin") == {"a": 1} - raw = fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT) - assert "__chunks__" not in json.loads(raw) - assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:1") is None + raw = json.loads(fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT)) + assert "__vault__" not in raw + assert raw["tokens"]["garmin"] == {"a": 1} + assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} -def test_missing_chunk_reads_as_empty_vault(fake_keyring, caplog): +def test_missing_chunk_raises_and_is_never_overwritten(fake_keyring, cred_file): + """A header whose chunk is gone is a damaged vault. Reading it as empty + would let the next store_token save a vault holding only that token over + every stored password.""" + from eufy_sync.credentials import VaultCorruptError, store_password + + store_password("default:eufy", "pw") store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + gen = _header(fake_keyring)["gen"] + fake_keyring.delete_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{gen}:2") + before = dict(fake_keyring.data) + + with pytest.raises(VaultCorruptError, match="chunk 2 of"): + get_token("garmin") + with pytest.raises(VaultCorruptError): + store_token("strava", {"access_token": "t"}) + + assert fake_keyring.data == before + + +def test_chunks_that_do_not_match_the_header_raise(fake_keyring): + from eufy_sync.credentials import VaultCorruptError + + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + gen = _header(fake_keyring)["gen"] + account = f"{VAULT_ACCOUNT}:{gen}:1" + chunk = fake_keyring.get_password(SERVICE_NAME, account) + fake_keyring.set_password(SERVICE_NAME, account, chunk.replace("x", "y")) + + with pytest.raises(VaultCorruptError, match="do not match"): + get_token("garmin") + + +@pytest.mark.parametrize("header", [ + {"__chunks__": 0}, + {"__chunks__": True}, + {"__chunks__": 10_000}, + {"__vault__": {"gen": 1, "chunks": 2}}, + {"__vault__": "nope"}, +]) +def test_malformed_chunk_header_raises(fake_keyring, header): + from eufy_sync.credentials import VaultCorruptError + + fake_keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps(header)) + with pytest.raises(VaultCorruptError): + get_token("garmin") + + +def test_absent_vault_reads_as_empty(fake_keyring): + assert get_token("garmin") is None + assert _vault_accounts(fake_keyring) == set() + + +# --- Crash safety ------------------------------------------------------------ +# +# A save is a sequence of keyring writes and deletes. Killing the process +# between any two of them must leave a vault that reads back as either the old +# contents or the new ones, and the next completed save must clear whatever +# the killed one left behind. + + +class _Killed(BaseException): + """Simulates the process dying: BaseException, so no `except Exception` + cleanup path inside the module can swallow it.""" + + +def _kill_after(monkeypatch, store: _FakeKeyringStore, steps: int) -> None: + """Let `steps` backend mutations through, then kill on the next one.""" + calls = {"n": 0} + + def tick(): + calls["n"] += 1 + if calls["n"] > steps: + raise _Killed() + + def set_password(service, account, password): + tick() + store.set_password(service, account, password) + + def delete_password(service, account): + tick() + store.delete_password(service, account) + + monkeypatch.setattr("keyring.set_password", set_password) + monkeypatch.setattr("keyring.delete_password", delete_password) + + +def _restore(monkeypatch, store: _FakeKeyringStore) -> None: + monkeypatch.setattr("keyring.set_password", store.set_password) + monkeypatch.setattr("keyring.delete_password", store.delete_password) + + +def _count_mutations(monkeypatch, store, action) -> int: + snapshot = dict(store.data) + counted = {"n": 0} + + def set_password(service, account, password): + counted["n"] += 1 + store.set_password(service, account, password) + + def delete_password(service, account): + counted["n"] += 1 + store.delete_password(service, account) + + monkeypatch.setattr("keyring.set_password", set_password) + monkeypatch.setattr("keyring.delete_password", delete_password) + action() + _restore(monkeypatch, store) + store.data = snapshot + return counted["n"] + + +_OLD_PW = {"default:eufy": "pw1", "default:garmin": "pw2"} + +# (description, token before, token after): big -> big, small -> big, +# big -> small, and big -> big starting from a vault already on a generation. +_TRANSITIONS = [ + ("big_to_big", _big_token(3 * CHUNK_LIMIT), _big_token(5 * CHUNK_LIMIT)), + ("big_to_smaller_big", _big_token(5 * CHUNK_LIMIT), _big_token(2 * CHUNK_LIMIT)), + ("small_to_big", {"a": 1}, _big_token(3 * CHUNK_LIMIT)), + ("big_to_small", _big_token(3 * CHUNK_LIMIT), {"a": 1}), +] + + +@pytest.mark.parametrize("name,old,new", _TRANSITIONS, ids=[t[0] for t in _TRANSITIONS]) +@pytest.mark.parametrize("presaves", [1, 2, "legacy"]) +def test_kill_at_every_step_of_a_save_leaves_a_readable_vault( + fake_keyring, cred_file, monkeypatch, name, old, new, presaves +): + """presaves "legacy" starts from the released "vault:i" chunk layout, so + the killed save is the one migrating it.""" + from eufy_sync.credentials import _load_vault, store_password + + def setup(): + fake_keyring.data.clear() + if presaves == "legacy": + _write_legacy_chunked( + fake_keyring, {"passwords": dict(_OLD_PW), "tokens": {"garmin": old}} + ) + return + for account, pw in _OLD_PW.items(): + store_password(account, pw) + for _ in range(presaves): + store_token("garmin", old) + + setup() + total = _count_mutations(monkeypatch, fake_keyring, lambda: store_token("garmin", new)) + assert total >= 2 + + for steps in range(total): + setup() + _kill_after(monkeypatch, fake_keyring, steps) + with pytest.raises(_Killed): + store_token("garmin", new) + _restore(monkeypatch, fake_keyring) + + vault = _load_vault() + assert vault["passwords"] == _OLD_PW, f"killed after {steps} of {total}" + assert vault["tokens"]["garmin"] in (old, new), f"killed after {steps} of {total}" + + # The next completed save leaves only the entries its header uses. + store_token("strava", {"access_token": "t"}) + vault = _load_vault() + assert vault["passwords"] == _OLD_PW + assert vault["tokens"]["strava"] == {"access_token": "t"} + raw = json.loads(fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT)) + expected = {VAULT_ACCOUNT} + if "__vault__" in raw: + expected |= _chunk_accounts(raw["__vault__"]["gen"], raw["__vault__"]["chunks"]) + assert _vault_accounts(fake_keyring) == expected, f"killed after {steps} of {total}" + + +def test_reader_retries_once_when_a_save_switches_the_header_mid_read(fake_keyring, monkeypatch): + """A save in another process can switch the header and delete the old + chunks while this process is reading them. The reader sees a missing + chunk, notices the header moved on, and reads the new vault.""" + from eufy_sync import credentials + + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + old_gen = _header(fake_keyring)["gen"] + real_get = fake_keyring.get_password + state = {"saved": False} + + def racing_get(service, account): + if account == f"{VAULT_ACCOUNT}:{old_gen}:2" and not state["saved"]: + state["saved"] = True + monkeypatch.setattr("keyring.get_password", real_get) + credentials._save_vault_to_keychain( + {"passwords": {}, "tokens": {"garmin": _big_token(4 * CHUNK_LIMIT)}} + ) + monkeypatch.setattr("keyring.get_password", racing_get) + return real_get(service, account) + + monkeypatch.setattr("keyring.get_password", racing_get) + assert get_token("garmin") == _big_token(4 * CHUNK_LIMIT) + + +# --- Upgrade from the released chunk layout ---------------------------------- + + +def _write_legacy_chunked(store: _FakeKeyringStore, vault: dict) -> int: + """Store `vault` exactly as released versions did: "vault:1".."vault:N" + under a {"__chunks__": N} header.""" + payload = json.dumps(vault) + chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] + for i, chunk in enumerate(chunks, start=1): + store.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}", chunk) + store.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps({"__chunks__": len(chunks)})) + return len(chunks) + + +def test_legacy_chunked_vault_reads_and_migrates_on_next_save(fake_keyring, cred_file): + from eufy_sync.credentials import get_password + + legacy = { + "passwords": {"default:eufy": "pw"}, + "tokens": {"garmin": _big_token(3 * CHUNK_LIMIT)}, + } + n = _write_legacy_chunked(fake_keyring, legacy) + assert n >= 3 + + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + assert get_password("default:eufy") == "pw" + + store_token("strava", {"access_token": "t"}) + + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], header["chunks"]) + ) + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + assert get_token("strava") == {"access_token": "t"} + assert get_password("default:eufy") == "pw" + + +def test_legacy_chunked_vault_with_missing_chunk_raises(fake_keyring): + from eufy_sync.credentials import VaultCorruptError + + _write_legacy_chunked(fake_keyring, {"passwords": {}, "tokens": {"garmin": _big_token(3 * CHUNK_LIMIT)}}) fake_keyring.delete_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:2") - assert get_token("garmin") is None # malformed vault treated as empty + with pytest.raises(VaultCorruptError): + get_token("garmin") + + +def test_legacy_single_entry_vault_reads_and_stays_single(fake_keyring, cred_file): + from eufy_sync.credentials import get_password + + fake_keyring.set_password( + SERVICE_NAME, VAULT_ACCOUNT, + json.dumps({"passwords": {"default:eufy": "pw"}, "tokens": {"garmin": {"a": 1}}}), + ) + assert get_password("default:eufy") == "pw" + store_token("strava", {"b": 2}) + assert get_token("garmin") == {"a": 1} + assert get_token("strava") == {"b": 2} + assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} + + +def test_stale_generations_left_by_crashes_are_cleaned_up(fake_keyring): + """Leftovers from a cleanup a crash cut short (the generation before the + current one) and from a save killed before its header switch (the + generation after it) are both deleted by the next save.""" + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + store_token("garmin", _big_token(3 * CHUNK_LIMIT + 1)) + gen = _header(fake_keyring)["gen"] + for stale in (gen - 1, gen + 1): + for i in (1, 2, 3): + fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{stale}:{i}", "junk") + + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT + 1) + store_token("garmin", _big_token(2 * CHUNK_LIMIT)) + + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], header["chunks"]) + ) + assert get_token("garmin") == _big_token(2 * CHUNK_LIMIT) + + +def test_shared_service_name_and_legacy_token_item_are_untouched(fake_keyring, cred_file): + """Another tool reads the legacy "token:garmin" item under the same + service name. Vault saves, chunked or not, must never touch it.""" + from eufy_sync.credentials import SERVICE_NAME as service + + assert service == "eufy-garmin-sync" + fake_keyring.set_password(SERVICE_NAME, "token:garmin", json.dumps({"x": 1})) + store_token("strava", _big_token(3 * CHUNK_LIMIT)) + store_token("strava", {"a": 1}) + assert fake_keyring.get_password(SERVICE_NAME, "token:garmin") == json.dumps({"x": 1}) def test_chunk_read_failure_raises_friendly_error(fake_keyring, cred_file, monkeypatch): @@ -829,15 +1179,12 @@ def test_use_file_store_deletes_chunk_entries_not_just_the_header(fake_keyring, from eufy_sync.credentials import _active_backend, get_token, use_file_store store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - header = json.loads(fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT)) - n = header["__chunks__"] - assert n >= 3 + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) # move past generation 1 + assert _header(fake_keyring)["chunks"] >= 3 use_file_store() - assert fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT) is None - for i in range(1, n + 1): - assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}") is None + assert _vault_accounts(fake_keyring) == set() on_disk = json.loads(cred_file.read_text()) assert on_disk["explicit"] is True From 1d542e4d4d59c1a30bcda863c59dbe505a2f5953 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 09:53:17 -0400 Subject: [PATCH 10/17] Label weigh-in age accurately in doctor and docs; declare test tools as a dev group --- README.md | 2 +- docs/command-reference.md | 2 +- eufy_sync/cli/doctor.py | 4 ++-- pyproject.toml | 3 +++ 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 111b0ee..e08f44c 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ If you want to add another target later, follow [Adding Strava](#adding-strava) ```bash eufy-sync # sync new measurements to every configured target -eufy-sync --status # show the last sync and token health +eufy-sync --status # show the latest synced weigh-in and token health eufy-sync --dry-run # preview a sync without uploading eufy-sync --doctor # check the setup and print fixes eufy-sync --history # show recent sync history diff --git a/docs/command-reference.md b/docs/command-reference.md index b1993d3..6d2e2ea 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -5,7 +5,7 @@ Running `eufy-sync` with no options syncs new measurements to every configured t ## Check and preview ```bash -eufy-sync --status # last sync and token health +eufy-sync --status # latest synced weigh-in and token health eufy-sync --history # the last 14 sync-history entries eufy-sync --history 30 # choose how many entries to show eufy-sync --dry-run # preview without uploading diff --git a/eufy_sync/cli/doctor.py b/eufy_sync/cli/doctor.py index b6421cc..2a53f60 100644 --- a/eufy_sync/cli/doctor.py +++ b/eufy_sync/cli/doctor.py @@ -281,9 +281,9 @@ def _check_state_db(report, db_path: Path, user) -> None: days = ago.days hours = int(ago.total_seconds() / 3600) if days > 0: - report("PASS", "state db", f"last sync {days}d ago") + report("PASS", "state db", f"latest weigh-in {days}d ago") else: - report("PASS", "state db", f"last sync {hours}h ago") + report("PASS", "state db", f"latest weigh-in {hours}h ago") except Exception as e: report("FAIL", "state db", str(e)) finally: diff --git a/pyproject.toml b/pyproject.toml index 65fddb5..4edf98f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,6 +45,9 @@ dependencies = [ # several times the size of everything else, and most installs never need it. browser = ["playwright>=1.40.0"] +[dependency-groups] +dev = ["pytest", "ruff"] + [project.scripts] eufy-sync = "eufy_sync.cli:main" From 918eb6840e4ef721a05042c60caa264aec71e2e8 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:03:53 -0400 Subject: [PATCH 11/17] Run vault-writing password migrations only under the sync lock --status and --history now resolve passwords without migrating legacy keychain items or YAML passwords, and the sync path migrates after it takes the lock, so an unlocked process never writes the credential vault. --- eufy_sync/cli/app.py | 101 +++++++++++++++++++++++---------------- eufy_sync/config.py | 30 ++++++++---- eufy_sync/credentials.py | 9 +++- tests/test_lock.py | 85 +++++++++++++++++++++++++++++++- 4 files changed, 171 insertions(+), 54 deletions(-) diff --git a/eufy_sync/cli/app.py b/eufy_sync/cli/app.py index 2b65438..cb2fdb2 100644 --- a/eufy_sync/cli/app.py +++ b/eufy_sync/cli/app.py @@ -106,6 +106,29 @@ def _sync_with_network_retry(user, state, **kwargs): sleep(NETWORK_RETRY_DELAY) +def _exit_could_not_start(error: Exception) -> None: + msg = f"eufy-sync could not start: {error}" + print(msg) + platform_support.notify("eufy-sync failed", str(error)[:200]) + sys.exit(1) + + +def _load_config_or_exit(config_path: Path, migrate: bool = True): + from eufy_sync.config import load_config + try: + return load_config(config_path, migrate=migrate) + except SystemExit: + raise + except Exception as e: + _exit_could_not_start(e) + + +def _check_target(args, config) -> None: + if args.target and not any(getattr(u, args.target, None) for u in config.users): + print(f"Target '{args.target}' is not configured. Check your config.") + sys.exit(1) + + def main() -> None: try: _main() @@ -286,58 +309,36 @@ def _main() -> None: platform_support.notify("eufy-sync", msg) sys.exit(1) - try: - if first_run: - # Setup stores passwords and can log in to Garmin and Zwift. The - # lock is released before the first sync takes it again below. - with _credential_lock("eufy-sync"): - setup._first_run_setup(config_path) - else: - # Migrate existing plaintext passwords to keychain (one-time) - setup._migrate_config_passwords(config_path) - # One-time upgrade notice for users coming from eufy-garmin-sync - setup._show_upgrade_notice() - - # Load config (passwords resolved from keychain or YAML fallback) - from eufy_sync.config import load_config - config = load_config(config_path) - except SystemExit: - raise - except Exception as e: - msg = f"eufy-sync could not start: {e}" - print(msg) - platform_support.notify("eufy-sync failed", str(e)[:200]) - sys.exit(1) - - has_garmin = any(u.garmin for u in config.users) - - if args.target and not any(getattr(u, args.target, None) for u in config.users): - print(f"Target '{args.target}' is not configured. Check your config.") - sys.exit(1) - - # Handle status - if args.status: + if args.status or args.history is not None: + # Read-only and unlocked, so it must not write the vault: no config + # password migration, and legacy keychain items are read in place + # rather than moved. A sync holding the lock may be saving the vault + # right now, and a second writer could undo its token refresh. + config = _load_config_or_exit(config_path, migrate=False) + _check_target(args, config) from eufy_sync.state import SyncState try: state = SyncState(db_path) except Exception as e: print(f"Could not read sync state: {e}") sys.exit(1) - status._show_status(state, config.users) + if args.status: + status._show_status(state, config.users) + else: + status._show_history(state, config.users, limit=args.history) state.close() return - # Handle history - if args.history is not None: - from eufy_sync.state import SyncState + if first_run: + # Setup stores passwords and can log in to Garmin and Zwift. The + # lock is released before the first sync takes it again below. try: - state = SyncState(db_path) + with _credential_lock("eufy-sync"): + setup._first_run_setup(config_path) + except SystemExit: + raise except Exception as e: - print(f"Could not read sync state: {e}") - sys.exit(1) - status._show_history(state, config.users, limit=args.history) - state.close() - return + _exit_could_not_start(e) # Run sync from eufy_sync.cli import lock @@ -356,6 +357,24 @@ def _main() -> None: print("Another eufy-sync run is in progress; skipping.") return + # The password migrations below write the credential vault, so they + # run only while this process holds the lock. + if not first_run: + try: + # Migrate existing plaintext passwords to keychain (one-time) + setup._migrate_config_passwords(config_path) + # One-time upgrade notice for users coming from eufy-garmin-sync + setup._show_upgrade_notice() + except SystemExit: + raise + except Exception as e: + _exit_could_not_start(e) + # Load config (passwords resolved from keychain or YAML fallback) + config = _load_config_or_exit(config_path) + + has_garmin = any(u.garmin for u in config.users) + _check_target(args, config) + updater._check_for_updates() backfill = args.backfill_days diff --git a/eufy_sync/config.py b/eufy_sync/config.py index 5c9b74a..6c504a2 100644 --- a/eufy_sync/config.py +++ b/eufy_sync/config.py @@ -72,12 +72,17 @@ def _walk_and_interpolate(obj: dict | list | str) -> dict | list | str: return obj -def _get_password(user_name: str, service: str, email: str, yaml_password: str | None) -> str: - """Resolve password: credential store first, then YAML fallback.""" +def _get_password( + user_name: str, service: str, email: str, yaml_password: str | None, migrate: bool = True +) -> str: + """Resolve password: credential store first, then YAML fallback. + + migrate=False reads a legacy keychain item without moving it into the + vault, for callers that run without the sync lock.""" from eufy_sync.credentials import get_password key = f"{user_name}:{service}" - stored = get_password(key) + stored = get_password(key, migrate=migrate) if stored: return stored @@ -90,7 +95,7 @@ def _get_password(user_name: str, service: str, email: str, yaml_password: str | ) -def _get_strava_secret(user_name: str, yaml_secret: str | None) -> str: +def _get_strava_secret(user_name: str, yaml_secret: str | None, migrate: bool = True) -> str: """Resolve the Strava API client secret: credential store first, then the YAML fallback for configs not yet migrated. @@ -100,7 +105,7 @@ def _get_strava_secret(user_name: str, yaml_secret: str | None) -> str: """ from eufy_sync.credentials import get_password - stored = get_password(f"{user_name}:strava") + stored = get_password(f"{user_name}:strava", migrate=migrate) if stored: return stored @@ -113,7 +118,12 @@ def _get_strava_secret(user_name: str, yaml_secret: str | None) -> str: ) -def load_config(path: Path) -> AppConfig: +def load_config(path: Path, migrate: bool = True) -> AppConfig: + """Parse the config and resolve its secrets. + + The default migrates legacy keychain items into the vault, which writes + the vault, so it belongs inside the sync lock. Unlocked read-only + commands (--status, --history) pass migrate=False.""" with open(path) as f: raw = yaml.safe_load(f) @@ -143,21 +153,21 @@ def load_config(path: Path) -> AppConfig: if "garmin" in u: garmin = GarminConfig( email=u["garmin"]["email"], - password=_get_password(name, "garmin", u["garmin"]["email"], u["garmin"].get("password")), + password=_get_password(name, "garmin", u["garmin"]["email"], u["garmin"].get("password"), migrate), ) strava = None if "strava" in u: strava = StravaConfig( client_id=str(u["strava"]["client_id"]), - client_secret=_get_strava_secret(name, u["strava"].get("client_secret")), + client_secret=_get_strava_secret(name, u["strava"].get("client_secret"), migrate), ) zwift = None if "zwift" in u: zwift = ZwiftConfig( email=u["zwift"]["email"], - password=_get_password(name, "zwift", u["zwift"]["email"], u["zwift"].get("password")), + password=_get_password(name, "zwift", u["zwift"]["email"], u["zwift"].get("password"), migrate), ) if not garmin and not strava and not zwift: @@ -170,7 +180,7 @@ def load_config(path: Path) -> AppConfig: name=name, eufy=EufyConfig( email=u["eufy"]["email"], - password=_get_password(name, "eufy", u["eufy"]["email"], u["eufy"].get("password")), + password=_get_password(name, "eufy", u["eufy"]["email"], u["eufy"].get("password"), migrate), customer_id=str(u["eufy"]["customer_id"]) if u["eufy"].get("customer_id") is not None else None, ), garmin=garmin, diff --git a/eufy_sync/credentials.py b/eufy_sync/credentials.py index 895de54..53e806a 100644 --- a/eufy_sync/credentials.py +++ b/eufy_sync/credentials.py @@ -445,9 +445,12 @@ def _save_vault(vault: dict) -> None: # --- Public API: passwords --------------------------------------------------- -def get_password(account: str) -> str | None: +def get_password(account: str, migrate: bool = True) -> str | None: """Return a stored password, migrating it from the legacy keychain item - (one account per password) into the vault on first access.""" + (one account per password) into the vault on first access. + + migrate=False still returns a legacy item but leaves it where it is, so a + caller that does not hold the sync lock never writes the vault.""" vault = _load_vault() if account in vault["passwords"]: return vault["passwords"][account] @@ -459,6 +462,8 @@ def get_password(account: str) -> str | None: except Exception: legacy = None if legacy is not None: + if not migrate: + return legacy vault["passwords"][account] = legacy _save_vault(vault) try: diff --git a/tests/test_lock.py b/tests/test_lock.py index c67c013..d1088e6 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -110,8 +110,10 @@ def boom(*a, **kw): out = capsys.readouterr().out assert "another eufy-sync run is in progress" in out.lower() mock_notify.assert_not_called() - # The skip happens before any other sync-path work. + # The skip happens before any other sync-path work, including the + # password migration, which writes the credential vault. mock_updates.assert_not_called() + _migrate.assert_not_called() @patch("eufy_sync.cli.status._print_summary") @@ -140,6 +142,87 @@ def test_sync_runs_and_releases_the_lock_for_the_next_run( assert acquired is True +class _DictKeyring: + def __init__(self): + self.data = {} + self.writes = [] + + def get_password(self, service, account): + return self.data.get((service, account)) + + def set_password(self, service, account, password): + self.writes.append(account) + self.data[(service, account)] = password + + def delete_password(self, service, account): + self.writes.append(account) + self.data.pop((service, account), None) + + +@pytest.mark.parametrize("flag", [["--status"], ["--history"]]) +def test_status_and_history_read_credentials_without_writing_the_vault(flag, tmp_path, monkeypatch): + """--status and --history run without the sync lock, so they must not + write the vault: a sync may be saving it at that moment. Plaintext YAML + passwords stay in the YAML and legacy per-password keychain items are + read where they are, both left for the next locked run to migrate.""" + from eufy_sync import credentials + from eufy_sync.cli.app import main + + store = _DictKeyring() + monkeypatch.setattr("keyring.get_password", store.get_password) + monkeypatch.setattr("keyring.set_password", store.set_password) + monkeypatch.setattr("keyring.delete_password", store.delete_password) + monkeypatch.setattr(credentials, "_keyring_available", lambda: True) + monkeypatch.setattr(credentials, "CRED_FILE", tmp_path / "credentials.json") + store.data[(credentials.SERVICE_NAME, "default:garmin")] = "legacy-pw" + + config_path = tmp_path / "config.yaml" + _write_config(config_path, { + "users": [{ + "name": "default", + "eufy": {"email": "e@example.com", "password": "yaml-pw"}, + "garmin": {"email": "g@example.com"}, + }], + }) + before = config_path.read_text() + argv = ["eufy-sync", "--config", str(config_path), "--db", str(tmp_path / "state.db"), *flag] + + with patch("sys.argv", argv), patch("eufy_sync.cli.setup._show_upgrade_notice"): + main() + + assert store.writes == [] + assert store.data[(credentials.SERVICE_NAME, "default:garmin")] == "legacy-pw" + assert config_path.read_text() == before + + +@patch("eufy_sync.cli.status._print_summary") +@patch("eufy_sync.platform_support.notify") +@patch("eufy_sync.cli.updater._check_for_updates") +@patch("eufy_sync.cli.setup._show_upgrade_notice") +@patch("eufy_sync.credentials._keyring_available", return_value=False) +def test_sync_runs_the_password_migration_while_holding_the_lock( + _keyring, _notice, _updates, _notify, _summary, tmp_path +): + """The migration writes the vault, so it runs inside the sync lock.""" + from eufy_sync.cli.app import main + + config_path = _write_synced_config(tmp_path) + argv = ["eufy-sync", "--config", str(config_path), "--db", str(tmp_path / "state.db"), "--headless"] + held = {} + + def migrate(path): + with lock.single_instance() as other: + held["by_us"] = other is False + + with patch("eufy_sync.cli.setup._migrate_config_passwords", side_effect=migrate), \ + patch("eufy_sync.sync.sync_user", return_value=({"garmin": 1}, {})), \ + patch("sys.argv", argv), \ + pytest.raises(SystemExit): + main() + + assert held == {"by_us": True} + + # --------------------------------------------------------------------------- # Commands that change credentials, tokens, or config wait for a running sync # --------------------------------------------------------------------------- From 0f0cdfbfd163f2e8dc5e1db31e10539f21c44f4f Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:04:02 -0400 Subject: [PATCH 12/17] Give each vault save its own chunk names and sweep every tracked leftover Chunk names now carry a random suffix recorded in the header, a save re-reads the header before committing and aborts with a retryable error if another process switched it, and a journal entry lists every tag that may still hold chunks so later saves and --use-file-store can delete leftovers from any number of interrupted saves. --- eufy_sync/credentials.py | 322 +++++++++++++++++++++++++++++--------- tests/test_credentials.py | 210 ++++++++++++++++++++++--- 2 files changed, 430 insertions(+), 102 deletions(-) diff --git a/eufy_sync/credentials.py b/eufy_sync/credentials.py index 53e806a..a78ea52 100644 --- a/eufy_sync/credentials.py +++ b/eufy_sync/credentials.py @@ -38,6 +38,9 @@ import json import logging import os +import re +import secrets +import time from pathlib import Path logger = logging.getLogger(__name__) @@ -47,16 +50,28 @@ # Windows Credential Manager caps one entry at ~2,560 bytes stored as UTF-16, # roughly 1,280 characters. A vault larger than CHUNK_LIMIT characters is split -# across "vault::" entries so set_password never fails on Windows; the -# "vault" entry then holds a small header naming the generation, the chunk -# count and a checksum. Each save writes a fresh generation and switches the -# header last, so an interrupted save never splices two vaults together. -# Released versions wrote chunks as "vault:"; those are still read and are -# replaced on the next save. MAX_CHUNKS bounds the chunk count a header may -# claim and how far a save probes for leftovers to delete, so a corrupt store -# can never make either run away. +# across "vault::" entries so set_password never fails on Windows; the +# "vault" entry then holds a small header naming the tag, the chunk count and a +# checksum. Each save writes its chunks under a fresh tag (the generation +# number plus a random suffix, so two processes saving at once never share +# chunk names), checks that the header is still the one it started from, and +# switches the header last. An interrupted or overtaken save never splices two +# vaults together. +# +# The keychain cannot list entries, so the "vault:journal" entry records every +# tag a save may have left chunks under. Each completed save deletes the +# chunks of the tag it replaced at once, and the chunks of any other journal +# tag once it is older than JOURNAL_GRACE seconds (a younger one may belong to +# a save still in progress in another process). Released versions wrote chunks +# as "vault:"; those are still read and are deleted by the next save. +# MAX_CHUNKS bounds the chunk count a header may claim and how far a sweep +# probes one tag, so a corrupt store can never make either run away. CHUNK_LIMIT = 1200 MAX_CHUNKS = 40 +JOURNAL_ACCOUNT = "vault:journal" +JOURNAL_GRACE = 600 +# Each entry is ~30 characters, so this stays well under CHUNK_LIMIT. +MAX_JOURNAL = 24 CRED_FILE = Path.home() / ".garmin-sync" / "credentials.json" @@ -177,12 +192,22 @@ def _valid_count(value) -> bool: return type(value) is int and 1 <= value <= MAX_CHUNKS +# "" (written by pre-release builds of the generation layout) or +# ".<8 hex>". Anything else is never used to build an account name, so a +# damaged header or journal cannot point a sweep at unrelated entries. +_TAG_RE = re.compile(r"[0-9]{1,9}(\.[0-9a-f]{8})?") + + +def _valid_tag(value) -> bool: + return isinstance(value, str) and _TAG_RE.fullmatch(value) is not None + + def _parse_header(raw: str) -> tuple: """Classify the "vault" entry. Returns one of: - ("single", vault) the whole vault in one entry - ("legacy", count) released-version chunks "vault:1".."vault:" - ("gen", gen, count, sha256) chunks "vault::1".."vault::" + ("single", vault) the whole vault in one entry + ("legacy", count) released-version chunks "vault:1".."vault:" + ("gen", tag, count, sha256, gen) chunks "vault::1".."vault::" Raises VaultCorruptError for anything else.""" try: @@ -200,7 +225,11 @@ def _parse_header(raw: str) -> tuple: and _valid_count(meta.get("chunks")) and isinstance(meta.get("sha256"), str) ): - return ("gen", meta["gen"], meta["chunks"], meta["sha256"]) + # Headers written before chunk names carried a random suffix + # have no "tag"; their chunks are named by the generation alone. + tag = meta.get("tag", str(meta["gen"])) + if _valid_tag(tag): + return ("gen", tag, meta["chunks"], meta["sha256"], meta["gen"]) raise _keychain_corrupt("the chunk header is malformed") if "__chunks__" in parsed: if _valid_count(parsed["__chunks__"]): @@ -209,11 +238,11 @@ def _parse_header(raw: str) -> tuple: return ("single", parsed) -def _chunk_account(gen: int | None, i: int) -> str: - # gen None is the layout released versions wrote ("vault:1", "vault:2"). - if gen is None: +def _chunk_account(tag: str | None, i: int) -> str: + # tag None is the layout released versions wrote ("vault:1", "vault:2"). + if tag is None: return f"{VAULT_ACCOUNT}:{i}" - return f"{VAULT_ACCOUNT}:{gen}:{i}" + return f"{VAULT_ACCOUNT}:{tag}:{i}" def _assemble(layout: tuple) -> dict: @@ -221,15 +250,15 @@ def _assemble(layout: tuple) -> dict: if layout[0] == "single": return _normalize_vault(layout[1]) if layout[0] == "legacy": - gen, count, digest = None, layout[1], None + tag, count, digest = None, layout[1], None else: - _, gen, count, digest = layout + _, tag, count, digest, _ = layout pieces = [] for i in range(1, count + 1): # A keyring failure here (locked, access denied) raises the # unreadable-keychain RuntimeError, same as the header read. A chunk # that comes back None is genuinely missing: the vault is damaged. - piece = _keychain_get(_chunk_account(gen, i)) + piece = _keychain_get(_chunk_account(tag, i)) if piece is None: raise _keychain_corrupt(f"chunk {i} of {count} is missing") pieces.append(piece) @@ -261,8 +290,8 @@ def _load_vault_from_keychain() -> dict: return _assemble(_parse_header(fresh)) -def _delete_chunk_run(gen: int | None, start: int) -> None: - # Delete one generation's chunk entries from `start` up to the first gap. +def _delete_chunk_run(tag: str | None, start: int = 1) -> None: + # Delete one tag's chunk entries from `start` up to the first gap. # Chunks are always written 1..N in order, so leftovers from an # interrupted save form an unbroken run. Deleting from the top down keeps # it unbroken if this sweep is itself cut short, so the next sweep still @@ -270,7 +299,7 @@ def _delete_chunk_run(gen: int | None, start: int) -> None: import keyring run = [] for i in range(start, MAX_CHUNKS + 1): - account = _chunk_account(gen, i) + account = _chunk_account(tag, i) if keyring.get_password(SERVICE_NAME, account) is None: break run.append(account) @@ -281,99 +310,240 @@ def _delete_chunk_run(gen: int | None, start: int) -> None: pass -def _sweep_chunks(prev_gen: int, keep_gen: int | None = None, keep_count: int = 0) -> None: +def _read_header() -> tuple[str | None, tuple | None]: + """The raw "vault" entry and its parsed layout. The layout is None when + the entry is absent or this module cannot parse it.""" + raw = _keychain_get(VAULT_ACCOUNT) + if raw is None: + return None, None + try: + return raw, _parse_header(raw) + except VaultCorruptError: + return raw, None + + +def _layout_tag(layout: tuple | None) -> str | None: + """The chunk tag a parsed header points at, if any. A single-entry vault + written by a pre-release build may carry "__gen__", the generation whose + chunks it had just replaced; those may still need sweeping.""" + if layout is None: + return None + if layout[0] == "gen": + return layout[1] + if layout[0] == "single": + gen = layout[1].get("__gen__") + if type(gen) is int and gen >= 1: + return str(gen) + return None + + +def _layout_gen(layout: tuple | None) -> int: + if layout is not None and layout[0] == "gen": + return layout[4] + return 0 + + +def _read_journal() -> dict[str, float]: + """Tags that may still have chunk entries, with when each was recorded. + Best-effort: an unreadable or malformed journal reads as empty.""" + import keyring + try: + raw = keyring.get_password(SERVICE_NAME, JOURNAL_ACCOUNT) + parsed = json.loads(raw) if raw else {} + except Exception: + return {} + if not isinstance(parsed, dict): + return {} + return { + tag: float(t) for tag, t in parsed.items() + if _valid_tag(tag) and isinstance(t, (int, float)) and not isinstance(t, bool) + } + + +def _write_journal(journal: dict[str, float]) -> None: + import keyring + if not journal: + try: + keyring.delete_password(SERVICE_NAME, JOURNAL_ACCOUNT) + except Exception: + pass + return + newest = sorted(journal.items(), key=lambda item: item[1], reverse=True)[:MAX_JOURNAL] + keyring.set_password(SERVICE_NAME, JOURNAL_ACCOUNT, json.dumps(dict(newest))) + + +def _journal_add(entries: dict[str, float]) -> None: + journal = _read_journal() + added = {tag: t for tag, t in entries.items() if tag not in journal} + if added: + journal.update(added) + _write_journal(journal) + + +def _journal_remove(tags: set[str]) -> None: + # Re-read right before writing so entries another process added since + # this one last looked are kept. + journal = _read_journal() + if tags & journal.keys(): + _write_journal({tag: t for tag, t in journal.items() if tag not in tags}) + + +def _sweep_chunks(prev_tag: str | None, force: bool = False) -> None: """Best-effort removal of chunk entries that no header points at. - prev_gen is the generation the header named before this change (0 when - it named none). Its neighbours are swept too: prev_gen - 1 catches a - cleanup a crash cut short, and prev_gen + 1 catches chunks a crashed save - wrote before it could switch the header. keep_gen/keep_count protect the - chunks the header now references. The released-version "vault:i" names - are probed until a save after generation 1 (the first one written after - them) has swept them.""" + Runs after a save has switched the header. Deletes the released-version + "vault:i" chunks, the chunks of prev_tag (the tag the header named before + this save), and the chunks of every journal tag that is older than + JOURNAL_GRACE, or every journal tag at all when force is set. The tag the + header names right now is always kept, even if another process switched + it after this save did.""" try: - if prev_gen <= 1: - _delete_chunk_run(None, 1) - for gen in (prev_gen - 1, prev_gen, prev_gen + 1): - if gen < 1: + _, layout = _read_header() + if layout is None and _keychain_get(VAULT_ACCOUNT) is not None: + # A header this module cannot parse might still point somewhere; + # deleting nothing is the safe answer. + return + current = layout[1] if layout is not None and layout[0] == "gen" else None + if layout is None or layout[0] != "legacy": + _delete_chunk_run(None) + if prev_tag is not None and prev_tag != current: + _delete_chunk_run(prev_tag) + now = time.time() + swept = set() + for tag, recorded in _read_journal().items(): + if tag == current: continue - _delete_chunk_run(gen, keep_count + 1 if gen == keep_gen else 1) + if force or tag == prev_tag or now - recorded >= JOURNAL_GRACE: + if tag != prev_tag: + _delete_chunk_run(tag) + swept.add(tag) + if prev_tag is not None: + swept.add(prev_tag) + _journal_remove(swept) + if set(_read_journal()) <= {current}: + # Only the live tag left: nothing to track. + _write_journal({}) except Exception: # The new vault is already in place; leftovers cost tidiness, not # data, so a failed cleanup must not fail the save. pass -def _current_gen() -> int: - """The generation the stored header names, or 0 for none. A header this - module cannot parse counts as 0: saves only ever follow a successful - load, so the next sweep still covers whatever it can identify.""" - raw = _keychain_get(VAULT_ACCOUNT) - if raw is None: - return 0 - try: - layout = _parse_header(raw) - except VaultCorruptError: - return 0 - if layout[0] == "gen": - return layout[1] - if layout[0] == "single": - gen = layout[1].get("__gen__") - return gen if type(gen) is int and gen >= 1 else 0 - return 0 +class VaultWriteConflict(RuntimeError): + """Another process saved the keychain vault while this save was running. + + This save was abandoned before it switched the header, so the vault holds + the other process's complete write. Retrying the command reads that write + and applies this change on top of it.""" + + +_WRITE_CONFLICT = ( + "Another eufy-sync process changed the stored credentials while this one " + "was saving, so this change was not saved. The stored credentials are " + "intact. Run the command again." +) def _save_vault_to_keychain(vault: dict) -> None: import keyring - prev_gen = _current_gen() + start_raw, start_layout = _read_header() + prev_tag = _layout_tag(start_layout) + pending = {} + if prev_tag is not None: + # Recorded before the switch, so a sweep a crash cuts short is + # finished by a later save. + pending[prev_tag] = 0.0 + if "." not in prev_tag: + # Pre-release generation layout: a crashed save there left its + # chunks at the neighbouring generation numbers. + gen = int(prev_tag) + for neighbour in (gen - 1, gen + 1): + if neighbour >= 1: + pending[str(neighbour)] = 0.0 # json.dumps escapes non-ASCII by default, so each character is one # UTF-16 unit and a CHUNK_LIMIT-character entry stays under the Windows cap. payload = json.dumps(vault) - # A vault that shrinks back into one entry keeps the last chunk generation - # as "__gen__" (dropped on load), so a sweep that a crash cuts short can - # still find that generation's leftovers on the next save. - single = json.dumps({**vault, "__gen__": prev_gen}) if prev_gen else payload - if len(single) <= CHUNK_LIMIT: - keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, single) - _sweep_chunks(prev_gen) + + if len(payload) <= CHUNK_LIMIT: + if pending: + _journal_add(pending) + if _keychain_get(VAULT_ACCOUNT) != start_raw: + raise VaultWriteConflict(_WRITE_CONFLICT) + keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, payload) + _sweep_chunks(prev_tag) return - # The new chunks go under a generation no header references yet, and the - # single-entry header write is the commit point. A save killed before it - # leaves the old vault readable; one killed after it leaves the new vault - # readable plus leftovers that the next save sweeps. - gen = prev_gen + 1 + + # The new chunks go under a tag no header references and no other writer + # can pick, and the header write is the commit point. A save killed + # before it leaves the old vault readable; one killed after it leaves the + # new vault readable. Either way the journal names the leftovers. + tag = f"{_layout_gen(start_layout) + 1}.{secrets.token_hex(4)}" + pending[tag] = time.time() + _journal_add(pending) chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] - for i, chunk in enumerate(chunks, start=1): - keyring.set_password(SERVICE_NAME, _chunk_account(gen, i), chunk) + try: + for i, chunk in enumerate(chunks, start=1): + keyring.set_password(SERVICE_NAME, _chunk_account(tag, i), chunk) + # Commit only on top of the header this save's vault was read under. + # Another writer that switched it in the meantime holds the newer + # vault; overwriting its header would also orphan its chunks. + if _keychain_get(VAULT_ACCOUNT) != start_raw: + raise VaultWriteConflict(_WRITE_CONFLICT) + except Exception: + try: + _delete_chunk_run(tag) + _journal_remove({tag}) + except Exception: + pass + raise header = { "__vault__": { - "gen": gen, + "gen": _layout_gen(start_layout) + 1, + "tag": tag, "chunks": len(chunks), "sha256": hashlib.sha256(payload.encode()).hexdigest(), } } keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps(header)) - _sweep_chunks(prev_gen, keep_gen=gen, keep_count=len(chunks)) + _sweep_chunks(prev_tag) def _delete_keychain_vault() -> None: - """Best-effort removal of the vault header and every chunk entry.""" + """Best-effort removal of the vault header, every chunk entry this + module can find, and the journal.""" import keyring try: - prev_gen = _current_gen() + _, layout = _read_header() except Exception: - prev_gen = 0 + layout = None try: keyring.delete_password(SERVICE_NAME, VAULT_ACCOUNT) except Exception: pass - _sweep_chunks(prev_gen) - if prev_gen: - # Released-version chunks an earlier install left behind. + tags = set() + tag = _layout_tag(layout) + if tag is not None: + tags.add(tag) + if "." not in tag: + tags.update(str(g) for g in (int(tag) - 1, int(tag) + 1) if g >= 1) + try: + tags.update(_read_journal()) + except Exception: + pass + for tag in tags: try: - _delete_chunk_run(None, 1) + _delete_chunk_run(tag) except Exception: pass + try: + # Released-version chunks an earlier install left behind. + _delete_chunk_run(None) + except Exception: + pass + try: + keyring.delete_password(SERVICE_NAME, JOURNAL_ACCOUNT) + except Exception: + pass def _load_vault_from_file() -> dict: diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 419bcae..3dd8259 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -1,12 +1,15 @@ from __future__ import annotations +import hashlib import json import os import stat +import time from unittest.mock import patch import pytest +from eufy_sync import credentials from eufy_sync.credentials import ( CHUNK_LIMIT, SERVICE_NAME, @@ -805,8 +808,8 @@ def _header(store: _FakeKeyringStore) -> dict: return json.loads(store.get_password(SERVICE_NAME, VAULT_ACCOUNT))["__vault__"] -def _chunk_accounts(gen: int, count: int) -> set[str]: - return {f"{VAULT_ACCOUNT}:{gen}:{i}" for i in range(1, count + 1)} +def _chunk_accounts(tag: str, count: int) -> set[str]: + return {f"{VAULT_ACCOUNT}:{tag}:{i}" for i in range(1, count + 1)} def test_oversized_vault_chunks_and_round_trips(fake_keyring): @@ -814,14 +817,14 @@ def test_oversized_vault_chunks_and_round_trips(fake_keyring): header = _header(fake_keyring) n = header["chunks"] assert n >= 3 - for account in _chunk_accounts(header["gen"], n): + for account in _chunk_accounts(header["tag"], n): chunk = fake_keyring.get_password(SERVICE_NAME, account) assert chunk is not None assert len(chunk) <= CHUNK_LIMIT # Windows caps one entry at 2,560 bytes of UTF-16. assert len(chunk.encode("utf-16-le")) <= 2560 assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) - assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], n) + assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], n) def test_non_ascii_vault_respects_the_windows_entry_cap(fake_keyring): @@ -829,7 +832,7 @@ def test_non_ascii_vault_respects_the_windows_entry_cap(fake_keyring): still splits into entries under the per-entry byte cap.""" store_token("garmin", {"access_token": "é\U0001f600" * CHUNK_LIMIT}) header = _header(fake_keyring) - for account in _chunk_accounts(header["gen"], header["chunks"]): + for account in _chunk_accounts(header["tag"], header["chunks"]): assert len(fake_keyring.get_password(SERVICE_NAME, account).encode("utf-16-le")) <= 2560 assert get_token("garmin") == {"access_token": "é\U0001f600" * CHUNK_LIMIT} @@ -842,7 +845,7 @@ def test_each_save_uses_a_new_generation_and_deletes_the_old_one(fake_keyring): assert second["gen"] == first["gen"] + 1 assert _vault_accounts(fake_keyring) == ( - {VAULT_ACCOUNT} | _chunk_accounts(second["gen"], second["chunks"]) + {VAULT_ACCOUNT} | _chunk_accounts(second["tag"], second["chunks"]) ) assert get_token("garmin") == _big_token(4 * CHUNK_LIMIT) @@ -865,8 +868,8 @@ def test_missing_chunk_raises_and_is_never_overwritten(fake_keyring, cred_file): store_password("default:eufy", "pw") store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - gen = _header(fake_keyring)["gen"] - fake_keyring.delete_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{gen}:2") + tag = _header(fake_keyring)["tag"] + fake_keyring.delete_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{tag}:2") before = dict(fake_keyring.data) with pytest.raises(VaultCorruptError, match="chunk 2 of"): @@ -881,8 +884,8 @@ def test_chunks_that_do_not_match_the_header_raise(fake_keyring): from eufy_sync.credentials import VaultCorruptError store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - gen = _header(fake_keyring)["gen"] - account = f"{VAULT_ACCOUNT}:{gen}:1" + tag = _header(fake_keyring)["tag"] + account = f"{VAULT_ACCOUNT}:{tag}:1" chunk = fake_keyring.get_password(SERVICE_NAME, account) fake_keyring.set_password(SERVICE_NAME, account, chunk.replace("x", "y")) @@ -1017,15 +1020,20 @@ def setup(): assert vault["passwords"] == _OLD_PW, f"killed after {steps} of {total}" assert vault["tokens"]["garmin"] in (old, new), f"killed after {steps} of {total}" - # The next completed save leaves only the entries its header uses. - store_token("strava", {"access_token": "t"}) + # The next completed save after the journal grace period leaves only + # the entries its header uses. (Inside the grace period the killed + # save's chunks are indistinguishable from a save still running in + # another process, so they are kept until then.) + later = time.time() + credentials.JOURNAL_GRACE + 1 + with patch("eufy_sync.credentials.time.time", return_value=later): + store_token("strava", {"access_token": "t"}) vault = _load_vault() assert vault["passwords"] == _OLD_PW assert vault["tokens"]["strava"] == {"access_token": "t"} raw = json.loads(fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT)) expected = {VAULT_ACCOUNT} if "__vault__" in raw: - expected |= _chunk_accounts(raw["__vault__"]["gen"], raw["__vault__"]["chunks"]) + expected |= _chunk_accounts(raw["__vault__"]["tag"], raw["__vault__"]["chunks"]) assert _vault_accounts(fake_keyring) == expected, f"killed after {steps} of {total}" @@ -1036,12 +1044,12 @@ def test_reader_retries_once_when_a_save_switches_the_header_mid_read(fake_keyri from eufy_sync import credentials store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - old_gen = _header(fake_keyring)["gen"] + old_tag = _header(fake_keyring)["tag"] real_get = fake_keyring.get_password state = {"saved": False} def racing_get(service, account): - if account == f"{VAULT_ACCOUNT}:{old_gen}:2" and not state["saved"]: + if account == f"{VAULT_ACCOUNT}:{old_tag}:2" and not state["saved"]: state["saved"] = True monkeypatch.setattr("keyring.get_password", real_get) credentials._save_vault_to_keychain( @@ -1085,7 +1093,7 @@ def test_legacy_chunked_vault_reads_and_migrates_on_next_save(fake_keyring, cred header = _header(fake_keyring) assert _vault_accounts(fake_keyring) == ( - {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], header["chunks"]) + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) ) assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) assert get_token("strava") == {"access_token": "t"} @@ -1115,27 +1123,177 @@ def test_legacy_single_entry_vault_reads_and_stays_single(fake_keyring, cred_fil assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} -def test_stale_generations_left_by_crashes_are_cleaned_up(fake_keyring): - """Leftovers from a cleanup a crash cut short (the generation before the - current one) and from a save killed before its header switch (the - generation after it) are both deleted by the next save.""" - store_token("garmin", _big_token(3 * CHUNK_LIMIT)) - store_token("garmin", _big_token(3 * CHUNK_LIMIT + 1)) - gen = _header(fake_keyring)["gen"] - for stale in (gen - 1, gen + 1): +def test_prerelease_generation_leftovers_are_cleaned_up(fake_keyring): + """Pre-release builds named chunks by generation alone ("vault::") + and left crash leftovers at the neighbouring generations. A vault still + on that layout reads, and the first save deletes its chunks and both + neighbours.""" + vault = {"passwords": {"default:eufy": "pw"}, "tokens": {"garmin": _big_token(3 * CHUNK_LIMIT)}} + payload = json.dumps(vault) + chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] + for i, chunk in enumerate(chunks, start=1): + fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:5:{i}", chunk) + fake_keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps({"__vault__": { + "gen": 5, "chunks": len(chunks), + "sha256": hashlib.sha256(payload.encode()).hexdigest(), + }})) + for stale in (4, 6): for i in (1, 2, 3): fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{stale}:{i}", "junk") - assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT + 1) + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) store_token("garmin", _big_token(2 * CHUNK_LIMIT)) header = _header(fake_keyring) + assert header["gen"] == 6 and header["tag"].startswith("6.") assert _vault_accounts(fake_keyring) == ( - {VAULT_ACCOUNT} | _chunk_accounts(header["gen"], header["chunks"]) + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) ) assert get_token("garmin") == _big_token(2 * CHUNK_LIMIT) +def _kill_on_header_write(monkeypatch, store: _FakeKeyringStore) -> None: + """Let a save write its chunks, then kill it as it switches the header.""" + def set_password(service, account, password): + if account == VAULT_ACCOUNT: + raise _Killed() + store.set_password(service, account, password) + + monkeypatch.setattr("keyring.set_password", set_password) + + +def test_consecutive_interrupted_saves_leave_no_orphaned_chunks(fake_keyring, monkeypatch): + """Each killed save leaves a full set of chunks (plaintext slices of the + vault) under a tag no header names. However many pile up, the first + completed save after the grace period deletes all of them, along with + released-version "vault:i" chunks.""" + credentials.store_password("default:eufy", "pw") + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + for i in (1, 2): + fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{i}", "legacy-junk") + + for n in range(4): + _kill_on_header_write(monkeypatch, fake_keyring) + with pytest.raises(_Killed): + store_token("garmin", _big_token((3 + n) * CHUNK_LIMIT)) + _restore(monkeypatch, fake_keyring) + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + orphans = _vault_accounts(fake_keyring) - {VAULT_ACCOUNT, credentials.JOURNAL_ACCOUNT} + assert len(orphans) > 4 * 3 + + later = time.time() + credentials.JOURNAL_GRACE + 1 + with patch("eufy_sync.credentials.time.time", return_value=later): + store_token("strava", {"access_token": "t"}) + + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) + ) + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + + +def test_fresh_journal_tags_survive_a_save_inside_the_grace_period(fake_keyring, monkeypatch): + """A tag recorded moments ago may be a save still running in another + process. A sweep inside the grace period must not delete its chunks.""" + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + _kill_on_header_write(monkeypatch, fake_keyring) + with pytest.raises(_Killed): + store_token("garmin", _big_token(4 * CHUNK_LIMIT)) + _restore(monkeypatch, fake_keyring) + pending = set(json.loads(fake_keyring.get_password(SERVICE_NAME, credentials.JOURNAL_ACCOUNT))) + live = _header(fake_keyring)["tag"] + other = (pending - {live}).pop() + + store_token("strava", {"access_token": "t"}) + + assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{other}:1") is not None + + +def test_switching_to_the_file_store_deletes_every_known_chunk(fake_keyring, cred_file, monkeypatch): + """--use-file-store must not leave plaintext slices of the vault in the + keychain the user just opted out of: the live chunks, journal-tracked + leftovers of any age, and released-version "vault:i" chunks all go.""" + from eufy_sync.credentials import _load_vault, use_file_store + + credentials.store_password("default:eufy", "pw") + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:1", "legacy-junk") + for n in range(2): + _kill_on_header_write(monkeypatch, fake_keyring) + with pytest.raises(_Killed): + store_token("garmin", _big_token((4 + n) * CHUNK_LIMIT)) + _restore(monkeypatch, fake_keyring) + + use_file_store() + + assert _vault_accounts(fake_keyring) == set() + assert _load_vault()["tokens"]["garmin"] == _big_token(3 * CHUNK_LIMIT) + + +def test_save_aborts_when_another_writer_switches_the_header_first(fake_keyring): + """Two writers start from the same header. The second to finish must not + commit over the first: it raises a retryable error, deletes its own + chunks, and leaves the first writer's vault readable.""" + from eufy_sync.credentials import VaultWriteConflict, _save_vault_to_keychain + + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + real_set = fake_keyring.set_password + state = {"raced": False} + + def racing_set(service, account, password): + real_set(service, account, password) + if account.endswith(":1") and account != f"{VAULT_ACCOUNT}:1" and not state["raced"]: + state["raced"] = True + # The other writer runs to completion between our chunk writes. + _save_vault_to_keychain( + {"passwords": {}, "tokens": {"garmin": _big_token(4 * CHUNK_LIMIT)}} + ) + + with patch("keyring.set_password", racing_set): + with pytest.raises(VaultWriteConflict, match="Run the command again"): + _save_vault_to_keychain( + {"passwords": {}, "tokens": {"garmin": _big_token(5 * CHUNK_LIMIT)}} + ) + + assert get_token("garmin") == _big_token(4 * CHUNK_LIMIT) + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) - {credentials.JOURNAL_ACCOUNT} == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) + ) + + +def test_writer_overtaken_at_its_header_write_leaves_a_readable_vault(fake_keyring): + """The review's race: writer B passes its header check, then writer A + saves completely before B's header write lands. Both used distinct chunk + names, and A's sweep keeps B's fresh chunks, so the vault B commits is + whole. (A's update is lost; the vault is never damaged.)""" + from eufy_sync.credentials import _load_vault_from_keychain, _save_vault_to_keychain + + def big(tag, n): + return {"passwords": {"u:eufy": "pw"}, "tokens": {"garmin": {"blob": tag * n}}} + + _save_vault_to_keychain(big("o", 3000)) + _save_vault_to_keychain(big("o", 3000)) + writer_b = _load_vault_from_keychain() + writer_b["passwords"]["u:garmin"] = "x" + + real_set = fake_keyring.set_password + hook = {"fn": lambda: _save_vault_to_keychain(big("a", 2000))} + + def racing_set(service, account, password): + if account == VAULT_ACCOUNT and hook["fn"]: + fn, hook["fn"] = hook["fn"], None + fn() + real_set(service, account, password) + + with patch("keyring.set_password", racing_set): + _save_vault_to_keychain(writer_b) + + after = _load_vault_from_keychain() + assert after["tokens"]["garmin"]["blob"][:1] in ("o", "a") + assert after["passwords"]["u:eufy"] == "pw" + + def test_shared_service_name_and_legacy_token_item_are_untouched(fake_keyring, cred_file): """Another tool reads the legacy "token:garmin" item under the same service name. Vault saves, chunked or not, must never touch it.""" From c84322c16314d3d1bf20ce5adb8ade3d4ec0c668 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:04:49 -0400 Subject: [PATCH 13/17] Allow one Garmin relogin per run even when it succeeds A later 401 or 403 in the same run now raises the stored relogin error or the call's own error instead of running another full login, so a Cloudflare block on the API no longer costs a login per call. --- eufy_sync/garmin_client.py | 27 ++++++++++++++++++--------- tests/test_garmin_client.py | 27 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/eufy_sync/garmin_client.py b/eufy_sync/garmin_client.py index bee63c1..160d653 100644 --- a/eufy_sync/garmin_client.py +++ b/eufy_sync/garmin_client.py @@ -90,10 +90,14 @@ def __init__(self, config: GarminConfig): self._auth = GarminAuth(config.email, config.password) self._garmin = None self._allow_interactive = True - # The error from a relogin that failed this run, if any. Later calls - # raise it again instead of trying another login: a second attempt - # minutes later meets the same MFA demand or wrong password, and every - # extra login raises the odds of a Garmin 429. + # At most one relogin per run, whether or not it worked. After a + # failed one, later calls raise its error again: a second attempt + # minutes later meets the same MFA demand or wrong password. After a + # successful one, a later 401/403 means the fresh session did not help + # (a Cloudflare block on the API while SSO still lets logins through), + # so the call's own error is raised. Every extra login raises the odds + # of a Garmin 429. + self._reauth_attempted = False self._reauth_error: Exception | None = None def authenticate(self, allow_interactive: bool = True) -> None: @@ -111,6 +115,7 @@ def _reauth(self) -> None: Only one relogin is tried per run. After a failure, every later call that needs one gets the same error back without contacting Garmin.""" + self._reauth_attempted = True try: if not self._allow_interactive: logger.info("Garmin session expired; re-authenticating without prompts") @@ -134,11 +139,15 @@ def _call_with_reauth(self, call): except (GarminConnectAuthenticationError, GarminConnectConnectionError) as e: if not _is_garmin_auth_failure(e): raise - if self._reauth_error is not None: - # This run already tried to log in again and failed. Report - # that failure, which names the fix, rather than this call's - # 401 or 403. - raise self._reauth_error from e + if self._reauth_attempted: + if self._reauth_error is not None: + # This run already tried to log in again and failed. + # Report that failure, which names the fix, rather than + # this call's 401 or 403. + raise self._reauth_error from e + # The relogin worked and Garmin still refuses: another login + # would not change that. + raise self._reauth() return call() diff --git a/tests/test_garmin_client.py b/tests/test_garmin_client.py index 46b1e09..8804b22 100644 --- a/tests/test_garmin_client.py +++ b/tests/test_garmin_client.py @@ -298,6 +298,33 @@ def test_duplicate_check_fails_open_when_the_relogin_fails(): assert client.has_weight_on_date(datetime(2026, 6, 10, tzinfo=timezone.utc)) is False +def test_a_successful_relogin_is_not_repeated_when_the_api_keeps_refusing(): + # Cloudflare 403s on the API while SSO logins succeed: the first call + # relogs in, and every later call must fail with its own error instead + # of running a full login each time. + from garminconnect import GarminConnectConnectionError + + blocked = GarminConnectConnectionError("API Error 403 - ") + stale = MagicMock() + stale.get_body_composition.side_effect = blocked + fresh = MagicMock() + fresh.get_body_composition.side_effect = blocked + fresh.get_daily_weigh_ins.side_effect = blocked + fresh.add_body_composition.side_effect = blocked + client = _client_with_fake_garmin(stale) + client._allow_interactive = False + bc = GarminBodyComposition(timestamp="2026-06-10T08:00:00+00:00", weight=86.2) + + with patch.object(client._auth, "silent_reauth", return_value=fresh) as reauth: + assert client.has_weight_on_date(datetime(2026, 6, 10, tzinfo=timezone.utc)) is False + with pytest.raises(GarminConnectConnectionError, match="403"): + client.check_connection() + with pytest.raises(GarminConnectConnectionError, match="403"): + client.upload_body_composition(bc) + + reauth.assert_called_once() + + # --------------------------------------------------------------------------- # close() persists whatever the library rotated during the run # --------------------------------------------------------------------------- From 0a442744bbfa0c03f60c0a2a9c616ab47b8e4fce Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:24:09 -0400 Subject: [PATCH 14/17] Serialize credential writes with a vault lock and close the review gaps Every credential change (store, delete, migration, store switches, uninstall cleanup) now holds an exclusive OS lock on vault.lock in the data dir from load to sweep. The lock is reentrant within a process, waits up to 30 s, and refuses to write if it cannot be opened or acquired. With writers serialized, the journal grace period and the header re-check are gone: any journal tag the live header does not name is a leftover and is deleted by the next save. - Uninstall deletes each lock file while still holding it on POSIX, and a lock taken on a file that was unlinked meanwhile is retried on the new one. Windows keeps release-then-delete. - A present but wrong-typed passwords/tokens section raises VaultCorruptError instead of reading as empty. - Keychain saves refuse more than MAX_CHUNKS chunks before writing anything, so use_keychain_store keeps the file. Released-layout vaults of any size still read. - An existing credentials file that cannot be read or parsed raises during backend selection instead of silently falling back to the keychain; doctor reports it as a failure. --- eufy_sync/cli/app.py | 7 +- eufy_sync/cli/doctor.py | 4 +- eufy_sync/cli/lock.py | 80 ++---- eufy_sync/cli/maintenance.py | 121 +++++---- eufy_sync/credentials.py | 462 +++++++++++++++++++++-------------- eufy_sync/file_lock.py | 104 ++++++++ tests/test_credentials.py | 393 +++++++++++++++++++++++------ tests/test_lock.py | 104 +++++++- 8 files changed, 916 insertions(+), 359 deletions(-) create mode 100644 eufy_sync/file_lock.py diff --git a/eufy_sync/cli/app.py b/eufy_sync/cli/app.py index cb2fdb2..61f8762 100644 --- a/eufy_sync/cli/app.py +++ b/eufy_sync/cli/app.py @@ -207,11 +207,14 @@ def _main() -> None: # Handle full uninstall if args.uninstall: + from eufy_sync.cli import lock with _credential_lock("--uninstall"): removed = maintenance._uninstall(shared.DATA_DIR, config_path=config_path, db_path=db_path) + if removed: + # POSIX: delete the sync lock file before releasing it. + lock.unlink_while_held() if removed: - # The lock file outlives the sweep because it was held open. - maintenance._remove_lock_file(shared.DATA_DIR) + maintenance._remove_lock_files(shared.DATA_DIR) return # Handle credential store mode switches diff --git a/eufy_sync/cli/doctor.py b/eufy_sync/cli/doctor.py index 2a53f60..3bbffcc 100644 --- a/eufy_sync/cli/doctor.py +++ b/eufy_sync/cli/doctor.py @@ -153,7 +153,9 @@ def _check_keychain(report) -> None: return report("PASS", "keychain", active_store_label()) except Exception as e: - report("PASS", "keychain", f"file store (no keychain prompts) ({e})") + # A damaged or unreadable credentials file now raises here instead + # of quietly falling back to the keychain. + report("FAIL", "keychain", str(e)) def _check_eufy_token(report, user): diff --git a/eufy_sync/cli/lock.py b/eufy_sync/cli/lock.py index 1ab5f0b..f00b8b0 100644 --- a/eufy_sync/cli/lock.py +++ b/eufy_sync/cli/lock.py @@ -11,7 +11,9 @@ The lock is a courtesy, not a guarantee. If the file cannot be created the run proceeds unlocked rather than failing: an overlap is a rare annoyance, while a sync that refuses to start is a real one (same trade-off failure_notify makes -with its counter file). +with its counter file). The credential vault does not depend on this: every +vault write separately takes the vault lock in eufy_sync.credentials, which +refuses to write rather than run unlocked. There is no PID or staleness handling on purpose. The OS drops the lock when the handle closes or the process dies, so a killed run leaves nothing behind. @@ -19,62 +21,21 @@ from __future__ import annotations import contextlib -import os import sys from pathlib import Path from typing import Iterator +from eufy_sync import file_lock from eufy_sync.cli import shared LOCK_NAME = "sync.lock" -if sys.platform == "win32": - import msvcrt -else: - import fcntl - def lock_path() -> Path: # Read at call time: the data dir is redirected in tests. return shared.DATA_DIR / LOCK_NAME -def _open() -> int | None: - """Open (creating if needed) the lock file. None if it cannot be made.""" - try: - shared.DATA_DIR.mkdir(parents=True, exist_ok=True, mode=0o700) - return os.open(str(lock_path()), os.O_RDWR | os.O_CREAT, 0o600) - except OSError: - return None - - -def _try_acquire(fd: int) -> bool: - """Take the exclusive lock without blocking. False when someone holds it.""" - try: - if sys.platform == "win32": - # msvcrt locks a byte range from the current position; one byte - # past EOF is fine and keeps the file empty. - os.lseek(fd, 0, os.SEEK_SET) - msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) - else: - fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - except OSError: - return False - return True - - -def _release(fd: int) -> None: - try: - if sys.platform == "win32": - os.lseek(fd, 0, os.SEEK_SET) - msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) - else: - fcntl.flock(fd, fcntl.LOCK_UN) - except OSError: - # Closing the handle below releases it anyway. - pass - - @contextlib.contextmanager def single_instance(require_lock: bool = False) -> Iterator[bool]: """Hold the sync lock for the block. @@ -84,17 +45,30 @@ def single_instance(require_lock: bool = False) -> Iterator[bool]: require_lock=True, that failure yields False so credential and token mutations cannot proceed without serialization. """ - fd = _open() - if fd is None: + try: + fd = file_lock.acquire(lock_path()) + except OSError: yield not require_lock return + if fd is None: + yield False + return try: - if not _try_acquire(fd): - yield False - return - try: - yield True - finally: - _release(fd) + yield True finally: - os.close(fd) + file_lock.release(fd) + + +def unlink_while_held() -> None: + """Delete the lock file while this process still holds it (call inside + single_instance). Deleting it after release would let another process + lock the old file while a third creates and locks a new one at the same + path. POSIX only: Windows refuses to delete an open file, so there the + caller deletes it after release, and a process that has it open blocks + that delete.""" + if sys.platform == "win32": + return + try: + lock_path().unlink(missing_ok=True) + except OSError: + pass diff --git a/eufy_sync/cli/maintenance.py b/eufy_sync/cli/maintenance.py index d9f75b8..76e3cc0 100644 --- a/eufy_sync/cli/maintenance.py +++ b/eufy_sync/cli/maintenance.py @@ -299,47 +299,64 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | except Exception: pass - # Clear the keychain vault. On a file-backend machine this gate skips the - # deletes, which is safe only because credentials.json lives inside data_dir - # and is erased by the rmtree below; keep them together if CRED_FILE ever - # moves outside ~/.garmin-sync. - from eufy_sync.credentials import _keyring_available, delete_password, delete_token - if _keyring_available(): - # Best-effort: a locked keychain makes the vault read raise, and a - # half-finished uninstall that leaves the data dir behind (the rmtree - # is below) plus a raw traceback is worse than skipping this. The - # rmtree still erases a file-backed vault under data_dir. - try: - for name in user_names: - # "strava" here is the API app's client secret, not an account - # password; it moved into the vault alongside the other two. - for suffix in ["eufy", "garmin", "strava", "zwift"]: - delete_password(f"{name}:{suffix}") - delete_token("eufy") - delete_token("garmin") - delete_token("strava") - delete_token("zwift") - delete_token("zwift_probe") - except Exception: - print("Note: could not clear keychain entries (the keychain may be locked).") - - # Remove data directory. A kept DB at a custom --db path lives outside - # data_dir, so only the default location needs the selective sweep. - # The sync lock file is skipped: --uninstall holds it open, and Windows - # refuses to delete an open file. _remove_lock_file clears it once the - # lock is released. - preserve_default_db = keep_db and db_path == default_db_path and db_path.exists() - if data_dir.exists(): - from eufy_sync.cli.lock import LOCK_NAME - keep = {LOCK_NAME, "state.db"} if preserve_default_db else {LOCK_NAME} - for item in data_dir.iterdir(): - if item.name in keep: - continue - if item.is_dir(): - shutil.rmtree(item) - else: - item.unlink() - _remove_dir_if_empty(data_dir) + from eufy_sync.cli.lock import LOCK_NAME + from eufy_sync.credentials import ( + VAULT_LOCK_NAME, + _keyring_available, + delete_password, + delete_token, + vault_lock, + ) + + # One vault lock across clearing the vault and deleting credentials.json, + # so no credential write can land in between. + with vault_lock(): + # Clear the keychain vault. On a file-backend machine this gate skips the + # deletes, which is safe only because credentials.json lives inside data_dir + # and is erased by the rmtree below; keep them together if CRED_FILE ever + # moves outside ~/.garmin-sync. + if _keyring_available(): + # Best-effort: a locked keychain makes the vault read raise, and a + # half-finished uninstall that leaves the data dir behind (the rmtree + # is below) plus a raw traceback is worse than skipping this. The + # rmtree still erases a file-backed vault under data_dir. + try: + for name in user_names: + # "strava" here is the API app's client secret, not an account + # password; it moved into the vault alongside the other two. + for suffix in ["eufy", "garmin", "strava", "zwift"]: + delete_password(f"{name}:{suffix}") + delete_token("eufy") + delete_token("garmin") + delete_token("strava") + delete_token("zwift") + delete_token("zwift_probe") + except Exception: + print("Note: could not clear keychain entries (the keychain may be locked).") + + # Remove data directory. A kept DB at a custom --db path lives outside + # data_dir, so only the default location needs the selective sweep. + # The sync lock file is skipped: --uninstall holds it open, and the + # caller deletes it (see _remove_lock_files). The vault lock file is + # held too; on POSIX the sweep deletes it while it is still held, so no + # other process can lock it between release and delete. Windows refuses + # to delete an open file, so there it is skipped and deleted after + # release instead. + preserve_default_db = keep_db and db_path == default_db_path and db_path.exists() + if data_dir.exists(): + keep = {LOCK_NAME} + if sys.platform == "win32": + keep.add(VAULT_LOCK_NAME) + if preserve_default_db: + keep.add("state.db") + for item in data_dir.iterdir(): + if item.name in keep: + continue + if item.is_dir(): + shutil.rmtree(item) + else: + item.unlink() + _remove_dir_if_empty(data_dir) # A custom --config/--db path lives outside data_dir, so it survives the # sweep above and must be removed explicitly. @@ -365,12 +382,20 @@ def _remove_dir_if_empty(path: Path) -> None: pass -def _remove_lock_file(data_dir: Path) -> None: - """Delete the sync lock file left by --uninstall, after it is released, - and the data dir with it when nothing else remains there.""" +def _remove_lock_files(data_dir: Path) -> None: + """Finish --uninstall once its locks are released. + + On Windows the lock files could not be deleted while open, so they go + now; another process that has one open blocks the delete, which keeps it + safe. On POSIX they were already deleted while still held, and deleting + now could remove a lock file another process has just created and + locked. Either way the data dir goes too when nothing else is left.""" from eufy_sync.cli.lock import LOCK_NAME - try: - (data_dir / LOCK_NAME).unlink(missing_ok=True) - except OSError: - return + from eufy_sync.credentials import VAULT_LOCK_NAME + if sys.platform == "win32": + for name in (LOCK_NAME, VAULT_LOCK_NAME): + try: + (data_dir / name).unlink(missing_ok=True) + except OSError: + pass _remove_dir_if_empty(data_dir) diff --git a/eufy_sync/credentials.py b/eufy_sync/credentials.py index a78ea52..88f4918 100644 --- a/eufy_sync/credentials.py +++ b/eufy_sync/credentials.py @@ -31,17 +31,31 @@ A lazy, one-time migration promotes secrets from the old per-item keychain layout (one keyring account per password/token) into the vault the first time each one is looked up. + +Every change to stored credentials (store, delete, migration, switching +stores) holds the vault lock, an exclusive OS lock on ~/.garmin-sync/ +vault.lock, from the moment it reads the vault until its cleanup is done. +Two processes therefore never interleave a read-modify-write: the second one +waits (up to VAULT_LOCK_TIMEOUT seconds) and then works on the first one's +result. A lock that cannot be created or acquired raises VaultLockError and +nothing is written. Reads do not take the lock. """ from __future__ import annotations +import contextlib +import functools import hashlib import json import logging import os import re import secrets +import threading import time from pathlib import Path +from typing import Callable, Iterator + +from eufy_sync import file_lock logger = logging.getLogger(__name__) @@ -53,28 +67,103 @@ # across "vault::" entries so set_password never fails on Windows; the # "vault" entry then holds a small header naming the tag, the chunk count and a # checksum. Each save writes its chunks under a fresh tag (the generation -# number plus a random suffix, so two processes saving at once never share -# chunk names), checks that the header is still the one it started from, and -# switches the header last. An interrupted or overtaken save never splices two -# vaults together. +# number plus a random suffix) and switches the header last, so a save killed +# at any step leaves either the old vault or the new one readable, never a +# splice of the two. # # The keychain cannot list entries, so the "vault:journal" entry records every -# tag a save may have left chunks under. Each completed save deletes the -# chunks of the tag it replaced at once, and the chunks of any other journal -# tag once it is older than JOURNAL_GRACE seconds (a younger one may belong to -# a save still in progress in another process). Released versions wrote chunks -# as "vault:"; those are still read and are deleted by the next save. -# MAX_CHUNKS bounds the chunk count a header may claim and how far a sweep -# probes one tag, so a corrupt store can never make either run away. +# tag a save may have left chunks under. Saves only run under the vault lock, +# so no other save can be in progress: every journal tag the live header does +# not name is a leftover of a finished or crashed save, and each save deletes +# them all once its header is in place. Released versions wrote chunks as +# "vault:"; those are still read and are deleted by the next save. +# +# MAX_CHUNKS caps how many chunks a save writes (a larger vault is refused +# before anything is written) and so how many a header of this layout may +# claim. Released versions had no cap, so a "vault:" header of any size is +# still read; its chunks end at the first missing one either way. CHUNK_LIMIT = 1200 MAX_CHUNKS = 40 JOURNAL_ACCOUNT = "vault:journal" -JOURNAL_GRACE = 600 -# Each entry is ~30 characters, so this stays well under CHUNK_LIMIT. +# Each entry is ~20 characters, so this stays well under CHUNK_LIMIT. MAX_JOURNAL = 24 CRED_FILE = Path.home() / ".garmin-sync" / "credentials.json" +VAULT_LOCK_NAME = "vault.lock" +VAULT_LOCK_TIMEOUT = 30.0 + + +class VaultLockError(RuntimeError): + """The vault lock could not be created or was not released in time. + Nothing was written.""" + + +def vault_lock_path() -> Path: + # Next to CRED_FILE, read at call time: tests redirect CRED_FILE. The + # file is never deleted in normal use; only --uninstall removes it. + return CRED_FILE.parent / VAULT_LOCK_NAME + + +# One holder per process: the thread lock serializes threads (and makes the +# lock reentrant for nested calls such as a migration inside a store), and +# the first entry takes the OS lock that serializes processes. +_thread_lock = threading.RLock() +_lock_depth = 0 +_lock_fd: int | None = None + + +@contextlib.contextmanager +def vault_lock() -> Iterator[None]: + """Hold the exclusive vault lock for the block. Reentrant within a + process. Raises VaultLockError, writing nothing, when the lock file + cannot be opened or another process keeps the lock past + VAULT_LOCK_TIMEOUT seconds.""" + global _lock_depth, _lock_fd + deadline = time.monotonic() + VAULT_LOCK_TIMEOUT + if not _thread_lock.acquire(timeout=VAULT_LOCK_TIMEOUT): + raise VaultLockError(_VAULT_BUSY) + try: + if _lock_depth == 0: + path = vault_lock_path() + try: + fd = file_lock.acquire(path, max(0.0, deadline - time.monotonic())) + except OSError as e: + raise VaultLockError( + f"The credential lock file {path} could not be created " + f"({e.strerror or e}), so nothing was saved. Check that " + f"{path.parent} is writable and retry." + ) from e + if fd is None: + raise VaultLockError(_VAULT_BUSY) + _lock_fd = fd + _lock_depth += 1 + try: + yield + finally: + _lock_depth -= 1 + if _lock_depth == 0: + fd, _lock_fd = _lock_fd, None + file_lock.release(fd) + finally: + _thread_lock.release() + + +_VAULT_BUSY = ( + "Another eufy-sync process kept the stored credentials locked for " + f"{int(VAULT_LOCK_TIMEOUT)} seconds, so nothing was saved. Retry when it " + "finishes." +) + + +def _locked(fn: Callable) -> Callable: + """Run fn under the vault lock.""" + @functools.wraps(fn) + def wrapper(*args, **kwargs): + with vault_lock(): + return fn(*args, **kwargs) + return wrapper + def _keyring_available() -> bool: try: @@ -94,15 +183,50 @@ def _keyring_available() -> bool: return False +def _file_corrupt(detail: str) -> VaultCorruptError: + return VaultCorruptError( + f"The credentials file {CRED_FILE} is damaged ({detail}). " + "It was left untouched so nothing is saved over it. Repair it or " + "move it aside, then run eufy-sync to sign in again." + ) + + +def _read_cred_file() -> dict | None: + """The parsed CRED_FILE object, or None when there is no file. + + Raises RuntimeError when the file exists but cannot be read, and + VaultCorruptError when it is not a JSON object. Treating either as + empty would let the next save replace the file with an empty vault, + and treating it as unmarked would silently switch to the keychain and + hide every secret in the file.""" + try: + text = CRED_FILE.read_text() + except FileNotFoundError: + return None + except ValueError: + # UnicodeDecodeError: non-UTF-8 bytes. + raise _file_corrupt("not a JSON object") from None + except OSError as e: + raise RuntimeError( + f"The credentials file {CRED_FILE} could not be read " + f"({e.strerror or e}). Check its permissions and retry." + ) from e + try: + parsed = json.loads(text) + except ValueError: + raise _file_corrupt("not a JSON object") from None + if not isinstance(parsed, dict): + raise _file_corrupt("not a JSON object") + return parsed + + def _file_store_is_explicit() -> bool: """True when CRED_FILE carries the opt-in marker that only - use_file_store() writes. Malformed content counts as no marker. - ValueError covers both bad JSON and non-UTF-8 bytes in the file.""" - try: - data = json.loads(CRED_FILE.read_text()) - except (ValueError, TypeError, OSError): - return False - return isinstance(data, dict) and bool(data.get("explicit")) + use_file_store() writes. Only a file that parses and has no marker + counts as unmarked; an existing file that cannot be read or parsed + raises (see _read_cred_file).""" + data = _read_cred_file() + return data is not None and bool(data.get("explicit")) def _active_backend() -> str: @@ -133,16 +257,21 @@ def _empty_vault() -> dict: return {"passwords": {}, "tokens": {}} -def _normalize_vault(vault: dict | None) -> dict: - """Tolerate a partially-shaped or missing vault dict.""" +def _normalize_vault(vault: dict | None, corrupt: Callable[[str], Exception] | None = None) -> dict: + """Fill in absent sections of a vault dict. + + A section that is present but not an object raises (via `corrupt`, the + store's own VaultCorruptError factory): reading it as empty would let the + next save overwrite whatever is still recoverable in it.""" if not isinstance(vault, dict): return _empty_vault() - passwords = vault.get("passwords") - tokens = vault.get("tokens") - normalized = { - "passwords": passwords if isinstance(passwords, dict) else {}, - "tokens": tokens if isinstance(tokens, dict) else {}, - } + corrupt = corrupt or _keychain_corrupt + normalized = {} + for section in ("passwords", "tokens"): + value = vault.get(section, {}) + if not isinstance(value, dict): + raise corrupt(f'its "{section}" section is not a JSON object') + normalized[section] = value # The opt-in marker must survive every load/save round trip of the file # backend, or the first store after --use-file-store would drop it and # silently flip the backend back to the keychain. @@ -187,9 +316,9 @@ def _keychain_get(account: str) -> str | None: raise RuntimeError(_KEYCHAIN_UNREADABLE) from e -def _valid_count(value) -> bool: +def _valid_count(value, limit: int | None = MAX_CHUNKS) -> bool: # bool is an int subclass; a header saying {"__chunks__": true} is junk. - return type(value) is int and 1 <= value <= MAX_CHUNKS + return type(value) is int and value >= 1 and (limit is None or value <= limit) # "" (written by pre-release builds of the generation layout) or @@ -232,7 +361,9 @@ def _parse_header(raw: str) -> tuple: return ("gen", tag, meta["chunks"], meta["sha256"], meta["gen"]) raise _keychain_corrupt("the chunk header is malformed") if "__chunks__" in parsed: - if _valid_count(parsed["__chunks__"]): + # Released versions wrote any number of chunks; reading stops at the + # first missing one, so no cap is needed to keep this bounded. + if _valid_count(parsed["__chunks__"], limit=None): return ("legacy", parsed["__chunks__"]) raise _keychain_corrupt("the chunk header is malformed") return ("single", parsed) @@ -281,33 +412,35 @@ def _load_vault_from_keychain() -> dict: try: return _assemble(_parse_header(raw)) except VaultCorruptError: - # A concurrent save may have switched the header and deleted the - # chunks this read was partway through. If the header moved on, read - # the new vault once; if it did not, the vault really is damaged. + # Reads do not take the vault lock, so a save in another process may + # have switched the header and deleted the chunks this read was + # partway through. If the header moved on, read the new vault once; + # if it did not, the vault really is damaged. fresh = _keychain_get(VAULT_ACCOUNT) if fresh is None or fresh == raw: raise return _assemble(_parse_header(fresh)) -def _delete_chunk_run(tag: str | None, start: int = 1) -> None: - # Delete one tag's chunk entries from `start` up to the first gap. - # Chunks are always written 1..N in order, so leftovers from an - # interrupted save form an unbroken run. Deleting from the top down keeps - # it unbroken if this sweep is itself cut short, so the next sweep still - # finds the rest. Bounded by MAX_CHUNKS. +def _delete_chunk_run(tag: str | None) -> None: + # Delete one tag's chunk entries from 1 up to the first gap. Chunks are + # always written 1..N in order, so leftovers form an unbroken run. + # Deleting from the top down, and stopping at the first failed delete, + # keeps what is left unbroken, so a later sweep still finds all of it. + # Raises on that failure so the caller keeps the tag in the journal. + # Tagged runs are bounded by MAX_CHUNKS; released-version runs had no + # cap and end at their first gap. import keyring run = [] - for i in range(start, MAX_CHUNKS + 1): + i = 1 + while tag is None or i <= MAX_CHUNKS: account = _chunk_account(tag, i) if keyring.get_password(SERVICE_NAME, account) is None: break run.append(account) + i += 1 for account in reversed(run): - try: - keyring.delete_password(SERVICE_NAME, account) - except Exception: - pass + keyring.delete_password(SERVICE_NAME, account) def _read_header() -> tuple[str | None, tuple | None]: @@ -343,171 +476,147 @@ def _layout_gen(layout: tuple | None) -> int: return 0 -def _read_journal() -> dict[str, float]: - """Tags that may still have chunk entries, with when each was recorded. - Best-effort: an unreadable or malformed journal reads as empty.""" +def _read_journal() -> list[str]: + """Tags that may still have chunk entries, oldest first. Best-effort: an + unreadable or malformed journal reads as empty. Pre-release builds + stored {tag: timestamp}; its keys are read the same way.""" import keyring try: raw = keyring.get_password(SERVICE_NAME, JOURNAL_ACCOUNT) - parsed = json.loads(raw) if raw else {} + parsed = json.loads(raw) if raw else [] except Exception: - return {} - if not isinstance(parsed, dict): - return {} - return { - tag: float(t) for tag, t in parsed.items() - if _valid_tag(tag) and isinstance(t, (int, float)) and not isinstance(t, bool) - } + return [] + if isinstance(parsed, dict): + parsed = list(parsed) + if not isinstance(parsed, list): + return [] + return list(dict.fromkeys(tag for tag in parsed if _valid_tag(tag))) -def _write_journal(journal: dict[str, float]) -> None: +def _write_journal(tags: list[str]) -> None: import keyring - if not journal: + if not tags: try: keyring.delete_password(SERVICE_NAME, JOURNAL_ACCOUNT) except Exception: pass return - newest = sorted(journal.items(), key=lambda item: item[1], reverse=True)[:MAX_JOURNAL] - keyring.set_password(SERVICE_NAME, JOURNAL_ACCOUNT, json.dumps(dict(newest))) + keyring.set_password(SERVICE_NAME, JOURNAL_ACCOUNT, json.dumps(tags[-MAX_JOURNAL:])) -def _journal_add(entries: dict[str, float]) -> None: +def _journal_add(tags: list[str]) -> None: journal = _read_journal() - added = {tag: t for tag, t in entries.items() if tag not in journal} + added = [tag for tag in tags if tag not in journal] if added: - journal.update(added) - _write_journal(journal) + _write_journal(journal + added) -def _journal_remove(tags: set[str]) -> None: - # Re-read right before writing so entries another process added since - # this one last looked are kept. - journal = _read_journal() - if tags & journal.keys(): - _write_journal({tag: t for tag, t in journal.items() if tag not in tags}) - - -def _sweep_chunks(prev_tag: str | None, force: bool = False) -> None: +def _sweep_chunks() -> None: """Best-effort removal of chunk entries that no header points at. - Runs after a save has switched the header. Deletes the released-version - "vault:i" chunks, the chunks of prev_tag (the tag the header named before - this save), and the chunks of every journal tag that is older than - JOURNAL_GRACE, or every journal tag at all when force is set. The tag the - header names right now is always kept, even if another process switched - it after this save did.""" + Runs under the vault lock after a save has switched the header. No other + save can be running, so every journal tag except the one the header + names is a leftover and its chunks are deleted, along with the + released-version "vault:i" chunks unless the header still uses them. A + tag whose chunks could not all be deleted stays in the journal for the + next save.""" try: - _, layout = _read_header() - if layout is None and _keychain_get(VAULT_ACCOUNT) is not None: + raw, layout = _read_header() + if layout is None and raw is not None: # A header this module cannot parse might still point somewhere; # deleting nothing is the safe answer. return current = layout[1] if layout is not None and layout[0] == "gen" else None if layout is None or layout[0] != "legacy": _delete_chunk_run(None) - if prev_tag is not None and prev_tag != current: - _delete_chunk_run(prev_tag) - now = time.time() - swept = set() - for tag, recorded in _read_journal().items(): + remaining = [] + for tag in _read_journal(): if tag == current: continue - if force or tag == prev_tag or now - recorded >= JOURNAL_GRACE: - if tag != prev_tag: - _delete_chunk_run(tag) - swept.add(tag) - if prev_tag is not None: - swept.add(prev_tag) - _journal_remove(swept) - if set(_read_journal()) <= {current}: - # Only the live tag left: nothing to track. - _write_journal({}) + try: + _delete_chunk_run(tag) + except Exception: + remaining.append(tag) + _write_journal(remaining) except Exception: # The new vault is already in place; leftovers cost tidiness, not # data, so a failed cleanup must not fail the save. pass -class VaultWriteConflict(RuntimeError): - """Another process saved the keychain vault while this save was running. - - This save was abandoned before it switched the header, so the vault holds - the other process's complete write. Retrying the command reads that write - and applies this change on top of it.""" - - -_WRITE_CONFLICT = ( - "Another eufy-sync process changed the stored credentials while this one " - "was saving, so this change was not saved. The stored credentials are " - "intact. Run the command again." -) +class VaultTooLargeError(RuntimeError): + """The vault is too large to store in the keychain. Nothing was written.""" +@_locked def _save_vault_to_keychain(vault: dict) -> None: import keyring - start_raw, start_layout = _read_header() + # json.dumps escapes non-ASCII by default, so each character is one + # UTF-16 unit and a CHUNK_LIMIT-character entry stays under the Windows cap. + payload = json.dumps(vault) + chunks = [] + if len(payload) > CHUNK_LIMIT: + chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] + if len(chunks) > MAX_CHUNKS: + # Checked before any write: a header claiming more chunks than the + # reader accepts would commit a vault nobody can read. + raise VaultTooLargeError( + f"The stored credentials are too large for the system keychain " + f"({len(payload)} characters; the limit is " + f"{MAX_CHUNKS * CHUNK_LIMIT}). Nothing was changed. Run " + "eufy-sync --use-file-store to keep them in a file instead." + ) + + _, start_layout = _read_header() + pending = [] prev_tag = _layout_tag(start_layout) - pending = {} if prev_tag is not None: # Recorded before the switch, so a sweep a crash cuts short is # finished by a later save. - pending[prev_tag] = 0.0 + pending.append(prev_tag) if "." not in prev_tag: # Pre-release generation layout: a crashed save there left its # chunks at the neighbouring generation numbers. gen = int(prev_tag) - for neighbour in (gen - 1, gen + 1): - if neighbour >= 1: - pending[str(neighbour)] = 0.0 - # json.dumps escapes non-ASCII by default, so each character is one - # UTF-16 unit and a CHUNK_LIMIT-character entry stays under the Windows cap. - payload = json.dumps(vault) + pending.extend(str(n) for n in (gen - 1, gen + 1) if n >= 1) - if len(payload) <= CHUNK_LIMIT: + if not chunks: if pending: _journal_add(pending) - if _keychain_get(VAULT_ACCOUNT) != start_raw: - raise VaultWriteConflict(_WRITE_CONFLICT) keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, payload) - _sweep_chunks(prev_tag) + _sweep_chunks() return - # The new chunks go under a tag no header references and no other writer - # can pick, and the header write is the commit point. A save killed - # before it leaves the old vault readable; one killed after it leaves the - # new vault readable. Either way the journal names the leftovers. - tag = f"{_layout_gen(start_layout) + 1}.{secrets.token_hex(4)}" - pending[tag] = time.time() - _journal_add(pending) - chunks = [payload[i:i + CHUNK_LIMIT] for i in range(0, len(payload), CHUNK_LIMIT)] + # The new chunks go under a tag no header references, and the header + # write is the commit point. A save killed before it leaves the old vault + # readable; one killed after it leaves the new vault readable. Either way + # the journal names the leftovers for the next save to delete. + gen = _layout_gen(start_layout) + 1 + tag = f"{gen}.{secrets.token_hex(4)}" + _journal_add(pending + [tag]) try: for i, chunk in enumerate(chunks, start=1): keyring.set_password(SERVICE_NAME, _chunk_account(tag, i), chunk) - # Commit only on top of the header this save's vault was read under. - # Another writer that switched it in the meantime holds the newer - # vault; overwriting its header would also orphan its chunks. - if _keychain_get(VAULT_ACCOUNT) != start_raw: - raise VaultWriteConflict(_WRITE_CONFLICT) except Exception: try: _delete_chunk_run(tag) - _journal_remove({tag}) + _write_journal([t for t in _read_journal() if t != tag]) except Exception: pass raise header = { "__vault__": { - "gen": _layout_gen(start_layout) + 1, + "gen": gen, "tag": tag, "chunks": len(chunks), "sha256": hashlib.sha256(payload.encode()).hexdigest(), } } keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps(header)) - _sweep_chunks(prev_tag) + _sweep_chunks() +@_locked def _delete_keychain_vault() -> None: """Best-effort removal of the vault header, every chunk entry this module can find, and the journal.""" @@ -547,41 +656,20 @@ def _delete_keychain_vault() -> None: def _load_vault_from_file() -> dict: - try: - text = CRED_FILE.read_text() - except FileNotFoundError: + parsed = _read_cred_file() + if parsed is None: return _empty_vault() - except ValueError: - parsed = None # non-UTF-8 bytes - except OSError as e: - raise RuntimeError( - f"The credentials file {CRED_FILE} could not be read " - f"({e.strerror or e}). Check its permissions and retry." - ) from e - else: - try: - parsed = json.loads(text) - except ValueError: - parsed = None - if not isinstance(parsed, dict): - # Treating this as empty would let the next save replace the file - # with an empty vault, destroying whatever is still recoverable in it. - raise VaultCorruptError( - f"The credentials file {CRED_FILE} is damaged (not a JSON object). " - "It was left untouched so nothing is saved over it. Repair it or " - "move it aside, then run eufy-sync to sign in again." - ) - return _normalize_vault(parsed) + return _normalize_vault(parsed, _file_corrupt) +@_locked def _save_vault_to_file(vault: dict) -> None: CRED_FILE.parent.mkdir(parents=True, exist_ok=True, mode=0o700) # Temp file + atomic rename: an interrupted in-place write would truncate # the vault, destroying the secrets and the opt-in marker (which would # silently flip the backend to an empty keychain on the next run). The - # temp name carries the pid so two concurrent writers (e.g. the 4-hourly - # Launch Agent and an interactive command) never share one temp inode and - # truncate each other's partial write before the rename. + # pid in the temp name is a second guard behind the vault lock, which + # already keeps two writers from running at once. tmp = CRED_FILE.with_name(f"{CRED_FILE.name}.{os.getpid()}.tmp") fd = os.open(str(tmp), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) try: @@ -605,6 +693,7 @@ def _load_vault() -> dict: return _load_vault_from_keychain() +@_locked def _save_vault(vault: dict) -> None: if _active_backend() == "file": _save_vault_to_file(vault) @@ -634,17 +723,23 @@ def get_password(account: str, migrate: bool = True) -> str | None: if legacy is not None: if not migrate: return legacy - vault["passwords"][account] = legacy - _save_vault(vault) - try: - keyring.delete_password(SERVICE_NAME, account) - except Exception: - pass - return legacy + with vault_lock(): + # Re-read under the lock: another process may have migrated + # (or changed) this password since the read above. + vault = _load_vault() + if account not in vault["passwords"]: + vault["passwords"][account] = legacy + _save_vault(vault) + try: + keyring.delete_password(SERVICE_NAME, account) + except Exception: + pass + return vault["passwords"][account] return None +@_locked def store_password(account: str, password: str) -> None: """Store a password in the vault (keychain or file, whichever is active).""" vault = _load_vault() @@ -652,6 +747,7 @@ def store_password(account: str, password: str) -> None: _save_vault(vault) +@_locked def delete_password(account: str) -> None: """Remove a password from the vault, and best-effort from the legacy keychain item if one is still lingering.""" @@ -690,17 +786,22 @@ def get_token(name: str) -> dict | None: legacy = json.loads(legacy_raw) except (json.JSONDecodeError, TypeError): return None - vault["tokens"][name] = legacy - _save_vault(vault) - try: - keyring.delete_password(SERVICE_NAME, f"token:{name}") - except Exception: - pass - return legacy + with vault_lock(): + # Re-read under the lock, as in get_password. + vault = _load_vault() + if name not in vault["tokens"]: + vault["tokens"][name] = legacy + _save_vault(vault) + try: + keyring.delete_password(SERVICE_NAME, f"token:{name}") + except Exception: + pass + return vault["tokens"][name] return None +@_locked def store_token(name: str, data: dict) -> None: """Store a token dict in the vault (keychain or file, whichever is active).""" vault = _load_vault() @@ -708,6 +809,7 @@ def store_token(name: str, data: dict) -> None: _save_vault(vault) +@_locked def delete_token(name: str) -> None: """Remove a token from the vault, and best-effort from the legacy keychain item if one is still lingering.""" @@ -727,6 +829,7 @@ def delete_token(name: str) -> None: # --- Mode switching ----------------------------------------------------------- +@_locked def use_file_store() -> None: """Adopt the 0o600 file as the permanent credential store. @@ -784,6 +887,7 @@ def use_file_store() -> None: _delete_keychain_vault() +@_locked def use_keychain_store() -> None: """Move the vault into the system keychain and stop using the file. diff --git a/eufy_sync/file_lock.py b/eufy_sync/file_lock.py new file mode 100644 index 0000000..b1b7b1d --- /dev/null +++ b/eufy_sync/file_lock.py @@ -0,0 +1,104 @@ +"""Cross-platform exclusive locks on a file (fcntl.flock or msvcrt.locking). + +Shared by the sync lock (eufy_sync.cli.lock) and the credential vault lock +(eufy_sync.credentials). The OS drops a lock when its handle closes or the +process dies, so a killed process never leaves a stale lock behind. + +On POSIX a lock belongs to the file, not the path. If a holder unlinks the +file (only --uninstall does), a process that opened the old file before the +unlink could lock it after the release while a third process creates and +locks a new file at the same path. acquire() rejects a lock on a file that is +no longer linked at its path and retries on the new one, so at most one +process ever holds the lock for a path. +""" +from __future__ import annotations + +import os +import sys +import time +from pathlib import Path + +if sys.platform == "win32": + import msvcrt +else: + import fcntl + +POLL_INTERVAL = 0.05 +MAX_ORPHAN_RETRIES = 5 + + +def _open(path: Path) -> int: + """Open (creating if needed) the lock file. Raises OSError if it cannot.""" + path.parent.mkdir(parents=True, exist_ok=True, mode=0o700) + return os.open(str(path), os.O_RDWR | os.O_CREAT, 0o600) + + +def try_lock(fd: int) -> bool: + """Take the exclusive lock without blocking. False when someone holds it.""" + try: + if sys.platform == "win32": + # msvcrt locks a byte range from the current position; one byte + # past EOF is fine and keeps the file empty. + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + else: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + return False + return True + + +def unlock(fd: int) -> None: + try: + if sys.platform == "win32": + os.lseek(fd, 0, os.SEEK_SET) + msvcrt.locking(fd, msvcrt.LK_UNLCK, 1) + else: + fcntl.flock(fd, fcntl.LOCK_UN) + except OSError: + # Closing the handle releases it anyway. + pass + + +def _still_linked(fd: int, path: Path) -> bool: + if sys.platform == "win32": + # Windows cannot delete a file another process has open. + return True + try: + on_disk = os.stat(path) + except OSError: + return False + held = os.fstat(fd) + return (on_disk.st_dev, on_disk.st_ino) == (held.st_dev, held.st_ino) + + +def acquire(path: Path, timeout: float = 0.0) -> int | None: + """Open `path` and take its exclusive lock, waiting up to `timeout` + seconds for another holder to let go. + + Returns the open handle holding the lock (pass it to release()), or None + if someone else still held it when the wait ran out. Raises OSError when + the lock file cannot be created or opened.""" + deadline = time.monotonic() + timeout + orphans = 0 + while True: + fd = _open(path) + if try_lock(fd): + if _still_linked(fd, path): + return fd + # The previous holder unlinked the file while holding it; this + # lock is on an orphan. Start over on whatever is at the path now. + release(fd) + orphans += 1 + if orphans <= MAX_ORPHAN_RETRIES: + continue + else: + os.close(fd) + if time.monotonic() >= deadline: + return None + time.sleep(POLL_INTERVAL) + + +def release(fd: int) -> None: + unlock(fd) + os.close(fd) diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 3dd8259..219b723 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -516,26 +516,84 @@ def test_marked_file_with_keyring_stays_file(fake_keyring, cred_file): assert get_password("default:eufy") == "file-pw" -def test_malformed_file_counts_as_unmarked(fake_keyring, cred_file): - from eufy_sync.credentials import _active_backend +@pytest.mark.parametrize("content", [b"{not valid json::", b"\x80\x81\xfe\xff", b"[1, 2]"]) +def test_damaged_file_raises_instead_of_counting_as_unmarked(fake_keyring, cred_file, content): + """A damaged file may be an explicit file store whose marker can no + longer be read. Picking the keychain would hide every secret in it, so + backend selection raises the corruption error instead. (Non-UTF-8 bytes + make read_text() raise UnicodeDecodeError, a ValueError.)""" + from eufy_sync.credentials import VaultCorruptError, _active_backend, get_token cred_file.parent.mkdir(parents=True, exist_ok=True) - cred_file.write_text("{not valid json::") + cred_file.write_bytes(content) - assert _active_backend() == "keychain" + with pytest.raises(VaultCorruptError, match="damaged"): + _active_backend() + with pytest.raises(VaultCorruptError): + get_token("garmin") + with pytest.raises(VaultCorruptError): + store_token("garmin", {"a": 1}) + assert cred_file.read_bytes() == content -def test_non_utf8_file_counts_as_unmarked(fake_keyring, cred_file): - """read_text() raises UnicodeDecodeError (a ValueError) on non-UTF-8 - bytes; that must count as no marker, not crash every backend lookup.""" +def test_unreadable_file_raises_instead_of_counting_as_unmarked(fake_keyring, cred_file, monkeypatch): from eufy_sync.credentials import _active_backend cred_file.parent.mkdir(parents=True, exist_ok=True) - cred_file.write_bytes(b"\x80\x81\xfe\xff") + cred_file.write_text(json.dumps({"explicit": True, "passwords": {}, "tokens": {}})) + + def denied(self, *args, **kwargs): + raise PermissionError(13, "Permission denied") + monkeypatch.setattr(type(cred_file), "read_text", denied) + with pytest.raises(RuntimeError, match="could not be read"): + _active_backend() + + +def test_parsed_unmarked_file_is_still_ignored(fake_keyring, cred_file): + from eufy_sync.credentials import _active_backend + + cred_file.parent.mkdir(parents=True, exist_ok=True) + cred_file.write_text(json.dumps({"passwords": {"a": "b"}, "tokens": {}})) assert _active_backend() == "keychain" +@pytest.mark.parametrize("section", ["passwords", "tokens"]) +@pytest.mark.parametrize("bad", [[], "x", None, 3]) +def test_wrong_type_section_in_file_raises_and_is_not_overwritten(no_keyring, cred_file, section, bad): + from eufy_sync.credentials import VaultCorruptError, store_password + + cred_file.parent.mkdir(parents=True, exist_ok=True) + original = json.dumps({"explicit": True, "passwords": {}, "tokens": {}, section: bad}) + cred_file.write_text(original) + + with pytest.raises(VaultCorruptError, match=section): + store_password("default:eufy", "pw") + assert cred_file.read_text() == original + + +@pytest.mark.parametrize("section", ["passwords", "tokens"]) +def test_wrong_type_section_in_keychain_raises_and_is_not_overwritten(fake_keyring, section): + from eufy_sync.credentials import VaultCorruptError, store_password + + original = json.dumps({"passwords": {}, "tokens": {}, section: ["recoverable"]}) + fake_keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, original) + + with pytest.raises(VaultCorruptError, match=section): + store_password("default:eufy", "pw") + assert fake_keyring.get_password(SERVICE_NAME, VAULT_ACCOUNT) == original + + +def test_absent_sections_still_read_as_empty(fake_keyring): + from eufy_sync.credentials import get_password, store_password + + fake_keyring.set_password(SERVICE_NAME, VAULT_ACCOUNT, json.dumps({"tokens": {"a": {"b": 1}}})) + assert get_password("default:eufy") is None + store_password("default:eufy", "pw") + assert get_password("default:eufy") == "pw" + assert get_token("a") == {"b": 1} + + # --- 11. explicit opt-in: use_file_store merge + marker ---------------------- @@ -1020,13 +1078,10 @@ def setup(): assert vault["passwords"] == _OLD_PW, f"killed after {steps} of {total}" assert vault["tokens"]["garmin"] in (old, new), f"killed after {steps} of {total}" - # The next completed save after the journal grace period leaves only - # the entries its header uses. (Inside the grace period the killed - # save's chunks are indistinguishable from a save still running in - # another process, so they are kept until then.) - later = time.time() + credentials.JOURNAL_GRACE + 1 - with patch("eufy_sync.credentials.time.time", return_value=later): - store_token("strava", {"access_token": "t"}) + # The next completed save leaves only the entries its header uses. + # Saves are serialized by the vault lock, so the killed save's + # chunks cannot belong to a save still running and go at once. + store_token("strava", {"access_token": "t"}) vault = _load_vault() assert vault["passwords"] == _OLD_PW assert vault["tokens"]["strava"] == {"access_token": "t"} @@ -1164,9 +1219,9 @@ def set_password(service, account, password): def test_consecutive_interrupted_saves_leave_no_orphaned_chunks(fake_keyring, monkeypatch): """Each killed save leaves a full set of chunks (plaintext slices of the - vault) under a tag no header names. However many pile up, the first - completed save after the grace period deletes all of them, along with - released-version "vault:i" chunks.""" + vault) under a tag no header names. However many pile up, the next + completed save deletes all of them, along with released-version + "vault:i" chunks.""" credentials.store_password("default:eufy", "pw") store_token("garmin", _big_token(3 * CHUNK_LIMIT)) for i in (1, 2): @@ -1181,9 +1236,7 @@ def test_consecutive_interrupted_saves_leave_no_orphaned_chunks(fake_keyring, mo orphans = _vault_accounts(fake_keyring) - {VAULT_ACCOUNT, credentials.JOURNAL_ACCOUNT} assert len(orphans) > 4 * 3 - later = time.time() + credentials.JOURNAL_GRACE + 1 - with patch("eufy_sync.credentials.time.time", return_value=later): - store_token("strava", {"access_token": "t"}) + store_token("strava", {"access_token": "t"}) header = _header(fake_keyring) assert _vault_accounts(fake_keyring) == ( @@ -1192,9 +1245,9 @@ def test_consecutive_interrupted_saves_leave_no_orphaned_chunks(fake_keyring, mo assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) -def test_fresh_journal_tags_survive_a_save_inside_the_grace_period(fake_keyring, monkeypatch): - """A tag recorded moments ago may be a save still running in another - process. A sweep inside the grace period must not delete its chunks.""" +def test_crash_leftovers_are_deleted_by_the_very_next_save(fake_keyring, monkeypatch): + """Under the vault lock no other save can be running, so a journal tag + the header does not name is a crash leftover with nothing to wait for.""" store_token("garmin", _big_token(3 * CHUNK_LIMIT)) _kill_on_header_write(monkeypatch, fake_keyring) with pytest.raises(_Killed): @@ -1203,10 +1256,29 @@ def test_fresh_journal_tags_survive_a_save_inside_the_grace_period(fake_keyring, pending = set(json.loads(fake_keyring.get_password(SERVICE_NAME, credentials.JOURNAL_ACCOUNT))) live = _header(fake_keyring)["tag"] other = (pending - {live}).pop() + assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{other}:1") is not None store_token("strava", {"access_token": "t"}) - assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{other}:1") is not None + assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:{other}:1") is None + assert fake_keyring.get_password(SERVICE_NAME, credentials.JOURNAL_ACCOUNT) is None + + +def test_prerelease_timestamped_journal_is_still_swept(fake_keyring): + """Pre-release builds stored the journal as {tag: timestamp}.""" + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + for i in (1, 2): + fake_keyring.set_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:9.0000abcd:{i}", "junk") + fake_keyring.set_password( + SERVICE_NAME, credentials.JOURNAL_ACCOUNT, json.dumps({"9.0000abcd": time.time()}) + ) + + store_token("strava", {"access_token": "t"}) + + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) + ) def test_switching_to_the_file_store_deletes_every_known_chunk(fake_keyring, cred_file, monkeypatch): @@ -1230,68 +1302,74 @@ def test_switching_to_the_file_store_deletes_every_known_chunk(fake_keyring, cre assert _load_vault()["tokens"]["garmin"] == _big_token(3 * CHUNK_LIMIT) -def test_save_aborts_when_another_writer_switches_the_header_first(fake_keyring): - """Two writers start from the same header. The second to finish must not - commit over the first: it raises a retryable error, deletes its own - chunks, and leaves the first writer's vault readable.""" - from eufy_sync.credentials import VaultWriteConflict, _save_vault_to_keychain +def test_a_save_inside_another_save_waits_for_the_lock(fake_keyring, monkeypatch): + """The review's race, replayed: writer B has loaded the vault, and writer + A tries to save completely before B's header write lands. With the vault + lock A cannot start until B finishes, so A works on B's result and both + updates survive. A runs in a thread, standing in for another process.""" + import threading - store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + from eufy_sync.credentials import _load_vault_from_keychain, store_password + + store_token("garmin", {"blob": "o" * 3000}) + store_token("garmin", {"blob": "o" * 3000}) real_set = fake_keyring.set_password - state = {"raced": False} + started = threading.Event() + other = {} + + def writer_a(): + started.set() + store_token("garmin", {"blob": "a" * 2000}) def racing_set(service, account, password): + if account == VAULT_ACCOUNT and "thread" not in other: + other["thread"] = threading.Thread(target=writer_a) + other["thread"].start() + started.wait() + # Give A every chance to run if it were not blocked. + other["thread"].join(timeout=0.3) + assert other["thread"].is_alive(), "writer A ran while B held the vault lock" real_set(service, account, password) - if account.endswith(":1") and account != f"{VAULT_ACCOUNT}:1" and not state["raced"]: - state["raced"] = True - # The other writer runs to completion between our chunk writes. - _save_vault_to_keychain( - {"passwords": {}, "tokens": {"garmin": _big_token(4 * CHUNK_LIMIT)}} - ) - with patch("keyring.set_password", racing_set): - with pytest.raises(VaultWriteConflict, match="Run the command again"): - _save_vault_to_keychain( - {"passwords": {}, "tokens": {"garmin": _big_token(5 * CHUNK_LIMIT)}} - ) + monkeypatch.setattr("keyring.set_password", racing_set) + store_password("u:garmin", "x") + other["thread"].join(timeout=10) + assert not other["thread"].is_alive() - assert get_token("garmin") == _big_token(4 * CHUNK_LIMIT) + after = _load_vault_from_keychain() + assert after["passwords"]["u:garmin"] == "x" + assert after["tokens"]["garmin"] == {"blob": "a" * 2000} header = _header(fake_keyring) - assert _vault_accounts(fake_keyring) - {credentials.JOURNAL_ACCOUNT} == ( + assert _vault_accounts(fake_keyring) == ( {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) ) -def test_writer_overtaken_at_its_header_write_leaves_a_readable_vault(fake_keyring): - """The review's race: writer B passes its header check, then writer A - saves completely before B's header write lands. Both used distinct chunk - names, and A's sweep keeps B's fresh chunks, so the vault B commits is - whole. (A's update is lost; the vault is never damaged.)""" - from eufy_sync.credentials import _load_vault_from_keychain, _save_vault_to_keychain - - def big(tag, n): - return {"passwords": {"u:eufy": "pw"}, "tokens": {"garmin": {"blob": tag * n}}} +def test_concurrent_threads_storing_different_tokens_all_persist(fake_keyring): + import threading - _save_vault_to_keychain(big("o", 3000)) - _save_vault_to_keychain(big("o", 3000)) - writer_b = _load_vault_from_keychain() - writer_b["passwords"]["u:garmin"] = "x" + names = [f"service{i}" for i in range(8)] + errors = [] - real_set = fake_keyring.set_password - hook = {"fn": lambda: _save_vault_to_keychain(big("a", 2000))} - - def racing_set(service, account, password): - if account == VAULT_ACCOUNT and hook["fn"]: - fn, hook["fn"] = hook["fn"], None - fn() - real_set(service, account, password) - - with patch("keyring.set_password", racing_set): - _save_vault_to_keychain(writer_b) - - after = _load_vault_from_keychain() - assert after["tokens"]["garmin"]["blob"][:1] in ("o", "a") - assert after["passwords"]["u:eufy"] == "pw" + def store(name): + try: + for round_ in range(5): + store_token(name, {"blob": name * 400, "round": round_}) + except Exception as e: # pragma: no cover - reported below + errors.append(e) + + threads = [threading.Thread(target=store, args=(name,)) for name in names] + for t in threads: + t.start() + for t in threads: + t.join(timeout=30) + assert not errors + for name in names: + assert get_token(name) == {"blob": name * 400, "round": 4} + header = _header(fake_keyring) + assert _vault_accounts(fake_keyring) == ( + {VAULT_ACCOUNT} | _chunk_accounts(header["tag"], header["chunks"]) + ) def test_shared_service_name_and_legacy_token_item_are_untouched(fake_keyring, cred_file): @@ -1392,3 +1470,172 @@ def payload_len(n: int) -> int: assert "__chunks__" not in data assert fake_keyring.get_password(SERVICE_NAME, f"{VAULT_ACCOUNT}:1") is None assert get_token("garmin") == {"access_token": "x" * n} + + +def test_doctor_fails_the_keychain_line_for_a_damaged_credentials_file(fake_keyring, cred_file): + from eufy_sync.cli import doctor + + cred_file.parent.mkdir(parents=True, exist_ok=True) + cred_file.write_text("{not json") + lines: list[tuple] = [] + doctor._check_keychain(lambda status, label, detail, fix=None: lines.append((status, detail))) + assert lines[0][0] == "FAIL" + assert "damaged" in lines[0][1] + + +# --- Vault lock ---------------------------------------------------------------- + +_FAKE_KEYRING_PRELUDE = """ +import sys, types +# No real keychain in the child: a stub module that is never used. +sys.modules["keyring"] = types.ModuleType("keyring") +from pathlib import Path +from eufy_sync import credentials +credentials.CRED_FILE = Path(sys.argv[1]) +credentials._keyring_available = lambda: False +""" + + +def _run_child(script: str, *args: str, timeout: float = 60): + import subprocess + import sys + return subprocess.Popen( + [sys.executable, "-c", _FAKE_KEYRING_PRELUDE + script, *args], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + ) + + +def test_concurrent_processes_storing_different_tokens_all_persist(no_keyring, cred_file): + """Separate processes share only the OS lock. Without it, two + read-modify-writes of the same file interleave and one token is lost.""" + script = """ +name = sys.argv[2] +for round_ in range(15): + credentials.store_token(name, {"blob": name * 200, "round": round_}) +""" + names = [f"proc{i}" for i in range(4)] + children = [_run_child(script, str(cred_file), name) for name in names] + for child in children: + _, err = child.communicate(timeout=60) + assert child.returncode == 0, err + for name in names: + assert get_token(name) == {"blob": name * 200, "round": 14} + + +def test_another_process_cannot_write_while_the_lock_is_held(no_keyring, cred_file): + from eufy_sync.credentials import vault_lock + + script = """ +credentials.VAULT_LOCK_TIMEOUT = 0.3 +try: + credentials.store_token("other", {"a": 1}) +except credentials.VaultLockError as e: + print("refused:", e) +""" + with vault_lock(): + child = _run_child(script, str(cred_file)) + out, err = child.communicate(timeout=60) + assert child.returncode == 0, err + assert "refused:" in out and "Retry" in out + assert not cred_file.exists() + + +def test_vault_lock_is_reentrant_and_released(no_keyring, cred_file): + from eufy_sync import file_lock + from eufy_sync.credentials import store_password, vault_lock, vault_lock_path + + with vault_lock(): + with vault_lock(): + store_password("default:eufy", "pw") + # Still held by this process after the inner block. + fd = file_lock.acquire(vault_lock_path()) + assert fd is None + fd = file_lock.acquire(vault_lock_path()) + assert fd is not None + file_lock.release(fd) + assert vault_lock_path().exists() + + +def test_store_refuses_when_the_lock_file_cannot_open(fake_keyring, cred_file): + from eufy_sync.credentials import VaultLockError, store_password + + with patch("eufy_sync.file_lock.os.open", side_effect=OSError(30, "Read-only file system")): + with pytest.raises(VaultLockError, match="could not be created"): + store_password("default:eufy", "pw") + assert _vault_accounts(fake_keyring) == set() + + +def test_migration_runs_under_the_lock(fake_keyring, cred_file): + from eufy_sync import credentials as creds + + fake_keyring.set_password(SERVICE_NAME, "default:eufy", "legacy-pw") + seen = [] + real_save = creds._save_vault_to_keychain.__wrapped__ + + def spy(vault): + seen.append(creds._lock_depth) + real_save(vault) + + with patch.object(creds, "_save_vault_to_keychain", spy): + assert creds.get_password("default:eufy") == "legacy-pw" + assert seen and all(depth >= 1 for depth in seen) + assert fake_keyring.get_password(SERVICE_NAME, "default:eufy") is None + + +# --- Chunk cap ----------------------------------------------------------------- + + +def test_oversized_vault_is_refused_before_any_write(fake_keyring): + from eufy_sync.credentials import MAX_CHUNKS, VaultTooLargeError + + store_token("garmin", _big_token(3 * CHUNK_LIMIT)) + before = dict(fake_keyring.data) + with pytest.raises(VaultTooLargeError, match="--use-file-store"): + store_token("huge", _big_token(MAX_CHUNKS * CHUNK_LIMIT)) + assert fake_keyring.data == before + assert get_token("garmin") == _big_token(3 * CHUNK_LIMIT) + + +def test_largest_allowed_vault_round_trips(fake_keyring): + from eufy_sync.credentials import MAX_CHUNKS + + overhead = len(json.dumps({"passwords": {}, "tokens": {"garmin": _big_token(0)}})) + token = _big_token(MAX_CHUNKS * CHUNK_LIMIT - overhead) + store_token("garmin", token) + assert _header(fake_keyring)["chunks"] == MAX_CHUNKS + assert get_token("garmin") == token + + +def test_use_keychain_store_keeps_the_file_when_the_vault_is_too_large(fake_keyring, cred_file): + from eufy_sync.credentials import MAX_CHUNKS, VaultTooLargeError, use_keychain_store + + cred_file.parent.mkdir(parents=True, exist_ok=True) + original = json.dumps({ + "explicit": True, "passwords": {"default:eufy": "pw"}, + "tokens": {"huge": _big_token(MAX_CHUNKS * CHUNK_LIMIT)}, + }) + cred_file.write_text(original) + + with pytest.raises(VaultTooLargeError): + use_keychain_store() + assert cred_file.read_text() == original + assert _vault_accounts(fake_keyring) == set() + + +def test_legacy_vault_with_more_chunks_than_the_cap_still_reads_and_migrates(fake_keyring, cred_file): + """Released versions wrote any number of "vault:i" chunks.""" + from eufy_sync.credentials import MAX_CHUNKS + + legacy = {"passwords": {"default:eufy": "pw"}, "tokens": {"garmin": _big_token((MAX_CHUNKS + 5) * CHUNK_LIMIT)}} + n = _write_legacy_chunked(fake_keyring, legacy) + assert n > MAX_CHUNKS + + assert get_token("garmin") == legacy["tokens"]["garmin"] + # Too big for the new layout, so the save is refused and the legacy + # vault is left as it was rather than half-migrated. + with pytest.raises(credentials.VaultTooLargeError): + store_token("strava", {"a": 1}) + assert get_token("garmin") == legacy["tokens"]["garmin"] + # Shrinking it migrates, and every legacy chunk is deleted. + store_token("garmin", {"a": 1}) + assert _vault_accounts(fake_keyring) == {VAULT_ACCOUNT} diff --git a/tests/test_lock.py b/tests/test_lock.py index d1088e6..1ac8051 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -1,6 +1,7 @@ """The single-instance lock that keeps a manual sync off the scheduled one.""" from __future__ import annotations +import sys from pathlib import Path from unittest.mock import patch @@ -55,7 +56,7 @@ def test_lock_is_released_when_the_block_raises(): def test_unusable_lock_file_runs_unlocked(): """The lock is a courtesy. A data dir we cannot write to must not become a new way for the sync to refuse to start.""" - with patch("eufy_sync.cli.lock.os.open", side_effect=OSError("read-only")): + with patch("eufy_sync.file_lock.os.open", side_effect=OSError("read-only")): with lock.single_instance() as acquired: assert acquired is True # Nothing is holding anything, so a second run is not blocked. @@ -64,7 +65,7 @@ def test_unusable_lock_file_runs_unlocked(): def test_strict_lock_refuses_to_run_when_lock_file_cannot_open(): - with patch("eufy_sync.cli.lock.os.open", side_effect=OSError("read-only")): + with patch("eufy_sync.file_lock.os.open", side_effect=OSError("read-only")): with lock.single_instance(require_lock=True) as acquired: assert acquired is False @@ -292,7 +293,7 @@ def check_lock(*args, **kwargs): def test_credential_command_refuses_when_the_lock_file_cannot_open(tmp_path, capsys, extra, target, flag): from eufy_sync.cli.app import main - with patch("eufy_sync.cli.lock.os.open", side_effect=OSError("read-only")), \ + with patch("eufy_sync.file_lock.os.open", side_effect=OSError("read-only")), \ patch(target) as command, \ patch("sys.argv", _command_argv(tmp_path, extra)), \ pytest.raises(SystemExit) as exc: @@ -322,3 +323,100 @@ def test_uninstall_under_the_lock_still_removes_the_whole_data_dir( assert not config_path.exists() assert not lock.lock_path().exists() assert not shared.DATA_DIR.exists() + + +@patch("eufy_sync.credentials._keyring_available", return_value=False) +@patch("eufy_sync.platform_support.agent_installed", return_value=False) +@patch("eufy_sync.cli.maintenance.sys.stdin") +@patch("builtins.input", return_value="y") +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX unlink ordering") +def test_uninstall_deletes_each_lock_file_before_releasing_it(_input, mock_stdin, _agent, _keyring, tmp_path): + """Deleting a lock file after release lets another process lock the old + file while a third creates and locks a new one at the same path. On + POSIX every lock --uninstall releases must already be unlinked.""" + import os + + from eufy_sync import file_lock + from eufy_sync.cli.app import main + + mock_stdin.isatty.return_value = True + config_path = _write_synced_config(shared.DATA_DIR) + real_release = file_lock.release + links_at_release = [] + + def spy(fd): + links_at_release.append(os.fstat(fd).st_nlink) + real_release(fd) + + with patch("eufy_sync.file_lock.release", spy), \ + patch("sys.argv", ["eufy-sync", "--uninstall", "--config", str(config_path)]): + main() + + # The vault lock (released when the sweep ends) and the sync lock. + assert len(links_at_release) == 2 + assert links_at_release == [0, 0] + assert not shared.DATA_DIR.exists() + + +def test_windows_uninstall_deletes_lock_files_only_after_release(tmp_path, monkeypatch): + """Windows cannot delete an open file, so the lock files are skipped + while held and deleted by _remove_lock_files afterwards.""" + from types import SimpleNamespace + + from eufy_sync.cli import maintenance + + fake_sys = SimpleNamespace(platform="win32") + monkeypatch.setattr(lock, "sys", fake_sys) + monkeypatch.setattr(maintenance, "sys", fake_sys) + shared.DATA_DIR.mkdir(parents=True) + for name in ("sync.lock", "vault.lock"): + (shared.DATA_DIR / name).write_text("") + + lock.unlink_while_held() + assert lock.lock_path().exists() + maintenance._remove_lock_files(shared.DATA_DIR) + assert not shared.DATA_DIR.exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX only") +def test_posix_remove_lock_files_never_deletes_a_lock_file_after_release(tmp_path): + """On POSIX a lock file present after release may be a new one another + process just created and locked; it must be left alone.""" + from eufy_sync.cli import maintenance + + data_dir = tmp_path / "data" + data_dir.mkdir() + (data_dir / "sync.lock").write_text("") + maintenance._remove_lock_files(data_dir) + assert (data_dir / "sync.lock").exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX only") +def test_lock_on_an_unlinked_file_is_retried_on_the_new_file(tmp_path): + """A process that opened the lock file before a holder unlinked it must + not treat a lock on that orphan as the lock for the path.""" + import os + + from eufy_sync import file_lock + + path = tmp_path / "x.lock" + stale = os.open(str(path), os.O_RDWR | os.O_CREAT, 0o600) + stale_inode = os.fstat(stale).st_ino + path.unlink() + real_open = file_lock._open + opened = [] + + def first_open_is_stale(p): + if not opened: + opened.append(stale) + return stale + fd = real_open(p) + opened.append(fd) + return fd + + with patch("eufy_sync.file_lock._open", first_open_is_stale): + fd = file_lock.acquire(path) + assert fd is not None and len(opened) == 2 + # (The fd number itself may be reused once the stale handle is closed.) + assert os.fstat(fd).st_ino == os.stat(path).st_ino != stale_inode + file_lock.release(fd) From 589a30d9b6b963bd7bdc8209c449e552dc707b6f Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:36:38 -0400 Subject: [PATCH 15/17] Keep the vault lock until uninstall finishes and recheck racing reads --- eufy_sync/cli/maintenance.py | 38 +++++++------ eufy_sync/credentials.py | 99 ++++++++++++++++++++++++++------- tests/test_credentials.py | 103 +++++++++++++++++++++++++++++++++++ tests/test_lock.py | 83 ++++++++++++++++++++++++++++ 4 files changed, 285 insertions(+), 38 deletions(-) diff --git a/eufy_sync/cli/maintenance.py b/eufy_sync/cli/maintenance.py index 76e3cc0..3a10bdc 100644 --- a/eufy_sync/cli/maintenance.py +++ b/eufy_sync/cli/maintenance.py @@ -336,17 +336,14 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | # Remove data directory. A kept DB at a custom --db path lives outside # data_dir, so only the default location needs the selective sweep. - # The sync lock file is skipped: --uninstall holds it open, and the - # caller deletes it (see _remove_lock_files). The vault lock file is - # held too; on POSIX the sweep deletes it while it is still held, so no - # other process can lock it between release and delete. Windows refuses - # to delete an open file, so there it is skipped and deleted after - # release instead. + # Both lock files are skipped: --uninstall holds them, and a lock file + # deleted mid-sweep lets another process create and lock a fresh one + # while credentials are still being removed. The vault lock file goes + # below, after everything else; the caller handles the sync lock file + # (see lock.unlink_while_held and _remove_lock_files). preserve_default_db = keep_db and db_path == default_db_path and db_path.exists() if data_dir.exists(): - keep = {LOCK_NAME} - if sys.platform == "win32": - keep.add(VAULT_LOCK_NAME) + keep = {LOCK_NAME, VAULT_LOCK_NAME} if preserve_default_db: keep.add("state.db") for item in data_dir.iterdir(): @@ -356,14 +353,21 @@ def _uninstall(data_dir: Path, config_path: Path | None = None, db_path: Path | shutil.rmtree(item) else: item.unlink() - _remove_dir_if_empty(data_dir) - - # A custom --config/--db path lives outside data_dir, so it survives the - # sweep above and must be removed explicitly. - if config_path != default_config_path and config_path.exists(): - config_path.unlink() - if db_path != default_db_path and not keep_db and db_path.exists(): - db_path.unlink() + + # A custom --config/--db path lives outside data_dir, so it survives the + # sweep above and must be removed explicitly. + if config_path != default_config_path and config_path.exists(): + config_path.unlink() + if db_path != default_db_path and not keep_db and db_path.exists(): + db_path.unlink() + + # Last step under the vault lock. On POSIX the lock file is deleted + # while still held, so no other process can lock it between release + # and delete. Windows refuses to delete an open file, so there + # _remove_lock_files deletes it after release. + if sys.platform != "win32": + (data_dir / VAULT_LOCK_NAME).unlink(missing_ok=True) + _remove_dir_if_empty(data_dir) print("") if keep_db: diff --git a/eufy_sync/credentials.py b/eufy_sync/credentials.py index 88f4918..9af6925 100644 --- a/eufy_sync/credentials.py +++ b/eufy_sync/credentials.py @@ -38,7 +38,8 @@ Two processes therefore never interleave a read-modify-write: the second one waits (up to VAULT_LOCK_TIMEOUT seconds) and then works on the first one's result. A lock that cannot be created or acquired raises VaultLockError and -nothing is written. Reads do not take the lock. +nothing is written. Reads do not take the lock; a read that lands on a store +switch in progress retries once on the new backend (see _load_vault). """ from __future__ import annotations @@ -687,12 +688,39 @@ def _save_vault_to_file(vault: dict) -> None: raise -def _load_vault() -> dict: - if _active_backend() == "file": +def _load_from(backend: str) -> dict: + if backend == "file": return _load_vault_from_file() return _load_vault_from_keychain() +def _load_vault() -> dict: + """Load the vault from the active backend. + + Reads do not take the vault lock, so a store switch in another process + can land between choosing the backend and reading it: use_keychain_store + unlinks the file this read picked, or use_file_store deletes the keychain + vault this read picked. Both switches write the new store before clearing + the old one, so in either case the old store reads as empty (or, for a + keychain half-deleted mid-read, damaged) while the new one already holds + everything. When the result is empty or damaged and the backend has + changed since it was chosen, the read is redone once on the new backend. + A populated read needs no recheck: it saw a complete vault.""" + backend = _active_backend() + try: + vault = _load_from(backend) + except VaultCorruptError: + current = _active_backend() + if current == backend: + raise + return _load_from(current) + if not vault["passwords"] and not vault["tokens"]: + current = _active_backend() + if current != backend: + return _load_from(current) + return vault + + @_locked def _save_vault(vault: dict) -> None: if _active_backend() == "file": @@ -704,6 +732,24 @@ def _save_vault(vault: dict) -> None: # --- Public API: passwords --------------------------------------------------- +def _legacy_get(account: str) -> str | None: + """A legacy per-item keychain value, or None when it is absent or the + keychain cannot be read.""" + try: + import keyring + return keyring.get_password(SERVICE_NAME, account) + except Exception: + return None + + +def _delete_legacy(account: str) -> None: + try: + import keyring + keyring.delete_password(SERVICE_NAME, account) + except Exception: + pass + + def get_password(account: str, migrate: bool = True) -> str | None: """Return a stored password, migrating it from the legacy keychain item (one account per password) into the vault on first access. @@ -724,17 +770,21 @@ def get_password(account: str, migrate: bool = True) -> str | None: if not migrate: return legacy with vault_lock(): - # Re-read under the lock: another process may have migrated - # (or changed) this password since the read above. + # Re-read both under the lock: another process may have + # migrated, changed, or deleted this password since the reads + # above. Saving the cached legacy value after a delete_password + # finished would bring the deleted secret back. vault = _load_vault() - if account not in vault["passwords"]: - vault["passwords"][account] = legacy - _save_vault(vault) - try: - keyring.delete_password(SERVICE_NAME, account) - except Exception: - pass - return vault["passwords"][account] + if account in vault["passwords"]: + _delete_legacy(account) + return vault["passwords"][account] + legacy = _legacy_get(account) + if legacy is None: + return None + vault["passwords"][account] = legacy + _save_vault(vault) + _delete_legacy(account) + return legacy return None @@ -787,16 +837,23 @@ def get_token(name: str) -> dict | None: except (json.JSONDecodeError, TypeError): return None with vault_lock(): - # Re-read under the lock, as in get_password. + # Re-read both under the lock, as in get_password. + account = f"token:{name}" vault = _load_vault() - if name not in vault["tokens"]: - vault["tokens"][name] = legacy - _save_vault(vault) + if name in vault["tokens"]: + _delete_legacy(account) + return vault["tokens"][name] + legacy_raw = _legacy_get(account) + if legacy_raw is None: + return None try: - keyring.delete_password(SERVICE_NAME, f"token:{name}") - except Exception: - pass - return vault["tokens"][name] + legacy = json.loads(legacy_raw) + except (json.JSONDecodeError, TypeError): + return None + vault["tokens"][name] = legacy + _save_vault(vault) + _delete_legacy(account) + return legacy return None diff --git a/tests/test_credentials.py b/tests/test_credentials.py index 219b723..0391adf 100644 --- a/tests/test_credentials.py +++ b/tests/test_credentials.py @@ -1582,6 +1582,109 @@ def spy(vault): assert fake_keyring.get_password(SERVICE_NAME, "default:eufy") is None +def _before_first_lock(monkeypatch, action): + """Run action once, just before the first vault_lock() entry: the window + after an unlocked read where another process can finish a write.""" + import contextlib + + from eufy_sync import credentials as creds + + real = creds.vault_lock + pending = [action] + + @contextlib.contextmanager + def hooked(): + if pending: + pending.pop()() + with real(): + yield + + monkeypatch.setattr(creds, "vault_lock", hooked) + + +def test_password_migration_does_not_undo_a_concurrent_delete(fake_keyring, cred_file, monkeypatch): + """get_password reads the legacy item before locking. If a delete_password + finishes in that window, the migration must not save the cached value.""" + from eufy_sync.credentials import get_password + + fake_keyring.set_password(SERVICE_NAME, "default:eufy", "legacy-pw") + _before_first_lock(monkeypatch, lambda: fake_keyring.delete_password(SERVICE_NAME, "default:eufy")) + + assert get_password("default:eufy") is None + assert credentials._load_vault()["passwords"] == {} + assert fake_keyring.get_password(SERVICE_NAME, "default:eufy") is None + + +def test_token_migration_does_not_undo_a_concurrent_delete(fake_keyring, cred_file, monkeypatch): + fake_keyring.set_password(SERVICE_NAME, "token:strava", json.dumps({"t": 1})) + _before_first_lock(monkeypatch, lambda: fake_keyring.delete_password(SERVICE_NAME, "token:strava")) + + assert get_token("strava") is None + assert credentials._load_vault()["tokens"] == {} + assert fake_keyring.get_password(SERVICE_NAME, "token:strava") is None + + +def test_migration_saves_the_legacy_value_read_under_the_lock(fake_keyring, cred_file, monkeypatch): + """A legacy item rewritten in the window is migrated with its new value.""" + from eufy_sync.credentials import get_password + + fake_keyring.set_password(SERVICE_NAME, "default:eufy", "old") + _before_first_lock(monkeypatch, lambda: fake_keyring.set_password(SERVICE_NAME, "default:eufy", "new")) + + assert get_password("default:eufy") == "new" + assert credentials._load_vault()["passwords"] == {"default:eufy": "new"} + assert fake_keyring.get_password(SERVICE_NAME, "default:eufy") is None + + +def _switch_during_first_read(monkeypatch, backend, switch): + """Run switch (a store change "in another process") after the read has + chosen backend but before it loads it.""" + from eufy_sync import credentials as creds + + real = creds._load_from + pending = [switch] + + def hooked(chosen): + if pending and chosen == backend: + pending.pop()() + return real(chosen) + + monkeypatch.setattr(creds, "_load_from", hooked) + + +def test_read_that_chose_the_file_survives_a_switch_to_the_keychain(fake_keyring, cred_file, monkeypatch): + """use_keychain_store unlinks the file the read picked; the credentials + are in the keychain by then and must not read as missing.""" + from eufy_sync.credentials import get_password, use_keychain_store + + cred_file.parent.mkdir(parents=True, exist_ok=True) + cred_file.write_text(json.dumps({"explicit": True, "passwords": {"default:eufy": "pw"}, "tokens": {}})) + _switch_during_first_read(monkeypatch, "file", use_keychain_store) + + assert get_password("default:eufy", migrate=False) == "pw" + assert not cred_file.exists() + + +def test_read_that_chose_the_keychain_survives_a_switch_to_the_file(fake_keyring, cred_file, monkeypatch): + from eufy_sync.credentials import store_token, use_file_store + + store_token("garmin", {"t": 1}) + _switch_during_first_read(monkeypatch, "keychain", use_file_store) + + assert get_token("garmin") == {"t": 1} + assert _vault_accounts(fake_keyring) == set() + + +def test_empty_vault_with_no_switch_reads_the_backend_once(fake_keyring, cred_file, monkeypatch): + from eufy_sync import credentials as creds + + calls = [] + real = creds._load_from + monkeypatch.setattr(creds, "_load_from", lambda b: calls.append(b) or real(b)) + assert creds._load_vault() == {"passwords": {}, "tokens": {}} + assert calls == ["keychain"] + + # --- Chunk cap ----------------------------------------------------------------- diff --git a/tests/test_lock.py b/tests/test_lock.py index 1ac8051..0ff4de0 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -358,6 +358,89 @@ def spy(fd): assert not shared.DATA_DIR.exists() +@patch("eufy_sync.credentials._keyring_available", return_value=True) +@patch("eufy_sync.platform_support.agent_installed", return_value=False) +@patch("eufy_sync.cli.maintenance.sys.stdin") +@patch("builtins.input", side_effect=["y", "n"]) +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX unlink ordering") +def test_uninstall_removes_the_vault_lock_last_and_holds_it_throughout( + _input, mock_stdin, _agent, _keyring, tmp_path +): + """A vault lock file deleted mid-sweep lets another process create and + lock a fresh one while uninstall is still removing credentials. Even when + the directory listing yields vault.lock first, every other removal (data + dir and keychain) must happen while it is still on disk and held, and it + must be the last thing deleted under the vault lock.""" + import shutil + + from eufy_sync import credentials + from eufy_sync.cli.app import main + from eufy_sync.credentials import store_password, store_token, vault_lock_path + + mock_stdin.isatty.return_value = True + config_path = _write_synced_config(shared.DATA_DIR) + store_password("default:eufy", "pw") + store_token("garmin", {"t": 1}) + (shared.DATA_DIR / "credentials.json").write_text('{"passwords": {}, "tokens": {}}') + (shared.DATA_DIR / "state.db").write_text("") + (shared.DATA_DIR / "cache").mkdir() + (shared.DATA_DIR / "cache" / "x").write_text("") + vault_lock_path().touch() + + events = [] + + def record(what): + events.append((what, vault_lock_path().exists(), credentials._lock_depth > 0)) + + real_unlink = Path.unlink + real_rmtree = shutil.rmtree + real_iterdir = Path.iterdir + real_delete_password = credentials.delete_password + real_delete_token = credentials.delete_token + + def unlink(self, *args, **kwargs): + if self.parent == shared.DATA_DIR: + record(self.name) + return real_unlink(self, *args, **kwargs) + + def rmtree(path, *args, **kwargs): + record(Path(path).name) + return real_rmtree(path, *args, **kwargs) + + def iterdir(self): + items = list(real_iterdir(self)) + # Worst case for the old loop: the vault lock comes first. + return iter(sorted(items, key=lambda p: p.name != "vault.lock")) + + def delete_password(account): + record(f"password:{account}") + real_delete_password(account) + + def delete_token(name): + record(f"token:{name}") + real_delete_token(name) + + with patch.object(Path, "unlink", unlink), \ + patch.object(Path, "iterdir", iterdir), \ + patch("eufy_sync.cli.maintenance.shutil.rmtree", rmtree), \ + patch("eufy_sync.credentials.delete_password", delete_password), \ + patch("eufy_sync.credentials.delete_token", delete_token), \ + patch("sys.argv", ["eufy-sync", "--uninstall", "--config", str(config_path)]): + main() + + names = [name for name, _, _ in events] + assert "vault.lock" in names + vault_index = names.index("vault.lock") + # Only the sync lock (handled by the caller) may go after the vault lock. + assert names[vault_index + 1:] == ["sync.lock"] + for name in ("credentials.json", "config.yaml", "state.db", "cache", + "password:default:eufy", "token:garmin"): + assert name in names[:vault_index] + for name, lock_on_disk, held in events[:vault_index + 1]: + assert lock_on_disk and held, name + assert not shared.DATA_DIR.exists() + + def test_windows_uninstall_deletes_lock_files_only_after_release(tmp_path, monkeypatch): """Windows cannot delete an open file, so the lock files are skipped while held and deleted by _remove_lock_files afterwards.""" From 14e0b73a6ccb66135535ab9b373a69035a26bc01 Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:37:16 -0400 Subject: [PATCH 16/17] Release 1.14.0 --- eufy_sync/__init__.py | 2 +- pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/eufy_sync/__init__.py b/eufy_sync/__init__.py index 1ce380c..b5a26ca 100644 --- a/eufy_sync/__init__.py +++ b/eufy_sync/__init__.py @@ -1,6 +1,6 @@ """Sync Eufy smart scale body composition data to Garmin Connect and Strava.""" -__version__ = "1.13.2" +__version__ = "1.14.0" # Public API for programmatic use from eufy_sync.eufy_client import EufyClient, EufyMeasurement diff --git a/pyproject.toml b/pyproject.toml index 4edf98f..9b017b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "eufy-sync" -version = "1.13.2" +version = "1.14.0" description = "Sync Eufy smart scale data to Garmin Connect, Strava, and Zwift" readme = "README.md" license = "MIT" From 32826f7150d398c77e087dce8cf837f1322e4eee Mon Sep 17 00:00:00 2001 From: Elias Sturim Date: Fri, 2 Oct 2026 10:40:00 -0400 Subject: [PATCH 17/17] Don't assume inode numbers are never reused in the lock retry test --- tests/test_lock.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_lock.py b/tests/test_lock.py index 0ff4de0..706848a 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -484,7 +484,6 @@ def test_lock_on_an_unlinked_file_is_retried_on_the_new_file(tmp_path): path = tmp_path / "x.lock" stale = os.open(str(path), os.O_RDWR | os.O_CREAT, 0o600) - stale_inode = os.fstat(stale).st_ino path.unlink() real_open = file_lock._open opened = [] @@ -499,7 +498,9 @@ def first_open_is_stale(p): with patch("eufy_sync.file_lock._open", first_open_is_stale): fd = file_lock.acquire(path) + # acquire noticed the orphan and opened the path a second time. Compare + # against the file now at the path, not the stale inode number: once the + # orphan is closed, filesystems such as ext4 may hand that number out again. assert fd is not None and len(opened) == 2 - # (The fd number itself may be reused once the stale handle is closed.) - assert os.fstat(fd).st_ino == os.stat(path).st_ino != stale_inode + assert os.fstat(fd).st_ino == os.stat(path).st_ino file_lock.release(fd)