feat(gitter): affected commits graph walking#4974
feat(gitter): affected commits graph walking#4974Ly-Joey wants to merge 19 commits intogoogle:masterfrom
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for determining affected commits by walking the git graph. However, it also introduces several critical security vulnerabilities, primarily related to command injection and SSRF. The most severe issue is in buildCommitGraph, where user-controlled data is concatenated into a shell command, and new endpoints lack the protocol validation present in existing ones, allowing for command execution via git protocols. Additionally, a potential Denial of Service vulnerability due to unvalidated input length during hash parsing was identified. Beyond these security concerns, a critical bug was found in the graph building logic, and there are opportunities to improve code structure and testing practices to enhance maintainability and robustness.
| if status := rr.Code; status != tt.expectedCode { | ||
| t.Errorf("handler returned wrong status code: got %v want %v", | ||
| status, tt.expectedCode) | ||
| } |
There was a problem hiding this comment.
The test case for a valid request (Valid range in public repo) only checks for http.StatusOK. It would be more robust to also validate the response body to ensure the correct affected commits are returned. You could unmarshal the protobuf response and compare it against an expected set of commits.
Ly-Joey
force-pushed
the
feat-gitter-affected-commits
branch
from
4227a68 to
97b9e25
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a new /affected-commits API endpoint to the gitter service, enabling clients to query for commits affected by specific events (introduced, fixed, last_affected, limit) within a Git repository. This includes implementing graph traversal logic (Affected and Limit methods) with support for cherry-pick detection, defining new Protobuf messages for the API response, and adding URL validation to both the new and existing /cache handlers. The Commit struct was updated to use Refs instead of Tags, and rootCommits were added to the Repository struct. Review comments highlighted several areas for improvement: optimizing regex compilation for URL validation in both cacheHandler and affectedCommitsHandler by compiling it once at the package level; enhancing test robustness and readability in TestAffectedCommitsHandler by using EventType constants instead of hardcoded strings; clarifying the Limit function's assumption about linear history when traversing commits by following only the first parent; and correcting a bug in the decodeSHA1 helper function in repository_test.go where fmt.Sprintf incorrectly pads with spaces instead of zeros, which would cause hex.DecodeString to fail.
1 participant
Add graph related logic and find affected commits.
Definition of affected commits:
Key changes
New endpoint:
POST /affected-commits:Cherry-pick Detection:
Optional support to expand introduced and fixed commits to include their cherry-picked equivalents (determined with PatchID)
There's a follow-up PR (WIP) to refactor gitter and add LRU cache of the in-memory repo.