From d0d66d62fae2b7cc9fb0fb814df3403397ae8831 Mon Sep 17 00:00:00 2001 From: lukyturnflow <3421906768@qq.com> Date: Sat, 24 Oct 2026 10:07:10 +0800 Subject: [PATCH] fix(uploads): report rejected filenames and make success mean "a file was stored" (#7165) Unsafe filenames and unnamed parts were dropped silently: the response kept `success: true` with an empty `files` list, so the #7122 frontend gate never blocked a chat send whose attachment had been rejected (#7160). Both drop arms now record the rejected name in `skipped_files`, and `success` follows `bool(uploaded_files)`: an all-rejected batch fails, a mixed batch succeeds and lists what was dropped. --- backend/app/gateway/routers/uploads.py | 21 +++++- .../tests/blocking_io/test_uploads_router.py | 71 +++++++++++++++++++ backend/tests/test_uploads_router.py | 6 +- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/backend/app/gateway/routers/uploads.py b/backend/app/gateway/routers/uploads.py index 391108535c..68d53c4962 100644 --- a/backend/app/gateway/routers/uploads.py +++ b/backend/app/gateway/routers/uploads.py @@ -441,11 +441,22 @@ async def upload_files( raise HTTPException(status_code=400, detail=str(e)) for file in files: if not file.filename: + # Starlette builds an UploadFile even for a part with an empty + # filename; dropping it silently kept `success: true` over an + # empty `files` + `skipped_files`, bypassing the #7122 gate + # exactly like the unsafe-name arm below (#7160). + logger.warning("Skipping upload part without a filename") + skipped_files.append("(unnamed file)") continue try: file_info = await service.ingest_chunks(_stream_upload_file(file), display_name=file.filename) - except UnsafeFilenameError: - logger.warning(f"Skipping file with unsafe filename: {file.filename!r}") + except UnsafeFilenameError as exc: + # The rejected name must reach the caller: leaving it out of + # skipped_files kept `success: true` over an empty `files` + # list, which let the chat send proceed without the + # attachment the user asked for (#7160). + logger.warning("Skipping upload with unsafe filename %r: %s", file.filename, exc) + skipped_files.append(file.filename) continue except UnsafeUploadDestinationError as e: logger.warning("Skipping upload with unsafe destination %s: %s", file.filename, e) @@ -467,7 +478,11 @@ async def upload_files( message += f"; skipped {len(skipped_files)} unsafe file(s)" return UploadResponse( - success=not skipped_files, + # ``success`` means "at least one file was stored": an all-rejected + # batch must be a failure so the #7122 frontend gate blocks the + # send, while a mixed batch still succeeds and reports the dropped + # names through ``skipped_files`` (#7160). + success=bool(uploaded_files), files=uploaded_files, message=message, skipped_files=skipped_files, diff --git a/backend/tests/blocking_io/test_uploads_router.py b/backend/tests/blocking_io/test_uploads_router.py index 7cf45255b1..49cf7e4c8c 100644 --- a/backend/tests/blocking_io/test_uploads_router.py +++ b/backend/tests/blocking_io/test_uploads_router.py @@ -92,6 +92,77 @@ async def test_upload_endpoint_mounted_provider_does_not_block_event_loop(tmp_pa assert await asyncio.to_thread(target.read_bytes) == b"hello uploads" +async def test_unsafe_filename_is_reported_as_skipped(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """A rejected filename must reach the caller in ``skipped_files`` (#7160).""" + _reset_paths(tmp_path, monkeypatch) + monkeypatch.setattr(uploads, "get_sandbox_provider", lambda: _MountedProvider()) + + result = await call_unwrapped( + uploads.upload_files, + "t-unsafe-mixed", + request=None, + files=[ + UploadFile(filename="ok.txt", file=BytesIO(b"kept document")), + UploadFile(filename="bad\x00name.txt", file=BytesIO(b"dropped document")), + ], + config=SimpleNamespace(), + ) + + # A mixed batch still succeeds because a file was stored; the dropped name + # is visible in skipped_files instead of disappearing. + assert result.success is True + assert [item.filename for item in result.files] == ["ok.txt"] + assert result.skipped_files == ["bad\x00name.txt"] + assert "skipped 1 unsafe file(s)" in result.message + user_id = get_effective_user_id() + listing = await asyncio.to_thread(lambda: sorted(path.name for path in get_uploads_dir("t-unsafe-mixed", user_id=user_id).iterdir())) + assert listing == ["ok.txt"] + + +async def test_all_unsafe_filenames_report_failure(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Every file rejected means ``success: false``: the #7122 frontend gate + blocks the chat send on that, so a silent ``success: true`` here let a + message go out without any of the requested attachments.""" + _reset_paths(tmp_path, monkeypatch) + monkeypatch.setattr(uploads, "get_sandbox_provider", lambda: _MountedProvider()) + + result = await call_unwrapped( + uploads.upload_files, + "t-unsafe-all", + request=None, + files=[ + UploadFile(filename="bad\x00one.txt", file=BytesIO(b"dropped one")), + UploadFile(filename="bad\x00two.txt", file=BytesIO(b"dropped two")), + ], + config=SimpleNamespace(), + ) + + assert result.success is False + assert result.files == [] + assert result.skipped_files == ["bad\x00one.txt", "bad\x00two.txt"] + + +async def test_unnamed_part_is_reported_as_skipped(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """A multipart part with an empty filename must not vanish silently (#7160).""" + _reset_paths(tmp_path, monkeypatch) + monkeypatch.setattr(uploads, "get_sandbox_provider", lambda: _MountedProvider()) + + result = await call_unwrapped( + uploads.upload_files, + "t-unnamed-part", + request=None, + files=[ + UploadFile(filename="", file=BytesIO(b"dropped part")), + UploadFile(filename="ok.txt", file=BytesIO(b"kept document")), + ], + config=SimpleNamespace(), + ) + + assert result.success is True + assert [item.filename for item in result.files] == ["ok.txt"] + assert result.skipped_files == ["(unnamed file)"] + + @pytest.mark.parametrize( "filename", [".upload-notes.part", r"folder\.upload-notes.part", r"C:\users\.upload-notes.part", ".upload-notes.part.", ".upload-notes.part ", r"folder\.upload-notes.part. ", ".UPLOAD-NOTES.PART", r"folder\.Upload-NoTeS.Part ."] ) diff --git a/backend/tests/test_uploads_router.py b/backend/tests/test_uploads_router.py index 8beaf84b5d..17281987c3 100644 --- a/backend/tests/test_uploads_router.py +++ b/backend/tests/test_uploads_router.py @@ -1260,8 +1260,12 @@ def test_upload_files_rejects_dotdot_and_dot_filenames(tmp_path): for bad_name in ["..", "."]: file = UploadFile(filename=bad_name, file=BytesIO(b"data")) result = asyncio.run(call_unwrapped(uploads.upload_files, "thread-local", request=MagicMock(), files=[file], config=SimpleNamespace())) - assert result.success is True + # Nothing was stored, so the batch must not report success: the + # #7122 frontend gate keys on it, and a silent success let a chat + # send proceed without the attachment (#7160). + assert result.success is False assert result.files == [], f"Expected no files for unsafe filename {bad_name!r}" + assert result.skipped_files == [bad_name] # Path-traversal prefixes are stripped to the basename and accepted safely file = UploadFile(filename="../etc/passwd", file=BytesIO(b"data"))