fix: whitelist GitHub web-flow in committer identity check

[myakove]

Problem

The security committer identity check (run_security_committer_identity) produces false positives when a PR is rebased via the GitHub web UI. GitHub sets the commit committer to web-flow (its internal bot account, user ID 19864447), while preserving the original author.

The check compares last_commit.committer.login against pull_request.user.login, so it flags web-flow != <pr-author> as a security mismatch.

Same false positive triggers for:

• GitHub UI "Update branch" button (merge or rebase)
• GitHub UI commit squash
• GitHub UI file edits

Fix

• Add GITHUB_WEB_FLOW_LOGIN constant for the web-flow system account
• When last committer is web-flow, pass the identity check with an informational message
• Added test for web-flow committer scenario

Closes #1119

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 fix: security committer identity check false positive on GitHub UI rebase (web-flow)

1. Direct last_commit.author.login access ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The new self.last_author assignment accesses PyGithub commit author fields directly in an async
flow, bypassing github_api_call() (retry/backoff) and risking event-loop blocking if the property
triggers IO. This violates the requirement that all PyGithub operations (including property reads)
be wrapped with github_api_call().

Code

webhook_server/libs/github_api.py[667]

+            self.last_author = getattr(self.last_commit.author, "login", "unknown")

Evidence

PR Compliance ID 8 requires wrapping PyGithub property access via github_api_call(). The added
line assigns self.last_author using getattr(self.last_commit.author, "login", "unknown") without
github_api_call(), introducing a new direct PyGithub access path.

CLAUDE.md: Wrap all PyGithub operations with github_api_call() (no direct calls, no raw asyncio.to_thread)
webhook_server/libs/github_api.py[664-667]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`self.last_author` is computed via direct PyGithub property access (`self.last_commit.author.login`) inside an async handler, which bypasses `github_api_call()`.

## Issue Context
This code runs in `GitHubWebhook.process()` and should avoid any direct PyGithub property/method access to prevent blocking the event loop and to keep retry/backoff behavior consistent.

## Fix Focus Areas
- webhook_server/libs/github_api.py[664-667]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. web-flow logged at info ✓ Resolved 📎 Requirement gap ◔ Observability

Description

When last_committer == GITHUB_WEB_FLOW_LOGIN, the code logs with self.logger.info(...) instead
of emitting a debug-level log. This fails the requirement to add debug logging confirming the
whitelist path was triggered.

Code

webhook_server/libs/handlers/runner_handler.py[R409-412]

+                self.logger.info(
+                    f"{self.log_prefix} Last committer is '{GITHUB_WEB_FLOW_LOGIN}' "
+                    f"(GitHub web UI operation) — passing committer identity check"
+                )

Evidence

PR Compliance ID 2 requires debug-level logging when web-flow is detected. The new code path for
last_committer == GITHUB_WEB_FLOW_LOGIN currently uses self.logger.info(...), so no debug log is
emitted for that condition.

Add debug logging when 'web-flow' is detected in committer identity check
webhook_server/libs/handlers/runner_handler.py[409-412]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `web-flow` committer handling path logs at `info` level, but compliance requires debug-level logging when `web-flow` is detected.

## Issue Context
The committer identity whitelist path for GitHub's `web-flow` system account must emit a debug log so operators can confirm the condition was triggered during troubleshooting.

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[409-412]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Web-flow bypasses identity check ✓ Resolved 🐞 Bug ⛨ Security

Description

run_security_committer_identity now marks the check as success whenever `last_committer ==
"web-flow"`, skipping the mismatch failure even if the PR author differs from the actual user who
authored the last commit. This reduces the check’s ability to detect unexpected changes made via
GitHub web UI operations.

Code

webhook_server/libs/handlers/runner_handler.py[R408-425]

+            elif last_committer == GITHUB_WEB_FLOW_LOGIN:
+                self.logger.info(
+                    f"{self.log_prefix} Last committer is '{GITHUB_WEB_FLOW_LOGIN}' "
+                    f"(GitHub web UI operation) — 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}`\n\n"
+                        f"The last commit was made via the GitHub web UI (rebase, merge, or edit). "
+                        f"The `web-flow` committer is GitHub's internal account for web-based operations "
+                        f"and is trusted."
+                    ),
+                }
+                await self.check_run_handler.set_check_success(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)

Evidence

The new web-flow branch returns a success before the mismatch check, and last_committer is
sourced from last_commit.committer.login, so any last commit with committer web-flow bypasses
the intended last_committer != parent_committer failure path.

webhook_server/libs/handlers/runner_handler.py[371-447]
webhook_server/libs/github_api.py[664-667]
webhook_server/utils/constants.py[18-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`run_security_committer_identity` unconditionally passes when the last commit’s *committer* login is `web-flow`. This turns the committer identity check into a blanket allow-list for any web-UI-generated commit, even when the commit’s *author* (or triggering actor) does not match the PR author.

## Issue Context
`last_committer` is derived from `last_commit.committer.login`, while the PR author is `pull_request.user.login`. The new web-flow branch returns success before the mismatch (`last_committer != parent_committer`) failure branch.

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[371-455]
- webhook_server/libs/github_api.py[664-667]
- webhook_server/tests/test_security_checks.py[291-307]

## Suggested fix
1. When `last_committer == GITHUB_WEB_FLOW_LOGIN`, do not auto-pass. Instead, verify identity via a stronger signal, e.g.:
  - Compare `self.github_webhook.last_commit.author.login` (or equivalent) to `parent_committer`.
  - If author login is missing/unknown, treat similarly to the existing `unknown` case (fail or at least do not mark as verified).
2. Only mark success in the web-flow branch when the author (or sender/actor if you prefer) matches the PR author; otherwise fail with a clear mismatch message.
3. Update/add tests:
  - Keep the existing “web-flow committer + matching author” passing test.
  - Add a “web-flow committer + author mismatch/unknown” test that fails (or produces the intended non-success outcome).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Empty identity defaults ✓ Resolved 🐞 Bug ⛨ Security

Description

GithubWebhook.__init__ initializes last_committer/last_author to empty strings, but downstream
committer-identity logic treats only the literal string "unknown" as the unverified sentinel; if
the handler runs before process() populates these fields, the check can incorrectly pass because
parent_committer and last_committer both default to "". Use a safe sentinel (e.g., "unknown"
or None with explicit handling) to avoid false “verified” results on uninitialized objects.

Code

webhook_server/libs/github_api.py[R121-122]

+        self.last_committer: str = ""
+        self.last_author: str = ""

Evidence

The new code sets the identity fields to empty strings, while the security check only recognizes
"unknown" as the “unverifiable” state; therefore an uninitialized object can be misclassified as
verified if run_security_committer_identity() is invoked before process() populates values.

webhook_server/libs/github_api.py[118-123]
webhook_server/libs/handlers/runner_handler.py[383-389]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GithubWebhook.__init__` sets `last_committer` and `last_author` to `""`. The committer identity security check treats `"unknown"` as the only unverified value, so an uninitialized webhook object can look like a valid match (`"" == ""`) and incorrectly pass the check.

## Issue Context
- The new fields were added to ensure the attributes exist.
- Downstream, `run_security_committer_identity()` branches on `last_committer == "unknown"` to fail as unverifiable.

## Fix Focus Areas
- webhook_server/libs/github_api.py[118-123]

### Suggested fix
Set `self.last_committer` and `self.last_author` defaults to `"unknown"` (or change them to `None` and update consumers to treat `None`/empty as unknown).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Missing last_author default ✓ Resolved 🐞 Bug ☼ Reliability

Description

run_security_committer_identity directly reads self.github_webhook.last_author in the new
web-flow branch; if a caller (tests/mocks or any non-standard entrypoint) sets last_committer to
web-flow without also setting last_author, the check will raise AttributeError and fail
unexpectedly. The normal GithubWebhook.process() path sets last_author, but the handler
currently assumes the attribute always exists.

Code

webhook_server/libs/handlers/runner_handler.py[409]

+                last_author = self.github_webhook.last_author

Evidence

The new web-flow code path reads last_author as a required attribute, but neither the
GithubWebhook constructor nor the main test fixture defines it, so a web-flow committer scenario
without explicitly setting last_author will crash with AttributeError.

webhook_server/libs/handlers/runner_handler.py[408-411]
webhook_server/libs/github_api.py[112-123]
webhook_server/tests/test_security_checks.py[41-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RunnerHandler.run_security_committer_identity()` assumes `github_webhook.last_author` exists when `last_committer == GITHUB_WEB_FLOW_LOGIN`. In tests/mocks or other callers that don’t go through `GithubWebhook.process()`, this attribute may be absent, causing an `AttributeError`.

### Issue Context
- `GithubWebhook.process()` sets `self.last_author`, but `GithubWebhook.__init__()` does not initialize it.
- The unit-test fixture `mock_github_webhook` also omits `last_author` by default.

### Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[408-413]
- webhook_server/libs/github_api.py[112-123]
- webhook_server/tests/test_security_checks.py[41-60]

### Proposed fix
- In `run_security_committer_identity`, use a safe access: `last_author = getattr(self.github_webhook, "last_author", "unknown")`.
- Optionally (more robust), initialize `self.last_author: str = "unknown"` in `GithubWebhook.__init__()` so the attribute always exists.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[myakove-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Pre-commit Checks: pre-commit runs automatically if .pre-commit-config.yaml exists
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (approvers only)
• /hold cancel - Unblock PR merging
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message
• /security-override - Set security check runs to pass (maintainers only)
• /security-override cancel - Re-run security checks

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest build-container - Rebuild and test container image
• /retest python-module-install - Test Python package installation
• /retest pre-commit - Run pre-commit hooks and checks
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Container Operations

• /build-and-push-container - Build and push container image (tagged with PR number)
  • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3
• /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

• /rebase - Rebase this PR branch onto its base branch

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. LGTM Count: Minimum 1 /lgtm from reviewers
3. Status Checks: All required status checks must pass
4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• rnetser

Reviewers:

• myakove
• rnetser

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
• Test Oracle: Triggers: approved (claude/claude-opus-4-6[1m]); /test-oracle can be used anytime

Security Checks

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Container Builds: Container images are automatically tagged with the PR number
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[qodo-code-review]

PR Summary by Qodo

Fix committer identity check false positives for GitHub web-flow commits
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Description

• Whitelist GitHub web-flow as a trusted committer for web UI rebases/merges/edits.
• Emit an informational success result instead of failing on web-flow committer.
• Add test coverage for the web-flow committer scenario to prevent regressions.

Diagram

graph TD
  A["GitHub PR event"] --> B["RunnerHandler: committer check"] --> C{"Last committer"}
  B --> H["Constant: web-flow"]
  C -->|"is web-flow"| D["Check success (info)"] --> E["GitHub Check Run"]
  C -->|"matches PR author"| G["Check success"] --> E
  C -->|"mismatch/unknown"| F["Check failure"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Generalize to an allowlist of trusted GitHub system committers

• ➕ Handles future GitHub bot/automation committers without repeated one-off changes
• ➕ Can be configured per deployment (org-specific bots)
• ➖ More configuration surface area for a security-sensitive check
• ➖ Needs documentation and governance to avoid over-broad whitelisting

2. Validate by committer user ID (19864447) instead of login string

• ➕ More robust against login rename/aliasing edge cases
• ➕ Explicitly ties trust decision to a stable identifier
• ➖ Requires user-id availability in the webhook/GitHub API object path used
• ➖ May complicate tests/mocking compared to a simple login string

3. Prefer author identity over committer identity for this check

• ➕ Avoids false positives from legitimate rebases/merges performed by UI tools
• ➕ Aligns with 'who wrote the code' rather than 'who applied the operation'
• ➖ Reduces detection of cases where a different actor rewrites history intentionally
• ➖ May weaken the original intent of a committer-focused security guardrail

Recommendation: The current approach (explicitly trusting GitHub’s web-flow committer) is the best targeted fix for #1119: it preserves the committer-identity signal for real mismatches while eliminating a well-understood false positive caused by GitHub UI operations. If additional bot-driven false positives appear over time, consider evolving to a small, audited allowlist (option 1) or validating by user ID (option 2).

Files changed (3) +38 / -0

Bug fix (2) +20 / -0

runner_handler.pyTreat 'web-flow' committer as trusted for committer identity check +19/-0

Treat 'web-flow' committer as trusted for committer identity check

• Adds a 'web-flow' allow case to 'run_security_committer_identity'. When the last committer is 'web-flow', the check now succeeds with an explanatory informational message instead of failing due to mismatched PR author.

webhook_server/libs/handlers/runner_handler.py

constants.pyDefine 'GITHUB_WEB_FLOW_LOGIN' constant +1/-0

Define 'GITHUB_WEB_FLOW_LOGIN' constant

• Adds a shared constant for GitHub’s web UI committer account ('web-flow') so security checks can reference it consistently.

webhook_server/utils/constants.py

Tests (1) +18 / -0

test_security_checks.pyAdd regression test for web-flow committer passing identity check +18/-0

Add regression test for web-flow committer passing identity check

• Introduces an async test that sets 'last_committer' to 'GITHUB_WEB_FLOW_LOGIN' and asserts the check reports success with web-flow-specific messaging. Ensures the new allow behavior remains stable.

webhook_server/tests/test_security_checks.py

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4b5258d

[myakove]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/handlers/runner_handler.py:409 (qodo requirement gap) — web-flow logged at info

Addressed: Fixed — changed logger.info to logger.debug for the web-flow whitelist path, matching the issue #1119 spec requirement.

webhook_server/libs/handlers/runner_handler.py:408 (qodo bug) — Web-flow bypasses identity check

Addressed: Fixed — web-flow branch now verifies commit author matches PR author via last_commit.author.login. Author match → pass, author unknown → fail, author mismatch → fail. Added tests for all three scenarios.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4e15090

[myakove]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:667 (qodo rule violation) — Direct last_commit.author.login access

Addressed: Fixed — wrapped both last_committer and last_author with github_api_call() for retry/backoff and non-blocking IO.

webhook_server/libs/handlers/runner_handler.py:409 (qodo bug) — Missing last_author default

Addressed: Fixed — initialized last_committer and last_author in init() so the attribute always exists.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8f2e5da

[myakove]

@qodo-code-review[bot]

The following review comments were reviewed and a decision was made:

webhook_server/libs/github_api.py:121 (qodo bug) — Empty identity defaults

Addressed: Fixed — defaults changed from empty string to "unknown" sentinel, consistent with the committer identity check logic.

[myakove-bot]

New container for ghcr.io/myk-org/github-webhook-server:latest published