Skip to content

fix: handle ValueError from os.path.commonpath on cross-drive paths (Windows)#13146

Open
Krishnachaitanyakc wants to merge 1 commit intoComfy-Org:masterfrom
Krishnachaitanyakc:fix/commonpath-valueerror-cross-drive
Open

fix: handle ValueError from os.path.commonpath on cross-drive paths (Windows)#13146
Krishnachaitanyakc wants to merge 1 commit intoComfy-Org:masterfrom
Krishnachaitanyakc:fix/commonpath-valueerror-cross-drive

Conversation

@Krishnachaitanyakc
Copy link
Copy Markdown
Contributor

Summary

Fixes #1488

On Windows, os.path.commonpath() raises ValueError when the two paths reside on different drives (e.g. C:\ vs D:\). This caused ComfyUI to crash with an unhandled exception whenever output, input, or upload directories were configured on a different drive than the ComfyUI installation.

Changes:

  • Add folder_paths.is_path_within_directory(path, directory) — a safe wrapper around os.path.commonpath that catches ValueError and returns False (different drives means the path is clearly not inside the directory)
  • Replace all direct os.path.commonpath security-check patterns in server.py (3 call sites), folder_paths.py (1 call site), and app/user_manager.py (2 call sites) with the new helper
  • In comfy/sd1_clip.py, narrow the bare except: clause to except ValueError: with a descriptive comment explaining the cross-drive scenario
  • Remove the second os.path.commonpath call that was embedded in the error message string in folder_paths.py (which itself would crash on different drives before the error could be logged)
  • Add unit tests covering the new helper function, including a mock-based test that simulates the Windows cross-drive ValueError

Test plan

  • All 19 existing + new unit tests pass (pytest tests-unit/comfy_test/folder_path_test.py)
  • Verify on Windows with output directory on a different drive (e.g. D:\output) than ComfyUI installation (e.g. C:\ComfyUI)
  • Verify image upload, image viewing, and mask upload still enforce path traversal protection
  • Verify embedding loading works correctly across drives

…Windows)

On Windows, os.path.commonpath raises ValueError when the two paths
reside on different drives (e.g. C:\ vs D:\).  This crashed ComfyUI
with an unhandled exception whenever output/input/upload directories
were on a different drive than the ComfyUI installation.

Add folder_paths.is_path_within_directory() as a safe wrapper that
catches ValueError and returns False (different drives means the path
is clearly not inside the directory).  Replace all bare
os.path.commonpath security checks across server.py, folder_paths.py,
and app/user_manager.py with this helper.

In comfy/sd1_clip.py, narrow the bare except clause to except ValueError
with a descriptive comment.

Fixes Comfy-Org#1488
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1e72a8e3-c998-4daf-b812-67fc2c0f50ed

📥 Commits

Reviewing files that changed from the base of the PR and between 5ebb0c2 and 40a25de.

📒 Files selected for processing (5)
  • app/user_manager.py
  • comfy/sd1_clip.py
  • folder_paths.py
  • server.py
  • tests-unit/comfy_test/folder_path_test.py

📝 Walkthrough

Walkthrough

The PR refactors directory traversal security checks across multiple files by introducing a new is_path_within_directory() helper function in folder_paths.py. This function safely validates whether a path resides within a target directory by normalizing paths with os.path.abspath and catching ValueError exceptions (which occur on Windows when paths span different drives). The helper replaces direct os.path.commonpath() comparisons in app/user_manager.py, server.py, and folder_paths.py. Additionally, comfy/sd1_clip.py narrows its exception handler from bare except: to specifically except ValueError:. Unit tests are added to verify the new helper function's behavior across various path scenarios.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: handling ValueError from os.path.commonpath on cross-drive paths specific to Windows.
Description check ✅ Passed The description is detailed and directly related to the changeset, explaining the Windows cross-drive issue and all code changes made.
Linked Issues check ✅ Passed The PR fully addresses issue #1488 by adding a safe wrapper function, replacing all os.path.commonpath security checks, and narrowing exception handling.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the ValueError issue from os.path.commonpath on cross-drive paths without introducing unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

ValueError exception when requesting an image on a different drive

1 participant