安全修复：加固初始化、OAuth 绑定鉴权、自动更新语支付回调校验 - #4504
Closed
ll539 wants to merge 4 commits into
Closed
Conversation
Contributor
WalkthroughThe PR adds stricter validation and logging across payment webhook handlers for multiple providers (Topup, Creem, Stripe, Waffo), the user account setup endpoint, and user profile update operations. It also requires authentication middleware on OAuth binding routes and logs warning messages for rejected auth attempts and setup requests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/user.go (1)
661-679:⚠️ Potential issue | 🟡 MinorReject non-string
sidebar_modules/languagepayloads instead of returning success.Both special-case branches only update on
string, but they currently fall through toMsgUpdateSuccesswhen the caller sends an object/array/number. That turns malformed input into a silent no-op and weakens the strict validation this PR adds elsewhere.🛠️ Proposed fix
// 更新sidebar_modules字段 - if sidebarModulesStr, ok := sidebarModules.(string); ok { - currentSetting.SidebarModules = sidebarModulesStr - } + sidebarModulesStr, ok := sidebarModules.(string) + if !ok { + common.ApiErrorI18n(c, i18n.MsgInvalidParams) + return + } + currentSetting.SidebarModules = sidebarModulesStr @@ // 更新language字段 - if langStr, ok := language.(string); ok { - currentSetting.Language = langStr - } + langStr, ok := language.(string) + if !ok { + common.ApiErrorI18n(c, i18n.MsgInvalidParams) + return + } + currentSetting.Language = langStrAlso applies to: 693-710
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/user.go` around lines 661 - 679, The code currently ignores non-string payloads for "sidebar_modules" (and similarly for "language") and returns success; change the handlers so that when requestData["sidebar_modules"] (and requestData["language"] in the other branch) exists but is not a string you call common.ApiErrorI18n(c, i18n.MsgInvalidInput) and return instead of proceeding to success; locate the logic around requestData, user.GetSetting(), and currentSetting.SidebarModules in controller/user.go (and the parallel branch at the other block) and add an explicit type-check rejection path so only string values are accepted before assigning and saving and before emitting MsgUpdateSuccess.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/topup_stripe.go`:
- Around line 283-314: The webhook wrongly hardcodes "USD" when validating
currency; update the flow to store the expected currency on the order when
creating the Stripe checkout session (the code that uses setting.StripePriceId)
and then use that stored value in the callback instead of "USD". Specifically:
add/persist an expected currency field on the TopUp record when creating the
Stripe session (lookup the Price/currency or read it from the created session in
the checkout creation code), populate topUp.Currency (or similar) there, and
change the callback logic that currently calls event.GetObjectValue("currency")
and compares to "USD" to compare against topUp.Currency (case-normalized) and
log appropriately via logPaymentReject / logger.LogWarn when mismatched. Ensure
GetTopUpByTradeNo, topUp.PaymentProvider, topUp.Status, and the amount checks
remain unchanged.
In `@controller/topup.go`:
- Around line 373-403: The handler currently writes "success" before performing
the critical post-payment operations, so move the
c.Writer.Write([]byte("success")) call to after the locked update/quota-credit
block (i.e., only after topUp.Update() and IncreaseUserQuota() succeed); if
either topUp.Update() or IncreaseUserQuota() fails, log the error (use
logger.LogError or existing logPaymentReject), write "fail" and return so the
gateway can retry. Locate code paths using GetTopUpByTradeNo,
normalizeMoneyDecimalFromFloat/FromString, logPaymentReject, and the subsequent
topUp.Update()/IncreaseUserQuota() calls and ensure acknowledgement is
conditional on their success. Ensure all early reject paths still write "fail"
immediately.
In `@controller/wechat.go`:
- Line 108: The warning log currently prints the raw external account identifier
wechatId in the logger.LogWarn call; update the code that calls logger.LogWarn
(the site using wechatId and c.ClientIP()) to avoid logging raw PII by deriving
and logging a non-reversible fingerprint (e.g., hash like SHA256 and truncate to
a short fixed length) or a truncated masked form of wechatId instead, and keep
c.ClientIP() as-is; ensure the new value replaces wechatId in the fmt.Sprintf
invocation so only the hashed/truncated/masked identifier is written to logs.
In `@router/api-router.go`:
- Around line 39-44: The bind routes now allow Authorization header auth via
middleware.UserAuth(), but the handlers EmailBind, WeChatBind, and TelegramBind
still read sessions.Default(c).Get("id").(int) and will panic on
access-token-only requests; update those handlers to safely read the
authenticated user id from the request context (use c.GetInt("id") or equivalent
context value set by UserAuth()) instead of casting session values, or
alternatively remove middleware.UserAuth() from these routes and enforce a
session-only guard so sessions.Default(c).Get("id") is always present; modify
either the middleware usage in router/api-router.go or the handlers (EmailBind,
WeChatBind, TelegramBind) accordingly to ensure no nil session lookup occurs.
---
Outside diff comments:
In `@controller/user.go`:
- Around line 661-679: The code currently ignores non-string payloads for
"sidebar_modules" (and similarly for "language") and returns success; change the
handlers so that when requestData["sidebar_modules"] (and
requestData["language"] in the other branch) exists but is not a string you call
common.ApiErrorI18n(c, i18n.MsgInvalidInput) and return instead of proceeding to
success; locate the logic around requestData, user.GetSetting(), and
currentSetting.SidebarModules in controller/user.go (and the parallel branch at
the other block) and add an explicit type-check rejection path so only string
values are accepted before assigning and saving and before emitting
MsgUpdateSuccess.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76846162-f566-4052-bb2c-a70cce55d0db
📒 Files selected for processing (8)
controller/setup.gocontroller/topup.gocontroller/topup_creem.gocontroller/topup_stripe.gocontroller/topup_waffo.gocontroller/user.gocontroller/wechat.gorouter/api-router.go
Comment on lines
+354
to
+377
| var products []CreemProduct | ||
| if err := common.UnmarshalJsonStr(setting.CreemProducts, &products); err == nil { | ||
| var matchedProducts []CreemProduct | ||
| for _, product := range products { | ||
| if product.Quota == topUp.Amount && decimal.NewFromFloat(product.Price).Round(2).Equal(expectedAmount) { | ||
| matchedProducts = append(matchedProducts, product) | ||
| } | ||
| } | ||
| if len(matchedProducts) == 1 { | ||
| expectedProduct := matchedProducts[0] | ||
| if expectedProduct.ProductId != "" && event.Object.Product.Id != expectedProduct.ProductId { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("Creem callback rejected provider=%s trade_no=%s reason=product_mismatch expected_product=%s actual_product=%s client_ip=%s", model.PaymentProviderCreem, referenceId, expectedProduct.ProductId, event.Object.Product.Id, c.ClientIP())) | ||
| c.AbortWithStatus(http.StatusBadRequest) | ||
| return | ||
| } | ||
| expectedCurrency := strings.ToUpper(expectedProduct.Currency) | ||
| actualCurrency := strings.ToUpper(event.Object.Order.Currency) | ||
| if expectedCurrency != "" && actualCurrency != expectedCurrency { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("Creem callback rejected provider=%s trade_no=%s reason=currency_mismatch expected_currency=%s actual_currency=%s client_ip=%s", model.PaymentProviderCreem, referenceId, expectedCurrency, actualCurrency, c.ClientIP())) | ||
| c.AbortWithStatus(http.StatusBadRequest) | ||
| return | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do not silently skip product/currency validation when the config is ambiguous or unreadable.
If CreemProducts fails to parse, or multiple configured products share the same (quota, price) pair, this branch falls through and accepts the callback on amount alone. That makes the new product/currency check best-effort instead of authoritative. Persist the selected product_id/currency with the local order and compare against those exact values here, or reject when the config cannot identify a unique match.
Comment on lines
+283
to
+314
| topUp := model.GetTopUpByTradeNo(referenceId) | ||
| if topUp == nil { | ||
| logger.LogWarn(ctx, fmt.Sprintf("Stripe callback rejected provider=%s trade_no=%s reason=order_not_found client_ip=%s", model.PaymentProviderStripe, referenceId, callerIp)) | ||
| return | ||
| } | ||
| if topUp.PaymentProvider != model.PaymentProviderStripe { | ||
| logger.LogWarn(ctx, fmt.Sprintf("Stripe callback rejected provider=%s trade_no=%s reason=provider_mismatch actual_provider=%s client_ip=%s", model.PaymentProviderStripe, referenceId, topUp.PaymentProvider, callerIp)) | ||
| return | ||
| } | ||
| if topUp.Status == common.TopUpStatusSuccess { | ||
| logger.LogInfo(ctx, fmt.Sprintf("Stripe callback idempotent success trade_no=%s client_ip=%s", referenceId, callerIp)) | ||
| return | ||
| } | ||
| if topUp.Status != common.TopUpStatusPending { | ||
| logger.LogWarn(ctx, fmt.Sprintf("Stripe callback rejected provider=%s trade_no=%s reason=invalid_status status=%s client_ip=%s", model.PaymentProviderStripe, referenceId, topUp.Status, callerIp)) | ||
| return | ||
| } | ||
| actualMinor, parseErr := strconv.ParseInt(event.GetObjectValue("amount_total"), 10, 64) | ||
| if parseErr != nil { | ||
| logger.LogWarn(ctx, fmt.Sprintf("Stripe callback rejected provider=%s trade_no=%s reason=invalid_amount_total client_ip=%s", model.PaymentProviderStripe, referenceId, callerIp)) | ||
| return | ||
| } | ||
| expectedAmount := decimal.NewFromFloat(topUp.Money).Round(2) | ||
| actualAmount := decimal.NewFromInt(actualMinor).Div(decimal.NewFromInt(100)).Round(2) | ||
| if !expectedAmount.Equal(actualAmount) { | ||
| logPaymentReject(ctx, model.PaymentProviderStripe, referenceId, expectedAmount, actualAmount, "amount_mismatch", callerIp) | ||
| return | ||
| } | ||
| if strings.ToUpper(event.GetObjectValue("currency")) != "USD" { | ||
| logger.LogWarn(ctx, fmt.Sprintf("Stripe callback rejected provider=%s trade_no=%s reason=currency_mismatch expected_currency=%s actual_currency=%s client_ip=%s", model.PaymentProviderStripe, referenceId, "USD", strings.ToUpper(event.GetObjectValue("currency")), callerIp)) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
rg -n --type=go 'StripePriceId|StripeUnitPrice|Stripe.*Currency|PaymentProviderStripe|currency|USD' controller setting modelRepository: QuantumNous/new-api
Length of output: 12683
Store or retrieve the expected Stripe currency per order instead of hardcoding USD.
The webhook at line 311 rejects any non-USD currency, but setting.StripePriceId (used in checkout creation at line 396) is configurable and can point to a Stripe Price in any currency. If a non-USD Price is configured, valid payments will be rejected. The codebase currently has no mechanism to persist or derive the expected currency; consider storing it with the order when the Stripe session is created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/topup_stripe.go` around lines 283 - 314, The webhook wrongly
hardcodes "USD" when validating currency; update the flow to store the expected
currency on the order when creating the Stripe checkout session (the code that
uses setting.StripePriceId) and then use that stored value in the callback
instead of "USD". Specifically: add/persist an expected currency field on the
TopUp record when creating the Stripe session (lookup the Price/currency or read
it from the created session in the checkout creation code), populate
topUp.Currency (or similar) there, and change the callback logic that currently
calls event.GetObjectValue("currency") and compares to "USD" to compare against
topUp.Currency (case-normalized) and log appropriately via logPaymentReject /
logger.LogWarn when mismatched. Ensure GetTopUpByTradeNo, topUp.PaymentProvider,
topUp.Status, and the amount checks remain unchanged.
Comment on lines
+373
to
403
| if verifyInfo.TradeStatus == epay.StatusTradeSuccess { | ||
| topUp := model.GetTopUpByTradeNo(verifyInfo.ServiceTradeNo) | ||
| if topUp == nil { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("Epay callback rejected provider=%s trade_no=%s reason=order_not_found client_ip=%s", model.PaymentProviderEpay, verifyInfo.ServiceTradeNo, c.ClientIP())) | ||
| _, _ = c.Writer.Write([]byte("fail")) | ||
| return | ||
| } | ||
| if topUp.PaymentProvider != model.PaymentProviderEpay { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("Epay callback rejected provider=%s trade_no=%s reason=provider_mismatch actual_provider=%s client_ip=%s", model.PaymentProviderEpay, verifyInfo.ServiceTradeNo, topUp.PaymentProvider, c.ClientIP())) | ||
| _, _ = c.Writer.Write([]byte("fail")) | ||
| return | ||
| } | ||
| expectedAmount := normalizeMoneyDecimalFromFloat(topUp.Money) | ||
| actualAmount, amountErr := normalizeMoneyDecimalFromString(verifyInfo.Money) | ||
| if amountErr != nil { | ||
| logPaymentReject(c.Request.Context(), model.PaymentProviderEpay, verifyInfo.ServiceTradeNo, expectedAmount, decimal.Zero, "invalid_callback_amount", c.ClientIP()) | ||
| _, _ = c.Writer.Write([]byte("fail")) | ||
| return | ||
| } | ||
| if !expectedAmount.Equal(actualAmount) { | ||
| logPaymentReject(c.Request.Context(), model.PaymentProviderEpay, verifyInfo.ServiceTradeNo, expectedAmount, actualAmount, "amount_mismatch", c.ClientIP()) | ||
| _, _ = c.Writer.Write([]byte("fail")) | ||
| return | ||
| } | ||
| if topUp.Status != common.TopUpStatusPending && topUp.Status != common.TopUpStatusSuccess { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("Epay callback rejected provider=%s trade_no=%s reason=invalid_status status=%s client_ip=%s", model.PaymentProviderEpay, verifyInfo.ServiceTradeNo, topUp.Status, c.ClientIP())) | ||
| _, _ = c.Writer.Write([]byte("fail")) | ||
| return | ||
| } | ||
| } | ||
| _, err := c.Writer.Write([]byte("success")) |
Contributor
There was a problem hiding this comment.
Do not acknowledge Epay before the recharge path succeeds.
The handler writes "success" before the locked update/quota-credit block runs. If topUp.Update() or IncreaseUserQuota() fails afterward, the gateway has already been told the callback succeeded, so the paid order can remain uncredited with no retry signal.
Also applies to: 420-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/topup.go` around lines 373 - 403, The handler currently writes
"success" before performing the critical post-payment operations, so move the
c.Writer.Write([]byte("success")) call to after the locked update/quota-credit
block (i.e., only after topUp.Update() and IncreaseUserQuota() succeed); if
either topUp.Update() or IncreaseUserQuota() fails, log the error (use
logger.LogError or existing logPaymentReject), write "fail" and return so the
gateway can retry. Locate code paths using GetTopUpByTradeNo,
normalizeMoneyDecimalFromFloat/FromString, logPaymentReject, and the subsequent
topUp.Update()/IncreaseUserQuota() calls and ensure acknowledgement is
conditional on their success. Ensure all early reject paths still write "fail"
immediately.
| return | ||
| } | ||
| } else { | ||
| logger.LogWarn(c.Request.Context(), fmt.Sprintf("wechat auto register rejected provider=wechat reason=register_disabled wechat_id=%s client_ip=%s", wechatId, c.ClientIP())) |
Contributor
There was a problem hiding this comment.
Avoid logging raw wechat_id.
This is a stable external account identifier, so writing it verbatim to warn logs increases PII exposure. Prefer a hash/truncated form or log only internal request context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/wechat.go` at line 108, The warning log currently prints the raw
external account identifier wechatId in the logger.LogWarn call; update the code
that calls logger.LogWarn (the site using wechatId and c.ClientIP()) to avoid
logging raw PII by deriving and logging a non-reversible fingerprint (e.g., hash
like SHA256 and truncate to a short fixed length) or a truncated masked form of
wechatId instead, and keep c.ClientIP() as-is; ensure the new value replaces
wechatId in the fmt.Sprintf invocation so only the hashed/truncated/masked
identifier is written to logs.
Comment on lines
+39
to
+44
| apiRouter.POST("/oauth/email/bind", middleware.UserAuth(), middleware.CriticalRateLimit(), controller.EmailBind) | ||
| // Non-standard OAuth (WeChat, Telegram) - keep original routes | ||
| apiRouter.GET("/oauth/wechat", middleware.CriticalRateLimit(), controller.WeChatAuth) | ||
| apiRouter.POST("/oauth/wechat/bind", middleware.CriticalRateLimit(), controller.WeChatBind) | ||
| apiRouter.POST("/oauth/wechat/bind", middleware.UserAuth(), middleware.CriticalRateLimit(), controller.WeChatBind) | ||
| apiRouter.GET("/oauth/telegram/login", middleware.CriticalRateLimit(), controller.TelegramLogin) | ||
| apiRouter.GET("/oauth/telegram/bind", middleware.CriticalRateLimit(), controller.TelegramBind) | ||
| apiRouter.GET("/oauth/telegram/bind", middleware.UserAuth(), middleware.CriticalRateLimit(), controller.TelegramBind) |
Contributor
There was a problem hiding this comment.
These bind routes now admit access-token auth, but the handlers still depend on session state.
middleware.UserAuth() accepts Authorization-based auth, while controller.EmailBind, controller.WeChatBind, and controller.TelegramBind still do sessions.Default(c).Get("id").(int). An access-token-only request can now pass the middleware and then panic on the nil session lookup. Either switch those handlers to c.GetInt("id") or keep these routes behind a session-only guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/api-router.go` around lines 39 - 44, The bind routes now allow
Authorization header auth via middleware.UserAuth(), but the handlers EmailBind,
WeChatBind, and TelegramBind still read sessions.Default(c).Get("id").(int) and
will panic on access-token-only requests; update those handlers to safely read
the authenticated user id from the request context (use c.GetInt("id") or
equivalent context value set by UserAuth()) instead of casting session values,
or alternatively remove middleware.UserAuth() from these routes and enforce a
session-only guard so sessions.Default(c).Get("id") is always present; modify
either the middleware usage in router/api-router.go or the handlers (EmailBind,
WeChatBind, TelegramBind) accordingly to ensure no nil session lookup occurs.
Collaborator
|
感谢您的贡献，您可以换个AI再扫描一下您的更改看有没有多余的操作或者破坏掉原有功能的。 |
Author
|
我已经重新过了一遍代码，并且做了对应的修改，我该怎么重新贡献呢？ |
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.
📝 变更描述 / Description
本次改动聚焦补齐几条高风险链路上的服务端校验，范围控制在初始化、OAuth 绑定、自助资料更新和支付回调四个模块，没有改数据库结构，也没有调整前端或接口字段。
具体包括：
/api/setup：当系统已初始化，或数据库中已存在 root 用户时，直接拒绝再次初始化，避免部署状态异常时被重复调用或抢占 root。/api/oauth/email/bind、/api/oauth/wechat/bind、/api/oauth/telegram/bind统一挂载UserAuth()，把“只能登录用户绑定”的约束放回服务端。UpdateSelf：普通用户自助更新改为白名单处理，只保留现有允许的自助字段，并显式拒绝quota、role、group、status、used_quota、request_count、access_token等敏感字段，避免通过 overposting 修改权限、额度或身份相关信息。RegisterEnabled，已绑定用户的正常登录流程保持不变。这些改动复用了现有 middleware、controller 和 model 的处理路径，属于有边界的安全修复，不引入新的权限体系，也不改变正常支付和已绑定账号的既有业务流程。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
本地已执行：
关键人工验收点：
Summary by CodeRabbit
Security
Improvements