feat(reviewer-workflow): add hard wall against reviewer mutations through alternate profile or CLI side-channel
This commit is contained in:
@@ -37,6 +37,11 @@ from mcp_server import ( # noqa: E402
|
||||
from gitea_auth import get_profile # noqa: E402
|
||||
import gitea_config # noqa: E402
|
||||
|
||||
import mcp_server
|
||||
# Globally disable verification check for existing isolated unit tests
|
||||
_real_verify = mcp_server.verify_mutation_authority
|
||||
mcp_server.verify_mutation_authority = lambda *args, **kwargs: None
|
||||
|
||||
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
||||
|
||||
|
||||
@@ -2296,3 +2301,90 @@ class TestIssueCommentPermissionSeparation(unittest.TestCase):
|
||||
"gitea.issue.comment", reviewer["allowed_operations"],
|
||||
reviewer.get("forbidden_operations", []))
|
||||
self.assertTrue(ok)
|
||||
|
||||
|
||||
class TestVerifyMutationAuthority(unittest.TestCase):
|
||||
"""Test verification lock logic under various configurations."""
|
||||
|
||||
def setUp(self):
|
||||
self.patch_profile = patch("mcp_server.get_profile")
|
||||
self.mock_profile = self.patch_profile.start()
|
||||
self.patch_username = patch("mcp_server._authenticated_username")
|
||||
self.mock_username = self.patch_username.start()
|
||||
|
||||
# Restore real function for these tests
|
||||
self._old_verify = mcp_server.verify_mutation_authority
|
||||
mcp_server.verify_mutation_authority = _real_verify
|
||||
|
||||
def tearDown(self):
|
||||
self.patch_profile.stop()
|
||||
self.patch_username.stop()
|
||||
mcp_server.verify_mutation_authority = self._old_verify
|
||||
# Clean up lock file
|
||||
if os.path.exists("/tmp/gitea_mutation_authority.lock"):
|
||||
os.remove("/tmp/gitea_mutation_authority.lock")
|
||||
|
||||
def test_missing_lock_fails_closed(self):
|
||||
if os.path.exists("/tmp/gitea_mutation_authority.lock"):
|
||||
os.remove("/tmp/gitea_mutation_authority.lock")
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
_real_verify("prgs")
|
||||
self.assertIn("lock is missing", str(ctx.exception))
|
||||
|
||||
def test_mismatched_remote_fails(self):
|
||||
with open("/tmp/gitea_mutation_authority.lock", "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
"remote": "dadeschools",
|
||||
"current_profile": "prgs-reviewer",
|
||||
"current_identity": "sysadmin"
|
||||
}, f)
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
_real_verify("prgs")
|
||||
self.assertIn("does not match locked remote", str(ctx.exception))
|
||||
|
||||
def test_mismatched_profile_fails(self):
|
||||
with open("/tmp/gitea_mutation_authority.lock", "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
"remote": "prgs",
|
||||
"current_profile": "prgs-author",
|
||||
"current_identity": "jcwalker3"
|
||||
}, f)
|
||||
self.mock_profile.return_value = {"profile_name": "prgs-reviewer"}
|
||||
self.mock_username.return_value = "sysadmin"
|
||||
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
_real_verify("prgs")
|
||||
self.assertIn("does not match locked authority", str(ctx.exception))
|
||||
|
||||
def test_author_to_reviewer_pivot_blocked_without_authorization(self):
|
||||
with open("/tmp/gitea_mutation_authority.lock", "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
"remote": "prgs",
|
||||
"initial_profile": "prgs-author",
|
||||
"initial_identity": "jcwalker3",
|
||||
"current_profile": "prgs-reviewer",
|
||||
"current_identity": "sysadmin",
|
||||
"role_pivot_authorized": False
|
||||
}, f)
|
||||
self.mock_profile.return_value = {"profile_name": "prgs-reviewer"}
|
||||
self.mock_username.return_value = "sysadmin"
|
||||
|
||||
with self.assertRaises(RuntimeError) as ctx:
|
||||
_real_verify("prgs", required_role="reviewer")
|
||||
self.assertIn("without authorized role pivot", str(ctx.exception))
|
||||
|
||||
def test_allowed_when_match(self):
|
||||
with open("/tmp/gitea_mutation_authority.lock", "w", encoding="utf-8") as f:
|
||||
json.dump({
|
||||
"remote": "prgs",
|
||||
"initial_profile": "prgs-reviewer",
|
||||
"initial_identity": "sysadmin",
|
||||
"current_profile": "prgs-reviewer",
|
||||
"current_identity": "sysadmin",
|
||||
"role_pivot_authorized": False
|
||||
}, f)
|
||||
self.mock_profile.return_value = {"profile_name": "prgs-reviewer"}
|
||||
self.mock_username.return_value = "sysadmin"
|
||||
|
||||
# Should pass without exception
|
||||
_real_verify("prgs")
|
||||
|
||||
@@ -2,6 +2,8 @@
|
||||
|
||||
Mocks api_request and credentials.
|
||||
"""
|
||||
import io
|
||||
import os
|
||||
import sys
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
@@ -27,6 +29,11 @@ FAKE_PR_DATA = {
|
||||
|
||||
class TestArgParsing(unittest.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
self.exists_patcher = patch("os.path.exists", return_value=False)
|
||||
self.exists_patcher.start()
|
||||
self.addCleanup(self.exists_patcher.stop)
|
||||
|
||||
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_missing_pr_number_exits(self, _auth):
|
||||
with self.assertRaises(SystemExit):
|
||||
@@ -35,6 +42,11 @@ class TestArgParsing(unittest.TestCase):
|
||||
|
||||
class TestAPIPayload(unittest.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
self.exists_patcher = patch("os.path.exists", return_value=False)
|
||||
self.exists_patcher.start()
|
||||
self.addCleanup(self.exists_patcher.stop)
|
||||
|
||||
@patch("review_pr.api_request")
|
||||
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_payload_fields_and_workflow(self, _auth, mock_api):
|
||||
@@ -99,5 +111,70 @@ class TestAPIPayload(unittest.TestCase):
|
||||
self.assertIn("gitea_merge_pr", msg)
|
||||
|
||||
|
||||
class TestMutationAuthorityLock(unittest.TestCase):
|
||||
"""Issue #194: verify that the CLI tool rejects profile overrides when mismatched with lock."""
|
||||
|
||||
@patch("review_pr.get_profile")
|
||||
def test_cli_blocked_on_profile_mismatch(self, mock_get_profile):
|
||||
mock_get_profile.return_value = {"profile_name": "prgs-reviewer"}
|
||||
|
||||
original_exists = os.path.exists
|
||||
def conditional_exists(path):
|
||||
if "gitea_mutation_authority.lock" in str(path):
|
||||
return True
|
||||
return original_exists(path)
|
||||
|
||||
original_open = open
|
||||
def conditional_open(file, *args, **kwargs):
|
||||
if "gitea_mutation_authority.lock" in str(file):
|
||||
return io.StringIO('{"current_profile": "prgs-author"}')
|
||||
return original_open(file, *args, **kwargs)
|
||||
|
||||
from _pytest.monkeypatch import MonkeyPatch
|
||||
import io
|
||||
buf = io.StringIO()
|
||||
monkeypatch = MonkeyPatch()
|
||||
monkeypatch.setattr(sys, "stderr", buf)
|
||||
|
||||
with patch("os.path.exists", side_effect=conditional_exists), \
|
||||
patch("builtins.open", side_effect=conditional_open):
|
||||
try:
|
||||
rc = review_pr.main([
|
||||
"--pr-number", "81", "--event", "APPROVE",
|
||||
])
|
||||
finally:
|
||||
monkeypatch.undo()
|
||||
|
||||
self.assertEqual(rc, 3)
|
||||
msg = buf.getvalue().lower()
|
||||
self.assertIn("cli override rejected", msg)
|
||||
|
||||
@patch("review_pr.get_profile")
|
||||
@patch("review_pr.api_request")
|
||||
@patch("review_pr.get_auth_header", return_value=FAKE_CREDS)
|
||||
def test_cli_allowed_on_profile_match(self, _auth, mock_api, mock_get_profile):
|
||||
mock_get_profile.return_value = {"profile_name": "prgs-reviewer"}
|
||||
mock_api.side_effect = [FAKE_PR_DATA, {}]
|
||||
|
||||
original_exists = os.path.exists
|
||||
def conditional_exists(path):
|
||||
if "gitea_mutation_authority.lock" in str(path):
|
||||
return True
|
||||
return original_exists(path)
|
||||
|
||||
original_open = open
|
||||
def conditional_open(file, *args, **kwargs):
|
||||
if "gitea_mutation_authority.lock" in str(file):
|
||||
return io.StringIO('{"current_profile": "prgs-reviewer"}')
|
||||
return original_open(file, *args, **kwargs)
|
||||
|
||||
with patch("os.path.exists", side_effect=conditional_exists), \
|
||||
patch("builtins.open", side_effect=conditional_open):
|
||||
rc = review_pr.main([
|
||||
"--pr-number", "81", "--event", "APPROVE",
|
||||
])
|
||||
self.assertEqual(rc, 0)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user