-
Notifications
You must be signed in to change notification settings - Fork 3
fix coverity and codeql high severity issues #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { exec } from 'node:child_process' | ||
| import { exec, execFile } from 'node:child_process' | ||
| import { promisify } from 'node:util' | ||
| import path from 'node:path' | ||
| import fs from 'fs' | ||
|
|
@@ -12,6 +12,7 @@ import { | |
| } from './uvBasedBackends/uv' | ||
|
|
||
| const execAsync = promisify(exec) | ||
| const execFileAsync = promisify(execFile) | ||
|
|
||
| // Backend name for ComfyUI | ||
| const COMFYUI_BACKEND = 'ComfyUI' | ||
|
|
@@ -97,7 +98,7 @@ async function installGitRepo(gitRepoUrl: string, targetDir: string): Promise<vo | |
| const absoluteTargetDir = path.resolve(targetDir) | ||
| removeExistingResource(absoluteTargetDir) | ||
| const gitPath = getGitBinaryPath() | ||
| await execAsync(`"${gitPath}" clone ${gitRepoUrl} "${absoluteTargetDir}"`) | ||
| await execFileAsync(gitPath, ['clone', gitRepoUrl, absoluteTargetDir]) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Inspect Git subprocess sinks and nearby validation.
rg -n -C4 "execFileAsync\\(gitPath, \\['(clone|checkout|rev-parse)'" WebUI/electron/subprocesses/comfyuiTools.tsRepository: TNG/AI-Playground Length of output: 1471 🏁 Script executed: cat -n WebUI/electron/subprocesses/comfyuiTools.tsRepository: TNG/AI-Playground Length of output: 19164 Block Git option injection at the argument boundary.
Proposed fix async function installGitRepo(gitRepoUrl: string, targetDir: string): Promise<void> {
try {
// Ensure targetDir is an absolute path
const absoluteTargetDir = path.resolve(targetDir)
removeExistingResource(absoluteTargetDir)
const gitPath = getGitBinaryPath()
- await execFileAsync(gitPath, ['clone', gitRepoUrl, absoluteTargetDir])
+ const trimmedGitRepoUrl = gitRepoUrl.trim()
+ if (trimmedGitRepoUrl.startsWith('-')) {
+ throw new Error('Invalid git repository URL')
+ }
+ await execFileAsync(gitPath, ['clone', '--', trimmedGitRepoUrl, absoluteTargetDir])
appLoggerInstance.info(`Cloned ${gitRepoUrl} into ${absoluteTargetDir}`, 'comfyui-tools')
} catch (error) { async function checkoutGitRef(repoDir: string, gitRef?: string): Promise<void> {
// Ensure repoDir is an absolute path
const absoluteRepoDir = path.resolve(repoDir)
- if (!gitRef || !gitRef.trim()) {
+ const trimmedGitRef = gitRef?.trim()
+ if (!trimmedGitRef) {
appLoggerInstance.info(`No valid git ref provided for ${absoluteRepoDir}`, 'comfyui-tools')
const currentRef = await getGitRef(absoluteRepoDir)
appLoggerInstance.warn(`Repo ${absoluteRepoDir} remains in ref ${currentRef}`, 'comfyui-tools')
return
}
+ if (trimmedGitRef.startsWith('-')) {
+ appLoggerInstance.warn(`Invalid git ref provided for ${absoluteRepoDir}`, 'comfyui-tools')
+ return
+ }
try {
const gitPath = getGitBinaryPath()
- await execFileAsync(gitPath, ['checkout', gitRef], { cwd: absoluteRepoDir })
- appLoggerInstance.info(`Checked out ${gitRef} in ${absoluteRepoDir}`, 'comfyui-tools')
+ await execFileAsync(gitPath, ['checkout', trimmedGitRef], { cwd: absoluteRepoDir })
+ appLoggerInstance.info(`Checked out ${trimmedGitRef} in ${absoluteRepoDir}`, 'comfyui-tools')Also applies to: 129-129 🧰 Tools🪛 ast-grep (0.44.0)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| appLoggerInstance.info(`Cloned ${gitRepoUrl} into ${absoluteTargetDir}`, 'comfyui-tools') | ||
| } catch (error) { | ||
| appLoggerInstance.error( | ||
|
|
@@ -125,7 +126,7 @@ async function checkoutGitRef(repoDir: string, gitRef?: string): Promise<void> { | |
|
|
||
| try { | ||
| const gitPath = getGitBinaryPath() | ||
| await execAsync(`"${gitPath}" checkout ${gitRef}`, { cwd: absoluteRepoDir }) | ||
| await execFileAsync(gitPath, ['checkout', gitRef], { cwd: absoluteRepoDir }) | ||
| appLoggerInstance.info(`Checked out ${gitRef} in ${absoluteRepoDir}`, 'comfyui-tools') | ||
| } catch (error) { | ||
| appLoggerInstance.warn( | ||
|
|
@@ -145,7 +146,7 @@ export async function getGitRef(repoDir: string): Promise<string | undefined> { | |
| // Ensure repoDir is an absolute path | ||
| const absoluteRepoDir = path.resolve(repoDir) | ||
| const gitPath = getGitBinaryPath() | ||
| const { stdout } = await execAsync(`"${gitPath}" rev-parse HEAD`, { cwd: absoluteRepoDir }) | ||
| const { stdout } = await execFileAsync(gitPath, ['rev-parse', 'HEAD'], { cwd: absoluteRepoDir }) | ||
| return stdout.trim() | ||
| } catch (error) { | ||
| const absoluteRepoDir = path.resolve(repoDir) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| <template> | ||||||
| <div class="v-loading"> | ||||||
| <p class="v-loading-text" v-html="util.html2Escape(porps.text)"></p> | ||||||
| <p class="v-loading-text">{{ util.html2Escape(porps.text) }}</p> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Stop escaping before Vue interpolation.
Proposed fix- <p class="v-loading-text">{{ util.html2Escape(porps.text) }}</p>
+ <p class="v-loading-text">{{ porps.text }}</p>If -import * as util from '`@/assets/js/util`'📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| <div class="v-loading-bar"> | ||||||
| <div class="v-loading-bar-thumb"></div> | ||||||
| </div> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| try: | ||
| sys.addaudithook(auditor) | ||
| except Exception: | ||
| pass | ||
| logging.warning('Failed to install audit hook - optional feature') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Missing Lines 16 and 50 reference Add Also applies to: 50-50 🧰 Tools🪛 GitHub Actions: Ruff / 0_ruff.txt[error] 16-16: Ruff: Undefined name 🪛 GitHub Actions: Ruff / ruff[error] 16-16: Ruff (rule F821): Undefined name 🪛 GitHub Check: ruff[failure] 16-16: ruff (F821) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
|
|
||
|
|
||
| def get_added_dll_directories(): | ||
|
|
@@ -47,7 +47,7 @@ | |
|
|
||
| sys.modules["torchvision.transforms.functional_tensor"] = functional | ||
| except ImportError: | ||
| pass | ||
| logging.warning('torchvision not available - optional feature') | ||
|
|
||
| import hmac | ||
| import os | ||
|
|
@@ -121,7 +121,9 @@ | |
| def _enforce_loopback_and_auth(): | ||
| if request.remote_addr not in _LOOPBACK_REMOTE_ADDRS: | ||
| logging.warning( | ||
| f"rejecting non-loopback request from {request.remote_addr} to {request.path}" | ||
| "rejecting non-loopback request from %s to %s", | ||
| str(request.remote_addr).replace('\n', ' ').replace('\r', ' '), | ||
| str(request.path).replace('\n', ' ').replace('\r', ' ') | ||
| ) | ||
| return jsonify({"error": "loopback only"}), 403 | ||
| # CORS preflight requests do NOT carry the X-AIPG-Auth header by | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t silently swallow cache invalidation failures.
This
catch {}masks real filesystem errors (e.g., permissions/EBUSY), making Electron setup failures non-actionable.Suggested patch
try { if (!looksLikeZip(cacheZip)) { log('Cached Electron zip is invalid/partial — removing it.') rmSync(cacheZip, { force: true }) } -} catch { - // If file doesn't exist or other error, rmSync with force: true will handle it +} catch (error) { + const detail = error instanceof Error ? error.message : String(error) + log(`Cache zip validation/invalidation failed: ${detail}`) }📝 Committable suggestion
🤖 Prompt for AI Agents