This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch db/8607 in repository https://gitbox.apache.org/repos/asf/allura.git
commit 10b5ec9bd21751d6ad048803d4fcc20d0774638a Author: Dave Brondsema <[email protected]> AuthorDate: Wed May 13 17:24:19 2026 -0400 [#8607] move webhook send from requests to urlopen so our NoInternal handlers run automatically including on redirects --- Allura/allura/tests/test_webhooks.py | 70 ++++++++++++++++++++---------------- Allura/allura/webhooks.py | 44 +++++++++++------------ 2 files changed, 61 insertions(+), 53 deletions(-) diff --git a/Allura/allura/tests/test_webhooks.py b/Allura/allura/tests/test_webhooks.py index 87213094b..43b4a2095 100644 --- a/Allura/allura/tests/test_webhooks.py +++ b/Allura/allura/tests/test_webhooks.py @@ -23,6 +23,8 @@ from mock import Mock, MagicMock, patch, call import pytest from formencode import Invalid + +from allura.tests.test_helpers import httpbin_domain from ming.odm import session from tg import tmpl_context as c from tg import config @@ -461,9 +463,10 @@ def test_log_msg(self): self.h.log_msg('OK') == 'OK: repo-push http://httpbin.org/post /adobe/adobe-1/src/') response = Mock( - status_code=500, - text='that is why', + status=500, + reason='that is why', headers={'Content-Type': 'application/json'}) + response.read.return_value = b'that is why' assert ( self.h.log_msg('Error', response=response) == "Error: repo-push http://httpbin.org/post /adobe/adobe-1/src/ 500 " @@ -474,31 +477,34 @@ def test_send_webhook_task(self, swh): send_webhook(self.wh._id, self.payload) swh.assert_called_once_with(self.wh, self.payload) - @patch('allura.webhooks.requests', autospec=True) + @patch('urllib.request.Request', autospec=True) + @patch('urllib.request.urlopen', autospec=True) @patch('allura.webhooks.log', autospec=True) - def test_send(self, log, requests): - requests.post.return_value = Mock(status_code=200) + def test_send(self, log, urlopen, Request): + urlopen.return_value = Mock(status=200) self.h.sign = Mock(return_value='sha1=abc') self.h.send() headers = {'content-type': 'application/json', 'User-Agent': 'Allura Webhook (https://allura.apache.org/)', 'X-Allura-Signature': 'sha1=abc'} - requests.post.assert_called_once_with( + Request.assert_called_once_with( self.wh.hook_url, - data=json.dumps(self.payload), + data=json.dumps(self.payload).encode('utf-8'), headers=headers, - timeout=30) + ) + urlopen.assert_called_once_with(Request.return_value, timeout=30) log.info.assert_called_once_with( 'Webhook successfully sent: {} {} {}'.format( self.wh.type, self.wh.hook_url, self.wh.app_config.url())) @patch('allura.webhooks.time', autospec=True) - @patch('allura.webhooks.requests', autospec=True) + @patch('urllib.request.urlopen', autospec=True) @patch('allura.webhooks.log', autospec=True) - def test_send_error_response_status(self, log, requests, time): - requests.post.return_value = Mock(status_code=500) + def test_send_error_response_status(self, log, urlopen, time): + urlopen.return_value = Mock(status=500, headers='Content-Type: text/plain') + urlopen.return_value.read.return_value = b'internal error' self.h.send() - assert requests.post.call_count == 4 # initial call + 3 retries + assert urlopen.call_count == 4 # initial call + 3 retries assert (time.sleep.call_args_list == [call(60), call(120), call(240)]) assert log.info.call_args_list == [ @@ -508,31 +514,30 @@ def test_send_error_response_status(self, log, requests, time): call('Retrying webhook in %s seconds', 240)] assert log.error.call_count == 4 log.error.assert_called_with( - 'Webhook send error: {} {} {} {} {} {}'.format( + "Webhook send error: {} {} {} 500 internal error Content-Type: text/plain".format( self.wh.type, self.wh.hook_url, - self.wh.app_config.url(), - requests.post.return_value.status_code, - requests.post.return_value.text, - requests.post.return_value.headers)) + self.wh.app_config.url() + ) + ) @patch('allura.webhooks.time', autospec=True) - @patch('allura.webhooks.requests', autospec=True) + @patch('urllib.request.urlopen', autospec=True) @patch('allura.webhooks.log', autospec=True) - def test_send_error_no_retries(self, log, requests, time): - requests.post.return_value = Mock(status_code=500) + def test_send_error_no_retries(self, log, urlopen, time): + urlopen.return_value = Mock(status=500, headers='Content-Type: text/plain') + urlopen.return_value.read.return_value = b'internal error' with h.push_config(config, **{'webhook.retry': ''}): self.h.send() - assert requests.post.call_count == 1 + assert urlopen.call_count == 1 assert time.call_count == 0 log.info.assert_called_once_with('Retrying webhook in: %s', []) assert log.error.call_count == 1 log.error.assert_called_with( - 'Webhook send error: {} {} {} {} {} {}'.format( + "Webhook send error: {} {} {} 500 internal error Content-Type: text/plain".format( self.wh.type, self.wh.hook_url, - self.wh.app_config.url(), - requests.post.return_value.status_code, - requests.post.return_value.text, - requests.post.return_value.headers)) + self.wh.app_config.url() + ) + ) @pytest.mark.parametrize('url', [ 'http://localhost/hook', @@ -540,13 +545,18 @@ def test_send_error_no_retries(self, log, requests, time): 'http://10.0.0.1/hook', 'http://192.168.1.1/hook', ]) - @patch('allura.webhooks.requests', autospec=True) @patch('allura.webhooks.log', autospec=True) - def test_send_ssrf_private_url_blocked(self, log, requests, url): + def test_send_ssrf_private_url_blocked(self, log, url): + result = self.h._send(url, json.dumps(self.payload), {}) + assert result is False + log.exception.assert_called_once() + + @patch('allura.webhooks.log', autospec=True) + def test_send_redir_private_url(self, log): + url = f'https://{httpbin_domain}/redirect-to?url=http://localhost' result = self.h._send(url, json.dumps(self.payload), {}) assert result is False - requests.post.assert_not_called() - log.error.assert_called_once() + log.exception.assert_called_once() class TestRepoPushWebhookSender(TestWebhookBase): diff --git a/Allura/allura/webhooks.py b/Allura/allura/webhooks.py index ed5a16ebb..c281316e5 100644 --- a/Allura/allura/webhooks.py +++ b/Allura/allura/webhooks.py @@ -22,8 +22,9 @@ import time import socket import ssl +import urllib.request +import urllib.error -import requests from bson import ObjectId from tg import expose, validate, redirect, flash, config from tg.decorators import with_trailing_slash, without_trailing_slash @@ -354,11 +355,15 @@ def log_msg(self, msg, response=None): self.webhook.hook_url, self.webhook.app_config.url()) if response is not None: - message = '{} {} {} {}'.format( - message, - response.status_code, - response.text, - response.headers) + try: + message = '{} {} {} {}'.format( + message, + response.status, + response.read(200).decode('utf-8', 'ignore'), + response.headers) + except Exception: + log.warning('could not get response details during webhook error logging', exc_info=True) + message = '{} {}'.format(message, response) return message def send(self): @@ -379,27 +384,20 @@ def send(self): def _send(self, url, data, headers) -> bool: try: - v.NonPrivateUrl().to_python(url) - except Invalid as e: - log.error(self.log_msg(f'Webhook send error: resolved to private address: {e!r}')) - return False - try: - r = requests.post( - url, - data=data, - headers=headers, - timeout=self.timeout) - except (requests.exceptions.RequestException, - socket.timeout, - ssl.SSLError): + req = urllib.request.Request(url, data=data.encode('utf-8'), headers=headers) # noqa: S310 + r = urllib.request.urlopen(req, timeout=self.timeout) # noqa: S310 + except urllib.error.HTTPError as e: + r = e + except (OSError, Invalid): log.exception(self.log_msg('Webhook send error')) return False - if r.status_code >= 200 and r.status_code < 300: + + if 200 <= r.status < 300: log.info(self.log_msg('Webhook successfully sent')) return True - else: - log.error(self.log_msg('Webhook send error', response=r)) - return False + + log.error(self.log_msg('Webhook send error', response=r)) + return False @task()
