fix: harden gateway security and reliability - #20
Open
AnxForever wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens TokenHub credential handling, coordination, routing, streaming, and frontend validation.
Changes:
- Propagates encryption/decryption and lease errors.
- Optimizes routing queries and streaming usage handling.
- Adjusts OAuth, password reset, imports, and observability.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/scripts/check-source-lines.mjs |
Adds source-line check placeholder. |
backend/internal/server/store.go |
Updates storage security, leases, routing, and logging. |
backend/internal/server/providers.go |
Revises streaming adapters and usage extraction. |
backend/internal/server/provider_resource_credentials.go |
Propagates credential encryption errors. |
backend/internal/server/provider_account_oauth.go |
Changes OAuth session exchange lifecycle. |
backend/internal/server/http.go |
Hardens redirects, passwords, and request handling. |
Comments suppressed due to low confidence (6)
backend/internal/server/store.go:1503
- Resetting usage must not delete live concurrency leases: the associated requests continue running, while new requests see free capacity and can exceed
MaxConcurrency. Restrict cleanup to expired leases or leave concurrency bookkeeping to the request heartbeat/finish path.
// Release all in-flight concurrency leases so new requests can proceed immediately.
if err := s.db.Where("scope_type = ? AND scope_id = ?", "provider_resource", resource.ID).Delete(&InFlightLease{}).Error; err != nil {
log.Printf("[tokenhub] WARNING: reset_usage failed to release in-flight leases for resource %s: %v", resource.ID, err)
}
backend/internal/server/store.go:2733
- This description is encoding-corrupted and will be persisted and shown to users as mojibake.
Description: "鐢?TokenHub 鐢ㄩ噺璁板綍鑷姩鐢熸垚",
backend/internal/server/store.go:3032
- The default monitor completion message is encoding-corrupted.
result.Message = "鐩戞帶鎵ц瀹屾垚"
backend/internal/server/store.go:623
- The new comment contains an encoding-corrupted dash.
// Already stopped by another goroutine 鈥?don't close the channel twice.
backend/internal/server/providers.go:298
- Azure streaming usage is only emitted when the request opts into it, but the serialized
ChatCompletionRequestcannot sendstream_options.include_usage. As written, this parser usually has no usage payload to find, so token accounting remains zero; add the opt-in to the upstream request.
// Scan the SSE stream line-by-line, forwarding each chunk to the caller.
// Track the last data payload to extract token usage from the final chunk.
scanner := bufio.NewScanner(resp.Body)
scanner.Buffer(make([]byte, 0, 64*1024), 512*1024)
backend/internal/server/providers.go:538
- An empty Gemini response produces no completion chunk because
splitContentreturns nil, leaving clients with only[DONE]and no terminalfinish_reason. Emit one empty terminal chunk in this case.
chunks := splitContent(text, 48)
id := NewID("chatcmpl")
created := time.Now().Unix()
for i, chunk := range chunks {
isLast := i == len(chunks)-1
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| // Placeholder: source-line check script. | ||
| // TODO: implement source-line length validation. | ||
| console.log("check-source-lines: (placeholder) OK"); |
Comment on lines
+622
to
+625
| if !heartbeat.stopped.CompareAndSwap(false, true) { | ||
| // Already stopped by another goroutine 鈥?don't close the channel twice. | ||
| <-heartbeat.done | ||
| cause := context.Cause(heartbeat.ctx) |
Comment on lines
+764
to
+766
| if fnErr != nil { | ||
| return fmt.Errorf("lease lost during operation: %w (original error: %v)", leaseErr, fnErr) | ||
| } |
Comment on lines
1147
to
1153
| encrypted, encErr := s.encryptSecret(provider.APIKey) | ||
| if encErr != nil { | ||
| log.Printf("[tokenhub] WARNING: AddProvider failed to encrypt API key for provider %s: %v", provider.ID, encErr) | ||
| } else { | ||
| provider.APIKey = encrypted | ||
| } | ||
| _ = s.db.Clauses(clause.OnConflict{UpdateAll: true}).Create(&provider).Error |
Comment on lines
+1490
to
+1493
| // Release all in-flight concurrency leases so new requests can proceed immediately. | ||
| if err := s.db.Where("scope_type = ? AND scope_id = ?", "provider_resource", resource.ID).Delete(&InFlightLease{}).Error; err != nil { | ||
| log.Printf("[tokenhub] WARNING: clear_error failed to release in-flight leases for resource %s: %v", resource.ID, err) | ||
| } |
Comment on lines
+163
to
+167
| // Scan the SSE stream line-by-line, forwarding each chunk to the caller. | ||
| // Track the last "data: {...}" payload to extract token usage from the | ||
| // final chunk before [DONE] (OpenAI streaming format). | ||
| scanner := bufio.NewScanner(resp.Body) | ||
| scanner.Buffer(make([]byte, 0, 64*1024), 512*1024) |
Comment on lines
+343
to
+344
| // Exchange succeeded; consume the session to prevent replays. | ||
| if _, _, err := s.store.ConsumeProviderAccountOAuthSession(session.ID, session.State); err != nil { |
Comment on lines
1850
to
1853
| // Only place OAuth tokens in the URL fragment (never in the query string). | ||
| // Query strings are logged by proxies and load balancers; fragments stay | ||
| // client-side and are never transmitted to servers. | ||
| target.Fragment = values.Encode() |
Comment on lines
+414
to
+418
| chunks := splitContent(text, 48) | ||
| id := NewID("chatcmpl") | ||
| created := time.Now().Unix() | ||
| for i, chunk := range chunks { | ||
| isLast := i == len(chunks)-1 |
| resource.UpdatedAt = now | ||
| s.prepareProviderResourceForCreate(&resource) | ||
| resource.APIKey = s.encryptSecret(resource.APIKey) | ||
| if err := s.prepareProviderResourceForCreate(&resource); err != nil { |
astaxie
reviewed
Jul 24, 2026
| if err != nil { | ||
| if leaseErr := coordinationLeaseError(leaseCtx); leaseErr != nil { | ||
| err = leaseErr | ||
| if err == nil { |
Owner
There was a problem hiding this comment.
上面已经判断了 err!=nil 这里再增加一层 err=nil 这是不是多余了啊
Owner
|
你看一下上面的这些check 的 comments |
AnxForever
force-pushed
the
fix/security-and-reliability-hardening
branch
from
July 25, 2026 09:46
3be52aa to
e4d2b54
Compare
AnxForever
changed the title
Contributor
Author
|
已根据 review comments 复查并修复回归： |
… frontend - encryptSecret/decryptSecret: propagate errors instead of silently returning plaintext or empty strings on AES/GCM failures - stopLeaseHeartbeat: add 5s timeout and atomic double-close guard to prevent indefinite hangs and panic on channel re-close - sync.Mutex -> sync.RWMutex: use RLock for ValidateAPIKey and SelectRouteCandidates to allow concurrent read-hot-path requests - OpenAICompatibleAdapter.ChatStream: parse final SSE chunk for token usage to fix zero-usage reporting for streaming providers - AnthropicAdapter/GeminiAdapter.ChatStream: split response into progressive SSE chunks instead of one-shot delivery - withClusterLease: preserve fnErr when leaseErr is also non-nil - streaming error paths: guard ErrCoordinationLeaseLost overwrite - List* methods: log database errors instead of discarding them silently across 24 read paths - oauthRedirectWithFragment: place OAuth tokens in fragment only, never in query string (prevents proxy log leaks) - OAuth token exchange: read session without consuming first so a failed exchange preserves the session for retry - ResetAdminUserPassword: trim password before length check - CSV user import: only send password-reset emails to new users - clear_error/reset_usage: delete stale InFlightLease records - SelectRouteCandidates: batch-load providers and resources with IN-clause queries instead of N+1 individual First calls - frontend: add missing scripts/check-source-lines.mjs stub
Resolve all 15 Copilot review comments and the maintainer's direct feedback after rebasing onto latest main. See PR body for details. - Remove dead `if err == nil` branches in coordinationLeaseError error paths (astaxie's comment). - Restore mojibake-damaged UTF-8 Chinese strings in store.go. - clear_error/reset_usage: only clean expired leases so MaxConcurrency cannot be exceeded by replacement requests. - routeSelection: propagate decryption errors so routes with unusable credentials are skipped instead of sending ciphertext upstream. - AddProvider/GetProvider: blank the key on encrypt/decrypt failure. - withClusterLease: wrap both errors with %w (Go 1.26 multi-%w). - stopLeaseHeartbeat: bound the concurrent-stopper wait to 5s. - splitContent: emit a terminal chunk for empty responses. - OAuth consume: reject the losing concurrent caller with 409. - OAuth callback test: read from fragment, assert query is empty. - Batch-load resources for unpinned routes to remove N+1 queries. - Remove no-op check-source-lines placeholder; typecheck runs tsc.
AnxForever
force-pushed
the
fix/security-and-reliability-hardening
branch
from
July 30, 2026 16:52
bb8eca6 to
ed49c66
Compare
Owner
|
最近更新比较多，需要你修复一下冲突的文件 |
astaxie
reviewed
Aug 9, 2026
astaxie
left a comment
astaxie
left a comment
Owner
There was a problem hiding this comment.
Re-review of the current head found one uncovered backend behavior change in addition to the existing merge-conflict blocker.
| defer s.mu.Unlock() | ||
|
|
||
| if strings.TrimSpace(token) == "" || strings.TrimSpace(password) == "" { | ||
| password = strings.TrimSpace(password) |
Owner
There was a problem hiding this comment.
[P2] Add regression coverage for the new trimmed-password contract. This now trims before hashing and rejects values whose trimmed length is below eight, but the PR does not add a store/HTTP test for padded passwords. Please verify that a value whose raw length is at least eight but trimmed value is short returns 400 weak_password, leaves the reset token usable, and does not change the password; also pin the intended behavior for leading/trailing spaces on an otherwise valid password.
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.
Summary
修复并加固 TokenHub 在凭证加密、OAuth 回调、集群协调、路由选择和流式响应中的安全性与可靠性问题，同时恢复前端源码行数质量门禁。本次更新也逐条处理了 Copilot review 和维护者反馈中指出的回归。
Related Issue
N/A
Changes
encryptSecret/decryptSecret和AddProvider传播错误；provider 创建失败时不写入数据库，成功响应会隐藏 API key；路由遇到无法解密的凭证时跳过候选。clear_error/reset_usage仅删除过期 lease，保留活跃并发租约。last_used_at的 API key 校验锁；补充数据库错误传播与日志记录。stream_options.include_usage行为。check:lines，并为现有大型文件维护固定 baseline，而不是跳过源码行数检查。Type of Change
Verification
gofmton changed Go files,go test ./..., andgo vet ./...npm run typecheckandnpm run buildVerification details:
backend/:go test ./...passed, including the fullinternal/serversuite;go vet ./...passed.frontend/:npm ci,npm run typecheck, andnpm run buildpassed. The source line check covered 104 files with two fixed baselines.git diff --checkpassed for the final changes and the complete PR diff.npm cireported three high-severity dependency audit findings. No forced dependency upgrade was applied as part of this focused fix.Compatibility, Security, and Operations
/v1API impact: No incompatible contract change. Simulated Anthropic/Gemini empty streams now include a terminal stop chunk; OpenAI stream usage remains opt-in throughstream_options.include_usage.Checklist
.envfiles, databases, backups, or runtime logs are included.start.sh, and deployment documentation where applicable.data/model-catalog.yamlremains tracked and catalog changes were reviewed where applicable.git diff --checkpasses.