feat: improve label print command with structured env vars and error logging#1529
Open
Trevor159 wants to merge 6 commits into
Open
feat: improve label print command with structured env vars and error logging#1529Trevor159 wants to merge 6 commits into
Trevor159 wants to merge 6 commits into
Conversation
Contributor
Summary by CodeRabbit
WalkthroughPass structured item metadata (Name, Description, Location, AssetID) from handlers into the labelmaker, include those fields in template rendering, expose them as LABEL_-prefixed environment variables to the print subprocess, and document the available variables for custom print commands. ChangesLabel Metadata Handling and Print Command Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Security Recommendations
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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
added
the
📖 documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/src/content/docs/en/user-guide/tips-tricks.mdx (1)
79-97: ⚡ Quick winAdd a shell-safety warning and safe usage example for print command templates.
Because this runs via a shell command, interpolating metadata directly into the command string can lead to unsafe execution or broken arguments. Add a short caution to prefer reading values from
LABEL_*env vars and always quote expansions in scripts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/content/docs/en/user-guide/tips-tricks.mdx` around lines 79 - 97, Add a short shell-safety caution to the HBOX_LABEL_MAKER_PRINT_COMMAND section warning that template interpolation runs via a shell and can lead to unsafe execution or broken arguments; recommend preferring the LABEL_* environment variables (e.g., LABEL_URL, LABEL_Name, LABEL_AssetID) over inline template expansion and explicitly instruct to always quote expansions in scripts. Also add one brief, non-code safe-usage example description showing: pass values via LABEL_* env vars to the print command and wrap expansions in quotes in the script/command to avoid word-splitting and shell injection, referencing the HBOX_LABEL_MAKER_PRINT_COMMAND setting and the supported template variables.backend/pkgs/labelmaker/labelmaker.go (2)
73-92: 💤 Low valueConsider an options struct for cleaner API evolution.
The function signature has grown to 14 parameters, making call sites harder to read and prone to positional errors. For future extensibility, consider grouping the item-metadata fields into a dedicated struct (e.g.,
ItemMetadata{Name, Description, Location, AssetID}).Not blocking for this PR, but worth considering for maintainability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 73 - 92, The NewGenerateParams signature has become unwieldy; refactor by introducing a new ItemMetadata struct (e.g., ItemMetadata with fields Name, Description, Location, AssetID) and replace the four item-related string parameters in NewGenerateParams with a single itemMetadata ItemMetadata parameter (or wrap all optional parameters in an options struct) and update the returned GenerateParameters construction to set Name, Description, Location, and AssetID from that ItemMetadata; ensure callers are updated to build and pass ItemMetadata and preserve AdditionalInformation, DynamicLength, and other existing fields.
470-506: 💤 Low valueSecurity note: Item metadata now exposed to print subprocess.
The print command subprocess now receives potentially sensitive item data (names, descriptions, locations, asset IDs) via
LABEL_*environment variables. This is the intended behavior per the PR, but worth documenting that:
- The print script (configured via
HBOX_LABEL_MAKER_PRINT_COMMAND) has full access to label metadata- Administrators should ensure print scripts handle this data appropriately, especially if logging or forwarding to external services
The implementation itself is secure—Go's
exec.Cmd.Envproperly isolates environment variable values from shell interpretation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 470 - 506, Add an explicit security comment and admin guidance above the code that builds and sets command.Env (around templateVars, env, and command.Env usage): state that the print subprocess receives label metadata via LABEL_* environment variables (e.g., Name, Description, Location, AssetID), reference the HBOX_LABEL_MAKER_PRINT_COMMAND configuration, and advise administrators to ensure their print scripts do not log, transmit, or mishandle sensitive item data (or to sanitize/limit which fields are exported) and to restrict access to the print command; keep the note concise and placed immediately before the env loop/exec.Command usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Line 16: The docker-compose service currently sets the wrong and insecure
default debug environment variable: replace the HBOX_DEBUG=true entry with the
documented key HBOX_DEBUG_ENABLED and disable debug by default (e.g., set
HBOX_DEBUG_ENABLED=false or remove the boolean and use variable substitution
like ${HBOX_DEBUG_ENABLED:-false}) so consumers must opt-in to enable debug;
update any references to HBOX_DEBUG in startup scripts or README to use
HBOX_DEBUG_ENABLED to keep behavior consistent.
---
Nitpick comments:
In `@backend/pkgs/labelmaker/labelmaker.go`:
- Around line 73-92: The NewGenerateParams signature has become unwieldy;
refactor by introducing a new ItemMetadata struct (e.g., ItemMetadata with
fields Name, Description, Location, AssetID) and replace the four item-related
string parameters in NewGenerateParams with a single itemMetadata ItemMetadata
parameter (or wrap all optional parameters in an options struct) and update the
returned GenerateParameters construction to set Name, Description, Location, and
AssetID from that ItemMetadata; ensure callers are updated to build and pass
ItemMetadata and preserve AdditionalInformation, DynamicLength, and other
existing fields.
- Around line 470-506: Add an explicit security comment and admin guidance above
the code that builds and sets command.Env (around templateVars, env, and
command.Env usage): state that the print subprocess receives label metadata via
LABEL_* environment variables (e.g., Name, Description, Location, AssetID),
reference the HBOX_LABEL_MAKER_PRINT_COMMAND configuration, and advise
administrators to ensure their print scripts do not log, transmit, or mishandle
sensitive item data (or to sanitize/limit which fields are exported) and to
restrict access to the print command; keep the note concise and placed
immediately before the env loop/exec.Command usage.
In `@docs/src/content/docs/en/user-guide/tips-tricks.mdx`:
- Around line 79-97: Add a short shell-safety caution to the
HBOX_LABEL_MAKER_PRINT_COMMAND section warning that template interpolation runs
via a shell and can lead to unsafe execution or broken arguments; recommend
preferring the LABEL_* environment variables (e.g., LABEL_URL, LABEL_Name,
LABEL_AssetID) over inline template expansion and explicitly instruct to always
quote expansions in scripts. Also add one brief, non-code safe-usage example
description showing: pass values via LABEL_* env vars to the print command and
wrap expansions in quotes in the script/command to avoid word-splitting and
shell injection, referencing the HBOX_LABEL_MAKER_PRINT_COMMAND setting and the
supported template variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63d90170-0300-4b63-9814-c5d2220fc158
📒 Files selected for processing (5)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.gobackend/pkgs/labelmaker/labelmaker.godocker-compose.ymldocs/src/content/docs/en/quick-start/configure/index.mdxdocs/src/content/docs/en/user-guide/tips-tricks.mdx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
46-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwagger annotation produces
image/pngbut declares@Produce json.The handler returns a PNG image (via
labelmaker.GenerateLabel) or plain text "Printed!", but the Swagger annotation at line 40 says@Produce json. This should be corrected for accurate API documentation.📝 Suggested fix
// `@Summary` Get Location label // `@Tags` Locations -// `@Produce` json +// `@Produce` png // `@Param` id path string true "Location ID"Note: The same issue exists in
HandleGetItemLabel(line 68) andHandleGetAssetLabel(line 104).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_labelmaker.go` around lines 46 - 61, The Swagger `@Produce` annotation is wrong: HandleGetLocationLabel (and similarly HandleGetItemLabel and HandleGetAssetLabel) actually return a PNG image or plain text "Printed!", so update the Swagger comments above these handler functions to declare the correct MIME types (e.g. image/png and text/plain) instead of json; ensure each function's comment block lists both image/png and text/plain so generated docs match the actual responses.backend/pkgs/labelmaker/labelmaker.go (2)
503-509:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSecurity consideration: Sanitize environment variable values to prevent injection.
Environment variable values are passed directly from user-controlled item metadata (Name, Description, Location, AssetID). While environment variables are generally safer than command-line arguments, values containing newlines or null bytes could cause unexpected behavior in some shells/scripts.
Consider sanitizing values by stripping or replacing control characters:
🛡️ Optional sanitization
+func sanitizeEnvValue(s string) string { + // Remove null bytes and replace newlines with spaces + s = strings.ReplaceAll(s, "\x00", "") + s = strings.ReplaceAll(s, "\n", " ") + s = strings.ReplaceAll(s, "\r", " ") + return s +} + env := os.Environ() for k, v := range templateVars { - env = append(env, "LABEL_"+k+"="+v) + env = append(env, "LABEL_"+k+"="+sanitizeEnvValue(v)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 503 - 509, Sanitize the templateVars values before appending to env to prevent injection via control characters (newlines, carriage returns, null bytes, other non-printables): iterate over templateVars and replace or strip control characters from each v before creating "LABEL_"+k+"="+v; update the code that builds env (the env variable construction using templateVars) so only cleaned values are appended and ensure command.Env (set on exec.Command) receives the sanitized env; reference the templateVars map, the env slice, and command.Env when making the change.
498-501:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCommand parsing with
strings.Fieldscan break quoted arguments containing spaces.If the user's print command template expands to include values with spaces (e.g.,
{{.Name}}where Name is "Living Room"),strings.Fieldssplits on whitespace, turning one argument into multiple. This undermines the stated PR goal of "fix spaces-in-variables being treated as multiple script arguments."The environment variable approach (lines 503-506) does provide a workaround since users can reference
$LABEL_Nameinstead of{{.Name}}, but the template variable path remains broken for values with spaces.💡 Suggested documentation or fix approach
Either:
- Document that template variables should only be used for the filename/URL (no spaces), and users should use
$LABEL_*env vars for metadata with spaces, OR- Consider shell-aware parsing (e.g., using a shell library or invoking via
sh -cwith the whole command as a single string):- commandParts := strings.Fields(builder.String()) - if len(commandParts) == 0 { - return nil - } - - env := os.Environ() - for k, v := range templateVars { - env = append(env, "LABEL_"+k+"="+v) - } - - command := exec.Command(commandParts[0], commandParts[1:]...) + cmdStr := builder.String() + if strings.TrimSpace(cmdStr) == "" { + return nil + } + + env := os.Environ() + for k, v := range templateVars { + env = append(env, "LABEL_"+k+"="+v) + } + + command := exec.Command("sh", "-c", cmdStr)Note: Using
sh -cchanges execution semantics and has its own security considerations if user-controlled values are interpolated into the command string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 498 - 501, The code currently splits the expanded command via strings.Fields(builder.String()), which breaks quoted arguments with spaces; update the command execution path in the function that builds commandParts so it preserves quotes — either parse the command string with a shell-aware parser (e.g., replace strings.Fields with a POSIX word-splitting library such as shlex or mvdan's shell parser) to produce correctly quoted arguments, or avoid splitting and invoke the command through the shell (e.g., call sh -c with the full builder.String()) and document the changed semantics/security tradeoffs; ensure references to builder.String(), commandParts and the code path that sets env vars ($LABEL_*) are adjusted so template-expanded values containing spaces are passed as single arguments.
🧹 Nitpick comments (2)
backend/pkgs/labelmaker/labelmaker.go (1)
488-492: 💤 Low valueMinor alignment inconsistency in templateVars map.
The new metadata fields (Name, Description, Location, AssetID) lack the tab alignment used by the preceding entries in the map literal. This is a cosmetic issue that doesn't affect functionality.
🔧 Suggested formatting fix
- "Name": params.Name, - "Description": params.Description, - "Location": params.Location, - "AssetID": params.AssetID, + "Name": params.Name, + "Description": params.Description, + "Location": params.Location, + "AssetID": params.AssetID,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 488 - 492, Align the tab spacing of the new entries in the templateVars map so they match the existing map literal alignment: adjust "Name": params.Name, "Description": params.Description, "Location": params.Location, and "AssetID": params.AssetID to use the same indentation/tabs as the preceding entries in the templateVars map to maintain consistent formatting.backend/app/api/handlers/v1/v1_ctrl_labelmaker.go (1)
87-96: 💤 Low valueClarify the distinction between
description(labelDesc) andItemMetadata.Description.The code correctly distinguishes between:
description(line 87-93): Text rendered ON the label showing location context ("Location: <parent>")ItemMetadata.Description(line 96): The actual item's description field exposed asLABEL_Descriptionenv varThis aligns with the documentation table showing
DescriptionTextvsDescriptionas separate concepts. However, a brief inline comment would help future maintainers understand this intentional distinction.📝 Optional clarifying comment
- description := "" + // labelDescription: text rendered ON the label (shows location context) + // ItemMetadata.Description: the item's actual description field for LABEL_Description env var + labelDescription := "" parentName := "" if item.Parent != nil { parentName = item.Parent.Name - description += fmt.Sprintf("\nLocation: %s", parentName) + labelDescription += fmt.Sprintf("\nLocation: %s", parentName) } hbURL := GetHBURL(r, &ctrl.config.Options, ctrl.url) - return generateOrPrint(ctrl, w, r, item.Name, description, fmt.Sprintf("%s/item/%s", hbURL, item.ID), labelmaker.ItemMetadata{Name: item.Name, Description: item.Description, Location: parentName}) + return generateOrPrint(ctrl, w, r, item.Name, labelDescription, fmt.Sprintf("%s/item/%s", hbURL, item.ID), labelmaker.ItemMetadata{Name: item.Name, Description: item.Description, Location: parentName})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_labelmaker.go` around lines 87 - 96, Add a brief inline comment above the creation of the local variable description (and/or above the generateOrPrint call) clarifying that the local variable description (labelDesc) is the text rendered on the physical label (e.g., "Location: <parent>"), whereas labelmaker.ItemMetadata.Description (passed as ItemMetadata.Description) is the item's actual description exposed as the LABEL_Description env var; update the block around description, parentName, GetHBURL and the generateOrPrint(...) call to include that comment so future maintainers understand the intentional difference between the two values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/app/api/handlers/v1/v1_ctrl_labelmaker.go`:
- Around line 46-61: The Swagger `@Produce` annotation is wrong:
HandleGetLocationLabel (and similarly HandleGetItemLabel and
HandleGetAssetLabel) actually return a PNG image or plain text "Printed!", so
update the Swagger comments above these handler functions to declare the correct
MIME types (e.g. image/png and text/plain) instead of json; ensure each
function's comment block lists both image/png and text/plain so generated docs
match the actual responses.
In `@backend/pkgs/labelmaker/labelmaker.go`:
- Around line 503-509: Sanitize the templateVars values before appending to env
to prevent injection via control characters (newlines, carriage returns, null
bytes, other non-printables): iterate over templateVars and replace or strip
control characters from each v before creating "LABEL_"+k+"="+v; update the code
that builds env (the env variable construction using templateVars) so only
cleaned values are appended and ensure command.Env (set on exec.Command)
receives the sanitized env; reference the templateVars map, the env slice, and
command.Env when making the change.
- Around line 498-501: The code currently splits the expanded command via
strings.Fields(builder.String()), which breaks quoted arguments with spaces;
update the command execution path in the function that builds commandParts so it
preserves quotes — either parse the command string with a shell-aware parser
(e.g., replace strings.Fields with a POSIX word-splitting library such as shlex
or mvdan's shell parser) to produce correctly quoted arguments, or avoid
splitting and invoke the command through the shell (e.g., call sh -c with the
full builder.String()) and document the changed semantics/security tradeoffs;
ensure references to builder.String(), commandParts and the code path that sets
env vars ($LABEL_*) are adjusted so template-expanded values containing spaces
are passed as single arguments.
---
Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_labelmaker.go`:
- Around line 87-96: Add a brief inline comment above the creation of the local
variable description (and/or above the generateOrPrint call) clarifying that the
local variable description (labelDesc) is the text rendered on the physical
label (e.g., "Location: <parent>"), whereas labelmaker.ItemMetadata.Description
(passed as ItemMetadata.Description) is the item's actual description exposed as
the LABEL_Description env var; update the block around description, parentName,
GetHBURL and the generateOrPrint(...) call to include that comment so future
maintainers understand the intentional difference between the two values.
In `@backend/pkgs/labelmaker/labelmaker.go`:
- Around line 488-492: Align the tab spacing of the new entries in the
templateVars map so they match the existing map literal alignment: adjust
"Name": params.Name, "Description": params.Description, "Location":
params.Location, and "AssetID": params.AssetID to use the same indentation/tabs
as the preceding entries in the templateVars map to maintain consistent
formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2daee6b-cfd0-4f59-8965-8a728e2717d6
📒 Files selected for processing (2)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.gobackend/pkgs/labelmaker/labelmaker.go
Trevor159
force-pushed
the
better-label-command
branch
from
73d2cfd to
d35aec6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/pkgs/labelmaker/labelmaker.go (1)
473-492:⚠️ Potential issue | 🟠 MajorFix argv splitting for free-text template vars in
HBOX_LABEL_MAKER_PRINT_COMMANDFree-text template vars like
Name,Description,Location(and other free-text fields) are inserted into the rendered command string, but the command is then split withstrings.Fields, which ignores shell-style quoting—so values containing spaces/newlines get “chomped” into multiple argv entries (e.g."Living Room Shelf"becomes["\"Living","Room","Shelf\""]), making the template vars unreliable and changing the arguments passed to the external print program.Security/robustness: build an argv slice directly from structured inputs, or switch to quote-aware parsing (e.g., shlex/shell-quote) instead of
strings.Fields; otherwise restrict/document these fields as env-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pkgs/labelmaker/labelmaker.go` around lines 473 - 492, The command string built from templateVars (keys like "Name", "Description", "Location" in the templateVars map) is being split with strings.Fields for HBOX_LABEL_MAKER_PRINT_COMMAND which breaks quoted/free-text values with spaces/newlines; fix by constructing the argv slice without strings.Fields: either 1) build the []string explicitly from the structured inputs (use the rendered program/args template to produce a slice of arguments and substitute templateVars into each element) or 2) replace strings.Fields with a quote-aware parser (shell-quote/shlex) so quoted values remain single argv entries; update the code path that references HBOX_LABEL_MAKER_PRINT_COMMAND and remove the strings.Fields call (or wrap it with the chosen shlex library) so templateVars like Name/Description/Location are preserved as single arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/pkgs/labelmaker/labelmaker.go`:
- Around line 511-514: The current failure path around command.CombinedOutput()
logs the entire child stdout/stderr (variables output and err); change this to
log only the exit error plus a redacted/capped excerpt of the combined output
instead of the full stream. Specifically, in the block handling output, capture
command.CombinedOutput() as before but when err != nil, create a short excerpt
(e.g., take the last N bytes or first N bytes, replace newlines with spaces, and
redact any LABEL_* tokens or other sensitive-looking patterns) and log a message
with err plus that capped/redacted excerpt; do not include the full output
variable in logs. Apply this change to the failure branch that currently prints
"label print command failed" so only the safer, capped excerpt is logged.
---
Outside diff comments:
In `@backend/pkgs/labelmaker/labelmaker.go`:
- Around line 473-492: The command string built from templateVars (keys like
"Name", "Description", "Location" in the templateVars map) is being split with
strings.Fields for HBOX_LABEL_MAKER_PRINT_COMMAND which breaks quoted/free-text
values with spaces/newlines; fix by constructing the argv slice without
strings.Fields: either 1) build the []string explicitly from the structured
inputs (use the rendered program/args template to produce a slice of arguments
and substitute templateVars into each element) or 2) replace strings.Fields with
a quote-aware parser (shell-quote/shlex) so quoted values remain single argv
entries; update the code path that references HBOX_LABEL_MAKER_PRINT_COMMAND and
remove the strings.Fields call (or wrap it with the chosen shlex library) so
templateVars like Name/Description/Location are preserved as single arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5768c51c-00ae-486b-94ae-4698d4942e3c
📒 Files selected for processing (4)
backend/app/api/handlers/v1/v1_ctrl_labelmaker.gobackend/pkgs/labelmaker/labelmaker.godocs/src/content/docs/en/quick-start/configure/index.mdxdocs/src/content/docs/en/user-guide/tips-tricks.mdx
✅ Files skipped from review due to trivial changes (2)
- docs/src/content/docs/en/quick-start/configure/index.mdx
- docs/src/content/docs/en/user-guide/tips-tricks.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/handlers/v1/v1_ctrl_labelmaker.go
Comment on lines
+511
to
514
| output, err := command.CombinedOutput() | ||
| if err != nil { | ||
| log.Printf("label print command failed: %v\noutput: %s", err, string(output)) | ||
| return err |
Contributor
There was a problem hiding this comment.
Don’t log raw subprocess output verbatim.
This now persists the child’s full stdout/stderr into application logs on failure. Because the command inherits both the process environment and the new LABEL_* metadata, a failing script that dumps diagnostics can leak sensitive inventory data or credentials into logs. Prefer logging the exit error plus a capped/redacted stderr excerpt instead of the full combined stream.
Suggested direction
- output, err := command.CombinedOutput()
+ output, err := command.CombinedOutput()
if err != nil {
- log.Printf("label print command failed: %v\noutput: %s", err, string(output))
+ const maxLogBytes = 4096
+ logOutput := string(output)
+ if len(logOutput) > maxLogBytes {
+ logOutput = logOutput[:maxLogBytes] + "…(truncated)"
+ }
+ log.Printf("label print command failed: %v; output: %q", err, logOutput)
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| output, err := command.CombinedOutput() | |
| if err != nil { | |
| log.Printf("label print command failed: %v\noutput: %s", err, string(output)) | |
| return err | |
| output, err := command.CombinedOutput() | |
| if err != nil { | |
| const maxLogBytes = 4096 | |
| logOutput := string(output) | |
| if len(logOutput) > maxLogBytes { | |
| logOutput = logOutput[:maxLogBytes] + "…(truncated)" | |
| } | |
| log.Printf("label print command failed: %v; output: %q", err, logOutput) | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/pkgs/labelmaker/labelmaker.go` around lines 511 - 514, The current
failure path around command.CombinedOutput() logs the entire child stdout/stderr
(variables output and err); change this to log only the exit error plus a
redacted/capped excerpt of the combined output instead of the full stream.
Specifically, in the block handling output, capture command.CombinedOutput() as
before but when err != nil, create a short excerpt (e.g., take the last N bytes
or first N bytes, replace newlines with spaces, and redact any LABEL_* tokens or
other sensitive-looking patterns) and log a message with err plus that
capped/redacted excerpt; do not include the full output variable in logs. Apply
this change to the failure branch that currently prints "label print command
failed" so only the safer, capped excerpt is logged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Why we need it:
This PR builds on the current
HBOX_LABEL_MAKER_PRINT_COMMANDconfiguration.The feature has the following issues:
{{.FileName}}, technically everything inhomebox/backend/pkgs/labelmaker/labelmaker.go
Line 462 in 139edf1
What this PR does
Special notes for your reviewer:
Anything I should add in the docs I added? Is there a way to see a preview of the doc website to make sure my links work?
Should I add parent locations to locations as well?
Testing
I tested this by building the docker container locally and configuring a print script. See the "these don't need to be in the PR" commit for my changes I used for local testing.
I have images with the python libraries needed for the brother ptouch label makers here that I am testing it with: https://github.com/Trevor159/homebox-brother-pt