From 41b3afed146c714c5491bfa7212d6d610904ea97 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 14 Sep 2026 21:46:09 +0100 Subject: [PATCH] fix: stop leaking connector OAuth session tokens to other origins The connector OAuth popup posted the session token to window.opener with a '*' target origin, so any page that opened the popup received it. An attacker with an account on a multi-user deployment could start a flow for their own pending session, get a victim to finish the provider consent, and receive a token backed by the victim's Drive/SharePoint/Confluence tokens. - Post popup results only to allowed frontend origins: the callback origin, OIDC_FRONTEND_URL, the new CONNECTOR_ALLOWED_ORIGINS, and localhost:5173 when the callback runs on a loopback host. - Render the success page from the callback itself so the token never appears in a URL; callback-status ignores session_token/user_email params. - ConnectorAuth accepts messages only from the popup it opened, on the callback origin reported by /api/connectors/auth. - /api/connectors/disconnect requires auth and only deletes the caller's session. - /api/connectors/sync and /api/remote reject session tokens the caller does not own. Fixes #2766 --- .../Integrations/confluence-connector.mdx | 2 + .../Integrations/google-drive-connector.mdx | 2 + .../Integrations/sharepoint-connector.mdx | 2 + docsgpt/api/connector/routes.py | 229 +++++++++----- docsgpt/api/user/sources/upload.py | 10 + docsgpt/core/settings.py | 3 + .../db/repositories/connector_sessions.py | 8 +- frontend/src/components/ConnectorAuth.tsx | 14 + frontend/src/utils/connectorAuthUtils.test.ts | 58 ++++ frontend/src/utils/connectorAuthUtils.ts | 12 + tests/api/test_connector_oauth_security.py | 282 ++++++++++++++++++ tests/api/test_connector_routes_happy.py | 22 +- .../user/sources/test_upload_idempotency.py | 7 + tests/test_extra_coverage.py | 11 +- 14 files changed, 575 insertions(+), 87 deletions(-) create mode 100644 frontend/src/utils/connectorAuthUtils.test.ts create mode 100644 frontend/src/utils/connectorAuthUtils.ts create mode 100644 tests/api/test_connector_oauth_security.py diff --git a/docs/content/Guides/Integrations/confluence-connector.mdx b/docs/content/Guides/Integrations/confluence-connector.mdx index e69458ae..6b0625ce 100644 --- a/docs/content/Guides/Integrations/confluence-connector.mdx +++ b/docs/content/Guides/Integrations/confluence-connector.mdx @@ -52,6 +52,7 @@ VITE_CONFLUENCE_CLIENT_ID=your-atlassian-client-id | `CONFLUENCE_CLIENT_ID` | Client ID from your Atlassian OAuth app | Yes | | `CONFLUENCE_CLIENT_SECRET` | Client secret from your Atlassian OAuth app | Yes | | `VITE_CONFLUENCE_CLIENT_ID` | Same Client ID, used by the frontend to show the Confluence option | Yes | +| `CONNECTOR_ALLOWED_ORIGINS` | Comma-separated frontend origins allowed to receive the sign-in result, e.g. `https://docsgpt.example.com`. Not needed when the frontend shares the API origin or runs on `localhost:5173` | When the frontend is on its own origin | ### Step 5: Restart and Use @@ -62,6 +63,7 @@ Restart your application, then go to the upload section in DocsGPT and select ** ## Troubleshooting - **Option not appearing** — Verify `VITE_CONFLUENCE_CLIENT_ID` is set in the frontend `.env`, then restart. +- **Sign-in popup closes but the account never connects** — The frontend origin is not allowed to receive the result. Add it to `CONNECTOR_ALLOWED_ORIGINS` in the backend `.env`. - **Authentication failed** — Check that the callback URL matches exactly, including `?provider=confluence`. - **No accessible sites** — Ensure the authenticating user has access to at least one Confluence Cloud site. - **Permission denied** — Verify that the Confluence API scopes are enabled in your Atlassian app settings. diff --git a/docs/content/Guides/Integrations/google-drive-connector.mdx b/docs/content/Guides/Integrations/google-drive-connector.mdx index 278dfb83..9dfcbde3 100644 --- a/docs/content/Guides/Integrations/google-drive-connector.mdx +++ b/docs/content/Guides/Integrations/google-drive-connector.mdx @@ -50,6 +50,7 @@ VITE_GOOGLE_CLIENT_ID=your-google-client-id | `GOOGLE_CLIENT_ID` | OAuth Client ID from GCP Credentials | Yes | | `GOOGLE_CLIENT_SECRET` | OAuth Client Secret from GCP Credentials | Yes | | `VITE_GOOGLE_CLIENT_ID` | Same Client ID, used by the frontend to show the Google Drive option | Yes | +| `CONNECTOR_ALLOWED_ORIGINS` | Comma-separated frontend origins allowed to receive the sign-in result, e.g. `https://docsgpt.example.com`. Not needed when the frontend shares the API origin or runs on `localhost:5173` | When the frontend is on its own origin | Make sure to use the same Google Client ID in both backend and frontend configurations. @@ -65,6 +66,7 @@ Restart your application, then go to the upload section in DocsGPT and select ** - **Option not appearing** — Verify `VITE_GOOGLE_CLIENT_ID` is set in the frontend `.env`, then restart. - **Authentication failed** — Check that the redirect URI matches exactly, including `?provider=google_drive`. Ensure the Google Drive API is enabled. +- **Sign-in popup closes but the account never connects** — The frontend origin is not allowed to receive the result. Add it to `CONNECTOR_ALLOWED_ORIGINS` in the backend `.env`. - **Permission denied** — Verify the OAuth consent screen is configured and the user has access to the target files. - **Files not processing** — Check backend logs and verify that backend environment variables are correctly set. diff --git a/docs/content/Guides/Integrations/sharepoint-connector.mdx b/docs/content/Guides/Integrations/sharepoint-connector.mdx index cf8b27bf..0b2a7c7b 100644 --- a/docs/content/Guides/Integrations/sharepoint-connector.mdx +++ b/docs/content/Guides/Integrations/sharepoint-connector.mdx @@ -45,6 +45,7 @@ MICROSOFT_TENANT_ID=your-azure-ad-tenant-id | `MICROSOFT_CLIENT_SECRET` | Client secret value | Yes | — | | `MICROSOFT_TENANT_ID` | Directory (tenant) ID | No | `common` | | `MICROSOFT_AUTHORITY` | Login endpoint override | No | Auto-constructed | +| `CONNECTOR_ALLOWED_ORIGINS` | Comma-separated frontend origins allowed to receive the sign-in result, e.g. `https://docsgpt.example.com` | When the frontend is on its own origin | API origin and `localhost:5173` | `MICROSOFT_TENANT_ID=common` (the default) allows any Microsoft account to authenticate. Set this to your specific tenant ID in production. @@ -60,4 +61,5 @@ Restart your application, then go to the upload section in DocsGPT and select ** - **Option not appearing** — Verify `MICROSOFT_CLIENT_ID` and `MICROSOFT_CLIENT_SECRET` are set, then restart. - **Authentication failed** — Check that the redirect URI matches exactly, including `?provider=share_point`. +- **Sign-in popup closes but the account never connects** — The frontend origin is not allowed to receive the result. Add it to `CONNECTOR_ALLOWED_ORIGINS` in the backend `.env`. - **Permission denied** — Ensure admin consent is granted and the user has access to the target files. diff --git a/docsgpt/api/connector/routes.py b/docsgpt/api/connector/routes.py index 7669c567..cd32f7d6 100644 --- a/docsgpt/api/connector/routes.py +++ b/docsgpt/api/connector/routes.py @@ -2,7 +2,8 @@ import base64 import html import json import uuid -from urllib.parse import urlencode +from typing import Optional +from urllib.parse import urlencode, urlsplit from flask import ( @@ -19,6 +20,7 @@ from docsgpt.api import api from docsgpt.api.user.tasks import ( ingest_connector_task, ) +from docsgpt.core.settings import settings from docsgpt.parser.connectors.connector_creator import ConnectorCreator from docsgpt.storage.db.repositories.connector_sessions import ( ConnectorSessionsRepository, @@ -44,6 +46,125 @@ def build_callback_redirect(params: dict) -> str: return f"{CALLBACK_STATUS_PATH}?{urlencode(params)}" +_LOOPBACK_HOSTS = frozenset({"localhost", "127.0.0.1", "::1"}) +_DEV_FRONTEND_PORT = 5173 + + +def _origin_of(url: Optional[str]) -> Optional[str]: + """Normalized ``scheme://host[:port]`` origin of an http(s) URL, or None.""" + if not url: + return None + try: + parts = urlsplit(url.strip()) + port = parts.port + except ValueError: + return None + host = parts.hostname + if parts.scheme not in ("http", "https") or not host: + return None + if ":" in host: + host = f"[{host}]" + if port is None or port == {"http": 80, "https": 443}[parts.scheme]: + return f"{parts.scheme}://{host}" + return f"{parts.scheme}://{host}:{port}" + + +def connector_allowed_origins(request_host_url: str) -> list[str]: + """Frontend origins the OAuth popup may hand a connector session token to.""" + candidates = [ + request_host_url, + settings.CONNECTOR_REDIRECT_BASE_URI, + settings.OIDC_FRONTEND_URL, + *(settings.CONNECTOR_ALLOWED_ORIGINS or "").split(","), + ] + callback_origin = _origin_of(settings.CONNECTOR_REDIRECT_BASE_URI) + if callback_origin: + callback = urlsplit(callback_origin) + if callback.hostname in _LOOPBACK_HOSTS: + for port in (_DEV_FRONTEND_PORT, callback.port or 80): + candidates += [f"http://localhost:{port}", f"http://127.0.0.1:{port}"] + origins: list[str] = [] + for candidate in candidates: + origin = _origin_of(candidate) + if origin and origin not in origins: + origins.append(origin) + return origins + + +def _js_literal(value) -> str: + """Encode a value as a JavaScript literal that is safe inside an inline script.""" + return json.dumps(value).replace("Connected as: {html.escape(user_email)}

" if status == "success" and user_email else "" + ) + closing_note = ( + f"Your {provider} is now connected and ready to use." if status == "success" + else "Feel free to close this window." + ) + html_content = f""" + + + + {provider} Authentication + + + + +
+

{provider} Authentication

+
+

{html.escape(message)}

+ {connected_as} +
+

You can close this window. {closing_note}

+
+ + + """ + return make_response( + html_content, + 200, + {"Content-Type": "text/html", "Cache-Control": "no-store", "Referrer-Policy": "no-referrer"}, + ) + + @connectors_ns.route("/api/connectors/auth") class ConnectorAuth(Resource): @@ -78,7 +199,8 @@ class ConnectorAuth(Resource): return make_response(jsonify({ "success": True, "authorization_url": authorization_url, - "state": state + "state": state, + "callback_origin": _origin_of(settings.CONNECTOR_REDIRECT_BASE_URI), }), 200) except Exception as e: current_app.logger.error(f"Error generating connector auth URL: {e}", exc_info=True) @@ -172,14 +294,12 @@ class ConnectorsCallback(Resource): if not updated: repo.update_by_legacy_id(value, patch) - # Redirect to success page with session token and user email - return redirect(build_callback_redirect({ - "status": "success", - "message": "Authentication successful", - "provider": provider, - "session_token": session_token, - "user_email": user_email - })) + # Render instead of redirecting so the session token never + # lands in a URL (browser history, access logs, Referer). + return _render_callback_page( + "success", "Authentication successful", provider, + session_token=session_token, user_email=user_email, + ) except Exception as e: current_app.logger.error(f"Error exchanging code for tokens: {str(e)}", exc_info=True) @@ -345,6 +465,9 @@ class ConnectorDisconnect(Resource): @api.expect(api.model("ConnectorDisconnectModel", {"provider": fields.String(required=True), "session_token": fields.String(required=False)})) @api.doc(description="Disconnect a connector session") def post(self): + decoded_token = request.decoded_token + if not decoded_token: + return make_response(jsonify({"success": False, "error": "Unauthorized"}), 401) try: data = request.get_json() provider = data.get('provider') @@ -352,11 +475,10 @@ class ConnectorDisconnect(Resource): if not provider: return make_response(jsonify({"success": False, "error": "provider is required"}), 400) - if session_token: with db_session() as conn: ConnectorSessionsRepository(conn).delete_by_session_token( - session_token, + session_token, decoded_token.get('sub'), ) return make_response(jsonify({"success": True}), 200) @@ -428,6 +550,14 @@ class ConnectorSync(Resource): 400 ) + with db_readonly() as conn: + session = ConnectorSessionsRepository(conn).get_by_session_token(session_token) + if not session or session.get("user_id") != user_id: + return make_response( + jsonify({"success": False, "error": "Invalid or unauthorized session"}), + 401, + ) + # Extract configuration from remote_data file_ids = remote_data.get('file_ids', []) folder_ids = remote_data.get('folder_ids', []) @@ -476,74 +606,13 @@ class ConnectorCallbackStatus(Resource): def get(self): """Return HTML page with connector authentication status""" try: - # Validate and sanitize status to a known value - status_raw = request.args.get('status', 'error') - status = status_raw if status_raw in ('success', 'error', 'cancelled') else 'error' - - # Escape all user-controlled values for HTML context - message = html.escape(request.args.get('message', '')) - provider_raw = request.args.get('provider', 'connector') - provider = html.escape(provider_raw.replace('_', ' ').title()) - session_token = request.args.get('session_token', '') - user_email = html.escape(request.args.get('user_email', '')) - - def safe_js_string(value: str) -> str: - """Safely encode a string for embedding in inline JavaScript.""" - js_encoded = json.dumps(value) - return js_encoded.replace(' - - - {provider} Authentication - - - - -
-

{provider} Authentication

-
-

{message}

- {f'

Connected as: {user_email}

' if status == 'success' else ''} -
-

You can close this window. {f"Your {provider} is now connected and ready to use." if status == 'success' else "Feel free to close this window."}

-
- - - """ - - return make_response(html_content, 200, {'Content-Type': 'text/html'}) + # Query params are attacker-controllable, so this page never + # carries a session token; the OAuth callback renders that itself. + return _render_callback_page( + request.args.get('status', 'error'), + request.args.get('message', ''), + request.args.get('provider', 'connector'), + ) except Exception as e: current_app.logger.error(f"Error rendering callback status page: {e}") return make_response("Authentication error occurred", 500, {'Content-Type': 'text/html'}) diff --git a/docsgpt/api/user/sources/upload.py b/docsgpt/api/user/sources/upload.py index 97971c48..953a3453 100644 --- a/docsgpt/api/user/sources/upload.py +++ b/docsgpt/api/user/sources/upload.py @@ -24,6 +24,7 @@ from docsgpt.security.zip_archive import ( ZipExtractionError, ZipExtractionLimits, ) +from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository from docsgpt.storage.db.repositories.idempotency import IdempotencyRepository from docsgpt.storage.db.repositories.sources import SourcesRepository from docsgpt.storage.db.source_config import SourceConfig @@ -496,6 +497,15 @@ class UploadRemote(Resource): ), 400, ) + with db_readonly() as conn: + connector_session = ConnectorSessionsRepository(conn).get_by_session_token(session_token) + if not connector_session or connector_session.get("user_id") != user: + if scoped_key: + _release_claim(scoped_key) + return make_response( + jsonify({"success": False, "error": "Invalid or unauthorized session"}), + 401, + ) # Process file_ids file_ids = config.get("file_ids", []) diff --git a/docsgpt/core/settings.py b/docsgpt/core/settings.py index a04861b3..c73c4501 100644 --- a/docsgpt/core/settings.py +++ b/docsgpt/core/settings.py @@ -251,6 +251,9 @@ class Settings(BaseSettings): CONNECTOR_REDIRECT_BASE_URI: Optional[str] = ( "http://127.0.0.1:7091/api/connectors/callback" ##add redirect url as it is to your provider's console(gcp) ) + # Comma-separated frontend origins allowed to receive connector OAuth results, e.g. https://docsgpt.example.com. + # The callback origin and OIDC_FRONTEND_URL are always allowed; a loopback callback also allows localhost:5173. + CONNECTOR_ALLOWED_ORIGINS: Optional[str] = None # Microsoft Entra ID (Azure AD) integration MICROSOFT_CLIENT_ID: Optional[str] = None # Azure AD Application (client) ID diff --git a/docsgpt/storage/db/repositories/connector_sessions.py b/docsgpt/storage/db/repositories/connector_sessions.py index 87f8ed0b..ee859b88 100644 --- a/docsgpt/storage/db/repositories/connector_sessions.py +++ b/docsgpt/storage/db/repositories/connector_sessions.py @@ -307,11 +307,13 @@ class ConnectorSessionsRepository: result = self._conn.execute(text(sql), params) return result.rowcount > 0 - def delete_by_session_token(self, session_token: str) -> bool: + def delete_by_session_token(self, session_token: str, user_id: str) -> bool: + """Delete the session behind ``session_token`` only if ``user_id`` owns it.""" result = self._conn.execute( text( - "DELETE FROM connector_sessions WHERE session_token = :token" + "DELETE FROM connector_sessions " + "WHERE session_token = :token AND user_id = :user_id" ), - {"token": session_token}, + {"token": session_token, "user_id": user_id}, ) return result.rowcount > 0 diff --git a/frontend/src/components/ConnectorAuth.tsx b/frontend/src/components/ConnectorAuth.tsx index c2bb0192..f18c077e 100644 --- a/frontend/src/components/ConnectorAuth.tsx +++ b/frontend/src/components/ConnectorAuth.tsx @@ -5,6 +5,7 @@ import { useSelector } from 'react-redux'; import userService from '../api/services/userService'; import { useDarkTheme } from '../hooks'; import { selectToken } from '../preferences/preferenceSlice'; +import { isTrustedConnectorMessage } from '../utils/connectorAuthUtils'; import { Button } from './ui/button'; interface ConnectorAuthProps { @@ -34,6 +35,8 @@ const ConnectorAuth: React.FC = ({ const completedRef = useRef(false); const intervalRef = useRef(null); const authWindowRef = useRef(null); + // Origin the OAuth callback page is served from, as reported by the backend. + const callbackOriginRef = useRef(null); // Hold the exact listener identity so unmount cleanup removes the same fn. const messageHandlerRef = useRef<((event: MessageEvent) => void) | null>( null, @@ -54,6 +57,16 @@ const ConnectorAuth: React.FC = ({ }; const handleAuthMessage = (event: MessageEvent) => { + // Only the popup we opened, on the callback origin, may report a result. + if ( + !isTrustedConnectorMessage( + event, + authWindowRef.current, + callbackOriginRef.current, + ) + ) { + return; + } const successGeneric = event.data?.type === 'connector_auth_success'; const successProvider = event.data?.type === `${provider}_auth_success`; const errorProvider = event.data?.type === `${provider}_auth_error`; @@ -139,6 +152,7 @@ const ConnectorAuth: React.FC = ({ onError(t('modals.uploadDoc.connectors.auth.authCancelled')); return; } + callbackOriginRef.current = authData.callback_origin || null; authWindow.location.href = authData.authorization_url; messageHandlerRef.current = handleAuthMessage; diff --git a/frontend/src/utils/connectorAuthUtils.test.ts b/frontend/src/utils/connectorAuthUtils.test.ts new file mode 100644 index 00000000..f36aa39d --- /dev/null +++ b/frontend/src/utils/connectorAuthUtils.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest'; + +import { isTrustedConnectorMessage } from './connectorAuthUtils'; + +const popup = {} as Window; +const otherWindow = {} as Window; + +describe('isTrustedConnectorMessage', () => { + it('rejects messages when no auth popup is open', () => { + expect( + isTrustedConnectorMessage( + { source: popup, origin: 'https://api.example.com' }, + null, + 'https://api.example.com', + ), + ).toBe(false); + }); + + it('rejects messages that do not come from the auth popup', () => { + expect( + isTrustedConnectorMessage( + { source: otherWindow, origin: 'https://api.example.com' }, + popup, + 'https://api.example.com', + ), + ).toBe(false); + }); + + it('rejects messages from the popup when its origin is unexpected', () => { + expect( + isTrustedConnectorMessage( + { source: popup, origin: 'https://evil.example.com' }, + popup, + 'https://api.example.com', + ), + ).toBe(false); + }); + + it('accepts messages from the popup on the callback origin', () => { + expect( + isTrustedConnectorMessage( + { source: popup, origin: 'https://api.example.com' }, + popup, + 'https://api.example.com', + ), + ).toBe(true); + }); + + it('falls back to the popup identity check when no origin is known', () => { + expect( + isTrustedConnectorMessage( + { source: popup, origin: 'https://api.example.com' }, + popup, + null, + ), + ).toBe(true); + }); +}); diff --git a/frontend/src/utils/connectorAuthUtils.ts b/frontend/src/utils/connectorAuthUtils.ts new file mode 100644 index 00000000..7342f41d --- /dev/null +++ b/frontend/src/utils/connectorAuthUtils.ts @@ -0,0 +1,12 @@ +/** + * Whether a window message is the OAuth result posted by the connector popup we opened. + * `expectedOrigin` is the callback origin reported by the backend, when known. + */ +export const isTrustedConnectorMessage = ( + event: Pick, + authWindow: Window | null, + expectedOrigin?: string | null, +): boolean => { + if (!authWindow || event.source !== authWindow) return false; + return !expectedOrigin || event.origin === expectedOrigin; +}; diff --git a/tests/api/test_connector_oauth_security.py b/tests/api/test_connector_oauth_security.py new file mode 100644 index 00000000..e4572779 --- /dev/null +++ b/tests/api/test_connector_oauth_security.py @@ -0,0 +1,282 @@ +"""Connector OAuth hardening: popup message origin, token exposure, session ownership.""" + +import base64 +import json +from contextlib import contextmanager +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask + + +@pytest.fixture +def app(): + return Flask(__name__) + + +@contextmanager +def _patch_db(conn, module="docsgpt.api.connector.routes"): + @contextmanager + def _yield(): + yield conn + + with patch(f"{module}.db_session", _yield), patch(f"{module}.db_readonly", _yield): + yield + + +def _encode_state(payload): + return base64.urlsafe_b64encode(json.dumps(payload).encode()).decode() + + +def _seed_session(pg_conn, user, token, provider="google_drive"): + from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository + + repo = ConnectorSessionsRepository(pg_conn) + row = repo.upsert(user, provider, status="authorized") + repo.update(str(row["id"]), {"session_token": token}) + return repo + + +class TestConnectorAllowedOrigins: + def test_collects_configured_origins(self): + from docsgpt.api.connector.routes import connector_allowed_origins + from docsgpt.core.settings import settings + + with patch.object(settings, "CONNECTOR_ALLOWED_ORIGINS", "https://app.example.com/, https://b.example.com/x"), \ + patch.object(settings, "OIDC_FRONTEND_URL", "https://sso.example.com/home"), \ + patch.object(settings, "CONNECTOR_REDIRECT_BASE_URI", "https://api.example.com/api/connectors/callback"): + origins = connector_allowed_origins("https://api.example.com/") + + assert set(origins) == { + "https://app.example.com", + "https://b.example.com", + "https://sso.example.com", + "https://api.example.com", + } + + def test_rejects_wildcards_and_non_http_values(self): + from docsgpt.api.connector.routes import connector_allowed_origins + from docsgpt.core.settings import settings + + with patch.object(settings, "CONNECTOR_ALLOWED_ORIGINS", "*, javascript:alert(1), null, not a url"), \ + patch.object(settings, "OIDC_FRONTEND_URL", None), \ + patch.object(settings, "CONNECTOR_REDIRECT_BASE_URI", "https://api.example.com/api/connectors/callback"): + origins = connector_allowed_origins("https://api.example.com/") + + assert origins == ["https://api.example.com"] + + def test_loopback_dev_frontend_allowed_only_for_loopback_callback(self): + from docsgpt.api.connector.routes import connector_allowed_origins + from docsgpt.core.settings import settings + + with patch.object(settings, "CONNECTOR_ALLOWED_ORIGINS", None), \ + patch.object(settings, "OIDC_FRONTEND_URL", None), \ + patch.object(settings, "CONNECTOR_REDIRECT_BASE_URI", "http://127.0.0.1:7091/api/connectors/callback"): + local = connector_allowed_origins("http://127.0.0.1:7091/") + with patch.object(settings, "CONNECTOR_ALLOWED_ORIGINS", None), \ + patch.object(settings, "OIDC_FRONTEND_URL", None), \ + patch.object(settings, "CONNECTOR_REDIRECT_BASE_URI", "https://api.example.com/api/connectors/callback"): + public = connector_allowed_origins("http://localhost:7091/") + + assert "http://localhost:5173" in local + assert "http://127.0.0.1:5173" in local + assert "http://localhost:5173" not in public + + +class TestCallbackStatusPage: + def test_never_posts_to_wildcard_origin(self, app): + from docsgpt.api.connector.routes import ConnectorCallbackStatus + + with app.test_request_context( + "/api/connectors/callback-status?status=success&provider=google_drive" + ): + r = ConnectorCallbackStatus().get() + body = r.get_data(as_text=True) + assert r.status_code == 200 + assert "'*'" not in body + assert '"*"' not in body + + def test_ignores_session_token_query_param(self, app): + from docsgpt.api.connector.routes import ConnectorCallbackStatus + + with app.test_request_context( + "/api/connectors/callback-status?status=success&provider=google_drive" + "&session_token=attacker-supplied&user_email=evil@example.com" + ): + r = ConnectorCallbackStatus().get() + body = r.get_data(as_text=True) + assert "attacker-supplied" not in body + assert "evil@example.com" not in body + + +class TestCallbackDeliversTokenSafely: + def test_success_renders_page_without_redirecting_token(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorsCallback + from docsgpt.core.settings import settings + from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository + + user = "u-cb-origin" + repo = ConnectorSessionsRepository(pg_conn) + pending = repo.upsert(user, "google_drive", status="pending") + state = _encode_state({"provider": "google_drive", "object_id": str(pending["id"])}) + + fake_auth = MagicMock() + fake_auth.exchange_code_for_tokens.return_value = {"access_token": "at"} + fake_auth.sanitize_token_info.return_value = {"access_token": "at"} + fake_auth.create_credentials_from_token_info.side_effect = RuntimeError("no creds") + + with _patch_db(pg_conn), patch( + "docsgpt.api.connector.routes.ConnectorCreator.is_supported", return_value=True, + ), patch( + "docsgpt.api.connector.routes.ConnectorCreator.create_auth", return_value=fake_auth, + ), patch.object( + settings, "CONNECTOR_ALLOWED_ORIGINS", "https://app.example.com", + ), app.test_request_context( + f"/api/connectors/callback?state={state}&code=auth-code", + base_url="https://api.example.com", + ): + r = ConnectorsCallback().get() + + token = repo.get_by_user_provider(user, "google_drive")["session_token"] + body = r.get_data(as_text=True) + assert token + assert r.status_code == 200 + assert "Location" not in r.headers + assert token in body + assert '"https://app.example.com"' in body + assert "'*'" not in body + assert r.headers["Cache-Control"] == "no-store" + assert r.headers["Referrer-Policy"] == "no-referrer" + + +class TestAuthUrlReportsCallbackOrigin: + def test_includes_callback_origin(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorAuth + from docsgpt.core.settings import settings + + fake_auth = MagicMock() + fake_auth.get_authorization_url.return_value = "https://ex/auth?state=x" + + with _patch_db(pg_conn), patch( + "docsgpt.api.connector.routes.ConnectorCreator.is_supported", return_value=True, + ), patch( + "docsgpt.api.connector.routes.ConnectorCreator.create_auth", return_value=fake_auth, + ), patch.object( + settings, "CONNECTOR_REDIRECT_BASE_URI", "https://api.example.com/api/connectors/callback", + ), app.test_request_context("/api/connectors/auth?provider=google_drive"): + from flask import request + request.decoded_token = {"sub": "u-auth-origin"} + r = ConnectorAuth().get() + + assert r.status_code == 200 + assert r.json["callback_origin"] == "https://api.example.com" + + +class TestDisconnectOwnership: + def test_requires_authentication(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorDisconnect + + repo = _seed_session(pg_conn, "u-owner", "st-noauth") + with _patch_db(pg_conn), app.test_request_context( + "/api/connectors/disconnect", method="POST", + json={"provider": "google_drive", "session_token": "st-noauth"}, + ): + from flask import request + request.decoded_token = None + r = ConnectorDisconnect().post() + + assert r.status_code == 401 + assert repo.get_by_session_token("st-noauth") is not None + + def test_cannot_delete_another_users_session(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorDisconnect + + repo = _seed_session(pg_conn, "u-victim", "st-victim") + with _patch_db(pg_conn), app.test_request_context( + "/api/connectors/disconnect", method="POST", + json={"provider": "google_drive", "session_token": "st-victim"}, + ): + from flask import request + request.decoded_token = {"sub": "u-attacker"} + ConnectorDisconnect().post() + + assert repo.get_by_session_token("st-victim") is not None + + def test_owner_can_delete_own_session(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorDisconnect + + repo = _seed_session(pg_conn, "u-self", "st-self") + with _patch_db(pg_conn), app.test_request_context( + "/api/connectors/disconnect", method="POST", + json={"provider": "google_drive", "session_token": "st-self"}, + ): + from flask import request + request.decoded_token = {"sub": "u-self"} + r = ConnectorDisconnect().post() + + assert r.status_code == 200 + assert repo.get_by_session_token("st-self") is None + + +class TestSyncSessionOwnership: + def test_rejects_foreign_session_token(self, app, pg_conn): + from docsgpt.api.connector.routes import ConnectorSync + from docsgpt.storage.db.repositories.sources import SourcesRepository + + _seed_session(pg_conn, "u-victim-sync", "st-victim-sync") + attacker = "u-attacker-sync" + src = SourcesRepository(pg_conn).create( + "drive-src", user_id=attacker, + remote_data={"provider": "google_drive", "file_ids": ["f"], "folder_ids": []}, + ) + + delay = MagicMock() + with _patch_db(pg_conn), patch( + "docsgpt.api.connector.routes.ingest_connector_task.delay", delay, + ), app.test_request_context( + "/api/connectors/sync", method="POST", + json={"source_id": str(src["id"]), "session_token": "st-victim-sync"}, + ): + from flask import request + request.decoded_token = {"sub": attacker} + r = ConnectorSync().post() + + assert r.status_code == 401 + delay.assert_not_called() + + +class TestRemoteUploadSessionOwnership: + def _post(self, app, pg_conn, user, token, apply_mock): + from docsgpt.api.user.sources.upload import UploadRemote + + with _patch_db(pg_conn, "docsgpt.api.user.sources.upload"), patch( + "docsgpt.api.user.sources.upload.ingest_connector_task.apply_async", apply_mock, + ), app.test_request_context( + "/api/remote", method="POST", + data={ + "user": user, "source": "google_drive", "name": "g", + "data": json.dumps({"session_token": token, "file_ids": ["f1"]}), + }, + content_type="multipart/form-data", + ): + from flask import request + request.decoded_token = {"sub": user} + return UploadRemote().post() + + def test_rejects_foreign_session_token(self, app, pg_conn): + _seed_session(pg_conn, "u-victim-up", "st-victim-up") + apply_mock = MagicMock(return_value=MagicMock(id="t")) + + r = self._post(app, pg_conn, "u-attacker-up", "st-victim-up", apply_mock) + + assert r.status_code == 401 + apply_mock.assert_not_called() + + def test_accepts_own_session_token(self, app, pg_conn): + _seed_session(pg_conn, "u-owner-up", "st-owner-up") + apply_mock = MagicMock(return_value=MagicMock(id="t")) + + r = self._post(app, pg_conn, "u-owner-up", "st-owner-up", apply_mock) + + assert r.status_code == 200 + apply_mock.assert_called_once() diff --git a/tests/api/test_connector_routes_happy.py b/tests/api/test_connector_routes_happy.py index 4fb5493d..141f87b0 100644 --- a/tests/api/test_connector_routes_happy.py +++ b/tests/api/test_connector_routes_happy.py @@ -199,8 +199,11 @@ class TestConnectorsCallback: f"/api/connectors/callback?state={state}&code=auth-code" ): r = ConnectorsCallback().get() - assert r.status_code == 302 - assert "status=success" in r.location + token = ConnectorSessionsRepository(pg_conn).get_by_user_provider( + "u-callback", "google_drive", + )["session_token"] + assert r.status_code == 200 + assert token and token in r.get_data(as_text=True) def test_token_exchange_failure_redirects_error(self, app, pg_conn): from docsgpt.api.connector.routes import ConnectorsCallback @@ -425,6 +428,8 @@ class TestConnectorDisconnect: with app.test_request_context( "/api/connectors/disconnect", method="POST", json={} ): + from flask import request + request.decoded_token = {"sub": "u"} r = ConnectorDisconnect().post() assert r.status_code == 400 @@ -444,9 +449,12 @@ class TestConnectorDisconnect: method="POST", json={"provider": "google_drive", "session_token": "st-disc"}, ): + from flask import request + request.decoded_token = {"sub": user} r = ConnectorDisconnect().post() assert r.status_code == 200 assert r.json["success"] is True + assert repo.get_by_session_token("st-disc") is None def test_disconnect_without_session_token_succeeds(self, app): from docsgpt.api.connector.routes import ConnectorDisconnect @@ -456,6 +464,8 @@ class TestConnectorDisconnect: method="POST", json={"provider": "google_drive"}, ): + from flask import request + request.decoded_token = {"sub": "u"} r = ConnectorDisconnect().post() assert r.status_code == 200 @@ -530,6 +540,14 @@ class TestConnectorSync: remote_data={"provider": "github", "file_ids": [], "folder_ids": []}, ) + from docsgpt.storage.db.repositories.connector_sessions import ( + ConnectorSessionsRepository, + ) + + repo = ConnectorSessionsRepository(pg_conn) + session = repo.upsert(user, "github", status="authorized") + repo.update(str(session["id"]), {"session_token": "st"}) + fake_task = MagicMock(id="task-abc") with _patch_db(pg_conn), patch( "docsgpt.api.connector.routes.ingest_connector_task.delay", diff --git a/tests/api/user/sources/test_upload_idempotency.py b/tests/api/user/sources/test_upload_idempotency.py index a3b842af..f5b069bc 100644 --- a/tests/api/user/sources/test_upload_idempotency.py +++ b/tests/api/user/sources/test_upload_idempotency.py @@ -597,6 +597,13 @@ class TestRemoteIdempotency: pytest.skip("no connectors registered in this build") connector_source = next(iter(supported)) + from docsgpt.storage.db.repositories.connector_sessions import ( + ConnectorSessionsRepository, + ) + repo = ConnectorSessionsRepository(pg_conn) + session = repo.upsert("u", connector_source, status="authorized") + repo.update(str(session["id"]), {"session_token": "tok"}) + with _patch_db(pg_conn), patch( "docsgpt.api.user.sources.upload.ingest_connector_task.apply_async", apply_mock, diff --git a/tests/test_extra_coverage.py b/tests/test_extra_coverage.py index 58a6822e..fcb3333f 100644 --- a/tests/test_extra_coverage.py +++ b/tests/test_extra_coverage.py @@ -158,12 +158,17 @@ class TestSourcesUploadExtra: response = UploadRemote().post() assert response.status_code == 400 - def test_remote_connector_triggers_task(self, app): + def test_remote_connector_triggers_task(self, app, pg_conn): from docsgpt.api.user.sources.upload import UploadRemote + from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository import json as _json + repo = ConnectorSessionsRepository(pg_conn) + session = repo.upsert("u", "google_drive", status="authorized") + repo.update(str(session["id"]), {"session_token": "st"}) + fake_task = MagicMock(id="conn-t") - with patch( + with _patch_upload_db(pg_conn), patch( "docsgpt.parser.connectors.connector_creator.ConnectorCreator" ".get_supported_connectors", return_value={"google_drive"}, @@ -210,6 +215,8 @@ class TestConnectorExtra: "/api/connectors/disconnect", method="POST", json={"provider": "x", "session_token": "y"}, ): + from flask import request + request.decoded_token = {"sub": "u"} response = ConnectorDisconnect().post() assert response.status_code == 500