From dcb714dc193add0c1fe7e12b260cb3a965a2aa6d Mon Sep 17 00:00:00 2001 From: Craig Williams Date: Fri, 9 Oct 2026 17:58:48 +0100 Subject: [PATCH] Some bug fixes: - logging: LogToFile shadowed the package-level fileLogger, so IsFileLogger was always false and Shutdown never closed the file - logging: replace IsJSONHandler's unsafe reflection into slog internals with a *slog.JSONHandler type check - cli: fail if the log file can't be opened instead of silently logging to the terminal; return the log shutdown error from Finish - build.sh: -X flags used the wrong module path, so the version, build time and commit were never stamped --- internal/cli/options.go | 7 ++++--- internal/config/config.go | 2 +- internal/config/load_test.go | 2 +- internal/config/modify_test.go | 6 +++++- internal/config/section_test.go | 10 ++++----- internal/logging/logging.go | 36 +++++---------------------------- scripts/build.sh | 6 +++--- 7 files changed, 24 insertions(+), 45 deletions(-) diff --git a/internal/cli/options.go b/internal/cli/options.go index c3df76b..c4f37a8 100644 --- a/internal/cli/options.go +++ b/internal/cli/options.go @@ -1,6 +1,7 @@ package cli import ( + "fmt" "log/slog" "git.avsdev.uk/Go/job-search/internal/config" @@ -58,8 +59,8 @@ func (o *Options) RunSetup( if !terminalLog { if len(conf.Logging.File) == 0 { slog.Warn("No log file configured, remaining in console logging mode!") - } else { - logging.LogToFile(conf.Logging.File) + } else if err := logging.LogToFile(conf.Logging.File); err != nil { + return nil, fmt.Errorf("opening log file: %w", err) } } } @@ -70,7 +71,7 @@ func (o *Options) RunSetup( // Finish shuts down any open file logger func (o *Options) Finish() error { if logging.IsFileLogger() { - logging.Shutdown() + return logging.Shutdown() } return nil diff --git a/internal/config/config.go b/internal/config/config.go index 1814b19..4768d5c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -84,7 +84,7 @@ func (conf JobSearchConfig) String() string { var sb strings.Builder for k, v := range conf.Flatten() { typ, _ := conf.TypeOf(k) - sb.WriteString(fmt.Sprint(k, " = ", v, " <", typ, ">\n")) + fmt.Fprint(&sb, k, " = ", v, " <", typ, ">\n") } return sb.String() } diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 8dadfac..24876d4 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -32,7 +32,7 @@ func initDir(t *testing.T, env []string, config string) string { func writeFile(t *testing.T, path, contents string) { t.Helper() - if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { t.Fatal(err) } } diff --git a/internal/config/modify_test.go b/internal/config/modify_test.go index abba1f7..d54b168 100644 --- a/internal/config/modify_test.go +++ b/internal/config/modify_test.go @@ -16,7 +16,7 @@ func useConfigFile(t *testing.T, contents string) string { t.Helper() resetViper(t) path := filepath.Join(t.TempDir(), "config.yaml") - if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { t.Fatal(err) } viper.SetConfigFile(path) @@ -92,6 +92,7 @@ func TestStoreValueErrors(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { path := useConfigFile(t, "logging:\n level: warn\n") + //nolint:gosec // G304: reading a file the test created in t.TempDir before, _ := os.ReadFile(path) conf := defaultConfig(t) @@ -102,6 +103,7 @@ func TestStoreValueErrors(t *testing.T) { if !strings.Contains(err.Error(), tt.wantErr) { t.Errorf("err = %v; want it to mention %q", err, tt.wantErr) } + //nolint:gosec // G304: reading a file the test created in t.TempDir if after, _ := os.ReadFile(path); string(after) != string(before) { t.Errorf("config file changed:\n%s", after) } @@ -135,12 +137,14 @@ func TestPurgeValue(t *testing.T) { func TestPurgeValueErrors(t *testing.T) { path := useConfigFile(t, "logging:\n level: debug\n") + //nolint:gosec // G304: reading a file the test created in t.TempDir before, _ := os.ReadFile(path) conf := defaultConfig(t) if err := PurgeValue(&conf, "nope"); err == nil { t.Error("expected an error for an unknown key") } + //nolint:gosec // G304: reading a file the test created in t.TempDir if after, _ := os.ReadFile(path); string(after) != string(before) { t.Errorf("config file changed:\n%s", after) } diff --git a/internal/config/section_test.go b/internal/config/section_test.go index 0850398..2a0b6e2 100644 --- a/internal/config/section_test.go +++ b/internal/config/section_test.go @@ -19,14 +19,14 @@ type testEmbedded struct { type testSection struct { testEmbedded - Str string `cfg:"str" default:"hello"` - Int int `cfg:"int" default:"-42"` + Str string `cfg:"str" default:"hello"` + Int int `cfg:"int" default:"-42"` Int8 int8 `cfg:"int8"` - Uint uint `cfg:"uint" default:"7"` + Uint uint `cfg:"uint" default:"7"` Uint16 uint16 `cfg:"uint16"` - Float float64 `cfg:"float" default:"1.5"` + Float float64 `cfg:"float" default:"1.5"` Float32 float32 `cfg:"float32"` - Dur time.Duration `cfg:"dur" default:"90s"` + Dur time.Duration `cfg:"dur" default:"90s"` When time.Time `cfg:"when"` Slice []string `cfg:"slice"` Inner testInner `cfg:"inner"` diff --git a/internal/logging/logging.go b/internal/logging/logging.go index d2959ee..098ca63 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -3,8 +3,6 @@ package logging import ( "log/slog" "os" - "reflect" - "unsafe" "github.com/lmittmann/tint" ) @@ -33,7 +31,8 @@ func LogToFile(logfile string) error { } // Open the log file - fileLogger, err := os.OpenFile(logfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) + var err error + fileLogger, err = os.OpenFile(logfile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) if err != nil { return err } @@ -67,35 +66,10 @@ func LogToTerminal() { slog.SetDefault(logger) } +// IsJSONHandler reports whether h writes JSON, as the file logger does. func IsJSONHandler(h slog.Handler) bool { - val := reflect.ValueOf(h) - - // Ensure we are working with a pointer to a struct (*JSONHandler or *TextHandler or derived) - if val.Kind() != reflect.Pointer || val.Elem().Kind() != reflect.Struct { - return false - } - - // Both JSONHandler and TextHandler wrap an unexported field named 'commonHandler' - commonHandlerField := val.Elem().FieldByName("commonHandler") - if !commonHandlerField.IsValid() { - return false - } - - // Get the underlying *commonHandler struct pointer - // commonHandlerField is unexported, so we must use unsafe to bypass Go's safety checks - ptr := reflect.NewAt( - commonHandlerField.Type().Elem(), - unsafe.Pointer(commonHandlerField.Pointer()), - ) - - // Find the unexported 'json' boolean field inside commonHandler - jsonField := ptr.Elem().FieldByName("json") - if !jsonField.IsValid() { - return false - } - - // Read the boolean value using unsafe - return *(*bool)(unsafe.Pointer(jsonField.UnsafeAddr())) + _, ok := h.(*slog.JSONHandler) + return ok } func IsFileLogger() bool { diff --git a/scripts/build.sh b/scripts/build.sh index 77c6811..31c25bd 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -31,12 +31,12 @@ if [ "$VERSION" != "$VERSION_TAG" ]; then fi LD_FLAGS=${LD_FLAGS:-""} -LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/JobSearch/internal/version.Version=${VERSION}" +LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/job-search/internal/version.Version=${VERSION}" if [[ $DO_RELEASE -ne 1 ]]; then - LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/JobSearch/internal/version.BuildTime=${BUILD_TIME}" + LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/job-search/internal/version.BuildTime=${BUILD_TIME}" fi if [ "$GIT_COMMIT" != "" ]; then - LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/JobSearch/internal/version.GitCommit=${GIT_COMMIT}" + LD_FLAGS="$LD_FLAGS -X git.avsdev.uk/Go/job-search/internal/version.GitCommit=${GIT_COMMIT}" fi # Process any generators