Bug Report: Inconsistent and weak path-traversal validation across upload/view endpoints in `server.py`
- Dominant language
- Python
- Stars
- 133k
- Forks
- 15.7k
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 158
Description
### Custom Node Testing
- [ ] I have tried disabling custom nodes and the issue persists (see [how to disable custom nodes](https://docs.comfy.org/troubleshooting/custom-node-issues#step-1%3A-test-with-all-custom-nodes-disabled) if you need help)
### Expected Behavior
User-supplied filename / subfolder values reaching the `/upload/image`, `/upload/mask`, and `/view` endpoints should be normalized and validated against a single, canonical "is this path inside the allowed base directory?" helper. Validation logic and rejection behavior should be identical across all endpoints that accept a user-controlled path component.
### Actual Behavior
[server.py](server.py) currently has **five distinct path-traversal checks** spread across three endpoints. Two different primitives are used, and they do not agree on what counts as unsafe input:
| # | Location | Check |
|---|---|---|
| 1 | [server.py:402](server.py#L402) (`/upload/image` via `image_upload`) | `os.path.commonpath((upload_dir, filepath)) != upload_dir` → `400` |
| 2 | [server.py:468](server.py#L468) (`/upload/mask` `image_save_function`) | `filename[0] == '/' or '..' in filename` → `400` |
| 3 | [server.py:480](server.py#L480) (`/upload/mask` `image_save_function`) | `os.path.commonpath((os.path.abspath(full_output_dir), output_dir)) != output_dir` → `403` |
| 4 | [server.py:525](server.py#L525) (`/view`) | `filename[0] == '/' or '..' in filename` → `400` |
| 5 | [server.py:537](server.py#L537) (`/view`) | `os.path.commonpath((os.path.abspath(full_output_dir), output_dir)) != output_dir` → `403` |
Concrete consequences:
1. **Inconsistent status codes.** The same logical condition ("path escapes base dir") returns `400` from `/upload/image` and `403` from `/upload/mask` and `/view`. Clients/tooling cannot rely on a single rejection signal.
2. **Weak primitive in checks #2 and #4.** The substring test `'..' in filename` is a known-weak guard. It does not cover:
- Backslash separators on Windows (`..\foo`) — substring still hits `..` but the `filename[0] == '/'` test misses absolute Windows paths like `C:\...` and UNC paths like `\\server\share`.
- Percent-encoded variants if upstream decoding ever changes (`%2e%2e`).
- Symlinked paths inside `upload_dir` that resolve outside it — `commonpath` on the unresolved string does not call `os.path.realpath`.
- Embedded null bytes that some platforms truncate.
3. **Drift risk.** Five copies of "the same" check guarantee that a future fix to one will leave the others behind. The existing pair (string-prefix check + `commonpath` check) inside the same function ([server.py:468](server.py#L468) and [server.py:480](server.py#L480)) already shows that drift has happened.
4. **Untestable.** The validation is inlined in nested closures inside `PromptServer.__init__`. There is no importable function to unit-test against a path-traversal corpus.
### Steps to Reproduce
This is a code-quality / security-hardening report rather than a single-input crash repro. To verify the duplication and divergence:
```powershell
# From the repo root
Select-String -Path .\server.py -Pattern 'commonpath|\.\.|abspath' -SimpleMatch:$false |
Select-Object LineNumber, Line
```
You should see entries on lines 400, 402, 468, 480, 525, 537.
To verify the status-code divergence empirically (with the server running on the default port):
1. Start ComfyUI with `--disable-all-custom-nodes`.
2. POST a multipart form to `/upload/image` with `subfolder=../../etc` → observe `400`.
3. POST a multipart form to `/upload/mask` with an `original_ref` JSON whose `filename` starts with `/` → observe `400` from check #2 *or* `403` from check #3, depending on which condition fires first.
4. GET `/view?filename=../etc/passwd&subfolder=..` → observe `400` or `403`, again depending on path through the validation.
### Debug Logs
```powershell
# No runtime crash — this is a code-path/security-hardening issue.
# The evidence is the source itself; see line references in "Actual Behavior".
```
### Other
### Proposed minimal fix (small, mergeable, security-positive)
1. Add `comfy/security/path_validator.py` with a single canonical helper, e.g.:
```python
from pathlib import Path
def resolve_safe_path(base_dir: str | Path, user_path: str | Path) -> Path | None:
"""Return the resolved absolute path if it stays inside base_dir, else None.
Resolves symlinks via Path.resolve() and uses Path.is_relative_to() for the
containment check (Python 3.9+)."""
base = Path(base_dir).resolve()
try:
candidate = (base / user_path).resolve()
except (OSError, ValueError):
return None
if not candidate.is_relative_to(base):
return None
return candidate
```
2. Replace all five sites in `server.py` with calls to this helper. Standardize on a single rejection status code (suggest `400` since the input is malformed; `403` implies authentication context that does not apply here).
3. Add unit tests covering: `..` segments, absolute paths, Windows backslash and drive-letter paths, UNC paths, symlink escape, embedded null bytes, and the empty-string / dot cases.
### Out of scope for this issue
The broader proposal to split `PromptServer` into `routes/*.py` modules and turn `__init__` into a thin composition root is **not** part of this report. That is a much larger refactor with extension-API stability implications and should be discussed separately if desired. Filing this issue narrowly so the security-relevant change can land independently.
### Notes on related claims that did *not* check out
- "50+ route handlers" — actually 25 (`@routes.(get|post|...)` decorator count in `server.py`).
- "Helpers re-bound per request" — incorrect. `PromptServer.__init__` runs once per instance, so the closures bind once at startup. The cost of the closure pattern is purely architectural (untestable / unimportable), not per-request overhead.
Contributor guide
Assessment
This issue has not been assessed yet.