add label generation api #498
Conversation
WalkthroughThis pull request introduces a comprehensive label maker feature for Homebox, enabling QR code generation and printing for locations, items, and assets. The implementation spans multiple components, including backend configuration, API handlers, frontend components, and documentation. The changes add new routes, configuration options, and user interface elements to support label generation and printing functionality. Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
🔇 Additional comments (3)
✨ 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 14
🔭 Outside diff range comments (1)
backend/app/api/static/docs/swagger.yaml (1)
Line range hint
1042-1044: Fix content-type mismatch in API specification.The endpoints are documented to produce "application/json" but return "image/jpeg". This should be corrected to:
produces: - ["application/json"] + ["image/jpeg"]Also applies to: 1081-1083, 1120-1122
🧹 Nitpick comments (8)
backend/pkgs/labelmaker/labelmaker.go (3)
1-1: Add a package-level comment for clarity and compliance with lint rules.Consider adding a brief comment describing what this package provides, aligning with Go's best practices.
🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: ST1000: at least one file in a package should have a package comment
(stylecheck)
24-34: Use "URL" instead of "Url" to conform to Go naming conventions.Per Go style conventions (ST1003), all acronyms should be capitalized in exported identifiers.
- Url string + URL string🧰 Tools
🪛 golangci-lint (1.62.2)
34-34: ST1003: struct field Url should be URL
(stylecheck)
52-58: Avoid unnecessary conversions.The conversions at lines 58 and 145/159 in the static analysis indicate possibly redundant casting. Check and remove them if not strictly necessary.
🧰 Tools
🪛 golangci-lint (1.62.2)
58-58: unnecessary conversion
(unconvert)
backend/internal/sys/config/conf.go (2)
58-58: Rename the YAML tag to match the field name.Currently,
yaml:"string"might be unintentional. If you intended to label the field asprint_command, rename the tag accordingly.- PrintCommand *string `yaml:"string"` + PrintCommand *string `yaml:"print_command"`
21-29: Confirm that added config fields are properly documented.The new
LabelMakerfield inConfigshould be clearly documented, explaining each property within your configuration reference to help users configure label printing properly.frontend/components/global/LabelMaker.vue (1)
55-67: Add type safety to label type handling.The current implementation uses string literals for type checking. Consider using TypeScript enums or union types for better type safety.
+type LabelType = 'item' | 'location' | 'asset'; + function getLabelUrl(print: boolean): string { const params = { print }; - if (props.type === "item") { + if (props.type === ('item' as LabelType)) { return route(`/labelmaker/item/${props.id}`, params); - } else if (props.type === "location") { + } else if (props.type === ('location' as LabelType)) { return route(`/labelmaker/location/${props.id}`, params); - } else if (props.type === "asset") { + } else if (props.type === ('asset' as LabelType)) { return route(`/labelmaker/asset/${props.id}`, params); } else { throw new Error(`Unexpected labelmaker type ${props.type}`); } }backend/app/api/handlers/v1/controller.go (1)
134-134: Validate print command configuration before exposing status.The current implementation directly exposes the print command status without validation.
Add validation:
- LabelPrinting: ctrl.config.LabelMaker.PrintCommand != nil, + LabelPrinting: ctrl.config.LabelMaker.PrintCommand != nil && ctrl.validatePrintCommand(),Add this helper method to the controller:
func (ctrl *V1Controller) validatePrintCommand() bool { if ctrl.config.LabelMaker.PrintCommand == nil { return false } // Validate that the print command doesn't contain shell metacharacters return !strings.ContainsAny(*ctrl.config.LabelMaker.PrintCommand, "&|;<>$`\\") }backend/app/api/static/docs/swagger.json (1)
1057-1062: Improve print parameter specification.The print parameter's default value is mentioned in the description but not formally specified in the OpenAPI schema.
Add the default value to the schema for all three labelmaker endpoints:
{ "type": "boolean", "description": "Print this label, defaults to false", "name": "print", - "in": "query" + "in": "query", + "default": false }Also applies to: 1096-1101, 1135-1140
🛑 Comments failed to post (14)
backend/pkgs/labelmaker/labelmaker.go (1)
180-220:
⚠️ Potential issueSanitize external commands to prevent security vulnerabilities.
You are executing a command that may be influenced by user input in
PrintLabel. Ensure the command template is not susceptible to injection or arbitrary command execution. Consider validating or restricting user inputs or employing a safer approach (e.g., using well-known command line utilities with strict arguments).backend/app/api/handlers/v1/helpers.go (1)
20-30: 🛠️ Refactor suggestion
Validate URL parsing and usage to avoid potential security issues.
When stripping paths from a URL, carefully handle untrusted input to ensure maliciously crafted URLs do not cause unexpected behavior. Logging the error is good, but consider further validation if the URL is intended to be used in other critical operations.
backend/internal/sys/config/conf.go (1)
53-59:
⚠️ Potential issueSecurity recommendation: Provide safe defaults.
Since these settings directly affect label generation and printing, ensure the default configuration is safe and does not inadvertently allow remote code execution when specifying a
PrintCommand. Provide signs in documentation to remind users.frontend/components/global/LabelMaker.vue (1)
46-53:
⚠️ Potential issueEnhance security for file download implementation.
The current implementation of
downloadLabelcreates and manipulates DOM elements directly, which could pose security risks. Consider using theContent-Dispositionheader from the backend instead.Apply this diff to improve security:
function downloadLabel() { - const link = document.createElement("a"); - link.download = `label-${props.id}.jpg`; - link.href = getLabelUrl(false); - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); + window.location.href = `${getLabelUrl(false)}&download=true`; }Committable suggestion skipped: line range outside the PR's diff.
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (3)
60-61:
⚠️ Potential issueSecurity: Avoid using Referer header for URL construction.
Using the Referer header for URL construction is unsafe as it can be spoofed or manipulated.
Consider using a configured base URL instead:
- hbUrl := GetHBURL(r.Header.Get("Referer"), ctrl.url) + hbUrl := ctrl.config.BaseURL return generateOrPrint(ctrl, w, r, location.Name, "Homebox Location", fmt.Sprintf("%s/location/%s", hbUrl, location.ID))Committable suggestion skipped: line range outside the PR's diff.
103-124: 🛠️ Refactor suggestion
Add security headers to prevent XSS and other attacks.
The handler should include security headers to protect against various web vulnerabilities.
Add these security headers at the beginning of the handler:
func (ctrl *V1Controller) HandleGetAssetLabel() errchain.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) error { + // Set security headers + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("X-Frame-Options", "DENY") + w.Header().Set("Content-Security-Policy", "default-src 'self'") + assetIDParam := chi.URLParam(r, "id")📝 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.func (ctrl *V1Controller) HandleGetAssetLabel() errchain.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) error { // Set security headers w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("X-Frame-Options", "DENY") w.Header().Set("Content-Security-Policy", "default-src 'self'") assetIDParam := chi.URLParam(r, "id") assetIDParam = strings.ReplaceAll(assetIDParam, "-", "") assetID, err := strconv.ParseInt(assetIDParam, 10, 64) if err != nil { return err } auth := services.NewContext(r.Context()) item, err := ctrl.repo.Items.QueryByAssetID(auth, auth.GID, repo.AssetID(assetID), 0, 1) if err != nil { return err } if len(item.Items) == 0 { return validate.NewRequestError(fmt.Errorf("failed to find asset id"), http.StatusNotFound) } hbUrl := GetHBURL(r.Header.Get("Referer"), ctrl.url) return generateOrPrint(ctrl, w, r, item.Items[0].AssetID.String(), fmt.Sprintf("%s\nHomebox Asset", item.Items[0].Name), fmt.Sprintf("%s/a/%s", hbUrl, item.Items[0].AssetID.String())) }
18-35:
⚠️ Potential issueEnhance security and error handling in generateOrPrint.
Several security and robustness improvements are needed:
- Missing content type headers
- No validation of configuration parameters
- Improper error handling for response writer
Apply these improvements:
func generateOrPrint(ctrl *V1Controller, w http.ResponseWriter, r *http.Request, title string, description string, url string) error { + // Validate configuration parameters + if ctrl.config.LabelMaker.Width <= 0 || ctrl.config.LabelMaker.Height <= 0 { + return validate.NewRequestError(fmt.Errorf("invalid label dimensions"), http.StatusInternalServerError) + } + params := labelmaker.NewGenerateParams(int(ctrl.config.LabelMaker.Width), int(ctrl.config.LabelMaker.Height), int(ctrl.config.LabelMaker.Padding), ctrl.config.LabelMaker.FontSize, title, description, url) print := queryBool(r.URL.Query().Get("print")) if print { err := labelmaker.PrintLabel(ctrl.config, ¶ms) if err != nil { return err } + w.Header().Set("Content-Type", "text/plain") w.Write([]byte("Printed!")) } else { + w.Header().Set("Content-Type", "image/jpeg") return labelmaker.GenerateLabel(w, ¶ms) } return nil }📝 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.func generateOrPrint(ctrl *V1Controller, w http.ResponseWriter, r *http.Request, title string, description string, url string) error { // Validate configuration parameters if ctrl.config.LabelMaker.Width <= 0 || ctrl.config.LabelMaker.Height <= 0 { return validate.NewRequestError(fmt.Errorf("invalid label dimensions"), http.StatusInternalServerError) } params := labelmaker.NewGenerateParams(int(ctrl.config.LabelMaker.Width), int(ctrl.config.LabelMaker.Height), int(ctrl.config.LabelMaker.Padding), ctrl.config.LabelMaker.FontSize, title, description, url) print := queryBool(r.URL.Query().Get("print")) if print { err := labelmaker.PrintLabel(ctrl.config, ¶ms) if err != nil { return err } w.Header().Set("Content-Type", "text/plain") w.Write([]byte("Printed!")) } else { w.Header().Set("Content-Type", "image/jpeg") return labelmaker.GenerateLabel(w, ¶ms) } return nil }backend/app/api/routes.go (1)
165-168: 🛠️ Refactor suggestion
Add rate limiting to protect label generation endpoints.
Label generation endpoints could be resource-intensive. Consider adding rate limiting to prevent abuse.
Add rate limiting middleware:
+ // Labelmaker rate limiting middleware + labelMakerLimiter := middleware.NewRateLimit( + middleware.RateLimit{ + Window: 1 * time.Minute, + Limit: 10, + Strategy: "sliding-window", + }, + ) + // Labelmaker - r.Get("/labelmaker/location/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetLocationLabel(), userMW...)) - r.Get("/labelmaker/item/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetItemLabel(), userMW...)) - r.Get("/labelmaker/asset/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetAssetLabel(), userMW...)) + r.Get("/labelmaker/location/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetLocationLabel(), append(userMW, labelMakerLimiter)...)) + r.Get("/labelmaker/item/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetItemLabel(), append(userMW, labelMakerLimiter)...)) + r.Get("/labelmaker/asset/{id}", chain.ToHandlerFunc(v1Ctrl.HandleGetAssetLabel(), append(userMW, labelMakerLimiter)...))Committable suggestion skipped: line range outside the PR's diff.
backend/app/api/handlers/v1/v1_ctrl_items.go (1)
341-341: 💡 Codebase verification
🚨 Security: URL validation required in GetHBURL function
The current implementation directly uses the Referer header without any validation, which could lead to security vulnerabilities:
- Add URL validation to prevent malformed or malicious URLs
- Implement URL sanitization before usage
- Consider using
url.Parse()to validate URL structure- Add allowlist validation for expected domains
🔗 Analysis chain
Verify security implications of the new GetHBURL function.
The change from local
getHBURLto importedGetHBURLrequires security validation to prevent potential URL manipulation attacks.Run this script to analyze the implementation of
GetHBURL:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of GetHBURL for security concerns # Test: Look for URL validation and sanitization in the new implementation echo "Checking GetHBURL implementation..." ast-grep --pattern 'func GetHBURL($_, $_) $$$' backend/app/api/handlers/v1/helpers.go echo "Checking for URL validation..." rg -A 5 'GetHBURL' backend/app/api/handlers/v1/helpers.goLength of output: 424
backend/app/api/static/docs/docs.go (1)
1081-1119: 🛠️ Refactor suggestion
Implement rate limiting for label generation endpoints.
The new label generation endpoints could be resource-intensive. Consider implementing rate limiting to prevent DoS attacks.
Apply rate limiting middleware to these endpoints. Example implementation:
+ import "github.com/go-chi/httprate" func (app *App) routes() { + // Rate limit: 100 requests per minute + labelRateLimit := httprate.LimitByIP(100, 1*time.Minute) + app.router.With(labelRateLimit).Get("/v1/labelmaker/item/{id}", app.handleItemLabel) + app.router.With(labelRateLimit).Get("/v1/labelmaker/location/{id}", app.handleLocationLabel) + app.router.With(labelRateLimit).Get("/v1/labelmaker/assets/{id}", app.handleAssetLabel) }Also applies to: 1120-1158
docs/en/configure-homebox.md (1)
29-33: 🛠️ Refactor suggestion
Document security considerations for label maker configuration.
The new label maker configuration, especially
HBOX_LABEL_MAKER_PRINT_COMMAND, requires security documentation:
- Warn about command injection risks
- Provide secure configuration examples
Add security documentation:
| HBOX_LABEL_MAKER_PRINT_COMMAND | | the command to use for printing labels. if empty, label printing is disabled. `{{.FileName}}` in the command will be replaced with the jpg filename of the label | +| | | **Security Warning**: The print command is executed with the same privileges as the Homebox process. Ensure the command and its arguments are properly validated and sanitized to prevent command injection attacks. Example of secure configuration: `HBOX_LABEL_MAKER_PRINT_COMMAND="lp -d myprinter {{.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.| HBOX_LABEL_MAKER_WIDTH | 526 | width for generated labels in pixels | | HBOX_LABEL_MAKER_HEIGHT | 200 | height for generated labels in pixels | | HBOX_LABEL_MAKER_PADDING | 8 | padding between edges of label and around QR code in pixels | | HBOX_LABEL_MAKER_FONT_SIZE | 32.0 | the size of the labels font | | HBOX_LABEL_MAKER_PRINT_COMMAND | | the command to use for printing labels. if empty, label printing is disabled. `{{.FileName}}` in the command will be replaced with the jpg filename of the label | | | | **Security Warning**: The print command is executed with the same privileges as the Homebox process. Ensure the command and its arguments are properly validated and sanitized to prevent command injection attacks. Example of secure configuration: `HBOX_LABEL_MAKER_PRINT_COMMAND="lp -d myprinter {{.FileName}}"` |backend/app/api/static/docs/swagger.json (3)
1042-1044:
⚠️ Potential issueFix response content type inconsistency.
The endpoints are marked to produce "application/json" but the response is documented as "image/jpeg". Update the
producesfield to match the actual response type.Apply this change to all three labelmaker endpoints:
"produces": [ - "application/json" + "image/jpeg" ],Also applies to: 1081-1083, 1120-1122
1064-1071: 🛠️ Refactor suggestion
Document error responses for better API clarity.
The endpoints should document possible error responses for scenarios like:
- 401 Unauthorized: When the Bearer token is invalid or missing
- 404 Not Found: When the requested ID doesn't exist
- 500 Internal Server Error: For server-side errors
Add error responses to all three labelmaker endpoints. Example:
"responses": { "200": { "description": "image/jpeg", "schema": { "type": "string" } }, + "401": { + "description": "Unauthorized" + }, + "404": { + "description": "Not Found" + }, + "500": { + "description": "Internal Server Error" + } }Also applies to: 1103-1110, 1142-1149
1035-1151: 💡 Codebase verification
🔒 Security Controls Need Enhancement
The labelmaker endpoints require additional security measures:
- Add rate limiting middleware to prevent DoS through excessive label generation (e.g., max 10 requests per minute per user)
- Implement audit logging for label generation/printing activities
- Enhance input validation:
- Add length limits for IDs
- Implement strict format validation
- Sanitize all input parameters
- Consider adding request size limits to prevent memory exhaustion
Example middleware implementation:
func RateLimitLabels(next http.Handler) http.Handler { limiter := rate.NewLimiter(rate.Limit(10), 1) // 10 requests per minute return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !limiter.Allow() { http.Error(w, "Too many requests", http.StatusTooManyRequests) return } next.ServeHTTP(w, r) }) }🔗 Analysis chain
Security: Review authentication and authorization controls.
While the endpoints are secured with Bearer token authentication, consider these additional security measures:
- Rate limiting to prevent DoS attacks through excessive label generation
- Input validation for the ID parameter to prevent injection attacks
- Audit logging for label generation and printing activities
- Access control policies to restrict label generation to authorized users
Let's verify the implementation of these security controls:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for rate limiting middleware echo "Checking for rate limiting implementation..." rg -l "rate.*limit|throttle" --type go # Check for input validation echo "Checking for ID parameter validation..." rg -l "validate.*id|sanitize.*id" --type go # Check for audit logging echo "Checking for audit logging implementation..." rg -l "audit.*log|activity.*log" --type go # Check for access control echo "Checking for access control policies..." rg -l "authorize|permission|acl" --type goLength of output: 986
Script:
#!/bin/bash # Check labelmaker controller implementation echo "=== Labelmaker Controller ===" rg -A 10 "func.*GetAssetLabel|func.*GetItemLabel|func.*GetLocationLabel" backend/app/api/handlers/v1/v1_ctrl_labelmaker.go # Check middleware setup echo -e "\n=== Middleware Chain ===" rg -A 10 "func.*Setup.*Middleware|func.*Use.*Middleware" backend/app/api/middleware.go # Check for any request limiting patterns echo -e "\n=== Request Limiting Patterns ===" rg -A 5 "func.*Limit|max.*requests|max.*concurrent" backend/app/api/middleware.goLength of output: 1689
Script:
#!/bin/bash # Check authentication middleware echo "=== Authentication Implementation ===" rg -A 10 "func.*Authenticate|func.*Auth.*Middleware" backend/app/api/middleware.go backend/internal/web/mid/*.go # Check for global request controls in main setup echo -e "\n=== Global Controls ===" rg -A 5 "func.*main|func.*Setup|func.*Init" backend/app/api/main.go backend/cmd/**/main.goLength of output: 807
jake-walker
force-pushed
the
label-generation
branch
from
d7588c7 to
0d2e6db
Compare
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: 4
🧹 Nitpick comments (5)
backend/pkgs/labelmaker/labelmaker.go (3)
1-1: Add a package-level comment.According to static analysis (stylecheck), at least one file in a package should have a package comment describing its purpose.
🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: ST1000: at least one file in a package should have a package comment
(stylecheck)
34-34: Use consistent naming:URLvsUrl.Go naming conventions suggest using
URL(all caps) instead ofUrlfor struct fields and variables.- Url string + URL string🧰 Tools
🪛 golangci-lint (1.62.2)
34-34: ST1003: struct field Url should be URL
(stylecheck)
58-58: Avoid unnecessary conversions.Static analysis indicates unnecessary conversions at lines 58, 145, and 159.
- return int(ctx.PointToFixed(float64(width)).Round()) + return ctx.PointToFixed(float64(width)).Round()(similarly for lines 145 and 159, returning an int if indeed needed, or removing unnecessary type conversions.)
Also applies to: 145-145, 159-159
🧰 Tools
🪛 golangci-lint (1.62.2)
58-58: unnecessary conversion
(unconvert)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
18-35: Handle error statuses gracefully for advanced user feedback.Currently, errors in generating or printing labels are returned with minimal feedback to the client. Consider returning an HTTP status code that more clearly indicates the type of failure (e.g., 500 for server errors, 400 for client-side misconfiguration).
backend/app/api/handlers/v1/controller.go (1)
134-134: Consider conditional logic for labeling.When
LabelPrintingis toggled in the API summary, if printing is unexpectedly off but the user has a valid command, consider logging the state or clarifying to the user how to enable/disable it.
🛑 Comments failed to post (4)
backend/pkgs/labelmaker/labelmaker.go (1)
180-220:
⚠️ Potential issueConsider validating print command parameters akin to a security measure.
While printing commands are user-specified in configuration, it is prudent to log or validate them to ensure they’re from a trusted source. Failing to do so could lead to a command injection vulnerability if your environment is untrusted.
backend/app/api/static/docs/swagger.yaml (3)
1409-1480: 🛠️ Refactor suggestion
Add security headers for label endpoints.
For all three label endpoints, ensure proper security headers are set:
- Content-Security-Policy
- X-Content-Type-Options: nosniff
- X-Frame-Options: DENY
- Cache-Control headers for sensitive data
Apply these headers in the middleware or handler:
+ w.Header().Set("Content-Security-Policy", "default-src 'self'") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("X-Frame-Options", "DENY") + w.Header().Set("Cache-Control", "no-store")Committable suggestion skipped: line range outside the PR's diff.
1433-1456: 💡 Codebase verification
Access controls are properly implemented with room for enhancement
The item label endpoint has robust security measures:
- Authentication via Bearer token
- Role-based access control via middleware
- Group-level authorization checks
- Input parameter validation
However, consider implementing rate limiting to prevent potential abuse of the label generation service.
🔗 Analysis chain
Verify access controls for item label generation.
Similar security considerations apply to the item label endpoint:
- Ensure proper access control checks
- Validate input parameters
- Consider adding rate limiting
Run this script to check for access control implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if access control checks are implemented for item labels ast-grep --pattern 'func $_(.*) { $$$ if !hasPermission($_, $_) { $$$ } $$$ }'Length of output: 92
Script:
#!/bin/bash # Find the implementation file for the labelmaker item endpoint rg -l "labelmaker/item" --type-add 'code:*.{go,py,js,ts}' -t code # Also search for potential route definitions rg -l "func.*labelmaker.*item" --type-add 'code:*.{go,py,js,ts}' -t codeLength of output: 252
Script:
#!/bin/bash # Check the handler implementation cat backend/app/api/handlers/v1/v1_ctrl_labelmaker.go # Check routes for middleware cat backend/app/api/routes.goLength of output: 13505
1457-1480: 💡 Codebase verification
Access controls are properly implemented, but rate limiting is recommended
The location label endpoint has robust security measures:
- ✓ Bearer token authentication
- ✓ Role-based access control
- ✓ Input validation
- ✓ Group-scoped data access
However, consider implementing rate limiting to prevent potential abuse of the label generation service.
🔗 Analysis chain
Verify access controls for location label generation.
The location label endpoint needs similar security measures:
- Implement proper access control checks
- Validate location ID input
- Consider rate limiting
Run this script to check for access control implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if access control checks are implemented for location labels ast-grep --pattern 'func $_(.*) { $$$ if !hasPermission($_, $_) { $$$ } $$$ }'Length of output: 92
Script:
#!/bin/bash # Find the implementation file and route registration rg -l "labelmaker/location/\{id\}" --type-add 'code:*.{go,py,js,ts}' -t code # Look for authentication middleware rg -l "Bearer" --type-add 'code:*.{go,py,js,ts}' -t code # Search for rate limiting implementations rg -l "RateLimit|rate.Limit|RateLimiter" --type-add 'code:*.{go,py,js,ts}' -t codeLength of output: 1191
Script:
#!/bin/bash # Check the label maker handler implementation cat backend/app/api/handlers/v1/v1_ctrl_labelmaker.go # Check middleware configuration cat backend/app/api/middleware.go # Check routes configuration cat backend/app/api/routes.goLength of output: 17560
|
Great work. Currently looking into reactivating HomeBox after I found this actively maintained Fork, this is exactly what I need. Why do the labels not include the location? Would be very handy to know where to put the stuff if lying around. Also the qr code references the url. I do not know if this is a good design decision. If the port or hostname changes, this breaks. If someone is referring by IP to the server in the home network this also is breaky. |
|
Yeah I like that idea of just referencing an ID or something, it makes the QR codes smaller too, but maybe I could have a look at adding an option for it so it can be turned on or off. |
|
Then scanning in the ui is required to support it. What about the location? I would like to be able to have a sticker on the tools where it says this belongs into this box. Example: Item: Wera Screwdriver PZ 2 only asset from its kind item. Located in toolbox 42. I would like to have the option to print QR Code, Name/ID and |
|
That'd be great, I've not started using HomeBox yet so I wouldn't have thought of that but I'll add it in 🙂 |
Printing is the single killer feature for me, since I have got a printer the effort seems reasonable to me. Without a printer and Homebox not supporting it, it seemed pointless/too much effort to me. |
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: 4
🔭 Outside diff range comments (1)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
233-238: Validate print command execution.The code executes shell commands without proper validation, which could lead to command injection.
Add command validation:
+ // Validate command parts against an allowlist + allowedCommands := map[string]bool{"lp": true, "brother_ql": true} + if !allowedCommands[commandParts[0]] { + return fmt.Errorf("invalid print command") + } command := exec.Command(commandParts[0], commandParts[1:]...)
🧹 Nitpick comments (1)
frontend/locales/en.json (1)
55-60: New Localization for Label Maker FeatureThe new
"label_maker"localization keys have been introduced with labels for titles, printing, downloading, and a print confirmation description. Verify that these strings align with the UI components consuming them. Additionally, consider whether additional keys (e.g., including location information as discussed in PR comments) might be useful for enhanced label details.
🛑 Comments failed to post (4)
frontend/pages/scanner.vue (2)
22-28:
⚠️ Potential issueSanitize error messages before display.
Error messages are directly interpolated into the UI, which could potentially expose sensitive information.
Implement error message sanitization:
- errorMessage.value = `${t("scanner.error")}: ${msg}`; + const sanitizedMsg = typeof msg === 'string' ? msg.replace(/[<>'"]/g, '') : 'Unknown error'; + errorMessage.value = `${t("scanner.error")}: ${sanitizedMsg}`;📝 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.const handleError = (error: unknown) => { let msg = error; if (error instanceof Error) { msg = error.message; } const sanitizedMsg = typeof msg === 'string' ? msg.replace(/[<>'"]/g, '') : 'Unknown error'; errorMessage.value = `${t("scanner.error")}: ${sanitizedMsg}`; };
54-60:
⚠️ Potential issueValidate scanned URLs before navigation.
The code directly navigates to the scanned URL's pathname without validation. This could potentially lead to navigation to malicious paths.
Add URL validation before navigation:
- navigateTo(url.pathname); + const validPathPattern = /^\/(?:item|location|a)\/[a-zA-Z0-9-]+$/; + if (!validPathPattern.test(url.pathname)) { + throw new Error(t("scanner.invalid_qr_code")); + } + navigateTo(url.pathname);📝 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.await codeReader.decodeFromVideoDevice(newSource, video.value!, (result, err) => { if (result && !loading.value) { loading.value = true; try { const url = new URL(result.getText()); const validPathPattern = /^\/(?:item|location|a)\/[a-zA-Z0-9-]+$/; if (!validPathPattern.test(url.pathname)) { throw new Error(t("scanner.invalid_qr_code")); } navigateTo(url.pathname); } catch (err) {backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
60-61:
⚠️ Potential issueAvoid using Referer header for URL construction.
Using the Referer header for URL construction is unreliable and potentially insecure as it can be spoofed.
Instead, use a configured base URL:
- hbUrl := GetHBURL(r.Header.Get("Referer"), ctrl.url) + hbUrl := ctrl.config.BaseURL📝 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.hbUrl := ctrl.config.BaseURL return generateOrPrint(ctrl, w, r, location.Name, "Homebox Location", fmt.Sprintf("%s/location/%s", hbUrl, location.ID))🧰 Tools
🪛 golangci-lint (1.62.2)
60-60: ST1003: var hbUrl should be hbURL
(stylecheck)
backend/pkgs/labelmaker/labelmaker.go (1)
220-226:
⚠️ Potential issueValidate template variables.
The command template execution could be vulnerable to injection if the template or variables are manipulated.
Add template variable validation:
+ // Validate filename before template execution + if strings.Contains(f.Name(), "{{") || strings.Contains(f.Name(), "}}") { + return fmt.Errorf("invalid filename") + } if err := commandTemplate.Execute(builder, map[string]string{ "FileName": f.Name(), }); err != nil {📝 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.commandTemplate := template.Must(template.New("command").Parse(*cfg.LabelMaker.PrintCommand)) builder := &strings.Builder{} // Validate filename before template execution if strings.Contains(f.Name(), "{{") || strings.Contains(f.Name(), "}}") { return fmt.Errorf("invalid filename") } if err := commandTemplate.Execute(builder, map[string]string{ "FileName": f.Name(), }); err != nil { return err }
|
I've updated the label generation code to add the location name in the description:
And added a new page where you can scan labels. I've not adjusted the labels to only be IDs, because I think it's probably quite useful to be able to scan them with other things, for example, your phone. But this scan page will ignore the start of the URL so if you were to change the server URL the barcodes would still scan and navigate to the right place from this page.
|
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: 3
🧹 Nitpick comments (2)
frontend/pages/scanner.vue (2)
57-72: Security: Add rate limiting for QR code processingThe current implementation processes QR codes continuously without rate limiting, which could lead to performance issues or potential security vulnerabilities.
Add rate limiting to QR code processing:
+ const SCAN_COOLDOWN = 2000; // 2 seconds + let lastScanTime = 0; await codeReader.decodeFromVideoDevice(newSource, video.value!, (result, err) => { if (result && !loading.value) { + const now = Date.now(); + if (now - lastScanTime < SCAN_COOLDOWN) { + return; + } + lastScanTime = now; loading.value = true;
1-77: Security: Add Content Security Policy (CSP) headersTo enhance security, ensure your application has appropriate CSP headers to control media device access and protect against XSS attacks.
Add the following CSP directives to your application's security headers:
Content-Security-Policy: default-src 'self'; media-src 'self' mediastream:; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline';This will:
- Restrict media access to secure sources
- Allow inline scripts and styles (required for Vue)
- Protect against XSS attacks
🛑 Comments failed to post (3)
frontend/pages/scanner.vue (3)
22-28: 🛠️ Refactor suggestion
Security: Sanitize error messages
The current error handling might expose sensitive information in error messages.
Implement error message sanitization:
const handleError = (error: unknown) => { - let msg = error; - if (error instanceof Error) { - msg = error.message; - } - errorMessage.value = `${t("scanner.error")}: ${msg}`; + // Log the full error for debugging + console.error(error); + // Show sanitized message to user + errorMessage.value = t("scanner.error"); };📝 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.const handleError = (error: unknown) => { // Log the full error for debugging console.error(error); // Show sanitized message to user errorMessage.value = t("scanner.error"); };
61-62:
⚠️ Potential issueSecurity: Add URL validation before navigation
The current implementation directly navigates to the scanned URL's pathname without validation, which could lead to potential security vulnerabilities.
Add URL validation to ensure only expected paths are processed:
- const url = new URL(result.getText()); - navigateTo(url.pathname); + const url = new URL(result.getText()); + // Validate URL origin matches current site + if (url.origin !== window.location.origin) { + throw new Error('Invalid QR code: URL origin mismatch'); + } + // Validate pathname against allowed patterns + const allowedPaths = /^\/(?:items|locations|assets)\/[\w-]+$/; + if (!allowedPaths.test(url.pathname)) { + throw new Error('Invalid QR code: Invalid path'); + } + navigateTo(url.pathname);📝 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.const url = new URL(result.getText()); // Validate URL origin matches current site if (url.origin !== window.location.origin) { throw new Error('Invalid QR code: URL origin mismatch'); } // Validate pathname against allowed patterns const allowedPaths = /^\/(?:items|locations|assets)\/[\w-]+$/; if (!allowedPaths.test(url.pathname)) { throw new Error('Invalid QR code: Invalid path'); } navigateTo(url.pathname);
30-34: 🛠️ Refactor suggestion
Security: Add explicit camera permission handling
The current implementation doesn't explicitly handle camera permissions, which could lead to a poor user experience or security concerns.
Add explicit permission handling:
onMounted(async () => { if (!(navigator && navigator.mediaDevices && "enumerateDevices" in navigator.mediaDevices)) { errorMessage.value = t("scanner.unsupported"); return; } + try { + // Request camera permission explicitly + const stream = await navigator.mediaDevices.getUserMedia({ video: true }); + stream.getTracks().forEach(track => track.stop()); // Release the stream + } catch (err) { + if (err instanceof DOMException && err.name === 'NotAllowedError') { + errorMessage.value = t("scanner.permission_denied"); + return; + } + handleError(err); + return; + }📝 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.onMounted(async () => { if (!(navigator && navigator.mediaDevices && "enumerateDevices" in navigator.mediaDevices)) { errorMessage.value = t("scanner.unsupported"); return; } try { // Request camera permission explicitly const stream = await navigator.mediaDevices.getUserMedia({ video: true }); stream.getTracks().forEach(track => track.stop()); // Release the stream } catch (err) { if (err instanceof DOMException && err.name === 'NotAllowedError') { errorMessage.value = t("scanner.permission_denied"); return; } handleError(err); return; } // ... rest of the onMounted logic if any ... });
That would be great. I've not got a label printer I can automate very easily, so it would be super useful to have someone test it out. I assume it's easy to link up with the command interface I added, but it seems potentially a bit flawed. And I'm certainly open to help on this :) |
Tell me what to do. |
|
Today I took the time to try it out. There was a misconception on my side: My Brother QL 800 is connected to my Windows Workshop PC and not my homeserver. I would like to see no margins in the image, so I can set them on the print dialog and a bit thicker font, to max out the printers ability while wasting as little tape as possible. Also, I was wondering about why jpeg is used and not png, but the jpeg artifacts seem not to matter in this context. I added this to print from dialog. function openBrowserPrintDialog() {
const printWindow = window.open(getLabelUrl(false));
printWindow.onload = function() {
printWindow.print();
};
}
<li>
<button @click="openBrowserPrintDialog">
<MdiPrinterPos name="mdi-printer-pos" class="mr-2" />
{{ $t("components.global.label_maker.print_browser") }}
</button>
</li>It would be also nice if there could be a free text field on the labels so that I can enter my surname and contact like a company ;) But I can create prs on my own to build this...
|
jake-walker
force-pushed
the
label-generation
branch
from
06260e1 to
5ac38e1
Compare
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: 6
🔭 Outside diff range comments (3)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (2)
220-234: Prevent command injection in print command.The current implementation uses template.Parse with user-controlled input, which could lead to command injection.
Validate and sanitize the print command:
+func validatePrintCommand(cmd string) error { + // Only allow specific commands and arguments + allowedCommands := []string{"lp", "brother_ql", "cups-pdf"} + cmdParts := strings.Fields(cmd) + if len(cmdParts) == 0 { + return fmt.Errorf("empty print command") + } + for _, allowed := range allowedCommands { + if cmdParts[0] == allowed { + return nil + } + } + return fmt.Errorf("unauthorized print command") +} func PrintLabel(cfg *config.Config, params *GenerateParameters) error { + if cfg.LabelMaker.PrintCommand == nil { + return fmt.Errorf("no print command specified") + } + + if err := validatePrintCommand(*cfg.LabelMaker.PrintCommand); err != nil { + return err + }
201-209: Secure temporary file handling.The temporary file creation needs additional security measures.
Enhance temporary file security:
- f, err := os.CreateTemp("", "label-*.jpg") + // Use more restrictive permissions + tmpFile := filepath.Join(os.TempDir(), fmt.Sprintf("label-%d.jpg", time.Now().UnixNano())) + f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return err } defer func() { _ = f.Close() - _ = os.Remove(f.Name()) + if err := os.Remove(f.Name()); err != nil { + log.Printf("Failed to remove temporary file: %v", err) + } }()backend/app/api/static/docs/docs.go (1)
1409-1481:🧩 Analysis chain
Ensure proper access control for label generation endpoints
While the endpoints are secured with Bearer authentication, consider additional security measures:
- Rate limiting to prevent DoS through excessive label generation
- Validation of print commands to prevent command injection
- Size limits on generated labels to prevent resource exhaustion
Let's check if rate limiting is implemented:
🏁 Script executed:
#!/bin/bash # Search for rate limiting implementation in the codebase rg -A 5 "rate.*limit|RateLimit"Length of output: 977
Security Recommendation: Implement Rate Limiting and Enhanced Input Validations for Label Generation Endpoints
The recent search for "rate.*limit|RateLimit" didn't reveal any implementation of rate limiting in the codebase—only references within the docs. This suggests that although Bearer authentication is in place, the recommended security measures (rate limiting to mitigate DoS risks, validation of print commands to prevent command injection, and size constraints on generated labels) are missing.
- No evidence of rate limiting logic was found in the repository.
- Additional security mechanisms such as input validation and resource constraints should be added to the label generation endpoints.
Please review the endpoints' implementation and consider integrating middleware or similar measures to address these critical security concerns.
🧹 Nitpick comments (3)
backend/app/api/static/docs/docs.go (1)
1042-1080: Review security implications of QR code data in labelsBased on the PR comments from fidoriel, there's a concern about QR codes containing URLs that could break if server configuration changes. This also has security implications:
- URLs in QR codes could expose internal network information
- URL changes could lead to QR codes pointing to unintended destinations
Consider these security-focused improvements:
- Use relative paths or IDs instead of full URLs in QR codes
- Add a URL redirection service to handle QR code URL changes
- Document security implications in API documentation
frontend/locales/en.json (2)
55-60: Add security confirmation for print actionsThe print confirmation dialog should include security warnings about executing print commands.
Add security-focused confirmation text:
- "confirm_description": "Are you sure you want to print this label?" + "confirm_description": "Are you sure you want to print this label? The print command will be executed on the server."
379-382: Add security notice for camera accessThe scanner functionality requires camera access, which should be clearly communicated to users.
Add privacy-focused message:
- "select_video_source": "Pick a video source", + "select_video_source": "Pick a video source (camera access required)",
🛑 Comments failed to post (6)
backend/app/api/handlers/v1/v1_ctrl_items.go (1)
341-341: 💡 Verification agent
⚠️ Potential issue🧩 Analysis chain
Verify secure URL handling in the new helper function.
The change from
getHBURLtoGetHBURLinvolves URL construction using the Referer header, which can be spoofed. Since these URLs will be embedded in QR codes and CSV exports, ensure proper URL validation is implemented in the helper function.Run this script to analyze the URL validation in the helper function:
Additionally, consider these security recommendations:
- Implement URL allowlisting to restrict valid domains
- Add rate limiting for the export endpoint
- Consider using signed URLs with expiration for QR codes
🏁 Script executed:
#!/bin/bash # Description: Check URL validation in GetHBURL helper function # Search for GetHBURL implementation and its validation logic ast-grep --pattern $'func GetHBURL($_, $_) string { $$$ }' # Search for URL validation patterns rg -A 5 'url\.Parse|url\.URL|strings\.HasPrefix|strings\.TrimPrefix'Length of output: 3836
Secure URL Handling Verification: Issues Found
- The current GetHBURL implementation (in backend/app/api/handlers/v1/helpers.go) only uses url.Parse and then strips the URL to its scheme and host. This approach does not verify that the URL belongs to an allowed domain; attackers could spoof the Referer header with malicious URLs.
- The fallback parameter (ctrl.url) is passed to GetHBURL but isn’t actually utilized on error. Instead of returning the fallback URL upon a parse failure, the helper returns an empty string, which might lead to unexpected behavior downstream.
- There is no additional check (e.g., allowlisting) to ensure that only trusted hostnames are returned. Given that these URLs are later embedded in QR codes and CSV exports, this could open up security risks such as redirecting users to malicious domains.
- Consider also implementing rate limiting or signed URLs with expiration if these URLs serve sensitive or high-traffic endpoints.
Please address these security improvements to mitigate potential manipulation of the Referer header.
frontend/pages/scanner.vue (3)
22-28:
⚠️ Potential issueAvoid exposing error details to users.
Error messages are directly exposed to users, which could leak sensitive information.
Sanitize error messages:
- let msg = error; - if (error instanceof Error) { - msg = error.message; - } - errorMessage.value = `${t("scanner.error")}: ${msg}`; + console.error('Scanner error:', error); + errorMessage.value = t("scanner.error_generic");📝 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.const handleError = (error: unknown) => { console.error('Scanner error:', error); errorMessage.value = t("scanner.error_generic"); };
30-34: 🛠️ Refactor suggestion
Add explicit permission request for camera access.
The code should explicitly request camera permissions and handle denials gracefully.
Add permission handling:
onMounted(async () => { + try { + const permission = await navigator.permissions.query({ name: 'camera' }); + if (permission.state === 'denied') { + errorMessage.value = t("scanner.camera_permission_denied"); + return; + } + } catch (err) { + console.error('Permission API not supported:', err); + } + if (!(navigator && navigator.mediaDevices && "enumerateDevices" in navigator.mediaDevices)) {📝 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.onMounted(async () => { try { const permission = await navigator.permissions.query({ name: 'camera' }); if (permission.state === 'denied') { errorMessage.value = t("scanner.camera_permission_denied"); return; } } catch (err) { console.error('Permission API not supported:', err); } if (!(navigator && navigator.mediaDevices && "enumerateDevices" in navigator.mediaDevices)) { errorMessage.value = t("scanner.unsupported"); return; }
61-62:
⚠️ Potential issueValidate scanned URLs before navigation.
The current implementation directly navigates to scanned URLs without validation, which could lead to open redirect vulnerabilities.
Add URL validation before navigation:
- const url = new URL(result.getText()); - navigateTo(url.pathname); + const url = new URL(result.getText()); + // Only allow internal paths + if (!url.pathname.startsWith('/')) { + throw new Error(t('scanner.invalid_url')); + } + // Sanitize pathname + const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ''); + navigateTo(sanitizedPath);📝 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.const url = new URL(result.getText()); // Only allow internal paths if (!url.pathname.startsWith('/')) { throw new Error(t('scanner.invalid_url')); } // Sanitize pathname const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ''); navigateTo(sanitizedPath);backend/pkgs/labelmaker/labelmaker.go (2)
112-119: 🛠️ Refactor suggestion
Add memory limits for QR code generation.
QR code generation should have memory limits to prevent DoS attacks.
Add memory protection:
+const MaxQRCodeSize = 1000 * 1000 // 1 million pixels func GenerateLabel(w io.Writer, params *GenerateParameters) error { + if params.QrSize * params.QrSize > MaxQRCodeSize { + return fmt.Errorf("QR code size exceeds maximum allowed") + } + + if err := params.Validate(); err != nil { + return err + }📝 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.package labelmaker import ( "fmt" "io" // other necessary imports ) const MaxQRCodeSize = 1000 * 1000 // 1 million pixels func GenerateLabel(w io.Writer, params *GenerateParameters) error { if params.QrSize*params.QrSize > MaxQRCodeSize { return fmt.Errorf("QR code size exceeds maximum allowed") } if err := params.Validate(); err != nil { return err } // Create QR code qr, err := qrcode.New(params.Url, qrcode.Medium) if err != nil { return err } qrImage := qr.Image(params.QrSize) // ... rest of the implementation return nil }
24-35:
⚠️ Potential issueAdd input validation for label parameters.
The GenerateParameters struct lacks input validation, which could lead to resource exhaustion.
Add parameter validation:
+const ( + MaxWidth = 2000 + MaxHeight = 2000 + MaxPadding = 100 + MaxFontSize = 72 + MaxTextLength = 1000 +) + +func (p *GenerateParameters) Validate() error { + if p.Width <= 0 || p.Width > MaxWidth { + return fmt.Errorf("invalid width") + } + if p.Height <= 0 || p.Height > MaxHeight { + return fmt.Errorf("invalid height") + } + if p.Padding < 0 || p.Padding > MaxPadding { + return fmt.Errorf("invalid padding") + } + if len(p.TitleText) > MaxTextLength || len(p.DescriptionText) > MaxTextLength { + return fmt.Errorf("text too long") + } + return nil +}📝 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.type GenerateParameters struct { Width int Height int QrSize int Padding int TitleText string TitleFontSize float64 DescriptionText string DescriptionFontSize float64 Dpi float64 Url string } const ( MaxWidth = 2000 MaxHeight = 2000 MaxPadding = 100 MaxFontSize = 72 MaxTextLength = 1000 ) func (p *GenerateParameters) Validate() error { if p.Width <= 0 || p.Width > MaxWidth { return fmt.Errorf("invalid width") } if p.Height <= 0 || p.Height > MaxHeight { return fmt.Errorf("invalid height") } if p.Padding < 0 || p.Padding > MaxPadding { return fmt.Errorf("invalid padding") } if len(p.TitleText) > MaxTextLength || len(p.DescriptionText) > MaxTextLength { return fmt.Errorf("text too long") } return nil }🧰 Tools
🪛 golangci-lint (1.62.2)
34-34: ST1003: struct field Url should be URL
(stylecheck)
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: 6
🧹 Nitpick comments (1)
backend/pkgs/labelmaker/labelmaker.go (1)
1-2: Add package documentation.Add a package comment to describe the purpose of the labelmaker package.
Apply this diff:
+// Package labelmaker provides functionality for generating and printing labels +// with QR codes for items, locations, and assets in the Homebox system. package labelmaker🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: ST1000: at least one file in a package should have a package comment
(stylecheck)
🛑 Comments failed to post (6)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (4)
47-63: 💡 Verification agent
⚠️ Potential issue🧩 Analysis chain
Security: Avoid using Referer header for URL construction.
Using the Referer header for URL construction is unsafe as it can be spoofed by malicious clients. Consider using a configured base URL instead.
🏁 Script executed:
#!/bin/bash # Check if base URL is configurable rg -A 5 "type Config struct" backend/internal/sys/config/conf.goLength of output: 331
Security: Do not use the Referer header to construct URLs
The code in "backend/app/api/handlers/v1/v1_ctrl_labelmaker.go" still relies on the Referer header for building the URL (via GetHBURL), which can be spoofed by malicious clients. Even though a configurable base URL is hinted at in the "Config" struct’s "Web" field within "backend/internal/sys/config/conf.go", the current implementation does not leverage it. Instead, it uses the potentially unsafe header value.
• Revisit the GetHBURL implementation to either validate the Referer header robustly or, better yet, use a securely configured base URL (e.g., one provided via the Web configuration).
• Consider updating the configuration (if it exists) to include an explicit base URL that can be safely used for URL construction, thereby mitigating the spoofing risk.
Security recommendations for PRs: Always use trusted, server-side configurations for constructing URLs rather than client-provided headers.
🧰 Tools
🪛 golangci-lint (1.62.2)
60-60: ST1003: var hbUrl should be hbURL
(stylecheck)
18-35:
⚠️ Potential issueAdd error handling and security headers.
- Add error handling for
w.Writeoperation- Set appropriate content-type and security headers
Apply this diff:
func generateOrPrint(ctrl *V1Controller, w http.ResponseWriter, r *http.Request, title string, description string, url string) error { params := labelmaker.NewGenerateParams(int(ctrl.config.LabelMaker.Width), int(ctrl.config.LabelMaker.Height), int(ctrl.config.LabelMaker.Padding), ctrl.config.LabelMaker.FontSize, title, description, url) print := queryBool(r.URL.Query().Get("print")) if print { err := labelmaker.PrintLabel(ctrl.config, ¶ms) if err != nil { return err } - w.Write([]byte("Printed!")) + w.Header().Set("Content-Type", "text/plain") + w.Header().Set("X-Content-Type-Options", "nosniff") + if _, err := w.Write([]byte("Printed!")); err != nil { + return fmt.Errorf("failed to write response: %w", err) + } } else { + w.Header().Set("Content-Type", "image/png") + w.Header().Set("X-Content-Type-Options", "nosniff") return labelmaker.GenerateLabel(w, ¶ms) } return nil }📝 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.func generateOrPrint(ctrl *V1Controller, w http.ResponseWriter, r *http.Request, title string, description string, url string) error { params := labelmaker.NewGenerateParams(int(ctrl.config.LabelMaker.Width), int(ctrl.config.LabelMaker.Height), int(ctrl.config.LabelMaker.Padding), ctrl.config.LabelMaker.FontSize, title, description, url) print := queryBool(r.URL.Query().Get("print")) if print { err := labelmaker.PrintLabel(ctrl.config, ¶ms) if err != nil { return err } w.Header().Set("Content-Type", "text/plain") w.Header().Set("X-Content-Type-Options", "nosniff") if _, err := w.Write([]byte("Printed!")); err != nil { return fmt.Errorf("failed to write response: %w", err) } } else { w.Header().Set("Content-Type", "image/png") w.Header().Set("X-Content-Type-Options", "nosniff") return labelmaker.GenerateLabel(w, ¶ms) } return nil }🧰 Tools
🪛 golangci-lint (1.62.2)
29-29: Error return value of
w.Writeis not checked(errcheck)
75-97: 🛠️ Refactor suggestion
Fix variable naming and URL construction.
- Variable
hbUrlshould behbURLper Go naming conventions- Same security concern as location handler regarding Referer header usage
Apply this diff:
- hbUrl := GetHBURL(r.Header.Get("Referer"), ctrl.url) + hbURL := ctrl.config.BaseURL // Assuming BaseURL is added to configCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
94-94: ST1003: var hbUrl should be hbURL
(stylecheck)
109-137:
⚠️ Potential issueAdd input validation for asset ID.
The asset ID sanitization lacks length validation, which could lead to integer overflow. Also has the same URL construction security concern.
Apply this diff:
assetIDParam := chi.URLParam(r, "id") assetIDParam = strings.ReplaceAll(assetIDParam, "-", "") + if len(assetIDParam) > 20 { // Adjust max length as needed + return validate.NewRequestError(fmt.Errorf("asset id too long"), http.StatusBadRequest) + } assetID, err := strconv.ParseInt(assetIDParam, 10, 64)📝 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.func (ctrl *V1Controller) HandleGetAssetLabel() errchain.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) error { assetIDParam := chi.URLParam(r, "id") assetIDParam = strings.ReplaceAll(assetIDParam, "-", "") if len(assetIDParam) > 20 { // Adjust max length as needed return validate.NewRequestError(fmt.Errorf("asset id too long"), http.StatusBadRequest) } assetID, err := strconv.ParseInt(assetIDParam, 10, 64) if err != nil { return err } auth := services.NewContext(r.Context()) item, err := ctrl.repo.Items.QueryByAssetID(auth, auth.GID, repo.AssetID(assetID), 0, 1) if err != nil { return err } if len(item.Items) == 0 { return validate.NewRequestError(fmt.Errorf("failed to find asset id"), http.StatusNotFound) } description := item.Items[0].Name if item.Items[0].Location != nil { description += fmt.Sprintf("\nLocation: %s", item.Items[0].Location.Name) } hbUrl := GetHBURL(r.Header.Get("Referer"), ctrl.url) return generateOrPrint(ctrl, w, r, item.Items[0].AssetID.String(), description, fmt.Sprintf("%s/a/%s", hbUrl, item.Items[0].AssetID.String())) } }🧰 Tools
🪛 golangci-lint (1.62.2)
134-134: ST1003: var hbUrl should be hbURL
(stylecheck)
backend/pkgs/labelmaker/labelmaker.go (2)
201-241: 💡 Verification agent
⚠️ Potential issue🧩 Analysis chain
Security: Sanitize command execution and fix file extension.
- The temporary file uses .jpg extension but contains PNG data
- Command execution from template needs additional sanitization
Apply this diff:
- f, err := os.CreateTemp("", "label-*.jpg") + f, err := os.CreateTemp("", "label-*.png") + // Validate command template before execution + if strings.Contains(*cfg.LabelMaker.PrintCommand, "{{") { + return fmt.Errorf("invalid print command: template syntax not allowed") + } commandTemplate := template.Must(template.New("command").Parse(*cfg.LabelMaker.PrintCommand))Also, consider adding a whitelist of allowed commands for printing:
🏁 Script executed:
#!/bin/bash # Check for any existing command validation rg -A 5 "PrintCommand.*string" backend/internal/sys/config/conf.goLength of output: 381
Security: Sanitize command execution and fix file extension
- The temporary file is being created with a ".jpg" suffix, but the generated output is actually in PNG format. Changing the suffix to ".png" will ensure that the file extension accurately reflects the file content.
- The command execution utilizes a template from the configuration without any validation. Since no existing validation is found (as confirmed by the search in backend/internal/sys/config/conf.go), it's important to sanitize the command template to prevent unexpected templating constructs that could lead to security vulnerabilities. For example, you might check for unwanted template syntax (like the presence of "{{") before proceeding or implement a whitelist of allowed commands.
These issues need to be addressed, e.g., by applying the following diff:
- f, err := os.CreateTemp("", "label-*.jpg") + f, err := os.CreateTemp("", "label-*.png") + // Validate command template before execution + if strings.Contains(*cfg.LabelMaker.PrintCommand, "{{") { + return fmt.Errorf("invalid print command: template syntax not allowed") + } commandTemplate := template.Must(template.New("command").Parse(*cfg.LabelMaker.PrintCommand))Always post security recommendations for PRs.
24-35: 🛠️ Refactor suggestion
Fix field naming and add validation.
- Field
Urlshould beURLper Go naming conventions- Add validation for dimensions and font sizes
Apply this diff:
type GenerateParameters struct { Width int Height int QrSize int Padding int TitleText string TitleFontSize float64 DescriptionText string DescriptionFontSize float64 Dpi float64 - Url string + URL string } +func (p *GenerateParameters) Validate() error { + if p.Width <= 0 || p.Height <= 0 { + return fmt.Errorf("invalid dimensions") + } + if p.TitleFontSize <= 0 || p.DescriptionFontSize <= 0 { + return fmt.Errorf("invalid font sizes") + } + return nil +}📝 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.type GenerateParameters struct { Width int Height int QrSize int Padding int TitleText string TitleFontSize float64 DescriptionText string DescriptionFontSize float64 Dpi float64 URL string } func (p *GenerateParameters) Validate() error { if p.Width <= 0 || p.Height <= 0 { return fmt.Errorf("invalid dimensions") } if p.TitleFontSize <= 0 || p.DescriptionFontSize <= 0 { return fmt.Errorf("invalid font sizes") } return nil }🧰 Tools
🪛 golangci-lint (1.62.2)
34-34: ST1003: struct field Url should be URL
(stylecheck)
jake-walker
force-pushed
the
label-generation
branch
from
b46d1a4 to
ecf3a0e
Compare
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: 2
🔭 Outside diff range comments (1)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
249-263: Secure command execution from template.The current implementation could be vulnerable to command injection if the print command template contains user-controlled input.
Add command validation and sanitization:
+ // Validate command template before execution + if strings.Contains(*cfg.LabelMaker.PrintCommand, ";") || + strings.Contains(*cfg.LabelMaker.PrintCommand, "&&") || + strings.Contains(*cfg.LabelMaker.PrintCommand, "||") { + return fmt.Errorf("invalid print command") + } + commandTemplate := template.Must(template.New("command").Parse(*cfg.LabelMaker.PrintCommand)) builder := &strings.Builder{} if err := commandTemplate.Execute(builder, map[string]string{ "FileName": f.Name(), }); err != nil { return err }
🧹 Nitpick comments (1)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
229-230: Secure temporary file creation.The temporary file permissions are set to 0600, which is good. However, consider using a more secure temporary file creation method.
Consider using
ioutil.TempFilewith a specific pattern:- tmpFile := filepath.Join(os.TempDir(), fmt.Sprintf("label-%d.png", time.Now().UnixNano())) - f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) + f, err := ioutil.TempFile("", "homebox-label-*.png")
🛑 Comments failed to post (2)
frontend/pages/scanner.vue (1)
72-73: 🛠️ Refactor suggestion
Strengthen URL sanitization for better security.
The current URL sanitization only checks for path validity and removes non-alphanumeric characters. Consider implementing additional security measures.
Apply this diff to enhance URL validation:
- const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ""); + // Validate URL structure + if (!url.protocol || !['http:', 'https:'].includes(url.protocol)) { + throw new Error(t("scanner.invalid_url")); + } + // Sanitize and validate path + const sanitizedPath = url.pathname + .replace(/[^a-zA-Z0-9-_/]/g, "") + .replace(/\/+/g, "/"); + if (!sanitizedPath.startsWith("/")) { + throw new Error(t("scanner.invalid_url")); + }📝 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.// Validate URL structure if (!url.protocol || !['http:', 'https:'].includes(url.protocol)) { throw new Error(t("scanner.invalid_url")); } // Sanitize and validate path const sanitizedPath = url.pathname .replace(/[^a-zA-Z0-9-_/]/g, "") .replace(/\/+/g, "/"); if (!sanitizedPath.startsWith("/")) { throw new Error(t("scanner.invalid_url")); } navigateTo(sanitizedPath);backend/pkgs/labelmaker/labelmaker.go (1)
134-137: 🛠️ Refactor suggestion
Add size limits to prevent resource exhaustion.
The label generation function should have maximum size limits to prevent resource exhaustion attacks.
Add size validation:
func GenerateLabel(w io.Writer, params *GenerateParameters) error { + const ( + maxWidth = 2000 + maxHeight = 2000 + ) + if params.Width > maxWidth || params.Height > maxHeight { + return fmt.Errorf("dimensions exceed maximum allowed size") + } if err := params.Validate(); err != nil { return err }📝 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.func GenerateLabel(w io.Writer, params *GenerateParameters) error { const ( maxWidth = 2000 maxHeight = 2000 ) if params.Width > maxWidth || params.Height > maxHeight { return fmt.Errorf("dimensions exceed maximum allowed size") } if err := params.Validate(); err != nil { return err } // ... rest of the function }
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: 4
🧹 Nitpick comments (2)
backend/app/api/static/docs/docs.go (1)
1065-1069: Add validation constraints for the print parameter.The print parameter lacks validation constraints which could lead to unexpected behavior.
Consider adding validation constraints to the print parameter:
- description: Print this label, defaults to false in: query name: print type: boolean + default: falseAlso applies to: 1443-1447
backend/pkgs/labelmaker/labelmaker.go (1)
1-2: Update package comment to follow Go conventions.The package comment should start with "Package labelmaker" according to Go conventions.
-// labelmaker package provides functionality for generating and printing labels for items, locations and assets stored in Homebox +// Package labelmaker provides functionality for generating and printing labels for items, locations and assets stored in Homebox🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: ST1000: package comment should be of the form "Package labelmaker..."
(stylecheck)
🛑 Comments failed to post (4)
backend/app/api/static/docs/docs.go (1)
1042-1158:
⚠️ Potential issueFix response content type mismatch in labelmaker endpoints.
The endpoints specify
produces: ["application/json"]but returnimage/png. This mismatch could cause issues with client implementations.Update the produces field to match the actual response type:
produces: - - application/json + - image/png📝 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."/v1/labelmaker/assets/{id}": { "get": { "security": [ { "Bearer": [] } ], "produces": [ "image/png" ], "tags": [ "Items" ], "summary": "Get Asset label", "parameters": [ { "type": "string", "description": "Asset ID", "name": "id", "in": "path", "required": true }, { "type": "boolean", "description": "Print this label, defaults to false", "name": "print", "in": "query" } ], "responses": { "200": { "description": "image/png", "schema": { "type": "string" } } } } }, "/v1/labelmaker/item/{id}": { "get": { "security": [ { "Bearer": [] } ], "produces": [ "image/png" ], "tags": [ "Items" ], "summary": "Get Item label", "parameters": [ { "type": "string", "description": "Item ID", "name": "id", "in": "path", "required": true }, { "type": "boolean", "description": "Print this label, defaults to false", "name": "print", "in": "query" } ], "responses": { "200": { "description": "image/png", "schema": { "type": "string" } } } } }, "/v1/labelmaker/location/{id}": { "get": { "security": [ { "Bearer": [] } ], "produces": [ "image/png" ], "tags": [ "Locations" ], "summary": "Get Location label", "parameters": [ { "type": "string", "description": "Location ID", "name": "id", "in": "path", "required": true }, { "type": "boolean", "description": "Print this label, defaults to false", "name": "print", "in": "query" } ], "responses": { "200": { "description": "image/png", "schema": { "type": "string" } } } } },backend/pkgs/labelmaker/labelmaker.go (3)
228-271:
⚠️ Potential issueAddress critical security vulnerabilities in PrintLabel function.
Several security issues need to be addressed:
- Command injection risk via template
- Race condition in temporary file creation
- No timeout on command execution
func PrintLabel(cfg *config.Config, params *GenerateParameters) error { + // Validate print command for injection prevention + if strings.ContainsAny(*cfg.LabelMaker.PrintCommand, ";&|") { + return fmt.Errorf("invalid characters in print command") + } + tmpFile := filepath.Join(os.TempDir(), fmt.Sprintf("label-%d.png", time.Now().UnixNano())) f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return err } defer func() { _ = f.Close() - if err := os.Remove(f.Name()); err != nil { + // Ensure file is removed even if command execution fails + _ = os.Remove(f.Name()) + }() // ... rest of the function command := exec.Command(commandParts[0], commandParts[1:]...) + // Add timeout + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + command = command.WithContext(ctx) _, err = command.CombinedOutput() if err != nil { return err }Also, consider using a more secure temporary file creation method:
tmpFile, err := os.CreateTemp("", "label-*.png") if err != nil { return err }
42-56: 🛠️ Refactor suggestion
Enhance parameter validation for better security and reliability.
The validation function should include additional checks for:
- URL field (non-empty and valid format)
- Font sizes (positive values)
- DPI (reasonable range, e.g., 72-300)
func (p *GenerateParameters) Validate() error { if p.Width <= 0 { return fmt.Errorf("invalid width") } if p.Height <= 0 { return fmt.Errorf("invalid height") } if p.Margin < 0 { return fmt.Errorf("invalid margin") } if p.ComponentPadding < 0 { return fmt.Errorf("invalid component padding") } + if p.URL == "" { + return fmt.Errorf("URL cannot be empty") + } + if p.TitleFontSize <= 0 || p.DescriptionFontSize <= 0 { + return fmt.Errorf("font sizes must be positive") + } + if p.Dpi < 72 || p.Dpi > 300 { + return fmt.Errorf("DPI must be between 72 and 300") + } return nil }📝 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.func (p *GenerateParameters) Validate() error { if p.Width <= 0 { return fmt.Errorf("invalid width") } if p.Height <= 0 { return fmt.Errorf("invalid height") } if p.Margin < 0 { return fmt.Errorf("invalid margin") } if p.ComponentPadding < 0 { return fmt.Errorf("invalid component padding") } if p.URL == "" { return fmt.Errorf("URL cannot be empty") } if p.TitleFontSize <= 0 || p.DescriptionFontSize <= 0 { return fmt.Errorf("font sizes must be positive") } if p.Dpi < 72 || p.Dpi > 300 { return fmt.Errorf("DPI must be between 72 and 300") } return nil }
83-132:
⚠️ Potential issueAdd safeguards against potential DoS attacks.
The text wrapping function should include limits to prevent excessive memory usage from malicious input:
- Maximum number of lines
- Maximum total text length
func wrapText(text string, face font.Face, maxWidth int, ctx *freetype.Context) []string { + const maxLines = 100 + const maxTextLength = 10000 + + if len(text) > maxTextLength { + text = text[:maxTextLength] + } + lines := strings.Split(text, "\n") var wrappedLines []string for _, line := range lines { + if len(wrappedLines) >= maxLines { + break + } words := strings.Fields(line) // ... rest of the function📝 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.// wrapText breaks text into lines that fit within maxWidth func wrapText(text string, face font.Face, maxWidth int, ctx *freetype.Context) []string { const maxLines = 100 const maxTextLength = 10000 if len(text) > maxTextLength { text = text[:maxTextLength] } lines := strings.Split(text, "\n") var wrappedLines []string for _, line := range lines { if len(wrappedLines) >= maxLines { break } words := strings.Fields(line) if len(words) == 0 { wrappedLines = append(wrappedLines, "") continue } currentLine := words[0] for _, word := range words[1:] { testLine := currentLine + " " + word width := measureString(testLine, face, ctx) if width <= maxWidth { currentLine = testLine } else { wrappedLines = append(wrappedLines, currentLine) currentLine = word } } wrappedLines = append(wrappedLines, currentLine) } // Handle lines that are too long and have no spaces for i, line := range wrappedLines { width := measureString(line, face, ctx) if width > maxWidth { var splitLines []string currentLine := "" for _, r := range line { testLine := currentLine + string(r) width := measureString(testLine, face, ctx) if width <= maxWidth { currentLine = testLine } else { splitLines = append(splitLines, currentLine) currentLine = string(r) } } splitLines = append(splitLines, currentLine) wrappedLines = append(wrappedLines[:i], append(splitLines, wrappedLines[i+1:]...)...) } } return wrappedLines }
Co-authored-by: fidoriel <[email protected]>
jake-walker
force-pushed
the
label-generation
branch
from
55e0154 to
00bccd4
Compare
Awesome, glad it worked for you. I've added in the ability to have 0 margins around the edge of the label (by setting argument I think saving to a jpeg was part of the example code I was using, but I've switched it over to png, which makes more sense as you say |
|
Thank you so much. But for some reason in the |
|
Sorted, looks like I just missed that when rebasing onto the main branch earlier |
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: 5
🔭 Outside diff range comments (1)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
250-268: Prevent command injection in print command template.The current implementation could be vulnerable to command injection through maliciously crafted filenames.
Apply this diff to enhance security:
+ // Validate filename contains only safe characters + if !regexp.MustCompile(`^[a-zA-Z0-9-_.]+$`).MatchString(filepath.Base(f.Name())) { + return fmt.Errorf("invalid filename") + } commandTemplate := template.Must(template.New("command").Parse(*cfg.LabelMaker.PrintCommand)) + // Use text/template with strict escaping + commandTemplate = template.Must(commandTemplate.Option("missingkey=error")) builder := &strings.Builder{} if err := commandTemplate.Execute(builder, map[string]string{ "FileName": f.Name(), }); err != nil { return err }
🧹 Nitpick comments (8)
frontend/pages/scanner.vue (1)
100-100: Add security attributes to video element.The video element should include security attributes to prevent potential misuse.
Apply this diff to add security attributes:
- <video ref="video" class="rounded-box shadow-lg" poster="data:image/gif,AAAA"></video> + <video ref="video" class="rounded-box shadow-lg" poster="data:image/gif,AAAA" + disablePictureInPicture + disableRemotePlayback></video>backend/app/api/static/docs/docs.go (1)
1042-1080: Enhance API documentation with security considerations.The label generation endpoints should include security notes and rate limiting information.
Add security notes to the endpoint descriptions:
"/v1/labelmaker/assets/{id}": { "get": { + "description": "Generate labels for assets. Rate limited to prevent abuse. Requires valid authentication token.", "security": [ { "Bearer": [] } ],Similar updates should be made to the item and location label endpoints.
Also applies to: 1081-1119, 1120-1158
backend/app/api/static/docs/swagger.yaml (1)
1411-1483: New Labelmaker Endpoints Documented
The swagger.yaml now includes three new GET endpoints for generating labels for assets, items, and locations with an optional "print" query parameter. One point to note: the endpoints declare a produced MIME type of "application/json" while the response description suggests an image/png output. It might be better aligned if the "produces" field is updated to "image/png" (or clearly document that a base64-encoded string is returned in JSON) to avoid confusion in API consumers. Additionally, ensure that all input parameters (especially the IDs) are rigorously validated and sanitized in the corresponding handler layer to prevent injection attacks or unauthorized access.
[security_recommendation]docs/docs/api/openapi-2.0.json (1)
1035-1151: New Labelmaker Endpoints in OpenAPI Specification
The newly added endpoints for/v1/labelmaker/assets/{id},/v1/labelmaker/item/{id}, and/v1/labelmaker/location/{id}are now specified. Similar to the Swagger YAML, the "produces" field is currently set to "application/json" while returning a response described as "image/png" with a string schema. Please ensure that either the MIME type is corrected or the returned string is clearly documented (e.g., base64 encoded image). Moreover, it is vital to enforce strong input validation and proper authentication checks on these endpoints to mitigate any potential security risks related to URL manipulation or parameter injection.
[security_recommendation]backend/app/api/static/docs/swagger.json (4)
1035-1073: Endpoint /v1/labelmaker/assets/{id}: Content-Type and Input Validation
This new endpoint is well secured with Bearer authentication and correctly defines the required parameters for asset ID and an optional "print" flag. However, note that the "produces" field is set to "application/json" while the response description and schema indicate that a PNG image is returned. For clarity and consistency in API documentation (and to help API clients correctly set their Accept headers), it is advisable to change the "produces" value to "image/png."
Additionally, ensure that the "print" parameter is robustly validated to prevent potential injection risks when incorporating it into any command execution paths.
1074-1112: Endpoint /v1/labelmaker/item/{id}: Documentation Consistency & Security Check
This endpoint follows a similar pattern to the previous asset label endpoint. While the security configuration and parameter definitions are solid, the same content-type mismatch is apparent: the "produces" field is "application/json" whereas the 200 response is described as "image/png" returning a string. Updating "produces" to "image/png" will improve clarity.
Also, verify that any downstream handling of the "print" flag properly sanitizes and validates the input to mitigate risks associated with executing print commands remotely.
1113-1151: Endpoint /v1/labelmaker/location/{id}: Update Content-Type and Secure Input Handling
The definition for generating location labels is clearly structured and secured. However, as with the other labelmaker endpoints, the "produces" field is currently set to "application/json" despite the endpoint returning a PNG image (as indicated by the response description). Changing this to "image/png" will align the documentation with the intended output.
Additionally, ensure that the "id" parameter for locations and the "print" query parameter are appropriately validated and sanitized to prevent any potential misuse or injection vulnerabilities when these inputs interact with remote printing operations.
3106-3108: API Summary Extension: 'labelPrinting' Property Addition
The addition of the "labelPrinting" boolean property to the APISummary definition is a clear and concise way to indicate the availability of label printing functionality. This enhancement will help clients determine at a glance if the label printing feature is enabled. For extra clarity, consider adding a descriptive comment or schema property that details its purpose and any security considerations (for example, ensuring that enabling label printing does not inadvertently expose internal configuration details).
🛑 Comments failed to post (5)
frontend/pages/scanner.vue (2)
21-24: 🛠️ Refactor suggestion
Enhance error handling to prevent information leakage.
The error handler logs raw errors to the console which could expose sensitive implementation details.
Apply this diff to improve error handling:
- console.error("Scanner error:", error); + console.error("Scanner error occurred"); errorMessage.value = t("scanner.error");📝 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.const handleError = (error: unknown) => { - console.error("Scanner error:", error); + console.error("Scanner error occurred"); errorMessage.value = t("scanner.error"); };
57-62:
⚠️ Potential issueStrengthen URL validation for enhanced security.
The current URL sanitization is basic and could allow malicious URLs. Consider implementing stricter validation.
Apply this diff to enhance URL validation:
- const url = new URL(result.getText()); - if (!url.pathname.startsWith("/")) { - throw new Error(t("scanner.invalid_url")); - } - const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ""); + const url = new URL(result.getText()); + // Validate URL is from the same origin + if (url.origin !== window.location.origin) { + throw new Error(t("scanner.invalid_url")); + } + // Ensure path starts with / and sanitize + if (!url.pathname.startsWith("/")) { + throw new Error(t("scanner.invalid_url")); + } + const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ""); + // Validate final path format + if (!/^\/[a-zA-Z0-9-_/]*$/.test(sanitizedPath)) { + throw new Error(t("scanner.invalid_url")); + }📝 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.const url = new URL(result.getText()); // Validate URL is from the same origin if (url.origin !== window.location.origin) { throw new Error(t("scanner.invalid_url")); } // Ensure path starts with / and sanitize if (!url.pathname.startsWith("/")) { throw new Error(t("scanner.invalid_url")); } const sanitizedPath = url.pathname.replace(/[^a-zA-Z0-9-_/]/g, ""); // Validate final path format if (!/^\/[a-zA-Z0-9-_/]*$/.test(sanitizedPath)) { throw new Error(t("scanner.invalid_url")); } navigateTo(sanitizedPath);backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
18-34: 🛠️ Refactor suggestion
Add rate limiting to prevent abuse of label generation.
The label generation endpoint could be abused to consume server resources. Consider implementing rate limiting.
Consider using a rate limiting middleware or implementing token bucket algorithm:
func rateLimitMiddleware(next http.Handler) http.Handler { limiter := rate.NewLimiter(rate.Every(1*time.Second), 10) // 10 requests per second return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !limiter.Allow() { http.Error(w, "Too many requests", http.StatusTooManyRequests) return } next.ServeHTTP(w, r) }) }backend/pkgs/labelmaker/labelmaker.go (2)
134-137: 🛠️ Refactor suggestion
Add input validation to prevent memory exhaustion.
Large input strings could cause excessive memory usage during image generation.
Add input validation:
func GenerateLabel(w io.Writer, params *GenerateParameters) error { if err := params.Validate(); err != nil { return err } + // Add input length validation + const maxTextLength = 1000 + if len(params.TitleText) > maxTextLength || len(params.DescriptionText) > maxTextLength { + return fmt.Errorf("text length exceeds maximum allowed") + }
229-239: 🛠️ Refactor suggestion
Improve temporary file security.
The temporary file handling could be enhanced for better security.
Apply this diff to improve temporary file security:
- tmpFile := filepath.Join(os.TempDir(), fmt.Sprintf("label-%d.png", time.Now().UnixNano())) - f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) + // Use secure random number for filename + randomBytes := make([]byte, 32) + if _, err := rand.Read(randomBytes); err != nil { + return fmt.Errorf("failed to generate random filename: %v", err) + } + filename := fmt.Sprintf("label-%x.png", randomBytes) + tmpFile := filepath.Join(os.TempDir(), filename) + // Create file with restricted permissions + f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600)📝 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.// Use secure random number for filename randomBytes := make([]byte, 32) if _, err := rand.Read(randomBytes); err != nil { return fmt.Errorf("failed to generate random filename: %v", err) } filename := fmt.Sprintf("label-%x.png", randomBytes) tmpFile := filepath.Join(os.TempDir(), filename) // Create file with restricted permissions f, err := os.OpenFile(tmpFile, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) if err != nil { return err } defer func() { _ = f.Close() if err := os.Remove(f.Name()); err != nil { log.Printf("failed to remove temporary label file: %v", err) } }()
Thank you. The settings now allow configuring everything printer location, label settings and so on. What about the optinal ability to set a branding string on every label. Something I can freely add once and then printed on each label. For example, Name and Phone? But that might also be out of scope and added in a later pr. |
This PR adds a label generator which can generate labels for items (either by ID or asset ID) and locations. In the config, a command can be set for printing labels out, the idea is that this would be you can pass the generated filename to a command that will print out the label. Alternatively, users can simply download the label through the UI to print another way.
This is just a draft for now as I'd like some feedback and still need to make this work nicely with the existing label generation "report" page.
Print confirmation modal
Location label
Item label
Asset label
What type of PR is this?
What this PR does / why we need it:
There are a bunch of people who want to be able to add labels to their physical Homebox locations, myself included, so this adds that functionality.
Which issue(s) this PR fixes:
Resolves #9 (now part of #436)
Special notes for your reviewer:
This is just a draft for now, I'm planning to give this a tidy up, and make sure it integrates nicely with the existing label generation page. In addition, I'd like to know any other issues I could potentially include (e.g. #495) or any feedback at all on this feature.
Testing
This has just been manually tested, but I can add tests if needed.
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive pull request summary, here are the updated release notes:
New Features
Configuration
User Interface
API Enhancements
/labelmakerendpoints for generating labels.These changes expand Homebox's labeling capabilities, providing users with more flexible options for generating and managing item, location, and asset labels.