Skip to content

fix: address top-5 high-severity Sighthound security findings - #48824

Merged
pelikhan merged 5 commits into
mainfrom
copilot/sighthound-fix-security-findings
Jul 29, 2026
Merged

fix: address top-5 high-severity Sighthound security findings#48824
pelikhan merged 5 commits into
mainfrom
copilot/sighthound-fix-security-findings

Conversation

Copilot AI commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Sighthound flagged path traversal, SSRF, and two false positives (XSS, SSTI) across Go and JS sources. This PR applies targeted remediations and documents the false positives.

Path traversal — pkg/cli/download_workflow.go

Validate and clean the path parameter at the top of downloadWorkflowContentViaGitClone before it is written into the git sparse-checkout config or joined with the temp directory:

cleanedPath := filepath.Clean(path)
if filepath.IsAbs(cleanedPath) || strings.HasPrefix(cleanedPath, ".."+string(filepath.Separator)) || cleanedPath == ".." {
    return nil, fmt.Errorf("unsafe path in workflow reference: %q", path)
}
path = cleanedPath

Path traversal — scripts/generate-agent-factory.js

Add path.basename() guards when constructing paths from filenames to strip any unexpected directory components:

.map(file => path.join(WORKFLOWS_DIR, path.basename(file)))
// ...
const mdFilePath = path.join(WORKFLOWS_DIR, path.basename(mdFilename));

SSRF — scripts/ensure-docs-slide-pdf.js

Validate repositoryPath and ref against allowlist patterns before interpolating into the outbound fetch URL. The target domain is already hardcoded to media.githubusercontent.com; this blocks path-injection via crafted env vars or git remote URLs:

const safeRefPattern = /^[a-zA-Z0-9_.\-/]+$/;
const safeRepoPattern = /^[a-zA-Z0-9._-]+\/[a-zA-Z0-9._-]+$/;

False positives — bootstrap_profile_helpers.go + logs_format_compact.go

  • SSTI (bootstrap_profile_helpers.go:258): template source is a package-level const; html/template auto-escapes all data values. Added // #nosec G203 with explanatory comment.
  • XSS (logs_format_compact.go:193): w is always os.Stdout or a test buffer, never an HTTP response. Added a doc comment clarifying the writer contract.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 18.4 AIC · ⌖ 8.65 AIC · ⊞ 7.6K ·
Comment /souschef to run again

Copilot AI linked an issue Jul 29, 2026 that may be closed by this pull request
…e positives)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix security findings flagged by Sighthound scan fix: address top-5 high-severity Sighthound security findings Jul 29, 2026
Copilot AI requested a review from pelikhan July 29, 2026 08:51
@pelikhan
pelikhan marked this pull request as ready for review July 29, 2026 08:51
Copilot AI review requested due to automatic review settings July 29, 2026 08:51

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 workflow tooling against path traversal and URL injection while documenting two security false positives.

Changes:

  • Adds path normalization for Go and JavaScript filesystem operations.
  • Validates repository and ref URL components.
  • Documents safe template and plain-text writer usage.
Show a summary per file
File Description
pkg/cli/download_workflow.go Validates sparse-checkout paths.
scripts/generate-agent-factory.js Constrains generated paths to filenames.
scripts/ensure-docs-slide-pdf.js Adds outbound URL component validation.
pkg/cli/bootstrap_profile_helpers.go Documents static template safety.
pkg/cli/logs_format_compact.go Clarifies the writer contract.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread pkg/cli/download_workflow.go Outdated
if filepath.IsAbs(cleanedPath) || strings.HasPrefix(cleanedPath, ".."+string(filepath.Separator)) || cleanedPath == ".." {
return nil, fmt.Errorf("unsafe path in workflow reference: %q", path)
}
path = cleanedPath
Comment thread scripts/ensure-docs-slide-pdf.js Outdated
Comment on lines +107 to +110
// A SHA is 40 hex characters; branch/tag names may contain alphanumerics,
// hyphens, underscores, dots, and forward-slashes (for namespaced refs).
const safeRefPattern = /^[a-zA-Z0-9_.\-/]+$/;
if (!safeRefPattern.test(ref)) {
Comment thread scripts/ensure-docs-slide-pdf.js Outdated
Comment on lines +114 to +115
const safeRepoPattern = /^[a-zA-Z0-9._-]+\/[a-zA-Z0-9._-]+$/;
if (!safeRepoPattern.test(repositoryPath)) {
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@copilot run pr-finisher skill

Addressed all three review points in commit fix: tighten security validations per review feedback:

  • download_workflow.go: Changed path = cleanedPath to path = filepath.ToSlash(cleanedPath) so the sparse-checkout pattern file always uses forward slashes (required by Git) on all platforms. filepath.Join handles forward-slash paths correctly when constructing the local read path.
  • ensure-docs-slide-pdf.js ref: Replaced the broad safeRefPattern with /^[0-9a-f]{40}$/i since getGitRef() always returns a 40-character hex commit SHA — this rejects traversal values like ../main.
  • ensure-docs-slide-pdf.js repo: Added an explicit .split("/").some(p => p === "." || p === "..") check after the regex so dot-only components like ../repo and owner/.. are rejected even though they satisfy [a-zA-Z0-9._-]+.

@github-actions

Copy link
Copy Markdown
Contributor

Thanks @app/copilot-swe-agent for addressing the Sighthound security findings! The remediations are well-targeted and clearly documented — path traversal validations, SSRF allowlisting, and documented false positives all look solid. Here is one item that would strengthen the PR:

  • Add tests — The security validations (path sanitization in download_workflow.go and generate-agent-factory.js, ref/repo allowlisting in ensure-docs-slide-pdf.js) do not have test coverage yet. Unit tests for the happy path and rejection cases would verify the fixes work as intended and prevent regression.
Add unit tests for the security remediations in PR #48824:

1. Test the path validation in pkg/cli/download_workflow.go:downloadWorkflowContentViaGitClone
2. Test the path.basename guards in scripts/generate-agent-factory.js
3. Test the safeRefPattern and safeRepoPattern validation in scripts/ensure-docs-slide-pdf.js

Include both positive cases (valid inputs) and negative cases (traversal attempts, malformed inputs).

Generated by ✅ Contribution Check · sonnet50 · 47.8 AIC · ⌖ 11.9 AIC · ⊞ 8.4K ·

@pelikhan
pelikhan merged commit 54b0997 into main Jul 29, 2026
37 checks passed
@pelikhan
pelikhan deleted the copilot/sighthound-fix-security-findings branch July 29, 2026 09:51
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.83.5

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

3 participants