fix(rpc): Keep request signatures out of error reports

src/scm/rpc/client.py

@@ -150,6 +150,11 @@ class RpcApiClient(ApiClient):
     regardless of request origin (i.e. within Sentry or without).
 
     Access controls are handling on the Sentry-side with required scoping metadata specified in the request's headers.
+
+    A request's signature, which could be replayed, is held in a ``Secret`` and revealed only in the call to the
+    session. The session gets it in plain text in its headers, as does requests below it, so a requests error that
+    reaches the caller carries it in those frames. The ``ResourceServiceUnavailable`` raised instead for a connection
+    error, with ``retry_connection_errors``, does not.
     """
 
     def __init__(
@@ -201,8 +206,8 @@ def request(
             )
         )
 
+        authorization = Secret(f"rpcsignature {sign_post(self.signing_secret, body)}")
         request_headers = {
-            "Authorization": f"rpcsignature {sign_post(self.signing_secret, body)}",
             "Content-Type": "application/json",
             "X-Organization-Id": str(self.organization_id),
             "X-Referrer": self.referrer,
@@ -226,7 +231,12 @@ def request(
         last_reason = ""  # trigger of the most recent retry; tags the recovery metric on success
         while True:
             try:
-                response = self.session.post(url=self.full_url, data=body, headers=request_headers, stream=stream)
+                response = self.session.post(
+                    url=self.full_url,
+                    data=body,
+                    headers={"Authorization": authorization.reveal(), **request_headers},
+                    stream=stream,
+                )
             except _RETRIABLE_TRANSPORT_ERRORS as exc:
                 # Only intercept connection errors when the caller opted in, and only for reads: a
                 # write that died at the transport layer may already have landed upstream, so it is
@@ -244,8 +254,10 @@ def request(
                 if attempt:
                     self._record_retry_outcome("exhausted", method, "connection_error")
                 # Surface a typed, RPC-serializable error instead of an opaque ConnectionError that
-                # would not survive the proxy boundary — independent of whether we retried.
-                raise ResourceServiceUnavailable(detail=f"{type(exc).__name__}: {exc}") from exc
+                # would not survive the proxy boundary — independent of whether we retried. "from None" drops the
+                # connection error, which is reported with its frames, and those of the session and of requests hold
+                # the Authorization header.
+                raise ResourceServiceUnavailable(detail=f"{type(exc).__name__}: {exc}") from None
 
             if idempotent and response.status_code in status_codes:
                 if attempt < max_retries:

src/scm/rpc/helpers.py

@@ -24,17 +24,22 @@ def sign(secret: Secret, message: bytes) -> str:
 
 
 def verify_get(
-    secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: str
+    secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: Secret | str
 ) -> bool:
     return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature)
 
 
-def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: str) -> bool:
+def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: Secret | str) -> bool:
     return verify(secrets, serialized_body, received_signature)
 
 
-def verify(secrets: Sequence[Secret], message: bytes, received_signature: str) -> bool:
-    return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets)
+def verify(secrets: Sequence[Secret], message: bytes, received_signature: Secret | str) -> bool:
+    """Whether ``received_signature`` signs ``message`` with one of ``secrets``.
+
+    A signature with its message can be replayed, so pass it as a ``Secret``. A plain string is still accepted.
+    """
+    received = received_signature if isinstance(received_signature, Secret) else Secret(received_signature)
+    return any(hmac.compare_digest(received.reveal(), sign(secret, message)) for secret in secrets)
 
 
 def deserialize_repository(content: bytes) -> Repository:

src/scm/rpc/server.py

@@ -41,9 +41,12 @@ def __init__(
 
     def get(self, headers: dict[str, str]):
         try:
-            authorization, organization_id, repository_id = self._extract_headers(headers)
+            # Rebound without the signature, so it shows as [Filtered] in the frames of a reported error rather than in
+            # ``headers``.
+            signature, headers = _take_signature(headers)
+            organization_id, repository_id = self._extract_headers(headers)
 
-            if not verify_get(self.secrets, organization_id, repository_id, authorization):
+            if not verify_get(self.secrets, organization_id, repository_id, signature):
                 raise RpcInvalidGrant()
 
             scm = SourceCodeManager.make_client(
@@ -73,7 +76,9 @@ def get(self, headers: dict[str, str]):
 
     def post(self, data: bytes, headers: dict[str, str]) -> StreamResponse:
         try:
-            result = self._post(data, headers)
+            # Rebound without the signature, which would otherwise be reported next to ``data``, the body it signs.
+            signature, headers = _take_signature(headers)
+            result = self._post(data, signature, headers)
             self.record_count("sentry.scm.rpc.request.post.succeeded", 1, {})
             return result
         except SCMCodedError as e:
@@ -90,10 +95,10 @@ def post(self, data: bytes, headers: dict[str, str]) -> StreamResponse:
                 content=iter([error_data]),
             )
 
-    def _post(self, data: bytes, headers: dict[str, str]) -> StreamResponse:
-        authorization, organization_id, repository_id = self._extract_headers(headers)
+    def _post(self, data: bytes, signature: Secret, headers: dict[str, str]) -> StreamResponse:
+        organization_id, repository_id = self._extract_headers(headers)
 
-        if not verify_post(self.secrets, data, authorization):
+        if not verify_post(self.secrets, data, signature):
             raise RpcInvalidGrant()
         if len(data) >= TEN_MEGABYTES:
             raise RpcRequestTooLarge()
@@ -140,12 +145,7 @@ def _post(self, data: bytes, headers: dict[str, str]) -> StreamResponse:
             content=iter_response(response),
         )
 
-    def _extract_headers(self, headers: Mapping[str, str]) -> tuple[str, int, RepositoryId]:
-        try:
-            authorization = headers["Authorization"].removeprefix("rpcsignature ")
-        except KeyError as e:
-            raise RpcMalformedRequestHeaders(detail="Could not find Authorization header") from e
-
+    def _extract_headers(self, headers: Mapping[str, str]) -> tuple[int, RepositoryId]:
         try:
             raw = headers["X-Organization-Id"]
             organization_id = int(raw)
@@ -161,7 +161,21 @@ def _extract_headers(self, headers: Mapping[str, str]) -> tuple[str, int, Reposi
         except msgspec.DecodeError as e:
             raise RpcMalformedRequestHeaders(detail="Could not parse X-Repository-Id header") from e
 
-        return authorization, organization_id, repository_id
+        return organization_id, repository_id
+
+
+def _take_signature(headers: Mapping[str, str]) -> tuple[Secret, dict[str, str]]:
+    """Split the request's signature from the other headers. Header names are case-insensitive."""
+    signature: Secret | None = None
+    rest: dict[str, str] = {}
+    for name, value in headers.items():
+        if name.lower() == "authorization":
+            signature = Secret(value.removeprefix("rpcsignature "))
+        else:
+            rest[name] = value
+    if signature is None:
+        raise RpcMalformedRequestHeaders(detail="Could not find Authorization header")
+    return signature, rest
 
 
 def iter_response(response: requests.Response) -> Iterator[bytes]:

tests/unit/error_reports.py

@@ -7,6 +7,19 @@ def assert_not_in_frames(error: BaseException, value: str) -> None:
     tb = error.__traceback__
     while tb is not None:
         for name, local in tb.tb_frame.f_locals.items():
-            shown = local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local)
-            assert value not in shown, f"{tb.tb_frame.f_code.co_name} holds it in {name}"
+            assert value not in _shown(local), f"{tb.tb_frame.f_code.co_name} holds it in {name}"
         tb = tb.tb_next
+
+
+def reported_local(error: BaseException, function: str, name: str) -> str:
+    """How an error report shows the local variable ``name`` of ``function``'s frame in the error's traceback."""
+    tb = error.__traceback__
+    while tb is not None:
+        if tb.tb_frame.f_code.co_name == function:
+            return _shown(tb.tb_frame.f_locals[name])
+        tb = tb.tb_next
+    raise AssertionError(f"The traceback has no frame of {function}.")
+
+
+def _shown(local: object) -> str:
+    return local.__sentry_repr__() if hasattr(local, "__sentry_repr__") else repr(local)

tests/unit/test_rpc_client.py

@@ -3,6 +3,7 @@
 
 import msgspec
 import pytest
+import requests
 from requests.exceptions import ConnectionError as RequestsConnectionError
 
 from scm.errors import ErrorCode, ResourceServiceUnavailable, SCMCodedError
@@ -19,6 +20,7 @@
 from scm.rpc.server import serialize_repository
 from scm.rpc.types import Error, ErrorResponse
 from scm.secret import Secret
+from tests.unit.error_reports import assert_not_in_frames, reported_local
 
 
 def make_repository(**overrides):
@@ -532,3 +534,43 @@ def test_does_not_retry_status_code_outside_config(self, mock_sleep):
         assert client.session.post.call_count == 1
         mock_sleep.assert_not_called()
         assert self._metric_names(client) == []
+
+
+class UnreachableSession:
+    """Keeps the Authorization header it is sent, then fails as if Sentry were unreachable."""
+
+    def __init__(self) -> None:
+        self.authorization = ""
+
+    def get(self, url: str, headers: dict[str, str]) -> requests.Response:
+        raise NotImplementedError
+
+    def post(self, url: str, data: bytes, headers: dict[str, str], stream: bool = False) -> requests.Response:
+        self.authorization = headers["Authorization"]
+        raise RequestsConnectionError("connection reset")
+
+
+class TestKeptOutOfErrorReports:
+    """An error is reported to Sentry with every frame's local variables, and a signature in one could be replayed."""
+
+    def test_exhausted_connection_retries_leave_out_the_signature(self):
+        session = UnreachableSession()
+        client = RpcApiClient(
+            full_url="http://base/api/0/internal/scm-rpc",
+            signing_secret=Secret("secret"),
+            organization_id=1,
+            referrer="shared",
+            repository_id=1,
+            session=lambda: session,
+            retry={"max_retries": 1, "backoff_seconds": 0, "status_codes": (), "retry_connection_errors": True},
+        )
+
+        with pytest.raises(ResourceServiceUnavailable) as exc_info:
+            client.request(method="GET", path="/repos/org/repo")
+
+        error = exc_info.value
+        assert_not_in_frames(error, session.authorization.removeprefix("rpcsignature "))
+        assert reported_local(error, "request", "authorization") == "[Filtered]"
+        # A chained exception is reported with its own frames, and the session's holds the headers it was sent.
+        assert error.__cause__ is None
+        assert error.__suppress_context__

tests/unit/test_rpc_server.py

@@ -4,12 +4,20 @@
 import msgspec
 import pytest
 
-from scm.errors import SCMCodedError
+from scm.errors import ResourceServiceUnavailable, SCMCodedError
 from scm.rpc.helpers import deserialize_repository, sign_get, sign_post
-from scm.rpc.server import RpcServer, is_safe_path, iter_response, normalize_headers, serialize_repository
+from scm.rpc.server import (
+    RpcServer,
+    _take_signature,
+    is_safe_path,
+    iter_response,
+    normalize_headers,
+    serialize_repository,
+)
 from scm.secret import Secret
 from scm.test_fixtures import BaseTestProvider
 from scm.types import Repository
+from tests.unit.error_reports import assert_not_in_frames, reported_local
 
 TEST_SECRET = Secret("test-secret")
 
@@ -55,28 +63,38 @@ def make_headers(**overrides) -> dict[str, str]:
     return defaults
 
 
+class TestTakeSignature:
+    def test_takes_the_signature_out_of_the_headers(self):
+        signature, headers = _take_signature(make_headers())
+        assert signature.reveal() == "rpc0:abc"
+        assert headers == {"X-Organization-Id": "1", "X-Repository-Id": "1"}
+
+    def test_header_name_is_case_insensitive(self):
+        signature, headers = _take_signature({"authorization": "rpcsignature rpc0:abc", "X-Organization-Id": "1"})
+        assert signature.reveal() == "rpc0:abc"
+        assert headers == {"X-Organization-Id": "1"}
+
+    def test_missing_authorization_raises(self):
+        headers = make_headers()
+        del headers["Authorization"]
+        with pytest.raises(SCMCodedError) as exc_info:
+            _take_signature(headers)
+        assert exc_info.value.code == "rpc_malformed_request_headers"
+
+
 class TestExtractHeaders:
     def test_extracts_int_repository_id(self):
         server = make_server()
-        auth, org_id, repo_id = server._extract_headers(make_headers())
-        assert auth == "rpc0:abc"
+        org_id, repo_id = server._extract_headers(make_headers())
         assert org_id == 1
         assert repo_id == 1
 
     def test_extracts_tuple_repository_id(self):
         server = make_server()
         headers = make_headers(**{"X-Repository-Id": '["github","ext-123"]'})
-        _, _, repo_id = server._extract_headers(headers)
+        _, repo_id = server._extract_headers(headers)
         assert repo_id == ("github", "ext-123")
 
-    def test_missing_authorization_raises(self):
-        server = make_server()
-        headers = make_headers()
-        del headers["Authorization"]
-        with pytest.raises(SCMCodedError) as exc_info:
-            server._extract_headers(headers)
-        assert exc_info.value.code == "rpc_malformed_request_headers"
-
     def test_missing_organization_id_raises(self):
         server = make_server()
         headers = make_headers()
@@ -290,6 +308,53 @@ def test_successful_post_streams_response(self):
         assert b'{"ref": "main"}' in chunks
 
 
+class UnavailableProvider(BaseTestProvider):
+    def request(self, *args, **kwargs):  # type: ignore[override]
+        raise ResourceServiceUnavailable()
+
+
+class TestKeptOutOfErrorReports:
+    """An error is reported to Sentry with every frame's local variables, and a signature in one could be replayed."""
+
+    @pytest.mark.parametrize("header", ["Authorization", "authorization"])
+    def test_post_reporting_a_provider_failure_leaves_out_the_signature(self, header):
+        errors: list[Exception] = []
+        server = make_server(fetch_provider=lambda org_id, repo: UnavailableProvider(), emit_error=errors.append)
+        body = msgspec.json.encode(
+            {
+                "type": "action",
+                "data": {
+                    "method": "GET",
+                    "path": "/repos/org/repo",
+                    "headers": None,
+                    "data": None,
+                    "params": None,
+                    "allow_redirects": None,
+                    "stream": None,
+                },
+            }
+        )
+        signature = sign_post(TEST_SECRET, body)
+
+        server.post(body, {header: f"rpcsignature {signature}", "X-Organization-Id": "1", "X-Repository-Id": "1"})
+
+        [error] = errors
+        assert error.code == "resource_service_unavailable"  # type: ignore[attr-defined]
+        assert_not_in_frames(error, signature)
+        assert reported_local(error, "post", "signature") == "[Filtered]"
+
+    def test_get_failing_to_fetch_the_repository_leaves_out_the_signature(self):
+        def failing_fetch(organization_id, repository_id):
+            raise RuntimeError("database down")
+
+        server = make_server(fetch_repository=failing_fetch)
+        # Not a coded error, so it reaches the caller, whose error reporting sees every frame here.
+        with pytest.raises(RuntimeError) as exc_info:
+            server.get(make_headers(Authorization=f"rpcsignature {sign_get(TEST_SECRET, 1, 1)}"))
+        assert_not_in_frames(exc_info.value, sign_get(TEST_SECRET, 1, 1))
+        assert reported_local(exc_info.value, "get", "signature") == "[Filtered]"
+
+
 class TestSerializeRepository:
     def test_serializes_all_fields(self):
         repo = make_repository()