feat: add user_token_getter_url, people can get user token from app manager#686
Open
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add user_token_getter_url, people can get user token from app manager#686wangjingling wants to merge 1 commit intolarksuite:mainfrom
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces OAuth-like user token acquisition via Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant CLI as CLI App
participant LocalSvr as Local HTTP Server
participant UserAuth as UserAuth Handler
participant LarkAPI as Lark API
participant Callback as OAuthCallback Handler
User->>CLI: Run login with --user-token-getter-url
CLI->>LocalSvr: Start local callback server on 127.0.0.1:random
CLI->>LocalSvr: Embed port as state in getter URL
CLI->>User: Open browser to getter URL
User->>UserAuth: GET /authorize?state=PORT&...
UserAuth->>UserAuth: Parse state (port) & redirect_uri
UserAuth->>User: HTTP 302 to Lark authorization
User->>LarkAPI: Authorize and receive code
LarkAPI->>User: Redirect to callback URL with code
User->>Callback: GET /oauth_callback?code=...
Callback->>LarkAPI: Call Authen.AccessToken.Create(code)
LarkAPI->>Callback: Return access_token, user info
Callback->>User: Return HTML with client-side script
User->>LocalSvr: POST /user_access_token with token payload
LocalSvr->>CLI: Token received via channel
CLI->>CLI: Parse token, store credentials
CLI->>User: Login success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
…anager without app secret, then use lark-cli
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
69b9437 to
b9db67f
Compare
wangjingling
commented
Apr 27, 2026
Author
wangjingling
left a comment
wangjingling
left a comment
There was a problem hiding this comment.
add user_token_getter_url, people can get user token from app manager without app secret, then use lark-cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/drive/drive_task_result.go (1)
292-303:⚠️ Potential issue | 🟡 MinorClarify
Readysemantics for emptyMoveResults.“Ready indicates whether all wiki move operations succeeded” is slightly ambiguous because the method returns
falsewhenlen(s.MoveResults) == 0.📝 Proposed doc-only tweak
-// Ready indicates whether all wiki move operations in the task succeeded. +// Ready indicates whether all wiki move operations in the task succeeded. +// It returns false when MoveResults is empty. func (s wikiMoveTaskQueryStatus) Ready() bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_task_result.go` around lines 292 - 303, Update the doc comment for wikiMoveTaskQueryStatus.Ready to explicitly state that Ready returns true only when MoveResults is non-empty and every entry has Status == 0; clarify that an empty MoveResults slice yields false. Reference the wikiMoveTaskQueryStatus type, the Ready() method, and the MoveResults field so callers understand the empty-slice semantics without changing the method logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/get_user_token_url_demo_test.go`:
- Around line 19-24: The demo constants appID, appSecret, redirectURI and
authHost are incorrect/empty causing a malformed OAuth URL; update authHost to a
single valid host string (e.g. "open.feishu.cn" or "open.larksuite.com") instead
of "open.feishu.cn/open.larksuite.com", provide a non-empty redirectURI (use a
valid local/test redirect like "https://example.com/callback" or a clearly
marked placeholder), and ensure appID/appSecret are set to realistic demo
placeholders so the URL builder used in the test (referenced by the constants
appID, appSecret, redirectURI, authHost) produces a correct auth endpoint; apply
the same fixes for the repeated constants in the later block referenced (lines
~40-50).
- Around line 63-66: The code is incorrectly treating the OAuth `state` as the
localhost callback port (state -> stateInt) which breaks CSRF semantics; instead
generate and validate `state` as a random CSRF token and move the callback port
into a separate signed/opaque value (e.g., callbackToken or callbackInfo) that
you encode/verify server-side. Change the flow in the functions handling the GET
and POST (the places that parse `state` via strconv.Atoi and use stateInt as a
port) to: 1) expect `state` to be a random string and only validate it against
the stored/expected CSRF value, 2) read the callback port from a distinct
parameter (or from a signed payload) and verify its signature/expiry before
using it to bind the local listener, and 3) replace the existing error branch
that returns "invalid state parameter, must be integer" with proper CSRF/state
validation errors while moving any port-parsing (Atoi) to the separate callback
param handling code.
In `@cmd/auth/login_test.go`:
- Around line 911-1454: Tests currently only simulate the callback by GETting
/user_access_token?token=..., but production uses an HTTP POST with JSON body;
add tests that exercise the POST path to catch regressions: create one success
test (e.g., TestFetchTokenViaGetter_PostSuccess or extend
TestFetchTokenViaGetter_Success) that posts {"token":"<json-encoded-token>"} to
/user_access_token and verifies fetchTokenViaGetter and authLoginViaGetter
accept it, and one malformed-body test (e.g.,
TestFetchTokenViaGetter_PostInvalidJSON) that posts invalid JSON or wrong
content-type and asserts the appropriate error (use the same test helpers and
mocks, referencing fetchTokenViaGetter, authLoginViaGetter, the
/user_access_token callback endpoint, and openBrowserFn to drive the flow).
In `@cmd/auth/login.go`:
- Around line 448-463: Replace the randomized port probing loop that calls
net.Listen on "127.0.0.1:8000+rand" with a single atomic bind to "127.0.0.1:0"
(use net.Listen("tcp", "127.0.0.1:0")), drop rand.Seed and the for-loop; after
successful listen, obtain the assigned port from
listener.Addr().(*net.TCPAddr).Port and use that value for the callback URL/port
variable (the existing listener, port and err symbols are still used), and
update the error path to report failure to start the local server if listener is
nil or listen fails.
- Around line 447-561: The /user_access_token handler in fetchTokenViaGetter
currently trusts any request and uses a predictable state (port); change it to
generate a cryptographically random nonce (instead of or in addition to port),
include that nonce in the callback query as "state", and validate incoming
requests in the handler by: 1) requiring POST (reject other methods except
respond to OPTIONS) and only reading the body for token; 2) verifying the
request's state parameter or a JSON field matches the generated nonce before
accepting the token; and 3) removing the permissive Access-Control-Allow-Origin:
* and instead only allow the known getter origin (parsed from getterURL) or omit
CORS headers if not needed for same-origin local callbacks; make these changes
in fetchTokenViaGetter, the mux.HandleFunc("/user_access_token") closure, and
where the final callback URL is built so the nonce is embedded.
In `@cmd/config/config_test.go`:
- Around line 314-315: Add a positive test that passes a non-empty
userTokenGetterUrl through saveAsProfile and asserts it is
persisted/updated/preserved: call saveAsProfile (the same invocation currently
using noopConfigKeychain and keychain.KeychainAccess) with a non-empty getter
URL string instead of "" and then load the saved profile (or inspect the
returned config) to assert the userTokenGetterUrl field equals the provided URL;
add analogous assertions for the case where an existing secret/profile is
updated to ensure the URL is preserved when not changed and updated when a new
URL is provided (use the existing test helpers and symbols saveAsProfile,
noopConfigKeychain, keychain.KeychainAccess, and core.PlainSecret).
In `@cmd/config/init.go`:
- Around line 249-260: The current branch clears the stored app secret because
`secret` is only populated when `opts.appSecret` is provided, so calling
`saveInitConfig(..., secret, ...)` with an empty `secret` overwrites the
existing one; change the logic so that if `opts.appSecret` is empty but there is
an existing profile/app (the `existing` value used later), initialize `secret`
from that existing profile's stored secret (use the existing Secret value of
type core.SecretInput) and only call `core.ForStorage` when `opts.appSecret` is
non-empty; ensure `saveInitConfig` receives the existing secret when
adding/updating only `opts.UserTokenGetterUrl` so the saved secret is not
cleared.
In `@internal/core/config.go`:
- Around line 248-275: The code currently skips resolving AppSecret whenever
app.UserTokenGetterUrl is set, causing cfg.AppSecret to be empty even if a
stored secret exists; change the logic so you resolve and validate the stored
secret whenever app.AppSecret is non-empty regardless of UserTokenGetterUrl.
Concretely, replace the "if app.UserTokenGetterUrl == \"\" { ... }" block with a
branch that checks "if app.AppSecret != \"\" { call
ValidateSecretKeyMatch(app.AppId, app.AppSecret) and
ResolveSecretInput(app.AppSecret, kc) and handle errors exactly as done today }"
so CliConfig.AppSecret is populated when a secret is present, while leaving the
getter URL as an alternate source when no stored secret exists.
In `@shortcuts/drive/drive_task_result.go`:
- Line 220: Update the function comment for validateDriveTaskResultScopes to
describe its actual "best-effort" behavior instead of saying it "ensures"
scopes: state that it performs best-effort validation and may skip validation
(return nil) when token resolution fails for non-cancellation/non-timeout errors
or when the resolved token has empty/absent scopes (result.Scopes == ""), so
callers know validation can be bypassed in those situations.
---
Outside diff comments:
In `@shortcuts/drive/drive_task_result.go`:
- Around line 292-303: Update the doc comment for wikiMoveTaskQueryStatus.Ready
to explicitly state that Ready returns true only when MoveResults is non-empty
and every entry has Status == 0; clarify that an empty MoveResults slice yields
false. Reference the wikiMoveTaskQueryStatus type, the Ready() method, and the
MoveResults field so callers understand the empty-slice semantics without
changing the method logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 083cfe88-c84b-424a-b33e-75ab1528ba53
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/auth/get_user_token_url_demo_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_interactive.gogo.modinternal/core/config.gointernal/credential/types.goshortcuts/drive/drive_task_result.goskills/lark-shared/SKILL.md
Comment on lines
+19
to
+24
| const ( | ||
| appID = "" | ||
| appSecret = "" | ||
| redirectURI | ||
| authHost = "open.feishu.cn/open.larksuite.com" | ||
| ) |
There was a problem hiding this comment.
The demo currently builds a broken authorization URL.
authHost is set to open.feishu.cn/open.larksuite.com, so this code generates https://open.feishu.cn/open.larksuite.com/open-apis/authen/v1/index instead of a valid Feishu or Lark auth endpoint. redirectURI is also empty, so even the redirect params are incomplete. As written, this demo cannot drive a successful OAuth redirect.
Also applies to: 40-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url_demo_test.go` around lines 19 - 24, The demo
constants appID, appSecret, redirectURI and authHost are incorrect/empty causing
a malformed OAuth URL; update authHost to a single valid host string (e.g.
"open.feishu.cn" or "open.larksuite.com") instead of
"open.feishu.cn/open.larksuite.com", provide a non-empty redirectURI (use a
valid local/test redirect like "https://example.com/callback" or a clearly
marked placeholder), and ensure appID/appSecret are set to realistic demo
placeholders so the URL builder used in the test (referenced by the constants
appID, appSecret, redirectURI, authHost) produces a correct auth endpoint; apply
the same fixes for the repeated constants in the later block referenced (lines
~40-50).
Comment on lines
+63
to
+66
| stateInt, err := strconv.Atoi(state) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be integer"}) | ||
| return |
There was a problem hiding this comment.
Keep the callback port separate from OAuth state.
Here state is only validated as an integer and then reused as the localhost port that receives the token POST. That drops the normal request/response binding that state is supposed to provide and lets the caller choose which local service receives the token. Use an independent random state for CSRF protection and carry the callback port in a separate signed/opaque value.
Also applies to: 102-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url_demo_test.go` around lines 63 - 66, The code is
incorrectly treating the OAuth `state` as the localhost callback port (state ->
stateInt) which breaks CSRF semantics; instead generate and validate `state` as
a random CSRF token and move the callback port into a separate signed/opaque
value (e.g., callbackToken or callbackInfo) that you encode/verify server-side.
Change the flow in the functions handling the GET and POST (the places that
parse `state` via strconv.Atoi and use stateInt as a port) to: 1) expect `state`
to be a random string and only validate it against the stored/expected CSRF
value, 2) read the callback port from a distinct parameter (or from a signed
payload) and verify its signature/expiry before using it to bind the local
listener, and 3) replace the existing error branch that returns "invalid state
parameter, must be integer" with proper CSRF/state validation errors while
moving any port-parsing (Atoi) to the separate callback param handling code.
Comment on lines
+911
to
+1454
| func TestFetchTokenViaGetter_Success(t *testing.T) { | ||
| // Setup a mock getter server | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
|
|
||
| // Send token back to the callback URL asynchronously | ||
| go func() { | ||
| // Small delay to ensure the local server is fully started | ||
| // (although it should be since fetchTokenViaGetter waits) | ||
| http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=mock_token_data", state)) | ||
| }() | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprint(w, "Mock Getter") | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| // We need to temporarily disable openBrowser so it doesn't try to open a real browser | ||
| // Just for this test | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| // Instead of opening browser, directly query the getter server | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| // Open a goroutine to act as the browser for this specific test | ||
| // We intercept the log messages to find out what port to hit | ||
|
|
||
| token, err := fetchTokenViaGetter(ctx, getterURL, "test_scope", logFn) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("fetchTokenViaGetter failed: %v", err) | ||
| } | ||
|
|
||
| if token != "mock_token_data" { | ||
| t.Errorf("Expected token %q, got %q", "mock_token_data", token) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchTokenViaGetter_MissingToken(t *testing.T) { | ||
| // Setup a mock getter server that sends empty token | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
|
|
||
| go func() { | ||
| http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token", state)) | ||
| }() | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| // We need to temporarily disable openBrowser so it doesn't try to open a real browser | ||
| // Just for this test | ||
| originalOpenBrowser := openBrowserFn | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
| defer func() { openBrowserFn = originalOpenBrowser }() | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| _, err = fetchTokenViaGetter(ctx, getterURL, "test_scope", logFn) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("fetchTokenViaGetter should fail with missing token") | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), "missing token data in callback request") { | ||
| t.Errorf("Expected missing token error, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchTokenViaGetter_Timeout(t *testing.T) { | ||
| // Setup a mock getter server that does nothing | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| // Do nothing, wait for timeout | ||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| // We need to temporarily disable openBrowser so it doesn't try to open a real browser | ||
| // Just for this test | ||
| originalOpenBrowser := openBrowserFn | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
| defer func() { openBrowserFn = originalOpenBrowser }() | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| // Create a short timeout context to trigger the timeout quickly | ||
| ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) | ||
| defer cancel() | ||
|
|
||
| _, err = fetchTokenViaGetter(ctx, getterURL, "test_scope", logFn) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("fetchTokenViaGetter should fail with timeout") | ||
| } | ||
|
|
||
| if !strings.Contains(err.Error(), "timeout waiting for token callback") && !strings.Contains(err.Error(), "context canceled") { | ||
| t.Errorf("Expected timeout or context canceled error, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestAuthLoginViaGetter_SuccessWithAllFields(t *testing.T) { | ||
| keyring.MockInit() | ||
| setupLoginConfigDir(t) | ||
| t.Setenv("HOME", t.TempDir()) | ||
|
|
||
| multi := &core.MultiAppConfig{ | ||
| CurrentApp: "default", | ||
| Apps: []core.AppConfig{ | ||
| {Name: "default", AppId: "cli_test", UserTokenGetterUrl: "http://example.com/getter"}, | ||
| }, | ||
| } | ||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| t.Fatalf("SaveMultiAppConfig() error = %v", err) | ||
| } | ||
|
|
||
| f, stdout, stderr, _ := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| ProfileName: "default", | ||
| AppID: "cli_test", | ||
| UserTokenGetterUrl: "http://example.com/getter", | ||
| Brand: core.BrandFeishu, | ||
| }) | ||
|
|
||
| // Instead of hitting the real fetchTokenViaGetter which would block, | ||
| // we will run authLoginRun but we need a way to mock fetchTokenViaGetter. | ||
| // We can't easily mock fetchTokenViaGetter without changing the production code to accept a mock. | ||
| // So we test authLoginViaGetter directly, and mock the HTTP server for the callback. | ||
|
|
||
| // We simulate what authLoginRun does but use a mocked local server that answers fetchTokenViaGetter | ||
|
|
||
| // Mock openBrowserFn to prevent it from trying to open a real browser | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| // Setup a mock getter server | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
|
|
||
| go func() { | ||
| tokenData := `{"access_token":"mock_user_access_token","expires_in":7200,"name":"Mock User","open_id":"ou_mock"}` | ||
|
|
||
| // Try a few times in case the local server isn't up yet | ||
| for i := 0; i < 5; i++ { | ||
| resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=%s", state, url.QueryEscape(tokenData))) | ||
| if err == nil && resp.StatusCode == http.StatusOK { | ||
| resp.Body.Close() | ||
| break | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| // Update config to use our mock getter server | ||
| config := &core.CliConfig{ | ||
| ProfileName: "default", | ||
| AppID: "cli_test", | ||
| UserTokenGetterUrl: getterURL, | ||
| Brand: core.BrandFeishu, | ||
| } | ||
|
|
||
| opts := &LoginOptions{ | ||
| Factory: f, | ||
| Ctx: context.Background(), | ||
| Scope: "test_scope", | ||
| } | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| err = authLoginViaGetter(opts, config, "test_scope", getLoginMsg("en"), logFn) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("authLoginViaGetter failed: %v", err) | ||
| } | ||
|
|
||
| // Check that the token was stored correctly | ||
| stored := larkauth.GetStoredToken("cli_test", "ou_mock") | ||
| if stored == nil { | ||
| t.Fatal("expected token to be stored") | ||
| } | ||
| if stored.AccessToken != "mock_user_access_token" { | ||
| t.Errorf("expected access token 'mock_user_access_token', got '%s'", stored.AccessToken) | ||
| } | ||
|
|
||
| // Check the profile was updated | ||
| cfg, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| t.Fatalf("LoadMultiAppConfig() error = %v", err) | ||
| } | ||
| if len(cfg.Apps) != 1 || len(cfg.Apps[0].Users) != 1 { | ||
| t.Fatalf("unexpected users in config: %#v", cfg.Apps) | ||
| } | ||
| if cfg.Apps[0].Users[0].UserOpenId != "ou_mock" { | ||
| t.Fatalf("stored user open id = %q", cfg.Apps[0].Users[0].UserOpenId) | ||
| } | ||
| if cfg.Apps[0].Users[0].UserName != "Mock User" { | ||
| t.Fatalf("stored user name = %q", cfg.Apps[0].Users[0].UserName) | ||
| } | ||
|
|
||
| // stdout shouldn't have much for text mode, stderr is tested via writeLoginSuccess elsewhere | ||
| _ = stdout | ||
| _ = stderr | ||
| } | ||
|
|
||
| func TestAuthLoginViaGetter_FallbackGetUserInfo(t *testing.T) { | ||
| keyring.MockInit() | ||
| setupLoginConfigDir(t) | ||
| t.Setenv("HOME", t.TempDir()) | ||
|
|
||
| multi := &core.MultiAppConfig{ | ||
| CurrentApp: "default", | ||
| Apps: []core.AppConfig{ | ||
| {Name: "default", AppId: "cli_test", UserTokenGetterUrl: "http://example.com/getter"}, | ||
| }, | ||
| } | ||
| if err := core.SaveMultiAppConfig(multi); err != nil { | ||
| t.Fatalf("SaveMultiAppConfig() error = %v", err) | ||
| } | ||
|
|
||
| f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| ProfileName: "default", | ||
| AppID: "cli_test", | ||
| UserTokenGetterUrl: "http://example.com/getter", | ||
| Brand: core.BrandFeishu, | ||
| }) | ||
|
|
||
| // Setup mock for user info since the getter won't return open_id and name | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: larkauth.PathUserInfoV1, | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "msg": "ok", | ||
| "data": map[string]interface{}{ | ||
| "open_id": "ou_fallback", | ||
| "name": "Fallback User", | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| // Mock openBrowserFn to prevent it from trying to open a real browser | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| // Setup a mock getter server that returns ONLY access token | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
|
|
||
| go func() { | ||
| tokenData := `{"access_token":"mock_user_access_token_only"}` | ||
| for i := 0; i < 5; i++ { | ||
| resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=%s", state, url.QueryEscape(tokenData))) | ||
| if err == nil && resp.StatusCode == http.StatusOK { | ||
| resp.Body.Close() | ||
| break | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| config := &core.CliConfig{ | ||
| ProfileName: "default", | ||
| AppID: "cli_test", | ||
| UserTokenGetterUrl: getterURL, | ||
| Brand: core.BrandFeishu, | ||
| } | ||
|
|
||
| opts := &LoginOptions{ | ||
| Factory: f, | ||
| Ctx: context.Background(), | ||
| Scope: "test_scope", | ||
| } | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| err = authLoginViaGetter(opts, config, "test_scope", getLoginMsg("en"), logFn) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("authLoginViaGetter failed: %v", err) | ||
| } | ||
|
|
||
| // Check that the token was stored with the fallback info | ||
| stored := larkauth.GetStoredToken("cli_test", "ou_fallback") | ||
| if stored == nil { | ||
| t.Fatal("expected token to be stored") | ||
| } | ||
| if stored.AccessToken != "mock_user_access_token_only" { | ||
| t.Errorf("expected access token 'mock_user_access_token_only', got '%s'", stored.AccessToken) | ||
| } | ||
|
|
||
| // Check the profile was updated | ||
| cfg, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| t.Fatalf("LoadMultiAppConfig() error = %v", err) | ||
| } | ||
| if len(cfg.Apps) != 1 || len(cfg.Apps[0].Users) != 1 { | ||
| t.Fatalf("unexpected users in config: %#v", cfg.Apps) | ||
| } | ||
| if cfg.Apps[0].Users[0].UserOpenId != "ou_fallback" { | ||
| t.Fatalf("stored user open id = %q", cfg.Apps[0].Users[0].UserOpenId) | ||
| } | ||
| if cfg.Apps[0].Users[0].UserName != "Fallback User" { | ||
| t.Fatalf("stored user name = %q", cfg.Apps[0].Users[0].UserName) | ||
| } | ||
| } | ||
|
|
||
| func TestAuthLoginViaGetter_InvalidJSON(t *testing.T) { | ||
| f, _, _, _ := cmdutil.TestFactory(t, nil) | ||
|
|
||
| // Mock openBrowserFn to prevent it from trying to open a real browser | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
| go func() { | ||
| tokenData := `invalid_json` | ||
| for i := 0; i < 5; i++ { | ||
| resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=%s", state, url.QueryEscape(tokenData))) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| break | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() | ||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| config := &core.CliConfig{ | ||
| UserTokenGetterUrl: getterURL, | ||
| } | ||
|
|
||
| opts := &LoginOptions{ | ||
| Factory: f, | ||
| Ctx: context.Background(), | ||
| } | ||
|
|
||
| err = authLoginViaGetter(opts, config, "", getLoginMsg("en"), func(string, ...interface{}) {}) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("expected error due to invalid json") | ||
| } | ||
| if !strings.Contains(err.Error(), "failed to unmarshal token JSON") { | ||
| t.Errorf("expected json unmarshal error, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestAuthLoginViaGetter_NoAccessToken(t *testing.T) { | ||
| f, _, _, _ := cmdutil.TestFactory(t, nil) | ||
|
|
||
| // Mock openBrowserFn to prevent it from trying to open a real browser | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
| go func() { | ||
| tokenData := `{"name":"User Without Token"}` | ||
| for i := 0; i < 5; i++ { | ||
| resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=%s", state, url.QueryEscape(tokenData))) | ||
| if err == nil { | ||
| resp.Body.Close() | ||
| break | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() | ||
| w.WriteHeader(http.StatusOK) | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| config := &core.CliConfig{ | ||
| UserTokenGetterUrl: getterURL, | ||
| } | ||
|
|
||
| opts := &LoginOptions{ | ||
| Factory: f, | ||
| Ctx: context.Background(), | ||
| } | ||
|
|
||
| err = authLoginViaGetter(opts, config, "", getLoginMsg("en"), func(string, ...interface{}) {}) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("expected error due to missing access token") | ||
| } | ||
| if !strings.Contains(err.Error(), "no access_token returned") { | ||
| t.Errorf("expected no access_token error, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Cover the real POST callback contract.
These new getter tests only feed /user_access_token via GET ?token=..., but the shipped demo flow posts JSON to that endpoint. A regression in the POST-body path would still leave this suite green. Please add at least one success-case and one malformed-body test that send POST JSON to /user_access_token.
Based on learnings "Every behavior change must have an accompanying test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login_test.go` around lines 911 - 1454, Tests currently only
simulate the callback by GETting /user_access_token?token=..., but production
uses an HTTP POST with JSON body; add tests that exercise the POST path to catch
regressions: create one success test (e.g., TestFetchTokenViaGetter_PostSuccess
or extend TestFetchTokenViaGetter_Success) that posts
{"token":"<json-encoded-token>"} to /user_access_token and verifies
fetchTokenViaGetter and authLoginViaGetter accept it, and one malformed-body
test (e.g., TestFetchTokenViaGetter_PostInvalidJSON) that posts invalid JSON or
wrong content-type and asserts the appropriate error (use the same test helpers
and mocks, referencing fetchTokenViaGetter, authLoginViaGetter, the
/user_access_token callback endpoint, and openBrowserFn to drive the flow).
Comment on lines
+447
to
+561
| func fetchTokenViaGetter(ctx context.Context, getterURL string, scope string, log func(string, ...interface{})) (string, error) { | ||
| rand.Seed(time.Now().UnixNano()) | ||
| var listener net.Listener | ||
| var port int | ||
| var err error | ||
|
|
||
| for i := 0; i < 10; i++ { | ||
| p := 8000 + rand.Intn(2000) | ||
| listener, err = net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", p)) | ||
| if err == nil { | ||
| port = p | ||
| break | ||
| } | ||
| } | ||
| if listener == nil { | ||
| return "", fmt.Errorf("failed to start local server for token retrieval: %w", err) | ||
| } | ||
|
|
||
| tokenCh := make(chan string, 1) | ||
| errCh := make(chan error, 1) | ||
|
|
||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/user_access_token", func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
| w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS") | ||
| w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization") | ||
|
|
||
| if r.Method == http.MethodOptions { | ||
| w.WriteHeader(http.StatusOK) | ||
| return | ||
| } | ||
|
|
||
| var tokenData string | ||
| if r.Method == http.MethodPost { | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| http.Error(w, "failed to read body", http.StatusBadRequest) | ||
| return | ||
| } | ||
| tokenData = string(body) | ||
| } else { | ||
| tokenData = r.URL.Query().Get("token") | ||
| } | ||
|
|
||
| if tokenData == "" { | ||
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprint(w, "<html><body><h2>Failed</h2><p>Missing token data</p></body></html>") | ||
| select { | ||
| case errCh <- fmt.Errorf("missing token data in callback request"): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| w.Header().Set("Content-Type", "text/html; charset=utf-8") | ||
| fmt.Fprint(w, `<html><body style="text-align:center;padding-top:100px;font-family:sans-serif"> | ||
| <h2>✓ Success</h2><p>You can close this page and return to the terminal.</p></body></html>`) | ||
|
|
||
| select { | ||
| case tokenCh <- tokenData: | ||
| default: | ||
| } | ||
| }) | ||
|
|
||
| server := &http.Server{Handler: mux} | ||
| go func() { | ||
| if err := server.Serve(listener); err != nil && err != http.ErrServerClosed { | ||
| select { | ||
| case errCh <- fmt.Errorf("local server error: %w", err): | ||
| default: | ||
| } | ||
| } | ||
| }() | ||
| defer func() { | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| server.Shutdown(shutdownCtx) | ||
| }() | ||
|
|
||
| log("Waiting for authorization, local server started on http://127.0.0.1:%d/user_access_token...", port) | ||
|
|
||
| u, err := url.Parse(getterURL) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to parse getterURL: %w", err) | ||
| } | ||
| q := u.Query() | ||
| q.Set("state", strconv.Itoa(port)) | ||
| if scope != "" { | ||
| q.Set("scope", scope) | ||
| } | ||
| u.RawQuery = q.Encode() | ||
| finalURL := u.String() | ||
|
|
||
| if err := openBrowserFn(ctx, finalURL); err != nil { | ||
| log("Could not open browser automatically. Please visit the following link manually:") | ||
| } else { | ||
| log("Opening browser to get token...") | ||
| log("If the browser does not open automatically, please visit:") | ||
| } | ||
| log(" %s\n", finalURL) | ||
|
|
||
| timer := time.NewTimer(5 * time.Minute) | ||
| defer timer.Stop() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return "", fmt.Errorf("context canceled: %w", ctx.Err()) | ||
| case token := <-tokenCh: | ||
| return token, nil | ||
| case err := <-errCh: | ||
| return "", err | ||
| case <-timer.C: | ||
| return "", fmt.Errorf("timeout waiting for token callback (5 minutes)") |
There was a problem hiding this comment.
Authenticate the localhost callback before accepting a token.
The listener accepts the first request to /user_access_token from any origin, and state is only the port number. That means any page running in the user's browser can race a POST into this localhost server and inject an arbitrary token/account into the CLI profile. Please bind the callback to an unguessable nonce and validate it before persisting anything, and tighten this endpoint to the configured getter origin / POST-only instead of Access-Control-Allow-Origin: *.
Possible hardening direction
- q.Set("state", strconv.Itoa(port))
+ q.Set("state", strconv.Itoa(port))
+ q.Set("callback_nonce", nonce)
mux.HandleFunc("/user_access_token", func(w http.ResponseWriter, r *http.Request) {
- w.Header().Set("Access-Control-Allow-Origin", "*")
- w.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS")
+ w.Header().Set("Access-Control-Allow-Origin", getterOrigin)
+ w.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS")
if r.Method == http.MethodOptions {
w.WriteHeader(http.StatusOK)
return
}
+ if r.Method != http.MethodPost {
+ http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
+ return
+ }
+ if r.URL.Query().Get("callback_nonce") != nonce {
+ http.Error(w, "invalid callback nonce", http.StatusForbidden)
+ return
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 447 - 561, The /user_access_token handler in
fetchTokenViaGetter currently trusts any request and uses a predictable state
(port); change it to generate a cryptographically random nonce (instead of or in
addition to port), include that nonce in the callback query as "state", and
validate incoming requests in the handler by: 1) requiring POST (reject other
methods except respond to OPTIONS) and only reading the body for token; 2)
verifying the request's state parameter or a JSON field matches the generated
nonce before accepting the token; and 3) removing the permissive
Access-Control-Allow-Origin: * and instead only allow the known getter origin
(parsed from getterURL) or omit CORS headers if not needed for same-origin local
callbacks; make these changes in fetchTokenViaGetter, the
mux.HandleFunc("/user_access_token") closure, and where the final callback URL
is built so the nonce is embedded.
Comment on lines
+448
to
+463
| rand.Seed(time.Now().UnixNano()) | ||
| var listener net.Listener | ||
| var port int | ||
| var err error | ||
|
|
||
| for i := 0; i < 10; i++ { | ||
| p := 8000 + rand.Intn(2000) | ||
| listener, err = net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", p)) | ||
| if err == nil { | ||
| port = p | ||
| break | ||
| } | ||
| } | ||
| if listener == nil { | ||
| return "", fmt.Errorf("failed to start local server for token retrieval: %w", err) | ||
| } |
There was a problem hiding this comment.
Let the OS allocate the callback port.
Probing 10 random ports in 8000-9999 can fail even when plenty of other local ports are available, which makes the new login path flaky on busy dev machines. Binding 127.0.0.1:0 is atomic and removes the retry/seed logic.
Suggested fix
- rand.Seed(time.Now().UnixNano())
- var listener net.Listener
- var port int
- var err error
-
- for i := 0; i < 10; i++ {
- p := 8000 + rand.Intn(2000)
- listener, err = net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", p))
- if err == nil {
- port = p
- break
- }
- }
- if listener == nil {
- return "", fmt.Errorf("failed to start local server for token retrieval: %w", err)
- }
+ listener, err := net.Listen("tcp", "127.0.0.1:0")
+ if err != nil {
+ return "", fmt.Errorf("failed to start local server for token retrieval: %w", err)
+ }
+ port := listener.Addr().(*net.TCPAddr).Port🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 448 - 463, Replace the randomized port
probing loop that calls net.Listen on "127.0.0.1:8000+rand" with a single atomic
bind to "127.0.0.1:0" (use net.Listen("tcp", "127.0.0.1:0")), drop rand.Seed and
the for-loop; after successful listen, obtain the assigned port from
listener.Addr().(*net.TCPAddr).Port and use that value for the callback URL/port
variable (the existing listener, port and err symbols are still used), and
update the error path to report failure to start the local server if listener is
nil or listen fails.
Comment on lines
+314
to
315
| err := saveAsProfile(existing, keychain.KeychainAccess(&noopConfigKeychain{}), "cli_prod", "app-new", core.PlainSecret("new-secret"), core.BrandLark, "en", "") | ||
| if err == nil { |
There was a problem hiding this comment.
Add a positive userTokenGetterUrl coverage case.
These edits only thread "" through the new parameter. They still do not assert that a non-empty getter URL is persisted, updated, or preserved alongside an existing secret, so the new config behavior can regress silently.
As per coding guidelines, "Every behavior change must have an accompanying test."
Also applies to: 338-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/config_test.go` around lines 314 - 315, Add a positive test that
passes a non-empty userTokenGetterUrl through saveAsProfile and asserts it is
persisted/updated/preserved: call saveAsProfile (the same invocation currently
using noopConfigKeychain and keychain.KeychainAccess) with a non-empty getter
URL string instead of "" and then load the saved profile (or inspect the
returned config) to assert the userTokenGetterUrl field equals the provided URL;
add analogous assertions for the case where an existing secret/profile is
updated to ensure the URL is preserved when not changed and updated when a new
URL is provided (use the existing test helpers and symbols saveAsProfile,
noopConfigKeychain, keychain.KeychainAccess, and core.PlainSecret).
Comment on lines
+248
to
275
| var secret string | ||
| var err error | ||
| if app.UserTokenGetterUrl == "" { | ||
| if err := ValidateSecretKeyMatch(app.AppId, app.AppSecret); err != nil { | ||
| return nil, &ConfigError{Code: 2, Type: "config", | ||
| Message: "appId and appSecret keychain key are out of sync", | ||
| Hint: err.Error()} | ||
| } | ||
|
|
||
| secret, err := ResolveSecretInput(app.AppSecret, kc) | ||
| if err != nil { | ||
| // If the error comes from the keychain, it will already be wrapped as an ExitError. | ||
| // For other errors (e.g. file read errors, unknown sources), wrap them as ConfigError. | ||
| var exitErr *output.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| return nil, exitErr | ||
| secret, err = ResolveSecretInput(app.AppSecret, kc) | ||
| if err != nil { | ||
| // If the error comes from the keychain, it will already be wrapped as an ExitError. | ||
| // For other errors (e.g. file read errors, unknown sources), wrap them as ConfigError. | ||
| var exitErr *output.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| return nil, exitErr | ||
| } | ||
| return nil, &ConfigError{Code: 2, Type: "config", Message: err.Error()} | ||
| } | ||
| return nil, &ConfigError{Code: 2, Type: "config", Message: err.Error()} | ||
| } | ||
| cfg := &CliConfig{ | ||
| ProfileName: app.ProfileName(), | ||
| AppID: app.AppId, | ||
| AppSecret: secret, | ||
| Brand: app.Brand, | ||
| DefaultAs: app.DefaultAs, | ||
| ProfileName: app.ProfileName(), | ||
| AppID: app.AppId, | ||
| AppSecret: secret, | ||
| Brand: app.Brand, | ||
| DefaultAs: app.DefaultAs, | ||
| UserTokenGetterUrl: app.UserTokenGetterUrl, | ||
| } |
There was a problem hiding this comment.
Still resolve AppSecret when a getter URL is also configured.
This branch treats UserTokenGetterUrl as a full replacement for the stored secret, so profiles that contain both values end up with cfg.AppSecret == "". That breaks any downstream path that still needs the app secret even though the config still has one. The getter URL should bypass secret resolution only when the profile truly has no stored secret.
Possible fix
var secret string
var err error
- if app.UserTokenGetterUrl == "" {
+ if !app.AppSecret.IsZero() {
if err := ValidateSecretKeyMatch(app.AppId, app.AppSecret); err != nil {
return nil, &ConfigError{Code: 2, Type: "config",
Message: "appId and appSecret keychain key are out of sync",
Hint: err.Error()}
}
secret, err = ResolveSecretInput(app.AppSecret, kc)
if err != nil {
var exitErr *output.ExitError
if errors.As(err, &exitErr) {
return nil, exitErr
}
return nil, &ConfigError{Code: 2, Type: "config", Message: err.Error()}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config.go` around lines 248 - 275, The code currently skips
resolving AppSecret whenever app.UserTokenGetterUrl is set, causing
cfg.AppSecret to be empty even if a stored secret exists; change the logic so
you resolve and validate the stored secret whenever app.AppSecret is non-empty
regardless of UserTokenGetterUrl. Concretely, replace the "if
app.UserTokenGetterUrl == \"\" { ... }" block with a branch that checks "if
app.AppSecret != \"\" { call ValidateSecretKeyMatch(app.AppId, app.AppSecret)
and ResolveSecretInput(app.AppSecret, kc) and handle errors exactly as done
today }" so CliConfig.AppSecret is populated when a secret is present, while
leaving the getter URL as an alternate source when no stored secret exists.
| }, nil | ||
| } | ||
|
|
||
| // validateDriveTaskResultScopes ensures the user has authorized the scopes required for specific drive tasks. |
There was a problem hiding this comment.
Make the scope-validation comment match the function’s “best-effort” behavior.
The current comment says it “ensures” required scopes, but validateDriveTaskResultScopes returns nil (skips validation) when:
- token resolution fails for non-cancellation/non-timeout errors, and/or
- the resolved token has empty/absent scopes (
result.Scopes == "").
📝 Proposed doc-only tweak
-// validateDriveTaskResultScopes ensures the user has authorized the scopes required for specific drive tasks.
+// validateDriveTaskResultScopes performs best-effort scope validation for specific drive tasks.
+// If token scope data can't be resolved (non-fatal token errors, empty scopes), validation is skipped.
func validateDriveTaskResultScopes(ctx context.Context, runtime *common.RuntimeContext, scenario string) error {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // validateDriveTaskResultScopes ensures the user has authorized the scopes required for specific drive tasks. | |
| // validateDriveTaskResultScopes performs best-effort scope validation for specific drive tasks. | |
| // If token scope data can't be resolved (non-fatal token errors, empty scopes), validation is skipped. | |
| func validateDriveTaskResultScopes(ctx context.Context, runtime *common.RuntimeContext, scenario string) error { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/drive/drive_task_result.go` at line 220, Update the function
comment for validateDriveTaskResultScopes to describe its actual "best-effort"
behavior instead of saying it "ensures" scopes: state that it performs
best-effort validation and may skip validation (return nil) when token
resolution fails for non-cancellation/non-timeout errors or when the resolved
token has empty/absent scopes (result.Scopes == ""), so callers know validation
can be bypassed in those situations.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/config/init.go (1)
309-313:⚠️ Potential issue | 🟡 Minor
--newflow persistsuserTokenGetterUrlbut omits it from the success JSON.Line 309 forwards
opts.UserTokenGetterUrlintosaveInitConfig, yet the JSON envelope on Line 312 reports onlyappId/appSecret/brand. AI agents parsing the output have no way to confirm the URL was stored. Align this with the non‑interactive branch (Lines 264‑270), which conditionally addsuserTokenGetterUrl.♻️ Suggested fix
- output.PrintJson(f.IOStreams.Out, map[string]interface{}{"appId": result.AppID, "appSecret": "****", "brand": result.Brand}) + outputMap := map[string]interface{}{"appId": result.AppID, "appSecret": "****", "brand": result.Brand} + if opts.UserTokenGetterUrl != "" { + outputMap["userTokenGetterUrl"] = opts.UserTokenGetterUrl + } + output.PrintJson(f.IOStreams.Out, outputMap)As per coding guidelines: "Design CLI flags, help text, and error messages with AI agent consumption in mind".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 309 - 313, The success JSON after the interactive `--new` flow omits the stored userTokenGetterUrl even though `opts.UserTokenGetterUrl` is passed into `saveInitConfig`; update the `output.PrintJson` call (near the `saveInitConfig` usage) to include a "userTokenGetterUrl" field when `opts.UserTokenGetterUrl` is non-empty, matching the conditional behavior used in the non‑interactive branch (the code around the earlier `output.PrintJson` block and the `saveInitConfig` call references these symbols), so AI agents can confirm the URL was persisted.
♻️ Duplicate comments (1)
cmd/config/init.go (1)
249-273:⚠️ Potential issue | 🟠 MajorExisting app secret is wiped when only
--user-token-getter-urlis supplied.When
opts.AppIDmatches an existing profile and the caller passes only--user-token-getter-url(no--app-secret-stdin),secretstays as a zerocore.SecretInput.saveInitConfig→saveAsProfile/saveAsOnlyAppthen writes that zero value overmulti.Apps[idx].AppSecretand (for the single-app path) callscleanupOldConfigwhich removes the keychain entry. The resulting config has the new getter URL but a blank app secret, breaking subsequent auth flows that still need it.Populate
secretfrom the existing profile whenopts.appSecretis empty:🛡️ Suggested fix
brand := parseBrand(opts.Brand) var secret core.SecretInput if opts.appSecret != "" { var err error secret, err = core.ForStorage(opts.AppID, core.PlainSecret(opts.appSecret), f.Keychain) if err != nil { return output.Errorf(output.ExitInternal, "internal", "%v", err) } + } else if existing != nil { + if opts.ProfileName != "" { + if idx := findProfileIndexByName(existing, opts.ProfileName); idx >= 0 && existing.Apps[idx].AppId == opts.AppID { + secret = existing.Apps[idx].AppSecret + } + } else if app := existing.CurrentAppConfig(""); app != nil && app.AppId == opts.AppID { + secret = app.AppSecret + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 249 - 273, When opts.AppID is provided but opts.appSecret is empty and opts.UserTokenGetterUrl is set, the local variable secret remains zero and overwrites the stored secret during saveInitConfig -> saveAsProfile/saveAsOnlyApp; fix by populating secret from the existing profile before calling saveInitConfig: if opts.appSecret == "" and existing != nil (and existing contains the matching app entry for opts.AppID), set secret to the existing app's secret (convert/copy into a core.SecretInput) so multi.Apps[idx].AppSecret and keychain entries are preserved and cleanupOldConfig is not triggered to remove the keychain secret.
🧹 Nitpick comments (3)
cmd/config/init.go (3)
65-65: Tighten the flag description.Two small polish items: capitalize "URL" for consistency with the rest of the codebase prose (e.g., the prompt "UserTokenGetterUrl (Optional)" on Line 409), and clarify that the URL is an alternative to
--app-secret-stdinrather than only a fallback whenapp-secretis missing.♻️ Suggested wording
- cmd.Flags().StringVar(&opts.UserTokenGetterUrl, "user-token-getter-url", "", "Custom url to fetch user token when app-secret is not provided") + cmd.Flags().StringVar(&opts.UserTokenGetterUrl, "user-token-getter-url", "", "Custom URL that returns a user access token (alternative to --app-secret-stdin / device authorization)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` at line 65, Update the flag description for cmd.Flags().StringVar that sets opts.UserTokenGetterUrl ("user-token-getter-url") to capitalize "URL" and clarify purpose: state that this URL is an alternative way to fetch the user token when not providing the app secret via --app-secret-stdin (instead of implying it only falls back when app-secret is missing). Keep the message concise and consistent with other help text (e.g., "Custom URL to fetch user token as an alternative to --app-secret-stdin").
211-213: No way to clearUserTokenGetterUrlonce set.
updateExistingProfileWithoutSecretonly writesUserTokenGetterUrlwhen the new value is non‑empty, so passing an empty string preserves the prior URL. If that's intentional (treat empty input as "leave unchanged"), please document it; otherwise consider a sentinel (e.g.,"-") or a separate--clear-user-token-getter-urlflag so users can revert to the device‑auth flow without hand‑editingconfig.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 211 - 213, The current updateExistingProfileWithoutSecret flow writes UserTokenGetterUrl only when userTokenGetterUrl is non-empty, preventing clearing an existing value; update the logic in updateExistingProfileWithoutSecret to support clearing by either adding a new flag (e.g., --clear-user-token-getter-url) or accepting a sentinel value (e.g., "-" ) and when that flag/sentinel is present explicitly set app.UserTokenGetterUrl = "" (otherwise preserve current behavior of only writing non-empty values); make sure the change checks the new flag/sentinel before the existing len(userTokenGetterUrl) > 0 branch and document the behavior in help text if you choose to treat empty string as "leave unchanged".
218-273: Add tests for the non-interactive URL-only and conditional output branches.The non-interactive flow supports three execution shapes:
appSecretonly, URL only, and both. The JSON output keys forappSecretanduserTokenGetterUrlare conditional (lines 265–269). Currently, only oneconfigInitRuntest exists (TestConfigInitRun_NonTerminal_NoFlags_RejectsWithHint), which covers a non-terminal error path. Per coding guidelines ("Every behavior change must have an accompanying test"), add coverage for: (a) URL-only init creating a new profile, (b) URL-only init updating an existing profile, and (c) the conditional JSON output keys in the success envelope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 218 - 273, Add unit tests for configInitRun to cover the non-interactive URL-only and conditional JSON output branches: implement tests that call configInitRun with AppID and UserTokenGetterUrl but no appSecret to (1) create a new profile (simulate no existing config via core.LoadMultiAppConfig returning nil) and assert saveInitConfig was invoked and output.PrintJson contains appId and userTokenGetterUrl but not appSecret, (2) update an existing profile (simulate core.LoadMultiAppConfig returning an existing config) and assert the existing profile was updated and output JSON still only contains appId and userTokenGetterUrl, and (3) verify the conditional JSON keys by running configInitRun twice—once with AppID+appSecret and once with AppID+UserTokenGetterUrl—and assert output.PrintJson includes "appSecret":"****" only when opts.appSecret is set and includes "userTokenGetterUrl" only when UserTokenGetterUrl is set; use or extend existing test helpers/mocks for f.Factory, saveInitConfig, core.LoadMultiAppConfig, and output.PrintJson to capture calls and outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/get_user_token_url_demo_test.go`:
- Around line 102-131: The HTML template embeds JSON unsafely via
string(dataBytes) into the script (variable html); fix by re-encoding dataBytes
with a json.Encoder that has SetEscapeHTML(true) into a bytes.Buffer (or
re-marshal via an Encoder) and then replace any literal "</" with "<\/" before
interpolating into the template to prevent breaking out of the script context;
use that escaped JSON in place of string(dataBytes). Also correct the UI string
"Token sended，close in 3 seconds..." to "Token sent, close in 3 seconds..."
(replace the full‑width comma with an ASCII comma).
- Around line 1-17: The demo handlers UserAuth and OAuthCallback are in a
*_test.go file so they are not visible to non-test code; either rename the file
to get_user_token_url_demo.go and add a build tag (e.g. add a first line
//go:build demo and the matching // +build demo) so it becomes optional demo
code, or keep the current filename and add proper Test functions (func
TestUserAuthDemo(t *testing.T) / func TestOAuthCallbackDemo(t *testing.T)) that
exercise UserAuth and OAuthCallback; update references to these symbols
accordingly so they are reachable as intended.
In `@cmd/config/init.go`:
- Around line 328-336: The interactive TUI and legacy readline flows call
core.ForStorage(result.AppSecret, ...) unconditionally allowing an empty
AppSecret to be written; mirror the non-interactive flag guard and only call
core.ForStorage when result.AppSecret != "" (or opts.appSecret != ""), otherwise
set the secret value to nil and pass that into saveInitConfig; update both the
existing==nil block where core.ForStorage is invoked and the analogous legacy
readline block to skip keychain writes when AppSecret is empty.
---
Outside diff comments:
In `@cmd/config/init.go`:
- Around line 309-313: The success JSON after the interactive `--new` flow omits
the stored userTokenGetterUrl even though `opts.UserTokenGetterUrl` is passed
into `saveInitConfig`; update the `output.PrintJson` call (near the
`saveInitConfig` usage) to include a "userTokenGetterUrl" field when
`opts.UserTokenGetterUrl` is non-empty, matching the conditional behavior used
in the non‑interactive branch (the code around the earlier `output.PrintJson`
block and the `saveInitConfig` call references these symbols), so AI agents can
confirm the URL was persisted.
---
Duplicate comments:
In `@cmd/config/init.go`:
- Around line 249-273: When opts.AppID is provided but opts.appSecret is empty
and opts.UserTokenGetterUrl is set, the local variable secret remains zero and
overwrites the stored secret during saveInitConfig ->
saveAsProfile/saveAsOnlyApp; fix by populating secret from the existing profile
before calling saveInitConfig: if opts.appSecret == "" and existing != nil (and
existing contains the matching app entry for opts.AppID), set secret to the
existing app's secret (convert/copy into a core.SecretInput) so
multi.Apps[idx].AppSecret and keychain entries are preserved and
cleanupOldConfig is not triggered to remove the keychain secret.
---
Nitpick comments:
In `@cmd/config/init.go`:
- Line 65: Update the flag description for cmd.Flags().StringVar that sets
opts.UserTokenGetterUrl ("user-token-getter-url") to capitalize "URL" and
clarify purpose: state that this URL is an alternative way to fetch the user
token when not providing the app secret via --app-secret-stdin (instead of
implying it only falls back when app-secret is missing). Keep the message
concise and consistent with other help text (e.g., "Custom URL to fetch user
token as an alternative to --app-secret-stdin").
- Around line 211-213: The current updateExistingProfileWithoutSecret flow
writes UserTokenGetterUrl only when userTokenGetterUrl is non-empty, preventing
clearing an existing value; update the logic in
updateExistingProfileWithoutSecret to support clearing by either adding a new
flag (e.g., --clear-user-token-getter-url) or accepting a sentinel value (e.g.,
"-" ) and when that flag/sentinel is present explicitly set
app.UserTokenGetterUrl = "" (otherwise preserve current behavior of only writing
non-empty values); make sure the change checks the new flag/sentinel before the
existing len(userTokenGetterUrl) > 0 branch and document the behavior in help
text if you choose to treat empty string as "leave unchanged".
- Around line 218-273: Add unit tests for configInitRun to cover the
non-interactive URL-only and conditional JSON output branches: implement tests
that call configInitRun with AppID and UserTokenGetterUrl but no appSecret to
(1) create a new profile (simulate no existing config via
core.LoadMultiAppConfig returning nil) and assert saveInitConfig was invoked and
output.PrintJson contains appId and userTokenGetterUrl but not appSecret, (2)
update an existing profile (simulate core.LoadMultiAppConfig returning an
existing config) and assert the existing profile was updated and output JSON
still only contains appId and userTokenGetterUrl, and (3) verify the conditional
JSON keys by running configInitRun twice—once with AppID+appSecret and once with
AppID+UserTokenGetterUrl—and assert output.PrintJson includes "appSecret":"****"
only when opts.appSecret is set and includes "userTokenGetterUrl" only when
UserTokenGetterUrl is set; use or extend existing test helpers/mocks for
f.Factory, saveInitConfig, core.LoadMultiAppConfig, and output.PrintJson to
capture calls and outputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68bf4483-6b53-449f-8034-c45c625a8e9a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/auth/get_user_token_url_demo_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_interactive.gogo.modinternal/core/config.gointernal/credential/types.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (5)
- skills/lark-shared/SKILL.md
- cmd/config/config_test.go
- internal/credential/types.go
- cmd/auth/login.go
- cmd/auth/login_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/config.go
- cmd/config/init_interactive.go
Comment on lines
+1
to
+17
| package auth | ||
|
|
||
| // this file is a demo for user_token_getter_url | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
| "strconv" | ||
|
|
||
| "github.com/cloudwego/hertz/pkg/app" | ||
| "github.com/cloudwego/hertz/pkg/common/utils" | ||
| lark "github.com/larksuite/oapi-sdk-go/v3" | ||
| larkauthen "github.com/larksuite/oapi-sdk-go/v3/service/authen/v1" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there are no Test* functions in this file and that the demo symbols
# are not referenced from any non-test production source.
set -euo pipefail
echo "---- Test* funcs in the demo file ----"
rg -n '^func Test' cmd/auth/get_user_token_url_demo_test.go || echo "(none)"
echo "---- references to UserAuth/OAuthCallback outside *_test.go ----"
rg -nP '\b(UserAuth|OAuthCallback)\b' --type=go -g '!**/*_test.go' || echo "(none)"Repository: larksuite/cli
Length of output: 175
🏁 Script executed:
cat -n cmd/auth/get_user_token_url_demo_test.goRepository: larksuite/cli
Length of output: 4617
🏁 Script executed:
fd -t f 'get_user_token_url_demo' cmd/auth/Repository: larksuite/cli
Length of output: 98
🏁 Script executed:
rg -l 'UserAuth|OAuthCallback' --type=go --type=mdRepository: larksuite/cli
Length of output: 98
🏁 Script executed:
rg -l 'get_user_token_url_demo|UserAuth|OAuthCallback' --type=md docs/ README* 2>/dev/null || echo "No doc references found"Repository: larksuite/cli
Length of output: 81
🏁 Script executed:
rg -A3 -B3 'user.?token|oauth.?callback' tests/ --type=go -i 2>/dev/null | head -30 || echo "No related tests found"Repository: larksuite/cli
Length of output: 2068
Rename file or add tests to fix unreachable demo handlers.
This file is named get_user_token_url_demo_test.go but contains no func TestXxx(t *testing.T) — only exported handlers UserAuth and OAuthCallback. In Go, identifiers in *_test.go files are visible only to other *_test.go files in the same package. These handlers cannot be referenced from production code or from any external consumer.
The file either needs to be renamed to get_user_token_url_demo.go with a build tag (//go:build demo) to make it reachable as optional demo code, or it should contain actual Test* functions that exercise these handlers. Currently it violates the guideline that every behavior change must have an accompanying test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url_demo_test.go` around lines 1 - 17, The demo
handlers UserAuth and OAuthCallback are in a *_test.go file so they are not
visible to non-test code; either rename the file to get_user_token_url_demo.go
and add a build tag (e.g. add a first line //go:build demo and the matching //
+build demo) so it becomes optional demo code, or keep the current filename and
add proper Test functions (func TestUserAuthDemo(t *testing.T) / func
TestOAuthCallbackDemo(t *testing.T)) that exercise UserAuth and OAuthCallback;
update references to these symbols accordingly so they are reachable as
intended.
Comment on lines
+102
to
+131
| html := fmt.Sprintf(`<!DOCTYPE html> | ||
| <html> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <title>Lark Token</title> | ||
| </head> | ||
| <body> | ||
| <p>Sending token...</p> | ||
| <script> | ||
| window.onload = function() { | ||
| var url = 'http://127.0.0.1:%d/user_access_token'; | ||
| var data = %s; | ||
| fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify(data) | ||
| }).then(function(response) { | ||
| document.body.innerHTML += '<p>Token sended，close in 3 seconds...</p>'; | ||
| setTimeout(function() { | ||
| window.close(); | ||
| }, 3000); | ||
| }).catch(function(err) { | ||
| document.body.innerHTML += '<p>Token send fail: ' + err + '</p>'; | ||
| }); | ||
| }; | ||
| </script> | ||
| </body> | ||
| </html>`, stateInt, string(dataBytes)) |
There was a problem hiding this comment.
Embedding marshaled JSON directly into a <script> body is unsafe.
json.Marshal does not escape <, >, or / by default, so any field in data (e.g. an error/msg string round-tripped through the SDK, or a future scope/extra field) that contains </script> will break out of the script context and become live HTML. The token JSON itself is unlikely to contain that today, but this is a footgun if data ever grows. Use json.Encoder.SetEscapeHTML(true) (the default) and additionally guard against </:
🛡️ Safer embedding
- dataBytes, err := json.Marshal(data)
+ var buf bytes.Buffer
+ enc := json.NewEncoder(&buf)
+ enc.SetEscapeHTML(true)
+ if err := enc.Encode(data); err != nil {
+ c.JSON(http.StatusInternalServerError, utils.H{"error": "failed to marshal data"})
+ return
+ }
+ // Defensively neutralize any "</" sequences before injecting into <script>.
+ dataBytes := bytes.ReplaceAll(bytes.TrimRight(buf.Bytes(), "\n"), []byte("</"), []byte("<\\/"))(Add "bytes" to the imports.)
Also, line 121 has a small typo: "Token sended，close in 3 seconds..." — please change to "Token sent, close in 3 seconds..." and replace the full‑width comma ， with an ASCII , to keep the UI string consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url_demo_test.go` around lines 102 - 131, The HTML
template embeds JSON unsafely via string(dataBytes) into the script (variable
html); fix by re-encoding dataBytes with a json.Encoder that has
SetEscapeHTML(true) into a bytes.Buffer (or re-marshal via an Encoder) and then
replace any literal "</" with "<\/" before interpolating into the template to
prevent breaking out of the script context; use that escaped JSON in place of
string(dataBytes). Also correct the UI string "Token sended，close in 3
seconds..." to "Token sent, close in 3 seconds..." (replace the full‑width comma
with an ASCII comma).
Comment on lines
+328
to
336
| if existing == nil { | ||
| // New secret provided (either from "create" or "existing" with input) | ||
| secret, err := core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain) | ||
| if err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "%v", err) | ||
| } | ||
| if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang); err != nil { | ||
| if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang, result.UserTokenGetterUrl); err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the interactive form can return an empty AppSecret when only the URL is provided.
fd -t f init_interactive.go cmd
rg -nP -C5 'AppSecret|UserTokenGetterUrl' cmd/config/init_interactive.goRepository: larksuite/cli
Length of output: 2825
🏁 Script executed:
#!/bin/bash
# Check the actual code in init.go at lines 328-336 and surrounding context
sed -n '320,350p' cmd/config/init.go | cat -nRepository: larksuite/cli
Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Check the non-interactive guard mentioned at line 323
sed -n '315,330p' cmd/config/init.go | cat -nRepository: larksuite/cli
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Check the legacy readline path at 444-446
sed -n '440,450p' cmd/config/init.go | cat -nRepository: larksuite/cli
Length of output: 534
🏁 Script executed:
#!/bin/bash
# Search for all ForStorage calls in init.go to understand the pattern
rg -n 'ForStorage' cmd/config/init.go -B3 -A3Repository: larksuite/cli
Length of output: 1282
🏁 Script executed:
#!/bin/bash
# Find ForStorage implementation to understand what it does with empty secrets
rg -n 'func ForStorage' internal/core/ -A10Repository: larksuite/cli
Length of output: 922
🏁 Script executed:
#!/bin/bash
# Check saveInitConfig to understand how it handles empty secret
rg -n 'func saveInitConfig' cmd/config/ -A20Repository: larksuite/cli
Length of output: 1684
Unconditional keychain write for URL-only auth in interactive and legacy readline flows.
In the interactive TUI (line 330) and legacy readline (line 448) paths, core.ForStorage() is called unconditionally with result.AppSecret, even when the user provides only UserTokenGetterUrl (no secret). Since init_interactive.go validation allows this (lines 142–143), result.AppSecret can be empty, causing an empty secret to be written to the keychain and a zero AppSecret persisted to the config.
The non-interactive flag path (line 254) already guards against this with if opts.appSecret != "". Apply the same guard to both the interactive TUI and legacy readline flows.
🛡️ Suggested fix for interactive TUI (line 328–336)
if existing == nil {
- // New secret provided (either from "create" or "existing" with input)
- secret, err := core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain)
- if err != nil {
- return output.Errorf(output.ExitInternal, "internal", "%v", err)
+ var secret core.SecretInput
+ if result.AppSecret != "" {
+ s, err := core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain)
+ if err != nil {
+ return output.Errorf(output.ExitInternal, "internal", "%v", err)
+ }
+ secret = s
}
if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang, result.UserTokenGetterUrl); err != nil {
return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err)
}The same fix applies to the legacy readline flow around line 448.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/init.go` around lines 328 - 336, The interactive TUI and legacy
readline flows call core.ForStorage(result.AppSecret, ...) unconditionally
allowing an empty AppSecret to be written; mirror the non-interactive flag guard
and only call core.ForStorage when result.AppSecret != "" (or opts.appSecret !=
""), otherwise set the secret value to nil and pass that into saveInitConfig;
update both the existing==nil block where core.ForStorage is invoked and the
analogous legacy readline block to skip keychain writes when AppSecret is empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
lark config init--newwhen skill lark-share calllark config initTest Plan
lark-cli config init,lark-cli auth,lark-doc ***command works as expectedRelated Issues
Summary by CodeRabbit
New Features
--user-token-getter-urlconfiguration optionTests
Documentation