Skip to content

Add git reference support#1698

Closed
maxcleme wants to merge 3 commits intodocker:mainfrom
maxcleme:feature/git-reference-support
Closed

Add git reference support#1698
maxcleme wants to merge 3 commits intodocker:mainfrom
maxcleme:feature/git-reference-support

Conversation

@maxcleme
Copy link
Member

@maxcleme maxcleme commented Feb 11, 2026

Summary

This PR adds git reference support to cagent, enabling the tool to work with specific git references (branches, tags, commits) when interacting with repositories.

eg.

cagent run git@github.com:maxcleme/awesome_agent_team.git

Changes

  • Implements git reference handling capabilities
  • Allows users to specify target references for operations
  • Enhances repository interaction flexibility

Type of Change

  • Feature: New functionality for git reference support

Notes

@maxcleme maxcleme force-pushed the feature/git-reference-support branch from 9c619e0 to a596430 Compare February 11, 2026 15:17
@maxcleme maxcleme marked this pull request as ready for review February 11, 2026 15:21
@maxcleme maxcleme requested a review from a team as a code owner February 11, 2026 15:21
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

I've reviewed the git reference support implementation. The code is generally well-structured with good test coverage. I found one potential concurrency issue worth addressing.

Findings

1 MEDIUM severity issue identified - See inline comment below.

@maxcleme maxcleme marked this pull request as draft February 11, 2026 15:30
@maxcleme maxcleme marked this pull request as ready for review February 11, 2026 15:59
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Found 3 issues in the git reference handling code:

  1. Git fetch doesn't specify branch for shallow clones - When updating an already-cloned repository, the fetch command doesn't specify which branch to fetch, which can cause failures when resetting to non-default branches.

  2. Git reset assumes ref was fetched - The reset command can fail if the target ref wasn't included in the shallow fetch.

  3. Incomplete error handling for repository detection - The code doesn't properly distinguish between "repository not found" and other errors like permission issues.

All three issues are in the new git source implementation and could cause runtime failures when working with git references.

@maxcleme maxcleme marked this pull request as draft February 11, 2026 16:14
@maxcleme maxcleme force-pushed the feature/git-reference-support branch from c10ecb9 to e2b86ac Compare February 11, 2026 16:15
@maxcleme maxcleme marked this pull request as ready for review February 11, 2026 16:19
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review Summary

⚠️ CRITICAL SECURITY ISSUES FOUND

This PR introduces git reference support, but the implementation has two critical security vulnerabilities that must be addressed:

  1. Git Option Injection (HIGH): The ref parameter (after #) is not validated, allowing attackers to inject arbitrary git options like --upload-pack which could enable remote code execution
  2. Race Condition (HIGH): Concurrent access to the same git repository can cause corrupted clones or failed operations

Recommendations

For Git Option Injection:

  • Add validation to ensure ref only contains safe characters (alphanumeric, /, -, _, .)
  • Reject refs starting with -- to prevent option injection
  • Consider using a whitelist approach for allowed characters

For Race Condition:

  • Use sync.Once or a mutex to ensure only one goroutine clones a given repository
  • Consider using file-based locking for cross-process protection
  • Add tests for concurrent access scenarios

Please address these security issues before merging.

@maxcleme maxcleme marked this pull request as draft February 11, 2026 16:29
@maxcleme maxcleme force-pushed the feature/git-reference-support branch from e2b86ac to 64791dd Compare February 15, 2026 11:08
Signed-off-by: maxcleme <maxime.clement@docker.com>
Signed-off-by: maxcleme <maxime.clement@docker.com>
@maxcleme maxcleme force-pushed the feature/git-reference-support branch from 64791dd to cec35b1 Compare February 15, 2026 11:18
Signed-off-by: maxcleme <maxime.clement@docker.com>
@maxcleme maxcleme marked this pull request as ready for review February 15, 2026 20:57
@maxcleme
Copy link
Member Author

@dgageot Any idea regarding the license check? I'm having similar issue when ran locally against upstream/main? 🤔

Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Found 2 issues in the git reference support implementation that should be addressed:

High Severity (1)

  • Sensitive information disclosure: Git command errors include full output which may contain credentials or private repository URLs

Medium Severity (1)

  • Ambiguous error handling: Context cancellation/timeout errors masked by generic "git not installed" message

Overall, the implementation is solid and the approach of using moby/buildkit's dfgitutil is good. The issues are related to error handling and information security.

if ref.Ref != "" && strings.Contains(string(output), "not found") {
return cloneAndCheckout(ctx, ref, destDir)
}
return fmt.Errorf("git clone failed: %s", string(output))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY: Sensitive information disclosure in error messages

The error message includes the full git command output, which can contain sensitive information:

  1. Credentials in URLs: If ref.Remote contains embedded credentials (e.g., https://user:password@github.com/repo.git), git error messages will include them
  2. Private repository paths: Full remote URLs are exposed in error output
  3. SSH key issues: Error output may include key paths or authentication details

The safeguards GIT_TERMINAL_PROMPT=0 and SSH BatchMode=yes prevent interactive prompts but don't sanitize error output.

Recommendation: Sanitize the git output before including it in error messages. Options:

  • Extract only the error type/category without full output
  • Redact credentials from URLs in the output
  • Use a allowlist of safe error patterns to include

Example fix:

if err != nil {
    // Don't expose full git output which may contain credentials
    return fmt.Errorf("git clone failed: %w", err)
}

)
output, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("git clone failed: %s", string(output))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH SEVERITY: Sensitive information disclosure in error messages

Same issue as line 435 - the full git command output is included in the error message, which can expose:

  • Embedded credentials in git URLs
  • Private repository paths
  • SSH authentication details

This error propagates to callers and may be logged or displayed to users.

Recommendation: Sanitize or redact the git output:

if err != nil {
    return fmt.Errorf("git clone failed: %w", err)
}

func checkGitInstalled(ctx context.Context) error {
cmd := exec.CommandContext(ctx, "git", "--version")
if err := cmd.Run(); err != nil {
return errors.New("git is not installed or not in PATH")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM SEVERITY: Ambiguous error handling masks context cancellation

The function returns the same error message for all failure types:

  • Git not installed
  • Context deadline exceeded
  • Context cancelled
  • Other execution errors

When ctx is cancelled or times out, cmd.Run() returns context.Canceled or context.DeadlineExceeded, but this is masked by the generic "git is not installed or not in PATH" message. This makes debugging difficult.

Recommendation: Preserve the original error for proper diagnosis:

func checkGitInstalled(ctx context.Context) error {
    cmd := exec.CommandContext(ctx, "git", "--version")
    if err := cmd.Run(); err != nil {
        return fmt.Errorf("git check failed: %w", err)
    }
    return nil
}

This allows callers to use errors.Is() to distinguish between different error types.

@maxcleme
Copy link
Member Author

Looks like we can already achieve similar results by using plain github raw http link, eg: https://raw.githubusercontent.com/maxcleme/awesome_agent_team/refs/heads/main/agent.yaml. Private repo also works if GITHUB_TOKEN is set.

Fine by me for now.

@maxcleme maxcleme closed this Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant