Skip to content

feat: app administrator provides an auth page, allowing others to obtain the user_access_token without requiring the app_secret #634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wangjingling wants to merge 1 commit into from
Closed

feat: app administrator provides an auth page, allowing others to obtain the user_access_token without requiring the app_secret #634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions cmd/auth/get_user_token_url.demo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
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"
)
Comment on lines +1 to +17
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm there are no Test/Benchmark/Example funcs in this file, and that login.go does not reference these handlers.
fd -t f 'get_user_token_url.demo_test.go' --exec rg -nP '^func (Test|Benchmark|Example)\w+\(' {}
echo "---"
rg -nP --type=go -C2 '\b(UserAuth|OAuthCallback)\b'

Repository: larksuite/cli

Length of output: 760

🏁 Script executed:

# First, read the full demo file to confirm its structure and purpose
cat -n cmd/auth/get_user_token_url.demo_test.go

Repository: larksuite/cli

Length of output: 4294

🏁 Script executed:

# Check if login.go references the UserAuth or OAuthCallback handlers
rg -n --type=go 'UserAuth|OAuthCallback' cmd/auth/login.go

Repository: larksuite/cli

Length of output: 39

🏁 Script executed:

# Search for any tests related to --user-token-getter-url or user_token_getter_url
rg -n --type=go 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' tests/ 2>/dev/null || echo "No tests found with that pattern"

Repository: larksuite/cli

Length of output: 89

🏁 Script executed:

# Check if there are actual test files for the login.go feature
fd -t f 'login.*_test\.go' cmd/auth/

Repository: larksuite/cli

Length of output: 206

🏁 Script executed:

# Check if this file is referenced in any documentation, tests, or configuration
rg -n 'get_user_token_url\.demo_test|user_token_getter_url' --type=go --type=md

Repository: larksuite/cli

Length of output: 248

🏁 Script executed:

# Check for any build tags or conditional compilation that might reference this
grep -r 'get_user_token_url.demo_test' .

Repository: larksuite/cli

Length of output: 194

🏁 Script executed:

# Check if there are any tests that actually exercise the --user-token-getter-url flag
rg -n 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' cmd/auth/login_test.go

Repository: larksuite/cli

Length of output: 39

🏁 Script executed:

# Check if the login.go file defines the --user-token-getter-url flag and how it uses the config
rg -n -A5 'user.token.getter.url|user_token_getter_url|UserTokenGetterUrl' cmd/auth/login.go

Repository: larksuite/cli

Length of output: 518

🏁 Script executed:

# Check what fetchTokenViaGetter does and whether it expects HTTP endpoints like UserAuth/OAuthCallback
rg -n -A10 'func fetchTokenViaGetter' cmd/auth/

Repository: larksuite/cli

Length of output: 674

🏁 Script executed:

# Verify if there are any E2E tests for the user-token-getter-url feature
fd -t f 'test.go$' tests/cli_e2e/ | xargs rg -l 'user.token.getter.url|UserTokenGetterUrl' 2>/dev/null || echo "No E2E tests found"

Repository: larksuite/cli

Length of output: 75

🏁 Script executed:

# Let me see the full fetchTokenViaGetter function to understand how it uses the getter URL
rg -n -A30 'func fetchTokenViaGetter' cmd/auth/login.go

Repository: larksuite/cli

Length of output: 1148

🏁 Script executed:

# Continue reading the fetchTokenViaGetter function to see how it uses getterURL
rg -n -A60 'func fetchTokenViaGetter' cmd/auth/login.go | head -80

Repository: larksuite/cli

Length of output: 2168

🏁 Script executed:

# Get the rest of the fetchTokenViaGetter function to see how it constructs and uses the getter URL
rg -n -A100 'func fetchTokenViaGetter' cmd/auth/login.go | tail -50

Repository: larksuite/cli

Length of output: 1614

🏁 Script executed:

# Check if there are any tests for the user-token-getter-url feature at all
rg -n 'UserTokenGetterUrl|user.token.getter.url' tests/ --type=go 2>/dev/null || echo "No tests found"

Repository: larksuite/cli

Length of output: 71

🏁 Script executed:

# Check login_test.go to see what scenarios are tested
rg -n 'func Test' cmd/auth/login_test.go | head -20

Repository: larksuite/cli

Length of output: 1250

🏁 Script executed:

# Double-check that the handlers in this demo file are completely unused by searching all production code
rg -n 'UserAuth|OAuthCallback' cmd/ --type=go --glob='!*_test.go' 2>/dev/null || echo "No matches in production code"

Repository: larksuite/cli

Length of output: 86

Rename or relocate this file — it is a demo, not a test.

The file is named *_test.go but contains no Test*, Benchmark*, or Example* functions, and the header explicitly identifies it as "a demo for user_token_getter_url." Go's test runner will include it in the test binary but never execute anything from it. The exported UserAuth and OAuthCallback handlers are reference implementations showing how a custom getter URL should implement the OAuth flow (as consumed by fetchTokenViaGetter in login.go), not test code.

Move this to an examples/ directory (e.g., examples/user_token_getter/main.go) as a standalone main package with proper documentation, or rename it without the _test.go suffix and add a //go:build demo tag to prevent it from bloating the default binary.

Additionally, per the coding guidelines requiring tests for every behavior change, the --user-token-getter-url feature in login.go lacks any unit or integration tests. The demo file cannot serve as a substitute — add tests that exercise the feature's actual code paths.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/auth/get_user_token_url.demo_test.go` around lines 1 - 17, This demo file
is misnamed as a test and should be moved or gated: remove the `_test.go` suffix
and either relocate the contents into an examples package (e.g.,
examples/user_token_getter/main.go under package main) or add a build tag like
`//go:build demo` so it isn't included in normal test binaries; ensure exported
handlers UserAuth and OAuthCallback remain intact for demonstration. Also add
real tests covering the --user-token-getter-url behavior exercised by
fetchTokenViaGetter in login.go (unit tests that mock the getter URL and
integration tests for the OAuth flow) so the feature has coverage rather than
relying on the demo file.


var (
appID = "app_id"
larkClient = lark.NewClient(appID, "app_secret")
redirectURI = "redirect_uri"
)

// UserAuth handles the initial step of the OAuth flow by redirecting the user to the Lark authorization page.
func UserAuth(ctx context.Context, c *app.RequestContext) {
state := c.Query("state")
if state == "" {
state = "8888"
}

scope := c.Query("scope")

authURLObj, _ := url.Parse("https://open.feishu.cn/open-apis/authen/v1/index")
query := authURLObj.Query()
query.Set("redirect_uri", redirectURI)
query.Set("app_id", appID)
query.Set("state", state)
if scope != "" {
query.Set("scope", scope)
}
authURLObj.RawQuery = query.Encode()

c.Redirect(http.StatusFound, []byte(authURLObj.String()))
}

// 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
code := c.Query("code")
state := c.Query("state")
if state == "" {
state = "8888"
}

if code == "" {
c.JSON(http.StatusBadRequest, utils.H{"error": "missing code"})
return
}

stateInt, err := strconv.Atoi(state)
if err != nil {
c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be integer"})
return
}
Comment on lines +48 to +64
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate state as a valid TCP port before using it as %d in the callback URL.

state is accepted from the query string and parsed with strconv.Atoi, then formatted into http://127.0.0.1:%d/user_access_token. Any integer — including negative numbers, 0, or values > 65535 — passes the current check. Clamp to a valid ephemeral/user port range (e.g., 1024 <= stateInt <= 65535) so a malformed/malicious state can't produce a bogus callback URL.

Proposed fix 
 	stateInt, err := strconv.Atoi(state)
-	if err != nil {
-		c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be integer"})
-		return
-	}
+	if err != nil || stateInt < 1 || stateInt > 65535 {
+		c.JSON(http.StatusBadRequest, utils.H{"error": "invalid state parameter, must be a TCP port in [1,65535]"})
+		return
+	}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/auth/get_user_token_url.demo_test.go` around lines 46 - 62, The
OAuthCallback handler currently accepts any integer state and uses stateInt when
formatting the callback URL; restrict stateInt to a valid TCP port range (e.g.,
1024 <= stateInt <= 65535) before using it. In OAuthCallback, after
strconv.Atoi(state) and err check, add a validation that returns
http.StatusBadRequest with an appropriate error JSON if stateInt is <1024 or
>65535 (or otherwise out of your chosen port bounds), so only a legitimate port
value is used when formatting "http://127.0.0.1:%d/user_access_token".


// get user_access_token
req := larkauthen.NewCreateAccessTokenReqBuilder().
Body(larkauthen.NewCreateAccessTokenReqBodyBuilder().
GrantType("authorization_code").
Code(code).
Build()).
Build()

resp, err := larkClient.Authen.AccessToken.Create(ctx, req)
if err != nil {
c.JSON(http.StatusInternalServerError, utils.H{"error": "failed to get access token", "detail": err.Error()})
return
}

if !resp.Success() {
c.JSON(http.StatusInternalServerError, utils.H{"error": "feishu API returned error", "code": resp.Code, "msg": resp.Msg})
return
}

data := resp.Data
if data == nil || data.AccessToken == nil {
c.JSON(http.StatusInternalServerError, utils.H{"error": "empty data in response"})
return
}

// TODO check user permission or scope

dataBytes, err := json.Marshal(data)
if err != nil {
c.JSON(http.StatusInternalServerError, utils.H{"error": "failed to marshal data"})
return
}

html := fmt.Sprintf(`<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>Lark Token</title>
</head>
<body>
<p>Sending token...</p>
<script>
window.onload = function() {
var url = 'http://127.0.0.1:%d/user_access_token';
var data = %s;
fetch(url, {
method: 'POST',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify(data)
}).then(function(response) {
document.body.innerHTML += '<p>Token sended，close in 3 seconds...</p>';
setTimeout(function() {
window.close();
}, 3000);
}).catch(function(err) {
document.body.innerHTML += '<p>Token send fail: ' + err + '</p>';
});
};
</script>
</body>
</html>`, stateInt, string(dataBytes))

c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(html))
Comment on lines +99 to +130
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Interpolating JSON into an inline <script> block risks </script> breakout; also fix the user-facing typo.

dataBytes is json.Marshaled struct output, which does not escape < or /, so any string field that happens to contain </script> would terminate the script tag and enable HTML injection into the demo page. Two low-effort fixes:

  1. Use json.Encoder with SetEscapeHTML(true) (default) and additionally replace <, >, & for JS-in-HTML safety, or simpler: emit the payload from a separate /token-data endpoint and fetch() it instead of string-interpolating into the HTML.
  2. Minor: Token sended → Token sent.
Minimal safer interpolation 
-    var data = %s;
+    var data = JSON.parse(%q);
...
-`, stateInt, string(dataBytes))
+`, stateInt, string(dataBytes))

(passing via %q so the JSON becomes a quoted JS string literal, then JSON.parse-ing it, which removes the </script> breakout vector.)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/auth/get_user_token_url.demo_test.go` around lines 97 - 128, The HTML
demo currently injects raw JSON (dataBytes) into the inline <script> via
fmt.Sprintf in the html variable (see html := fmt.Sprintf(..., stateInt,
string(dataBytes))) which allows a </script> breakout and XSS; change this by
not interpolating raw JSON into the script — either serve the JSON from a
separate handler (e.g., a /token-data endpoint fetched by window.onload before
POSTing to /user_access_token) or ensure safe embedding by passing the JSON as a
quoted JS string (use %q) and JSON.parse on the client or use json.Encoder with
SetEscapeHTML(true) and replace problematic chars, and also fix the user-facing
message "Token sended" to "Token sent" (references: dataBytes,
fmt.Sprintf(html...), window.onload, /user_access_token, c.Data).

}
Loading
Loading