Feat/session token account#155
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds session_token as an alternative credential end-to-end: request types and admin handler accept session_token and batch per-account seeds; uTLS HTTP/2 transport with proxy support is added and used for session refreshes with browser-like headers; frontend UI, form handling, and i18n support session-token input. ChangesSession Token Authentication Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for adding accounts via Session Token and improves session-token refresh robustness against Cloudflare-protected endpoints by introducing a uTLS + HTTP/2 client.
Changes:
- Frontend: add a “Session Token” add method/tab, form fields, and i18n strings.
- Backend: accept
session_tokenin add-account API (including batch rules) and store credentials via a credentials map. - Auth: introduce a uTLS-based HTTP client pool and use it for session-token refresh requests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types.ts | Extends add-account request typing to include optional session_token and makes refresh_token optional. |
| frontend/src/pages/Accounts.tsx | Adds UI flow for session-token based account creation and adjusts payload validation/submission. |
| frontend/src/locales/en.json | Adds English labels/placeholders for Session Token add method. |
| frontend/src/locales/zh.json | Adds Chinese labels/placeholders for Session Token add method. |
| auth/utls_client.go | New uTLS + HTTP/2 round-tripper, proxy dialers, and pooled client with TTL eviction. |
| auth/token.go | Uses uTLS client for session-token refresh and adds browser-like request headers. |
| admin/handler.go | Accepts session_token, supports batch parsing/mapping, and inserts accounts with credential maps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+166
to
+169
| case "http", "https": | ||
| return buildAuthHTTPProxyDialer(u) | ||
| case "socks5", "socks5h": | ||
| return buildAuthSOCKS5Dialer(u) |
Comment on lines
+30
to
+44
| func newUTLSAuthTransport(proxyURL string) http.RoundTripper { | ||
| var dialer xproxy.Dialer = xproxy.Direct | ||
| if proxyURL != "" { | ||
| d, err := buildAuthProxyDialer(proxyURL) | ||
| if err != nil { | ||
| dialer = xproxy.Direct | ||
| } else { | ||
| dialer = d | ||
| } | ||
| } | ||
| return &utlsAuthRoundTripper{ | ||
| connections: make(map[string]*http2.ClientConn), | ||
| pending: make(map[string]*sync.Cond), | ||
| dialer: dialer, | ||
| } |
| } | ||
|
|
||
| func (d *authHTTPConnectDialer) Dial(network, addr string) (net.Conn, error) { | ||
| conn, err := net.DialTimeout("tcp", d.proxyAddr, 10*time.Second) |
Comment on lines
+274
to
+289
| var utlsAuthClientPoolStop = make(chan struct{}) | ||
|
|
||
| func init() { | ||
| go func() { | ||
| ticker := time.NewTicker(utlsAuthClientPoolCleanupInterval) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| evictExpiredUTLSAuthClients() | ||
| case <-utlsAuthClientPoolStop: | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/utls_client.go`:
- Around line 55-66: The waiter loop can deadlock on an old cond when another
goroutine replaces t.pending[host]; to fix it, capture the current cond (e.g.
cur := cond) before calling cur.Wait() and after being signaled check that
t.pending[host] still equals that same cur (pointer equality); if it does not,
stop waiting on the old cond (break/return to re-evaluate) so you never call
Wait() again on a stale condition variable. Ensure these checks reference
t.pending, the local cond variable, and the same mutex t.mu surrounding
Wait/inspection, and keep the existing checks for t.connections[host] and
CanTakeNewRequest().
- Around line 30-45: In newUTLSAuthTransport, do not silently ignore errors from
buildAuthProxyDialer; when buildAuthProxyDialer(proxyURL) returns an error, log
the error and the proxyURL before falling back to xproxy.Direct so configuration
issues are visible. Update the error branch in newUTLSAuthTransport to call the
package logger (or standard log) with a clear message including the error and
proxyURL, then set dialer = xproxy.Direct; keep the rest of the function and
utlsAuthRoundTripper initialization unchanged.
🪄 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 Plus
Run ID: b06c22f2-3de7-4d15-87a1-5cbeb5cbfa80
📒 Files selected for processing (7)
admin/handler.goauth/token.goauth/utls_client.gofrontend/src/locales/en.jsonfrontend/src/locales/zh.jsonfrontend/src/pages/Accounts.tsxfrontend/src/types.ts
Comment on lines
+30
to
+45
| func newUTLSAuthTransport(proxyURL string) http.RoundTripper { | ||
| var dialer xproxy.Dialer = xproxy.Direct | ||
| if proxyURL != "" { | ||
| d, err := buildAuthProxyDialer(proxyURL) | ||
| if err != nil { | ||
| dialer = xproxy.Direct | ||
| } else { | ||
| dialer = d | ||
| } | ||
| } | ||
| return &utlsAuthRoundTripper{ | ||
| connections: make(map[string]*http2.ClientConn), | ||
| pending: make(map[string]*sync.Cond), | ||
| dialer: dialer, | ||
| } | ||
| } |
There was a problem hiding this comment.
Proxy dialer errors are silently swallowed, masking configuration issues.
When buildAuthProxyDialer fails (e.g., malformed proxy URL), the code falls back to direct connection without any logging or error indication. Users won't know their proxy configuration is being ignored.
Proposed fix: Log the error before falling back
func newUTLSAuthTransport(proxyURL string) http.RoundTripper {
var dialer xproxy.Dialer = xproxy.Direct
if proxyURL != "" {
d, err := buildAuthProxyDialer(proxyURL)
if err != nil {
+ log.Printf("[utls] failed to build proxy dialer for %q, falling back to direct: %v", proxyURL, err)
dialer = xproxy.Direct
} else {
dialer = d
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/utls_client.go` around lines 30 - 45, In newUTLSAuthTransport, do not
silently ignore errors from buildAuthProxyDialer; when
buildAuthProxyDialer(proxyURL) returns an error, log the error and the proxyURL
before falling back to xproxy.Direct so configuration issues are visible. Update
the error branch in newUTLSAuthTransport to call the package logger (or standard
log) with a clear message including the error and proxyURL, then set dialer =
xproxy.Direct; keep the rest of the function and utlsAuthRoundTripper
initialization unchanged.
Comment on lines
+55
to
+66
| if cond, ok := t.pending[host]; ok { | ||
| for { | ||
| cond.Wait() | ||
| if h2Conn, ok := t.connections[host]; ok && h2Conn.CanTakeNewRequest() { | ||
| t.mu.Unlock() | ||
| return h2Conn, nil | ||
| } | ||
| if _, still := t.pending[host]; !still { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: waiters can deadlock on stale condition variable.
When multiple goroutines wait for a connection and the creator fails:
- Thread A creates
cond, adds topending[host], starts creating connection - Threads B and C see
pending[host], both capturecondand callWait() - A fails, broadcasts, deletes
pending[host] - B wakes first, sees no pending, breaks loop at line 64, creates a new cond and stores it in
pending[host] - C wakes, checks
pending[host]at line 62 — sees B's new cond exists (still = true) - C loops back to
Wait()on A's old cond, which will never signal again → deadlock
The fix is to detect when the condition variable has been replaced:
Proposed fix
if cond, ok := t.pending[host]; ok {
for {
cond.Wait()
if h2Conn, ok := t.connections[host]; ok && h2Conn.CanTakeNewRequest() {
t.mu.Unlock()
return h2Conn, nil
}
- if _, still := t.pending[host]; !still {
+ newCond, still := t.pending[host]
+ if !still {
break
}
+ if newCond != cond {
+ cond = newCond // switch to the replacement condition variable
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/utls_client.go` around lines 55 - 66, The waiter loop can deadlock on an
old cond when another goroutine replaces t.pending[host]; to fix it, capture the
current cond (e.g. cur := cond) before calling cur.Wait() and after being
signaled check that t.pending[host] still equals that same cur (pointer
equality); if it does not, stop waiting on the old cond (break/return to
re-evaluate) so you never call Wait() again on a stale condition variable.
Ensure these checks reference t.pending, the local cond variable, and the same
mutex t.mu surrounding Wait/inspection, and keep the existing checks for
t.connections[host] and CanTakeNewRequest().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/Accounts.tsx (1)
749-757:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the RT + session-token fallback path reachable from the UI.
This change makes
refresh_tokenandsession_tokenmutually exclusive:handleAddblanks the other field before POSTing, and the RT tab no longer exposes the optional session-token input.admin/handler.go:1100-1163still supports paired RT/ST batches and a shared ST fallback, so that flow is now unreachable from the frontend.Also applies to: 3193-3263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/Accounts.tsx` around lines 749 - 757, handleAdd currently forces refresh_token and session_token to be mutually exclusive by blanking the other field and the RT UI no longer exposes the optional session_token, which prevents sending paired RT+ST batches that the backend (admin/handler.go handlers) still supports; fix by changing payload construction in handleAdd (function name handleAdd, type AddAccountRequest, and source object addForm) to include both refresh_token and session_token from addForm without overwriting one with an empty string, keep the existing validation that at least one token is non-empty, and restore or ensure the RT tab UI still exposes an optional session_token input so the frontend can submit RT+ST together when provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/pages/Accounts.tsx`:
- Around line 749-757: handleAdd currently forces refresh_token and
session_token to be mutually exclusive by blanking the other field and the RT UI
no longer exposes the optional session_token, which prevents sending paired
RT+ST batches that the backend (admin/handler.go handlers) still supports; fix
by changing payload construction in handleAdd (function name handleAdd, type
AddAccountRequest, and source object addForm) to include both refresh_token and
session_token from addForm without overwriting one with an empty string, keep
the existing validation that at least one token is non-empty, and restore or
ensure the RT tab UI still exposes an optional session_token input so the
frontend can submit RT+ST together when provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46f4774c-e360-4bc1-99a0-2b44fe7fd19c
📒 Files selected for processing (1)
frontend/src/pages/Accounts.tsx
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
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.
简单修复session token需要过盾，以及前端不支持session token导入的问题
不过ip不干净仍然会导入失败
Summary by CodeRabbit
New Features
Bug Fixes
Localization