gchat: fail-closed JWT verification at construction (port of 9824d33)#130
gchat: fail-closed JWT verification at construction (port of 9824d33)#130patrick-chinchill wants to merge 2 commits into
Conversation
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
|
Warning Review limit reached
More reviews will be available in 60 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fail-closed security mechanism to the Google Chat adapter. The adapter's constructor now requires webhook signature verification to be configured via google_chat_project_number or pubsub_audience, or explicitly disabled using the new disable_signature_verification configuration option (or its corresponding environment variable). If none of these are set, a ValidationError is raised. The test suite has been updated to support this new validation, and comprehensive tests have been added to verify the constructor's fail-closed behavior. There are no review comments to address, and the implementation is clean and correct.
|
@codex review Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd4a2927e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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): |
There was a problem hiding this comment.
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 👍 / 👎.
| adapter = GoogleChatAdapter(_config()) | ||
| assert adapter._disable_signature_verification is True | ||
| finally: | ||
| os.environ.update(saved) |
There was a problem hiding this comment.
In the env-fallback tests, this finally only re-adds variables that existed before the test; if GOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION was absent, setting it to true above leaves it set after the test. That can make later GoogleChatAdapter constructions pass the new fail-closed gate through the env fallback and hide missing configuration, so these tests should use monkeypatch or clear the gating keys before restoring saved.
Useful? React with 👍 / 👎.
| # 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). | ||
| disable_signature_verification: bool | None = None |
There was a problem hiding this comment.
Because GoogleChatAdapterConfig is a public dataclass with positional __init__ arguments, inserting this field in the middle shifts every existing positional argument after google_chat_project_number; a caller that previously passed impersonate_user, logger, or pubsub_audience positionally will now populate disable_signature_verification with that value, potentially making the new gate truthy and misassigning the remaining config. Add the new option at the end of the dataclass or make the config keyword-only to avoid breaking existing callers.
Useful? React with 👍 / 👎.
… 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
|
@GHCodex review Generated by Claude Code |
2 participants
What this ports
Ports the gchat slice of upstream
chatcommit9824d33(PR #441 "adapter hardening pass") toGoogleChatAdapter.9824d33is a 4-part security pass across multiple adapters; this PR is the Google Chat portion only (worklist P1-5).GoogleChatAdapternow fails closed at two layers:ValidationErrorunless webhook signature verification can be performed for at least one transport, or the operator explicitly opts out.handle_webhookruntime per-path routing: each webhook shape (direct vs Pub/Sub push) is independently verified or rejected with 401, even if the OTHER shape is configured. The construct-time any-of gate is necessary but not sufficient — the runtime check is what stops a Pub/Sub-only-configured adapter from accepting an unverified direct webhook (and vice versa). This matches upstream'shandleWebhookbyte-for-byte (vercel/chat'sadapter-gchat/src/index.ts:705-764).Exact gating condition (construct-time)
The constructor raises unless one of these is satisfied:
google_chat_project_numberis set (orGOOGLE_CHAT_PROJECT_NUMBER) — direct webhooks, orpubsub_audienceis set (orGOOGLE_CHAT_PUBSUB_AUDIENCE) — Pub/Sub push, ordisable_signature_verification: boolconfig flag is explicitlyTrue(withGOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATION=trueenv fallback) — the dev-only escape hatch.This mirrors upstream's exact condition set (
googleChatProjectNumber || pubsubAudience || disableSignatureVerification).Runtime per-path verification (in
handle_webhook)The Python port previously had a
warn-and-allowpath for the unconfigured shape — upstream had already fixed this in9824d33but the Python port shipped the gap. Now matched:pubsub_audience-only config → 401 Unauthorizedgoogle_chat_project_number-only config → 401 Unauthorizeddisable_signature_verification=True→ both shapes accepted (warn-and-process; escape hatch activated only when the real verifier is absent for that shape)Pub/Sub detection is unambiguous:
isinstance(parsed.get("message"), dict) and parsed["message"].get("data") and parsed.get("subscription")— anything not matching falls to the direct branch, and each branch independently requires its own verifier. No shape-ambiguity bypass.New config field
Added
disable_signature_verification: bool | None = NonetoGoogleChatAdapterConfig, placed at the END of the dataclass field list to preserve positional-call compatibility with existing callers. The bool/env resolution usesx if x is not None else defaultso an explicitdisable_signature_verification=Falseis distinct from unset (None) and still fails closed — matching upstream'sconfig.x ?? process.env... === "true". A plainorwould have silently discarded an explicitFalse.Escape-hatch rationale
disable_signature_verificationexists for local development (or when an upstream layer such as authenticated Cloud Run invocations provides equivalent guarantees). It is not recommended in production. When it is the sole reason the adapter constructs (no real verifier configured), the constructor logs awarn.Existing fixtures updated
A fail-closed change breaks every existing gchat adapter instantiation that did not set a gating field. Updated test fixtures across 11 files to set a valid gate (mostly the explicit
disable_signature_verification=Trueopt-out, since those tests exercise non-verification mechanics):test_google_chat_adapter.py,test_gchat_webhook.py,test_gchat_webhook_extended.py,test_gchat_api.py,test_gchat_comprehensive.py,test_gchat_verification.py(helpers + inline ctors)test_critical_fixes.py,test_production_fixes.py,test_fixture_replay.py,test_dispatch_key_validation.py,test_get_user_adapters.py(cross-cutting fixtures)Auth-error tests (
test_missing_auth,test_no_auth_raises,test_throws_when_no_auth_configured_and_no_env_vars) were given a gating field so they still reach the auth check (the verification check now precedes auth, matching upstream order).Confirmed these are fixture fixes, not masked breaks: the per-path runtime routing is the intended security tightening; the only thing that needed updating in fixtures was construction without a gate and webhook tests that previously relied on the unverified-shape warn-and-allow path.
New load-bearing tests (
test_gchat_verification.py)Construct-time gate:
ValidationErrorGOOGLE_CHAT_DISABLE_SIGNATURE_VERIFICATIONenv fallback works (true satisfies; non-"true"fails closed)disable_signature_verification=Falsestill fails closed and overrides envtrue(the unset-vs-explicit-False distinction)Runtime per-path verification (
TestPerShapeVerificationRejection):test_pubsub_push_rejected_when_only_project_number_configured— load-bearing (pre-fix returned 200; post-fix returns 401)test_direct_webhook_rejected_when_only_pubsub_audience_configured— load-bearing (symmetric)test_disable_signature_verification_allows_both_shapes— escape-hatch pathAPI compatibility (
TestDisableSignatureVerificationFieldOrder):test_disable_signature_verification_is_last_field— pins the dataclass field order at runtimetest_old_positional_call_does_not_misalign— load-bearing against the field-insertion bugTest isolation (
TestDisableSignatureVerificationEnvFallback):test_env_does_not_leak_to_subsequent_construction— env-fallback tests converted tomonkeypatch, no leak across testsBlast radius
This is a breaking change for downstream gchat configs. Any
GoogleChatAdapterinstantiation that relied on the old "construct anyway, accept unverified webhooks" default will now raiseValidationErrorat construction. Downstream callers must set one gating field (google_chat_project_number,pubsub_audience, ordisable_signature_verification=True) per gchat adapter instance.chinchill-api impact: NONE. A check of chinchill-api's codebase confirmed it does not currently use
chat_sdk's GoogleChatAdapter (only Slack and Teams adapters are registered). The CI-sidegoogle-chat-notifyaction is an incoming webhook poster, unrelated to the SDK adapter. So this PR can land without coordinated chinchill-api changes.Within this repo the blast radius was limited to test fixtures only (11 files); the single
src/caller is thecreate_google_chat_adapterfactory, which just passes config through. No new runtime dependencies.Validation
uv run ruff check src/ tests/→ All checks passeduv run ruff format --check src/ tests/→ all formatteduv run python scripts/audit_test_quality.py→ 0 hard failures (pre-existing cross-adapter name-duplicate warnings only)uv run pytest tests/ -k google_chat -q→ 26 passeduv run pytest tests/ -q→ 4063 passed, 3 skipped (skips pre-existing, unrelated)Notes
pyproject.tomlbump in this PR (cut PR handles versioning).9824d33.https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code