Merge pull request #4417 from pipecat-ai/mb/resolve-runner-filepath
This commit is contained in:
1
.github/workflows/coverage.yaml
vendored
1
.github/workflows/coverage.yaml
vendored
@@ -42,6 +42,7 @@ jobs:
|
||||
--extra langchain \
|
||||
--extra livekit \
|
||||
--extra piper \
|
||||
--extra runner \
|
||||
--extra sagemaker \
|
||||
--extra tracing \
|
||||
--extra websocket
|
||||
|
||||
1
.github/workflows/tests.yaml
vendored
1
.github/workflows/tests.yaml
vendored
@@ -46,6 +46,7 @@ jobs:
|
||||
--extra langchain \
|
||||
--extra livekit \
|
||||
--extra piper \
|
||||
--extra runner \
|
||||
--extra sagemaker \
|
||||
--extra tracing \
|
||||
--extra websocket
|
||||
|
||||
1
changelog/4417.security.md
Normal file
1
changelog/4417.security.md
Normal file
@@ -0,0 +1 @@
|
||||
- Fixed a path traversal issue in the development runner's `/files/{filename:path}` download endpoint. Previously, when the runner was started with `--folder`, a request like `/files/..%2F..%2Fetc%2Fpasswd` could escape the configured folder because `%2F`-encoded separators bypassed Starlette's path normalisation. The endpoint now resolves the joined path and rejects any filename that escapes the allowed base with a 403, and also returns 404 (instead of an implicit `null` 200) when `--folder` is unset.
|
||||
@@ -209,6 +209,17 @@ def _configure_server_app(args: argparse.Namespace):
|
||||
logger.warning(f"Unknown transport type: {args.transport}")
|
||||
|
||||
|
||||
def _resolve_download_path(folder: str, filename: str) -> Path:
|
||||
"""Resolve a download path and ensure it stays within the downloads folder."""
|
||||
allowed_base = Path(folder).resolve()
|
||||
file_path = (allowed_base / filename).resolve()
|
||||
|
||||
if not file_path.is_relative_to(allowed_base):
|
||||
raise HTTPException(status_code=403, detail="Access denied")
|
||||
|
||||
return file_path
|
||||
|
||||
|
||||
def _setup_webrtc_routes(app: FastAPI, args: argparse.Namespace):
|
||||
"""Set up WebRTC-specific routes."""
|
||||
try:
|
||||
@@ -250,16 +261,16 @@ def _setup_webrtc_routes(app: FastAPI, args: argparse.Namespace):
|
||||
async def download_file(filename: str):
|
||||
"""Handle file downloads."""
|
||||
if not args.folder:
|
||||
logger.warning(f"Attempting to dowload {filename}, but downloads folder not setup.")
|
||||
return
|
||||
logger.warning(f"Attempting to download {filename}, but downloads folder not setup.")
|
||||
raise HTTPException(404)
|
||||
|
||||
file_path = Path(args.folder) / filename
|
||||
if not os.path.exists(file_path):
|
||||
file_path = _resolve_download_path(args.folder, filename)
|
||||
if not file_path.exists():
|
||||
raise HTTPException(404)
|
||||
|
||||
media_type, _ = mimetypes.guess_type(file_path)
|
||||
|
||||
return FileResponse(path=file_path, media_type=media_type, filename=filename)
|
||||
return FileResponse(path=file_path, media_type=media_type, filename=file_path.name)
|
||||
|
||||
# Initialize the SmallWebRTC request handler
|
||||
small_webrtc_handler: SmallWebRTCRequestHandler = SmallWebRTCRequestHandler(
|
||||
|
||||
84
tests/test_runner_downloads.py
Normal file
84
tests/test_runner_downloads.py
Normal file
@@ -0,0 +1,84 @@
|
||||
#
|
||||
# Copyright (c) 2024-2026, Daily
|
||||
#
|
||||
# SPDX-License-Identifier: BSD 2-Clause License
|
||||
#
|
||||
|
||||
import os
|
||||
import tempfile
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
|
||||
from fastapi import HTTPException
|
||||
|
||||
from pipecat.runner.run import _resolve_download_path
|
||||
|
||||
|
||||
class TestRunnerDownloads(unittest.TestCase):
|
||||
def test_resolve_download_path_allows_files_inside_folder(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
downloads = Path(tmpdir) / "downloads"
|
||||
nested = downloads / "nested"
|
||||
nested.mkdir(parents=True)
|
||||
file_path = nested / "recording.txt"
|
||||
file_path.write_text("session transcript")
|
||||
|
||||
resolved = _resolve_download_path(str(downloads), "nested/recording.txt")
|
||||
|
||||
self.assertEqual(resolved, file_path.resolve())
|
||||
|
||||
def test_resolve_download_path_blocks_parent_traversal(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
root = Path(tmpdir)
|
||||
downloads = root / "downloads"
|
||||
downloads.mkdir()
|
||||
(root / "secret.txt").write_text("secret")
|
||||
|
||||
with self.assertRaises(HTTPException) as context:
|
||||
_resolve_download_path(str(downloads), "../secret.txt")
|
||||
|
||||
self.assertEqual(context.exception.status_code, 403)
|
||||
|
||||
def test_resolve_download_path_blocks_decoded_encoded_slashes(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
root = Path(tmpdir)
|
||||
downloads = root / "media"
|
||||
downloads.mkdir()
|
||||
outside = root / "outside"
|
||||
outside.mkdir()
|
||||
(outside / "secret.txt").write_text("secret")
|
||||
|
||||
with self.assertRaises(HTTPException) as context:
|
||||
_resolve_download_path(str(downloads), "../outside/secret.txt")
|
||||
|
||||
self.assertEqual(context.exception.status_code, 403)
|
||||
|
||||
def test_resolve_download_path_blocks_absolute_paths(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
downloads = Path(tmpdir) / "downloads"
|
||||
downloads.mkdir()
|
||||
|
||||
with self.assertRaises(HTTPException) as context:
|
||||
_resolve_download_path(str(downloads), "/etc/passwd")
|
||||
|
||||
self.assertEqual(context.exception.status_code, 403)
|
||||
|
||||
@unittest.skipUnless(hasattr(os, "symlink"), "os.symlink is not available")
|
||||
def test_resolve_download_path_blocks_symlink_escape(self):
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
root = Path(tmpdir)
|
||||
downloads = root / "downloads"
|
||||
outside = root / "outside"
|
||||
downloads.mkdir()
|
||||
outside.mkdir()
|
||||
(outside / "secret.txt").write_text("secret")
|
||||
|
||||
try:
|
||||
os.symlink(outside, downloads / "linked")
|
||||
except OSError as e:
|
||||
self.skipTest(f"Unable to create symlink: {e}")
|
||||
|
||||
with self.assertRaises(HTTPException) as context:
|
||||
_resolve_download_path(str(downloads), "linked/secret.txt")
|
||||
|
||||
self.assertEqual(context.exception.status_code, 403)
|
||||
Reference in New Issue
Block a user