From 3572d968ec365066977491f3e8e2724188a80a06 Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:14:07 +0100 Subject: [PATCH] Read private repositories only with the user's own GitHub token GITHUB_ACCESS_TOKEN belongs to the server, but any user could ingest any repository it can see, private ones included. It is now used only for public repositories; the loader checks the repository's visibility first and asks for a GitHub connection otherwise. The loader also takes a token from the connection a source syncs from. Merging a connection's keys into a plain repository URL no longer fails on json.loads, manual Sync now passes the source's connection, and a token GitHub rejects pauses the connection's sources for reconnect. --- docsgpt/api/user/sources/routes.py | 2 + docsgpt/api/user/tasks.py | 2 + docsgpt/connectors/service.py | 21 ++++ docsgpt/parser/remote/github_loader.py | 111 +++++++++++++++++++--- docsgpt/worker.py | 39 ++++++-- tests/api/user/sources/test_routes.py | 32 +++++++ tests/api/user/test_tasks.py | 12 ++- tests/connectors/test_runtime.py | 56 +++++++++++ tests/parser/remote/test_github_loader.py | 104 ++++++++++++++++++++ 9 files changed, 357 insertions(+), 22 deletions(-) diff --git a/docsgpt/api/user/sources/routes.py b/docsgpt/api/user/sources/routes.py index 53fe8b69..f4e06e30 100644 --- a/docsgpt/api/user/sources/routes.py +++ b/docsgpt/api/user/sources/routes.py @@ -452,6 +452,8 @@ class SyncSource(Resource): sync_frequency=doc.get("sync_frequency", "never"), retriever=doc.get("retriever", "classic"), doc_id=str(doc["id"]), + # S3 and GitHub sources made from a connection read with its keys. + connection_id=str(doc["connection_id"]) if doc.get("connection_id") else None, ) except Exception as err: current_app.logger.error( diff --git a/docsgpt/api/user/tasks.py b/docsgpt/api/user/tasks.py index 94f5fcb6..996c3b49 100644 --- a/docsgpt/api/user/tasks.py +++ b/docsgpt/api/user/tasks.py @@ -277,6 +277,7 @@ def sync_source( sync_frequency, retriever, doc_id, + connection_id=None, ): resp = sync( self, @@ -287,6 +288,7 @@ def sync_source( sync_frequency, retriever, doc_id, + connection_id=connection_id, ) return resp diff --git a/docsgpt/connectors/service.py b/docsgpt/connectors/service.py index 1a18c0f2..f8a24bc4 100644 --- a/docsgpt/connectors/service.py +++ b/docsgpt/connectors/service.py @@ -654,6 +654,27 @@ def get_credentials(row: dict) -> dict: return dict(secrets.get("credentials") or {}) +def access_credentials(row: dict) -> dict: + """What a loader or tool authenticates with through this connection. + + Pasted credentials for an ``api_key`` connection. An OAuth connection + whose token a loader or tool sends itself (GitHub's App sign-in) gives + its current access token as ``access_token``, refreshed first when it + has expired. + + Raises: + ConnectionUnavailable: Disconnected, flagged, undecryptable, or the + refresh was refused (the connection is then flagged). + TransientConnectionError: The provider failed in a retryable way. + """ + if (row.get("auth_kind") or "") == "oauth": + status = normalize_status(row) + if status in (STATUS_DISCONNECTED, STATUS_RECONNECT): + raise ConnectionUnavailable(f"Connection is {status}", connection_id=str(row["id"]), status=status) + return {"access_token": get_valid_token_info(str(row["id"])).get("access_token")} + return get_credentials(row) + + def _api_key_account( repo: ConnectorSessionsRepository, user_id: str, diff --git a/docsgpt/parser/remote/github_loader.py b/docsgpt/parser/remote/github_loader.py index 3ba4a55c..9a997c5c 100644 --- a/docsgpt/parser/remote/github_loader.py +++ b/docsgpt/parser/remote/github_loader.py @@ -3,7 +3,7 @@ import logging import mimetypes import time from concurrent.futures import ThreadPoolExecutor -from typing import Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, Union import requests @@ -39,6 +39,14 @@ SKIP_SUFFIXES = ( ) +class GitHubTokenRejected(PermissionError): + """GitHub answered 401 to a user's own token (revoked or expired).""" + + +class PrivateRepositoryError(PermissionError): + """A repository the instance-wide token may not read for a user.""" + + class GitHubLoader(BaseRemote): """Load a GitHub repository's text files as ``Document`` objects. @@ -48,15 +56,74 @@ class GitHubLoader(BaseRemote): downloads the survivors in parallel. """ - def __init__(self): - self.access_token = settings.GITHUB_ACCESS_TOKEN - self.headers = { - "Authorization": f"token {self.access_token}", - "Accept": "application/vnd.github.v3+json" - } if self.access_token else { - "Accept": "application/vnd.github.v3+json" - } - return + def __init__(self, access_token: Optional[str] = None): + """Create a loader. + + Args: + access_token: A user's own GitHub token (from their GitHub + connection). Without one the instance-wide + ``GITHUB_ACCESS_TOKEN`` is used, and only for public + repositories. + """ + self._user_token = access_token + self._use_token(access_token or settings.GITHUB_ACCESS_TOKEN) + + def _use_token(self, token: Optional[str]) -> None: + """Send ``token`` (or nothing) on every following request.""" + self.access_token = token + self.headers = {"Accept": "application/vnd.github.v3+json"} + if token: + self.headers["Authorization"] = f"Bearer {token}" + + @staticmethod + def _parse_inputs(inputs: Union[str, Dict[str, Any]]) -> Tuple[str, Optional[str]]: + """``(repo_url, token)`` from a URL string or a connection's loader input. + + A source synced from a GitHub connection gets a dict: the repository + (``repo_url`` or ``url``) plus the connection's ``access_token``. + """ + if isinstance(inputs, dict): + repo = inputs.get("repo_url") or inputs.get("url") or "" + token = inputs.get("access_token") or inputs.get("token") + return str(repo), (str(token) if token else None) + return str(inputs or ""), None + + def ensure_instance_token_allowed(self, repo_name: str) -> None: + """Refuse a repository the instance-wide token must not read. + + ``GITHUB_ACCESS_TOKEN`` belongs to the server, not to the user asking, + and may see private repositories. Without a user's own token it is + used only for public ones (for their higher rate limit); anything + else needs the user's GitHub connection. + + Raises: + PrivateRepositoryError: The repository is private, internal, or + not visible to the token. + """ + if self._user_token or not self.access_token: + # A user's own token, or anonymous requests (which only ever see + # public repositories). + return + url = f"https://api.github.com/repos/{repo_name}" + response = requests.get(url, headers=self.headers, timeout=30) + if response.status_code == 401: + # A stale instance token: public repositories still read anonymously. + logger.warning("GitHub rejected GITHUB_ACCESS_TOKEN (401); reading %s anonymously", repo_name) + self._use_token(None) + return + if response.status_code == 200: + try: + data = response.json() + except ValueError: + data = {} + if isinstance(data, dict) and data.get("private") is False and ( + data.get("visibility") or "public" + ) == "public": + return + raise PrivateRepositoryError( + f"{repo_name} is private or could not be found. Connect your GitHub account " + "under Connectors to sync private repositories." + ) def is_text_file(self, file_path: str) -> bool: """Determine if a file is a text file based on extension.""" @@ -344,6 +411,10 @@ class GitHubLoader(BaseRemote): raise # If we can't parse the response, raise the original error response.raise_for_status() + elif response.status_code == 401 and self._user_token: + # The user's own token: a retry without it could only read + # public repositories, which is not what they asked for. + raise GitHubTokenRejected("GitHub rejected the connection's token. Reconnect GitHub to continue.") elif response.status_code == 401 and self.access_token: # An expired or revoked PAT makes even public repos 401, which # is strictly worse than not sending one. Retry unauthenticated @@ -404,14 +475,30 @@ class GitHubLoader(BaseRemote): paths = self.fetch_repo_files(repo_name) return self.select_files([(p, 0) for p in paths]) - def load_data(self, repo_url: str) -> List[Document]: - """Load every ingestable text file in ``repo_url`` as a Document.""" + def load_data(self, inputs: Union[str, Dict[str, Any]]) -> List[Document]: + """Load every ingestable text file of a repository as a Document. + + Args: + inputs: The repository URL, or ``{"repo_url", "access_token"}`` + for a source synced from a GitHub connection. + + Raises: + ValueError: ``inputs`` names no github.com repository. + PrivateRepositoryError: Only the instance-wide token is available + and the repository is not public. + GitHubTokenRejected: GitHub refused the connection's token. + """ + repo_url, token = self._parse_inputs(inputs) + if token: + self._user_token = token + self._use_token(token) repo_name = self.normalize_repo(repo_url) if not repo_name or "/" not in repo_name: raise ValueError( f"Not a valid GitHub repository: {repo_url!r}. " "Expected a github.com URL like https://github.com/owner/name." ) + self.ensure_instance_token_allowed(repo_name) branch = self.get_default_branch(repo_name) files = self._list_candidate_files(repo_name, branch) logger.info( diff --git a/docsgpt/worker.py b/docsgpt/worker.py index 6dede185..5e70393c 100755 --- a/docsgpt/worker.py +++ b/docsgpt/worker.py @@ -32,6 +32,7 @@ from docsgpt.parser.file.image_parser import ( VISION_CONVERTIBLE_MIME_TYPES, convert_image_to_png, ) +from docsgpt.parser.remote.github_loader import GitHubTokenRejected from docsgpt.parser.remote.remote_creator import ( RemoteCreator, normalize_remote_data, @@ -1291,7 +1292,19 @@ def remote_worker( from docsgpt.connectors.service import ConnectionUnavailable raise ConnectionUnavailable("Reconnect to continue", connection_id=str(connection_id)) - raw_docs = remote_loader.load_data(loader_input) + try: + raw_docs = remote_loader.load_data(loader_input) + except GitHubTokenRejected as exc: + # A revoked token pauses the connection's sources until the + # owner reconnects, instead of failing on every schedule. + from docsgpt.connectors import service as connection_service + + if not connection_id: + raise + connection_service.mark_reconnect_needed(str(connection_id), str(exc)) + raise connection_service.ConnectionUnavailable( + str(exc), connection_id=str(connection_id), + ) from exc cfg = SourceConfig.parse(config) chunker = ChunkerCreator.create_chunker( @@ -2184,9 +2197,12 @@ def _webhook_tool_allowlist(agent_config): def _with_connection_credentials(source_data, connection_id: str): """Loader input with the connection's stored keys merged in, or None. - S3 and Reddit sources made from a connection keep their keys on the - connection only, never in ``sources.remote_data``. Returns None when the - connection is gone, needs reconnecting, or its connector is turned off. + S3, Reddit and GitHub sources made from a connection keep their keys on + the connection only, never in ``sources.remote_data``. A JSON string + stays a JSON string and a dict a dict; any other string (a GitHub + repository URL) becomes ``{"url": ...}`` next to the keys. Returns None + when the connection is gone, needs reconnecting, or its connector is + turned off. """ from docsgpt.connectors import service from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository @@ -2197,13 +2213,18 @@ def _with_connection_credentials(source_data, connection_id: str): if row is None or not enabled: return None try: - credentials = service.get_credentials(row) + credentials = service.access_credentials(row) except service.ConnectionUnavailable: return None - as_text = isinstance(source_data, str) - data = json.loads(source_data) if as_text else dict(source_data or {}) - data.update(credentials) - return json.dumps(data) if as_text else data + if isinstance(source_data, str): + try: + parsed = json.loads(source_data) + except ValueError: + parsed = None + if isinstance(parsed, dict): + return json.dumps({**parsed, **credentials}) + return {"url": source_data, **credentials} + return {**dict(source_data or {}), **credentials} def _link_source_to_connection(source_id: str, connection_id: str) -> None: diff --git a/tests/api/user/sources/test_routes.py b/tests/api/user/sources/test_routes.py index 0fb117c8..d0504f6f 100644 --- a/tests/api/user/sources/test_routes.py +++ b/tests/api/user/sources/test_routes.py @@ -600,6 +600,38 @@ class TestSyncSource: assert response.status_code == 200 assert response.json["task_id"] == "task-123" + def test_syncs_with_the_sources_connection(self, app, pg_conn): + """A source synced from a connection (S3, GitHub) reads with that connection's keys.""" + from sqlalchemy import text + + from docsgpt.api.user.sources.routes import SyncSource + + user = "u-conn-sync" + cid = str(pg_conn.execute(text( + "INSERT INTO connector_sessions (user_id, provider, connector_key, auth_kind, status) " + "VALUES (:u, 'github', 'github', 'api_key', 'connected') RETURNING id" + ), {"u": user}).scalar()) + src = _seed_source( + pg_conn, user, name="repo", type="github", + remote_data=json.dumps({"repo_url": "acme/private"}), + ) + pg_conn.execute(text("UPDATE sources SET connection_id = CAST(:c AS uuid) WHERE id = :s"), + {"c": cid, "s": src["id"]}) + + with _patch_db(pg_conn), patch( + "docsgpt.api.user.sources.routes.sync_source.delay", + return_value=MagicMock(id="task-conn"), + ) as mock_delay, app.test_request_context( + "/api/sync_source", method="POST", json={"source_id": str(src["id"])}, + ): + from flask import request + request.decoded_token = {"sub": user} + response = SyncSource().post() + + assert response.status_code == 200 + assert mock_delay.call_args.kwargs["connection_id"] == cid + assert mock_delay.call_args.kwargs["source_data"] == "acme/private" + def test_normalizes_dict_remote_data_before_dispatch(self, app, pg_conn): """The route must hand the sync task the normalized URL string.""" from docsgpt.api.user.sources.routes import SyncSource diff --git a/tests/api/user/test_tasks.py b/tests/api/user/test_tasks.py index dcf5be29..e2b6a003 100644 --- a/tests/api/user/test_tasks.py +++ b/tests/api/user/test_tasks.py @@ -165,10 +165,20 @@ class TestSyncSourceTask: ) mock_sync.assert_called_once_with( - ANY, {"data": 1}, "job1", "user1", "web", "daily", "classic", "doc1" + ANY, {"data": 1}, "job1", "user1", "web", "daily", "classic", "doc1", + connection_id=None, ) assert result == {"status": "ok"} + @pytest.mark.unit + @patch("docsgpt.api.user.tasks.sync") + def test_passes_the_sources_connection(self, mock_sync): + from docsgpt.api.user.tasks import sync_source + + sync_source({"data": 1}, "job1", "user1", "s3", "daily", "classic", "doc1", connection_id="c-1") + + assert mock_sync.call_args.kwargs["connection_id"] == "c-1" + class TestStoreAttachmentTask: @pytest.mark.unit diff --git a/tests/connectors/test_runtime.py b/tests/connectors/test_runtime.py index 765dfb98..cde70b67 100644 --- a/tests/connectors/test_runtime.py +++ b/tests/connectors/test_runtime.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import logging from contextlib import contextmanager from types import SimpleNamespace @@ -295,6 +296,61 @@ class TestScheduledSync: ConnectorPoliciesRepository(pg_conn).upsert("s3", enabled=False) assert worker._with_connection_credentials({"bucket": "b"}, cid) is None + def test_repository_url_gets_the_connections_token(self, pg_conn): + """A GitHub source's loader input is a plain URL, not JSON.""" + from docsgpt import worker + + cid = _connection(pg_conn, provider="github", auth_kind="api_key", + secrets={"credentials": {"access_token": "github_pat_x"}}) + + @contextmanager + def _yield(): + yield pg_conn + + with patch.object(worker, "db_readonly", _yield): + data = worker._with_connection_credentials("https://github.com/acme/private", cid) + as_json = worker._with_connection_credentials('{"search_queries": ["x"]}', cid) + assert data == {"url": "https://github.com/acme/private", "access_token": "github_pat_x"} + assert json.loads(as_json)["access_token"] == "github_pat_x" + + def test_oauth_connection_gives_its_current_access_token(self, pg_conn): + """A GitHub App sign-in keeps an OAuth token, refreshed before use.""" + from docsgpt import worker + + cid = _connection(pg_conn, provider="github", auth_kind="oauth", + secrets={"token_info": {"access_token": "ghu_fresh", "refresh_token": "ghr_x"}}) + + @contextmanager + def _yield(): + yield pg_conn + + with patch.object(worker, "db_readonly", _yield), _service_db(pg_conn), patch( + "docsgpt.connectors.service.get_valid_token_info", return_value={"access_token": "ghu_fresh"}, + ) as valid: + data = worker._with_connection_credentials({"repo_url": "acme/private"}, cid) + valid.assert_called_once_with(cid) + assert data == {"repo_url": "acme/private", "access_token": "ghu_fresh"} + + def test_rejected_token_flags_the_connection(self, pg_conn): + """A revoked token pauses the source for reconnect instead of failing every sync.""" + from docsgpt import worker + from docsgpt.connectors.service import ConnectionUnavailable + from docsgpt.parser.remote.github_loader import GitHubTokenRejected + + loader = MagicMock() + loader.load_data.side_effect = GitHubTokenRejected("revoked") + task = MagicMock() + task.request.retries = 1 + with patch.object(worker.RemoteCreator, "create_loader", return_value=loader), patch.object( + worker, "_with_connection_credentials", return_value={"url": "acme/r", "access_token": "t"}, + ), patch.object(worker, "publish_user_event"), patch( + "docsgpt.connectors.service.mark_reconnect_needed", + ) as flag: + with pytest.raises(ConnectionUnavailable): + worker.remote_worker(task, "acme/r", "repo", "alice", "github", connection_id="c-1") + flag.assert_called_once() + assert flag.call_args.args[0] == "c-1" + def test_sync_runs_as_the_connection_without_a_browser(self, pg_conn): from docsgpt import worker diff --git a/tests/parser/remote/test_github_loader.py b/tests/parser/remote/test_github_loader.py index 61684af1..5d60d77a 100644 --- a/tests/parser/remote/test_github_loader.py +++ b/tests/parser/remote/test_github_loader.py @@ -512,3 +512,107 @@ class TestGitHubLoaderStaleTokenFallback: assert mock_get.call_count == 2 # Second attempt carried no Authorization header. assert "Authorization" not in mock_get.call_args.kwargs["headers"] + + +def _stub_repo(loader, monkeypatch): + monkeypatch.setattr(loader, "get_default_branch", lambda repo: "main") + monkeypatch.setattr(loader, "fetch_repo_tree", lambda repo, branch: ([("README.md", 5)], False)) + monkeypatch.setattr(loader, "fetch_file_content", lambda repo, path: "readme") + + +class TestGitHubLoaderConnectionToken: + def test_uses_the_connections_token(self, monkeypatch): + from docsgpt.core.settings import settings + + monkeypatch.setattr(settings, "GITHUB_ACCESS_TOKEN", "instance-token") + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + with patch("docsgpt.parser.remote.github_loader.requests.get") as get: + docs = loader.load_data({"repo_url": "owner/private-repo", "access_token": "user-token"}) + # A user's own token reads their private repos: no public check. + get.assert_not_called() + assert loader.headers["Authorization"] == "Bearer user-token" + assert [d.doc_id for d in docs] == ["README.md"] + + def test_accepts_a_url_key_and_a_plain_string(self, monkeypatch): + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + assert loader.load_data({"url": "https://github.com/owner/repo"})[0].extra_info["source"].startswith( + "https://github.com/owner/repo/" + ) + assert loader.load_data("owner/repo") + + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_rejected_connection_token_is_not_retried_anonymously(self, mock_get): + from docsgpt.parser.remote.github_loader import GitHubTokenRejected + + loader = GitHubLoader(access_token="revoked") + mock_get.return_value = MagicMock(status_code=401) + with pytest.raises(GitHubTokenRejected): + loader._make_request("https://api.github.com/repos/o/r") + assert mock_get.call_count == 1 + + +class TestGitHubLoaderInstanceTokenIsPublicOnly: + """GITHUB_ACCESS_TOKEN is shared by every user: it must never read a private repo.""" + + @pytest.fixture(autouse=True) + def _instance_token(self, monkeypatch): + from docsgpt.core.settings import settings + + monkeypatch.setattr(settings, "GITHUB_ACCESS_TOKEN", "instance-token") + + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_private_repo_is_refused(self, mock_get, monkeypatch): + from docsgpt.parser.remote.github_loader import PrivateRepositoryError + + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + fetched = MagicMock() + monkeypatch.setattr(loader, "fetch_file_content", fetched) + mock_get.return_value = make_response({"private": True, "visibility": "private"}) + + with pytest.raises(PrivateRepositoryError, match="Connect"): + loader.load_data("https://github.com/acme/secret") + fetched.assert_not_called() + + @pytest.mark.parametrize("status", [404, 403]) + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_unreadable_repo_is_refused(self, mock_get, status, monkeypatch): + from docsgpt.parser.remote.github_loader import PrivateRepositoryError + + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + mock_get.return_value = make_response({"message": "Not Found"}, status_code=status) + with pytest.raises(PrivateRepositoryError): + loader.load_data("acme/secret") + + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_internal_repo_is_refused(self, mock_get, monkeypatch): + from docsgpt.parser.remote.github_loader import PrivateRepositoryError + + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + mock_get.return_value = make_response({"private": False, "visibility": "internal"}) + with pytest.raises(PrivateRepositoryError): + loader.load_data("acme/internal") + + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_public_repo_uses_the_instance_token(self, mock_get, monkeypatch): + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + mock_get.return_value = make_response({"private": False, "visibility": "public"}) + + docs = loader.load_data("owner/repo") + + assert [d.doc_id for d in docs] == ["README.md"] + assert mock_get.call_args.kwargs["headers"]["Authorization"] == "Bearer instance-token" + + @patch("docsgpt.parser.remote.github_loader.requests.get") + def test_stale_instance_token_reads_public_repos_anonymously(self, mock_get, monkeypatch): + loader = GitHubLoader() + _stub_repo(loader, monkeypatch) + mock_get.side_effect = [MagicMock(status_code=401), make_response({"private": False})] + + assert loader.load_data("owner/repo") + assert "Authorization" not in loader.headers