Skip to content

fix(controlplane): end git options before ls-remote source and ref - #43

Merged
Steel-tech merged 1 commit into
mainfrom
fix/ls-remote-end-of-options
Nov 4, 2026
Merged

Steel-tech merged 1 commit into
mainfrom
fix/ls-remote-end-of-options

Conversation

@Steel-tech

@Steel-tech Steel-tech commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

What

Adds -- to the git ls-remote call that pins a run target's base commit (resolveBaseSHA).

Why

This is hardening. Nothing was exploitable before the change:

  • git does not parse options after the repository argument. Checked on git 2.56: a ref of --upload-pack=touch X resolves as a plain name and runs nothing.
  • NormalizeRepositoryIdentity only produces an absolute path or https://github.com/... as the source, never a value starting with -.

The safety rested on both of those facts. With --, it no longer does. This also answers CodeQL's go/command-injection alerts #2 and #3.

Verification

  • New test TestPinningNeverReadsASourceOrRefAsAGitOption passes a source and a ref of --upload-pack=touch <marker> and checks the marker never appears.
  • With the runs.go change reverted and the test kept, the test fails (git ran --upload-pack), so it covers the fix.
  • just check passes: format, vet, boundary, definitions, all tests, build.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Source addresses and revision names that begin with a hyphen are now handled as values during revision pinning, rather than being interpreted as Git options. Invalid inputs are rejected without running unintended commands. This improves the reliability of pinning for unusual source addresses and revision names while preserving existing timeout and error behavior.

Pinning ran `git ls-remote --exit-code <source> <ref>`. Neither value can
be read as an option today: git parses nothing after the repository, and
normalization keeps sources off "-". CodeQL flagged it anyway
(go/command-injection, alerts #2 and #3). "--" makes the guarantee local
instead of resting on both facts.

The test passes a source and a ref of --upload-pack=touch <marker> and
checks the marker never appears. It fails with "--" removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

resolveBaseSHA now passes -- before the source and ref when it runs git ls-remote. A new test checks that option-shaped values fail and do not run the command that creates a marker file.

Changes: Git operand safety

Layer / File(s) Summary
Terminate Git option parsing
internal/controlplane/runs.go, internal/controlplane/runs_test.go
resolveBaseSHA passes -- before the source and ref. The test checks that option-shaped source and ref values fail without creating a marker file.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: Minimal · up to 7a803

The change hardens Git argument handling without changing normal operand handling. No actionable merge blocker is identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main change: the fix adds an end-of-options marker (--) to the git ls-remote call in resolveBaseSHA, so the source and ref are read as operands and not as options. The title …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: the fix adds an end-of-options marker (--) to the git ls-remote call in resolveBaseSHA, so the source and ref are read as operands and not as options. The title follows the conventional commit format and is specific. The raw summary and objectives confirm the change in internal/controlplane/runs.go and a regression test in runs_test.go; the title covers the primary change and does not need to list the test.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 23815f14-ccc9-449d-9d05-5359795cdadf
Commits

Reviewing files that changed from the base of the PR and between b65ca96 and 7a8033c.

Files selected for processing: 2
  • internal/controlplane/runs.go
  • internal/controlplane/runs_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@Steel-tech
Steel-tech merged commit c7dad3e into main Nov 4, 2026
9 checks passed
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