Skip to content

fix(files): serve transit files whose name is outside Latin-1 - #249

Merged
l1shen merged 1 commit into
oomol-lab:mainfrom
DevMello:fix/transit-file-non-ascii-filename
Aug 1, 2026
Merged

fix(files): serve transit files whose name is outside Latin-1#249
l1shen merged 1 commit into
oomol-lab:mainfrom
DevMello:fix/transit-file-non-ascii-filename

Conversation

@DevMello

@DevMello DevMello commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Downloading a transit file whose name has a character above U+00FF returns HTTP 500. The upload succeeds, so you only find out at download time, and the file stays unreachable for the rest of its TTL.

invoice.pdf   -> 200  attachment; filename="invoice.pdf"
发票.pdf       -> 500  TypeError: Cannot convert argument to a ByteString
                       because the character at index 22 has a value of 21457

Header values are ByteStrings, so a name outside Latin-1 throws while the response is being built, before any byte is written.

It is easy to hit through mail, where an attachment keeps the filename the server sent and CJK names are common.

The name now follows RFC 6266. Anything outside ASCII is carried percent-encoded, with a plain ASCII fallback for clients that do not read it:

发票.pdf -> 200  attachment; filename="__.pdf"; filename*=UTF-8''%E5%8F%91%E7%A5%A8.pdf

Header values are ByteStrings, so a name holding a character above
U+00FF threw while the download response was built, failing with HTTP
500. Mail hit this often, since an attachment keeps the filename the
server sent.

Names now follow RFC 6266: non-ASCII travels percent-encoded in
`filename*`, with an ASCII-only `filename` fallback. ASCII names are
unchanged.
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ef5cda2-92e3-4cee-a084-1381ad5a8f96

📥 Commits

Reviewing files that changed from the base of the PR and between 195f811 and d7a83e1.

📒 Files selected for processing (6)
  • src/server/files/kv-transit-files.test.ts
  • src/server/files/kv-transit-files.ts
  • src/server/files/r2-transit-files.ts
  • src/server/files/transit-file-store.test.ts
  • src/server/files/transit-file-store.ts
  • src/server/files/transit-files.ts

Summary by CodeRabbit

  • Bug Fixes

    • Improved downloaded file names in response headers.
    • Non-ASCII names now include a safe ASCII fallback while preserving the original UTF-8 filename.
    • Added safer handling for special characters and extended Unicode characters.
  • Tests

    • Added coverage for filename encoding, escaping, fallbacks, and file response headers.

Walkthrough

Transit file response handling now uses the shared contentDispositionForFileName helper. The helper creates ASCII-safe fallback names and RFC 8187 UTF-8 filename* parameters for non-ASCII names. The transit, KV transit, and R2 transit services use the helper and remove their local escaping functions. Tests cover ASCII names, escaping, non-ASCII and astral characters, response headers, and preserved file contents.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format and accurately describes the fix for transit files with non-Latin-1 names.
Description check ✅ Passed The description clearly explains the HTTP 500 cause and the RFC 6266 filename handling change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@l1shen
l1shen merged commit 8785a4d into oomol-lab:main Aug 1, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants