Skip to content

Harden dynamic exec arguments in experiments/dependabot paths - #52905

Merged
pelikhan merged 6 commits into
mainfrom
copilot/sighthound-fix-security-findings
Aug 15, 2026
Merged

Harden dynamic exec arguments in experiments/dependabot paths#52905
pelikhan merged 6 commits into
mainfrom
copilot/sighthound-fix-security-findings

Conversation

Copilot AI commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Sighthound reported high-confidence command-injection findings around dynamic exec.Command arguments. Most call sites were already shell-safe; this change closes remaining gaps by enforcing strict argument validation and preventing npm lifecycle-script execution in lockfile generation.

  • Git show argument hardening (pkg/cli/experiments_command.go)

    • Added buildSafeGitShowObjectArg(ref, fileName) to validate both components before building ref:path.
    • Added isSafeGitTreePath(fileName) to reject unsafe path forms (flag-like values, traversal, absolute paths, separators/characters not valid for git tree-path intent).
    • Replaced direct string concatenation at local state/evals read sites with validated object construction.
  • Dependabot lockfile generation hardening (pkg/workflow/dependabot.go)

    • Updated lockfile generation command to disable lifecycle scripts:
      • npm install --package-lock-only --ignore-scripts
      • NPM_CONFIG_IGNORE_SCRIPTS=true
    • Preserves lockfile behavior while removing script-execution risk from workflow-derived manifests.
  • Focused regression coverage

    • Added tests for safe/unsafe ref:file construction in pkg/cli/experiments_command_test.go.
    • Added test verifying npm is invoked with script-disabling args/env in pkg/workflow/dependabot_test.go.
objectArg, err := buildSafeGitShowObjectArg(ref, fileName)
if err != nil {
    return nil
}
cmd := exec.Command("git", "show", objectArg)

cmd = exec.Command(npmPath, "install", "--package-lock-only", "--ignore-scripts")
cmd.Env = append(os.Environ(), "NPM_CONFIG_IGNORE_SCRIPTS=true")

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection engine failed to produce results.

Review the workflow run logs for details.

Branch refresh requested by PR Sous Chef: https://github.com/github/gh-aw/actions/runs/31890765464> Generated by 👨‍🍳 PR Sous Chef · gpt54 · 11.2 AIC · ⊞ 8.7K ·

Comment /souschef to run again


Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

Potential security threats were detected in the agent output.

Review the workflow run logs for details.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 18.8 AIC · ⌖ 8.53 AIC · ⊞ 6.3K ·
Comment /souschef to run again

Copilot AI and others added 2 commits August 15, 2026 14:27
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix security findings in github/gh-aw Harden dynamic exec arguments in experiments/dependabot paths Aug 15, 2026
Copilot AI requested a review from pelikhan August 15, 2026 14:32
@pelikhan
pelikhan marked this pull request as ready for review August 15, 2026 14:36
Copilot AI balanced review requested due to automatic review settings August 15, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Hardens experiment-state Git access and Dependabot lockfile generation against unsafe dynamic execution.

Changes:

  • Validates Git show object arguments.
  • Disables npm lifecycle scripts during lockfile generation.
  • Adds focused regression tests.
Show a summary per file
File Description
pkg/cli/experiments_command.go Adds Git object-argument validation.
pkg/cli/experiments_command_test.go Tests accepted and rejected Git arguments.
pkg/workflow/dependabot.go Disables npm lifecycle scripts.
pkg/workflow/dependabot_test.go Verifies npm arguments and environment.

Review details

  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/cli/experiments_command.go Outdated
// buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts
// before joining them, preventing flag and path-traversal style injections.
func buildSafeGitShowObjectArg(ref, fileName string) (string, error) {
if !isSafeGitRevisionArg(ref) {
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection engine failed to produce results.

Review the workflow run logs for details.

@copilot Quick triage for maintainer-ready follow-up:

Please refresh the branch if needed, review the current security hardening changes with the pr-finisher skill, and hand this PR back to maintainers with a brief validation summary.

Outstanding review items (newest first):

  • No blocking review comments are visible yet; please proactively confirm the new git-argument validation rejects unsafe ref:path inputs without blocking valid tree paths.
  • Confirm the Dependabot lockfile path still works with --ignore-scripts and NPM_CONFIG_IGNORE_SCRIPTS=true, and summarize the focused tests you ran.

Failed checks from the compact candidate set:

  • None listed.

Branch update was requested automatically for this run when GitHub allows it.
Run context: https://github.com/github/gh-aw/actions/runs/31890765464

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 11.2 AIC · ⊞ 8.7K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

[!WARNING]
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

...

Addressed in b77a1c6.

  • Tightened local experiment/evals ref validation before git show ref:path by enforcing check-ref-format-equivalent refname rules and rejecting revision-expression forms (e.g. ~1, ^{tree}, :) while still allowing expected state refs and hex object IDs.
  • Added focused regression coverage for valid/invalid refs (including local experiments/..., unsafe revision expressions, and SHA boundary cases).
  • Re-validated Dependabot lockfile hardening path with TestGeneratePackageLock_DisablesNpmScripts (checks --ignore-scripts arg + NPM_CONFIG_IGNORE_SCRIPTS=true env).

Validation run:

  • make fmt
  • go test ./pkg/cli -run 'TestBuildSafeGitShowObjectArg|TestIsSafeExperimentStateRef'
  • go test ./pkg/workflow -run TestGeneratePackageLock_DisablesNpmScripts
  • make lint
  • make test-unit && make test (impacted-first reported missing merge-base initially, then ran successfully after fetching origin/main)
  • CI log triage for run 31890765464: no failed jobs reported.

Copilot AI requested a review from gh-aw-bot August 15, 2026 15:18
@pelikhan
pelikhan merged commit 060d503 into main Aug 15, 2026
27 checks passed
@pelikhan
pelikhan deleted the copilot/sighthound-fix-security-findings branch August 15, 2026 15:38
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.86.3

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.

[sighthound] Security findings in github/gh-aw

4 participants