diff --git a/docs/scm.md b/docs/scm.md index 1230b2a6..0847e8ca 100644 --- a/docs/scm.md +++ b/docs/scm.md @@ -189,6 +189,19 @@ and [OAuth changes effective May 2026](https://community.developer.atlassian.com ## Protect and revoke access +GitLab comment receipts use the positive note ID, project ID, per-project issue +or merge-request IID and noteable type returned by the +[Notes API](https://docs.gitlab.com/api/notes/). forge builds the `#note_ID` link +from the grant-checked registered repository URL; response links do not select +the destination. Self-managed HTTPS origins and ports are supported. Application +hosts with a relative URL root remain unsupported and are refused. + +The accepted note reference is retained before reading that exact note back. +If this read fails or returns another identity, the receipt needs reconciliation +and keeps the known reference. Reprocessing or synchronization never posts it +again. A lost POST response still has an unknown outcome and requires explicit +provider inspection. These receipts do not imply GitLab board or field sync. + Private keys, client secrets and OAuth grants stay in `configuration/` under the server's private data directory. Files have `0600` permissions and the directory has `0700` permissions. API summaries return configured flags for secrets; diff --git a/src/firefly_forge/collaboration.py b/src/firefly_forge/collaboration.py index 781703f7..e9fb6e52 100644 --- a/src/firefly_forge/collaboration.py +++ b/src/firefly_forge/collaboration.py @@ -42,6 +42,7 @@ from firefly_forge.product_provenance import AIOrigin, validate_origin from firefly_forge.prompts import prompt_text from firefly_forge.scm_connections import SCMConnections +from firefly_forge.scm_gitlab_notes import GitLabNoteReadbackUncertain, GitLabNoteThread from firefly_forge.scm_product_change_models import SCMProductChangeCommandV1 from firefly_forge.scm_provider_backoff import DeferredFeedbackChanged, ProviderRequestDeferred, retained_deferred from firefly_forge.scm_resume_models import SCMResumeCommandV2, SCMResumeStatusV2 @@ -1650,6 +1651,7 @@ async def _remote_write( *, before_write: Callable[[], Awaitable[None]] | None = None, before_dispatch: Callable[[], Awaitable[None]] | None = None, + after_confirmation: Callable[[Data], Awaitable[None]] | None = None, ) -> Data: provider = str(repo["connection_provider"]) ownership_required = self.store.execution_ownership_acquired or ( @@ -1676,15 +1678,41 @@ async def _remote_write( ): await self.store.assert_ownership() self.store.reject_credentials(payload) + thread = GitLabNoteThread.prepare(repo, issue, api) if provider == "gitlab" and issue is not None else None dispatch = {"before_dispatch": before_dispatch} if before_dispatch is not None else {} response = await self.scm._request( "POST", - api + self._issue_path(provider, repo, issue), + thread.endpoint if thread else api + self._issue_path(provider, repo, issue), headers={"Authorization": "Bearer " + token, "Accept": "application/json"}, json=payload, **dispatch, ) result: Data = response.json() + if thread is not None: + remote, noteable_id = thread.confirm(result) + try: + if after_confirmation is not None: + await after_confirmation(remote) + if ownership_required: + await self.store.assert_ownership() + if before_write is not None: + await before_write() + readback = await self.scm._request( + "GET", + thread.endpoint + "/" + str(remote["id"]), + headers={"Authorization": "Bearer " + token, "Accept": "application/json"}, + ) + observed, observed_noteable_id = thread.confirm(readback.json()) + if observed != remote or observed_noteable_id != noteable_id: + raise ValueError("GitLab readback did not match the accepted note") + if ownership_required: + await self.store.assert_ownership() + if before_write is not None: + await before_write() + except Exception: + # POST already confirmed this identity; a deferred GET cannot make it safe to post again. + raise GitLabNoteReadbackUncertain(remote) from None + return remote return { "id": result.get("id"), "number": result.get("number", result.get("iid", result.get("id"))), @@ -2215,12 +2243,19 @@ async def maintain_dispatch_check() -> None: dispatch_check = maintain_dispatch_check + async def retain_confirmed_note(remote: Data) -> None: + async with self.store._unit(): + row = await self.inbox.find_by_id(identifier(receipt_id), lock=LockMode.PESSIMISTIC_WRITE) + assert row is not None + await self._write_row(row, feedback_remote=remote) + remote = await self._remote_write( repo, payload, data["issue"], before_write=retained_before_write if retained is not None else before_write, before_dispatch=dispatch_check, + **({"after_confirmation": retain_confirmed_note} if data["provider"] == "gitlab" else {}), ) if data.get("maintain_ticket") is not None: from firefly_forge.scm_maintain_ticket_models import confirmed_remote @@ -2256,8 +2291,13 @@ async def maintain_dispatch_check() -> None: await self._write_row( row, feedback_status="held" if held else "needs_reconciliation", - reason="deferred_feedback_authority_changed" if held else "remote_feedback_outcome_unknown", + reason="deferred_feedback_authority_changed" + if held + else "gitlab_note_readback_unconfirmed" + if isinstance(error, GitLabNoteReadbackUncertain) + else "remote_feedback_outcome_unknown", feedback_deferred=row.data.get("feedback_deferred") if held else None, + **({"feedback_remote": error.remote} if isinstance(error, GitLabNoteReadbackUncertain) else {}), ) else: async with self.store._unit(): diff --git a/src/firefly_forge/scm_gitlab_notes.py b/src/firefly_forge/scm_gitlab_notes.py new file mode 100644 index 00000000..820f425f --- /dev/null +++ b/src/firefly_forge/scm_gitlab_notes.py @@ -0,0 +1,73 @@ +# Copyright 2026 Firefly Software Foundation. SPDX-License-Identifier: Apache-2.0 +"""Bind GitLab Notes responses to a grant-checked repository and exact thread.""" + +import re +from dataclasses import dataclass +from typing import Any +from urllib.parse import urlsplit + +from firefly_forge.scm_authority_models import canonical_provider_id, exact_origin + +Data = dict[str, Any] + + +def _positive_id(value: Any) -> str: + if type(value) not in (int, str): + raise ValueError("GitLab note references need canonical positive IDs") + return canonical_provider_id("gitlab", str(value)) + + +class GitLabNoteReadbackUncertain(ValueError): + def __init__(self, remote: Data) -> None: + super().__init__("GitLab accepted the note, but its exact readback could not be confirmed") + self.remote = remote + + +@dataclass(frozen=True) +class GitLabNoteThread: + project_id: str + iid: str + noteable_type: str + endpoint: str + web_url: str + + @classmethod + def prepare(cls, repo: Data, issue: Data, api: str) -> "GitLabNoteThread": + parsed = urlsplit(api) + origin = exact_origin(parsed.scheme + "://" + parsed.netloc) + # SCM application configuration currently accepts an origin, not a relative URL root. + if parsed.path != "/api/v4" or parsed.query or parsed.fragment: + raise ValueError("GitLab notes require the configured API base") + name = repo["name"] + if not isinstance(name, str) or any( + part in {".", ".."} or not re.fullmatch(r"[A-Za-z0-9_.-]+", part) for part in name.split("/") + ): + raise ValueError("Invalid connected GitLab repository path") + registered = repo["url"] + if registered not in {origin + "/" + name, origin + "/" + name + ".git"}: + raise ValueError("The registered GitLab repository does not match its configured origin and path") + kind = issue["kind"] + if kind not in {"issue", "merge_request"}: + raise ValueError("GitLab notes require an issue or merge request thread") + project_id, iid = _positive_id(repo["remote_repository_id"]), _positive_id(issue["number"]) + path = "issues" if kind == "issue" else "merge_requests" + return cls( + project_id, + iid, + "Issue" if kind == "issue" else "MergeRequest", + api + f"/projects/{project_id}/{path}/{iid}/notes", + origin + "/" + name + f"/-/{path}/{iid}", + ) + + def confirm(self, result: Any) -> tuple[Data, str]: + if not isinstance(result, dict): + raise ValueError("GitLab returned an invalid note response") + note_id = _positive_id(result.get("id")) + noteable_id = _positive_id(result.get("noteable_id")) + if ( + _positive_id(result.get("project_id")) != self.project_id + or _positive_id(result.get("noteable_iid")) != self.iid + or result.get("noteable_type") != self.noteable_type + ): + raise ValueError("GitLab returned a note for a different project or thread") + return {"id": int(note_id), "number": int(note_id), "url": self.web_url + "#note_" + note_id}, noteable_id diff --git a/tests/test_gitlab_note_provenance.py b/tests/test_gitlab_note_provenance.py new file mode 100644 index 00000000..d3bc62e9 --- /dev/null +++ b/tests/test_gitlab_note_provenance.py @@ -0,0 +1,209 @@ +# Copyright 2026 Firefly Software Foundation. SPDX-License-Identifier: Apache-2.0 +"""GitLab Notes identities come from the admitted thread, never response links.""" + +import copy +from types import SimpleNamespace +from uuid import UUID + +import httpx +import pytest +from test_scm_delegated_feedback import RECEIPT, received +from test_scm_delegated_feedback import delegated_feedback as delegated_feedback + +from firefly_forge.collaboration import Collaboration + +REPOSITORY = dict( + connection_provider="gitlab", + remote_repository_id="42", + name="team/nested/product", + url="https://gitlab.selfmanaged.example:8443/team/nested/product.git", +) +API = "https://gitlab.selfmanaged.example:8443/api/v4" + + +def note(kind="issue", **changes): + return dict( + id=91, + body="Confirmed action", + project_id=42, + noteable_type="Issue" if kind == "issue" else "MergeRequest", + noteable_id=999, + noteable_iid=3, + author={"id": 123, "username": "service-bot"}, + **changes, + ) + + +async def publish(transport, *, repo=None, issue=None, api=API, **callbacks): + async def authentication(*args, **kwargs): + return "fixture-only-provider-token", api + + async with httpx.AsyncClient(transport=httpx.MockTransport(transport)) as client: + service = Collaboration.__new__(Collaboration) + service.store = SimpleNamespace(execution_ownership_acquired=False, reject_credentials=lambda _: None) + service.deployments = None + service.scm = SimpleNamespace(issue_authentication=authentication, _request=client.request) + return await service._remote_write( + repo or copy.deepcopy(REPOSITORY), + {"body": "Confirmed action"}, + issue or dict(kind="issue", number=3), + **callbacks, + ) + + +@pytest.mark.parametrize("kind,path", [("issue", "issues"), ("merge_request", "merge_requests")]) +async def test_real_notes_response_uses_registered_self_hosted_thread(kind, path): + calls = [] + + def transport(request): + calls.append((request.method, str(request.url))) + result = note(kind) + result["web_url"] = "https://untrusted.example/foreign-note" + return httpx.Response(201 if request.method == "POST" else 200, json=result) + + remote = await publish(transport, issue=dict(kind=kind, number=3)) + assert calls == [("POST", API + f"/projects/42/{path}/3/notes"), ("GET", API + f"/projects/42/{path}/3/notes/91")] + assert remote == dict( + id=91, + number=91, + url=f"https://gitlab.selfmanaged.example:8443/team/nested/product/-/{path}/3#note_91", + ) + + +@pytest.mark.parametrize( + "field,value", + [ + ("id", True), + ("id", 0), + ("id", "091"), + ("id", 1.5), + ("project_id", 43), + ("project_id", True), + ("noteable_iid", 4), + ("noteable_iid", "03"), + ("noteable_type", "MergeRequest"), + ("noteable_id", False), + ], +) +async def test_wrong_or_noncanonical_post_identity_cannot_confirm_note(field, value): + calls = [] + + def transport(request): + calls.append(request.method) + result = note() + result[field] = value + return httpx.Response(201, json=result) + + with pytest.raises(ValueError): + await publish(transport) + assert calls == ["POST"] + + +@pytest.mark.parametrize( + "field,value", + [ + ("url", "https://foreign.example/team/nested/product.git"), + ("url", "https://gitlab.selfmanaged.example:8443/other/product.git"), + ("url", "https://gitlab.selfmanaged.example:8443/team/nested/product.git?x=1"), + ("remote_repository_id", "042"), + ("remote_repository_id", True), + ], +) +async def test_registered_repository_must_match_api_and_canonical_project(field, value): + calls = [] + repo = {**REPOSITORY, field: value} + with pytest.raises(ValueError): + await publish(lambda request: calls.append(request), repo=repo) + assert calls == [] + + +@pytest.mark.parametrize("failure", ["timeout", "wrong_id", "wrong_thread", "wrong_type", "wrong_noteable", "deferred"]) +async def test_uncertain_readback_retains_known_post_identity_without_reposting(failure): + from firefly_forge.scm_provider_backoff import ProviderRequestDeferred + + calls = [] + + def transport(request): + calls.append(request.method) + result = note() + if request.method == "GET": + if failure == "timeout": + raise httpx.ReadTimeout("Readback unavailable", request=request) + if failure == "deferred": + raise ProviderRequestDeferred("Readback deferred", retry_at=10) + field, value = { + "wrong_id": ("id", 92), + "wrong_thread": ("noteable_iid", 4), + "wrong_type": ("noteable_type", "MergeRequest"), + "wrong_noteable": ("noteable_id", 1000), + }[failure] + result[field] = value + return httpx.Response(201 if request.method == "POST" else 200, json=result) + + with pytest.raises(ValueError) as raised: + await publish(transport) + assert raised.value.remote == dict( + id=91, + number=91, + url="https://gitlab.selfmanaged.example:8443/team/nested/product/-/issues/3#note_91", + ) + assert calls == ["POST", "GET"] + + +@pytest.mark.parametrize( + "api", + [ + "https://gitlab.selfmanaged.example:8443/gitlab/api/v4", + "https://gitlab.selfmanaged.example:8443/api/v4?foreign=1", + "https://user:secret@gitlab.selfmanaged.example:8443/api/v4", + ], +) +async def test_unconfigured_relative_roots_and_credential_urls_are_refused_before_post(api): + calls = [] + with pytest.raises(ValueError): + await publish(lambda request: calls.append(request), api=api) + assert calls == [] + + +@pytest.mark.parametrize("failure", ["timeout", "deferred", "cancelled"]) +async def test_receipt_retains_confirmed_note_before_uncertain_readback(delegated_feedback, monkeypatch, failure): + import asyncio + + from firefly_forge.scm_provider_backoff import ProviderRequestDeferred + + service, row, _, _, _ = delegated_feedback + calls = [] + + def transport(request): + calls.append(request.method) + if request.method == "GET": + assert row.data["feedback_status"] == "writing" + assert row.data["feedback_remote"]["id"] == 91 + assert row.data["feedback_remote"]["url"].endswith("/issues/3#note_91") + if failure == "cancelled": + raise asyncio.CancelledError() + if failure == "deferred": + raise ProviderRequestDeferred("Readback deferred", retry_at=10) + raise httpx.ReadTimeout("Readback unavailable", request=request) + return httpx.Response(201, json=note()) + + async def remote(repo, payload, issue, **callbacks): + return await publish(transport, **callbacks) + + monkeypatch.setattr(service, "_remote_write", remote) + received(row, "status") + row.provider = row.data["provider"] = "gitlab" + repository = await service.store.repository_records.find_by_id(UUID(row.data["repository_id"])) + repository.data.update(REPOSITORY) + if failure == "cancelled": + with pytest.raises(asyncio.CancelledError): + await service.process(RECEIPT) + await service.sync_feedback() + else: + await service.process(RECEIPT) + assert row.data["feedback_status"] == "needs_reconciliation" + assert row.data["feedback_remote"]["id"] == 91 + await service.process(RECEIPT) + await service.sync_feedback() + assert calls == ["POST", "GET"] + assert row.data.get("feedback_deferred") is None diff --git a/tests/test_scm_http_lifecycle.py b/tests/test_scm_http_lifecycle.py index 89e8acc6..75c48749 100644 --- a/tests/test_scm_http_lifecycle.py +++ b/tests/test_scm_http_lifecycle.py @@ -109,7 +109,15 @@ def test_authenticated_provider_http_signed_ingress_feedback_and_reconciliation( "gitlab": "gitlab.selfmanaged.example", "bitbucket": "bitbucket.org", }[provider] - state = {"writes": [], "errors": [], "revoked": False, "drop_response": False, "refreshes": 0} + state = { + "writes": [], + "errors": [], + "revoked": False, + "drop_response": False, + "refreshes": 0, + "notes": {}, + "drop_readback": False, + } expiry = (datetime.now(UTC) + timedelta(hours=1)).isoformat() class Handler(BaseHTTPRequestHandler): @@ -176,19 +184,26 @@ def handle_provider(self): ] result = {"repositories": [] if state["revoked"] else entries} elif provider == "gitlab": - assert path == "/api/v4/projects" and self.headers["Host"] == host - result = ( - [] - if state["revoked"] - else [ - { - "id": 42, - "path_with_namespace": "team/product", - "http_url_to_repo": f"https://{host}/team/product.git", - "default_branch": "main", - } - ] - ) + assert self.headers["Host"] == host + if path.startswith("/api/v4/projects/42/merge_requests/12/notes/"): + if state["drop_readback"]: + self.close_connection = True + return + result = state["notes"][int(path.rsplit("/", 1)[1])] + else: + assert path == "/api/v4/projects" + result = ( + [] + if state["revoked"] + else [ + { + "id": 42, + "path_with_namespace": "team/product", + "http_url_to_repo": f"https://{host}/team/product.git", + "default_branch": "main", + } + ] + ) else: assert path == "/2.0/repositories/team" and self.headers["Host"] == "api.bitbucket.org" result = { @@ -226,6 +241,17 @@ def handle_provider(self): "id": len(state["writes"]), "html_url": f"https://{host}/team/product/comments/{len(state['writes'])}", } + if provider == "gitlab": + result = { + "id": len(state["writes"]), + "body": message, + "project_id": 42, + "noteable_id": 999, + "noteable_iid": 12, + "noteable_type": "MergeRequest", + "author": {"id": 123, "username": "service-bot"}, + } + state["notes"][result["id"]] = result encoded = json.dumps(result).encode() self.send_response(201 if self.command == "POST" else 200) self.send_header("Content-Type", "application/json") @@ -306,9 +332,29 @@ def handle_provider(self): result = client.post(process, headers=auth) assert result.status_code == 200, result.text assert result.json()["feedback_status"] == "sent" + if provider == "gitlab": + assert result.json()["feedback_remote"] == { + "id": 1, + "number": 1, + "url": f"https://{host}/team/product/-/merge_requests/12#note_1", + } assert client.post(ingress, content=raw, headers=signed).json()["id"] == receipt["id"] assert client.post(process, headers=auth).json()["feedback_status"] == "sent" assert len(state["writes"]) == 1 + accepted = 1 + if provider == "gitlab": + state["drop_readback"] = True + raw, signed = comment(provider, 94) + receipt = client.post(ingress, content=raw, headers=signed).json() + note_process = f"/api/v1/collaboration/receipts/{receipt['id']}/process" + uncertain = client.post(note_process, headers=auth).json() + assert uncertain["feedback_status"] == "needs_reconciliation" + assert uncertain["feedback_remote"]["id"] == 2 + assert uncertain["feedback_remote"]["url"].endswith("/merge_requests/12#note_2") + assert client.post(note_process, headers=auth).json() == uncertain + assert len(state["writes"]) == 2 + state["drop_readback"] = False + accepted = 2 state["drop_response"] = True raw, signed = comment(provider, 92) receipt = client.post(ingress, content=raw, headers=signed).json() @@ -325,13 +371,13 @@ def handle_provider(self): == 200 ) assert client.post(process, headers=auth).json()["feedback_status"] == "sent" - assert len(state["writes"]) == 2 + assert len(state["writes"]) == accepted + 1 state["revoked"] = True raw, signed = comment(provider, 93) receipt = client.post(ingress, content=raw, headers=signed).json() result = client.post(f"/api/v1/collaboration/receipts/{receipt['id']}/process", headers=auth).json() assert result["feedback_status"] == "needs_reconciliation" - assert len(state["writes"]) == 2 + assert len(state["writes"]) == accepted + 1 assert client.get("/api/v1/runs", headers=auth).json() == [] assert not state["errors"], state["errors"] if provider == "gitlab":