fix(sourcehut): assert HTTPS and reject redirects on GraphQL requests

tools/bitbucket/tests/test_bitbucket.py

@@ -27,7 +27,14 @@
 
 from magpie_bitbucket import cloud, datacenter
 from magpie_bitbucket.cli import main
-from magpie_bitbucket.client import BitbucketError, SameHostRedirectHandler, load_config, make_auth_header
+from magpie_bitbucket.client import (
+    BitbucketError,
+    NoAuthRedirectHandler,
+    SameHostRedirectHandler,
+    _require_https,
+    load_config,
+    make_auth_header,
+)
 from magpie_bitbucket.normalize import (
     created_issue_comment,
     issue,
@@ -106,6 +113,26 @@ def urllib_request(url: str) -> urllib.request.Request:
     )
 
 
+def test_no_auth_redirect_handler_rejects_all_redirects() -> None:
+    handler = NoAuthRedirectHandler()
+    request = urllib_request("https://bitbucket.example.test/rest/api/1.0/foo")
+
+    with pytest.raises(BitbucketError, match="refusing to forward credentials"):
+        handler.redirect_request(
+            request,
+            None,
+            302,
+            "Found",
+            {},
+            "https://bitbucket.example.test/rest/api/1.0/bar",
+        )
+
+
+def test_require_https_rejects_http() -> None:
+    with pytest.raises(BitbucketError, match="Bitbucket API URLs must use HTTPS"):
+        _require_https("http://bitbucket.example.test/rest/api/1.0/foo")
+
+
 def test_same_host_redirect_handler_allows_same_origin() -> None:
     handler = SameHostRedirectHandler()
     request = urllib_request("https://bitbucket.example.test/rest/api/1.0/foo")

tools/sourcehut/src/magpie_sourcehut/client.py

@@ -22,6 +22,7 @@
 import json
 import os
 import urllib.error
+import urllib.parse
 import urllib.request
 from typing import Any
 
@@ -30,6 +31,28 @@ class SourceHutError(Exception):
     """General exception for SourceHut client errors."""
 
 
+class NoAuthRedirectHandler(urllib.request.HTTPRedirectHandler):
+    """Reject redirects so Authorization is not forwarded to another host."""
+
+    def redirect_request(
+        self,
+        req: urllib.request.Request,
+        fp: Any,
+        code: int,
+        msg: str,
+        headers: Any,
+        newurl: str,
+    ) -> urllib.request.Request | None:
+        raise SourceHutError(f"SourceHut request redirected to {newurl}; refusing to forward credentials")
+
+
+def _require_https(url: str) -> None:
+    """Require HTTPS for SourceHut API URLs."""
+    parsed = urllib.parse.urlparse(url)
+    if parsed.scheme != "https":
+        raise SourceHutError("SourceHut API URLs must use HTTPS")
+
+
 def query_graphql(service: str, query: str, variables: dict[str, Any] | None = None) -> dict[str, Any]:
     """Execute a GraphQL query/mutation against a specific SourceHut service.
 
@@ -46,6 +69,7 @@ def query_graphql(service: str, query: str, variables: dict[str, Any] | None = N
         raise SourceHutError("SRHT_TOKEN environment variable is not set")
 
     url = f"https://{service}.sr.ht/query"
+    _require_https(url)
     payload: dict[str, Any] = {"query": query}
     if variables:
         payload["variables"] = variables
@@ -61,15 +85,21 @@ def query_graphql(service: str, query: str, variables: dict[str, Any] | None = N
         method="POST",
     )
 
+    # Writes never follow redirects: repeating a mutation at a redirected
+    # location is less safe than failing and requiring the caller to retry.
+    opener = urllib.request.build_opener(NoAuthRedirectHandler)
+
     try:
-        with urllib.request.urlopen(req) as resp:
+        with opener.open(req) as resp:
             body = resp.read().decode("utf-8")
             res_json = json.loads(body)
             errors = res_json.get("errors")
             if errors:
                 err_msgs = [e.get("message", "Unknown error") for e in errors]
                 raise SourceHutError(f"GraphQL error from {service}.sr.ht: {'; '.join(err_msgs)}")
             return res_json.get("data", {})
+    except SourceHutError:
+        raise
     except urllib.error.HTTPError as exc:
         err_msg = None
         try:

tools/sourcehut/tests/test_sourcehut.py

@@ -18,6 +18,7 @@
 import io
 import json
 import urllib.error
+import urllib.request
 from email.message import Message
 from typing import Any
 from unittest.mock import MagicMock, patch
@@ -26,7 +27,12 @@
 
 from magpie_sourcehut.builds import get_job
 from magpie_sourcehut.cli import main
-from magpie_sourcehut.client import SourceHutError, query_graphql
+from magpie_sourcehut.client import (
+    NoAuthRedirectHandler,
+    SourceHutError,
+    _require_https,
+    query_graphql,
+)
 from magpie_sourcehut.lists import get_patchset, list_patchsets, map_patchset_to_pr
 from magpie_sourcehut.repo import get_repo
 from magpie_sourcehut.todo import (
@@ -50,9 +56,53 @@ def make_mock_response(status_code: int, body_dict: dict[str, Any]) -> MagicMock
     return mock_resp
 
 
-@patch("urllib.request.urlopen")
-def test_query_graphql_success(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(200, {"data": {"version": "1.0"}})
+def mock_opener(mock_build_opener: MagicMock, *bodies: dict[str, Any]) -> MagicMock:
+    opener = MagicMock()
+    opener.open.side_effect = [
+        MagicMock(
+            __enter__=MagicMock(return_value=make_mock_response(200, body)),
+            __exit__=MagicMock(return_value=None),
+        )
+        for body in bodies
+    ]
+    mock_build_opener.return_value = opener
+    return opener
+
+
+def test_no_auth_redirect_handler_rejects_redirect() -> None:
+    handler = NoAuthRedirectHandler()
+    request = urllib.request.Request(
+        "https://todo.sr.ht/query",
+        headers={"Authorization": "Bearer mock_token_123"},
+        method="POST",
+    )
+
+    with pytest.raises(SourceHutError, match="refusing to forward credentials"):
+        handler.redirect_request(
+            request,
+            None,
+            302,
+            "Found",
+            {},
+            "https://todo.sr.ht/redirected",
+        )
+
+
+def test_require_https_rejects_http() -> None:
+    with pytest.raises(SourceHutError, match="SourceHut API URLs must use HTTPS"):
+        _require_https("http://todo.sr.ht/query")
+
+
+@patch("urllib.request.build_opener")
+def test_query_graphql_uses_no_auth_redirect_handler(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"version": "1.0"}})
+    query_graphql("todo", "{ version }")
+    mock_build_opener.assert_called_once_with(NoAuthRedirectHandler)
+
+
+@patch("urllib.request.build_opener")
+def test_query_graphql_success(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"version": "1.0"}})
     res = query_graphql("todo", "{ version }")
     assert res == {"version": "1.0"}
 
@@ -62,11 +112,9 @@ def test_query_graphql_no_token() -> None:
         query_graphql("todo", "{ version }")
 
 
-@patch("urllib.request.urlopen")
-def test_query_graphql_error_in_json(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"errors": [{"message": "Invalid query syntax"}]}
-    )
+@patch("urllib.request.build_opener")
+def test_query_graphql_error_in_json(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"errors": [{"message": "Invalid query syntax"}]})
     with pytest.raises(SourceHutError, match=r"GraphQL error from todo\.sr\.ht: Invalid query syntax"):
         query_graphql("todo", "invalid_query")
 
@@ -83,41 +131,45 @@ def test_query_graphql_error_in_json(mock_urlopen: MagicMock, mock_env: None) ->
         pytest.param(b'{"errors": ["boom"]}', id="errors-list-of-strings"),
     ],
 )
-@patch("urllib.request.urlopen")
+@patch("urllib.request.build_opener")
 def test_query_graphql_http_error_body_never_leaks(
-    mock_urlopen: MagicMock, mock_env: None, body: bytes
+    mock_build_opener: MagicMock, mock_env: None, body: bytes
 ) -> None:
     # Parsing the error body is best-effort: whatever shape it arrives in, the
     # caller must see a SourceHutError, never a raw UnicodeDecodeError /
     # AttributeError from the parse attempt.
-    mock_urlopen.side_effect = urllib.error.HTTPError(
+    opener = MagicMock()
+    opener.open.side_effect = urllib.error.HTTPError(
         "https://todo.sr.ht/query", 400, "Bad Request", Message(), io.BytesIO(body)
     )
+    mock_build_opener.return_value = opener
 
     with pytest.raises(
         SourceHutError, match=r"HTTP request to https://todo\.sr\.ht/query failed with status 400"
     ):
         query_graphql("todo", "{ version }")
 
 
-@patch("urllib.request.urlopen")
+@patch("urllib.request.build_opener")
 def test_query_graphql_http_error_uses_body_message_when_parseable(
-    mock_urlopen: MagicMock, mock_env: None
+    mock_build_opener: MagicMock, mock_env: None
 ) -> None:
-    mock_urlopen.side_effect = urllib.error.HTTPError(
+    opener = MagicMock()
+    opener.open.side_effect = urllib.error.HTTPError(
         "https://todo.sr.ht/query",
         400,
         "Bad Request",
         Message(),
         io.BytesIO(b'{"errors": [{"message": "bad query"}]}'),
     )
+    mock_build_opener.return_value = opener
 
     with pytest.raises(SourceHutError, match=r"HTTP 400: bad query"):
         query_graphql("todo", "{ version }")
 
 
-@patch("urllib.request.urlopen")
-def test_get_ticket(mock_urlopen: MagicMock, mock_env: None) -> None:
+@patch("urllib.request.build_opener")
+def test_get_ticket(mock_build_opener: MagicMock, mock_env: None) -> None:
     ticket_data = {
         "id": 42,
         "title": "Fix memory leak",
@@ -127,52 +179,45 @@ def test_get_ticket(mock_urlopen: MagicMock, mock_env: None) -> None:
         "labels": [{"id": 1, "name": "bug"}],
         "comments": [],
     }
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"tracker": {"ticket": ticket_data}}}
-    )
+    mock_opener(mock_build_opener, {"data": {"tracker": {"ticket": ticket_data}}})
     res = get_ticket("~user", "my-project", 42)
     assert res["id"] == 42
     assert res["title"] == "Fix memory leak"
 
 
-@patch("urllib.request.urlopen")
-def test_submit_ticket(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"submitTicket": {"id": 101, "title": "New issue"}}}
-    )
+@patch("urllib.request.build_opener")
+def test_submit_ticket(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"submitTicket": {"id": 101, "title": "New issue"}}})
     res = submit_ticket("~user", "my-project", "New issue", "Description here")
     assert res["id"] == 101
 
 
-@patch("urllib.request.urlopen")
-def test_submit_comment(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"submitComment": {"id": 501, "body": "Comment body"}}}
-    )
+@patch("urllib.request.build_opener")
+def test_submit_comment(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"submitComment": {"id": 501, "body": "Comment body"}}})
     res = submit_comment("~user", "my-project", 42, "Comment body")
     assert res["id"] == 501
 
 
-@patch("urllib.request.urlopen")
-def test_label_ticket(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"labelTicket": {"id": 42}}}
-    )
+@patch("urllib.request.build_opener")
+def test_label_ticket(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"labelTicket": {"id": 42}}})
     res = label_ticket("~user", "my-project", 42, 10)
     assert res == {"id": 42}
 
 
-@patch("urllib.request.urlopen")
-def test_update_ticket_status(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"updateTicketStatus": {"id": 42, "status": "RESOLVED", "resolution": "FIXED"}}}
+@patch("urllib.request.build_opener")
+def test_update_ticket_status(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(
+        mock_build_opener,
+        {"data": {"updateTicketStatus": {"id": 42, "status": "RESOLVED", "resolution": "FIXED"}}},
     )
     res = update_ticket_status("~user", "my-project", 42, "RESOLVED", "FIXED")
     assert res["status"] == "RESOLVED"
 
 
-@patch("urllib.request.urlopen")
-def test_get_patchset_and_mapping(mock_urlopen: MagicMock, mock_env: None) -> None:
+@patch("urllib.request.build_opener")
+def test_get_patchset_and_mapping(mock_build_opener: MagicMock, mock_env: None) -> None:
     patchset_data = {
         "id": 200,
         "subject": "[PATCH 0/2] Fix some logs",
@@ -208,9 +253,7 @@ def test_get_patchset_and_mapping(mock_urlopen: MagicMock, mock_env: None) -> No
             },
         },
     }
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"list": {"patchset": patchset_data}}}
-    )
+    mock_opener(mock_build_opener, {"data": {"list": {"patchset": patchset_data}}})
     raw = get_patchset("~user", "my-list", 200)
     assert raw["id"] == 200
 
@@ -316,10 +359,10 @@ def get(self, key: str, default: object = None) -> object:
         map_patchset_to_pr(patchset)
 
 
-@patch("urllib.request.urlopen")
-def test_list_patchsets(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200,
+@patch("urllib.request.build_opener")
+def test_list_patchsets(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(
+        mock_build_opener,
         {
             "data": {
                 "list": {
@@ -338,29 +381,27 @@ def test_list_patchsets(mock_urlopen: MagicMock, mock_env: None) -> None:
     assert res[0]["subject"] == "P1"
 
 
-@patch("urllib.request.urlopen")
-def test_get_job(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"job": {"id": 55, "status": "SUCCESS", "tasks": []}}}
-    )
+@patch("urllib.request.build_opener")
+def test_get_job(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(mock_build_opener, {"data": {"job": {"id": 55, "status": "SUCCESS", "tasks": []}}})
     res = get_job(55)
     assert res["status"] == "SUCCESS"
 
 
-@patch("urllib.request.urlopen")
-def test_get_repo(mock_urlopen: MagicMock, mock_env: None) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"repository": {"id": 9, "name": "my-repo", "description": "VCS"}}}
+@patch("urllib.request.build_opener")
+def test_get_repo(mock_build_opener: MagicMock, mock_env: None) -> None:
+    mock_opener(
+        mock_build_opener, {"data": {"repository": {"id": 9, "name": "my-repo", "description": "VCS"}}}
     )
     res = get_repo("git", "~user", "my-repo")
     assert res["name"] == "my-repo"
 
 
-@patch("urllib.request.urlopen")
-def test_cli_dispatch(mock_urlopen: MagicMock, mock_env: None, capsys: pytest.CaptureFixture[str]) -> None:
-    mock_urlopen.return_value.__enter__.return_value = make_mock_response(
-        200, {"data": {"job": {"id": 12, "status": "FAILED"}}}
-    )
+@patch("urllib.request.build_opener")
+def test_cli_dispatch(
+    mock_build_opener: MagicMock, mock_env: None, capsys: pytest.CaptureFixture[str]
+) -> None:
+    mock_opener(mock_build_opener, {"data": {"job": {"id": 12, "status": "FAILED"}}})
     code = main(["build", "get", "12"])
     assert code == 0
     captured = capsys.readouterr()