From 7ac5ee2bca255da5a0fca70cc2b911c17c2a5b11 Mon Sep 17 00:00:00 2001 From: AlbertSun Date: Sat, 13 Jun 2026 16:32:14 +0800 Subject: [PATCH 1/3] feat: simplify proxy plugin warning and gate on tty Collapse the multi-line proxy plugin warning to a single concise line and suppress it entirely on non-interactive stderr (pipes, CI, agent runs). --- internal/cmdutil/factory_default.go | 8 +++- internal/cmdutil/iostreams.go | 9 +++++ internal/transport/warn.go | 14 +------ internal/transport/warn_test.go | 58 +++++++---------------------- 4 files changed, 30 insertions(+), 59 deletions(-) diff --git a/internal/cmdutil/factory_default.go b/internal/cmdutil/factory_default.go index 514aaf93fa..a11899de53 100644 --- a/internal/cmdutil/factory_default.go +++ b/internal/cmdutil/factory_default.go @@ -102,7 +102,9 @@ func safeRedirectPolicy(req *http.Request, via []*http.Request) error { func cachedHttpClientFunc(f *Factory) func() (*http.Client, error) { return sync.OnceValues(func() (*http.Client, error) { - transport.WarnIfProxied(f.IOStreams.ErrOut) + if f.IOStreams.StderrIsTerminal() { + transport.WarnIfProxied(f.IOStreams.ErrOut) + } var rt http.RoundTripper = transport.Shared() rt = &RetryTransport{Base: rt} @@ -129,7 +131,9 @@ func cachedLarkClientFunc(f *Factory) func() (*lark.Client, error) { lark.WithLogLevel(larkcore.LogLevelError), lark.WithHeaders(BaseSecurityHeaders()), } - transport.WarnIfProxied(f.IOStreams.ErrOut) + if f.IOStreams.StderrIsTerminal() { + transport.WarnIfProxied(f.IOStreams.ErrOut) + } opts = append(opts, lark.WithHttpClient(&http.Client{ Transport: buildSDKTransport(), CheckRedirect: safeRedirectPolicy, diff --git a/internal/cmdutil/iostreams.go b/internal/cmdutil/iostreams.go index 864300f524..3b77845577 100644 --- a/internal/cmdutil/iostreams.go +++ b/internal/cmdutil/iostreams.go @@ -31,6 +31,15 @@ func NewIOStreams(in io.Reader, out, errOut io.Writer) *IOStreams { return &IOStreams{In: in, Out: out, ErrOut: errOut, IsTerminal: isTerminal} } +// StderrIsTerminal reports whether ErrOut is an interactive terminal. Advisory +// warnings written to stderr (e.g. the proxy notice) gate on this so they stay +// out of non-interactive output (pipes, CI, agent runs). Buffers (tests) and +// redirects are not *os.File terminals, so they yield false. +func (s *IOStreams) StderrIsTerminal() bool { + f, ok := s.ErrOut.(*os.File) + return ok && term.IsTerminal(int(f.Fd())) +} + // SystemIO creates an IOStreams wired to the process's standard file descriptors. // //nolint:forbidigo // entry point for real stdio diff --git a/internal/transport/warn.go b/internal/transport/warn.go index cac050f724..2971b2be3d 100644 --- a/internal/transport/warn.go +++ b/internal/transport/warn.go @@ -10,8 +10,6 @@ import ( "os" "strings" "sync" - - "github.com/larksuite/cli/internal/envvars" ) // Proxy environment constants control shared transport proxy behavior. @@ -79,16 +77,8 @@ func WarnIfProxied(w io.Writer) { // Shared), so its warning and disable instructions take precedence. // Emitting the env-proxy warning here would be misleading: it tells the // user to set LARK_CLI_NO_PROXY=1, which does NOT disable the plugin proxy. - if addr, caPath, enabled := proxyPluginStatus(); enabled { - fmt.Fprintf(w, "[lark-cli] [WARN] proxy plugin enabled: all requests (including credentials) are forced through %s. To disable, set %s=false or remove %s.\n", - redactProxyURL(addr), envvars.CliProxyEnable, Path()) - if strings.TrimSpace(caPath) != "" { - // A custom CA means upstream TLS can be intercepted/inspected by - // the proxy (MITM). Surface it so the operator is aware traffic - // (including Bearer tokens) is decryptable on this host. - fmt.Fprintf(w, "[lark-cli] [WARN] proxy plugin trusts a custom CA (%s); TLS to upstreams can be intercepted/inspected by this proxy.\n", - caPath) - } + if _, _, enabled := proxyPluginStatus(); enabled { + fmt.Fprintln(w, "[lark-cli] [WARN] proxy plugin enabled: all requests are forced through proxy.") return } if os.Getenv(EnvNoProxy) != "" { diff --git a/internal/transport/warn_test.go b/internal/transport/warn_test.go index 13708ca7e4..a4e10b8dad 100644 --- a/internal/transport/warn_test.go +++ b/internal/transport/warn_test.go @@ -8,8 +8,6 @@ import ( "strings" "sync" "testing" - - "github.com/larksuite/cli/internal/envvars" ) // TestDetectProxyEnv verifies proxy environment detection priority and empty-state behavior. @@ -120,9 +118,9 @@ func TestWarnIfProxied_OnlyOnce(t *testing.T) { } // TestWarnIfProxied_ProxyPluginEnabled verifies that when proxy plugin mode is -// enabled, the warning describes the plugin proxy and the correct disable method -// (LARKSUITE_CLI_PROXY_ENABLE=false) instead of the misleading LARK_CLI_NO_PROXY -// instruction — even when env proxy and LARK_CLI_NO_PROXY are also set. +// enabled, the warning is a single concise line that does not leak the proxy +// address or give the misleading LARK_CLI_NO_PROXY disable instruction — even +// when env proxy and LARK_CLI_NO_PROXY are also set. func TestWarnIfProxied_ProxyPluginEnabled(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) unsetProxyPluginEnv(t) @@ -140,51 +138,24 @@ func TestWarnIfProxied_ProxyPluginEnabled(t *testing.T) { WarnIfProxied(&buf) out := buf.String() - if !strings.Contains(out, "127.0.0.1:3128") { - t.Errorf("warning should mention the plugin proxy address, got: %s", out) + if !strings.Contains(out, "proxy plugin enabled") { + t.Errorf("warning should announce proxy plugin enabled, got: %s", out) + } + // Single line only — no address, CA, or disable hints. + if strings.Count(out, "\n") != 1 { + t.Errorf("warning must be a single line, got: %s", out) } - if !strings.Contains(out, envvars.CliProxyEnable) { - t.Errorf("warning should mention %s as the disable method, got: %s", envvars.CliProxyEnable, out) + if strings.Contains(out, "127.0.0.1:3128") || strings.Contains(out, "corp-proxy") { + t.Errorf("warning must not leak the proxy address, got: %s", out) } if strings.Contains(out, "Set "+EnvNoProxy+"=1") { t.Errorf("warning must NOT give the misleading %s disable instruction when plugin is enabled, got: %s", EnvNoProxy, out) } - // No custom CA configured -> no interception warning. - if strings.Contains(out, "custom CA") { - t.Errorf("warning should not mention a custom CA when none is configured, got: %s", out) - } -} - -// TestWarnIfProxied_ProxyPluginCustomCAWarns verifies that when a custom CA is -// trusted, the warning surfaces the TLS-interception capability. -func TestWarnIfProxied_ProxyPluginCustomCAWarns(t *testing.T) { - t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) - unsetProxyPluginEnv(t) - proxyWarningOnce = sync.Once{} - - old := proxyPluginStatus - proxyPluginStatus = func() (string, string, bool) { - return "http://127.0.0.1:3128", "/etc/lark/extra_ca.pem", true - } - t.Cleanup(func() { proxyPluginStatus = old }) - - var buf bytes.Buffer - WarnIfProxied(&buf) - out := buf.String() - - if !strings.Contains(out, "custom CA") { - t.Errorf("warning should mention the custom CA, got: %s", out) - } - if !strings.Contains(out, "/etc/lark/extra_ca.pem") { - t.Errorf("warning should include the CA path, got: %s", out) - } - if !strings.Contains(out, "intercept") { - t.Errorf("warning should mention TLS interception, got: %s", out) - } } // TestWarnIfProxied_ProxyPluginEnabledRedactsCredentials verifies the plugin -// warning never leaks credentials embedded in the configured proxy address. +// warning never leaks credentials embedded in the configured proxy address — +// the simplified message omits the address entirely. func TestWarnIfProxied_ProxyPluginEnabledRedactsCredentials(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) unsetProxyPluginEnv(t) @@ -204,9 +175,6 @@ func TestWarnIfProxied_ProxyPluginEnabledRedactsCredentials(t *testing.T) { if strings.Contains(out, "user:") { t.Errorf("plugin warning leaked username, got: %s", out) } - if !strings.Contains(out, "***@127.0.0.1:3128") { - t.Errorf("plugin warning should contain redacted proxy URL, got: %s", out) - } } // TestRedactProxyURL verifies redaction of proxy credentials across supported formats. From 1a941ba60886d4c457e747e55e835ab1c05d1fb6 Mon Sep 17 00:00:00 2001 From: AlbertSun Date: Sat, 13 Jun 2026 16:44:26 +0800 Subject: [PATCH 2/3] test: add proxy-warning gate tests for client init --- internal/cmdutil/factory_default.go | 18 ++++- internal/cmdutil/factory_proxy_warn_test.go | 84 +++++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 internal/cmdutil/factory_proxy_warn_test.go diff --git a/internal/cmdutil/factory_default.go b/internal/cmdutil/factory_default.go index a11899de53..cc6de49103 100644 --- a/internal/cmdutil/factory_default.go +++ b/internal/cmdutil/factory_default.go @@ -100,10 +100,20 @@ func safeRedirectPolicy(req *http.Request, via []*http.Request) error { return nil } +// Seams for unit-testing the proxy-warning gate. Production wires them to the +// real implementations; tests substitute a predicate and a spy to verify the +// warning fires only on an interactive stderr (see factory_proxy_warn_test.go). +// StderrIsTerminal is a concrete method (real TTY only) and WarnIfProxied is a +// package function, so neither is otherwise injectable. +var ( + warnIfProxied = transport.WarnIfProxied + stderrIsTerminal = func(s *IOStreams) bool { return s.StderrIsTerminal() } +) + func cachedHttpClientFunc(f *Factory) func() (*http.Client, error) { return sync.OnceValues(func() (*http.Client, error) { - if f.IOStreams.StderrIsTerminal() { - transport.WarnIfProxied(f.IOStreams.ErrOut) + if stderrIsTerminal(f.IOStreams) { + warnIfProxied(f.IOStreams.ErrOut) } var rt http.RoundTripper = transport.Shared() @@ -131,8 +141,8 @@ func cachedLarkClientFunc(f *Factory) func() (*lark.Client, error) { lark.WithLogLevel(larkcore.LogLevelError), lark.WithHeaders(BaseSecurityHeaders()), } - if f.IOStreams.StderrIsTerminal() { - transport.WarnIfProxied(f.IOStreams.ErrOut) + if stderrIsTerminal(f.IOStreams) { + warnIfProxied(f.IOStreams.ErrOut) } opts = append(opts, lark.WithHttpClient(&http.Client{ Transport: buildSDKTransport(), diff --git a/internal/cmdutil/factory_proxy_warn_test.go b/internal/cmdutil/factory_proxy_warn_test.go new file mode 100644 index 0000000000..d9efec9ea5 --- /dev/null +++ b/internal/cmdutil/factory_proxy_warn_test.go @@ -0,0 +1,84 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmdutil + +import ( + "io" + "testing" + + _ "github.com/larksuite/cli/extension/credential/env" // registers the env-backed account provider + "github.com/larksuite/cli/internal/envvars" +) + +// installProxyWarnSpy swaps the package-level proxy-warning seams for one test: +// stderrIsTerminal is forced to `terminal`, and warnIfProxied is replaced with a +// counter. The real implementations are restored on cleanup. Returns a pointer +// to the call count so the caller can assert how many times the warning fired. +func installProxyWarnSpy(t *testing.T, terminal bool) *int { + t.Helper() + prevWarn, prevTTY := warnIfProxied, stderrIsTerminal + t.Cleanup(func() { + warnIfProxied, stderrIsTerminal = prevWarn, prevTTY + }) + calls := 0 + warnIfProxied = func(io.Writer) { calls++ } + stderrIsTerminal = func(*IOStreams) bool { return terminal } + return &calls +} + +var proxyWarnGateCases = []struct { + name string + terminal bool + want int +}{ + {"terminal stderr warns once", true, 1}, + {"non-terminal stderr stays silent", false, 0}, +} + +// TestCachedHttpClientFunc_ProxyWarnGate verifies the http-client init path +// invokes WarnIfProxied only when stderr is an interactive terminal. +func TestCachedHttpClientFunc_ProxyWarnGate(t *testing.T) { + for _, tc := range proxyWarnGateCases { + t.Run(tc.name, func(t *testing.T) { + calls := installProxyWarnSpy(t, tc.terminal) + + fn := cachedHttpClientFunc(&Factory{IOStreams: &IOStreams{ErrOut: io.Discard}}) + if _, err := fn(); err != nil { + t.Fatalf("http client init: %v", err) + } + + if *calls != tc.want { + t.Errorf("WarnIfProxied calls = %d, want %d", *calls, tc.want) + } + }) + } +} + +// TestCachedLarkClientFunc_ProxyWarnGate verifies the lark-client init path +// invokes WarnIfProxied only when stderr is an interactive terminal. The gate +// runs after ResolveAccount, so an env-backed credential is wired up to let +// account resolution succeed without network or config files. +func TestCachedLarkClientFunc_ProxyWarnGate(t *testing.T) { + for _, tc := range proxyWarnGateCases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(envvars.CliAppID, "env-app") + t.Setenv(envvars.CliAppSecret, "env-secret") + t.Setenv(envvars.CliDefaultAs, "") + t.Setenv(envvars.CliUserAccessToken, "") + t.Setenv(envvars.CliTenantAccessToken, "") + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + + calls := installProxyWarnSpy(t, tc.terminal) + + f := NewDefault(&IOStreams{ErrOut: io.Discard}, InvocationContext{}) + if _, err := cachedLarkClientFunc(f)(); err != nil { + t.Fatalf("lark client init: %v", err) + } + + if *calls != tc.want { + t.Errorf("WarnIfProxied calls = %d, want %d", *calls, tc.want) + } + }) + } +} From b74f5e247fd691a4383047860e320b02674c2403 Mon Sep 17 00:00:00 2001 From: AlbertSun Date: Sat, 13 Jun 2026 20:14:19 +0800 Subject: [PATCH 3/3] refactor: derive StderrIsTerminal as a precomputed field Mirror the existing IsTerminal field instead of a live method, computed once in NewIOStreams. This drops the stderrIsTerminal package-level test seam (the gate now reads the field directly); tests inject the field like bind_test.go does for IsTerminal. The warnIfProxied seam stays, since the real function is guarded by an internal sync.Once and cannot be counted across subtests. --- internal/cmdutil/factory_default.go | 20 +++++++------- internal/cmdutil/factory_proxy_warn_test.go | 29 +++++++++++---------- internal/cmdutil/iostreams.go | 26 +++++++++--------- 3 files changed, 38 insertions(+), 37 deletions(-) diff --git a/internal/cmdutil/factory_default.go b/internal/cmdutil/factory_default.go index cc6de49103..2051e0beaa 100644 --- a/internal/cmdutil/factory_default.go +++ b/internal/cmdutil/factory_default.go @@ -100,19 +100,17 @@ func safeRedirectPolicy(req *http.Request, via []*http.Request) error { return nil } -// Seams for unit-testing the proxy-warning gate. Production wires them to the -// real implementations; tests substitute a predicate and a spy to verify the -// warning fires only on an interactive stderr (see factory_proxy_warn_test.go). -// StderrIsTerminal is a concrete method (real TTY only) and WarnIfProxied is a -// package function, so neither is otherwise injectable. -var ( - warnIfProxied = transport.WarnIfProxied - stderrIsTerminal = func(s *IOStreams) bool { return s.StderrIsTerminal() } -) +// warnIfProxied is a test seam for the proxy-warning gate. Production wires it +// to transport.WarnIfProxied; tests swap in a spy to count invocations. It is +// needed because the real function is guarded by an internal sync.Once, so +// calling it directly would only fire on the first test (see +// factory_proxy_warn_test.go). The terminal check is the IOStreams +// .StderrIsTerminal field, which tests set directly. +var warnIfProxied = transport.WarnIfProxied func cachedHttpClientFunc(f *Factory) func() (*http.Client, error) { return sync.OnceValues(func() (*http.Client, error) { - if stderrIsTerminal(f.IOStreams) { + if f.IOStreams.StderrIsTerminal { warnIfProxied(f.IOStreams.ErrOut) } @@ -141,7 +139,7 @@ func cachedLarkClientFunc(f *Factory) func() (*lark.Client, error) { lark.WithLogLevel(larkcore.LogLevelError), lark.WithHeaders(BaseSecurityHeaders()), } - if stderrIsTerminal(f.IOStreams) { + if f.IOStreams.StderrIsTerminal { warnIfProxied(f.IOStreams.ErrOut) } opts = append(opts, lark.WithHttpClient(&http.Client{ diff --git a/internal/cmdutil/factory_proxy_warn_test.go b/internal/cmdutil/factory_proxy_warn_test.go index d9efec9ea5..ffdeb488fc 100644 --- a/internal/cmdutil/factory_proxy_warn_test.go +++ b/internal/cmdutil/factory_proxy_warn_test.go @@ -11,19 +11,16 @@ import ( "github.com/larksuite/cli/internal/envvars" ) -// installProxyWarnSpy swaps the package-level proxy-warning seams for one test: -// stderrIsTerminal is forced to `terminal`, and warnIfProxied is replaced with a -// counter. The real implementations are restored on cleanup. Returns a pointer -// to the call count so the caller can assert how many times the warning fired. -func installProxyWarnSpy(t *testing.T, terminal bool) *int { +// installProxyWarnSpy replaces warnIfProxied with a counter for one test and +// restores it on cleanup. Returns a pointer to the call count so the caller can +// assert how many times the warning fired. The terminal state is controlled via +// the IOStreams.StderrIsTerminal field, not a seam. +func installProxyWarnSpy(t *testing.T) *int { t.Helper() - prevWarn, prevTTY := warnIfProxied, stderrIsTerminal - t.Cleanup(func() { - warnIfProxied, stderrIsTerminal = prevWarn, prevTTY - }) + prevWarn := warnIfProxied + t.Cleanup(func() { warnIfProxied = prevWarn }) calls := 0 warnIfProxied = func(io.Writer) { calls++ } - stderrIsTerminal = func(*IOStreams) bool { return terminal } return &calls } @@ -41,9 +38,11 @@ var proxyWarnGateCases = []struct { func TestCachedHttpClientFunc_ProxyWarnGate(t *testing.T) { for _, tc := range proxyWarnGateCases { t.Run(tc.name, func(t *testing.T) { - calls := installProxyWarnSpy(t, tc.terminal) + calls := installProxyWarnSpy(t) - fn := cachedHttpClientFunc(&Factory{IOStreams: &IOStreams{ErrOut: io.Discard}}) + fn := cachedHttpClientFunc(&Factory{IOStreams: &IOStreams{ + ErrOut: io.Discard, StderrIsTerminal: tc.terminal, + }}) if _, err := fn(); err != nil { t.Fatalf("http client init: %v", err) } @@ -69,9 +68,11 @@ func TestCachedLarkClientFunc_ProxyWarnGate(t *testing.T) { t.Setenv(envvars.CliTenantAccessToken, "") t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) - calls := installProxyWarnSpy(t, tc.terminal) + calls := installProxyWarnSpy(t) - f := NewDefault(&IOStreams{ErrOut: io.Discard}, InvocationContext{}) + // normalizeStreams copies the struct (out := *s), so the + // StderrIsTerminal field survives into f.IOStreams. + f := NewDefault(&IOStreams{ErrOut: io.Discard, StderrIsTerminal: tc.terminal}, InvocationContext{}) if _, err := cachedLarkClientFunc(f)(); err != nil { t.Fatalf("lark client init: %v", err) } diff --git a/internal/cmdutil/iostreams.go b/internal/cmdutil/iostreams.go index 3b77845577..a067b67da4 100644 --- a/internal/cmdutil/iostreams.go +++ b/internal/cmdutil/iostreams.go @@ -18,26 +18,28 @@ type IOStreams struct { Out io.Writer ErrOut io.Writer IsTerminal bool + // StderrIsTerminal reports whether ErrOut is an interactive terminal. + // Advisory warnings written to stderr (e.g. the proxy notice) gate on this + // so they stay out of non-interactive output (pipes, CI, agent runs). + // Computed once in NewIOStreams, mirroring IsTerminal; tests assign it + // directly like cmd/config/bind_test.go does for IsTerminal. + StderrIsTerminal bool } // NewIOStreams builds an IOStreams from arbitrary readers/writers. -// IsTerminal is derived from in's underlying *os.File, if any; non-file -// readers (bytes.Buffer, strings.Reader, …) yield IsTerminal=false. +// IsTerminal / StderrIsTerminal are derived from in's / errOut's underlying +// *os.File, if any; non-file streams (bytes.Buffer, strings.Reader, …) yield +// false. func NewIOStreams(in io.Reader, out, errOut io.Writer) *IOStreams { isTerminal := false if f, ok := in.(*os.File); ok { isTerminal = term.IsTerminal(int(f.Fd())) } - return &IOStreams{In: in, Out: out, ErrOut: errOut, IsTerminal: isTerminal} -} - -// StderrIsTerminal reports whether ErrOut is an interactive terminal. Advisory -// warnings written to stderr (e.g. the proxy notice) gate on this so they stay -// out of non-interactive output (pipes, CI, agent runs). Buffers (tests) and -// redirects are not *os.File terminals, so they yield false. -func (s *IOStreams) StderrIsTerminal() bool { - f, ok := s.ErrOut.(*os.File) - return ok && term.IsTerminal(int(f.Fd())) + stderrIsTerminal := false + if f, ok := errOut.(*os.File); ok { + stderrIsTerminal = term.IsTerminal(int(f.Fd())) + } + return &IOStreams{In: in, Out: out, ErrOut: errOut, IsTerminal: isTerminal, StderrIsTerminal: stderrIsTerminal} } // SystemIO creates an IOStreams wired to the process's standard file descriptors.