Skip to content

cli-plugins/hooks: fix max-message off-by-one #7006

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
fallintoplace wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cli-plugins/hooks: fix max-message off-by-one #7006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli-plugins/hooks/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func ParseTemplate(hookTemplate string, cmd *cobra.Command) ([]string, error) {
}
out = b.String()
}
if n := strings.Count(out, "\n"); n > maxMessages {
if n := strings.Count(out, "\n") + 1; n > maxMessages {

@docker-agent docker-agent Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[medium] Trailing-newline output incorrectly rejected: +1 overcounts messages when output ends with \n

The fix changes the guard from strings.Count(out, "\n") > maxMessages to strings.Count(out, "\n") + 1 > maxMessages. This is correct for output that does not end with a trailing newline, but it overcounts when output does end with \n.

Trigger path: A template that renders to exactly 10 messages each followed by a newline — e.g., "msg1\nmsg2\n...\nmsg10\n" — has 10 newline characters. The new guard computes n = 10 + 1 = 11 > 10 = maxMessages and rejects the output with the error "hook template contains too many messages (11): maximum is 10". Yet the template only produced 10 real messages; the 11th "segment" from strings.Split is just an empty string after the final \n.

Impact: Valid 10-message templates whose rendered output ends with a trailing newline (common when Go text/template templates place a literal newline after the last field) are incorrectly refused. The regression test added in this PR uses strings.Repeat("line\n", 10)+"line" (no trailing newline), so it does not catch this edge case.

Suggested fix: Trim a trailing newline before counting, so an empty final segment is not treated as an extra message:

Suggested change
if n := strings.Count(out, "\n") + 1; n > maxMessages {
if n := strings.Count(strings.TrimRight(out, "\n"), "\n") + 1; n > maxMessages {
Confidence Score
🟢 strong 100/100

return nil, fmt.Errorf("hook template contains too many messages (%d): maximum is %d", n, maxMessages)
}
return strings.SplitN(out, "\n", maxMessages), nil
Expand Down
8 changes: 8 additions & 0 deletions cli-plugins/hooks/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package hooks_test

import (
"strings"
"testing"

"github.com/docker/cli/cli-plugins/hooks"
Expand Down Expand Up @@ -123,3 +124,10 @@ func TestParseTemplate(t *testing.T) {
})
}
}

func TestParseTemplateTooManyMessages(t *testing.T) {
testCmd := &cobra.Command{Use: "pull"}

_, err := hooks.ParseTemplate(strings.Repeat("line\n", 10)+"line", testCmd)
assert.Error(t, err, "hook template contains too many messages (11): maximum is 10")
}
Loading