Conversation
… and updating runtime retrieval methods; improve Bash tool to utilize runtime information for environment setup
…d improve ReadBinaryFile tool parameters for clarity
| let gitPath: string = ''; | ||
| const { gitUrl } = input; | ||
|
|
||
| if (gitUrl.includes('github.com')) { |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, the fix is to stop using substring checks on the raw URL string and instead parse the URL and compare its hostname (or host) to an explicit whitelist (here, github.com and optionally www.github.com). This ensures that github.com appearing in the path or query string does not cause the input to be treated as a GitHub URL.
For this specific code, the minimal change is:
- Parse
gitUrlwith the standardURLclass inside thepreviewGitSkillmethod. - Replace
if (gitUrl.includes('github.com'))with a safe check on the parsed hostname, e.g.if (this.isGitHubUrl(gitUrl)), whereisGitHubUrlusesnew URL()and compareshostnameto an allowlist (github.com,www.github.com). - Add a small helper method
private isGitHubUrl(rawUrl: string): booleaninside the same class (insrc/main/tools/index.ts) that:- Tries
new URL(rawUrl)inside atry/catch. - Returns
trueonly whenurl.hostnamestrictly matches the allowed values. - Returns
falseon parse errors or non-HTTP(S) schemes.
- Tries
This keeps all existing functionality (GitHub URLs still go through parseGitHubUrl, owner/repo strings still go through the other branch) while eliminating the substring-based decision. No external dependencies are needed; URL is built in to Node/Electron.
| @@ -926,6 +926,18 @@ | ||
| .find((x) => x.tool.id === id) | ||
| ?.abortController?.abort(); | ||
| } | ||
|
|
||
| private isGitHubUrl(rawUrl: string): boolean { | ||
| try { | ||
| const url = new URL(rawUrl); | ||
| const hostname = url.hostname.toLowerCase(); | ||
| return hostname === 'github.com' || hostname === 'www.github.com'; | ||
| } catch { | ||
| // Not a valid absolute URL | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| @channel(ToolChannel.PreviewGitSkill) | ||
| public async previewGitSkill(input: { gitUrl: string }) { | ||
| const tmpDir = path.join(os.tmpdir(), `git-clone-${crypto.randomUUID()}`); | ||
| @@ -935,7 +947,7 @@ | ||
| let gitPath: string = ''; | ||
| const { gitUrl } = input; | ||
|
|
||
| if (gitUrl.includes('github.com')) { | ||
| if (this.isGitHubUrl(gitUrl)) { | ||
| // Parse full GitHub URL | ||
| const parsed = this.parseGitHubUrl(gitUrl); | ||
| repoUrl = `https://github.com/${parsed.owner}/${parsed.repo}.git`; |
| let gitPath: string = ''; | ||
| const { repo_or_url } = data; | ||
|
|
||
| if (repo_or_url.includes('github.com')) { |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, the fix is to stop using substring checks like includes('github.com') on the full URL string and instead either (a) parse the URL and check its hostname equals github.com (or is on an explicit whitelist), or (b) if full host parsing is overkill here, at least ensure the string starts with an expected GitHub prefix such as https://github.com/ or http://github.com/. This avoids treating github.com appearing in the path, query, or in the middle of another host name as valid.
For this code path, the minimal, behavior‑preserving fix is to refine the branch condition to use prefix checks that indicate the URL is actually targeting GitHub: treat repo_or_url as a GitHub URL only if it starts with https://github.com/ or http://github.com/. The rest of the logic (calling this.parseGitHubUrl and constructing repoUrl/gitPath) can remain unchanged. This avoids breaking the shorthand "owner/repo" forms, which are still handled in the else branch. No new imports are strictly necessary; we can implement this using simple string methods.
Concretely, in src/main/tools/index.ts, around line 1144, replace:
if (repo_or_url.includes('github.com')) {with a stricter check such as:
if (
repo_or_url.startsWith('https://github.com/') ||
repo_or_url.startsWith('http://github.com/')
) {This ensures only genuine GitHub URLs go through the parseGitHubUrl path, and other inputs (including malformed or attacker-crafted strings containing github.com in the wrong place) fall through to the existing shorthand handling. No other lines in this file need changes for this particular issue.
| @@ -1141,7 +1141,10 @@ | ||
| let gitPath: string = ''; | ||
| const { repo_or_url } = data; | ||
|
|
||
| if (repo_or_url.includes('github.com')) { | ||
| if ( | ||
| repo_or_url.startsWith('https://github.com/') || | ||
| repo_or_url.startsWith('http://github.com/') | ||
| ) { | ||
| // Parse full GitHub URL | ||
| const parsed = this.parseGitHubUrl(repo_or_url); | ||
| repoUrl = `https://github.com/${parsed.owner}/${parsed.repo}.git`; |
No description provided.