fix: more aggressive file name sanitizing - #1001
Conversation
removes emojis and additional unsafe characters
WalkthroughReimplemented SanitizeFileName with Unicode-aware logic: removes emoji/control characters, preserves only RFC3986 unreserved ASCII, converts separators/whitespace to single underscores, trims edge characters and defaults empty results to "unnamed_file". Tests moved to a table-driven format validating safety and length. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
internal/utils/utils.go (2)
92-94: Redundant trim operation.Line 94 is redundant—
_is already included in the cutset on line 93 ("._- ~"). Both lines perform the same trim for underscores.Proposed fix
// trim separators/dots/spaces and collapse underscores already handled above. out = strings.Trim(out, "._- ~") // also avoids trailing dot/space issues - out = strings.Trim(out, "_")
96-101: Consider enforcing the 255-character filename limit.The test suite validates that output length does not exceed 255 characters, but this function doesn't enforce that constraint. A very long input with mostly ASCII unreserved characters would produce an output exceeding typical filesystem limits.
Proposed enhancement
// Handle empty/special names if out == "" || out == "." || out == ".." { out = "unnamed_file" } + // Enforce filesystem-safe length limit + if len(out) > 255 { + out = out[:255] + // Trim any trailing separator chars introduced by truncation + out = strings.TrimRight(out, "._-~") + } + return out
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/utils/utils.gointernal/utils/utils_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
SanitizeFileName(50-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build OCI Images (ubuntu-24.04-arm, arm64, linux/arm64)
- GitHub Check: Build OCI Images (ubuntu-24.04, amd64, linux/amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/utils/utils_test.go (2)
8-46: Well-structured table-driven tests with good coverage.The test cases cover important scenarios: whitespace handling, illegal filesystem characters, emoji removal (including ZWJ sequences), Unicode fallback to
unnamed_file, and edge cases like.and...One minor observation: the test at line 23 is named
"trailing_underscore_trim"but actually tests that spaces aroundfile...nameare trimmed while internal dots are preserved. Consider renaming to"surrounding_spaces_trim"for clarity.Also note: the tests validate a 255-character limit (line 38-40), but the implementation in
utils.godoes not enforce this constraint. If a very long input is passed, the output could exceed 255 characters.
48-61: LGTM!The helper correctly validates RFC3986 unreserved characters. Byte-by-byte iteration is appropriate here since
SanitizeFileNameguarantees ASCII-only output.internal/utils/utils.go (2)
104-118: LGTM!Correctly identifies RFC3986 unreserved characters with a clean switch implementation.
120-182: Comprehensive emoji detection.Good coverage of emoji blocks, ZWJ sequences, modifiers, and common singletons. The comments documenting each range are helpful for maintainability.
Note that Unicode continues to add new emoji in each version, so this is inherently a best-effort approach. The current coverage should handle the vast majority of real-world cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/utils/utils.go (2)
93-99: Consider extracting repeated logic into a helper.The trimming and empty-name fallback logic is duplicated before and after truncation. Consider extracting this into a helper function to improve maintainability.
🔎 Proposed refactor
+// finalizeName trims separators and handles empty/reserved names +func finalizeName(s string) string { + s = strings.Trim(s, "._- ~") + s = strings.Trim(s, "_") + if s == "" || s == "." || s == ".." { + return "unnamed_file" + } + return s +} + func SanitizeFileName(in string) string { // ... existing loop logic ... out := b.String() - // trim separators/dots/spaces and collapse underscores already handled above. - out = strings.Trim(out, "._- ~") // also avoids trailing dot/space issues - out = strings.Trim(out, "_") - - // Handle empty/special names - if out == "" || out == "." || out == ".." { - out = "unnamed_file" - } + out = finalizeName(out) if len(out) > 255 { out = truncatePreserveExt(out, 255) - out = strings.Trim(out, "._- ~") - out = strings.Trim(out, "_") - if out == "" || out == "." || out == ".." { - out = "unnamed_file" - } + out = finalizeName(out) } return out }Also applies to: 103-107
49-111: Consider handling Windows reserved filenames.The function doesn't check for Windows reserved names like
CON,PRN,AUX,NUL,COM1-9,LPT1-9, etc. On Windows systems, these names (even with extensions likeCON.txt) can cause issues. If cross-platform compatibility is important, consider adding a check.🔎 Example implementation
Add a helper to check reserved names:
// isWindowsReserved checks if name is a Windows reserved filename func isWindowsReserved(name string) bool { // Remove extension for check base := name if i := strings.LastIndexByte(name, '.'); i > 0 { base = name[:i] } base = strings.ToUpper(base) reserved := []string{ "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", } for _, r := range reserved { if base == r { return true } } return false }Then in
SanitizeFileName, after the final name is determined:out = finalizeName(out) + + // Avoid Windows reserved names + if isWindowsReserved(out) { + out = "_" + out + } return out
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/utils/utils.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: Analyze (go)
- GitHub Check: Build OCI Images (ubuntu-24.04, amd64, linux/amd64)
- GitHub Check: Build OCI Images (ubuntu-24.04-arm, arm64, linux/arm64)
🔇 Additional comments (5)
internal/utils/utils.go (5)
56-88: Character filtering logic is sound.The implementation correctly:
- Removes emoji using the comprehensive
isEmojiRunehelper- Collapses consecutive separators with the
lastWasUnderscoreflag- Preserves only RFC3986 unreserved ASCII characters
The control character check (line 72) covers ASCII control characters (0x00-0x1F, 0x7F) but doesn't explicitly handle Unicode control characters (e.g., C0/C1 control blocks). Given that non-ASCII characters become separators anyway (line 84), this is acceptable.
113-140: Extension preservation works correctly for single extensions.The implementation properly:
- Preserves the file extension when truncating
- Handles edge cases (extension too long, base becomes empty)
- Falls back to "file" as a base when needed
Note that for compound extensions like ".tar.gz", only the last part (".gz") is preserved. While this might not be ideal for all use cases, it's a reasonable simplification.
142-156: RFC3986 unreserved character set correctly implemented.The function accurately checks for RFC3986 unreserved characters:
a-z,A-Z,0-9,-,.,_, and~.
158-220: Emoji detection is comprehensive but not exhaustive.The implementation covers:
- Common emoji blocks (emoticons, symbols, pictographs, transport, etc.)
- Modifiers and combining characters (skin tones, ZWJ, variation selectors)
- Regional indicators (flags) and tag characters
- Specific symbols (©, ®, ™)
Note that emoji specifications evolve with new Unicode releases. While this implementation covers the most common cases, new emoji may not be detected until the ranges are updated. This is a reasonable best-effort approach.
Also note that ©, ®, and ™ are removed, which might affect legitimate use cases like "Product™" → "Product_". Ensure this aligns with your requirements.
93-93: Tilde trimming from edges is intentional behavior.The test case
"trim_dashes_dots_spaces"with input-._~abc~_.-expecting outputabcconfirms that tildes are intentionally removed from edges viastrings.Trim(out, "._- ~"). While tilde is correctly preserved in the middle (as an RFC3986 unreserved character viaisUnreservedASCII), the edge-trimming appears intentional for consistency with other separators.Consider adding a code comment at line 93 explaining the design rationale—specifically that RFC3986 unreserved characters are preserved internally but trimmed from edges to avoid trailing special characters, even though this creates an asymmetric treatment of tilde.
|
Not sure if this is a good idea given that only English characters are now allowed. I have video titles in directory/file names for ease of organisation, and I (and probably every non-Windows, non-Traefik user) have had no issues with Japanese characters, emojis, spaces, etc. thus far. Having this be a toggleable setting would be nice. |
|
In tests I've had issues with weird characters causing tests to fail so I still think doing this is a good idea. I do think it's fine to be more lenient about RFC3986 so I'll allow non-english characters while still removing emojis as browsers are pretty good and encoding characters. |
2 participants
removes emojis and additional unsafe characters