diff --git a/examples/config.yaml b/examples/config.yaml index 14d219499..cd77362f0 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -150,6 +150,8 @@ security-checks: - ".github/workflows/" - ".github/actions/" committer-identity-check: true + trusted-committers: # Additional trusted committers (GitHub App bot, web-flow, and API users are auto-included) + - "pre-commit-ci[bot]" repositories: my-repository: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index bfdf84129..301fecfb0 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -95,6 +95,16 @@ $defs: When enabled, compares the PR author (parent committer) against the last commit's committer. Fails the security-committer-identity check run if they differ. default: true + trusted-committers: + type: array + description: | + List of committer logins that are always trusted for the committer-identity check. + When the last committer matches an entry in this list, the check passes regardless + of the PR author. Useful for bots (e.g., pre-commit-ci[bot]) and org identities. + Note: The GitHub App bot, web-flow, and github-tokens API users are + automatically included — you only need to list additional external committers here. + items: + type: string additionalProperties: false type: object properties: diff --git a/webhook_server/docs/pr-flow-api.md b/webhook_server/docs/pr-flow-api.md index 79d42fcea..9f7f061c9 100644 --- a/webhook_server/docs/pr-flow-api.md +++ b/webhook_server/docs/pr-flow-api.md @@ -243,12 +243,14 @@ The PR Flow API integrates seamlessly with the log viewer system to provide: ```python import httpx + async def analyze_pr_workflow(hook_id: str, base_url: str = "http://192.168.10.44:5003") -> dict: async with httpx.AsyncClient() as client: # Example using production webhook server response = await client.get(f"{base_url}/logs/api/pr-flow/{hook_id}") return response.json() + # Usage workflow_data = await analyze_pr_workflow("f4b3c2d1-a9b8-4c5d-9e8f-1a2b3c4d5e6f") print(f"Workflow health: {workflow_data['performance_metrics']['workflow_health']}") diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 64f87853e..a1c9947c2 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -36,6 +36,7 @@ CONFIGURABLE_LABEL_CATEGORIES, CONVENTIONAL_TITLE_STR, DEFAULT_SUSPICIOUS_PATHS, + GITHUB_WEB_FLOW_LOGIN, OTHER_MAIN_BRANCH, PRE_COMMIT_STR, PYTHON_MODULE_INSTALL_STR, @@ -504,7 +505,7 @@ async def _recheck_merge_eligibility(self, pull_request: PullRequest) -> None: async def process(self) -> Any: # Early exit for pull_request_review_thread events that don't need processing. - # Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid + # Must run BEFORE get_api_users() to avoid # burning rate limit on get_user() calls for skipped events. if self.github_event == "pull_request_review_thread": action = self.hook_data["action"] @@ -526,7 +527,7 @@ async def process(self) -> Any: # Early exit for status events with pending state — only terminal states # (success, failure, error) need processing. - # Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid + # Must run BEFORE get_api_users() to avoid # burning rate limit on get_user() calls for skipped events. if self.github_event == "status": state = self.hook_data["state"] @@ -542,7 +543,8 @@ async def process(self) -> Any: return None # Initialize auto-verified users from API users (async operation) - await self.add_api_users_to_auto_verified_and_merged_users() + api_users = await self.get_api_users() + self.auto_verified_and_merged_users.extend(user for user in api_users if user is not None) # Initialize app bot login for bot-PR identification (async) if not self.app_bot_login: @@ -569,6 +571,9 @@ async def process(self) -> Any: else: self.logger.debug(f"{self.log_prefix} No GitHub App API available — app_bot_login not set") + # Build final trusted-committers list with dynamic identities + await self._build_trusted_committers(api_users=api_users) + event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" # Start webhook routing context step @@ -668,7 +673,7 @@ async def process(self) -> Any: self.last_commit = await self._get_last_commit(pull_request=pull_request) self.parent_committer = pull_request.user.login self.last_committer = await github_api_call( - lambda: getattr(self.last_commit.committer, "login", "unknown"), + lambda: getattr(self.last_commit.committer, "login", "unknown") or "unknown", logger=self.logger, log_prefix=self.log_prefix, ) @@ -835,27 +840,18 @@ async def process(self) -> Any: await self._update_context_metrics() return None - async def add_api_users_to_auto_verified_and_merged_users(self) -> None: + async def get_api_users(self) -> list[str | None]: apis_and_tokens = get_apis_and_tokes_from_config(config=self.config) async def check_token(api: github.Github, token: str) -> str | None: """Check a single API token and return the user login if valid, None otherwise.""" token_suffix = f"...{token[-4:]}" if token else "unknown" try: - rate_limit_remaining = await github_api_call( - lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix - ) - except Exception as ex: - self.logger.warning( - f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', " - f"skipping. {ex}" - ) - return None - - if rate_limit_remaining == 60: - self.logger.warning( - f"{self.log_prefix} API has rate limit set to 60 which indicates an invalid token " - f"(token ending in '{token_suffix}'), skipping" + # Pre-flight probe: verify token is functional before attempting get_user() + await github_api_call(lambda: api.rate_limiting[-1], logger=self.logger, log_prefix=self.log_prefix) + except Exception: + self.logger.exception( + f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', skipping" ) return None @@ -871,8 +867,7 @@ async def check_token(api: github.Github, token: str) -> str | None: return _api_user - results = await asyncio.gather(*[check_token(api, token) for api, token in apis_and_tokens]) - self.auto_verified_and_merged_users.extend(user for user in results if user is not None) + return await asyncio.gather(*[check_token(api, token) for api, token in apis_and_tokens]) def prepare_log_prefix(self, pull_request: PullRequest | None = None) -> str: return prepare_log_prefix( @@ -997,6 +992,19 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: _mandatory_raw = True self.security_mandatory: bool = _mandatory_raw + _trusted_committers = _security_config.get("trusted-committers", []) + if not isinstance(_trusted_committers, list): + self.logger.warning( + f"{self.log_prefix} security-checks.trusted-committers must be array, " + f"got {type(_trusted_committers).__name__}. Defaulting to empty list." + ) + _trusted_committers = [] + self.security_trusted_committers: list[str] = [ + str(entry).strip().lower() + for entry in _trusted_committers + if isinstance(entry, (str, int, float)) and not isinstance(entry, bool) and str(entry).strip() + ] + _auto_merge_prs = self.config.get_value( value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config ) @@ -1110,6 +1118,39 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) + async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: + """Add dynamic entries to trusted-committers list. + + Adds: + - GitHub App bot login (app_bot_login) + - GitHub's web-flow system account + - API user logins from github-tokens + + Static config entries are already loaded in _repo_data_from_config(). + Called once from process() after app_bot_login is initialized. + """ + # GitHub App bot + if self.app_bot_login: + bot = str(self.app_bot_login).strip().lower() + if bot and bot not in self.security_trusted_committers: + self.security_trusted_committers.append(bot) + + # GitHub's web-flow system account + web_flow = GITHUB_WEB_FLOW_LOGIN.lower() + if web_flow not in self.security_trusted_committers: + self.security_trusted_committers.append(web_flow) + + # API users from github-tokens + if api_users is None: + api_users = await self.get_api_users() + for user in api_users: + if user: + normalized = str(user).strip().lower() + if normalized and normalized not in self.security_trusted_committers: + self.security_trusted_committers.append(normalized) + + self.logger.debug(f"{self.log_prefix} Trusted committers: {self.security_trusted_committers}") + async def get_pull_request(self, number: int | None = None) -> PullRequest | None: if number: self.logger.debug(f"{self.log_prefix} Attempting to get PR by number: {number}") diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index e9b415efd..d42d1a38d 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -372,8 +372,8 @@ async def run_security_suspicious_paths(self) -> None: async def run_security_committer_identity(self) -> None: """Check if the last committer matches the PR author. - Fails the check run if the last commit's committer differs from the PR author - (parent_committer), which may indicate a commit was authored by someone unexpected. + Uses a unified trusted-committers list that includes static config entries, + the GitHub App bot login, web-flow, and API users from github-tokens. """ if not self.github_webhook.security_committer_identity_check: self.logger.debug(f"{self.log_prefix} Committer identity check disabled, skipping") @@ -385,15 +385,17 @@ async def run_security_committer_identity(self) -> None: parent_committer = self.github_webhook.parent_committer last_committer = self.github_webhook.last_committer + # SECURITY: "unknown" check MUST precede the trusted-committers check. + # An unverifiable committer identity should always fail. if last_committer == "unknown": output: CheckRunOutput = { - "title": "\u274c Security: Committer Identity Unknown", + "title": "❌ Security: Committer Identity Unknown", "summary": "Committer identity could not be verified", "text": ( "## Committer Identity Check\n\n" f"**PR author:** `{parent_committer}`\n" "**Last commit committer:** unknown\n\n" - "Committer identity could not be verified \u2014 " + "Committer identity could not be verified — " "last commit has no associated GitHub user.\n\n" "This may indicate:\n" "- A commit was made with a local Git identity not linked to a GitHub account\n" @@ -406,68 +408,78 @@ async def run_security_committer_identity(self) -> None: f"PR author={parent_committer}, last committer has no GitHub user" ) await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) - elif last_committer == GITHUB_WEB_FLOW_LOGIN: - last_committer_id = self.github_webhook.last_committer_id - if last_committer_id == GITHUB_WEB_FLOW_USER_ID: - self.logger.debug( - f"{self.log_prefix} Last committer is GitHub's web-flow " - f"(user ID {last_committer_id}) — passing committer identity check" + + elif last_committer.lower() != parent_committer.lower(): + if last_committer.lower() in self.github_webhook.security_trusted_committers: + # Extra guard: verify web-flow by immutable user ID to prevent impersonation + if last_committer.lower() == GITHUB_WEB_FLOW_LOGIN: + last_committer_id = self.github_webhook.last_committer_id + if last_committer_id != GITHUB_WEB_FLOW_USER_ID: + self.logger.warning( + f"{self.log_prefix} Committer login is 'web-flow' but user ID " + f"{last_committer_id} does not match GitHub's web-flow ID " + f"{GITHUB_WEB_FLOW_USER_ID} — possible impersonation" + ) + output = { + "title": "❌ Security: Committer Identity Suspicious", + "summary": ( + f"Committer claims to be web-flow but has unexpected user ID {last_committer_id}" + ), + "text": ( + f"## Committer Identity Check\n\n" + f"**PR author:** `{parent_committer}`\n" + f"**Last commit committer:** `{last_committer}` (ID: {last_committer_id})\n" + f"**Expected web-flow ID:** {GITHUB_WEB_FLOW_USER_ID}\n\n" + f"The committer login is `web-flow` but the user ID does not match " + f"GitHub's official web-flow account. This may indicate an impersonation attempt." + ), + } + await self.check_run_handler.set_check_failure( + name=SECURITY_COMMITTER_IDENTITY_STR, output=output + ) + return + + # Trusted committer — pass + self.logger.info( + f"{self.log_prefix} Committer identity: '{last_committer}' is in unified trusted list" ) output = { "title": "Security: Committer Identity", - "summary": "Committer identity verified (GitHub web-flow)", + "summary": f"Committer '{last_committer}' is trusted", "text": ( f"## Committer Identity Check\n\n" f"**PR author:** `{parent_committer}`\n" - f"**Last commit committer:** `{last_committer}` (ID: {last_committer_id})\n\n" - f"The last commit was made via the GitHub web UI (rebase, merge, or edit). " - f"The committer is GitHub's verified `web-flow` system account " - f"(user ID {GITHUB_WEB_FLOW_USER_ID}), confirming this is a legitimate " - f"GitHub web operation." + f"**Last commit committer:** `{last_committer}`\n\n" + f"The committer differs from the PR author but is in the trusted committers list.\n" + f"This is expected for automated workflows (bots, CI tools, org identities, " + f"GitHub web operations)." ), } await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) else: - self.logger.warning( - f"{self.log_prefix} Committer login is 'web-flow' but user ID " - f"{last_committer_id} does not match GitHub's web-flow ID " - f"{GITHUB_WEB_FLOW_USER_ID} — possible impersonation" - ) + # Untrusted mismatch — fail output = { - "title": "\u274c Security: Committer Identity Suspicious", - "summary": f"Committer claims to be web-flow but has unexpected user ID {last_committer_id}", + "title": "❌ Security: Committer Identity Mismatch", + "summary": f"Last committer '{last_committer}' differs from PR author '{parent_committer}'", "text": ( f"## Committer Identity Check\n\n" f"**PR author:** `{parent_committer}`\n" - f"**Last commit committer:** `{last_committer}` (ID: {last_committer_id})\n" - f"**Expected web-flow ID:** {GITHUB_WEB_FLOW_USER_ID}\n\n" - f"The committer login is `web-flow` but the user ID does not match " - f"GitHub's official web-flow account. This may indicate an impersonation attempt." + f"**Last commit committer:** `{last_committer}`\n\n" + f"The last commit in this PR was made by a different user than the PR author. " + f"This may indicate:\n" + f"- An unauthorized commit was pushed to the PR branch\n" + f"- A bot or automation tool committed with unexpected credentials\n" + f"- A legitimate co-author contribution (review carefully)\n\n" + f"Please verify this is expected before merging." ), } + self.logger.warning( + f"{self.log_prefix} Committer identity mismatch: " + f"PR author={parent_committer}, last committer={last_committer}" + ) await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) - elif last_committer != parent_committer: - output = { - "title": "\u274c Security: Committer Identity Mismatch", - "summary": f"Last committer '{last_committer}' differs from PR author '{parent_committer}'", - "text": ( - f"## Committer Identity Check\n\n" - f"**PR author:** `{parent_committer}`\n" - f"**Last commit committer:** `{last_committer}`\n\n" - f"The last commit in this PR was made by a different user than the PR author. " - f"This may indicate:\n" - f"- An unauthorized commit was pushed to the PR branch\n" - f"- A bot or automation tool committed with unexpected credentials\n" - f"- A legitimate co-author contribution (review carefully)\n\n" - f"Please verify this is expected before merging." - ), - } - self.logger.warning( - f"{self.log_prefix} Committer identity mismatch: " - f"PR author={parent_committer}, last committer={last_committer}" - ) - await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) else: + # Match — pass output = { "title": "Security: Committer Identity", "summary": "Committer identity verified", diff --git a/webhook_server/tests/conftest.py b/webhook_server/tests/conftest.py index 7f3d580bf..f48b76b6a 100644 --- a/webhook_server/tests/conftest.py +++ b/webhook_server/tests/conftest.py @@ -124,7 +124,8 @@ def github_webhook(mocker, request): mock_github_api.get_rate_limit.return_value.rate.remaining = 5000 mocker.patch(f"{base_import_path}.get_api_with_highest_rate_limit", return_value=(mock_github_api, "TOKEN", "USER")) mocker.patch(f"{base_import_path}.get_github_repo_api", return_value=Repository()) - mocker.patch(f"{base_import_path}.GithubWebhook.add_api_users_to_auto_verified_and_merged_users", return_value=None) + mocker.patch(f"{base_import_path}.GithubWebhook.get_api_users", return_value=()) + mocker.patch(f"{base_import_path}.GithubWebhook._build_trusted_committers", return_value=None) # Use standard Python logger for caplog compatibility test_logger = python_logging.getLogger("GithubWebhook") diff --git a/webhook_server/tests/e2e/README.md b/webhook_server/tests/e2e/README.md index 2615a22a0..0512ec0bb 100644 --- a/webhook_server/tests/e2e/README.md +++ b/webhook_server/tests/e2e/README.md @@ -198,6 +198,7 @@ Main fixture that manages complete E2E infrastructure lifecycle. ```python import pytest + @pytest.mark.e2e def test_webhook_processing(e2e_server): """Infrastructure is running, ready to test.""" @@ -223,6 +224,7 @@ def test_webhook_processing(e2e_server): import pytest import subprocess + @pytest.mark.e2e def test_webhook_flow(e2e_server): """Test description explaining what is being validated.""" @@ -240,6 +242,7 @@ def test_webhook_flow(e2e_server): # Step 2: Wait for webhook processing (if needed) import time + time.sleep(5) # Give server time to process webhook # Step 3: Verify results using gh CLI diff --git a/webhook_server/tests/test_api_call_counting.py b/webhook_server/tests/test_api_call_counting.py index b725b74b1..b1390b528 100644 --- a/webhook_server/tests/test_api_call_counting.py +++ b/webhook_server/tests/test_api_call_counting.py @@ -46,7 +46,8 @@ async def test_github_webhook_token_metrics_with_counter(): patch("webhook_server.libs.github_api.get_repository_github_app_api"), patch("webhook_server.libs.github_api.prepare_log_prefix"), # Patch this method to avoid calls to get_apis_and_tokes_from_config which isn't mocked - patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users"), + patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()), + patch("webhook_server.libs.github_api.GithubWebhook._build_trusted_committers", return_value=None), ): # Setup Config mock_config = MockConfig.return_value diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index e1357983a..38764d21c 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -241,7 +241,7 @@ def test_process_ping_event( @patch("webhook_server.libs.handlers.pull_request_handler.PullRequestHandler.process_pull_request_webhook_data") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_pull_request_event( self, mock_auto_verified_prop: Mock, @@ -312,7 +312,7 @@ async def test_process_pull_request_event( @patch("webhook_server.libs.handlers.push_handler.PushHandler.process_push_webhook_data") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_push_event( self, mock_auto_verified_prop: Mock, @@ -356,7 +356,7 @@ async def test_process_push_event( @patch("webhook_server.libs.handlers.issue_comment_handler.IssueCommentHandler.process_comment_webhook_data") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_issue_comment_event( self, mock_auto_verified_prop: Mock, @@ -435,7 +435,7 @@ async def test_pr_sha_storage_from_webhook_payload( patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_repo_api, patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") as mock_get_apis, patch("webhook_server.libs.config.Config.repository_local_data") as mock_repo_local_data, - patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users"), + patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()), ): mock_api = Mock() mock_api.rate_limiting = [100, 5000] @@ -487,7 +487,7 @@ async def test_pr_sha_storage_fallback_for_non_pr_events(self, issue_comment_pay patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_repo_api, patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") as mock_get_apis, patch("webhook_server.libs.config.Config.repository_local_data") as mock_repo_local_data, - patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users"), + patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()), ): mock_api = Mock() mock_api.rate_limiting = [100, 5000] @@ -532,7 +532,7 @@ async def test_pr_sha_storage_fallback_for_non_pr_events(self, issue_comment_pay @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_unsupported_event( self, mock_auto_verified_prop: Mock, @@ -736,7 +736,7 @@ def test_init_failed_repository_objects( @patch("webhook_server.libs.github_api.get_repository_github_app_api") @patch("webhook_server.utils.helpers.get_repository_color_for_log_prefix") @patch("webhook_server.libs.github_api.get_apis_and_tokes_from_config") - async def test_add_api_users_to_auto_verified_and_merged_users( + async def test_get_api_users( self, mock_get_apis, mock_color, @@ -748,7 +748,7 @@ async def test_add_api_users_to_auto_verified_and_merged_users( minimal_headers, logger, ) -> None: - """Test the add_api_users_to_auto_verified_and_merged_users method.""" + """Test the get_api_users method.""" mock_config.return_value.repository = True mock_config.return_value.repository_local_data.return_value = {} mock_get_api.return_value = (Mock(), "token", "apiuser") @@ -798,7 +798,8 @@ def get_value_side_effect(value, *args, **kwargs): mock_api.get_user.return_value = mock_user mock_get_apis.return_value = [(mock_api, TEST_GITHUB_TOKEN)] gh = GithubWebhook(minimal_hook_data, minimal_headers, logger) - await gh.add_api_users_to_auto_verified_and_merged_users() + api_users = await gh.get_api_users() + gh.auto_verified_and_merged_users.extend(user for user in api_users if user is not None) assert "test-user" in gh.auto_verified_and_merged_users @patch("webhook_server.libs.github_api.get_apis_and_tokes_from_config") @@ -998,8 +999,9 @@ async def test_process_status_event(self, state: str, should_recheck: bool) -> N with patch.object( webhook, - "add_api_users_to_auto_verified_and_merged_users", + "get_api_users", new_callable=AsyncMock, + return_value=(), ) as mock_add_api_users: with patch.object( webhook, @@ -2036,7 +2038,7 @@ async def mock_run_command(command: str, **_kwargs: object) -> tuple[bool, str, @patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_push_event_deletion( self, mock_auto_verified_prop: Mock, @@ -2115,7 +2117,7 @@ async def test_process_push_event_deletion( @patch("webhook_server.libs.handlers.push_handler.PushHandler.process_push_webhook_data") @patch("webhook_server.utils.helpers.get_apis_and_tokes_from_config") @patch("webhook_server.libs.config.Config.repository_local_data") - @patch("webhook_server.libs.github_api.GithubWebhook.add_api_users_to_auto_verified_and_merged_users") + @patch("webhook_server.libs.github_api.GithubWebhook.get_api_users", return_value=()) async def test_process_push_event_branch_push_skips_clone( self, mock_auto_verified_prop: Mock, @@ -2490,8 +2492,9 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo with patch.object( webhook, - "add_api_users_to_auto_verified_and_merged_users", + "get_api_users", new_callable=AsyncMock, + return_value=(), ) as mock_add_api_users: with patch.object( webhook, "get_pull_request", new_callable=AsyncMock @@ -2561,8 +2564,9 @@ async def test_process_review_thread_skips_when_conversation_resolution_disabled with patch.object( webhook, - "add_api_users_to_auto_verified_and_merged_users", + "get_api_users", new_callable=AsyncMock, + return_value=(), ) as mock_add_api_users: with patch.object( webhook, "get_pull_request", new_callable=AsyncMock diff --git a/webhook_server/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index fcdd866f0..dd27ee574 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -56,6 +56,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.config.get_value = Mock(return_value=None) mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS mock_webhook.security_committer_identity_check = True + mock_webhook.security_trusted_committers = [] mock_webhook.parent_committer = "test-user" mock_webhook.last_committer = "test-user" return mock_webhook @@ -90,8 +91,8 @@ async def test_suspicious_paths_no_match( with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success: await runner_handler.run_security_suspicious_paths() - mock_progress.assert_called_once_with(name=SECURITY_SUSPICIOUS_PATHS_STR) - mock_success.assert_called_once() + mock_progress.assert_awaited_once_with(name=SECURITY_SUSPICIOUS_PATHS_STR) + mock_success.assert_awaited_once() call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_SUSPICIOUS_PATHS_STR assert "No security-sensitive paths modified" in call_args.kwargs["output"]["summary"] @@ -107,7 +108,7 @@ async def test_suspicious_paths_match_single( with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_suspicious_paths() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_SUSPICIOUS_PATHS_STR assert "1 file(s)" in call_args.kwargs["output"]["summary"] @@ -129,7 +130,7 @@ async def test_suspicious_paths_match_multiple( with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_suspicious_paths() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_SUSPICIOUS_PATHS_STR assert "3 file(s)" in call_args.kwargs["output"]["summary"] @@ -152,7 +153,7 @@ async def test_suspicious_paths_custom_config( with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_suspicious_paths() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert "custom/sensitive/config.yaml" in call_args.kwargs["output"]["text"] @@ -163,7 +164,7 @@ async def test_suspicious_paths_empty_config(self, runner_handler: RunnerHandler with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress: await runner_handler.run_security_suspicious_paths() - mock_progress.assert_not_called() + mock_progress.assert_not_awaited() @pytest.mark.asyncio async def test_suspicious_paths_all_default_prefixes( @@ -184,7 +185,7 @@ async def test_suspicious_paths_all_default_prefixes( with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_suspicious_paths() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert "7 file(s)" in call_args.kwargs["output"]["summary"] @@ -215,6 +216,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.config.get_value = Mock(return_value=None) mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS mock_webhook.security_committer_identity_check = True + mock_webhook.security_trusted_committers = [] mock_webhook.parent_committer = "test-user" mock_webhook.last_committer = "test-user" return mock_webhook @@ -247,8 +249,8 @@ async def test_committer_identity_match(self, runner_handler: RunnerHandler) -> with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success: await runner_handler.run_security_committer_identity() - mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) - mock_success.assert_called_once() + mock_progress.assert_awaited_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) + mock_success.assert_awaited_once() call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR assert "Committer identity verified" in call_args.kwargs["output"]["summary"] @@ -263,7 +265,7 @@ async def test_committer_identity_mismatch(self, runner_handler: RunnerHandler) with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_committer_identity() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR output = call_args.kwargs["output"] @@ -282,7 +284,7 @@ async def test_committer_identity_unknown(self, runner_handler: RunnerHandler) - with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_committer_identity() - mock_failure.assert_called_once() + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR output = call_args.kwargs["output"] @@ -295,34 +297,36 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) runner_handler.github_webhook.parent_committer = "legit-user" runner_handler.github_webhook.last_committer = GITHUB_WEB_FLOW_LOGIN runner_handler.github_webhook.last_committer_id = GITHUB_WEB_FLOW_USER_ID + runner_handler.github_webhook.security_trusted_committers = ["web-flow"] with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress: with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success: await runner_handler.run_security_committer_identity() - mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) - mock_success.assert_called_once() + mock_progress.assert_awaited_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) + mock_success.assert_awaited_once() call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "web-flow" in call_args.kwargs["output"]["summary"] - assert str(GITHUB_WEB_FLOW_USER_ID) in call_args.kwargs["output"]["text"] + assert "trusted" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio async def test_committer_identity_web_flow_fake_id(self, runner_handler: RunnerHandler) -> None: - """Check fails when committer claims to be web-flow but has wrong user ID.""" + """Web-flow with wrong user ID is flagged as suspicious impersonation.""" runner_handler.github_webhook.parent_committer = "legit-user" runner_handler.github_webhook.last_committer = GITHUB_WEB_FLOW_LOGIN - runner_handler.github_webhook.last_committer_id = 99999999 + runner_handler.github_webhook.last_committer_id = 99999 + runner_handler.github_webhook.security_trusted_committers = ["web-flow"] with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress: with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: await runner_handler.run_security_committer_identity() - mock_progress.assert_called_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) - mock_failure.assert_called_once() + mock_progress.assert_awaited_once_with(name=SECURITY_COMMITTER_IDENTITY_STR) + mock_failure.assert_awaited_once() call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "99999999" in call_args.kwargs["output"]["summary"] + assert "web-flow" in call_args.kwargs["output"]["summary"] + assert str(GITHUB_WEB_FLOW_USER_ID) in call_args.kwargs["output"]["text"] @pytest.mark.asyncio async def test_committer_identity_check_disabled(self, runner_handler: RunnerHandler) -> None: @@ -331,7 +335,70 @@ async def test_committer_identity_check_disabled(self, runner_handler: RunnerHan with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()) as mock_progress: await runner_handler.run_security_committer_identity() - mock_progress.assert_not_called() + mock_progress.assert_not_awaited() + + @pytest.mark.asyncio + async def test_committer_identity_mismatch_trusted(self, runner_handler: RunnerHandler) -> None: + """Check passes when last committer is in trusted-committers list.""" + runner_handler.github_webhook.parent_committer = "legit-user" + runner_handler.github_webhook.last_committer = "pre-commit-ci[bot]" + runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]", "myorg"] + + with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()): + with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success: + await runner_handler.run_security_committer_identity() + + mock_success.assert_awaited_once() + call_args = mock_success.call_args + assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR + assert "trusted" in call_args.kwargs["output"]["summary"] + + @pytest.mark.asyncio + async def test_committer_identity_mismatch_not_trusted(self, runner_handler: RunnerHandler) -> None: + """Check fails when last committer is NOT in trusted-committers list.""" + runner_handler.github_webhook.parent_committer = "legit-user" + runner_handler.github_webhook.last_committer = "suspicious-user" + runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]"] + + with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()): + with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: + await runner_handler.run_security_committer_identity() + + mock_failure.assert_awaited_once() + call_args = mock_failure.call_args + assert "suspicious-user" in call_args.kwargs["output"]["summary"] + + @pytest.mark.asyncio + async def test_committer_identity_unknown_not_bypassed_by_trusted_list(self, runner_handler: RunnerHandler) -> None: + """Check fails for unknown committer even when 'unknown' is in trusted-committers list.""" + runner_handler.github_webhook.parent_committer = "legit-user" + runner_handler.github_webhook.last_committer = "unknown" + runner_handler.github_webhook.security_trusted_committers = ["unknown"] + + with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()): + with patch.object(runner_handler.check_run_handler, "set_check_failure", new=AsyncMock()) as mock_failure: + await runner_handler.run_security_committer_identity() + + mock_failure.assert_awaited_once() + call_args = mock_failure.call_args + assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR + assert "could not be verified" in call_args.kwargs["output"]["summary"].lower() + + @pytest.mark.asyncio + async def test_committer_identity_mismatch_trusted_case_insensitive(self, runner_handler: RunnerHandler) -> None: + """Check passes when committer login differs in case from trusted-committers entry.""" + runner_handler.github_webhook.parent_committer = "legit-user" + runner_handler.github_webhook.last_committer = "Pre-Commit-CI[bot]" + runner_handler.github_webhook.security_trusted_committers = ["pre-commit-ci[bot]"] + + with patch.object(runner_handler.check_run_handler, "set_check_in_progress", new=AsyncMock()): + with patch.object(runner_handler.check_run_handler, "set_check_success", new=AsyncMock()) as mock_success: + await runner_handler.run_security_committer_identity() + + mock_success.assert_awaited_once() + call_args = mock_success.call_args + assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR + assert "trusted" in call_args.kwargs["output"]["summary"] class TestAutoMergeSecurityOverride: @@ -351,6 +418,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.set_auto_merge_prs = [] mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS mock_webhook.security_committer_identity_check = True + mock_webhook.security_trusted_committers = [] mock_webhook.security_mandatory = True mock_webhook.last_commit = Mock() mock_webhook.ctx = None @@ -600,6 +668,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.custom_check_runs = [] mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS mock_webhook.security_committer_identity_check = True + mock_webhook.security_trusted_committers = [] mock_webhook.security_mandatory = True mock_webhook.last_commit = Mock() mock_webhook.last_commit.sha = "abc123" @@ -670,6 +739,7 @@ async def test_security_checks_partial_config( """Only configured security checks are added to required list.""" check_run_handler.github_webhook.security_suspicious_paths = [] check_run_handler.github_webhook.security_committer_identity_check = True + check_run_handler.github_webhook.security_trusted_committers = [] with patch.object( check_run_handler, @@ -696,6 +766,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.security_mandatory = True mock_webhook.security_suspicious_paths = DEFAULT_SUSPICIOUS_PATHS mock_webhook.security_committer_identity_check = True + mock_webhook.security_trusted_committers = [] mock_webhook.ctx = None mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) @@ -771,7 +842,7 @@ async def test_security_override_rejected_for_non_maintainer( ) # Check runs should NOT be set to success - mock_success.assert_not_called() + mock_success.assert_not_awaited() @pytest.mark.asyncio async def test_security_override_cancel(self, mock_github_webhook: Mock, mock_pull_request: Mock) -> None: @@ -803,5 +874,5 @@ async def test_security_override_cancel(self, mock_github_webhook: Mock, mock_pu is_draft=False, ) - mock_run_paths.assert_called_once() - mock_run_identity.assert_called_once() + mock_run_paths.assert_awaited_once() + mock_run_identity.assert_awaited_once()