From 0c41c34ccce2e3a326eed645615dc9e29f357e17 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 22:30:02 -0400 Subject: [PATCH 1/6] fix(update): enforce update integrity verification (fail-closed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The production update path (Help menu + auto-update) called download_update() without passing the GitHub release dict, so the checksum-verification block guarded by `if release is not None` was skipped entirely — downloaded artifacts were installed with no integrity check at all. - Attach the originating release to UpdateInfo (non-compared field) so download_update() can locate the checksum asset itself. - Make verification fail-closed: when a checksum asset exists, any failure to download/parse/match it raises ChecksumVerificationError and the artifact is deleted. Refuse to install when no release metadata is available at all. - Harden config load: a non-numeric precipitation_likelihood_threshold no longer raises and wipes the entire config to defaults (use _as_float). - macOS update script: shell-quote interpolated paths and mount the specific hdiutil mountpoint instead of globbing /Volumes/*. Co-Authored-By: Claude Opus 4.8 --- .../models/config_serialization.py | 4 +- src/accessiweather/services/simple_update.py | 116 +++++++++++------- src/accessiweather/services/update_restart.py | 35 ++++-- tests/test_simple_update.py | 67 +++++++++- 4 files changed, 167 insertions(+), 55 deletions(-) diff --git a/src/accessiweather/models/config_serialization.py b/src/accessiweather/models/config_serialization.py index 9f9003699..2040ce011 100644 --- a/src/accessiweather/models/config_serialization.py +++ b/src/accessiweather/models/config_serialization.py @@ -159,8 +159,8 @@ def from_dict(cls, data: dict) -> AppSettings: notify_precipitation_likelihood=settings_cls._as_bool( data.get("notify_precipitation_likelihood"), False ), - precipitation_likelihood_threshold=float( - data.get("precipitation_likelihood_threshold", 0.5) + precipitation_likelihood_threshold=settings_cls._as_float( + data.get("precipitation_likelihood_threshold"), 0.5 ), github_backend_url=data.get("github_backend_url", ""), alert_radius_type=data.get("alert_radius_type", "county"), diff --git a/src/accessiweather/services/simple_update.py b/src/accessiweather/services/simple_update.py index 2e5967041..8528f52a4 100644 --- a/src/accessiweather/services/simple_update.py +++ b/src/accessiweather/services/simple_update.py @@ -10,7 +10,7 @@ import sys import tempfile from collections.abc import Callable -from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -76,6 +76,11 @@ class UpdateInfo: commit_hash: str | None is_nightly: bool is_prerelease: bool + # The originating GitHub release dict, retained so download_update() can + # locate the checksum asset and verify integrity without the caller having + # to thread the release through. Excluded from equality/hash/repr because + # it is large and not part of the update's identity. + release: dict[str, Any] | None = field(default=None, compare=False, repr=False) def parse_commit_hash(release_notes: str) -> str | None: @@ -360,6 +365,7 @@ async def check_for_updates( commit_hash=commit_hash, is_nightly=release_type == "nightly", is_prerelease=bool(latest.get("prerelease")), + release=latest, ) async def download_update( @@ -407,48 +413,72 @@ async def download_update( logger.info("Downloaded update to %s", dest_path) - # Verify integrity if a checksum asset is available - if release is not None: - checksum_asset = find_checksum_asset(release, update_info.artifact_name) - if checksum_asset: - checksum_url = checksum_asset.get("browser_download_url", "") - if checksum_url: - try: - checksum_response = await self.http_client.get(checksum_url) - checksum_response.raise_for_status() - checksum_content = checksum_response.text - - parsed = parse_checksum_file(checksum_content, update_info.artifact_name) - if parsed: - algo, expected_hash = parsed - if not verify_file_checksum(dest_path, algo, expected_hash): - # Remove the corrupt/tampered file - dest_path.unlink(missing_ok=True) - raise ChecksumVerificationError( - f"Checksum verification failed for {update_info.artifact_name}. " - f"Expected {algo}:{expected_hash}. " - "The downloaded file may be corrupted or tampered with." - ) - logger.info( - "Checksum verified (%s) for %s", - algo, - update_info.artifact_name, - ) - else: - logger.warning( - "Could not parse checksum for %s from checksum file", - update_info.artifact_name, - ) - except httpx.HTTPError: - logger.warning( - "Failed to download checksum file for %s; skipping verification", - update_info.artifact_name, - ) - else: - logger.warning( - "No checksum file found for %s; integrity not verified", - update_info.artifact_name, - ) + # Verify integrity. Fall back to the release attached to the UpdateInfo + # so the integrity check runs even when the caller does not pass one. + if release is None: + release = update_info.release + + if release is None: + # We cannot identify the source release, so we cannot prove the + # artifact is genuine. Refuse to hand back an unverifiable update. + dest_path.unlink(missing_ok=True) + raise ChecksumVerificationError( + f"Cannot verify {update_info.artifact_name}: no release metadata available. " + "Refusing to install an unverified update." + ) + + checksum_asset = find_checksum_asset(release, update_info.artifact_name) + if checksum_asset is None: + # No checksum was published for this release. This is unusual for + # current releases (the build pipeline always uploads checksums.txt) + # so treat a missing checksum as a soft failure: warn loudly but do + # not block, to stay forward-compatible with releases that omit it. + logger.warning( + "No checksum asset found for %s; integrity could not be verified", + update_info.artifact_name, + ) + return dest_path + + # A checksum asset exists, so verification is mandatory. Any failure to + # download, parse, or match it is treated as a verification failure and + # the downloaded file is discarded (fail-closed). + checksum_url = checksum_asset.get("browser_download_url", "") + if not checksum_url: + dest_path.unlink(missing_ok=True) + raise ChecksumVerificationError( + f"Checksum asset for {update_info.artifact_name} has no download URL; " + "cannot verify integrity." + ) + + try: + checksum_response = await self.http_client.get(checksum_url) + checksum_response.raise_for_status() + checksum_content = checksum_response.text + except httpx.HTTPError as err: + dest_path.unlink(missing_ok=True) + raise ChecksumVerificationError( + f"Failed to download checksum file for {update_info.artifact_name}; " + "cannot verify integrity." + ) from err + + parsed = parse_checksum_file(checksum_content, update_info.artifact_name) + if parsed is None: + dest_path.unlink(missing_ok=True) + raise ChecksumVerificationError( + f"Could not find a checksum for {update_info.artifact_name} in the " + "published checksum file; cannot verify integrity." + ) + + algo, expected_hash = parsed + if not verify_file_checksum(dest_path, algo, expected_hash): + # Remove the corrupt/tampered file + dest_path.unlink(missing_ok=True) + raise ChecksumVerificationError( + f"Checksum verification failed for {update_info.artifact_name}. " + f"Expected {algo}:{expected_hash}. " + "The downloaded file may be corrupted or tampered with." + ) + logger.info("Checksum verified (%s) for %s", algo, update_info.artifact_name) return dest_path diff --git a/src/accessiweather/services/update_restart.py b/src/accessiweather/services/update_restart.py index a2a0b8a61..77632778c 100644 --- a/src/accessiweather/services/update_restart.py +++ b/src/accessiweather/services/update_restart.py @@ -4,6 +4,7 @@ import os import platform +import shlex import sys import tempfile import textwrap @@ -22,21 +23,37 @@ def build_macos_update_script( update_path: Path, app_path: Path, ) -> str: - """Build a shell script to apply a macOS update.""" + """ + Build a shell script to apply a macOS update. + + Paths are shell-quoted so spaces or shell metacharacters in the install + location cannot break or inject into the generated script. The DMG branch + mounts to a specific mountpoint reported by ``hdiutil`` instead of globbing + ``/Volumes/*``, which could otherwise pick up an unrelated mounted volume. + """ app_dir = app_path.parent + q_update = shlex.quote(str(update_path)) + q_app_dir = shlex.quote(str(app_dir)) + q_app_path = shlex.quote(str(app_path)) return textwrap.dedent( f""" #!/bin/bash sleep 2 - if [[ "{update_path}" == *.zip ]]; then - unzip -o "{update_path}" -d "{app_dir}" - elif [[ "{update_path}" == *.dmg ]]; then - hdiutil attach "{update_path}" -nobrowse -quiet - cp -R /Volumes/*/*.app "{app_dir}/" - hdiutil detach /Volumes/* -quiet + UPDATE_PATH={q_update} + APP_DIR={q_app_dir} + APP_PATH={q_app_path} + if [[ "$UPDATE_PATH" == *.zip ]]; then + unzip -o "$UPDATE_PATH" -d "$APP_DIR" + elif [[ "$UPDATE_PATH" == *.dmg ]]; then + MOUNT_DIR=$(hdiutil attach "$UPDATE_PATH" -nobrowse -quiet \ + | grep -oE '/Volumes/[^[:cntrl:]]+' | tail -1) + if [[ -n "$MOUNT_DIR" ]]; then + cp -R "$MOUNT_DIR"/*.app "$APP_DIR/" + hdiutil detach "$MOUNT_DIR" -quiet + fi fi - open "{app_path}" --args --updated - rm -f "$0" "{update_path}" + open "$APP_PATH" --args --updated + rm -f "$0" "$UPDATE_PATH" """ ).strip() diff --git a/tests/test_simple_update.py b/tests/test_simple_update.py index e5f883f06..a0f0d474d 100644 --- a/tests/test_simple_update.py +++ b/tests/test_simple_update.py @@ -211,11 +211,15 @@ def test_is_installed_version_false_for_portable(): @pytest.mark.asyncio async def test_simple_update_service_download(tmp_path): - """Test download_update method.""" + """Test download_update method with a valid checksum (happy path).""" + import hashlib from unittest.mock import AsyncMock, MagicMock from accessiweather.services.simple_update import UpdateInfo, UpdateService + artifact_content = b"x" * 100 + correct_hash = hashlib.sha256(artifact_content).hexdigest() + # Create a mock HTTP client mock_response = MagicMock() mock_response.headers = {"content-length": "100"} @@ -229,11 +233,26 @@ async def mock_aiter_bytes(chunk_size=None): mock_response.__aenter__ = AsyncMock(return_value=mock_response) mock_response.__aexit__ = AsyncMock(return_value=None) + checksum_response = MagicMock() + checksum_response.raise_for_status = MagicMock() + checksum_response.text = f"{correct_hash} update.zip" + mock_client = MagicMock() mock_client.stream = MagicMock(return_value=mock_response) + mock_client.get = AsyncMock(return_value=checksum_response) service = UpdateService("TestApp", http_client=mock_client) + release = { + "assets": [ + {"name": "update.zip", "browser_download_url": "https://example.com/update.zip"}, + { + "name": "update.zip.sha256", + "browser_download_url": "https://example.com/update.zip.sha256", + }, + ] + } + update_info = UpdateInfo( version="1.0.0", download_url="https://example.com/update.zip", @@ -242,6 +261,7 @@ async def mock_aiter_bytes(chunk_size=None): commit_hash=None, is_nightly=False, is_prerelease=False, + release=release, ) progress_calls = [] @@ -258,6 +278,51 @@ def progress_callback(downloaded, total): assert progress_calls[-1] == (100, 100) +@pytest.mark.asyncio +async def test_download_update_refuses_without_release_metadata(tmp_path): + """Fail-closed: an update with no release metadata cannot be verified, so it must be rejected.""" + from unittest.mock import AsyncMock, MagicMock + + from accessiweather.services.simple_update import ( + ChecksumVerificationError, + UpdateInfo, + UpdateService, + ) + + mock_response = MagicMock() + mock_response.headers = {"content-length": "100"} + mock_response.raise_for_status = MagicMock() + + async def mock_aiter_bytes(chunk_size=None): + yield b"x" * 100 + + mock_response.aiter_bytes = mock_aiter_bytes + mock_response.__aenter__ = AsyncMock(return_value=mock_response) + mock_response.__aexit__ = AsyncMock(return_value=None) + + mock_client = MagicMock() + mock_client.stream = MagicMock(return_value=mock_response) + + service = UpdateService("TestApp", http_client=mock_client) + + update_info = UpdateInfo( + version="1.0.0", + download_url="https://example.com/update.zip", + artifact_name="update.zip", + release_notes="Test release", + commit_hash=None, + is_nightly=False, + is_prerelease=False, + # No release attached -> integrity cannot be verified. + ) + + with pytest.raises(ChecksumVerificationError, match="no release metadata"): + await service.download_update(update_info, tmp_path) + + # The unverifiable file must not be left on disk. + assert not (tmp_path / "update.zip").exists() + + class TestApplyUpdateNoShellInjection: """Regression tests: subprocess calls in apply_update must never use shell=True.""" From ef35e5a0cc2f679a0c9a37f703be910e96c1fc36 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 22:41:37 -0400 Subject: [PATCH 2/6] fix(ui): crash/freeze fixes for tray, NOAA radio, alerts, async dialogs - system_tray.show_main_window: declare Win32 signatures via WinDLL so the 64-bit HWND isn't truncated to c_int (the sibling of fix #698 that was missed at this call site). - NOAA radio dialog: guard the background-thread completion handlers against a closed dialog (the old `_station_choice is None` check could never fire), and warm the per-call-sign WeatherIndex cache for displayed stations in the existing background prewarm worker so the Play lookup doesn't block the UI thread on first play. - weather_notifier: normalize alert `sent` timestamps to tz-aware before sorting so a mix of naive/aware values can't raise TypeError and drop all notifications for that cycle. - Add guard_destroyed decorator and apply it to wx.CallAfter completion handlers in discussion, forecast-product, weather-assistant, and advanced text-product dialogs, so closing a dialog mid-fetch can't crash the app. Co-Authored-By: Claude Opus 4.8 --- src/accessiweather/noaa_radio/stream_url.py | 15 +++++++- .../notifications/weather_notifier.py | 4 ++ .../dialogs/advanced_text_product_dialog.py | 9 ++++- src/accessiweather/ui/dialogs/async_guard.py | 38 +++++++++++++++++++ .../ui/dialogs/discussion_dialog.py | 6 +++ .../ui/dialogs/forecast_product_panel.py | 6 +++ .../ui/dialogs/noaa_radio_dialog.py | 16 +++++++- .../ui/dialogs/weather_assistant_dialog.py | 3 ++ src/accessiweather/ui/system_tray.py | 15 +++++++- tests/test_system_tray.py | 15 +++++--- 10 files changed, 116 insertions(+), 11 deletions(-) create mode 100644 src/accessiweather/ui/dialogs/async_guard.py diff --git a/src/accessiweather/noaa_radio/stream_url.py b/src/accessiweather/noaa_radio/stream_url.py index 387ba54e9..228daac32 100644 --- a/src/accessiweather/noaa_radio/stream_url.py +++ b/src/accessiweather/noaa_radio/stream_url.py @@ -423,4 +423,17 @@ def prewarm_cache(self) -> None: # WeatherIndex requires call sign specific queries, so we can't # pre-warm all at once - this would require knowing all station - # call signs ahead of time. The cache will warm on first play. + # call signs ahead of time. Use prewarm_stations() to warm a known set. + + def prewarm_stations(self, call_signs: list[str]) -> None: + """ + Pre-warm per-call-sign stream URL caches for a set of stations. + + Resolves stream URLs for each call sign so a later, main-thread + ``get_stream_urls`` lookup (e.g. when the user presses Play) hits a warm + cache instead of blocking on a network request. Intended to be called + from a background thread. Safe to call multiple times. + """ + for call_sign in call_signs: + with suppress(Exception): + self.get_stream_urls(call_sign) diff --git a/src/accessiweather/notifications/weather_notifier.py b/src/accessiweather/notifications/weather_notifier.py index 310da22f7..78ff435e9 100644 --- a/src/accessiweather/notifications/weather_notifier.py +++ b/src/accessiweather/notifications/weather_notifier.py @@ -348,6 +348,10 @@ def alert_priority(alert): sent_time = isoparse(sent_str) if sent_str else datetime.min.replace(tzinfo=UTC) except Exception: sent_time = datetime.min.replace(tzinfo=UTC) + # Normalize to tz-aware so sorting never mixes naive and aware + # datetimes (which raises TypeError and would drop all notifications). + if sent_time.tzinfo is None: + sent_time = sent_time.replace(tzinfo=UTC) # Return tuple for sorting: (severity_score, sent_time) # Higher severity score and more recent time are preferred diff --git a/src/accessiweather/ui/dialogs/advanced_text_product_dialog.py b/src/accessiweather/ui/dialogs/advanced_text_product_dialog.py index 81de41809..4e8f76982 100644 --- a/src/accessiweather/ui/dialogs/advanced_text_product_dialog.py +++ b/src/accessiweather/ui/dialogs/advanced_text_product_dialog.py @@ -11,6 +11,8 @@ import wx +from .async_guard import guard_destroyed + if TYPE_CHECKING: from ...models import Location, TextProduct from ...services.forecast_product_service import ForecastProductService @@ -538,7 +540,12 @@ async def _run_lookup(self, coro) -> None: except Exception as exc: # noqa: BLE001 logger.warning("Advanced text product lookup failed: %s", exc) text = f"Lookup failed: {exc}" - wx.CallAfter(self.result_text.SetValue, text) + wx.CallAfter(self._apply_lookup_result, text) + + @guard_destroyed + def _apply_lookup_result(self, text: str) -> None: + """Apply the lookup result to the UI (guarded against dialog close).""" + self.result_text.SetValue(text) def _run_lookup_sync(self) -> str: """Run a lookup synchronously for stub GUI tests.""" diff --git a/src/accessiweather/ui/dialogs/async_guard.py b/src/accessiweather/ui/dialogs/async_guard.py new file mode 100644 index 000000000..40da5ffdd --- /dev/null +++ b/src/accessiweather/ui/dialogs/async_guard.py @@ -0,0 +1,38 @@ +""" +Guard helpers for wx.CallAfter completion handlers. + +Background work in the dialogs marshals its result back onto the main thread +via ``wx.CallAfter``. If the user closes the dialog before that callback runs, +the queued handler touches widgets whose underlying C++ objects have already +been destroyed, which raises ``RuntimeError: wrapped C/C++ object of type ... +has been deleted``. The :func:`guard_destroyed` decorator swallows exactly that +case so a closed dialog can never crash the app, while re-raising any other +``RuntimeError`` so genuine bugs stay visible. +""" + +from __future__ import annotations + +import functools +import logging +from collections.abc import Callable +from typing import Any, TypeVar + +logger = logging.getLogger(__name__) + +F = TypeVar("F", bound=Callable[..., Any]) + + +def guard_destroyed(method: F) -> F: + """Skip a wx completion handler if its dialog/widgets were already destroyed.""" + + @functools.wraps(method) + def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: + try: + return method(self, *args, **kwargs) + except RuntimeError as exc: + if "has been deleted" in str(exc): + logger.debug("Skipping %s: window destroyed before callback ran", method.__name__) + return None + raise + + return wrapper # type: ignore[return-value] diff --git a/src/accessiweather/ui/dialogs/discussion_dialog.py b/src/accessiweather/ui/dialogs/discussion_dialog.py index 97248cff1..4946a6000 100644 --- a/src/accessiweather/ui/dialogs/discussion_dialog.py +++ b/src/accessiweather/ui/dialogs/discussion_dialog.py @@ -8,6 +8,7 @@ import wx from ...screen_reader import ScreenReaderAnnouncer +from .async_guard import guard_destroyed if TYPE_CHECKING: from ...app import AccessiWeatherApp @@ -235,6 +236,7 @@ async def _do_fetch(self, location): logger.error(f"Discussion fetch failed: {e}") wx.CallAfter(self._on_fetch_error, str(e)) + @guard_destroyed def _on_fetch_complete(self, location_name: str, discussion: str | None) -> None: """Handle fetch completion.""" self._is_loading = False @@ -253,6 +255,7 @@ def _on_fetch_complete(self, location_name: str, discussion: str | None) -> None ) self.explain_button.Disable() + @guard_destroyed def _on_fetch_error(self, error: str) -> None: """Handle fetch error.""" self._is_loading = False @@ -319,6 +322,7 @@ def _on_explain(self, event) -> None: # Run async explanation self.app.run_async(self._do_explain()) + @guard_destroyed def _on_explain_status(self, message: str) -> None: """Show model-selection progress while the summary is running.""" DiscussionDialog._show_ai_summary_section(self) @@ -412,6 +416,7 @@ def _build_model_info( info_lines.append("Cached: Yes") return "\n".join(info_lines) + @guard_destroyed def _on_explain_complete( self, explanation: str, @@ -452,6 +457,7 @@ def _on_regenerate(self, event) -> None: cache.clear() self._on_explain(event) + @guard_destroyed def _on_explain_error(self, error: str) -> None: """Handle explanation error.""" self._is_explaining = False diff --git a/src/accessiweather/ui/dialogs/forecast_product_panel.py b/src/accessiweather/ui/dialogs/forecast_product_panel.py index d571ba1a3..01a9b806f 100644 --- a/src/accessiweather/ui/dialogs/forecast_product_panel.py +++ b/src/accessiweather/ui/dialogs/forecast_product_panel.py @@ -25,6 +25,7 @@ import wx from ...screen_reader import ScreenReaderAnnouncer +from .async_guard import guard_destroyed from .forecast_product_ai import ( build_explainer, has_openrouter_key, @@ -269,6 +270,7 @@ def _render_no_cwa_state(self) -> None: self._hide_ai_summary_section() self.explain_button.Disable() + @guard_destroyed def _on_load_complete( self, result: TextProduct | list[TextProduct] | None, @@ -352,6 +354,7 @@ def _on_sps_choice_changed(self, event) -> None: self._hide_ai_summary_section() self._set_post_explain_buttons(has_attempted=False) + @guard_destroyed def _on_load_error(self, exc: Exception) -> None: """Render the fetch-failed state.""" del exc @@ -425,6 +428,7 @@ def _on_explain(self, event) -> None: ) self._schedule_explain(self._current_text) + @guard_destroyed def _on_explain_status(self, message: str) -> None: """Show model-selection progress while the summary is running.""" self._show_ai_summary_section() @@ -527,6 +531,7 @@ def _build_model_info( info_lines.append("Cached: Yes") return "\n".join(info_lines) + @guard_destroyed def _on_explain_complete( self, summary: str, @@ -560,6 +565,7 @@ def _on_explain_complete( completion_message = f"Plain Language Summary generated using {model_used}." self._announce_explain_status(completion_message) + @guard_destroyed def _on_explain_error(self, message: str) -> None: """Populate the AI summary TextCtrl with an error message.""" self._is_explaining = False diff --git a/src/accessiweather/ui/dialogs/noaa_radio_dialog.py b/src/accessiweather/ui/dialogs/noaa_radio_dialog.py index b04faba9e..585e24304 100644 --- a/src/accessiweather/ui/dialogs/noaa_radio_dialog.py +++ b/src/accessiweather/ui/dialogs/noaa_radio_dialog.py @@ -107,6 +107,9 @@ def __init__(self, parent: wx.Window, lat: float, lon: float) -> None: self._current_urls: list[str] = [] self._current_url_index: int = 0 self._playing_station: Station | None = None + # Set once the dialog is closed so background-thread completion handlers + # (delivered via wx.CallAfter) don't touch destroyed widgets. + self._closed = False self._health_timer = wx.Timer(self) self.Bind(wx.EVT_TIMER, self._on_health_check, self._health_timer) @@ -175,6 +178,11 @@ def _prewarm_stream_cache_worker(self, stations: list[Station]) -> None: """Worker that pre-warms stream cache.""" try: self._url_provider.prewarm_cache() + # Also warm the per-call-sign WeatherIndex cache for the displayed + # stations so the synchronous lookup in _on_play doesn't block the + # UI thread on first play. + call_signs = [station.call_sign for station in stations] + self._url_provider.prewarm_stations(call_signs) logger.debug("Pre-warmed stream cache") except Exception as e: logger.debug(f"Failed to pre-warm stream cache: {e}") @@ -182,7 +190,7 @@ def _prewarm_stream_cache_worker(self, stations: list[Station]) -> None: def _on_stations_loaded(self, stations: list[Station], choices: list[str]) -> None: """Handle stations loaded in background thread.""" # Check if dialog was closed while background thread was running - if self._station_choice is None: + if getattr(self, "_closed", False): return previous_station = self._get_selected_station() previous_call_sign = previous_station.call_sign if previous_station is not None else None @@ -242,6 +250,9 @@ def _on_play(self, _event: wx.CommandEvent) -> None: self._set_status("No station selected") return + # Stream URLs for the displayed stations are warmed in the background by + # _prewarm_stream_cache_worker after the list loads, so this lookup + # normally hits a warm per-call-sign cache instead of blocking on HTTP. urls = self._url_provider.get_stream_urls(station.call_sign) if not urls: self._set_status(f"No stream available for {station.call_sign}") @@ -385,6 +396,8 @@ def _auto_advance_stream(self) -> None: def _set_status(self, text: str) -> None: """Update the status text.""" + if getattr(self, "_closed", False): + return self._status_text.SetLabel(text) def _on_show_unavailable_changed(self, _event: wx.CommandEvent) -> None: @@ -439,6 +452,7 @@ def _on_char_hook(self, event: wx.KeyEvent) -> None: def _on_close(self, _event: wx.Event) -> None: """Handle dialog close.""" + self._closed = True self._health_timer.Stop() self._player.stop() self.Destroy() diff --git a/src/accessiweather/ui/dialogs/weather_assistant_dialog.py b/src/accessiweather/ui/dialogs/weather_assistant_dialog.py index f034c93a2..ceb6380c4 100644 --- a/src/accessiweather/ui/dialogs/weather_assistant_dialog.py +++ b/src/accessiweather/ui/dialogs/weather_assistant_dialog.py @@ -16,6 +16,7 @@ from ...ai_tools import WeatherToolExecutor, get_tools_for_message from ...screen_reader import ScreenReaderAnnouncer +from .async_guard import guard_destroyed from .weather_assistant_context import build_weather_context as _build_weather_context from .weather_assistant_prompt import SYSTEM_PROMPT from .weather_assistant_widgets import create_weather_assistant_widgets @@ -439,6 +440,7 @@ def do_generate(): thread = threading.Thread(target=do_generate, daemon=True) thread.start() + @guard_destroyed def _on_response_received(self, text: str, model_used: str) -> None: """Handle successful AI response.""" self._conversation.append({"role": "assistant", "content": text}) @@ -447,6 +449,7 @@ def _on_response_received(self, text: str, model_used: str) -> None: self._set_status(f"Model: {model_used}") self._set_generating(False) + @guard_destroyed def _on_response_error(self, error: str) -> None: """Handle AI response error.""" error_message = f"Sorry, I couldn't respond: {error}" diff --git a/src/accessiweather/ui/system_tray.py b/src/accessiweather/ui/system_tray.py index 27cfec96c..6c9b5a0ef 100644 --- a/src/accessiweather/ui/system_tray.py +++ b/src/accessiweather/ui/system_tray.py @@ -257,14 +257,25 @@ def show_main_window(self) -> None: import ctypes hwnd = frame.GetHandle() - user32 = ctypes.windll.user32 + # Use WinDLL with explicit signatures so the 64-bit HWND is + # not truncated to a c_int (see fix #698). + user32 = ctypes.WinDLL("user32", use_last_error=True) + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + user32.IsIconic.restype = ctypes.c_bool + user32.IsIconic.argtypes = [ctypes.c_void_p] + user32.ShowWindow.restype = ctypes.c_bool + user32.ShowWindow.argtypes = [ctypes.c_void_p, ctypes.c_int] + user32.AllowSetForegroundWindow.argtypes = [ctypes.c_ulong] + user32.SetForegroundWindow.restype = ctypes.c_bool + user32.SetForegroundWindow.argtypes = [ctypes.c_void_p] + kernel32.GetCurrentProcessId.restype = ctypes.c_ulong SW_RESTORE = 9 SW_SHOWNORMAL = 1 if user32.IsIconic(hwnd): user32.ShowWindow(hwnd, SW_RESTORE) else: user32.ShowWindow(hwnd, SW_SHOWNORMAL) - user32.AllowSetForegroundWindow(ctypes.windll.kernel32.GetCurrentProcessId()) + user32.AllowSetForegroundWindow(kernel32.GetCurrentProcessId()) user32.SetForegroundWindow(hwnd) except Exception: frame.Raise() diff --git a/tests/test_system_tray.py b/tests/test_system_tray.py index 66bd8f7ae..5035a08df 100644 --- a/tests/test_system_tray.py +++ b/tests/test_system_tray.py @@ -137,12 +137,15 @@ def test_show_main_window_forces_hidden_windows_visible_on_windows(self, monkeyp kernel32 = SimpleNamespace(GetCurrentProcessId=MagicMock(return_value=67890)) monkeypatch.setattr(system_tray.sys, "platform", "win32") - monkeypatch.setattr( - ctypes, - "windll", - SimpleNamespace(user32=user32, kernel32=kernel32), - raising=False, - ) + + def fake_windll(name, *args, **kwargs): + if name == "user32": + return user32 + if name == "kernel32": + return kernel32 + raise ValueError(name) + + monkeypatch.setattr(ctypes, "WinDLL", fake_windll, raising=False) tray.show_main_window() From 0c8b3ebaa9c0dd7218135c99f9a1407c9c539593 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 22:53:08 -0400 Subject: [PATCH 3/6] fix: correctness fixes across cache, forecast, notifications, updates - cache_serialization: persist discussion_issuance_time (it was read back on load but never serialized, so it was always None after a round-trip). - openmeteo_forecast_mapper: treat the daily "time" as a local calendar date instead of converting to UTC, so the weekday label and 6am/6pm period boundaries are correct (the UTC round-trip shifted the day for locations east of UTC and placed boundaries in UTC, not local). - Unify update channel vocabulary on "stable"/"nightly": the settings UI now writes "nightly" (not "dev"), config validation accepts "nightly" and migrates legacy "dev"/"beta" to it instead of resetting to stable (which silently downgraded nightly users), and the release filter accepts "nightly". Prevents nightly users from being dropped to the stable channel. - toast_notifier_windows: start the watchdog so a dead toast worker thread is detected and restarted instead of silently going dark. - alert_manager_state: compare ignored alert categories case-insensitively so a muted category like "Tornado Warning" actually suppresses. - taskbar {wind} placeholder: format speed through the unit-aware helper instead of hardcoding "mph" for metric-only sources. - forecast_confidence: compare each source's first daytime-high period rather than periods[0], so a high isn't compared against an overnight low. - soundpack wizard: bind EVT_CLOSE so the title-bar X cleans up the staging temp dir, and wrap pack creation in error handling that removes a partially written pack and reports failures. - api_client core_client: buffer the response body before the httpx client context closes (defensive against future streaming changes). Co-Authored-By: Claude Opus 4.8 --- src/accessiweather/alert_manager_state.py | 5 +- src/accessiweather/api_client/core_client.py | 3 + src/accessiweather/cache_serialization.py | 1 + src/accessiweather/forecast_confidence.py | 30 ++++++-- .../models/config_validation.py | 9 ++- .../notifications/toast_notifier_windows.py | 3 + .../openmeteo_forecast_mapper.py | 23 ++++-- .../services/update_service/releases.py | 7 +- src/accessiweather/taskbar_icon_updater.py | 11 ++- .../ui/dialogs/settings_tabs/updates.py | 6 +- .../ui/dialogs/soundpack_wizard_dialog.py | 77 +++++++++++-------- tests/test_openmeteo_mapper.py | 14 ++++ 12 files changed, 132 insertions(+), 57 deletions(-) diff --git a/src/accessiweather/alert_manager_state.py b/src/accessiweather/alert_manager_state.py index 9c1ef85b3..7c90a3ded 100644 --- a/src/accessiweather/alert_manager_state.py +++ b/src/accessiweather/alert_manager_state.py @@ -157,4 +157,7 @@ def should_notify_category(self, event: str) -> bool: """Check if we should notify for this event category.""" if not event: return True - return event.lower() not in self.ignored_categories + # Compare case-insensitively: ignored_categories is populated from + # settings/imports without case normalization, so a stored value like + # "Tornado Warning" must still match the lowercased event. + return event.lower() not in {category.lower() for category in self.ignored_categories} diff --git a/src/accessiweather/api_client/core_client.py b/src/accessiweather/api_client/core_client.py index 6c1b3a3aa..a379fc9d2 100644 --- a/src/accessiweather/api_client/core_client.py +++ b/src/accessiweather/api_client/core_client.py @@ -157,6 +157,9 @@ def _make_request( response = client.get( request_url, headers=self.headers, params=params, timeout=10 ) + # Buffer the body while the client is still open; .json() + # and .text are accessed below after the client closes. + response.read() logger.debug( f"Received response from {request_url} with status code: " f"{response.status_code}" diff --git a/src/accessiweather/cache_serialization.py b/src/accessiweather/cache_serialization.py index 3d8441a7d..2fac63ff1 100644 --- a/src/accessiweather/cache_serialization.py +++ b/src/accessiweather/cache_serialization.py @@ -436,6 +436,7 @@ def _serialize_weather_data(weather: WeatherData) -> dict: "forecast": _serialize_forecast(weather.forecast), "hourly_forecast": _serialize_hourly(weather.hourly_forecast), "discussion": weather.discussion, + "discussion_issuance_time": _serialize_datetime(weather.discussion_issuance_time), "alerts": _serialize_alerts(weather.alerts), "environmental": _serialize_environmental(weather.environmental), "trend_insights": _serialize_trends(weather.trend_insights), diff --git a/src/accessiweather/forecast_confidence.py b/src/accessiweather/forecast_confidence.py index 17b462482..d25b43352 100644 --- a/src/accessiweather/forecast_confidence.py +++ b/src/accessiweather/forecast_confidence.py @@ -71,6 +71,24 @@ def _valid_sources(sources: list[SourceData]) -> list[SourceData]: return [s for s in sources if s.success and s.forecast is not None and s.forecast.has_data()] +def _representative_period(forecast): + """ + Pick a comparable period: the first daytime high, not an overnight low. + + Sources don't agree on whether ``periods[0]`` is a daytime "Today" high or + an overnight "Tonight" low (it depends on the time of day a source is + fetched), so comparing ``periods[0]`` blindly can pit a high against a low + and produce a falsely large spread. Prefer the first period that has a + temperature and isn't named like a night period; fall back to the first + period when nothing better is found. + """ + periods = forecast.periods + for period in periods: + if period.temperature is not None and "night" not in (period.name or "").lower(): + return period + return periods[0] if periods else None + + def calculate_forecast_confidence(sources: list[SourceData]) -> ForecastConfidence: """ Compute a confidence level by comparing the first forecast period across sources. @@ -111,12 +129,12 @@ def calculate_forecast_confidence(sources: list[SourceData]) -> ForecastConfiden for s in valid: assert s.forecast is not None # already filtered above - if s.forecast.periods: - p0 = s.forecast.periods[0] - if p0.temperature is not None: - temps.append(p0.temperature) - if p0.precipitation_probability is not None: - precips.append(p0.precipitation_probability) + period = _representative_period(s.forecast) + if period is not None: + if period.temperature is not None: + temps.append(period.temperature) + if period.precipitation_probability is not None: + precips.append(period.precipitation_probability) temp_spread = (max(temps) - min(temps)) if len(temps) >= 2 else 0.0 precip_spread = (max(precips) - min(precips)) if len(precips) >= 2 else None diff --git a/src/accessiweather/models/config_validation.py b/src/accessiweather/models/config_validation.py index 2b2fb4a1d..317d702d3 100644 --- a/src/accessiweather/models/config_validation.py +++ b/src/accessiweather/models/config_validation.py @@ -70,8 +70,13 @@ def validate_on_access(self, setting_name: str) -> bool: setattr(settings, setting_name, "standard") elif setting_name == "update_channel": - valid_channels = {"stable", "beta", "dev"} - if value not in valid_channels: + # Canonical channels are "stable" and "nightly". "dev"/"beta" are + # legacy aliases for the non-stable channel — migrate them to + # "nightly" rather than resetting to stable (which would silently + # downgrade a nightly user). + if value in {"dev", "beta"}: + setattr(settings, setting_name, "nightly") + elif value not in {"stable", "nightly"}: setattr(settings, setting_name, "stable") elif setting_name == "time_display_mode": diff --git a/src/accessiweather/notifications/toast_notifier_windows.py b/src/accessiweather/notifications/toast_notifier_windows.py index 2a773b0fe..f4cc43ba6 100644 --- a/src/accessiweather/notifications/toast_notifier_windows.py +++ b/src/accessiweather/notifications/toast_notifier_windows.py @@ -74,6 +74,9 @@ def __init__( logger.info("ToastedWindowsNotifier initialized (app_id=%s)", WINDOWS_APP_USER_MODEL_ID) # Eagerly start worker thread to avoid first-notification delay self._ensure_worker() + # Start the watchdog so a dead worker thread is detected and restarted; + # without this, toasts silently stop until the app is relaunched. + self._start_watchdog() # -- worker thread management ------------------------------------------ diff --git a/src/accessiweather/openmeteo_forecast_mapper.py b/src/accessiweather/openmeteo_forecast_mapper.py index 8089dedf8..47034dc8f 100644 --- a/src/accessiweather/openmeteo_forecast_mapper.py +++ b/src/accessiweather/openmeteo_forecast_mapper.py @@ -3,7 +3,7 @@ from __future__ import annotations import logging -from datetime import UTC, datetime, timedelta +from datetime import UTC, datetime, timedelta, timezone from typing import Any from .openmeteo_client import OpenMeteoApiClient @@ -38,18 +38,25 @@ def map_forecast( for i, date_str in enumerate(dates): try: - # Parse the date - convert from local time to UTC - date_str_utc = parse_datetime(date_str, utc_offset_seconds) - if date_str_utc: - date_obj = datetime.fromisoformat(date_str_utc) - else: - # Fallback to treating as UTC if parsing fails + # Open-Meteo daily "time" values are calendar dates (e.g. + # "2026-05-28") already in the location's local timezone. Treat + # them as local dates so the weekday label and the 6am/6pm + # period boundaries are correct. Converting to UTC (as the + # hourly path does) would shift the day backwards for locations + # east of UTC and place the boundaries in UTC rather than local. + try: + date_obj = datetime.fromisoformat(date_str) + except ValueError: date_obj = datetime.fromisoformat(date_str.replace("Z", "+00:00")) + if date_obj.tzinfo is None and utc_offset_seconds is not None: + date_obj = date_obj.replace( + tzinfo=timezone(timedelta(seconds=utc_offset_seconds)) + ) # Create day and night periods (NWS style) day_period = { "number": i * 2 + 1, - "name": date_obj.strftime("%A") if i == 0 else date_obj.strftime("%A"), + "name": date_obj.strftime("%A"), "startTime": date_obj.replace(hour=6).isoformat(), "endTime": date_obj.replace(hour=18).isoformat(), "isDaytime": True, diff --git a/src/accessiweather/services/update_service/releases.py b/src/accessiweather/services/update_service/releases.py index 247189244..58ae6a0ba 100644 --- a/src/accessiweather/services/update_service/releases.py +++ b/src/accessiweather/services/update_service/releases.py @@ -19,7 +19,8 @@ GITHUB_API_URL = "https://api.github.com/repos/{owner}/{repo}/releases" CACHE_EXPIRY_SECONDS = 3600 # 1 hour -VALID_CHANNELS = {"stable", "dev"} +# "nightly" is the canonical non-stable channel; "dev" is kept as a legacy alias. +VALID_CHANNELS = {"stable", "nightly", "dev"} class ReleaseManager: @@ -198,8 +199,8 @@ def filter_releases_by_channel( if not prerelease and not is_nightly: filtered.append(release) - elif channel == "dev": - # Dev: all releases + else: + # Non-stable ("nightly"/"dev"): all releases filtered.append(release) return filtered diff --git a/src/accessiweather/taskbar_icon_updater.py b/src/accessiweather/taskbar_icon_updater.py index ed53b7b39..928f484e4 100644 --- a/src/accessiweather/taskbar_icon_updater.py +++ b/src/accessiweather/taskbar_icon_updater.py @@ -277,17 +277,16 @@ def _format_wind_direction(self, direction: Any) -> str: def _format_wind(self, current: Any) -> str: """Format wind direction and speed.""" direction = getattr(current, "wind_direction", None) - speed = getattr(current, "wind_speed", None) - - if direction is None and speed is None: - return PLACEHOLDER_NA + # Format the speed through the unit-aware helper rather than assuming + # mph: wind_speed may carry km/h for metric-only sources. + speed_str = self._format_wind_speed(current) parts = [] formatted_direction = self._format_wind_direction(direction) if formatted_direction != PLACEHOLDER_NA: parts.append(formatted_direction) - if speed is not None: - parts.append(f"at {speed:.0f} mph") + if speed_str != PLACEHOLDER_NA: + parts.append(f"at {speed_str}") return " ".join(parts) if parts else PLACEHOLDER_NA diff --git a/src/accessiweather/ui/dialogs/settings_tabs/updates.py b/src/accessiweather/ui/dialogs/settings_tabs/updates.py index c7a2055c8..aea4054cd 100644 --- a/src/accessiweather/ui/dialogs/settings_tabs/updates.py +++ b/src/accessiweather/ui/dialogs/settings_tabs/updates.py @@ -101,7 +101,11 @@ def save(self) -> dict: controls = self.dialog._controls return { "auto_update_enabled": controls["auto_update"].GetValue(), - "update_channel": "stable" if controls["update_channel"].GetSelection() == 0 else "dev", + # The canonical non-stable channel is "nightly" (matches the build + # migration default, select_latest_release, and the startup guard). + "update_channel": ( + "stable" if controls["update_channel"].GetSelection() == 0 else "nightly" + ), "update_check_interval_hours": controls["update_check_interval"].GetValue(), } diff --git a/src/accessiweather/ui/dialogs/soundpack_wizard_dialog.py b/src/accessiweather/ui/dialogs/soundpack_wizard_dialog.py index 5a1047e40..9f72e0397 100644 --- a/src/accessiweather/ui/dialogs/soundpack_wizard_dialog.py +++ b/src/accessiweather/ui/dialogs/soundpack_wizard_dialog.py @@ -42,6 +42,9 @@ def __init__(self, parent: wx.Window, soundpacks_dir: Path) -> None: self._create_ui() self.Bind(wx.EVT_CHAR_HOOK, self._on_char_hook) + # Route the title-bar close button through the same cancel/cleanup path + # so the staging temp directory isn't leaked when closed via the "X". + self.Bind(wx.EVT_CLOSE, self._on_close) self._render_step() self.Centre() @@ -421,37 +424,48 @@ def _create_pack(self) -> None: suffix += 1 pack_dir = self.soundpacks_dir / pack_id - pack_dir.mkdir(parents=True) - - # Copy sound files - sounds_mapping = {} - for key, src_path_str in self.state.sound_mappings.items(): - if not src_path_str: - continue - src_path = Path(src_path_str) - if src_path.exists(): - dest = pack_dir / src_path.name - shutil.copy2(src_path, dest) - sounds_mapping[key] = src_path.name - - # Create pack.json - pack_data = { - "name": self.state.pack_name, - "author": self.state.author or "Unknown", - "description": self.state.description, - "version": "1.0.0", - "sounds": sounds_mapping, - } - pack_json = pack_dir / "pack.json" - with open(pack_json, "w", encoding="utf-8") as f: - json.dump(pack_data, f, indent=2) + try: + pack_dir.mkdir(parents=True) + + # Copy sound files + sounds_mapping = {} + for key, src_path_str in self.state.sound_mappings.items(): + if not src_path_str: + continue + src_path = Path(src_path_str) + if src_path.exists(): + dest = pack_dir / src_path.name + shutil.copy2(src_path, dest) + sounds_mapping[key] = src_path.name + + # Create pack.json + pack_data = { + "name": self.state.pack_name, + "author": self.state.author or "Unknown", + "description": self.state.description, + "version": "1.0.0", + "sounds": sounds_mapping, + } + + pack_json = pack_dir / "pack.json" + with open(pack_json, "w", encoding="utf-8") as f: + json.dump(pack_data, f, indent=2) + except OSError as exc: + logger.error("Failed to create sound pack %s: %s", pack_id, exc) + # Remove the partially written pack directory so it isn't left behind. + with contextlib.suppress(Exception): + shutil.rmtree(pack_dir) + wx.MessageBox( + f"Failed to create sound pack:\n{exc}", + "Pack Creation Failed", + wx.OK | wx.ICON_ERROR, + ) + return self.created_pack_id = pack_id - # Clean up staging directory - with contextlib.suppress(Exception): - shutil.rmtree(self.staging_dir) + self._cleanup_staging() wx.MessageBox( f"Sound pack '{self.state.pack_name}' created successfully!", @@ -461,6 +475,11 @@ def _create_pack(self) -> None: self.EndModal(wx.ID_OK) + def _cleanup_staging(self) -> None: + """Remove the temporary staging directory if it still exists.""" + with contextlib.suppress(Exception): + shutil.rmtree(self.staging_dir) + def _on_char_hook(self, event: wx.KeyEvent) -> None: """Handle keyboard shortcuts for the dialog.""" if event.GetKeyCode() == wx.WXK_ESCAPE: @@ -492,9 +511,7 @@ def _on_cancel(self, event) -> None: if result != wx.YES: return - # Clean up staging directory - with contextlib.suppress(Exception): - shutil.rmtree(self.staging_dir) + self._cleanup_staging() self.EndModal(wx.ID_CANCEL) diff --git a/tests/test_openmeteo_mapper.py b/tests/test_openmeteo_mapper.py index 4629e8904..9fb4ff452 100644 --- a/tests/test_openmeteo_mapper.py +++ b/tests/test_openmeteo_mapper.py @@ -101,6 +101,20 @@ def test_missing_night_temperature_leaves_day_low_unset(self): assert "temperature_low" not in periods[0] + def test_daily_weekday_uses_local_date_for_eastern_hemisphere(self): + # 2026-03-19 is a Thursday. With a +10h offset (e.g. Australia), naive + # local midnight converts to the previous day in UTC; the period name + # must still reflect the local calendar day, not the UTC-shifted one. + payload = _make_daily_payload(["2026-03-19"], [54.0], [39.0]) + payload["utc_offset_seconds"] = 10 * 3600 + + periods = OpenMeteoMapper().map_forecast(payload)["properties"]["periods"] + + assert periods[0]["name"] == "Thursday" + assert periods[1]["name"] == "Thursday Night" + # Day period should start at 6am local (not in UTC). + assert periods[0]["startTime"].startswith("2026-03-19T06:00:00") + class TestOpenMeteoMapperHourlyFields: def test_hourly_mapping_includes_humidity_and_dewpoint(self): From c327c2d14c91f048bdc6f6f804d4985d1a1f0fc1 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 22:57:08 -0400 Subject: [PATCH 4/6] fix: low-severity cleanup (security, crash guards, correctness) - config/locations: reject out-of-range/non-numeric coordinates in add_location and add_location_with_enrichment (CLAUDE.md requirement). - config/settings: redact github_app_id and github_app_installation_id in logs (they're stored as secrets but were logged in cleartext). - community_soundpack_service: guard repo-pack downloads against path traversal / zip-slip by ensuring each tree path stays inside staging. - impact_summary: guard against NaN reference temperature (next() over the temperature bands would raise StopIteration and crash the summary). - taskbar tooltip: only blank on the parser's "Error:" prefix, not any occurrence of "Error", so legitimate text isn't discarded. - alert_manager: actually call _reset_hourly_counter so notifications_this_hour is a per-hour count rather than a lifetime total. - discussion_dialog: shut the screen-reader announcer down on close to match the weather assistant dialog and avoid leaking its backend. Co-Authored-By: Claude Opus 4.8 --- src/accessiweather/alert_manager.py | 3 +++ src/accessiweather/config/locations.py | 21 +++++++++++++++++++ src/accessiweather/config/settings.py | 4 +++- src/accessiweather/impact_summary.py | 3 ++- .../services/community_soundpack_service.py | 5 +++++ src/accessiweather/taskbar_icon_updater.py | 5 ++++- .../ui/dialogs/discussion_dialog.py | 3 +++ 7 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/accessiweather/alert_manager.py b/src/accessiweather/alert_manager.py index f2e0c900e..ee5d304a7 100644 --- a/src/accessiweather/alert_manager.py +++ b/src/accessiweather/alert_manager.py @@ -425,6 +425,9 @@ def process_alerts(self, alerts: WeatherAlerts) -> list[tuple[WeatherAlert, str] # Update global notification time if we're sending any notifications if notifications_to_send: self.last_global_notification = current_time + # Roll the hourly counter over at the hour boundary so it reflects + # notifications in the current hour, not a lifetime total. + self._reset_hourly_counter() self.notifications_this_hour += len(notifications_to_send) # Save state changes - always save to persist any alert state modifications diff --git a/src/accessiweather/config/locations.py b/src/accessiweather/config/locations.py index 1b2165eda..f18605752 100644 --- a/src/accessiweather/config/locations.py +++ b/src/accessiweather/config/locations.py @@ -43,6 +43,21 @@ def __init__( def logger(self) -> logging.Logger: return self._manager._get_logger() + def _valid_coordinates(self, name: str, latitude: float, longitude: float) -> bool: + """Return True if the coordinates are within valid geographic bounds.""" + try: + lat = float(latitude) + lon = float(longitude) + except (TypeError, ValueError): + self.logger.warning(f"Location {name} has non-numeric coordinates; rejecting") + return False + if not (-90.0 <= lat <= 90.0 and -180.0 <= lon <= 180.0): + self.logger.warning( + f"Location {name} has out-of-range coordinates ({latitude}, {longitude}); rejecting" + ) + return False + return True + def add_location( self, name: str, @@ -52,6 +67,9 @@ def add_location( marine_mode: bool = False, ) -> bool: """Add a new location if it doesn't already exist.""" + if not self._valid_coordinates(name, latitude, longitude): + return False + config = self._manager.get_config() for existing_location in config.locations: @@ -95,6 +113,9 @@ async def add_location_with_enrichment( those cases the zone fields remain null and the location is still saved. """ + if not self._valid_coordinates(name, latitude, longitude): + return False + config = self._manager.get_config() for existing_location in config.locations: diff --git a/src/accessiweather/config/settings.py b/src/accessiweather/config/settings.py index f279deb70..c90cbbe2c 100644 --- a/src/accessiweather/config/settings.py +++ b/src/accessiweather/config/settings.py @@ -187,9 +187,11 @@ def update_settings(self, **kwargs) -> bool: } if is_portable: secure_keys -= portable_api_keys - # These keys should be redacted in logs + # These keys should be redacted in logs (everything stored as a secret) redacted_keys = { "github_app_private_key", + "github_app_id", + "github_app_installation_id", "pirate_weather_api_key", "openrouter_api_key", "avwx_api_key", diff --git a/src/accessiweather/impact_summary.py b/src/accessiweather/impact_summary.py index fb0142925..15add8c56 100644 --- a/src/accessiweather/impact_summary.py +++ b/src/accessiweather/impact_summary.py @@ -17,6 +17,7 @@ from __future__ import annotations +import math import re from dataclasses import dataclass from typing import TYPE_CHECKING @@ -76,7 +77,7 @@ def _outdoor_from_conditions( 3. Appends precipitation warning when condition text implies active precipitation. """ ref_temp = feels_like_f if feels_like_f is not None else temp_f - if ref_temp is None: + if ref_temp is None or not math.isfinite(ref_temp): return None comfort = next(label for upper, label in _OUTDOOR_TEMP_BANDS if ref_temp < upper) diff --git a/src/accessiweather/services/community_soundpack_service.py b/src/accessiweather/services/community_soundpack_service.py index cbb3bd1eb..65cad4368 100644 --- a/src/accessiweather/services/community_soundpack_service.py +++ b/src/accessiweather/services/community_soundpack_service.py @@ -422,6 +422,11 @@ async def _download_repo_pack( continue rel_path = item.get("path") or "" target_path = staging_dir / rel_path + # Defense-in-depth: ensure a malicious/unexpected tree path can't + # escape the staging directory (path traversal / zip-slip). + staging_root = staging_dir.resolve() + if not target_path.resolve().is_relative_to(staging_root): + raise RuntimeError(f"Unsafe path in repository tree: {rel_path!r}") target_path.parent.mkdir(parents=True, exist_ok=True) raw_url = ( f"https://raw.githubusercontent.com/{self.repo_owner}/{self.repo_name}/" diff --git a/src/accessiweather/taskbar_icon_updater.py b/src/accessiweather/taskbar_icon_updater.py index 928f484e4..859fefe21 100644 --- a/src/accessiweather/taskbar_icon_updater.py +++ b/src/accessiweather/taskbar_icon_updater.py @@ -463,7 +463,10 @@ def _format_with_fallback(self, format_string: str, data: dict[str, str]) -> str result = self.parser.format_string(format_string, data) - if "Error" in result: + # The parser signals failure with a string starting with "Error:". + # Match the prefix so a legitimate value containing the word "error" + # doesn't blank the tooltip. + if result.startswith("Error:"): logger.warning("Format error: %s", result) return DEFAULT_TOOLTIP_TEXT diff --git a/src/accessiweather/ui/dialogs/discussion_dialog.py b/src/accessiweather/ui/dialogs/discussion_dialog.py index 4946a6000..a7a8ebc34 100644 --- a/src/accessiweather/ui/dialogs/discussion_dialog.py +++ b/src/accessiweather/ui/dialogs/discussion_dialog.py @@ -479,6 +479,9 @@ def _on_key(self, event): def _on_close(self, event) -> None: """Handle close button press.""" + announcer = getattr(self, "_announcer", None) + if announcer is not None: + announcer.shutdown() self.EndModal(wx.ID_CLOSE) From 1ded2ec94472660ddf93df340cf96f1d58dc4c61 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 23:10:28 -0400 Subject: [PATCH 5/6] fix: finish bug-hunt reliability cleanup --- CHANGELOG.md | 1 + src/accessiweather/taskbar_icon_updater.py | 4 ++ src/accessiweather/utils/unit_utils.py | 47 ++++++++++++---------- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 163d4e395..40c2e5c0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ All notable changes to this project will be documented in this file. - Alert sounds and sound-pack creation now focus on alert severity instead of a long list of specific alert names, while existing packs can keep their older mappings. ### Fixed +- Reliability fixes now prevent several update, notification, tray tooltip, dialog, NOAA radio, and alert-refresh edge cases from interrupting normal use. - Detected current locations outside the US now get an editable place name when reverse geocoding can identify the coordinates. - Editing a location after using "Use my current location" now refreshes the editable name and saved US metadata for the detected point, so locations don't quietly fall back to metric defaults. - Notification sounds now recover after Windows sleep or hibernation, so update diff --git a/src/accessiweather/taskbar_icon_updater.py b/src/accessiweather/taskbar_icon_updater.py index 859fefe21..36cbf3cdf 100644 --- a/src/accessiweather/taskbar_icon_updater.py +++ b/src/accessiweather/taskbar_icon_updater.py @@ -287,6 +287,10 @@ def _format_wind(self, current: Any) -> str: parts.append(formatted_direction) if speed_str != PLACEHOLDER_NA: parts.append(f"at {speed_str}") + else: + legacy_speed = getattr(current, "wind_speed", None) + if legacy_speed is not None: + parts.append(f"at {legacy_speed:.0f} mph") return " ".join(parts) if parts else PLACEHOLDER_NA diff --git a/src/accessiweather/utils/unit_utils.py b/src/accessiweather/utils/unit_utils.py index e3de86150..12ba757ff 100644 --- a/src/accessiweather/utils/unit_utils.py +++ b/src/accessiweather/utils/unit_utils.py @@ -5,25 +5,30 @@ copied from the wx version for consistency. """ +from __future__ import annotations + import logging +from typing import TYPE_CHECKING -from ..units import DisplayUnitSystem from .temperature_utils import TemperatureUnit +if TYPE_CHECKING: + from ..units import DisplayUnitSystem + logger = logging.getLogger(__name__) -def _normalize_unit_system(unit_system: DisplayUnitSystem | str | None) -> DisplayUnitSystem | None: +def _normalize_unit_system(unit_system: DisplayUnitSystem | str | None) -> str | None: """Normalize optional unit-system hints.""" - if isinstance(unit_system, DisplayUnitSystem): - return unit_system - if isinstance(unit_system, str): - normalized = unit_system.strip().lower() - if normalized == "uk2": - normalized = "uk" - for candidate in DisplayUnitSystem: - if candidate.value == normalized: - return candidate + value = getattr(unit_system, "value", unit_system) + if not isinstance(value, str): + return None + + normalized = value.strip().lower() + if normalized == "uk2": + normalized = "uk" + if normalized in {"us", "uk", "ca", "si"}: + return normalized return None @@ -62,13 +67,13 @@ def format_wind_speed( assert wind_speed_kph is not None normalized_system = _normalize_unit_system(unit_system) - if normalized_system == DisplayUnitSystem.US: + if normalized_system == "us": return f"{wind_speed_mph:.{precision}f} mph" - if normalized_system == DisplayUnitSystem.UK: + if normalized_system == "uk": return f"{wind_speed_mph:.{precision}f} mph" - if normalized_system == DisplayUnitSystem.CA: + if normalized_system == "ca": return f"{wind_speed_kph:.{precision}f} km/h" - if normalized_system == DisplayUnitSystem.SI: + if normalized_system == "si": wind_speed_mps = wind_speed_kph / 3.6 return f"{wind_speed_mps:.{precision}f} m/s" @@ -114,9 +119,9 @@ def format_pressure( pressure_mb = pressure_inhg * 33.8639 normalized_system = _normalize_unit_system(unit_system) - if normalized_system == DisplayUnitSystem.US: + if normalized_system == "us": return f"{pressure_inhg:.{precision}f} inHg" - if normalized_system in {DisplayUnitSystem.UK, DisplayUnitSystem.CA, DisplayUnitSystem.SI}: + if normalized_system in {"uk", "ca", "si"}: return f"{pressure_mb:.{precision}f} hPa" # Format based on user preference @@ -161,9 +166,9 @@ def format_visibility( visibility_km = visibility_miles * 1.60934 normalized_system = _normalize_unit_system(unit_system) - if normalized_system in {DisplayUnitSystem.US, DisplayUnitSystem.UK}: + if normalized_system in {"us", "uk"}: return f"{visibility_miles:.{precision}f} mi" - if normalized_system in {DisplayUnitSystem.CA, DisplayUnitSystem.SI}: + if normalized_system in {"ca", "si"}: return f"{visibility_km:.{precision}f} km" # Format based on user preference @@ -208,9 +213,9 @@ def format_precipitation( precipitation_mm = precipitation_inches * 25.4 normalized_system = _normalize_unit_system(unit_system) - if normalized_system == DisplayUnitSystem.US: + if normalized_system == "us": return f"{precipitation_inches:.{precision}f} in" - if normalized_system in {DisplayUnitSystem.UK, DisplayUnitSystem.CA, DisplayUnitSystem.SI}: + if normalized_system in {"uk", "ca", "si"}: return f"{precipitation_mm:.{precision}f} mm" # Format based on user preference From fc3ba5dcee9a6ef6a59cfde883b393e3dad111e6 Mon Sep 17 00:00:00 2001 From: Orinks Date: Thu, 28 May 2026 23:30:22 -0400 Subject: [PATCH 6/6] test: cover bug-hunt reliability paths --- tests/test_community_soundpack_service.py | 38 ++++++++ tests/test_config_manager.py | 7 ++ tests/test_forecast_confidence.py | 12 +++ tests/test_models.py | 11 +++ tests/test_noaa_radio_stream_url.py | 8 ++ tests/test_openmeteo_mapper.py | 7 ++ tests/test_unit_utils.py | 6 ++ tests/test_update_checksum_verification.py | 105 +++++++++++++++++++++ tests/test_zone_enrichment_service.py | 14 +++ 9 files changed, 208 insertions(+) create mode 100644 tests/test_community_soundpack_service.py diff --git a/tests/test_community_soundpack_service.py b/tests/test_community_soundpack_service.py new file mode 100644 index 000000000..db90a39ac --- /dev/null +++ b/tests/test_community_soundpack_service.py @@ -0,0 +1,38 @@ +"""Tests for community sound pack download hardening.""" + +from __future__ import annotations + +from unittest.mock import AsyncMock + +import pytest + +from accessiweather.services.community_soundpack_models import CommunityPack +from accessiweather.services.community_soundpack_service import CommunitySoundPackService + + +def _repo_pack() -> CommunityPack: + return CommunityPack( + name="Example", + author="Tester", + description="Test pack", + version="1.0", + download_url="", + file_size=None, + repository_url="https://github.com/example/repo", + release_tag="", + repo_path="packs/example", + ) + + +@pytest.mark.asyncio +async def test_download_repo_pack_rejects_path_traversal(tmp_path): + service = CommunitySoundPackService(repo_owner="example", repo_name="repo") + service._fetch_tree_entries = AsyncMock( + return_value=[{"type": "blob", "path": "../escape.wav", "size": 4}] + ) + + try: + with pytest.raises(RuntimeError, match="Unsafe path"): + await service._download_repo_pack(_repo_pack(), tmp_path / "pack.zip", None) + finally: + await service.aclose() diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 9b91362dc..361ddddd9 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -121,6 +121,13 @@ def test_add_location(self, manager): assert len(locations) == 1 assert locations[0].name == "New York" + def test_add_location_rejects_non_numeric_coordinates(self, manager): + """Invalid coordinate values are rejected before saving.""" + result = manager.add_location("Bad", "north", -74.0060) # type: ignore[arg-type] + + assert result is False + assert manager.get_all_locations() == [] + def test_add_location_persists_marine_mode_roundtrip(self, manager): """Marine mode should save and reload with locations.""" result = manager.add_location( diff --git a/tests/test_forecast_confidence.py b/tests/test_forecast_confidence.py index 58a8c5b7d..88103ccb0 100644 --- a/tests/test_forecast_confidence.py +++ b/tests/test_forecast_confidence.py @@ -80,6 +80,18 @@ def test_empty_forecast_treated_as_invalid(self): result = calculate_forecast_confidence([make_empty_forecast_source()]) assert result.level == ForecastConfidenceLevel.LOW + def test_night_only_forecast_falls_back_to_first_period(self): + source = SourceData( + source="nws", + forecast=Forecast(periods=[ForecastPeriod(name="Tonight", temperature=62.0)]), + success=True, + ) + + result = calculate_forecast_confidence([source]) + + assert result.level == ForecastConfidenceLevel.MEDIUM + assert result.sources_compared == 1 + def test_single_valid_source_returns_medium(self): result = calculate_forecast_confidence([make_source(72.0)]) assert result.level == ForecastConfidenceLevel.MEDIUM diff --git a/tests/test_models.py b/tests/test_models.py index c5504c0df..7e811c673 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -8,6 +8,8 @@ from datetime import UTC, datetime, timedelta +import pytest + from accessiweather.models import ( AppConfig, AppSettings, @@ -456,6 +458,15 @@ def test_forecast_time_reference_validation(self): assert settings.validate_on_access("forecast_time_reference") is True assert settings.forecast_time_reference == "location" + @pytest.mark.parametrize("legacy_channel", ["dev", "beta"]) + def test_update_channel_legacy_aliases_migrate_to_nightly(self, legacy_channel): + """Legacy non-stable update channels should remain on the nightly track.""" + settings = AppSettings() + settings.update_channel = legacy_channel + + assert settings.validate_on_access("update_channel") is True + assert settings.update_channel == "nightly" + def test_forecast_duration_days_validation(self): """Ensure invalid forecast duration values fall back to default.""" settings = AppSettings() diff --git a/tests/test_noaa_radio_stream_url.py b/tests/test_noaa_radio_stream_url.py index eb8a0814c..83beb80a6 100644 --- a/tests/test_noaa_radio_stream_url.py +++ b/tests/test_noaa_radio_stream_url.py @@ -165,3 +165,11 @@ def test_generated_fallback_is_last_when_enabled(self) -> None: assert provider.get_stream_urls("UNKNOWN99") == [ "https://broadcastify.cdnstream1.com/noaa/UNKNOWN99" ] + + def test_prewarm_stations_fetches_each_station_and_suppresses_errors(self) -> None: + provider = StreamURLProvider(use_fallback=False) + provider.get_stream_urls = MagicMock(side_effect=[["https://example.com/live"], OSError]) + + provider.prewarm_stations(["WXJ76", "BAD"]) + + assert provider.get_stream_urls.call_count == 2 diff --git a/tests/test_openmeteo_mapper.py b/tests/test_openmeteo_mapper.py index 9fb4ff452..ac3774666 100644 --- a/tests/test_openmeteo_mapper.py +++ b/tests/test_openmeteo_mapper.py @@ -115,6 +115,13 @@ def test_daily_weekday_uses_local_date_for_eastern_hemisphere(self): # Day period should start at 6am local (not in UTC). assert periods[0]["startTime"].startswith("2026-03-19T06:00:00") + def test_daily_date_with_z_suffix_uses_value_error_fallback(self): + payload = _make_daily_payload(["2026-03-19Z"], [54.0], [39.0]) + + periods = OpenMeteoMapper().map_forecast(payload)["properties"]["periods"] + + assert periods[0]["name"] == "Thursday" + class TestOpenMeteoMapperHourlyFields: def test_hourly_mapping_includes_humidity_and_dewpoint(self): diff --git a/tests/test_unit_utils.py b/tests/test_unit_utils.py index fd6e4d1cf..d7e883f48 100644 --- a/tests/test_unit_utils.py +++ b/tests/test_unit_utils.py @@ -58,6 +58,12 @@ def test_format_wind_speed_precision(self) -> None: result = format_wind_speed(5.556, unit=TemperatureUnit.FAHRENHEIT, precision=2) assert result == "5.56 mph" + def test_format_wind_speed_accepts_legacy_uk2_unit_system(self) -> None: + """Legacy UK2 unit-system values should behave like UK.""" + result = format_wind_speed(10.0, wind_speed_kph=16.0934, unit_system="uk2") + + assert result == "10.0 mph" + class TestFormatPressure: """Tests for pressure formatting.""" diff --git a/tests/test_update_checksum_verification.py b/tests/test_update_checksum_verification.py index 4fa6ce0d7..4d5cb1788 100644 --- a/tests/test_update_checksum_verification.py +++ b/tests/test_update_checksum_verification.py @@ -10,6 +10,7 @@ import hashlib from unittest.mock import AsyncMock, MagicMock +import httpx import pytest from accessiweather.services.simple_update import ( @@ -61,6 +62,33 @@ def test_find_checksum_asset_no_match(): assert find_checksum_asset(release, "app.zip") is None +def _mock_update_stream(content: bytes) -> MagicMock: + response = MagicMock() + response.headers = {"content-length": str(len(content))} + response.raise_for_status = MagicMock() + + async def aiter_bytes(chunk_size=None): + yield content + + response.aiter_bytes = aiter_bytes + response.__aenter__ = AsyncMock(return_value=response) + response.__aexit__ = AsyncMock(return_value=None) + return response + + +def _update_info_with_release(release: dict | None = None) -> UpdateInfo: + return UpdateInfo( + version="1.0.0", + download_url="https://example.com/update.zip", + artifact_name="update.zip", + release_notes="", + commit_hash=None, + is_nightly=False, + is_prerelease=False, + release=release, + ) + + def test_find_checksum_asset_generic_checksums_file(): release = { "assets": [ @@ -234,3 +262,80 @@ async def mock_aiter_bytes(chunk_size=None): result = await service.download_update(update_info, tmp_path, release=release) assert result.exists() assert result.read_bytes() == artifact_content + + +@pytest.mark.asyncio +async def test_download_update_allows_release_without_checksum_asset(tmp_path): + artifact_content = b"valid artifact content" + release = {"assets": [{"name": "update.zip", "browser_download_url": "https://example.com/u"}]} + mock_client = MagicMock() + mock_client.stream = MagicMock(return_value=_mock_update_stream(artifact_content)) + + service = UpdateService("TestApp", http_client=mock_client) + + result = await service.download_update(_update_info_with_release(release), tmp_path) + + assert result.exists() + assert result.read_bytes() == artifact_content + + +@pytest.mark.asyncio +async def test_download_update_rejects_checksum_asset_without_url(tmp_path): + release = {"assets": [{"name": "update.zip.sha256"}]} + mock_client = MagicMock() + mock_client.stream = MagicMock(return_value=_mock_update_stream(b"artifact")) + + service = UpdateService("TestApp", http_client=mock_client) + + with pytest.raises(ChecksumVerificationError, match="no download URL"): + await service.download_update(_update_info_with_release(release), tmp_path) + + assert not (tmp_path / "update.zip").exists() + + +@pytest.mark.asyncio +async def test_download_update_rejects_checksum_download_failure(tmp_path): + release = { + "assets": [ + { + "name": "update.zip.sha256", + "browser_download_url": "https://example.com/update.zip.sha256", + } + ] + } + mock_client = MagicMock() + mock_client.stream = MagicMock(return_value=_mock_update_stream(b"artifact")) + mock_client.get = AsyncMock(side_effect=httpx.ConnectError("no net")) + + service = UpdateService("TestApp", http_client=mock_client) + + with pytest.raises(ChecksumVerificationError, match="Failed to download checksum"): + await service.download_update(_update_info_with_release(release), tmp_path) + + assert not (tmp_path / "update.zip").exists() + + +@pytest.mark.asyncio +async def test_download_update_rejects_checksum_file_without_artifact_entry(tmp_path): + release = { + "assets": [ + { + "name": "update.zip.sha256", + "browser_download_url": "https://example.com/update.zip.sha256", + } + ] + } + checksum_response = MagicMock() + checksum_response.raise_for_status = MagicMock() + checksum_response.text = f"{hashlib.sha256(b'other').hexdigest()} other.zip" + + mock_client = MagicMock() + mock_client.stream = MagicMock(return_value=_mock_update_stream(b"artifact")) + mock_client.get = AsyncMock(return_value=checksum_response) + + service = UpdateService("TestApp", http_client=mock_client) + + with pytest.raises(ChecksumVerificationError, match="Could not find a checksum"): + await service.download_update(_update_info_with_release(release), tmp_path) + + assert not (tmp_path / "update.zip").exists() diff --git a/tests/test_zone_enrichment_service.py b/tests/test_zone_enrichment_service.py index f21531d85..5d4578bc7 100644 --- a/tests/test_zone_enrichment_service.py +++ b/tests/test_zone_enrichment_service.py @@ -422,6 +422,20 @@ async def test_duplicate_name_rejected_without_calling_service(self): mock_service.enrich_location.assert_not_awaited() assert manager.save_calls == 0 + async def test_invalid_coordinates_rejected_without_calling_service(self): + mock_service = MagicMock(spec=ZoneEnrichmentService) + mock_service.enrich_location = AsyncMock(side_effect=lambda loc: loc) + + manager = _FakeConfigManager() + ops = LocationOperations(cast(ConfigManager, manager), zone_enrichment_service=mock_service) + + ok = await ops.add_location_with_enrichment("Bad", 91.0, -75.16, country_code="US") + + assert ok is False + mock_service.enrich_location.assert_not_awaited() + assert manager.get_config().locations == [] + assert manager.save_calls == 0 + class TestNoModalDialog: """Scenario 6: save flow never spawns a modal dialog on /points failure."""