fix(uploads): keep deduplicated filenames within the 255-byte limit (#5059)

normalize_filename accepts names up to 255 UTF-8 bytes, but
claim_unique_filename appended _N to the stem without re-checking the
budget. A duplicate at maximum length therefore produced a 257-byte
name, and the write path (open_upload_file_no_symlink ->
normalize_filename) rejected it with ValueError. In the Gateway upload
route that error falls into the generic handler: the whole request
fails with a 500 and files already written in the same batch are rolled
back — including unrelated ones. The same helper backs the Feishu and
DingTalk channel downloads and client-side attachment staging.

Truncate the stem on a UTF-8 code-point boundary when appending the
dedupe tag would exceed 255 bytes, so the result always round-trips
through normalize_filename. Names short enough to fit keep the exact
dedupe shape they had before.

Tests: red on main, green here —
- unit: max-length dedupe stays within the limit and round-trips;
  repeated collisions stay unique; multibyte stems truncate on a
  code-point boundary; short names keep the historical _N shape
- router: a batch with a max-length duplicate now succeeds and keeps
  every file instead of failing with a 500

Co-authored-by: Terminator666666 <Terminator666666@users.noreply.github.com>
This commit is contained in:
Terminator666666 2026-08-28 09:16:51 +08:00 committed by GitHub
parent 24001e80b7
commit e09b2d48df
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 107 additions and 4 deletions

View File

@ -29,6 +29,8 @@ logger = logging.getLogger(__name__)
UPLOAD_STAGING_PREFIX = ".upload-"
UPLOAD_STAGING_SUFFIX = ".part"
_MAX_FILENAME_BYTES = 255
def get_uploads_dir(thread_id: str, *, user_id: str | None = None) -> Path:
"""Return the uploads directory path for a thread (no side effects)."""
@ -66,16 +68,31 @@ def normalize_filename(filename: str) -> str:
# but they indicate a Windows-style path that should be stripped or rejected.
if "\\" in safe:
raise ValueError(f"Filename contains backslash: {filename!r}")
if len(safe.encode("utf-8")) > 255:
if len(safe.encode("utf-8")) > _MAX_FILENAME_BYTES:
raise ValueError(f"Filename too long: {len(safe)} chars")
return safe
def _fit_utf8_bytes(text: str, budget: int) -> str:
"""Truncate *text* to at most *budget* UTF-8 bytes without splitting a code point."""
encoded = text.encode("utf-8")
if len(encoded) <= budget:
return text
return encoded[:budget].decode("utf-8", errors="ignore")
def claim_unique_filename(name: str, seen: set[str]) -> str:
"""Generate a unique filename by appending ``_N`` suffix on collision.
Automatically adds the returned name to *seen* so callers don't need to.
The deduplicated name stays within the 255-byte filename limit that
:func:`normalize_filename` enforces: when appending ``_N`` (plus the
preserved extension) would exceed it, the stem is truncated on a UTF-8
boundary to make room. Otherwise a maximum-length upload that collides
would produce a name the filesystem (and a later ``normalize_filename``
call on the write path) rejects.
Args:
name: Candidate filename.
seen: Set of filenames already claimed (mutated in place).
@ -88,10 +105,18 @@ def claim_unique_filename(name: str, seen: set[str]) -> str:
return name
stem, suffix = Path(name).stem, Path(name).suffix
counter = 1
candidate = f"{stem}_{counter}{suffix}"
while candidate in seen:
while True:
tag = f"_{counter}"
budget = _MAX_FILENAME_BYTES - len(tag.encode("utf-8")) - len(suffix.encode("utf-8"))
if budget < 1:
# Pathological suffix that leaves no room for a stem; keep the
# unique tag and fit the rest (stem + suffix tail) around it.
candidate = _fit_utf8_bytes(stem + suffix, _MAX_FILENAME_BYTES - len(tag.encode("utf-8"))) + tag
else:
candidate = f"{_fit_utf8_bytes(stem, budget)}{tag}{suffix}"
if candidate not in seen:
break
counter += 1
candidate = f"{stem}_{counter}{suffix}"
seen.add(candidate)
return candidate

View File

@ -73,6 +73,40 @@ class TestDeduplicateFilename:
claim_unique_filename("a.txt", seen)
assert seen == {"a.txt", "a_1.txt"}
def test_max_length_name_stays_within_filename_limit(self):
# A 255-byte name passes normalize_filename; the deduplicated name
# must not exceed that limit, or the write path rejects it.
name = "a" * 251 + ".txt"
seen = {name}
deduped = claim_unique_filename(name, seen)
assert deduped != name
assert deduped.endswith("_1.txt")
assert len(deduped.encode("utf-8")) <= 255
# The truncated result must round-trip through normalize_filename.
assert normalize_filename(deduped) == deduped
def test_max_length_collisions_stay_unique_across_truncation(self):
name = "a" * 251 + ".txt"
seen = {name}
first = claim_unique_filename(name, seen)
second = claim_unique_filename(name, seen)
assert first != second
assert len(second.encode("utf-8")) <= 255
def test_multibyte_stem_is_truncated_on_a_codepoint_boundary(self):
# 85 CJK chars × 3 bytes = 255 bytes.
name = "" * 85
seen = {name}
deduped = claim_unique_filename(name, seen)
assert len(deduped.encode("utf-8")) <= 255
assert deduped.endswith("_1")
# No replacement characters / decode artifacts.
deduped.encode("utf-8").decode("utf-8")
def test_short_names_keep_existing_dedupe_shape(self):
seen = {"data.txt"}
assert claim_unique_filename("data.txt", seen) == "data_1.txt"
# ---------------------------------------------------------------------------
# validate_path_traversal

View File

@ -132,6 +132,50 @@ def test_upload_files_auto_renames_duplicate_form_filenames(tmp_path):
assert (thread_uploads_dir / "data_1.txt").read_bytes() == b"second"
def test_upload_files_deduplicates_max_length_filenames_without_failing_the_batch(tmp_path):
# A 255-byte filename is the longest normalize_filename accepts. Before
# the byte-budget truncation in claim_unique_filename, deduplicating a
# duplicate at that length produced a 257-byte name that the write path
# rejected, failing the whole request with a 500 and rolling back files
# that had already been written.
thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True)
provider = MagicMock()
provider.uses_thread_data_mounts = True
max_length_name = "a" * 251 + ".txt"
with (
patch.object(uploads, "get_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "ensure_uploads_dir", return_value=thread_uploads_dir),
patch.object(uploads, "get_sandbox_provider", return_value=provider),
):
result = asyncio.run(
call_unwrapped(
uploads.upload_files,
"thread-local",
request=MagicMock(),
files=[
UploadFile(filename="innocent.txt", file=BytesIO(b"kept")),
UploadFile(filename=max_length_name, file=BytesIO(b"first")),
UploadFile(filename=max_length_name, file=BytesIO(b"second")),
],
config=SimpleNamespace(),
)
)
assert result.success is True
assert len(result.files) == 3
deduped_name = result.files[2].filename
assert deduped_name != max_length_name
assert deduped_name.endswith("_1.txt")
assert len(deduped_name.encode("utf-8")) <= 255
assert (thread_uploads_dir / "innocent.txt").read_bytes() == b"kept"
assert (thread_uploads_dir / max_length_name).read_bytes() == b"first"
assert (thread_uploads_dir / deduped_name).read_bytes() == b"second"
def test_upload_files_skips_acquire_when_thread_data_is_mounted(tmp_path):
thread_uploads_dir = tmp_path / "uploads"
thread_uploads_dir.mkdir(parents=True)