[IMP] runbot: hooks'n'stuff

runbot/controllers/hook.py

@@ -3,19 +3,46 @@
 import time
 import json
 import logging
+import hashlib
+import hmac
+
+from werkzeug.exceptions import BadRequest
 
 from odoo import http
 from odoo.http import request
 
 _logger = logging.getLogger(__name__)
 
 
+def verify_signature(payload_body, remote, signature_header):
+    """Verify that the payload was sent from GitHub by validating SHA256.
+
+    Raise and return 403 if not authorized.
+
+    Args:
+        payload_body: original request body to verify (request.body())
+        remote: runbot.remote
+        signature_header: header received from GitHub (x-hub-signature-256)
+    """
+    if not remote.webhook_secret:
+        return
+    if not signature_header:
+        _logger.info('Received payload without signature header')
+        raise BadRequest(description="x-hub-signature-256 header is missing!")
+    hash_object = hmac.new(remote.webhook_secret.encode('utf-8'), msg=payload_body, digestmod=hashlib.sha256)
+    expected_signature = "sha256=" + hash_object.hexdigest()
+    if not hmac.compare_digest(expected_signature, signature_header):
+        _logger.info('Received payload with invalid signature for remote %s', remote.name)
+        raise BadRequest(description="Request signatures didn't match!")
+
+
 class Hook(http.Controller):
 
     @http.route(['/runbot/hook', '/runbot/hook/<int:remote_id>'], type='http', auth="public", website=True, csrf=False, sitemap=False)
     def hook(self, remote_id=None, **_post):
         event = request.httprequest.headers.get("X-Github-Event")
-        payload = json.loads(request.params.get('payload', '{}'))
+        payload_str = request.params.get('payload', '{}')
+        payload = json.loads(payload_str)
         if remote_id is None:
             repo_data = payload.get('repository')
             if repo_data:
@@ -31,22 +58,21 @@ def hook(self, remote_id=None, **_post):
                 remote_id = remote.id
                 if not remote_id:
                     _logger.error("Remote %s not found", repo_data['ssh_url'])
-        remote = request.env['runbot.remote'].sudo().browse(remote_id)
+        remote = request.env['runbot.remote'].sudo().browse(remote_id).exists()
+        if not remote:
+            raise BadRequest(description='Invalid remote')
+        verify_signature(
+            payload_str.encode('utf-8'), remote.webhook_secret,
+            request.httprequest.headers.get('X-Hub-Signature-256')
+        )
 
         # force update of dependencies too in case a hook is lost
         if not payload or event == 'push':
             remote.repo_id._set_hook_time(time.time())
-        elif event == 'pull_request':
-            pr_number = payload.get('pull_request', {}).get('number', '')
-            branch = request.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', pr_number)])
-            branch._recompute_infos(payload.get('pull_request', {}))
-            if payload.get('action') in ('synchronize', 'opened', 'reopened'):
-                remote.repo_id._set_hook_time(time.time())
-            # remaining recurrent actions: labeled, review_requested, review_request_removed
-        elif event == 'delete':
-            if payload.get('ref_type') == 'branch':
-                branch_ref = payload.get('ref')
-                _logger.info('Branch %s in repo %s was deleted', branch_ref, remote.repo_id.name)
-                branch = request.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', branch_ref)])
-                branch.alive = False
+        else:
+            request.env['runbot.repo.hook.payload'].sudo().create({
+                'remote_id': remote.id,
+                'payload': payload,
+                'event': event,
+            })
         return ""

runbot/models/branch.py

@@ -87,6 +87,29 @@ def _compute_reference_name(self):
                 reference_name = f'{forced_version.name}---{reference_name}'
             branch.reference_name = reference_name
 
+    def _match_errors_from_body(self):
+        """
+        Checks the pr_body for any reference to runbot errors and links the branch
+        to those errors if they exist.
+        """
+        self.ensure_one()
+        if not self.pr_body or not self.pull_head_remote_id:
+            return
+        pr_body_regexes = [
+            r'^(?:.*runbot(?:.build)?.error(?:.id)?[ :\-~]+(\d+))\r?$',
+            r'^(?:.*https://[\w\.]+/odoo/[\w\.-]*error/(\d+)\/?(?:\).*)?)\r?$',
+            ]
+        error_ids = []
+        for pr_body_regex in pr_body_regexes:
+            error_ids += re.findall(pr_body_regex, self.pr_body, re.IGNORECASE | re.MULTILINE)
+        # We need to check if they exist and search does nothing if error_ids is empty
+        errors = self.env['runbot.build.error'].sudo().search([
+            ('id', 'in', error_ids), ('fixing_pr_id', '=', False),
+        ])
+        errors.fixing_pr_id = self
+        for error in errors:
+            error.message_post(body='Fixing pr automatically set through PR message')
+
     def _update_branch_infos(self, pull_info=None):
         """compute branch_url, pull_head_name and target_branch_name based on name"""
         name_to_remote = {}
@@ -131,6 +154,7 @@ def _update_branch_infos(self, pull_info=None):
                                 owner, repo_name = pull_head_repo_name.split('/')
                                 name_to_remote[pull_head_repo_name] = self.env['runbot.remote'].search([('owner', '=', owner), ('repo_name', '=', repo_name)], limit=1)
                             branch.pull_head_remote_id = name_to_remote[pull_head_repo_name]
+                        branch._match_errors_from_body()
                     except (TypeError, AttributeError):
                         _logger.exception('Error for pr %s using pull_info %s', branch.name, pi)
                         raise

runbot/models/repo.py

@@ -221,6 +221,7 @@ class Remote(models.Model):
     send_status = fields.Boolean('Send status', default=False, tracking=True)
 
     token = fields.Char("Github token", groups="runbot.group_runbot_admin")
+    webhook_secret = fields.Char("Webhook secret", groups="runbot.group_runbot_admin")
 
     @api.depends('name')
     def _compute_base_infos(self):
@@ -762,3 +763,41 @@ class HookTime(models.Model):
 
     time = fields.Float('Time')
     repo_id = fields.Many2one('runbot.repo', 'Repository', required=True, ondelete='cascade')
+
+
+class HookPayload(models.Model):
+    _name = 'runbot.repo.hook.payload'
+    _description = "Repo hook payloads"
+    _log_access = False
+
+    create_date = fields.Datetime(default=fields.Datetime.now, required=True)
+    remote_id = fields.Many2one('runbot.remote', required=True, ondelete='cascade')
+    event = fields.Char(required=True)
+    payload = JsonDictField(required=True)
+
+    @api.model
+    def _process_all(self):
+        create_date_limit = self.env.cr.now()
+        while (records := self.search([('create_date', '<=', create_date_limit)], limit=1000)):
+            for record in records:
+                record._process_one()
+            records.unlink()
+            self.env.cr.commit()
+
+    def _process_one(self):
+        self.ensure_one()
+        payload = self.payload.dict
+        remote = self.remote_id
+        if self.event == 'pull_request':
+            pr_number = payload.get('pull_request', {}).get('number', '')
+            branch = self.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', pr_number)])
+            branch._recompute_infos(payload.get('pull_request', {}))
+            if payload.get('action') in ('synchronize', 'opened', 'reopened'):
+                remote.repo_id._set_hook_time(time.time())
+            # remaining recurrent actions: labeled, review_requested, review_request_removed
+        elif self.event == 'delete':
+            if payload.get('ref_type') == 'branch':
+                branch_ref = payload.get('ref')
+                _logger.info('Branch %s in repo %s was deleted', branch_ref, remote.repo_id.name)
+                branch = self.env['runbot.branch'].sudo().search([('remote_id', '=', remote.id), ('name', '=', branch_ref)])
+                branch.alive = False

runbot/models/runbot.py

@@ -245,6 +245,8 @@ def _cron(self):
 
     def _fetch_loop_turn(self, host, pull_info_failures, default_sleep=1):
         with self._manage_host_exception(host) as manager:
+            self.env['runbot.repo.hook.payload']._process_all()
+
             repos = self.env['runbot.repo'].search([('mode', '!=', 'disabled')])
             processing_batch = self.env['runbot.batch'].search([('state', 'in', ('preparing', 'ready'))], order='id asc')
             preparing_batch = processing_batch.filtered(lambda b: b.state == 'preparing')

runbot/security/ir.model.access.csv

@@ -64,6 +64,9 @@ access_runbot_error_log_manager,runbot_error_log_manager,runbot.model_runbot_err
 access_runbot_repo_hooktime,runbot_repo_hooktime,runbot.model_runbot_repo_hooktime,group_user,1,0,0,0
 access_runbot_repo_referencetime,runbot_repo_referencetime,runbot.model_runbot_repo_reftime,group_user,1,0,0,0
 
+access_runbot_repo_payload_user,runbot_repo_payload_user,runbot.model_runbot_repo_hook_payload,group_user,0,0,0,0
+access_runbot_repo_payload_admin,runbot_repo_payload_admin,runbot.model_runbot_repo_hook_payload,runbot.group_runbot_admin,1,1,1,1
+
 access_runbot_build_stat_user,runbot_build_stat_user,runbot.model_runbot_build_stat,group_user,1,0,0,0
 access_runbot_build_stat_admin,runbot_build_stat_admin,runbot.model_runbot_build_stat,runbot.group_runbot_admin,1,1,1,1
 

runbot/tests/test_branch.py

@@ -65,6 +65,66 @@ def test_branch_dname_search(self):
             self.Branch.search([('dname', '=', branch.dname)]),
         )
 
+    def test_automatic_match_with_error(self):
+        self.env['runbot.build.error'].search([]).active = False
+        another_unrelated_error = self.env['runbot.build.error'].create([{}])
+        error = self.env['runbot.build.error'].create([{}])
+        branch = self.Branch.create({
+            'name': '989898',
+            'remote_id': self.remote_server.id,
+            'is_pr': True,
+            'pull_head_remote_id': self.remote_server.id,
+        })
+        # Does not crash without pr_body or nothing on pr_body
+        branch._match_errors_from_body()
+        branch.pr_body = ''
+        branch._match_errors_from_body()
+        # Does not link unknown errors
+        branch.pr_body = f'This is an error {error.id}'
+        branch._match_errors_from_body()
+        self.assertFalse(error.fixing_pr_id)
+        # Test multiple formats
+        self.assertNotEqual(error.id, 123, "Error should not be specific action id")
+        for format in [
+            f"Runbot error \n{another_unrelated_error.id}",
+            f"[Runbot error](https://domain.com/runbot/build/{another_unrelated_error.id})",
+        ]:
+            error.fixing_pr_id = False
+            branch.pr_body = format
+            branch._match_errors_from_body()
+            self.assertEqual(another_unrelated_error.fixing_pr_id.id, False, f"{format!r} should not match the error {error.id}")
+
+        for format in [
+            f"Runbot error {error.id}",
+            f"runbot error {error.id}",
+            f"runbot-error-{error.id}",
+            f"runbot build error {error.id}",
+            f"runbot-error-id~{error.id}",
+            f"https://domain.com/odoo/runbot.build.error/{error.id}",
+            f"https://domain.com/odoo/runbot-error/{error.id}/",
+            f"[runbot_error {error.id}](https://runbot.odoo.com/odoo/error/{error.id})",
+            f"[runbot_error](https://runbot.odoo.com/odoo/error/{error.id})",
+            f"Runbot Errors:\r\nhttps://runbot.odoo.com/odoo/error/{error.id}\r\nhttps://runbot.odoo.com/odoo/error/849849848'"
+            # f"Runbot error: https://domain.com/odoo/action-{another_unrelated_error.id}/{error.id}",
+        ]:
+            error.fixing_pr_id = False
+            branch.pr_body = format
+            branch._match_errors_from_body()
+            self.assertNotEqual(another_unrelated_error.fixing_pr_id.id, branch.id, f"'{format}' should not match the error {error.id}")
+            self.assertEqual(error.fixing_pr_id.id, branch.id, f"{format!r} should match the error {error.id}")
+
+        # Test that it does not reassign
+        error.fixing_pr_id = False
+        branch.pr_body = f'Runbot error {error.id}'
+        other_branch = self.Branch.create({
+            'name': 'other-branch',
+            'remote_id': self.remote_server.id,
+            'is_pr': True,
+        })
+        error.fixing_pr_id = other_branch
+        branch._match_errors_from_body()
+        self.assertEqual(error.fixing_pr_id, other_branch)
+
 class TestBranchRelations(RunbotCase):
 
     def setUp(self):

runbot/tests/test_repo.py

@@ -3,15 +3,20 @@
 import logging
 import time
 import re
+import hmac
+import hashlib
 
 from unittest import skip
 from unittest.mock import patch, Mock
 from subprocess import CalledProcessError
+from werkzeug.exceptions import HTTPException
 
 from odoo import fields
 from odoo.tests import common, TransactionCase
 from odoo.tools import mute_logger
 
+from odoo.addons.runbot.controllers.hook import verify_signature
+
 from .common import RunbotCase, RunbotCaseMinimalSetup
 
 _logger = logging.getLogger(__name__)
@@ -364,6 +369,31 @@ def test_github(self):
 
             self.assertEqual(2, mock_session.return_value.post.call_count, "_github method should try two times by default")
 
+    def test_verify_signature(self):
+        project = self.env['runbot.project'].create({'name': 'Tests'})
+        repo_server = self.env['runbot.repo'].create({
+            'name': 'server',
+            'project_id': project.id,
+        })
+        remote = self.env['runbot.remote'].create({
+            'name': '[email protected]:base/server',
+            'repo_id': repo_server.id,
+        })
+        # Should not raise -> no secret on remote
+        payload_body = b'{"payload": "data"}'
+        verify_signature(payload_body, remote, '')
+        verify_signature(payload_body, remote, None)
+        # Should not raise, valid signature
+        remote.webhook_secret = 'IAMRUNBOT'
+        signature_header = f'sha256={hmac.new(remote.webhook_secret.encode("utf-8"), msg=payload_body, digestmod=hashlib.sha256).hexdigest()}'
+        verify_signature(payload_body, remote, signature_header)
+        # Should raise invalid signature
+        with self.assertRaises(HTTPException):
+            verify_signature(payload_body, remote, 'invalid header')
+        # Should raise if no signature and webhook_secret is set
+        with self.assertRaises(HTTPException):
+            verify_signature(payload_body, remote, None)
+
 
 class TestFetch(RunbotCase):
 

runbot/views/branch_views.xml

@@ -21,7 +21,7 @@
                 <field name="head"/>
                 <field name="alive"/>
                 <field name="pr_title"/>
-                <field name="pr_body"/>
+                <field name="pr_body" widget="text"/>
               </group>
             </sheet>
           </form>

runbot/views/build_error_views.xml

@@ -482,6 +482,7 @@
       <field name="path">error</field>
       <field name="view_mode">list,form</field>
       <field name="context">{'search_default_not_fixed_errors': True, 'active_test': False}</field>
+      <field name="path">runbot-error</field>
     </record>
 
     <record id="open_view_build_error_content_tree" model="ir.actions.act_window">

runbot/views/repo_views.xml

@@ -180,6 +180,7 @@
                       <field name="fetch_pull" string="PR"/>
                       <field name="send_status"/>
                       <field name="token" password="True"/>
+                      <field name="webhook_secret" password="True"/>
                     </list>
                   </field>
                 </group>
@@ -218,6 +219,7 @@
                 <field name="sequence"/>
                 <field name="repo_id"/>
                 <field name="token"/>
+                <field name="webhook_secret"/>
                 <field name="fetch_pull"/>
                 <field name="fetch_heads"/>
                 <field name="send_status"/>