Fix file uploads with non-ASCII filenames - #2876
Conversation
uploadFile() sent the file path in a raw HTTP header, but headers only accept ISO-8859-1. Filenames with accents (e.g. macOS's NFD-decomposed "café.txt") fall outside that range, so fetch() throws before the request is even sent. deleteFile() and getDownloadUrl() already avoided this by sending the path as an encoded query param; upload was the odd one out, across the core SDK, admin SDK, and the dashboard's storage explorer. content-disposition has the same issue and gets the same fix. Clients now send path/content-disposition as an encoded query param and only mirror them into headers when the value is ISO-8859-1-safe, so requests to older self-hosted servers keep working. The corresponding server upload handlers merge headers and query params (query params win), matching how file-delete and signed-download-url-get already read their params, so newer clients keep working against older servers too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQ8ju4b8aRBBpVTXsfxdSp
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughStorage upload clients now send encoded path and metadata query parameters. Servers prioritize these parameters while retaining legacy header support. Header mirroring is limited to ISO-8859-1-safe values. Tests cover precedence and non-ASCII filenames. ChangesStorage upload query parameters
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change moves non-ASCII upload metadata into encoded query parameters while preserving legacy header compatibility across clients and servers; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant UploadRoute
participant StorageCoordinator
Client->>UploadRoute: PUT request with encoded query parameters and safe legacy headers
UploadRoute->>UploadRoute: Merge query parameters over keywordized headers
UploadRoute->>StorageCoordinator: Upload file with resolved path and metadata
StorageCoordinator-->>Client: Upload response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (5 skipped: 5 unsupported.) 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. Comment |
Documents isHeaderSafe and the touched uploadFile/upload-put functions across the client and server, matching the docstring style already used by their siblings (e.g. the other functions in storage/coordinator.clj). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQ8ju4b8aRBBpVTXsfxdSp
The rationale for the header/query-param split is already in the PR description; the code doesn't need to restate it inline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQ8ju4b8aRBBpVTXsfxdSp
Summary
uploadFile()sent the file'spathin a raw HTTP header, but header values must be ISO-8859-1. Filenames with accents (e.g. macOS's NFD-decomposedcafé.txt, whereàisa+ a combining grave accent) fall outside that range, sofetch()throws aTypeErrorbefore the request even goes out.content-dispositionhas the same problem.deleteFile()andgetDownloadUrl()already avoid this by sending the filename as an encoded query param —uploadFile()was the odd one out. The same bug exists in three places: the core SDK, the admin SDK, and the dashboard's storage explorer (each with its own server handler).path(andcontent-disposition) as an encoded query param, and only mirror them into a header when the value is safe to encode as ISO-8859-1. Servers now merge headers and query params (query params win) when reading these, matching howfile-delete/signed-download-url-getalready read theirs. This keeps compatibility in both directions: newer clients still work against servers that only read headers, and older clients still work against the updated servers.Client
client/packages/core/src/StorageAPI.ts—uploadFileclient/packages/admin/src/index.ts—Storage.uploadFileclient/packages/components/src/components/explorer/inner-explorer.tsx— dashboard storage explorer upload (this is the path that breaks when uploading a file from the dashboard UI)Server
server/src/instant/storage/routes.clj—upload-putserver/src/instant/admin/routes.clj—upload-putserver/src/instant/dash/routes.clj—upload-putDocs
client/www/app/docs/http-api/page.md— updated thecurlexample to use the query param form.Test plan
pnpm run build-packages(typecheck across all client packages) passespnpm run check-formatpasses on the changed filespnpm run test— existing suite passes (206 tests, 1 pre-existing unrelated failure from a missing local Playwright browser binary)server/test/instant/storage/routes_test.cljcovering thatupload-putreadspathfrom query params, still falls back to the legacy header, and that query params win when both are presentstorage-impersonation-testinserver/test/instant/admin/routes_test.cljuploading a file with an accented filename via query params