Added sanitization of journald keys (#751)

Also, raised text coverage for the journald sub-package to 92.2%
Fixes #668

journald/journald.go

@@ -11,8 +11,9 @@ package journald
 
 // Zerolog's Top level key/Value Pairs are translated to
 // journald's args - all Values are sent to journald as strings.
-// And all key strings are converted to uppercase before sending
+// And all key strings are converted to uppercase and sanitized
-// to journald (as required by journald).
+// by replacing any characters not in [A-Z0-9_] with '_' before
+// sending to journald (as required by journald).
 
 // In addition, entire log message (all Key Value Pairs), is also
 // sent to journald under the key "JSON".
@@ -31,6 +32,12 @@ import (
 
 const defaultJournalDPrio = journal.PriNotice
 
+// SendFunc is the function used to send logs to journald.
+// It can be replaced in tests for mocking. If nil, journal.Send is used directly.
+// This variable should only be modified in tests and must not be changed while the
+// writer is in use. Tests that modify this variable should not use t.Parallel().
+var SendFunc func(string, journal.Priority, map[string]string) error
+
 // NewJournalDWriter returns a zerolog log destination
 // to be used as parameter to New() calls. Writing logs
 // to this writer will send the log messages to journalD
@@ -69,6 +76,24 @@ func levelToJPrio(zLevel string) journal.Priority {
 	return defaultJournalDPrio
 }
 
+// sanitizeKey converts a key to uppercase and replaces invalid characters with '_'
+// JournalD requires keys start with A-Z and contain only  A-Z, 0-9, or _
+func sanitizeKey(key string) string {
+	sanitized := strings.Map(func(r rune) rune {
+		if r >= 'a' && r <= 'z' {
+			return r - 'a' + 'A'
+		} else if (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' {
+			return r
+		} else {
+			return '_'
+		}
+	}, key)
+	if len(sanitized) == 0 || sanitized[0] >= '0' && sanitized[0] <= '9' || sanitized[0] == '_' {
+		sanitized = "X" + sanitized
+	}
+	return sanitized
+}
+
 func (w journalWriter) Write(p []byte) (n int, err error) {
 	var event map[string]interface{}
 	origPLen := len(p)
@@ -87,7 +112,7 @@ func (w journalWriter) Write(p []byte) (n int, err error) {
 
 	msg := ""
 	for key, value := range event {
-		jKey := strings.ToUpper(key)
+		jKey := sanitizeKey(key)
 		switch key {
 		case zerolog.LevelFieldName, zerolog.TimestampFieldName:
 			continue
@@ -111,7 +136,11 @@ func (w journalWriter) Write(p []byte) (n int, err error) {
 		}
 	}
 	args["JSON"] = string(p)
-	err = journal.Send(msg, jPrio, args)
+	if SendFunc != nil {
+		err = SendFunc(msg, jPrio, args)
+	} else {
+		err = journal.Send(msg, jPrio, args)
+	}
 
 	if err == nil {
 		n = origPLen

journald/journald_test.go

@@ -1,18 +1,21 @@
+//go:build linux
 // +build linux
 
-package journald_test
+package journald
 
 import (
 	"bytes"
+	"fmt"
 	"io"
+	"strings"
 	"testing"
 
+	"github.com/coreos/go-systemd/v22/journal"
 	"github.com/rs/zerolog"
-	"github.com/rs/zerolog/journald"
 )
 
 func ExampleNewJournalDWriter() {
-	log := zerolog.New(journald.NewJournalDWriter())
+	log := zerolog.New(NewJournalDWriter())
 	log.Info().Str("foo", "bar").Uint64("small", 123).Float64("float", 3.14).Uint64("big", 1152921504606846976).Msg("Journal Test")
 	// Output:
 }
@@ -49,9 +52,37 @@ Thu 2018-04-26 22:30:20.768136 PDT [s=3284d695bde946e4b5017c77a399237f;i=329f0;b
     _SOURCE_REALTIME_TIMESTAMP=1524807020768136
 */
 
+func TestSanitizeKey(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected string
+	}{
+		{"test", "TEST"},
+		{"Test", "TEST"},
+		{"test-key", "TEST_KEY"},
+		{"Test.Key", "TEST_KEY"},
+		{"test_key123", "TEST_KEY123"},
+		{"invalid@key!", "INVALID_KEY_"},
+		{"a1B2_c3D4", "A1B2_C3D4"},
+		{"_", "X_"},
+		{"", "X"},
+		{"123", "X123"},
+		{"a-b.c_d", "A_B_C_D"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			result := sanitizeKey(tt.input)
+			if result != tt.expected {
+				t.Errorf("sanitizeKey(%q) = %q; want %q", tt.input, result, tt.expected)
+			}
+		})
+	}
+}
+
 func TestWriteReturnsNoOfWrittenBytes(t *testing.T) {
 	input := []byte(`{"level":"info","time":1570912626,"message":"Starting..."}`)
-	wr := journald.NewJournalDWriter()
+	wr := NewJournalDWriter()
 	want := len(input)
 	got, err := wr.Write(input)
 
@@ -68,7 +99,7 @@ func TestMultiWrite(t *testing.T) {
 	var (
 		w1 = new(bytes.Buffer)
 		w2 = new(bytes.Buffer)
-		w3 = journald.NewJournalDWriter()
+		w3 = NewJournalDWriter()
 	)
 
 	zerolog.ErrorHandler = func(err error) {
@@ -84,3 +115,168 @@ func TestMultiWrite(t *testing.T) {
 		log.Info().Msg("Tick!")
 	}
 }
+
+func TestWriteWithVariousTypes(t *testing.T) {
+	mock := &mockSend{}
+	oldSend := SendFunc
+	SendFunc = mock.send
+	defer func() { SendFunc = oldSend }()
+
+	wr := NewJournalDWriter()
+	log := zerolog.New(wr)
+
+	// This should cover the default case in the switch for value types
+	log.Info().Bool("flag", true).Str("foo", "bar").Uint64("small", 123).Float64("float", 3.14).Uint64("big", 1152921504606846976).Interface("data", map[string]int{"a": 1}).Msg("Test various types")
+
+	// Verify the call
+	if len(mock.calls) != 1 {
+		t.Fatalf("Expected 1 call, got %d", len(mock.calls))
+	}
+
+	call := mock.calls[0]
+
+	// Check that flag is sanitized to FLAG and value is "true"
+	if call.args["FLAG"] != "true" {
+		t.Errorf("Expected FLAG=true, got %s", call.args["FLAG"])
+	}
+
+	// Check that data is marshaled (should be a JSON string)
+	expectedData := `{"a":1}`
+	if call.args["DATA"] != expectedData {
+		t.Errorf("Expected DATA=%q, got %q", expectedData, call.args["DATA"])
+	}
+}
+
+func TestWriteWithAllLevels(t *testing.T) {
+	wr := NewJournalDWriter()
+
+	// Save original FatalExitFunc
+	oldFatalExitFunc := zerolog.FatalExitFunc
+	defer func() { zerolog.FatalExitFunc = oldFatalExitFunc }()
+
+	// Set FatalExitFunc to prevent actual exit
+	zerolog.FatalExitFunc = func() {}
+
+	log := zerolog.New(wr)
+
+	// Test all zerolog levels to cover levelToJPrio switch cases
+	log.Trace().Msg("Trace level")
+	log.Debug().Msg("Debug level")
+	log.Info().Msg("Info level")
+	log.Warn().Msg("Warn level")
+	log.Error().Msg("Error level")
+	log.Log().Msg("No level")
+
+	// For Fatal, it will call FatalExitFunc instead of exiting
+	log.Fatal().Msg("Fatal level")
+
+	// For Panic, use recover to catch the panic, do last because it will stop of this test execution
+	defer func() {
+		if r := recover(); r == nil {
+			t.Error("Expected panic from Panic level")
+		}
+	}()
+	log.Panic().Msg("Panic level")
+
+}
+
+func TestWriteOutputs(t *testing.T) {
+	mock := &mockSend{}
+	oldSend := SendFunc
+	SendFunc = mock.send
+	defer func() { SendFunc = oldSend }()
+
+	wr := NewJournalDWriter()
+	log := zerolog.New(wr)
+
+	// Log a message with various fields
+	log.Info().Str("test-key", "value").Int("number", 42).Msg("Test message")
+
+	// Check that SendFunc was called
+	if len(mock.calls) != 1 {
+		t.Fatalf("Expected 1 call to SendFunc, got %d", len(mock.calls))
+	}
+
+	call := mock.calls[0]
+
+	// Check message
+	if call.msg != "Test message" {
+		t.Errorf("Expected msg 'Test message', got %q", call.msg)
+	}
+
+	// Check priority
+	if call.prio != journal.PriInfo {
+		t.Errorf("Expected prio %d (PriInfo), got %d", journal.PriInfo, call.prio)
+	}
+
+	// Check args
+	expectedArgs := map[string]string{
+		"TEST_KEY": "value",
+		"NUMBER":   "42",
+		"JSON":     `{"level":"info","test-key":"value","number":42,"message":"Test message"}` + "\n",
+	}
+
+	for k, v := range expectedArgs {
+		if call.args[k] != v {
+			t.Errorf("Expected args[%q] = %q, got %q", k, v, call.args[k])
+		}
+	}
+
+	// Check that LEVEL is not in args (since it's skipped)
+	if _, ok := call.args["LEVEL"]; ok {
+		t.Error("LEVEL should not be in args")
+	}
+}
+
+func TestWriteWithMarshalError(t *testing.T) {
+	mock := &mockSend{}
+	oldSend := SendFunc
+	SendFunc = mock.send
+	defer func() { SendFunc = oldSend }()
+
+	// Save original marshal func
+	originalMarshal := zerolog.InterfaceMarshalFunc
+	defer func() { zerolog.InterfaceMarshalFunc = originalMarshal }()
+
+	// Set marshal func to fail
+	zerolog.InterfaceMarshalFunc = func(v interface{}) ([]byte, error) {
+		return nil, fmt.Errorf("fake error")
+	}
+
+	wr := NewJournalDWriter()
+	log := zerolog.New(wr)
+
+	// This should trigger the error handling in the default case
+	log.Info().Interface("data", map[string]int{"a": 1}).Msg("Test with error")
+
+	// Verify the call
+	if len(mock.calls) != 1 {
+		t.Fatalf("Expected 1 call, got %d", len(mock.calls))
+	}
+
+	call := mock.calls[0]
+
+	// Check that data has the error message
+	got := call.args["DATA"]
+	want := "error: fake error"
+	if !strings.Contains(got, want) {
+		t.Errorf("Expected DATA to contain %q, got %q", want, got)
+	}
+}
+
+type mockSend struct {
+	calls []struct {
+		msg  string
+		prio journal.Priority
+		args map[string]string
+	}
+}
+
+func (m *mockSend) send(msg string, prio journal.Priority, args map[string]string) error {
+	m.calls = append(m.calls, struct {
+		msg  string
+		prio journal.Priority
+		args map[string]string
+	}{msg, prio, args})
+	return nil
+}