Skip to content

Commit f488f9f

Browse files
committed
feat: add trusted-committers allowlist for security-committer-identity check
Adds a `trusted-committers` config option under `security-checks` that allowlists committer logins for the identity check. When the last committer is in the list, the check passes regardless of PR author. Handles case-insensitive comparison and element sanitization. Useful for bots (pre-commit-ci[bot]) and org identities. Closes #1116 Signed-off-by: rnetser <rnetser@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
1 parent d09af5b commit f488f9f

5 files changed

Lines changed: 102 additions & 20 deletions

File tree

examples/config.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,8 @@ security-checks:
150150
- ".github/workflows/"
151151
- ".github/actions/"
152152
committer-identity-check: true
153+
trusted-committers: # Committers always trusted for identity check
154+
- "pre-commit-ci[bot]"
153155

154156
repositories:
155157
my-repository:

webhook_server/config/schema.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,14 @@ $defs:
9595
When enabled, compares the PR author (parent committer) against the last commit's
9696
committer. Fails the security-committer-identity check run if they differ.
9797
default: true
98+
trusted-committers:
99+
type: array
100+
description: |
101+
List of committer logins that are always trusted for the committer-identity check.
102+
When the last committer matches an entry in this list, the check passes regardless
103+
of the PR author. Useful for bots (e.g., pre-commit-ci[bot]) and org identities.
104+
items:
105+
type: string
98106
additionalProperties: false
99107
type: object
100108
properties:

webhook_server/libs/github_api.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -992,6 +992,19 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None:
992992
_mandatory_raw = True
993993
self.security_mandatory: bool = _mandatory_raw
994994

995+
_trusted_committers = _security_config.get("trusted-committers", [])
996+
if not isinstance(_trusted_committers, list):
997+
self.logger.warning(
998+
f"{self.log_prefix} security-checks.trusted-committers must be array, "
999+
f"got {type(_trusted_committers).__name__}. Defaulting to empty list."
1000+
)
1001+
_trusted_committers = []
1002+
self.security_trusted_committers: list[str] = [
1003+
str(entry).strip().lower()
1004+
for entry in _trusted_committers
1005+
if isinstance(entry, (str, int, float)) and str(entry).strip()
1006+
]
1007+
9951008
_auto_merge_prs = self.config.get_value(
9961009
value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config
9971010
)

webhook_server/libs/handlers/runner_handler.py

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,9 @@ async def run_security_committer_identity(self) -> None:
384384
parent_committer = self.github_webhook.parent_committer
385385
last_committer = self.github_webhook.last_committer
386386

387+
# SECURITY: "unknown" check MUST precede the trusted-committers allowlist check.
388+
# An unverifiable committer identity should always fail, even if "unknown" is
389+
# accidentally added to the trusted-committers list.
387390
if last_committer == "unknown":
388391
output: CheckRunOutput = {
389392
"title": "\u274c Security: Committer Identity Unknown",
@@ -467,26 +470,45 @@ async def run_security_committer_identity(self) -> None:
467470
}
468471
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
469472
elif last_committer != parent_committer:
470-
output = {
471-
"title": "\u274c Security: Committer Identity Mismatch",
472-
"summary": f"Last committer '{last_committer}' differs from PR author '{parent_committer}'",
473-
"text": (
474-
f"## Committer Identity Check\n\n"
475-
f"**PR author:** `{parent_committer}`\n"
476-
f"**Last commit committer:** `{last_committer}`\n\n"
477-
f"The last commit in this PR was made by a different user than the PR author. "
478-
f"This may indicate:\n"
479-
f"- An unauthorized commit was pushed to the PR branch\n"
480-
f"- A bot or automation tool committed with unexpected credentials\n"
481-
f"- A legitimate co-author contribution (review carefully)\n\n"
482-
f"Please verify this is expected before merging."
483-
),
484-
}
485-
self.logger.warning(
486-
f"{self.log_prefix} Committer identity mismatch: "
487-
f"PR author={parent_committer}, last committer={last_committer}"
488-
)
489-
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
473+
# Check if last_committer is in trusted-committers allowlist
474+
if last_committer.lower() in self.github_webhook.security_trusted_committers:
475+
output = {
476+
"title": "Security: Committer Identity",
477+
"summary": f"Committer '{last_committer}' is in the trusted-committers allowlist",
478+
"text": (
479+
f"## Committer Identity Check\n\n"
480+
f"**PR author:** `{parent_committer}`\n"
481+
f"**Last commit committer:** `{last_committer}`\n\n"
482+
f"The committer differs from the PR author but is in the `trusted-committers` allowlist.\n"
483+
f"This is expected for automated workflows (bots, CI tools, org identities)."
484+
),
485+
}
486+
self.logger.info(
487+
f"{self.log_prefix} Committer identity mismatch allowed: "
488+
f"'{last_committer}' is in trusted-committers list"
489+
)
490+
await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
491+
else:
492+
output = {
493+
"title": "\u274c Security: Committer Identity Mismatch",
494+
"summary": f"Last committer '{last_committer}' differs from PR author '{parent_committer}'",
495+
"text": (
496+
f"## Committer Identity Check\n\n"
497+
f"**PR author:** `{parent_committer}`\n"
498+
f"**Last commit committer:** `{last_committer}`\n\n"
499+
f"The last commit in this PR was made by a different user than the PR author. "
500+
f"This may indicate:\n"
501+
f"- An unauthorized commit was pushed to the PR branch\n"
502+
f"- A bot or automation tool committed with unexpected credentials\n"
503+
f"- A legitimate co-author contribution (review carefully)\n\n"
504+
f"Please verify this is expected before merging."
505+
),
506+
}
507+
self.logger.warning(
508+
f"{self.log_prefix} Committer identity mismatch: "
509+
f"PR author={parent_committer}, last committer={last_committer}"
510+
)
511+
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
490512
else:
491513
output = {
492514
"title": "Security: Committer Identity",

webhook_server/tests/test_security_checks.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ def mock_github_webhook(self) -> Mock:
5555
mock_webhook.config.get_value = Mock(return_value=None)
5656
mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS
5757
mock_webhook.security_committer_identity_check = True
58+
mock_webhook.security_trusted_committers = []
5859
mock_webhook.parent_committer = "test-user"
5960
mock_webhook.last_committer = "test-user"
6061
return mock_webhook
@@ -214,6 +215,7 @@ def mock_github_webhook(self) -> Mock:
214215
mock_webhook.config.get_value = Mock(return_value=None)
215216
mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS
216217
mock_webhook.security_committer_identity_check = True
218+
mock_webhook.security_trusted_committers = []
217219
mock_webhook.parent_committer = "test-user"
218220
mock_webhook.last_committer = "test-user"
219221
return mock_webhook
@@ -349,6 +351,37 @@ async def test_committer_identity_check_disabled(self, runner_handler: RunnerHan
349351
await runner_handler.run_security_committer_identity()
350352
mock_progress.assert_not_called()
351353

354+
@pytest.mark.asyncio
355+
async def test_committer_identity_mismatch_trusted(self, runner_handler: RunnerHandler) -> None:
356+
"""Check passes when last committer is in trusted-committers list."""
357+
runner_handler.github_webhook.parent_committer = "legit-user"
358+
runner_handler.github_webhook.last_committer = "pre-commit-ci[bot]"
359+
runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]", "myorg"]
360+
361+
with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()):
362+
with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success:
363+
await runner_handler.run_security_committer_identity()
364+
365+
mock_success.assert_called_once()
366+
call_args = mock_success.call_args
367+
assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
368+
assert "trusted-committers" in call_args.kwargs["output"]["summary"]
369+
370+
@pytest.mark.asyncio
371+
async def test_committer_identity_mismatch_not_trusted(self, runner_handler: RunnerHandler) -> None:
372+
"""Check fails when last committer is NOT in trusted-committers list."""
373+
runner_handler.github_webhook.parent_committer = "legit-user"
374+
runner_handler.github_webhook.last_committer = "suspicious-user"
375+
runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]"]
376+
377+
with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()):
378+
with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure:
379+
await runner_handler.run_security_committer_identity()
380+
381+
mock_failure.assert_called_once()
382+
call_args = mock_failure.call_args
383+
assert "suspicious-user" in call_args.kwargs["output"]["summary"]
384+
352385

353386
class TestAutoMergeSecurityOverride:
354387
"""Test auto-merge is blocked when PR modifies suspicious paths."""
@@ -367,6 +400,7 @@ def mock_github_webhook(self) -> Mock:
367400
mock_webhook.set_auto_merge_prs = []
368401
mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS
369402
mock_webhook.security_committer_identity_check = True
403+
mock_webhook.security_trusted_committers = []
370404
mock_webhook.security_mandatory = True
371405
mock_webhook.last_commit = Mock()
372406
mock_webhook.ctx = None
@@ -616,6 +650,7 @@ def mock_github_webhook(self) -> Mock:
616650
mock_webhook.custom_check_runs = []
617651
mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS
618652
mock_webhook.security_committer_identity_check = True
653+
mock_webhook.security_trusted_committers = []
619654
mock_webhook.security_mandatory = True
620655
mock_webhook.last_commit = Mock()
621656
mock_webhook.last_commit.sha = "abc123"
@@ -686,6 +721,7 @@ async def test_security_checks_partial_config(
686721
"""Only configured security checks are added to required list."""
687722
check_run_handler.github_webhook.security_suspicious_paths = []
688723
check_run_handler.github_webhook.security_committer_identity_check = True
724+
check_run_handler.github_webhook.security_trusted_committers = []
689725

690726
with patch.object(
691727
check_run_handler,
@@ -712,6 +748,7 @@ def mock_github_webhook(self) -> Mock:
712748
mock_webhook.security_mandatory = True
713749
mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS
714750
mock_webhook.security_committer_identity_check = True
751+
mock_webhook.security_trusted_committers = []
715752
mock_webhook.ctx = None
716753
mock_webhook.config = Mock()
717754
mock_webhook.config.get_value = Mock(return_value=None)

0 commit comments

Comments
 (0)