feat: add user_token_getter_url, people can get user token from app manager#695
Open
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add user_token_getter_url, people can get user token from app manager#695wangjingling wants to merge 1 commit intolarksuite:mainfrom
wangjingling wants to merge 1 commit intolarksuite:mainfrom
Conversation
…anager without app secret, then use lark-cli
📝 WalkthroughWalkthroughThis PR introduces an alternative OAuth authentication flow through Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Browser as Browser
participant LocalServer as Local Server<br/>(Loopback)
participant LarkOAuth as Lark OAuth<br/>Server
participant CLIApp as CLI App
User->>CLIApp: Initiate login with UserTokenGetterUrl
CLIApp->>LocalServer: Start callback listener on port (via state)
CLIApp->>Browser: Open UserTokenGetterUrl with state=port&scope=...
Browser->>LarkOAuth: Navigate to Lark auth endpoint
LarkOAuth->>Browser: User grants permission
Browser->>LarkOAuth: Redirect with authorization code
LarkOAuth->>User: Approval complete
User->>Browser: User navigates to callback URL or auto-redirect occurs
Browser->>LocalServer: POST callback payload (code, state, data)
LocalServer->>LocalServer: Parse response JSON
LocalServer->>LarkOAuth: Exchange code for access_token<br/>(Authen.AccessToken.Create)
LarkOAuth-->>LocalServer: Return access token & user data
LocalServer->>Browser: Return HTML page with success message
Browser->>Browser: Auto-close after 3 seconds
LocalServer->>CLIApp: Token callback received
CLIApp->>CLIApp: Store token & profile in config
CLIApp-->>User: Login complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
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: 4
🧹 Nitpick comments (1)
cmd/config/init_interactive.go (1)
90-97: Consider clarifying the help text for users.The title "the url must get token and send token data to http://127.0.0.1:${state}" describes internal implementation details. Users might not understand what
${state}means or how the callback mechanism works.Consider a more user-friendly description like: "URL that provides OAuth callback integration for token retrieval".
🤖 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, Update the help/title text for the interactive input created via huh.NewInput() assigned to userTokenGetterUrlInput so it doesn't expose internal callback details; replace the current Title("UserTokenGetterUrl (Optional, the url must get token and send token data to http://127.0.0.1:${state})") with a clearer, user-friendly description (for example: "UserTokenGetter URL (optional) — URL that provides OAuth callback integration for token retrieval"), keeping the existing Placeholder logic that uses firstApp.UserTokenGetterUrl when available.
🤖 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.go.demo`:
- Around line 19-24: The authHost constant is malformed
("open.feishu.cn/open.larksuite.com") causing UserAuth to build an invalid
redirect URL; update the authHost constant to a single valid domain (for example
"open.feishu.cn" or "open.larksuite.com" depending on target region) and ensure
the UserAuth URL construction uses only the authHost constant (refer to the
authHost, appID, redirectURI constants and the UserAuth URL builder) so the
resulting OAuth endpoint is a correct host + path like
"https://<authHost>/open-apis/authen/v1/index".
In `@cmd/auth/login.go`:
- Around line 364-366: The authLoginViaGetter path currently passes finalScope
into fetchTokenViaGetter but then writes an empty Scope, skips
ensureRequestedScopesGranted, and omits the scope summary; update
authLoginViaGetter (and the similar block around lines 421-441) to store the
granted scopes returned by fetchTokenViaGetter into the msg.Scope (or equivalent
field), call ensureRequestedScopesGranted(finalScope, grantedScopes, ...) to
validate the getter didn't narrow permissions, and include the resulting scope
summary in the success reporting/logging so behavior matches the device flow.
- Around line 459-463: The /user_access_token handler currently trusts the first
request based only on the exposed state (port); fix this by generating a
cryptographically-random per-login nonce when starting the local server, include
that nonce as a required query param in the getter URL, and validate that the
incoming callback's nonce matches before sending to tokenCh (the handler for
"/user_access_token" should reject mismatched or missing nonces and not write to
tokenCh). Stop using Access-Control-Allow-Origin: * unconditionally — only set a
restrictive CORS origin (or remove the header) until the callback is
authenticated and the nonce verified. Apply the same nonce verification and CORS
tightening to the other local handlers mentioned in the comment (the other
mux.HandleFunc callbacks around the same flow) so the getter cannot race and
inject tokens.
- Around line 470-480: The handler currently accepts tokens from the URL query
when r.Method != http.MethodPost which leaks credentials; change the logic in
the login handler (the block that sets tokenData based on r.Method) to reject
any non-POST request instead of reading r.URL.Query().Get("token") — return an
HTTP 405 (or 400) with a clear error like "token must be POSTed as JSON" and
only parse tokenData from the POST body (using io.ReadAll or JSON decoding as
appropriate); update any references to tokenData and ensure early return on
non-POST to prevent further processing.
---
Nitpick comments:
In `@cmd/config/init_interactive.go`:
- Around line 90-97: Update the help/title text for the interactive input
created via huh.NewInput() assigned to userTokenGetterUrlInput so it doesn't
expose internal callback details; replace the current Title("UserTokenGetterUrl
(Optional, the url must get token and send token data to
http://127.0.0.1:${state})") with a clearer, user-friendly description (for
example: "UserTokenGetter URL (optional) — URL that provides OAuth callback
integration for token retrieval"), keeping the existing Placeholder logic that
uses firstApp.UserTokenGetterUrl when available.
🪄 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: 320e14ed-217d-45db-bf86-6dcc158b50e2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/auth/get_user_token_url.go.democmd/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
+24
| const ( | ||
| appID = "" | ||
| appSecret = "" | ||
| redirectURI | ||
| authHost = "open.feishu.cn/open.larksuite.com" | ||
| ) |
There was a problem hiding this comment.
Fix the demo authorization host.
authHost currently concatenates the Feishu and Larksuite domains, so UserAuth builds https://open.feishu.cn/open.larksuite.com/open-apis/authen/v1/index instead of a real OAuth endpoint. Anyone copying this demo will hit a broken redirect immediately.
Suggested fix
const (
appID = ""
appSecret = ""
redirectURI
- authHost = "open.feishu.cn/open.larksuite.com"
+ authHost = "open.feishu.cn" // or "open.larksuite.com", selected from the target brand
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/get_user_token_url.go.demo` around lines 19 - 24, The authHost
constant is malformed ("open.feishu.cn/open.larksuite.com") causing UserAuth to
build an invalid redirect URL; update the authHost constant to a single valid
domain (for example "open.feishu.cn" or "open.larksuite.com" depending on target
region) and ensure the UserAuth URL construction uses only the authHost constant
(refer to the authHost, appID, redirectURI constants and the UserAuth URL
builder) so the resulting OAuth endpoint is a correct host + path like
"https://<authHost>/open-apis/authen/v1/index".
Comment on lines
+364
to
+366
| 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) |
There was a problem hiding this comment.
Keep scope handling consistent with the device flow.
This path forwards finalScope to the getter, but it stores Scope: "", skips ensureRequestedScopesGranted, and reports success without any scope summary. That means auth login --scope/--domain/--recommend can succeed even if the getter ignored or narrowed the requested permissions, which is a behavior change from the existing device flow.
Also applies to: 421-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 364 - 366, The authLoginViaGetter path
currently passes finalScope into fetchTokenViaGetter but then writes an empty
Scope, skips ensureRequestedScopesGranted, and omits the scope summary; update
authLoginViaGetter (and the similar block around lines 421-441) to store the
granted scopes returned by fetchTokenViaGetter into the msg.Scope (or equivalent
field), call ensureRequestedScopesGranted(finalScope, grantedScopes, ...) to
validate the getter didn't narrow permissions, and include the resulting scope
summary in the success reporting/logging so behavior matches the device flow.
Comment on lines
+459
to
+463
| 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") |
There was a problem hiding this comment.
Authenticate the loopback callback before accepting a token.
The localhost handler trusts the first request that reaches /user_access_token, and the only value shared with the getter is the listening port (state). Because that port is exposed in the getter URL and never verified on callback, any page that learns it can race the real callback and inject arbitrary token JSON into the CLI. Please add a per-login nonce and require the same nonce on the callback before writing to tokenCh; the permissive Access-Control-Allow-Origin: * should also go away once the callback is authenticated.
Also applies to: 498-500, 525-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 459 - 463, The /user_access_token handler
currently trusts the first request based only on the exposed state (port); fix
this by generating a cryptographically-random per-login nonce when starting the
local server, include that nonce as a required query param in the getter URL,
and validate that the incoming callback's nonce matches before sending to
tokenCh (the handler for "/user_access_token" should reject mismatched or
missing nonces and not write to tokenCh). Stop using
Access-Control-Allow-Origin: * unconditionally — only set a restrictive CORS
origin (or remove the header) until the callback is authenticated and the nonce
verified. Apply the same nonce verification and CORS tightening to the other
local handlers mentioned in the comment (the other mux.HandleFunc callbacks
around the same flow) so the getter cannot race and inject tokens.
Comment on lines
+470
to
+480
| 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") | ||
| } |
There was a problem hiding this comment.
Reject token delivery through the query string.
The GET ?token= fallback leaks access tokens into browser history, proxy logs, and crash reports. The bundled demo already uses a JSON POST, so non-POST requests should be rejected instead of reading r.URL.Query().Get("token").
Suggested fix
- 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 r.Method != http.MethodPost {
+ http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
+ return
+ }
+ body, err := io.ReadAll(r.Body)
+ if err != nil {
+ http.Error(w, "failed to read body", http.StatusBadRequest)
+ return
+ }
+ tokenData := string(body)📝 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 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 r.Method != http.MethodPost { | |
| http.Error(w, "method not allowed", http.StatusMethodNotAllowed) | |
| return | |
| } | |
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| http.Error(w, "failed to read body", http.StatusBadRequest) | |
| return | |
| } | |
| tokenData := string(body) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/login.go` around lines 470 - 480, The handler currently accepts
tokens from the URL query when r.Method != http.MethodPost which leaks
credentials; change the logic in the login handler (the block that sets
tokenData based on r.Method) to reject any non-POST request instead of reading
r.URL.Query().Get("token") — return an HTTP 405 (or 400) with a clear error like
"token must be POSTed as JSON" and only parse tokenData from the POST body
(using io.ReadAll or JSON decoding as appropriate); update any references to
tokenData and ensure early return on non-POST to prevent further processing.
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
Release Notes
New Features
config initcommand now accepts--user-token-getter-urlflag for flexible authentication options.Documentation