fix coverity and codeql high severity issues#245
Conversation
📝 WalkthroughWalkthroughThis PR applies security hardening across the frontend, Electron main process, and Python services: Vue components replace ChangesSecurity Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WebUI/src/components/MarkdownRenderer.vue (1)
37-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize the fallback before assigning to
v-html.The normal path now uses DOMPurify, but the catch path still writes raw
contentintorenderedHtml; Line 82 renders that withv-html, so any input that trips the catch bypasses the new sanitizer.Proposed fix
} catch (error) { console.error('Failed to process markdown:', error) - renderedHtml.value = content.replace(/\n/g, '<br>') + renderedHtml.value = DOMPurify.sanitize(content.replace(/\n/g, '<br>'), { + ALLOWED_TAGS: ['br'], + ALLOWED_ATTR: [], + }) } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WebUI/src/components/MarkdownRenderer.vue` around lines 37 - 48, The catch block's fallback HTML assignment to renderedHtml.value is missing sanitization, creating a security vulnerability since this value is rendered with v-html. In the catch block where you assign the fallback content with content.replace(/\n/g, '<br>'), wrap that assignment with DOMPurify.sanitize() to match the sanitization applied in the normal code path, ensuring all HTML rendered to the DOM is properly sanitized regardless of error conditions.
🧹 Nitpick comments (3)
WebUI/electron/subprocesses/langchain.ts (1)
142-145: 🗄️ Data Integrity & Integration | 🔵 TrivialRename function to match implementation: use
generateFileSha256Hashinstead ofgenerateFileMD5Hash.The function at line 142 computes SHA-256 but is named
generateFileMD5Hash, creating a misleading API. WhileRagFileItem.md5: stringexists in type definitions, it appears to be unused; the active data structure isIndexedDocument.hashintextInference.ts, which treats the hash as an opaque string for deduplication and state management—SHA-256's 64-character format doesn't break equality checks or downstream logic. However, the naming mismatch is a code clarity issue that should be resolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WebUI/electron/subprocesses/langchain.ts` around lines 142 - 145, Rename the function `generateFileMD5Hash` to `generateFileSha256Hash` to accurately reflect the actual implementation. The function computes SHA-256 hashes (as seen in the `createHash('sha256')` call) rather than MD5, so the function name should be updated to match the algorithm being used. This improves code clarity and prevents confusion for developers using this function.service/model_downloader.py (1)
180-182: 📐 Maintainability & Code Quality | 🔵 TrivialRedundant
safe_relpathternary; consider a clearer containment check.The conditional on line 180 collapses to a no-op: when
path.dirname(file.relpath) == '', the file has no directory component, sopath.basename(file.relpath)returns the same value asfile.relpath. Both branches yield the same result, reducing the logic to justpath.normpath(file.relpath).The containment check itself is sound: absolute paths, path traversal attempts with
../, and join-discard cases all resolve outsidesave_path_tmpand get correctly rejected. Thestartswith(... + os.sep)pattern works, butPath.is_relative_tooros.path.commonpathexpresses the intent more idiomatically and avoids the long line.The unsanitized
file.relpathpassed downstream toHFDownloadItemis safe—it's stored as the item's name but never used for filesystem operations; all file I/O ininit_downloaduses the sanitizedsave_filename.Since the project targets Python 3.12,
Path.is_relative_to(available since 3.9) is viable.♻️ Suggested simplification
- # Prevent path traversal: verify resolved path stays within save_path_tmp - safe_relpath = path.normpath(path.basename(file.relpath) if path.dirname(file.relpath) == '' else file.relpath) - save_filename = path.abspath(path.join(self.save_path_tmp, safe_relpath)) - if not save_filename.startswith(path.abspath(self.save_path_tmp) + os.sep): - logging.warning(f'Skipping file with path traversal attempt: {file.relpath}') - continue + # Prevent path traversal: verify resolved path stays within save_path_tmp + base_tmp = Path(self.save_path_tmp).resolve() + save_path = (base_tmp / path.normpath(file.relpath)).resolve() + if not save_path.is_relative_to(base_tmp): + logging.warning(f'Skipping file with path traversal attempt: {file.relpath}') + continue + save_filename = str(save_path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/model_downloader.py` around lines 180 - 182, Simplify the ternary operator on line 180 that assigns to safe_relpath, as both branches of the condition yield the same result—just use path.normpath(file.relpath) directly. Additionally, replace the containment check pattern on line 182 that uses startswith with os.sep concatenation; instead, convert save_filename and the save_path_tmp directory to Path objects and use the is_relative_to method, which is idiomatic for Python 3.9+ and available in the project's Python 3.12 target, making the intent clearer and the code more readable.WebUI/build/scripts/after-pack.cjs (1)
46-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the original error as
causewhen rethrowing.Line 47 currently drops the original stack/context, which makes packaging failures harder to diagnose.
Suggested patch
- } catch (error) { - throw new Error(`afterPack failed: ${error.message}`) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + throw new Error(`afterPack failed: ${message}`, { + cause: error instanceof Error ? error : undefined + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@WebUI/build/scripts/after-pack.cjs` around lines 46 - 47, The catch block in the afterPack function is rethrowing an error with only the message, which drops the original error's stack trace and context. When throwing the new Error on line 47, add the original error as the cause by passing an options object as the second parameter to the Error constructor with the caught error assigned to the cause property. This preserves the full error chain for better debugging of packaging failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/web_api.py`:
- Line 16: The logging module is referenced in the logging.warning() calls at
lines 16 and 50 but is not imported at the top of the file, which will cause a
NameError at runtime. Add `import logging` at the beginning of the
service/web_api.py file, before any code that uses logging.warning(), to resolve
the undefined name error.
In `@WebUI/build/scripts/ensure-electron.mjs`:
- Around line 111-117: The empty catch block in the try-catch statement around
the looksLikeZip and rmSync calls silently swallows filesystem errors without
logging them, making it difficult to debug Electron setup failures. Instead of
an empty catch block, capture the error parameter and log it using the log
function with descriptive context about what operation failed (e.g., cache
validation or removal), include the actual error details, and consider whether
setup should continue or fail based on the error type.
In `@WebUI/electron/subprocesses/comfyuiTools.ts`:
- Line 101: The Git clone and checkout commands are vulnerable to option
injection because Git interprets arguments starting with dashes as options
rather than values. In the execFileAsync call for the clone command at line 101,
add the `--` terminator after the clone subcommand to stop Git from treating
subsequent arguments as options. Similarly, apply the same fix to the checkout
command at line 129. Additionally, before the checkout operation, validate and
reject any git references (refs) that start with a leading dash to prevent them
from being interpreted as Git options, ensuring only safe ref names are passed
to the git checkout command.
In `@WebUI/src/components/LoadingBar.vue`:
- Line 3: The issue is that util.html2Escape() is being called on porps.text
before Vue interpolation, which causes double escaping since Vue's {{ ... }}
syntax already handles HTML escaping automatically. This will result in HTML
entities being displayed as visible text (like < instead of <). Remove the
util.html2Escape() function wrapper and update the interpolation in the
LoadingBar component to use {{ porps.text }} directly without any additional
escaping.
---
Outside diff comments:
In `@WebUI/src/components/MarkdownRenderer.vue`:
- Around line 37-48: The catch block's fallback HTML assignment to
renderedHtml.value is missing sanitization, creating a security vulnerability
since this value is rendered with v-html. In the catch block where you assign
the fallback content with content.replace(/\n/g, '<br>'), wrap that assignment
with DOMPurify.sanitize() to match the sanitization applied in the normal code
path, ensuring all HTML rendered to the DOM is properly sanitized regardless of
error conditions.
---
Nitpick comments:
In `@service/model_downloader.py`:
- Around line 180-182: Simplify the ternary operator on line 180 that assigns to
safe_relpath, as both branches of the condition yield the same result—just use
path.normpath(file.relpath) directly. Additionally, replace the containment
check pattern on line 182 that uses startswith with os.sep concatenation;
instead, convert save_filename and the save_path_tmp directory to Path objects
and use the is_relative_to method, which is idiomatic for Python 3.9+ and
available in the project's Python 3.12 target, making the intent clearer and the
code more readable.
In `@WebUI/build/scripts/after-pack.cjs`:
- Around line 46-47: The catch block in the afterPack function is rethrowing an
error with only the message, which drops the original error's stack trace and
context. When throwing the new Error on line 47, add the original error as the
cause by passing an options object as the second parameter to the Error
constructor with the caught error assigned to the cause property. This preserves
the full error chain for better debugging of packaging failures.
In `@WebUI/electron/subprocesses/langchain.ts`:
- Around line 142-145: Rename the function `generateFileMD5Hash` to
`generateFileSha256Hash` to accurately reflect the actual implementation. The
function computes SHA-256 hashes (as seen in the `createHash('sha256')` call)
rather than MD5, so the function name should be updated to match the algorithm
being used. This improves code clarity and prevents confusion for developers
using this function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d47e7e17-6e3c-4295-8983-7cfbcfe25b05
📒 Files selected for processing (17)
WebUI/build/scripts/after-pack.cjsWebUI/build/scripts/ensure-electron.mjsWebUI/electron/main.tsWebUI/electron/subprocesses/comfyuiTools.tsWebUI/electron/subprocesses/langchain.tsWebUI/src/assets/js/store/comfyUiPresets.tsWebUI/src/components/AddLLMDialog.vueWebUI/src/components/LoadingBar.vueWebUI/src/components/MarkdownRenderer.vueWebUI/src/components/ProgressBar.vueWebUI/src/components/WarningDialog.vuecomfyui-deps/custom_nodes/OpenVINOImageUpscale/nodes.pycomfyui-deps/custom_nodes/aipg-auth/middleware.pyservice/model_downloader.pyservice/static/assets/history.jsservice/utils.pyservice/web_api.py
| sys.addaudithook(auditor) | ||
| except Exception: | ||
| pass | ||
| logging.warning('Failed to install audit hook - optional feature') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing logging module import causes runtime NameError.
Lines 16 and 50 reference logging.warning() but the logging module is not imported. This will cause a NameError at runtime when the audit hook fails to install or when torchvision is unavailable. Ruff correctly flags this as F821 (undefined name).
Add import logging at the top of the file to resolve this critical issue.
Also applies to: 50-50
🧰 Tools
🪛 GitHub Actions: Ruff / 0_ruff.txt
[error] 16-16: Ruff: Undefined name logging (F821).
🪛 GitHub Actions: Ruff / ruff
[error] 16-16: Ruff (rule F821): Undefined name logging.
🪛 GitHub Check: ruff
[failure] 16-16: ruff (F821)
service/web_api.py:16:5: F821 Undefined name logging
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@service/web_api.py` at line 16, The logging module is referenced in the
logging.warning() calls at lines 16 and 50 but is not imported at the top of the
file, which will cause a NameError at runtime. Add `import logging` at the
beginning of the service/web_api.py file, before any code that uses
logging.warning(), to resolve the undefined name error.
Source: Linters/SAST tools
| 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 |
There was a problem hiding this comment.
🩺 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| try { | |
| if (!looksLikeZip(cacheZip)) { | |
| log('Cached Electron zip is invalid/partial — removing it.') | |
| rmSync(cacheZip, { force: true }) | |
| } | |
| } catch (error) { | |
| const detail = error instanceof Error ? error.message : String(error) | |
| log(`Cache zip validation/invalidation failed: ${detail}`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WebUI/build/scripts/ensure-electron.mjs` around lines 111 - 117, The empty
catch block in the try-catch statement around the looksLikeZip and rmSync calls
silently swallows filesystem errors without logging them, making it difficult to
debug Electron setup failures. Instead of an empty catch block, capture the
error parameter and log it using the log function with descriptive context about
what operation failed (e.g., cache validation or removal), include the actual
error details, and consider whether setup should continue or fail based on the
error type.
| removeExistingResource(absoluteTargetDir) | ||
| const gitPath = getGitBinaryPath() | ||
| await execAsync(`"${gitPath}" clone ${gitRepoUrl} "${absoluteTargetDir}"`) | ||
| await execFileAsync(gitPath, ['clone', gitRepoUrl, absoluteTargetDir]) |
There was a problem hiding this comment.
🔒 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.
execFile removes shell injection, but Git still treats leading-dash values as options. Terminate clone options with -- and reject leading-dash refs before checkout.
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.
Context: import { exec, execFile } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WebUI/electron/subprocesses/comfyuiTools.ts` at line 101, The Git clone and
checkout commands are vulnerable to option injection because Git interprets
arguments starting with dashes as options rather than values. In the
execFileAsync call for the clone command at line 101, add the `--` terminator
after the clone subcommand to stop Git from treating subsequent arguments as
options. Similarly, apply the same fix to the checkout command at line 129.
Additionally, before the checkout operation, validate and reject any git
references (refs) that start with a leading dash to prevent them from being
interpreted as Git options, ensuring only safe ref names are passed to the git
checkout command.
Source: Linters/SAST tools
| <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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stop escaping before Vue interpolation.
{{ ... }} already text-escapes output, so util.html2Escape(porps.text) will render characters like < as visible entities instead of the original text.
Proposed fix
- <p class="v-loading-text">{{ util.html2Escape(porps.text) }}</p>
+ <p class="v-loading-text">{{ porps.text }}</p>If util is no longer used in this component:
-import * as util from '`@/assets/js/util`'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p class="v-loading-text">{{ util.html2Escape(porps.text) }}</p> | |
| <p class="v-loading-text">{{ porps.text }}</p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WebUI/src/components/LoadingBar.vue` at line 3, The issue is that
util.html2Escape() is being called on porps.text before Vue interpolation, which
causes double escaping since Vue's {{ ... }} syntax already handles HTML
escaping automatically. This will result in HTML entities being displayed as
visible text (like < instead of <). Remove the util.html2Escape() function
wrapper and update the interpolation in the LoadingBar component to use {{
porps.text }} directly without any additional escaping.
1 participant
Description:
Please provide a brief description of the changes made in this pull request.
Related Issue:
If this pull request is related to any existing issue, please mention the issue number here.
Changes Made:
Please list down the specific changes made in this pull request.
Testing Done:
Please describe the testing that has been done to ensure the changes made in this pull request are functioning as expected.
Screenshots:
If applicable, please provide screenshots or GIFs or videos to visually demonstrate the changes made.
Checklist:
Summary by CodeRabbit
Security
Bug Fixes
Improvements