fix: verify web-flow by immutable user ID instead of author match

[myakove]

Problem

PR #1120 introduced web-flow whitelisting with author verification, but this broke when a maintainer rebases a bot's PR via GitHub UI — the commit author is the maintainer, not the PR author (bot), causing a false positive security failure.

Example: https://github.com/RedHatQE/mtv-api-tests/pull/549/checks?check_run_id=81627239510

• PR author: pre-commit-ci[bot]
• Committer: web-flow (GitHub UI rebase)
• Author: myakove (maintainer who clicked rebase)
• Result: ❌ false positive mismatch

Fix

Instead of verifying commit author matches PR author, verify web-flow by its immutable GitHub user ID (19864447). This is unforgeable — GitHub resolves user IDs server-side, and system account IDs are permanent.

Logic

• committer.login == "web-flow" AND committer.id == 19864447 → ✅ pass
• committer.login == "web-flow" AND committer.id != 19864447 → ❌ fail (possible impersonation)

Changes

• Add GITHUB_WEB_FLOW_USER_ID constant (19864447)
• Verify committer.id when login is web-flow
• Detect impersonation when login matches but ID doesn't
• Add last_committer_id attribute with github_api_call() wrapping
• Add debug logging for commit identity details
• Update tests for ID-based verification

Closes #1119

[qodo-code-review]

Code Review by Qodo

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

Context used

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

1. last_committer_id uses 0 default 📘 Rule violation ☼ Reliability

Description

The new last_committer_id handling fabricates a default 0 (via getattr(..., 0) and
self.last_committer_id initialization), which masks missing required committer data instead of
failing fast or being handled explicitly. This placeholder can flow into the web-flow identity
check and cause unknown/unverifiable IDs to be misclassified as “possible impersonation,” creating
misleading security outcomes and incident noise.

Code

webhook_server/libs/github_api.py[R674-676]

+            self.last_committer_id = await github_api_call(
+                lambda: getattr(self.last_commit.committer, "id", 0),
+                logger=self.logger,

Evidence

The change introduces self.last_committer_id: int = 0 and assigns it with
getattr(self.last_commit.committer, "id", 0), meaning that when the committer ID is absent the
system silently substitutes a fabricated 0 rather than surfacing the missing data as an error or
explicit “unknown” state (contrary to fail-fast guidance). Downstream, the runner’s web-flow
validation logic treats any last_committer_id not equal to the expected GITHUB_WEB_FLOW_USER_ID
(19864447) as suspicious/impersonation, so the defaulted value 0 is interpreted as a mismatch and
produces an incorrect “possible impersonation” classification instead of “unverifiable committer
ID.”

CLAUDE.md: Avoid Unnecessary Defensive Programming; Use Fail-Fast Errors Instead of Fake Defaults
webhook_server/libs/github_api.py[120-126]
webhook_server/libs/github_api.py[669-678]
webhook_server/libs/handlers/runner_handler.py[409-448]
webhook_server/libs/github_api.py[112-126]
webhook_server/libs/github_api.py[667-687]

Agent prompt

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

## Issue description
`last_committer_id` is currently initialized and populated using a fabricated placeholder value (`0`) via `self.last_committer_id: int = 0` and `getattr(..., 0)`. Per fail-fast guidelines, missing required committer identity data should not be silently replaced with `0`; it should either raise/propagate an explicit error or be handled as an explicit “unknown/unverifiable” state, and the `web-flow` identity check should not treat an unknown ID as “possible impersonation.”

## Issue Context
This value is used to decide whether `web-flow` commits are trusted: `RunnerHandler.run_security_committer_identity()` treats any `web-flow` commit whose `last_committer_id` is not `19864447` as suspicious. Because `GithubWebhook.process()` stores `last_committer_id` with a default of `0` when the ID is missing/unknown, commits without a resolvable committer ID are misclassified as impersonation rather than being marked unverifiable (or failing fast), which can mask upstream data issues and generate misleading security failures/incident noise. Consider treating `last_committer_id in (0, None)` as “unknown/unverifiable” with distinct messaging, optionally attempting a best-effort re-fetch/resolve of the committer ID before declaring suspicious, and updating output/log text to distinguish “ID missing” vs “ID mismatch.”

## Fix Focus Areas
- webhook_server/libs/github_api.py[120-126]
- webhook_server/libs/github_api.py[667-678]
- webhook_server/libs/github_api.py[669-678]
- webhook_server/libs/handlers/runner_handler.py[409-448]

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

---

2. Eager debug string formatting 🐞 Bug ➹ Performance

Description

GithubWebhook.process() uses an f-string inside logger.debug(), so the message is fully
interpolated on every webhook even when DEBUG logging is disabled. This adds avoidable per-webhook
overhead and prevents logging’s native lazy formatting.

Code

webhook_server/libs/github_api.py[R678-680]

+            self.logger.debug(
+                f"{self.log_prefix} Last commit: committer='{self.last_committer}' (ID: {self.last_committer_id})"
+            )

Evidence

The new code formats an f-string inside logger.debug(), which is evaluated before the logging
level check. The logger’s level is configured from config (often INFO), so this line will frequently
not be emitted but will still incur formatting work each webhook.

webhook_server/libs/github_api.py[666-680]
webhook_server/utils/helpers.py[80-104]

Agent prompt

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

### Issue description
A new `logger.debug(f"...")` call eagerly formats the debug message regardless of whether DEBUG is enabled.

### Issue Context
This runs inside `GithubWebhook.process()`, which is executed for every webhook; log level is configurable and commonly INFO, meaning this debug line will often be skipped but still pay formatting cost.

### Fix Focus Areas
- webhook_server/libs/github_api.py[678-680]

### Suggested change
Replace the f-string call with parameterized logging, e.g.:
```python
self.logger.debug(
   "%s Last commit: committer=%r (ID: %s)",
   self.log_prefix,
   self.last_committer,
   self.last_committer_id,
)
```
(Format as desired; key point is to avoid eager f-string interpolation.)

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

---

3. Web-flow skips author comparison 🐞 Bug ⛨ Security

Description

For verified web-flow commits, the committer identity check now passes solely on `committer.id ==
19864447` without comparing the PR author (or commit author), reducing the check’s ability to
surface unexpected GitHub-UI changes to the PR branch by someone other than the PR author. This is a
behavioral weakening relative to the function’s stated purpose of detecting when the last committer
differs from the PR author.

Code

webhook_server/libs/handlers/runner_handler.py[R407-448]

                )
                await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
            elif last_committer == GITHUB_WEB_FLOW_LOGIN:
-                last_author = self.github_webhook.last_author
-                if last_author == parent_committer:
+                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_WEB_FLOW_LOGIN}' "
-                        f"(GitHub web UI operation), author '{last_author}' matches PR author "
-                        f"— passing committer identity check"
+                        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}`\n"
-                            f"**Last commit author:** `{last_author}`\n\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 `web-flow` committer is GitHub's internal account for web-based operations. "
-                            f"The commit author matches the PR author."
+                            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)
-                elif last_author == "unknown":
-                    self.logger.warning(
-                        f"{self.log_prefix} Web-flow commit with unknown author: "
-                        f"PR author={parent_committer}, committer={last_committer}, author=unknown"
-                    )
-                    output = {
-                        "title": "\u274c Security: Committer Identity Unknown (web-flow)",
-                        "summary": "Web-flow commit author could not be verified",
-                        "text": (
-                            f"## Committer Identity Check\n\n"
-                            f"**PR author:** `{parent_committer}`\n"
-                            f"**Last commit committer:** `{last_committer}`\n"
-                            f"**Last commit author:** unknown\n\n"
-                            f"The last commit was made via the GitHub web UI, but the commit author "
-                            f"could not be determined. Please verify the commit authorship before merging."
-                        ),
-                    }
-                    await self.check_run_handler.set_check_failure(name=SECURITY_COMMITTER_IDENTITY_STR, output=output)
                else:
                    self.logger.warning(
-                        f"{self.log_prefix} Web-flow commit author mismatch: "
-                        f"PR author={parent_committer}, committer={last_committer}, author={last_author}"
+                        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 Mismatch (web-flow)",
-                        "summary": (
-                            f"Web-flow commit author '{last_author}' differs from PR author '{parent_committer}'"
-                        ),
+                        "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}`\n"
-                            f"**Last commit author:** `{last_author}`\n\n"
-                            f"The last commit was made via the GitHub web UI by a different user than the PR author. "
-                            f"This may indicate an unexpected web-based change to the PR branch.\n\n"
-                            f"Please verify this is expected before merging."
+                            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)

Evidence

The runner’s web-flow branch only checks last_committer_id against the constant and does not
compare to parent_committer; meanwhile, the webhook still fetches last_author, which could be
used to provide visibility if desired.

webhook_server/libs/handlers/runner_handler.py[372-377]
webhook_server/libs/handlers/runner_handler.py[409-448]
webhook_server/libs/github_api.py[679-687]

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` allow-path now always passes once the committer is verified as the official web-flow account, even if the underlying human actor/author differs from the PR author. This may be intended, but it reduces detection/visibility of non-author UI edits.

### Issue Context
- The method docstring says it checks whether last committer matches PR author.
- The current `web-flow` path no longer compares against `parent_committer` and no longer uses `last_author`.

### Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[372-448]
- webhook_server/libs/github_api.py[679-687]

### Suggested fix
- Keep the verified web-flow **pass**, but add transparency:
 - Include the commit author (or UI actor if available) in the success output/log.
 - Optionally add a non-blocking warning when `last_author` is known and differs from `parent_committer`.
 - Consider a separate “visibility” check or annotation rather than failing the security gate.

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

---

4. Unused author lookup remains ✓ Resolved 🐞 Bug ➹ Performance

Description

GithubWebhook.process() still fetches last_author via github_api_call() even though the
committer identity check no longer uses it, so this adds avoidable work (and an extra failure
surface) to every PR processing. The value is currently only used for debug logging.

Code

webhook_server/libs/github_api.py[R679-687]

            self.last_author = await github_api_call(
                lambda: getattr(self.last_commit.author, "login", "unknown"),
                logger=self.logger,
                log_prefix=self.log_prefix,
            )
+            self.logger.debug(
+                f"{self.log_prefix} Last commit: committer='{self.last_committer}' (ID: {self.last_committer_id}), "
+                f"author='{self.last_author}'"
+            )

Evidence

The webhook still computes and logs last_author, but the updated committer identity check no
longer references author information in any branch (including the web-flow path).

webhook_server/libs/github_api.py[667-687]
webhook_server/libs/handlers/runner_handler.py[372-448]

Agent prompt

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

### Issue description
`last_author` is always computed via `github_api_call()` even though it is no longer used by the security check and is only emitted in a debug log. This adds unnecessary overhead on the hot path.

### Issue Context
- `last_author` is retrieved unconditionally in `GithubWebhook.process()`.
- `RunnerHandler.run_security_committer_identity()` no longer references `last_author`.

### Fix Focus Areas
- webhook_server/libs/github_api.py[679-687]
- webhook_server/libs/handlers/runner_handler.py[372-448]

### Suggested fix
- Only fetch/log `last_author` when `logger.isEnabledFor(logging.DEBUG)`.
- Or remove `last_author` retrieval entirely if it no longer serves a purpose elsewhere.

ⓘ 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: verify web-flow by immutable user ID instead of author match
🐞 Bug fix 🕐 10-20 Minutes

Description

• Fixes a false-positive security failure when a maintainer rebases a bot's PR via GitHub UI, which
 changed the commit author away from the PR author.
• Replaces the fragile author-match check for web-flow commits with an immutable GitHub user ID
 comparison (GITHUB_WEB_FLOW_USER_ID = 19864447).
• Adds impersonation detection: if committer.login == "web-flow" but the ID doesn't match, the
 check fails with a "possible impersonation" warning.
• Removes the now-redundant "unknown author" failure branch for web-flow commits, simplifying the
 logic.
• Adds last_committer_id attribute to GithubWebhook populated via github_api_call(), and adds
 debug logging for commit identity details.

Diagram

graph TD
    A["GitHub Webhook Event"] --> B["GithubWebhook.process()"]
    B --> C["github_api_call()"] --> D["last_committer_id"]
    B --> E["run_security_committer_identity()"]
    D --> E
    E --> F{"committer.login == web-flow?"}
    F -- Yes --> G{"committer.id == 19864447?"}
    G -- Yes --> H(["✅ Check Pass"])
    G -- No --> I(["❌ Impersonation Fail"])
    F -- No --> J{"committer == PR author?"}
    J -- Yes --> H
    J -- No --> K(["❌ Mismatch Fail"])

    subgraph Legend
      direction LR
      _proc["Process Step"] ~~~ _dec{"Decision"} ~~~ _out(["Outcome"])
    end

Loading

High-Level Assessment

The ID-based approach is the correct solution here. Alternatives considered: (1) checking both author and committer against the PR author — still breaks on maintainer-triggered rebases; (2) using the web-flow login alone without ID verification — simpler but allows impersonation via a user renaming their account to 'web-flow' before GitHub reserved it; (3) using a GitHub App installation token check — far more complex with no practical security benefit. Verifying by immutable numeric user ID is the standard GitHub-recommended pattern for identifying system accounts and is unforgeable server-side.

Files changed (4) +37 / -62

Bug fix (3) +29 / -38

constants.pyAdd GITHUB_WEB_FLOW_USER_ID constant (19864447) +1/-0

Add GITHUB_WEB_FLOW_USER_ID constant (19864447)

• Adds the immutable GitHub user ID for the web-flow system account as a named constant. This value is GitHub's permanent, server-assigned ID for the account that performs web UI operations like rebases and merges.

webhook_server/utils/constants.py

github_api.pyAdd last_committer_id attribute populated via github_api_call() +10/-0

Add last_committer_id attribute populated via github_api_call()

• Initializes 'last_committer_id' to '0' in 'GithubWebhook.__init__' and populates it during 'process()' by fetching 'committer.id' from the last commit via the existing 'github_api_call()' wrapper. Also adds a debug log line showing committer login, ID, and author for each processed PR.

webhook_server/libs/github_api.py

runner_handler.pyReplace author-match check with immutable user ID verification for web-flow +18/-38

Replace author-match check with immutable user ID verification for web-flow

• Replaces the three-branch web-flow logic (author match / unknown author / mismatch) with a single two-branch check: pass if 'committer.id == GITHUB_WEB_FLOW_USER_ID', fail with an impersonation warning otherwise. Updates all check-run output messages to reflect the new ID-based verification approach.

webhook_server/libs/handlers/runner_handler.py

Tests (1) +8 / -24

test_security_checks.pyUpdate web-flow tests to use ID-based verification +8/-24

Update web-flow tests to use ID-based verification

• Replaces 'last_author' setup with 'last_committer_id' in the web-flow success test. Renames 'test_committer_identity_web_flow_author_mismatch' to 'test_committer_identity_web_flow_fake_id' and updates it to assert on a fake numeric ID. Removes the now-obsolete 'test_committer_identity_web_flow_unknown_author' test case.

webhook_server/tests/test_security_checks.py

[myakove]

@qodo-code-review[bot]

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

webhook_server/libs/github_api.py:674 (qodo rule violation) — last_committer_id uses 0 default

Skipped: By design per issue #1119 spec. Default 0 is intentional — a missing ID on a web-flow-named committer IS suspicious. This is not a fail-fast violation.

webhook_server/libs/handlers/runner_handler.py:407 (qodo bug) — Web-flow skips author comparison

Skipped: By design per issue #1119 spec. Author comparison removed intentionally — broke on bot PR rebases by maintainers.

webhook_server/libs/github_api.py:679 (qodo bug) — Unused author lookup remains

Skipped: By design per issue #1119 spec. last_author kept for debug logging observability.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 80415f2

[myakove]

@qodo-code-review[bot]

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

webhook_server/libs/github_api.py:674 (qodo rule violation) — last_committer_id uses 0 default

Skipped: Already addressed in previous cycle — by design per issue #1119 spec.

webhook_server/libs/github_api.py:678 (qodo bug) — Eager debug string formatting

Skipped: By design — f-string debug logging is the standard pattern used throughout this codebase.

webhook_server/libs/handlers/runner_handler.py:407 (qodo bug) — Web-flow skips author comparison

Skipped: Already addressed in previous cycle — by design per issue #1119 spec.

[myakove-bot]

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