Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add label generation api #498

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jake-walker
Copy link

@jake-walker jake-walker commented Jan 28, 2025

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.

Screenshot of new label dropdown on an item

Print confirmation modal
Screenshot of print label confirmation

Location label
Location label example

Item label
Item label example

Asset label
Asset label example

What type of PR is this?

  • feature

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.

  • A label button has been added to item & location pages
  • Generated label width, height, font size and padding can be configured in the application config
  • A command can be set in the config to print labels remotely from the UI

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

Based on the comprehensive pull request summary, here are the updated release notes:

  • New Features

    • Added label generation and printing functionality for items, locations, and assets.
    • Introduced new API endpoints for retrieving and printing labels.
    • Implemented a new Vue component for label management with print and download options.
  • Configuration

    • Added new configuration options for label dimensions, padding, font size, and print commands.
  • User Interface

    • Integrated label maker component into item and location detail pages.
  • API Enhancements

    • Created new /labelmaker endpoints for generating labels.
    • Added support for generating labels with QR codes.
    • Introduced optional print functionality for labels.

These changes expand Homebox's labeling capabilities, providing users with more flexible options for generating and managing item, location, and asset labels.

@jake-walker jake-walker marked this pull request as draft January 28, 2025 20:44
Copy link
Contributor

coderabbitai bot commented Jan 28, 2025

Walkthrough

This 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

File Change Summary
backend/app/api/handlers/v1/controller.go Added config field to V1Controller, updated constructor and API summary
backend/app/api/handlers/v1/helpers.go Added URL handling functions GetHBURL and stripPathFromURL
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go New package with label generation handlers for locations, items, and assets
backend/app/api/routes.go Added new labelmaker routes for locations, items, and assets
backend/internal/sys/config/conf.go Added LabelMaker configuration options
backend/pkgs/labelmaker/labelmaker.go New package for label generation and printing
frontend/components/global/LabelMaker.vue New Vue component for label printing and downloading
frontend/pages/item/[id]/index.vue, frontend/pages/location/[id].vue Added LabelMaker component to item and location pages

Assessment against linked issues

Objective Addressed Explanation
Location QR Code Generation
QR Code for Locations, Bins, Cabinets

Possibly related PRs

Suggested Labels

⬆️ enhancement

Suggested Reviewers

  • tonyaellie
  • tankerkiller125

Security Recommendations

  1. Validate and sanitize all input parameters for label generation to prevent potential injection attacks.
  2. Implement rate limiting on label generation and printing endpoints to prevent abuse.
  3. Ensure print commands are securely configured and do not allow arbitrary command execution.
  4. Add logging for label generation and printing activities to track potential misuse.
  5. Implement proper access controls to restrict label generation to authorized users only.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7588c7 and 0d2e6db.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (17)
  • backend/app/api/handlers/v1/controller.go (4 hunks)
  • backend/app/api/handlers/v1/helpers.go (1 hunks)
  • backend/app/api/handlers/v1/v1_ctrl_items.go (1 hunks)
  • backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1 hunks)
  • backend/app/api/routes.go (2 hunks)
  • backend/app/api/static/docs/docs.go (1 hunks)
  • backend/app/api/static/docs/swagger.json (1 hunks)
  • backend/app/api/static/docs/swagger.yaml (1 hunks)
  • backend/go.mod (2 hunks)
  • backend/internal/sys/config/conf.go (2 hunks)
  • backend/pkgs/labelmaker/labelmaker.go (1 hunks)
  • docs/docs/api/openapi-2.0.json (1 hunks)
  • docs/en/configure-homebox.md (2 hunks)
  • frontend/components/global/LabelMaker.vue (1 hunks)
  • frontend/locales/en.json (1 hunks)
  • frontend/pages/item/[id]/index.vue (3 hunks)
  • frontend/pages/location/[id].vue (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
  • backend/go.mod
  • backend/internal/sys/config/conf.go
  • frontend/pages/item/[id]/index.vue
  • docs/docs/api/openapi-2.0.json
  • frontend/pages/location/[id].vue
  • backend/app/api/handlers/v1/helpers.go
  • backend/app/api/routes.go
  • frontend/components/global/LabelMaker.vue
  • docs/en/configure-homebox.md
  • frontend/locales/en.json
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/pkgs/labelmaker/labelmaker.go

58-58: unnecessary conversion

(unconvert)


145-145: unnecessary conversion

(unconvert)


159-159: unnecessary conversion

(unconvert)


1-1: ST1000: at least one file in a package should have a package comment

(stylecheck)


34-34: ST1003: struct field Url should be URL

(stylecheck)

🔇 Additional comments (8)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)

104-125: Validate asset ID rigorously.

The code strips dashes and attempts to parse the asset ID from a path parameter. If the path param is malformed, it returns an error, but consider verifying user privileges or adding a sanity check that the ID is within an expected numeric range. This reduces the risk of malicious param tampering.
[security_recommendation]

backend/app/api/handlers/v1/controller.go (3)

76-76: Inject config after thorough validation.

Injecting a *config.Config pointer can introduce new attack surfaces unless validated. Consider verifying that critical fields like PrintCommand are sanitized or whitelisted.
[security_recommendation]


97-97: Expose label printing capability in metadata.

Exposing the presence or absence of a print command can be helpful but also reveals internal configuration to consumers. If security is a concern, consider limiting this detail to authenticated or admin routes.
[security_recommendation]


101-107: Check for nil configuration.

We see that NewControllerV1 sets ctrl.config = config. If config might be nil, calls to ctrl.config.LabelMaker would panic. Add a safeguard to ensure config != nil.

backend/app/api/handlers/v1/v1_ctrl_items.go (1)

341-341: Verify URL handling security in GetHBURL.

The URL handling through GetHBURL could be vulnerable to Server-Side Request Forgery (SSRF) if not properly validated. Ensure that the URL is properly sanitized and validated before use.

✅ Verification successful

✅ URL handling in GetHBURL is properly secured

The implementation includes proper URL validation through url.Parse and safely handles malformed URLs by returning an empty string. The function also strips paths from URLs, reducing the risk of path traversal attacks. Since it's only used for generating return URLs and doesn't make direct network requests, the SSRF risk is minimal.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for URL validation in GetHBURL implementation
ast-grep --pattern 'func GetHBURL($_, $_) {
  $$$
}'

Length of output: 54


Script:

#!/bin/bash
# Search for GetHBURL mentions with context
rg -A 10 "GetHBURL" --type go

# Also search for URL validation patterns
rg -A 5 "url\.Parse|ValidateURL|ParseRequestURI" --type go

Length of output: 4393

backend/app/api/static/docs/swagger.yaml (1)

1409-1432: Verify access controls for asset label generation.

While the endpoint is secured with Bearer authentication, consider implementing additional checks:

  1. Ensure users can only access labels for assets they have permission to view
  2. Consider rate limiting to prevent abuse
  3. Add proper input validation for the asset ID

Run this script to check for access control implementation:

backend/app/api/static/docs/swagger.json (1)

1035-1151: Content matches swagger.yaml specification.

The JSON specification correctly mirrors the YAML version.

backend/app/api/static/docs/docs.go (1)

1042-1158: Content matches swagger specification.

The Go template correctly defines the same API specification.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as print_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 LabelMaker field in Config should 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 issue

Sanitize 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 issue

Security 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 issue

Enhance security for file download implementation.

The current implementation of downloadLabel creates and manipulates DOM elements directly, which could pose security risks. Consider using the Content-Disposition header 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 issue

Security: 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 issue

Enhance security and error handling in generateOrPrint.

Several security and robustness improvements are needed:

  1. Missing content type headers
  2. No validation of configuration parameters
  3. 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, &params)
         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, &params)
     }
 
     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, &params)
        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, &params)
    }

    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 getHBURL to imported GetHBURL requires 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.go

Length 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:

  1. Warn about command injection risks
  2. 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 issue

Fix response content type inconsistency.

The endpoints are marked to produce "application/json" but the response is documented as "image/jpeg". Update the produces field 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:

  1. Rate limiting to prevent DoS attacks through excessive label generation
  2. Input validation for the ID parameter to prevent injection attacks
  3. Audit logging for label generation and printing activities
  4. 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 go

Length 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.go

Length 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.go

Length of output: 807

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: URL vs Url.

Go naming conventions suggest using URL (all caps) instead of Url for 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 LabelPrinting is 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 issue

Consider 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:

  1. Content-Security-Policy
  2. X-Content-Type-Options: nosniff
  3. X-Frame-Options: DENY
  4. 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:

  1. Ensure proper access control checks
  2. Validate input parameters
  3. 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 code

Length 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.go

Length 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:

  1. Implement proper access control checks
  2. Validate location ID input
  3. 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 code

Length 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.go

Length of output: 17560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Location QR Codes
1 participant