diff --git a/changedetectionio/blueprint/check_proxies/__init__.py b/changedetectionio/blueprint/check_proxies/__init__.py index 2a07222f..8466eb63 100644 --- a/changedetectionio/blueprint/check_proxies/__init__.py +++ b/changedetectionio/blueprint/check_proxies/__init__.py @@ -7,7 +7,7 @@ from changedetectionio.store import ChangeDetectionStore from functools import wraps from flask import Blueprint -from flask_login import login_required +from changedetectionio.auth_decorator import login_optionally_required STATUS_CHECKING = 0 STATUS_FAILED = 1 @@ -94,14 +94,14 @@ def construct_blueprint(datastore: ChangeDetectionStore): return results - @login_required @check_proxies_blueprint.route("//status", methods=['GET']) + @login_optionally_required def get_recheck_status(uuid): results = _recalc_check_status(uuid=uuid) return results - @login_required @check_proxies_blueprint.route("//start", methods=['GET']) + @login_optionally_required def start_check(uuid): if not datastore.proxy_list: diff --git a/changedetectionio/blueprint/price_data_follower/__init__.py b/changedetectionio/blueprint/price_data_follower/__init__.py index 9050d27a..8cb504ae 100644 --- a/changedetectionio/blueprint/price_data_follower/__init__.py +++ b/changedetectionio/blueprint/price_data_follower/__init__.py @@ -1,7 +1,7 @@ from changedetectionio.strtobool import strtobool from flask import Blueprint, flash, redirect, url_for -from flask_login import login_required +from changedetectionio.auth_decorator import login_optionally_required from changedetectionio.store import ChangeDetectionStore from changedetectionio import queuedWatchMetaData from changedetectionio import worker_pool @@ -14,8 +14,8 @@ def construct_blueprint(datastore: ChangeDetectionStore, update_q: PriorityQueue price_data_follower_blueprint = Blueprint('price_data_follower', __name__) - @login_required @price_data_follower_blueprint.route("//accept", methods=['GET']) + @login_optionally_required def accept(uuid): datastore.data['watching'][uuid]['track_ldjson_price_data'] = PRICE_DATA_TRACK_ACCEPT datastore.data['watching'][uuid]['processor'] = 'restock_diff' @@ -24,8 +24,8 @@ def construct_blueprint(datastore: ChangeDetectionStore, update_q: PriorityQueue worker_pool.queue_item_async_safe(update_q, queuedWatchMetaData.PrioritizedItem(priority=1, item={'uuid': uuid})) return redirect(url_for("watchlist.index")) - @login_required @price_data_follower_blueprint.route("//reject", methods=['GET']) + @login_optionally_required def reject(uuid): datastore.data['watching'][uuid]['track_ldjson_price_data'] = PRICE_DATA_TRACK_REJECT datastore.data['watching'][uuid].commit() diff --git a/changedetectionio/flask_app.py b/changedetectionio/flask_app.py index 2d52b695..8cc0961b 100644 --- a/changedetectionio/flask_app.py +++ b/changedetectionio/flask_app.py @@ -624,8 +624,13 @@ def changedetection_app(config=None, datastore_o=None): # Permitted - static flag icons need to load on login page elif request.endpoint and request.endpoint == 'static_flags': return None - # Permitted - language selection should work on login page - elif request.endpoint and request.endpoint == 'set_language': + # Permitted - language selection should work on login page. + # Both halves of the language modal must be exempt: it renders for anonymous + # users (base.html deliberately leaves it outside the is_authenticated guard), + # so exempting only set_language let you pick a language but bounced + # "Auto-detect from browser" to /login without clearing the session locale. + elif request.endpoint and request.endpoint in ('set_language', + 'ui.delete_locale_language_session_var_if_it_exists'): return None # Permitted elif request.endpoint and 'login' in request.endpoint: diff --git a/changedetectionio/tests/test_language_selector_anonymous.py b/changedetectionio/tests/test_language_selector_anonymous.py new file mode 100644 index 00000000..404c1297 --- /dev/null +++ b/changedetectionio/tests/test_language_selector_anonymous.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python3 +""" +The language modal renders for anonymous users on the login page (base.html keeps it +outside the `current_user.is_authenticated or not has_password` guard that wraps the +search modal). Both of its actions must therefore work while logged out. + +Regression: only `set_language` was exempted in check_authentication(), so an anonymous +user at the login screen could pick a specific language but clicking "Auto-detect from +browser" right below it 302'd to /login without clearing the session locale. +""" + +from flask import url_for +from .util import live_server_setup, wait_for_all_checks + + +def test_language_endpoints_work_for_anonymous_users(client, live_server, measure_memory_usage, datastore_path): + # Enable password protection so the global auth wall in check_authentication() is active + res = client.post( + url_for("settings.settings_page"), + data={ + "application-password": "hunter2", + "requests-time_between_check-minutes": 180, + "application-fetch_backend": "html_requests", + }, + follow_redirects=True) + assert res.status_code == 200 + + client.get(url_for("logout"), follow_redirects=True) + + # Both language links are rendered on the login page, so both must be reachable + res = client.get(url_for("login")) + assert res.status_code == 200 + assert b'language-selector' in res.data, "Language modal trigger should render for anonymous users" + + # Picking a specific language must not redirect to the login page + res = client.get(url_for("set_language", locale="de"), follow_redirects=False) + assert res.status_code == 302 + assert '/login' not in res.headers.get("Location", ""), \ + "set_language must not bounce anonymous users to /login" + + # ...and neither must clearing it back to auto-detect + res = client.get(url_for("ui.delete_locale_language_session_var_if_it_exists"), follow_redirects=False) + assert res.status_code == 302 + assert '/login' not in res.headers.get("Location", ""), \ + "Auto-detect must not bounce anonymous users to /login (it renders on the login page)" diff --git a/changedetectionio/tests/unit/test_auth_decorator_order.py b/changedetectionio/tests/unit/test_auth_decorator_order.py index ff803172..f3690cbe 100644 --- a/changedetectionio/tests/unit/test_auth_decorator_order.py +++ b/changedetectionio/tests/unit/test_auth_decorator_order.py @@ -1,11 +1,13 @@ """ -Static analysis test: verify @login_optionally_required is always applied -AFTER (inner to) @blueprint.route(), not before it. +Static analysis test: verify @blueprint.route() is always the outermost +decorator on a view, so nothing sits above it. -In Flask, @route() must be the outermost decorator because it registers -whatever function it receives. If @login_optionally_required is placed -above @route(), the raw unprotected function gets registered and auth is -silently bypassed (GHSA-jmrh-xmgh-x9j4). +In Flask, @route() must be outermost because it registers whatever function +it receives and then returns that function unchanged. Any decorator placed +above @route() is applied only to the module-level name, never to the view +the blueprint actually dispatches to — so it is silently dead code. When the +dead decorator is an auth wrapper, the route is left unprotected +(GHSA-jmrh-xmgh-x9j4). Correct order (route outermost, auth inner): @blueprint.route('/path') @@ -13,9 +15,19 @@ Correct order (route outermost, auth inner): def view(): ... Wrong order (auth never called): - @login_optionally_required ← registered by route, then discarded + @login_optionally_required ← discarded; route registered the raw fn @blueprint.route('/path') def view(): ... + +This check is deliberately name-agnostic. An earlier version matched only +the literal name `login_optionally_required`, which missed four routes using +plain flask_login `login_required` (GHSA-q85w-c766-h5g8) — an allowlist of +decorator names only ever catches the names someone remembered to add. We +now flag *any* non-route decorator above @route, which also covers attribute +forms (@flask_login.login_required), aliased imports, and non-auth +decorators that are equally dead up there. + +Stacked @route decorators are a legitimate Flask idiom and are exempt. """ import ast @@ -35,11 +47,6 @@ def _is_route_decorator(node: ast.expr) -> bool: ) -def _is_auth_decorator(node: ast.expr) -> bool: - """Return True if the decorator is @login_optionally_required.""" - return isinstance(node, ast.Name) and node.id == "login_optionally_required" - - def collect_violations() -> list[str]: violations = [] @@ -54,20 +61,22 @@ def collect_violations() -> list[str]: continue decorators = node.decorator_list - auth_indices = [i for i, d in enumerate(decorators) if _is_auth_decorator(d)] route_indices = [i for i, d in enumerate(decorators) if _is_route_decorator(d)] + if not route_indices: + continue - # Bad order: auth decorator appears at a lower index (higher up) than a route decorator - for auth_idx in auth_indices: - for route_idx in route_indices: - if auth_idx < route_idx: - rel = path.relative_to(REPO_ROOT) - violations.append( - f"{rel}:{node.lineno} — `{node.name}`: " - f"@login_optionally_required (line {decorators[auth_idx].lineno}) " - f"is above @route (line {decorators[route_idx].lineno}); " - f"auth wrapper will never be called" - ) + # Everything above the last @route is discarded by the registration. + # Other @route decorators up there are fine — stacking routes is normal. + last_route = max(route_indices) + for i, decorator in enumerate(decorators): + if i < last_route and not _is_route_decorator(decorator): + rel = path.relative_to(REPO_ROOT) + violations.append( + f"{rel}:{node.lineno} — `{node.name}`: " + f"@{ast.unparse(decorator)} (line {decorator.lineno}) is above @route " + f"(line {decorators[last_route].lineno}); it will never be applied " + f"to the registered view" + ) return violations @@ -76,9 +85,10 @@ def test_auth_decorator_order(): violations = collect_violations() if violations: msg = ( - "\n\nFound routes where @login_optionally_required is placed ABOVE @blueprint.route().\n" - "This silently disables authentication — @route() registers the raw function\n" - "and the auth wrapper is never called.\n\n" + "\n\nFound decorators placed ABOVE @blueprint.route().\n" + "@route() registers the raw function and returns it unchanged, so anything\n" + "above it is never applied to the view Flask dispatches to. If the decorator\n" + "is an auth wrapper, the route is left completely unauthenticated.\n\n" "Fix: move @blueprint.route() to be the outermost (topmost) decorator.\n\n" + "\n".join(f" • {v}" for v in violations) )