diff --git a/shelfmark/config/migrations.py b/shelfmark/config/migrations.py index b2f7008f..0dc6ff60 100644 --- a/shelfmark/config/migrations.py +++ b/shelfmark/config/migrations.py @@ -67,7 +67,10 @@ def _pick_legacy_settings_restriction(config: dict[str, Any]) -> bool | None: def migrate_audiobook_formats( *, load_general_config: Callable[[], dict[str, Any]], - save_general_config: Callable[[dict[str, Any]], None], + # `object` rather than `None`: the result is discarded, and savers that report + # success (settings_registry.save_config_file returns bool) are not assignable to a + # `-> None` callable. + save_general_config: Callable[[dict[str, Any]], object], widened_formats: Sequence[str], logger: MigrationLogger, ) -> None: diff --git a/shelfmark/download/http.py b/shelfmark/download/http.py index 70fe001b..4b669511 100644 --- a/shelfmark/download/http.py +++ b/shelfmark/download/http.py @@ -272,8 +272,11 @@ def html_get_page( selector: Mirror selector used for AA mirror and DNS rotation. cancel_flag: Optional event used to abort retries early. status_callback: Optional callback for UI status updates. - allow_bypasser_fallback: If False, 403 errors will trigger mirror rotation - instead of switching to the bypasser. Use for search operations. + allow_bypasser_fallback: Whether a challenge may be handed to the bypasser. + If False, a 403 triggers mirror rotation instead, and an AA redirect loop + gives up immediately rather than waiting on a browser solve. Use False for + best-effort fetches whose result is optional (e.g. the download count on + the details modal); search and detail pages pass True. use_bypasser: Whether to start with the bypasser instead of direct HTTP. include_response_url: If True, return `(html, final_url)` to expose the resolved response URL after redirects. @@ -319,6 +322,29 @@ def html_get_page( finally: release_activity_grace(status_callback) + def _bypass_handoff_allowed() -> bool: + """Whether a challenge on the current URL may be handed to the bypasser. + + allow_bypasser_fallback is honoured for the same reason the 403 path honours it: + callers such as the /dyn/md5/summary fetch behind the details modal pass False + precisely so a best-effort request fails fast instead of holding the UI open for + a minutes-long browser solve. + """ + return allow_bypasser_fallback and _is_cf_bypass_enabled() and not use_bypasser_now + + def _redirect_loop_handoff(bypass_url: str) -> str | tuple[str, str]: + """Drop the host's stale clearance cookies, then bypass `bypass_url`. + + A `?check=1` loop is how DDoS-Guard answers a clearance cookie that has gone + stale, so the dead cookie has to go before the solve — otherwise it is merged + back over the fresh one on the next request and the loop simply resumes. Purging + is internal-bypasser only; with an external one get_cf_cookies_for_domain() + already returns {}. + """ + if not _is_using_external_bypasser(): + _get_internal_bypasser().clear_cf_cookies(urlparse(bypass_url).hostname or "") + return _run_bypasser(bypass_url) + configured_retry = normalize_positive_int(app_config.MAX_RETRY) retry_limit = ( retry if retry is not None else (configured_retry if configured_retry is not None else 1) @@ -433,25 +459,26 @@ def html_get_page( redirects_followed += 1 if redirects_followed > _MAX_REDIRECTS: # A same-host redirect loop on AA is not a network fault — it is - # how an unsolved DDoS-Guard handshake presents: /search redirects - # to /search&check=1, which redirects back, indefinitely. Raising - # sends it down the retry path, which re-runs the whole loop on - # every attempt (10 x 6 = ~60 requests to AA) and never once offers - # the URL to the bypasser, so it can only ever fail. Hand it over - # directly — not via `continue`, which would target this inner - # redirect loop rather than the retry branch above. - # allow_bypasser_fallback is honoured here for the same - # reason the 403 path honours it: callers such as the - # /dyn/md5/summary fetch behind the details modal pass False - # precisely so a best-effort request fails fast instead of - # holding the UI open for a minutes-long browser solve. - if allow_bypasser_fallback and _is_cf_bypass_enabled() and not use_bypasser_now: + # how DDoS-Guard presents a handshake that is unsolved, or whose + # clearance cookie has gone stale: /search redirects to + # /search&check=1, which redirects back, indefinitely. Hand it + # straight to the bypasser rather than raising, which would send it + # down the retry path to re-run the whole loop on every attempt + # (10 x 6 = ~60 requests to AA) without ever offering the URL to the + # bypasser. `continue` is no use here either — it would target this + # inner redirect loop rather than the retry branch below. + if _bypass_handoff_allowed(): logger.info( - "redirect loop on %s; switching to bypasser", current_url + "Redirect loop detected; switching to bypasser: %s", current_url ) - use_bypasser_now = True - return _run_bypasser(current_url) - _raise_too_many_redirects(f"Too many redirects for {current_url}") + return _redirect_loop_handoff(current_url) + # No bypasser to hand it to. Every AA mirror shares the challenge, + # so rotating only collects another loop — give up now instead of + # raising and burning the same ~60 requests over the retry budget. + logger.warning( + "Redirect loop and no bypasser available, giving up: %s", current_url + ) + return _result("", current_url) current_url = redirect_url continue @@ -463,23 +490,14 @@ def html_get_page( except Exception as e: status = _get_status_code(e) - # DDoS-Guard also answers stale clearance cookies with an endless `?check=1` - # redirect. TooManyRedirects carries no status, so the 403 rescue below never - # fires and every retry re-sends the dead cookies. - if ( - isinstance(e, requests.exceptions.TooManyRedirects) - and allow_bypasser_fallback - and _is_cf_bypass_enabled() - and not use_bypasser_now - ): - if not _is_using_external_bypasser(): - hostname = urlparse(current_url).hostname or "" - _get_internal_bypasser().clear_cf_cookies(hostname) + # The same DDoS-Guard rescue for loops the manual AA follower above does not + # see: non-AA hosts keep allow_redirects=True, so `requests` follows the loop + # itself and raises, and an AA redirect missing its Location header raises + # too. TooManyRedirects carries no status, so the 403 rescue below never fires + # and every retry would re-send the dead cookies. + if isinstance(e, requests.exceptions.TooManyRedirects) and _bypass_handoff_allowed(): logger.info("Redirect loop detected; switching to bypasser: %s", current_url) - if status_callback: - status_callback("resolving", "Bypassing protection...") - use_bypasser_now = True - continue + return _redirect_loop_handoff(current_url) # 403 = Cloudflare/DDoS-Guard protection if status == _HTTP_STATUS_FORBIDDEN: @@ -505,12 +523,11 @@ def html_get_page( ) continue logger.info("403 detected; switching to bypasser: %s", current_url) - # Invoke it here rather than setting the flag and continuing. The - # branch that acts on the flag runs at the top of the *next* retry - # attempt, so under the supported MAX_RETRY=1 there is no next - # attempt and the bypasser was never reached — a 403 simply ended - # the search. Same reasoning as the redirect-loop handoff below. - use_bypasser_now = True + # Invoke it here rather than setting use_bypasser_now and continuing. + # The branch that acts on that flag runs at the top of the *next* retry + # attempt, so under the supported MAX_RETRY=1 there is no next attempt + # and the bypasser was never reached — a 403 simply ended the search. + # Same reasoning as the redirect-loop handoffs. return _run_bypasser(current_url) logger.warning("403 error, giving up: %s", current_url) return _result("", current_url) diff --git a/tests/download/test_http_bypasser_fallbacks.py b/tests/download/test_http_bypasser_fallbacks.py index 1716e8eb..47033889 100644 --- a/tests/download/test_http_bypasser_fallbacks.py +++ b/tests/download/test_http_bypasser_fallbacks.py @@ -283,3 +283,96 @@ def test_get_bypassed_page_uses_external_bypasser_when_enabled(monkeypatch): assert http.get_bypassed_page("https://example.com", selector, cancel_flag) == "EXT" assert calls == [("https://example.com", selector, cancel_flag)] + + +def test_redirect_loop_gives_up_immediately_when_bypasser_not_allowed(monkeypatch): + """A loop the bypasser may not rescue must fail fast, not burn the retry budget. + + Guards the regression where the unrescued loop raised TooManyRedirects into the + retry path: that error is not retryable and carries no status, so every attempt + re-ran the full 6-redirect loop for ~60 requests to AA before giving up. + """ + import shelfmark.download.http as http + + monkeypatch.setattr(http, "_is_cf_bypass_enabled", lambda: True) + monkeypatch.setattr(http, "_apply_cf_bypass", lambda _url, _headers: {}) + monkeypatch.setattr(http, "get_proxies", lambda _url: {}) + monkeypatch.setattr(http, "get_ssl_verify", lambda _url: True) + monkeypatch.setattr(http.network, "should_rotate_dns_for_url", lambda _url: True) + monkeypatch.setattr(http.network, "is_aa_auto_mode", lambda: True) + monkeypatch.setattr(http.time, "sleep", lambda _seconds: None) + + requested: list[str] = [] + + def check_redirect(url: str, **_kwargs): + requested.append(url) + response = _FakeResponse(302, url=url) + response.is_redirect = True + response.headers = {"Location": f"{url}&check=1"} + return response + + def unreachable_bypasser(*_args, **_kwargs): + msg = "bypasser must not run when allow_bypasser_fallback is False" + raise AssertionError(msg) + + monkeypatch.setattr(http.requests, "get", check_redirect) + monkeypatch.setattr(http, "get_bypassed_page", unreachable_bypasser) + + html = http.html_get_page( + "https://annas-archive.gl/dyn/md5/summary/abc", + retry=10, + allow_bypasser_fallback=False, + success_delay=0, + ) + + assert html == "" + # One pass through the redirect cap, not one pass per retry attempt. + assert len(requested) == http._MAX_REDIRECTS + 1 + + +def test_requests_raised_redirect_loop_still_reaches_bypasser(monkeypatch): + """Non-AA hosts keep allow_redirects=True, so `requests` raises the loop itself. + + The AA follower above never sees that one, so the rescue in the exception path has + to stay — and it must purge the stale cookie exactly like the inline AA handoff. + """ + import shelfmark.download.http as http + + cleared: list[str] = [] + + class _FakeInternalBypasser: + @staticmethod + def clear_cf_cookies(domain: str) -> None: + cleared.append(domain) + + monkeypatch.setattr(http, "_is_cf_bypass_enabled", lambda: True) + monkeypatch.setattr(http, "_is_using_external_bypasser", lambda: False) + monkeypatch.setattr(http, "_get_internal_bypasser", lambda: _FakeInternalBypasser) + monkeypatch.setattr(http, "_bypass_grace_seconds", lambda: 100.0) + monkeypatch.setattr(http, "_apply_cf_bypass", lambda _url, _headers: {}) + monkeypatch.setattr(http, "get_proxies", lambda _url: {}) + monkeypatch.setattr(http, "get_ssl_verify", lambda _url: True) + monkeypatch.setattr(http.network, "should_rotate_dns_for_url", lambda _url: False) + monkeypatch.setattr(http.time, "sleep", lambda _seconds: None) + + def looping(_url: str, **_kwargs): + raise requests.exceptions.TooManyRedirects("too many redirects") + + bypassed: list[str] = [] + monkeypatch.setattr(http.requests, "get", looping) + monkeypatch.setattr( + http, + "get_bypassed_page", + lambda url, *_a, **_k: bypassed.append(url) or "ok", + ) + + html = http.html_get_page( + "https://z-lib.fm/s/dune", + retry=1, + allow_bypasser_fallback=True, + success_delay=0, + ) + + assert html == "ok" + assert cleared == ["z-lib.fm"] + assert bypassed == ["https://z-lib.fm/s/dune"]