feat: app administrator provides an auth page, allowing others to obtain the user_access_token without requiring the app_secret#634
Open
wangjingling wants to merge 4 commits intolarksuite:mainfrom
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" login path activated when UserTokenGetterUrl is set and AppSecret is empty; persists the getter URL through config and credential layers, updates init/interactive flows and tests, adds demo handlers, and adds related dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant LocalCB as Local HTTP\nCallback (127.0.0.1:<port>/user_access_token)
participant CLI as CLI Login Command
participant SDK as User Info SDK
participant Store as Token Storage / Profile Sync
Browser->>LocalCB: GET/POST token JSON (query or body)
LocalCB->>CLI: Deliver token payload
CLI->>CLI: Parse UserTokenData, validate access_token
alt missing open_id or name
CLI->>SDK: Fetch user info using access_token
SDK-->>CLI: Return open_id/name
end
CLI->>Store: Compute expiry, store StoredUAToken (no refresh/scope)
CLI->>Store: Sync login user profile
LocalCB-->>Browser: Serve HTML success/failure page and shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/auth/login.go (1)
6-22:⚠️ Potential issue | 🟡 MinorRun gofmt on the import block.
The added standard-library imports are split into a second group;
gofmtwill collapse and sort them. As per coding guidelines, Code must passgofmt -l .with no output to ensure proper formatting.🎨 Expected import shape
import ( "context" "encoding/json" "fmt" + "io" + "math/rand" + "net" + "net/http" + "net/url" + "os/exec" + "runtime" "sort" + "strconv" "strings" "time" - - "io" - "math/rand" - "net" - "net/http" - "net/url" - "os/exec" - "runtime" - "strconv" "github.com/spf13/cobra"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 6 - 22, The import block in cmd/auth/login.go is incorrectly grouped/ordered; run gofmt (or gofmt -w .) to collapse and sort the standard-library imports into a single block and ensure imports follow the canonical grouping (standard library first, blank line, then third-party/internal packages). Adjust the import statement around the existing symbols (context, encoding/json, fmt, sort, strings, time, io, math/rand, net, net/http, net/url, os/exec, runtime, strconv) so they are sorted and grouped per gofmt output and re-run gofmt -l . to confirm no remaining formatting issues.cmd/config/init.go (1)
117-153:⚠️ Potential issue | 🟠 MajorRemove the old secret when switching a profile to getter-only auth.
When an existing profile keeps the same App ID but is updated with an empty
secretand aUserTokenGetterUrl, the config no longer references the secret, but the old keychain entry is never removed. That leaves an app secret retained after the user moved to the no-secret flow.🧹 Proposed direction
- // Update existing profile + oldSecret := multi.Apps[idx].AppSecret + // Update existing profile multi.Apps[idx].AppId = appId multi.Apps[idx].AppSecret = secret multi.Apps[idx].Brand = brand multi.Apps[idx].Lang = lang multi.Apps[idx].UserTokenGetterUrl = userTokenGetterUrl + // After a successful save, best-effort remove oldSecret when the new + // config intentionally no longer references an app secret.Apply the cleanup after
core.SaveMultiAppConfig(multi)succeeds, so a failed config write does not delete the still-active secret.Also applies to: 179-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 117 - 153, The saveAsProfile function currently leaves an old keychain secret when an existing profile is changed to getter-only auth (same AppId, new secret empty and UserTokenGetterUrl set); update saveAsProfile so that when an existing profile is updated from having an AppSecret to having no secret (e.g., secret is empty/nil and UserTokenGetterUrl is non-empty) you perform the cleanup of the old secret (call core.RemoveSecretStore with the previous AppSecret and keychain) and remove stored user tokens, but only after core.SaveMultiAppConfig(multi) successfully returns; adjust the branch that updates an existing profile (found via findProfileIndexByName) to detect this transition, defer core.RemoveSecretStore and auth.RemoveStoredToken calls until after SaveMultiAppConfig succeeds, and apply the same fix to the analogous update logic elsewhere in this file that manipulates AppSecret and UserTokenGetterUrl.cmd/config/config_test.go (1)
311-335:⚠️ Potential issue | 🟡 MinorAdd coverage for non-empty
UserTokenGetterUrl.These updates only exercise the empty-string path. Please add tests that verify a non-empty getter URL is persisted for new/profile configs and that app-secret-less updates preserve or apply it correctly. As per coding guidelines, 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/config/config_test.go` around lines 311 - 335, Add unit tests that cover non-empty UserTokenGetterUrl for both creating and updating profiles: extend the existing test cases around saveAsProfile and updateExistingProfileWithoutSecret to include a non-empty UserTokenGetterUrl value (e.g., "https://getter.example") and assert it is persisted into the resulting core.AppConfig/core.MultiAppConfig for new profiles and that updateExistingProfileWithoutSecret either preserves an existing non-empty UserTokenGetterUrl or applies a provided non-empty value when updating an app without a secret; reference the saveAsProfile and updateExistingProfileWithoutSecret functions to locate where to set the input UserTokenGetterUrl and where to assert the stored value.
🤖 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/login.go`:
- Around line 360-575: Add unit tests for the getter login flow covering
fetchTokenViaGetter and authLoginViaGetter: write tests that (1) simulate a
successful callback POST/GET returning a valid JSON with access_token and
optional fields to assert stored token via larkauth.SetStoredToken and profile
sync via syncLoginUserToProfile; (2) simulate callback returning JSON missing
access_token to verify authLoginViaGetter returns the ErrAuth error path; (3)
simulate callback with a mismatched state query param to ensure the handler
rejects the request and returns an error into errCh; (4) simulate timeout/cancel
by not sending a callback and asserting fetchTokenViaGetter returns the timeout
error; and (5) test scope mismatch by invoking fetchTokenViaGetter with scope
and ensuring the finalURL contains the scope param (or that behavior is
asserted). Use test helpers to start the local server listener behavior (or mock
net.Listen and http.Server) and stub openBrowser to avoid launching a real
browser; reference functions fetchTokenViaGetter, authLoginViaGetter,
openBrowser, getUserInfo, larkauth.SetStoredToken, syncLoginUserToProfile to
locate code paths to assert.
- Around line 544-554: The select waiting for tokenCh/errCh/timer in
fetchTokenViaGetter ignores command cancellation via ctx; add a case for
<-ctx.Done() to the select and return an appropriate error (e.g., ctx.Err())
when cancellation occurs, referencing the existing tokenCh, errCh, timer
variables and the fetchTokenViaGetter function so the wait exits immediately if
the context is canceled.
- Around line 417-437: The current getter flow stores Scope: "" in the
StoredUAToken and calls writeLoginSuccess without verifying scopes; change this
to capture the token's returned scope (e.g., use gt.Scope or equivalent) when
constructing storedToken.Scope, then call
ensureRequestedScopesGranted(requestedScope, returnedScope, ...) after syncing
the profile and before writeLoginSuccess; if scope verification fails, remove
the stored token via larkauth.RemoveStoredToken(config.AppID, openId) and return
a structured auth error instead of reporting success (references:
StoredUAToken.Scope, gt (token response), larkauth.SetStoredToken,
syncLoginUserToProfile, ensureRequestedScopesGranted, writeLoginSuccess).
- Around line 441-555: fetchTokenViaGetter needs an unguessable callback state
and a capped request body: generate a cryptographically-random nonce before
starting the listener and set q.Set("state", nonce) (replace the current
port-as-state usage), then in the "/user_access_token" handler validate that the
incoming state (r.URL.Query().Get("state") or a dedicated header) exactly
matches that nonce and reject mismatches by writing a 400 and sending an error
to errCh; additionally wrap r.Body with http.MaxBytesReader to cap the size
(e.g. a few KB) before io.ReadAll to prevent large-body attacks, and ensure both
GET and POST paths still validate the state and push errors to errCh when
validation or body-read fails (refer to fetchTokenViaGetter, the
"/user_access_token" mux.HandleFunc, tokenCh and errCh).
In `@cmd/config/init_interactive.go`:
- Around line 90-97: The prompt for userTokenGetterUrlInput currently references
a vague callback "http://127.0.0.1:${state}" which can cause admins to post
tokens to the wrong path; update the Title string built by NewInput() (and any
Placeholder values, including when firstApp.UserTokenGetterUrl is used) to
explicitly show the exact callback endpoint
"http://127.0.0.1:<port>/user_access_token" (or include the path
"/user_access_token" next to the existing host:port placeholder) so the prompt
unambiguously instructs callers to POST tokens to /user_access_token.
In `@cmd/config/init.go`:
- Around line 321-327: The interactive flow is calling core.ForStorage with an
empty secret; add the same zero-secret guard used in runExistingAppForm: before
calling core.ForStorage (where secret is created with
core.PlainSecret(result.AppSecret) and passed to saveInitConfig), check
resolvedSecret.IsZero() (or equivalently check result.AppSecret == "" /
resolvedSecret.IsZero()) and skip calling ForStorage / avoid creating a keychain
entry when the secret is empty; apply the same pattern for the non-interactive
`--new` path as hinted around line 298 so that ForStorage is only invoked when
the secret is non-zero.
In `@internal/core/config.go`:
- Around line 255-281: The current branch skips resolving the app secret
whenever app.UserTokenGetterUrl is set, which causes AppSecret to be dropped
even when present; instead, only skip secret resolution when the app secret is
truly absent. Update the logic around ValidateSecretKeyMatch and
ResolveSecretInput so they run when app.AppSecret is set (e.g., check
app.AppSecret != "" or similar) rather than depending on UserTokenGetterUrl;
keep the same error handling (preserve the errors.As exitErr handling and return
ConfigError on other errors) and ensure the constructed CliConfig.AppSecret is
populated from the resolved secret when present.
In `@skills/lark-shared/SKILL.md`:
- Line 19: The docs instruct agents to run "lark-cli config init" but
cmd/config/init.go currently rejects non-TTY, so either update the docs to
require the explicit flag or make the command tolerant of background (non-TTY)
runs; specifically, modify the handler in cmd/config/init.go (the config init
command's Run/RunE function) to detect non-terminal stdout/stderr (using isatty
or equivalent) and if not a TTY, behave as if --new was passed (or at minimum
print the setup URL and exit successfully), or alternatively change SKILL.md to
instruct agents to run "lark-cli config init --new" when running in background.
Ensure the change references the init command logic so background agents receive
the setup URL rather than being rejected.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 6-22: The import block in cmd/auth/login.go is incorrectly
grouped/ordered; run gofmt (or gofmt -w .) to collapse and sort the
standard-library imports into a single block and ensure imports follow the
canonical grouping (standard library first, blank line, then
third-party/internal packages). Adjust the import statement around the existing
symbols (context, encoding/json, fmt, sort, strings, time, io, math/rand, net,
net/http, net/url, os/exec, runtime, strconv) so they are sorted and grouped per
gofmt output and re-run gofmt -l . to confirm no remaining formatting issues.
In `@cmd/config/config_test.go`:
- Around line 311-335: Add unit tests that cover non-empty UserTokenGetterUrl
for both creating and updating profiles: extend the existing test cases around
saveAsProfile and updateExistingProfileWithoutSecret to include a non-empty
UserTokenGetterUrl value (e.g., "https://getter.example") and assert it is
persisted into the resulting core.AppConfig/core.MultiAppConfig for new profiles
and that updateExistingProfileWithoutSecret either preserves an existing
non-empty UserTokenGetterUrl or applies a provided non-empty value when updating
an app without a secret; reference the saveAsProfile and
updateExistingProfileWithoutSecret functions to locate where to set the input
UserTokenGetterUrl and where to assert the stored value.
In `@cmd/config/init.go`:
- Around line 117-153: The saveAsProfile function currently leaves an old
keychain secret when an existing profile is changed to getter-only auth (same
AppId, new secret empty and UserTokenGetterUrl set); update saveAsProfile so
that when an existing profile is updated from having an AppSecret to having no
secret (e.g., secret is empty/nil and UserTokenGetterUrl is non-empty) you
perform the cleanup of the old secret (call core.RemoveSecretStore with the
previous AppSecret and keychain) and remove stored user tokens, but only after
core.SaveMultiAppConfig(multi) successfully returns; adjust the branch that
updates an existing profile (found via findProfileIndexByName) to detect this
transition, defer core.RemoveSecretStore and auth.RemoveStoredToken calls until
after SaveMultiAppConfig succeeds, and apply the same fix to the analogous
update logic elsewhere in this file that manipulates AppSecret and
UserTokenGetterUrl.
🪄 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: 85d5b0ad-e961-4622-b2c2-b5a680532639
📒 Files selected for processing (7)
cmd/auth/login.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_interactive.gointernal/core/config.gointernal/credential/types.goskills/lark-shared/SKILL.md
Comment on lines
+360
to
+575
| 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 | ||
| } | ||
|
|
||
| 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() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancel() | ||
| server.Shutdown(ctx) | ||
| }() | ||
|
|
||
| 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 := openBrowser(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 token := <-tokenCh: | ||
| return token, nil | ||
| case err := <-errCh: | ||
| return "", err | ||
| case <-timer.C: | ||
| return "", fmt.Errorf("timeout waiting for token callback (5 minutes)") | ||
| } | ||
| } | ||
|
|
||
| func openBrowser(ctx context.Context, url string) error { | ||
| // 简单的跨平台打开浏览器实现 | ||
| var err error | ||
| switch runtime.GOOS { | ||
| case "linux": | ||
| err = exec.CommandContext(ctx, "xdg-open", url).Start() | ||
| case "darwin": | ||
| err = exec.CommandContext(ctx, "open", url).Start() | ||
| case "windows": | ||
| err = exec.CommandContext(ctx, "rundll32", "url.dll,FileProtocolHandler", url).Start() | ||
| default: | ||
| // Go fallback | ||
| err = exec.CommandContext(ctx, "open", url).Start() | ||
| if err != nil { | ||
| err = exec.CommandContext(ctx, "xdg-open", url).Start() | ||
| } | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Add tests for the getter login flow.
Please cover success, missing access_token, callback state rejection, timeout/cancel, and requested-scope mismatch. As per coding guidelines, 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.go` around lines 360 - 575, Add unit tests for the getter
login flow covering fetchTokenViaGetter and authLoginViaGetter: write tests that
(1) simulate a successful callback POST/GET returning a valid JSON with
access_token and optional fields to assert stored token via
larkauth.SetStoredToken and profile sync via syncLoginUserToProfile; (2)
simulate callback returning JSON missing access_token to verify
authLoginViaGetter returns the ErrAuth error path; (3) simulate callback with a
mismatched state query param to ensure the handler rejects the request and
returns an error into errCh; (4) simulate timeout/cancel by not sending a
callback and asserting fetchTokenViaGetter returns the timeout error; and (5)
test scope mismatch by invoking fetchTokenViaGetter with scope and ensuring the
finalURL contains the scope param (or that behavior is asserted). Use test
helpers to start the local server listener behavior (or mock net.Listen and
http.Server) and stub openBrowser to avoid launching a real browser; reference
functions fetchTokenViaGetter, authLoginViaGetter, openBrowser, getUserInfo,
larkauth.SetStoredToken, syncLoginUserToProfile to locate code paths to assert.
Comment on lines
+417
to
+437
| 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) |
There was a problem hiding this comment.
Verify requested scopes before reporting getter login success.
The device flow checks granted scopes, but the getter flow stores Scope: "" and calls writeLoginSuccess with no scope summary. A command like auth login --scope ... can therefore succeed even if the returned token lacks the requested permissions.
🛠️ Proposed direction
type UserTokenData struct {
AccessToken *string `json:"access_token,omitempty"`
+ Scope *string `json:"scope,omitempty"`
TokenType *string `json:"token_type,omitempty"`
ExpiresIn *int `json:"expires_in,omitempty"`
Name *string `json:"name,omitempty"`Then store the returned scope and call ensureRequestedScopesGranted(finalScope, returnedScope, ...) before writing success, or return a structured auth error when the getter cannot provide scope evidence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 417 - 437, The current getter flow stores
Scope: "" in the StoredUAToken and calls writeLoginSuccess without verifying
scopes; change this to capture the token's returned scope (e.g., use gt.Scope or
equivalent) when constructing storedToken.Scope, then call
ensureRequestedScopesGranted(requestedScope, returnedScope, ...) after syncing
the profile and before writeLoginSuccess; if scope verification fails, remove
the stored token via larkauth.RemoveStoredToken(config.AppID, openId) and return
a structured auth error instead of reporting success (references:
StoredUAToken.Scope, gt (token response), larkauth.SetStoredToken,
syncLoginUserToProfile, ensureRequestedScopesGranted, writeLoginSuccess).
Comment on lines
+441
to
+555
| 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() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancel() | ||
| server.Shutdown(ctx) | ||
| }() | ||
|
|
||
| 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 := openBrowser(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 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.
Add an unguessable callback state and harden the local handler.
The callback currently accepts the first token posted to 127.0.0.1:<random-port>/user_access_token; the state value is only the port and is not validated. Any local process or browser page can race a fake callback and cause the CLI to store the wrong user token. Generate a nonce, require it on the callback, and cap the request body size.
🔒 Proposed hardening sketch
+ nonce := generateCallbackNonce()
+
mux := http.NewServeMux()
mux.HandleFunc("/user_access_token", func(w http.ResponseWriter, r *http.Request) {
+ if r.URL.Query().Get("state") != nonce {
+ http.Error(w, "invalid state", http.StatusBadRequest)
+ return
+ }
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")
@@
var tokenData string
if r.Method == http.MethodPost {
- body, err := io.ReadAll(r.Body)
+ r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
+ body, err := io.ReadAll(r.Body)
if err != nil {
http.Error(w, "failed to read body", http.StatusBadRequest)
return
}
@@
q := u.Query()
- q.Set("state", strconv.Itoa(port))
+ q.Set("port", strconv.Itoa(port))
+ q.Set("state", nonce)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 441 - 555, fetchTokenViaGetter needs an
unguessable callback state and a capped request body: generate a
cryptographically-random nonce before starting the listener and set
q.Set("state", nonce) (replace the current port-as-state usage), then in the
"/user_access_token" handler validate that the incoming state
(r.URL.Query().Get("state") or a dedicated header) exactly matches that nonce
and reject mismatches by writing a 400 and sending an error to errCh;
additionally wrap r.Body with http.MaxBytesReader to cap the size (e.g. a few
KB) before io.ReadAll to prevent large-body attacks, and ensure both GET and
POST paths still validate the state and push errors to errCh when validation or
body-read fails (refer to fetchTokenViaGetter, the "/user_access_token"
mux.HandleFunc, tokenCh and errCh).
Comment on lines
+90
to
+97
| userTokenGetterUrlInput := huh.NewInput(). | ||
| Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})"). | ||
| Value(&userTokenGetterUrl) | ||
| if firstApp != nil && firstApp.UserTokenGetterUrl != "" { | ||
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder(firstApp.UserTokenGetterUrl) | ||
| } else { | ||
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder("http://...") | ||
| } |
There was a problem hiding this comment.
Include the exact callback endpoint in the prompt.
The login server listens on http://127.0.0.1:<port>/user_access_token, but the prompt only mentions http://127.0.0.1:${state}. That can lead administrators to post the token to the wrong URL.
📝 Proposed prompt tweak
- Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})").
+ Title("UserTokenGetterUrl (Optional; it must get a token and send token data to http://127.0.0.1:${state}/user_access_token)").📝 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
| userTokenGetterUrlInput := huh.NewInput(). | |
| Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})"). | |
| Value(&userTokenGetterUrl) | |
| if firstApp != nil && firstApp.UserTokenGetterUrl != "" { | |
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder(firstApp.UserTokenGetterUrl) | |
| } else { | |
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder("http://...") | |
| } | |
| userTokenGetterUrlInput := huh.NewInput(). | |
| Title("UserTokenGetterUrl (Optional; it must get a token and send token data to http://127.0.0.1:${state}/user_access_token)"). | |
| Value(&userTokenGetterUrl) | |
| if firstApp != nil && firstApp.UserTokenGetterUrl != "" { | |
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder(firstApp.UserTokenGetterUrl) | |
| } else { | |
| userTokenGetterUrlInput = userTokenGetterUrlInput.Placeholder("http://...") | |
| } |
🤖 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 for
userTokenGetterUrlInput currently references a vague callback
"http://127.0.0.1:${state}" which can cause admins to post tokens to the wrong
path; update the Title string built by NewInput() (and any Placeholder values,
including when firstApp.UserTokenGetterUrl is used) to explicitly show the exact
callback endpoint "http://127.0.0.1:<port>/user_access_token" (or include the
path "/user_access_token" next to the existing host:port placeholder) so the
prompt unambiguously instructs callers to POST tokens to /user_access_token.
Comment on lines
+321
to
+327
| if existing == nil { | ||
| // New secret provided (either from "create" or "existing" with input) | ||
| secret, err := core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain) | ||
| if err != nil { | ||
| return output.Errorf(output.ExitInternal, "internal", "%v", err) | ||
| } | ||
| if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang); err != nil { | ||
| if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang, result.UserTokenGetterUrl); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate ForStorage and related SecretInput helpers.
rg -n -C4 'func ForStorage|type SecretInput|func PlainSecret|func \(.*SecretInput.*\) IsZero' --type=goRepository: larksuite/cli
Length of output: 2056
🏁 Script executed:
#!/bin/bash
# Get the full ForStorage implementation
rg -n -A 20 'func ForStorage\(appId string' internal/core/secret_resolve.goRepository: larksuite/cli
Length of output: 1133
🏁 Script executed:
#!/bin/bash
# Examine the flagged lines in cmd/config/init.go (321-327 and 441-445)
sed -n '315,335p' cmd/config/init.go
echo "---"
sed -n '435,450p' cmd/config/init.goRepository: larksuite/cli
Length of output: 1892
🏁 Script executed:
#!/bin/bash
# Check if there's any zero-secret handling logic elsewhere
rg -n -B2 -A2 'IsZero\(\)|zero.*secret|empty.*secret' --type=go internal/core/Repository: larksuite/cli
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Find IsPlain implementation
rg -n -A 3 'func \(s SecretInput\) IsPlain' internal/core/Repository: larksuite/cli
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Get more context around lines 310-330 to understand result.AppSecret handling
sed -n '300,335p' cmd/config/init.goRepository: larksuite/cli
Length of output: 1798
🏁 Script executed:
#!/bin/bash
# Check if there's a zero-secret guard pattern used elsewhere
rg -n -B3 'PlainSecret\(\)' cmd/config/ --type=goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check what the non-interactive path does with secrets - look further up
sed -n '270,305p' cmd/config/init.goRepository: larksuite/cli
Length of output: 1242
🏁 Script executed:
#!/bin/bash
# Find where result.AppSecret comes from in the interactive path
rg -n -B5 -A5 'runInteractiveConfigInit' cmd/config/init.go | head -30Repository: larksuite/cli
Length of output: 451
🏁 Script executed:
#!/bin/bash
# Check if there's any guard before calling ForStorage in non-interactive paths
rg -n -B10 'ForStorage.*result\.AppSecret' cmd/config/init.goRepository: larksuite/cli
Length of output: 935
🏁 Script executed:
#!/bin/bash
# Find where result values come from - check the interactive config result struct
rg -n 'type.*Result|struct.*{' cmd/config/init.go | head -20Repository: larksuite/cli
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Check the actual runInteractiveConfigInit function to see what AppSecret can be
rg -n -A 30 'func runInteractiveConfigInit' cmd/config/init.go | head -50Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for how bind.go or other flows handle zero/empty secrets
rg -n -B3 -A3 'IsZero\(\)|PlainSecret.*""' cmd/config/bind.goRepository: larksuite/cli
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Find bind.go file
find . -name 'bind.go' -type fRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for result struct and runInteractiveConfigInit more carefully
sed -n '500,600p' cmd/config/init.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for the result struct definition in init.go
grep -n 'type.*result\|struct' cmd/config/init.go | head -20Repository: larksuite/cli
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the runInteractiveConfigInit function
sed -n '600,700p' cmd/config/init.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check file size
wc -l cmd/config/init.goRepository: larksuite/cli
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Search for runInteractiveConfigInit and result struct definition
rg -n 'runInteractiveConfigInit|AppSecret.*string|type.*configResult' cmd/config/init.goRepository: larksuite/cli
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Look at the actual lines 441-445 that are also flagged in the review
sed -n '435,450p' cmd/config/init.goRepository: larksuite/cli
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Find the keychain.Set implementation to see if it rejects empty strings
rg -n 'Set.*error|func.*Set' --type=go internal/ | grep -i keychainRepository: larksuite/cli
Length of output: 2054
🏁 Script executed:
#!/bin/bash
# Look at the full init.go to understand the flow at line 441-445
sed -n '380,450p' cmd/config/init.goRepository: larksuite/cli
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern in bind.go (or similar file) that handles zero secrets
find . -name '*.go' -path '*/cmd/config/*' -exec basename {} \;Repository: larksuite/cli
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Find runInteractiveConfigInit function - it should be in init_interactive.go
cat cmd/config/init_interactive.go | head -100Repository: larksuite/cli
Length of output: 2996
🏁 Script executed:
#!/bin/bash
# Check the keychain.Set function to see if it validates/rejects empty values
sed -n '14,30p' internal/keychain/default.goRepository: larksuite/cli
Length of output: 403
🏁 Script executed:
#!/bin/bash
# See the full validation at line 316 and what happens after it
sed -n '310,330p' cmd/config/init.goRepository: larksuite/cli
Length of output: 992
Add zero-secret guard before calling ForStorage in the interactive flow.
The interactive TUI path (line 323) bypasses the zero-secret check that exists in runExistingAppForm (lines 436–439). When a user provides only a UserTokenGetterUrl and leaves AppSecret empty, the code calls ForStorage with an empty secret, unnecessarily storing it in the keychain.
Match the pattern in runExistingAppForm: check resolvedSecret.IsZero() before passing to ForStorage, ensuring empty secrets are skipped just as they are in the non-interactive path.
Also applies to: lines 298 (non-interactive --new path).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/init.go` around lines 321 - 327, The interactive flow is calling
core.ForStorage with an empty secret; add the same zero-secret guard used in
runExistingAppForm: before calling core.ForStorage (where secret is created with
core.PlainSecret(result.AppSecret) and passed to saveInitConfig), check
resolvedSecret.IsZero() (or equivalently check result.AppSecret == "" /
resolvedSecret.IsZero()) and skip calling ForStorage / avoid creating a keychain
entry when the secret is empty; apply the same pattern for the non-interactive
`--new` path as hinted around line 298 so that ForStorage is only invoked when
the secret is non-zero.
Comment on lines
+255
to
+281
| 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.
Resolve the app secret when it is still configured.
UserTokenGetterUrl should remove the requirement for an app secret, not mask an existing one. With the current condition, a profile that has both values resolves AppSecret as empty, so bot/device flows that still need the real secret can fail.
🛠️ Proposed 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 {
// 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()}
}
+ } else if app.UserTokenGetterUrl == "" {
+ return nil, &ConfigError{Code: 2, Type: "config", Message: "App Secret and UserTokenGetterUrl cannot be both empty"}
}📝 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
| 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, | |
| var secret string | |
| var err error | |
| 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 { | |
| // 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()} | |
| } | |
| } else if app.UserTokenGetterUrl == "" { | |
| return nil, &ConfigError{Code: 2, Type: "config", Message: "App Secret and UserTokenGetterUrl cannot be both empty"} | |
| } | |
| cfg := &CliConfig{ | |
| ProfileName: app.ProfileName(), | |
| AppID: app.AppId, | |
| AppSecret: secret, | |
| Brand: app.Brand, | |
| DefaultAs: app.DefaultAs, | |
| UserTokenGetterUrl: app.UserTokenGetterUrl, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config.go` around lines 255 - 281, The current branch skips
resolving the app secret whenever app.UserTokenGetterUrl is set, which causes
AppSecret to be dropped even when present; instead, only skip secret resolution
when the app secret is truly absent. Update the logic around
ValidateSecretKeyMatch and ResolveSecretInput so they run when app.AppSecret is
set (e.g., check app.AppSecret != "" or similar) rather than depending on
UserTokenGetterUrl; keep the same error handling (preserve the errors.As exitErr
handling and return ConfigError on other errors) and ensure the constructed
CliConfig.AppSecret is populated from the resolved secret when present.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cmd/auth/get_user_token_url.demo_test.go (2)
46-46://ignore_security_alert IDOR— confirm this suppression is intentional and scoped.The comment suppresses an IDOR alert on the callback, but the handler trusts
code(exchanged via the SDK, OK) andstate(used only as a port int after validation). If the intent is just "state is not a user identifier here," consider narrowing the suppression or adding a brief rationale so reviewers/linters know why. Not a blocker for a demo, but worth a sentence of context.🤖 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` at line 46, The inline suppression "//ignore_security_alert IDOR" on the OAuthCallback function is too broad; update the comment to either narrow the scope or add a short rationale clarifying that "state" is validated and only used as a port integer (not a user identifier) and that "code" is exchanged via the SDK, so the IDOR risk is not applicable. Locate the OAuthCallback function and replace or augment the suppression with a scoped comment like a one-line rationale explaining validation steps and intended use of state to satisfy reviewers/linters.
19-23: Placeholder credentials at package scope will be evaluated on test binary init.
lark.NewClient("app_id", "app_secret")runs during package initialization of the test binary forcmd/auth. Even though this is labeled a demo, having bogus credentials baked in at init time can trigger unexpected network/config side effects inside the SDK and is easy to accidentally copy-paste. Consider constructing the client lazily inside the handlers, or move the file out of the test compilation unit (see the earlier comment about the_test.gosuffix).🤖 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 - 23, The package-scope call to lark.NewClient (via variables appID, larkClient, redirectURI) causes client construction at test-binary init; change this so the Lark client is created lazily (e.g., inside the handler/test setup function that uses getUserTokenURL or a helper like NewTestClient) or move the demo file out of the _test.go compilation unit so no side-effecting init runs; locate references to larkClient and replace package-level usage with a constructor/getter that returns a new or cached client at runtime.
🤖 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 46-62: The OAuthCallback handler currently accepts any integer
state and uses stateInt when formatting the callback URL; restrict stateInt to a
valid TCP port range (e.g., 1024 <= stateInt <= 65535) before using it. In
OAuthCallback, after strconv.Atoi(state) and err check, add a validation that
returns http.StatusBadRequest with an appropriate error JSON if stateInt is
<1024 or >65535 (or otherwise out of your chosen port bounds), so only a
legitimate port value is used when formatting
"http://127.0.0.1:%d/user_access_token".
- Around line 97-128: The HTML demo currently injects raw JSON (dataBytes) into
the inline <script> via fmt.Sprintf in the html variable (see html :=
fmt.Sprintf(..., stateInt, string(dataBytes))) which allows a </script> breakout
and XSS; change this by not interpolating raw JSON into the script — either
serve the JSON from a separate handler (e.g., a /token-data endpoint fetched by
window.onload before POSTing to /user_access_token) or ensure safe embedding by
passing the JSON as a quoted JS string (use %q) and JSON.parse on the client or
use json.Encoder with SetEscapeHTML(true) and replace problematic chars, and
also fix the user-facing message "Token sended" to "Token sent" (references:
dataBytes, fmt.Sprintf(html...), window.onload, /user_access_token, c.Data).
- Around line 1-17: This demo file is misnamed as a test and should be moved or
gated: remove the `_test.go` suffix and either relocate the contents into an
examples package (e.g., examples/user_token_getter/main.go under package main)
or add a build tag like `//go:build demo` so it isn't included in normal test
binaries; ensure exported handlers UserAuth and OAuthCallback remain intact for
demonstration. Also add real tests covering the --user-token-getter-url behavior
exercised by fetchTokenViaGetter in login.go (unit tests that mock the getter
URL and integration tests for the OAuth flow) so the feature has coverage rather
than relying on the demo file.
In `@go.mod`:
- Line 8: The go.mod currently lists a direct require for
github.com/cloudwego/hertz even though it's only used in tests
(cmd/auth/get_user_token_url.demo_test.go); run `go mod tidy` to remove that
unnecessary direct dependency so it becomes indirect, then run the test suite to
verify nothing breaks and commit the updated go.mod/go.sum.
---
Nitpick comments:
In `@cmd/auth/get_user_token_url.demo_test.go`:
- Line 46: The inline suppression "//ignore_security_alert IDOR" on the
OAuthCallback function is too broad; update the comment to either narrow the
scope or add a short rationale clarifying that "state" is validated and only
used as a port integer (not a user identifier) and that "code" is exchanged via
the SDK, so the IDOR risk is not applicable. Locate the OAuthCallback function
and replace or augment the suppression with a scoped comment like a one-line
rationale explaining validation steps and intended use of state to satisfy
reviewers/linters.
- Around line 19-23: The package-scope call to lark.NewClient (via variables
appID, larkClient, redirectURI) causes client construction at test-binary init;
change this so the Lark client is created lazily (e.g., inside the handler/test
setup function that uses getUserTokenURL or a helper like NewTestClient) or move
the demo file out of the _test.go compilation unit so no side-effecting init
runs; locate references to larkClient and replace package-level usage with a
constructor/getter that returns a new or cached client at runtime.
🪄 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: cd308a5c-6ccf-4772-9fba-1028f6752ec1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/auth/get_user_token_url.demo_test.gogo.mod
Comment on lines
+1
to
+17
| package auth | ||
|
|
||
| // this file is a demo for user_token_getter_url | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
| "strconv" | ||
|
|
||
| "github.com/cloudwego/hertz/pkg/app" | ||
| "github.com/cloudwego/hertz/pkg/common/utils" | ||
| lark "github.com/larksuite/oapi-sdk-go/v3" | ||
| larkauthen "github.com/larksuite/oapi-sdk-go/v3/service/authen/v1" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there are no Test/Benchmark/Example funcs in this file, and that login.go does not reference these handlers.
fd -t f 'get_user_token_url.demo_test.go' --exec rg -nP '^func (Test|Benchmark|Example)\w+\(' {}
echo "---"
rg -nP --type=go -C2 '\b(UserAuth|OAuthCallback)\b'Repository: larksuite/cli
Length of output: 760
🏁 Script executed:
# First, read the full demo file to confirm its structure and purpose
cat -n cmd/auth/get_user_token_url.demo_test.goRepository: larksuite/cli
Length of output: 4294
🏁 Script executed:
# Check if login.go references the UserAuth or OAuthCallback handlers
rg -n --type=go 'UserAuth|OAuthCallback' cmd/auth/login.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Search for any tests related to --user-token-getter-url or user_token_getter_url
rg -n --type=go 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' tests/ 2>/dev/null || echo "No tests found with that pattern"Repository: larksuite/cli
Length of output: 89
🏁 Script executed:
# Check if there are actual test files for the login.go feature
fd -t f 'login.*_test\.go' cmd/auth/Repository: larksuite/cli
Length of output: 206
🏁 Script executed:
# Check if this file is referenced in any documentation, tests, or configuration
rg -n 'get_user_token_url\.demo_test|user_token_getter_url' --type=go --type=mdRepository: larksuite/cli
Length of output: 248
🏁 Script executed:
# Check for any build tags or conditional compilation that might reference this
grep -r 'get_user_token_url.demo_test' .Repository: larksuite/cli
Length of output: 194
🏁 Script executed:
# Check if there are any tests that actually exercise the --user-token-getter-url flag
rg -n 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' cmd/auth/login_test.goRepository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check if the login.go file defines the --user-token-getter-url flag and how it uses the config
rg -n -A5 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' cmd/auth/login.goRepository: larksuite/cli
Length of output: 518
🏁 Script executed:
# Check what fetchTokenViaGetter does and whether it expects HTTP endpoints like UserAuth/OAuthCallback
rg -n -A10 'func fetchTokenViaGetter' cmd/auth/Repository: larksuite/cli
Length of output: 674
🏁 Script executed:
# Verify if there are any E2E tests for the user-token-getter-url feature
fd -t f 'test.go$' tests/cli_e2e/ | xargs rg -l 'user.token.getter.url|UserTokenGetterUrl' 2>/dev/null || echo "No E2E tests found"Repository: larksuite/cli
Length of output: 75
🏁 Script executed:
# Let me see the full fetchTokenViaGetter function to understand how it uses the getter URL
rg -n -A30 'func fetchTokenViaGetter' cmd/auth/login.goRepository: larksuite/cli
Length of output: 1148
🏁 Script executed:
# Continue reading the fetchTokenViaGetter function to see how it uses getterURL
rg -n -A60 'func fetchTokenViaGetter' cmd/auth/login.go | head -80Repository: larksuite/cli
Length of output: 2168
🏁 Script executed:
# Get the rest of the fetchTokenViaGetter function to see how it constructs and uses the getter URL
rg -n -A100 'func fetchTokenViaGetter' cmd/auth/login.go | tail -50Repository: larksuite/cli
Length of output: 1614
🏁 Script executed:
# Check if there are any tests for the user-token-getter-url feature at all
rg -n 'UserTokenGetterUrl|user.token.getter.url' tests/ --type=go 2>/dev/null || echo "No tests found"Repository: larksuite/cli
Length of output: 71
🏁 Script executed:
# Check login_test.go to see what scenarios are tested
rg -n 'func Test' cmd/auth/login_test.go | head -20Repository: larksuite/cli
Length of output: 1250
🏁 Script executed:
# Double-check that the handlers in this demo file are completely unused by searching all production code
rg -n 'UserAuth|OAuthCallback' cmd/ --type=go --glob='!*_test.go' 2>/dev/null || echo "No matches in production code"Repository: larksuite/cli
Length of output: 86
Rename or relocate this file — it is a demo, not a test.
The file is named *_test.go but contains no Test*, Benchmark*, or Example* functions, and the header explicitly identifies it as "a demo for user_token_getter_url." Go's test runner will include it in the test binary but never execute anything from it. The exported UserAuth and OAuthCallback handlers are reference implementations showing how a custom getter URL should implement the OAuth flow (as consumed by fetchTokenViaGetter in login.go), not test code.
Move this to an examples/ directory (e.g., examples/user_token_getter/main.go) as a standalone main package with proper documentation, or rename it without the _test.go suffix and add a //go:build demo tag to prevent it from bloating the default binary.
Additionally, per the coding guidelines requiring tests for every behavior change, the --user-token-getter-url feature in login.go lacks any unit or integration tests. The demo file cannot serve as a substitute — add tests that exercise the feature's actual code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url.demo_test.go` around lines 1 - 17, This demo file
is misnamed as a test and should be moved or gated: remove the `_test.go` suffix
and either relocate the contents into an examples package (e.g.,
examples/user_token_getter/main.go under package main) or add a build tag like
`//go:build demo` so it isn't included in normal test binaries; ensure exported
handlers UserAuth and OAuthCallback remain intact for demonstration. Also add
real tests covering the --user-token-getter-url behavior exercised by
fetchTokenViaGetter in login.go (unit tests that mock the getter URL and
integration tests for the OAuth flow) so the feature has coverage rather than
relying on the demo file.
Comment on lines
+46
to
+62
| func OAuthCallback(ctx context.Context, c *app.RequestContext) { //ignore_security_alert IDOR | ||
| code := c.Query("code") | ||
| state := c.Query("state") | ||
| if state == "" { | ||
| state = "8888" | ||
| } | ||
|
|
||
| if code == "" { | ||
| c.JSON(http.StatusBadRequest, utils.H{"error": "missing code"}) | ||
| return | ||
| } | ||
|
|
||
| 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.
Validate state as a valid TCP port before using it as %d in the callback URL.
state is accepted from the query string and parsed with strconv.Atoi, then formatted into http://127.0.0.1:%d/user_access_token. Any integer — including negative numbers, 0, or values > 65535 — passes the current check. Clamp to a valid ephemeral/user port range (e.g., 1024 <= stateInt <= 65535) so a malformed/malicious state can't produce a bogus callback URL.
Proposed fix
stateInt, err := strconv.Atoi(state)
- if err != nil {
- c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be integer"})
- return
- }
+ if err != nil || stateInt < 1 || stateInt > 65535 {
+ c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be a TCP port in [1,65535]"})
+ return
+ }🤖 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 46 - 62, The
OAuthCallback handler currently accepts any integer state and uses stateInt when
formatting the callback URL; restrict stateInt to a valid TCP port range (e.g.,
1024 <= stateInt <= 65535) before using it. In OAuthCallback, after
strconv.Atoi(state) and err check, add a validation that returns
http.StatusBadRequest with an appropriate error JSON if stateInt is <1024 or
>65535 (or otherwise out of your chosen port bounds), so only a legitimate port
value is used when formatting "http://127.0.0.1:%d/user_access_token".
Comment on lines
+97
to
+128
| 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)) | ||
|
|
||
| c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(html)) |
There was a problem hiding this comment.
Interpolating JSON into an inline <script> block risks </script> breakout; also fix the user-facing typo.
dataBytes is json.Marshaled struct output, which does not escape < or /, so any string field that happens to contain </script> would terminate the script tag and enable HTML injection into the demo page. Two low-effort fixes:
- Use
json.EncoderwithSetEscapeHTML(true)(default) and additionally replace<,>,&for JS-in-HTML safety, or simpler: emit the payload from a separate/token-dataendpoint andfetch()it instead of string-interpolating into the HTML. - Minor:
Token sended→Token sent.
Minimal safer interpolation
- var data = %s;
+ var data = JSON.parse(%q);
...
-`, stateInt, string(dataBytes))
+`, stateInt, string(dataBytes))(passing via %q so the JSON becomes a quoted JS string literal, then JSON.parse-ing it, which removes the </script> breakout vector.)
🤖 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 97 - 128, The HTML
demo currently injects raw JSON (dataBytes) into the inline <script> via
fmt.Sprintf in the html variable (see html := fmt.Sprintf(..., stateInt,
string(dataBytes))) which allows a </script> breakout and XSS; change this by
not interpolating raw JSON into the script — either serve the JSON from a
separate handler (e.g., a /token-data endpoint fetched by window.onload before
POSTing to /user_access_token) or ensure safe embedding by passing the JSON as a
quoted JS string (use %q) and JSON.parse on the client or use json.Encoder with
SetEscapeHTML(true) and replace problematic chars, and also fix the user-facing
message "Token sended" to "Token sent" (references: dataBytes,
fmt.Sprintf(html...), window.onload, /user_access_token, c.Data).
| require ( | ||
| github.com/charmbracelet/huh v1.0.0 | ||
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| github.com/cloudwego/hertz v0.10.4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all hertz imports and whether they come from _test.go or production files.
rg -nP --type=go -C1 'cloudwego/hertz'
echo "---"
# Production-only usages
rg -lP --type=go 'cloudwego/hertz' | rg -v '_test\.go$' || echo "No non-test files import hertz"Repository: larksuite/cli
Length of output: 397
Run go mod tidy to remove unnecessary direct dependencies.
The github.com/cloudwego/hertz import is only used in cmd/auth/get_user_token_url.demo_test.go. Since it's a test-only dependency, it should not be a direct require in go.mod — it will be added as an indirect dependency automatically. Run go mod tidy to resync the module files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 8, The go.mod currently lists a direct require for
github.com/cloudwego/hertz even though it's only used in tests
(cmd/auth/get_user_token_url.demo_test.go); run `go mod tidy` to remove that
unnecessary direct dependency so it becomes indirect, then run the test suite to
verify nothing breaks and commit the updated go.mod/go.sum.
… app manager without app secret, then use lark-cli
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
f2d5c4b to
a65ad25
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/auth/login.go (2)
441-529:⚠️ Potential issue | 🔴 CriticalHarden the localhost callback before accepting tokens.
stateis the callback port and the handler does not validate it, so any local process or browser page can race a fake/user_access_tokencallback. Generate an unguessable nonce, validate it on the callback, and cap POST body size.Hardening sketch
+ nonce, err := newCallbackNonce() + if err != nil { + return "", fmt.Errorf("failed to generate callback state: %w", err) + } + mux := http.NewServeMux() mux.HandleFunc("/user_access_token", func(w http.ResponseWriter, r *http.Request) { @@ if r.Method == http.MethodOptions { w.WriteHeader(http.StatusOK) return } + if r.URL.Query().Get("state") != nonce { + http.Error(w, "invalid callback state", http.StatusBadRequest) + select { + case errCh <- fmt.Errorf("invalid callback state"): + default: + } + return + } var tokenData string if r.Method == http.MethodPost { - body, err := io.ReadAll(r.Body) + r.Body = http.MaxBytesReader(w, r.Body, 64<<10) + body, err := io.ReadAll(r.Body) if err != nil { http.Error(w, "failed to read body", http.StatusBadRequest) @@ q := u.Query() - q.Set("state", strconv.Itoa(port)) + q.Set("port", strconv.Itoa(port)) + q.Set("state", nonce)func newCallbackNonce() (string, error) { var b [32]byte if _, err := rand.Read(b[:]); err != nil { return "", err } return hex.EncodeToString(b[:]), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 441 - 529, The handler in fetchTokenViaGetter trusts the callback "state" (currently the port) and accepts arbitrary bodies; fix by generating a cryptographically-secure nonce (use crypto/rand) when building getterURL (in fetchTokenViaGetter), set it as the "state" query value instead of the port, and modify the /user_access_token handler to validate the incoming r.URL.Query().Get("state") equals the nonce before accepting the token; also cap POST body size with an io.LimitReader or http.MaxBytesReader (e.g., a small fixed limit) when reading r.Body to prevent large payloads. Ensure the nonce is available inside the handler closure (capture it) and return an error via errCh if the state mismatches.
344-358:⚠️ Potential issue | 🟠 MajorDon’t report getter login success without scope evidence.
The getter flow still stores
Scope: ""and callswriteLoginSuccesswith nologinScopeSummary, soauth login --scope ...can succeed even when the returned token lacks the requested permissions.Suggested direction
type UserTokenData struct { AccessToken *string `json:"access_token,omitempty"` // user_access_token，用于获取用户资源 + Scope *string `json:"scope,omitempty"` // granted scopes, space-separated TokenType *string `json:"token_type,omitempty"` // token 类型 @@ expiresIn := 0 if gt.ExpiresIn != nil { expiresIn = *gt.ExpiresIn } + grantedScope := "" + if gt.Scope != nil { + grantedScope = *gt.Scope + } @@ ExpiresAt: expiresAt, RefreshExpiresAt: expiresAt, - Scope: "", // 这种方式暂不解析 scope + Scope: grantedScope, GrantedAt: now, } @@ - writeLoginSuccess(opts, msg, f, openId, userName, nil) + scopeSummary := loadLoginScopeSummary(config.AppID, openId, finalScope, grantedScope) + if issue := ensureRequestedScopesGranted(finalScope, grantedScope, msg, scopeSummary); issue != nil { + return handleLoginScopeIssue(opts, msg, f, issue, openId, userName) + } + + writeLoginSuccess(opts, msg, f, openId, userName, scopeSummary) return nil }Also applies to: 417-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 344 - 358, The PR reports login success even when the token lacks the requested scopes; add a Scope field to UserTokenData, populate it in the getter/token-parsing flow, and pass a computed loginScopeSummary to writeLoginSuccess only after verifying the returned Scope contains the requested scopes; if the Scope is empty or does not include the requested scope, return an error (or fail the login) instead of calling writeLoginSuccess. Ensure you update the UserTokenData struct, the token parsing code in the getter flow that constructs UserTokenData, and the call sites of writeLoginSuccess to use the verified scope summary.
🤖 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/login_test.go`:
- Line 952: The tests call fetchTokenViaGetter and authLoginViaGetter which
currently invoke the real openBrowser and can hang in headless CI; add a test
helper stubOpenBrowser(t) that replaces the package-level openBrowser function
with a no-op that captures/validates the URL and returns nil (and restores the
original openBrowser on cleanup), then call stubOpenBrowser(t) at the start of
each getter-flow test (those calling fetchTokenViaGetter and authLoginViaGetter)
so the mock getter callback is triggered reliably without launching a real
browser; ensure the stub handles contexts/timeouts used in production (e.g.,
context.Background() and the 5-minute timeout) and is used in the tests at lines
referenced (including tests around fetchTokenViaGetter and authLoginViaGetter).
---
Duplicate comments:
In `@cmd/auth/login.go`:
- Around line 441-529: The handler in fetchTokenViaGetter trusts the callback
"state" (currently the port) and accepts arbitrary bodies; fix by generating a
cryptographically-secure nonce (use crypto/rand) when building getterURL (in
fetchTokenViaGetter), set it as the "state" query value instead of the port, and
modify the /user_access_token handler to validate the incoming
r.URL.Query().Get("state") equals the nonce before accepting the token; also cap
POST body size with an io.LimitReader or http.MaxBytesReader (e.g., a small
fixed limit) when reading r.Body to prevent large payloads. Ensure the nonce is
available inside the handler closure (capture it) and return an error via errCh
if the state mismatches.
- Around line 344-358: The PR reports login success even when the token lacks
the requested scopes; add a Scope field to UserTokenData, populate it in the
getter/token-parsing flow, and pass a computed loginScopeSummary to
writeLoginSuccess only after verifying the returned Scope contains the requested
scopes; if the Scope is empty or does not include the requested scope, return an
error (or fail the login) instead of calling writeLoginSuccess. Ensure you
update the UserTokenData struct, the token parsing code in the getter flow that
constructs UserTokenData, and the call sites of writeLoginSuccess to use the
verified scope summary.
🪄 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: 0a1bade2-b237-438c-88cb-e603db21ab1a
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
| // 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) |
There was a problem hiding this comment.
Avoid launching a real browser from unit tests.
These tests only trigger the mock getter callback if openBrowser actually opens the getter URL. In headless CI this can fail or hang, especially the authLoginViaGetter cases using context.Background() and the production 5-minute timeout.
Suggested testability refactor
Production:
-func openBrowser(ctx context.Context, url string) error {
+var openBrowser = defaultOpenBrowser
+
+func defaultOpenBrowser(ctx context.Context, url string) error {
// 简单的跨平台打开浏览器实现
var err errorTests:
+func stubOpenBrowser(t *testing.T) {
+ t.Helper()
+ original := openBrowser
+ t.Cleanup(func() { openBrowser = original })
+ openBrowser = func(ctx context.Context, target string) error {
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, target, nil)
+ if err != nil {
+ return err
+ }
+ resp, err := http.DefaultClient.Do(req)
+ if err != nil {
+ return err
+ }
+ _ = resp.Body.Close()
+ return nil
+ }
+}Then call stubOpenBrowser(t) in each getter-flow test before invoking fetchTokenViaGetter or authLoginViaGetter.
Also applies to: 994-994, 1123-1123, 1242-1242, 1312-1312, 1361-1361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login_test.go` at line 952, The tests call fetchTokenViaGetter and
authLoginViaGetter which currently invoke the real openBrowser and can hang in
headless CI; add a test helper stubOpenBrowser(t) that replaces the
package-level openBrowser function with a no-op that captures/validates the URL
and returns nil (and restores the original openBrowser on cleanup), then call
stubOpenBrowser(t) at the start of each getter-flow test (those calling
fetchTokenViaGetter and authLoginViaGetter) so the mock getter callback is
triggered reliably without launching a real browser; ensure the stub handles
contexts/timeouts used in production (e.g., context.Background() and the
5-minute timeout) and is used in the tests at lines referenced (including tests
around fetchTokenViaGetter and authLoginViaGetter).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
cmd/auth/login.go (2)
344-439:⚠️ Potential issue | 🟠 MajorGetter flow stores empty
Scope— requested scopes silently unverified.When users run
lark-cli auth login --scope foo:bar --user-token-getter-url ...,authLoginViaGetterwritesScope: ""into theStoredUATokenand callswriteLoginSuccess(..., nil). The device flow (lines 312, 336) callsensureRequestedScopesGranted; this path does not. The command therefore reports success even if the returned token is missing the requested scopes, andlark-cli auth statuswill subsequently show empty scopes.Add a
Scope *stringfield toUserTokenData, persist it intostoredToken.Scope, and runensureRequestedScopesGranted(finalScope, returnedScope, ...)before reporting success (removing the stored token on mismatch, as the device flow does).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 344 - 439, The getter flow currently ignores scopes: add a Scope *string field to UserTokenData, read it after unmarshalling in authLoginViaGetter, set storedToken.Scope = "" or *gt.Scope accordingly before calling larkauth.SetStoredToken, then call ensureRequestedScopesGranted(finalScope, storedToken.Scope, config.AppID, openId) (or the same signature used by device flow) and if it returns an error remove the stored token via larkauth.RemoveStoredToken(config.AppID, openId) and return that error; only call writeLoginSuccess(...) after ensureRequestedScopesGranted succeeds.
441-505:⚠️ Potential issue | 🔴 CriticalCallback is unauthenticated — any local process can race a forged token.
stateis just the port (strconv.Itoa(port)), which is neither unguessable nor validated on the handler side. Any local page or process can POST{"access_token":"attacker_uat"}tohttp://127.0.0.1:<port>/user_access_tokenbefore the legitimate admin page does; the CLI will accept the first payload and store it as the current user's UAT. Additionally, theio.ReadAll(r.Body)on line 475 has no size cap.Generate a cryptographically-random nonce (
crypto/rand), require it on the callback (r.URL.Query().Get("state") == nonce), and wrap the body withhttp.MaxBytesReader. Keep the port in a separate query param so the getter can still reach the right 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 441 - 505, The callback handler in fetchTokenViaGetter is unauthenticated and reads the full body unbounded; generate a cryptographically-random nonce (use crypto/rand) when starting the listener and include it as the expected "state" query parameter (separate from the port), then require that the incoming request's r.URL.Query().Get("state") matches this nonce before accepting the token in the /user_access_token handler; also wrap r.Body with http.MaxBytesReader to enforce a reasonable size limit (e.g., a few KB) before calling io.ReadAll and reject requests that exceed or fail validation, and continue sending errors to errCh when validation fails.internal/core/config.go (1)
255-282:⚠️ Potential issue | 🟠 Major
UserTokenGetterUrlshould complement, not shadow, a configuredAppSecret.Condition
if app.UserTokenGetterUrl == ""on line 257 gates bothValidateSecretKeyMatchandResolveSecretInput. For a profile that has both a realAppSecretand aUserTokenGetterUrl(valid configuration — bot/TAT flows still need the real secret, and the getter flow is only an alternative UAT acquisition path), the resolvedCliConfig.AppSecretwill be"". Any downstream code that needs a tenant access token (e.g.+sendcard,--as bot) will then fail to authenticate.The guard should be on whether
AppSecretis set, not on whether the getter URL is set:🔧 Proposed fix
- var secret string - var err error - if app.UserTokenGetterUrl == "" { + var secret string + 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) + resolved, 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()} } + secret = resolved + } else if app.UserTokenGetterUrl == "" { + return nil, &ConfigError{Code: 2, Type: "config", + Message: "profile has neither AppSecret nor UserTokenGetterUrl configured"} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 255 - 282, The code incorrectly gates ValidateSecretKeyMatch and ResolveSecretInput on app.UserTokenGetterUrl, causing a configured AppSecret to be ignored when a UserTokenGetterUrl is present; change the condition to check AppSecret instead (e.g., if app.AppSecret != "" or if app.AppSecret == "" depending on current logic) so that ValidateSecretKeyMatch(app.AppId, app.AppSecret) and secret, err = ResolveSecretInput(app.AppSecret, kc) run for profiles that provide an AppSecret regardless of app.UserTokenGetterUrl, ensuring CliConfig.AppSecret is populated; update references around ValidateSecretKeyMatch, ResolveSecretInput, app.AppSecret, app.UserTokenGetterUrl, and CliConfig.AppSecret accordingly.cmd/config/init.go (1)
321-329:⚠️ Potential issue | 🟠 MajorInteractive TUI path still unconditionally calls
ForStorage, even when onlyUserTokenGetterUrlwas provided.When
existing == niland the user completes the form with aUserTokenGetterUrland an emptyApp Secret,result.AppSecret == ""falls through tocore.ForStorage(result.AppID, core.PlainSecret(""), f.Keychain), which writes an empty keychain entry. The legacy readline branch at lines 437-438/441 correctly guards this viaresolvedSecret.IsZero(), and the non-interactive branch at lines 244-251 does the same. Match those paths:🔧 Proposed fix
if existing == nil { - // New secret provided (either from "create" or "existing" with input) - secret, err := core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain) - if err != nil { - return output.Errorf(output.ExitInternal, "internal", "%v", err) + var secret core.SecretInput + if result.AppSecret != "" { + var err error + secret, err = core.ForStorage(result.AppID, core.PlainSecret(result.AppSecret), f.Keychain) + if err != nil { + return output.Errorf(output.ExitInternal, "internal", "%v", err) + } } if err := saveInitConfig(opts.ProfileName, existing, f, result.AppID, secret, result.Brand, opts.Lang, result.UserTokenGetterUrl); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 321 - 329, When handling the interactive TUI path inside the existing == nil branch, avoid calling core.ForStorage with an empty AppSecret; check result.AppSecret (or construct a resolvedSecret and call resolvedSecret.IsZero()) and only call core.ForStorage(result.AppID, core.PlainSecret(...), f.Keychain) when a non-empty secret was provided; if the secret is empty, pass a nil/zero secret into saveInitConfig (matching the legacy readline and non-interactive branches) so you don't write an empty keychain entry. Ensure you update the logic around core.ForStorage and the subsequent saveInitConfig call accordingly.
🧹 Nitpick comments (2)
cmd/auth/login.go (2)
559-577:exec.CommandContextwill SIGKILL the user's browser whenctxcancels.The
ctxpassed here isopts.Ctx(the root command context), which is cancelled as soon asauthLoginViaGetterreturns — including on the success path via thedefer server.Shutdown. On Linux/macOS the launchedxdg-open/openprocess is transient and has typically already exec'd, but on Windowsrundll32stays alive long enough that cancellation after success can still terminate it mid-launch. Thedefaultcase is also dead code — Go only supports a fixed set ofGOOSvalues and every one you'd target has its own case above.Prefer
exec.Command(...).Start()(orexec.CommandContext(context.Background(), ...)) so browser-launching is decoupled from the login-wait context, and drop the unreachabledefaultbranch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 559 - 577, The openBrowser function uses exec.CommandContext(ctx, ...) which lets the root command context (opts.Ctx) cancel and SIGKILL the browser launcher; change the calls in openBrowser to use exec.Command(...) or exec.CommandContext(context.Background(), ...) so the browser launch is decoupled from opts.Ctx and will not be killed when authLoginViaGetter returns, and remove the unreachable default branch (keep explicit cases for "linux", "darwin", "windows") to simplify the switch.
441-457: Replacerand.Seed+ port-scanning loop with127.0.0.1:0.Two issues in the listener setup:
rand.Seedis deprecated since Go 1.20; the top-level generator is now auto-seeded. Withgo 1.23.0pinned, this call emits a staticcheck/SA1019 warning.- Picking a random port in
[8000, 10000)and retrying up to 10 times risks collisions with other local services (Prometheus on 9090, Node dev servers, etc.). Letting the OS pick via127.0.0.1:0is more reliable.The refactor uses
listener.Addr().(*net.TCPAddr).Portto extract the assigned port. Drop themath/randimport; keepstrconv(still used at line 529 forq.Set("state", strconv.Itoa(port))).♻️ Proposed refactor
-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) - } +func fetchTokenViaGetter(ctx context.Context, getterURL string, scope string, log func(string, ...interface{})) (string, error) { + 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 441 - 457, In fetchTokenViaGetter, remove the rand.Seed call and the manual port-scanning loop; instead call net.Listen on "127.0.0.1:0" to let the OS pick an available port, then obtain the assigned port via listener.Addr().(*net.TCPAddr).Port; keep the same error handling (returning fmt.Errorf on failure) and continue using strconv.Itoa(port) for q.Set("state", ...); also remove the math/rand import from the file since it’s no longer used.
🤖 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/login.go`:
- Around line 485-499: The response header Content-Type is being set after
calling w.WriteHeader (seen at the success branch where
w.WriteHeader(http.StatusOK) precedes w.Header().Set("Content-Type", ...)), so
move the Content-Type header set to before the WriteHeader call; do the same
ordering for the success case (the error branch already sets Content-Type before
WriteHeader where tokenData == ""), ensuring both branches set
w.Header().Set("Content-Type", "text/html; charset=utf-8") before calling
w.WriteHeader.
---
Duplicate comments:
In `@cmd/auth/login.go`:
- Around line 344-439: The getter flow currently ignores scopes: add a Scope
*string field to UserTokenData, read it after unmarshalling in
authLoginViaGetter, set storedToken.Scope = "" or *gt.Scope accordingly before
calling larkauth.SetStoredToken, then call
ensureRequestedScopesGranted(finalScope, storedToken.Scope, config.AppID,
openId) (or the same signature used by device flow) and if it returns an error
remove the stored token via larkauth.RemoveStoredToken(config.AppID, openId) and
return that error; only call writeLoginSuccess(...) after
ensureRequestedScopesGranted succeeds.
- Around line 441-505: The callback handler in fetchTokenViaGetter is
unauthenticated and reads the full body unbounded; generate a
cryptographically-random nonce (use crypto/rand) when starting the listener and
include it as the expected "state" query parameter (separate from the port),
then require that the incoming request's r.URL.Query().Get("state") matches this
nonce before accepting the token in the /user_access_token handler; also wrap
r.Body with http.MaxBytesReader to enforce a reasonable size limit (e.g., a few
KB) before calling io.ReadAll and reject requests that exceed or fail
validation, and continue sending errors to errCh when validation fails.
In `@cmd/config/init.go`:
- Around line 321-329: When handling the interactive TUI path inside the
existing == nil branch, avoid calling core.ForStorage with an empty AppSecret;
check result.AppSecret (or construct a resolvedSecret and call
resolvedSecret.IsZero()) and only call core.ForStorage(result.AppID,
core.PlainSecret(...), f.Keychain) when a non-empty secret was provided; if the
secret is empty, pass a nil/zero secret into saveInitConfig (matching the legacy
readline and non-interactive branches) so you don't write an empty keychain
entry. Ensure you update the logic around core.ForStorage and the subsequent
saveInitConfig call accordingly.
In `@internal/core/config.go`:
- Around line 255-282: The code incorrectly gates ValidateSecretKeyMatch and
ResolveSecretInput on app.UserTokenGetterUrl, causing a configured AppSecret to
be ignored when a UserTokenGetterUrl is present; change the condition to check
AppSecret instead (e.g., if app.AppSecret != "" or if app.AppSecret == ""
depending on current logic) so that ValidateSecretKeyMatch(app.AppId,
app.AppSecret) and secret, err = ResolveSecretInput(app.AppSecret, kc) run for
profiles that provide an AppSecret regardless of app.UserTokenGetterUrl,
ensuring CliConfig.AppSecret is populated; update references around
ValidateSecretKeyMatch, ResolveSecretInput, app.AppSecret,
app.UserTokenGetterUrl, and CliConfig.AppSecret accordingly.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 559-577: The openBrowser function uses exec.CommandContext(ctx,
...) which lets the root command context (opts.Ctx) cancel and SIGKILL the
browser launcher; change the calls in openBrowser to use exec.Command(...) or
exec.CommandContext(context.Background(), ...) so the browser launch is
decoupled from opts.Ctx and will not be killed when authLoginViaGetter returns,
and remove the unreachable default branch (keep explicit cases for "linux",
"darwin", "windows") to simplify the switch.
- Around line 441-457: In fetchTokenViaGetter, remove the rand.Seed call and the
manual port-scanning loop; instead call net.Listen on "127.0.0.1:0" to let the
OS pick an available port, then obtain the assigned port via
listener.Addr().(*net.TCPAddr).Port; keep the same error handling (returning
fmt.Errorf on failure) and continue using strconv.Itoa(port) for q.Set("state",
...); also remove the math/rand import from the file since it’s no longer used.
🪄 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: dbdd3763-043e-48e2-bbf0-07f6e1099f58
⛔ 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
- cmd/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/auth/get_user_token_url.demo_test.go
Comment on lines
+485
to
+499
| 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>`) |
There was a problem hiding this comment.
WriteHeader is called before Header().Set("Content-Type", ...).
Once w.WriteHeader(http.StatusOK) runs on line 496, the response headers are flushed to the wire; the w.Header().Set("Content-Type", ...) on line 497 is a silent no-op on the network response (Go logs http: superfluous response.WriteHeader call in some cases). Browsers then rely on MIME-sniffing of the HTML body, which may trigger X-Content-Type-Options warnings or render as plain text in strict clients.
🔧 Proposed fix
- w.WriteHeader(http.StatusOK)
w.Header().Set("Content-Type", "text/html; charset=utf-8")
+ w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `<html>…`)Apply the same ordering to the error branch at lines 486-488 (it is already correct there).
📝 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
| 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>`) | |
| 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.Header().Set("Content-Type", "text/html; charset=utf-8") | |
| w.WriteHeader(http.StatusOK) | |
| 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>`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 485 - 499, The response header Content-Type
is being set after calling w.WriteHeader (seen at the success branch where
w.WriteHeader(http.StatusOK) precedes w.Header().Set("Content-Type", ...)), so
move the Content-Type header set to before the WriteHeader call; do the same
ordering for the success case (the error branch already sets Content-Type before
WriteHeader where tokenData == ""), ensuring both branches set
w.Header().Set("Content-Type", "text/html; charset=utf-8") before calling
w.WriteHeader.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/auth/login.go (2)
443-535:⚠️ Potential issue | 🔴 CriticalUse an unguessable callback state and cap callback bodies.
The callback still trusts the first request to
/user_access_token;stateis only the port and is never validated. A local process or malicious browser page can race a fake token into the CLI, and POST bodies are read without a size limit.🔒 Proposed hardening sketch
func fetchTokenViaGetter(ctx context.Context, getterURL string, scope string, log func(string, ...interface{})) (string, error) { + nonce, err := generateCallbackNonce() + if err != nil { + return "", fmt.Errorf("failed to generate callback state: %w", err) + } rand.Seed(time.Now().UnixNano()) @@ mux := http.NewServeMux() mux.HandleFunc("/user_access_token", func(w http.ResponseWriter, r *http.Request) { @@ if r.Method == http.MethodOptions { w.WriteHeader(http.StatusOK) return } + if r.URL.Query().Get("state") != nonce { + http.Error(w, "invalid state", http.StatusBadRequest) + select { + case errCh <- fmt.Errorf("invalid state in callback request"): + default: + } + return + } var tokenData string if r.Method == http.MethodPost { + r.Body = http.MaxBytesReader(w, r.Body, 1<<20) body, err := io.ReadAll(r.Body) @@ q := u.Query() - q.Set("state", strconv.Itoa(port)) + q.Set("state", nonce) + q.Set("port", strconv.Itoa(port))Add
generateCallbackNonceusingcrypto/randand base64url encoding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 443 - 535, fetchTokenViaGetter currently uses the port as the callback state and accepts the first request blindly and reads POST bodies without limits; fix by generating an unguessable nonce with crypto/rand (e.g., generateCallbackNonce) and include it in the redirect state (e.g., state = fmt.Sprintf("%d:%s") or separate state param), then in the /user_access_token handler validate the incoming state matches the generated nonce/port combo before accepting the token; also cap POST body reads by wrapping r.Body with http.MaxBytesReader (or io.LimitReader) to a reasonable size and reject oversized bodies, and reject requests where the state is missing or doesn't match.
344-358:⚠️ Potential issue | 🟠 MajorVerify getter-token scopes before reporting login success.
The getter flow still persists
Scope: ""and callswriteLoginSuccesswithout checkingfinalScope, soauth login --scope ...can succeed with a token that lacks the requested permissions.🛠️ Proposed direction
type UserTokenData struct { AccessToken *string `json:"access_token,omitempty"` // user_access_token，用于获取用户资源 + Scope *string `json:"scope,omitempty"` // granted scopes TokenType *string `json:"token_type,omitempty"` // token 类型 ExpiresIn *int `json:"expires_in,omitempty"` // `access_token`的有效期，单位: 秒 @@ expiresIn := 0 if gt.ExpiresIn != nil { expiresIn = *gt.ExpiresIn } + returnedScope := "" + if gt.Scope != nil { + returnedScope = *gt.Scope + } @@ ExpiresAt: expiresAt, RefreshExpiresAt: expiresAt, - Scope: "", // 这种方式暂不解析 scope + Scope: returnedScope, GrantedAt: now, } @@ - writeLoginSuccess(opts, msg, f, openId, userName, nil) + scopeSummary := loadLoginScopeSummary(config.AppID, openId, finalScope, returnedScope) + if issue := ensureRequestedScopesGranted(finalScope, returnedScope, msg, scopeSummary); issue != nil { + _ = larkauth.RemoveStoredToken(config.AppID, openId) + return handleLoginScopeIssue(opts, msg, f, issue, openId, userName) + } + + writeLoginSuccess(opts, msg, f, openId, userName, scopeSummary)Also applies to: 418-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 344 - 358, The getter flow currently calls writeLoginSuccess even when the returned token lacks requested permissions (finalScope can be empty), so update the getter flow to verify that finalScope (or the token's scope) contains all requested scopes before calling writeLoginSuccess; if finalScope does not satisfy requestedScope (the scope passed via auth login --scope or the local requestedScope variable), return an error or fail the login with a clear message and do not call writeLoginSuccess. Ensure this check is applied in the getter flow and the same logic is added where writeLoginSuccess is invoked (referencing finalScope and writeLoginSuccess) so login only succeeds when the token grants the required scopes.
🤖 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/login.go`:
- Around line 390-407: Ensure a non-empty openId before persisting credentials:
after calling getUserInfo (and before calling SetStoredToken or
syncLoginUserToProfile), check if openId is still empty and return an
authentication error via output.ErrAuth (or similar) instead of continuing.
Update the login flow around getUserInfo in the same block that calls
f.LarkClient(), getUserInfo, SetStoredToken and syncLoginUserToProfile so that
if fetchedOpenId (or final openId) is empty you abort with a clear error rather
than storing the token or syncing the profile.
---
Duplicate comments:
In `@cmd/auth/login.go`:
- Around line 443-535: fetchTokenViaGetter currently uses the port as the
callback state and accepts the first request blindly and reads POST bodies
without limits; fix by generating an unguessable nonce with crypto/rand (e.g.,
generateCallbackNonce) and include it in the redirect state (e.g., state =
fmt.Sprintf("%d:%s") or separate state param), then in the /user_access_token
handler validate the incoming state matches the generated nonce/port combo
before accepting the token; also cap POST body reads by wrapping r.Body with
http.MaxBytesReader (or io.LimitReader) to a reasonable size and reject
oversized bodies, and reject requests where the state is missing or doesn't
match.
- Around line 344-358: The getter flow currently calls writeLoginSuccess even
when the returned token lacks requested permissions (finalScope can be empty),
so update the getter flow to verify that finalScope (or the token's scope)
contains all requested scopes before calling writeLoginSuccess; if finalScope
does not satisfy requestedScope (the scope passed via auth login --scope or the
local requestedScope variable), return an error or fail the login with a clear
message and do not call writeLoginSuccess. Ensure this check is applied in the
getter flow and the same logic is added where writeLoginSuccess is invoked
(referencing finalScope and writeLoginSuccess) so login only succeeds when the
token grants the required scopes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+390
to
+407
| // 如果没有 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 | ||
| } |
There was a problem hiding this comment.
Require a resolved openId before storing the token.
If the callback omits open_id and getUserInfo returns an empty value, this flow continues and syncs the profile with an empty user identity. Fail before SetStoredToken/syncLoginUserToProfile when openId is still empty.
🛡️ Proposed fix
if openId == "" || userName == "" {
log(msg.AuthSuccess)
sdk, err := f.LarkClient()
@@
if userName == "" {
userName = fetchedUserName
}
} else {
log(msg.AuthSuccess)
}
+ if openId == "" {
+ return output.ErrAuth("authorization succeeded but user open_id could not be resolved")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 390 - 407, Ensure a non-empty openId before
persisting credentials: after calling getUserInfo (and before calling
SetStoredToken or syncLoginUserToProfile), check if openId is still empty and
return an authentication error via output.ErrAuth (or similar) instead of
continuing. Update the login flow around getUserInfo in the same block that
calls f.LarkClient(), getUserInfo, SetStoredToken and syncLoginUserToProfile so
that if fetchedOpenId (or final openId) is empty you abort with a clear error
rather than storing the token or syncing the profile.
bd9f955 to
087bb86
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