gchat: fail-closed JWT verification at construction (port of 9824d33) (#130)

* gchat: fail-closed JWT verification at construction (port of 9824d33)

Port the gchat slice of upstream chat 9824d33 (PR #441 "adapter hardening
pass") to GoogleChatAdapter. The constructor now fails closed: it raises
ValidationError unless at least one of the following gates webhook signature
verification --

  - google_chat_project_number (or GOOGLE_CHAT_PROJECT_NUMBER), or
  - pubsub_audience (or GOOGLE_CHAT_PUBSUB_AUDIENCE), or
  - a new disable_signature_verification config flag explicitly set True
    (with GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION=true env fallback).

Previously the adapter constructed in any state and silently accepted
unverified webhooks when no verifier was configured, allowing forged payloads
to impersonate users / trigger handlers. The new disable_signature_verification
flag is an explicit, dev-only escape hatch; when it is the sole reason the
adapter constructs, a warning is logged.

The bool/env resolution uses `x if x is not None else default` so an explicit
disable_signature_verification=False is distinct from unset and still fails
closed (matching upstream `config.x ?? env === "true"`).

Existing gchat test fixtures constructed the adapter without any gating field
and now break under fail-closed; updated those fixtures (helpers + inline
constructors) to set a valid gate (mostly the explicit opt-out, since they
exercise non-verification mechanics). Verified this is a fixture fix, not a
masked behavioral break: runtime webhook routing is unchanged in this slice.

Tracking: #98

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

* fix(gchat): per-shape verification + dataclass field order + test env isolation (codex review)

Addresses three findings from Codex's review of #130.

1. Per-shape webhook verification (P1, security).

   handle_webhook accepts BOTH direct Google Chat webhooks AND Pub/Sub push
   shapes on a single endpoint. The constructor's "at least one verifier OR
   disable_signature_verification" check is a coarse gate: an operator who
   configured only pubsub_audience left the direct-webhook shape going through
   the warned-but-unverified path (and symmetrically for project_number vs
   Pub/Sub). An attacker could pick the unconfigured shape to bypass the
   configured verifier.

   handle_webhook now REJECTS (401) any shape whose verifier isn't configured,
   unless disable_signature_verification is explicitly set -- in which case
   the existing warn-and-process path is preserved. Mirrors upstream
   adapter-gchat/src/index.ts (vercel/chat) faithfully; upstream solves this
   the same way (per-path require, construct-time stays any-of).

2. GoogleChatAdapterConfig.disable_signature_verification field order (P2).

   The PR inserted the new field in the MIDDLE of the dataclass, shifting
   every later positional arg. A caller like
       GoogleChatAdapterConfig("creds", "audience", impersonate, logger)
   would silently route `impersonate` into `disable_signature_verification`.
   Moved the field to the END so existing positional callers keep working
   (the field has a default so trailing positional callers are unaffected).
   Added a regression test that pins the field's position via
   dataclasses.fields().

3. Env-var leakage in fail-closed tests (P2, test isolation).

   The env-fallback tests used a manual try/finally that only restored env
   vars SET before the test. Setting
   GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION=true in a test where the var
   was originally absent leaked it to later tests, silently satisfying the
   fail-closed gate in unrelated construction tests.

   Converted to pytest's monkeypatch fixture, which restores both was-set
   and was-absent cases at teardown. Added a load-bearing leak-detection
   test that exercises the previously-leaky pattern within a single test.

All three findings have load-bearing tests added under tests/test_gchat_verification.py:
- TestPerShapeVerificationRejection (3 tests; first two would 200 instead of 401 pre-fix)
- TestDisableSignatureVerificationFieldOrder (2 tests; both fail pre-fix)
- TestDisableSignatureVerificationEnvFallback::test_env_does_not_leak_to_subsequent_construction

Validation: uv run ruff check, ruff format --check, audit_test_quality.py
(0 hard failures), full pytest (4063 passed, 3 skipped).

https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: patrick-chinchill

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

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

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)
 

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

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

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,
             )
         )
 

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):