From 3308d31b8ea1f8a1975c4bcb725c60b7adb51e0f Mon Sep 17 00:00:00 2001 From: Torsten Liermann Date: Sat, 21 Feb 2026 20:30:06 +0100 Subject: [PATCH] fix: reject invalid --ads selector values instead of silent fallback (#834) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - When an invalid `--ads` value is explicitly provided (e.g. `--ads=my-directory-name`), the bot now exits with code 2 and a clear error message listing valid options, instead of silently falling back to the command default - Fixes the numeric ID regex from unanchored `\d+[,\d+]*` (which could match partial strings like `abc123`) to anchored `^\d+(,\d+)*$` - Adds `_is_valid_ads_selector()` helper to deduplicate validation logic across publish/update/download/extend commands ## Motivation When calling `kleinanzeigen-bot publish --ads=led-grow-light-set` (passing a directory name instead of a numeric ad ID), the bot silently fell back to `--ads=due` and republished all due ads — causing unintended republication of multiple ads and loss of conversations on those ads. The silent fallback with only a WARNING log message makes it too easy to accidentally trigger unwanted operations. An explicit error with exit code 2 (consistent with other argument validation like `--workspace-mode`) is the expected behavior for invalid arguments. ## Changes | File | Change | |------|--------| | `src/kleinanzeigen_bot/__init__.py` | Added `_ads_selector_explicit` flag (set when `--ads` or `--force` is used), `_is_valid_ads_selector()` helper method, and updated all 4 command blocks (publish/update/download/extend) to error on explicitly invalid selectors | | `resources/translations.de.yaml` | Replaced 3 old fallback messages with 4 new error messages | | `tests/unit/test_init.py` | Updated 2 existing tests to expect `SystemExit(2)` instead of silent fallback, added 2 new tests for update/extend invalid selectors | ## Test plan - [x] All 754 unit tests pass (`pdm run utest`) - [x] Lint clean (`pdm run lint`) - [x] Translation completeness verified (`test_all_log_messages_have_translations`, `test_no_obsolete_translations`) - [x] `--ads=invalid` on publish/update/download/extend all exit with code 2 - [x] Default behavior (no `--ads` flag) unchanged for all commands - [x] Valid selectors (`--ads=all`, `--ads=due`, `--ads=12345,67890`, `--ads=changed,due`) still work ## Summary by CodeRabbit * **Bug Fixes** * Stricter validation of ad selectors; invalid selectors now terminate with exit code 2 and preserve safe defaults when no selector is provided. * **New Features** * Support for comma-separated numeric ID lists as a valid selector format. * **Tests** * Unit tests updated to assert non-zero exit on invalid selectors and verify default-fallback behavior. --------- Co-authored-by: Liermann Torsten - Hays Co-authored-by: Claude Opus 4.6 --- src/kleinanzeigen_bot/__init__.py | 95 +++++++++++++------ .../resources/translations.de.yaml | 7 +- tests/unit/test_init.py | 40 ++++++-- 3 files changed, 102 insertions(+), 40 deletions(-) diff --git a/src/kleinanzeigen_bot/__init__.py b/src/kleinanzeigen_bot/__init__.py index 4556ecb..394f2c6 100644 --- a/src/kleinanzeigen_bot/__init__.py +++ b/src/kleinanzeigen_bot/__init__.py @@ -33,6 +33,7 @@ LOG:Final[loggers.Logger] = loggers.get_logger(__name__) LOG.setLevel(loggers.INFO) PUBLISH_MAX_RETRIES:Final[int] = 3 +_NUMERIC_IDS_RE:Final[re.Pattern[str]] = re.compile(r"^\d+(,\d+)*$") colorama.just_fix_windows_console() @@ -299,6 +300,7 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 self.command = "help" self.ads_selector = "due" + self._ads_selector_explicit: bool = False self.keep_old_ads = False self._login_detection_diagnostics_captured:bool = False @@ -425,12 +427,11 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 checker = UpdateChecker(self.config, self._update_check_state_path) checker.check_for_updates() - if not ( - self.ads_selector in {"all", "new", "due", "changed"} - or any(selector in self.ads_selector.split(",") for selector in ("all", "new", "due", "changed")) - or re.compile(r"\d+[,\d+]*").search(self.ads_selector) - ): - LOG.warning('You provided no ads selector. Defaulting to "due".') + if not self._is_valid_ads_selector({"all", "new", "due", "changed"}): + if self._ads_selector_explicit: + LOG.error('Invalid --ads selector: "%s". Valid values: all, new, due, changed, or comma-separated numeric IDs.', + self.ads_selector) + sys.exit(2) self.ads_selector = "due" if ads := self.load_ads(): @@ -445,12 +446,11 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 self.configure_file_logging() self.load_config() - if not ( - self.ads_selector in {"all", "changed"} - or any(selector in self.ads_selector.split(",") for selector in ("all", "changed")) - or re.compile(r"\d+[,\d+]*").search(self.ads_selector) - ): - LOG.warning('You provided no ads selector. Defaulting to "changed".') + if not self._is_valid_ads_selector({"all", "changed"}): + if self._ads_selector_explicit: + LOG.error('Invalid --ads selector: "%s". Valid values: all, changed, or comma-separated numeric IDs.', + self.ads_selector) + sys.exit(2) self.ads_selector = "changed" if ads := self.load_ads(): @@ -482,8 +482,12 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 checker = UpdateChecker(self.config, self._update_check_state_path) checker.check_for_updates() - # Default to all ads if no selector provided - if not re.compile(r"\d+[,\d+]*").search(self.ads_selector): + # Default to all ads if no selector provided, but reject invalid values + if not self._is_valid_ads_selector({"all"}): + if self._ads_selector_explicit: + LOG.error('Invalid --ads selector: "%s". Valid values: all or comma-separated numeric IDs.', + self.ads_selector) + sys.exit(2) LOG.info("Extending all ads within 8-day window...") self.ads_selector = "all" @@ -498,8 +502,11 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 case "download": self.configure_file_logging() # ad IDs depends on selector - if not (self.ads_selector in {"all", "new"} or re.compile(r"\d+[,\d+]*").search(self.ads_selector)): - LOG.warning('You provided no ads selector. Defaulting to "new".') + if not self._is_valid_ads_selector({"all", "new"}): + if self._ads_selector_explicit: + LOG.error('Invalid --ads selector: "%s". Valid values: all, new, or comma-separated numeric IDs.', + self.ads_selector) + sys.exit(2) self.ads_selector = "new" self.load_config() # Check for updates on startup @@ -567,13 +574,15 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 * all: Lädt alle Anzeigen aus Ihrem Profil herunter * new: Lädt Anzeigen aus Ihrem Profil herunter, die lokal noch nicht gespeichert sind * : Gibt eine oder mehrere Anzeigen-IDs zum Herunterladen an, z. B. "--ads=1,2,3" - --ads=changed| (update) - Gibt an, welche Anzeigen aktualisiert werden sollen (STANDARD: changed) + --ads=all|changed| (update) - Gibt an, welche Anzeigen aktualisiert werden sollen (STANDARD: changed) Mögliche Werte: + * all: Aktualisiert alle Anzeigen * changed: Aktualisiert nur Anzeigen, die seit der letzten Veröffentlichung geändert wurden * : Gibt eine oder mehrere Anzeigen-IDs zum Aktualisieren an, z. B. "--ads=1,2,3" - --ads= (extend) - Gibt an, welche Anzeigen verlängert werden sollen - Standardmäßig werden alle Anzeigen verlängert, die innerhalb von 8 Tagen ablaufen. - Mit dieser Option können Sie bestimmte Anzeigen-IDs angeben, z. B. "--ads=1,2,3" + --ads=all| (extend) - Gibt an, welche Anzeigen verlängert werden sollen + Mögliche Werte: + * all: Verlängert alle Anzeigen, die innerhalb von 8 Tagen ablaufen + * : Gibt bestimmte Anzeigen-IDs an, z. B. "--ads=1,2,3" --force - Alias für '--ads=all' --keep-old - Verhindert das Löschen alter Anzeigen bei erneuter Veröffentlichung --config= - Pfad zur YAML- oder JSON-Konfigurationsdatei (ändert den Workspace-Modus nicht implizit) @@ -620,13 +629,15 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 * all: downloads all ads from your profile * new: downloads ads from your profile that are not locally saved yet * : provide one or several ads by ID to download, like e.g. "--ads=1,2,3" - --ads=changed| (update) - specifies which ads to update (DEFAULT: changed) + --ads=all|changed| (update) - specifies which ads to update (DEFAULT: changed) Possible values: + * all: update all ads * changed: only update ads that have been modified since last publication * : provide one or several ads by ID to update, like e.g. "--ads=1,2,3" - --ads= (extend) - specifies which ads to extend - By default, extends all ads expiring within 8 days. - Use this option to specify ad IDs, e.g. "--ads=1,2,3" + --ads=all| (extend) - specifies which ads to extend + Possible values: + * all: extend all ads expiring within 8 days + * : specify ad IDs to extend, e.g. "--ads=1,2,3" --force - alias for '--ads=all' --keep-old - don't delete old ads on republication --config= - path to the config YAML or JSON file (does not implicitly change workspace mode) @@ -638,6 +649,18 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 ) ) + def _is_valid_ads_selector(self, valid_keywords:set[str]) -> bool: + """Check if the current ads_selector is valid for the given set of keyword selectors. + + Accepts a single keyword, a comma-separated list of keywords, or a comma-separated + list of numeric IDs. Mixed keyword+numeric selectors are not supported. + """ + return ( + self.ads_selector in valid_keywords + or all(s.strip() in valid_keywords for s in self.ads_selector.split(",")) + or bool(_NUMERIC_IDS_RE.match(self.ads_selector)) + ) + def parse_args(self, args:list[str]) -> None: try: options, arguments = getopt.gnu_getopt( @@ -673,8 +696,10 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 self._workspace_mode_arg = cast(xdg_paths.InstallationMode, mode) case "--ads": self.ads_selector = value.strip().lower() + self._ads_selector_explicit = True case "--force": self.ads_selector = "all" + self._ads_selector_explicit = True case "--keep-old": self.keep_old_ads = True case "--lang": @@ -859,7 +884,7 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 use_specific_ads = False selectors = self.ads_selector.split(",") - if re.compile(r"\d+[,\d+]*").search(self.ads_selector): + if _NUMERIC_IDS_RE.match(self.ads_selector): ids = [int(n) for n in self.ads_selector.split(",")] use_specific_ads = True LOG.info("Start fetch task for the ad(s) with id(s):") @@ -2182,13 +2207,21 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 xdg_paths.ensure_directory(workspace.download_dir, "downloaded ads directory") ad_extractor = extract.AdExtractor(self.browser, self.config, workspace.download_dir, published_ads_by_id = published_ads_by_id) - # use relevant download routine - if self.ads_selector in {"all", "new"}: # explore ads overview for these two modes + # Normalize comma-separated keyword selectors; set deduplication collapses "new,new" → {"new"} + selector_tokens = {s.strip() for s in self.ads_selector.split(",")} + if "all" in selector_tokens: + effective_selector = "all" + elif len(selector_tokens) == 1: + effective_selector = next(iter(selector_tokens)) # e.g. "new,new" → "new" + else: + effective_selector = self.ads_selector # numeric IDs: "123,456" — unchanged + + if effective_selector in {"all", "new"}: # explore ads overview for these two modes LOG.info("Scanning your ad overview...") own_ad_urls = await ad_extractor.extract_own_ads_urls() LOG.info("%s found.", pluralize("ad", len(own_ad_urls))) - if self.ads_selector == "all": # download all of your adds + if effective_selector == "all": # download all of your adds LOG.info("Starting download of all ads...") success_count = 0 @@ -2200,7 +2233,7 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 success_count += 1 LOG.info("%d of %d ads were downloaded from your profile.", success_count, len(own_ad_urls)) - elif self.ads_selector == "new": # download only unsaved ads + elif effective_selector == "new": # download only unsaved ads # check which ads already saved saved_ad_ids = [] ads = self.load_ads(ignore_inactive = False, exclude_ads_with_id = False) # do not skip because of existing IDs @@ -2227,8 +2260,8 @@ class KleinanzeigenBot(WebScrapingMixin): # noqa: PLR0904 new_count += 1 LOG.info("%s were downloaded from your profile.", pluralize("new ad", new_count)) - elif re.compile(r"\d+[,\d+]*").search(self.ads_selector): # download ad(s) with specific id(s) - ids = [int(n) for n in self.ads_selector.split(",")] + elif _NUMERIC_IDS_RE.match(effective_selector): # download ad(s) with specific id(s) + ids = [int(n) for n in effective_selector.split(",")] LOG.info("Starting download of ad(s) with the id(s):") LOG.info(" | ".join([str(ad_id) for ad_id in ids])) diff --git a/src/kleinanzeigen_bot/resources/translations.de.yaml b/src/kleinanzeigen_bot/resources/translations.de.yaml index 7a75e25..c93ed75 100644 --- a/src/kleinanzeigen_bot/resources/translations.de.yaml +++ b/src/kleinanzeigen_bot/resources/translations.de.yaml @@ -253,14 +253,15 @@ kleinanzeigen_bot/__init__.py: run: "DONE: No configuration errors found.": "FERTIG: Keine Konfigurationsfehler gefunden." "DONE: No active ads found.": "FERTIG: Keine aktiven Anzeigen gefunden." - "You provided no ads selector. Defaulting to \"due\".": "Es wurden keine Anzeigen-Selektor angegeben. Es wird \"due\" verwendet." + "Invalid --ads selector: \"%s\". Valid values: all, new, due, changed, or comma-separated numeric IDs.": "Ungültiger --ads-Selektor: \"%s\". Gültige Werte: all, new, due, changed oder kommagetrennte numerische IDs." + "Invalid --ads selector: \"%s\". Valid values: all, changed, or comma-separated numeric IDs.": "Ungültiger --ads-Selektor: \"%s\". Gültige Werte: all, changed oder kommagetrennte numerische IDs." + "Invalid --ads selector: \"%s\". Valid values: all, new, or comma-separated numeric IDs.": "Ungültiger --ads-Selektor: \"%s\". Gültige Werte: all, new oder kommagetrennte numerische IDs." + "Invalid --ads selector: \"%s\". Valid values: all or comma-separated numeric IDs.": "Ungültiger --ads-Selektor: \"%s\". Gültige Werte: all oder kommagetrennte numerische IDs." "DONE: No new/outdated ads found.": "FERTIG: Keine neuen/veralteten Anzeigen gefunden." "DONE: No ads to delete found.": "FERTIG: Keine zu löschenden Anzeigen gefunden." "DONE: No changed ads found.": "FERTIG: Keine geänderten Anzeigen gefunden." "Extending all ads within 8-day window...": "Verlängere alle Anzeigen innerhalb des 8-Tage-Zeitfensters..." "DONE: No ads found to extend.": "FERTIG: Keine Anzeigen zum Verlängern gefunden." - "You provided no ads selector. Defaulting to \"new\".": "Es wurden keine Anzeigen-Selektor angegeben. Es wird \"new\" verwendet." - "You provided no ads selector. Defaulting to \"changed\".": "Es wurden keine Anzeigen-Selektor angegeben. Es wird \"changed\" verwendet." "Unknown command: %s": "Unbekannter Befehl: %s" "Timing collector flush failed: %s": "Zeitmessdaten konnten nicht gespeichert werden: %s" diff --git a/tests/unit/test_init.py b/tests/unit/test_init.py index d4d60ea..44afdbb 100644 --- a/tests/unit/test_init.py +++ b/tests/unit/test_init.py @@ -1152,6 +1152,20 @@ class TestKleinanzeigenBotAdOperations: await test_bot.run(["script.py", "download"]) assert test_bot.ads_selector == "new" + @pytest.mark.asyncio + async def test_run_update_default_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument + """Test running update command with default selector falls back to changed.""" + with patch.object(test_bot, "load_ads", return_value = []): + await test_bot.run(["script.py", "update"]) + assert test_bot.ads_selector == "changed" + + @pytest.mark.asyncio + async def test_run_extend_default_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument + """Test running extend command with default selector falls back to all.""" + with patch.object(test_bot, "load_ads", return_value = []): + await test_bot.run(["script.py", "extend"]) + assert test_bot.ads_selector == "all" + def test_load_ads_no_files(self, test_bot:KleinanzeigenBot) -> None: """Test loading ads with no files.""" test_bot.config.ad_files = ["nonexistent/*.yaml"] @@ -1172,17 +1186,31 @@ class TestKleinanzeigenBotAdManagement: @pytest.mark.asyncio async def test_run_publish_invalid_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument - """Test running publish with invalid selector.""" - with patch.object(test_bot, "load_ads", return_value = []): + """Test running publish with invalid selector exits with error.""" + with pytest.raises(SystemExit) as exc_info: await test_bot.run(["script.py", "publish", "--ads=invalid"]) - assert test_bot.ads_selector == "due" + assert exc_info.value.code == 2 @pytest.mark.asyncio async def test_run_download_invalid_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument - """Test running download with invalid selector.""" - with patch.object(test_bot, "download_ads", new_callable = AsyncMock): + """Test running download with invalid selector exits with error.""" + with pytest.raises(SystemExit) as exc_info: await test_bot.run(["script.py", "download", "--ads=invalid"]) - assert test_bot.ads_selector == "new" + assert exc_info.value.code == 2 + + @pytest.mark.asyncio + async def test_run_update_invalid_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument + """Test running update with invalid selector exits with error.""" + with pytest.raises(SystemExit) as exc_info: + await test_bot.run(["script.py", "update", "--ads=invalid"]) + assert exc_info.value.code == 2 + + @pytest.mark.asyncio + async def test_run_extend_invalid_selector(self, test_bot:KleinanzeigenBot, mock_config_setup:None) -> None: # pylint: disable=unused-argument + """Test running extend with invalid selector exits with error.""" + with pytest.raises(SystemExit) as exc_info: + await test_bot.run(["script.py", "extend", "--ads=invalid"]) + assert exc_info.value.code == 2 class TestKleinanzeigenBotAdConfiguration: