refactor: enhance clip name sanitization and path handling in media handler #408
Conversation
…andler - Improved the `sanitizeClipName` function to handle path traversal attempts more robustly, including case-insensitive checks for Windows compatibility. - Added normalization of path separators and ensured the removal of the 'clips/' prefix is case-insensitive. - Enhanced the `getFullPath` and `getSpectrogramPath` functions for consistent path handling and validation. - Implemented checks for final path length to prevent exceeding system limits. - Added detailed debug logging for better traceability during path sanitization and file existence checks.
WalkthroughThe pull request enhances the security and robustness of media file handling in the Changes
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
internal/httpcontroller/handlers/media.go (8)
64-67: Consider using a consolidated approach for normalizing path separators.
For instance,filepath.ToSlash()automatically converts all path separators to forward slashes, and repeated slashes can be collapsed in a single pass rather than performing multiple string replacements.- cleanPath = strings.ReplaceAll(cleanPath, "\\", "/") - cleanPath = strings.ReplaceAll(cleanPath, "//", "/") + cleanPath = filepath.ToSlash(cleanPath) + // Optionally handle multiple slashes if needed: + for strings.Contains(cleanPath, "//") { + cleanPath = strings.ReplaceAll(cleanPath, "//", "/") + }
76-80: Ensure ‘clips/’ removal only applies at path start.
If a path has “clips/” in the middle, it might be unintentionally removed. Currently, the code checks only the prefix, which is good, but be mindful of partial matches. This seems fine for your use case, just clarifying the behavior.
109-113: Confirm the chosen path length limit.
The limit of 250 might suffice on most systems, but some Windows setups allow 260 or even longer with extended paths. Consider making this configurable if needed.
131-134: Consider validating boundaries for the export path check here as well.
In the event thatrelativePathhas a partial match, you may inadvertently treat it as starting with the export path.
210-214: Be mindful of logging PII or internal paths.
While these logs are helpful for debugging, ensure they don’t expose sensitive data in production logs.
248-262: Potential concurrency issues.
Multiple requests for the same spectrogram might cause duplicate generation attempts. You could handle concurrency with locks or a “spectrogram in-progress” marker.
277-284: Extra sanitization ingetSpectrogramPathcould unify withsanitizeClipName.
While this separate approach is valid, reusing the same sanitize logic can centralize path security checks.
285-294: Validate actual subdirectory membership for the base directory.
Similar to earlier checks, confirm that the path definitely resides underexportPath.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/httpcontroller/handlers/media.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
internal/httpcontroller/handlers/media.go (14)
100-105: Apply similar boundary checks for non-Windows systems.
Ensurestrings.HasPrefix(cleanPath, exportPath)is validated with a slash boundary to avoid matching siblings.
125-126: Good practice: cleaning the relative path proactively.
This step helps remove any unexpected./or../segments.
137-138: Joining and cleaning the final path is correct.
This ensures the resulting path is canonical and helps avoid path traversal pitfalls.
219-222: Error logging is appropriate.
It provides visibility into sanitization failures, aiding in debugging.
224-227: Full path debug log might be sensitive.
Consider your logging level or whether to redact part of the path if necessary in production.
229-238: Properly handling missing audio files.
Returning a placeholder image is a neat fallback.
264-271: Verifying spectrogram creation post-attempt is sensible.
This final check reduces confusion if generation silently fails.
296-299: Use offilepath.Diris appropriate.
These lines correctly derive the directory path.
301-302: Debug lines look fine.
They won’t affect production flow.
304-304: Spectrogram path computation is straightforward and clean.
Ensures consistent naming.Also applies to: 306-308
311-320: Checking for existing spectrogram is good for performance.
Avoids regenerating spectrograms unnecessarily.
323-332: Ensuring the source audio file’s existence is crucial.
Graceful failure here helps avoid wasted processing.
334-334: Return statement looks correct.
Nothing else to address here.
89-98: Validate in Windows mode that the path boundary is respected.
Usingstrings.HasPrefix(cleanPathLower, exportPathLower)might incorrectly match ifexportPathis “C:\export” andcleanPathis “C:\exportxyz”. Consider ensuring a trailing slash boundary when checking.if strings.HasPrefix(cleanPathLower, exportPathLower) { cleanPath = cleanPath[len(exportPath):] cleanPath = strings.TrimPrefix(cleanPath, string(os.PathSeparator)) } else { return "", fmt.Errorf("invalid path: absolute path not under export directory") } +// Optionally confirm boundary: +if len(cleanPathLower) > len(exportPathLower) { + nextChar := cleanPathLower[len(exportPathLower)] + if nextChar != '/' && nextChar != '\\' { + return "", fmt.Errorf("invalid path: not a proper subdirectory") + } +}
| // Check for path traversal attempts | ||
| if strings.Contains(cleanPath, "../") || strings.Contains(cleanPath, "..\\") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen path traversal detection.
Relying solely on checking for '../' or '..\\' can be bypassed if encoded differently or if path segments are re-encoded. Consider comparing the absolute cleaned path against the base export directory to confirm it’s a valid subpath.
if strings.Contains(cleanPath, "../") || strings.Contains(cleanPath, "..\\") {
h.Debug("sanitizeClipName: Path traversal attempt detected: %s", cleanPath)
return "", ErrPathTraversal
}
+// Alternative approach
+absPath, err := filepath.Abs(filepath.Join(conf.Setting().Realtime.Audio.Export.Path, cleanPath))
+if err != nil {
+ return "", err
+}
+exportAbs, _ := filepath.Abs(conf.Setting().Realtime.Audio.Export.Path)
+if !strings.HasPrefix(absPath, exportAbs) {
+ return "", ErrPathTraversal
+}Committable suggestion skipped: line range outside the PR's diff.
- Improved the `sanitizeClipName` function to include absolute path resolution for better security against path traversal attacks. - Added checks to ensure the resolved path remains within the designated export directory. - Enhanced debug logging for clearer traceability of path sanitization processes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/httpcontroller/handlers/media.go (1)
124-127: Consider making the path length limit configurable.The hard-coded path length limit of 250 characters could be made configurable to accommodate different system requirements.
// MaxClipNameLength is the maximum allowed length for a clip name const MaxClipNameLength = 255 +// MaxPathLength is the maximum allowed length for the full file path +const MaxPathLength = 250 // AllowedCharacters is a regex pattern for allowed characters in clip names const AllowedCharacters = `^[a-zA-Z0-9_/.-]+$` // In the sanitizeClipName function: - if len(fullPath) > 250 { // Safe limit for most OS + if len(fullPath) > MaxPathLength { return "", fmt.Errorf("final path length exceeds system limits") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/httpcontroller/handlers/media.go(4 hunks)
🔇 Additional comments (5)
internal/httpcontroller/handlers/media.go (5)
64-89: LGTM! Robust path traversal prevention implemented.The implementation correctly resolves and compares absolute paths to prevent path traversal attacks.
90-94: LGTM! Case-insensitive prefix handling added.The implementation properly handles the 'clips/' prefix removal with case-insensitivity for better cross-platform compatibility.
139-152: LGTM! Improved path handling with proper sanitization.The function now properly handles paths by:
- Cleaning paths before processing
- Using case-insensitive comparison for Windows compatibility
- Correctly handling paths that already include the export path
291-348: LGTM! Robust path handling and validation in getSpectrogramPath.The function implementation includes:
- Proper path cleaning and normalization
- Case-insensitive path comparison
- Comprehensive debug logging
- Thorough existence checks for both audio and spectrogram files
224-287: Consider atomic file operations to prevent race conditions.The file existence checks and subsequent operations are not atomic, which could lead to race conditions in a concurrent environment. Consider using atomic operations or file locks where possible.
sanitizeClipNamefunction to handle path traversal attempts more robustly, including case-insensitive checks for Windows compatibility.getFullPathandgetSpectrogramPathfunctions for consistent path handling and validation.