diff --git a/package.json b/package.json index 2d2edd48dc244..4e96e8a10a68d 100644 --- a/package.json +++ b/package.json @@ -114,6 +114,7 @@ "@types/convert-source-map": "^2.0.3", "@types/css-tree": "^3.2.0", "@types/eslint-config-prettier": "^6.11.3", + "@types/estree": "^1.0.9", "@types/gtag.js": "^0.0.20", "@types/is-url": "^1.2.32", "@types/jquery": "^4.0.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 48b0c728e1b1d..608d657e1e7b2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -354,6 +354,9 @@ importers: '@types/eslint-config-prettier': specifier: ^6.11.3 version: 6.11.3 + '@types/estree': + specifier: ^1.0.9 + version: 1.0.9 '@types/gtag.js': specifier: ^0.0.20 version: 0.0.20 diff --git a/zerver/decorator.py b/zerver/decorator.py index 779b0f9fc6c4d..7a63586f83488 100644 --- a/zerver/decorator.py +++ b/zerver/decorator.py @@ -54,7 +54,9 @@ from zerver.lib.utils import has_api_key_format from zerver.lib.webhooks.common import ( MissingHTTPEventHeaderError, + WebhookSignatureConfig, notify_bot_owner_about_invalid_json, + validate_webhook_signature, ) from zerver.models import UserProfile from zerver.models.clients import get_client @@ -372,6 +374,7 @@ def webhook_view( webhook_client_name: str, notify_bot_owner_on_invalid_json: bool = True, all_event_types: Sequence[str] | None = None, + signature_config: WebhookSignatureConfig | None = None, ) -> Callable[[Callable[..., HttpResponse]], Callable[..., HttpResponse]]: # Unfortunately, callback protocols are insufficient for this: # https://mypy.readthedocs.io/en/stable/protocols.html#callback-protocols @@ -390,6 +393,11 @@ def _wrapped_func_arguments( allow_webhook_access=True, client_name=full_webhook_client_name(webhook_client_name), ) + validate_webhook_signature( + request, + user_profile, + signature_config, + ) request_notes = RequestNotes.get_notes(request) request_notes.is_webhook_view = True diff --git a/zerver/lib/integrations.py b/zerver/lib/integrations.py index 35c715d6e1fe0..1955a2da0bfa4 100644 --- a/zerver/lib/integrations.py +++ b/zerver/lib/integrations.py @@ -12,7 +12,12 @@ from typing_extensions import override from zerver.lib.storage import static_path -from zerver.lib.webhooks.common import PresetUrlOption, WebhookConfigOption, WebhookUrlOption +from zerver.lib.webhooks.common import ( + PresetUrlOption, + WebhookConfigOption, + WebhookSignatureConfig, + WebhookUrlOption, +) from zerver.webhooks import fixtureless_integrations """This module declares all of the (documented) integrations available @@ -1196,6 +1201,15 @@ def is_enabled_in_catalog(self) -> bool: | hubot_integration_names ) +WEBHOOK_SIGNATURE_CONFIGS: dict[str, WebhookSignatureConfig] = { + "github": WebhookSignatureConfig( + integration_name="github", + header="X_HUB_SIGNATURE_256", + algorithm="sha256", + prefix="sha256=", + ), +} + # Add integrations that are not meant to have example screenshots here INTEGRATIONS_WITHOUT_SCREENSHOTS = ( # Integration frameworks diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index 9ebac70fd6059..e65b05123ee3f 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -35,6 +35,7 @@ from django.test.testcases import SerializeMixin from django.urls import resolve from django.utils import translation +from django.utils.encoding import force_bytes from django.utils.module_loading import import_string from django.utils.timezone import now as timezone_now from fakeldap import MockLDAP @@ -54,9 +55,11 @@ from zerver.actions.user_settings import do_change_full_name, do_change_user_setting from zerver.actions.users import do_change_user_role from zerver.decorator import do_two_factor_login +from zerver.lib.bot_config import set_bot_config from zerver.lib.cache import bounce_key_prefix_for_testing from zerver.lib.email_notifications import MissedMessageData, handle_missedmessage_emails from zerver.lib.initial_password import initial_password +from zerver.lib.integrations import WEBHOOK_SIGNATURE_CONFIGS from zerver.lib.mdiff import diff_strings from zerver.lib.message import access_message from zerver.lib.notification_data import UserMessageNotificationsData @@ -92,8 +95,10 @@ from zerver.lib.upload import upload_message_attachment_from_request from zerver.lib.user_groups import get_system_user_group_for_user from zerver.lib.webhooks.common import ( + WEBHOOK_SECRET_TOKEN_KEY, call_fixture_to_headers, check_send_webhook_message, + compute_webhook_signature, standardize_headers, ) from zerver.models import ( @@ -2547,6 +2552,8 @@ class WebhookTestCase(ZulipTestCase): DEFAULT_URL_TEMPLATE: str = ( "/api/v1/external/{webhook_dir_name}?stream={stream}&api_key={api_key}" ) + WEBHOOK_TEST_SECRET: str | None = None + VERIFY_WEBHOOK_SIGNATURES: bool = True def get_webhook_dir_name(self) -> str: module_parts = self.__module__.split(".") @@ -2563,6 +2570,19 @@ def setUp(self) -> None: self.url_template = self.URL_TEMPLATE or self.DEFAULT_URL_TEMPLATE self.url = self.build_webhook_url() + if self.WEBHOOK_TEST_SECRET is not None: + config = WEBHOOK_SIGNATURE_CONFIGS.get(self.webhook_dir_name.lower()) + if config is None: + raise AssertionError( + f"WEBHOOK_TEST_SECRET was set for '{self.webhook_dir_name}', " + f"but no WebhookSignatureConfig is registered in WEBHOOK_SIGNATURE_CONFIGS." + ) + set_bot_config( + self.test_user, + WEBHOOK_SECRET_TOKEN_KEY.format(integration_name=self.webhook_dir_name.lower()), + self.WEBHOOK_TEST_SECRET, + ) + function = import_string( f"zerver.webhooks.{self.webhook_dir_name}.view.api_{self.webhook_dir_name}_webhook" ) @@ -2653,26 +2673,38 @@ def check_webhook( """ self.subscribe(self.test_user, self.channel_name) + url = self.url + webhook_secret = self.WEBHOOK_TEST_SECRET + config = WEBHOOK_SIGNATURE_CONFIGS.get(self.webhook_dir_name.lower()) + payload = self.get_payload(fixture_name) if content_type is not None: extra["content_type"] = content_type + + if webhook_secret is not None and config is not None: + header_val = compute_webhook_signature( + force_bytes(webhook_secret), + force_bytes(payload), + config, + ) + + django_header = "HTTP_" + config.header.upper().replace("-", "_") + if django_header not in extra: + extra[django_header] = header_val + headers = call_fixture_to_headers(self.webhook_dir_name, fixture_name) headers = standardize_headers(headers) extra.update(headers) - try: - msg = self.send_webhook_payload( - self.test_user, - self.url, - payload, - **extra, - ) - except EmptyResponseError: - if expect_noop: - return - else: - raise AssertionError( - "No message was sent. Pass expect_noop=True if this is intentional." - ) + with self.settings(VERIFY_WEBHOOK_SIGNATURES=self.VERIFY_WEBHOOK_SIGNATURES): + try: + msg = self.send_webhook_payload(self.test_user, url, payload, **extra) + except EmptyResponseError: + if expect_noop: + return + else: + raise AssertionError( + "No message was sent. Pass expect_noop=True if this is intentional." + ) if expect_noop: raise Exception( @@ -2718,6 +2750,7 @@ def send_and_test_private_message( Most webhooks send to streams, and you will want to look at check_webhook. """ + payload = self.get_payload(fixture_name) extra["content_type"] = content_type @@ -2728,12 +2761,13 @@ def send_and_test_private_message( if sender is None: sender = self.test_user - msg = self.send_webhook_payload( - sender, - self.url, - payload, - **extra, - ) + with self.settings(VERIFY_WEBHOOK_SIGNATURES=self.VERIFY_WEBHOOK_SIGNATURES): + msg = self.send_webhook_payload( + sender, + self.url, + payload, + **extra, + ) self.assertEqual(msg.content, expected_message) return msg diff --git a/zerver/lib/webhooks/common.py b/zerver/lib/webhooks/common.py index ac2d8ba7e7a16..3ff00efa16dc6 100644 --- a/zerver/lib/webhooks/common.py +++ b/zerver/lib/webhooks/common.py @@ -25,6 +25,7 @@ check_send_stream_message_by_id, send_rate_limited_pm_notification_to_bot_owner, ) +from zerver.lib.bot_config import ConfigError, get_bot_config from zerver.lib.exceptions import ( AnomalousWebhookPayloadError, ErrorCode, @@ -58,6 +59,8 @@ SETUP_MESSAGE_TEMPLATE = "{integration} webhook has been successfully configured" SETUP_MESSAGE_USER_PART = " by {user_name}" +WEBHOOK_SECRET_TOKEN_KEY = "{integration_name}:webhook_secret_token" + OptionalUserSpecifiedTopicStr: TypeAlias = Annotated[str | None, ApiParamConfig("topic")] @@ -74,6 +77,16 @@ class WebhookConfigOption: validator: Callable[[str, str], str | bool | None] +@dataclass(frozen=True) +class WebhookSignatureConfig: + integration_name: str + header: str + algorithm: str = "sha256" + prefix: str = "" + # This will override the default compute_webhook_signature function if provided for unique formats + custom_formatter: Callable[[str], str] | None = None + + @dataclass class WebhookUrlOption: name: str @@ -321,36 +334,65 @@ def parse_multipart_string(body: str) -> dict[str, str]: def validate_webhook_signature( - request: HttpRequest, payload: str, signature: str, algorithm: str = "sha256" + request: HttpRequest, + user_profile: UserProfile, + config: WebhookSignatureConfig | None, ) -> None: - if not settings.VERIFY_WEBHOOK_SIGNATURES: # nocoverage + if not settings.VERIFY_WEBHOOK_SIGNATURES or not config: return - if algorithm not in hashlib.algorithms_available: + if config.algorithm not in hashlib.algorithms_available: raise AssertionError( - _("The algorithm '{algorithm}' is not supported.").format(algorithm=algorithm) + _("The algorithm '{algorithm}' is not supported.").format(algorithm=config.algorithm) ) - webhook_secret: str | None = request.GET.get("webhook_secret") - if webhook_secret is None: - raise JsonableError( - _( - "The webhook secret is missing. Please set the webhook_secret while generating the URL." - ) - ) - webhook_secret_bytes = force_bytes(webhook_secret) - payload_bytes = force_bytes(payload) + signature_header = request.headers.get(config.header) + if not signature_header: + return - signed_payload = hmac.new( - webhook_secret_bytes, - payload_bytes, - algorithm, - ).hexdigest() + try: + bot_config = get_bot_config(user_profile) + except ConfigError: + return - if not constant_time_compare(signed_payload, signature): + webhook_secret = bot_config.get( + WEBHOOK_SECRET_TOKEN_KEY.format(integration_name=config.integration_name.lower()) + ) + + if not webhook_secret or not webhook_secret.strip(): + raise JsonableError(_("Webhook secret is not configured for this bot.")) + + payload = request.body.decode("utf-8") + + expected_header_val = compute_webhook_signature( + force_bytes(webhook_secret), + force_bytes(payload), + config, + ) + if not constant_time_compare(expected_header_val, signature_header): raise JsonableError(_("Webhook signature verification failed.")) +def compute_webhook_signature( + secret_bytes: bytes, + payload_bytes: bytes, + config: WebhookSignatureConfig, +) -> str: + """Computes and formats the HMAC signature for a webhook payload.""" + signer = hmac.new( + secret_bytes, + payload_bytes, + config.algorithm, + ) + digest = signer.hexdigest() + + if config.custom_formatter is not None: + digest = config.custom_formatter(digest) + if config.prefix: + return f"{config.prefix}{digest}" + return digest + + def guess_zulip_user_from_external_account( realm: Realm, external_username: str, diff --git a/zerver/tests/test_webhooks_common.py b/zerver/tests/test_webhooks_common.py index 02db0bdd1ce09..4fd7f6949c7ad 100644 --- a/zerver/tests/test_webhooks_common.py +++ b/zerver/tests/test_webhooks_common.py @@ -1,5 +1,3 @@ -import hashlib -import hmac from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -14,6 +12,7 @@ from zerver.actions.custom_profile_fields import try_add_realm_custom_profile_field from zerver.actions.streams import do_rename_stream from zerver.decorator import webhook_view +from zerver.lib.bot_config import ConfigError, set_bot_config from zerver.lib.exceptions import InvalidJSONError, JsonableError from zerver.lib.request import RequestNotes from zerver.lib.send_email import FromAddress @@ -22,9 +21,12 @@ from zerver.lib.webhooks.common import ( INVALID_JSON_MESSAGE, MISSING_EVENT_HEADER_MESSAGE, + WEBHOOK_SECRET_TOKEN_KEY, MissingHTTPEventHeaderError, + WebhookSignatureConfig, call_fixture_to_headers, check_send_webhook_message, + compute_webhook_signature, get_event_header, get_service_api_data, guess_zulip_user_from_external_account, @@ -152,34 +154,95 @@ def test_standardize_headers(self) -> None: @override_settings(VERIFY_WEBHOOK_SIGNATURES=True) def test_validate_webhook_signature(self) -> None: - request = HostRequestMock() - request.GET = QueryDict("", mutable=True) - - # Valid signature + webhook_bot = get_user("webhook-bot@zulip.com", get_realm("zulip")) webhook_secret = "test_secret" + config = WebhookSignatureConfig( + integration_name="github", + header="X-Hub-Signature-256", + algorithm="sha256", + prefix="sha256=", + ) payload = '{"key": "value"}' - signature = hmac.new( - force_bytes(webhook_secret), force_bytes(payload), hashlib.sha256 - ).hexdigest() + signature = compute_webhook_signature( + force_bytes(webhook_secret), force_bytes(payload), config + ) + + # Missing header early return check + request = HostRequestMock(meta_data={}) + request.user = webhook_bot + request.GET = QueryDict("", mutable=True) + request._body = force_bytes(payload) + validate_webhook_signature(request, webhook_bot, config) + + # ConfigError early return check + request = HostRequestMock(meta_data={"HTTP_X_HUB_SIGNATURE_256": signature}) + request.user = webhook_bot + request.GET = QueryDict("", mutable=True) + request._body = force_bytes(payload) + with patch("zerver.lib.webhooks.common.get_bot_config", side_effect=ConfigError): + validate_webhook_signature(request, webhook_bot, config) - request.GET.update({"webhook_secret": webhook_secret}) - validate_webhook_signature(request, payload, signature) + # Unconfigured secret initial pass + request = HostRequestMock(meta_data={"HTTP_X_HUB_SIGNATURE_256": signature}) + request.user = webhook_bot + request.GET = QueryDict("", mutable=True) + request._body = force_bytes(payload) + validate_webhook_signature(request, webhook_bot, config) - # Invalid signature - invalid_signature = "invalid_signature" + # Valid signature with configured token key + set_bot_config( + webhook_bot, WEBHOOK_SECRET_TOKEN_KEY.format(integration_name="github"), webhook_secret + ) + request = HostRequestMock(meta_data={"HTTP_X_HUB_SIGNATURE_256": signature}) + request.user = webhook_bot + request.GET = QueryDict("", mutable=True) + request._body = force_bytes(payload) + validate_webhook_signature(request, webhook_bot, config) + + # Invalid signature check + request.META["HTTP_X_HUB_SIGNATURE_256"] = "sha256=invalid_signature" + del request.headers with self.assertRaisesRegex( JsonableError, "Webhook signature verification failed.", ): - validate_webhook_signature(request, payload, invalid_signature) + validate_webhook_signature(request, webhook_bot, config) - # No webhook_secret parameter - request.GET.clear() + # Missing secret check using token key + set_bot_config(webhook_bot, WEBHOOK_SECRET_TOKEN_KEY.format(integration_name="github"), "") + request.META["HTTP_X_HUB_SIGNATURE_256"] = signature + del request.headers with self.assertRaisesRegex( JsonableError, - "The webhook secret is missing. Please set the webhook_secret while generating the URL.", + "Webhook secret is not configured for this bot.", ): - validate_webhook_signature(request, payload, signature) + validate_webhook_signature(request, webhook_bot, config=config) + + def test_compute_webhook_signature_formatter_and_prefix(self) -> None: + # Tests the custom_formatter + config_formatter = WebhookSignatureConfig( + integration_name="test", + header="X-Test-Signature", + custom_formatter=lambda d: f"sha256={d.upper()}", + ) + sig_formatter = compute_webhook_signature(b"secret", b"payload", config_formatter) + self.assertTrue(sig_formatter.startswith("sha256=")) + + # Tests prefix + config_prefix = WebhookSignatureConfig( + integration_name="test", + header="X-Test-Signature", + prefix="sha256=", + ) + sig_prefix = compute_webhook_signature(b"secret", b"payload", config_prefix) + self.assertTrue(sig_prefix.startswith("sha256=")) + + config_default = WebhookSignatureConfig( + integration_name="test", + header="X-Test-Signature", + ) + sig_default = compute_webhook_signature(b"secret", b"payload", config_default) + self.assertFalse(sig_default.startswith("sha256=")) def test_check_send_webhook_message_returns_id(self) -> None: webhook_bot = get_user("webhook-bot@zulip.com", get_realm("zulip")) diff --git a/zerver/webhooks/github/tests.py b/zerver/webhooks/github/tests.py index 90c563f0e2db2..002748bdab0fb 100644 --- a/zerver/webhooks/github/tests.py +++ b/zerver/webhooks/github/tests.py @@ -1,7 +1,9 @@ from unittest.mock import patch import orjson +from django.test import override_settings +from zerver.lib.bot_config import set_bot_config from zerver.lib.message import truncate_topic from zerver.lib.test_classes import WebhookTestCase from zerver.lib.webhooks.git import COMMITS_LIMIT @@ -22,6 +24,8 @@ class GitHubWebhookTest(WebhookTestCase): + WEBHOOK_TEST_SECRET = "testingthis" + def test_ping_event(self) -> None: expected_message = "GitHub webhook has been successfully configured by TomaszKolek." self.check_webhook("ping", TOPIC_REPO, expected_message) @@ -851,6 +855,52 @@ def test_issue_comment_silent_mention_with_multiple_matches(self) -> None: expected_message = "baxterthehacker [commented](https://github.com/baxterthehacker/public-repo/issues/2#issuecomment-99262140) on [issue #2](https://github.com/baxterthehacker/public-repo/issues/2):\n\n``` quote\nYou are totally right! I'll get this fixed right away.\n```" self.check_webhook("issue_comment", TOPIC_ISSUE, expected_message) + def test_github_webhook_bad_signature(self) -> None: + with override_settings(VERIFY_WEBHOOK_SIGNATURES=True): + url = self.build_webhook_url() + set_bot_config(self.test_user, "github:webhook_secret_token", self.WEBHOOK_TEST_SECRET) + + result = self.client_post( + url, + self.get_payload("ping"), + content_type="application/json", + HTTP_X_HUB_SIGNATURE_256="sha256=completely_invalid_hash_value", + ) + self.assert_json_error(result, "Webhook signature verification failed.") + + def test_github_webhook_signature_disabled_skips_validation(self) -> None: + """Verifies that when VERIFY_WEBHOOK_SIGNATURES is explicitly disabled, + requests pass through even if the signature value is completely bogus. + """ + self.VERIFY_WEBHOOK_SIGNATURES = False + try: + expected_message = "GitHub webhook has been successfully configured by TomaszKolek." + self.check_webhook( + "ping", + TOPIC_REPO, + expected_message, + HTTP_X_HUB_SIGNATURE_256="sha256=invalid_hash", + ) + finally: + self.VERIFY_WEBHOOK_SIGNATURES = True + + def test_github_webhook_valid_signature_success(self) -> None: + """Verifies that a mathematically correct HMAC signature passes + cleanly when verification enforcement is active.""" + expected_message = "GitHub webhook has been successfully configured by TomaszKolek." + self.check_webhook("ping", TOPIC_REPO, expected_message) + + def test_github_webhook_missing_secret(self) -> None: + """Verifies that if no webhook secret is configured for the bot, + the request is processed normally without requiring signature verification.""" + self.WEBHOOK_TEST_SECRET = None # type: ignore[assignment] # Allows testing missing secrets + try: + set_bot_config(self.test_user, "github:webhook_secret_token", "") + expected_message = "GitHub webhook has been successfully configured by TomaszKolek." + self.check_webhook("ping", TOPIC_REPO, expected_message) + finally: + self.WEBHOOK_TEST_SECRET = "testingthis" + class GitHubSponsorsHookTests(WebhookTestCase): URL_TEMPLATE = "/api/v1/external/githubsponsors?stream={stream}&api_key={api_key}" diff --git a/zerver/webhooks/github/view.py b/zerver/webhooks/github/view.py index c1a13b5ad0daf..446f2b5b21e3c 100644 --- a/zerver/webhooks/github/view.py +++ b/zerver/webhooks/github/view.py @@ -9,6 +9,7 @@ from zerver.decorator import log_unsupported_webhook_event, webhook_view from zerver.lib.exceptions import UnsupportedWebhookEventTypeError from zerver.lib.external_accounts import DEFAULT_EXTERNAL_ACCOUNTS +from zerver.lib.integrations import WEBHOOK_SIGNATURE_CONFIGS from zerver.lib.markdown.fenced_code import get_unused_fence from zerver.lib.mention import silent_mention_syntax_for_user from zerver.lib.partial import partial @@ -1196,7 +1197,12 @@ def get_topic_based_on_type(payload: WildValue, event: str) -> str: ALL_EVENT_TYPES = list(EVENT_FUNCTION_MAPPER.keys()) -@webhook_view("GitHub", notify_bot_owner_on_invalid_json=True, all_event_types=ALL_EVENT_TYPES) +@webhook_view( + "GitHub", + notify_bot_owner_on_invalid_json=True, + all_event_types=ALL_EVENT_TYPES, + signature_config=WEBHOOK_SIGNATURE_CONFIGS["github"], +) @typed_endpoint def api_github_webhook( request: HttpRequest,