Skip to content

fix(cli): validate external download inputs - #11179

Merged
marius-kilocode merged 1 commit into
mainfrom
fix/security-code-scanning-alerts
Jun 12, 2026
Merged

fix(cli): validate external download inputs#11179
marius-kilocode merged 1 commit into
mainfrom
fix/security-code-scanning-alerts

Conversation

@marius-kilocode

Copy link
Copy Markdown
Collaborator

GitHub Action comments currently allow attachment-like URLs to flow directly into authenticated requests, while language server release metadata influences archive and executable paths. This exposes request-forgery and command-injection trust boundaries reported by code scanning.

Canonicalize GitHub attachment URLs to the two supported route shapes before fetching them. Language server downloads now use fixed local archive names, clangd release tags are allowlisted before becoming executable paths, and Windows ZIP extraction passes paths through environment variables instead of embedding them in PowerShell source.

The attachment policy and regression coverage live in Kilo-owned paths. Shared OpenCode files retain only narrow, annotated validation hooks to limit future upstream merge conflicts.

Addresses:

@kilo-code-bot

kilo-code-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/opencode/src/kilocode/security/github.ts 10 encodeURIComponent on a validated UUID is a no-op — UUID characters ([0-9a-fA-F-]) are all URL-safe. Consider using the captured group directly for clarity.
Other Observations (not in diff)
  • packages/opencode/src/lsp/server.ts line 1469: release.tag_name is used unvalidated to construct assetName for the LuaLS download. This is only used for asset matching by name (not as a local file path, since the archive path is now fixed), but it's inconsistent with the Clangd fix. Low risk because the fixed archive path (lua-language-server.${ext}) is what actually hits the filesystem.
  • packages/opencode/src/lsp/server.ts line 1054/1492: asset.browser_download_url from GitHub releases API is passed directly to fetch() without URL validation (pre-existing, not introduced here). The PR correctly fixes the local path injection vectors; the SSRF surface from browser_download_url is a separate concern and out of scope for this PR.
Files Reviewed (7 files)
  • .changeset/secure-external-downloads.md
  • packages/kilo-docs/source-links.md
  • packages/opencode/src/cli/cmd/github.ts
  • packages/opencode/src/kilocode/security/github.ts
  • packages/opencode/src/lsp/server.ts
  • packages/opencode/src/util/archive.ts
  • packages/opencode/test/kilocode/archive-security.test.ts
  • packages/opencode/test/kilocode/security-github.test.ts

Overall assessment: The PR correctly addresses the three reported code-scanning alerts. The GitHubSecurity.attachment allowlist is well-designed — origin pinning, no query/fragment, and explicit UUID/numeric-ID pattern matching handle path traversal and SSRF vectors cleanly. The PowerShell env-variable approach for archive paths is the right fix. The clangd tag allowlist regex is conservative and appropriate for a version string.

Fix these issues in Kilo Cloud


Reviewed by claude-4.6-sonnet-20260217 · 1,730,931 tokens

Review guidance: REVIEW.md from base branch main

Comment thread packages/opencode/src/kilocode/security/github.ts
@marius-kilocode
marius-kilocode enabled auto-merge (squash) June 12, 2026 17:50
@marius-kilocode
marius-kilocode merged commit 96a1610 into main Jun 12, 2026
23 checks passed
@marius-kilocode
marius-kilocode deleted the fix/security-code-scanning-alerts branch June 12, 2026 18:37
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 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.

2 participants