fix path traversal vulnerability in audio endpoint#161
Conversation
fixed minor security issue
There was a problem hiding this comment.
Pull request overview
This PR addresses a path traversal vulnerability in audio file endpoints by implementing security checks to ensure requested file paths remain within allowed directories.
Changes:
- Added path validation using
os.path.realpath()to resolve and verify file paths - Implemented directory boundary checks to prevent access outside allowed directories
- Updated error messages to be less verbose and avoid exposing internal path information
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| acestep/gradio_ui/api_routes.py | Added path traversal protection to the /v1/audio endpoint using DEFAULT_RESULTS_DIR as the allowed directory |
| acestep/api_server.py | Added path traversal protection to the /v1/audio endpoint using request.app.state.temp_audio_dir as the allowed directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | ||
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
There was a problem hiding this comment.
The path validation logic has a flaw on Windows systems. When allowed_dir ends with a drive letter path like C:\allowed, an attacker could potentially access C:\allowed_malicious because the check uses string prefix matching. Consider using os.path.commonpath([resolved_path, allowed_dir]) to verify the resolved path is within the allowed directory, or ensure allowed_dir is normalized with a trailing separator before comparison.
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") | |
| try: | |
| common_path = os.path.commonpath([resolved_path, allowed_dir]) | |
| except ValueError: | |
| # On Windows, different drives cause ValueError; treat as outside allowed directory | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") | |
| if common_path != allowed_dir: | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | ||
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
There was a problem hiding this comment.
The path validation logic has a flaw on Windows systems. When allowed_dir ends with a drive letter path like C:\allowed, an attacker could potentially access C:\allowed_malicious because the check uses string prefix matching. Consider using os.path.commonpath([resolved_path, allowed_dir]) to verify the resolved path is within the allowed directory, or ensure allowed_dir is normalized with a trailing separator before comparison.
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") | |
| try: | |
| common = os.path.commonpath([resolved_path, allowed_dir]) | |
| except ValueError: | |
| # On Windows, paths on different drives raise ValueError -> treat as outside allowed dir | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") | |
| if common != allowed_dir: | |
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | ||
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
There was a problem hiding this comment.
The path validation doesn't handle symlinks within the allowed directory that may point outside it. After validating the directory boundary, consider checking if the resolved path contains symlinks using os.path.islink() on each path component, or use Path.resolve(strict=True) which raises an exception if the path doesn't exist, preventing symlink-based bypasses.
| if not resolved_path.startswith(allowed_dir + os.sep) and resolved_path != allowed_dir: | ||
| raise HTTPException(status_code=403, detail="Access denied: path outside allowed directory") |
There was a problem hiding this comment.
The path validation doesn't handle symlinks within the allowed directory that may point outside it. After validating the directory boundary, consider checking if the resolved path contains symlinks using os.path.islink() on each path component, or use Path.resolve(strict=True) which raises an exception if the path doesn't exist, preventing symlink-based bypasses.
fixed minor security issue