# Patch Analysis — CVE-2026-58166 Variant Stage

## Patch / Fix Identified

- Repository: `https://github.com/OpenBMB/ChatDev`
- Vulnerable target used by the parent reproduction: `a6a5cda5560053136897aa44301eacc6c48d8168` (`4fd4da603801766b14ad8788649cfc1ad21f99a6^`)
- Fixed commit tested: `4fd4da603801766b14ad8788649cfc1ad21f99a6`
- Latest/default branch resolved during this run: `4fd4da603801766b14ad8788649cfc1ad21f99a6`
- Fix title: `fix: sanitize upload filename to prevent path traversal (#638)`

## What the Fix Changes

The fix modifies `server/services/attachment_service.py` and adds regression tests in `tests/test_attachment_upload_filename.py`.

Before the fix, `AttachmentService.save_upload_file()` used the multipart filename directly:

```python
filename = upload.filename or "upload.bin"
temp_dir = Path(tempfile.mkdtemp(prefix="mac_upload_"))
temp_path = temp_dir / filename
with temp_path.open("wb") as buffer:
    ...
```

After the fix, `save_upload_file()` calls a new helper before constructing `temp_path`:

```python
filename = self._safe_upload_filename(upload.filename)
temp_dir = Path(tempfile.mkdtemp(prefix="mac_upload_"))
temp_path = temp_dir / filename
```

The new helper is:

```python
candidate = os.path.basename((raw or "").replace("\\", "/")).strip()
if not candidate or candidate in {".", ".."}:
    return "upload.bin"
return candidate
```

This normalizes Windows separators to `/`, strips directory components with `os.path.basename`, trims whitespace, and replaces empty/special names with `upload.bin`.

The added tests cover normal names, POSIX `../` traversal, Windows `..\\` traversal, absolute paths, nested path components, empty values, `.`, and `..`. The test matrix explicitly includes `"/abs/path/secret.key" -> "secret.key"`.

## Covered Code Paths

The fixed function is the sink reached by the parent product route:

- `server/routes/uploads.py::upload_attachment()` (`POST /api/uploads/{session_id}`) receives `UploadFile = File(...)`.
- It resolves the session via `ensure_known_session(session_id, require_connection=False)`.
- It calls `manager.attachment_service.save_upload_file(session_id, file)`.
- `server/services/attachment_service.py::AttachmentService.save_upload_file()` now sanitizes `file.filename` before any filesystem write.

A repository scan found no other server route that calls `AttachmentService.save_upload_file()`.

## Fix Assumptions

The fix assumes:

1. All web-upload filename writes flow through `AttachmentService.save_upload_file()`.
2. Reducing attacker-controlled `UploadFile.filename` to a basename is sufficient to prevent the filename from influencing parent directories or absolute paths.
3. The remaining filename does not need a stricter allowlist for the specific traversal bug class because it is only used beneath an already server-created temporary directory.
4. Windows path separators must be treated as directory separators even when the server is POSIX.

The source review supports assumption 1 for the attachment upload sink: `POST /api/uploads/{session_id}` is the only route that calls `save_upload_file()`.

## What the Fix Does Not Cover

The patch is intentionally narrow. It does not address:

- unauthenticated WebSocket session minting on `/ws`;
- `POST /api/workflow/run` accepting absolute `yaml_file` paths;
- workflow execution of Python nodes, which is product functionality but increases impact when combined with an arbitrary file-write primitive;
- unrelated file-writing APIs such as workflow content upload, local tool creation, Vue graph SQLite storage, or batch output writing.

Those areas were reviewed to avoid conflating separate product risks with a bypass of this specific upload filename traversal fix. They do not provide another path to the same `AttachmentService.save_upload_file()` vulnerable filename join.

## Variant Candidate Matrix

| Candidate | Same sink? | Vulnerable result | Fixed/latest result | Bypass? |
|---|---:|---|---|---:|
| Parent POSIX `../` traversal filename | Yes | Escaped temp dir via symlink write and cleanup removed supplied symlink | Basename stored; no escaped write/delete | No |
| Absolute multipart filename with symlink at supplied path | Yes | `temp_dir / absolute_path` discarded `temp_dir`; write followed symlink and cleanup removed symlink | `_safe_upload_filename()` reduced absolute path to basename; no target write | No |
| Absolute multipart filename pointing at existing file | Yes | Existing file was overwritten and then deleted by cleanup | Existing file preserved; sanitized basename stored under temp dir | No |

The absolute-filename candidates are materially different filename encodings for the same missing path confinement root cause. They are not fixed-version bypasses because the patch explicitly covers absolute paths through basename normalization.

## Behavior Before and After the Fix

### Vulnerable commit (`a6a5cda5560053136897aa44301eacc6c48d8168`)

- Parent baseline `../` traversal produced `escaped_temp_dir_via_write=true` and `deleted_supplied_path=true`.
- Absolute filename symlink candidate produced `escaped_temp_dir_via_write=true` and `deleted_supplied_path=true`.
- Absolute filename delete candidate produced `deleted_supplied_path=true`.

### Fixed/latest commit (`4fd4da603801766b14ad8788649cfc1ad21f99a6`)

- Parent baseline produced `fixed_escaped_or_deleted=false`.
- Absolute filename symlink candidate produced `fixed_escaped_or_deleted=false` and sanitized record name `fixed_candidate_absolute_filename_symlink_link.txt`.
- Absolute filename delete candidate produced `fixed_escaped_or_deleted=false`, sanitized record name `fixed_candidate_absolute_filename_delete_target.txt`, and `target_preserved_original=true`.

## Threat Model / Security Scope Review

No `SECURITY.md`, explicit threat-model document, or serialization-security policy was present in the tested source tree. The public product exposes HTTP/WebSocket workflow APIs. The parent reproduction established that unauthenticated network input can reach the upload sink; therefore, attacker-controlled multipart filenames crossing the HTTP boundary are in security scope. The variant stage did not claim local-only behavior as a vulnerability.

## Completeness Assessment

For the upload filename path traversal root cause, the fix appears complete for the reviewed and tested candidates. It covers POSIX traversal, Windows-style traversal, nested directory names, and absolute paths before the path join. No fixed-version bypass was reproduced.

Recommended hardening remains:

- add an explicit post-join confinement check, e.g. verify `temp_path.resolve().is_relative_to(temp_dir.resolve())` before opening;
- consider symlink-resistant temporary file creation where feasible;
- keep regression tests for absolute multipart filenames as well as `../` and `..\\` forms;
- separately review absolute `yaml_file` handling in `/api/workflow/run` and unauthenticated session creation, because these magnify the impact of any future write primitive but are not bypasses of this patch.