feat: add user_token_getter_url, people can get user token from app manager - #686
Closed
wangjingling wants to merge 1 commit into
Closed
feat: add user_token_getter_url, people can get user token from app manager#686wangjingling wants to merge 1 commit into
wangjingling wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a browser-based user-token getter flow: demo HTTP handlers to redirect and exchange codes, CLI login path to open a browser and accept a local callback, config fields to persist Changes
Sequence DiagramsequenceDiagram
participant User as Browser/User
participant CLI as CLI App
participant LocalSvr as Local Callback Server
participant Getter as Getter Handler
participant LarkAPI as Lark Auth Server
participant Callback as OAuthCallback Handler
CLI->>LocalSvr: start on 127.0.0.1:port
CLI->>User: open browser to Getter URL (state=port)
User->>Getter: GET /authorize?state=port&...
Getter->>User: HTTP 302 redirect to Lark auth
User->>LarkAPI: complete consent → receive code
LarkAPI->>Callback: redirect to callback URL with code
User->>Callback: GET /oauth_callback?code=...
Callback->>LarkAPI: exchange code for access token
LarkAPI->>Callback: return token JSON
Callback->>User: HTML page that POSTs token JSON to LocalSvr
User->>LocalSvr: POST /user_access_token with token JSON
LocalSvr->>CLI: deliver token to waiting flow
CLI->>CLI: validate/store token and finish login
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
April 27, 2026 14:25
69b9437 to
b9db67f
Compare
wangjingling
commented
Apr 27, 2026
wangjingling
left a comment
wangjingling
left a comment
Author
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
+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
+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).
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
April 28, 2026 08:08
b9db67f to
cc60549
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
cmd/config/init.go (2)
328-334:⚠️ Potential issue | 🟠 MajorSkip keychain writes when the setup is URL-only.
These branches still call
core.ForStorage(...)even though the new validation allowsAppSecretto be empty whenUserTokenGetterUrlis set. That creates empty-secret keychain state for URL-only profiles. Guard thecore.ForStorage(...)call the same way as the non-interactive path and pass a zerocore.SecretInputdirectly when no secret was entered.Also applies to: 448-452
🤖 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 - 334, The code calls core.ForStorage(...) unconditionally which creates an empty keychain entry when AppSecret may be empty for URL-only setups; change the branch handling new secrets (the block that constructs secret via core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain)) to first check if result.AppSecret is empty AND result.UserTokenGetterUrl is non-empty, and if so skip calling core.ForStorage and pass a zero-value core.SecretInput (or nil-equivalent) into saveInitConfig; otherwise keep the existing core.ForStorage path. Apply the same guard to the other occurrence around saveInitConfig (the branch at lines ~448-452) so both interactive and non-interactive URL-only profiles do not write empty secrets to the keychain.
249-259:⚠️ Potential issue | 🟠 MajorPreserve the existing secret on URL-only updates.
When
opts.appSecretis empty,secretstays zero andsaveInitConfig(...)overwrites the profile's storedAppSecretwith that zero value. Soconfig init --app-id <same> --user-token-getter-url ...silently strips a previously working secret from the profile instead of just adding the getter URL. Reuse the existingcore.SecretInputwhen only the URL is being updated.🤖 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 - 259, The code overwrites the stored AppSecret with a zero-value when opts.appSecret is empty; fix by reusing the profile's existing secret instead of leaving secret zero: if opts.appSecret == "" and an existing profile (existing) has a stored secret, set secret = existing.Config.AppSecret (or the appropriate core.SecretInput field) before calling saveInitConfig; otherwise when opts.appSecret is provided continue to create secret via core.ForStorage(opts.AppID, core.PlainSecret(opts.appSecret), f.Keychain). Ensure you reference opts.appSecret, core.SecretInput, core.ForStorage, existing and saveInitConfig to locate and update the logic.cmd/auth/login.go (2)
447-463:⚠️ Potential issue | 🟠 MajorLet the OS allocate the callback port.
Probing 10 random ports in
8000-9999makes this flow fail on busy machines even when other local ports are free.net.Listen("tcp", "127.0.0.1:0")is atomic and removes the retry/seed logic entirely.🤖 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 - 463, In fetchTokenViaGetter, remove the manual port probing and rand.Seed logic and instead call net.Listen("tcp", "127.0.0.1:0") once to let the OS pick an available port; assign listener from that call, extract the chosen port via listener.Addr().(*net.TCPAddr).Port, and keep existing error handling and return path (ensure you still return an error if listen fails). This eliminates the loop and non-atomic port-race logic around listener/port while preserving the rest of the token retrieval flow.
469-548:⚠️ Potential issue | 🔴 CriticalAuthenticate the localhost callback before accepting a token.
The first request to
/user_access_tokenwins,stateis only the port number, and the handler allowsGETplusAccess-Control-Allow-Origin: *. Any page or local process that can hit127.0.0.1:<port>can race in an arbitrary token/account before the real getter returns. Please bind the callback to an unguessable nonce, validate it on receipt, and narrow the endpoint to the expected POST/origin contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 469 - 548, Generate a cryptographically strong random nonce when starting the flow and use it instead of the plain port for the "state" query param (modify the code that builds q.Set("state", strconv.Itoa(port)) to set the nonce); change the /user_access_token handler (mux.HandleFunc for "/user_access_token") to only accept POST requests, validate that the incoming request includes the expected nonce (from the URL or a header/body field) and reject any request with a missing/mismatched nonce by returning 400 and sending an error on errCh; remove or tighten Access-Control-Allow-Origin "*" (don't allow all origins) and require a specific content-type/authorization contract for the POST (e.g., application/json or a header) before writing tokenData to tokenCh; ensure the server logs/returns a clear failure page on nonce mismatch and that tokenCh only receives tokenData after successful nonce validation.cmd/auth/login_test.go (1)
911-1454:⚠️ Potential issue | 🟡 MinorCover the POST callback path too.
These tests only exercise
GET /user_access_token?token=..., but the new getter flow also accepts POST callbacks. A regression in the POST-body path would still leave this suite green. Please add at least one success case and one malformed-body case for the POST contract.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, Add tests exercising the POST /user_access_token callback path (both success and malformed-body) so the POST-body handling in fetchTokenViaGetter/authLoginViaGetter is covered; create a new test similar to TestFetchTokenViaGetter_Success that starts a mock getter server which POSTs the token JSON to the local callback (instead of GET) and asserts the returned/stored token equals the expected value, and add a counterpart test like TestFetchTokenViaGetter_MissingToken that POSTs invalid/malformed JSON (or an empty body) and asserts the function returns the appropriate error (e.g. contains "failed to unmarshal token JSON" or "missing token data in callback request"); reference fetchTokenViaGetter and authLoginViaGetter to hook the tests into existing test scaffolding and reuse the openBrowserFn override, mock getter server handlers, and logFn patterns already used in the file.
🤖 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/config/init.go`:
- Around line 328-347: The code currently treats the "create" flow as new-only
based on existing == nil; instead branch on result.Mode instead: if result.Mode
== "create" (regardless of existing) treat it as the interactive-create path —
call core.ForStorage(...) with result.AppSecret and then saveInitConfig(...) to
persist the new secret; if result.Mode == "existing" and result.AppID != "" keep
the updateExistingProfileWithoutSecret(...) behavior but also handle the case
where the interactive edit supplied a new secret (i.e., when result.Mode ==
"existing" and result.AppSecret is non-empty, call core.ForStorage(...) and
saveInitConfig(...) to replace the secret instead of routing every edit through
updateExistingProfileWithoutSecret); leave the final validation error ("App ID
cannot be empty") only for cases where result.AppID is empty.
---
Duplicate comments:
In `@cmd/auth/login_test.go`:
- Around line 911-1454: Add tests exercising the POST /user_access_token
callback path (both success and malformed-body) so the POST-body handling in
fetchTokenViaGetter/authLoginViaGetter is covered; create a new test similar to
TestFetchTokenViaGetter_Success that starts a mock getter server which POSTs the
token JSON to the local callback (instead of GET) and asserts the
returned/stored token equals the expected value, and add a counterpart test like
TestFetchTokenViaGetter_MissingToken that POSTs invalid/malformed JSON (or an
empty body) and asserts the function returns the appropriate error (e.g.
contains "failed to unmarshal token JSON" or "missing token data in callback
request"); reference fetchTokenViaGetter and authLoginViaGetter to hook the
tests into existing test scaffolding and reuse the openBrowserFn override, mock
getter server handlers, and logFn patterns already used in the file.
In `@cmd/auth/login.go`:
- Around line 447-463: In fetchTokenViaGetter, remove the manual port probing
and rand.Seed logic and instead call net.Listen("tcp", "127.0.0.1:0") once to
let the OS pick an available port; assign listener from that call, extract the
chosen port via listener.Addr().(*net.TCPAddr).Port, and keep existing error
handling and return path (ensure you still return an error if listen fails).
This eliminates the loop and non-atomic port-race logic around listener/port
while preserving the rest of the token retrieval flow.
- Around line 469-548: Generate a cryptographically strong random nonce when
starting the flow and use it instead of the plain port for the "state" query
param (modify the code that builds q.Set("state", strconv.Itoa(port)) to set the
nonce); change the /user_access_token handler (mux.HandleFunc for
"/user_access_token") to only accept POST requests, validate that the incoming
request includes the expected nonce (from the URL or a header/body field) and
reject any request with a missing/mismatched nonce by returning 400 and sending
an error on errCh; remove or tighten Access-Control-Allow-Origin "*" (don't
allow all origins) and require a specific content-type/authorization contract
for the POST (e.g., application/json or a header) before writing tokenData to
tokenCh; ensure the server logs/returns a clear failure page on nonce mismatch
and that tokenCh only receives tokenData after successful nonce validation.
In `@cmd/config/init.go`:
- Around line 328-334: The code calls core.ForStorage(...) unconditionally which
creates an empty keychain entry when AppSecret may be empty for URL-only setups;
change the branch handling new secrets (the block that constructs secret via
core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain))
to first check if result.AppSecret is empty AND result.UserTokenGetterUrl is
non-empty, and if so skip calling core.ForStorage and pass a zero-value
core.SecretInput (or nil-equivalent) into saveInitConfig; otherwise keep the
existing core.ForStorage path. Apply the same guard to the other occurrence
around saveInitConfig (the branch at lines ~448-452) so both interactive and
non-interactive URL-only profiles do not write empty secrets to the keychain.
- Around line 249-259: The code overwrites the stored AppSecret with a
zero-value when opts.appSecret is empty; fix by reusing the profile's existing
secret instead of leaving secret zero: if opts.appSecret == "" and an existing
profile (existing) has a stored secret, set secret = existing.Config.AppSecret
(or the appropriate core.SecretInput field) before calling saveInitConfig;
otherwise when opts.appSecret is provided continue to create secret via
core.ForStorage(opts.AppID, core.PlainSecret(opts.appSecret), f.Keychain).
Ensure you reference opts.appSecret, core.SecretInput, core.ForStorage, existing
and saveInitConfig to locate and update the 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: 572b09b7-6de3-4b99-9b45-267325da8588
⛔ 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 (2)
- skills/lark-shared/SKILL.md
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/config/config_test.go
- cmd/auth/get_user_token_url_demo_test.go
- internal/core/config.go
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
April 28, 2026 08:54
cc60549 to
09b7204
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/auth/login.go (2)
469-510:⚠️ Potential issue | 🔴 CriticalAuthenticate the localhost callback before trusting it.
The handler currently accepts requests from any origin, allows
GET, and uses a predictablestatevalue (the port). Any page running in the user’s browser can race a request into127.0.0.1and inject an arbitrary token/profile. Please bind the callback to an unguessable nonce, requirePOST, and restrict CORS to the configured getter origin (or remove CORS entirely if it is not needed).Also applies to: 530-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 469 - 510, The /user_access_token handler currently trusts any origin, allows GET, and uses a predictable state (port); fix by requiring POST only, validating a stored unguessable nonce/state before accepting the token, and restricting CORS/Origin to the configured getter origin (or remove the CORS headers entirely if not needed). Update the mux.HandleFunc("/user_access_token", ...) to: reject non-POST methods (except respond to OPTIONS appropriately), read and compare a server-generated nonce/state (not the port) against the request (e.g., a "state" form field or header) before sending tokenData to tokenCh, and validate the request Origin/Referer matches the configured getter origin; mirror the same changes for the duplicate handler at the other occurrence (lines ~530-539) and keep tokenCh/errCh usage unchanged.
448-463:⚠️ Potential issue | 🟠 MajorLet the OS allocate the callback port.
Probing 10 random ports in
8000-9999makes this login path flaky even when plenty of other local ports are free.net.Listen("tcp", "127.0.0.1:0")is atomic, removes the retry loop, and avoids needless failures on busy dev machines.🤖 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 manual port probing loop with an OS-assigned port by calling net.Listen("tcp", "127.0.0.1:0") once (remove rand.Seed and the for-loop). After successfully creating listener, extract the chosen port via listener.Addr() (cast to *net.TCPAddr and read .Port) and keep the existing error check that returns the fmt.Errorf when listener is nil/err. Update any references to the old loop variables (listener, port) accordingly in the surrounding code (e.g., where port is used to build the callback URL).
🤖 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/config/init_interactive.go`:
- Around line 90-97: Update the prompt text for userTokenGetterUrlInput to
reference the correct callback endpoint used by fetchTokenViaGetter: include the
path "/user_access_token" (e.g., "http://127.0.0.1:${state}/user_access_token")
instead of just "http://127.0.0.1:${state}"; adjust the Title string in the
userTokenGetterUrlInput initialization so callers know to post token data to
http://127.0.0.1:${state}/user_access_token and ensure any Placeholder text
still reflects the corrected example URL.
- Around line 146-151: The interactive flow is always returning Mode: "existing"
and thus never persists a newly-entered AppSecret; change the returned
configInitResult to indicate when a new secret was provided (e.g., set Mode to
"existing_with_secret" or add a boolean field like PersistAppSecret) by checking
the appSecret value in this function (the code building and returning the
configInitResult that calls parseBrand). Ensure the init routing logic
(currently sending "existing" to updateExistingProfileWithoutSecret in
cmd/config/init.go) can detect this new mode/flag and call the secret-persisting
updater instead so new secrets entered in the interactive path are saved.
---
Duplicate comments:
In `@cmd/auth/login.go`:
- Around line 469-510: The /user_access_token handler currently trusts any
origin, allows GET, and uses a predictable state (port); fix by requiring POST
only, validating a stored unguessable nonce/state before accepting the token,
and restricting CORS/Origin to the configured getter origin (or remove the CORS
headers entirely if not needed). Update the mux.HandleFunc("/user_access_token",
...) to: reject non-POST methods (except respond to OPTIONS appropriately), read
and compare a server-generated nonce/state (not the port) against the request
(e.g., a "state" form field or header) before sending tokenData to tokenCh, and
validate the request Origin/Referer matches the configured getter origin; mirror
the same changes for the duplicate handler at the other occurrence (lines
~530-539) and keep tokenCh/errCh usage unchanged.
- Around line 448-463: Replace the manual port probing loop with an OS-assigned
port by calling net.Listen("tcp", "127.0.0.1:0") once (remove rand.Seed and the
for-loop). After successfully creating listener, extract the chosen port via
listener.Addr() (cast to *net.TCPAddr and read .Port) and keep the existing
error check that returns the fmt.Errorf when listener is nil/err. Update any
references to the old loop variables (listener, port) accordingly in the
surrounding code (e.g., where port is used to build the callback URL).
🪄 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: 76508c82-abfb-443a-9896-77df4b5c2d35
⛔ 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 (3)
- cmd/config/config_test.go
- go.mod
- cmd/auth/login_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-shared/SKILL.md
- cmd/auth/get_user_token_url_demo_test.go
- internal/core/config.go
- cmd/config/init.go
Comment on lines
+365
to
+443
| func authLoginViaGetter(opts *LoginOptions, config *core.CliConfig, finalScope string, msg *loginMsg, log func(string, ...interface{})) error { | ||
| f := opts.Factory | ||
| token, err := fetchTokenViaGetter(opts.Ctx, config.UserTokenGetterUrl, finalScope, log) | ||
| if err != nil { | ||
| return output.ErrAuth("failed to fetch user token via url: %v", err) | ||
| } | ||
|
|
||
| var gt UserTokenData | ||
| if err := json.Unmarshal([]byte(token), >); err != nil { | ||
| return output.ErrAuth("failed to unmarshal token JSON: %v", err) | ||
| } | ||
|
|
||
| if gt.AccessToken == nil || *gt.AccessToken == "" { | ||
| return output.ErrAuth("authorization succeeded but no access_token returned") | ||
| } | ||
|
|
||
| openId := "" | ||
| if gt.OpenId != nil { | ||
| openId = *gt.OpenId | ||
| } | ||
| userName := "" | ||
| if gt.Name != nil { | ||
| userName = *gt.Name | ||
| } | ||
| expiresIn := 0 | ||
| if gt.ExpiresIn != nil { | ||
| expiresIn = *gt.ExpiresIn | ||
| } | ||
|
|
||
| // 如果没有 open_id/name，依然需要通过 SDK 获取 | ||
| if openId == "" || userName == "" { | ||
| log(msg.AuthSuccess) | ||
| sdk, err := f.LarkClient() | ||
| if err != nil { | ||
| return output.ErrAuth("failed to get SDK: %v", err) | ||
| } | ||
| // NOTE: getUserInfo requires access token | ||
| fetchedOpenId, fetchedUserName, err := getUserInfo(opts.Ctx, sdk, *gt.AccessToken) | ||
| if err != nil { | ||
| return output.ErrAuth("failed to get user info: %v", err) | ||
| } | ||
| if openId == "" { | ||
| openId = fetchedOpenId | ||
| } | ||
| if userName == "" { | ||
| userName = fetchedUserName | ||
| } | ||
| } else { | ||
| log(msg.AuthSuccess) | ||
| } | ||
|
|
||
| now := time.Now().UnixMilli() | ||
| expiresAt := now + int64(expiresIn)*1000 | ||
| if expiresIn <= 0 { | ||
| expiresAt = now + 7200*1000 // 默认 2h | ||
| } | ||
|
|
||
| storedToken := &larkauth.StoredUAToken{ | ||
| UserOpenId: openId, | ||
| AppId: config.AppID, | ||
| AccessToken: *gt.AccessToken, | ||
| RefreshToken: "", // 这种方式不支持 refresh token | ||
| ExpiresAt: expiresAt, | ||
| RefreshExpiresAt: expiresAt, | ||
| Scope: "", // 这种方式暂不解析 scope | ||
| GrantedAt: now, | ||
| } | ||
|
|
||
| if err := larkauth.SetStoredToken(storedToken); err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "failed to save token: %v", err) | ||
| } | ||
|
|
||
| if err := syncLoginUserToProfile(config.ProfileName, config.AppID, openId, userName); err != nil { | ||
| _ = larkauth.RemoveStoredToken(config.AppID, openId) | ||
| return output.Errorf(output.ExitInternal, "internal", "failed to update login profile: %v", err) | ||
| } | ||
|
|
||
| writeLoginSuccess(opts, msg, f, openId, userName, nil) | ||
| return nil |
There was a problem hiding this comment.
Getter-based login drops requested-scope verification.
This path forwards finalScope to the getter URL, then stores the token with Scope: "" and skips ensureRequestedScopesGranted. A caller can run auth login --scope ... and get a success response even when the returned token lacks that grant. Please either carry granted scopes through UserTokenData and reuse the existing scope-summary/validation flow, or reject --scope/--domain/--recommend for getter-based login until that verification is available.
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
April 28, 2026 09:03
09b7204 to
f582474
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (7)
cmd/config/init_interactive.go (2)
90-97:⚠️ Potential issue | 🟠 MajorDocument the actual callback endpoint.
fetchTokenViaGetterlistens onhttp://127.0.0.1:${state}/user_access_token, but this prompt still tells integrators to send token data tohttp://127.0.0.1:${state}. Following the current text posts to the wrong URL and the login never completes.Possible fix
- Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})"). + Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state}/user_access_token)").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init_interactive.go` around lines 90 - 97, The prompt text for the UserTokenGetterUrl is wrong — it tells integrators to post to http://127.0.0.1:${state} but the code actually expects callbacks at http://127.0.0.1:${state}/user_access_token (handled by fetchTokenViaGetter). Update the Title string passed to huh.NewInput() (the userTokenGetterUrlInput initialization) to explicitly mention the full callback path (/user_access_token) so integrators send token data to the correct endpoint; keep existing placeholder behavior (firstApp.UserTokenGetterUrl or "http://...").
146-151:⚠️ Potential issue | 🟠 MajorThis result still hides whether the user entered a new secret or chose getter-only auth.
In
cmd/config/init.go,Mode == "existing"goes throughupdateExistingProfileWithoutSecret, while the new-config path always callscore.ForStorage(result.AppSecret, ...). Returning the same shape here for "unchanged secret", "new secret", and "getter-only" means existing-profile secret rotation is dropped, and getter-only interactive init still falls into empty-secret storage when no config exists. This needs an explicit flag/mode so the caller can pick the correct save path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init_interactive.go` around lines 146 - 151, The returned configInitResult currently hides whether the user supplied a new secret, left the secret unchanged, or chose getter-only auth; add an explicit field (e.g., SecretState string or SecretProvided bool / AuthMode enum) to configInitResult in cmd/config/init_interactive.go, set it in the interactive flow to one of ("new", "unchanged", "getter-only"), and update callers (notably the Mode check in cmd/config/init.go that currently calls updateExistingProfileWithoutSecret and core.ForStorage) to branch on this new field so existing-profile secret rotation uses updateExistingProfileWithoutSecret and getter-only does not call core.ForStorage.cmd/auth/login.go (3)
422-443:⚠️ Potential issue | 🟠 MajorGetter-based login still skips requested-scope verification.
This path stores
Scope: ""and goes straight towriteLoginSuccess, soauth login --scope,--domain, or--recommendcan report success even when the returned token lacks the requested grant. Either carry granted scopes through the getter payload and reuseensureRequestedScopesGranted, or reject scope-selecting flags for this flow until that validation exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 422 - 443, The getter-based login path is saving an empty Scope in StoredUAToken and calling writeLoginSuccess without verifying requested scopes, letting flags like --scope/--domain/--recommend report success erroneously; modify the flow in the getter login branch (where StoredUAToken is created and SetStoredToken, syncLoginUserToProfile and writeLoginSuccess are called) to extract and store the granted scopes from the getter payload, then call ensureRequestedScopesGranted before persisting or returning success (or alternatively reject scope-selecting flags for this flow until validation exists) so requested scopes are validated prior to calling writeLoginSuccess.
448-463:⚠️ Potential issue | 🟠 MajorLet the OS allocate the callback port.
Probing 10 random ports in
8000-9999makes this flow fail spuriously on busy machines even when plenty of ephemeral ports are available.net.Listen("tcp", "127.0.0.1:0")is atomic and removes the retry/seed logic.Possible 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 { + 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, Current random-port probing loop (rand.Seed, loop picking p, net.Listen, port variable) can race; replace it by letting the OS pick an ephemeral port via a single atomic net.Listen("tcp", "127.0.0.1:0") call, remove the rand.Seed and retry loop, and then derive the actual port from listener.Addr() (use the TCPAddr.Port from listener.Addr().(*net.TCPAddr)). Preserve the existing error handling (return the same formatted error on failure) and ensure listener and port variables are still set/used consistently (listener, port) after the change.
469-511:⚠️ Potential issue | 🔴 CriticalAuthenticate the localhost callback before trusting it.
Any page running in the user's browser can hit this listener because CORS is
*, GET callbacks are accepted, andstateis only the chosen port number. That lets an attacker race an arbitrary token into the local server and persist it into the profile. Bind the callback to an unguessable nonce, require POST for token delivery, validate the nonce/state on receipt, and restrict CORS to the getter origin (or omit CORS entirely if it is unnecessary).Also applies to: 530-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 469 - 511, The /user_access_token HTTP handler in login.go must be tightened: generate and store an unguessable nonce (state) when starting the OAuth flow and require the incoming request to use POST with the nonce included (e.g., form field or JSON) and validated against the stored value before accepting tokenData; only accept the token from the POST body (do not read token from query parameters), and on mismatch send an error and push to errCh; restrict or remove the Access-Control-Allow-Origin wildcard so only the expected origin is allowed (or omit CORS if not needed); keep using tokenCh and errCh for successful/failed delivery but only after state validation and POST method check.internal/core/config.go (1)
249-266:⚠️ Potential issue | 🟠 MajorDon't treat
UserTokenGetterUrlas a replacement for a stored secret.This branch skips
ValidateSecretKeyMatchandResolveSecretInputwhenever a getter URL is configured, so profiles that contain both values come back withCliConfig.AppSecret == "". That forces getter-only behavior and breaks downstream paths that still rely on the real secret. Resolve the secret wheneverapp.AppSecretis present;UserTokenGetterUrlshould stay an alternate auth source, not erase the secret from the resolved config.Possible fix
- 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()} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 249 - 266, The code currently skips ValidateSecretKeyMatch and ResolveSecretInput whenever app.UserTokenGetterUrl is set, which can clear CliConfig.AppSecret; change the condition so you resolve and validate the stored secret whenever app.AppSecret is non-empty regardless of UserTokenGetterUrl. Concretely, move the ValidateSecretKeyMatch(app.AppId, app.AppSecret) and ResolveSecretInput(app.AppSecret, kc) calls under a check like "if app.AppSecret != \"\" { ... }", preserve the existing error handling (errors.As exitErr and ConfigError wrapping) around ResolveSecretInput, and leave UserTokenGetterUrl as an alternate auth source that does not erase or skip resolving the secret.cmd/auth/login_test.go (1)
944-1494:⚠️ Potential issue | 🟡 MinorThe new suite still doesn't cover the POST callback contract.
All getter tests feed
/user_access_tokenthroughGET ?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 happy-path POST test and one malformed-body test for the callback handler.🤖 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 944 - 1494, Add tests that exercise the POST callback path: create TestFetchTokenViaGetter_PostSuccess which starts a mock getter that POSTS a JSON body like {"token":"<url-escaped-token-json>"} to the local /user_access_token callback (instead of using GET ?token=...), call fetchTokenViaGetter(ctx, getterURL, ...) and assert the returned token equals the embedded token (e.g., "mock_token_data"); and create TestFetchTokenViaGetter_PostMalformed which posts invalid JSON or a JSON missing the "token" field and assert fetchTokenViaGetter returns an error containing the expected message (e.g., "missing token data in callback request" or "failed to unmarshal token JSON"). Reuse the existing patterns: mock openBrowserFn, start a getter server via http.NewServeMux, spin up a listener, build getterURL from listener.Addr().String(), and use the same context timeouts and logFn; reference function fetchTokenViaGetter and the /user_access_token callback path when locating code to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/auth/login_test.go`:
- Around line 944-1494: Add tests that exercise the POST callback path: create
TestFetchTokenViaGetter_PostSuccess which starts a mock getter that POSTS a JSON
body like {"token":"<url-escaped-token-json>"} to the local /user_access_token
callback (instead of using GET ?token=...), call fetchTokenViaGetter(ctx,
getterURL, ...) and assert the returned token equals the embedded token (e.g.,
"mock_token_data"); and create TestFetchTokenViaGetter_PostMalformed which posts
invalid JSON or a JSON missing the "token" field and assert fetchTokenViaGetter
returns an error containing the expected message (e.g., "missing token data in
callback request" or "failed to unmarshal token JSON"). Reuse the existing
patterns: mock openBrowserFn, start a getter server via http.NewServeMux, spin
up a listener, build getterURL from listener.Addr().String(), and use the same
context timeouts and logFn; reference function fetchTokenViaGetter and the
/user_access_token callback path when locating code to test.
In `@cmd/auth/login.go`:
- Around line 422-443: The getter-based login path is saving an empty Scope in
StoredUAToken and calling writeLoginSuccess without verifying requested scopes,
letting flags like --scope/--domain/--recommend report success erroneously;
modify the flow in the getter login branch (where StoredUAToken is created and
SetStoredToken, syncLoginUserToProfile and writeLoginSuccess are called) to
extract and store the granted scopes from the getter payload, then call
ensureRequestedScopesGranted before persisting or returning success (or
alternatively reject scope-selecting flags for this flow until validation
exists) so requested scopes are validated prior to calling writeLoginSuccess.
- Around line 448-463: Current random-port probing loop (rand.Seed, loop picking
p, net.Listen, port variable) can race; replace it by letting the OS pick an
ephemeral port via a single atomic net.Listen("tcp", "127.0.0.1:0") call, remove
the rand.Seed and retry loop, and then derive the actual port from
listener.Addr() (use the TCPAddr.Port from listener.Addr().(*net.TCPAddr)).
Preserve the existing error handling (return the same formatted error on
failure) and ensure listener and port variables are still set/used consistently
(listener, port) after the change.
- Around line 469-511: The /user_access_token HTTP handler in login.go must be
tightened: generate and store an unguessable nonce (state) when starting the
OAuth flow and require the incoming request to use POST with the nonce included
(e.g., form field or JSON) and validated against the stored value before
accepting tokenData; only accept the token from the POST body (do not read token
from query parameters), and on mismatch send an error and push to errCh;
restrict or remove the Access-Control-Allow-Origin wildcard so only the expected
origin is allowed (or omit CORS if not needed); keep using tokenCh and errCh for
successful/failed delivery but only after state validation and POST method
check.
In `@cmd/config/init_interactive.go`:
- Around line 90-97: The prompt text for the UserTokenGetterUrl is wrong — it
tells integrators to post to http://127.0.0.1:${state} but the code actually
expects callbacks at http://127.0.0.1:${state}/user_access_token (handled by
fetchTokenViaGetter). Update the Title string passed to huh.NewInput() (the
userTokenGetterUrlInput initialization) to explicitly mention the full callback
path (/user_access_token) so integrators send token data to the correct
endpoint; keep existing placeholder behavior (firstApp.UserTokenGetterUrl or
"http://...").
- Around line 146-151: The returned configInitResult currently hides whether the
user supplied a new secret, left the secret unchanged, or chose getter-only
auth; add an explicit field (e.g., SecretState string or SecretProvided bool /
AuthMode enum) to configInitResult in cmd/config/init_interactive.go, set it in
the interactive flow to one of ("new", "unchanged", "getter-only"), and update
callers (notably the Mode check in cmd/config/init.go that currently calls
updateExistingProfileWithoutSecret and core.ForStorage) to branch on this new
field so existing-profile secret rotation uses
updateExistingProfileWithoutSecret and getter-only does not call
core.ForStorage.
In `@internal/core/config.go`:
- Around line 249-266: The code currently skips ValidateSecretKeyMatch and
ResolveSecretInput whenever app.UserTokenGetterUrl is set, which can clear
CliConfig.AppSecret; change the condition so you resolve and validate the stored
secret whenever app.AppSecret is non-empty regardless of UserTokenGetterUrl.
Concretely, move the ValidateSecretKeyMatch(app.AppId, app.AppSecret) and
ResolveSecretInput(app.AppSecret, kc) calls under a check like "if app.AppSecret
!= \"\" { ... }", preserve the existing error handling (errors.As exitErr and
ConfigError wrapping) around ResolveSecretInput, and leave UserTokenGetterUrl as
an alternate auth source that does not erase or skip resolving the secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fefe7e2-0dbe-4193-97d9-527b51a3dbc0
⛔ 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 (1)
- skills/lark-shared/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/auth/get_user_token_url_demo_test.go
- cmd/config/config_test.go
- go.mod
- cmd/config/init.go
…anager without app secret, then use lark-cli
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
April 28, 2026 09:22
f582474 to
edf9499
Compare
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
Tests
Documentation