Skip to content

Commit 79427a2

Browse files
authored
feat: PR security checks — suspicious paths, committer identity, auto-merge override (#1109)
<h3>PR Summary by Qodo</h3> Add PR security checks for suspicious paths, committer identity, and auto-merge override <code>✨ Enhancement</code> <code>🧪 Tests</code> <code>⚙️ Configuration changes</code> <code>📝 Documentation</code> <code>🕐 40+ Minutes</code> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <h3>Walkthroughs</h3> <details open> <summary>User Description</summary> <br/> ## Summary Three configurable security checks to detect and block malicious PR attack vectors: ### Suspicious Path Detection - New `security-suspicious-paths` check run - Configurable path prefixes (default: `.claude/`, `.vscode/`, `.cursor/`, `.devcontainer/`, `.pi/`, `.github/workflows/`, `.github/actions/`) - Fails check when PR modifies files in sensitive locations ### Committer Identity Check - New `security-committer-identity` check run - Flags when last committer differs from PR author - Handles unknown committer identity explicitly ### Auto-Merge Override - Blocks auto-merge when PR touches suspicious paths - Posts comment explaining why auto-merge was blocked ### Security Design - Config only from server-side `config.yaml` (not overridable by in-repo `.github-webhook-server.yaml`) - Prevents attackers from weakening security policy via PR Closes #1106 </details> <details open> <summary>AI Description</summary> <br/> <pre> • Add configurable PR security check runs for suspicious paths and committer identity. • Block auto-merge and comment when PR touches security-sensitive path prefixes. • Document/validate security-checks config and add full test coverage for new behavior. </pre> </details> <details> <summary>Diagram</summary> <br/> ```mermaid graph TD cfg["server config.yaml"] --> wh(["GitHubWebhook"]) --> prh(["PullRequestHandler"]) --> ofh(["OwnersFileHandler"]) prh --> rh(["RunnerHandler"]) --> crh(["CheckRunHandler"]) --> gh{{"GitHub API"}} prh -. "comment / enable automerge" .-> gh subgraph Legend direction LR _file["Config/File"] ~~~ _svc(["Handler/Service"]) ~~~ _ext{{"External"}} end ``` </details> <details> <summary>High-Level Assessment</summary> <br/> The following are alternative approaches to this PR: <details> <summary><b>1. Use CODEOWNERS + branch protection for sensitive paths</b></summary> - ➕ Native GitHub enforcement; no extra webhook-side logic - ➕ Can require reviews for .github/workflows/ and similar paths - ➖ CODEOWNERS lives in-repo; attacker PR may attempt to modify it (mitigations require additional protections) - ➖ Doesn’t detect committer mismatch vs PR author - ➖ Doesn’t provide an explicit auto-merge override message path </details> <details> <summary><b>2. Required GitHub Actions workflow with path filters</b></summary> - ➕ Clear CI signal in GitHub UI; can block merges via required checks - ➕ Easy to extend with additional detectors - ➖ Workflows are in-repo and are themselves part of the attack surface - ➖ May not run as expected depending on repo settings/permissions; weaker central enforcement than server-side config </details> <details> <summary><b>3. Org policy: require verified commits / signature enforcement</b></summary> - ➕ Stronger identity assurance than comparing GitHub usernames alone - ➕ Moves trust decision into cryptographic verification - ➖ Org-wide operational overhead; may block legitimate contributors - ➖ Doesn’t address suspicious-path modifications; still needs path-based review controls </details> **Recommendation:** The PR’s approach (server-side, non-overridable security policy + explicit check-runs + auto-merge override) is a good fit for defending against repo-config supply-chain attacks. Consider adding CODEOWNERS/branch protections as defense-in-depth, but keep these webhook checks as the centrally enforced gate. </details> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <h3>File Changes</h3> <details> <summary><strong>Enhancement</strong> (4)</summary> <blockquote> <details> <summary><strong>github_api.py</strong> <code>Load server-side security policy and make committer identity explicit</code> <code>+10/-1</code></summary> <br/> >Load server-side security policy and make committer identity explicit > ><pre> >• Sets last_committer to &quot;unknown&quot; when no GitHub user is associated with the last commit committer. Loads security-checks only from server configuration (no per-repo extra_dict override) and exposes security_suspicious_paths and security_committer_identity_check on the webhook context. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-7c5f6dfcadb38e75c2d0f1d418ba1a861cc9f6c0efe72905a250e9f43a6cfdcf'>webhook_server/libs/github_api.py</a> <hr/> </details> </blockquote> <blockquote> <details> <summary><strong>pull_request_handler.py</strong> <code>Queue/run security check-runs and block auto-merge on suspicious paths</code> <code>+63/-0</code></summary> <br/> >Queue/run security check-runs and block auto-merge on suspicious paths > ><pre> >• Adds a Security Checks section to the welcome comment, queues and launches two new security check-runs during PR open/sync, and blocks auto-merge with an explanatory PR comment when changed_files match suspicious path prefixes. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-8644dc42c86db802123c2ba72847dca72589fe19f330ecc70621af895a72fc8a'>webhook_server/libs/handlers/pull_request_handler.py</a> <hr/> </details> </blockquote> <blockquote> <details> <summary><strong>runner_handler.py</strong> <code>Implement suspicious-path and committer-identity security check runners</code> <code>+114/-0</code></summary> <br/> >Implement suspicious-path and committer-identity security check runners > ><pre> >• Adds runner methods to evaluate changed files against configured prefixes and to compare PR author vs last committer (including an explicit &quot;unknown&quot; case). Each runner publishes detailed check-run outputs and sets success/failure accordingly. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-0cb54c95cafda12d8d169c7b03ac484738f4cf925c22f6e6b8b8c5db0730ce42'>webhook_server/libs/handlers/runner_handler.py</a> <hr/> </details> </blockquote> <blockquote> <details> <summary><strong>constants.py</strong> <code>Define security check names and default suspicious path prefixes</code> <code>+14/-0</code></summary> <br/> >Define security check names and default suspicious path prefixes > ><pre> >• Introduces constants for the two new check-run names, registers them as non-overridable built-in checks, and defines the default suspicious path prefix list used for detection. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-9a2d73fb31266bc568369ee81f15b1ebb12d9703f412a0ab65cf7c5a5b98060f'>webhook_server/utils/constants.py</a> <hr/> </details> </blockquote> </details> <details> <summary><strong>Tests</strong> (2)</summary> <blockquote> <details> <summary><strong>test_pull_request_handler.py</strong> <code>Extend PR handler tests to account for new security runners</code> <code>+9/-0</code></summary> <br/> >Extend PR handler tests to account for new security runners > ><pre> >• Updates the webhook mock with security-related attributes and patches the new runner methods in existing workflow tests. Ensures PR processing test scaffolding stays compatible with the new queued/started tasks. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-ba922f135da60a85cf895a8d3fb3154c635c21019e6b7b6ab07c92caa9cbc163'>webhook_server/tests/test_pull_request_handler.py</a> <hr/> </details> </blockquote> <blockquote> <details> <summary><strong>test_security_checks.py</strong> <code>Add dedicated tests for security checks and auto-merge override</code> <code>+459/-0</code></summary> <br/> >Add dedicated tests for security checks and auto-merge override > ><pre> >• Adds coverage for suspicious path detection outcomes, committer identity match/mismatch/unknown, and auto-merge blocking behavior (including comment posting). Also asserts security constants and default suspicious path list are wired into BUILTIN_CHECK_NAMES. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-d7e0086d532052be0150224126f9b67dd174e7e039409d47316dd027f508a8c8'>webhook_server/tests/test_security_checks.py</a> <hr/> </details> </blockquote> </details> <details> <summary><strong>Other</strong> (2)</summary> <blockquote> <details> <summary><strong>config.yaml</strong> <code>Document security-checks settings and defaults in example config</code> <code>+14/-0</code></summary> <br/> >Document security-checks settings and defaults in example config > ><pre> >• Adds a new security-checks section with default suspicious path prefixes and a committer identity toggle. Provides inline commentary describing the intent of these checks. ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-e3ac27ce128a0ddae53723bc8c3133257530a13c00f97c9edfeef152ebc3b8ce'>examples/config.yaml</a> <hr/> </details> </blockquote> <blockquote> <details> <summary><strong>schema.yaml</strong> <code>Add JSON schema for security-checks configuration</code> <code>+24/-0</code></summary> <br/> >Add JSON schema for security-checks configuration > ><pre> >• Introduces a security-checks schema definition (suspicious-paths list and committer-identity-check boolean) and wires it into the root schema. Documents behavioral effects (check-run failure and auto-merge blocking). ></pre> > ><a href='https://github.com/myk-org/github-webhook-server/pull/1109/files#diff-0eaf85d7f2a5888c61710a31c06434f63fe254f177f3df114332204780388f67'>webhook_server/config/schema.yaml</a> <hr/> </details> </blockquote> </details> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <a href="https://www.qodo.ai"><img src="https://www.qodo.ai/wp-content/uploads/2025/03/qodo-logo.svg" width="80" alt="Qodo Logo"></a>
1 parent ef8d7e6 commit 79427a2

14 files changed

Lines changed: 1228 additions & 3 deletions

examples/.github-webhook-server.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,3 +171,11 @@ test-oracle:
171171
- "tests/**/*.py"
172172
triggers:
173173
- approved
174+
175+
# Security Checks (overrides global config)
176+
security-checks:
177+
mandatory: true
178+
suspicious-paths:
179+
- ".github/workflows/"
180+
- ".github/actions/"
181+
committer-identity-check: true

examples/config.yaml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,21 @@ ai-features:
136136
enabled: true
137137
timeout-minutes: 10 # Timeout in minutes for AI CLI (default: 10)
138138

139+
# Security Checks configuration
140+
# Detects potentially malicious PR patterns like modifications to
141+
# security-sensitive paths or mismatched committer identities.
142+
security-checks:
143+
mandatory: true # true (default) = blocks can-be-merged. false = advisory only
144+
suspicious-paths:
145+
- ".claude/"
146+
- ".vscode/"
147+
- ".cursor/"
148+
- ".devcontainer/"
149+
- ".pi/"
150+
- ".github/workflows/"
151+
- ".github/actions/"
152+
committer-identity-check: true
153+
139154
repositories:
140155
my-repository:
141156
name: my-org/my-repository
@@ -286,3 +301,10 @@ repositories:
286301
- "tests/**/*.py"
287302
triggers:
288303
- approved
304+
305+
# Security Checks (overrides global)
306+
# security-checks:
307+
# suspicious-paths:
308+
# - ".github/workflows/"
309+
# - ".github/actions/"
310+
# committer-identity-check: true

webhook_server/config/schema.yaml

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,34 @@ $defs:
6868
- ai-provider
6969
- ai-model
7070
additionalProperties: false
71+
security-checks:
72+
type: object
73+
description: |
74+
Security checks for pull requests. Detects potentially malicious PR patterns
75+
such as modifications to security-sensitive paths or mismatched committer identities.
76+
properties:
77+
mandatory:
78+
type: boolean
79+
description: |
80+
When true (default), security checks block can-be-merged.
81+
When false, security checks are advisory only (run but don't block merge).
82+
default: true
83+
suspicious-paths:
84+
type: array
85+
description: |
86+
List of path prefixes considered security-sensitive.
87+
PRs modifying files under these paths will fail the security-suspicious-paths check run
88+
and have auto-merge blocked.
89+
Default: [".claude/", ".vscode/", ".cursor/", ".devcontainer/", ".pi/", ".github/workflows/", ".github/actions/"]
90+
items:
91+
type: string
92+
committer-identity-check:
93+
type: boolean
94+
description: |
95+
When enabled, compares the PR author (parent committer) against the last commit's
96+
committer. Fails the security-committer-identity check run if they differ.
97+
default: true
98+
additionalProperties: false
7199
type: object
72100
properties:
73101
log-level:
@@ -213,6 +241,8 @@ properties:
213241
additionalProperties: false
214242
ai-features:
215243
$ref: '#/$defs/ai-features'
244+
security-checks:
245+
$ref: '#/$defs/security-checks'
216246
labels:
217247
type: object
218248
description: Configure which labels are enabled and their colors
@@ -631,6 +661,8 @@ properties:
631661
additionalProperties: false
632662
ai-features:
633663
$ref: '#/$defs/ai-features'
664+
security-checks:
665+
$ref: '#/$defs/security-checks'
634666
custom-check-runs:
635667
type: array
636668
description: |

webhook_server/libs/github_api.py

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
CAN_BE_MERGED_STR,
3636
CONFIGURABLE_LABEL_CATEGORIES,
3737
CONVENTIONAL_TITLE_STR,
38+
DEFAULT_SUSPICIOUS_PATHS,
3839
OTHER_MAIN_BRANCH,
3940
PRE_COMMIT_STR,
4041
PYTHON_MODULE_INSTALL_STR,
@@ -662,7 +663,7 @@ async def process(self) -> Any:
662663

663664
self.last_commit = await self._get_last_commit(pull_request=pull_request)
664665
self.parent_committer = pull_request.user.login
665-
self.last_committer = getattr(self.last_commit.committer, "login", self.parent_committer)
666+
self.last_committer = getattr(self.last_commit.committer, "login", "unknown")
666667

667668
# Store PR SHAs: prefer webhook payload (avoids race condition with live API)
668669
# For pull_request events, base.sha and head.sha are guaranteed by GitHub webhook spec.
@@ -953,6 +954,33 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None:
953954
self.ai_features: dict[str, Any] | None = self.config.get_value(
954955
value="ai-features", return_on_none=None, extra_dict=repository_config
955956
)
957+
_security_checks: dict[str, Any] | None = self.config.get_value(
958+
value="security-checks", return_on_none=None, extra_dict=repository_config
959+
)
960+
_security_config = _security_checks if isinstance(_security_checks, dict) else {}
961+
_suspicious_paths = _security_config.get("suspicious-paths", DEFAULT_SUSPICIOUS_PATHS)
962+
self.security_suspicious_paths: list[str] = (
963+
[str(p).strip() for p in _suspicious_paths if isinstance(p, (str, int, float)) and str(p).strip()]
964+
if isinstance(_suspicious_paths, list)
965+
else DEFAULT_SUSPICIOUS_PATHS
966+
)
967+
_committer_check_raw = _security_config.get("committer-identity-check", True)
968+
if not isinstance(_committer_check_raw, bool):
969+
self.logger.warning(
970+
f"{self.log_prefix} security-checks.committer-identity-check must be boolean, "
971+
f"got {type(_committer_check_raw).__name__}. Defaulting to true."
972+
)
973+
_committer_check_raw = True
974+
self.security_committer_identity_check: bool = _committer_check_raw
975+
_mandatory_raw = _security_config.get("mandatory", True)
976+
if not isinstance(_mandatory_raw, bool):
977+
self.logger.warning(
978+
f"{self.log_prefix} security-checks.mandatory must be boolean, got {type(_mandatory_raw).__name__}. "
979+
"Defaulting to true."
980+
)
981+
_mandatory_raw = True
982+
self.security_mandatory: bool = _mandatory_raw
983+
956984
_auto_merge_prs = self.config.get_value(
957985
value="set-auto-merge-prs", return_on_none=[], extra_dict=repository_config
958986
)

webhook_server/libs/handlers/check_run_handler.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
IN_PROGRESS_STR,
1919
PYTHON_MODULE_INSTALL_STR,
2020
QUEUED_STR,
21+
SECURITY_COMMITTER_IDENTITY_STR,
22+
SECURITY_SUSPICIOUS_PATHS_STR,
2123
SUCCESS_STR,
2224
TOX_STR,
2325
VERIFIED_LABEL_STR,
@@ -427,6 +429,14 @@ async def all_required_status_checks(self, pull_request: PullRequest) -> list[st
427429
check_name = custom_check["name"]
428430
all_required_status_checks.append(check_name)
429431

432+
# Add mandatory security checks
433+
if self.github_webhook.security_mandatory:
434+
if self.github_webhook.security_suspicious_paths:
435+
all_required_status_checks.append(SECURITY_SUSPICIOUS_PATHS_STR)
436+
437+
if self.github_webhook.security_committer_identity_check:
438+
all_required_status_checks.append(SECURITY_COMMITTER_IDENTITY_STR)
439+
430440
# Use ordered deduplication to combine branch and config checks without duplicates
431441
_all_required_status_checks = list(dict.fromkeys(branch_required_status_checks + all_required_status_checks))
432442
self.logger.debug(f"{self.log_prefix} All required status checks: {_all_required_status_checks}")

webhook_server/libs/handlers/issue_comment_handler.py

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from github.PullRequest import PullRequest
1010
from github.Repository import Repository
1111

12-
from webhook_server.libs.handlers.check_run_handler import CheckRunHandler
12+
from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput
1313
from webhook_server.libs.handlers.labels_handler import LabelsHandler
1414
from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler
1515
from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler
@@ -30,9 +30,12 @@
3030
COMMAND_REGENERATE_WELCOME_STR,
3131
COMMAND_REPROCESS_STR,
3232
COMMAND_RETEST_STR,
33+
COMMAND_SECURITY_OVERRIDE_STR,
3334
COMMAND_TEST_ORACLE_STR,
3435
HOLD_LABEL_STR,
3536
REACTIONS,
37+
SECURITY_COMMITTER_IDENTITY_STR,
38+
SECURITY_SUSPICIOUS_PATHS_STR,
3639
USER_LABELS_DICT,
3740
VERIFIED_LABEL_STR,
3841
WIP_STR,
@@ -171,6 +174,7 @@ async def user_commands(
171174
COMMAND_ADD_ALLOWED_USER_STR,
172175
COMMAND_REGENERATE_WELCOME_STR,
173176
COMMAND_TEST_ORACLE_STR,
177+
COMMAND_SECURITY_OVERRIDE_STR,
174178
]
175179

176180
command_and_args: list[str] = command.split(" ", 1)
@@ -362,6 +366,64 @@ async def user_commands(
362366
pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix
363367
)
364368

369+
elif _command == COMMAND_SECURITY_OVERRIDE_STR:
370+
maintainers = await self.owners_file_handler.get_all_repository_maintainers()
371+
if reviewed_user not in maintainers:
372+
msg = "Only maintainers can use `/security-override`"
373+
self.logger.debug(f"{self.log_prefix} {msg}")
374+
await github_api_call(
375+
pull_request.create_issue_comment, body=msg, logger=self.logger, log_prefix=self.log_prefix
376+
)
377+
return
378+
379+
if remove:
380+
# Re-run security checks to re-evaluate
381+
await self.runner_handler.run_security_suspicious_paths()
382+
await self.runner_handler.run_security_committer_identity()
383+
self.logger.info(f"{self.log_prefix} Security checks re-run by {reviewed_user}")
384+
await github_api_call(
385+
pull_request.create_issue_comment,
386+
body=f"Security override removed by @{reviewed_user}. Security checks re-run.",
387+
logger=self.logger,
388+
log_prefix=self.log_prefix,
389+
)
390+
else:
391+
# Set security check runs to success (override)
392+
if (
393+
not self.github_webhook.security_suspicious_paths
394+
and not self.github_webhook.security_committer_identity_check
395+
):
396+
msg = "No security checks are enabled — nothing to override."
397+
self.logger.debug(f"{self.log_prefix} {msg}")
398+
await github_api_call(
399+
pull_request.create_issue_comment, body=msg, logger=self.logger, log_prefix=self.log_prefix
400+
)
401+
return
402+
403+
override_output: CheckRunOutput = {
404+
"title": f"Overridden by maintainer @{reviewed_user}",
405+
"summary": "Security check overridden by maintainer",
406+
"text": (
407+
f"This security check was overridden by maintainer @{reviewed_user}.\n\n"
408+
"Use `/security-override cancel` to re-run security checks."
409+
),
410+
}
411+
if self.github_webhook.security_suspicious_paths:
412+
await self.check_run_handler.set_check_success(
413+
name=SECURITY_SUSPICIOUS_PATHS_STR, output=override_output
414+
)
415+
if self.github_webhook.security_committer_identity_check:
416+
await self.check_run_handler.set_check_success(
417+
name=SECURITY_COMMITTER_IDENTITY_STR, output=override_output
418+
)
419+
self.logger.info(f"{self.log_prefix} Security override applied by {reviewed_user}")
420+
await github_api_call(
421+
pull_request.create_issue_comment,
422+
body=f"Security checks overridden by @{reviewed_user}. Security check runs set to pass.",
423+
logger=self.logger,
424+
log_prefix=self.log_prefix,
425+
)
426+
365427
elif _command == WIP_STR:
366428
if not await self.owners_file_handler.is_user_valid_to_run_commands(
367429
pull_request=pull_request, reviewed_user=reviewed_user

0 commit comments

Comments
 (0)