add timestamp to the export filename #366
Conversation
WalkthroughThe changes in this pull request enhance the item export feature in the Changes
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 golangci-lint (1.62.2)backend/app/api/handlers/v1/v1_ctrl_items.go349-349: illegal rune literal (typecheck) 🔇 Additional comments (2)backend/app/api/handlers/v1/v1_ctrl_items.go (2)
The time package import is correctly placed and necessary for the timestamp functionality. Line range hint Let's verify there are no other timestamp implementations that should follow the same format for consistency. ✅ Verification successfulThe previous search might have been too specific. Let's broaden our search to catch different variations of timestamp formatting and CSV operations where timestamps might be used. Timestamp format implementation is consistent and secure The timestamp format
Security recommendation: The current implementation is secure as it:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other timestamp format implementations
rg -l "time\.Now\(\)\.Format\(" | grep -v "v1_ctrl_items.go"
Length of output: 60 Script: #!/bin/bash
# Let's try a broader search for timestamp-related patterns
rg -A 2 "time\.Now\(\)" --type go
# Also search for Format specifically
rg -A 2 "\.Format\(" --type go
# And look for any CSV-related operations that might have timestamps
rg -A 2 "\.csv" --type go
Length of output: 14942 🧰 Tools🪛 golangci-lint (1.62.2)349-349: illegal rune literal (typecheck) 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
backend/app/api/handlers/v1/v1_ctrl_items.go (2)
344-346: Consider sanitizing the filename for additional securityWhile the current implementation is functional, consider adding filename sanitization to prevent potential header injection attacks. Even though we're generating the filename internally, it's a good security practice.
Consider adding a helper function:
+func sanitizeFilename(filename string) string { + // Remove or replace potentially dangerous characters + return strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', + r >= 'A' && r <= 'Z', + r >= '0' && r <= '9', + r == '-' || r == '_' || r == '.': + return r + default: + return '-' + } + }, filename) +}And use it:
-filename := fmt.Sprintf("homebox-items_%s.csv", timestamp) +filename := sanitizeFilename(fmt.Sprintf("homebox-items_%s.csv", timestamp))
Line range hint
338-354: Consider adding rate limiting for file downloadsThe export endpoint could be vulnerable to DoS attacks if users repeatedly request large exports. Consider implementing rate limiting for this endpoint.
Suggestions:
- Add a rate limiter middleware
- Consider implementing request size limits
- Add monitoring for excessive downloads
Would you like assistance in implementing these security measures?
🛑 Comments failed to post (1)
backend/app/api/handlers/v1/v1_ctrl_items.go (1)
349-349: 🛠️ Refactor suggestion
Add quotes to Content-Disposition filename for security
The Content-Disposition header should have the filename properly quoted to prevent potential header injection and ensure proper handling of special characters.
-w.Header().Set("Content-Disposition", fmt.Sprintf("attachment;filename=%s", filename)) +w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filename))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filename))
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
backend/app/api/handlers/v1/v1_ctrl_items.go (2)
345-345: Consider moving the timestamp format to a constantThe timestamp format string "2006-01-02_15-04-05" should be defined as a constant at package level for better maintainability and reusability.
+const ( + ExportTimestampFormat = "2006-01-02_15-04-05" +) func (ctrl *V1Controller) HandleItemsExport() errchain.HandlerFunc {
Line range hint
344-353: Security: Add content security headersConsider adding security headers to prevent content sniffing and force file download.
w.Header().Set("Content-Type", "text/csv") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Content-Security-Policy", "default-src 'none'") w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", filename))🧰 Tools
🪛 golangci-lint (1.62.2)
349-349: illegal rune literal
(typecheck)
🛑 Comments failed to post (1)
backend/app/api/handlers/v1/v1_ctrl_items.go (1)
349-349:
⚠️ Potential issueFix string literal syntax and add filename sanitization
There are two issues here:
- Using single quotes instead of double quotes for string literal
- Missing filename sanitization for security
Apply this fix:
- w.Header().Set("Content-Disposition", fmt.Sprintf('attachment;filename=%s', filename)) + // Sanitize filename to prevent header injection + sanitizedFilename := strings.ReplaceAll(filename, `"`, `\"`) + w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", sanitizedFilename))This change:
- Fixes the string literal syntax
- Properly escapes quotes in filename
- Follows RFC 6266 format for Content-Disposition header
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Sanitize filename to prevent header injection sanitizedFilename := strings.ReplaceAll(filename, `"`, `\"`) w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", sanitizedFilename))🧰 Tools
🪛 golangci-lint (1.62.2)
349-349: illegal rune literal
(typecheck)
|
I will reset it back to the original double quotes and wait for a human to say what to fix. |
What type of PR is this?
What this PR does / why we need it:
This PR introduces the ability to include a timestamp in the filenames of exported CSVs in the items export feature. Previously, filenames were static, making it difficult to distinguish between multiple exports. Adding a timestamp ensures that each export is uniquely identifiable and reflects the time of export.
Changes:
HandleItemsExportmethod to dynamically generate a timestamp using Go’s time formatting.Content-Dispositionheader to include the dynamically generated filename.Special notes for your reviewer:
I am new to contributing to any software and new to the Go language. If I made any mistakes, I would love to be informed and guided. I thought this change was necessary, so I got the timestamp code from StackOverflow and made it work.
Testing
I manually exported files after several minutes and confirmed that the timestamp in the filenames was accurate. I didn’t perform any other tests. Please advise on additional testing methods if necessary.
Summary by CodeRabbit