From c651181345aa9884d33bd2df0ceb1ae994583129 Mon Sep 17 00:00:00 2001 From: skypank Date: Thu, 27 Aug 2026 15:34:18 +0530 Subject: [PATCH 1/4] =?UTF-8?q?feat(api):=20migrate=20OAuth=20routes=20to?= =?UTF-8?q?=20/rest/v1/auth/*=20(RFC=20#876=20TODO=201)=20=E2=80=94=20#963?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/hooks/useResourceSelection.test.ts | 5 +- .../src/hooks/useResourceSelection.ts | 5 +- .../frontend/src/hooks/useUser.test.ts | 66 +++++ application/frontend/src/hooks/useUser.ts | 6 +- .../frontend/src/pages/chatbot/chatbot.tsx | 6 +- application/tests/admin_imports_api_test.py | 22 ++ application/tests/auth_routes_test.py | 258 ++++++++++++++++++ application/tests/user_resources_api_test.py | 15 +- application/web/web_main.py | 99 +++++-- 9 files changed, 455 insertions(+), 27 deletions(-) create mode 100644 application/frontend/src/hooks/useUser.test.ts create mode 100644 application/tests/auth_routes_test.py diff --git a/application/frontend/src/hooks/useResourceSelection.test.ts b/application/frontend/src/hooks/useResourceSelection.test.ts index a7b39ee32..26d353ffd 100644 --- a/application/frontend/src/hooks/useResourceSelection.test.ts +++ b/application/frontend/src/hooks/useResourceSelection.test.ts @@ -44,7 +44,10 @@ describe('useResourceSelection', () => { const { getByTestId } = render(React.createElement(Probe)); await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); - expect(fetchMock).toHaveBeenCalledWith('/rest/v1/user/resources', { method: 'GET' }); + expect(fetchMock).toHaveBeenCalledWith('/rest/v1/user/resources', { + method: 'GET', + headers: { Accept: 'application/json' }, + }); expect(getByTestId('selected').textContent).toBe('ASVS,CWE'); }); diff --git a/application/frontend/src/hooks/useResourceSelection.ts b/application/frontend/src/hooks/useResourceSelection.ts index 0f6a90d47..77d5919c9 100644 --- a/application/frontend/src/hooks/useResourceSelection.ts +++ b/application/frontend/src/hooks/useResourceSelection.ts @@ -29,7 +29,10 @@ export const useResourceSelection = (): ResourceSelectionState => { const load = async () => { try { - const res = await fetch(`${apiUrl}/user/resources`, { method: 'GET' }); + const res = await fetch(`${apiUrl}/user/resources`, { + method: 'GET', + headers: { Accept: 'application/json' }, + }); if (res.status === 401) { return; // anonymous / feature not available — not an error } diff --git a/application/frontend/src/hooks/useUser.test.ts b/application/frontend/src/hooks/useUser.test.ts new file mode 100644 index 000000000..3f1245a70 --- /dev/null +++ b/application/frontend/src/hooks/useUser.test.ts @@ -0,0 +1,66 @@ +import { act, render, waitFor } from '@testing-library/react'; +import React from 'react'; + +import { useUser } from './useUser'; + +jest.mock('./useEnvironment', () => ({ + useEnvironment: () => ({ name: 'test', apiUrl: '/rest/v1' }), +})); + +// react-testing-library v11 has no renderHook; drive the hook via a probe. +type Captured = ReturnType; +let captured: Captured; + +function Probe(): React.ReactElement { + captured = useUser(); + return React.createElement('span', { 'data-testid': 'loading' }, String(captured.loading)); +} + +function jsonResponse(body: unknown, status = 200): Response { + return { + status, + ok: status >= 200 && status < 300, + json: () => Promise.resolve(body), + text: () => Promise.resolve(typeof body === 'string' ? body : JSON.stringify(body)), + } as unknown as Response; +} + +describe('useUser (auth route migration #963)', () => { + const originalLocation = window.location; + + beforeEach(() => { + delete (window as any).location; + (window as any).location = { href: '' }; + }); + + afterEach(() => { + (window as any).location = originalLocation; + jest.resetAllMocks(); + }); + + it('GETs /rest/v1/auth/user with Accept: application/json (so anon gets 401, not a Google redirect)', async () => { + const fetchMock = jest.fn().mockResolvedValueOnce(jsonResponse(null, 401)); + (global as any).fetch = fetchMock; + + const { getByTestId } = render(React.createElement(Probe)); + await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); + + expect(fetchMock).toHaveBeenCalledWith('/rest/v1/auth/user', { + method: 'GET', + headers: { Accept: 'application/json' }, + }); + }); + + it('login() navigates to /auth/login and logout() to /auth/logout', async () => { + (global as any).fetch = jest.fn().mockResolvedValueOnce(jsonResponse(null, 401)); + + const { getByTestId } = render(React.createElement(Probe)); + await waitFor(() => expect(getByTestId('loading').textContent).toBe('false')); + + act(() => captured.login()); + expect((window as any).location.href).toBe('/rest/v1/auth/login'); + + act(() => captured.logout()); + expect((window as any).location.href).toBe('/rest/v1/auth/logout'); + }); +}); diff --git a/application/frontend/src/hooks/useUser.ts b/application/frontend/src/hooks/useUser.ts index 0575fe365..91a967e55 100644 --- a/application/frontend/src/hooks/useUser.ts +++ b/application/frontend/src/hooks/useUser.ts @@ -15,7 +15,7 @@ export const useUser = () => { useEffect(() => { let active = true; - fetch(`${apiUrl}/user`, { method: 'GET' }) + fetch(`${apiUrl}/auth/user`, { method: 'GET', headers: { Accept: 'application/json' } }) .then((res) => { if (res.status === 200) { return res.text(); @@ -51,11 +51,11 @@ export const useUser = () => { }, [apiUrl]); const login = () => { - window.location.href = `${apiUrl}/login`; + window.location.href = `${apiUrl}/auth/login`; }; const logout = () => { - window.location.href = `${apiUrl}/logout`; + window.location.href = `${apiUrl}/auth/logout`; }; return { user, isLoggedIn: user !== null, loading, login, logout }; diff --git a/application/frontend/src/pages/chatbot/chatbot.tsx b/application/frontend/src/pages/chatbot/chatbot.tsx index 698ae0128..79a175632 100644 --- a/application/frontend/src/pages/chatbot/chatbot.tsx +++ b/application/frontend/src/pages/chatbot/chatbot.tsx @@ -88,12 +88,12 @@ export const Chatbot = () => { }, [chatMessages]); function login() { - fetch(`${apiUrl}/user`, { method: 'GET' }) + fetch(`${apiUrl}/auth/user`, { method: 'GET', headers: { Accept: 'application/json' } }) .then((response) => { if (response.status === 200) { response.text().then((user) => setUser(user)); } else { - window.location.href = `${apiUrl}/login`; + window.location.href = `${apiUrl}/auth/login`; } }) .catch((error) => { @@ -156,7 +156,7 @@ export const Chatbot = () => { fetch(`${apiUrl}/completion`, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, body: JSON.stringify({ prompt: currentTerm }), }) .then(async (response) => { diff --git a/application/tests/admin_imports_api_test.py b/application/tests/admin_imports_api_test.py index 718982bba..26d9cbc69 100644 --- a/application/tests/admin_imports_api_test.py +++ b/application/tests/admin_imports_api_test.py @@ -94,6 +94,28 @@ def test_admin_imports_disabled_flag_returns_404(self) -> None: os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True ) def test_admin_imports_requires_login(self) -> None: + with self.app.test_client() as c: + # API client (Accept: application/json) gets a 401; browsers (Accept: + # text/html) get a 302 to the login flow (login_required content + # negotiation, #963 — default is now 401). + r = c.get("/admin/imports/runs", headers={"Accept": "application/json"}) + self.assertEqual(r.status_code, 401) + + @patch.dict( + os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True + ) + def test_admin_imports_star_accept_returns_401(self) -> None: + # /admin/* tooling with curl's default Accept "*/*" must get a clean 401, + # not a 302 into login HTML (the case Spyros called out). + with self.app.test_client() as c: + r = c.get("/admin/imports/runs", headers={"Accept": "*/*"}) + self.assertEqual(r.status_code, 401) + + @patch.dict( + os.environ, {"CRE_ALLOW_IMPORT": "1", "INSECURE_REQUESTS": "1"}, clear=True + ) + def test_admin_imports_no_accept_header_returns_401(self) -> None: + # /admin/* tooling with no Accept header at all -> 401, not 302. with self.app.test_client() as c: r = c.get("/admin/imports/runs") self.assertEqual(r.status_code, 401) diff --git a/application/tests/auth_routes_test.py b/application/tests/auth_routes_test.py new file mode 100644 index 000000000..bdf48e37e --- /dev/null +++ b/application/tests/auth_routes_test.py @@ -0,0 +1,258 @@ +"""Tests for the /rest/v1/auth/* migration (issue #963, RFC #876 TODO 1). + +Canonical auth routes, deprecated aliases (header-only), login_required content +negotiation (browser 302 vs JSON 401), the user_id-keyed session predicate, and +the NO_LOGIN dev bypass. OpenAPI documentation is intentionally out of scope here. +""" + +import os +import unittest +import urllib.parse +from typing import Any +from unittest.mock import patch + +from application import create_app, sqla +from application.database import db + + +class TestAuthRoutes(unittest.TestCase): + def setUp(self) -> None: + self._prev_no_load_graph = os.environ.get("NO_LOAD_GRAPH_DB") + os.environ["NO_LOAD_GRAPH_DB"] = "1" + self.app = create_app(mode="test") + self.app.secret_key = "test-secret" + self.app_context = self.app.app_context() + self.app_context.push() + sqla.create_all() + + def tearDown(self) -> None: + sqla.session.remove() + sqla.drop_all() + self.app_context.pop() + if self._prev_no_load_graph is None: + os.environ.pop("NO_LOAD_GRAPH_DB", None) + else: + os.environ["NO_LOAD_GRAPH_DB"] = self._prev_no_load_graph + + # --- canonical routes --- + def test_auth_logout_clears_session_and_redirects(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + resp = client.get("/rest/v1/auth/logout") + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.headers["Location"].endswith("/")) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + + def test_auth_login_dev_bypass_sets_session(self) -> None: + with patch.dict( + os.environ, + {"NO_LOGIN": "1", "CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"}, + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/login") + self.assertEqual(resp.status_code, 302) + self.assertTrue(resp.headers["Location"].endswith("/chatbot")) + with client.session_transaction() as sess: + self.assertIn("user_id", sess) + + def test_auth_user_returns_email_when_logged_in(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + sess["email"] = "e@x.com" + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data.decode(), "e@x.com") + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_sets_user_id( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-xyz", + "name": "Test User", + "email": "test@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + client.get("/rest/v1/auth/callback?state=xyz") + with client.session_transaction() as sess: + self.assertIn("user_id", sess) + self.assertEqual(sqla.session.query(db.User).count(), 1) + + # --- deprecated aliases: header-only --- + def test_logout_alias_carries_deprecation_header(self) -> None: + with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + resp = client.get("/rest/v1/logout") + self.assertEqual(resp.status_code, 302) + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/logout", resp.headers.get("Link", "")) + self.assertIn("successor-version", resp.headers.get("Link", "")) + + def test_user_alias_carries_deprecation_header(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["user_id"] = "uid" + sess["email"] = "e@x.com" + resp = client.get( + "/rest/v1/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/user", resp.headers.get("Link", "")) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_callback_alias_carries_deprecation_header( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-abc", + "name": "T", + "email": "t@x.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/callback?state=xyz") + # Header-only: not a redirect to the canonical path (still runs + # the OAuth flow and lands on /chatbot). + self.assertEqual(resp.headers.get("Deprecation"), "true") + self.assertIn("/rest/v1/auth/callback", resp.headers.get("Link", "")) + + # --- login_required content negotiation --- + def test_login_required_json_returns_401(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 401) + + def test_login_required_browser_redirects_to_auth_login_with_next(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user", headers={"Accept": "text/html"}) + self.assertEqual(resp.status_code, 302) + loc = resp.headers["Location"] + self.assertTrue(loc.startswith("/rest/v1/auth/login?next=")) + self.assertIn("/rest/v1/auth/user", urllib.parse.unquote(loc)) + + def test_login_required_browser_multivalue_accept_redirects(self) -> None: + # A real browser sends a multi-value Accept; "text/html" is present as a + # substring, so the browser still gets the 302 (not an exact-match check). + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", + headers={ + "Accept": ( + "text/html,application/xhtml+xml," + "application/xml;q=0.9,*/*;q=0.8" + ) + }, + ) + self.assertEqual(resp.status_code, 302) + self.assertTrue( + resp.headers["Location"].startswith("/rest/v1/auth/login?next=") + ) + + def test_login_required_star_accept_returns_401(self) -> None: + # curl's default Accept is "*/*": tooling must get a clean 401, NOT a 302 + # into login HTML. This is the key assertion of the inverted default. + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user", headers={"Accept": "*/*"}) + self.assertEqual(resp.status_code, 401) + + def test_login_required_no_accept_header_returns_401(self) -> None: + # No Accept header at all (scripts/requests without one) -> 401, not 302. + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.get("/rest/v1/auth/user") + self.assertEqual(resp.status_code, 401) + + # --- the re-baselined predicate: keyed on user_id, not google_id+name --- + def test_session_with_google_id_but_no_user_id_is_anonymous(self) -> None: + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["google_id"] = "sub-1" + sess["name"] = "U" # but NO user_id + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 401) + + def test_completion_anonymous_json_returns_401(self) -> None: + # /rest/v1/completion is login_required; an anonymous API client + # (Accept: application/json) must get a clean 401, not the browser 302 + # toward Google (which the chatbot's fetch would fail to follow). + with patch.dict( + os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} + ): + with self.app.test_client() as client: + resp = client.post( + "/rest/v1/completion", + json={"prompt": "x"}, + headers={"Accept": "application/json"}, + ) + self.assertEqual(resp.status_code, 401) + + # --- NO_LOGIN dev bypass preserved --- + def test_no_login_bypasses_login_required(self) -> None: + with patch.dict(os.environ, {"NO_LOGIN": "1", "INSECURE_REQUESTS": "1"}): + with self.app.test_client() as client: + resp = client.get( + "/rest/v1/auth/user", headers={"Accept": "application/json"} + ) + self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.data.decode(), "foobar") + + +if __name__ == "__main__": + unittest.main() diff --git a/application/tests/user_resources_api_test.py b/application/tests/user_resources_api_test.py index f8eb019cf..a1ccc1a0b 100644 --- a/application/tests/user_resources_api_test.py +++ b/application/tests/user_resources_api_test.py @@ -39,7 +39,13 @@ def tearDown(self) -> None: os.environ["NO_LOAD_GRAPH_DB"] = self._prev_no_load_graph def _login(self, client: Any, google_sub: str = "sub-1", name: str = "U") -> None: + # login_required keys off session['user_id'] (post-#980, #963), so a test + # session must carry it. Resolve/create the user and record its id. + user = self.collection.upsert_user( + google_sub=google_sub, email="", display_name=name + ) with client.session_transaction() as sess: + sess["user_id"] = user.id sess["google_id"] = google_sub sess["name"] = name @@ -74,7 +80,10 @@ def test_get_401_when_anonymous(self) -> None: }, ): with self.app.test_client() as client: - resp = client.get("/rest/v1/user/resources") + resp = client.get( + "/rest/v1/user/resources", + headers={"Accept": "application/json"}, + ) self.assertEqual(resp.status_code, 401) def test_put_401_when_anonymous(self) -> None: @@ -88,7 +97,9 @@ def test_put_401_when_anonymous(self) -> None: ): with self.app.test_client() as client: resp = client.put( - "/rest/v1/user/resources", json={"selected": ["ASVS"]} + "/rest/v1/user/resources", + json={"selected": ["ASVS"]}, + headers={"Accept": "application/json"}, ) self.assertEqual(resp.status_code, 401) diff --git a/application/web/web_main.py b/application/web/web_main.py index 604e75e9b..004b4f0e0 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -35,6 +35,7 @@ Blueprint, abort, jsonify, + make_response, redirect, request, send_from_directory, @@ -882,19 +883,52 @@ def add_header(response): return response +def _is_logged_in() -> bool: + """Single source of truth for session presence. + + Keyed on ``session['user_id']`` (recorded by the login flow since #980) — not + ``google_id``/``name``. Both ``login_required`` and its content-negotiation + branch route through here, so the predicate lives in exactly one place. + """ + return "user_id" in session + + +def _safe_next(target: str) -> str: + """Return an open-redirect-safe relative ``next`` target (default '/').""" + if target and target.startswith("/") and not target.startswith("//"): + return target + return "/" + + +def _auth_challenge(): + """Response for an unauthenticated request, negotiated by Accept. + + Default is a clean 401 so tooling (curl/requests/scripts, a bare ``*/*`` or + a missing Accept header) and ``/admin/*`` tooling get a machine-readable + challenge instead of being 302'd into login HTML. Only real browsers -- which + advertise ``text/html`` in Accept (e.g. + ``text/html,application/xhtml+xml,...;q=0.9,*/*;q=0.8``) -- get the 302 to the + login flow with a safe relative ``?next``. Matched as a substring, so a bare + ``*/*`` does not qualify. + """ + if "text/html" in request.headers.get("Accept", ""): + next_url = _safe_next(request.full_path) + return redirect(f"/rest/v1/auth/login?next={urllib.parse.quote(next_url)}") + allowed_domains = os.environ.get("LOGIN_ALLOWED_DOMAINS") + abort( + 401, + description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", + ) + + def login_required(f): @wraps(f) def login_r(*args, **kwargs): if os.environ.get("NO_LOGIN") == "1": return f(*args, **kwargs) - if "google_id" not in session or "name" not in session: - allowed_domains = os.environ.get("LOGIN_ALLOWED_DOMAINS") - abort( - 401, - description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", - ) - else: - return f(*args, **kwargs) + if not _is_logged_in(): + return _auth_challenge() + return f(*args, **kwargs) return login_r @@ -1241,8 +1275,8 @@ def __init__(sel): raise ValueError("class is a singleton, please call instance() instead") -@app.route("/rest/v1/login") -def login(): +@app.route("/rest/v1/auth/login") +def auth_login(): if os.environ.get("NO_LOGIN") == "1": session["state"] = {"state": True} session["google_id"] = "some dev id" @@ -1269,16 +1303,16 @@ def login(): return redirect(authorization_url) -@app.route("/rest/v1/user") +@app.route("/rest/v1/auth/user") @login_required -def logged_in_user(): +def auth_user(): if os.environ.get("NO_LOGIN") == "1": return "foobar" return session.get("email") -@app.route("/rest/v1/callback") -def callback(): +@app.route("/rest/v1/auth/callback") +def auth_callback(): flow_instance = CREFlow.instance() try: flow_instance.flow.fetch_token( @@ -1287,7 +1321,7 @@ def callback(): except oauthlib.oauth2.rfc6749.errors.MismatchingStateError as mse: return redirect("/chatbot") if not session.get("state") or session.get("state") != request.args["state"]: - redirect(url_for("web.login")) # State does not match! + redirect(url_for("web.auth_login")) # State does not match! credentials = flow_instance.flow.credentials token_request = google.auth.transport.requests.Request() id_info = id_token.verify_oauth2_token( @@ -1342,12 +1376,43 @@ def callback(): return redirect("/chatbot") -@app.route("/rest/v1/logout") -def logout(): +@app.route("/rest/v1/auth/logout") +def auth_logout(): session.clear() return redirect("/") +# --- Deprecated auth aliases (RFC #876 TODO 1 migration) -------------------- +# Keep the old paths working during the migration but signal deprecation. These +# are header-only (no redirect to the canonical path), so /callback's OAuth +# code/state flow is left untouched. +def _deprecated_auth(response, canonical): + resp = make_response(response) + resp.headers["Deprecation"] = "true" + resp.headers["Link"] = f'<{canonical}>; rel="successor-version"' + return resp + + +@app.route("/rest/v1/login") +def login(): + return _deprecated_auth(auth_login(), "/rest/v1/auth/login") + + +@app.route("/rest/v1/user") +def logged_in_user(): + return _deprecated_auth(auth_user(), "/rest/v1/auth/user") + + +@app.route("/rest/v1/callback") +def callback(): + return _deprecated_auth(auth_callback(), "/rest/v1/auth/callback") + + +@app.route("/rest/v1/logout") +def logout(): + return _deprecated_auth(auth_logout(), "/rest/v1/auth/logout") + + @openapi_documented("get_user_resources") @app.route("/rest/v1/user/resources", methods=["GET"]) @feature_enabled_or_default( From 3cc8a0ca1e40b9ddae2d9dca10ed435eb12c10ba Mon Sep 17 00:00:00 2001 From: skypank Date: Thu, 27 Aug 2026 16:37:33 +0530 Subject: [PATCH 2/4] =?UTF-8?q?fix(api):=20address=20#963=20review=20?= =?UTF-8?q?=E2=80=94=20callback=20redirect=5Furi,=20login-failure,=20drop?= =?UTF-8?q?=20dead=20next?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Point the OAuth redirect_uri at the canonical url_for("web.auth_callback") instead of the deprecated web.callback alias (CodeRabbit). - On the OIDC callback, fail explicitly instead of leaving a broken session: abort 503 when user persistence raises SQLAlchemyError, abort 401 when the provider returns no 'sub'. Previously these logged and redirected to /chatbot without session['user_id'], bouncing the user into an endless login loop. - Drop the dead '?next=' from the browser auth challenge (auth_login never consumed it and the callback always lands on /chatbot); redirect to the constant /rest/v1/auth/login. Removes the CodeQL "URL redirection from remote source" finding and the now-unused _safe_next helper. - Tests: assert the no-next redirect; add callback persistence-failure (503) and missing-sub (401) cases. --- application/tests/auth_routes_test.py | 79 +++++++++++++++++++++++++-- application/web/web_main.py | 63 +++++++++++---------- 2 files changed, 107 insertions(+), 35 deletions(-) diff --git a/application/tests/auth_routes_test.py b/application/tests/auth_routes_test.py index bdf48e37e..e4afed043 100644 --- a/application/tests/auth_routes_test.py +++ b/application/tests/auth_routes_test.py @@ -7,7 +7,6 @@ import os import unittest -import urllib.parse from typing import Any from unittest.mock import patch @@ -99,6 +98,71 @@ def test_auth_callback_sets_user_id( self.assertIn("user_id", sess) self.assertEqual(sqla.session.query(db.User).count(), 1) + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + @patch("application.web.web_main.db.Node_collection") + def test_auth_callback_persistence_failure_returns_503( + self, node_collection_mock: Any, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # If upsert_user fails, we must NOT redirect as if logged in (that leaves + # a broken session that fails every login_required call). Surface a + # retryable 503 and leave user_id unset. + from sqlalchemy.exc import SQLAlchemyError + + id_token_mock.verify_oauth2_token.return_value = { + "sub": "sub-boom", + "name": "T", + "email": "t@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + node_collection_mock.return_value.upsert_user.side_effect = SQLAlchemyError( + "db down" + ) + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/auth/callback?state=xyz") + self.assertEqual(resp.status_code, 503) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_missing_sub_returns_401( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # No 'sub' claim -> identity can't be established -> explicit 401, not a + # silently broken session. + id_token_mock.verify_oauth2_token.return_value = { + "sub": None, + "name": "T", + "email": "t@example.com", + } + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "xyz" + resp = client.get("/rest/v1/auth/callback?state=xyz") + self.assertEqual(resp.status_code, 401) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + # --- deprecated aliases: header-only --- def test_logout_alias_carries_deprecation_header(self) -> None: with patch.dict(os.environ, {"INSECURE_REQUESTS": "1"}): @@ -163,16 +227,19 @@ def test_login_required_json_returns_401(self) -> None: ) self.assertEqual(resp.status_code, 401) - def test_login_required_browser_redirects_to_auth_login_with_next(self) -> None: + def test_login_required_browser_redirects_to_auth_login(self) -> None: with patch.dict( os.environ, {"CRE_ENABLE_LOGIN": "1", "INSECURE_REQUESTS": "1"} ): with self.app.test_client() as client: resp = client.get("/rest/v1/auth/user", headers={"Accept": "text/html"}) self.assertEqual(resp.status_code, 302) - loc = resp.headers["Location"] - self.assertTrue(loc.startswith("/rest/v1/auth/login?next=")) - self.assertIn("/rest/v1/auth/user", urllib.parse.unquote(loc)) + # Redirect to the constant login route -- no ?next (auth_login does + # not consume a return target; callback always lands on /chatbot). + self.assertTrue( + resp.headers["Location"].endswith("/rest/v1/auth/login") + ) + self.assertNotIn("next=", resp.headers["Location"]) def test_login_required_browser_multivalue_accept_redirects(self) -> None: # A real browser sends a multi-value Accept; "text/html" is present as a @@ -192,7 +259,7 @@ def test_login_required_browser_multivalue_accept_redirects(self) -> None: ) self.assertEqual(resp.status_code, 302) self.assertTrue( - resp.headers["Location"].startswith("/rest/v1/auth/login?next=") + resp.headers["Location"].endswith("/rest/v1/auth/login") ) def test_login_required_star_accept_returns_401(self) -> None: diff --git a/application/web/web_main.py b/application/web/web_main.py index 004b4f0e0..946b1f43d 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -893,13 +893,6 @@ def _is_logged_in() -> bool: return "user_id" in session -def _safe_next(target: str) -> str: - """Return an open-redirect-safe relative ``next`` target (default '/').""" - if target and target.startswith("/") and not target.startswith("//"): - return target - return "/" - - def _auth_challenge(): """Response for an unauthenticated request, negotiated by Accept. @@ -908,12 +901,16 @@ def _auth_challenge(): challenge instead of being 302'd into login HTML. Only real browsers -- which advertise ``text/html`` in Accept (e.g. ``text/html,application/xhtml+xml,...;q=0.9,*/*;q=0.8``) -- get the 302 to the - login flow with a safe relative ``?next``. Matched as a substring, so a bare - ``*/*`` does not qualify. + login flow. Matched as a substring, so a bare ``*/*`` does not qualify. + + The redirect target is the constant login route with no ``?next`` param: + ``auth_login`` does not consume a return target and ``auth_callback`` always + lands on ``/chatbot``, so forwarding the request path here would be a dead + (and taint-flagged) value. Return-to-page is a separate, future feature that + would store a validated target in the session at login time. """ if "text/html" in request.headers.get("Accept", ""): - next_url = _safe_next(request.full_path) - return redirect(f"/rest/v1/auth/login?next={urllib.parse.quote(next_url)}") + return redirect("/rest/v1/auth/login") allowed_domains = os.environ.get("LOGIN_ALLOWED_DOMAINS") abort( 401, @@ -1266,7 +1263,7 @@ def instance(cls): "openid", ], redirect_uri=( - request.root_url.rstrip("/") + url_for("web.callback") + request.root_url.rstrip("/") + url_for("web.auth_callback") ).replace("http://", "https://"), ) return cls.__instance @@ -1351,28 +1348,36 @@ def auth_callback(): description=f"You need an account with one of the following providers to access this functionality {allowed_domains}", ) - # Persist the account when login is enabled; the session keeps working - # unchanged if this no-ops (flag off) or fails. + # Persist the account when login is enabled. ``session['user_id']`` is what + # ``_is_logged_in`` checks, so if persistence cannot establish it we must NOT + # redirect as if login succeeded: that would leave a "logged-in-looking" + # session that fails every ``login_required`` call and bounces the user back + # into the login flow. Fail explicitly (retryable) instead. if is_login_enabled(): google_sub = id_info.get("sub") if not google_sub: logger.error( - "OIDC callback returned no 'sub' claim; skipping user persistence" + "OIDC callback returned no 'sub' claim; cannot establish session" + ) + abort( + 401, description="Login failed: identity provider returned no subject" + ) + try: + user = db.Node_collection().upsert_user( + google_sub=google_sub, + email=id_info.get("email") or "", + display_name=id_info.get("name"), + ) + session["user_id"] = user.id + except SQLAlchemyError as e: + # Log only the exception class: the message can carry SQL parameters + # such as the user's email or OIDC subject. Surface a retryable 503 + # rather than a silently broken session. + logger.error("failed to persist user on login: %s", type(e).__name__) + abort( + 503, + description="Login temporarily unavailable, please try again", ) - else: - try: - user = db.Node_collection().upsert_user( - google_sub=google_sub, - email=id_info.get("email") or "", - display_name=id_info.get("name"), - ) - session["user_id"] = user.id - except SQLAlchemyError as e: - # Keep DB failures soft so persistence can never block login, but - # let unexpected (non-DB) bugs surface instead of being swallowed. - # Log only the exception class: the message can carry SQL - # parameters such as the user's email or OIDC subject. - logger.error("failed to persist user on login: %s", type(e).__name__) return redirect("/chatbot") From cb7cedb251cb9767af9101175b28d3eff6f9c79b Mon Sep 17 00:00:00 2001 From: skypank Date: Wed, 2 Sep 2026 11:41:50 +0530 Subject: [PATCH 3/4] chore(api): exempt canonical /rest/v1/auth/* routes from OpenAPI guardrail (#963) The four canonical auth routes (/rest/v1/auth/{login,callback,logout,user}) are OAuth/redirect endpoints, not part of the documented read-only public API. Add them to OPENAPI_GUARDRAIL_EXEMPT_RULES alongside their deprecated pre-#963 aliases, matching how the old paths were already treated. No PathSpecs and no openapi.yaml regeneration -- the guardrail's route-coverage check passes because these rules are exempt, not documented. --- application/web/openapi_registry.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/application/web/openapi_registry.py b/application/web/openapi_registry.py index 7a65b0239..4d2677e44 100644 --- a/application/web/openapi_registry.py +++ b/application/web/openapi_registry.py @@ -21,6 +21,13 @@ OPENAPI_GUARDRAIL_EXEMPT_RULES: Set[str] = { "/rest/v1/openapi.yaml", + # Canonical OAuth/auth routes (#963). Auth/redirect endpoints, not part of + # the documented read-only public API -- exempt rather than documented. + "/rest/v1/auth/login", + "/rest/v1/auth/callback", + "/rest/v1/auth/logout", + "/rest/v1/auth/user", + # Deprecated pre-#963 aliases of the above (header-only successor links). "/rest/v1/login", "/rest/v1/callback", "/rest/v1/logout", From 13ff2897a2fe5182183da2e7145ed491ddf3de8e Mon Sep 17 00:00:00 2001 From: Spyros Date: Sat, 5 Sep 2026 00:12:20 +0100 Subject: [PATCH 4/4] fix(auth): return on OAuth state mismatch (#1021) The canonical /rest/v1/auth/callback path called redirect() without return, so token verification and session writes continued after a state mismatch. Add a regression test. --- application/tests/auth_routes_test.py | 28 +++++++++++++++++++++++++++ application/web/web_main.py | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/application/tests/auth_routes_test.py b/application/tests/auth_routes_test.py index e4afed043..9d6f8f059 100644 --- a/application/tests/auth_routes_test.py +++ b/application/tests/auth_routes_test.py @@ -98,6 +98,34 @@ def test_auth_callback_sets_user_id( self.assertIn("user_id", sess) self.assertEqual(sqla.session.query(db.User).count(), 1) + @patch("application.web.web_main.id_token") + @patch("application.web.web_main.CREFlow") + def test_auth_callback_state_mismatch_returns_without_continuing( + self, cre_flow_mock: Any, id_token_mock: Any + ) -> None: + # Regression for #1021: missing ``return`` on the state-mismatch redirect + # let the handler continue into token verification / session writes. + cre_flow_mock.instance.return_value.flow.credentials._id_token = "tok" + with patch.dict( + os.environ, + { + "CRE_ENABLE_LOGIN": "1", + "LOGIN_ALLOWED_DOMAINS": "*", + "INSECURE_REQUESTS": "1", + }, + ): + with self.app.test_client() as client: + with client.session_transaction() as sess: + sess["state"] = "expected-state" + resp = client.get("/rest/v1/auth/callback?state=wrong-state") + self.assertEqual(resp.status_code, 302) + self.assertIn("/rest/v1/auth/login", resp.headers.get("Location", "")) + with client.session_transaction() as sess: + self.assertNotIn("user_id", sess) + self.assertNotIn("google_id", sess) + id_token_mock.verify_oauth2_token.assert_not_called() + self.assertEqual(sqla.session.query(db.User).count(), 0) + @patch("application.web.web_main.id_token") @patch("application.web.web_main.CREFlow") @patch("application.web.web_main.db.Node_collection") diff --git a/application/web/web_main.py b/application/web/web_main.py index 946b1f43d..a40b5fa7d 100644 --- a/application/web/web_main.py +++ b/application/web/web_main.py @@ -1318,7 +1318,7 @@ def auth_callback(): except oauthlib.oauth2.rfc6749.errors.MismatchingStateError as mse: return redirect("/chatbot") if not session.get("state") or session.get("state") != request.args["state"]: - redirect(url_for("web.auth_login")) # State does not match! + return redirect(url_for("web.auth_login")) # State does not match! credentials = flow_instance.flow.credentials token_request = google.auth.transport.requests.Request() id_info = id_token.verify_oauth2_token(