From c0730b46bf315ecd3683165279b2590530725c72 Mon Sep 17 00:00:00 2001 From: AlbertSun Date: Sat, 13 Jun 2026 20:32:16 +0800 Subject: [PATCH] feat: simplify proxy plugin warning and gate on tty (#1448) --- internal/cmdutil/factory_default.go | 16 +++- internal/cmdutil/factory_proxy_warn_test.go | 85 +++++++++++++++++++++ internal/cmdutil/iostreams.go | 17 ++++- internal/transport/warn.go | 14 +--- internal/transport/warn_test.go | 58 ++++---------- 5 files changed, 128 insertions(+), 62 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 514aaf93..2051e0be 100644 --- a/internal/cmdutil/factory_default.go +++ b/internal/cmdutil/factory_default.go @@ -100,9 +100,19 @@ func safeRedirectPolicy(req *http.Request, via []*http.Request) error { return nil } +// 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) { - transport.WarnIfProxied(f.IOStreams.ErrOut) + if f.IOStreams.StderrIsTerminal { + warnIfProxied(f.IOStreams.ErrOut) + } var rt http.RoundTripper = transport.Shared() rt = &RetryTransport{Base: rt} @@ -129,7 +139,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 { + warnIfProxied(f.IOStreams.ErrOut) + } opts = append(opts, lark.WithHttpClient(&http.Client{ Transport: buildSDKTransport(), CheckRedirect: safeRedirectPolicy, diff --git a/internal/cmdutil/factory_proxy_warn_test.go b/internal/cmdutil/factory_proxy_warn_test.go new file mode 100644 index 00000000..ffdeb488 --- /dev/null +++ b/internal/cmdutil/factory_proxy_warn_test.go @@ -0,0 +1,85 @@ +// 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 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 := warnIfProxied + t.Cleanup(func() { warnIfProxied = prevWarn }) + calls := 0 + warnIfProxied = func(io.Writer) { calls++ } + 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) + + fn := cachedHttpClientFunc(&Factory{IOStreams: &IOStreams{ + ErrOut: io.Discard, StderrIsTerminal: tc.terminal, + }}) + 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) + + // 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) + } + + if *calls != tc.want { + t.Errorf("WarnIfProxied calls = %d, want %d", *calls, tc.want) + } + }) + } +} diff --git a/internal/cmdutil/iostreams.go b/internal/cmdutil/iostreams.go index 864300f5..a067b67d 100644 --- a/internal/cmdutil/iostreams.go +++ b/internal/cmdutil/iostreams.go @@ -18,17 +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 := 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. diff --git a/internal/transport/warn.go b/internal/transport/warn.go index cac050f7..2971b2be 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 13708ca7..a4e10b8d 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) } - if !strings.Contains(out, envvars.CliProxyEnable) { - t.Errorf("warning should mention %s as the disable method, got: %s", envvars.CliProxyEnable, 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, "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.