From 40de8a44dc4f0171ca083f182038fb8dca2e3f78 Mon Sep 17 00:00:00 2001 From: AlbertSun Date: Wed, 10 Jun 2026 21:35:05 +0800 Subject: [PATCH] opt(auth): validate auth-method at config resolution; document init back-compat - ResolveConfigFromMulti: reject unknown authMethod and require keyRef for private_key_jwt at resolution time (fail-fast vs. silent client_secret degrade or later token-signing failure) - init_interactive: comment why an empty SupportedAuthMethods intentionally allows the requested private_key_jwt (older-server back-compat, mirrors resolveFinalAuthMethod) - tests: invalid authMethod & missing keyRef resolution errors; empty SupportedAuthMethods init parse; explicit empty-slice resolveFinalAuthMethod --- cmd/config/init_auth_method_test.go | 7 +++ cmd/config/init_interactive.go | 5 +++ internal/auth/app_registration_test.go | 31 ++++++++++++++ internal/core/config.go | 17 ++++++++ internal/core/config_test.go | 59 ++++++++++++++++++++++++++ 5 files changed, 119 insertions(+) diff --git a/cmd/config/init_auth_method_test.go b/cmd/config/init_auth_method_test.go index 4d5e2ea95..65562940f 100644 --- a/cmd/config/init_auth_method_test.go +++ b/cmd/config/init_auth_method_test.go @@ -70,6 +70,13 @@ func TestResolveFinalAuthMethod(t *testing.T) { if m := resolveFinalAuthMethod(nil, core.AuthMethodPrivateKeyJWT); m != core.AuthMethodPrivateKeyJWT { t.Errorf("fallback to requested when server is silent: got %q", m) } + // Explicit empty slice (not just nil) also falls back to requested — the same + // len()==0 back-compat allowance the init guard relies on to let private_key_jwt + // proceed against an older server (see internal/auth + // TestRequestAppRegistrationInit_EmptySupportedAuthMethods). + if m := resolveFinalAuthMethod([]string{}, core.AuthMethodPrivateKeyJWT); m != core.AuthMethodPrivateKeyJWT { + t.Errorf("empty []string should fall back to requested private_key_jwt: got %q", m) + } if m := resolveFinalAuthMethod(nil, ""); m != core.AuthMethodClientSecret { t.Errorf("default to client_secret: got %q", m) } diff --git a/cmd/config/init_interactive.go b/cmd/config/init_interactive.go index 3e1794578..b5a8cdb13 100644 --- a/cmd/config/init_interactive.go +++ b/cmd/config/init_interactive.go @@ -250,6 +250,11 @@ func runCreateAppFlow(ctx context.Context, f *cmdutil.Factory, brandOverride cor if initErr != nil { return nil, errs.NewConfigError(errs.SubtypeInvalidClient, "app registration init failed: %v", initErr).WithCause(initErr) } + // An empty SupportedAuthMethods is intentionally treated as "older server / + // unknown": len()==0 makes this guard false, so the requested + // private_key_jwt proceeds. This mirrors resolveFinalAuthMethod's + // back-compat fallback to the requested method. Only an explicit list that + // omits private_key_jwt rejects here. if len(initResp.SupportedAuthMethods) > 0 && !slices.Contains(initResp.SupportedAuthMethods, core.AuthMethodPrivateKeyJWT) { return nil, errs.NewConfigError(errs.SubtypeInvalidClient, "server does not support private_key_jwt for this app type (supported: %s)", strings.Join(initResp.SupportedAuthMethods, ", ")). diff --git a/internal/auth/app_registration_test.go b/internal/auth/app_registration_test.go index c490df048..2a607224b 100644 --- a/internal/auth/app_registration_test.go +++ b/internal/auth/app_registration_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/url" + "slices" "strings" "testing" @@ -83,6 +84,36 @@ func TestRequestAppRegistrationInit_ErrorOnMissingNonce(t *testing.T) { } } +// TestRequestAppRegistrationInit_EmptySupportedAuthMethods covers the older-server +// back-compat path: an empty supported_auth_methods array parses to an empty +// slice, so the init guard in cmd/config/init_interactive.go +// (`len(SupportedAuthMethods) > 0 && !slices.Contains(...)`) stays false and does +// NOT reject the requested private_key_jwt. This aligns with +// resolveFinalAuthMethod(nil/[], private_key_jwt) == private_key_jwt +// (see cmd/config TestResolveFinalAuthMethod). +func TestRequestAppRegistrationInit_EmptySupportedAuthMethods(t *testing.T) { + var body url.Values + hc := captureClient(&body, `{"nonce":"n-1","supported_auth_methods":[]}`) + + out, err := RequestAppRegistrationInit(hc) + if err != nil { + t.Fatal(err) + } + if out.Nonce != "n-1" { + t.Errorf("nonce = %q, want n-1", out.Nonce) + } + if len(out.SupportedAuthMethods) != 0 { + t.Errorf("SupportedAuthMethods = %v, want empty", out.SupportedAuthMethods) + } + // Reproduce the init guard expression on the real parsed result: an empty + // slice must NOT reject private_key_jwt. + rejected := len(out.SupportedAuthMethods) > 0 && + !slices.Contains(out.SupportedAuthMethods, core.AuthMethodPrivateKeyJWT) + if rejected { + t.Error("empty SupportedAuthMethods must allow private_key_jwt (older-server back-compat)") + } +} + const beginRespJSON = `{"device_code":"dc","user_code":"uc","verification_uri":"https://example/verify","expires_in":300,"interval":5}` func TestRequestAppRegistration_BeginDefaultsToClientSecret(t *testing.T) { diff --git a/internal/core/config.go b/internal/core/config.go index 2a09809e5..98bae0cb9 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -278,6 +278,23 @@ func ResolveConfigFromMulti(raw *MultiAppConfig, kc keychain.KeychainAccess, pro } return nil, &ConfigError{Code: 3, Type: "config", Message: err.Error()} } + // Validate the auth method at resolution time so a malformed profile fails + // here rather than silently degrading to client_secret (unknown method) or + // failing later at token-signing (private_key_jwt without a key handle). + // Empty stays empty — downstream treats it as client_secret (back-compat). + switch app.AuthMethod { + case "", AuthMethodClientSecret, AuthMethodPrivateKeyJWT: + default: + return nil, &ConfigError{Code: 3, Type: "config", + Message: fmt.Sprintf("unknown authMethod %q", app.AuthMethod), + Hint: fmt.Sprintf("supported: %s, %s (empty defaults to %s)", AuthMethodClientSecret, AuthMethodPrivateKeyJWT, AuthMethodClientSecret)} + } + if app.AuthMethod == AuthMethodPrivateKeyJWT && app.KeyRef == nil { + return nil, &ConfigError{Code: 3, Type: "config", + Message: "private_key_jwt requires a key handle (keyRef) but none is configured", + Hint: "re-run: lark-cli config init --new --auth-method private_key_jwt"} + } + cfg := &CliConfig{ ProfileName: app.ProfileName(), AppID: app.AppId, diff --git a/internal/core/config_test.go b/internal/core/config_test.go index 3d727c504..87b931153 100644 --- a/internal/core/config_test.go +++ b/internal/core/config_test.go @@ -132,6 +132,65 @@ func TestResolveConfigFromMulti_AcceptsPlainSecret(t *testing.T) { } } +// TestResolveConfigFromMulti_RejectsUnknownAuthMethod ensures an unsupported +// authMethod fails at resolution rather than silently degrading to client_secret. +func TestResolveConfigFromMulti_RejectsUnknownAuthMethod(t *testing.T) { + raw := &MultiAppConfig{ + Apps: []AppConfig{ + { + AppId: "cli_abc", + AppSecret: PlainSecret("my-secret"), + Brand: BrandFeishu, + AuthMethod: "bogus_method", + }, + }, + } + + _, err := ResolveConfigFromMulti(raw, nil, "") + if err == nil { + t.Fatal("expected error for unknown authMethod") + } + var cfgErr *ConfigError + if !errors.As(err, &cfgErr) { + t.Fatalf("expected ConfigError, got %T: %v", err, err) + } +} + +// TestResolveConfigFromMulti_PrivateKeyJWTRequiresKeyRef ensures private_key_jwt +// without a key handle fails at resolution rather than later at token-signing. +func TestResolveConfigFromMulti_PrivateKeyJWTRequiresKeyRef(t *testing.T) { + raw := &MultiAppConfig{ + Apps: []AppConfig{ + { + AppId: "cli_abc", + AppSecret: SecretInput{}, // private_key_jwt carries no app secret + Brand: BrandFeishu, + AuthMethod: AuthMethodPrivateKeyJWT, + // KeyRef intentionally nil + }, + }, + } + + _, err := ResolveConfigFromMulti(raw, nil, "") + if err == nil { + t.Fatal("expected error for private_key_jwt without keyRef") + } + var cfgErr *ConfigError + if !errors.As(err, &cfgErr) { + t.Fatalf("expected ConfigError, got %T: %v", err, err) + } + + // Control: same config WITH a keyRef resolves cleanly and sets KeyLabel. + raw.Apps[0].KeyRef = &SecretRef{Source: "tee", ID: "larksuite-cli-agent"} + cfg, err := ResolveConfigFromMulti(raw, nil, "") + if err != nil { + t.Fatalf("unexpected error with keyRef present: %v", err) + } + if cfg.KeyLabel != "larksuite-cli-agent" { + t.Errorf("KeyLabel = %q, want larksuite-cli-agent", cfg.KeyLabel) + } +} + func TestResolveConfigFromMulti_MatchingKeychainRefPassesValidation(t *testing.T) { // Keychain ref matches appId, so validation passes. // The subsequent ResolveSecretInput will fail (no real keychain),