Add customizable label template system #1161
Conversation
This commit introduces a comprehensive label printing system with: Backend: - Label template CRUD with visual editor support (JSON-based canvas) - Printer management with network/local printer support - PDF rendering engine with barcode generation (Code128, QR, DataMatrix, EAN, UPC) - SSRF protection for printer addresses (configurable via HBOX_PRINTER_ALLOW_PUBLIC_ADDRESSES) - Template presets for common label sizes (Dymo, Brother, Avery) - Batch printing support for items and locations Frontend: - Visual label template editor with drag-and-drop canvas - Real-time preview with item/location data binding - Printer configuration UI with WebUSB support for direct printing - Print dialogs for items and locations with template selection - Template library with duplicate/export functionality Database: - New label_templates table with canvas JSON storage - New printers table with address and default flag - Migrations for both SQLite and PostgreSQL Documentation: - User guide for label template creation - Label editor features reference - Configuration options for printer security Security improvements: - Private IP validation for printer addresses (RFC 1918 ranges) - Cloud metadata endpoint blocking (169.254.169.254) Note: This branch was rebased from 'labels' to remove accidentally committed files and provide a clean history.
WalkthroughAdds end-to-end label templates and direct-printer support: new APIs, repositories, DB migrations, labelmaker (barcode/presets/PDF renderer), printer workflows (status/test/print), route/config entries, path-normalization test, and DTO/manifest updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as v1 Handler
participant Repo as Repo (LabelTemplates / Printers)
participant DB as Database
participant Renderer as LabelRenderer
participant PrinterClient as Printer Client
participant Printer as Physical Printer
Client->>API: POST /v1/templates/render (templateID, items/locations)
API->>Repo: GetOne(templateID)
Repo->>DB: SELECT label_templates ...
DB-->>Repo: Template record
Repo-->>API: LabelTemplateOut
API->>API: fetch items/locations data, build render models
API->>Renderer: RenderToPDF/RenderToSheets(template, data, opts)
Renderer-->>API: PDF/PNG bytes
API-->>Client: 200 application/pdf
alt Print flow (printer chosen)
Client->>API: POST /v1/templates/print (printerID, copies)
API->>Repo: Printers.GetOne(printerID)
Repo->>DB: SELECT printers ...
Repo-->>API: PrinterOut
API->>API: validate address (PrinterConf.AllowPublicAddresses)
API->>Renderer: Render images/URF for print
API->>PrinterClient: Submit print job(s)
PrinterClient->>Printer: IPP/URF job
Printer-->>PrinterClient: job id/status
PrinterClient-->>API: result
API-->>Client: PrintResponse (success, job IDs)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Security Recommendations
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
requested review from
katosdev,
tankerkiller125 and
tonyaellie
coderabbitai
bot
added
go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
backend/internal/data/repo/repo_item_templates.go (4)
56-57: Consider semantic clarity for nullable UUID in API contract.The combination of
uuid.UUID(non-pointer) withomitemptyandextensions:"x-nullable"creates an ambiguity:uuid.Nilwill be omitted from JSON serialization, but the OpenAPI extension suggests the field can benull. API consumers may expect explicitnullrather than field absence.If the intent is to allow clients to send
nullto clear a location, consider using*uuid.UUIDfor the API layer or documenting this behavior clearly.
180-194: Silent error handling could return incomplete label data.When the label query fails (line 186), the error is silently swallowed and an empty labels slice is returned. Callers won't know if labels are genuinely empty or if a database error occurred.
Consider logging the error or propagating it to help diagnose issues:
🔎 Proposed fix
labels := make([]TemplateLabelSummary, 0) if len(template.DefaultLabelIds) > 0 { labelEntities, err := r.db.Label.Query(). Where(label.IDIn(template.DefaultLabelIds...)). All(ctx) - if err == nil { + if err != nil { + log.Warn().Err(err).Msg("failed to fetch default labels for template") + } else { for _, l := range labelEntities { labels = append(labels, TemplateLabelSummary{ ID: l.ID, Name: l.Name, }) } } }
263-314: Lack of transactional boundaries could leave orphaned templates on field creation failure.The template is created first (line 292), then fields are created in a loop (lines 298-310). If any field creation fails, the template exists without all its fields, leaving the database in an inconsistent state.
Consider wrapping the entire operation in a transaction:
🔎 Proposed approach
// Use a transaction for atomicity tx, err := r.db.Tx(ctx) if err != nil { return ItemTemplateOut{}, err } defer func() { if err != nil { _ = tx.Rollback() } }() // Create template and fields within transaction // ... if err = tx.Commit(); err != nil { return ItemTemplateOut{}, err }
419-427: Field deletion errors are logged but not propagated.When deleting orphaned fields fails (line 422-425), the error is only logged, allowing the operation to complete successfully despite partial failure. This could leave stale fields in the database.
Consider either:
- Returning the error to fail the entire update, or
- Accumulating errors and returning them at the end
🔎 Proposed fix (fail-fast approach)
for _, field := range existingFields { if !updatedFieldIDs[field.ID] { err = r.db.TemplateField.DeleteOne(field).Exec(ctx) if err != nil { - log.Err(err).Msg("failed to delete template field") + log.Err(err).Msg("failed to delete template field") + return ItemTemplateOut{}, err } } }backend/internal/data/repo/repo_locations.go (1)
184-227: SQL injection protection verified; consider error handling consistency.The method correctly uses parameterized queries with
$1and$2placeholders, protecting against SQL injection. Authorization is properly enforced by filtering ongroup_locations = $1.However, the error handling pattern differs from the existing
GetOneByGroupmethod (line 180-182). WhileGetOneByGroupuses the ORM (which typically returns typed errors that can be checked with helpers likeent.IsNotFound), this method returns a plainfmt.Errorfon Line 217. This inconsistency may require callers to use different error-checking approaches for similar "not found" scenarios.💡 Optional: Align error types with existing patterns
Consider returning a typed error that matches the ORM's not-found error pattern, or document the difference so callers know to check error strings rather than types:
err := row.Scan(&ct.ID, &ct.Name, &ct.Description, &ct.CreatedAt, &ct.UpdatedAt, &maybeCount) if err != nil { if errors.Is(err, sql.ErrNoRows) { - return LocationOutCount{}, fmt.Errorf("location not found: %s", id.String()) + return LocationOutCount{}, ent.NewNotFoundError("location", id.String()) } return LocationOutCount{}, err }Alternatively, if
ent.NewNotFoundErrorisn't exported, you could define a sentinel error in this package or simply document the error format for callers.backend/pkgs/labelmaker/presets.go (1)
483-490: Consider refactoring to avoid taking address of loop variable.Line 486 returns
&presetwherepresetis the loop variable. While this is safe because you return immediately (before the next iteration), it's a common Go gotcha and may confuse readers.🔎 Recommended refactoring to use index-based access
func GetPresetByKey(key string) *LabelPreset { - for _, preset := range LabelPresets { - if preset.Key == key { - return &preset + for i := range LabelPresets { + if LabelPresets[i].Key == key { + return &LabelPresets[i] } } return nil }This makes it explicit that you're returning a pointer to the element in the slice, not to a loop-local copy.
backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
223-234: Security: Attachment token exposure is intentional but requires threat model documentation.The attachment token cookie is set with
HttpOnly: falseto allow JavaScript access for query parameter attachment, which is necessary for serving attachment endpoints. This is a reasoned design choice with limited scope—the token grants onlyRoleAttachmentsaccess (QR codes and item attachments), not full user privileges.Security recommendations:
- Add clear code comment documenting the threat model assumption (e.g., "This limited-scope token accepts XSS risk to enable client-side attachment serving")
- Consider implementing a Content Security Policy (CSP) to mitigate XSS vectors
- Ensure token rotation/expiration is properly enforced (currently set to session expiry)
- Document why alternative approaches (e.g., server-side attachment proxying with HttpOnly-only tokens) were not chosen
backend/internal/data/repo/repo_label_templates.go (1)
28-28: Consider canvas_data size limits.The
canvas_datafield stores arbitrary JSON from the Fabric.js editor with no size validation at the repository layer. Extremely large or deeply nested JSON could cause issues.Consider adding a validation constraint or documenting the expected size limits, especially if this is user-generated content. You could add a custom validator or database constraint to limit the JSON size.
backend/app/api/handlers/v1/v1_ctrl_printers.go (2)
242-246: Inconsistent HTTP status codes on errors.Connection failures return HTTP 200 with
success: falserather than an error status code (4xx/5xx). This makes it harder for clients to distinguish between successful responses and failures.Consider returning appropriate error status codes for failures, or document why 200 is preferred for this endpoint. The pattern is consistent (also line 260-263), but unusual.
180-180: Status update errors silently ignored.Failed status updates are discarded without logging. If status persistence fails repeatedly, debugging becomes difficult.
Consider logging the error:
-_ = ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline") +if err := ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline"); err != nil { + // Log error but don't fail the request + log.Warn().Err(err).Msg("failed to update printer status") +}Same for line 189.
backend/pkgs/labelmaker/pdf_renderer.go (2)
122-144: Consider logging render failures for visibility.The rendering errors on lines 127 and 133 are silently ignored with
continue, which means users won't know if some labels failed to render. This could lead to confusion when the output has fewer labels than expected.🔎 Proposed enhancement to log render failures
// Render label to image imgData, err := r.RenderTemplate(RenderContext{ Item: item, Template: template, }) if err != nil { + log.Warn().Err(err).Str("itemIndex", fmt.Sprintf("%d", i)).Msg("failed to render label in PDF") continue } // Decode PNG img, err := png.Decode(bytes.NewReader(imgData)) if err != nil { + log.Warn().Err(err).Str("itemIndex", fmt.Sprintf("%d", i)).Msg("failed to decode label image") continue }Note: You'll need to import
"github.com/rs/zerolog/log"at the top of the file.Also, line 143 has an unused variable assignment
_ = imgthat can be removed entirely.
202-225: Apply consistent error logging for location label rendering.Similar to the item rendering above, location label render failures at lines 207 and 213 are silently skipped. Consider adding logging here as well for consistency and debugging purposes. Line 224 also has an unnecessary
_ = imgassignment.backend/internal/data/repo/repo_item_attachments.go (1)
113-129: Minor optimization: Remove redundant empty string check.The logic is correct, but there's a redundant check for empty
normalizedPrefixat lines 119 and 124. After normalizing the prefix, you can combine these checks.🔎 Proposed refactor to eliminate redundancy
func (r *AttachmentRepo) fullPath(relativePath string) string { // Normalize path separators to forward slashes for blob storage // The blob library expects forward slashes in keys regardless of OS normalizedRelativePath := normalizePath(relativePath) // Always use forward slashes when joining paths for blob storage - if r.storage.PrefixPath == "" { - return normalizedRelativePath - } normalizedPrefix := normalizePath(r.storage.PrefixPath) if normalizedPrefix == "" { return normalizedRelativePath } return fmt.Sprintf("%s/%s", normalizedPrefix, normalizedRelativePath) }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (52)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.yamlis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/ent.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/has_id.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/hook/hook.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate/labeltemplate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/labeltemplate_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/predicate/predicate.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer/printer.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/printer_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/label_template.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/printer.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/tx.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**docs/en/api/openapi-3.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-3.0.yamlis excluded by!docs/en/api/**docs/en/api/swagger-2.0.jsonis excluded by!docs/en/api/**docs/en/api/swagger-2.0.yamlis excluded by!docs/en/api/**frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (94)
backend/app/api/handlers/v1/v1_ctrl_auth.gobackend/app/api/handlers/v1/v1_ctrl_label_templates.gobackend/app/api/handlers/v1/v1_ctrl_printers.gobackend/app/api/handlers/v1/v1_ctrl_user.gobackend/app/api/routes.gobackend/go.modbackend/internal/data/migrations/migrations.gobackend/internal/data/migrations/postgres/20251221140000_add_label_templates.sqlbackend/internal/data/migrations/postgres/20251222140000_add_printers.sqlbackend/internal/data/migrations/postgres/20251222160000_add_label_template_media_type.sqlbackend/internal/data/migrations/sqlite3/20251221140000_add_label_templates.sqlbackend/internal/data/migrations/sqlite3/20251222140000_add_printers.sqlbackend/internal/data/migrations/sqlite3/20251222160000_add_label_template_media_type.sqlbackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gobackend/internal/data/repo/repo_item_templates.gobackend/internal/data/repo/repo_item_templates_test.gobackend/internal/data/repo/repo_items_search_test.gobackend/internal/data/repo/repo_items_test.gobackend/internal/data/repo/repo_label_templates.gobackend/internal/data/repo/repo_locations.gobackend/internal/data/repo/repo_printers.gobackend/internal/data/repo/repos_all.gobackend/internal/sys/config/conf.gobackend/pkgs/labelmaker/barcode_generator.gobackend/pkgs/labelmaker/pdf_renderer.gobackend/pkgs/labelmaker/presets.gobackend/pkgs/labelmaker/template_renderer.gobackend/pkgs/printer/address_validation.gobackend/pkgs/printer/address_validation_test.gobackend/pkgs/printer/brother_media.gobackend/pkgs/printer/brother_raster.gobackend/pkgs/printer/cups_client.gobackend/pkgs/printer/ipp_client.gobackend/pkgs/printer/printer.gobackend/pkgs/printer/urf.gobackend/pkgs/printer/urf_test.gobackend/pkgs/textutils/normalize.godocs/.vitepress/menus/en.mtsdocs/en/advanced/external-label-service.mddocs/en/advanced/import-csv.mddocs/en/configure/index.mddocs/en/configure/oidc.mddocs/en/label-editor-features.mddocs/en/user-guide/label-templates.mdfrontend/components/Item/CreateModal.vuefrontend/components/Item/Selector.vuefrontend/components/Item/View/Selectable.vuefrontend/components/Item/View/table/data-table-dropdown.vuefrontend/components/Label/CreateModal.vuefrontend/components/LabelTemplate/Card.vuefrontend/components/LabelTemplate/CreateModal.vuefrontend/components/LabelTemplate/Editor/Canvas.vuefrontend/components/LabelTemplate/Editor/LayerPanel.vuefrontend/components/LabelTemplate/Editor/PreviewPanel.vuefrontend/components/LabelTemplate/Editor/Ruler.vuefrontend/components/LabelTemplate/Editor/TextToolbar.vuefrontend/components/LabelTemplate/Editor/Toolbar.vuefrontend/components/LabelTemplate/Editor/TransformPanel.vuefrontend/components/LabelTemplate/PrintDialog.vuefrontend/components/Location/CreateModal.vuefrontend/components/Location/Selector.vuefrontend/components/Location/Tree/Node.vuefrontend/components/Location/Tree/Root.vuefrontend/components/Template/Card.vuefrontend/components/Template/CreateModal.vuefrontend/components/Template/Selector.vuefrontend/components/ui/dialog-provider/utils.tsfrontend/composables/use-label-templates.tsfrontend/composables/use-local-printers.tsfrontend/composables/use-preferences.tsfrontend/composables/use-printers.tsfrontend/global.d.tsfrontend/layouts/default.vuefrontend/lib/api/base/base-api.tsfrontend/lib/api/classes/label-templates.tsfrontend/lib/api/classes/printers.tsfrontend/lib/api/types/data-contracts.tsfrontend/lib/api/user.tsfrontend/lib/printing/index.tsfrontend/lib/printing/protocols/brother-raster.tsfrontend/lib/printing/types.tsfrontend/lib/printing/webbluetooth-printer.tsfrontend/lib/printing/webusb-printer.tsfrontend/locales/en.jsonfrontend/package.jsonfrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/label-templates/[id]/edit.vuefrontend/pages/label-templates/batch.vuefrontend/pages/label-templates/index.vuefrontend/pages/location/[id].vuefrontend/pages/printers.vuefrontend/pages/template/[id].vue
💤 Files with no reviewable changes (2)
- backend/internal/data/repo/repo_items_test.go
- backend/internal/data/migrations/migrations.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-06T18:51:05.524Z
Learnt from: mcarbonne
Repo: sysadminsmedia/homebox PR: 869
File: backend/internal/data/migrations/sqlite3/20250706190000_fix_old_timestamps.sql:0-0
Timestamp: 2025-07-06T18:51:05.524Z
Learning: Goose database migrations are automatically wrapped in transactions by the migration framework, eliminating the need for explicit BEGIN TRANSACTION and COMMIT statements in migration scripts.
Applied to files:
backend/internal/data/migrations/sqlite3/20251222160000_add_label_template_media_type.sql
📚 Learning: 2025-07-12T19:31:54.055Z
Learnt from: tankerkiller125
Repo: sysadminsmedia/homebox PR: 897
File: backend/app/api/setup.go:25-25
Timestamp: 2025-07-12T19:31:54.055Z
Learning: In backend/app/api/setup.go, the manual conversion of backslashes to forward slashes using strings.ReplaceAll(storageDir, "\\", "/") is intentional because the underlying storage library only accepts forward slashes for file paths and performs its own path conversion internally.
Applied to files:
backend/internal/data/repo/repo_item_attachments_test.gobackend/internal/data/repo/repo_item_attachments.go
🧬 Code graph analysis (6)
backend/internal/data/repo/repo_locations.go (2)
backend/internal/core/services/contexts.go (1)
Context(19-30)frontend/lib/api/types/data-contracts.ts (1)
LocationOutCount(1147-1154)
backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
backend/app/api/handlers/v1/controller.go (1)
V1Controller(68-79)
backend/internal/data/repo/repos_all.go (2)
backend/internal/data/repo/repo_label_templates.go (1)
LabelTemplatesRepository(15-18)backend/internal/data/repo/repo_printers.go (1)
PrintersRepository(15-18)
backend/internal/data/repo/repo_label_templates.go (2)
backend/internal/core/services/reporting/eventbus/eventbus.go (3)
EventBus(28-34)EventLabelMutation(14-14)GroupMutationEvent(19-21)backend/internal/core/services/contexts.go (1)
Context(19-30)
backend/internal/data/repo/repo_item_attachments_test.go (2)
backend/internal/data/repo/repo_item_attachments.go (1)
AttachmentRepo(53-58)backend/internal/sys/config/conf_database.go (1)
Storage(7-11)
backend/pkgs/labelmaker/pdf_renderer.go (2)
backend/pkgs/labelmaker/template_renderer.go (5)
TemplateRenderer(100-103)TemplateData(54-62)ItemData(24-40)RenderContext(65-69)LocationData(43-51)backend/pkgs/labelmaker/presets.go (1)
SheetLayout(21-30)
🔇 Additional comments (30)
backend/internal/data/repo/repo_items_search_test.go (1)
167-167: Good refactor to use the more idiomatic assertion.Replacing
assert.NotEqual(t, "", normalizedSearch, ...)withassert.NotEmptyis a best-practice improvement. TheNotEmptyassertion is clearer in intent, more concise, and the canonical way to verify non-empty strings in testify.backend/internal/data/repo/repo_item_templates_test.go (2)
238-264: LGTM! Test correctly updated for value-type DefaultLocationID.The test properly assigns
loc.IDdirectly instead of using a pointer, aligning with the updatedItemTemplateCreatestruct signature whereDefaultLocationIDchanged from*uuid.UUIDtouuid.UUID.
300-337: LGTM! Test correctly usesuuid.Nilto indicate location removal.The update test properly uses
uuid.Nil(line 326) instead ofnilto signal that the location should be cleared. This aligns with the new semantics where the zero-value UUID represents "no location" rather than a nil pointer.backend/internal/data/repo/repo_item_templates.go (4)
283-286: LGTM! Proper uuid.Nil check for location assignment.The logic correctly sets the location only when a non-nil UUID is provided, aligning with the new value-type semantics.
317-328: Good security practice: Group ownership verification before mutation.The template ownership is properly verified against the group ID before allowing updates. This prevents unauthorized cross-group access.
347-352: LGTM! Location update/clear logic is correct.The conditional properly sets the location when a valid UUID is provided and clears it when
uuid.Nilis passed, matching the documented behavior.
433-455: Good security practice: Group ownership verification on delete.The delete operation properly verifies template ownership before proceeding, preventing unauthorized deletions across groups.
backend/internal/data/repo/repo_locations.go (1)
5-7: LGTM! Imports are appropriate.The added imports (
database/sql,errors,fmt) are all necessary for the newGetOneByGroupWithCountmethod's error handling and are used correctly.backend/app/api/handlers/v1/v1_ctrl_user.go (1)
18-24: LGTM! Formatting improvements to documentation.The godoc comment formatting changes improve readability without altering any functionality.
backend/internal/sys/config/conf.go (1)
105-110: LGTM! Secure-by-default SSRF protection.The
AllowPublicAddressesflag defaults tofalse, providing SSRF mitigation out of the box. The documentation clearly explains the security rationale.backend/app/api/handlers/v1/v1_ctrl_auth.go (1)
190-235: Approve attachment token propagation logic with security caveat.The implementation correctly propagates the attachment token through login, refresh, and OIDC flows, and properly cleans it up on logout. The cookie handling is consistent across all authentication paths.
However, see the security concern raised separately regarding the non-HttpOnly cookie.
backend/pkgs/labelmaker/presets.go (1)
1-514: LGTM! Comprehensive label preset definitions.The preset definitions cover a wide range of label formats from major brands (Brother, Dymo, Avery, Brady, Zebra), and the helper functions provide convenient access patterns. The struct definitions are well-documented and appropriate for the use case.
backend/internal/data/migrations/postgres/20251222160000_add_label_template_media_type.sql (1)
1-6: LGTM! Clean PostgreSQL migration for media_type column.The migration properly adds the
media_typecolumn with an appropriate data type (VARCHAR(50)) and includes a safe rollback usingIF EXISTS.backend/internal/data/migrations/sqlite3/20251222160000_add_label_template_media_type.sql (1)
1-10: LGTM! Pragmatic approach to SQLite limitations.The migration correctly adds the
media_typecolumn, and the no-op down migration is well-documented with a clear explanation of the SQLite limitation and its implications (schema drift, nullable column remaining).This is a reasonable approach given that older SQLite versions don't support
DROP COLUMN.Based on learnings, Goose automatically wraps migrations in transactions, so explicit transaction management is not needed.
backend/internal/data/migrations/sqlite3/20251221140000_add_label_templates.sql (1)
1-40: LGTM! Well-structured label_templates table with proper indexes and constraints.The migration creates a comprehensive table with:
- Appropriate data types and sensible defaults
- Foreign key constraints with cascade deletes for data integrity
- Indexes on key searchable/filterable columns
- Clean rollback that drops resources in reverse order
backend/internal/data/migrations/postgres/20251221140000_add_label_templates.sql (1)
3-21: Well-designed schema with appropriate constraints.The table structure is solid with proper foreign keys, cascade deletes, and sensible defaults. The
canvas_dataJSONB field provides flexibility for storing Fabric.js canvas definitions, and the indexes cover common access patterns.backend/internal/data/repo/repos_all.go (1)
12-40: LGTM!The repository additions follow the existing pattern and are properly initialized with the database client and event bus.
backend/internal/data/migrations/postgres/20251222140000_add_printers.sql (1)
11-11: Verify single default printer enforcement.The
is_defaultboolean allows multiple printers to be marked as default within a group at the database level. The repository layer clears existing defaults before setting a new one (repo_printers.go lines 196-201, 246-251), but this isn't enforced by a database constraint.Consider whether a partial unique index would add defense-in-depth:
CREATE UNIQUE INDEX idx_printers_single_default ON printers (group_printers) WHERE is_default = true;This would prevent race conditions where concurrent requests could create multiple defaults.
backend/internal/data/migrations/sqlite3/20251222140000_add_printers.sql (1)
3-24: LGTM - consistent with PostgreSQL migration.The SQLite schema matches the PostgreSQL version with appropriate type mappings.
backend/internal/data/repo/repo_label_templates.go (2)
136-162: Access control properly enforced.The query correctly restricts templates to those owned by the user OR marked as shared within the group, providing appropriate multi-tenancy isolation.
219-266: Update enforces owner-only permissions.Good security practice: only the template owner can modify it, even if the template is shared with the group.
backend/pkgs/labelmaker/barcode_generator.go (1)
50-53: LGTM - empty content validation.Proper validation prevents generating invalid barcodes.
backend/app/api/handlers/v1/v1_ctrl_printers.go (1)
64-68: SSRF protection properly implemented.Address validation occurs before persisting printer configuration, preventing attackers from adding printers that target internal/private networks (unless explicitly allowed via config).
Based on the PR objectives, SSRF protection includes blocking private addresses and cloud metadata endpoints, with
HBOX_PRINTER_ALLOW_PUBLIC_ADDRESSESas an opt-in override.backend/internal/data/repo/repo_printers.go (1)
22-30: SSRF protection is already implemented.Address validation occurs correctly at the handler layer. Both
HandlePrintersCreate(line 66) andHandlePrintersUpdate(line 90) callprinter.ValidatePrinterAddress()before invoking repository methods. The validation function includes comprehensive SSRF protections: blocking cloud metadata endpoints (169.254.169.254, metadata.google.internal), enforcing private IP ranges when configured, and properly resolving hostnames. The security control is in place and functioning as intended.Likely an incorrect or invalid review comment.
backend/app/api/handlers/v1/v1_ctrl_label_templates.go (1)
533-537: Use proper HTTP status codes for error responses.Multiple locations return HTTP 200 OK with
Success: falsein the response body for error conditions. This violates HTTP semantics and prevents clients from using standard HTTP status codes for error handling.Examples:
- Lines 533-537: No printer configured → should be 404 Not Found or 400 Bad Request
- Lines 563-568: Failed to connect to printer → should be 502 Bad Gateway or 500 Internal Server Error
- Lines 578-583: Invalid media type → should be 400 Bad Request
- Lines 652-658: Print job failed → should be 500 Internal Server Error or 502 Bad Gateway
- Lines 665-670: No labels printed → should be 400 Bad Request
🔎 Recommended approach for error responses
Return appropriate HTTP status codes for different error scenarios:
if err != nil { - return server.JSON(w, http.StatusOK, LabelTemplatePrintResponse{ + return server.JSON(w, http.StatusNotFound, LabelTemplatePrintResponse{ Success: false, Message: "No printer found. Please configure a printer first.", }) }Similarly update other error cases:
- Connection failures → 502 Bad Gateway
- Invalid input → 400 Bad Request
- Internal errors → 500 Internal Server Error
This allows clients to use standard HTTP status code checking (
if response.status >= 400) rather than parsing response bodies.Also applies to: 563-568, 578-583, 652-670
⛔ Skipped due to learnings
Learnt from: crumbowl Repo: sysadminsmedia/homebox PR: 920 File: backend/app/api/handlers/v1/v1_ctrl_barcodes.go:32-42 Timestamp: 2025-08-24T21:12:46.584Z Learning: In the Homebox barcode API handler (backend/app/api/handlers/v1/v1_ctrl_barcodes.go), the user prefers to return HTTP 500 for all external barcode API call failures rather than distinguishing between different error types like network failures or authentication errors. Their reasoning is that from the application's perspective, these are internal API call failures, and returning codes like 401 or 503 would be misleading to clients about where the actual problem lies.backend/app/api/routes.go (1)
156-181: LGTM! Comprehensive API surface with proper authentication and SSRF protection.The new route groups for label templates and printers are well-structured and consistently follow existing patterns. All endpoints properly enforce authentication via user middleware. Handler implementations correctly extract the auth context and pass
auth.GIDthrough the repository layer for group-level authorization scoping.A security positive: The printer creation and update handlers (
HandlePrintersCreateandHandlePrintersUpdate) validate printer addresses against SSRF vulnerabilities usingprinter.ValidatePrinterAddress()with configurable public address restrictions—good defense-in-depth for preventing unintended network access.backend/internal/data/repo/repo_item_attachments.go (2)
100-105: LGTM – Path normalization is correctly implemented.The normalization logic correctly converts backslashes to forward slashes and trims leading/trailing slashes, ensuring consistent blob storage keys across platforms. This aligns with the learning that the underlying storage library expects forward slashes.
107-111: LGTM – Path construction uses consistent forward slashes.The path method correctly constructs attachment paths using forward slashes via
fmt.Sprintf, ensuring platform-independent storage keys.backend/internal/data/repo/repo_item_attachments_test.go (2)
13-13: LGTM – Config import is necessary for the new test.The import is required to access
config.Storage.PrefixPathin the path normalization test.
286-339: Excellent test coverage for path normalization!The test comprehensively validates:
- Forward slash enforcement in
path()output- Backslash-to-forward-slash conversion in
fullPath()- Various
PrefixPathconfigurations (empty, single slash, standard)- Legacy Windows path handling
This provides strong confidence in cross-platform path handling.
auzroz
changed the title
Resolved whitespace-only conflicts in: - repo_item_attachments.go - repo_item_attachments_test.go - repo_item_templates.go 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Re-validate printer addresses before creating printer clients to prevent SSRF attacks in case the database is compromised. While addresses are validated on create/update, re-validating before use provides defense-in-depth. Added validation to: - HandleLabelTemplatesPrint - HandleLabelTemplatesPrintLocations - HandlePrintersStatus - HandlePrintersTest Addresses CodeRabbit review feedback on PR sysadminsmedia#1161. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace disintegration/imaging (which has CVE-2023-36308 for TIFF parsing) with anthonynsimon/bild, a pure Go image processing library with no known CVEs. While the CVE didn't affect this codebase (we only used imaging.Rotate on programmatically-generated images, not user uploads), this change removes the vulnerable dependency entirely. Changes: - Replace imaging.Rotate with transform.Rotate from bild - Remove disintegration/imaging dependency - Add anthonynsimon/bild v0.14.0 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Migrate from archived github.com/go-pdf/fpdf to the actively maintained codeberg.org/go-pdf/fpdf (v0.11.1). The Codeberg repository has recent commits (Sept 2025) and is the new official home for the project after GitHub was archived. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
backend/pkgs/labelmaker/pdf_renderer.go (2)
143-143: Dead code:imgvariable is decoded but unused.The PNG is decoded into
img(line 131), but only used as_ = imgat line 143. If the image bounds aren't needed, remove the decode step; otherwise, consider using bounds for validation.🔎 Proposed fix to remove unused decode
- // Decode PNG - img, err := png.Decode(bytes.NewReader(imgData)) - if err != nil { - continue - } - // Register image with PDF imgName := fmt.Sprintf("label_%d", i) pdf.RegisterImageOptionsReader(imgName, fpdf.ImageOptions{ImageType: "PNG"}, bytes.NewReader(imgData)) // Place image on PDF pdf.ImageOptions(imgName, x, y, labelWidth, labelHeight, false, fpdf.ImageOptions{ImageType: "PNG"}, 0, "") - - _ = img
55-73: Landscape orientation swap logic is correct.The custom page dimensions are currently used only with preset sheet layouts (which have hardcoded values), so immediate validation isn't necessary. However, consider adding validation for positive dimensions if custom page size support is expanded via the API in the future, as a defensive measure.
backend/app/api/handlers/v1/v1_ctrl_printers.go (2)
188-189: Consider loggingUpdateStatuserrors instead of ignoring them.The
UpdateStatuscalls at lines 189 and 198 use_ =to discard errors. While not critical for the status response, logging these failures would aid debugging.🔎 Proposed fix
- _ = ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline") + if updateErr := ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline"); updateErr != nil { + log.Warn().Err(updateErr).Str("printerID", ID.String()).Msg("failed to update printer status") + }Also applies to: 198-198
344-364: Media types endpoint returns non-deterministic order.Iterating over
printer.BrotherMediaTypesmap produces unpredictable ordering. If stable API responses are desired, sort the results.🔎 Proposed fix
+import "sort" + func (ctrl *V1Controller) HandlePrintersMediaTypes() errchain.HandlerFunc { fn := func(r *http.Request) ([]BrotherMediaInfo, error) { mediaTypes := printer.BrotherMediaTypes result := make([]BrotherMediaInfo, 0, len(mediaTypes)) for id, media := range mediaTypes { result = append(result, BrotherMediaInfo{ ID: id, Name: media.Name, WidthMM: media.WidthMM, LengthMM: media.LengthMM, IsContinuous: media.LengthMM == 0, TwoColor: media.TwoColor, }) } + sort.Slice(result, func(i, j int) bool { + return result[i].ID < result[j].ID + }) + return result, nil }backend/app/api/handlers/v1/v1_ctrl_label_templates.go (3)
764-765: PathForLoc errors are silently ignored.At lines 764, 810, and 974, errors from
PathForLocare discarded with_. While the path is optional for label generation, logging these failures would help diagnose incomplete labels.🔎 Proposed fix
- path, _ := ctrl.repo.Locations.PathForLoc(r.Context(), auth.GID, locID) + path, pathErr := ctrl.repo.Locations.PathForLoc(r.Context(), auth.GID, locID) + if pathErr != nil { + log.Debug().Err(pathErr).Str("locationID", locID.String()).Msg("failed to fetch location path") + }Also applies to: 810-810, 974-974
580-594: Hardcoded default media type "DK-22251" for Brother printers.The fallback to "DK-22251" is hardcoded. Consider making this configurable or documenting the assumption.
+const defaultBrotherMediaType = "DK-22251" // Default Brother QL label media + mediaType := template.MediaType if mediaType == "" { - mediaType = "DK-22251" // Default to DK-22251 + mediaType = defaultBrotherMediaType }
407-420:applyPresetAndGetSheetLayoutmodifies templateData in place.The function mutates
templateData.WidthandtemplateData.Height(lines 417-418) as a side effect. While this works, the mutation could be surprising. Consider documenting this behavior or returning the modified templateData.Consider adding a comment to document the side effect:
// applyPresetAndGetSheetLayout extracts sheet layout from template preset if available +// NOTE: This function modifies templateData.Width and templateData.Height in place +// to match the preset dimensions when a sheet layout is applied. func applyPresetAndGetSheetLayout(template repo.LabelTemplateOut, templateData *labelmaker.TemplateData) *labelmaker.SheetLayout {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
backend/app/api/handlers/v1/v1_ctrl_label_templates.gobackend/app/api/handlers/v1/v1_ctrl_printers.gobackend/go.modbackend/pkgs/labelmaker/pdf_renderer.gobackend/pkgs/labelmaker/template_renderer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
backend/pkgs/labelmaker/pdf_renderer.go (2)
backend/pkgs/labelmaker/template_renderer.go (5)
TemplateRenderer(100-103)TemplateData(54-62)ItemData(24-40)RenderContext(65-69)LocationData(43-51)backend/pkgs/labelmaker/presets.go (1)
SheetLayout(21-30)
🔇 Additional comments (13)
backend/pkgs/labelmaker/pdf_renderer.go (2)
556-589:RenderToSheetsrelies onRenderToSheetvalidation.This function delegates to
RenderToSheet, which will catch zero columns/rows if properly validated per the earlier suggestion. The implementation looks correct otherwise.
39-53:DefaultPDFOptions()provides sensible defaults.Good defaults for margins, spacing, and auto-calculation of labels per row/column.
backend/app/api/handlers/v1/v1_ctrl_printers.go (4)
166-173: SSRF defense-in-depth validation now in place.Good security practice to re-validate the printer address before use, even though it was validated during creation. This mitigates risks if the database is compromised or the address is modified through other means.
248-254: SSRF validation also applied to test print endpoint.Consistent with the status endpoint, the test print handler now validates the printer address before creating the client.
63-75: Create endpoint validates SSRF before persisting.Good: The address is validated before being stored, preventing malicious addresses from entering the database.
87-100: Update endpoint also validates SSRF.Consistent security check on address modification.
backend/app/api/handlers/v1/v1_ctrl_label_templates.go (7)
560-567: SSRF defense-in-depth validation added to print handler.The printer address is now re-validated before use, addressing the past review concern. This prevents SSRF attacks if the database is compromised.
922-929: SSRF validation also applied to print-locations handler.Consistent security posture with the items print handler.
326-351:buildLocationDatacorrectly constructs hierarchical paths.The path building logic properly excludes the current location from its own path and constructs the full path string with proper separators.
353-391:buildItemDatacorrectly maps repository model to labelmaker data.Good conversion of labels, custom fields, and location information.
21-36: Standard CRUD handlers look correct.
HandleLabelTemplatesGetAllproperly uses auth context for group-scoped access.
172-210: Preview handler correctly renders template with sample data.The preview endpoint renders the template using
RenderPreviewwhich provides a visual representation without requiring actual item data.
278-278: Input validation forRefererheader is already in place viaGetHBURL.The function uses Go's standard
url.Parse()to parse the Referer header, then reconstructs the URL using only the Scheme and Host components (viastripPathFromURL). This approach effectively prevents path, query string, and fragment injection attacks. Invalid URLs return an empty string, preventing malformed URLs from being used in label data. The ItemURL is then safely constructed by concatenating the baseURL with a UUID, which cannot be injected.
Resolved conflicts in: - backend/go.mod: Keep bild, barcode deps; accept ardanlabs/conf v3.10.0 - backend/go.sum: Regenerated - frontend/package.json: Keep fabric; accept dompurify v3.3.1 - frontend/pnpm-lock.yaml: Regenerated - frontend/locales/en.json: Merge table features with label printing strings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add validation for layout.Columns and layout.Rows to prevent divide-by-zero panic when computing labelsPerPage - Add logging for skipped labels during PDF generation so failures are visible in server logs rather than silently ignored - Apply fixes to both renderPDFWithSheetLayout and renderLocationsPDFWithSheetLayout functions Addresses CodeRabbit review feedback on PR sysadminsmedia#1161. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The boombuler/barcode library does not support true UPC-E encoding. The previous implementation approximated UPC-E using EAN-8, which would not scan as valid UPC-E barcodes. Removing this feature to avoid user confusion. UPC-A remains available as an alternative. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Align DPI validation limits (templates now allow up to 1200 DPI like printers) - Log png.Encode errors in test pattern generator instead of ignoring - Add path traversal validation to normalizePath for defense-in-depth - Track and report skipped labels in print response message - Add X-Labels-Warning header when PNG sheet output truncates multiple sheets Note: Default printer race condition deferred - requires transaction refactoring 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wrap clearDefault + create/update operations in database transactions to ensure atomicity when setting the default printer. This prevents race conditions where two concurrent requests could both set IsDefault=true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/app/api/handlers/v1/v1_ctrl_printers.go (2)
190-190: Consider logging ignored status update errors.Database update failures are silently discarded. While the primary operation (checking status) still succeeds, logging these errors would help diagnose persistent database issues.
🔎 Proposed improvement
// Update status in database -_ = ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline") +if err := ctrl.repo.Printers.UpdateStatus(r.Context(), auth.GID, ID, "offline"); err != nil { + log.Warn().Err(err).Str("printer_id", ID.String()).Msg("failed to update printer status to offline") +}Apply the same pattern to line 199.
Also applies to: 199-199
294-319: Optional: Simplify test image generation.The manual pixel-by-pixel loops work correctly but could be simplified using Go's
image/drawpackage for filling backgrounds.🔎 Alternative approach using image/draw
+import ( + "image/draw" + // ... other imports +) + func createTestLabelPNG() []byte { - // Create a 100x100 image with a simple pattern img := image.NewRGBA(image.Rect(0, 0, 100, 100)) - - // Fill with white background white := color.RGBA{255, 255, 255, 255} black := color.RGBA{0, 0, 0, 255} - for y := 0; y < 100; y++ { - for x := 0; x < 100; x++ { - img.Set(x, y, white) - } - } + // Fill with white background + draw.Draw(img, img.Bounds(), &image.Uniform{white}, image.Point{}, draw.Src) // Draw a simple border for i := 0; i < 100; i++ {backend/pkgs/labelmaker/pdf_renderer.go (2)
111-157: Good improvement: Skip tracking added to sheet layout rendering.The sheet layout rendering methods now track skipped labels with counters and log warnings (lines 111-157, 204-250). This is a significant improvement over silently dropping failed labels.
Note: The non-sheet rendering methods (RenderToPDF lines 432-440, RenderLocationsToPDF lines 329-337) still silently skip errors without tracking. Consider adding similar skip tracking for consistency, though this is less critical since sheet layouts are the primary use case.
Also applies to: 204-250
152-152: Unused variable assignments.The decoded image variable is assigned but not used beyond the initial decode operation. The comment at line 455 suggests future use for bounds checking.
These unused assignments are harmless but could be removed if the image bounds or metadata won't be used.
Also applies to: 245-245, 352-352, 455-455
backend/app/api/handlers/v1/v1_ctrl_label_templates.go (1)
262-267: Canvas data parsing accepts any JSON structure.The live preview feature parses the provided canvas data as JSON (lines 262-267) without validating its structure. While this provides flexibility, malformed canvas data could potentially cause issues during rendering.
The current approach is pragmatic (let the renderer handle invalid structures), but consider whether basic structure validation (e.g., checking for required keys) would prevent confusing errors downstream.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/api/handlers/v1/v1_ctrl_label_templates.gobackend/app/api/handlers/v1/v1_ctrl_printers.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_label_templates.gobackend/internal/data/repo/repo_printers.gobackend/pkgs/labelmaker/barcode_generator.gobackend/pkgs/labelmaker/pdf_renderer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/internal/data/repo/repo_printers.go
- backend/internal/data/repo/repo_item_attachments.go
🧰 Additional context used
🧬 Code graph analysis (3)
backend/internal/data/repo/repo_label_templates.go (2)
backend/internal/core/services/reporting/eventbus/eventbus.go (3)
EventBus(28-34)EventLabelMutation(14-14)GroupMutationEvent(19-21)backend/internal/core/services/contexts.go (1)
Context(19-30)
backend/pkgs/labelmaker/pdf_renderer.go (3)
backend/pkgs/labelmaker/template_renderer.go (4)
TemplateData(54-62)ItemData(24-40)RenderContext(65-69)LocationData(43-51)backend/pkgs/labelmaker/presets.go (1)
SheetLayout(21-30)backend/pkgs/set/set.go (1)
Set(8-10)
backend/app/api/handlers/v1/v1_ctrl_label_templates.go (8)
backend/app/api/handlers/v1/controller.go (1)
V1Controller(68-79)backend/internal/data/repo/repo_label_templates.go (1)
LabelTemplateOut(61-77)backend/pkgs/labelmaker/presets.go (4)
LabelPreset(5-18)LabelPresets(33-480)SheetLayout(21-30)GetPresetByKey(483-490)backend/pkgs/labelmaker/template_renderer.go (6)
NewTemplateRenderer(106-121)TemplateData(54-62)RenderContext(65-69)LocationData(43-51)ItemData(24-40)TemplateRenderer(100-103)backend/internal/web/adapters/decoders.go (1)
DecodeBody(36-57)backend/app/api/handlers/v1/helpers.go (1)
GetHBURL(9-16)backend/pkgs/printer/address_validation.go (1)
ValidatePrinterAddress(59-94)backend/pkgs/printer/printer.go (3)
NewPrinterClient(82-93)PrinterType(10-10)PrinterTypeBrotherRaster(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: build (linux/arm64)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (17)
backend/app/api/handlers/v1/v1_ctrl_printers.go (10)
28-35: LGTM! Standard CRUD pattern correctly implemented.The handler properly extracts authentication context and delegates to the repository layer.
47-53: LGTM! Clean implementation.Standard get-by-ID pattern with proper authentication.
65-76: LGTM! SSRF protection properly implemented.The handler validates printer addresses before persistence, preventing malicious addresses from being stored.
89-101: LGTM! Consistent security validation.Update handler properly validates addresses and correctly assigns the ID from the path parameter.
113-120: LGTM! Standard delete implementation.Properly authenticated delete operation with correct HTTP status code.
132-139: LGTM! Clean default setter.Simple and correct implementation for setting a default printer.
167-174: LGTM! Defense-in-depth validation properly implemented.The SSRF concern from the previous review has been addressed with address re-validation before creating the printer client.
249-255: LGTM! Defense-in-depth SSRF protection.Consistent address re-validation prevents compromised database values from being exploited.
323-325: LGTM! Error handling addressed.The previous review concern about ignored
png.Encodeerrors has been resolved with proper logging.
348-367: LGTM! Clean media types endpoint.The handler correctly exposes Brother media types as global reference data. Not filtering by group ID is appropriate since these are hardware specifications.
backend/internal/data/repo/repo_label_templates.go (1)
1-368: LGTM! Well-structured repository implementation.The label templates repository follows established patterns and includes:
- Proper access control in all methods (ownership and sharing checks)
- Event bus integration for mutation notifications
- Correct handling of optional fields (Preset, CanvasData, MediaType)
- Duplicate logic with proper name truncation
The DPI validation has been correctly updated to 1200 to match printer capabilities, addressing the previous review concern.
backend/pkgs/labelmaker/barcode_generator.go (1)
1-262: LGTM! Barcode generation implementation is solid.The barcode generator provides comprehensive format support with proper validation:
- Empty content validation (line 50-52)
- Format-specific length validation (EAN-13, EAN-8, UPC-A)
- UPC-A correctly converts to EAN-13 via leading zero
- Metadata and placeholder generation for UI/preview needs
All supported formats are production-ready.
backend/pkgs/labelmaker/pdf_renderer.go (1)
92-94: Excellent fix for the divide-by-zero vulnerability!The validation at lines 92-94 and 185-187 now prevents the panic that could occur when
layout.Columnsorlayout.Rowsis zero. The error message also includes the actual values, which aids debugging.This addresses the critical issue from the previous review.
Also applies to: 185-187
backend/app/api/handlers/v1/v1_ctrl_label_templates.go (4)
564-570: Excellent security improvement: SSRF validation added!The defense-in-depth approach of re-validating printer addresses before use (lines 564, 935) is a strong security practice. This prevents SSRF attacks even if the database is compromised, addressing the critical security concern from the previous review.
Also applies to: 935-941
607-691: Great improvement: Item print tracking now reports skipped labels.The print handler now tracks both printed and skipped labels (lines 607-628) and includes this information in the response message (lines 688-691). This provides transparency to users when some labels fail to print.
This addresses the concern from the previous review.
457-459: Good mitigation: Warning header for multi-sheet PNG limitation.The warning header at line 458 informs users when only the first sheet is returned from a multi-sheet render. This partially addresses the concern from the previous review by making the limitation transparent.
For a complete solution, consider implementing ZIP generation for multiple sheets in a future enhancement, but the current approach is acceptable.
583-597: Brother raster media type configuration is well-implemented.The media type configuration for Brother raster printers (lines 583-597, 954-968) includes:
- Default fallback to "DK-22251" if not specified
- Validation via
SetMediaTypewith error handling- Consistent application for both item and location printing
This ensures labels print correctly on Brother label printers.
Also applies to: 954-968
- Add zero columns/rows validation to RenderToSheet and RenderToSheets - Add skipped labels tracking to location printing (consistency with item printing) - Parse and use PrinterTestRequest body in HandlePrintersTest 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added skipped label tracking and warning logs to RenderToSheet to match the error handling pattern in renderPDFWithSheetLayout and renderLocationsPDFWithSheetLayout. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/pkgs/labelmaker/pdf_renderer.go (2)
309-353: Inconsistent error handling: Silent skipping without logging.Unlike the sheet-aware rendering paths (lines 85-167, 178-260) that now log warnings and track skipped labels, this function silently continues on errors at lines 328-331 and 334-337. For consistency and transparency, consider adding warning logs and skipped-label tracking here as well.
🔎 Proposed fix for consistency
// Render each label as an image and place on PDF + skipped := 0 for i, location := range locations { // Add new page if needed if i%labelsPerPage == 0 { pdf.AddPage() } // Calculate position on page posInPage := i % labelsPerPage col := posInPage % labelsPerRow row := posInPage / labelsPerRow x := opts.MarginLeft + float64(col)*(labelWidth+opts.LabelSpacingX) y := opts.MarginTop + float64(row)*(labelHeight+opts.LabelSpacingY) // Render label to image imgData, err := r.RenderTemplate(RenderContext{ Location: location, Template: template, }) if err != nil { - // Skip this label on error but continue with others + log.Warn().Err(err).Int("index", i).Msg("failed to render location label, skipping") + skipped++ continue } // Decode PNG img, err := png.Decode(bytes.NewReader(imgData)) if err != nil { + log.Warn().Err(err).Int("index", i).Msg("failed to decode rendered PNG, skipping") + skipped++ continue } // ... rest of function } + + if skipped > 0 { + log.Warn().Int("skipped", skipped).Int("total", len(locations)).Msg("some location labels were skipped during PDF generation") + }
412-456: Inconsistent error handling: Silent skipping without logging.Similar to
RenderLocationsToPDF, this function silently continues on errors at lines 431-434 and 437-440 without logging or tracking skipped labels. Consider adding warning logs and skipped-label tracking to match the sheet-aware rendering paths.🔎 Proposed fix for consistency
// Render each label as an image and place on PDF + skipped := 0 for i, item := range items { // Add new page if needed if i%labelsPerPage == 0 { pdf.AddPage() } // Calculate position on page posInPage := i % labelsPerPage col := posInPage % labelsPerRow row := posInPage / labelsPerRow x := opts.MarginLeft + float64(col)*(labelWidth+opts.LabelSpacingX) y := opts.MarginTop + float64(row)*(labelHeight+opts.LabelSpacingY) // Render label to image imgData, err := r.RenderTemplate(RenderContext{ Item: item, Template: template, }) if err != nil { - // Skip this label on error but continue with others + log.Warn().Err(err).Int("index", i).Msg("failed to render label, skipping") + skipped++ continue } // Decode PNG img, err := png.Decode(bytes.NewReader(imgData)) if err != nil { + log.Warn().Err(err).Int("index", i).Msg("failed to decode rendered PNG, skipping") + skipped++ continue } // ... rest of function } + + if skipped > 0 { + log.Warn().Int("skipped", skipped).Int("total", len(items)).Msg("some labels were skipped during PDF generation") + }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/handlers/v1/v1_ctrl_label_templates.gobackend/app/api/handlers/v1/v1_ctrl_printers.gobackend/pkgs/labelmaker/pdf_renderer.go
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/api/handlers/v1/v1_ctrl_printers.go (4)
backend/internal/web/adapters/command.go (2)
Command(26-35)CommandID(48-62)backend/pkgs/printer/address_validation.go (1)
ValidatePrinterAddress(59-94)backend/pkgs/printer/printer.go (3)
NewPrinterClient(82-93)PrinterType(10-10)PrintJob(42-49)backend/pkgs/printer/brother_media.go (1)
BrotherMediaTypes(15-171)
backend/pkgs/labelmaker/pdf_renderer.go (2)
backend/pkgs/labelmaker/template_renderer.go (5)
TemplateRenderer(100-103)TemplateData(54-62)ItemData(24-40)RenderContext(65-69)LocationData(43-51)backend/pkgs/labelmaker/presets.go (1)
SheetLayout(21-30)
🔇 Additional comments (15)
backend/app/api/handlers/v1/v1_ctrl_printers.go (7)
28-35: LGTM!The handler follows the established adapter pattern and properly retrieves all printers for the authenticated group.
64-76: LGTM! Proper SSRF protection.The handler correctly validates printer addresses before persistence, preventing Server-Side Request Forgery attacks by enforcing private network restrictions (configurable via
AllowPublicAddresses).
88-101: LGTM! Consistent SSRF protection on update.Address validation is properly enforced on the update path as well, maintaining security consistency across CRUD operations.
158-210: LGTM! Excellent defense-in-depth security.The status endpoint re-validates the printer address even though it was validated at creation/update time (lines 167-174). This protects against database-level tampering and is a security best practice. The handler also properly updates printer status in the database for both success and failure cases.
234-303: LGTM! Past issues addressed.The handler now properly parses and uses the optional
PrinterTestRequestbody (lines 238-280), enabling custom test messages. SSRF protection is correctly applied via address re-validation (lines 256-262).
305-340: LGTM! Error handling improved.The PNG encoding error is now properly logged (lines 336-338) instead of being silently ignored. Since this generates a simple test pattern, returning an empty buffer with logging is acceptable for this internal helper.
360-380: LGTM!The handler correctly exposes Brother printer media types by transforming the internal representation to the API response format with computed
IsContinuousfield.backend/pkgs/labelmaker/pdf_renderer.go (4)
85-167: LGTM! Past issues addressed.The function now properly validates that columns and rows are positive (lines 92-94) and tracks skipped labels with warning logs (lines 131-143, 155-157) instead of silently continuing. This provides transparency about partial failures.
178-260: LGTM! Consistent error handling.This location variant mirrors the item rendering function with proper validation (lines 185-187) and warning logs for skipped labels (lines 224-236, 248-250).
488-589: LGTM! Consistent with sheet-aware rendering.The function properly validates layout dimensions (lines 498-500) and tracks skipped labels with warning logs (lines 544-556, 578-580), matching the error-handling approach in other sheet-aware rendering paths.
593-629: LGTM! Proper multi-sheet rendering.The function validates layout dimensions (lines 603-605) and correctly delegates to
RenderToSheetfor each sheet, propagating any errors.backend/app/api/handlers/v1/v1_ctrl_label_templates.go (4)
232-324: LGTM! Flexible rendering with live preview support.The handler supports multiple output formats (PDF, PNG sheet, single PNG) and includes live preview functionality via optional canvas data (lines 262-267). The silent fallback on unmarshal failure is appropriate for the live preview use case.
498-701: LGTM! Comprehensive print workflow with proper security.The handler correctly re-validates printer addresses for defense-in-depth security (lines 564-570) and tracks skipped labels with detailed error messages (lines 607-691). The backward-compatible API design supporting both
itemIdsanditemsis well-implemented.
870-1071: LGTM! Consistent location printing workflow.This handler mirrors the item printing implementation with proper SSRF protection (lines 935-941) and skipped-label tracking (lines 978-1061), ensuring consistency across both printing workflows.
443-462: LGTM! Multi-sheet handling with user notification.The function now includes a warning header (lines 457-459) when multiple sheets are generated but only the first is returned, properly informing users to use PDF format for all sheets.
1 participant
What type of PR is this?
What this PR does / why we need it:
This PR adds a comprehensive label template system for creating, editing, and printing custom labels for items and locations. Previously, Homebox only supported basic QR code generation. This feature enables users to design professional labels with barcodes, text, and dynamic data fields.
Backend Changes
New Packages:
New API Handlers:
Database:
Frontend Changes
Label Template Editor:
Pages:
Print Integration:
Label Presets
Documentation
Which issue(s) this PR fixes:
Addresses multiple label oriented issues, including Discussion #53
Special notes for your reviewer:
Areas to focus on:
Testing
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.