From 544b8b211257320bb2270b28b67d733b09615f41 Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Thu, 22 Oct 2026 17:02:37 +0900 Subject: [PATCH] fix(uploads): keep OSError text out of upload, delete and attach 500s (#7217) * fix(uploads): keep OSError text out of upload, delete and attach 500s Thread upload, upload delete and shelf attach-to-thread put str(exc) in their 500 detail. A staging or unlink OSError names its file, so a permission error, or a disk too full for mkstemp, returned the absolute host path of the thread's uploads directory, user and thread IDs included. Return a fixed detail that still names the caller's file. The existing error log lines keep the full message for operators. * docs(changelog): reference #7217 for the upload 500 detail fix --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 9 ++++ CHANGELOG_zh.md | 8 ++++ backend/app/gateway/routers/AGENTS.md | 4 ++ .../app/gateway/routers/project_documents.py | 2 +- backend/app/gateway/routers/uploads.py | 4 +- backend/docs/API.md | 6 ++- .../tests/test_project_documents_promotion.py | 19 ++++++++ backend/tests/test_uploads_router.py | 46 ++++++++++++++++++- 8 files changed, 92 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fe26d23af..2017c5dace 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6916,6 +6916,14 @@ As of 2026-10-21, the milestone includes **829 merged pull requests**. ### Security +- **uploads:** Thread uploads (`POST /api/threads/{thread_id}/uploads`), upload + deletes (`DELETE /api/threads/{thread_id}/uploads/{filename}`) and shelf + attach-to-thread no longer put the raw `OSError` text in their `500` detail. A + permission error, or a disk too full to create the staging file, answered with + the absolute host path of the thread's uploads directory, including the user + and thread IDs. They now return `Failed to upload .` / + `Failed to delete .`, and the full error stays in the Gateway log. ([#7217]) + - **agents:** The custom-agent and user-profile routes no longer put the raw exception text in their `500` detail. An unparsable agent `config.yaml` or a filesystem error (`[Errno 13]` and the like) made them answer with the @@ -12994,4 +13002,5 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#7170]: https://github.com/bytedance/deer-flow/pull/7170 [#7205]: https://github.com/bytedance/deer-flow/pull/7205 [#7207]: https://github.com/bytedance/deer-flow/pull/7207 +[#7217]: https://github.com/bytedance/deer-flow/pull/7217 [#7255]: https://github.com/bytedance/deer-flow/pull/7255 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index faf7a2b15c..733bb1e555 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -5256,6 +5256,13 @@ ### 安全 +- **上传：** 线程上传（`POST /api/threads/{thread_id}/uploads`）、删除上传文件 + （`DELETE /api/threads/{thread_id}/uploads/{filename}`）以及把资料库文档附加到线程， + 其 `500` 响应的 detail 不再包含原始 `OSError` 文本。此前遇到权限错误，或磁盘已满到 + 无法创建暂存文件时，响应会返回该线程上传目录的宿主机绝对路径（含用户 ID 和线程 ID）。 + 现在它们返回 `Failed to upload .` / `Failed to delete .`，完整错误只记录在 + Gateway 日志中。([#7217]) + - **agents：** 自定义智能体与用户画像路由的 `500` detail 不再包含原始异常文本。 此前无法解析的智能体 `config.yaml` 或文件系统错误（如 `[Errno 13]`）会让响应 带出 `users//` 下的宿主机绝对路径；现在返回与智能体导入、导出一致的 @@ -10371,4 +10378,5 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#7170]: https://github.com/bytedance/deer-flow/pull/7170 [#7205]: https://github.com/bytedance/deer-flow/pull/7205 [#7207]: https://github.com/bytedance/deer-flow/pull/7207 +[#7217]: https://github.com/bytedance/deer-flow/pull/7217 [#7255]: https://github.com/bytedance/deer-flow/pull/7255 diff --git a/backend/app/gateway/routers/AGENTS.md b/backend/app/gateway/routers/AGENTS.md index 7ecf3692be..ee762f70ea 100644 --- a/backend/app/gateway/routers/AGENTS.md +++ b/backend/app/gateway/routers/AGENTS.md @@ -21,6 +21,10 @@ signed-in user, so their unexpected-failure 500s return a fixed detail. Load errors (e.g. a malformed `extensions_config.json`) carry host paths and stay in the server log. +Thread upload, upload delete and shelf attach-to-thread 500s name the file but +not the cause: staging and unlink `OSError`s carry host paths and stay in the +server log. + Skill mutation operation lists return `next_cursor: null` when `has_more` is false, including empty pages after a supplied `after_id`. The next cursor is the final returned operation only when a further page exists. `GET /api/agent-tool-groups` projects only configured group names for the diff --git a/backend/app/gateway/routers/project_documents.py b/backend/app/gateway/routers/project_documents.py index 7b5558054e..453c06d4fa 100644 --- a/backend/app/gateway/routers/project_documents.py +++ b/backend/app/gateway/routers/project_documents.py @@ -419,7 +419,7 @@ async def attach_project_document_to_thread( except Exception as exc: logger.error(f"Failed to attach {row['name']} to thread {thread_id}: {exc}") await service.cleanup_written() - raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}: {exc}") + raise HTTPException(status_code=500, detail=f"Failed to upload {row['name']}.") # Sync-phase failures (permissions/provider sync) follow ordinary # uploads: they propagate and the host files stay in place. await service.finalize() diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 44f14773a4..391108535c 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -457,7 +457,7 @@ async def upload_files( except Exception as e: logger.error(f"Failed to upload {file.filename}: {e}") await service.cleanup_written() - raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}: {str(e)}") + raise HTTPException(status_code=500, detail=f"Failed to upload {file.filename}.") uploaded_files.append(file_info) await service.finalize() @@ -513,4 +513,4 @@ async def delete_uploaded_file(thread_id: ThreadId, filename: str, request: Requ raise HTTPException(status_code=400, detail=str(e)) except Exception as e: logger.error(f"Failed to delete {filename}: {e}") - raise HTTPException(status_code=500, detail=f"Failed to delete {filename}: {str(e)}") + raise HTTPException(status_code=500, detail=f"Failed to delete {filename}.") diff --git a/backend/docs/API.md b/backend/docs/API.md index 042a88a797..361601273e 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -1335,6 +1335,8 @@ Content-Type: multipart/form-data ``` +**Error behavior:** an unexpected storage failure answers `500` with `{"detail": "Failed to upload ."}`; the underlying error, which can name host paths, stays in server logs. + **Name collisions:** filenames are claimed unique against the thread's existing uploads and reserved atomically — a same-name upload never replaces the existing file; it lands as `document_1.pdf` (the response's `filename`/`original_filename` reflect the claimed name). Use the artifacts `PUT` endpoint for sanctioned in-place updates. **Supported Document Formats** (converted to Markdown only when automatic conversion is enabled): @@ -1381,6 +1383,8 @@ DELETE /api/threads/{thread_id}/uploads/{filename} } ``` +**Error behavior:** an unexpected storage failure answers `500` with `{"detail": "Failed to delete ."}`; the underlying error stays in server logs. + ### Thread Cleanup Remove DeerFlow-managed local thread files under `.deer-flow/threads/{thread_id}` after the LangGraph thread itself has been deleted. @@ -1540,7 +1544,7 @@ POST /api/projects/{project_id}/documents/{document_id}/attach-to-thread/{thread Materializes an independent copy of a live shelf document into the target thread's uploads directory through the same ingestion pipeline as an ordinary upload (filename claiming, size checks, optional conversion under `uploads.auto_convert_documents`, sandbox-readable permissions, and sandbox sync for non-mounted providers; a caller denied `sandbox:execute` keeps the host upload without allocating a sandbox). Reading an archived source project's shelf is allowed and does not mutate it; a missing/foreign document, or a target thread the caller cannot write, is `404`. A row whose original bytes are missing or size-mismatched answers `409` `"content_missing"`. -**Response:** `200 OK` with `{"filename", "size_bytes", "virtual_path", "artifact_url"}` — returned only after ingestion succeeds. +**Response:** `200 OK` with `{"filename", "size_bytes", "virtual_path", "artifact_url"}` — returned only after ingestion succeeds. An unexpected ingestion failure answers `500` with `{"detail": "Failed to upload ."}`; the underlying error stays in server logs. #### Get Document Content diff --git a/backend/tests/test_project_documents_promotion.py b/backend/tests/test_project_documents_promotion.py index 19d2855c71..fc929efddb 100644 --- a/backend/tests/test_project_documents_promotion.py +++ b/backend/tests/test_project_documents_promotion.py @@ -11,6 +11,7 @@ attach-vs-purge staged-copy serialization (§13 concurrency). from __future__ import annotations +import errno import os import stat import threading @@ -550,6 +551,24 @@ class TestAttach: # scope released the partially acquired holder. release.assert_awaited_once() + def test_attach_keeps_os_error_text_out_of_500(self, tmp_path, caplog): + """A staging OSError names its host path, so it stays in the server log.""" + app = _build_app(tmp_path) + with TestClient(app) as client, patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()): + pid = _create_project(client)["id"] + doc = _upload(client, pid, "report.txt", b"data").json()["document"] + _seed_thread(app, "thread-1") + staged_path = _thread_uploads("thread-1") / ".upload-abc123.part" + disk_full = OSError(errno.ENOSPC, os.strerror(errno.ENOSPC), str(staged_path)) + + with patch.object(uploads, "_prepare_upload_destination", side_effect=disk_full): + response = _attach(client, pid, doc["id"], "thread-1") + + assert response.status_code == 500 + assert response.json() == {"detail": "Failed to upload report.txt."} + assert str(staged_path) in caplog.text + assert not (_thread_uploads("thread-1") / "report.txt").exists() + def test_attach_sync_failure_retains_host_file(self, tmp_path): app = _build_app(tmp_path) provider, sandbox = _remote_provider() diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 3226097f75..8beaf84b5d 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -493,7 +493,7 @@ def test_upload_files_removes_the_staged_part_when_the_platform_blocks_an_open_u @pytest.mark.parametrize("filename", ["notes.txt", "report.pdf"]) @pytest.mark.parametrize("simulate_sharing_violation", [False, True]) -def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conversion_fd(tmp_path, monkeypatch, filename, simulate_sharing_violation): +def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conversion_fd(tmp_path, monkeypatch, caplog, filename, simulate_sharing_violation): thread_uploads_dir = tmp_path / "uploads" thread_uploads_dir.mkdir() conversion_fds: set[int] = set() @@ -536,7 +536,9 @@ def test_upload_files_preserves_link_error_and_cleans_staging_after_closing_conv asyncio.run(call_unwrapped(uploads.upload_files, "thread-link-error", request=MagicMock(), files=[file], config=SimpleNamespace())) assert exc_info.value.status_code == 500 - assert "original link failure" in exc_info.value.detail + assert exc_info.value.detail == f"Failed to upload {filename}." + # The logged cause is the link failure, not a later cleanup error. + assert "original link failure" in caplog.text assert not blocked_unlinks assert not conversion_fds assert len(duplicated_fds) == (1 if filename == "report.pdf" else 0) @@ -1559,6 +1561,27 @@ def test_upload_files_closes_conversion_descriptor_when_private_dir_fails(tmp_pa assert duplicated[0] in closed +def test_upload_files_keeps_os_error_text_out_of_500(tmp_path, caplog): + """A staging OSError names its host path, so it stays in the server log.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + staged_path = thread_uploads_dir / ".upload-abc123.part" + disk_full = OSError(errno.ENOSPC, os.strerror(errno.ENOSPC), str(staged_path)) + + with ( + patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "get_sandbox_provider", return_value=_mounted_provider()), + patch.object(uploads, "_prepare_upload_destination", side_effect=disk_full), + ): + file = ChunkedUpload("report.txt", [b"bytes"]) + with pytest.raises(HTTPException) as exc_info: + asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace())) + + assert exc_info.value.status_code == 500 + assert exc_info.value.detail == "Failed to upload report.txt." + assert str(staged_path) in caplog.text + + def test_upload_files_closes_conversion_descriptor_when_ingestion_is_cancelled(tmp_path): """Cancelling mid-duplication must not strand the descriptor the worker still produces.""" thread_uploads_dir = tmp_path / "uploads" @@ -1791,6 +1814,25 @@ def test_delete_uploaded_file_rejects_symlink_to_sibling_upload(tmp_path): assert (thread_uploads_dir / "alias.pdf").is_symlink() +def test_delete_uploaded_file_keeps_os_error_text_out_of_500(tmp_path, caplog): + """An unlink OSError names its host path, so it stays in the server log.""" + thread_uploads_dir = tmp_path / "uploads" + thread_uploads_dir.mkdir(parents=True) + file_path = thread_uploads_dir / "report.pdf" + denied = PermissionError(errno.EACCES, os.strerror(errno.EACCES), str(file_path)) + + with ( + patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir), + patch.object(uploads, "delete_file_safe", side_effect=denied), + ): + with pytest.raises(HTTPException) as exc_info: + asyncio.run(call_unwrapped(uploads.delete_uploaded_file, "thread-local", "report.pdf", request=MagicMock())) + + assert exc_info.value.status_code == 500 + assert exc_info.value.detail == "Failed to delete report.pdf." + assert str(file_path) in caplog.text + + def test_auto_convert_documents_enabled_defaults_to_false_on_config_errors(): class BrokenConfig: def __getattribute__(self, name):