Skip to content

feat: add user_token_getter_url, people can get user token from app manager #655

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: add user_token_getter_url, people can get user token from app manager #655

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
134 changes: 134 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,134 @@
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"
)

const (
appID = ""
appSecret = ""
redirectURI
authHost = "open.feishu.cn/open.larksuite.com"
)

var (
larkClient = lark.NewClient(appID, appSecret)
)
Comment on lines +26 to +28
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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 +1 to +28
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

_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:

  1. If this is purely illustrative documentation, move it to examples/ (or an _examples doc subtree) and drop the _test.go suffix so it's clearly a sample, not test code.
  2. If it is meant to be the actual server-side handler that backs UserTokenGetterUrl, rename to get_user_token_url_demo.go (or similar) so it builds normally, and address the security concerns flagged in past reviews (state-as-port CSRF, hardcoded app_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).


// 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 == "" {
c.JSON(http.StatusBadRequest, utils.H{"error": "missing state"})
return
}
Comment on lines +32 to +36
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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:

  1. CSRF check degenerates to "is it a positive integer?" — any crafted request with a numeric state passes.
  2. 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.
  3. 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.


scope := c.Query("scope")

authURLObj, _ := url.Parse(fmt.Sprintf("https://%s/open-apis/authen/v1/index", authHost))
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) {
code := c.Query("code")
state := c.Query("state")

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
}

// 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>';
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

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))
}
Loading
Loading