From 5b81a9c256fc20622772708dd42f5ba638a93825 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 16:28:03 +0200 Subject: [PATCH 1/6] fix(rpc): Keep RPC signing secrets out of error reports Sentry's SDK sends the local variables of every frame in a reported traceback, and its default scrubbing only replaces a few exact variable names, never looking inside a list or a dict. "secret" is one of them; "secrets" and "signing_secret" are not. So when make_proxy_client fails to fetch the repository, for example because Sentry is unreachable, the event carries the client's signing secret three times: in make_proxy_client, in its fetch_repository lambda and in fetch_repository. If anything fails while RpcServer verifies a signature, the verify frames carry the server's list of secrets. A small Secret wrapper shows as [Filtered] to the SDK, through __sentry_repr__, and to repr() and str(). The RPC client and server wrap the secrets they are given on entry, and sign() unwraps one inline where it signs. Callers can keep passing plain strings. The plain string is still in the caller's frame and in any frame after reveal(), so this adds to the SDK's scrubbing rather than replacing it. Co-Authored-By: Claude Opus 5.5 --- src/scm/manager.py | 6 ++- src/scm/rpc/client.py | 9 ++-- src/scm/rpc/helpers.py | 18 +++++--- src/scm/rpc/server.py | 6 ++- src/scm/secret.py | 33 ++++++++++++++ tests/unit/test_secret.py | 96 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 src/scm/secret.py create mode 100644 tests/unit/test_secret.py diff --git a/src/scm/manager.py b/src/scm/manager.py index 1c882fd5..b38a9ae4 100644 --- a/src/scm/manager.py +++ b/src/scm/manager.py @@ -10,6 +10,7 @@ ) from scm.rpc.client import fetch_provider as fetch_proxy_provider from scm.rpc.client import fetch_repository as fetch_proxy_repository +from scm.secret import Secret from scm.types import Provider, Referrer, Repository, RepositoryId @@ -44,10 +45,13 @@ def make_proxy_client( *, referrer: Referrer = "shared", base_url: str, - signing_secret: str, + signing_secret: str | Secret, record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, retry: RetryConfig | None = None, ): + # Rebound, so the plain string is not in this frame, or in the lambda below, when fetching the repository + # fails and the error is reported. + signing_secret = Secret.of(signing_secret) full_url = SCM_API_URL.format(base_url=base_url) # A specialized RpcApiClient is initialized. It will proxy the service-provider requests through Sentry. This diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index 831a880d..067915f5 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -17,6 +17,7 @@ from scm.providers.gitlab.provider import GitLabProvider from scm.rpc.helpers import deserialize_repository, sign_get, sign_post from scm.rpc.types import ActionAttributes, ActionRequest, ErrorResponse +from scm.secret import Secret from scm.types import ApiClient, CredentialsSet, Provider, Repository, RepositoryId SCM_API_URL = "{base_url}/api/0/internal/scm-rpc/" @@ -73,12 +74,14 @@ def post(self, url: str, data: bytes, headers: dict[str, str], stream: bool = Fa def fetch_repository( url: str, - signing_secret: str, + signing_secret: str | Secret, organization_id: int, repository_id: RepositoryId, session: Callable[[], Session] = lambda: RequestsSession(), ) -> Repository: """Fetch repositorty metadata.""" + # Rebound, so the plain string is not in this frame when the request fails and the error is reported. + signing_secret = Secret.of(signing_secret) response = session().get( url, headers={ @@ -154,7 +157,7 @@ class RpcApiClient(ApiClient): def __init__( self, full_url: str, - signing_secret: str, + signing_secret: str | Secret, organization_id: int, referrer: str, repository_id: RepositoryId, @@ -163,7 +166,7 @@ def __init__( record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, ) -> None: self.full_url = full_url - self.signing_secret = signing_secret + self.signing_secret = Secret.of(signing_secret) self.organization_id = organization_id self.referrer = referrer self.repository_id = repository_id diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index 46c1d895..91e7512d 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -1,35 +1,39 @@ import hashlib import hmac +from collections.abc import Sequence from typing import Literal import msgspec from scm.errors import RepositoryCouldNotBeDeserialized from scm.rpc.types import JsonApiData, JsonApiPayload, RepositoryAttributes +from scm.secret import Secret from scm.types import Repository, RepositoryId -def sign_get(secret: str, organization_id: int, repository_id: RepositoryId) -> str: +def sign_get(secret: str | Secret, organization_id: int, repository_id: RepositoryId) -> str: return sign(secret, f"{organization_id}{repository_id}".encode()) -def sign_post(secret: str, serialized_body: bytes) -> str: +def sign_post(secret: str | Secret, serialized_body: bytes) -> str: return sign(secret, serialized_body) -def sign(secret: str, message: bytes) -> str: - return f"rpc0:{hmac.new(secret.encode(), message, hashlib.sha256).hexdigest()}" +def sign(secret: str | Secret, message: bytes) -> str: + return f"rpc0:{hmac.new(Secret.of(secret).reveal().encode(), message, hashlib.sha256).hexdigest()}" -def verify_get(secrets: list[str], organization_id: int, repository_id: RepositoryId, received_signature: str) -> bool: +def verify_get( + secrets: Sequence[str | Secret], organization_id: int, repository_id: RepositoryId, received_signature: str +) -> bool: return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature) -def verify_post(secrets: list[str], serialized_body: bytes, received_signature: str) -> bool: +def verify_post(secrets: Sequence[str | Secret], serialized_body: bytes, received_signature: str) -> bool: return verify(secrets, serialized_body, received_signature) -def verify(secrets: list[str], message: bytes, received_signature: str) -> bool: +def verify(secrets: Sequence[str | Secret], message: bytes, received_signature: str) -> bool: return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets) diff --git a/src/scm/rpc/server.py b/src/scm/rpc/server.py index f7b9a785..341010d0 100644 --- a/src/scm/rpc/server.py +++ b/src/scm/rpc/server.py @@ -17,6 +17,7 @@ from scm.rpc.errors import serialize_error from scm.rpc.helpers import serialize_repository, verify_get, verify_post from scm.rpc.types import ActionRequest, Response, StreamResponse +from scm.secret import Secret from scm.types import Provider, Repository, RepositoryId TEN_MEGABYTES = 10 * 1024 * 1024 @@ -26,13 +27,14 @@ class RpcServer: def __init__( self, - secrets: list[str], + secrets: list[str] | list[Secret], fetch_repository: Callable[[int, RepositoryId], Repository | None], fetch_provider: Callable[[int, Repository], Provider | None], record_count: Callable[[str, int, dict[str, str]], None], emit_error: Callable[[Exception], None], ) -> None: - self.secrets = secrets + # Wrapped, so the secrets show as [Filtered] in the frames of a reported error, e.g. while verifying. + self.secrets = [Secret.of(secret) for secret in secrets] self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.record_count = record_count diff --git a/src/scm/secret.py b/src/scm/secret.py new file mode 100644 index 00000000..a6083d11 --- /dev/null +++ b/src/scm/secret.py @@ -0,0 +1,33 @@ +class Secret: + """A string that must not reach a log line or an error report, such as a signing secret. + + Sentry's SDK sends the local variables of every frame in a reported traceback, and its default scrubbing only + replaces variables with a few exact names (``secret`` is one, ``secrets`` and ``signing_secret`` are not), never + looking inside a list or a dict. Held in a ``Secret``, the value shows as ``[Filtered]`` wherever it is: the SDK + calls ``__sentry_repr__`` first, and ``repr()`` and ``str()`` hide it too. + + ``reveal()`` returns the plain string, which is then in the frame that called it. Call it where the value is used, + inline, rather than keeping the result in a variable. + """ + + __slots__ = ("_value",) + + def __init__(self, value: str) -> None: + self._value = value + + @classmethod + def of(cls, value: "str | Secret") -> "Secret": + """Wrap ``value``, unless it already is a ``Secret``.""" + return value if isinstance(value, Secret) else cls(value) + + def reveal(self) -> str: + return self._value + + def __repr__(self) -> str: + return "Secret('[Filtered]')" + + def __str__(self) -> str: + return "[Filtered]" + + def __sentry_repr__(self) -> str: + return "[Filtered]" diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py new file mode 100644 index 00000000..b38c2753 --- /dev/null +++ b/tests/unit/test_secret.py @@ -0,0 +1,96 @@ +import pytest +import requests + +from scm.manager import SourceCodeManager +from scm.rpc import helpers +from scm.rpc.client import RpcApiClient +from scm.rpc.helpers import sign_get +from scm.rpc.server import RpcServer +from scm.secret import Secret + +SECRET = "rpc-signing-secret-0000000000000000" + + +def assert_not_in_frames(error: BaseException, value: str) -> None: + """Fail if any frame of the error's traceback holds ``value`` where an error report would show it. + + Sentry's SDK sends every frame's local variables, using ``__sentry_repr__`` or else ``repr()`` for each, and its + default scrubbing only replaces variables with a few exact names, such as ``secret``. + """ + 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}" + tb = tb.tb_next + + +class TestSecret: + def test_reveal(self): + assert Secret(SECRET).reveal() == SECRET + + def test_hidden_from_repr_str_and_formatting(self): + secret = Secret(SECRET) + for shown in (repr(secret), str(secret), f"{secret}", f"{secret!r}", repr([secret]), repr({"key": secret})): + assert SECRET not in shown + assert repr(secret) == "Secret('[Filtered]')" + + def test_hidden_from_sentry(self): + assert Secret(SECRET).__sentry_repr__() == "[Filtered]" + + def test_has_no_attributes_to_look_into(self): + # With __slots__ there is no __dict__ for a serializer to walk. + assert not hasattr(Secret(SECRET), "__dict__") + + def test_of_wraps_a_string_once(self): + secret = Secret(SECRET) + assert Secret.of(secret) is secret + assert Secret.of(SECRET).reveal() == SECRET + + def test_signing_accepts_either(self): + assert sign_get(Secret(SECRET), 1, 5) == sign_get(SECRET, 1, 5) + + +class TestKeptOutOfErrorReports: + def test_proxy_client_that_cannot_reach_sentry(self, monkeypatch): + def unreachable(*args, **kwargs): + raise requests.ConnectionError("Sentry is unreachable") + + monkeypatch.setattr(requests, "get", unreachable) + with pytest.raises(requests.ConnectionError) as exc_info: + SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=SECRET) + assert_not_in_frames(exc_info.value, SECRET) + + def test_api_client_that_cannot_reach_sentry(self): + class Unreachable: + def post(self, url, data, headers, stream=False): + raise requests.ConnectionError("Sentry is unreachable") + + client = RpcApiClient( + full_url="https://sentry.example/api/0/internal/scm-rpc/", + signing_secret=SECRET, + organization_id=1, + referrer="shared", + repository_id=5, + session=Unreachable, + ) + with pytest.raises(requests.ConnectionError) as exc_info: + client.request("GET", "/repos/getsentry/sentry") + assert_not_in_frames(exc_info.value, SECRET) + + def test_server_failing_while_it_verifies(self, monkeypatch): + def failing_compare(a, b): + raise RuntimeError("compare failed") + + monkeypatch.setattr(helpers.hmac, "compare_digest", failing_compare) + server = RpcServer( + secrets=[SECRET], + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error: None, + ) + headers = {"Authorization": "rpcsignature rpc0:abc", "X-Organization-Id": "1", "X-Repository-Id": "5"} + with pytest.raises(RuntimeError) as exc_info: + server.get(headers) + assert_not_in_frames(exc_info.value, SECRET) From 12029e32fd0fed864e6c870766abea60c952f39e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 16:40:53 +0200 Subject: [PATCH 2/6] fix(rpc): Deprecate passing RPC signing secrets as plain strings Wrapping the secret on entry keeps it out of this library's frames, but a caller that passes a plain str still holds it in its own frame, where an error report shows it. Only the caller can wrap it where it reads it. Requiring Secret outright would fail type checks in Sentry and Seer, and dropping the coercion would break their RPC calls at runtime, so plain strings keep working for now with a DeprecationWarning that points at the calling line. Secret is exported from the scm package so callers have a stable import, and the bin/ clients and servers wrap theirs already. Co-Authored-By: Claude Opus 5.5 --- bin/github-client | 3 +- bin/github-server | 3 +- bin/gitlab-client | 3 +- bin/gitlab-server | 3 +- src/scm/__init__.py | 3 ++ src/scm/manager.py | 4 +- src/scm/rpc/client.py | 6 +-- src/scm/rpc/server.py | 4 +- src/scm/secret.py | 33 ++++++++++++ tests/unit/test_rpc_client.py | 25 ++++----- tests/unit/test_rpc_integration.py | 9 ++-- tests/unit/test_rpc_server.py | 3 +- tests/unit/test_secret.py | 83 +++++++++++++++++++++++++++++- 13 files changed, 154 insertions(+), 28 deletions(-) diff --git a/bin/github-client b/bin/github-client index 8b96aa83..e3d6d524 100755 --- a/bin/github-client +++ b/bin/github-client @@ -94,6 +94,7 @@ import pathlib import sys from datetime import date +from scm import Secret from scm.manager import SourceCodeManager from scm.types import ( CheckRunOutput, @@ -518,7 +519,7 @@ def main() -> None: args.org_id, ("github", args.repo), base_url=args.base_url if args.base_url else "http://127.0.0.1:8080", - signing_secret=args.secret if args.secret else "secret", + signing_secret=Secret(args.secret if args.secret else "secret"), ) if args.command == "get-pull-request": diff --git a/bin/github-server b/bin/github-server index 66d8e9c8..0f568da7 100755 --- a/bin/github-server +++ b/bin/github-server @@ -53,6 +53,7 @@ from typing import Any import jwt import requests +from scm import Secret from scm.providers.github.provider import GitHubProvider from scm.rpc.server import RpcServer from scm.types import CredentialsSet, Provider, Repository, RepositoryId @@ -288,7 +289,7 @@ def main(): app_id = resolve("GITHUB_APP_ID", args.app_id, creds) private_key_path = resolve("GITHUB_PRIVATE_KEY_PATH", args.private_key, creds) installation_id = resolve("GITHUB_INSTALLATION_ID", args.installation_id, creds) - secret = resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret" + secret = Secret(resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret") base_url = resolve("GITHUB_BASE_URL", args.base_url, creds) or GITHUB_API_BASE web_base_url = resolve("GITHUB_WEB_BASE_URL", args.web_base_url, creds) or _derive_web_base_url(base_url) provider_name = resolve("GITHUB_PROVIDER_NAME", args.provider_name, creds) or _derive_provider_name(base_url) diff --git a/bin/gitlab-client b/bin/gitlab-client index d9fcb90e..8779b0b3 100755 --- a/bin/gitlab-client +++ b/bin/gitlab-client @@ -88,6 +88,7 @@ import pathlib import sys from datetime import date +from scm import Secret from scm.manager import SourceCodeManager from scm.types import ( CheckRunOutput, @@ -411,7 +412,7 @@ def main(): kwargs = { "base_url": args.base_url if args.base_url else "http://127.0.0.1:8080", - "signing_secret": args.secret if args.secret else "secret", + "signing_secret": Secret(args.secret if args.secret else "secret"), } scm = SourceCodeManager.make_proxy_client( diff --git a/bin/gitlab-server b/bin/gitlab-server index 9a6f871f..c2266ba7 100755 --- a/bin/gitlab-server +++ b/bin/gitlab-server @@ -34,6 +34,7 @@ from urllib.parse import urlparse import requests +from scm import Secret from scm.providers.gitlab.provider import API_VERSION, GitLabProvider from scm.rpc.server import RpcServer from scm.types import CredentialsSet, Provider, Repository, RepositoryId @@ -192,7 +193,7 @@ def main(): access_token = resolve("GITLAB_ACCESS_TOKEN", args.access_token, creds) base_url = resolve("GITLAB_BASE_URL", args.base_url, creds) or "https://gitlab.com" - secret = resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret" + secret = Secret(resolve("SCM_RPC_SIGNING_SECRET", args.secret, creds) or "secret") if not access_token: logger.error("Error: missing required config: GITLAB_ACCESS_TOKEN") diff --git a/src/scm/__init__.py b/src/scm/__init__.py index e69de29b..aaed2808 100644 --- a/src/scm/__init__.py +++ b/src/scm/__init__.py @@ -0,0 +1,3 @@ +from scm.secret import Secret + +__all__ = ("Secret",) diff --git a/src/scm/manager.py b/src/scm/manager.py index b38a9ae4..1e41f6b6 100644 --- a/src/scm/manager.py +++ b/src/scm/manager.py @@ -10,7 +10,7 @@ ) from scm.rpc.client import fetch_provider as fetch_proxy_provider from scm.rpc.client import fetch_repository as fetch_proxy_repository -from scm.secret import Secret +from scm.secret import Secret, accept_secret from scm.types import Provider, Referrer, Repository, RepositoryId @@ -51,7 +51,7 @@ def make_proxy_client( ): # Rebound, so the plain string is not in this frame, or in the lambda below, when fetching the repository # fails and the error is reported. - signing_secret = Secret.of(signing_secret) + signing_secret = accept_secret(signing_secret, parameter="signing_secret") full_url = SCM_API_URL.format(base_url=base_url) # A specialized RpcApiClient is initialized. It will proxy the service-provider requests through Sentry. This diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index 067915f5..60026239 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -17,7 +17,7 @@ from scm.providers.gitlab.provider import GitLabProvider from scm.rpc.helpers import deserialize_repository, sign_get, sign_post from scm.rpc.types import ActionAttributes, ActionRequest, ErrorResponse -from scm.secret import Secret +from scm.secret import Secret, accept_secret from scm.types import ApiClient, CredentialsSet, Provider, Repository, RepositoryId SCM_API_URL = "{base_url}/api/0/internal/scm-rpc/" @@ -81,7 +81,7 @@ def fetch_repository( ) -> Repository: """Fetch repositorty metadata.""" # Rebound, so the plain string is not in this frame when the request fails and the error is reported. - signing_secret = Secret.of(signing_secret) + signing_secret = accept_secret(signing_secret, parameter="signing_secret") response = session().get( url, headers={ @@ -166,7 +166,7 @@ def __init__( record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, ) -> None: self.full_url = full_url - self.signing_secret = Secret.of(signing_secret) + self.signing_secret = accept_secret(signing_secret, parameter="signing_secret") self.organization_id = organization_id self.referrer = referrer self.repository_id = repository_id diff --git a/src/scm/rpc/server.py b/src/scm/rpc/server.py index 341010d0..c2903c0f 100644 --- a/src/scm/rpc/server.py +++ b/src/scm/rpc/server.py @@ -17,7 +17,7 @@ from scm.rpc.errors import serialize_error from scm.rpc.helpers import serialize_repository, verify_get, verify_post from scm.rpc.types import ActionRequest, Response, StreamResponse -from scm.secret import Secret +from scm.secret import Secret, accept_secrets from scm.types import Provider, Repository, RepositoryId TEN_MEGABYTES = 10 * 1024 * 1024 @@ -34,7 +34,7 @@ def __init__( emit_error: Callable[[Exception], None], ) -> None: # Wrapped, so the secrets show as [Filtered] in the frames of a reported error, e.g. while verifying. - self.secrets = [Secret.of(secret) for secret in secrets] + self.secrets = accept_secrets(secrets, parameter="secrets") self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.record_count = record_count diff --git a/src/scm/secret.py b/src/scm/secret.py index a6083d11..d86bd35b 100644 --- a/src/scm/secret.py +++ b/src/scm/secret.py @@ -1,3 +1,7 @@ +import warnings +from collections.abc import Sequence + + class Secret: """A string that must not reach a log line or an error report, such as a signing secret. @@ -31,3 +35,32 @@ def __str__(self) -> str: def __sentry_repr__(self) -> str: return "[Filtered]" + + +def accept_secret(value: str | Secret, *, parameter: str) -> Secret: + """Wrap a secret given to a public entry point, warning if it came as a plain string. + + Call it first thing in the entry point, so the warning points at the entry point's caller. + """ + if isinstance(value, str): + _warn_plain_string(parameter) + return Secret.of(value) + + +def accept_secrets(values: Sequence[str | Secret], *, parameter: str) -> list[Secret]: + """Like ``accept_secret``, for a list of secrets, warning once however many are plain strings.""" + if any(isinstance(value, str) for value in values): + _warn_plain_string(parameter) + return [Secret.of(value) for value in values] + + +def _warn_plain_string(parameter: str) -> None: + # A plain string stays in the caller's frame, where an error report shows it, so callers should wrap it where they + # read it. Accepted until the next major release, so Sentry and Seer can move over first. + warnings.warn( + f"Passing {parameter} as a plain str is deprecated and will stop working in the next major release. " + 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).', + DeprecationWarning, + # This function, accept_secret(s), the entry point, then the entry point's caller. + stacklevel=4, + ) diff --git a/tests/unit/test_rpc_client.py b/tests/unit/test_rpc_client.py index 94b4e9fb..3e4c394d 100644 --- a/tests/unit/test_rpc_client.py +++ b/tests/unit/test_rpc_client.py @@ -18,6 +18,7 @@ ) from scm.rpc.server import serialize_repository from scm.rpc.types import Error, ErrorResponse +from scm.secret import Secret def make_repository(**overrides): @@ -62,7 +63,7 @@ class TestFetchRepository: @patch("scm.rpc.client.requests.get") def test_success(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - repo = fetch_repository("http://base", "secret", 1, 1) + repo = fetch_repository("http://base", Secret("secret"), 1, 1) assert repo["name"] == "org/repo" assert repo["provider_name"] == "github" @@ -70,7 +71,7 @@ def test_success(self, mock_get): def test_single_error_raises_coded_error(self, mock_get): mock_get.return_value = MagicMock(status_code=404, content=make_error_response("repository_not_found")) with pytest.raises(SCMCodedError) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) assert exc_info.value.code == "repository_not_found" @patch("scm.rpc.client.requests.get") @@ -80,7 +81,7 @@ def test_multiple_errors_raises_exception_group(self, mock_get): content=make_error_response("repository_not_found", "rpc_invalid_grant"), ) with pytest.raises(ExceptionGroup) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) codes = {e.code for e in exc_info.value.exceptions} assert codes == {"repository_not_found", "rpc_invalid_grant"} @@ -89,13 +90,13 @@ def test_multiple_errors_raises_exception_group(self, mock_get): def test_undeserializable_error_response(self, mock_get): mock_get.return_value = MagicMock(status_code=500, content=b"not valid json") with pytest.raises(SCMCodedError) as exc_info: - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) assert exc_info.value.code == "rpc_errors_could_not_be_deserialized" @patch("scm.rpc.client.requests.get") def test_signs_get_request_headers(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - fetch_repository("http://base", "secret", 1, 1) + fetch_repository("http://base", Secret("secret"), 1, 1) call_kwargs = mock_get.call_args headers = call_kwargs.kwargs["headers"] @@ -105,7 +106,7 @@ def test_signs_get_request_headers(self, mock_get): @patch("scm.rpc.client.requests.get") def test_tuple_repository_id(self, mock_get): mock_get.return_value = MagicMock(status_code=200, content=make_serialized_repository()) - fetch_repository("http://base", "secret", 1, ("github", "ext-123")) + fetch_repository("http://base", Secret("secret"), 1, ("github", "ext-123")) headers = mock_get.call_args.kwargs["headers"] assert headers["X-Repository-Id"] == '["github","ext-123"]' @@ -183,7 +184,7 @@ class TestRpcApiClient: def test_request_encodes_action_and_signs(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="test-referrer", repository_id=1, @@ -224,7 +225,7 @@ def test_request_encodes_action_and_signs(self): def test_request_with_tuple_repository_id(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=("github", "ext-123"), @@ -240,7 +241,7 @@ def test_request_with_tuple_repository_id(self): def test_request_forwards_stream_opt_out(self): client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -269,7 +270,7 @@ def _make_client( # Retries are off by default in the library; these tests opt in to exercise the retry path. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -409,7 +410,7 @@ def test_no_retries_by_default(self, mock_sleep): # and no transport_retry metric fires. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, @@ -459,7 +460,7 @@ def test_connection_error_intercept_defaults_off_when_omitted(self, mock_sleep): # retry_connection_errors is optional; omitting it leaves connection errors untouched. client = RpcApiClient( full_url="http://base/api/0/internal/scm-rpc", - signing_secret="secret", + signing_secret=Secret("secret"), organization_id=1, referrer="shared", repository_id=1, diff --git a/tests/unit/test_rpc_integration.py b/tests/unit/test_rpc_integration.py index ec266701..ef6d1262 100644 --- a/tests/unit/test_rpc_integration.py +++ b/tests/unit/test_rpc_integration.py @@ -17,6 +17,7 @@ from scm.rpc.errors import deserialize_error from scm.rpc.helpers import sign_get from scm.rpc.server import RpcServer +from scm.secret import Secret from scm.test_fixtures import ( make_github_assignee, make_github_branch, @@ -83,7 +84,7 @@ def make_github_api_response( def make_rpc_server(repository: Repository, server_provider: MagicMock) -> RpcServer: return RpcServer( - secrets=[SIGNING_SECRET], + secrets=[Secret(SIGNING_SECRET)], fetch_repository=lambda org_id, repo_id: repository, fetch_provider=lambda org_id, repo: server_provider, record_count=lambda name, value, tags: None, @@ -129,7 +130,7 @@ def make_client_scm(organization_id, repository_id, server): # This is a real AoiClient but we'll client = RpcApiClient( full_url=BASE_URL, - signing_secret=SIGNING_SECRET, + signing_secret=Secret(SIGNING_SECRET), organization_id=organization_id, referrer="shared", repository_id=repository_id, @@ -141,7 +142,9 @@ def make_client_scm(organization_id, repository_id, server): scm = SourceCodeManager.make_client( organization_id, repository_id, - fetch_repository=lambda oid, rid: fetch_proxy_repository(BASE_URL, SIGNING_SECRET, oid, rid, client.session), + fetch_repository=lambda oid, rid: fetch_proxy_repository( + BASE_URL, Secret(SIGNING_SECRET), oid, rid, client.session + ), fetch_provider=lambda oid, repo: fetch_proxy_provider(client, oid, repo), record_count=lambda a, b, c: None, ) diff --git a/tests/unit/test_rpc_server.py b/tests/unit/test_rpc_server.py index 62fea8dc..bf6399f8 100644 --- a/tests/unit/test_rpc_server.py +++ b/tests/unit/test_rpc_server.py @@ -7,6 +7,7 @@ from scm.errors import 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.secret import Secret from scm.test_fixtures import BaseTestProvider from scm.types import Repository @@ -34,7 +35,7 @@ def mock_record_count(a, b, c): def make_server(**overrides) -> RpcServer: defaults = dict( - secrets=[TEST_SECRET], + secrets=[Secret(TEST_SECRET)], fetch_repository=lambda org_id, repo_id: make_repository(), fetch_provider=lambda org_id, repo: BaseTestProvider(), record_count=mock_record_count, diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py index b38c2753..4eaeb7cc 100644 --- a/tests/unit/test_secret.py +++ b/tests/unit/test_secret.py @@ -1,9 +1,12 @@ +import warnings + import pytest import requests +from scm import Secret as ExportedSecret from scm.manager import SourceCodeManager from scm.rpc import helpers -from scm.rpc.client import RpcApiClient +from scm.rpc.client import RpcApiClient, fetch_repository from scm.rpc.helpers import sign_get from scm.rpc.server import RpcServer from scm.secret import Secret @@ -51,6 +54,8 @@ def test_signing_accepts_either(self): assert sign_get(Secret(SECRET), 1, 5) == sign_get(SECRET, 1, 5) +# Plain strings, the worst case: wrapping on entry must still keep them out of this library's frames. +@pytest.mark.filterwarnings("ignore::DeprecationWarning") class TestKeptOutOfErrorReports: def test_proxy_client_that_cannot_reach_sentry(self, monkeypatch): def unreachable(*args, **kwargs): @@ -94,3 +99,79 @@ def failing_compare(a, b): with pytest.raises(RuntimeError) as exc_info: server.get(headers) assert_not_in_frames(exc_info.value, SECRET) + + +def unreachable_get(*args, **kwargs): + raise requests.ConnectionError("Sentry is unreachable") + + +class UnreachableSession: + def get(self, url, headers): + raise requests.ConnectionError("Sentry is unreachable") + + +def make_server(secrets): + return RpcServer( + secrets=secrets, + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error: None, + ) + + +def make_api_client(signing_secret): + return RpcApiClient( + full_url="https://sentry.example/api/0/internal/scm-rpc/", + signing_secret=signing_secret, + organization_id=1, + referrer="shared", + repository_id=5, + ) + + +class TestPlainStringDeprecated: + def assert_warns_at_caller(self, record: pytest.WarningsRecorder, parameter: str) -> None: + assert len(record) == 1 + assert f"Passing {parameter} as a plain str is deprecated" in str(record[0].message) + # Points at the code that passed the string, not at this library. + assert record[0].filename == __file__ + + def test_exported_from_the_package(self): + assert ExportedSecret is Secret + + def test_proxy_client(self, monkeypatch): + monkeypatch.setattr(requests, "get", unreachable_get) + with pytest.warns(DeprecationWarning) as record, pytest.raises(requests.ConnectionError): + SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=SECRET) + self.assert_warns_at_caller(record, "signing_secret") + + def test_fetch_repository(self): + with pytest.warns(DeprecationWarning) as record, pytest.raises(requests.ConnectionError): + fetch_repository("https://sentry.example", SECRET, 1, 5, session=UnreachableSession) + self.assert_warns_at_caller(record, "signing_secret") + + def test_api_client(self): + with pytest.warns(DeprecationWarning) as record: + make_api_client(SECRET) + # make_api_client is in this file too, so the caller check still holds. + self.assert_warns_at_caller(record, "signing_secret") + + def test_server_warns_once_for_several(self): + with pytest.warns(DeprecationWarning) as record: + server = make_server([SECRET, "another-secret"]) + self.assert_warns_at_caller(record, "secrets") + assert all(isinstance(secret, Secret) for secret in server.secrets) + + def test_no_warning_for_secrets(self, monkeypatch): + monkeypatch.setattr(requests, "get", unreachable_get) + with warnings.catch_warnings(): + warnings.simplefilter("error") + with pytest.raises(requests.ConnectionError): + SourceCodeManager.make_proxy_client( + 1, 5, base_url="https://sentry.example", signing_secret=Secret(SECRET) + ) + with pytest.raises(requests.ConnectionError): + fetch_repository("https://sentry.example", Secret(SECRET), 1, 5, session=UnreachableSession) + make_api_client(Secret(SECRET)) + make_server([Secret(SECRET)]) From 4d55d68d8ca9fbf3af185aa48769aeced33a2f59 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 18:10:54 +0200 Subject: [PATCH 3/6] fix(rpc): Deprecate plain-string secrets in the signing helpers too The helpers in scm.rpc.helpers look internal, but Sentry's tests import sign_get and sign_post to sign requests with plain strings, so moving them would break those tests on upgrade. They warn the same way as the other entry points, so the next major release can require Secret everywhere. Co-Authored-By: Claude Opus 5.5 --- src/scm/rpc/helpers.py | 13 +++++++++++-- tests/unit/test_rpc_server.py | 4 ++-- tests/unit/test_secret.py | 25 ++++++++++++++++++++++++- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index 91e7512d..c04f8859 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -7,33 +7,42 @@ from scm.errors import RepositoryCouldNotBeDeserialized from scm.rpc.types import JsonApiData, JsonApiPayload, RepositoryAttributes -from scm.secret import Secret +from scm.secret import Secret, accept_secret, accept_secrets from scm.types import Repository, RepositoryId +# Each public function wraps its secrets first thing, so a plain string warns once, at the caller's line, and the +# functions it calls only ever see a Secret. + def sign_get(secret: str | Secret, organization_id: int, repository_id: RepositoryId) -> str: + secret = accept_secret(secret, parameter="secret") return sign(secret, f"{organization_id}{repository_id}".encode()) def sign_post(secret: str | Secret, serialized_body: bytes) -> str: + secret = accept_secret(secret, parameter="secret") return sign(secret, serialized_body) def sign(secret: str | Secret, message: bytes) -> str: - return f"rpc0:{hmac.new(Secret.of(secret).reveal().encode(), message, hashlib.sha256).hexdigest()}" + secret = accept_secret(secret, parameter="secret") + return f"rpc0:{hmac.new(secret.reveal().encode(), message, hashlib.sha256).hexdigest()}" def verify_get( secrets: Sequence[str | Secret], organization_id: int, repository_id: RepositoryId, received_signature: str ) -> bool: + secrets = accept_secrets(secrets, parameter="secrets") return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature) def verify_post(secrets: Sequence[str | Secret], serialized_body: bytes, received_signature: str) -> bool: + secrets = accept_secrets(secrets, parameter="secrets") return verify(secrets, serialized_body, received_signature) def verify(secrets: Sequence[str | Secret], message: bytes, received_signature: str) -> bool: + secrets = accept_secrets(secrets, parameter="secrets") return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets) diff --git a/tests/unit/test_rpc_server.py b/tests/unit/test_rpc_server.py index bf6399f8..0c0e8e54 100644 --- a/tests/unit/test_rpc_server.py +++ b/tests/unit/test_rpc_server.py @@ -11,7 +11,7 @@ from scm.test_fixtures import BaseTestProvider from scm.types import Repository -TEST_SECRET = "test-secret" +TEST_SECRET = Secret("test-secret") def make_repository(**overrides) -> Repository: @@ -35,7 +35,7 @@ def mock_record_count(a, b, c): def make_server(**overrides) -> RpcServer: defaults = dict( - secrets=[Secret(TEST_SECRET)], + secrets=[TEST_SECRET], fetch_repository=lambda org_id, repo_id: make_repository(), fetch_provider=lambda org_id, repo: BaseTestProvider(), record_count=mock_record_count, diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py index 4eaeb7cc..918a57f6 100644 --- a/tests/unit/test_secret.py +++ b/tests/unit/test_secret.py @@ -51,7 +51,9 @@ def test_of_wraps_a_string_once(self): assert Secret.of(SECRET).reveal() == SECRET def test_signing_accepts_either(self): - assert sign_get(Secret(SECRET), 1, 5) == sign_get(SECRET, 1, 5) + with pytest.warns(DeprecationWarning): + plain = sign_get(SECRET, 1, 5) + assert sign_get(Secret(SECRET), 1, 5) == plain # Plain strings, the worst case: wrapping on entry must still keep them out of this library's frames. @@ -163,6 +165,27 @@ def test_server_warns_once_for_several(self): self.assert_warns_at_caller(record, "secrets") assert all(isinstance(secret, Secret) for secret in server.secrets) + @pytest.mark.parametrize( + ("call", "parameter"), + [ + (lambda secret: helpers.sign_get(secret, 1, 5), "secret"), + (lambda secret: helpers.sign_post(secret, b"body"), "secret"), + (lambda secret: helpers.sign(secret, b"body"), "secret"), + (lambda secret: helpers.verify_get([secret], 1, 5, "rpc0:abc"), "secrets"), + (lambda secret: helpers.verify_post([secret], b"body", "rpc0:abc"), "secrets"), + (lambda secret: helpers.verify([secret], b"body", "rpc0:abc"), "secrets"), + ], + ids=["sign_get", "sign_post", "sign", "verify_get", "verify_post", "verify"], + ) + def test_signing_helpers(self, call, parameter): + with pytest.warns(DeprecationWarning) as record: + call(SECRET) + self.assert_warns_at_caller(record, parameter) + + with warnings.catch_warnings(): + warnings.simplefilter("error") + call(Secret(SECRET)) + def test_no_warning_for_secrets(self, monkeypatch): monkeypatch.setattr(requests, "get", unreachable_get) with warnings.catch_warnings(): From f1c07ffdd872b9880fb747d543e61ddaadf9d95d Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 18:27:32 +0200 Subject: [PATCH 4/6] fix(git): Keep grant secrets and grants out of error reports GitServer reports every refused grant through emit_error, and the SDK sends each frame's locals without looking inside lists or dicts. So a bad or expired grant put every signing secret in the event, through verify_grant's `secrets`, and any failure after the grant check put the grant itself in the event, through `headers` in GitServer.handle and in GitRelay.forward. The git API has not been released, so it takes Secret only rather than going through the plain-string deprecation the RPC API needs. A plain string gets a TypeError that says what to do, for untyped callers. Secret is falsy when empty, so the existing empty-secret checks still refuse one. handle takes the grant out of the headers on entry. The relay never sent it upstream, so it only loses a copy it had no use for. Co-Authored-By: Claude Opus 5.5 --- bin/git-server | 7 ++--- src/scm/git/grant.py | 17 +++++++----- src/scm/git/server.py | 28 ++++++++++++++----- src/scm/secret.py | 18 +++++++++++++ tests/unit/error_reports.py | 12 +++++++++ tests/unit/test_git_grant.py | 48 ++++++++++++++++++++++----------- tests/unit/test_git_server.py | 51 ++++++++++++++++++++++++++++++----- tests/unit/test_secret.py | 20 +++++--------- 8 files changed, 150 insertions(+), 51 deletions(-) create mode 100644 tests/unit/error_reports.py diff --git a/bin/git-server b/bin/git-server index efe23c2c..22962e17 100755 --- a/bin/git-server +++ b/bin/git-server @@ -91,6 +91,7 @@ from pathlib import Path from types import ModuleType from urllib.parse import urlsplit +from scm import Secret from scm.git.grant import sign_grant from scm.git.relay import GitRelay from scm.git.server import GitServer @@ -220,7 +221,7 @@ def _require(**values: str | None) -> None: def make_git_server( backend: Backend, - secret: str, + secret: Secret, ca_bundle: str | None, get_token: GetToken | None = None, repository: Repository | None = None, @@ -250,7 +251,7 @@ def make_git_server( ) -def grant_for(secret: str, operation: GitOperation, repository_id: RepositoryId) -> str: +def grant_for(secret: Secret, operation: GitOperation, repository_id: RepositoryId) -> str: grant = Grant( organization_id=ORGANIZATION_ID, referrer=REFERRER, @@ -564,7 +565,7 @@ def main() -> None: creds = load_credentials() args.repository = resolve("SCM_GIT_REPOSITORY", args.repository, creds) # The server prints grants for its repository, so nothing else needs to know a secret it makes up. - args.secret = resolve("SCM_GIT_GRANT_SECRET", args.secret, creds) or secrets.token_hex(32) + args.secret = Secret(resolve("SCM_GIT_GRANT_SECRET", args.secret, creds) or secrets.token_hex(32)) args.ca_bundle = resolve("SCM_GIT_CA_BUNDLE", args.ca_bundle, creds) _require(SCM_GIT_REPOSITORY=args.repository) path = args.repository.strip("/").removesuffix(".git") diff --git a/src/scm/git/grant.py b/src/scm/git/grant.py index 89586d24..a18adbff 100644 --- a/src/scm/git/grant.py +++ b/src/scm/git/grant.py @@ -9,6 +9,7 @@ import hashlib import time +from collections.abc import Sequence from typing import Any import jwt @@ -16,6 +17,7 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.types import GRANT_MAX_LENGTH, Grant +from scm.secret import Secret, require_secret GRANT_AUDIENCE = "scm-git-grant" _ALGORITHM = "HS256" @@ -23,7 +25,8 @@ _JWT_CLAIMS = frozenset({"aud", "exp", "iat"}) -def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: +def sign_grant(secret: Secret, grant: Grant, *, now: float | None = None) -> str: + require_secret(secret, parameter="secret") # HMAC accepts an empty key, so an empty secret, say from an unset setting, would let anyone sign a grant. if not secret: raise ValueError("A git grant secret is empty.") @@ -31,7 +34,7 @@ def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: claims["exp"] = claims.pop("expires_at") claims["iat"] = int(time.time() if now is None else now) claims["aud"] = GRANT_AUDIENCE - token = jwt.encode(claims, secret, algorithm=_ALGORITHM, headers={"kid": _key_id(secret)}) + token = jwt.encode(claims, secret.reveal(), algorithm=_ALGORITHM, headers={"kid": _key_id(secret)}) # Refused here, the signer's tests catch a grant that is too long, rather than the sandbox router in production. if len(token) > GRANT_MAX_LENGTH: raise ValueError( @@ -41,12 +44,14 @@ def sign_grant(secret: str, grant: Grant, *, now: float | None = None) -> str: return token -def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> Grant: +def verify_grant(secrets: Sequence[Secret], token: str, *, now: float | None = None) -> Grant: """Return the grant if one of ``secrets`` signed it and it has not expired. Verification takes a list of secrets, as ``RpcServer`` does, so a new secret can be added before signers switch to it and the old one removed after their grants expire. """ + for candidate in secrets: + require_secret(candidate, parameter="secrets") # As in sign_grant: a grant signed with an empty secret proves nothing. if not all(secrets): raise ValueError("A git grant secret is empty.") @@ -64,7 +69,7 @@ def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> try: claims = jwt.decode( token, - secret, + secret.reveal(), algorithms=[_ALGORITHM], audience=GRANT_AUDIENCE, # exp is checked below against an injectable clock, so tests can pin it. PyJWT checks iat, which it @@ -97,6 +102,6 @@ def verify_grant(secrets: list[str], token: str, *, now: float | None = None) -> return grant -def _key_id(secret: str) -> str: +def _key_id(secret: Secret) -> str: # The first 8 hex characters of the secret's SHA-256, the same key id Sentry's viewer contexts use. - return hashlib.sha256(secret.encode()).hexdigest()[:8] + return hashlib.sha256(secret.reveal().encode()).hexdigest()[:8] diff --git a/src/scm/git/server.py b/src/scm/git/server.py index ba7a851c..f2d0acc9 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -1,14 +1,13 @@ -from collections.abc import Callable, Mapping +from collections.abc import Callable, Mapping, Sequence from urllib.parse import urlsplit -from requests.structures import CaseInsensitiveDict - from scm.errors import ErrorCode, GitProviderNotSupported, GitTokenRejected, GitTokenUnavailable, SCMCodedError from scm.git.grant import verify_grant from scm.git.relay import GitRelay, ResponseBody, authorize_request from scm.git.types import GRANT_HEADER, GetToken, Readable from scm.helpers import initialize_provider from scm.rpc.types import StreamResponse +from scm.secret import Secret, require_secret from scm.types import GetGitRemoteProtocol, Provider, Repository, RepositoryId # The status of each refusal GitServer answers itself. None is a 401: on a 401 git asks the user for credentials, @@ -75,7 +74,7 @@ class GitServer: def __init__( self, - secrets: list[str], + secrets: Sequence[Secret], fetch_repository: Callable[[int, RepositoryId], Repository | None], fetch_provider: Callable[[int, Repository], Provider | None], get_token: GetToken, @@ -83,10 +82,12 @@ def __init__( emit_error: Callable[[Exception, dict[str, str]], None], relay: GitRelay, ) -> None: + for candidate in secrets: + require_secret(candidate, parameter="secrets") # Fail when the server is built rather than on every request, because no grant could ever verify. if not secrets or not all(secrets): raise ValueError("GitServer needs at least one secret, and none may be empty.") - self.secrets = secrets + self.secrets = list(secrets) self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.get_token = get_token @@ -112,11 +113,14 @@ def handle( body as empty, and git sends a push over about 1 MiB chunked, so a Django view needs to pass a stream that has the whole body. """ + # Rebound without the grant, so it shows as [Filtered] in the frames of a reported error rather than in + # ``headers``, here and in the relay, which never sends it upstream anyway. + grant_token, headers = _take_grant(headers) counted_body = _CountingReader(body) if body is not None else None context = {"repository_id": str(repository_id)} referrer_tag: dict[str, str] = {} try: - grant = verify_grant(self.secrets, CaseInsensitiveDict(headers).get(GRANT_HEADER, "")) + grant = verify_grant(self.secrets, grant_token.reveal()) context["organization_id"] = str(grant.organization_id) referrer_tag["referrer"] = context["referrer"] = grant.referrer operation = authorize_request(grant, repository_id, method, route, query) @@ -181,6 +185,18 @@ def finish(failure: str | None, bytes_sent: int) -> None: ) +def _take_grant(headers: Mapping[str, str]) -> tuple[Secret, dict[str, str]]: + """Split the grant, empty if there is none, from the other headers. Header names are case-insensitive.""" + grant = Secret("") + rest: dict[str, str] = {} + for name, value in headers.items(): + if name.lower() == GRANT_HEADER.lower(): + grant = Secret(value) + else: + rest[name] = value + return grant, rest + + class _CountingReader: """The client's request body, counting the bytes the relay reads from it.""" diff --git a/src/scm/secret.py b/src/scm/secret.py index d86bd35b..5cdb98e0 100644 --- a/src/scm/secret.py +++ b/src/scm/secret.py @@ -36,6 +36,24 @@ def __str__(self) -> str: def __sentry_repr__(self) -> str: return "[Filtered]" + def __bool__(self) -> bool: + # False for an empty secret, as for an empty string, so a check such as ``if not secret`` still refuses one. + return bool(self._value) + + +def require_secret(value: object, *, parameter: str) -> Secret: + """Return ``value`` if it is a ``Secret``, for an entry point that never took plain strings. + + Type checkers catch a plain string already; this tells an untyped caller what to do instead of failing later + with an AttributeError. + """ + if not isinstance(value, Secret): + raise TypeError( + f"{parameter} must be a scm.Secret, not {type(value).__name__}. " + 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_GIT_GRANT_SECRET"]).' + ) + return value + def accept_secret(value: str | Secret, *, parameter: str) -> Secret: """Wrap a secret given to a public entry point, warning if it came as a plain string. diff --git a/tests/unit/error_reports.py b/tests/unit/error_reports.py new file mode 100644 index 00000000..1bb905ed --- /dev/null +++ b/tests/unit/error_reports.py @@ -0,0 +1,12 @@ +def assert_not_in_frames(error: BaseException, value: str) -> None: + """Fail if any frame of the error's traceback holds ``value`` where an error report would show it. + + Sentry's SDK sends every frame's local variables, using ``__sentry_repr__`` or else ``repr()`` for each, and its + default scrubbing only replaces variables with a few exact names, such as ``secret``. + """ + 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}" + tb = tb.tb_next diff --git a/tests/unit/test_git_grant.py b/tests/unit/test_git_grant.py index cea9fd3d..dcbc1482 100644 --- a/tests/unit/test_git_grant.py +++ b/tests/unit/test_git_grant.py @@ -9,18 +9,20 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.grant import GRANT_AUDIENCE, sign_grant, verify_grant from scm.git.types import GRANT_MAX_LENGTH, Grant +from scm.secret import Secret from tests.unit.git_builders import FAR_FUTURE, make_grant # Before the real time: PyJWT checks iat against the real clock, so a grant must not be issued after it. NOW = 1_700_000_000 # HS256 secrets of at least 32 bytes, as PyJWT recommends. -SECRET = "test-secret-0000000000000000000000" -OLD_SECRET = "test-secret-old-000000000000000000" -NEW_SECRET = "test-secret-new-000000000000000000" +SECRET = Secret("test-secret-0000000000000000000000") +OLD_SECRET = Secret("test-secret-old-000000000000000000") +NEW_SECRET = Secret("test-secret-new-000000000000000000") +OTHER_SECRET = Secret("other-secret-0000000000000000000000") -def key_id(secret: str) -> str: - return hashlib.sha256(secret.encode()).hexdigest()[:8] +def key_id(secret: Secret) -> str: + return hashlib.sha256(secret.reveal().encode()).hexdigest()[:8] def claims_of(grant: Grant) -> dict: @@ -29,12 +31,12 @@ def claims_of(grant: Grant) -> dict: return claims | {"iat": NOW, "aud": GRANT_AUDIENCE} -def sign_claims(claims: dict, secret: str = SECRET, algorithm: str = "HS256", kid: str | None = None) -> str: +def sign_claims(claims: dict, secret: Secret = SECRET, algorithm: str = "HS256", kid: str | None = None) -> str: """Sign arbitrary claims the way sign_grant does, to reach the checks that run after the signature.""" - return jwt.encode(claims, secret, algorithm=algorithm, headers={"kid": kid or key_id(secret)}) + return jwt.encode(claims, secret.reveal(), algorithm=algorithm, headers={"kid": kid or key_id(secret)}) -def assert_invalid(token: str, detail: str, secrets: list[str] | None = None) -> None: +def assert_invalid(token: str, detail: str, secrets: list[Secret] | None = None) -> None: with pytest.raises(GitInvalidGrant) as exc_info: verify_grant(secrets or [SECRET], token, now=NOW) assert detail in exc_info.value.detail @@ -75,7 +77,7 @@ def test_expires_at_its_expiry_time(self): def test_forged_grant_is_invalid_even_when_expired(self): # Expiry is checked only once the signature matches, so an expired grant is always a genuine one. - token = sign_grant("other-secret-0000000000000000000000", make_grant(expires_at=NOW - 1), now=NOW) + token = sign_grant(OTHER_SECRET, make_grant(expires_at=NOW - 1), now=NOW) assert_invalid(token, "not signed with a known secret") def test_valid_until_its_expiry_time(self): @@ -118,12 +120,10 @@ def test_changed_signature(self): assert_invalid(f"{header}.{payload}.{resigned}", "signature does not match") def test_unknown_secret(self): - assert_invalid( - sign_grant("other-secret-0000000000000000000000", make_grant(), now=NOW), "not signed with a known" - ) + assert_invalid(sign_grant(OTHER_SECRET, make_grant(), now=NOW), "not signed with a known") def test_other_secret_claiming_a_known_key_id(self): - token = sign_claims(claims_of(make_grant()), secret="other-secret-0000000000000000000000", kid=key_id(SECRET)) + token = sign_claims(claims_of(make_grant()), secret=OTHER_SECRET, kid=key_id(SECRET)) assert_invalid(token, "signature does not match") def test_no_secrets(self): @@ -179,13 +179,29 @@ class TestSecrets: # PyJWT signs with an empty key, so an empty secret would let anyone sign grants. def test_refuses_to_sign_with_an_empty_secret(self): with pytest.raises(ValueError, match="empty"): - sign_grant("", make_grant(), now=NOW) + sign_grant(Secret(""), make_grant(), now=NOW) @pytest.mark.filterwarnings("ignore::jwt.warnings.InsecureKeyLengthWarning") def test_refuses_to_verify_with_an_empty_secret(self): - token = jwt.encode(claims_of(make_grant()), "", algorithm="HS256", headers={"kid": key_id("")}) + token = jwt.encode(claims_of(make_grant()), "", algorithm="HS256", headers={"kid": key_id(Secret(""))}) with pytest.raises(ValueError, match="empty"): - verify_grant([SECRET, ""], token, now=NOW) + verify_grant([SECRET, Secret("")], token, now=NOW) + + +class TestRequiresSecret: + def test_sign_refuses_a_plain_string(self): + with pytest.raises(TypeError, match=r"secret must be a scm\.Secret, not str"): + sign_grant(SECRET.reveal(), make_grant(), now=NOW) # type: ignore[arg-type] + + def test_verify_refuses_a_plain_string(self): + token = sign_grant(SECRET, make_grant(), now=NOW) + with pytest.raises(TypeError, match=r"secrets must be a scm\.Secret, not str"): + verify_grant([SECRET, SECRET.reveal()], token, now=NOW) # type: ignore[list-item] + + def test_message_leaves_out_the_value(self): + with pytest.raises(TypeError) as exc_info: + sign_grant(SECRET.reveal(), make_grant(), now=NOW) # type: ignore[arg-type] + assert SECRET.reveal() not in str(exc_info.value) class TestRotatedSecrets: diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 332221cf..3c65f955 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -13,11 +13,13 @@ from scm.git.server import _STATUS, GitServer from scm.git.types import GRANT_HEADER, GitOperation, Readable from scm.rpc.types import StreamResponse +from scm.secret import Secret from scm.types import GitRemote, Provider, Repository, RepositoryId +from tests.unit.error_reports import assert_not_in_frames from tests.unit.git_builders import make_grant, make_provider, make_repository from tests.unit.git_upstream import ClosingAdapter, basic_auth, running_upstream -SECRET = "test-secret-0000000000000000000000" +SECRET = Secret("test-secret-0000000000000000000000") TOKEN = "ghs_secrettoken" REFERRER = make_grant().referrer @@ -138,7 +140,7 @@ def test_requires_a_relay(): ) -@pytest.mark.parametrize("secrets", [[], [SECRET, ""]]) +@pytest.mark.parametrize("secrets", [[], [SECRET, Secret("")]]) def test_requires_non_empty_secrets(secrets): with pytest.raises(ValueError, match="secret"): GitServer( @@ -152,6 +154,19 @@ def test_requires_non_empty_secrets(secrets): ) +def test_requires_secret_objects(): + with pytest.raises(TypeError, match=r"secrets must be a scm\.Secret, not str"): + GitServer( + secrets=[SECRET.reveal()], # type: ignore[list-item] + fetch_repository=lambda organization_id, repository_id: None, + fetch_provider=lambda organization_id, repository: None, + get_token=lambda repository, operation: None, + record_count=lambda name, value, tags: None, + emit_error=lambda error, context: None, + relay=MagicMock(spec=GitRelay), + ) + + class TestRefusedBeforeTheProvider: """Nothing the grant does not allow may look up a repository or a token.""" @@ -161,7 +176,7 @@ class TestRefusedBeforeTheProvider: (None, "missing", "git_invalid_grant"), ("garbage", "malformed", "git_invalid_grant"), ( - sign_grant("other-secret-0000000000000000000000", make_grant()), + sign_grant(Secret("other-secret-0000000000000000000000"), make_grant()), "not signed with a known secret", "git_invalid_grant", ), @@ -272,8 +287,7 @@ def test_token_asked_for_the_grants_operation(self): def test_forwards_with_remote_and_token(self): recorder = Recorder() server = make_server(recorder) - grant = signed_grant() - response = request(server, grant, headers={"Git-Protocol": "version=2"}) + response = request(server, signed_grant(), headers={"Git-Protocol": "version=2"}) assert b"".join(response.content) == b"refs" server.relay.forward.assert_called_once_with( # type: ignore[attr-defined] GitRemote(url="https://github.com/getsentry/sentry.git", basic_auth_username="x-access-token"), @@ -281,7 +295,8 @@ def test_forwards_with_remote_and_token(self): method="GET", route="info/refs", query="service=git-upload-pack", - headers={GRANT_HEADER: grant, "Git-Protocol": "version=2"}, + # Without the grant, which the relay has no use for and would otherwise hold in its frame. + headers={"Git-Protocol": "version=2"}, body=None, ) assert recorder.counts == [ @@ -508,3 +523,27 @@ def test_relays_end_to_end(self): assert received.path == "/getsentry/sentry.git/info/refs?service=git-upload-pack" assert received.headers["Authorization"] == basic_auth("x-access-token", TOKEN) assert GRANT_HEADER not in received.headers + + +class TestKeptOutOfErrorReports: + """What GitServer passes to emit_error is reported to Sentry, with every frame's local variables.""" + + def test_refused_grant_leaves_out_the_signing_secret(self): + recorder = Recorder() + request(make_server(recorder), sign_grant(Secret("other-secret-0000000000000000000000"), make_grant())) + [(error, _)] = recorder.errors + assert error.code == "git_invalid_grant" # type: ignore[attr-defined] + assert_not_in_frames(error, SECRET.reveal()) + + @pytest.mark.parametrize("header", [GRANT_HEADER, GRANT_HEADER.lower()]) + def test_relay_failure_leaves_out_the_grant_and_the_signing_secret(self, header): + recorder = Recorder( + repository=make_repository(provider_name="github_enterprise", web_base_url="https://127.0.0.1:1") + ) + grant = signed_grant() + request(make_server(recorder, GitRelay(timeout=(1, 1))), None, headers={header: grant}) + [(error, _)] = recorder.errors + # Failed after the grant check, in the relay. + assert error.code != "git_invalid_grant" # type: ignore[attr-defined] + assert_not_in_frames(error, grant) + assert_not_in_frames(error, SECRET.reveal()) diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py index 918a57f6..0aa7b44c 100644 --- a/tests/unit/test_secret.py +++ b/tests/unit/test_secret.py @@ -10,24 +10,11 @@ from scm.rpc.helpers import sign_get from scm.rpc.server import RpcServer from scm.secret import Secret +from tests.unit.error_reports import assert_not_in_frames SECRET = "rpc-signing-secret-0000000000000000" -def assert_not_in_frames(error: BaseException, value: str) -> None: - """Fail if any frame of the error's traceback holds ``value`` where an error report would show it. - - Sentry's SDK sends every frame's local variables, using ``__sentry_repr__`` or else ``repr()`` for each, and its - default scrubbing only replaces variables with a few exact names, such as ``secret``. - """ - 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}" - tb = tb.tb_next - - class TestSecret: def test_reveal(self): assert Secret(SECRET).reveal() == SECRET @@ -45,6 +32,11 @@ def test_has_no_attributes_to_look_into(self): # With __slots__ there is no __dict__ for a serializer to walk. assert not hasattr(Secret(SECRET), "__dict__") + def test_false_when_empty(self): + # So an empty-secret check such as ``if not secret`` works on a Secret as on a string. + assert not Secret("") + assert Secret(SECRET) + def test_of_wraps_a_string_once(self): secret = Secret(SECRET) assert Secret.of(secret) is secret From fd5e30ce5db949de4f96f19f862c8e09ec3ab502 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 21:26:15 +0200 Subject: [PATCH 5/6] fix(rpc): Require Secret for RPC signing secrets A plain string stays in the caller's own frame, where an error report shows it, so wrapping it on entry only ever protected this library's frames. Requiring Secret moves the wrapping to where the caller reads the secret, as the git API already does, and one rule for both is simpler than a deprecation period. This breaks Sentry and Seer, which pass strings today, until they wrap theirs; their changes and the major release are coordinated separately. Secret.of goes too, since nothing coerces strings any more. Co-Authored-By: Claude Opus 5.5 --- src/scm/git/grant.py | 5 +- src/scm/git/server.py | 5 +- src/scm/manager.py | 8 +-- src/scm/rpc/client.py | 11 ++-- src/scm/rpc/helpers.py | 25 +++------ src/scm/rpc/server.py | 9 ++- src/scm/secret.py | 41 ++------------ tests/unit/test_secret.py | 113 ++++++++++++-------------------------- 8 files changed, 67 insertions(+), 150 deletions(-) diff --git a/src/scm/git/grant.py b/src/scm/git/grant.py index a18adbff..569a6116 100644 --- a/src/scm/git/grant.py +++ b/src/scm/git/grant.py @@ -17,7 +17,7 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.types import GRANT_MAX_LENGTH, Grant -from scm.secret import Secret, require_secret +from scm.secret import Secret, require_secret, require_secrets GRANT_AUDIENCE = "scm-git-grant" _ALGORITHM = "HS256" @@ -50,8 +50,7 @@ def verify_grant(secrets: Sequence[Secret], token: str, *, now: float | None = N Verification takes a list of secrets, as ``RpcServer`` does, so a new secret can be added before signers switch to it and the old one removed after their grants expire. """ - for candidate in secrets: - require_secret(candidate, parameter="secrets") + require_secrets(secrets, parameter="secrets") # As in sign_grant: a grant signed with an empty secret proves nothing. if not all(secrets): raise ValueError("A git grant secret is empty.") diff --git a/src/scm/git/server.py b/src/scm/git/server.py index f2d0acc9..744f2ac6 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -7,7 +7,7 @@ from scm.git.types import GRANT_HEADER, GetToken, Readable from scm.helpers import initialize_provider from scm.rpc.types import StreamResponse -from scm.secret import Secret, require_secret +from scm.secret import Secret, require_secrets from scm.types import GetGitRemoteProtocol, Provider, Repository, RepositoryId # The status of each refusal GitServer answers itself. None is a 401: on a 401 git asks the user for credentials, @@ -82,8 +82,7 @@ def __init__( emit_error: Callable[[Exception, dict[str, str]], None], relay: GitRelay, ) -> None: - for candidate in secrets: - require_secret(candidate, parameter="secrets") + require_secrets(secrets, parameter="secrets") # Fail when the server is built rather than on every request, because no grant could ever verify. if not secrets or not all(secrets): raise ValueError("GitServer needs at least one secret, and none may be empty.") diff --git a/src/scm/manager.py b/src/scm/manager.py index 1e41f6b6..af85f571 100644 --- a/src/scm/manager.py +++ b/src/scm/manager.py @@ -10,7 +10,7 @@ ) from scm.rpc.client import fetch_provider as fetch_proxy_provider from scm.rpc.client import fetch_repository as fetch_proxy_repository -from scm.secret import Secret, accept_secret +from scm.secret import Secret, require_secret from scm.types import Provider, Referrer, Repository, RepositoryId @@ -45,13 +45,11 @@ def make_proxy_client( *, referrer: Referrer = "shared", base_url: str, - signing_secret: str | Secret, + signing_secret: Secret, record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, retry: RetryConfig | None = None, ): - # Rebound, so the plain string is not in this frame, or in the lambda below, when fetching the repository - # fails and the error is reported. - signing_secret = accept_secret(signing_secret, parameter="signing_secret") + require_secret(signing_secret, parameter="signing_secret") full_url = SCM_API_URL.format(base_url=base_url) # A specialized RpcApiClient is initialized. It will proxy the service-provider requests through Sentry. This diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index 60026239..2fd99fdd 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -17,7 +17,7 @@ from scm.providers.gitlab.provider import GitLabProvider from scm.rpc.helpers import deserialize_repository, sign_get, sign_post from scm.rpc.types import ActionAttributes, ActionRequest, ErrorResponse -from scm.secret import Secret, accept_secret +from scm.secret import Secret, require_secret from scm.types import ApiClient, CredentialsSet, Provider, Repository, RepositoryId SCM_API_URL = "{base_url}/api/0/internal/scm-rpc/" @@ -74,14 +74,13 @@ def post(self, url: str, data: bytes, headers: dict[str, str], stream: bool = Fa def fetch_repository( url: str, - signing_secret: str | Secret, + signing_secret: Secret, organization_id: int, repository_id: RepositoryId, session: Callable[[], Session] = lambda: RequestsSession(), ) -> Repository: """Fetch repositorty metadata.""" - # Rebound, so the plain string is not in this frame when the request fails and the error is reported. - signing_secret = accept_secret(signing_secret, parameter="signing_secret") + require_secret(signing_secret, parameter="signing_secret") response = session().get( url, headers={ @@ -157,7 +156,7 @@ class RpcApiClient(ApiClient): def __init__( self, full_url: str, - signing_secret: str | Secret, + signing_secret: Secret, organization_id: int, referrer: str, repository_id: RepositoryId, @@ -166,7 +165,7 @@ def __init__( record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, ) -> None: self.full_url = full_url - self.signing_secret = accept_secret(signing_secret, parameter="signing_secret") + self.signing_secret = require_secret(signing_secret, parameter="signing_secret") self.organization_id = organization_id self.referrer = referrer self.repository_id = repository_id diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index 0153d5dc..f9054d17 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -7,42 +7,35 @@ from scm.errors import RepositoryCouldNotBeDeserialized from scm.rpc.types import JsonApiData, JsonApiPayload, RepositoryAttributes -from scm.secret import Secret, accept_secret, accept_secrets +from scm.secret import Secret, require_secret, require_secrets from scm.types import Repository, RepositoryId -# Each public function wraps its secrets first thing, so a plain string warns once, at the caller's line, and the -# functions it calls only ever see a Secret. - -def sign_get(secret: str | Secret, organization_id: int, repository_id: RepositoryId) -> str: - secret = accept_secret(secret, parameter="secret") +def sign_get(secret: Secret, organization_id: int, repository_id: RepositoryId) -> str: return sign(secret, f"{organization_id}{repository_id}".encode()) -def sign_post(secret: str | Secret, serialized_body: bytes) -> str: - secret = accept_secret(secret, parameter="secret") +def sign_post(secret: Secret, serialized_body: bytes) -> str: return sign(secret, serialized_body) -def sign(secret: str | Secret, message: bytes) -> str: - secret = accept_secret(secret, parameter="secret") +def sign(secret: Secret, message: bytes) -> str: + require_secret(secret, parameter="secret") return f"rpc0:{hmac.new(secret.reveal().encode(), message, hashlib.sha256).hexdigest()}" def verify_get( - secrets: Sequence[str | Secret], organization_id: int, repository_id: RepositoryId, received_signature: str + secrets: Sequence[Secret], organization_id: int, repository_id: RepositoryId, received_signature: str ) -> bool: - secrets = accept_secrets(secrets, parameter="secrets") return verify(secrets, f"{organization_id}{repository_id}".encode(), received_signature) -def verify_post(secrets: Sequence[str | Secret], serialized_body: bytes, received_signature: str) -> bool: - secrets = accept_secrets(secrets, parameter="secrets") +def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_signature: str) -> bool: return verify(secrets, serialized_body, received_signature) -def verify(secrets: Sequence[str | Secret], message: bytes, received_signature: str) -> bool: - secrets = accept_secrets(secrets, parameter="secrets") +def verify(secrets: Sequence[Secret], message: bytes, received_signature: str) -> bool: + require_secrets(secrets, parameter="secrets") return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets) diff --git a/src/scm/rpc/server.py b/src/scm/rpc/server.py index c2903c0f..38168ba1 100644 --- a/src/scm/rpc/server.py +++ b/src/scm/rpc/server.py @@ -1,4 +1,4 @@ -from collections.abc import Callable, Iterator, Mapping, MutableMapping +from collections.abc import Callable, Iterator, Mapping, MutableMapping, Sequence from urllib.parse import urlparse import msgspec @@ -17,7 +17,7 @@ from scm.rpc.errors import serialize_error from scm.rpc.helpers import serialize_repository, verify_get, verify_post from scm.rpc.types import ActionRequest, Response, StreamResponse -from scm.secret import Secret, accept_secrets +from scm.secret import Secret, require_secrets from scm.types import Provider, Repository, RepositoryId TEN_MEGABYTES = 10 * 1024 * 1024 @@ -27,14 +27,13 @@ class RpcServer: def __init__( self, - secrets: list[str] | list[Secret], + secrets: Sequence[Secret], fetch_repository: Callable[[int, RepositoryId], Repository | None], fetch_provider: Callable[[int, Repository], Provider | None], record_count: Callable[[str, int, dict[str, str]], None], emit_error: Callable[[Exception], None], ) -> None: - # Wrapped, so the secrets show as [Filtered] in the frames of a reported error, e.g. while verifying. - self.secrets = accept_secrets(secrets, parameter="secrets") + self.secrets = require_secrets(secrets, parameter="secrets") self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.record_count = record_count diff --git a/src/scm/secret.py b/src/scm/secret.py index 5cdb98e0..3054bbe8 100644 --- a/src/scm/secret.py +++ b/src/scm/secret.py @@ -1,4 +1,3 @@ -import warnings from collections.abc import Sequence @@ -19,11 +18,6 @@ class Secret: def __init__(self, value: str) -> None: self._value = value - @classmethod - def of(cls, value: "str | Secret") -> "Secret": - """Wrap ``value``, unless it already is a ``Secret``.""" - return value if isinstance(value, Secret) else cls(value) - def reveal(self) -> str: return self._value @@ -42,43 +36,20 @@ def __bool__(self) -> bool: def require_secret(value: object, *, parameter: str) -> Secret: - """Return ``value`` if it is a ``Secret``, for an entry point that never took plain strings. + """Return ``value`` if it is a ``Secret``. Type checkers catch a plain string already; this tells an untyped caller what to do instead of failing later with an AttributeError. """ if not isinstance(value, Secret): + # A plain string would stay in the caller's frame, where an error report shows it. raise TypeError( f"{parameter} must be a scm.Secret, not {type(value).__name__}. " - 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_GIT_GRANT_SECRET"]).' + 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).' ) return value -def accept_secret(value: str | Secret, *, parameter: str) -> Secret: - """Wrap a secret given to a public entry point, warning if it came as a plain string. - - Call it first thing in the entry point, so the warning points at the entry point's caller. - """ - if isinstance(value, str): - _warn_plain_string(parameter) - return Secret.of(value) - - -def accept_secrets(values: Sequence[str | Secret], *, parameter: str) -> list[Secret]: - """Like ``accept_secret``, for a list of secrets, warning once however many are plain strings.""" - if any(isinstance(value, str) for value in values): - _warn_plain_string(parameter) - return [Secret.of(value) for value in values] - - -def _warn_plain_string(parameter: str) -> None: - # A plain string stays in the caller's frame, where an error report shows it, so callers should wrap it where they - # read it. Accepted until the next major release, so Sentry and Seer can move over first. - warnings.warn( - f"Passing {parameter} as a plain str is deprecated and will stop working in the next major release. " - 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).', - DeprecationWarning, - # This function, accept_secret(s), the entry point, then the entry point's caller. - stacklevel=4, - ) +def require_secrets(values: Sequence[object], *, parameter: str) -> list[Secret]: + """Like ``require_secret``, for each of ``values``.""" + return [require_secret(value, parameter=parameter) for value in values] diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py index 0aa7b44c..74237f3e 100644 --- a/tests/unit/test_secret.py +++ b/tests/unit/test_secret.py @@ -1,4 +1,5 @@ -import warnings +import hashlib +import hmac import pytest import requests @@ -37,19 +38,14 @@ def test_false_when_empty(self): assert not Secret("") assert Secret(SECRET) - def test_of_wraps_a_string_once(self): - secret = Secret(SECRET) - assert Secret.of(secret) is secret - assert Secret.of(SECRET).reveal() == SECRET + def test_exported_from_the_package(self): + assert ExportedSecret is Secret - def test_signing_accepts_either(self): - with pytest.warns(DeprecationWarning): - plain = sign_get(SECRET, 1, 5) - assert sign_get(Secret(SECRET), 1, 5) == plain + def test_signs_with_the_revealed_value(self): + assert sign_get(Secret(SECRET), 1, 5) == "rpc0:" + hmac.new(SECRET.encode(), b"15", hashlib.sha256).hexdigest() -# Plain strings, the worst case: wrapping on entry must still keep them out of this library's frames. -@pytest.mark.filterwarnings("ignore::DeprecationWarning") +# A revealed value kept in a variable, rather than used inline, would put the secret back in a frame. class TestKeptOutOfErrorReports: def test_proxy_client_that_cannot_reach_sentry(self, monkeypatch): def unreachable(*args, **kwargs): @@ -57,7 +53,7 @@ def unreachable(*args, **kwargs): monkeypatch.setattr(requests, "get", unreachable) with pytest.raises(requests.ConnectionError) as exc_info: - SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=SECRET) + SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=Secret(SECRET)) assert_not_in_frames(exc_info.value, SECRET) def test_api_client_that_cannot_reach_sentry(self): @@ -67,7 +63,7 @@ def post(self, url, data, headers, stream=False): client = RpcApiClient( full_url="https://sentry.example/api/0/internal/scm-rpc/", - signing_secret=SECRET, + signing_secret=Secret(SECRET), organization_id=1, referrer="shared", repository_id=5, @@ -83,7 +79,7 @@ def failing_compare(a, b): monkeypatch.setattr(helpers.hmac, "compare_digest", failing_compare) server = RpcServer( - secrets=[SECRET], + secrets=[Secret(SECRET)], fetch_repository=lambda organization_id, repository_id: None, fetch_provider=lambda organization_id, repository: None, record_count=lambda name, value, tags: None, @@ -95,15 +91,6 @@ def failing_compare(a, b): assert_not_in_frames(exc_info.value, SECRET) -def unreachable_get(*args, **kwargs): - raise requests.ConnectionError("Sentry is unreachable") - - -class UnreachableSession: - def get(self, url, headers): - raise requests.ConnectionError("Sentry is unreachable") - - def make_server(secrets): return RpcServer( secrets=secrets, @@ -124,42 +111,19 @@ def make_api_client(signing_secret): ) -class TestPlainStringDeprecated: - def assert_warns_at_caller(self, record: pytest.WarningsRecorder, parameter: str) -> None: - assert len(record) == 1 - assert f"Passing {parameter} as a plain str is deprecated" in str(record[0].message) - # Points at the code that passed the string, not at this library. - assert record[0].filename == __file__ - - def test_exported_from_the_package(self): - assert ExportedSecret is Secret - - def test_proxy_client(self, monkeypatch): - monkeypatch.setattr(requests, "get", unreachable_get) - with pytest.warns(DeprecationWarning) as record, pytest.raises(requests.ConnectionError): - SourceCodeManager.make_proxy_client(1, 5, base_url="https://sentry.example", signing_secret=SECRET) - self.assert_warns_at_caller(record, "signing_secret") - - def test_fetch_repository(self): - with pytest.warns(DeprecationWarning) as record, pytest.raises(requests.ConnectionError): - fetch_repository("https://sentry.example", SECRET, 1, 5, session=UnreachableSession) - self.assert_warns_at_caller(record, "signing_secret") - - def test_api_client(self): - with pytest.warns(DeprecationWarning) as record: - make_api_client(SECRET) - # make_api_client is in this file too, so the caller check still holds. - self.assert_warns_at_caller(record, "signing_secret") - - def test_server_warns_once_for_several(self): - with pytest.warns(DeprecationWarning) as record: - server = make_server([SECRET, "another-secret"]) - self.assert_warns_at_caller(record, "secrets") - assert all(isinstance(secret, Secret) for secret in server.secrets) - +class TestRequiresSecret: @pytest.mark.parametrize( ("call", "parameter"), [ + ( + lambda secret: SourceCodeManager.make_proxy_client( + 1, 5, base_url="https://sentry.example", signing_secret=secret + ), + "signing_secret", + ), + (lambda secret: fetch_repository("https://sentry.example", secret, 1, 5), "signing_secret"), + (make_api_client, "signing_secret"), + (lambda secret: make_server([Secret(SECRET), secret]), "secrets"), (lambda secret: helpers.sign_get(secret, 1, 5), "secret"), (lambda secret: helpers.sign_post(secret, b"body"), "secret"), (lambda secret: helpers.sign(secret, b"body"), "secret"), @@ -167,26 +131,21 @@ def test_server_warns_once_for_several(self): (lambda secret: helpers.verify_post([secret], b"body", "rpc0:abc"), "secrets"), (lambda secret: helpers.verify([secret], b"body", "rpc0:abc"), "secrets"), ], - ids=["sign_get", "sign_post", "sign", "verify_get", "verify_post", "verify"], + ids=[ + "make_proxy_client", + "fetch_repository", + "RpcApiClient", + "RpcServer", + "sign_get", + "sign_post", + "sign", + "verify_get", + "verify_post", + "verify", + ], ) - def test_signing_helpers(self, call, parameter): - with pytest.warns(DeprecationWarning) as record: + def test_refuses_a_plain_string(self, call, parameter): + # Refused before anything is sent, so no request is made. + with pytest.raises(TypeError, match=rf"^{parameter} must be a scm\.Secret, not str\.") as exc_info: call(SECRET) - self.assert_warns_at_caller(record, parameter) - - with warnings.catch_warnings(): - warnings.simplefilter("error") - call(Secret(SECRET)) - - def test_no_warning_for_secrets(self, monkeypatch): - monkeypatch.setattr(requests, "get", unreachable_get) - with warnings.catch_warnings(): - warnings.simplefilter("error") - with pytest.raises(requests.ConnectionError): - SourceCodeManager.make_proxy_client( - 1, 5, base_url="https://sentry.example", signing_secret=Secret(SECRET) - ) - with pytest.raises(requests.ConnectionError): - fetch_repository("https://sentry.example", Secret(SECRET), 1, 5, session=UnreachableSession) - make_api_client(Secret(SECRET)) - make_server([Secret(SECRET)]) + assert SECRET not in str(exc_info.value) From 26c44ad06bdee08ca1ebec0c28370b8e84e4f7a7 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Thu, 22 Oct 2026 21:38:36 +0200 Subject: [PATCH 6/6] refactor(secret): Check for Secret only in the server constructors mypy already refuses a plain string wherever a caller is type-checked, Sentry's settings included. The runtime check only matters where a string that slipped past it does harm: a server would hold it for every request, fail each one inside verification, and report the strings in its frames. Elsewhere a string just fails with an AttributeError on first use, so the checks there were noise. Co-Authored-By: Claude Opus 5.5 --- src/scm/git/grant.py | 4 +-- src/scm/git/server.py | 3 +- src/scm/manager.py | 3 +- src/scm/rpc/client.py | 5 ++-- src/scm/rpc/helpers.py | 4 +-- src/scm/secret.py | 30 +++++++++---------- tests/unit/test_git_grant.py | 16 ----------- tests/unit/test_git_server.py | 2 +- tests/unit/test_secret.py | 54 ++++------------------------------- 9 files changed, 27 insertions(+), 94 deletions(-) diff --git a/src/scm/git/grant.py b/src/scm/git/grant.py index 569a6116..20b54707 100644 --- a/src/scm/git/grant.py +++ b/src/scm/git/grant.py @@ -17,7 +17,7 @@ from scm.errors import GitGrantExpired, GitInvalidGrant from scm.git.types import GRANT_MAX_LENGTH, Grant -from scm.secret import Secret, require_secret, require_secrets +from scm.secret import Secret GRANT_AUDIENCE = "scm-git-grant" _ALGORITHM = "HS256" @@ -26,7 +26,6 @@ def sign_grant(secret: Secret, grant: Grant, *, now: float | None = None) -> str: - require_secret(secret, parameter="secret") # HMAC accepts an empty key, so an empty secret, say from an unset setting, would let anyone sign a grant. if not secret: raise ValueError("A git grant secret is empty.") @@ -50,7 +49,6 @@ def verify_grant(secrets: Sequence[Secret], token: str, *, now: float | None = N Verification takes a list of secrets, as ``RpcServer`` does, so a new secret can be added before signers switch to it and the old one removed after their grants expire. """ - require_secrets(secrets, parameter="secrets") # As in sign_grant: a grant signed with an empty secret proves nothing. if not all(secrets): raise ValueError("A git grant secret is empty.") diff --git a/src/scm/git/server.py b/src/scm/git/server.py index 744f2ac6..c8bdd0ca 100644 --- a/src/scm/git/server.py +++ b/src/scm/git/server.py @@ -82,11 +82,10 @@ def __init__( emit_error: Callable[[Exception, dict[str, str]], None], relay: GitRelay, ) -> None: - require_secrets(secrets, parameter="secrets") # Fail when the server is built rather than on every request, because no grant could ever verify. if not secrets or not all(secrets): raise ValueError("GitServer needs at least one secret, and none may be empty.") - self.secrets = list(secrets) + self.secrets = require_secrets(secrets, parameter="secrets") self.fetch_repository = fetch_repository self.fetch_provider = fetch_provider self.get_token = get_token diff --git a/src/scm/manager.py b/src/scm/manager.py index af85f571..014550cf 100644 --- a/src/scm/manager.py +++ b/src/scm/manager.py @@ -10,7 +10,7 @@ ) from scm.rpc.client import fetch_provider as fetch_proxy_provider from scm.rpc.client import fetch_repository as fetch_proxy_repository -from scm.secret import Secret, require_secret +from scm.secret import Secret from scm.types import Provider, Referrer, Repository, RepositoryId @@ -49,7 +49,6 @@ def make_proxy_client( record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, retry: RetryConfig | None = None, ): - require_secret(signing_secret, parameter="signing_secret") full_url = SCM_API_URL.format(base_url=base_url) # A specialized RpcApiClient is initialized. It will proxy the service-provider requests through Sentry. This diff --git a/src/scm/rpc/client.py b/src/scm/rpc/client.py index 2fd99fdd..f67b9b55 100644 --- a/src/scm/rpc/client.py +++ b/src/scm/rpc/client.py @@ -17,7 +17,7 @@ from scm.providers.gitlab.provider import GitLabProvider from scm.rpc.helpers import deserialize_repository, sign_get, sign_post from scm.rpc.types import ActionAttributes, ActionRequest, ErrorResponse -from scm.secret import Secret, require_secret +from scm.secret import Secret from scm.types import ApiClient, CredentialsSet, Provider, Repository, RepositoryId SCM_API_URL = "{base_url}/api/0/internal/scm-rpc/" @@ -80,7 +80,6 @@ def fetch_repository( session: Callable[[], Session] = lambda: RequestsSession(), ) -> Repository: """Fetch repositorty metadata.""" - require_secret(signing_secret, parameter="signing_secret") response = session().get( url, headers={ @@ -165,7 +164,7 @@ def __init__( record_count: Callable[[str, int, dict[str, str]], None] = lambda name, value, tags: None, ) -> None: self.full_url = full_url - self.signing_secret = require_secret(signing_secret, parameter="signing_secret") + self.signing_secret = signing_secret self.organization_id = organization_id self.referrer = referrer self.repository_id = repository_id diff --git a/src/scm/rpc/helpers.py b/src/scm/rpc/helpers.py index f9054d17..c91793fb 100644 --- a/src/scm/rpc/helpers.py +++ b/src/scm/rpc/helpers.py @@ -7,7 +7,7 @@ from scm.errors import RepositoryCouldNotBeDeserialized from scm.rpc.types import JsonApiData, JsonApiPayload, RepositoryAttributes -from scm.secret import Secret, require_secret, require_secrets +from scm.secret import Secret from scm.types import Repository, RepositoryId @@ -20,7 +20,6 @@ def sign_post(secret: Secret, serialized_body: bytes) -> str: def sign(secret: Secret, message: bytes) -> str: - require_secret(secret, parameter="secret") return f"rpc0:{hmac.new(secret.reveal().encode(), message, hashlib.sha256).hexdigest()}" @@ -35,7 +34,6 @@ def verify_post(secrets: Sequence[Secret], serialized_body: bytes, received_sign def verify(secrets: Sequence[Secret], message: bytes, received_signature: str) -> bool: - require_secrets(secrets, parameter="secrets") return any(hmac.compare_digest(received_signature, sign(secret, message)) for secret in secrets) diff --git a/src/scm/secret.py b/src/scm/secret.py index 3054bbe8..5ce9fb9d 100644 --- a/src/scm/secret.py +++ b/src/scm/secret.py @@ -35,21 +35,19 @@ def __bool__(self) -> bool: return bool(self._value) -def require_secret(value: object, *, parameter: str) -> Secret: - """Return ``value`` if it is a ``Secret``. +def require_secrets(values: Sequence[object], *, parameter: str) -> list[Secret]: + """Return ``values`` as a list, if each is a ``Secret``. - Type checkers catch a plain string already; this tells an untyped caller what to do instead of failing later - with an AttributeError. + For the servers, which hold their secrets for every request. A plain string that got past the type checker, say + from untyped code, would make every request fail inside verification, and each failure's error report would + carry the strings in its frames. Refused here, the server fails once, when it is built. """ - if not isinstance(value, Secret): - # A plain string would stay in the caller's frame, where an error report shows it. - raise TypeError( - f"{parameter} must be a scm.Secret, not {type(value).__name__}. " - 'Wrap it in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).' - ) - return value - - -def require_secrets(values: Sequence[object], *, parameter: str) -> list[Secret]: - """Like ``require_secret``, for each of ``values``.""" - return [require_secret(value, parameter=parameter) for value in values] + secrets: list[Secret] = [] + for value in values: + if not isinstance(value, Secret): + raise TypeError( + f"{parameter} must be scm.Secret objects, not {type(value).__name__}. " + 'Wrap each in scm.Secret where you read it, e.g. Secret(os.environ["SCM_RPC_SHARED_SECRET"]).' + ) + secrets.append(value) + return secrets diff --git a/tests/unit/test_git_grant.py b/tests/unit/test_git_grant.py index dcbc1482..db308f0e 100644 --- a/tests/unit/test_git_grant.py +++ b/tests/unit/test_git_grant.py @@ -188,22 +188,6 @@ def test_refuses_to_verify_with_an_empty_secret(self): verify_grant([SECRET, Secret("")], token, now=NOW) -class TestRequiresSecret: - def test_sign_refuses_a_plain_string(self): - with pytest.raises(TypeError, match=r"secret must be a scm\.Secret, not str"): - sign_grant(SECRET.reveal(), make_grant(), now=NOW) # type: ignore[arg-type] - - def test_verify_refuses_a_plain_string(self): - token = sign_grant(SECRET, make_grant(), now=NOW) - with pytest.raises(TypeError, match=r"secrets must be a scm\.Secret, not str"): - verify_grant([SECRET, SECRET.reveal()], token, now=NOW) # type: ignore[list-item] - - def test_message_leaves_out_the_value(self): - with pytest.raises(TypeError) as exc_info: - sign_grant(SECRET.reveal(), make_grant(), now=NOW) # type: ignore[arg-type] - assert SECRET.reveal() not in str(exc_info.value) - - class TestRotatedSecrets: @pytest.mark.parametrize("signer", [OLD_SECRET, NEW_SECRET]) def test_both_secrets_verify_during_rotation(self, signer): diff --git a/tests/unit/test_git_server.py b/tests/unit/test_git_server.py index 3c65f955..bdf67479 100644 --- a/tests/unit/test_git_server.py +++ b/tests/unit/test_git_server.py @@ -155,7 +155,7 @@ def test_requires_non_empty_secrets(secrets): def test_requires_secret_objects(): - with pytest.raises(TypeError, match=r"secrets must be a scm\.Secret, not str"): + with pytest.raises(TypeError, match=r"^secrets must be scm\.Secret objects, not str\."): GitServer( secrets=[SECRET.reveal()], # type: ignore[list-item] fetch_repository=lambda organization_id, repository_id: None, diff --git a/tests/unit/test_secret.py b/tests/unit/test_secret.py index 74237f3e..731769d2 100644 --- a/tests/unit/test_secret.py +++ b/tests/unit/test_secret.py @@ -7,7 +7,7 @@ from scm import Secret as ExportedSecret from scm.manager import SourceCodeManager from scm.rpc import helpers -from scm.rpc.client import RpcApiClient, fetch_repository +from scm.rpc.client import RpcApiClient from scm.rpc.helpers import sign_get from scm.rpc.server import RpcServer from scm.secret import Secret @@ -101,51 +101,9 @@ def make_server(secrets): ) -def make_api_client(signing_secret): - return RpcApiClient( - full_url="https://sentry.example/api/0/internal/scm-rpc/", - signing_secret=signing_secret, - organization_id=1, - referrer="shared", - repository_id=5, - ) - - -class TestRequiresSecret: - @pytest.mark.parametrize( - ("call", "parameter"), - [ - ( - lambda secret: SourceCodeManager.make_proxy_client( - 1, 5, base_url="https://sentry.example", signing_secret=secret - ), - "signing_secret", - ), - (lambda secret: fetch_repository("https://sentry.example", secret, 1, 5), "signing_secret"), - (make_api_client, "signing_secret"), - (lambda secret: make_server([Secret(SECRET), secret]), "secrets"), - (lambda secret: helpers.sign_get(secret, 1, 5), "secret"), - (lambda secret: helpers.sign_post(secret, b"body"), "secret"), - (lambda secret: helpers.sign(secret, b"body"), "secret"), - (lambda secret: helpers.verify_get([secret], 1, 5, "rpc0:abc"), "secrets"), - (lambda secret: helpers.verify_post([secret], b"body", "rpc0:abc"), "secrets"), - (lambda secret: helpers.verify([secret], b"body", "rpc0:abc"), "secrets"), - ], - ids=[ - "make_proxy_client", - "fetch_repository", - "RpcApiClient", - "RpcServer", - "sign_get", - "sign_post", - "sign", - "verify_get", - "verify_post", - "verify", - ], - ) - def test_refuses_a_plain_string(self, call, parameter): - # Refused before anything is sent, so no request is made. - with pytest.raises(TypeError, match=rf"^{parameter} must be a scm\.Secret, not str\.") as exc_info: - call(SECRET) +class TestServerRequiresSecret: + def test_refuses_a_plain_string(self): + # Built with one, the server would fail every request inside verification, with the strings in its frames. + with pytest.raises(TypeError, match=r"^secrets must be scm\.Secret objects, not str\.") as exc_info: + make_server([Secret(SECRET), SECRET]) assert SECRET not in str(exc_info.value)