Skip to content

Commit d09af5b

Browse files
authored
fix: whitelist GitHub web-flow in committer identity check (#1120)
## Problem The security committer identity check (`run_security_committer_identity`) produces false positives when a PR is rebased via the GitHub web UI. GitHub sets the commit **committer** to `web-flow` (its internal bot account, user ID 19864447), while preserving the original **author**. The check compares `last_commit.committer.login` against `pull_request.user.login`, so it flags `web-flow != <pr-author>` as a security mismatch. Same false positive triggers for: - GitHub UI "Update branch" button (merge or rebase) - GitHub UI commit squash - GitHub UI file edits ## Fix - Add `GITHUB_WEB_FLOW_LOGIN` constant for the `web-flow` system account - When last committer is `web-flow`, pass the identity check with an informational message - Added test for web-flow committer scenario Closes #1119
1 parent 79427a2 commit d09af5b

4 files changed

Lines changed: 128 additions & 1 deletion

File tree

webhook_server/libs/github_api.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging.
118118
self.repository_full_name: str = hook_data["repository"]["full_name"]
119119
self._bg_tasks: set[Task[Any]] = set()
120120
self.parent_committer: str = ""
121+
self.last_committer: str = "unknown"
122+
self.last_author: str = "unknown"
121123
self.pr_base_sha: str = ""
122124
self.pr_head_sha: str = ""
123125
self.x_github_delivery: str = headers.get("X-GitHub-Delivery", "")
@@ -663,7 +665,16 @@ async def process(self) -> Any:
663665

664666
self.last_commit = await self._get_last_commit(pull_request=pull_request)
665667
self.parent_committer = pull_request.user.login
666-
self.last_committer = getattr(self.last_commit.committer, "login", "unknown")
668+
self.last_committer = await github_api_call(
669+
lambda: getattr(self.last_commit.committer, "login", "unknown"),
670+
logger=self.logger,
671+
log_prefix=self.log_prefix,
672+
)
673+
self.last_author = await github_api_call(
674+
lambda: getattr(self.last_commit.author, "login", "unknown"),
675+
logger=self.logger,
676+
log_prefix=self.log_prefix,
677+
)
667678

668679
# Store PR SHAs: prefer webhook payload (avoids race condition with live API)
669680
# For pull_request events, base.sha and head.sha are guaranteed by GitHub webhook spec.

webhook_server/libs/handlers/runner_handler.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
BUILD_CONTAINER_STR,
2626
CHERRY_PICKED_LABEL,
2727
CONVENTIONAL_TITLE_STR,
28+
GITHUB_WEB_FLOW_LOGIN,
2829
PRE_COMMIT_STR,
2930
PREK_STR,
3031
PYTHON_MODULE_INSTALL_STR,
@@ -404,6 +405,67 @@ async def run_security_committer_identity(self) -> None:
404405
f"PR author={parent_committer}, last committer has no GitHub user"
405406
)
406407
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
408+
elif last_committer == GITHUB_WEB_FLOW_LOGIN:
409+
last_author = self.github_webhook.last_author
410+
if last_author == parent_committer:
411+
self.logger.debug(
412+
f"{self.log_prefix} Last committer is '{GITHUB_WEB_FLOW_LOGIN}' "
413+
f"(GitHub web UI operation), author '{last_author}' matches PR author "
414+
f"— passing committer identity check"
415+
)
416+
output = {
417+
"title": "Security: Committer Identity",
418+
"summary": "Committer identity verified (GitHub web-flow)",
419+
"text": (
420+
f"## Committer Identity Check\n\n"
421+
f"**PR author:** `{parent_committer}`\n"
422+
f"**Last commit committer:** `{last_committer}`\n"
423+
f"**Last commit author:** `{last_author}`\n\n"
424+
f"The last commit was made via the GitHub web UI (rebase, merge, or edit). "
425+
f"The `web-flow` committer is GitHub's internal account for web-based operations. "
426+
f"The commit author matches the PR author."
427+
),
428+
}
429+
await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
430+
elif last_author == "unknown":
431+
self.logger.warning(
432+
f"{self.log_prefix} Web-flow commit with unknown author: "
433+
f"PR author={parent_committer}, committer={last_committer}, author=unknown"
434+
)
435+
output = {
436+
"title": "\u274c Security: Committer Identity Unknown (web-flow)",
437+
"summary": "Web-flow commit author could not be verified",
438+
"text": (
439+
f"## Committer Identity Check\n\n"
440+
f"**PR author:** `{parent_committer}`\n"
441+
f"**Last commit committer:** `{last_committer}`\n"
442+
f"**Last commit author:** unknown\n\n"
443+
f"The last commit was made via the GitHub web UI, but the commit author "
444+
f"could not be determined. Please verify the commit authorship before merging."
445+
),
446+
}
447+
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
448+
else:
449+
self.logger.warning(
450+
f"{self.log_prefix} Web-flow commit author mismatch: "
451+
f"PR author={parent_committer}, committer={last_committer}, author={last_author}"
452+
)
453+
output = {
454+
"title": "\u274c Security: Committer Identity Mismatch (web-flow)",
455+
"summary": (
456+
f"Web-flow commit author '{last_author}' differs from PR author '{parent_committer}'"
457+
),
458+
"text": (
459+
f"## Committer Identity Check\n\n"
460+
f"**PR author:** `{parent_committer}`\n"
461+
f"**Last commit committer:** `{last_committer}`\n"
462+
f"**Last commit author:** `{last_author}`\n\n"
463+
f"The last commit was made via the GitHub web UI by a different user than the PR author. "
464+
f"This may indicate an unexpected web-based change to the PR branch.\n\n"
465+
f"Please verify this is expected before merging."
466+
),
467+
}
468+
await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
407469
elif last_committer != parent_committer:
408470
output = {
409471
"title": "\u274c Security: Committer Identity Mismatch",

webhook_server/tests/test_security_checks.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
BUILTIN_CHECK_NAMES,
2222
COMMAND_SECURITY_OVERRIDE_STR,
2323
DEFAULT_SUSPICIOUS_PATHS,
24+
GITHUB_WEB_FLOW_LOGIN,
2425
SECURITY_COMMITTER_IDENTITY_STR,
2526
SECURITY_SUSPICIOUS_PATHS_STR,
2627
)
@@ -287,6 +288,58 @@ async def test_committer_identity_unknown(self, runner_handler: RunnerHandler) -
287288
assert "could not be verified" in output["summary"]
288289
assert "no associated GitHub user" in output["text"]
289290

291+
@pytest.mark.asyncio
292+
async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) -> None:
293+
"""Check passes when last committer is GitHub's web-flow bot."""
294+
runner_handler.github_webhook.parent_committer = "legit-user"
295+
runner_handler.github_webhook.last_committer = GITHUB_WEB_FLOW_LOGIN
296+
runner_handler.github_webhook.last_author = "legit-user"
297+
298+
with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress:
299+
with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success:
300+
await runner_handler.run_security_committer_identity()
301+
302+
mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR)
303+
mock_success.assert_called_once()
304+
call_args = mock_success.call_args
305+
assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
306+
assert "web-flow" in call_args.kwargs["output"]["summary"]
307+
assert GITHUB_WEB_FLOW_LOGIN in call_args.kwargs["output"]["text"]
308+
309+
@pytest.mark.asyncio
310+
async def test_committer_identity_web_flow_author_mismatch(self, runner_handler: RunnerHandler) -> None:
311+
"""Check fails when web-flow commit has different author than PR author."""
312+
runner_handler.github_webhook.parent_committer = "legit-user"
313+
runner_handler.github_webhook.last_committer = GITHUB_WEB_FLOW_LOGIN
314+
runner_handler.github_webhook.last_author = "other-user"
315+
316+
with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress:
317+
with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure:
318+
await runner_handler.run_security_committer_identity()
319+
320+
mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR)
321+
mock_failure.assert_called_once()
322+
call_args = mock_failure.call_args
323+
assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
324+
assert "other-user" in call_args.kwargs["output"]["summary"]
325+
326+
@pytest.mark.asyncio
327+
async def test_committer_identity_web_flow_unknown_author(self, runner_handler: RunnerHandler) -> None:
328+
"""Check fails when web-flow commit has unknown author."""
329+
runner_handler.github_webhook.parent_committer = "legit-user"
330+
runner_handler.github_webhook.last_committer = GITHUB_WEB_FLOW_LOGIN
331+
runner_handler.github_webhook.last_author = "unknown"
332+
333+
with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress:
334+
with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure:
335+
await runner_handler.run_security_committer_identity()
336+
337+
mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR)
338+
mock_failure.assert_called_once()
339+
call_args = mock_failure.call_args
340+
assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR
341+
assert "could not be verified" in call_args.kwargs["output"]["summary"]
342+
290343
@pytest.mark.asyncio
291344
async def test_committer_identity_check_disabled(self, runner_handler: RunnerHandler) -> None:
292345
"""Check is skipped when committer-identity-check is disabled."""

webhook_server/utils/constants.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
CONVENTIONAL_TITLE_STR: str = "conventional-title"
1919
SECURITY_SUSPICIOUS_PATHS_STR: str = "security-suspicious-paths"
2020
SECURITY_COMMITTER_IDENTITY_STR: str = "security-committer-identity"
21+
GITHUB_WEB_FLOW_LOGIN: str = "web-flow"
2122
COMMAND_SECURITY_OVERRIDE_STR: str = "security-override"
2223
WIP_STR: str = "wip"
2324
LGTM_STR: str = "lgtm"

0 commit comments

Comments
 (0)