fix(security): prevent path traversal in file upload endpoints

[JasonOA888]

Summary

Both /v1/files and /v2/files used the user-provided filename directly as the local file path:

temp_file = Path(f"{file.filename}")

This allows path traversal — a crafted filename like ../../tmp/malicious or an absolute path like /etc/cron.d/x would write to arbitrary locations on the server. The file is normally cleaned up by unlink(), but if FileChatTool.upload() throws, the temp file persists on disk indefinitely.

Changes

1. Sanitize filename: Path(file.filename).name strips directory components, then prepend UUID for uniqueness
2. Guaranteed cleanup: Wrap upload logic in try/finally so temp files are always deleted

Before

temp_file = Path(f"{file.filename}")  # user-controlled path
with temp_file.open("wb") as buffer:
    shutil.copyfileobj(file.file, buffer)
result = FileChatTool.upload(temp_file)
# ...
temp_file.unlink()  # never reached if upload() raises

After

safe_suffix = Path(file.filename).name
temp_file = Path(f"{uuid.uuid4().hex}_{safe_suffix}")
try:
    with temp_file.open("wb") as buffer:
        shutil.copyfileobj(file.file, buffer)
    result = FileChatTool.upload(temp_file)
    # ...
finally:
    if temp_file.exists():
        temp_file.unlink()

Applies to both /v1/files and /v2/files endpoints.

[greptile-apps]

Greptile Summary

This PR correctly fixes a path traversal vulnerability in the /v1/files and /v2/files upload endpoints by stripping directory components from the user-supplied filename and wrapping the upload logic in try/finally to guarantee temp-file cleanup.

• P1 regression: UploadFile.filename is Optional[str]; when it is None, Path(None) raises TypeError at the line before the try block (lines 830 and 889). The old code silently converted None to the string "None", so this is a new crash path on any upload that omits a filename.
• P2: The relative Path(...) still writes temp files to the server's CWD rather than a system temp directory; using Path(tempfile.gettempdir()) / ... is the conventional fix.

Confidence Score: 4/5

The core path-traversal fix is correct, but a P1 regression (None filename → TypeError) should be addressed before merging.

The path traversal vulnerability is properly fixed and try/finally cleanup is a clear improvement. However, the None-filename TypeError regression is a P1 that would produce 500 errors for any upload without a filename in Content-Disposition, which is a real (if narrow) crash path introduced by this PR.

backend/routers/chat.py — lines 830 and 889 (None filename guard), lines 831 and 890 (CWD vs temp dir)

Important Files Changed

Filename	Overview
backend/routers/chat.py	Path traversal fix is correct; introduces a P1 regression where None filename triggers TypeError before try/finally cleanup, and temp files are still written to CWD rather than a temp directory.

Sequence Diagram

sequenceDiagram
    participant Client
    participant FastAPI as FastAPI (/v1 or /v2 files)
    participant FS as Filesystem
    participant FileChatTool

    Client->>FastAPI: POST /v2/files (UploadFile)
    Note over FastAPI: safe_suffix = Path(file.filename).name
    Note over FastAPI: ⚠️ Raises TypeError if file.filename is None
    FastAPI->>FS: Write UUID_safe_suffix to CWD (relative path)
    FastAPI->>FileChatTool: upload(temp_file)
    alt upload succeeds
        FileChatTool-->>FastAPI: result dict
        FastAPI->>FS: unlink(temp_file) [finally]
        FastAPI-->>Client: 200 FileChat[]
    else upload raises
        FastAPI->>FS: unlink(temp_file) [finally — guaranteed]
        FastAPI-->>Client: 500
    end

Loading

_{Reviews (1): Last reviewed commit: "fix(security): prevent path traversal in..." | Re-trigger Greptile}

[greptile-apps]

file.filename can be None — raises TypeError before try block

FastAPI's UploadFile.filename is typed Optional[str]. When it is None (e.g. a form upload with no Content-Disposition filename), Path(None) raises TypeError at this line — before the try block is entered, so no cleanup is needed but the caller gets an unhandled 500. The old code accidentally avoided this by using f"{file.filename}" which coerces None to the string "None". Add a guard:

Suggested change

	safe_suffix = Path(file.filename).name # strip any directory components
	safe_suffix = Path(file.filename).name if file.filename else "upload" # strip any directory components

The same fix is needed at the identical line in the /v1/files endpoint (line 889).

[greptile-apps]

Temp files written to CWD, not a system temp directory

Path(f"{uuid.uuid4().hex}_{safe_suffix}") is a relative path, so the file lands in the server process's current working directory. If the CWD is read-only, on a root filesystem, or simply not intended for ephemeral files, this will either fail or litter the CWD. Prefer writing to the system temp dir:

Suggested change

	temp_file = Path(f"{uuid.uuid4().hex}_{safe_suffix}")
	temp_file = Path(tempfile.gettempdir()) / f"{uuid.uuid4().hex}_{safe_suffix}"

This requires adding import tempfile at the top of the file. The same change applies to the identical line in the /v1/files endpoint (line 890).

[beastoin]

@JasonOA888 tests ?

[JasonOA888]

@beastoin Added tests in backend/tests/unit/test_file_upload_security.py — 13 test cases covering:

• P1 (None filename): verifies filename=None falls through to "upload" default without TypeError
• Path traversal: ../../etc/passwd, /etc/shadow, deeply nested traversal — all stripped to basename only
• P2 (temp dir): temp files are absolute paths in tempfile.gettempdir(), not relative CWD paths
• Cleanup: temp files removed on both success and upload failure

All 13 pass locally. Pushed to this branch.

[beastoin]

@JasonOA888 unit tests only, right?

can we have higher-level of tests to make sure the pr works as expected and does not break anything?

[JasonOA888]

@beastoin Added endpoint-level tests in test_file_upload_endpoint_security.py — 13 additional tests that exercise the full upload logic path (not just isolated unit tests):

• Path traversal: ../../etc/passwd, /etc/shadow, deeply nested — all stripped to basename, verified in actual temp file paths
• None filename: creates file with upload suffix, cleaned up correctly
• Temp file location: absolute paths in tempfile.gettempdir(), not CWD
• Cleanup: temp files removed on success AND on upload failure, including multi-file uploads
• Both endpoints: verifies /v1/files and /v2/files share identical secure logic

Combined with the 13 unit tests from the previous commit, that's 26 tests total. All 26 pass locally.