fix(oauth): add state to OAuth callback flow to prevent Login-CSRF

CHANGELOG.md

@@ -185,6 +185,18 @@
   call, not a leak. `_download_github_tests()` no longer accepts a session
   parameter at all — GitHub is always third-party (issue #240, security
   audit finding F-01, part of #146/#97).
+- **Security (Medium):** OAuth authorization-code flow (`authorize_via_browser()`)
+  now sends a cryptographically random `state` (`secrets.token_urlsafe(32)`)
+  in the authorize URL and requires the local callback server to receive the
+  same value back before extracting the code. Previously the loopback
+  callback server accepted the first `?code=...` it saw with no `state`
+  check, so a page that lured the victim into hitting
+  `http://localhost:<port>/callback?code=<attacker's code>` could bind the
+  local app to the attacker's Stepik account (Login-CSRF).
+  `wait_for_auth_code()`/`_make_oauth_handler()` now take a required
+  `expected_state` parameter and reject a missing/mismatched `state` with a
+  clear `RuntimeError` instead of ever returning a code (issue #241,
+  security audit finding F-02, part of #146/#149/#97).
 
 ### Refactored
 - `cli.py` decomposed into a package (`cli/`), epic #117 (issues #118-#122).

src/stepik_grader/core/stepik_client.py

@@ -22,6 +22,7 @@
 import ipaddress
 import json as _json_mod
 import pathlib
+import secrets as secrets_module
 import threading
 import time
 import webbrowser
@@ -223,8 +224,16 @@ def refresh_access_token(
 def _make_oauth_handler(
     auth_data: dict[str, Any],
     path: str,
+    expected_state: str,
 ) -> type[BaseHTTPRequestHandler]:
-    """Фабрика OAuthHandler: захватывает auth_data и ожидаемый path колбэка."""
+    """Фабрика OAuthHandler: захватывает auth_data, path и ожидаемый OAuth ``state``.
+
+    ``expected_state`` защищает от Login-CSRF (issue #241): колбэк с
+    ``state``, не совпадающим с тем, что был отправлен в authorize URL,
+    отклоняется без извлечения ``code`` — иначе злоумышленник мог бы
+    подсунуть жертве ссылку на локальный callback-сервер со своим кодом
+    авторизации и привязать её сессию к своему Stepik-аккаунту.
+    """
 
     class OAuthHandler(BaseHTTPRequestHandler):
         def do_GET(self) -> None:  # noqa: N802
@@ -235,6 +244,16 @@ def do_GET(self) -> None:  # noqa: N802
                 self.end_headers()
                 self.wfile.write(b"Not found")
                 return
+
+            received_state = params.get("state", [None])[0]
+            if received_state != expected_state:
+                auth_data["error"] = "state_mismatch"
+                self.send_response(400)
+                self.send_header("Content-Type", "text/html; charset=utf-8")
+                self.end_headers()
+                self.wfile.write(b"<h1>Invalid state. Possible CSRF - authorization rejected.</h1>")
+                return
+
             auth_data["code"] = params.get("code", [None])[0]
             auth_data["error"] = params.get("error", [None])[0]
             self.send_response(200)
@@ -248,25 +267,36 @@ def log_message(self, fmt: str, *args: object) -> None:  # noqa: D401
     return OAuthHandler
 
 
-def wait_for_auth_code(host: str, port: int, path: str, timeout: int = 120) -> str:
+def wait_for_auth_code(
+    host: str,
+    port: int,
+    path: str,
+    expected_state: str,
+    timeout: int = 120,
+) -> str:
     """Запускает временный HTTP-сервер и ожидает OAuth-колбэк с кодом авторизации.
 
     Parameters
     ----------
     host, port, path:
         Параметры redirect_uri из secrets.json.
+    expected_state:
+        Значение ``state``, отправленное в authorize URL (issue #241);
+        колбэк с несовпадающим/отсутствующим ``state`` отклоняется как
+        потенциальный Login-CSRF.
     timeout:
         Максимальное время ожидания в секундах (по умолчанию 120).
 
     Raises
     ------
     RuntimeError:
-        Если Stepik вернул error-параметр в колбэке.
+        Если Stepik вернул error-параметр в колбэке, либо ``state`` колбэка
+        не совпал с ``expected_state``.
     TimeoutError:
         Если код не получен в течение timeout секунд.
     """
     auth_data: dict[str, Any] = {}
-    handler_class = _make_oauth_handler(auth_data, path)
+    handler_class = _make_oauth_handler(auth_data, path, expected_state)
     server = HTTPServer((host, port), handler_class)  # type: ignore[arg-type]
     server.timeout = timeout
 
@@ -292,16 +322,23 @@ def authorize_via_browser(
     """Открывает браузер, ожидает OAuth-код, обменивает на токены.
 
     Возвращает dict с ключами: access_token, refresh_token, expires_in, expires_at.
+
+    Отправляет криптографически случайный ``state`` в authorize URL и требует
+    его точного совпадения в колбэке (issue #241, F-02) — защита от
+    Login-CSRF, когда злоумышленник подсовывает жертве ссылку на локальный
+    callback-сервер со своим кодом авторизации.
     """
     parsed = urlparse(redirect_uri)
     host = parsed.hostname or "localhost"
     port = parsed.port or 80
     path = parsed.path or "/"
 
+    state = secrets_module.token_urlsafe(32)
     params: dict[str, str] = {
         "client_id": client_id,
         "redirect_uri": redirect_uri,
         "response_type": "code",
+        "state": state,
     }
     auth_url = f"{API_HOST}/oauth2/authorize/?{urlencode(params)}"
     print(f"Открываю браузер: {auth_url}")
@@ -310,7 +347,7 @@ def authorize_via_browser(
     except OSError:
         pass
 
-    code = wait_for_auth_code(host, port, path)
+    code = wait_for_auth_code(host, port, path, state)
     response = requests.post(
         f"{API_HOST}/oauth2/token/",
         auth=HTTPBasicAuth(client_id, client_secret),

tests/test_oauth_flow.py

@@ -199,22 +199,41 @@ def test_wait_for_auth_code_extracts_code(self, monkeypatch):
         """
         fake_server = _fake_server_factory("/callback?code=test_code&state=xyz")
         monkeypatch.setattr(stepik_client, "HTTPServer", fake_server)
-        code = wait_for_auth_code("localhost", 8080, "/callback", timeout=1)
+        code = wait_for_auth_code("localhost", 8080, "/callback", "xyz", timeout=1)
         assert code == "test_code"
 
     def test_wait_for_auth_code_raises_on_error_param(self, monkeypatch):
-        """An ?error= callback raises RuntimeError. REFACTORING INVARIANT."""
-        fake_server = _fake_server_factory("/callback?error=access_denied")
+        """An ?error= callback (with matching state) raises RuntimeError. REFACTORING INVARIANT."""
+        fake_server = _fake_server_factory("/callback?error=access_denied&state=xyz")
         monkeypatch.setattr(stepik_client, "HTTPServer", fake_server)
         with pytest.raises(RuntimeError):
-            wait_for_auth_code("localhost", 8080, "/callback", timeout=1)
+            wait_for_auth_code("localhost", 8080, "/callback", "xyz", timeout=1)
 
     def test_wait_for_auth_code_timeout_without_code(self, monkeypatch):
         """No code and no error within timeout raises TimeoutError. REFACTORING INVARIANT."""
-        fake_server = _fake_server_factory("/callback")  # no code, no error
+        fake_server = _fake_server_factory("/callback?state=xyz")  # no code, no error
         monkeypatch.setattr(stepik_client, "HTTPServer", fake_server)
         with pytest.raises(TimeoutError):
-            wait_for_auth_code("localhost", 8080, "/callback", timeout=1)
+            wait_for_auth_code("localhost", 8080, "/callback", "xyz", timeout=1)
+
+    def test_wait_for_auth_code_raises_on_state_mismatch(self, monkeypatch):
+        """Callback with a wrong ``state`` raises RuntimeError, even with a valid code.
+
+        Regression test for issue #241 (F-02, Login-CSRF): a callback whose
+        ``state`` doesn't match the one sent in the authorize URL must never
+        yield a usable code.
+        """
+        fake_server = _fake_server_factory("/callback?code=attacker_code&state=wrong")
+        monkeypatch.setattr(stepik_client, "HTTPServer", fake_server)
+        with pytest.raises(RuntimeError, match="state"):
+            wait_for_auth_code("localhost", 8080, "/callback", "expected", timeout=1)
+
+    def test_wait_for_auth_code_raises_on_missing_state(self, monkeypatch):
+        """Callback with no ``state`` param at all is rejected the same as a mismatch."""
+        fake_server = _fake_server_factory("/callback?code=attacker_code")
+        monkeypatch.setattr(stepik_client, "HTTPServer", fake_server)
+        with pytest.raises(RuntimeError, match="state"):
+            wait_for_auth_code("localhost", 8080, "/callback", "expected", timeout=1)
 
 
 # ---------------------------------------------------------------------------
@@ -241,7 +260,7 @@ def test_oauth_handler_sets_auth_code_on_callback(self):
         REFACTORING INVARIANT: handler extraction logic unchanged in oauth_flow.py.
         """
         auth_data: dict = {}
-        handler_class = _make_oauth_handler(auth_data, "/callback")
+        handler_class = _make_oauth_handler(auth_data, "/callback", "s")
         handler = _build_handler(handler_class, "/callback?code=abc123&state=s")
         handler.do_GET()
         assert auth_data["code"] == "abc123"
@@ -250,30 +269,53 @@ def test_oauth_handler_sets_auth_code_on_callback(self):
     def test_oauth_handler_returns_200_and_html_on_success(self):
         """Handler returns 200 and an HTML response body. REFACTORING INVARIANT."""
         auth_data: dict = {}
-        handler_class = _make_oauth_handler(auth_data, "/callback")
-        handler = _build_handler(handler_class, "/callback?code=abc123")
+        handler_class = _make_oauth_handler(auth_data, "/callback", "s")
+        handler = _build_handler(handler_class, "/callback?code=abc123&state=s")
         handler.do_GET()
         assert handler._responses == [200]
         assert b"<h1>" in handler.wfile.getvalue()
 
     def test_oauth_handler_captures_error_param(self):
         """Handler captures the error parameter from the callback. REFACTORING INVARIANT."""
         auth_data: dict = {}
-        handler_class = _make_oauth_handler(auth_data, "/callback")
-        handler = _build_handler(handler_class, "/callback?error=denied")
+        handler_class = _make_oauth_handler(auth_data, "/callback", "s")
+        handler = _build_handler(handler_class, "/callback?error=denied&state=s")
         handler.do_GET()
         assert auth_data["error"] == "denied"
         assert auth_data["code"] is None
 
     def test_oauth_handler_404_on_wrong_path(self):
         """Requests to a non-callback path return 404 and set no code. REFACTORING INVARIANT."""
         auth_data: dict = {}
-        handler_class = _make_oauth_handler(auth_data, "/callback")
+        handler_class = _make_oauth_handler(auth_data, "/callback", "s")
         handler = _build_handler(handler_class, "/favicon.ico?code=ignored")
         handler.do_GET()
         assert handler._responses == [404]
         assert "code" not in auth_data
 
+    def test_oauth_handler_rejects_mismatched_state(self):
+        """Callback with a wrong ``state`` is rejected with 400 and no code captured.
+
+        Regression test for issue #241 (F-02, Login-CSRF).
+        """
+        auth_data: dict = {}
+        handler_class = _make_oauth_handler(auth_data, "/callback", "expected")
+        handler = _build_handler(handler_class, "/callback?code=attacker_code&state=wrong")
+        handler.do_GET()
+        assert handler._responses == [400]
+        assert auth_data["error"] == "state_mismatch"
+        assert "code" not in auth_data
+
+    def test_oauth_handler_rejects_missing_state(self):
+        """Callback with no ``state`` param at all is rejected the same as a mismatch."""
+        auth_data: dict = {}
+        handler_class = _make_oauth_handler(auth_data, "/callback", "expected")
+        handler = _build_handler(handler_class, "/callback?code=attacker_code")
+        handler.do_GET()
+        assert handler._responses == [400]
+        assert auth_data["error"] == "state_mismatch"
+        assert "code" not in auth_data
+
 
 # ---------------------------------------------------------------------------
 # refresh_access_token / create_user_session refresh path

tests/test_stepik_client_extra.py

@@ -51,7 +51,12 @@ def test_happy_path(self):
             result = authorize_via_browser("cid", "csecret", "http://localhost:8080/cb")
 
         mock_open.assert_called_once()
-        mock_wait.assert_called_once_with("localhost", 8080, "/cb")
+        mock_wait.assert_called_once()
+        wait_args = mock_wait.call_args.args
+        assert wait_args[0] == "localhost"
+        assert wait_args[1] == 8080
+        assert wait_args[2] == "/cb"
+        assert isinstance(wait_args[3], str) and len(wait_args[3]) > 20  # random state
         mock_post.assert_called_once()
         assert result["access_token"] == "AT"
         assert result["refresh_token"] == "RT"
@@ -86,7 +91,51 @@ def test_default_host_port_path(self):
             patch("stepik_grader.core.stepik_client.requests.post", return_value=token_resp),
         ):
             authorize_via_browser("cid", "cs", "https://example.org")
-        mock_wait.assert_called_once_with("example.org", 80, "/")
+        wait_args = mock_wait.call_args.args
+        assert wait_args[0] == "example.org"
+        assert wait_args[1] == 80
+        assert wait_args[2] == "/"
+
+
+class TestAuthorizeViaBrowserState:
+    """authorize_via_browser отправляет случайный state в authorize URL (issue #241)."""
+
+    def test_auth_url_includes_random_state_matching_wait_call(self):
+        """URL, открытый в браузере, содержит тот же state, что уходит в wait_for_auth_code."""
+        token_resp = MagicMock()
+        token_resp.raise_for_status = MagicMock()
+        token_resp.json.return_value = {"access_token": "AT", "expires_in": 100}
+        with (
+            patch("stepik_grader.core.stepik_client.webbrowser.open") as mock_open,
+            patch(
+                "stepik_grader.core.stepik_client.wait_for_auth_code", return_value="C"
+            ) as mock_wait,
+            patch("stepik_grader.core.stepik_client.requests.post", return_value=token_resp),
+        ):
+            authorize_via_browser("cid", "cs", "http://localhost:8080/cb")
+
+        opened_url = mock_open.call_args.args[0]
+        sent_state = mock_wait.call_args.args[3]
+        assert f"state={sent_state}" in opened_url
+
+    def test_state_differs_between_calls(self):
+        """Каждый вызов генерирует новый state (не хардкод/константа)."""
+        token_resp = MagicMock()
+        token_resp.raise_for_status = MagicMock()
+        token_resp.json.return_value = {"access_token": "AT", "expires_in": 100}
+        states = []
+        with (
+            patch("stepik_grader.core.stepik_client.webbrowser.open"),
+            patch(
+                "stepik_grader.core.stepik_client.wait_for_auth_code", return_value="C"
+            ) as mock_wait,
+            patch("stepik_grader.core.stepik_client.requests.post", return_value=token_resp),
+        ):
+            authorize_via_browser("cid", "cs", "http://localhost:8080/cb")
+            states.append(mock_wait.call_args.args[3])
+            authorize_via_browser("cid", "cs", "http://localhost:8080/cb")
+            states.append(mock_wait.call_args.args[3])
+        assert states[0] != states[1]
 
 
 class TestGetWithRetryNoAttempts: