Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 78 additions & 11 deletions src/chat_sdk/adapters/google_chat/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require verification for each accepted webhook shape

When only one verifier is configured, this gate still lets the adapter start while the other webhook shape remains accepted without verification: for example, with only pubsub_audience set, a non-Pub/Sub/direct Google Chat payload falls through to the direct branch, logs the existing warning, and is processed without a project-number JWT check; the symmetric case exists for Pub/Sub when only google_chat_project_number is set. If handle_webhook continues to accept both shapes on the same endpoint, construction should either require verifiers for both paths or require the explicit disable flag for any unverified path.

Useful? React with 👍 / 👎.

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] = {}
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions src/chat_sdk/adapters/google_chat/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 3 additions & 0 deletions tests/test_critical_fixes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
7 changes: 6 additions & 1 deletion tests/test_dispatch_key_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions tests/test_fixture_replay.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions tests/test_gchat_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1088,6 +1091,7 @@ def test_logs_context_in_error(self):
GoogleChatAdapterConfig(
credentials=_make_credentials(),
logger=mock_logger,
disable_signature_verification=True,
)
)

Expand Down
22 changes: 16 additions & 6 deletions tests/test_gchat_comprehensive.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)

Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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):
Expand Down
Loading
Loading