From c73bc3cde803eb1a627a95b9c8215cb57b50ada0 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 15 Jun 2026 16:54:32 +0300 Subject: [PATCH 01/13] 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 Assisted-by: Claude --- examples/config.yaml | 2 + webhook_server/config/schema.yaml | 8 +++ webhook_server/libs/github_api.py | 13 ++++ .../libs/handlers/runner_handler.py | 62 +++++++++++++------ webhook_server/tests/test_security_checks.py | 37 +++++++++++ 5 files changed, 102 insertions(+), 20 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index 14d219499..3f61534e4 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: # Committers always trusted for identity check + - "pre-commit-ci[bot]" repositories: my-repository: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index bfdf84129..6cedab373 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -95,6 +95,14 @@ $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. + items: + type: string additionalProperties: false type: object properties: diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 64f87853e..5fa87c4f7 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -997,6 +997,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 str(entry).strip() + ] + _auto_merge_prs = self.config.get_value( value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config ) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index e9b415efd..82393fe31 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -385,6 +385,9 @@ 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 allowlist check. + # An unverifiable committer identity should always fail, even if "unknown" is + # accidentally added to the trusted-committers list. if last_committer == "unknown": output: CheckRunOutput = { "title": "\u274c Security: Committer Identity Unknown", @@ -447,26 +450,45 @@ async def run_security_committer_identity(self) -> None: } 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) + # Check if last_committer is in trusted-committers allowlist + if last_committer.lower() in self.github_webhook.security_trusted_committers: + output = { + "title": "Security: Committer Identity", + "summary": f"Committer '{last_committer}' is in the trusted-committers allowlist", + "text": ( + f"## Committer Identity Check\n\n" + f"**PR author:** `{parent_committer}`\n" + f"**Last commit committer:** `{last_committer}`\n\n" + f"The committer differs from the PR author but is in the `trusted-committers` allowlist.\n" + f"This is expected for automated workflows (bots, CI tools, org identities)." + ), + } + self.logger.info( + f"{self.log_prefix} Committer identity mismatch allowed: " + f"'{last_committer}' is in trusted-committers list" + ) + await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) + else: + 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: output = { "title": "Security: Committer Identity", diff --git a/webhook_server/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index fcdd866f0..8831294c1 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 @@ -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 @@ -333,6 +335,37 @@ async def test_committer_identity_check_disabled(self, runner_handler: RunnerHan await runner_handler.run_security_committer_identity() mock_progress.assert_not_called() + @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_called_once() + call_args = mock_success.call_args + assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR + assert "trusted-committers" 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_called_once() + call_args = mock_failure.call_args + assert "suspicious-user" in call_args.kwargs["output"]["summary"] + class TestAutoMergeSecurityOverride: """Test auto-merge is blocked when PR modifies suspicious paths.""" @@ -351,6 +384,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 +634,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 +705,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 +732,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) From 3d5e9219b5a2a8e84b73db2894aaca025d6eeb22 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 15 Jun 2026 17:12:39 +0300 Subject: [PATCH 02/13] fix: filter boolean values from trusted-committers allowlist Python's bool is a subclass of int, so YAML true/false would pass the isinstance(entry, (str, int, float)) check and become "true"/"false" strings in the allowlist. Added explicit bool exclusion. Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 5fa87c4f7..6d2c6a601 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -1007,7 +1007,7 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: self.security_trusted_committers: list[str] = [ str(entry).strip().lower() for entry in _trusted_committers - if isinstance(entry, (str, int, float)) and str(entry).strip() + if isinstance(entry, (str, int, float)) and not isinstance(entry, bool) and str(entry).strip() ] _auto_merge_prs = self.config.get_value( From 65857d6e84d3cf490bcc8b7e3b54a0932a1476fa Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 14:42:56 +0300 Subject: [PATCH 03/13] feat: auto-trust server bot and API users in committer identity check Adds _build_dynamic_trusted_committers() to automatically include app_bot_login and API users in the trusted-committers allowlist. Adds str() coercion on dynamic entries for consistency. Adds tests for "unknown" bypass prevention and case-insensitive matching. Signed-off-by: rnetser Assisted-by: Claude --- webhook_server/config/schema.yaml | 2 ++ webhook_server/libs/github_api.py | 26 ++++++++++++++++ webhook_server/tests/test_security_checks.py | 32 ++++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 6cedab373..f34bab67c 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -101,6 +101,8 @@ $defs: 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 server's own bot login and API users are automatically trusted — + you only need to list additional external committers here. items: type: string additionalProperties: false diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 6d2c6a601..69809a59d 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -569,6 +569,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 server identities + self._build_dynamic_trusted_committers() + event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" # Start webhook routing context step @@ -1123,6 +1126,29 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) + def _build_dynamic_trusted_committers(self) -> None: + """Add server bot and API users to trusted-committers list. + + Called after app_bot_login and auto_verified_and_merged_users are initialized. + Combines static config entries with dynamic server identities. + """ + dynamic_entries: list[str] = [] + + if self.app_bot_login: + dynamic_entries.append(str(self.app_bot_login).strip().lower()) + + for user in self.auto_verified_and_merged_users: + if user: + dynamic_entries.append(str(user).strip().lower()) + + # Add dynamic entries that aren't already in the static list + for entry in dynamic_entries: + if entry not in self.security_trusted_committers: + self.security_trusted_committers.append(entry) + + if dynamic_entries: + self.logger.debug(f"{self.log_prefix} Added dynamic trusted committers: {dynamic_entries}") + 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/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index 8831294c1..499de504a 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -366,6 +366,38 @@ async def test_committer_identity_mismatch_not_trusted(self, runner_handler: Run 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_called_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_called_once() + call_args = mock_success.call_args + assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR + assert "trusted-committers" in call_args.kwargs["output"]["summary"] + class TestAutoMergeSecurityOverride: """Test auto-merge is blocked when PR modifies suspicious paths.""" From a17f68ba97781b95bbbb568d234fa718437ab3c4 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 14:52:07 +0300 Subject: [PATCH 04/13] refactor: add dedicated app_bot_login check, remove API users from dynamic list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the same pattern as web-flow — app_bot_login gets its own dedicated elif branch in run_security_committer_identity(). Removed auto_verified_and_merged_users from dynamic trusted list. Static trusted-committers config remains for external bots and org identities. Signed-off-by: rnetser Assisted-by: Claude --- webhook_server/config/schema.yaml | 2 +- webhook_server/libs/github_api.py | 25 ++++++------------- .../libs/handlers/runner_handler.py | 17 +++++++++++++ 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index f34bab67c..882db95e5 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -101,7 +101,7 @@ $defs: 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 server's own bot login and API users are automatically trusted — + Note: The server's own bot login is automatically trusted — you only need to list additional external committers here. items: type: string diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 69809a59d..74d75e40d 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -1127,27 +1127,16 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) def _build_dynamic_trusted_committers(self) -> None: - """Add server bot and API users to trusted-committers list. + """Add server bot to trusted-committers list. - Called after app_bot_login and auto_verified_and_merged_users are initialized. - Combines static config entries with dynamic server identities. + Called after app_bot_login is initialized. + Combines static config entries with the server's bot identity. """ - dynamic_entries: list[str] = [] - if self.app_bot_login: - dynamic_entries.append(str(self.app_bot_login).strip().lower()) - - for user in self.auto_verified_and_merged_users: - if user: - dynamic_entries.append(str(user).strip().lower()) - - # Add dynamic entries that aren't already in the static list - for entry in dynamic_entries: - if entry not in self.security_trusted_committers: - self.security_trusted_committers.append(entry) - - if dynamic_entries: - self.logger.debug(f"{self.log_prefix} Added dynamic trusted committers: {dynamic_entries}") + bot_login = str(self.app_bot_login).strip().lower() + if bot_login and bot_login not in self.security_trusted_committers: + self.security_trusted_committers.append(bot_login) + self.logger.debug(f"{self.log_prefix} Added server bot '{bot_login}' to trusted committers") async def get_pull_request(self, number: int | None = None) -> PullRequest | None: if number: diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 82393fe31..04db49249 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -449,6 +449,23 @@ async def run_security_committer_identity(self) -> None: ), } await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) + elif self.github_webhook.app_bot_login and last_committer == self.github_webhook.app_bot_login: + self.logger.debug( + f"{self.log_prefix} Last committer is the server's bot " + f"'{last_committer}' — passing committer identity check" + ) + output = { + "title": "Security: Committer Identity", + "summary": f"Committer identity verified (server bot: {last_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 was made by the webhook server's own GitHub App bot. " + f"This is expected for automated operations (cherry-picks, rebases, auto-fixes)." + ), + } + await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) elif last_committer != parent_committer: # Check if last_committer is in trusted-committers allowlist if last_committer.lower() in self.github_webhook.security_trusted_committers: From 52842b0d5040ee96263c39e4140ba1f9c17ad13a Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 15:27:45 +0300 Subject: [PATCH 05/13] refactor: unified trusted committers list with single check Replaces separate elif branches for web-flow and app_bot_login with a single unified trusted list. The list combines: static config trusted-committers, server's GitHub App bot login, GitHub web-flow, and github-tokens API user logins. Web-flow retains extra user ID verification to prevent impersonation. One list, one check. Assisted-by: Claude Signed-off-by: rnetser --- examples/config.yaml | 2 +- webhook_server/config/schema.yaml | 4 +- webhook_server/libs/github_api.py | 36 ++++-- .../libs/handlers/runner_handler.py | 119 +++++++----------- webhook_server/tests/test_security_checks.py | 9 +- 5 files changed, 83 insertions(+), 87 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index 3f61534e4..e72f70f5a 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -150,7 +150,7 @@ security-checks: - ".github/workflows/" - ".github/actions/" committer-identity-check: true - trusted-committers: # Committers always trusted for identity check + trusted-committers: # Additional trusted committers (server bot, web-flow, and API users are auto-included) - "pre-commit-ci[bot]" repositories: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index 882db95e5..f281a45af 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -101,8 +101,8 @@ $defs: 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 server's own bot login is automatically trusted — - you only need to list additional external committers here. + Note: The server's 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 diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 74d75e40d..a1c6868c7 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, @@ -1127,16 +1128,37 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) def _build_dynamic_trusted_committers(self) -> None: - """Add server bot to trusted-committers list. + """Build unified trusted-committers list from all sources. - Called after app_bot_login is initialized. - Combines static config entries with the server's bot identity. + Combines: + - Static config entries (trusted-committers from security-checks) + - Server's GitHub App bot login (app_bot_login) + - GitHub's web-flow system account + - API user logins from github-tokens (auto_verified_and_merged_users) + + Called after app_bot_login and auto_verified_and_merged_users are initialized. """ + dynamic_entries: list[str] = [] + + # Server's GitHub App bot if self.app_bot_login: - bot_login = str(self.app_bot_login).strip().lower() - if bot_login and bot_login not in self.security_trusted_committers: - self.security_trusted_committers.append(bot_login) - self.logger.debug(f"{self.log_prefix} Added server bot '{bot_login}' to trusted committers") + dynamic_entries.append(str(self.app_bot_login).strip().lower()) + + # GitHub's web-flow system account + dynamic_entries.append(GITHUB_WEB_FLOW_LOGIN.lower()) + + # API users from github-tokens + for user in self.auto_verified_and_merged_users: + if user: + dynamic_entries.append(str(user).strip().lower()) + + # Add dynamic entries not already in static list + for entry in dynamic_entries: + if entry and entry not in self.security_trusted_committers: + self.security_trusted_committers.append(entry) + + if dynamic_entries: + self.logger.debug(f"{self.log_prefix} Unified trusted committers list: {self.security_trusted_committers}") async def get_pull_request(self, number: int | None = None) -> PullRequest | None: if number: diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 04db49249..5bbd85f17 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 server's bot login, GitHub's 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,18 +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 allowlist check. - # An unverifiable committer identity should always fail, even if "unknown" is - # accidentally added to the trusted-committers list. + # 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" @@ -409,85 +408,58 @@ 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" - ) - output = { - "title": "Security: Committer Identity", - "summary": "Committer identity verified (GitHub web-flow)", - "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." - ), - } - 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" - ) - output = { - "title": "\u274c 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) - elif self.github_webhook.app_bot_login and last_committer == self.github_webhook.app_bot_login: - self.logger.debug( - f"{self.log_prefix} Last committer is the server's bot " - f"'{last_committer}' — passing committer identity check" - ) - output = { - "title": "Security: Committer Identity", - "summary": f"Committer identity verified (server bot: {last_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 was made by the webhook server's own GitHub App bot. " - f"This is expected for automated operations (cherry-picks, rebases, auto-fixes)." - ), - } - await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) + elif last_committer != parent_committer: - # Check if last_committer is in trusted-committers allowlist 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 == 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": f"Committer '{last_committer}' is in the trusted-committers allowlist", + "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}`\n\n" - f"The committer differs from the PR author but is in the `trusted-committers` allowlist.\n" - f"This is expected for automated workflows (bots, CI tools, org identities)." + 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)." ), } - self.logger.info( - f"{self.log_prefix} Committer identity mismatch allowed: " - f"'{last_committer}' is in trusted-committers list" - ) await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) else: + # Untrusted mismatch — fail output = { - "title": "\u274c Security: Committer Identity Mismatch", + "title": "❌ Security: Committer Identity Mismatch", "summary": f"Last committer '{last_committer}' differs from PR author '{parent_committer}'", "text": ( f"## Committer Identity Check\n\n" @@ -507,6 +479,7 @@ async def run_security_committer_identity(self) -> None: ) 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/test_security_checks.py b/webhook_server/tests/test_security_checks.py index 499de504a..60195a82a 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -297,6 +297,7 @@ 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: @@ -306,8 +307,7 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) mock_success.assert_called_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: @@ -315,6 +315,7 @@ async def test_committer_identity_web_flow_fake_id(self, runner_handler: RunnerH 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.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: @@ -349,7 +350,7 @@ async def test_committer_identity_mismatch_trusted(self, runner_handler: RunnerH mock_success.assert_called_once() call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "trusted-committers" in call_args.kwargs["output"]["summary"] + assert "trusted" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio async def test_committer_identity_mismatch_not_trusted(self, runner_handler: RunnerHandler) -> None: @@ -396,7 +397,7 @@ async def test_committer_identity_mismatch_trusted_case_insensitive(self, runner mock_success.assert_called_once() call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "trusted-committers" in call_args.kwargs["output"]["summary"] + assert "trusted" in call_args.kwargs["output"]["summary"] class TestAutoMergeSecurityOverride: From c05d652882ad6e600e199b0988790e701639de76 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 16:16:32 +0300 Subject: [PATCH 06/13] refactor: consolidate trusted committers into single _build_trusted_committers method Extracts get_api_users() for reuse. Moves all trusted list building into _build_trusted_committers(): static config, app_bot_login, web-flow, and github-tokens API users. Removes unreliable rate_limit==60 heuristic for token validation. Signed-off-by: rnetser Assisted-by: Claude --- webhook_server/libs/github_api.py | 94 ++++++++++++++++--------------- 1 file changed, 48 insertions(+), 46 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index a1c6868c7..948086955 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -571,7 +571,7 @@ async def process(self) -> Any: self.logger.debug(f"{self.log_prefix} No GitHub App API available — app_bot_login not set") # Build final trusted-committers list with dynamic server identities - self._build_dynamic_trusted_committers() + await self._build_trusted_committers() event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" @@ -839,16 +839,14 @@ 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) -> tuple[Any]: 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 - ) + 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}', " @@ -856,13 +854,6 @@ async def check_token(api: github.Github, token: str) -> str | None: ) 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" - ) - return None - try: _api_user = await github_api_call( lambda: api.get_user().login, logger=self.logger, log_prefix=self.log_prefix @@ -875,7 +866,10 @@ 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]) + return await asyncio.gather(*[check_token(api, token) for api, token in apis_and_tokens]) + + async def add_api_users_to_auto_verified_and_merged_users(self) -> None: + results = await self.get_api_users() self.auto_verified_and_merged_users.extend(user for user in results if user is not None) def prepare_log_prefix(self, pull_request: PullRequest | None = None) -> str: @@ -1001,18 +995,7 @@ 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() - ] + self.security_trusted_committers: list[str] = [] _auto_merge_prs = self.config.get_value( value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config @@ -1127,38 +1110,57 @@ def _sanitize_item(item: Any) -> str: self.mask_sensitive = self.config.get_value("mask-sensitive-data", return_on_none=True) - def _build_dynamic_trusted_committers(self) -> None: - """Build unified trusted-committers list from all sources. + async def _build_trusted_committers(self) -> None: + """Build the complete trusted-committers list from all sources. - Combines: + Sources: - Static config entries (trusted-committers from security-checks) - Server's GitHub App bot login (app_bot_login) - GitHub's web-flow system account - - API user logins from github-tokens (auto_verified_and_merged_users) + - API user logins from github-tokens (via get_api_users) - Called after app_bot_login and auto_verified_and_merged_users are initialized. + Called once from process() after app_bot_login is initialized. """ - dynamic_entries: list[str] = [] + trusted: list[str] = [] - # Server's GitHub App bot - if self.app_bot_login: - dynamic_entries.append(str(self.app_bot_login).strip().lower()) + # 1. Static config entries + _security_checks: dict[str, Any] | None = self.config.get_value(value="security-checks", return_on_none=None) + _security_config = _security_checks if isinstance(_security_checks, dict) else {} + _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 = [] - # GitHub's web-flow system account - dynamic_entries.append(GITHUB_WEB_FLOW_LOGIN.lower()) + for entry in _trusted_committers: + if isinstance(entry, (str, int, float)) and not isinstance(entry, bool): + normalized = str(entry).strip().lower() + if normalized and normalized not in trusted: + trusted.append(normalized) - # API users from github-tokens - for user in self.auto_verified_and_merged_users: + # 2. Server's GitHub App bot + if self.app_bot_login: + bot = str(self.app_bot_login).strip().lower() + if bot and bot not in trusted: + trusted.append(bot) + + # 3. GitHub's web-flow system account + web_flow = GITHUB_WEB_FLOW_LOGIN.lower() + if web_flow not in trusted: + trusted.append(web_flow) + + # 4. API users from github-tokens + api_users = await self.get_api_users() + for user in api_users: if user: - dynamic_entries.append(str(user).strip().lower()) - - # Add dynamic entries not already in static list - for entry in dynamic_entries: - if entry and entry not in self.security_trusted_committers: - self.security_trusted_committers.append(entry) + normalized = str(user).strip().lower() + if normalized and normalized not in trusted: + trusted.append(normalized) - if dynamic_entries: - self.logger.debug(f"{self.log_prefix} Unified trusted committers list: {self.security_trusted_committers}") + self.security_trusted_committers = trusted + 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: From ca4e448b142d011245d44e4e706102a5212c54a5 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 16:39:14 +0300 Subject: [PATCH 07/13] refactor: remove dead web-flow impersonation guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit web-flow is a reserved GitHub system account — the committer.login can only be "web-flow" from GitHub's API, which always has user ID 19864447. The ID mismatch case is impossible, making the guard dead code. Renamed fake-ID test to verify it passes via trusted list. Signed-off-by: Ruth Netser Assisted-by: Claude --- .../libs/handlers/runner_handler.py | 30 ------------------- webhook_server/tests/test_security_checks.py | 14 +++++---- 2 files changed, 8 insertions(+), 36 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 5bbd85f17..414acfd3e 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -25,8 +25,6 @@ BUILD_CONTAINER_STR, CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, - GITHUB_WEB_FLOW_LOGIN, - GITHUB_WEB_FLOW_USER_ID, PRE_COMMIT_STR, PREK_STR, PYTHON_MODULE_INSTALL_STR, @@ -411,34 +409,6 @@ async def run_security_committer_identity(self) -> None: elif last_committer != parent_committer: 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 == 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" diff --git a/webhook_server/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index 60195a82a..9245652e9 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -310,22 +310,24 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) 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.""" + async def test_committer_identity_web_flow_fake_id_passes_via_trusted_list( + self, runner_handler: RunnerHandler + ) -> None: + """Web-flow with wrong user ID passes because web-flow is in trusted list.""" 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.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: + 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_failure.assert_called_once() - call_args = mock_failure.call_args + mock_success.assert_called_once() + call_args = mock_success.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "99999999" in call_args.kwargs["output"]["summary"] + assert "trusted" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio async def test_committer_identity_check_disabled(self, runner_handler: RunnerHandler) -> None: From 6225ee3a23cd920adea2ce4b11b6020175308f20 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 17:19:23 +0300 Subject: [PATCH 08/13] =?UTF-8?q?refactor:=20address=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20single=20API=20call,=20fix=20types,=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call get_api_users() once and pass result to both consumers. Fix return type to tuple[str | None, ...]. Use case-insensitive parent/last committer comparison. Remove dead GITHUB_WEB_FLOW_USER_ID constant. Update all test patches for renamed method. Add pre-flight probe comment. Assisted-by: Claude Signed-off-by: rnetser --- examples/config.yaml | 2 +- webhook_server/config/schema.yaml | 2 +- webhook_server/libs/github_api.py | 84 ++++++++----------- .../libs/handlers/runner_handler.py | 4 +- webhook_server/tests/conftest.py | 3 +- .../tests/test_api_call_counting.py | 3 +- webhook_server/tests/test_github_api.py | 29 +++---- webhook_server/tests/test_security_checks.py | 3 +- webhook_server/utils/constants.py | 1 - 9 files changed, 61 insertions(+), 70 deletions(-) diff --git a/examples/config.yaml b/examples/config.yaml index e72f70f5a..cd77362f0 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -150,7 +150,7 @@ security-checks: - ".github/workflows/" - ".github/actions/" committer-identity-check: true - trusted-committers: # Additional trusted committers (server bot, web-flow, and API users are auto-included) + trusted-committers: # Additional trusted committers (GitHub App bot, web-flow, and API users are auto-included) - "pre-commit-ci[bot]" repositories: diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index f281a45af..301fecfb0 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -101,7 +101,7 @@ $defs: 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 server's GitHub App bot, web-flow, and github-tokens API users are + 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 diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 948086955..28d492fcd 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -505,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"] @@ -527,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"] @@ -543,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: @@ -570,8 +571,8 @@ 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 server identities - await self._build_trusted_committers() + # 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}" @@ -839,13 +840,14 @@ async def process(self) -> Any: await self._update_context_metrics() return None - async def get_api_users(self) -> tuple[Any]: + async def get_api_users(self) -> tuple[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: + # 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 as ex: self.logger.warning( @@ -868,10 +870,6 @@ async def check_token(api: github.Github, token: str) -> str | None: return await asyncio.gather(*[check_token(api, token) for api, token in apis_and_tokens]) - async def add_api_users_to_auto_verified_and_merged_users(self) -> None: - results = await self.get_api_users() - self.auto_verified_and_merged_users.extend(user for user in results if user is not None) - def prepare_log_prefix(self, pull_request: PullRequest | None = None) -> str: return prepare_log_prefix( event_type=self.github_event, @@ -995,7 +993,18 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: _mandatory_raw = True self.security_mandatory: bool = _mandatory_raw - self.security_trusted_committers: list[str] = [] + _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,56 +1119,37 @@ 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) -> None: - """Build the complete trusted-committers list from all sources. + async def _build_trusted_committers(self, api_users: tuple[str | None, ...] | None = None) -> None: + """Add dynamic entries to trusted-committers list. - Sources: - - Static config entries (trusted-committers from security-checks) - - Server's GitHub App bot login (app_bot_login) + Adds: + - GitHub App bot login (app_bot_login) - GitHub's web-flow system account - - API user logins from github-tokens (via get_api_users) + - 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. """ - trusted: list[str] = [] - - # 1. Static config entries - _security_checks: dict[str, Any] | None = self.config.get_value(value="security-checks", return_on_none=None) - _security_config = _security_checks if isinstance(_security_checks, dict) else {} - _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 = [] - - for entry in _trusted_committers: - if isinstance(entry, (str, int, float)) and not isinstance(entry, bool): - normalized = str(entry).strip().lower() - if normalized and normalized not in trusted: - trusted.append(normalized) - - # 2. Server's GitHub App bot + # GitHub App bot if self.app_bot_login: bot = str(self.app_bot_login).strip().lower() - if bot and bot not in trusted: - trusted.append(bot) + if bot and bot not in self.security_trusted_committers: + self.security_trusted_committers.append(bot) - # 3. GitHub's web-flow system account + # GitHub's web-flow system account web_flow = GITHUB_WEB_FLOW_LOGIN.lower() - if web_flow not in trusted: - trusted.append(web_flow) + if web_flow not in self.security_trusted_committers: + self.security_trusted_committers.append(web_flow) - # 4. API users from github-tokens - api_users = await self.get_api_users() + # 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 trusted: - trusted.append(normalized) + if normalized and normalized not in self.security_trusted_committers: + self.security_trusted_committers.append(normalized) - self.security_trusted_committers = trusted 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: diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 414acfd3e..509d0aed2 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -371,7 +371,7 @@ async def run_security_committer_identity(self) -> None: """Check if the last committer matches the PR author. Uses a unified trusted-committers list that includes static config entries, - the server's bot login, GitHub's web-flow, and API users from github-tokens. + 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") @@ -407,7 +407,7 @@ async def run_security_committer_identity(self) -> None: ) await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output) - elif last_committer != parent_committer: + elif last_committer.lower() != parent_committer.lower(): if last_committer.lower() in self.github_webhook.security_trusted_committers: # Trusted committer — pass self.logger.info( 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/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..1c409bb6b 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,7 +999,7 @@ 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, ) as mock_add_api_users: with patch.object( @@ -2036,7 +2037,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 +2116,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,7 +2491,7 @@ 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, ) as mock_add_api_users: with patch.object( @@ -2561,7 +2562,7 @@ 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, ) as mock_add_api_users: with patch.object( diff --git a/webhook_server/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index 9245652e9..36a875831 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -22,7 +22,6 @@ COMMAND_SECURITY_OVERRIDE_STR, DEFAULT_SUSPICIOUS_PATHS, GITHUB_WEB_FLOW_LOGIN, - GITHUB_WEB_FLOW_USER_ID, SECURITY_COMMITTER_IDENTITY_STR, SECURITY_SUSPICIOUS_PATHS_STR, ) @@ -296,7 +295,7 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) """Check passes when last committer is GitHub's verified web-flow account.""" 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.last_committer_id = 19864447 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: diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 6e4c5111f..850ef37b7 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -19,7 +19,6 @@ SECURITY_SUSPICIOUS_PATHS_STR: str = "security-suspicious-paths" SECURITY_COMMITTER_IDENTITY_STR: str = "security-committer-identity" GITHUB_WEB_FLOW_LOGIN: str = "web-flow" -GITHUB_WEB_FLOW_USER_ID: int = 19864447 # GitHub's permanent system account for web UI operations (immutable ID) COMMAND_SECURITY_OVERRIDE_STR: str = "security-override" WIP_STR: str = "wip" LGTM_STR: str = "lgtm" From 8206550eb75e0e87566fe14f386d58d220b77da3 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 19:33:50 +0300 Subject: [PATCH 09/13] fix: restore web-flow ID verification guard Restores GITHUB_WEB_FLOW_USER_ID constant and web-flow impersonation guard in runner_handler. Even though web-flow is in the trusted list, the ID guard verifies the committer's user ID matches GitHub's official web-flow account (19864447). Uses case-insensitive comparison to prevent bypass via mixed-case login. Assisted-by: Claude Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 30 +++++++++++++++++++ webhook_server/tests/test_security_checks.py | 20 ++++++------- webhook_server/utils/constants.py | 1 + 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 509d0aed2..d42d1a38d 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -25,6 +25,8 @@ BUILD_CONTAINER_STR, CHERRY_PICKED_LABEL, CONVENTIONAL_TITLE_STR, + GITHUB_WEB_FLOW_LOGIN, + GITHUB_WEB_FLOW_USER_ID, PRE_COMMIT_STR, PREK_STR, PYTHON_MODULE_INSTALL_STR, @@ -409,6 +411,34 @@ async def run_security_committer_identity(self) -> None: 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" diff --git a/webhook_server/tests/test_security_checks.py b/webhook_server/tests/test_security_checks.py index 36a875831..1c65a31b1 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -22,6 +22,7 @@ COMMAND_SECURITY_OVERRIDE_STR, DEFAULT_SUSPICIOUS_PATHS, GITHUB_WEB_FLOW_LOGIN, + GITHUB_WEB_FLOW_USER_ID, SECURITY_COMMITTER_IDENTITY_STR, SECURITY_SUSPICIOUS_PATHS_STR, ) @@ -295,7 +296,7 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) """Check passes when last committer is GitHub's verified web-flow account.""" 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 = 19864447 + 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: @@ -309,24 +310,23 @@ async def test_committer_identity_web_flow(self, runner_handler: RunnerHandler) assert "trusted" in call_args.kwargs["output"]["summary"] @pytest.mark.asyncio - async def test_committer_identity_web_flow_fake_id_passes_via_trusted_list( - self, runner_handler: RunnerHandler - ) -> None: - """Web-flow with wrong user ID passes because web-flow is in trusted list.""" + async def test_committer_identity_web_flow_fake_id(self, runner_handler: RunnerHandler) -> None: + """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_success", new=AsyncMock()) as mock_success: + 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_success.assert_called_once() - call_args = mock_success.call_args + mock_failure.assert_called_once() + call_args = mock_failure.call_args assert call_args.kwargs["name"] == SECURITY_COMMITTER_IDENTITY_STR - assert "trusted" 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: diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 850ef37b7..640c6fe58 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -19,6 +19,7 @@ SECURITY_SUSPICIOUS_PATHS_STR: str = "security-suspicious-paths" SECURITY_COMMITTER_IDENTITY_STR: str = "security-committer-identity" GITHUB_WEB_FLOW_LOGIN: str = "web-flow" +GITHUB_WEB_FLOW_USER_ID: int = 19864447 COMMAND_SECURITY_OVERRIDE_STR: str = "security-override" WIP_STR: str = "wip" LGTM_STR: str = "lgtm" From 8682eb0fc89a1cc25f66a09aa6007e85ebc5bb30 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 19:46:32 +0300 Subject: [PATCH 10/13] fix: address Qodo review findings Use assert_awaited_once for async methods in security tests. Change rate-limit probe to logger.exception for traceback. Set return_value=() on get_api_users AsyncMock patches. Add null safety coercion for last_committer. Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 9 ++-- webhook_server/tests/test_github_api.py | 3 ++ webhook_server/tests/test_security_checks.py | 46 ++++++++++---------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 28d492fcd..d6bbe5e84 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -673,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, ) @@ -849,10 +849,9 @@ async def check_token(api: github.Github, token: str) -> str | None: try: # 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 as ex: - self.logger.warning( - f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', " - f"skipping. {ex}" + except Exception: + self.logger.exception( + f"{self.log_prefix} Failed to get API rate limit for token ending in '{token_suffix}', skipping" ) return None diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 1c409bb6b..38764d21c 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -1001,6 +1001,7 @@ async def test_process_status_event(self, state: str, should_recheck: bool) -> N webhook, "get_api_users", new_callable=AsyncMock, + return_value=(), ) as mock_add_api_users: with patch.object( webhook, @@ -2493,6 +2494,7 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo webhook, "get_api_users", new_callable=AsyncMock, + return_value=(), ) as mock_add_api_users: with patch.object( webhook, "get_pull_request", new_callable=AsyncMock @@ -2564,6 +2566,7 @@ async def test_process_review_thread_skips_when_conversation_resolution_disabled webhook, "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 1c65a31b1..dd27ee574 100644 --- a/webhook_server/tests/test_security_checks.py +++ b/webhook_server/tests/test_security_checks.py @@ -91,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"] @@ -108,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"] @@ -130,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"] @@ -153,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"] @@ -164,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( @@ -185,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"] @@ -249,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"] @@ -265,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"] @@ -284,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"] @@ -303,8 +303,8 @@ async def test_committer_identity_web_flow(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 "trusted" in call_args.kwargs["output"]["summary"] @@ -321,8 +321,8 @@ async def test_committer_identity_web_flow_fake_id(self, runner_handler: RunnerH 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 "web-flow" in call_args.kwargs["output"]["summary"] @@ -335,7 +335,7 @@ 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: @@ -348,7 +348,7 @@ async def test_committer_identity_mismatch_trusted(self, runner_handler: RunnerH 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_called_once() + 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"] @@ -364,7 +364,7 @@ async def test_committer_identity_mismatch_not_trusted(self, runner_handler: Run 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 "suspicious-user" in call_args.kwargs["output"]["summary"] @@ -379,7 +379,7 @@ async def test_committer_identity_unknown_not_bypassed_by_trusted_list(self, run 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 assert "could not be verified" in call_args.kwargs["output"]["summary"].lower() @@ -395,7 +395,7 @@ async def test_committer_identity_mismatch_trusted_case_insensitive(self, runner 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_called_once() + 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"] @@ -842,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: @@ -874,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() From 7e45e746ba9bba51a371c77c1d9e66abe6f14dec Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 20:13:23 +0300 Subject: [PATCH 11/13] style: fix ruff formatting in markdown code blocks Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/docs/pr-flow-api.md | 2 ++ webhook_server/tests/e2e/README.md | 3 +++ 2 files changed, 5 insertions(+) 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/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 From 72874d52d334e06415c2807c79a3d45ab0683b4d Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 21:06:14 +0300 Subject: [PATCH 12/13] docs: add inline comment for GITHUB_WEB_FLOW_USER_ID constant Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/utils/constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 640c6fe58..6e4c5111f 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -19,7 +19,7 @@ SECURITY_SUSPICIOUS_PATHS_STR: str = "security-suspicious-paths" SECURITY_COMMITTER_IDENTITY_STR: str = "security-committer-identity" GITHUB_WEB_FLOW_LOGIN: str = "web-flow" -GITHUB_WEB_FLOW_USER_ID: int = 19864447 +GITHUB_WEB_FLOW_USER_ID: int = 19864447 # GitHub's permanent system account for web UI operations (immutable ID) COMMAND_SECURITY_OVERRIDE_STR: str = "security-override" WIP_STR: str = "wip" LGTM_STR: str = "lgtm" From 406d4dc1e7def574c9584e13069f5bd143e463bb Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 16 Jun 2026 22:26:16 +0300 Subject: [PATCH 13/13] fix: correct get_api_users return type from tuple to list asyncio.gather returns list, not tuple. Fix return type annotation of get_api_users() and _build_trusted_committers() parameter to match. Fixes mypy CI failure. Assisted-by: Claude Signed-off-by: rnetser --- webhook_server/libs/github_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index d6bbe5e84..a1c9947c2 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -840,7 +840,7 @@ async def process(self) -> Any: await self._update_context_metrics() return None - async def get_api_users(self) -> tuple[str | 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: @@ -1118,7 +1118,7 @@ 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: tuple[str | None, ...] | None = None) -> None: + async def _build_trusted_committers(self, api_users: list[str | None] | None = None) -> None: """Add dynamic entries to trusted-committers list. Adds: