feat(gitter): affected commits graph walking#4974
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.
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.
another-rex
left a comment
another-rex
left a comment
There was a problem hiding this comment.
Oh btw for the bfs and the queue, did you know about: https://pkg.go.dev/container/list
Removed isIntroduced arg from parseHashes since it's unnecessary.
Add graph related logic and find affected commits to gitter Definition of affected commits: > A `commit` is affected when: from at least one `introduced` that is an ancestor of the `commit`, there is no path between them that passes through a fix. A fix can either be a `fixed` commit, or the children of a `lastAffected` commit. ### Key changes New endpoint: - `POST /affected-commits`: - Accepts a list of events (each containing an eventType (introduced, fixed, lastAffected, or limit) and commit hash) - Returns a list of affected commits and refs (tags and branches) Cherry-pick Detection (optional): Expand introduced, fixed, limit commits to include their cherry-picked equivalents (determined with PatchID)
4 participants
Add graph related logic and find affected commits to gitter
Definition of affected commits:
Key changes
New endpoint:
POST /affected-commits:Cherry-pick Detection (optional):
Expand introduced, fixed, limit commits to include their cherry-picked equivalents (determined with PatchID)