diff --git a/src/chat_sdk/adapters/google_chat/adapter.py b/src/chat_sdk/adapters/google_chat/adapter.py index f12da78..99861be 100644 --- a/src/chat_sdk/adapters/google_chat/adapter.py +++ b/src/chat_sdk/adapters/google_chat/adapter.py @@ -155,6 +155,45 @@ def __init__(self, config: GoogleChatAdapterConfig | None = None) -> None: "GOOGLE_CHAT_PROJECT_NUMBER" ) self._pubsub_audience = config.pubsub_audience or os.environ.get("GOOGLE_CHAT_PUBSUB_AUDIENCE") + # Explicit opt-out of signature verification. An explicit + # config.disable_signature_verification value (True OR False) wins over + # the env var; only fall back to the env var when the config field is + # unset (None). Note: ``x if x is not None else default`` -- a plain + # ``or`` would silently discard an explicit ``False``. + self._disable_signature_verification = ( + config.disable_signature_verification + if config.disable_signature_verification is not None + else os.environ.get("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION") == "true" + ) + + # Fail-closed: refuse to construct unless webhook signature verification + # can be performed for at least one transport, or the operator has + # explicitly opted into the unverified path. Previously the adapter + # accepted any webhook in this state, allowing forged payloads to + # impersonate users / trigger handlers. Mirrors the gchat slice of + # upstream 9824d33 (PR #441). + if not (self._google_chat_project_number or self._pubsub_audience or self._disable_signature_verification): + raise ValidationError( + "gchat", + "Webhook signature verification is required. Set " + "google_chat_project_number (or GOOGLE_CHAT_PROJECT_NUMBER) for " + "direct webhooks and/or pubsub_audience (or " + "GOOGLE_CHAT_PUBSUB_AUDIENCE) for Pub/Sub. To accept unverified " + "webhooks (NOT recommended in production), set " + "disable_signature_verification=True.", + ) + + # The escape hatch is dev-only -- warn loudly whenever it is the only + # reason the adapter was allowed to construct without a verifier. + if self._disable_signature_verification and not (self._google_chat_project_number or self._pubsub_audience): + self._logger.warn( + "Google Chat webhook signature verification is disabled " + "(disable_signature_verification / " + "GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION). Incoming webhooks " + "will be accepted without JWT verification. Do not use this in " + "production -- set google_chat_project_number and/or " + "pubsub_audience instead.", + ) # In-progress subscription creations to prevent duplicate requests self._pending_subscriptions: dict[str, Any] = {} @@ -839,31 +878,59 @@ async def handle_webhook( and parsed["message"].get("data") and parsed.get("subscription") ): - # Verify Pub/Sub JWT if audience is configured + # Verify Pub/Sub JWT if audience is configured. The two transports + # share a single endpoint, so each shape must be independently + # verified -- otherwise an attacker could send the unconfigured + # shape to bypass the configured verifier. The constructor's "at + # least one verifier OR disable_signature_verification" check is + # not sufficient here for that reason. Mirrors upstream + # adapter-gchat/src/index.ts. if self._pubsub_audience: valid = await self._verify_bearer_token(request, self._pubsub_audience) if not valid: return {"body": "Unauthorized", "status": 401} - elif not self._warned_no_pubsub_verification: - self._warned_no_pubsub_verification = True + elif self._disable_signature_verification: + if not self._warned_no_pubsub_verification: + self._warned_no_pubsub_verification = True + self._logger.warn( + "Pub/Sub webhook verification is disabled. " + "Set GOOGLE_CHAT_PUBSUB_AUDIENCE or pubsubAudience to verify incoming requests." + ) + else: + # Direct-webhook verifier may be configured but Pub/Sub + # verifier is not -- reject rather than silently process an + # unverified payload that a peer transport's config does + # nothing to authenticate. self._logger.warn( - "Pub/Sub webhook verification is disabled. " - "Set GOOGLE_CHAT_PUBSUB_AUDIENCE or pubsubAudience to verify incoming requests." + "Rejected Pub/Sub webhook: pubsub_audience is not configured. " + "Set GOOGLE_CHAT_PUBSUB_AUDIENCE, or set " + "disable_signature_verification to accept unverified Pub/Sub payloads." ) + return {"body": "Unauthorized", "status": 401} return self._handle_pub_sub_message(parsed, options) - # Verify direct Google Chat webhook JWT if project number is configured + # Verify direct Google Chat webhook JWT if project number is configured. + # Same reasoning as the Pub/Sub branch: each shape requires its own + # verifier (or explicit opt-out) to prevent cross-transport bypass. if self._google_chat_project_number: valid = await self._verify_bearer_token(request, self._google_chat_project_number) if not valid: return {"body": "Unauthorized", "status": 401} - elif not self._warned_no_webhook_verification: - self._warned_no_webhook_verification = True + elif self._disable_signature_verification: + if not self._warned_no_webhook_verification: + self._warned_no_webhook_verification = True + self._logger.warn( + "Google Chat webhook verification is disabled. " + "Set GOOGLE_CHAT_PROJECT_NUMBER or googleChatProjectNumber " + "to verify incoming requests." + ) + else: self._logger.warn( - "Google Chat webhook verification is disabled. " - "Set GOOGLE_CHAT_PROJECT_NUMBER or googleChatProjectNumber " - "to verify incoming requests." + "Rejected direct Google Chat webhook: google_chat_project_number " + "is not configured. Set GOOGLE_CHAT_PROJECT_NUMBER, or set " + "disable_signature_verification to accept unverified payloads." ) + return {"body": "Unauthorized", "status": 401} # Treat as a direct Google Chat webhook event event: dict[str, Any] = parsed diff --git a/src/chat_sdk/adapters/google_chat/types.py b/src/chat_sdk/adapters/google_chat/types.py index cee3e70..f02e6a8 100644 --- a/src/chat_sdk/adapters/google_chat/types.py +++ b/src/chat_sdk/adapters/google_chat/types.py @@ -315,3 +315,16 @@ class GoogleChatAdapterConfig: # Override bot username user_name: str | None = None + + # Explicit opt-in to disable webhook signature verification. Required to + # construct the adapter when neither google_chat_project_number nor + # pubsub_audience is configured. Without this flag the constructor raises + # ValidationError -- fail-closed by default. Only enable in development or + # when an upstream layer (e.g. authenticated Cloud Run invocations) provides + # equivalent guarantees. Falls back to the + # GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION env var when left unset (None). + # + # Kept at the END of the field list intentionally: GoogleChatAdapterConfig + # is a positional-args dataclass, so inserting a new field in the middle + # would silently shift every later positional arg for existing callers. + disable_signature_verification: bool | None = None diff --git a/tests/test_critical_fixes.py b/tests/test_critical_fixes.py index 9d1baea..06a8b9c 100644 --- a/tests/test_critical_fixes.py +++ b/tests/test_critical_fixes.py @@ -90,6 +90,9 @@ def _make_google_chat_adapter() -> GoogleChatAdapter: private_key="-----BEGIN RSA PRIVATE KEY-----\nfake\n-----END RSA PRIVATE KEY-----", project_id="test-project", ), + # Adapter fails closed without a verification gating field; this test + # exercises non-verification mechanics, so use the explicit opt-out. + disable_signature_verification=True, ) return GoogleChatAdapter(config) diff --git a/tests/test_dispatch_key_validation.py b/tests/test_dispatch_key_validation.py index f6828ce..78c836c 100644 --- a/tests/test_dispatch_key_validation.py +++ b/tests/test_dispatch_key_validation.py @@ -327,7 +327,12 @@ def _make_adapter(self) -> Any: from chat_sdk.adapters.google_chat.adapter import GoogleChatAdapter from chat_sdk.adapters.google_chat.types import GoogleChatAdapterConfig - adapter = GoogleChatAdapter(GoogleChatAdapterConfig(use_application_default_credentials=True)) + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + use_application_default_credentials=True, + disable_signature_verification=True, + ) + ) adapter._chat = _make_mock_chat() adapter._bot_user_id = "users/bot123" return adapter diff --git a/tests/test_fixture_replay.py b/tests/test_fixture_replay.py index 5b8ba34..842fc33 100644 --- a/tests/test_fixture_replay.py +++ b/tests/test_fixture_replay.py @@ -467,6 +467,9 @@ def _make_adapter(self, fixture: dict[str, Any]) -> GoogleChatAdapter: client_email="test@test.iam.gserviceaccount.com", private_key="-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----", ), + # Replay fixtures carry no JWT; opt out of verification so the + # adapter constructs (fail-closed default would otherwise raise). + disable_signature_verification=True, ) adapter = GoogleChatAdapter(config) # Set bot user ID from fixture so the adapter can detect self-messages diff --git a/tests/test_gchat_api.py b/tests/test_gchat_api.py index 2fbfb34..ce62d8e 100644 --- a/tests/test_gchat_api.py +++ b/tests/test_gchat_api.py @@ -133,6 +133,9 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides: Any) -> GoogleChatAdapter: + # Adapter fails closed without a verification gating field; default these + # non-verification tests to the explicit opt-out. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, @@ -1088,6 +1091,7 @@ def test_logs_context_in_error(self): GoogleChatAdapterConfig( credentials=_make_credentials(), logger=mock_logger, + disable_signature_verification=True, ) ) diff --git a/tests/test_gchat_comprehensive.py b/tests/test_gchat_comprehensive.py index c80862c..7d18d8a 100644 --- a/tests/test_gchat_comprehensive.py +++ b/tests/test_gchat_comprehensive.py @@ -77,6 +77,9 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides: Any) -> GoogleChatAdapter: + # Adapter fails closed without a verification gating field; default these + # non-verification tests to the explicit opt-out. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, @@ -279,8 +282,10 @@ def test_throws_when_no_auth_configured_and_no_env_vars(self): saved = {k: v for k, v in os.environ.items() if k.startswith("GOOGLE_CHAT_")} try: self._clear_gchat_env() + # Gating field set so construction reaches the auth check rather + # than the fail-closed verification check. with pytest.raises(ValidationError, match="Authentication"): - GoogleChatAdapter(GoogleChatAdapterConfig()) + GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) finally: os.environ.update(saved) @@ -294,7 +299,7 @@ def test_resolves_credentials_from_env_var(self): "private_key": "-----BEGIN PRIVATE KEY-----\nfake\n-----END PRIVATE KEY-----\n", } ) - adapter = GoogleChatAdapter(GoogleChatAdapterConfig()) + adapter = GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) assert adapter.name == "gchat" finally: self._clear_gchat_env() @@ -305,7 +310,7 @@ def test_resolves_adc_from_env_var(self): try: self._clear_gchat_env() os.environ["GOOGLE_CHAT_USE_ADC"] = "true" - adapter = GoogleChatAdapter(GoogleChatAdapterConfig()) + adapter = GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) assert adapter.name == "gchat" finally: self._clear_gchat_env() @@ -322,7 +327,7 @@ def test_resolves_pubsub_topic_from_env_var(self): } ) os.environ["GOOGLE_CHAT_PUBSUB_TOPIC"] = "projects/test/topics/test" - adapter = GoogleChatAdapter(GoogleChatAdapterConfig()) + adapter = GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) assert adapter._pubsub_topic == "projects/test/topics/test" finally: self._clear_gchat_env() @@ -339,7 +344,7 @@ def test_resolves_impersonate_user_from_env_var(self): } ) os.environ["GOOGLE_CHAT_IMPERSONATE_USER"] = "user@example.com" - adapter = GoogleChatAdapter(GoogleChatAdapterConfig()) + adapter = GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) assert adapter._impersonate_user == "user@example.com" finally: self._clear_gchat_env() @@ -366,7 +371,12 @@ def test_config_credentials_take_priority_over_env_vars(self): class TestConstructorWithADC: def test_accepts_adc_config(self): - adapter = GoogleChatAdapter(GoogleChatAdapterConfig(use_application_default_credentials=True)) + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + use_application_default_credentials=True, + disable_signature_verification=True, + ) + ) assert adapter.name == "gchat" def test_default_user_name_is_bot(self): diff --git a/tests/test_gchat_verification.py b/tests/test_gchat_verification.py index 61143f3..d691347 100644 --- a/tests/test_gchat_verification.py +++ b/tests/test_gchat_verification.py @@ -1,12 +1,15 @@ """Tests for Google Chat webhook verification behaviour. -Covers: rejecting webhooks without auth header, rejecting invalid tokens, +Covers: the constructor fail-closed verification gate (google_chat_project_number, +pubsub_audience, or the disable_signature_verification escape hatch, with env +fallback), rejecting webhooks without auth header, rejecting invalid tokens, warning when no project number is configured, and allowing webhooks when verification is unconfigured. """ from __future__ import annotations +import dataclasses import json from typing import Any from unittest.mock import AsyncMock, MagicMock @@ -18,6 +21,29 @@ GoogleChatAdapterConfig, ServiceAccountCredentials, ) +from chat_sdk.shared.errors import ValidationError + +# Env vars that gate the constructor's fail-closed check. Cleared on a +# per-test basis with the `clear_verification_env` fixture below so suite +# ordering doesn't leak state into construction tests. +_VERIFICATION_ENV_KEYS = ( + "GOOGLE_CHAT_PROJECT_NUMBER", + "GOOGLE_CHAT_PUBSUB_AUDIENCE", + "GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", +) + + +@pytest.fixture +def clear_verification_env(monkeypatch: pytest.MonkeyPatch) -> pytest.MonkeyPatch: + """Remove gating env vars for the test, restore on teardown. + + Uses monkeypatch so both was-set and was-absent cases are handled -- + leaks here would silently satisfy the fail-closed gate in unrelated tests. + """ + for key in _VERIFICATION_ENV_KEYS: + monkeypatch.delenv(key, raising=False) + return monkeypatch + # ============================================================================= # Helpers @@ -33,6 +59,12 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides: Any) -> GoogleChatAdapter: + # The adapter now fails closed at construction unless one of + # google_chat_project_number, pubsub_audience, or + # disable_signature_verification is set. Tests that want the unconfigured + # runtime path default to the explicit opt-out; verification-gated tests + # pass google_chat_project_number / pubsub_audience to override it. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, @@ -174,6 +206,10 @@ async def test_warns_when_no_project_number_configured(self): logger.child = MagicMock(return_value=logger) adapter = _make_adapter(logger=logger) + # The constructor emits a dev-only warning when the escape hatch is the + # sole gate; reset the mock so this test asserts only the *runtime* + # warn path on the first unconfigured webhook. + logger.warn.reset_mock() # Explicitly clear project number adapter._google_chat_project_number = None adapter._warned_no_webhook_verification = False @@ -225,3 +261,264 @@ async def test_allows_webhook_without_verification_when_unconfigured(self): assert result["status"] == 200 # process_message should have been called since the event was valid chat.process_message.assert_called_once() + + +# ============================================================================= +# Tests -- constructor fail-closed verification gate +# +# Ports the gchat slice of upstream 9824d33 (PR #441): the constructor refuses +# to start unless webhook signature verification can be performed for at least +# one transport, or the operator explicitly opts out. +# ============================================================================= + + +def _config(**overrides: Any) -> GoogleChatAdapterConfig: + """Build a config with valid auth but no gating field unless overridden.""" + return GoogleChatAdapterConfig(credentials=_make_credentials(), **overrides) + + +class TestConstructorFailsClosed: + """The constructor must fail closed when no verifier is configured.""" + + def test_raises_when_no_gating_field_set(self, clear_verification_env: pytest.MonkeyPatch): + with pytest.raises(ValidationError, match="signature verification is required"): + GoogleChatAdapter(_config()) + + def test_explicit_disable_false_still_fails_closed(self, clear_verification_env: pytest.MonkeyPatch): + # An explicit False must be treated as "verification required", NOT as + # unset -- otherwise the env fallback / fail-closed logic would be wrong. + with pytest.raises(ValidationError, match="signature verification is required"): + GoogleChatAdapter(_config(disable_signature_verification=False)) + + +class TestConstructorEachGatingFieldSatisfiesIndividually: + """Any one of the three gating fields must allow construction.""" + + def test_google_chat_project_number_satisfies(self, clear_verification_env: pytest.MonkeyPatch): + adapter = GoogleChatAdapter(_config(google_chat_project_number="123456789")) + assert adapter.name == "gchat" + assert adapter._google_chat_project_number == "123456789" + + def test_pubsub_audience_satisfies(self, clear_verification_env: pytest.MonkeyPatch): + adapter = GoogleChatAdapter(_config(pubsub_audience="https://example.com/webhook")) + assert adapter.name == "gchat" + assert adapter._pubsub_audience == "https://example.com/webhook" + + def test_disable_signature_verification_satisfies(self, clear_verification_env: pytest.MonkeyPatch): + adapter = GoogleChatAdapter(_config(disable_signature_verification=True)) + assert adapter.name == "gchat" + assert adapter._disable_signature_verification is True + + +class TestEscapeHatchEmitsWarning: + """The dev-only escape hatch must construct AND log a warning.""" + + def test_escape_hatch_logs_warning(self, clear_verification_env: pytest.MonkeyPatch): + logger = MagicMock() + logger.child = MagicMock(return_value=logger) + adapter = GoogleChatAdapter(_config(disable_signature_verification=True, logger=logger)) + assert adapter._disable_signature_verification is True + warn_messages = [str(call) for call in logger.warn.call_args_list] + assert any("disabled" in m.lower() for m in warn_messages), ( + f"Expected a dev-only warning when the escape hatch is used, got: {warn_messages}" + ) + + def test_no_warning_when_real_verifier_configured(self, clear_verification_env: pytest.MonkeyPatch): + # The warning is specific to the escape hatch; a real verifier must not + # trigger it even if disable_signature_verification is also set. + logger = MagicMock() + logger.child = MagicMock(return_value=logger) + GoogleChatAdapter(_config(google_chat_project_number="123456789", logger=logger)) + warn_messages = [str(call) for call in logger.warn.call_args_list] + assert not any("disabled" in m.lower() for m in warn_messages), ( + f"Did not expect an escape-hatch warning with a real verifier, got: {warn_messages}" + ) + + +class TestDisableSignatureVerificationEnvFallback: + """The GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION env var must gate. + + Uses the ``clear_verification_env`` fixture which is built on pytest's + ``monkeypatch``; the previous manual try/finally pattern only restored + env vars that were SET before the test, leaking any newly-set var to + later tests and silently satisfying their fail-closed gate. + """ + + def test_env_true_satisfies_construction(self, clear_verification_env: pytest.MonkeyPatch): + clear_verification_env.setenv("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", "true") + adapter = GoogleChatAdapter(_config()) + assert adapter._disable_signature_verification is True + + def test_env_non_true_value_does_not_satisfy(self, clear_verification_env: pytest.MonkeyPatch): + # Only the literal "true" enables the opt-out; anything else fails closed. + clear_verification_env.setenv("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", "false") + with pytest.raises(ValidationError, match="signature verification is required"): + GoogleChatAdapter(_config()) + + def test_explicit_config_false_overrides_env_true(self, clear_verification_env: pytest.MonkeyPatch): + # An explicit config value wins over the env var, so a config False must + # fail closed even when the env var says "true". + clear_verification_env.setenv("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", "true") + with pytest.raises(ValidationError, match="signature verification is required"): + GoogleChatAdapter(_config(disable_signature_verification=False)) + + def test_env_does_not_leak_to_subsequent_construction(self, clear_verification_env: pytest.MonkeyPatch): + # Load-bearing for the monkeypatch fix: set the env var via monkeypatch, + # construct successfully, then simulate a fresh test by undoing the env + # var with the same fixture's API. A subsequent construction with no + # gating field must STILL raise -- proving the env var didn't leak. + clear_verification_env.setenv("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", "true") + GoogleChatAdapter(_config()) + clear_verification_env.delenv("GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION", raising=False) + with pytest.raises(ValidationError, match="signature verification is required"): + GoogleChatAdapter(_config()) + + +# ============================================================================= +# Tests -- per-shape verification gap (Finding 1 / upstream parity) +# +# handle_webhook accepts BOTH the direct webhook shape AND the Pub/Sub push +# shape on a single endpoint. If only one verifier is configured, the OTHER +# shape must be REJECTED (not warned-but-processed) -- otherwise an attacker +# could pick the unconfigured shape to bypass the configured verifier. +# Mirrors upstream adapter-gchat/src/index.ts. +# ============================================================================= + + +def _make_pubsub_push(*, space_name: str = "spaces/ABC123") -> dict[str, Any]: + """Build a minimal Pub/Sub push envelope. + + The body content doesn't matter -- _handle_pub_sub_message is reached only + after verification, so a payload that would fail to decode is fine as long + as we reach the rejection branch first. + """ + return { + "subscription": "projects/p/subscriptions/s", + "message": { + "data": "eyJmYWtlIjogInBheWxvYWQifQ==", # base64 of {"fake": "payload"} + "attributes": {"ce-type": "google.workspace.chat.message.v1.created"}, + "messageId": "1", + "publishTime": "2024-01-01T00:00:00Z", + }, + } + + +class TestPerShapeVerificationRejection: + """Each webhook shape must be rejected unless ITS verifier (or the explicit + escape hatch) is configured.""" + + @pytest.mark.asyncio + async def test_pubsub_push_rejected_when_only_project_number_configured(self): + # Only the direct-webhook verifier is set; a Pub/Sub push must be + # rejected -- under the previous code this returned 200 with just a + # warning, allowing an attacker to forge Pub/Sub-shaped payloads. + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + credentials=_make_credentials(), + google_chat_project_number="123456789", + ) + ) + state = _make_mock_state() + chat = _make_mock_chat(state) + await adapter.initialize(chat) + + request = FakeRequest(json.dumps(_make_pubsub_push()), headers={}) + result = await adapter.handle_webhook(request) + + assert result["status"] == 401 + assert "Unauthorized" in result["body"] + chat.process_message.assert_not_called() + + @pytest.mark.asyncio + async def test_direct_webhook_rejected_when_only_pubsub_audience_configured(self): + # Symmetric to the above: only the Pub/Sub verifier is set; a direct + # webhook payload must be rejected. + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + credentials=_make_credentials(), + pubsub_audience="https://example.com/webhook", + ) + ) + state = _make_mock_state() + chat = _make_mock_chat(state) + await adapter.initialize(chat) + + request = FakeRequest(json.dumps(_make_message_event()), headers={}) + result = await adapter.handle_webhook(request) + + assert result["status"] == 401 + assert "Unauthorized" in result["body"] + chat.process_message.assert_not_called() + + @pytest.mark.asyncio + async def test_disable_signature_verification_allows_both_shapes(self): + # The escape hatch is the explicit "accept unverified" mode -- it must + # let BOTH shapes through (with warnings) so the operator's opt-out + # actually covers both transports. + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + credentials=_make_credentials(), + disable_signature_verification=True, + ) + ) + state = _make_mock_state() + chat = _make_mock_chat(state) + await adapter.initialize(chat) + + # Direct webhook path + direct_result = await adapter.handle_webhook(FakeRequest(json.dumps(_make_message_event()), headers={})) + assert direct_result["status"] == 200 + + # Pub/Sub path (decoding may fail downstream; we only care that the + # 401 rejection branch wasn't taken) + pubsub_result = await adapter.handle_webhook(FakeRequest(json.dumps(_make_pubsub_push()), headers={})) + assert pubsub_result["status"] != 401 + + +# ============================================================================= +# Tests -- GoogleChatAdapterConfig field order (Finding 2) +# +# GoogleChatAdapterConfig is a positional-args dataclass. Inserting a new +# optional field in the MIDDLE silently shifts every later positional arg for +# existing callers (e.g. `Config("creds", "audience", impersonate, logger)` +# would put `impersonate` into `disable_signature_verification`). Pin the new +# field to the END of the field list to keep old positional callers working. +# ============================================================================= + + +class TestDisableSignatureVerificationFieldOrder: + def test_disable_signature_verification_is_last_field(self): + # Load-bearing: this test fails if a future change re-inserts the field + # in the middle of the dataclass. + field_names = [f.name for f in dataclasses.fields(GoogleChatAdapterConfig)] + assert field_names[-1] == "disable_signature_verification", ( + f"disable_signature_verification must be the LAST field of " + f"GoogleChatAdapterConfig (positional-args back-compat); got order: {field_names}" + ) + + def test_old_positional_call_does_not_misalign(self, clear_verification_env: pytest.MonkeyPatch): + # Simulates a pre-fail-closed-PR caller using the OLD positional order: + # (credentials, use_adc, endpoint_url, project_number, impersonate, logger, pubsub_audience, ...) + # The new field must not steal any of these positions. We assert each + # value lands in its named field. + logger_sentinel = MagicMock() + creds = _make_credentials() + config = GoogleChatAdapterConfig( + creds, # credentials + False, # use_application_default_credentials + "https://example.com/endpoint", # endpoint_url + "123456789", # google_chat_project_number + "alice@example.com", # impersonate_user + logger_sentinel, # logger + "https://example.com/audience", # pubsub_audience + ) + assert config.credentials is creds + assert config.use_application_default_credentials is False + assert config.endpoint_url == "https://example.com/endpoint" + assert config.google_chat_project_number == "123456789" + assert config.impersonate_user == "alice@example.com" + assert config.logger is logger_sentinel + assert config.pubsub_audience == "https://example.com/audience" + # New field falls back to its default rather than absorbing any of the + # above positional args. + assert config.disable_signature_verification is None diff --git a/tests/test_gchat_webhook.py b/tests/test_gchat_webhook.py index e3d9c9f..0b8174f 100644 --- a/tests/test_gchat_webhook.py +++ b/tests/test_gchat_webhook.py @@ -43,6 +43,9 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides: Any) -> GoogleChatAdapter: + # Adapter fails closed without a verification gating field; default these + # non-verification tests to the explicit opt-out. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, @@ -195,8 +198,10 @@ def test_custom_user_name(self): assert adapter.user_name == "mybot" def test_no_auth_raises(self): - with pytest.raises(ValidationError): - GoogleChatAdapter(GoogleChatAdapterConfig()) + # Pass a verification gating field so construction reaches (and fails + # on) the auth check rather than the fail-closed verification check. + with pytest.raises(ValidationError, match="Authentication"): + GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) # --------------------------------------------------------------------------- diff --git a/tests/test_gchat_webhook_extended.py b/tests/test_gchat_webhook_extended.py index e5c0186..cf1072e 100644 --- a/tests/test_gchat_webhook_extended.py +++ b/tests/test_gchat_webhook_extended.py @@ -46,6 +46,9 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides: Any) -> GoogleChatAdapter: + # Adapter fails closed without a verification gating field; default these + # non-verification tests to the explicit opt-out. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, diff --git a/tests/test_get_user_adapters.py b/tests/test_get_user_adapters.py index f9dd124..3f53807 100644 --- a/tests/test_get_user_adapters.py +++ b/tests/test_get_user_adapters.py @@ -287,6 +287,7 @@ def _make_adapter(self): private_key="-----BEGIN RSA PRIVATE KEY-----\nfake\n-----END RSA PRIVATE KEY-----", project_id="test-project", ), + disable_signature_verification=True, ) ) diff --git a/tests/test_google_chat_adapter.py b/tests/test_google_chat_adapter.py index c726af0..cc862cc 100644 --- a/tests/test_google_chat_adapter.py +++ b/tests/test_google_chat_adapter.py @@ -34,6 +34,11 @@ def _make_credentials() -> ServiceAccountCredentials: def _make_adapter(**overrides) -> GoogleChatAdapter: """Create a GoogleChatAdapter with minimal valid config.""" + # The adapter now fails closed unless a verification gating field is set + # (google_chat_project_number, pubsub_audience, or the explicit opt-out). + # These tests exercise non-verification mechanics, so default to the + # explicit opt-out; individual tests override it via kwargs as needed. + overrides.setdefault("disable_signature_verification", True) config = GoogleChatAdapterConfig( credentials=overrides.pop("credentials", _make_credentials()), **overrides, @@ -176,15 +181,22 @@ def test_with_credentials(self): assert adapter.name == "gchat" def test_with_adc(self): - adapter = GoogleChatAdapter(GoogleChatAdapterConfig(use_application_default_credentials=True)) + adapter = GoogleChatAdapter( + GoogleChatAdapterConfig( + use_application_default_credentials=True, + disable_signature_verification=True, + ) + ) assert adapter.name == "gchat" def test_missing_auth(self): old_creds = os.environ.pop("GOOGLE_CHAT_CREDENTIALS", None) old_adc = os.environ.pop("GOOGLE_CHAT_USE_ADC", None) try: + # Provide a verification gating field so construction reaches the + # auth check rather than failing closed on verification first. with pytest.raises(ValidationError, match="Authentication"): - GoogleChatAdapter(GoogleChatAdapterConfig()) + GoogleChatAdapter(GoogleChatAdapterConfig(disable_signature_verification=True)) finally: if old_creds is not None: os.environ["GOOGLE_CHAT_CREDENTIALS"] = old_creds diff --git a/tests/test_production_fixes.py b/tests/test_production_fixes.py index 8ee61bf..d6ad5e7 100644 --- a/tests/test_production_fixes.py +++ b/tests/test_production_fixes.py @@ -111,7 +111,15 @@ async def test_concurrent_refreshes_only_issue_one_http_request(self): """Two concurrent _get_access_token calls should only trigger one refresh.""" from chat_sdk.adapters.google_chat.adapter import GoogleChatAdapter - with patch.dict("os.environ", {"GOOGLE_CHAT_CREDENTIALS": '{"client_email":"a@b.iam","private_key":"fake"}'}): + # Adapter fails closed without a verification gating field; opt out via + # env for this non-verification test. + with patch.dict( + "os.environ", + { + "GOOGLE_CHAT_CREDENTIALS": '{"client_email":"a@b.iam","private_key":"fake"}', + "GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION": "true", + }, + ): adapter = GoogleChatAdapter() call_count = 0