fix: only set extend notation on non-ascii filename #558
Conversation
WalkthroughThe changes introduce a new regular expression constant, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FormData
participant FileSystem
User->>FormData: Upload File
FormData->>FileSystem: Check filename
alt Non-ASCII filename
FormData->>FormData: Append UTF-8 encoded version
end
FormData->>FileSystem: Construct contentDisposition
FileSystem-->>User: Return contentDisposition
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
==========================================
+ Coverage 94.60% 94.62% +0.01%
==========================================
Files 11 11
Lines 1223 1227 +4
Branches 297 298 +1
==========================================
+ Hits 1157 1161 +4
Misses 62 62
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/FormData.ts (1)
4-5: Consider simplifying the regex pattern.The regex pattern correctly identifies non-ASCII characters, but can be improved:
- The case-insensitive flag
iis unnecessary for character ranges- Consider using a more semantic variable name like
NON_ASCII_RE-const ASCII_RE = /[^\x00-\x7F]/i; +const NON_ASCII_RE = /[^\x00-\x7F]/;🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/FormData.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/FormData.ts
[error] 5-5: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (1)
src/FormData.ts (1)
30-33: LGTM! Implementation follows RFC 6266 correctly.
The changes properly implement conditional extended filename notation:
- Always includes the base filename parameter
- Only adds UTF-8 extended notation for non-ASCII filenames
- Follows RFC 6266 section 4.1 format
Let's verify the handling of various filename types:
✅ Verification successful
Content-Disposition implementation verified with test coverage
The test files show comprehensive coverage of filename handling including:
- ASCII filenames (e.g., 'foo.txt', 'index.js')
- Non-ASCII filenames with emojis (e.g., '😄foo😭.txt')
- Multiple file uploads in various formats
- Different file types and extensions
The implementation correctly handles both ASCII and non-ASCII filenames as evidenced by the passing tests, particularly in test/options.files.test.ts which verifies the content disposition headers through multipart form submissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test files that verify filename handling
# Look for related test files
fd -e test.ts -e spec.ts -e test.js -e spec.js | xargs rg -l "filename|FormData|Content-Disposition"
# Look for actual usage patterns
rg -A 2 "new FormData\(\)"
Length of output: 515
Script:
#!/bin/bash
# Let's examine the test files for content disposition and filename handling tests
# Check test files for content disposition tests
rg -B 2 -A 4 "Content-Disposition|filename" test/options.files.test.ts test/options.data.test.ts
# Look for FormData usage in tests with context
rg -B 2 -A 4 "new FormData\(\)" test/options.files.test.ts test/options.data.test.ts
# Search for actual filename test cases to verify different scenarios
rg -B 1 -A 1 "['\"](.*(?:\.txt|\.png|\.jpg))['\"]" test/options.files.test.ts
Length of output: 15718
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/FormData.ts (1)
4-5: LGTM with minor suggestions.The regex pattern correctly identifies non-ASCII characters. Consider these optional improvements:
- Remove the unnecessary case-insensitive flag 'i' since character ranges are not affected by case
- Add a brief comment explaining the ASCII range check
-const NON_ASCII_RE = /[^\x00-\x7F]/i; +// Matches any character outside the ASCII range (0-127) +const NON_ASCII_RE = /[^\x00-\x7F]/;🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/FormData.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/FormData.ts
[error] 5-5: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (1)
src/FormData.ts (1)
30-33: Implementation correctly follows RFC 6266 specification.
The changes properly implement the Content-Disposition header format by:
- Always including the standard filename parameter
- Only adding the extended UTF-8 notation when necessary (non-ASCII characters present)
This fixes potential issues with unnecessary encoding while maintaining compatibility.
Let's verify the behavior with different filename types:
✅ Verification successful
The test results show comprehensive coverage of filename handling, including:
- ASCII filenames (e.g., 'index.js', 'package.json')
- Non-ASCII filenames (e.g., '😄foo😭.txt')
- Various file types and encodings
The implementation correctly handles both scenarios as evidenced by passing tests, particularly in test/options.files.test.ts where non-ASCII filenames are properly processed.
Content-Disposition header implementation verified as correct
The changes properly implement RFC 6266 by:
- Always including the base filename parameter
- Adding UTF-8 encoded version only for non-ASCII filenames
- Using proper encoding via encodeURIComponent
The test suite confirms proper handling of both ASCII and non-ASCII filenames across different scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create test cases to verify the content disposition logic
echo "Testing content disposition handling..."
# Find and analyze all filename usages in tests
rg -l "filename" --type ts --type js | while read -r file; do
echo "=== $file ==="
# Show test cases and their context
rg -B2 -A2 "filename.*[^a-zA-Z0-9]" "$file"
done
Length of output: 15000
[skip ci] ## [4.6.3](v4.6.2...v4.6.3) (2024-12-05) ### Bug Fixes * only set extend notation on non-ascii filename ([#558](#558)) ([0cd9b06](0cd9b06))
Summary by CodeRabbit
New Features
Bug Fixes