Apply Bitbucket SCM review feedback

- Fix paginated comment fetch: previous code passed base_url='' for the
  absolute 'next' URL, which CliClient (falsy check) silently mapped to
  Socket's API base. Split the URL into origin + path so the request
  hits Bitbucket.
- Drop content.get('markup') body fallback (markup is the format name,
  not body text); fall back to content.get('html') instead.
- Remove dead hasattr() branch in has_thumbsup_reaction.
- Move PROCESSED_MARKER to top of class with a comment explaining why
  there's no Bearer/Basic auth-fallback retry like GitLab has.
- Document that --scm bitbucket exits 2 without BITBUCKET_TOKEN or
  BITBUCKET_USERNAME+BITBUCKET_APP_PASSWORD set.
- Expand tests: pagination (origin split, no-next, error response,
  no-PR), has_thumbsup_reaction (marker present/absent, no-PR,
  exception swallowing), _mark_comment_processed (append, idempotence,
  swallow update error), check_for_socket_comments classification, and
  _split_absolute_url round-trip.

Signed-off-by: lelia <2418071+lelia@users.noreply.github.com>

Author: lelia

socketsecurity/core/scm/bitbucket.py

@@ -3,7 +3,8 @@
 import os
 import sys
 from dataclasses import dataclass
-from typing import Optional
+from typing import Optional, Tuple
+from urllib.parse import urlparse
 
 from socketsecurity import USER_AGENT
 from socketsecurity.core import log
@@ -107,10 +108,33 @@ def _get_auth_headers(
 
 
 class Bitbucket:
+    PROCESSED_MARKER = "<!-- socket-ignore-processed -->"
+
+    # No Bearer/Basic fallback retry (cf. Gitlab._request_with_fallback) because
+    # Bitbucket's auth scheme is unambiguous: BITBUCKET_TOKEN selects Bearer,
+    # BITBUCKET_USERNAME+BITBUCKET_APP_PASSWORD selects Basic. If both routes
+    # fail, the credential itself is wrong, not the scheme.
+
     def __init__(self, client: CliClient, config: Optional[BitbucketConfig] = None):
         self.config = config or BitbucketConfig.from_env()
         self.client = client
 
+    @staticmethod
+    def _split_absolute_url(url: str) -> Tuple[str, str]:
+        """Split an absolute URL into (origin, path+query) for CliClient.request.
+
+        CliClient builds URLs as f"{base_url}/{path}", so an empty base_url
+        would fall back to Socket's API URL. To request a Bitbucket-absolute
+        URL (like the 'next' link in paginated responses), we hand the origin
+        in as base_url and the path/query as path.
+        """
+        parsed = urlparse(url)
+        origin = f"{parsed.scheme}://{parsed.netloc}"
+        path = parsed.path.lstrip("/")
+        if parsed.query:
+            path = f"{path}?{parsed.query}"
+        return origin, path
+
     def check_event_type(self) -> str:
         """Bitbucket Pipelines does not expose a 'comment' trigger.
 
@@ -165,11 +189,13 @@ def get_comments_for_pr(self) -> dict:
 
         while True:
             if next_url:
-                # Bitbucket returns absolute 'next' URLs; pass via base_url=''.
+                # Bitbucket returns absolute 'next' URLs; split origin off so
+                # CliClient doesn't prepend the Socket API base.
+                origin, abs_path = self._split_absolute_url(next_url)
                 response = self.client.request(
-                    path=next_url,
+                    path=abs_path,
                     headers=self.config.headers,
-                    base_url="",
+                    base_url=origin,
                 )
             else:
                 response = self.client.request(
@@ -207,7 +233,9 @@ def _normalize_comment(raw: dict) -> Optional[dict]:
         if raw.get("deleted"):
             return None
         content = raw.get("content") or {}
-        body = content.get("raw") or content.get("markup") or ""
+        # Bitbucket Cloud's `markup` field is the markup type ("markdown"),
+        # not body text; `html` is the rendered fallback for HTML-only edges.
+        body = content.get("raw") or content.get("html") or ""
         user = raw.get("user") or {}
         normalized_user = {
             "login": user.get("nickname") or user.get("display_name", ""),
@@ -267,8 +295,6 @@ def handle_ignore_reactions(self, comments: dict) -> None:
             if "SocketSecurity ignore" in comment.body and not self.has_thumbsup_reaction(comment.id):
                 self._mark_comment_processed(comment)
 
-    PROCESSED_MARKER = "<!-- socket-ignore-processed -->"
-
     def has_thumbsup_reaction(self, comment_id) -> bool:
         """Bitbucket has no reactions; detect our hidden processed marker."""
         if not self.config.pr_id:
@@ -279,8 +305,8 @@ def has_thumbsup_reaction(self, comment_id) -> bool:
                 headers=self.config.headers,
                 base_url=self.config.api_url,
             )
-            data = response.json() if hasattr(response, "json") else {}
-            body = ((data or {}).get("content") or {}).get("raw", "")
+            data = response.json() or {}
+            body = (data.get("content") or {}).get("raw", "")
             return self.PROCESSED_MARKER in body
         except Exception as error:
             log.debug(f"Could not fetch Bitbucket comment {comment_id} for marker check: {error}")

tests/unit/test_bitbucket.py

@@ -9,6 +9,30 @@
 from socketsecurity.core.scm.bitbucket import Bitbucket, BitbucketConfig
 
 
+def _make_config(pr_id="42", api_url="https://api.bitbucket.org/2.0"):
+    return BitbucketConfig(
+        api_url=api_url,
+        workspace="acme",
+        repo_slug="widgets",
+        repository="widgets",
+        pr_id=pr_id,
+        source_branch="feature",
+        destination_branch="main",
+        default_branch="main",
+        commit_sha="abc",
+        is_default_branch=False,
+        token="t",
+        username=None,
+        headers={"Authorization": "Bearer t", "Content-Type": "application/json"},
+    )
+
+
+def _json_response(payload):
+    resp = MagicMock()
+    resp.json.return_value = payload
+    return resp
+
+
 class TestBitbucketConfigFromEnv:
     @patch.dict(os.environ, {
         "BITBUCKET_TOKEN": "bbtoken-xyz",
@@ -193,5 +217,177 @@ def test_add_socket_comments_no_pr_is_noop(self):
         client.request.assert_not_called()
 
 
+class TestSplitAbsoluteUrl:
+    def test_splits_origin_and_path(self):
+        origin, path = Bitbucket._split_absolute_url(
+            "https://api.bitbucket.org/2.0/repositories/acme/widgets/pullrequests/42/comments?page=2"
+        )
+        assert origin == "https://api.bitbucket.org"
+        assert path == "2.0/repositories/acme/widgets/pullrequests/42/comments?page=2"
+
+    def test_no_query_string(self):
+        origin, path = Bitbucket._split_absolute_url(
+            "https://bitbucket.example.com/rest/api/1.0/foo"
+        )
+        assert origin == "https://bitbucket.example.com"
+        assert path == "rest/api/1.0/foo"
+
+    def test_reconstructed_url_matches_clicliclient_join(self):
+        """CliClient does f'{base_url}/{path}' — verify our split round-trips."""
+        original = "https://api.bitbucket.org/2.0/foo/bar?x=1&y=2"
+        origin, path = Bitbucket._split_absolute_url(original)
+        assert f"{origin}/{path}" == original
+
+
+class TestBitbucketPagination:
+    def test_follows_next_url_via_origin_split(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        # Use Socket-style bodies so check_for_socket_comments keeps them and
+        # we can observe that BOTH pages were fetched.
+        page1 = {
+            "values": [
+                {
+                    "id": 1,
+                    "content": {"raw": "socket-security-comment-actions: page1"},
+                    "user": {"nickname": "alice", "uuid": "{u-1}"},
+                }
+            ],
+            "next": (
+                "https://api.bitbucket.org/2.0/repositories/acme/widgets"
+                "/pullrequests/42/comments?page=2"
+            ),
+        }
+        page2 = {
+            "values": [
+                {
+                    "id": 2,
+                    "content": {"raw": "socket-overview-comment-actions: page2"},
+                    "user": {"nickname": "bob", "uuid": "{u-2}"},
+                }
+            ],
+        }
+        scm.client.request.side_effect = [_json_response(page1), _json_response(page2)]
+
+        result = scm.get_comments_for_pr()
+
+        # Both pages were scanned: the security comment came from page 1 and
+        # the overview comment from page 2.
+        assert "security" in result and result["security"].body.endswith("page1")
+        assert "overview" in result and result["overview"].body.endswith("page2")
+
+        # First call: relative path against Bitbucket API base.
+        first_call = scm.client.request.call_args_list[0]
+        assert first_call.kwargs["base_url"] == "https://api.bitbucket.org/2.0"
+        assert "pagelen=100" in first_call.kwargs["path"]
+
+        # Second call: origin pulled out of the absolute next URL — must NOT
+        # be empty (which would fall back to Socket's API URL in CliClient).
+        second_call = scm.client.request.call_args_list[1]
+        assert second_call.kwargs["base_url"] == "https://api.bitbucket.org"
+        assert second_call.kwargs["base_url"]  # non-empty -> avoids fallback
+        assert second_call.kwargs["path"].startswith(
+            "2.0/repositories/acme/widgets/pullrequests/42/comments"
+        )
+        assert "page=2" in second_call.kwargs["path"]
+
+    def test_stops_when_no_next(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.return_value = _json_response({"values": []})
+        scm.get_comments_for_pr()
+        assert scm.client.request.call_count == 1
+
+    def test_no_pr_skips_fetch(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config(pr_id=None))
+        result = scm.get_comments_for_pr()
+        assert result == {}
+        scm.client.request.assert_not_called()
+
+    def test_error_response_breaks_loop(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.return_value = _json_response(
+            {"type": "error", "error": {"message": "boom"}}
+        )
+        result = scm.get_comments_for_pr()
+        assert result == {}
+        assert scm.client.request.call_count == 1
+
+
+class TestHasThumbsupReaction:
+    def test_detects_processed_marker(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.return_value = _json_response(
+            {"content": {"raw": f"some body\n\n{Bitbucket.PROCESSED_MARKER}"}}
+        )
+        assert scm.has_thumbsup_reaction(123) is True
+
+    def test_returns_false_when_marker_absent(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.return_value = _json_response(
+            {"content": {"raw": "plain comment with no marker"}}
+        )
+        assert scm.has_thumbsup_reaction(123) is False
+
+    def test_returns_false_when_no_pr(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config(pr_id=None))
+        assert scm.has_thumbsup_reaction(123) is False
+        scm.client.request.assert_not_called()
+
+    def test_returns_false_on_request_exception(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.side_effect = RuntimeError("network down")
+        assert scm.has_thumbsup_reaction(123) is False
+
+
+class TestMarkCommentProcessed:
+    def test_appends_marker_and_updates(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        comment = MagicMock(id=99, body="original body")
+        scm._mark_comment_processed(comment)
+        call = scm.client.request.call_args
+        assert call.kwargs["method"] == "PUT"
+        payload = json.loads(call.kwargs["payload"])
+        assert payload["content"]["raw"].startswith("original body")
+        assert Bitbucket.PROCESSED_MARKER in payload["content"]["raw"]
+
+    def test_is_idempotent_when_marker_already_present(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        comment = MagicMock(
+            id=99, body=f"already processed\n\n{Bitbucket.PROCESSED_MARKER}"
+        )
+        scm._mark_comment_processed(comment)
+        scm.client.request.assert_not_called()
+
+    def test_swallows_update_errors(self):
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        scm.client.request.side_effect = RuntimeError("boom")
+        comment = MagicMock(id=99, body="x")
+        # Should not raise.
+        scm._mark_comment_processed(comment)
+
+
+class TestSocketCommentClassification:
+    def test_get_comments_runs_check_for_socket_comments(self):
+        """Normalized comments must flow through Comments.check_for_socket_comments
+        so the overview/security/ignore keys are populated for downstream code."""
+        scm = Bitbucket(client=MagicMock(), config=_make_config())
+        # A Socket-style "ignore" comment body.
+        page = {
+            "values": [
+                {
+                    "id": 1,
+                    "content": {"raw": "@SocketSecurity ignore npm/foo@1.0.0"},
+                    "user": {"nickname": "alice", "uuid": "{u-1}"},
+                }
+            ]
+        }
+        scm.client.request.return_value = _json_response(page)
+        result = scm.get_comments_for_pr()
+        # Comments.check_for_socket_comments populates the "ignore" bucket.
+        assert "ignore" in result
+        assert any(
+            "SocketSecurity ignore" in c.body for c in result["ignore"]
+        )
+
+
 if __name__ == "__main__":
     pytest.main([__file__])

workflows/bitbucket-pipelines.yml

@@ -24,6 +24,7 @@ definitions:
         #                          "Pull requests: Write" scope
         #                          (or set BITBUCKET_USERNAME and
         #                           BITBUCKET_APP_PASSWORD for an app password)
+        # Without either credential set, --scm bitbucket exits 2.
 
 pipelines:
   # Run on all branches