diff --git a/internal/authlog/authlog.go b/internal/authlog/authlog.go index 2ea988d47..129ddf944 100644 --- a/internal/authlog/authlog.go +++ b/internal/authlog/authlog.go @@ -137,6 +137,18 @@ func (l *Logger) logDir() string { if err == nil { return safeDir } + // The caller asked for a specific directory and it was rejected. Staying + // quiet would send the logs elsewhere while they keep watching the path + // they configured. This fires only on a rejected override, and logDir + // resolves once per process. + // + //nolint:forbidigo // leaf package with no IOStreams in scope; keychain + // surfaces its own directory warning the same way. + fmt.Fprintf( + os.Stderr, + "[lark-cli] [WARN] LARKSUITE_CLI_LOG_DIR is unusable (%v); writing auth logs under the default directory instead\n", + err, + ) } return filepath.Join(l.runtimeDir(), "logs") @@ -194,16 +206,38 @@ func (l *Logger) close() error { return file.Close() } +// FormatAuthCmdline renders the command path for the log and stops at the first +// flag. +// +// Everything from the first flag onward is dropped rather than filtered by name. +// A denylist of sensitive flags has to be extended whenever one is added, and +// the rule it replaced — keep the first three arguments — only held while no +// sensitive flag happened to appear early. Neither survives contact with a flag +// nobody remembered to classify. What the log needs is which command ran, and +// that is entirely in the leading non-flag arguments. +// +// args[0] is reduced to its base name so an absolute install path does not end +// up in the file either. func FormatAuthCmdline(args []string) string { if len(args) == 0 { return "" } - if len(args) <= 3 { - return strings.Join(args, " ") + path := []string{filepath.Base(args[0])} + dropped := false + for _, arg := range args[1:] { + if strings.HasPrefix(arg, "-") { + dropped = true + break + } + path = append(path, arg) } - return strings.Join(args[:3], " ") + " ..." + line := strings.Join(path, " ") + if dropped { + line += " ..." + } + return line } // LogResponse records one authentication HTTP response. diff --git a/internal/authlog/authlog_test.go b/internal/authlog/authlog_test.go index deab489f8..e4c3bcde0 100644 --- a/internal/authlog/authlog_test.go +++ b/internal/authlog/authlog_test.go @@ -5,6 +5,8 @@ package authlog import ( "errors" + "io" + "os" "path/filepath" "strings" "testing" @@ -197,6 +199,112 @@ func TestSetShared_ReleasesTheSupersededFallback(t *testing.T) { } } +// TestFormatAuthCmdline_DropsEverythingFromTheFirstFlag is the regression guard +// for the rule this replaced. Keeping the first three arguments was safe only +// while no sensitive flag appeared early; a global flag in front of the +// subcommand put its value straight into the file. +func TestFormatAuthCmdline_DropsEverythingFromTheFirstFlag(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + { + name: "flag before the subcommand", + args: []string{"lark-cli", "--token=super-secret", "auth", "login"}, + want: "lark-cli ...", + }, + { + name: "separated flag value", + args: []string{"lark-cli", "auth", "login", "--device-code", "device-secret"}, + want: "lark-cli auth login ...", + }, + { + name: "absolute install path is reduced to the binary name", + args: []string{"/opt/internal-tools/build-42/lark-cli", "auth", "status"}, + want: "lark-cli auth status", + }, + { + name: "no flags at all", + args: []string{"lark-cli", "auth", "status"}, + want: "lark-cli auth status", + }, + { + name: "empty", + args: nil, + want: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := FormatAuthCmdline(tc.args) + if got != tc.want { + t.Fatalf("FormatAuthCmdline(%q) = %q, want %q", tc.args, got, tc.want) + } + for _, arg := range tc.args { + if strings.Contains(arg, "secret") && strings.Contains(got, "secret") { + t.Fatalf("FormatAuthCmdline leaked %q into %q", arg, got) + } + } + }) + } +} + +// TestAuthLogDir_RejectedOverrideWarns covers the one case where staying silent +// misleads: the caller set the directory explicitly and it was refused. +func TestAuthLogDir_RejectedOverrideWarns(t *testing.T) { + t.Setenv("LARKSUITE_CLI_LOG_DIR", "relative-logs") + configDir := t.TempDir() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir) + + logger := New(Options{RuntimeDir: func() string { return configDir }}) + warning := captureStderr(t, func() { _ = logger.logDir() }) + + for _, want := range []string{"LARKSUITE_CLI_LOG_DIR", "default directory"} { + if !strings.Contains(warning, want) { + t.Errorf("warning %q does not mention %q", warning, want) + } + } +} + +// TestAuthLogDir_AcceptedOverrideStaysQuiet keeps the warning scoped to the +// failure: a usable override must not print anything. +func TestAuthLogDir_AcceptedOverrideStaysQuiet(t *testing.T) { + base := t.TempDir() + base, _ = filepath.EvalSymlinks(base) + t.Setenv("LARKSUITE_CLI_LOG_DIR", filepath.Join(base, "auth")) + + logger := New(Options{RuntimeDir: func() string { return base }}) + if warning := captureStderr(t, func() { _ = logger.logDir() }); warning != "" { + t.Fatalf("a usable override printed %q", warning) + } +} + +// captureStderr redirects os.Stderr for the duration of fn and returns what was +// written to it. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + + reader, writer, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + previous := os.Stderr + os.Stderr = writer + defer func() { os.Stderr = previous }() + + fn() + if err := writer.Close(); err != nil { + t.Fatalf("close writer: %v", err) + } + out, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("read captured stderr: %v", err) + } + return string(out) +} + // restoreShared isolates each test from the process-wide logger. func restoreShared(t *testing.T) { t.Helper()