diff --git a/db_lib/CmdGitClient.go b/db_lib/CmdGitClient.go index aed436b92e..243e80c221 100644 --- a/db_lib/CmdGitClient.go +++ b/db_lib/CmdGitClient.go @@ -156,8 +156,6 @@ func (c CmdGitClient) GetLastCommitMessage(r GitRepository) (msg string, err err return } - msg = truncateCommitMessage(msg) - return } diff --git a/db_lib/GoGitClient.go b/db_lib/GoGitClient.go index cfb7a1aa82..b0d0384877 100644 --- a/db_lib/GoGitClient.go +++ b/db_lib/GoGitClient.go @@ -13,6 +13,7 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport/ssh" "github.com/go-git/go-git/v5/storage/memory" "github.com/semaphoreui/semaphore/db" + "github.com/semaphoreui/semaphore/pkg/conv" "github.com/semaphoreui/semaphore/pkg/task_logger" "github.com/semaphoreui/semaphore/util" @@ -230,9 +231,9 @@ func (c GoGitClient) GetLastCommitMessage(r GitRepository) (msg string, err erro return } - msg = truncateCommitMessage(headCommit.Message) + msg = headCommit.Message - r.Logger.Log("Message: " + msg) + r.Logger.Log("Message: " + conv.TruncateValidUTF8(msg)) return } diff --git a/db_lib/commit_message.go b/db_lib/commit_message.go deleted file mode 100644 index 7c3756c685..0000000000 --- a/db_lib/commit_message.go +++ /dev/null @@ -1,13 +0,0 @@ -package db_lib - -// Matches `commit_message VARCHAR(100)` in db/sql/migrations. -const commitMessageMaxRunes = 100 - -// Slice by runes โ€” byte slicing can split a multi-byte UTF-8 sequence. -func truncateCommitMessage(msg string) string { - runes := []rune(msg) - if len(runes) > commitMessageMaxRunes { - return string(runes[:commitMessageMaxRunes]) - } - return msg -} diff --git a/db_lib/commit_message_test.go b/db_lib/commit_message_test.go deleted file mode 100644 index c99354eb5b..0000000000 --- a/db_lib/commit_message_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package db_lib - -import ( - "strings" - "testing" - "unicode/utf8" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestTruncateCommitMessage_KeepsFullString(t *testing.T) { - tests := []struct { - name string - input string - }{ - {"empty", ""}, - {"short ASCII", "fix: short"}, - {"100 ASCII", strings.Repeat("a", 100)}, - {"100 Cyrillic", strings.Repeat("ั‹", 100)}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.input, truncateCommitMessage(tt.input)) - }) - } -} - -func TestTruncateCommitMessage_Truncates(t *testing.T) { - tests := []struct { - name string - input string - want string - }{ - {"101 ASCII", strings.Repeat("a", 101), strings.Repeat("a", 100)}, - {"150 Cyrillic", strings.Repeat("ั‹", 150), strings.Repeat("ั‹", 100)}, - {"150 emoji", strings.Repeat("๐Ÿ™‚", 150), strings.Repeat("๐Ÿ™‚", 100)}, - {"99 ASCII + 3 Cyrillic", strings.Repeat("a", 99) + "ั‹ั‹ั‹", strings.Repeat("a", 99) + "ั‹"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, truncateCommitMessage(tt.input)) - }) - } -} - -func TestLegacyByteSliceProducesInvalidUTF8(t *testing.T) { - msg := "Hello ะผะธั€, ฮณฮตฮนฮฌ ฮบฯŒฯƒฮผฮต, ู…ุฑุญุจุง ุจุงู„ุนุงู„ู…, ืฉืœื•ื ืขื•ืœื, ไฝ ๅฅฝไธ–็•Œ, ใ“ใ‚“ใซใกใฏ ์•ˆ๋…•ํ•˜์„ธ์š” ๐ŸŒ๐ŸŽ‰" - - require.Greater(t, len(msg), 100) - assert.False(t, utf8.ValidString(msg[:100])) -} diff --git a/pkg/conv/conv.go b/pkg/conv/conv.go index d623b18c8f..e02893711c 100644 --- a/pkg/conv/conv.go +++ b/pkg/conv/conv.go @@ -3,8 +3,58 @@ package conv import ( "reflect" "strings" + "unicode/utf8" ) + +const commitMessageMaxRunes = 100 + +// TruncateValidUTF8 sanitizes s so it can be stored in the task.commit_message +// column (VARCHAR(100)): NUL bytes are removed, invalid UTF-8 byte sequences are +// replaced with U+FFFD, and the result is capped at commitMessageMaxRunes runes. +// +// This is not a general-purpose string helper: the hard cap is tied to the +// commit-message column width, so reusing it elsewhere would truncate at 100 +// runes unexpectedly. +// +// It guards two failure modes that both make PostgreSQL reject the value with +// `invalid byte sequence for encoding "UTF8"` and abort the transaction: +// - Byte slicing can split a multi-byte character; slicing by runes cannot. +// - Text originating from a non-UTF-8 encoding (e.g. latin-1) can already +// contain invalid bytes even when short, so it is sanitized regardless of +// length. +func TruncateValidUTF8(s string) string { + // Decode at most the first commitMessageMaxRunes runes we intend to keep. + // Converting the whole (possibly very large) message to []rune just to + // discard all but the leading runes wastes CPU and allocations, since + // callers no longer truncate before reaching here. + var b strings.Builder + // Cap the initial allocation: the kept runes never exceed + // commitMessageMaxRunes*utf8.UTFMax bytes, and shorter inputs need even less. + grow := commitMessageMaxRunes * utf8.UTFMax + if len(s) < grow { + grow = len(s) + } + b.Grow(grow) + + count := 0 + // Ranging over a string decodes runes and yields U+FFFD for invalid UTF-8 + // bytes (matching a []rune conversion), so the result is always storable as + // UTF-8 without splitting a multi-byte character. + for _, r := range s { + // PostgreSQL text/varchar cannot store NUL, even though it is valid UTF-8. + if r == 0 { + continue + } + b.WriteRune(r) + count++ + if count == commitMessageMaxRunes { + break + } + } + return b.String() +} + func ConvertFloatToIntIfPossible(v any) (int64, bool) { switch v := v.(type) { diff --git a/pkg/conv/conv_test.go b/pkg/conv/conv_test.go new file mode 100644 index 0000000000..0e50b034b2 --- /dev/null +++ b/pkg/conv/conv_test.go @@ -0,0 +1,79 @@ +package conv + +import ( + "strings" + "testing" + "unicode/utf8" + + "github.com/stretchr/testify/assert" +) + +func TestTruncateValidUTF8_KeepsValidInput(t *testing.T) { + tests := []struct { + name string + in string + }{ + {"empty", ""}, + {"short ASCII", "fix: short"}, + {"exactly max ASCII", strings.Repeat("a", 100)}, + {"exactly max Cyrillic", strings.Repeat("ั‹", 100)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := TruncateValidUTF8(tt.in) + assert.Equal(t, tt.in, out) + assert.True(t, utf8.ValidString(out)) + }) + } +} + +func TestTruncateValidUTF8_CapsByRunes(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"ASCII over limit", strings.Repeat("a", 101), strings.Repeat("a", 100)}, + {"Cyrillic over limit", strings.Repeat("ั‹", 150), strings.Repeat("ั‹", 100)}, + {"emoji over limit", strings.Repeat("๐Ÿ™‚", 150), strings.Repeat("๐Ÿ™‚", 100)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := TruncateValidUTF8(tt.in) + assert.Equal(t, tt.want, out) + assert.Equal(t, 100, utf8.RuneCountInString(out)) + assert.True(t, utf8.ValidString(out)) + }) + } +} + +// A short message authored in a non-UTF-8 encoding (latin-1 'รฉ' == 0xE9) must be +// sanitized even though it is well under the length limit โ€” this is the case a +// pure rune-truncation misses. +func TestTruncateValidUTF8_SanitizesShortInvalid(t *testing.T) { + in := "caf\xe9" // 0xE9 is a lone byte, invalid UTF-8 + + assert.False(t, utf8.ValidString(in)) + + out := TruncateValidUTF8(in) + assert.True(t, utf8.ValidString(out)) + assert.Equal(t, "caf๏ฟฝ", out) +} + +func TestTruncateValidUTF8_StripsNUL(t *testing.T) { + out := TruncateValidUTF8("fix\x00patch") + + assert.NotContains(t, out, "\x00") + assert.Equal(t, "fixpatch", out) + assert.True(t, utf8.ValidString(out)) +} + +func TestTruncateValidUTF8_SanitizesThenCaps(t *testing.T) { + // Invalid byte inside a message longer than the limit: result must be both + // valid UTF-8 and within the rune cap. + in := strings.Repeat("a", 60) + "\xe9" + strings.Repeat("b", 60) + + out := TruncateValidUTF8(in) + assert.True(t, utf8.ValidString(out)) + assert.Equal(t, 100, utf8.RuneCountInString(out)) +} \ No newline at end of file diff --git a/services/tasks/TaskRunner_logging.go b/services/tasks/TaskRunner_logging.go index e7a705fcba..e32f882886 100644 --- a/services/tasks/TaskRunner_logging.go +++ b/services/tasks/TaskRunner_logging.go @@ -11,6 +11,7 @@ import ( "github.com/semaphoreui/semaphore/pkg/tz" "github.com/semaphoreui/semaphore/api/sockets" + "github.com/semaphoreui/semaphore/pkg/conv" "github.com/semaphoreui/semaphore/pkg/task_logger" "github.com/semaphoreui/semaphore/util" log "github.com/sirupsen/logrus" @@ -72,7 +73,12 @@ func (t *TaskRunner) WaitLog() { func (t *TaskRunner) SetCommit(hash, message string) { t.Task.CommitHash = &hash - t.Task.CommitMessage = message + // Sanitize before persisting to the DB. This is the persistence point for + // remote-runner commit reports; local tasks reach it too via the task logger + // (LocalExecutor.SetCommit -> Logger.SetCommit), and that local mirror + // additionally sanitizes the copy it exposes as task extra vars. + // See conv.TruncateValidUTF8. + t.Task.CommitMessage = conv.TruncateValidUTF8(message) if err := t.pool.store.UpdateTask(t.Task); err != nil { t.panicOnError(err, "Failed to update task commit") diff --git a/services/tasks/local_executor.go b/services/tasks/local_executor.go index a381761a3d..ba47b3c8dd 100644 --- a/services/tasks/local_executor.go +++ b/services/tasks/local_executor.go @@ -14,6 +14,7 @@ import ( "github.com/semaphoreui/semaphore/db" "github.com/semaphoreui/semaphore/db_lib" + "github.com/semaphoreui/semaphore/pkg/conv" "github.com/semaphoreui/semaphore/pkg/task_logger" "github.com/semaphoreui/semaphore/util" ) @@ -105,6 +106,12 @@ func (t *LocalExecutor) SetLogger(logger task_logger.Logger) { func (t *LocalExecutor) SetCommit(hash, message string) { // TODO: is this the correct place to do? + // Sanitize the commit message: it comes straight from the external git repo + // and may contain NUL/invalid UTF-8 bytes or be arbitrarily large. Without + // this the raw value reaches t.Task.CommitMessage and taskDetails + // ("commit_message"), i.e. task extra vars and API responses. Mirrors + // TaskRunner.SetCommit. See conv.TruncateValidUTF8. + message = conv.TruncateValidUTF8(message) t.Task.CommitHash = &hash t.Task.CommitMessage = message t.Logger.SetCommit(hash, message)