diff --git a/backend/app/api/routes/updates.py b/backend/app/api/routes/updates.py index 2e0a8f031..938189c2f 100644 --- a/backend/app/api/routes/updates.py +++ b/backend/app/api/routes/updates.py @@ -78,6 +78,78 @@ def _find_executable(name: str) -> str | None: return None +def _parse_github_remote(url: str) -> tuple[str, str] | None: + """Extract `(owner, repo)` from a GitHub remote URL, or None if it isn't a + GitHub URL we recognise. + + Handles the four forms `git remote -v` typically prints: + - `git@github.com:owner/repo.git` (SSH, the dev default) + - `git@github.com:owner/repo` (SSH without .git suffix) + - `https://github.com/owner/repo.git` (HTTPS, what _perform_update sets) + - `https://github.com/owner/repo` (HTTPS without .git) + + Anything else (a fork URL, a different host, a malformed value, the empty + string from a missing origin) returns None so the caller treats it as + "not pointing at our repo" and resets it. + """ + s = url.strip() + if not s: + return None + # SSH form: git@github.com:owner/repo[.git] + ssh_prefix = "git@github.com:" + https_prefix_a = "https://github.com/" + https_prefix_b = "http://github.com/" # tolerated for legacy + if s.startswith(ssh_prefix): + path = s[len(ssh_prefix) :] + elif s.startswith(https_prefix_a): + path = s[len(https_prefix_a) :] + elif s.startswith(https_prefix_b): + path = s[len(https_prefix_b) :] + else: + return None + if path.endswith(".git"): + path = path[:-4] + parts = path.strip("/").split("/") + if len(parts) != 2 or not parts[0] or not parts[1]: + return None + return (parts[0], parts[1]) + + +async def _origin_points_at_repo(git_path: str, git_config: list[str], base_dir, expected_repo: str) -> bool: + """Return True iff the working tree's `origin` already resolves to + `/` matching `expected_repo` (e.g. "maziggy/bambuddy"), + regardless of whether it's the SSH or HTTPS form. Used to skip the + `git remote set-url origin https://...` rewrite when the developer's + SSH origin is already correct — see `_perform_update` for context.""" + try: + process = await asyncio.create_subprocess_exec( + git_path, + *git_config, + "remote", + "get-url", + "origin", + cwd=str(base_dir), + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, _ = await process.communicate() + except (OSError, asyncio.CancelledError): + # Fail closed: let the caller go through the rewrite branch if we + # can't even invoke git. The unconditional set-url is the safer + # fallback, only mildly destructive. + return False + if process.returncode != 0: + # Most likely cause: no `origin` defined yet (fresh clone-style + # checkout). Caller will set it. + return False + parsed = _parse_github_remote(stdout.decode().strip()) + if parsed is None: + return False + owner, repo = parsed + expected_owner, expected_repo_name = expected_repo.split("/", 1) + return owner == expected_owner and repo == expected_repo_name + + def parse_version(version: str) -> tuple: """Parse version string into tuple for comparison. @@ -341,20 +413,32 @@ async def _perform_update(): "error": None, } - # Ensure remote uses HTTPS (SSH may not be available) + # Ensure remote points at the expected repo. We previously rewrote + # origin to HTTPS unconditionally on the assumption that systemd + # service users wouldn't have SSH keys configured — which is fine + # for that case, but stomps on developer checkouts where origin is + # legitimately `git@github.com:maziggy/bambuddy.git` and the user + # auths via SSH keys. After the rewrite, `git push` prompts for + # HTTPS credentials and fails. + # New behaviour: read the current origin, parse out the + # `/` pair, and only rewrite if it doesn't already + # resolve to the right GitHub repo. SSH origins pointing at the + # correct repo are preserved; only missing / wrong / corrupted + # origins get reset to HTTPS. https_url = f"https://github.com/{GITHUB_REPO}.git" - process = await asyncio.create_subprocess_exec( - git_path, - *git_config, - "remote", - "set-url", - "origin", - https_url, - cwd=str(base_dir), - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - ) - await process.communicate() + if not await _origin_points_at_repo(git_path, git_config, base_dir, GITHUB_REPO): + process = await asyncio.create_subprocess_exec( + git_path, + *git_config, + "remote", + "set-url", + "origin", + https_url, + cwd=str(base_dir), + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + await process.communicate() _update_status = { "status": "downloading", diff --git a/backend/tests/integration/test_updates_api.py b/backend/tests/integration/test_updates_api.py index f09df69a0..1e78c17b8 100644 --- a/backend/tests/integration/test_updates_api.py +++ b/backend/tests/integration/test_updates_api.py @@ -47,6 +47,135 @@ class TestUpdatesAPI: assert is_newer_version("0.1.5", "0.1.5b7") is True + def test_parse_github_remote_recognises_ssh_https_and_dotgit(self): + """`_parse_github_remote` must accept the four canonical forms `git + remote -v` prints; anything else returns None so callers can treat + it as 'reset to expected URL'.""" + from backend.app.api.routes.updates import _parse_github_remote + + assert _parse_github_remote("git@github.com:maziggy/bambuddy.git") == ( + "maziggy", + "bambuddy", + ) + assert _parse_github_remote("git@github.com:maziggy/bambuddy") == ( + "maziggy", + "bambuddy", + ) + assert _parse_github_remote("https://github.com/maziggy/bambuddy.git") == ( + "maziggy", + "bambuddy", + ) + assert _parse_github_remote("https://github.com/maziggy/bambuddy") == ( + "maziggy", + "bambuddy", + ) + # Non-GitHub host → None (we don't claim ownership over arbitrary + # forge URLs). + assert _parse_github_remote("git@gitlab.com:maziggy/bambuddy.git") is None + # Empty / malformed → None. + assert _parse_github_remote("") is None + assert _parse_github_remote("not-a-url") is None + assert _parse_github_remote("https://github.com/maziggy") is None # no /repo + + @pytest.mark.asyncio + async def test_perform_update_preserves_ssh_origin_when_pointing_at_correct_repo(self, tmp_path): + """Regression for the developer-checkout footgun: if origin already + points at github.com/maziggy/bambuddy via SSH, the updater must + leave it alone instead of clobbering it with HTTPS. Pre-fix, every + Apply Update click rewrote `git@github.com:...` to `https://...`, + breaking subsequent `git push` for any developer testing the + upgrade flow against their own checkout.""" + from backend.app.api.routes import updates as updates_module + + app_dir = tmp_path / "app" + data_dir = tmp_path / "app" / "data" + app_dir.mkdir() + data_dir.mkdir() + (app_dir / "requirements.txt").write_text("fastapi\n") + + calls: list[dict] = [] + + async def fake_create_subprocess_exec(*args, **kwargs): + calls.append({"args": args, "cwd": kwargs.get("cwd")}) + proc = MagicMock() + # When the updater asks `git remote get-url origin`, return the + # SSH URL. Every other subprocess returns successfully with no + # output. + if "get-url" in args and "origin" in args: + proc.communicate = AsyncMock(return_value=(b"git@github.com:maziggy/bambuddy.git\n", b"")) + else: + proc.communicate = AsyncMock(return_value=(b"", b"")) + proc.returncode = 0 + return proc + + with ( + patch.object(updates_module.settings, "base_dir", data_dir), + patch.object(updates_module.settings, "app_dir", app_dir), + patch.object(updates_module, "_find_executable", return_value="/usr/bin/git"), + patch.object( + updates_module.asyncio, + "create_subprocess_exec", + side_effect=fake_create_subprocess_exec, + ), + ): + await updates_module._perform_update() + + # The updater MUST NOT have run `git remote set-url origin ` + # because origin already pointed at the right repo over SSH. + set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]] + assert not set_url_calls, ( + "Updater clobbered an SSH origin pointing at the correct repo. " + "Captured set-url calls: " + repr([c["args"] for c in set_url_calls]) + ) + + @pytest.mark.asyncio + async def test_perform_update_resets_origin_when_pointing_elsewhere(self, tmp_path): + """Defensive: if origin points at a fork or unrelated repo (or is + missing), the updater should still rewrite it to the canonical + HTTPS URL so subsequent fetch / reset works against the right + repo. This is the original behaviour that the SSH-preservation + fix above must NOT regress.""" + from backend.app.api.routes import updates as updates_module + from backend.app.core.config import GITHUB_REPO + + app_dir = tmp_path / "app" + data_dir = tmp_path / "app" / "data" + app_dir.mkdir() + data_dir.mkdir() + (app_dir / "requirements.txt").write_text("fastapi\n") + + calls: list[dict] = [] + + async def fake_create_subprocess_exec(*args, **kwargs): + calls.append({"args": args, "cwd": kwargs.get("cwd")}) + proc = MagicMock() + # origin is set to a fork — must be rewritten. + if "get-url" in args and "origin" in args: + proc.communicate = AsyncMock(return_value=(b"git@github.com:somefork/bambuddy.git\n", b"")) + else: + proc.communicate = AsyncMock(return_value=(b"", b"")) + proc.returncode = 0 + return proc + + with ( + patch.object(updates_module.settings, "base_dir", data_dir), + patch.object(updates_module.settings, "app_dir", app_dir), + patch.object(updates_module, "_find_executable", return_value="/usr/bin/git"), + patch.object( + updates_module.asyncio, + "create_subprocess_exec", + side_effect=fake_create_subprocess_exec, + ), + ): + await updates_module._perform_update() + + set_url_calls = [c for c in calls if "set-url" in c["args"] and "origin" in c["args"]] + assert set_url_calls, "Updater must rewrite origin when it points at a fork." + rewritten_to = set_url_calls[0]["args"][-1] + assert rewritten_to == f"https://github.com/{GITHUB_REPO}.git", ( + f"Expected origin to be reset to canonical HTTPS URL; got: {rewritten_to}" + ) + @pytest.mark.asyncio async def test_perform_update_runs_pip_in_app_dir_not_data_dir(self, tmp_path): """Native install: `requirements.txt` lives at INSTALL_PATH (the source-