Skip to content

fix(server): encode Unicode attachment filenames in response headers - #2374

Open
roytrack wants to merge 2 commits into
first-tree-ai:mainfrom
roytrack:fix/unicode-attachment-content-disposition-v2
Open

fix(server): encode Unicode attachment filenames in response headers#2374
roytrack wants to merge 2 commits into
first-tree-ai:mainfrom
roytrack:fix/unicode-attachment-content-disposition-v2

Conversation

@roytrack

@roytrack roytrack commented Aug 31, 2026

Copy link
Copy Markdown

PR: Encode Unicode attachment filenames in response headers

Suggested title

fix(server): encode Unicode attachment filenames in response headers

Summary

  • Percent-encode the complete attachment filename before placing it in the
    Content-Disposition response header.
  • Preserve the existing quoted filename="..." wire shape and the current
    client-side decodeURIComponent behavior.
  • Bound canonical filenames to 255 UTF-8 bytes at the shared Client/Web and
    Server ingress contract, keeping worst-case percent-encoded values well
    below Node's default 16 KiB header limit.
  • Truncate legacy overlong database values on UTF-8 code-point boundaries at
    response time so pre-limit rows cannot overflow clients.
  • Add an upload-to-download regression test covering Unicode, spaces, a
    literal percent sign, and full-width punctuation.
  • Add real Node fetch boundary regressions for the maximum accepted filename
    and a historical 5,500-character filename.

Root cause

The attachment download route only percent-encoded CR, LF, quotes, and
backslashes. A filename containing characters outside Latin-1 was therefore
passed directly to Node's HTTP response implementation. Node rejects that
header value with ERR_INVALID_CHAR while Fastify is sending the attachment
stream, which can terminate the server process.

Web uploads already transported filenames with encodeURIComponent, while
the Client SDK sent the raw value. Both paths now validate and percent-encode
the filename before transport; the Server decodes it and applies the same
shared validation before storage. The client already decodes percent-encoded
filename="..." values, so the download wire shape remains compatible.

Complete percent-encoding can expand accepted ASCII punctuation by 3x. Without
an ingress bound, a long stored name can exceed Node's 16 KiB response-header
limit. The shared 255-byte UTF-8 limit caps the filename parameter at 765
characters in the worst case, and the response-time legacy bound covers rows
created before this validation existed.

Scope

The issue is not specific to attachment size or MIME type. It affects any
consumer of GET /api/v1/attachments/:id when the stored filename contains
non-ASCII characters. That includes chat files and images as well as other
features backed by the shared attachment store, such as Team Skill bundles.

Verification

  • biome check for all ten changed files: passed.
  • Shared, Server, Client, and Web typechecks: passed.
  • Shared 2/2, Client 14/14, and Web 9/9 focused tests: passed.
  • Shared, Server, Client, and Web production builds: passed.
  • git diff --check: passed.
  • Node header reproduction: old implementation raises ERR_INVALID_CHAR;
    patched implementation is ASCII-only and round-trips the original filename.
  • The equivalent built Server bundle was exercised against an isolated
    database clone: multiple Unicode filenames and an ASCII control file all
    returned HTTP 200 with matching decoded names, byte lengths, and SHA-256
    content hashes, with zero Server restarts or severe errors.
  • Server product regressions cover Unicode round-trip, the 255-byte boundary
    through real Node fetch, legacy overlong-row response bounding, and
    ingress rejection. Local execution was blocked by the workstation pnpm
    dependency self-repair loop inside test global setup; GitHub CI is the
    authoritative exact-head Server run.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

All contributors are covered by the First Tree CLA.
Posted by the CLA Assistant Lite bot.

@roytrack

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Aug 31, 2026
yuezengwu
yuezengwu previously approved these changes Aug 31, 2026

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at exact head 3fcdc3012b1a362205118d956c4628689d6e0f2a.

I reviewed the complete PR context, diff, upload/download contract, and current comments/checks. The change keeps the existing quoted filename="..." wire shape while encoding the stored semantic filename to ASCII. Unicode, spaces, literal %, quotes, backslashes, slashes, CR/LF, and control characters cannot escape the quoted header; the existing client performs one decodeURIComponent, so names round-trip without double encoding.

Exact-head verification:

  • attachments-route.test.ts: 25/25
  • client sdk-comprehensive.test.ts: 13/13
  • full Server suite: 3488/3489; the unrelated rendezvous-bound test passed 10/10 in isolation
  • pnpm check, pnpm typecheck, pnpm build, and git diff --check: pass
  • mutation check: the new regression test fails on base with Node/Fastify ERR_INVALID_CHAR and passes on this head
  • CLA passed; no unresolved review threads

No blocking correctness, header-injection, compatibility, identity, CLA, copyright, or license finding.

@roytrack

Copy link
Copy Markdown
Author

#2291

@yuezengwu

Copy link
Copy Markdown
Contributor

Blocking local validation finding: Content-Disposition expansion can exceed Node's header limit

Please continue revising this PR before merge. This finding applies to exact head 3fcdc3012b1a362205118d956c4628689d6e0f2a.

The new encodeURIComponent(name) behavior can expand an accepted filename by 3x. The attachment input and database schema currently have no filename-length bound, while the official Client SDK sends opts.filename unchanged in x-attachment-filename.

I reproduced a regression with a filename consisting of 5,500 semicolons:

  • SDK-style raw upload header: 5,500 bytes, accepted.
  • Base response header: 5,519 bytes; Node fetch succeeds.
  • PR response header: 16,519 bytes; Node's default http.maxHeaderSize is 16,384 bytes.
  • PR download result: fetch failed, cause UND_ERR_HEADERS_OVERFLOW.

This is attributable to the PR: the base encoder leaves semicolons unchanged inside the quoted value, while this head encodes each one as %3B.

Before re-review, please:

  1. Ensure every accepted/stored filename produces a response header within the supported client limit, or reject/normalize it at a defined ingress boundary.
  2. Add a deterministic product regression covering accepted upload -> download through Node fetch around the header-size boundary.
  3. Reconcile the PR description with the actual upload paths: Web percent-encodes filenames, but packages/client/src/cloud/sdk.ts currently sends them unchanged. Please also update the now-stale client decoder comment that says only CR, LF, quote, and backslash are encoded.

The intended Unicode fix itself is valid: raw Unicode reproduces ERR_INVALID_CHAR, and Unicode/control/injection-sensitive character round-trips are ASCII-safe on this head. Local targeted tests (server 25/25, client 13/13), full Server tests (3,489/3,489), check, typecheck, and build all passed, but none covers the overflow boundary above.

Please request a fresh exact-head validation after pushing the revision; prior findings and test results do not transfer to a new head.

@yuezengwu
yuezengwu dismissed their stale review September 1, 2026 02:26

Dismissed after exact-head local validation found a blocking Content-Disposition header overflow regression. See issue comment #5487849382; a fresh review is required after the author pushes a fix.

@roytrack

roytrack commented Sep 1, 2026

Copy link
Copy Markdown
Author

Addressed the blocking header-overflow finding in commit b403412c39ffab3b532e56a04e1206f8700ec38f without rewriting the existing PR history.

Changes:

  • Added a shared 255 UTF-8 byte filename limit used by Client SDK, Web, and the Server createAttachment trust boundary.
  • Client SDK now percent-encodes upload filenames, matching Web and Server decode behavior.
  • Bounded legacy overlong database filenames on UTF-8 code-point boundaries before response-header encoding.
  • Added real Node fetch regressions at the accepted boundary and for a legacy 5,500-character value, plus HTTP/service/client/web rejection coverage.
  • Updated the stale SDK decoder comment and corrected the PR upload-path description.

Local validation: changed-file Biome, four package typechecks, Shared 2/2, Client 14/14, Web 9/9, and Shared/Server/Client/Web builds passed. Please run a fresh exact-head validation; GitHub CI is the authoritative Server suite because the local Server global setup was blocked by the workstation pnpm dependency self-repair loop.

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