From b9c129405e142d41ff73f823f29cf8afa1a662da Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Tue, 26 May 2026 20:55:00 +0800 Subject: [PATCH 1/7] feat: add FetchTAT helper for direct TAT acquisition --- internal/credential/tat_fetch.go | 81 +++++++++++ internal/credential/tat_fetch_test.go | 197 ++++++++++++++++++++++++++ 2 files changed, 278 insertions(+) create mode 100644 internal/credential/tat_fetch.go create mode 100644 internal/credential/tat_fetch_test.go diff --git a/internal/credential/tat_fetch.go b/internal/credential/tat_fetch.go new file mode 100644 index 000000000..4af672b0e --- /dev/null +++ b/internal/credential/tat_fetch.go @@ -0,0 +1,81 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package credential + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + + "github.com/larksuite/cli/internal/core" +) + +// HTTPStatusError indicates the TAT request returned a non-2xx HTTP status. +// Callers can use errors.As to inspect the status code. +type HTTPStatusError struct{ StatusCode int } + +func (e *HTTPStatusError) Error() string { + return fmt.Sprintf("TAT API returned HTTP %d", e.StatusCode) +} + +// FetchTAT performs a single HTTP POST to obtain a tenant access token using +// the provided credentials. It does not read configuration or keychain. +// +// Return values: +// +// - token: tenant access token when err is nil +// - apiCode: TAT response `code` field; 0 unless the request was 2xx and +// the body parsed successfully into a structured response +// - err: non-nil for non-zero apiCode, non-2xx HTTP, transport errors, +// or JSON parse failures +// +// The caller is responsible for context timeouts. +func FetchTAT( + ctx context.Context, + httpClient *http.Client, + brand core.LarkBrand, + appID, appSecret string, +) (token string, apiCode int, err error) { + ep := core.ResolveEndpoints(brand) + url := ep.Open + "/open-apis/auth/v3/tenant_access_token/internal" + + body, err := json.Marshal(map[string]string{ + "app_id": appID, + "app_secret": appSecret, + }) + if err != nil { + return "", 0, fmt.Errorf("marshal TAT request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + if err != nil { + return "", 0, err + } + req.Header.Set("Content-Type", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return "", 0, err + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return "", 0, &HTTPStatusError{StatusCode: resp.StatusCode} + } + + var parsed struct { + Code int `json:"code"` + Msg string `json:"msg"` + TenantAccessToken string `json:"tenant_access_token"` + } + if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil { + return "", 0, fmt.Errorf("parse TAT response: %w", err) + } + if parsed.Code != 0 { + return "", parsed.Code, fmt.Errorf("TAT API error: [%d] %s", parsed.Code, parsed.Msg) + } + return parsed.TenantAccessToken, 0, nil +} diff --git a/internal/credential/tat_fetch_test.go b/internal/credential/tat_fetch_test.go new file mode 100644 index 000000000..4f3cee173 --- /dev/null +++ b/internal/credential/tat_fetch_test.go @@ -0,0 +1,197 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package credential + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/larksuite/cli/internal/core" +) + +// stubRoundTripper lets us assert request shape and return canned responses. +type stubRoundTripper struct { + gotReq *http.Request + gotBody string + respCode int + respBody string + err error +} + +func (s *stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + s.gotReq = req + if req.Body != nil { + b, _ := io.ReadAll(req.Body) + s.gotBody = string(b) + } + if s.err != nil { + return nil, s.err + } + return &http.Response{ + StatusCode: s.respCode, + Body: io.NopCloser(strings.NewReader(s.respBody)), + Header: make(http.Header), + }, nil +} + +func TestFetchTAT_Success(t *testing.T) { + rt := &stubRoundTripper{ + respCode: 200, + respBody: `{"code":0,"tenant_access_token":"t-abc","msg":"ok"}`, + } + hc := &http.Client{Transport: rt} + + token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if token != "t-abc" { + t.Errorf("token = %q, want t-abc", token) + } + if code != 0 { + t.Errorf("apiCode = %d, want 0", code) + } + if rt.gotReq.URL.String() != "https://open.feishu.cn/open-apis/auth/v3/tenant_access_token/internal" { + t.Errorf("url = %s", rt.gotReq.URL.String()) + } + if !strings.Contains(rt.gotBody, `"app_id":"cli_app"`) || !strings.Contains(rt.gotBody, `"app_secret":"secret_x"`) { + t.Errorf("request body missing credentials: %s", rt.gotBody) + } +} + +func TestFetchTAT_BodyCodeNonZero(t *testing.T) { + rt := &stubRoundTripper{ + respCode: 200, + respBody: `{"code":10003,"msg":"invalid app_id"}`, + } + hc := &http.Client{Transport: rt} + + token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error for code 10003") + } + if token != "" { + t.Errorf("token = %q, want empty", token) + } + if code != 10003 { + t.Errorf("apiCode = %d, want 10003", code) + } +} + +func TestFetchTAT_HTTPStatusError(t *testing.T) { + rt := &stubRoundTripper{respCode: 401, respBody: `unauthorized`} + hc := &http.Client{Transport: rt} + + token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error for HTTP 401") + } + if token != "" || code != 0 { + t.Errorf("token=%q code=%d", token, code) + } + var hse *HTTPStatusError + if !errors.As(err, &hse) { + t.Fatalf("error not HTTPStatusError: %v", err) + } + if hse.StatusCode != 401 { + t.Errorf("StatusCode = %d, want 401", hse.StatusCode) + } +} + +func TestFetchTAT_TransportError(t *testing.T) { + sentinel := errors.New("network down") + rt := &stubRoundTripper{err: sentinel} + hc := &http.Client{Transport: rt} + + _, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error") + } + if code != 0 { + t.Errorf("apiCode = %d, want 0", code) + } + if !errors.Is(err, sentinel) { + t.Errorf("error chain does not include sentinel: %v", err) + } +} + +func TestFetchTAT_BodyParseError(t *testing.T) { + rt := &stubRoundTripper{respCode: 200, respBody: `not json`} + hc := &http.Client{Transport: rt} + + _, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected parse error") + } + if code != 0 { + t.Errorf("apiCode = %d, want 0", code) + } +} + +func TestFetchTAT_BrandRouting(t *testing.T) { + tests := []struct { + brand core.LarkBrand + wantURL string + }{ + {core.BrandFeishu, "https://open.feishu.cn/open-apis/auth/v3/tenant_access_token/internal"}, + {core.BrandLark, "https://open.larksuite.com/open-apis/auth/v3/tenant_access_token/internal"}, + } + for _, tc := range tests { + t.Run(string(tc.brand), func(t *testing.T) { + rt := &stubRoundTripper{ + respCode: 200, + respBody: `{"code":0,"tenant_access_token":"t"}`, + } + hc := &http.Client{Transport: rt} + if _, _, err := FetchTAT(context.Background(), hc, tc.brand, "a", "b"); err != nil { + t.Fatal(err) + } + if got := rt.gotReq.URL.String(); got != tc.wantURL { + t.Errorf("url = %s, want %s", got, tc.wantURL) + } + }) + } +} + +func TestFetchTAT_ContextCanceled(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // will not respond before cancel + <-r.Context().Done() + })) + defer srv.Close() + + // Point at the test server by using a stub that rewrites URL. + rt := &urlRewriteRT{base: srv.URL} + hc := &http.Client{Transport: rt} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // pre-canceled + + _, _, err := FetchTAT(ctx, hc, core.BrandFeishu, "a", "b") + if err == nil { + t.Fatal("expected error for canceled context") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error chain missing context.Canceled: %v", err) + } +} + +// urlRewriteRT forwards requests to a fixed base URL (test server). +type urlRewriteRT struct{ base string } + +func (r *urlRewriteRT) RoundTrip(req *http.Request) (*http.Response, error) { + // Rewrite to test server origin. + newURL := r.base + req.URL.Path + req2, err := http.NewRequestWithContext(req.Context(), req.Method, newURL, req.Body) + if err != nil { + return nil, err + } + req2.Header = req.Header + return http.DefaultTransport.RoundTrip(req2) +} From a420e481a9b4ffdf6ab6abea955cce46485b9bc4 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Wed, 27 May 2026 11:02:38 +0800 Subject: [PATCH 2/7] refactor: delegate doResolveTAT to FetchTAT helper --- internal/credential/default_provider.go | 39 ++----------------------- 1 file changed, 2 insertions(+), 37 deletions(-) diff --git a/internal/credential/default_provider.go b/internal/credential/default_provider.go index 45ec3ef00..81dc11d34 100644 --- a/internal/credential/default_provider.go +++ b/internal/credential/default_provider.go @@ -4,9 +4,7 @@ package credential import ( - "bytes" "context" - "encoding/json" "fmt" "io" "net/http" @@ -135,42 +133,9 @@ func (p *DefaultTokenProvider) doResolveTAT(ctx context.Context) (*TokenResult, if err != nil { return nil, err } - ep := core.ResolveEndpoints(acct.Brand) - url := ep.Open + "/open-apis/auth/v3/tenant_access_token/internal" - - body, err := json.Marshal(map[string]string{ - "app_id": acct.AppID, - "app_secret": acct.AppSecret, - }) - if err != nil { - return nil, fmt.Errorf("failed to marshal TAT request: %w", err) - } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) - if err != nil { - return nil, err - } - req.Header.Set("Content-Type", "application/json") - - resp, err := httpClient.Do(req) + token, _, err := FetchTAT(ctx, httpClient, acct.Brand, acct.AppID, acct.AppSecret) if err != nil { return nil, err } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("TAT API returned HTTP %d", resp.StatusCode) - } - - var result struct { - Code int `json:"code"` - Msg string `json:"msg"` - TenantAccessToken string `json:"tenant_access_token"` - } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return nil, fmt.Errorf("failed to parse TAT response: %w", err) - } - if result.Code != 0 { - return nil, fmt.Errorf("TAT API error: [%d] %s", result.Code, result.Msg) - } - return &TokenResult{Token: result.TenantAccessToken}, nil + return &TokenResult{Token: token}, nil } From 45be78496efcb4d6fdc3dc85d698d95f2e679401 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Wed, 27 May 2026 11:07:59 +0800 Subject: [PATCH 3/7] feat: add credential probe helper for config init --- cmd/config/init_probe.go | 114 +++++++++++++ cmd/config/init_probe_test.go | 293 ++++++++++++++++++++++++++++++++++ 2 files changed, 407 insertions(+) create mode 100644 cmd/config/init_probe.go create mode 100644 cmd/config/init_probe_test.go diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go new file mode 100644 index 000000000..519a4c567 --- /dev/null +++ b/cmd/config/init_probe.go @@ -0,0 +1,114 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package config + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "net/http" + "time" + + "github.com/larksuite/cli/internal/build" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/credential" +) + +// probeTimeout is the total wall-clock budget for the credential probe step +// (covering both TAT acquisition and the subsequent probe request). +const probeTimeout = 3 * time.Second + +// credentialRejectionCodes lists the TAT response codes that the probe treats +// as deterministic credential-rejection signals. Anything outside this set is +// classified as ambiguous and produces no user-visible output. +var credentialRejectionCodes = map[int]struct{}{ + 10003: {}, + 10005: {}, + 10009: {}, +} + +// credentialRejectionHTTPStatuses lists the TAT HTTP response statuses that +// unambiguously indicate the supplied credentials were rejected. +var credentialRejectionHTTPStatuses = map[int]struct{}{ + 401: {}, + 403: {}, +} + +// runProbe runs a best-effort credential validation after config init has +// persisted the App ID and App Secret. It emits at most one stderr line and +// never affects the parent command's exit code. +// +// The function performs two HTTP calls in series, bounded by probeTimeout: +// +// 1. A TAT request using the just-saved credentials. If the response is one +// of the well-defined credential-rejection signals (response code 10003, +// 10005, 10009, or HTTP 401/403), a warning is written to ErrOut and the +// function returns. +// +// 2. If TAT succeeded, a POST to the probe endpoint is fired. The outcome of +// that call (success, server error, timeout, parse failure) is always +// ignored — no log, no warning. +func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret string, brand core.LarkBrand) { + if factory == nil || factory.IOStreams == nil { + return + } + httpClient, err := factory.HttpClient() + if err != nil { + return + } + + ctx, cancel := context.WithTimeout(parent, probeTimeout) + defer cancel() + + token, apiCode, err := credential.FetchTAT(ctx, httpClient, brand, appID, appSecret) + if err != nil { + if label, warn := classifyTATError(apiCode, err); warn { + fmt.Fprintf( + factory.IOStreams.ErrOut, + "warning: configured credentials may be invalid (code %s): please verify App ID and App Secret in lark-cli config.\n", + label, + ) + } + return + } + + // TAT succeeded — fire the probe call. Any outcome is ignored. + url := core.ResolveEndpoints(brand).Open + "/open-apis/application/v6/larksuite_cli_app/probe" + body := []byte(fmt.Sprintf(`{"from":"lark-cli/%s"}`, build.Version)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + if err != nil { + return + } + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + + resp, err := httpClient.Do(req) + if err != nil { + return + } + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) +} + +// classifyTATError returns (label, warn) where warn is true only when err +// represents a deterministic credential-rejection signal. label is the +// human-readable code string used in the warning text. +func classifyTATError(apiCode int, err error) (string, bool) { + if apiCode != 0 { + if _, ok := credentialRejectionCodes[apiCode]; ok { + return fmt.Sprintf("%d", apiCode), true + } + return "", false + } + var hse *credential.HTTPStatusError + if errors.As(err, &hse) { + if _, ok := credentialRejectionHTTPStatuses[hse.StatusCode]; ok { + return fmt.Sprintf("HTTP %d", hse.StatusCode), true + } + } + return "", false +} diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go new file mode 100644 index 000000000..53192a88c --- /dev/null +++ b/cmd/config/init_probe_test.go @@ -0,0 +1,293 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package config + +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + "time" + + "github.com/larksuite/cli/internal/build" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" +) + +// fakeRT routes requests to per-path handlers and records what it saw. +type fakeRT struct { + tatHandler func(req *http.Request) (*http.Response, error) + probeHandler func(req *http.Request) (*http.Response, error) + tatCalls int + probeCalls int + probeReq *http.Request + probeBody string +} + +func (f *fakeRT) RoundTrip(req *http.Request) (*http.Response, error) { + switch { + case strings.HasSuffix(req.URL.Path, "/auth/v3/tenant_access_token/internal"): + f.tatCalls++ + if f.tatHandler == nil { + return jsonResp(200, `{"code":0,"tenant_access_token":"t-ok"}`), nil + } + return f.tatHandler(req) + case strings.HasSuffix(req.URL.Path, "/application/v6/larksuite_cli_app/probe"): + f.probeCalls++ + f.probeReq = req + if req.Body != nil { + b, _ := io.ReadAll(req.Body) + f.probeBody = string(b) + } + if f.probeHandler == nil { + return jsonResp(200, `{"code":0,"data":{},"msg":"success"}`), nil + } + return f.probeHandler(req) + } + return nil, errors.New("unexpected URL: " + req.URL.String()) +} + +func jsonResp(code int, body string) *http.Response { + return &http.Response{ + StatusCode: code, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + } +} + +// fakeFactory builds a Factory with a stubbed HttpClient and a buffer ErrOut. +func fakeFactory(t *testing.T, rt http.RoundTripper) (*cmdutil.Factory, *bytes.Buffer) { + t.Helper() + errBuf := &bytes.Buffer{} + stdout := &bytes.Buffer{} + streams := &cmdutil.IOStreams{ + In: strings.NewReader(""), + Out: stdout, + ErrOut: errBuf, + } + f := &cmdutil.Factory{ + IOStreams: streams, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + } + return f, errBuf +} + +func TestRunProbe_TATCode10003_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(200, `{"code":10003,"msg":"invalid app_id"}`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + + if rt.probeCalls != 0 { + t.Error("probe endpoint must not be called when TAT fails") + } + got := errBuf.String() + want := "warning: configured credentials may be invalid (code 10003): please verify App ID and App Secret in lark-cli config.\n" + if got != want { + t.Errorf("stderr = %q\nwant %q", got, want) + } +} + +func TestRunProbe_TATCode10005_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(200, `{"code":10005,"msg":"invalid app_secret"}`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if !strings.Contains(errBuf.String(), "(code 10005)") { + t.Errorf("stderr missing code 10005: %q", errBuf.String()) + } +} + +func TestRunProbe_TATCode10009_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(200, `{"code":10009,"msg":"app_id and app_secret mismatch"}`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if !strings.Contains(errBuf.String(), "(code 10009)") { + t.Errorf("stderr missing code 10009: %q", errBuf.String()) + } +} + +func TestRunProbe_TATHTTP401_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(401, `unauthorized`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if !strings.Contains(errBuf.String(), "(code HTTP 401)") { + t.Errorf("stderr missing HTTP 401 label: %q", errBuf.String()) + } +} + +func TestRunProbe_TATHTTP403_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(403, `forbidden`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if !strings.Contains(errBuf.String(), "(code HTTP 403)") { + t.Errorf("stderr missing HTTP 403 label: %q", errBuf.String()) + } +} + +func TestRunProbe_TATHTTP500_Silent(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(500, `server error`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_TATTransportError_Silent(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return nil, errors.New("network down") + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_TATUnknownCode_Silent(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(200, `{"code":99999,"msg":"weird"}`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_TATSuccess_ProbeFails_Silent(t *testing.T) { + rt := &fakeRT{ + probeHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(500, `server error`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if rt.probeCalls != 1 { + t.Errorf("probe should be called once, got %d", rt.probeCalls) + } + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_TATSuccess_ProbeOK_Silent(t *testing.T) { + rt := &fakeRT{} + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if rt.tatCalls != 1 || rt.probeCalls != 1 { + t.Errorf("expected 1/1 calls, got tat=%d probe=%d", rt.tatCalls, rt.probeCalls) + } + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_ProbeRequestShape(t *testing.T) { + rt := &fakeRT{} + f, _ := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + + if rt.probeReq == nil { + t.Fatal("probe request not captured") + } + if rt.probeReq.Method != http.MethodPost { + t.Errorf("probe method = %s, want POST", rt.probeReq.Method) + } + if got := rt.probeReq.URL.String(); got != "https://open.feishu.cn/open-apis/application/v6/larksuite_cli_app/probe" { + t.Errorf("probe URL = %s", got) + } + if got := rt.probeReq.Header.Get("Authorization"); got != "Bearer t-ok" { + t.Errorf("Authorization = %q, want Bearer t-ok", got) + } + if !strings.Contains(rt.probeBody, `"from":"lark-cli/`+build.Version+`"`) { + t.Errorf("probe body missing from field: %s", rt.probeBody) + } +} + +func TestRunProbe_LarkBrand_HostRoutedCorrectly(t *testing.T) { + rt := &fakeRT{} + f, _ := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandLark) + if rt.probeReq == nil { + t.Fatal("probe request not captured") + } + if !strings.Contains(rt.probeReq.URL.Host, "larksuite.com") { + t.Errorf("probe host = %s, want larksuite.com", rt.probeReq.URL.Host) + } +} + +func TestRunProbe_HTTPClientError_Silent(t *testing.T) { + errBuf := &bytes.Buffer{} + streams := &cmdutil.IOStreams{ + In: strings.NewReader(""), + Out: &bytes.Buffer{}, + ErrOut: errBuf, + } + f := &cmdutil.Factory{ + IOStreams: streams, + HttpClient: func() (*http.Client, error) { + return nil, errors.New("client init failed") + }, + } + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if errBuf.Len() != 0 { + t.Errorf("expected silent, got: %q", errBuf.String()) + } +} + +func TestRunProbe_TimeoutHonored(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + // Block until context is canceled. + <-req.Context().Done() + return nil, req.Context().Err() + }, + } + f, errBuf := fakeFactory(t, rt) + + start := time.Now() + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + elapsed := time.Since(start) + + if elapsed > 4*time.Second { + t.Errorf("runProbe took %v, expected <= ~3s", elapsed) + } + if errBuf.Len() != 0 { + t.Errorf("expected silent on timeout, got: %q", errBuf.String()) + } +} From c025f43bf039c6e00019094e75c542616308c00c Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Wed, 27 May 2026 11:10:11 +0800 Subject: [PATCH 4/7] feat: wire credential probe into config init paths --- cmd/config/init.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/config/init.go b/cmd/config/init.go index b505ce8c7..873b1a1d8 100644 --- a/cmd/config/init.go +++ b/cmd/config/init.go @@ -317,6 +317,7 @@ func configInitRun(opts *ConfigInitOptions) error { output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf("Configuration saved to %s", core.GetConfigPath())) printLangPreferenceConfirmation(opts) output.PrintJson(f.IOStreams.Out, map[string]interface{}{"appId": opts.AppID, "appSecret": "****", "brand": brand}) + runProbe(opts.Ctx, f, opts.AppID, opts.appSecret, brand) return nil } @@ -356,6 +357,7 @@ func configInitRun(opts *ConfigInitOptions) error { } printLangPreferenceConfirmation(opts) output.PrintJson(f.IOStreams.Out, map[string]interface{}{"appId": result.AppID, "appSecret": "****", "brand": result.Brand}) + runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand) return nil } @@ -398,6 +400,9 @@ func configInitRun(opts *ConfigInitOptions) error { output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf(msg.ConfigSaved, result.AppID)) } printLangPreferenceConfirmation(opts) + if result.AppSecret != "" { + runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand) + } return nil } @@ -485,5 +490,8 @@ func configInitRun(opts *ConfigInitOptions) error { } output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf("Configuration saved to %s", core.GetConfigPath())) printLangPreferenceConfirmation(opts) + if appSecretInput != "" { + runProbe(opts.Ctx, f, resolvedAppId, appSecretInput, parseBrand(resolvedBrand)) + } return nil } From 0a76a4bf16578aa1e5ee9b6a8dcf00f3f08eee9e Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Thu, 28 May 2026 11:20:28 +0800 Subject: [PATCH 5/7] refactor: align FetchTAT errors with errs typed taxonomy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the ad-hoc *HTTPStatusError and classify TAT outcomes via the canonical errs.* taxonomy: * HTTP 401/403 + non-zero TAT body code → *errs.AuthenticationError with Problem.Code carrying the upstream signal * HTTP 5xx → *errs.NetworkError with CauseKind "5xx" and Retryable=true * 4xx other than 401/403, JSON parse, or 2xx with empty tenant_access_token → *errs.InternalError with subtype "sdk_error" * Transport / DNS / TLS / context cancel → *errs.NetworkError wrapping the underlying cause classifyTATError now extracts the typed signal via errors.As, removing the parallel apiCode return. Test factory switched to cmdutil.TestFactory(t, nil) with LARKSUITE_CLI_CONFIG_DIR=t.TempDir() per repo guideline, then HttpClient overridden with stubRoundTripper to keep per-test response control. --- cmd/config/init_probe.go | 38 +++--- cmd/config/init_probe_test.go | 41 +++--- internal/credential/default_provider.go | 2 +- internal/credential/tat_fetch.go | 129 +++++++++++++++---- internal/credential/tat_fetch_test.go | 160 ++++++++++++++++++++---- 5 files changed, 277 insertions(+), 93 deletions(-) diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go index 519a4c567..9300650ca 100644 --- a/cmd/config/init_probe.go +++ b/cmd/config/init_probe.go @@ -12,6 +12,7 @@ import ( "net/http" "time" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/build" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" @@ -22,8 +23,8 @@ import ( // (covering both TAT acquisition and the subsequent probe request). const probeTimeout = 3 * time.Second -// credentialRejectionCodes lists the TAT response codes that the probe treats -// as deterministic credential-rejection signals. Anything outside this set is +// credentialRejectionCodes lists the TAT body codes that the probe treats as +// deterministic credential-rejection signals. Anything outside this set is // classified as ambiguous and produces no user-visible output. var credentialRejectionCodes = map[int]struct{}{ 10003: {}, @@ -34,8 +35,8 @@ var credentialRejectionCodes = map[int]struct{}{ // credentialRejectionHTTPStatuses lists the TAT HTTP response statuses that // unambiguously indicate the supplied credentials were rejected. var credentialRejectionHTTPStatuses = map[int]struct{}{ - 401: {}, - 403: {}, + http.StatusUnauthorized: {}, + http.StatusForbidden: {}, } // runProbe runs a best-effort credential validation after config init has @@ -64,9 +65,9 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret ctx, cancel := context.WithTimeout(parent, probeTimeout) defer cancel() - token, apiCode, err := credential.FetchTAT(ctx, httpClient, brand, appID, appSecret) + token, err := credential.FetchTAT(ctx, httpClient, brand, appID, appSecret) if err != nil { - if label, warn := classifyTATError(apiCode, err); warn { + if label, warn := classifyTATError(err); warn { fmt.Fprintf( factory.IOStreams.ErrOut, "warning: configured credentials may be invalid (code %s): please verify App ID and App Secret in lark-cli config.\n", @@ -95,20 +96,21 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret } // classifyTATError returns (label, warn) where warn is true only when err -// represents a deterministic credential-rejection signal. label is the -// human-readable code string used in the warning text. -func classifyTATError(apiCode int, err error) (string, bool) { - if apiCode != 0 { - if _, ok := credentialRejectionCodes[apiCode]; ok { - return fmt.Sprintf("%d", apiCode), true - } +// represents a deterministic credential-rejection signal from FetchTAT. +// FetchTAT routes both body-code rejections and HTTP 401/403 to +// *errs.AuthenticationError with Problem.Code carrying the upstream signal, +// so a single errors.As extraction covers both paths. +func classifyTATError(err error) (string, bool) { + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { return "", false } - var hse *credential.HTTPStatusError - if errors.As(err, &hse) { - if _, ok := credentialRejectionHTTPStatuses[hse.StatusCode]; ok { - return fmt.Sprintf("HTTP %d", hse.StatusCode), true - } + code := authErr.Code + if _, ok := credentialRejectionCodes[code]; ok { + return fmt.Sprintf("%d", code), true + } + if _, ok := credentialRejectionHTTPStatuses[code]; ok { + return fmt.Sprintf("HTTP %d", code), true } return "", false } diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go index 53192a88c..7254917dd 100644 --- a/cmd/config/init_probe_test.go +++ b/cmd/config/init_probe_test.go @@ -59,21 +59,21 @@ func jsonResp(code int, body string) *http.Response { } } -// fakeFactory builds a Factory with a stubbed HttpClient and a buffer ErrOut. +// fakeFactory builds a test Factory whose HttpClient is overridden to use +// the caller-supplied RoundTripper. +// +// Wired through cmdutil.TestFactory(t, nil) so the canonical IOStreams, +// Credential, Keychain and FileIO wiring is in place (per repo test-factory +// guidance). The HttpClient is then swapped to our stub so we can drive +// exact HTTP responses for the probe. Config-dir isolation is set up via +// t.Setenv(LARKSUITE_CLI_CONFIG_DIR, t.TempDir()) so any incidental config +// touch lands in a temp dir rather than the developer's real config. func fakeFactory(t *testing.T, rt http.RoundTripper) (*cmdutil.Factory, *bytes.Buffer) { t.Helper() - errBuf := &bytes.Buffer{} - stdout := &bytes.Buffer{} - streams := &cmdutil.IOStreams{ - In: strings.NewReader(""), - Out: stdout, - ErrOut: errBuf, - } - f := &cmdutil.Factory{ - IOStreams: streams, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: rt}, nil - }, + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, _, errBuf, _ := cmdutil.TestFactory(t, nil) + f.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil } return f, errBuf } @@ -252,17 +252,10 @@ func TestRunProbe_LarkBrand_HostRoutedCorrectly(t *testing.T) { } func TestRunProbe_HTTPClientError_Silent(t *testing.T) { - errBuf := &bytes.Buffer{} - streams := &cmdutil.IOStreams{ - In: strings.NewReader(""), - Out: &bytes.Buffer{}, - ErrOut: errBuf, - } - f := &cmdutil.Factory{ - IOStreams: streams, - HttpClient: func() (*http.Client, error) { - return nil, errors.New("client init failed") - }, + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, _, errBuf, _ := cmdutil.TestFactory(t, nil) + f.HttpClient = func() (*http.Client, error) { + return nil, errors.New("client init failed") } runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) if errBuf.Len() != 0 { diff --git a/internal/credential/default_provider.go b/internal/credential/default_provider.go index 81dc11d34..4b4050b6d 100644 --- a/internal/credential/default_provider.go +++ b/internal/credential/default_provider.go @@ -133,7 +133,7 @@ func (p *DefaultTokenProvider) doResolveTAT(ctx context.Context) (*TokenResult, if err != nil { return nil, err } - token, _, err := FetchTAT(ctx, httpClient, acct.Brand, acct.AppID, acct.AppSecret) + token, err := FetchTAT(ctx, httpClient, acct.Brand, acct.AppID, acct.AppSecret) if err != nil { return nil, err } diff --git a/internal/credential/tat_fetch.go b/internal/credential/tat_fetch.go index 4af672b0e..a3634b67a 100644 --- a/internal/credential/tat_fetch.go +++ b/internal/credential/tat_fetch.go @@ -10,35 +10,35 @@ import ( "fmt" "net/http" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" ) -// HTTPStatusError indicates the TAT request returned a non-2xx HTTP status. -// Callers can use errors.As to inspect the status code. -type HTTPStatusError struct{ StatusCode int } - -func (e *HTTPStatusError) Error() string { - return fmt.Sprintf("TAT API returned HTTP %d", e.StatusCode) -} - // FetchTAT performs a single HTTP POST to obtain a tenant access token using // the provided credentials. It does not read configuration or keychain. // -// Return values: +// On failure it returns a typed error from the github.com/larksuite/cli/errs +// taxonomy. The caller is responsible for context timeouts. // -// - token: tenant access token when err is nil -// - apiCode: TAT response `code` field; 0 unless the request was 2xx and -// the body parsed successfully into a structured response -// - err: non-nil for non-zero apiCode, non-2xx HTTP, transport errors, -// or JSON parse failures +// Error classification: // -// The caller is responsible for context timeouts. +// - HTTP 401/403, or non-zero body code → *errs.AuthenticationError, with +// Problem.Code carrying the upstream signal (HTTP status when the request +// was rejected pre-body; body "code" when the response decoded successfully). +// - HTTP 5xx → *errs.NetworkError with CauseKind "5xx" and Retryable=true. +// - HTTP 4xx other than 401/403 (or any other non-2xx) → *errs.InternalError +// with subtype "sdk_error" — these indicate caller-side request shape +// problems, not transport failures. +// - Transport / dial / TLS / DNS failure → *errs.NetworkError with the +// underlying error wrapped via Cause. +// - JSON encode/decode failure, or 2xx with empty tenant_access_token → +// *errs.InternalError with subtype "sdk_error". func FetchTAT( ctx context.Context, httpClient *http.Client, brand core.LarkBrand, appID, appSecret string, -) (token string, apiCode int, err error) { +) (string, error) { ep := core.ResolveEndpoints(brand) url := ep.Open + "/open-apis/auth/v3/tenant_access_token/internal" @@ -47,23 +47,80 @@ func FetchTAT( "app_secret": appSecret, }) if err != nil { - return "", 0, fmt.Errorf("marshal TAT request: %w", err) + return "", &errs.InternalError{ + Problem: errs.Problem{ + Category: errs.CategoryInternal, + Subtype: errs.SubtypeSDKError, + Message: fmt.Sprintf("marshal TAT request: %v", err), + }, + Cause: err, + } } req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) if err != nil { - return "", 0, err + return "", &errs.InternalError{ + Problem: errs.Problem{ + Category: errs.CategoryInternal, + Subtype: errs.SubtypeSDKError, + Message: fmt.Sprintf("build TAT request: %v", err), + }, + Cause: err, + } } req.Header.Set("Content-Type", "application/json") resp, err := httpClient.Do(req) if err != nil { - return "", 0, err + return "", &errs.NetworkError{ + Problem: errs.Problem{ + Category: errs.CategoryNetwork, + Subtype: errs.SubtypeNetworkTransport, + Message: fmt.Sprintf("TAT request transport error: %v", err), + Retryable: true, + }, + Cause: err, + } } defer resp.Body.Close() + // HTTP 401/403 are unambiguous credential-rejection signals at the TAT + // endpoint — surface them as authentication errors so callers can + // distinguish them from generic network failures. + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + return "", &errs.AuthenticationError{ + Problem: errs.Problem{ + Category: errs.CategoryAuthentication, + Subtype: errs.SubtypeTokenInvalid, + Code: resp.StatusCode, + Message: fmt.Sprintf("TAT endpoint rejected credentials with HTTP %d", resp.StatusCode), + }, + } + } + // 5xx is upstream/server side — transport-class with retry hint. + if resp.StatusCode >= 500 { + return "", &errs.NetworkError{ + Problem: errs.Problem{ + Category: errs.CategoryNetwork, + Subtype: errs.SubtypeNetworkTransport, + Message: fmt.Sprintf("TAT endpoint returned HTTP %d", resp.StatusCode), + Retryable: true, + }, + CauseKind: "5xx", + } + } + // Any other non-2xx (4xx other than 401/403, 1xx, 3xx without redirect + // handling) indicates a client-side request shape problem — treat as an + // internal error rather than overloading NetworkError, whose taxonomy is + // reserved for transport-layer failures (timeout / DNS / TLS / 5xx). if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return "", 0, &HTTPStatusError{StatusCode: resp.StatusCode} + return "", &errs.InternalError{ + Problem: errs.Problem{ + Category: errs.CategoryInternal, + Subtype: errs.SubtypeSDKError, + Message: fmt.Sprintf("TAT endpoint returned unexpected HTTP %d", resp.StatusCode), + }, + } } var parsed struct { @@ -72,10 +129,36 @@ func FetchTAT( TenantAccessToken string `json:"tenant_access_token"` } if err := json.NewDecoder(resp.Body).Decode(&parsed); err != nil { - return "", 0, fmt.Errorf("parse TAT response: %w", err) + return "", &errs.InternalError{ + Problem: errs.Problem{ + Category: errs.CategoryInternal, + Subtype: errs.SubtypeSDKError, + Message: fmt.Sprintf("parse TAT response: %v", err), + }, + Cause: err, + } } if parsed.Code != 0 { - return "", parsed.Code, fmt.Errorf("TAT API error: [%d] %s", parsed.Code, parsed.Msg) + return "", &errs.AuthenticationError{ + Problem: errs.Problem{ + Category: errs.CategoryAuthentication, + Subtype: errs.SubtypeTokenInvalid, + Code: parsed.Code, + Message: fmt.Sprintf("TAT API rejected credentials: [%d] %s", parsed.Code, parsed.Msg), + }, + } + } + // Defensive: a 2xx response with code=0 but no token is a contract + // violation by upstream — surface it as an internal SDK error so callers + // fail fast rather than receiving a silent empty token. + if parsed.TenantAccessToken == "" { + return "", &errs.InternalError{ + Problem: errs.Problem{ + Category: errs.CategoryInternal, + Subtype: errs.SubtypeSDKError, + Message: "TAT response missing tenant_access_token despite code=0", + }, + } } - return parsed.TenantAccessToken, 0, nil + return parsed.TenantAccessToken, nil } diff --git a/internal/credential/tat_fetch_test.go b/internal/credential/tat_fetch_test.go index 4f3cee173..3517620bc 100644 --- a/internal/credential/tat_fetch_test.go +++ b/internal/credential/tat_fetch_test.go @@ -6,12 +6,14 @@ package credential import ( "context" "errors" + "fmt" "io" "net/http" "net/http/httptest" "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" ) @@ -47,16 +49,13 @@ func TestFetchTAT_Success(t *testing.T) { } hc := &http.Client{Transport: rt} - token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + token, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err != nil { t.Fatalf("unexpected error: %v", err) } if token != "t-abc" { t.Errorf("token = %q, want t-abc", token) } - if code != 0 { - t.Errorf("apiCode = %d, want 0", code) - } if rt.gotReq.URL.String() != "https://open.feishu.cn/open-apis/auth/v3/tenant_access_token/internal" { t.Errorf("url = %s", rt.gotReq.URL.String()) } @@ -65,72 +64,174 @@ func TestFetchTAT_Success(t *testing.T) { } } -func TestFetchTAT_BodyCodeNonZero(t *testing.T) { +func TestFetchTAT_BodyCodeNonZero_AuthenticationError(t *testing.T) { rt := &stubRoundTripper{ respCode: 200, respBody: `{"code":10003,"msg":"invalid app_id"}`, } hc := &http.Client{Transport: rt} - token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + token, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { t.Fatal("expected error for code 10003") } if token != "" { t.Errorf("token = %q, want empty", token) } - if code != 10003 { - t.Errorf("apiCode = %d, want 10003", code) + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("error not *errs.AuthenticationError: %T %v", err, err) + } + if authErr.Code != 10003 { + t.Errorf("Code = %d, want 10003", authErr.Code) + } + if authErr.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want %q", authErr.Category, errs.CategoryAuthentication) + } + if authErr.Subtype != errs.SubtypeTokenInvalid { + t.Errorf("Subtype = %q, want %q", authErr.Subtype, errs.SubtypeTokenInvalid) } } -func TestFetchTAT_HTTPStatusError(t *testing.T) { +func TestFetchTAT_HTTPUnauthorized_AuthenticationError(t *testing.T) { rt := &stubRoundTripper{respCode: 401, respBody: `unauthorized`} hc := &http.Client{Transport: rt} - token, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + token, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { t.Fatal("expected error for HTTP 401") } - if token != "" || code != 0 { - t.Errorf("token=%q code=%d", token, code) + if token != "" { + t.Errorf("token = %q, want empty", token) + } + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("error not *errs.AuthenticationError: %T %v", err, err) + } + if authErr.Code != 401 { + t.Errorf("Code = %d, want 401 (HTTP status preserved)", authErr.Code) + } +} + +func TestFetchTAT_HTTPForbidden_AuthenticationError(t *testing.T) { + rt := &stubRoundTripper{respCode: 403, respBody: `forbidden`} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) || authErr.Code != 403 { + t.Fatalf("want *errs.AuthenticationError Code=403, got %T %v", err, err) + } +} + +func TestFetchTAT_HTTP5xx_NetworkError(t *testing.T) { + rt := &stubRoundTripper{respCode: 503, respBody: `service unavailable`} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + var netErr *errs.NetworkError + if !errors.As(err, &netErr) { + t.Fatalf("want *errs.NetworkError, got %T %v", err, err) + } + if !netErr.Retryable { + t.Errorf("5xx should be retryable") + } + if netErr.CauseKind != "5xx" { + t.Errorf("CauseKind = %q, want \"5xx\"", netErr.CauseKind) } - var hse *HTTPStatusError - if !errors.As(err, &hse) { - t.Fatalf("error not HTTPStatusError: %v", err) +} + +// Non-401/403 4xx must NOT be classified as NetworkError. NetworkError's +// taxonomy is reserved for transport-layer failures; a client-side HTTP +// shape problem (e.g. 400 / 404 / 429) is the caller's responsibility and +// belongs in InternalError. +func TestFetchTAT_HTTP4xxOther_InternalError(t *testing.T) { + tests := []int{ + http.StatusBadRequest, // 400 + http.StatusNotFound, // 404 + http.StatusTooManyRequests, // 429 + http.StatusUnsupportedMediaType, // 415 } - if hse.StatusCode != 401 { - t.Errorf("StatusCode = %d, want 401", hse.StatusCode) + for _, code := range tests { + t.Run(fmt.Sprintf("HTTP %d", code), func(t *testing.T) { + rt := &stubRoundTripper{respCode: code, respBody: `whatever`} + hc := &http.Client{Transport: rt} + + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatalf("expected error for HTTP %d", code) + } + var intErr *errs.InternalError + if !errors.As(err, &intErr) { + t.Fatalf("want *errs.InternalError, got %T %v", err, err) + } + var netErr *errs.NetworkError + if errors.As(err, &netErr) { + t.Errorf("HTTP %d must not be classified as NetworkError", code) + } + }) } } -func TestFetchTAT_TransportError(t *testing.T) { +func TestFetchTAT_TransportError_NetworkError(t *testing.T) { sentinel := errors.New("network down") rt := &stubRoundTripper{err: sentinel} hc := &http.Client{Transport: rt} - _, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { t.Fatal("expected error") } - if code != 0 { - t.Errorf("apiCode = %d, want 0", code) + var netErr *errs.NetworkError + if !errors.As(err, &netErr) { + t.Fatalf("want *errs.NetworkError, got %T %v", err, err) } + // Underlying transport error must still be reachable for diagnostics. if !errors.Is(err, sentinel) { t.Errorf("error chain does not include sentinel: %v", err) } } -func TestFetchTAT_BodyParseError(t *testing.T) { +// 2xx with code=0 but empty tenant_access_token is treated as a contract +// violation by upstream; FetchTAT must fail fast rather than return a +// silent empty token that callers cannot debug. +func TestFetchTAT_EmptyTokenOnSuccess_InternalError(t *testing.T) { + rt := &stubRoundTripper{ + respCode: 200, + respBody: `{"code":0,"msg":"ok"}`, // tenant_access_token field absent + } + hc := &http.Client{Transport: rt} + + token, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + if err == nil { + t.Fatal("expected error when tenant_access_token is empty") + } + if token != "" { + t.Errorf("token = %q, want empty", token) + } + var intErr *errs.InternalError + if !errors.As(err, &intErr) { + t.Fatalf("want *errs.InternalError, got %T %v", err, err) + } + if intErr.Subtype != errs.SubtypeSDKError { + t.Errorf("Subtype = %q, want %q", intErr.Subtype, errs.SubtypeSDKError) + } +} + +func TestFetchTAT_BodyParseError_InternalError(t *testing.T) { rt := &stubRoundTripper{respCode: 200, respBody: `not json`} hc := &http.Client{Transport: rt} - _, code, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") if err == nil { t.Fatal("expected parse error") } - if code != 0 { - t.Errorf("apiCode = %d, want 0", code) + var intErr *errs.InternalError + if !errors.As(err, &intErr) { + t.Fatalf("want *errs.InternalError, got %T %v", err, err) + } + if intErr.Subtype != errs.SubtypeSDKError { + t.Errorf("Subtype = %q, want %q", intErr.Subtype, errs.SubtypeSDKError) } } @@ -149,7 +250,7 @@ func TestFetchTAT_BrandRouting(t *testing.T) { respBody: `{"code":0,"tenant_access_token":"t"}`, } hc := &http.Client{Transport: rt} - if _, _, err := FetchTAT(context.Background(), hc, tc.brand, "a", "b"); err != nil { + if _, err := FetchTAT(context.Background(), hc, tc.brand, "a", "b"); err != nil { t.Fatal(err) } if got := rt.gotReq.URL.String(); got != tc.wantURL { @@ -173,10 +274,15 @@ func TestFetchTAT_ContextCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // pre-canceled - _, _, err := FetchTAT(ctx, hc, core.BrandFeishu, "a", "b") + _, err := FetchTAT(ctx, hc, core.BrandFeishu, "a", "b") if err == nil { t.Fatal("expected error for canceled context") } + // Canceled context surfaces as a transport-class NetworkError. + var netErr *errs.NetworkError + if !errors.As(err, &netErr) { + t.Fatalf("want *errs.NetworkError, got %T %v", err, err) + } if !errors.Is(err, context.Canceled) { t.Errorf("error chain missing context.Canceled: %v", err) } From 72dabf540f8f3583a1888bdd93f56e821b1d28c0 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Thu, 28 May 2026 17:01:02 +0800 Subject: [PATCH 6/7] fix: warn on any non-zero TAT body code, not just allowlisted ones --- cmd/config/init_probe.go | 55 +++++++++++++++-------------------- cmd/config/init_probe_test.go | 33 ++++++++++++++++++--- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go index 9300650ca..4067d243a 100644 --- a/cmd/config/init_probe.go +++ b/cmd/config/init_probe.go @@ -23,32 +23,18 @@ import ( // (covering both TAT acquisition and the subsequent probe request). const probeTimeout = 3 * time.Second -// credentialRejectionCodes lists the TAT body codes that the probe treats as -// deterministic credential-rejection signals. Anything outside this set is -// classified as ambiguous and produces no user-visible output. -var credentialRejectionCodes = map[int]struct{}{ - 10003: {}, - 10005: {}, - 10009: {}, -} - -// credentialRejectionHTTPStatuses lists the TAT HTTP response statuses that -// unambiguously indicate the supplied credentials were rejected. -var credentialRejectionHTTPStatuses = map[int]struct{}{ - http.StatusUnauthorized: {}, - http.StatusForbidden: {}, -} - // runProbe runs a best-effort credential validation after config init has // persisted the App ID and App Secret. It emits at most one stderr line and // never affects the parent command's exit code. // // The function performs two HTTP calls in series, bounded by probeTimeout: // -// 1. A TAT request using the just-saved credentials. If the response is one -// of the well-defined credential-rejection signals (response code 10003, -// 10005, 10009, or HTTP 401/403), a warning is written to ErrOut and the -// function returns. +// 1. A TAT request using the just-saved credentials. The classifier treats +// any TAT-side credential-rejection signal — a non-zero body code or HTTP +// 401/403 — as deterministic evidence of a configuration problem and +// writes a warning to ErrOut. Ambiguous failures (transport errors, 5xx, +// JSON parse errors, timeouts) are silent so that valid configurations +// are never disturbed by upstream noise. // // 2. If TAT succeeded, a POST to the probe endpoint is fired. The outcome of // that call (success, server error, timeout, parse failure) is always @@ -95,22 +81,27 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret _, _ = io.Copy(io.Discard, resp.Body) } -// classifyTATError returns (label, warn) where warn is true only when err -// represents a deterministic credential-rejection signal from FetchTAT. -// FetchTAT routes both body-code rejections and HTTP 401/403 to -// *errs.AuthenticationError with Problem.Code carrying the upstream signal, -// so a single errors.As extraction covers both paths. +// classifyTATError returns (label, warn) where warn is true when err carries +// a TAT-side credential-rejection signal. +// +// FetchTAT routes both body-code rejections and HTTP 401/403 onto +// *errs.AuthenticationError (Problem.Code carries the upstream code or HTTP +// status). Any such error is, by construction, a server saying "I looked at +// your payload and rejected the credentials" — which is exactly the +// configuration-problem signal we want to warn on. Network errors, 5xx, +// JSON parse failures and timeouts surface as *errs.NetworkError or +// *errs.InternalError, never *errs.AuthenticationError, so this single +// errors.As gate is sufficient to keep ambiguous failures silent. +// +// Label format: HTTP statuses (401, 403) render as "HTTP "; any other +// code value (which originates from the TAT body) renders as "". func classifyTATError(err error) (string, bool) { var authErr *errs.AuthenticationError if !errors.As(err, &authErr) { return "", false } - code := authErr.Code - if _, ok := credentialRejectionCodes[code]; ok { - return fmt.Sprintf("%d", code), true - } - if _, ok := credentialRejectionHTTPStatuses[code]; ok { - return fmt.Sprintf("HTTP %d", code), true + if authErr.Code == http.StatusUnauthorized || authErr.Code == http.StatusForbidden { + return fmt.Sprintf("HTTP %d", authErr.Code), true } - return "", false + return fmt.Sprintf("%d", authErr.Code), true } diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go index 7254917dd..7a2322d61 100644 --- a/cmd/config/init_probe_test.go +++ b/cmd/config/init_probe_test.go @@ -124,6 +124,23 @@ func TestRunProbe_TATCode10009_EmitsWarning(t *testing.T) { } } +// 10014 ("app secret invalid") is the code feishu's TAT v3 endpoint actually +// returns when an existing appID is paired with the wrong secret — the +// single most common real-world failure. Locked in by this test because the +// original whitelist missed it. +func TestRunProbe_TATCode10014_EmitsWarning(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + return jsonResp(200, `{"code":10014,"msg":"app secret invalid"}`), nil + }, + } + f, errBuf := fakeFactory(t, rt) + runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if !strings.Contains(errBuf.String(), "(code 10014)") { + t.Errorf("stderr missing code 10014: %q", errBuf.String()) + } +} + func TestRunProbe_TATHTTP401_EmitsWarning(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { @@ -176,16 +193,24 @@ func TestRunProbe_TATTransportError_Silent(t *testing.T) { } } -func TestRunProbe_TATUnknownCode_Silent(t *testing.T) { +// Policy: any non-zero TAT body code is treated as a deterministic +// credential-rejection signal (the server saw the payload and refused it), +// regardless of whether the specific code is one we recognize. This guards +// against upstream adding new credential-class codes without us noticing — +// previously, missing 10014 from a hand-rolled allowlist caused real-world +// "wrong app_secret" failures to go silent. The trade-off: ambiguous +// failures stay in NetworkError / InternalError lanes (transport / 5xx / +// parse / timeout) and remain silent, so valid configs are not disturbed. +func TestRunProbe_TATUnknownBodyCode_EmitsWarning(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(200, `{"code":99999,"msg":"weird"}`), nil + return jsonResp(200, `{"code":99999,"msg":"future-unknown"}`), nil }, } f, errBuf := fakeFactory(t, rt) runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) + if !strings.Contains(errBuf.String(), "(code 99999)") { + t.Errorf("stderr missing code 99999: %q", errBuf.String()) } } From 10924af6b41a99d91adc4dc33853bbe371b6c243 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Sat, 30 May 2026 12:12:59 +0800 Subject: [PATCH 7/7] feat: surface probe credential rejection as typed error Previously a deterministic credential-rejection signal from the post-init probe was printed as a best-effort stderr warning with exit 0. An AI-agent caller that keys off exit code and stdout could miss it entirely. Propagate the *errs.AuthenticationError up through RunE instead: config init now exits 3 with a structured authentication error envelope when the saved App ID / App Secret are rejected by the open platform. The error is re-wrapped with a jargon-free message and an actionable hint, preserving the upstream code and chaining the original cause. Scope is unchanged from the empirical-lane policy: only AuthenticationError (any non-zero TAT body code or HTTP 401/403) propagates. Ambiguous failures (transport, 5xx, JSON parse, timeout, http-client init) stay silent and non-blocking, so valid configurations are never disturbed by upstream noise. The saved config is intentionally not rolled back; the stdout envelope still records what was saved, while exit 3 + stderr report that it does not authenticate. --- cmd/config/init.go | 16 +++- cmd/config/init_probe.go | 87 ++++++++--------- cmd/config/init_probe_test.go | 173 ++++++++++++++++++---------------- 3 files changed, 145 insertions(+), 131 deletions(-) diff --git a/cmd/config/init.go b/cmd/config/init.go index 873b1a1d8..f7f9a9648 100644 --- a/cmd/config/init.go +++ b/cmd/config/init.go @@ -317,7 +317,9 @@ func configInitRun(opts *ConfigInitOptions) error { output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf("Configuration saved to %s", core.GetConfigPath())) printLangPreferenceConfirmation(opts) output.PrintJson(f.IOStreams.Out, map[string]interface{}{"appId": opts.AppID, "appSecret": "****", "brand": brand}) - runProbe(opts.Ctx, f, opts.AppID, opts.appSecret, brand) + if err := runProbe(opts.Ctx, f, opts.AppID, opts.appSecret, brand); err != nil { + return err + } return nil } @@ -357,7 +359,9 @@ func configInitRun(opts *ConfigInitOptions) error { } printLangPreferenceConfirmation(opts) output.PrintJson(f.IOStreams.Out, map[string]interface{}{"appId": result.AppID, "appSecret": "****", "brand": result.Brand}) - runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand) + if err := runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand); err != nil { + return err + } return nil } @@ -401,7 +405,9 @@ func configInitRun(opts *ConfigInitOptions) error { } printLangPreferenceConfirmation(opts) if result.AppSecret != "" { - runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand) + if err := runProbe(opts.Ctx, f, result.AppID, result.AppSecret, result.Brand); err != nil { + return err + } } return nil } @@ -491,7 +497,9 @@ func configInitRun(opts *ConfigInitOptions) error { output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf("Configuration saved to %s", core.GetConfigPath())) printLangPreferenceConfirmation(opts) if appSecretInput != "" { - runProbe(opts.Ctx, f, resolvedAppId, appSecretInput, parseBrand(resolvedBrand)) + if err := runProbe(opts.Ctx, f, resolvedAppId, appSecretInput, parseBrand(resolvedBrand)); err != nil { + return err + } } return nil } diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go index 4067d243a..96bafb426 100644 --- a/cmd/config/init_probe.go +++ b/cmd/config/init_probe.go @@ -24,28 +24,33 @@ import ( const probeTimeout = 3 * time.Second // runProbe runs a best-effort credential validation after config init has -// persisted the App ID and App Secret. It emits at most one stderr line and -// never affects the parent command's exit code. +// persisted the App ID and App Secret. It returns a non-nil error only for a +// deterministic credential-rejection signal; every other outcome returns nil +// so that valid configurations and transient/upstream noise never block the +// command. // -// The function performs two HTTP calls in series, bounded by probeTimeout: +// The function performs up to two HTTP calls in series, bounded by +// probeTimeout: // -// 1. A TAT request using the just-saved credentials. The classifier treats -// any TAT-side credential-rejection signal — a non-zero body code or HTTP -// 401/403 — as deterministic evidence of a configuration problem and -// writes a warning to ErrOut. Ambiguous failures (transport errors, 5xx, -// JSON parse errors, timeouts) are silent so that valid configurations -// are never disturbed by upstream noise. +// 1. A TAT request using the just-saved credentials. FetchTAT surfaces a +// deterministic credential-rejection signal — a non-zero TAT body code or +// HTTP 401/403 — as *errs.AuthenticationError. That is the only outcome +// propagated to the caller, re-wrapped with an actionable, jargon-free +// message so the root dispatcher renders a typed error envelope and +// `config init` exits non-zero. Ambiguous failures (transport errors, +// 5xx, JSON parse errors, timeouts) surface as *errs.NetworkError / +// *errs.InternalError and are swallowed (return nil). // // 2. If TAT succeeded, a POST to the probe endpoint is fired. The outcome of // that call (success, server error, timeout, parse failure) is always -// ignored — no log, no warning. -func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret string, brand core.LarkBrand) { - if factory == nil || factory.IOStreams == nil { - return +// ignored — return nil regardless. +func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret string, brand core.LarkBrand) error { + if factory == nil { + return nil } httpClient, err := factory.HttpClient() if err != nil { - return + return nil } ctx, cancel := context.WithTimeout(parent, probeTimeout) @@ -53,14 +58,24 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret token, err := credential.FetchTAT(ctx, httpClient, brand, appID, appSecret) if err != nil { - if label, warn := classifyTATError(err); warn { - fmt.Fprintf( - factory.IOStreams.ErrOut, - "warning: configured credentials may be invalid (code %s): please verify App ID and App Secret in lark-cli config.\n", - label, - ) + var authErr *errs.AuthenticationError + if errors.As(err, &authErr) { + // Deterministic credential rejection: re-surface with an + // actionable, jargon-free message while preserving the upstream + // code (10003 / 10014 / HTTP 401 / ...) and the error chain. + return &errs.AuthenticationError{ + Problem: errs.Problem{ + Category: errs.CategoryAuthentication, + Subtype: authErr.Subtype, + Code: authErr.Code, + Message: "configured credentials may be invalid: please verify the App ID and App Secret", + Hint: "re-run `lark-cli config init` with the correct App ID and App Secret", + }, + Cause: err, + } } - return + // Ambiguous failure (transport / 5xx / parse / timeout) — stay silent. + return nil } // TAT succeeded — fire the probe call. Any outcome is ignored. @@ -68,40 +83,16 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret body := []byte(fmt.Sprintf(`{"from":"lark-cli/%s"}`, build.Version)) req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) if err != nil { - return + return nil } req.Header.Set("Authorization", "Bearer "+token) req.Header.Set("Content-Type", "application/json") resp, err := httpClient.Do(req) if err != nil { - return + return nil } defer resp.Body.Close() _, _ = io.Copy(io.Discard, resp.Body) -} - -// classifyTATError returns (label, warn) where warn is true when err carries -// a TAT-side credential-rejection signal. -// -// FetchTAT routes both body-code rejections and HTTP 401/403 onto -// *errs.AuthenticationError (Problem.Code carries the upstream code or HTTP -// status). Any such error is, by construction, a server saying "I looked at -// your payload and rejected the credentials" — which is exactly the -// configuration-problem signal we want to warn on. Network errors, 5xx, -// JSON parse failures and timeouts surface as *errs.NetworkError or -// *errs.InternalError, never *errs.AuthenticationError, so this single -// errors.As gate is sufficient to keep ambiguous failures silent. -// -// Label format: HTTP statuses (401, 403) render as "HTTP "; any other -// code value (which originates from the TAT body) renders as "". -func classifyTATError(err error) (string, bool) { - var authErr *errs.AuthenticationError - if !errors.As(err, &authErr) { - return "", false - } - if authErr.Code == http.StatusUnauthorized || authErr.Code == http.StatusForbidden { - return fmt.Sprintf("HTTP %d", authErr.Code), true - } - return fmt.Sprintf("%d", authErr.Code), true + return nil } diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go index 7a2322d61..ecf18bbab 100644 --- a/cmd/config/init_probe_test.go +++ b/cmd/config/init_probe_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/build" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" @@ -68,6 +69,10 @@ func jsonResp(code int, body string) *http.Response { // exact HTTP responses for the probe. Config-dir isolation is set up via // t.Setenv(LARKSUITE_CLI_CONFIG_DIR, t.TempDir()) so any incidental config // touch lands in a temp dir rather than the developer's real config. +// +// The returned buffer is the Factory's stderr. runProbe no longer writes any +// warning to stderr (it propagates a typed error instead), so every test +// asserts this buffer stays empty as an invariant. func fakeFactory(t *testing.T, rt http.RoundTripper) (*cmdutil.Factory, *bytes.Buffer) { t.Helper() t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) @@ -78,7 +83,33 @@ func fakeFactory(t *testing.T, rt http.RoundTripper) (*cmdutil.Factory, *bytes.B return f, errBuf } -func TestRunProbe_TATCode10003_EmitsWarning(t *testing.T) { +// assertAuthError asserts err is a deterministic credential-rejection signal: +// a non-nil *errs.AuthenticationError carrying the expected upstream code and +// the actionable, jargon-free message runProbe re-wraps it with. +func assertAuthError(t *testing.T, err error, wantCode int) { + t.Helper() + if err == nil { + t.Fatalf("expected *errs.AuthenticationError (code %d), got nil", wantCode) + } + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("expected *errs.AuthenticationError, got %T: %v", err, err) + } + if authErr.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want %q", authErr.Category, errs.CategoryAuthentication) + } + if authErr.Code != wantCode { + t.Errorf("Code = %d, want %d", authErr.Code, wantCode) + } + if !strings.Contains(authErr.Message, "configured credentials may be invalid") { + t.Errorf("Message = %q, want it to mention 'configured credentials may be invalid'", authErr.Message) + } + if authErr.Hint == "" { + t.Error("expected a non-empty Hint guiding the user to re-run config init") + } +} + +func TestRunProbe_TATCode10003_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(200, `{"code":10003,"msg":"invalid app_id"}`), nil @@ -86,132 +117,119 @@ func TestRunProbe_TATCode10003_EmitsWarning(t *testing.T) { } f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) if rt.probeCalls != 0 { t.Error("probe endpoint must not be called when TAT fails") } - got := errBuf.String() - want := "warning: configured credentials may be invalid (code 10003): please verify App ID and App Secret in lark-cli config.\n" - if got != want { - t.Errorf("stderr = %q\nwant %q", got, want) + assertAuthError(t, err, 10003) + if errBuf.Len() != 0 { + t.Errorf("runProbe must not write to stderr, got: %q", errBuf.String()) } } -func TestRunProbe_TATCode10005_EmitsWarning(t *testing.T) { +func TestRunProbe_TATCode10005_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(200, `{"code":10005,"msg":"invalid app_secret"}`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code 10005)") { - t.Errorf("stderr missing code 10005: %q", errBuf.String()) - } + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 10005) } -func TestRunProbe_TATCode10009_EmitsWarning(t *testing.T) { +func TestRunProbe_TATCode10009_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(200, `{"code":10009,"msg":"app_id and app_secret mismatch"}`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code 10009)") { - t.Errorf("stderr missing code 10009: %q", errBuf.String()) - } + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 10009) } // 10014 ("app secret invalid") is the code feishu's TAT v3 endpoint actually // returns when an existing appID is paired with the wrong secret — the // single most common real-world failure. Locked in by this test because the // original whitelist missed it. -func TestRunProbe_TATCode10014_EmitsWarning(t *testing.T) { +func TestRunProbe_TATCode10014_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(200, `{"code":10014,"msg":"app secret invalid"}`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code 10014)") { - t.Errorf("stderr missing code 10014: %q", errBuf.String()) - } + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 10014) } -func TestRunProbe_TATHTTP401_EmitsWarning(t *testing.T) { +func TestRunProbe_TATHTTP401_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(401, `unauthorized`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code HTTP 401)") { - t.Errorf("stderr missing HTTP 401 label: %q", errBuf.String()) - } + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 401) } -func TestRunProbe_TATHTTP403_EmitsWarning(t *testing.T) { +func TestRunProbe_TATHTTP403_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { return jsonResp(403, `forbidden`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code HTTP 403)") { - t.Errorf("stderr missing HTTP 403 label: %q", errBuf.String()) - } + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 403) } -func TestRunProbe_TATHTTP500_Silent(t *testing.T) { +// Policy: any non-zero TAT body code is treated as a deterministic +// credential-rejection signal (the server saw the payload and refused it), +// regardless of whether the specific code is one we recognize. This guards +// against upstream adding new credential-class codes without us noticing — +// previously, missing 10014 from a hand-rolled allowlist caused real-world +// "wrong app_secret" failures to go silent. The trade-off: ambiguous +// failures stay in NetworkError / InternalError lanes (transport / 5xx / +// parse / timeout) and remain silent, so valid configs are not disturbed. +func TestRunProbe_TATUnknownBodyCode_ReturnsAuthError(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(500, `server error`), nil + return jsonResp(200, `{"code":99999,"msg":"future-unknown"}`), nil }, } - f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + f, _ := fakeFactory(t, rt) + assertAuthError(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), 99999) +} + +// assertSilent asserts runProbe stayed quiet: no propagated error and nothing +// written to stderr. Used for every ambiguous (non-credential) outcome. +func assertSilent(t *testing.T, err error, errBuf *bytes.Buffer) { + t.Helper() + if err != nil { + t.Errorf("expected nil (silent), got error: %v", err) + } if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) + t.Errorf("expected no stderr output, got: %q", errBuf.String()) } } -func TestRunProbe_TATTransportError_Silent(t *testing.T) { +func TestRunProbe_TATHTTP500_Silent(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return nil, errors.New("network down") + return jsonResp(500, `server error`), nil }, } f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) - } + assertSilent(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf) } -// Policy: any non-zero TAT body code is treated as a deterministic -// credential-rejection signal (the server saw the payload and refused it), -// regardless of whether the specific code is one we recognize. This guards -// against upstream adding new credential-class codes without us noticing — -// previously, missing 10014 from a hand-rolled allowlist caused real-world -// "wrong app_secret" failures to go silent. The trade-off: ambiguous -// failures stay in NetworkError / InternalError lanes (transport / 5xx / -// parse / timeout) and remain silent, so valid configs are not disturbed. -func TestRunProbe_TATUnknownBodyCode_EmitsWarning(t *testing.T) { +func TestRunProbe_TATTransportError_Silent(t *testing.T) { rt := &fakeRT{ tatHandler: func(req *http.Request) (*http.Response, error) { - return jsonResp(200, `{"code":99999,"msg":"future-unknown"}`), nil + return nil, errors.New("network down") }, } f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if !strings.Contains(errBuf.String(), "(code 99999)") { - t.Errorf("stderr missing code 99999: %q", errBuf.String()) - } + assertSilent(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf) } func TestRunProbe_TATSuccess_ProbeFails_Silent(t *testing.T) { @@ -221,31 +239,29 @@ func TestRunProbe_TATSuccess_ProbeFails_Silent(t *testing.T) { }, } f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) if rt.probeCalls != 1 { t.Errorf("probe should be called once, got %d", rt.probeCalls) } - if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) - } + assertSilent(t, err, errBuf) } func TestRunProbe_TATSuccess_ProbeOK_Silent(t *testing.T) { rt := &fakeRT{} f, errBuf := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) if rt.tatCalls != 1 || rt.probeCalls != 1 { t.Errorf("expected 1/1 calls, got tat=%d probe=%d", rt.tatCalls, rt.probeCalls) } - if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) - } + assertSilent(t, err, errBuf) } func TestRunProbe_ProbeRequestShape(t *testing.T) { rt := &fakeRT{} f, _ := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + if err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu); err != nil { + t.Fatalf("unexpected error: %v", err) + } if rt.probeReq == nil { t.Fatal("probe request not captured") @@ -267,7 +283,9 @@ func TestRunProbe_ProbeRequestShape(t *testing.T) { func TestRunProbe_LarkBrand_HostRoutedCorrectly(t *testing.T) { rt := &fakeRT{} f, _ := fakeFactory(t, rt) - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandLark) + if err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandLark); err != nil { + t.Fatalf("unexpected error: %v", err) + } if rt.probeReq == nil { t.Fatal("probe request not captured") } @@ -282,10 +300,7 @@ func TestRunProbe_HTTPClientError_Silent(t *testing.T) { f.HttpClient = func() (*http.Client, error) { return nil, errors.New("client init failed") } - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) - if errBuf.Len() != 0 { - t.Errorf("expected silent, got: %q", errBuf.String()) - } + assertSilent(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf) } func TestRunProbe_TimeoutHonored(t *testing.T) { @@ -299,13 +314,13 @@ func TestRunProbe_TimeoutHonored(t *testing.T) { f, errBuf := fakeFactory(t, rt) start := time.Now() - runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) + err := runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu) elapsed := time.Since(start) if elapsed > 4*time.Second { t.Errorf("runProbe took %v, expected <= ~3s", elapsed) } - if errBuf.Len() != 0 { - t.Errorf("expected silent on timeout, got: %q", errBuf.String()) - } + // A timeout is an ambiguous failure (context deadline → not an + // AuthenticationError), so it must stay silent and not block. + assertSilent(t, err, errBuf) }