feat: add user_token_getter_url, people can get user token from app manager#655
Open
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add user_token_getter_url, people can get user token from app manager#655wangjingling wants to merge 1 commit intolarksuite:mainfrom
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a browser-based OAuth login flow using a configurable UserTokenGetterUrl and a localhost callback to receive user access tokens; extends config, credential, and CLI login logic to support the getter flow, includes demo HTTP handlers and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant CLI as CLI App
participant Local as Local HTTP Server
participant Getter as Token Getter Service
participant Lark as Lark OAuth API
Browser->>CLI: initiate login (no AppSecret)
CLI->>Local: start localhost listener (random port)
CLI->>Browser: open Getter URL (state=port, optional scope)
Browser->>Getter: authenticate & authorize
Getter->>Lark: exchange code for access token
Lark-->>Getter: return token JSON
Getter->>Local: POST token JSON to callback URL
Local-->>CLI: deliver token payload
CLI->>Lark: fetch user info if identity fields missing
Lark-->>CLI: return user info
CLI->>CLI: persist token/profile (keyring, config)
CLI-->>Browser: show login success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/config/init_interactive.go (1)
81-97:⚠️ Potential issue | 🟡 MinorNew prompt titles are not localized; consider validating URL format.
Two issues on the new interactive inputs:
- The surrounding form pulls titles through
msg.Platform/msg.SelectAction/msg.SelectPlatform, but the new labels ("App Secret (App Secret or UserTokenGetterUrl, enter at least one)" on line 81 and the whole block at lines 90–97) are hardcoded English. Chinese-lang users will see mixed localization.- The URL input accepts any string; if the user mistypes (missing scheme, stray spaces), the failure only surfaces later in
fetchTokenViaGetteras"failed to parse getterURL". Adding a lightweightValidateon the huh input catches it at entry time.🌐 Suggested validation + l10n hook
userTokenGetterUrlInput := huh.NewInput(). - Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})"). - Value(&userTokenGetterUrl) + Title(msg.UserTokenGetterUrlTitle). + Value(&userTokenGetterUrl). + Validate(func(s string) error { + if s == "" { + return nil + } + u, err := url.Parse(s) + if err != nil || (u.Scheme != "http" && u.Scheme != "https") { + return fmt.Errorf("must be a valid http(s) URL") + } + return nil + })🤖 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 81 - 97, Replace the hardcoded prompt titles for appSecretInput and userTokenGetterUrlInput with localized strings from the existing msg namespace (e.g., use msg.AppSecret and msg.UserTokenGetterUrl or similar) so labels follow the same localization flow as msg.Platform/msg.SelectAction; additionally add a lightweight Validate on userTokenGetterUrlInput that trims whitespace and returns nil for empty (since it's optional) but attempts to parse the URL (e.g., via net/url ParseRequestURI) and returns a user-facing error if invalid, so problems are caught at input time rather than later in fetchTokenViaGetter.cmd/config/init.go (1)
288-307:⚠️ Potential issue | 🟡 MinorSemantically inconsistent to combine
--newwith--user-token-getter-url.
--newcreates a fresh app via the app manager and always produces a realAppSecret. Persistingopts.UserTokenGetterUrlalongside it stores a configuration the auth flow can't unambiguously interpret (percmd/auth/login.go, the token-getter branch is selected whenAppSecretis empty, but here it won't be). Consider either ignoringUserTokenGetterUrlin this branch or rejecting the flag combination upfront inNewCmdConfigInit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 288 - 307, The init command currently allows combining opts.New with opts.UserTokenGetterUrl, which is semantically invalid because runCreateAppFlow produces a real AppSecret and cmd/auth/login.go treats an empty AppSecret as the token-getter branch; add validation in NewCmdConfigInit to detect the combination (opts.New && opts.UserTokenGetterUrl != "") and return a user-facing error instructing they cannot be used together, or alternatively clear opts.UserTokenGetterUrl before saving in init.go (before calling saveInitConfig) if you prefer to silently ignore it; update any usages/tests accordingly.
🧹 Nitpick comments (7)
cmd/auth/login_test.go (2)
952-955:logsslice is written but never asserted.
logsaccumulates vialogFnbut no test verifies any log content. Either assert something meaningful (e.g., the "Waiting for authorization" message) or drop the accumulation and passfunc(string, ...interface{}) {}like the invalid-JSON/no-token tests at lines 1385 and 1446 already do.Also applies to: 1010-1013, 1060-1063, 1167-1170, 1298-1301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_test.go` around lines 952 - 955, The test creates a logs slice and a logFn that appends formatted messages to logs but never asserts on it; either assert the expected log message (e.g., that "Waiting for authorization" or other expected string appears in logs) after the code under test, or change logFn to a no-op (func(string, ...interface{}) {}) like other tests; locate the variables logs and logFn in the login tests (the local variables named logs and logFn in login_test.go) and either add an assertion checking logs contains the expected entry or replace logFn with the noop function used in the invalid-JSON/no-token cases.
940-950: GlobalopenBrowserFnoverride precludes parallelism and leaks on failure paths.Every test in this batch mutates the package-level
openBrowserFnvariable. Consequences:
- These tests can never use
t.Parallel()(or be safely interleaved with other tests that touchopenBrowserFn) without data races.- In
TestFetchTokenViaGetter_MissingToken(line 999) and_Timeout(line 1049), the restore is done with a baredeferrather thant.Cleanup, which won't run if the subtest panics mid-setup.A cleaner approach is to push the browser-opener through the
cmdutil.Factory(or as an argument tofetchTokenViaGetter) so tests can inject a stub locally without touching package state. Short of that, at least uset.Cleanupeverywhere for symmetry.Also applies to: 999-1008, 1049-1058, 1110-1119, 1243-1253, 1338-1347, 1399-1408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_test.go` around lines 940 - 950, Tests globally override package-level openBrowserFn causing data races and cleanup leaks; change the design to avoid global mutation by passing the opener through cmdutil.Factory or as an explicit parameter to fetchTokenViaGetter so each test injects a local stub (reference symbols: openBrowserFn, fetchTokenViaGetter, cmdutil.Factory, TestFetchTokenViaGetter_MissingToken); if refactoring the API is not feasible right now, at minimum replace all bare defers that restore openBrowserFn with t.Cleanup calls in the listed tests (e.g., TestFetchTokenViaGetter_MissingToken and the other ranges) and stop mutating the package-level state concurrently so tests can safely run in parallel.internal/core/config.go (1)
41-49: JSON tag convention inconsistency and non-idiomatic Go name.Two small things worth correcting before this ships as a stored-on-disk schema:
user_token_getter_urlis snake_case, but every other key in this struct (appId,appSecret,defaultAs,strictMode) is camelCase. UsinguserTokenGetterUrlkeeps the persisted config schema consistent.- Go style recommends uppercasing initialisms:
UserTokenGetterURLrather thanUserTokenGetterUrl(same applies toCliConfig.UserTokenGetterUrlandcredential.Account.UserTokenGetterUrl). This is easier to change now than after the JSON tag lands on users' disks.🎨 Suggested change
- UserTokenGetterUrl string `json:"user_token_getter_url,omitempty"` // UserTokenGetterUrl defines an endpoint for fetching user tokens without manual authorization flow. + UserTokenGetterURL string `json:"userTokenGetterUrl,omitempty"` // UserTokenGetterURL defines an endpoint for fetching user tokens without manual authorization flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 41 - 49, Rename the struct field UserTokenGetterUrl to UserTokenGetterURL in internal/core/config.go (and analogous fields CliConfig.UserTokenGetterUrl and credential.Account.UserTokenGetterUrl) and update its JSON tag from "user_token_getter_url,omitempty" to "userTokenGetterUrl,omitempty" so the Go identifier uses the standard initialism capitalization and the persisted JSON key matches the existing camelCase convention; update all references, serializers/deserializers, and any tests to use the new field name and tag.cmd/auth/login.go (2)
347-361: Avoid all-pointer fields onUserTokenData; also prefer English comments.Every field is a
*T, presumably to distinguish "absent" from "zero value". For this payload that distinction is not meaningful (access_token=""is already invalid, as your own check on line 376 shows). All-pointer fields force defensive dereferencing at every callsite (see lines 381–391) and are a common source of nil-panics; plain value types withomitemptytags are idiomatic and safer here.Comments are Chinese-only; if the codebase targets a global audience this struct is part of the public shape worth documenting in English as well. (If Chinese comments are the house style here, ignore this half.)
🎨 Suggested simpler shape
type UserTokenData struct { - AccessToken *string `json:"access_token,omitempty"` // user_access_token，用于获取用户资源 - TokenType *string `json:"token_type,omitempty"` // token 类型 - ExpiresIn *int `json:"expires_in,omitempty"` // `access_token`的有效期，单位: 秒 - Name *string `json:"name,omitempty"` // 用户姓名 - EnName *string `json:"en_name,omitempty"` // 用户英文名称 - AvatarUrl *string `json:"avatar_url,omitempty"` // 用户头像 - AvatarThumb *string `json:"avatar_thumb,omitempty"` // 用户头像 72x72 - AvatarMiddle *string `json:"avatar_middle,omitempty"` // 用户头像 240x240 - AvatarBig *string `json:"avatar_big,omitempty"` // 用户头像 640x640 - OpenId *string `json:"open_id,omitempty"` // 用户在应用内的唯一标识 - UnionId *string `json:"union_id,omitempty"` // 用户统一ID - UserId *string `json:"user_id,omitempty"` // 用户 user_id - TenantKey *string `json:"tenant_key,omitempty"` // 当前企业标识 + AccessToken string `json:"access_token,omitempty"` + TokenType string `json:"token_type,omitempty"` + ExpiresIn int `json:"expires_in,omitempty"` + Name string `json:"name,omitempty"` + EnName string `json:"en_name,omitempty"` + AvatarUrl string `json:"avatar_url,omitempty"` + AvatarThumb string `json:"avatar_thumb,omitempty"` + AvatarMiddle string `json:"avatar_middle,omitempty"` + AvatarBig string `json:"avatar_big,omitempty"` + OpenId string `json:"open_id,omitempty"` + UnionId string `json:"union_id,omitempty"` + UserId string `json:"user_id,omitempty"` + TenantKey string `json:"tenant_key,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 347 - 361, The UserTokenData struct should use plain value types (string/int) instead of pointer types to avoid pervasive nil dereferences; change fields in UserTokenData (AccessToken, TokenType, ExpiresIn, Name, EnName, AvatarUrl, AvatarThumb, AvatarMiddle, AvatarBig, OpenId, UnionId, UserId, TenantKey) to non-pointer types with the same json `omitempty` tags and replace Chinese comments with concise English descriptions; then update call sites in this file that currently dereference pointers (see the checks and uses around the login handling logic referenced near lines ~376–391) to use direct value checks (e.g. AccessToken == "" or ExpiresIn == 0) and remove defensive nil checks accordingly. Ensure any JSON unmarshalling behavior remains correct and tests/usage that expect pointer semantics are updated to the new value semantics.
564-582:openBrowserdefaultbranch is unreachable for mainstream OSes and unlikely to help otherwise.Go's
runtime.GOOSis"linux","darwin","windows", plus a small set of BSDs /js/wasip1/plan9. Thedefaultcase here will only fire on BSDs etc., where tryingopen(macOS) and thenxdg-openis unlikely to be useful — the first likely doesn't exist, the second might. Simpler and clearer is to return an error and let the caller print the "please visit URL manually" message that already handles this case at line 542.♻️ Suggested simplification
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() - } + err = fmt.Errorf("automatic browser opening not supported on %s", runtime.GOOS) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 564 - 582, The default branch in openBrowser should not attempt macOS/linux commands for obscure OSes; instead return a clear error so the caller can fall back to its "please visit URL manually" message. Change the default case in openBrowser to return fmt.Errorf("unsupported platform: %s", runtime.GOOS) (or a similar descriptive error), remove the attempts to run "open" and "xdg-open" there, and ensure fmt is imported; leave the linux/darwin/windows cases unchanged so known OSes still invoke the proper commands.cmd/auth/get_user_token_url.demo_test.go (1)
26-45:UserAuthignores the incomingctxand swallowsurl.Parseerror.
ctx context.Contextis declared but never used; if this is intentional (Hertz-style handler), add_ = ctxor drop the parameter once the signature is locked.authURLObj, _ := url.Parse("https://open.feishu.cn/…")silently discards the error. While the literal is constant today, ignoring errors fromurl.Parseis a bad pattern to copy into follow-up work.🤖 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 26 - 45, In UserAuth, either acknowledge the unused ctx parameter (add "_ = ctx") or remove it per your handler signature, and stop ignoring url.Parse's error: capture the error from url.Parse("https://open.feishu.cn/open-apis/authen/v1/index"), check if err != nil and handle it (e.g., log via your logger and return a 500 or call c.String/c.Abort with an error) instead of discarding it; ensure you still build query on authURLObj and call c.Redirect(http.StatusFound, []byte(authURLObj.String())) only when parsing succeeded.cmd/config/init.go (1)
316-316: Minor wording: prefer "cannot both be empty".Tiny grammar polish in two places (Line 316 and Line 438).
✏️ Proposed tweak
- return output.ErrValidation("App ID cannot be empty, App Secret and UserTokenGetterUrl cannot be both empty") + return output.ErrValidation("App ID cannot be empty, App Secret and UserTokenGetterUrl cannot both be empty")- return output.ErrValidation("App Secret and UserTokenGetterUrl cannot be both empty") + return output.ErrValidation("App Secret and UserTokenGetterUrl cannot both be empty")Also applies to: 437-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` at line 316, Update the validation error messages to fix the grammar: change the text returned where it currently reads "App ID cannot be empty, App Secret and UserTokenGetterUrl cannot be both empty" to "App ID cannot be empty; App Secret and UserTokenGetterUrl cannot both be empty" (and make the same adjustment for the similar message around the UserTokenGetterUrl check at the other occurrence). Locate the return strings in the init/config validation code (the existing ErrValidation calls) and replace the message texts accordingly so they use "cannot both be empty" and a clearer separator (e.g., semicolon).
🤖 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`:
- Line 48: The IDOR suppression on OAuthCallback hides a real vulnerability: the
handler reads a user-controlled state and uses it to build a destination/port on
the caller machine; remove the suppression and fix by validating and restricting
state before use. In OAuthCallback, stop treating raw state as a
destination—either map incoming state to a server-side session entry or
decode/verify a signed token that contains an allowed destination, or validate
against a whitelist of permitted ports/hosts; reject any state that fails
verification and return an error. After implementing strict validation/mapping
in OAuthCallback (and any helper that parses state), remove the
"//ignore_security_alert IDOR" comment.
- Around line 27-30: The handler currently conflates the OAuth CSRF nonce
(c.Query("state")) with the client callback TCP port and defaults it to "8888";
change this so state is a cryptographically-random per-request nonce (do not
default to "8888" or accept a client-supplied numeric state), store it
server-side (e.g., in a signed cookie, session map, or in-memory map keyed by
nonce), and verify the callback exactly matches the stored nonce in the callback
handler (replace the loose integer parse check on state). Pass the callback port
separately and server-controlled (e.g., a server-side callbackPort variable or a
server-set "port" query param embedded in the redirect_uri) and stop using state
as the destination port when building the localhost POST URL; instead read the
port from your server-configured source. Ensure any fallback behavior is removed
so an absent or mismatched state rejects the callback.
- Around line 19-23: The package-level hardcoded values (appID, the
lark.NewClient call using "app_secret", and redirectURI) should be removed and
replaced so they are obtained at call time: read AppID, AppSecret and
RedirectURI from environment/config or from the CLI config object (e.g.,
CliConfig.AppID and CliConfig.RedirectURL) inside the test/demo setup or the
function that constructs the Lark client rather than at package init;
instantiate the Lark client via lark.NewClient(appID, appSecret) only after
fetching these values and ensure the redirect URI is derived from the runtime
test/server port or CliConfig before exercising token exchange flows.
- Line 118: Fix the user-facing typos in get_user_token_url.demo_test.go by
replacing the incorrect strings: change "Token sended，close in 3 seconds..."
(note the full-width comma and wrong tense) to a proper message such as "Token
sent, closing in 3 seconds..." and update the error message "Token send fail" to
"Failed to send token"; locate these exact string literals in the demo/test HTML
generation code and correct punctuation and wording accordingly.
In `@cmd/auth/login_test.go`:
- Around line 911-972: The test TestFetchTokenViaGetter_Success can race because
the mocked openBrowserFn triggers the getter callback before the local listener
is fully ready; mirror the retry logic used in the _SuccessWithAllFields test:
call fetchTokenViaGetter up to 5 times with a 100ms sleep between attempts
(break on success), and only t.Fatalf after all retries fail, keeping
openBrowserFn and logFn as-is; reference fetchTokenViaGetter, openBrowserFn, and
TestFetchTokenViaGetter_Success when applying this change.
In `@cmd/auth/login.go`:
- Around line 421-441: The stored token and success output for the getter-flow
currently drop the requested scopes; set the storedToken.Scope to finalScope
when building the larkauth.StoredUAToken (instead of ""), and pass a constructed
scope summary into writeLoginSuccess (e.g. build a loginScopeSummary with
Requested: splitScopes(finalScope) and Granted: splitScopes(finalScope) so the
getter-flow mirrors the device-flow output). Update references in this block
where storedToken is created and where writeLoginSuccess(...) is called so
syncLoginUserToProfile and token persistence remain unchanged but scope and the
login summary are populated.
- Around line 229-231: The branch decision should depend on whether an app
secret actually resolves, not on the view where ResolveConfigFromMulti blanked
it when a UserTokenGetterUrl exists; change the condition around the call to
authLoginViaGetter to attempt resolving the app secret (or call the existing
resolver helper used by ResolveConfigFromMulti) and branch on the resolved
result instead of checking config.AppSecret == ""; specifically, replace the
conjuct "config.AppSecret == \"\"" with a check that invokes the secret
resolution helper (or a new helper like HasResolvedAppSecret/ResolveAppSecret)
and use that boolean to decide whether to call authLoginViaGetter so profiles
that have both a getter URL and a real secret can still take the device-code
path.
- Around line 446-562: fetchTokenViaGetter's local callback is vulnerable:
replace the predictable port-as-state and GET-token flow with a
cryptographically random nonce and POST-only body handling, validate the
returned state using constant-time compare, and add basic origin/referer checks
and server timeouts. Concretely, in fetchTokenViaGetter generate a random state
nonce (not the port), include it in the getter URL (e.g., q.Set("state",
stateNonce)) and require the callback handler for "/user_access_token" to accept
only POST with the token in the request body (reject GETs and do not read token
from r.URL.Query()), verify the posted state matches the nonce before accepting
the token (use subtle.ConstantTimeCompare), and enforce Content-Type and
Origin/Referer checks as appropriate; also construct http.Server with sensible
timeouts (ReadHeaderTimeout/ReadTimeout/IdleTimeout) to avoid Slowloris.
- Around line 447-459: Remove the call to rand.Seed
(rand.Seed(time.Now().UnixNano())) and stop using the port number as the OAuth
state; keep the existing listener/port selection (net.Listen loop) but generate
a cryptographically secure random state using crypto/rand (e.g., read secure
random bytes and URL-safe/base64-encode them) and assign that to the state
parameter used in the OAuth flow instead of deriving it from port; ensure the
generated state is stored in the same variable/reference used later for
verification and sent with the authorization request.
In `@cmd/config/config_test.go`:
- Line 311: Add tests exercising the new userTokenGetterUrl path: call
saveAsProfile with a non-empty trailing URL and assert that
core.LoadMultiAppConfig().Apps[idx].UserTokenGetterUrl equals that URL (covers
persistence in saveAsProfile and assignment to
multi.Apps[idx].UserTokenGetterUrl), and add cases for
updateExistingProfileWithoutSecret that (1) update with a non-empty URL and
assert the app.UserTokenGetterUrl is overwritten, and (2) omit the URL (pass
empty string) and assert the existing UserTokenGetterUrl is preserved; reference
saveAsProfile, updateExistingProfileWithoutSecret, UserTokenGetterUrl, and
multi.Apps in your assertions.
In `@cmd/config/init.go`:
- Around line 205-207: The assignment to app.UserTokenGetterUrl is currently
guarded by "len(userTokenGetterUrl) > 0" which prevents clearing an existing
value; remove that guard and assign app.UserTokenGetterUrl = userTokenGetterUrl
unconditionally (like AppId, Brand, Lang) so an empty string clears the stored
URL; update any surrounding comment or logic if necessary to preserve intent.
- Line 65: Add table-driven unit tests covering the new --user-token-getter-url
flag and its precedence/validation: create tests that call configInitRun (via
the same CLI-helper used in other init tests) exercising these cases: (a)
--app-id + --user-token-getter-url (no secret) should succeed and persist
UserTokenGetterUrl in the saved profile, (b) both --app-id and --app-secret and
--user-token-getter-url provided should trigger validation failure (App Secret
and UserTokenGetterUrl cannot both be empty/contradicting rules), (c) only
--app-id with no secret and no getter URL should fail validation, and (d) update
flow that exercises saveAsProfile / updateExistingProfileWithoutSecret to ensure
an existing profile’s UserTokenGetterUrl is cleared or overwritten correctly
when updating; assert profile fields persisted/cleared as expected. Include
table rows for inputs, expected exit code/error, and expected stored profile
value for UserTokenGetterUrl, and wire tests to exercise opts.UserTokenGetterUrl
and the configInitRun/saveAsProfile/updateExistingProfileWithoutSecret code
paths.
- Around line 319-341: The interactive branch unconditionally calls
core.ForStorage with core.PlainSecret(result.AppSecret) which writes an empty
secret when the user chose the UserTokenGetterUrl path; change the logic so you
only call core.ForStorage when result.AppSecret != "" (mirror the
non-interactive check), otherwise leave the storage secret nil and pass that nil
secret into saveInitConfig; also update the final else to only return
output.ErrValidation("App ID cannot be empty") when result.AppID == "" and
otherwise return a clearer validation error (or handle the non-"existing" mode
explicitly) so the message is not misleading when existing != nil.
In `@internal/core/config.go`:
- Around line 255-274: The current block skips ValidateSecretKeyMatch and
ResolveSecretInput whenever UserTokenGetterUrl is set, which disables secret
resolution even if an AppSecret is configured; change the guard so secret
validation/resolution run only when an app secret is actually present (i.e.,
when app.AppSecret.IsZero() is false). Specifically, replace the condition using
app.UserTokenGetterUrl with a check on app.AppSecret (e.g., if
!app.AppSecret.IsZero() { ... }) so ValidateSecretKeyMatch(app.AppId,
app.AppSecret) and secret, err = ResolveSecretInput(app.AppSecret, kc) still
execute for profiles that have an AppSecret while preserving the existing
exitErr error handling and ConfigError wrapping.
---
Outside diff comments:
In `@cmd/config/init_interactive.go`:
- Around line 81-97: Replace the hardcoded prompt titles for appSecretInput and
userTokenGetterUrlInput with localized strings from the existing msg namespace
(e.g., use msg.AppSecret and msg.UserTokenGetterUrl or similar) so labels follow
the same localization flow as msg.Platform/msg.SelectAction; additionally add a
lightweight Validate on userTokenGetterUrlInput that trims whitespace and
returns nil for empty (since it's optional) but attempts to parse the URL (e.g.,
via net/url ParseRequestURI) and returns a user-facing error if invalid, so
problems are caught at input time rather than later in fetchTokenViaGetter.
In `@cmd/config/init.go`:
- Around line 288-307: The init command currently allows combining opts.New with
opts.UserTokenGetterUrl, which is semantically invalid because runCreateAppFlow
produces a real AppSecret and cmd/auth/login.go treats an empty AppSecret as the
token-getter branch; add validation in NewCmdConfigInit to detect the
combination (opts.New && opts.UserTokenGetterUrl != "") and return a user-facing
error instructing they cannot be used together, or alternatively clear
opts.UserTokenGetterUrl before saving in init.go (before calling saveInitConfig)
if you prefer to silently ignore it; update any usages/tests accordingly.
---
Nitpick comments:
In `@cmd/auth/get_user_token_url.demo_test.go`:
- Around line 26-45: In UserAuth, either acknowledge the unused ctx parameter
(add "_ = ctx") or remove it per your handler signature, and stop ignoring
url.Parse's error: capture the error from
url.Parse("https://open.feishu.cn/open-apis/authen/v1/index"), check if err !=
nil and handle it (e.g., log via your logger and return a 500 or call
c.String/c.Abort with an error) instead of discarding it; ensure you still build
query on authURLObj and call c.Redirect(http.StatusFound,
[]byte(authURLObj.String())) only when parsing succeeded.
In `@cmd/auth/login_test.go`:
- Around line 952-955: The test creates a logs slice and a logFn that appends
formatted messages to logs but never asserts on it; either assert the expected
log message (e.g., that "Waiting for authorization" or other expected string
appears in logs) after the code under test, or change logFn to a no-op
(func(string, ...interface{}) {}) like other tests; locate the variables logs
and logFn in the login tests (the local variables named logs and logFn in
login_test.go) and either add an assertion checking logs contains the expected
entry or replace logFn with the noop function used in the invalid-JSON/no-token
cases.
- Around line 940-950: Tests globally override package-level openBrowserFn
causing data races and cleanup leaks; change the design to avoid global mutation
by passing the opener through cmdutil.Factory or as an explicit parameter to
fetchTokenViaGetter so each test injects a local stub (reference symbols:
openBrowserFn, fetchTokenViaGetter, cmdutil.Factory,
TestFetchTokenViaGetter_MissingToken); if refactoring the API is not feasible
right now, at minimum replace all bare defers that restore openBrowserFn with
t.Cleanup calls in the listed tests (e.g., TestFetchTokenViaGetter_MissingToken
and the other ranges) and stop mutating the package-level state concurrently so
tests can safely run in parallel.
In `@cmd/auth/login.go`:
- Around line 347-361: The UserTokenData struct should use plain value types
(string/int) instead of pointer types to avoid pervasive nil dereferences;
change fields in UserTokenData (AccessToken, TokenType, ExpiresIn, Name, EnName,
AvatarUrl, AvatarThumb, AvatarMiddle, AvatarBig, OpenId, UnionId, UserId,
TenantKey) to non-pointer types with the same json `omitempty` tags and replace
Chinese comments with concise English descriptions; then update call sites in
this file that currently dereference pointers (see the checks and uses around
the login handling logic referenced near lines ~376–391) to use direct value
checks (e.g. AccessToken == "" or ExpiresIn == 0) and remove defensive nil
checks accordingly. Ensure any JSON unmarshalling behavior remains correct and
tests/usage that expect pointer semantics are updated to the new value
semantics.
- Around line 564-582: The default branch in openBrowser should not attempt
macOS/linux commands for obscure OSes; instead return a clear error so the
caller can fall back to its "please visit URL manually" message. Change the
default case in openBrowser to return fmt.Errorf("unsupported platform: %s",
runtime.GOOS) (or a similar descriptive error), remove the attempts to run
"open" and "xdg-open" there, and ensure fmt is imported; leave the
linux/darwin/windows cases unchanged so known OSes still invoke the proper
commands.
In `@cmd/config/init.go`:
- Line 316: Update the validation error messages to fix the grammar: change the
text returned where it currently reads "App ID cannot be empty, App Secret and
UserTokenGetterUrl cannot be both empty" to "App ID cannot be empty; App Secret
and UserTokenGetterUrl cannot both be empty" (and make the same adjustment for
the similar message around the UserTokenGetterUrl check at the other
occurrence). Locate the return strings in the init/config validation code (the
existing ErrValidation calls) and replace the message texts accordingly so they
use "cannot both be empty" and a clearer separator (e.g., semicolon).
In `@internal/core/config.go`:
- Around line 41-49: Rename the struct field UserTokenGetterUrl to
UserTokenGetterURL in internal/core/config.go (and analogous fields
CliConfig.UserTokenGetterUrl and credential.Account.UserTokenGetterUrl) and
update its JSON tag from "user_token_getter_url,omitempty" to
"userTokenGetterUrl,omitempty" so the Go identifier uses the standard initialism
capitalization and the persisted JSON key matches the existing camelCase
convention; update all references, serializers/deserializers, and any tests to
use the new field name and tag.
🪄 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: 9ed75da8-4fae-4feb-aca3-9c26799d8a4d
⛔ 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
Comment on lines
+19
to
+23
| var ( | ||
| appID = "app_id" | ||
| larkClient = lark.NewClient(appID, "app_secret") | ||
| redirectURI = "redirect_uri" | ||
| ) |
There was a problem hiding this comment.
Hardcoded placeholder credentials/URL at package scope.
appID = "app_id", lark.NewClient(appID, "app_secret"), and redirectURI = "redirect_uri" are initialized at package load (so they run for every go test ./cmd/auth/... execution) and will fail any real token exchange. If this file is truly a demo, these should be read from env/config at call time, not pinned as package vars. If it's meant to be wired into the CLI, the real values must come from CliConfig (AppID, and a redirect URL the local Hertz server knows its own port for) — not package-level constants.
🤖 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-level hardcoded values (appID, the lark.NewClient call using
"app_secret", and redirectURI) should be removed and replaced so they are
obtained at call time: read AppID, AppSecret and RedirectURI from
environment/config or from the CLI config object (e.g., CliConfig.AppID and
CliConfig.RedirectURL) inside the test/demo setup or the function that
constructs the Lark client rather than at package init; instantiate the Lark
client via lark.NewClient(appID, appSecret) only after fetching these values and
ensure the redirect URI is derived from the runtime test/server port or
CliConfig before exercising token exchange flows.
Comment on lines
+27
to
+30
| state := c.Query("state") | ||
| if state == "" { | ||
| state = "8888" | ||
| } |
There was a problem hiding this comment.
state is conflated with the local callback TCP port, weakening CSRF protection and enabling attacker-controlled port selection.
state in OAuth 2.0 is a CSRF-prevention nonce that should be opaque and verified against a per-request value. Here it is:
- Defaulted to
"8888"when absent (Line 29, 52), - Validated only as "parses as integer" (Line 60),
- Used directly as the destination TCP port in the localhost POST URL injected into the browser (Line 109).
Consequences:
- CSRF check degenerates to "is it a positive integer?" — any crafted request with a numeric
statepasses. - An attacker who can induce a victim to visit
/callback?code=…&state=<port>can steer the victim's browser to POST the access token to any localhost port the victim is running, not just the one the CLI is listening on. - Loss of binding between the original authorization request and the callback (normally achieved by signing/HMAC the state or storing it server-side).
Please pass the callback port via a separate, server-set query param (or embed it in the server-configured redirect_uri) and use state as a per-session random nonce that the callback verifies.
Also applies to: 50-53, 60-64, 108-110
🤖 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 27 - 30, The handler
currently conflates the OAuth CSRF nonce (c.Query("state")) with the client
callback TCP port and defaults it to "8888"; change this so state is a
cryptographically-random per-request nonce (do not default to "8888" or accept a
client-supplied numeric state), store it server-side (e.g., in a signed cookie,
session map, or in-memory map keyed by nonce), and verify the callback exactly
matches the stored nonce in the callback handler (replace the loose integer
parse check on state). Pass the callback port separately and server-controlled
(e.g., a server-side callbackPort variable or a server-set "port" query param
embedded in the redirect_uri) and stop using state as the destination port when
building the localhost POST URL; instead read the port from your
server-configured source. Ensure any fallback behavior is removed so an absent
or mismatched state rejects the callback.
| } | ||
|
|
||
| // OAuthCallback processes the OAuth callback from Lark, fetches the user access token, and sends it back to the local server. | ||
| func OAuthCallback(ctx context.Context, c *app.RequestContext) { //ignore_security_alert IDOR |
There was a problem hiding this comment.
Don't silence the IDOR scanner alert — fix the root cause.
//ignore_security_alert IDOR suppresses the warning rather than addressing it. The IDOR concern here is real: the handler accepts a user-controlled state and turns it into a destination on the caller's machine. See the separate comment on the state-as-port design; once that is fixed, this suppression should be removable.
🤖 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 48, The IDOR suppression on
OAuthCallback hides a real vulnerability: the handler reads a user-controlled
state and uses it to build a destination/port on the caller machine; remove the
suppression and fix by validating and restricting state before use. In
OAuthCallback, stop treating raw state as a destination—either map incoming
state to a server-side session entry or decode/verify a signed token that
contains an allowed destination, or validate against a whitelist of permitted
ports/hosts; reject any state that fails verification and return an error. After
implementing strict validation/mapping in OAuthCallback (and any helper that
parses state), remove the "//ignore_security_alert IDOR" comment.
| }, | ||
| body: JSON.stringify(data) | ||
| }).then(function(response) { | ||
| document.body.innerHTML += '<p>Token sended，close in 3 seconds...</p>'; |
There was a problem hiding this comment.
Typo in user-facing HTML: "Token sended".
Should read "Token sent". Also "Token send fail" (Line 123) → "Failed to send token".
✏️ Proposed fix
- document.body.innerHTML += '<p>Token sended，close in 3 seconds...</p>';
+ document.body.innerHTML += '<p>Token sent, closing in 3 seconds...</p>';
...
- document.body.innerHTML += '<p>Token send fail: ' + err + '</p>';
+ document.body.innerHTML += '<p>Failed to send token: ' + err + '</p>';(Also note the full-width comma ， on Line 118 — likely unintentional.)
🤖 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 118, Fix the user-facing
typos in get_user_token_url.demo_test.go by replacing the incorrect strings:
change "Token sended，close in 3 seconds..." (note the full-width comma and wrong
tense) to a proper message such as "Token sent, closing in 3 seconds..." and
update the error message "Token send fail" to "Failed to send token"; locate
these exact string literals in the demo/test HTML generation code and correct
punctuation and wording accordingly.
Comment on lines
+911
to
+972
| func TestFetchTokenViaGetter_Success(t *testing.T) { | ||
| // Setup a mock getter server | ||
| getterMux := http.NewServeMux() | ||
| getterMux.HandleFunc("/getter", func(w http.ResponseWriter, r *http.Request) { | ||
| state := r.URL.Query().Get("state") | ||
|
|
||
| // Send token back to the callback URL asynchronously | ||
| go func() { | ||
| // Small delay to ensure the local server is fully started | ||
| // (although it should be since fetchTokenViaGetter waits) | ||
| http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=mock_token_data", state)) | ||
| }() | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprint(w, "Mock Getter") | ||
| }) | ||
|
|
||
| getterServer := &http.Server{Addr: "127.0.0.1:0", Handler: getterMux} | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| t.Fatalf("Failed to start mock getter server: %v", err) | ||
| } | ||
| go getterServer.Serve(listener) | ||
| defer getterServer.Close() | ||
|
|
||
| getterURL := fmt.Sprintf("http://%s/getter", listener.Addr().String()) | ||
|
|
||
| // We need to temporarily disable openBrowser so it doesn't try to open a real browser | ||
| // Just for this test | ||
| originalOpenBrowser := openBrowserFn | ||
| t.Cleanup(func() { openBrowserFn = originalOpenBrowser }) | ||
| openBrowserFn = func(ctx context.Context, u string) error { | ||
| // Instead of opening browser, directly query the getter server | ||
| go func() { | ||
| // Small delay to ensure the mock HTTP server is ready | ||
| time.Sleep(10 * time.Millisecond) | ||
| http.Get(u) | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| var logs []string | ||
| logFn := func(format string, args ...interface{}) { | ||
| logs = append(logs, fmt.Sprintf(format, args...)) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| // Open a goroutine to act as the browser for this specific test | ||
| // We intercept the log messages to find out what port to hit | ||
|
|
||
| token, err := fetchTokenViaGetter(ctx, getterURL, "test_scope", logFn) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("fetchTokenViaGetter failed: %v", err) | ||
| } | ||
|
|
||
| if token != "mock_token_data" { | ||
| t.Errorf("Expected token %q, got %q", "mock_token_data", token) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential flaky test due to listener-vs-callback race.
fetchTokenViaGetter binds its local listener after returning from port selection, and your mocked openBrowserFn fires the callback from a goroutine with a 10 ms sleep before hitting the getter. Under a loaded CI machine, the goroutine that hits /user_access_token may race with the local HTTP server's Serve loop being ready to accept. The _SuccessWithAllFields variant at line 1126 already hedges with a 5-iteration retry loop — the bare _Success case here has none and can flake.
Consider mirroring the retry pattern (5×100 ms) used in _SuccessWithAllFields:
🧪 Suggested hardening
- go func() {
- // Small delay to ensure the local server is fully started
- // (although it should be since fetchTokenViaGetter waits)
- http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=mock_token_data", state))
- }()
+ go func() {
+ for i := 0; i < 5; i++ {
+ resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:%s/user_access_token?token=mock_token_data", state))
+ if err == nil {
+ resp.Body.Close()
+ return
+ }
+ time.Sleep(100 * time.Millisecond)
+ }
+ }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login_test.go` around lines 911 - 972, The test
TestFetchTokenViaGetter_Success can race because the mocked openBrowserFn
triggers the getter callback before the local listener is fully ready; mirror
the retry logic used in the _SuccessWithAllFields test: call fetchTokenViaGetter
up to 5 times with a 100ms sleep between attempts (break on success), and only
t.Fatalf after all retries fail, keeping openBrowserFn and logFn as-is;
reference fetchTokenViaGetter, openBrowserFn, and
TestFetchTokenViaGetter_Success when applying this change.
| } | ||
|
|
||
| err := saveAsProfile(existing, keychain.KeychainAccess(&noopConfigKeychain{}), "cli_prod", "app-new", core.PlainSecret("new-secret"), core.BrandLark, "en") | ||
| err := saveAsProfile(existing, keychain.KeychainAccess(&noopConfigKeychain{}), "cli_prod", "app-new", core.PlainSecret("new-secret"), core.BrandLark, "en", "") |
There was a problem hiding this comment.
Missing test coverage for the new userTokenGetterUrl parameter path.
Both call sites were updated to thread a trailing empty string, but no test exercises the behavior that makes the new parameter meaningful:
saveAsProfilenow persistsUserTokenGetterUrlintomulti.Apps[idx].UserTokenGetterUrl(seecmd/config/init.go:137). No assertion verifies that.updateExistingProfileWithoutSecretconditionally assignsapp.UserTokenGetterUrlonly when non-empty (seecmd/config/init.go:205-207). Neither the "update with non-empty URL" nor the "omitted URL preserves existing" behavior is tested.
As per coding guidelines: “Every behavior change must have an accompanying test.”
🧪 Suggested additional cases
func TestSaveAsProfile_PersistsUserTokenGetterUrl(t *testing.T) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
existing := &core.MultiAppConfig{}
if err := saveAsProfile(existing, keychain.KeychainAccess(&noopConfigKeychain{}),
"p1", "app-1", core.SecretInput{}, core.BrandFeishu, "en",
"https://example.com/token"); err != nil {
t.Fatalf("saveAsProfile error = %v", err)
}
got, err := core.LoadMultiAppConfig()
if err != nil || got.Apps[0].UserTokenGetterUrl != "https://example.com/token" {
t.Fatalf("UserTokenGetterUrl = %q, err = %v", got.Apps[0].UserTokenGetterUrl, err)
}
}Also applies to: 335-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/config_test.go` at line 311, Add tests exercising the new
userTokenGetterUrl path: call saveAsProfile with a non-empty trailing URL and
assert that core.LoadMultiAppConfig().Apps[idx].UserTokenGetterUrl equals that
URL (covers persistence in saveAsProfile and assignment to
multi.Apps[idx].UserTokenGetterUrl), and add cases for
updateExistingProfileWithoutSecret that (1) update with a non-empty URL and
assert the app.UserTokenGetterUrl is overwritten, and (2) omit the URL (pass
empty string) and assert the existing UserTokenGetterUrl is preserved; reference
saveAsProfile, updateExistingProfileWithoutSecret, UserTokenGetterUrl, and
multi.Apps in your assertions.
| cmd.Flags().StringVar(&opts.AppID, "app-id", "", "App ID (non-interactive)") | ||
| cmd.Flags().BoolVar(&opts.AppSecretStdin, "app-secret-stdin", false, "Read App Secret from stdin to avoid process list exposure") | ||
| cmd.Flags().StringVar(&opts.Brand, "brand", "feishu", "feishu or lark (non-interactive, default feishu)") | ||
| cmd.Flags().StringVar(&opts.UserTokenGetterUrl, "user-token-getter-url", "", "Custom url to fetch user token when app-secret is not provided") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing tests for the new --user-token-getter-url flag and its precedence rules.
The PR adds a new non-interactive flag, new validation (App Secret and UserTokenGetterUrl cannot both be empty), new propagation through saveAsProfile / updateExistingProfileWithoutSecret, and branch changes in configInitRun, but the PR description lists unit tests as not completed. Please add table-driven tests for at least: (a) --app-id + --user-token-getter-url (no secret), (b) both provided, (c) only --app-id with no secret and no getter URL, and (d) update flow clearing/overwriting UserTokenGetterUrl.
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/init.go` at line 65, Add table-driven unit tests covering the new
--user-token-getter-url flag and its precedence/validation: create tests that
call configInitRun (via the same CLI-helper used in other init tests) exercising
these cases: (a) --app-id + --user-token-getter-url (no secret) should succeed
and persist UserTokenGetterUrl in the saved profile, (b) both --app-id and
--app-secret and --user-token-getter-url provided should trigger validation
failure (App Secret and UserTokenGetterUrl cannot both be empty/contradicting
rules), (c) only --app-id with no secret and no getter URL should fail
validation, and (d) update flow that exercises saveAsProfile /
updateExistingProfileWithoutSecret to ensure an existing profile’s
UserTokenGetterUrl is cleared or overwritten correctly when updating; assert
profile fields persisted/cleared as expected. Include table rows for inputs,
expected exit code/error, and expected stored profile value for
UserTokenGetterUrl, and wire tests to exercise opts.UserTokenGetterUrl and the
configInitRun/saveAsProfile/updateExistingProfileWithoutSecret code paths.
Comment on lines
+205
to
+207
| if len(userTokenGetterUrl) > 0 { | ||
| app.UserTokenGetterUrl = userTokenGetterUrl | ||
| } |
There was a problem hiding this comment.
UserTokenGetterUrl cannot be cleared through this update path.
Guarding the assignment on len(userTokenGetterUrl) > 0 means a user who previously configured a token-getter URL and now wants to unset it (e.g., switch back to app-secret auth in the TUI “existing” flow) cannot do so — the old value silently persists. Other fields (AppId, Brand, Lang) are overwritten unconditionally and should behave the same way for consistency.
🛠️ Proposed fix
- if len(userTokenGetterUrl) > 0 {
- app.UserTokenGetterUrl = userTokenGetterUrl
- }
+ app.UserTokenGetterUrl = userTokenGetterUrl📝 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 len(userTokenGetterUrl) > 0 { | |
| app.UserTokenGetterUrl = userTokenGetterUrl | |
| } | |
| app.UserTokenGetterUrl = userTokenGetterUrl |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/init.go` around lines 205 - 207, The assignment to
app.UserTokenGetterUrl is currently guarded by "len(userTokenGetterUrl) > 0"
which prevents clearing an existing value; remove that guard and assign
app.UserTokenGetterUrl = userTokenGetterUrl unconditionally (like AppId, Brand,
Lang) so an empty string clears the stored URL; update any surrounding comment
or logic if necessary to preserve intent.
Comment on lines
+255
to
274
| 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()} | ||
| } |
There was a problem hiding this comment.
Bypassing secret resolution when UserTokenGetterUrl is set silently disables the bot/TAT flow.
The new condition skips ValidateSecretKeyMatch and ResolveSecretInput whenever UserTokenGetterUrl != "", regardless of whether AppSecret is also configured. Downstream, cfg.AppSecret is always "" for any profile with a getter URL, which means:
cmd/auth/login.gopicks the getter branch only whenconfig.AppSecret == "", so this condition and the login branch are effectively tautological — the user cannot configure both and have the CLI choose by flow.- Any bot/TAT path (e.g.,
--as botcommands) on a profile that happens to have a getter URL will silently run without an app secret and fail later with a less-obvious error.
Consider only skipping secret resolution when AppSecret.IsZero(), so that a profile can legitimately have both a real secret (bot usage) and a getter URL (user-token-only usage), and resolution errors still surface when a secret is actually configured:
🔒 Suggested condition
- var secret string
- var err error
- if app.UserTokenGetterUrl == "" {
- if err := ValidateSecretKeyMatch(app.AppId, app.AppSecret); err != nil {
+ var secret string
+ var err error
+ if !(app.UserTokenGetterUrl != "" && 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)
+ secret, err = ResolveSecretInput(app.AppSecret, kc)
if err != nil {🤖 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 - 274, The current block skips
ValidateSecretKeyMatch and ResolveSecretInput whenever UserTokenGetterUrl is
set, which disables secret resolution even if an AppSecret is configured; change
the guard so secret validation/resolution run only when an app secret is
actually present (i.e., when app.AppSecret.IsZero() is false). Specifically,
replace the condition using app.UserTokenGetterUrl with a check on app.AppSecret
(e.g., if !app.AppSecret.IsZero() { ... }) so ValidateSecretKeyMatch(app.AppId,
app.AppSecret) and secret, err = ResolveSecretInput(app.AppSecret, kc) still
execute for profiles that have an AppSecret while preserving the existing
exitErr error handling and ConfigError wrapping.
wangjingling
changed the title
wangjingling
changed the title
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
3a9bdfa to
1339d66
Compare
… app manager without app secret, then use lark-cli
wangjingling
force-pushed
the
feature/user_access_token_getter
branch
from
1339d66 to
a2a9e30
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 1-23: This demo file is incorrectly suffixed with _test.go so its
exported handlers (UserAuth, OAuthCallback) are not built or reusable; either
move the content to an examples/ directory and remove the _test.go suffix so it
becomes a true sample, or rename the file (e.g., get_user_token_url_demo.go) so
it is compiled into the binary; when making it non-test, also remove hardcoded
appID/appSecret and fix the state-as-port CSRF pattern described in prior
reviews (e.g., inject credentials via config/env and replace the state handling
in UserAuth/OAuthCallback with a secure CSRF/state mechanism).
- Line 34: Replace the hardcoded "https://open.feishu.cn/..." in the demo with
the brand-resolved authorization endpoint using core.ResolveEndpoints(brand);
call ResolveEndpoints with the same brand value used elsewhere in auth flows (as
in cmd/auth/login.go or cmd/config/init_interactive.go) and construct the auth
URL from the returned Endpoints.AuthOpenApi (or equivalent) instead of the
literal string, updating the authURLObj assignment to parse the resolved
endpoint.
In `@cmd/config/init_interactive.go`:
- Line 81: The prompt string in the Title(...) call exposes the Go field name
UserTokenGetterUrl, duplicates "App Secret", and includes an unclear URL
placeholder; update the Title text in cmd/config/init_interactive.go (the
Title("App Secret (App Secret or UserTokenGetterUrl, enter at least one)") and
the similar strings around lines 90–97) to a concise, user-facing phrase that
does not leak internal identifiers—e.g. "App Secret or User Token URL (enter at
least one)"—and reword the helper/instruction text to clearly state that the URL
should return or forward an authentication token to the local callback address
using a readable port placeholder (avoid `${state}` and internal names). Ensure
all similar Title/label occurrences in that block are changed consistently.
In `@internal/core/config.go`:
- Line 47: The JSON tag for the AppConfig field UserTokenGetterUrl is snake_case
and inconsistent with the existing camelCase tags; change the struct tag on
UserTokenGetterUrl in AppConfig from `user_token_getter_url` to
`userTokenGetterUrl` so it matches the other camelCase JSON keys and avoids a
breaking config key change.
🪄 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: 1c43dcc0-b6c2-4fe0-802d-aecdd08259be
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/auth/get_user_token_url.demo_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_interactive.gogo.modinternal/core/config.gointernal/credential/types.goskills/lark-shared/SKILL.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-shared/SKILL.md
- internal/credential/types.go
- cmd/auth/login_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- cmd/auth/login.go
- cmd/config/init.go
Comment on lines
+1
to
+23
| 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" | ||
| ) | ||
|
|
||
| var ( | ||
| appID = "app_id" | ||
| larkClient = lark.NewClient(appID, "app_secret") | ||
| redirectURI = "redirect_uri" | ||
| ) |
There was a problem hiding this comment.
_test.go filename means this "demo" is unreachable from production code.
Files matching *_test.go are only compiled by go test. The exported handlers UserAuth and OAuthCallback therefore cannot be referenced from any non-test file in this binary, and the file contains no func TestXxx(t *testing.T) either — so it neither tests anything nor serves as a callable demo. Two cleaner options:
- If this is purely illustrative documentation, move it to
examples/(or an_examplesdoc subtree) and drop the_test.gosuffix so it's clearly a sample, not test code. - If it is meant to be the actual server-side handler that backs
UserTokenGetterUrl, rename toget_user_token_url_demo.go(or similar) so it builds normally, and address the security concerns flagged in past reviews (state-as-port CSRF, hardcodedapp_id/app_secret).
As-is, the file ships in the repo, is read by reviewers and tools, but cannot be reused.
🤖 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 - 23, This demo file
is incorrectly suffixed with _test.go so its exported handlers (UserAuth,
OAuthCallback) are not built or reusable; either move the content to an
examples/ directory and remove the _test.go suffix so it becomes a true sample,
or rename the file (e.g., get_user_token_url_demo.go) so it is compiled into the
binary; when making it non-test, also remove hardcoded appID/appSecret and fix
the state-as-port CSRF pattern described in prior reviews (e.g., inject
credentials via config/env and replace the state handling in
UserAuth/OAuthCallback with a secure CSRF/state mechanism).
|
|
||
| scope := c.Query("scope") | ||
|
|
||
| authURLObj, _ := url.Parse("https://open.feishu.cn/open-apis/authen/v1/index") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the brand-specific host helper(s) used elsewhere for authen URLs.
rg -nP -C2 '\b(open\.feishu\.cn|open\.larksuite\.com|BrandLark|BrandFeishu)\b' --type=go -g '!**/*_test.go'Repository: larksuite/cli
Length of output: 11725
🏁 Script executed:
cat -n cmd/auth/get_user_token_url.demo_test.goRepository: larksuite/cli
Length of output: 4459
🏁 Script executed:
# Check what references this demo file
rg -l 'get_user_token_url' --type=go
# Look for other demo files with similar patterns
fd -e 'demo*' --type f | head -20Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# List files in cmd/auth/ directory
ls -la cmd/auth/
# Check if the demo test file has any build tags or special markers
head -30 cmd/auth/get_user_token_url.demo_test.go | cat -ARepository: larksuite/cli
Length of output: 2123
🏁 Script executed:
# Check how login.go handles brand in auth flows
rg -n -A5 -B5 'open.feishu.cn|open.larksuite.com|ResolveEndpoints' cmd/auth/login.go | head -100Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Look at login.go directly for auth-related patterns
head -100 cmd/auth/login.go
# Also check how init_interactive.go (which showed up in initial results) handles brand
rg -n -A3 -B3 'ResolveEndpoints|PollAppRegistration' cmd/config/init_interactive.goRepository: larksuite/cli
Length of output: 4205
🏁 Script executed:
# Search for any references to UserAuth or OAuthCallback
rg 'UserAuth|OAuthCallback' --type=go
# Check if there are any test functions in the demo_test.go file
grep -n '^func Test' cmd/auth/get_user_token_url.demo_test.goRepository: larksuite/cli
Length of output: 584
Hardcoded open.feishu.cn ignores the Lark brand.
The authorization endpoint is hardcoded to https://open.feishu.cn/..., which will be wrong for users on the Lark brand (endpoint should be open.larksuite.com). The codebase provides core.ResolveEndpoints(brand) for exactly this purpose (see internal/core/types.go), and other auth flows like cmd/auth/login.go and cmd/config/init_interactive.go already thread brand through their authorization logic. This demo should derive the endpoint from brand instead.
🤖 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 34, Replace the hardcoded
"https://open.feishu.cn/..." in the demo with the brand-resolved authorization
endpoint using core.ResolveEndpoints(brand); call ResolveEndpoints with the same
brand value used elsewhere in auth flows (as in cmd/auth/login.go or
cmd/config/init_interactive.go) and construct the auth URL from the returned
Endpoints.AuthOpenApi (or equivalent) instead of the literal string, updating
the authURLObj assignment to parse the resolved endpoint.
|
|
||
| appSecretInput := huh.NewInput(). | ||
| Title("App Secret"). | ||
| Title("App Secret (App Secret or UserTokenGetterUrl, enter at least one)"). |
There was a problem hiding this comment.
Prompt copy leaks the Go identifier name and reads awkwardly.
User-facing strings should not expose UserTokenGetterUrl (Go field name) as a noun. Also, "App Secret (App Secret or UserTokenGetterUrl, enter at least one)" duplicates "App Secret", and "the url must get token and send token data to http://127.0.0.1:${state}" is hard to parse for end users (and the ${state} placeholder reflects the demo's questionable use of state as a TCP port — see review on cmd/auth/get_user_token_url.demo_test.go).
Suggested rewording
- appSecretInput := huh.NewInput().
- Title("App Secret (App Secret or UserTokenGetterUrl, enter at least one)").
+ appSecretInput := huh.NewInput().
+ Title("App Secret (required unless a User Token Getter URL is provided)").
...
- userTokenGetterUrlInput := huh.NewInput().
- Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})").
+ userTokenGetterUrlInput := huh.NewInput().
+ Title("User Token Getter URL (optional; endpoint that completes user authorization and POSTs the token to the local CLI callback)").Also applies to: 90-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/init_interactive.go` at line 81, The prompt string in the
Title(...) call exposes the Go field name UserTokenGetterUrl, duplicates "App
Secret", and includes an unclear URL placeholder; update the Title text in
cmd/config/init_interactive.go (the Title("App Secret (App Secret or
UserTokenGetterUrl, enter at least one)") and the similar strings around lines
90–97) to a concise, user-facing phrase that does not leak internal
identifiers—e.g. "App Secret or User Token URL (enter at least one)"—and reword
the helper/instruction text to clearly state that the URL should return or
forward an authentication token to the local callback address using a readable
port placeholder (avoid `${state}` and internal names). Ensure all similar
Title/label occurrences in that block are changed consistently.
| Brand LarkBrand `json:"brand"` | ||
| Lang string `json:"lang,omitempty"` | ||
| DefaultAs Identity `json:"defaultAs,omitempty"` // AsUser | AsBot | AsAuto | ||
| UserTokenGetterUrl string `json:"user_token_getter_url,omitempty"` // UserTokenGetterUrl defines an endpoint for fetching user tokens without manual authorization flow. |
There was a problem hiding this comment.
Inconsistent JSON field casing.
The new user_token_getter_url tag uses snake_case while all other fields in AppConfig (e.g., appId, appSecret, defaultAs, strictMode) use camelCase. Once any user writes a config file with this key, switching it later becomes a breaking change. Consider aligning to userTokenGetterUrl for consistency before this ships.
Proposed change
- UserTokenGetterUrl string `json:"user_token_getter_url,omitempty"` // UserTokenGetterUrl defines an endpoint for fetching user tokens without manual authorization flow.
+ UserTokenGetterUrl string `json:"userTokenGetterUrl,omitempty"` // UserTokenGetterUrl defines an endpoint for fetching user tokens without manual authorization flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config.go` at line 47, The JSON tag for the AppConfig field
UserTokenGetterUrl is snake_case and inconsistent with the existing camelCase
tags; change the struct tag on UserTokenGetterUrl in AppConfig from
`user_token_getter_url` to `userTokenGetterUrl` so it matches the other
camelCase JSON keys and avoids a breaking config key change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
564-582: Consider using an established browser-opening library instead of hand-rolled implementation.The
openBrowserfunction re-implements logic that well-maintained packages likegithub.com/pkg/browserhandle more robustly—proper handling of WSL, headless environments,BROWSERenv var, sandboxed environments, and avoiding spawning detached child processes without lifecycle tracking.Specific issues to address:
- On Windows (line 573),
rundll32 url.dll,FileProtocolHandlercan mangle URLs containing&or%;cmd /c start ""is more reliable.- The default case (lines 574–579) tries
openbeforexdg-open, butopenis macOS-specific and will fail on FreeBSD/OpenBSD where onlyxdg-openis available.Confirmed:
github.com/pkg/browserand similar packages are not yet in go.mod, so adopting one would add a new dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 564 - 582, Replace the hand-rolled openBrowser implementation with a call to a well-maintained library (e.g., github.com/pkg/browser) or, if you prefer minimal changes, fix platform-specific commands in openBrowser: on Windows use "cmd /c start \"\" <url>" instead of "rundll32 url.dll,FileProtocolHandler", ensure the default branch tries "xdg-open" before macOS "open" (or detect darwin explicitly via runtime.GOOS == \"darwin\"), and handle BROWSER env/WSL/headless cases by delegating to the chosen library which properly addresses those edge cases; update imports and go.mod to add the dependency if you opt for the library.
🤖 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 501-504: Move the Content-Type header set so it takes effect: call
w.Header().Set("Content-Type", "text/html; charset=utf-8") before
w.WriteHeader(http.StatusOK) (or simply remove the explicit w.WriteHeader call
and rely on fmt.Fprint to implicitly set status 200), ensuring the sequence
around w.Header().Set, w.WriteHeader, and fmt.Fprint in the success branch
matches the error branch order.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 564-582: Replace the hand-rolled openBrowser implementation with a
call to a well-maintained library (e.g., github.com/pkg/browser) or, if you
prefer minimal changes, fix platform-specific commands in openBrowser: on
Windows use "cmd /c start \"\" <url>" instead of "rundll32
url.dll,FileProtocolHandler", ensure the default branch tries "xdg-open" before
macOS "open" (or detect darwin explicitly via runtime.GOOS == \"darwin\"), and
handle BROWSER env/WSL/headless cases by delegating to the chosen library which
properly addresses those edge cases; update imports and go.mod to add the
dependency if you opt for the library.
🪄 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: 3b3d3eb5-e88e-435e-af70-4ea8bf9ab340
⛔ 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 as they are similar to previous changes (7)
- skills/lark-shared/SKILL.md
- internal/credential/types.go
- cmd/config/config_test.go
- go.mod
- cmd/config/init_interactive.go
- cmd/auth/login_test.go
- cmd/config/init.go
Comment on lines
+501
to
+504
| 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.
Header set after WriteHeader has no effect.
w.Header().Set("Content-Type", ...) on line 502 runs after w.WriteHeader(http.StatusOK) on line 501, so the header is dropped — only headers set before WriteHeader are sent on the wire. The browser will rely on DetectContentType instead. This is also inconsistent with the error branch at lines 491–493, which correctly orders Set → WriteHeader → Fprint. (Note: fmt.Fprint/Write will also implicitly call WriteHeader(200) if not yet called, so explicitly calling it here isn't strictly needed.)
🛠️ Proposed fix
- 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">
+ 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>`)📝 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
| 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>`) | |
| 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 501 - 504, Move the Content-Type header set
so it takes effect: call w.Header().Set("Content-Type", "text/html;
charset=utf-8") before w.WriteHeader(http.StatusOK) (or simply remove the
explicit w.WriteHeader call and rely on fmt.Fprint to implicitly set status
200), ensuring the sequence around w.Header().Set, w.WriteHeader, and fmt.Fprint
in the success branch matches the error branch order.
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
1 participant
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
Documentation
Tests