fix(visual-review): Block client-supplied github_check_run_id#60549
Merged
Conversation
…idate SHA before CI rerun Prevents a confused-deputy authorization bypass where a user with visual_review:write could inject an arbitrary GitHub Actions job ID via run metadata and force the app's GitHub App installation token (which holds actions:write) to rerun any job in the repository. Two layers of defense: - Add github_check_run_id to _RESERVED_RUN_METADATA_KEYS so the facade strips it from any client-supplied metadata at run creation time. - In _rerun_github_job, fetch the check run from GitHub and verify its head_sha matches run.commit_sha before issuing the POST rerun request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gantoine
force-pushed
the
fix/visual-review-check-run
branch
from
48ac6f9 to
e99efa9
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/visual_review/backend/logic.py:1151-1155
The `.json()` call on the successful response is outside the try/except block. If GitHub unexpectedly returns non-JSON content for a 200 (e.g., a proxy injects an HTML error page), a `json.JSONDecodeError` propagates up through `recompute_run` as an unhandled exception instead of returning the `(False, error_string)` tuple the function contract promises.
```suggestion
if check_run_response.status_code != 200:
return False, f"Could not fetch check run details (status {check_run_response.status_code})"
try:
check_run_data = check_run_response.json()
except Exception:
return False, "Failed to parse check run response"
if check_run_data.get("head_sha") != run.commit_sha:
```
Reviews (1): Last reviewed commit: "fix(visual-review): block client-supplie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Piccirello
approved these changes
May 28, 2026
rorylshanks
pushed a commit
that referenced
this pull request
May 29, 2026
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Two layered fixes:
products/visual_review/backend/facade/api.py— Addedgithub_check_run_idto_RESERVED_RUN_METADATA_KEYS. The facade's_sanitize_run_metadatanow strips this key from all client-supplied metadata before it reaches the DB, so the only waygithub_check_run_idcan enterrun.metadatais via server-side code (the CI workflow integration).products/visual_review/backend/logic.py— Added commit SHA ownership check in_rerun_github_job. Before issuing thePOST actions/jobs/{id}/rerunrequest, the function now callsGET check-runs/{id}on the GitHub API and verifies thathead_shamatchesrun.commit_sha. A mismatch returns an error and emits a structured warning logHow did you test this code?
Agent-authored. Automated tests only — no manual testing.
TestRunAPI::test_create_run_strips_reserved_metadata_keysto assertgithub_check_run_idis stripped alongside the other reserved keys.TestRerunGithubJob(4 cases): invalid ID format rejected, SHA mismatch rejected, GitHub fetch failure (404) rejected, successful rerun when SHA matches.hogli test), existingTestRecomputeRuntests unaffected (they mock_rerun_github_jobdirectly).