Draft
Conversation
Contributor
WalkthroughIntroduces the "Waffo Pancake" payment method for user top-ups, adding configuration settings, payment request/webhook handlers, checkout session creation, signature verification, transaction management, API routes, and frontend UI components to support this new payment gateway integration. Changes
Sequence DiagramssequenceDiagram
actor User
participant Frontend as Frontend UI
participant Controller as Payment Controller
participant Service as Waffo Service
participant WaffoDB as Database
participant WaffoPay as Waffo API
User->>Frontend: Enter top-up amount
Frontend->>Frontend: Validate amount >= minTopUp
Frontend->>Controller: POST /api/user/waffo-pancake/pay<br/>(amount)
Controller->>WaffoDB: Create pending TopUp record<br/>(generate tradeNo)
Controller->>Service: CreateWaffoPancakeCheckoutSession<br/>(params with tradeNo metadata)
Service->>Service: Sign request<br/>(SHA-256 + RSA/PKCS#1)
Service->>WaffoPay: POST checkout session<br/>(signed payload)
WaffoPay-->>Service: Return sessionID, checkoutURL, expiresAt
Service-->>Controller: Return session details
Controller-->>Frontend: Return checkoutURL
Frontend->>User: Open checkout URL (new window)
rect rgba(200, 150, 100, 0.5)
Note over User,WaffoPay: User completes payment on Waffo platform
end
WaffoPay->>Controller: POST /api/waffo-pancake/webhook<br/>(order.completed event + signature)
Controller->>Service: VerifyConfiguredWaffoPancakeWebhook<br/>(payload + header signature)
Service->>Service: Verify timestamp tolerance
Service->>Service: Verify RSA signature<br/>(against configured pubkey)
Service-->>Controller: Return verified event
Controller->>Service: ExtractWaffoPancakeTradeNo(event)
Service-->>Controller: Return tradeNo
Controller->>WaffoDB: LockOrder(tradeNo)
Controller->>WaffoDB: RechargeWaffoPancake(tradeNo)
WaffoDB->>WaffoDB: Update TopUp to Success<br/>Increment user quota
WaffoDB-->>Controller: Success
Controller-->>WaffoPay: HTTP 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (7)
model/topup.go (1)
439-494: Consolidate duplicated Waffo recharge transaction logic.
RechargeWaffoandRechargeWaffoPancakenow share almost identical idempotency + transaction + quota-credit code. This is drift-prone when one path gets patched and the other is missed.♻️ Proposed refactor sketch
+func rechargeWaffoByTradeNo(tradeNo string) (topUp *TopUp, quotaToAdd int, err error) { + // shared lock/query/status/quota/update logic +} + func RechargeWaffo(tradeNo string) (err error) { - // duplicated logic... + topUp, quotaToAdd, err := rechargeWaffoByTradeNo(tradeNo) + // provider-specific logging } func RechargeWaffoPancake(tradeNo string) (err error) { - // duplicated logic... + topUp, quotaToAdd, err := rechargeWaffoByTradeNo(tradeNo) + // provider-specific logging }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup.go` around lines 439 - 494, Both RechargeWaffo and RechargeWaffoPancake duplicate the same idempotency + transaction + quota-credit logic; extract that common flow into a single helper (e.g., processWaffoTopUp or applyTopUpTransaction) that accepts the identifying tradeNo and any source-specific metadata, performs the FOR UPDATE lookup on TopUp (mirroring the current refCol logic), validates status, computes quotaToAdd, updates TopUp (CompleteTime/Status) and increments User.quota inside one DB.Transaction, and returns the updated TopUp, quotaToAdd and an error; then simplify RechargeWaffo and RechargeWaffoPancake to call this helper and only handle post-success logging (RecordLog) and error mapping. Ensure the helper preserves existing behaviors: PostgreSQL quoting, idempotent success return (no-op if already success), same error messages, and calling tx.Save / tx.Model updates as currently implemented.controller/topup_waffo_pancake.go (2)
169-171: Ignored error when updating TopUp status to failed.If
topUp.Update()fails, the database will have a stale pending record that never transitions to failed. Consider logging the error to aid debugging.Proposed fix
log.Printf("create Waffo Pancake checkout session failed: %v", err) topUp.Status = common.TopUpStatusFailed - _ = topUp.Update() + if updateErr := topUp.Update(); updateErr != nil { + log.Printf("failed to mark Waffo Pancake topup as failed: %v, trade_no=%s", updateErr, tradeNo) + } c.JSON(200, gin.H{"message": "error", "data": "拉起支付失败"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 169 - 171, The call to topUp.Update() ignores errors causing stale pending records; change the code around setting topUp.Status = common.TopUpStatusFailed to capture the returned error from topUp.Update(), handle it (e.g., log via your logger or c.Error/c.JSON with an internal error message), and only proceed to respond after ensuring the update succeeded or after logging the update error; reference the topUp variable and the topUp.Update() method and ensure the failure path logs the error before sending c.JSON(200, gin.H{...}).
206-216: Silently returning OK for missing trade number may hide webhook issues.When
tradeNois empty, the handler logs and returns200 OK. This prevents retries, but if the metadata is malformed due to a bug, the order will never be fulfilled. Consider whether this should be a400to trigger investigation, or add monitoring/alerting for this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/topup_waffo_pancake.go` around lines 206 - 216, The handler currently logs and returns 200 OK when tradeNo is empty (in the block using service.ExtractWaffoPancakeTradeNo and event.NormalizedEventType), which suppresses retries and hides malformed webhook metadata; change this to return a client/error status (e.g., c.String(400, "missing trade no") or 422) instead of 200, enrich the log (replace log.Printf call) to include event.ID and raw payload/metadata, and emit/record a monitoring metric or alert (e.g., call metrics.IncWebhookMalformed or monitoring.Alert) so missing trade numbers are visible and actionable rather than silently ignored.web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx (3)
300-307:min={0}allows zero unit price, but validation requires> 0.The
Form.InputNumberforWaffoPancakeUnitPricesetsmin={0}, but the submit validation at line 141 rejects values<= 0. Setmin={0.01}to provide consistent feedback and prevent user confusion.Proposed fix
<Form.InputNumber field='WaffoPancakeUnitPrice' precision={2} label={t('充值价格（x元/美金）')} placeholder='1.0' - min={0} + min={0.01} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 300 - 307, The Form.InputNumber for WaffoPancakeUnitPrice currently sets min={0} which contradicts the form submit validation that rejects values <= 0; update the InputNumber to use min={0.01} so the UI prevents zero input and matches the submit-side check (locate the Form.InputNumber with field='WaffoPancakeUnitPrice' in SettingsPaymentGatewayWaffoPancake.jsx and align it with the submit validation logic that enforces > 0).
180-193: Parallel API calls may result in partial updates without clear feedback.Using
Promise.allmeans all requests run in parallel. If some succeed and some fail, the user sees error toasts for failures but no indication of what succeeded. The earlyreturnat line 192 also preventsprops.refresh?.()and the success toast, leaving the UI potentially out of sync.Consider using
Promise.allSettledto handle partial success more gracefully, or switch to sequential updates if atomicity is important.Alternative approach using Promise.allSettled
- const results = await Promise.all( - options.map((opt) => - API.put('/api/option/', { - key: opt.key, - value: opt.value, - }), - ), - ); - - const errorResults = results.filter((res) => !res.data.success); - if (errorResults.length > 0) { - errorResults.forEach((res) => showError(res.data.message)); - return; - } + const results = await Promise.allSettled( + options.map((opt) => + API.put('/api/option/', { + key: opt.key, + value: opt.value, + }), + ), + ); + + const failures = results.filter( + (r) => r.status === 'rejected' || !r.value?.data?.success, + ); + if (failures.length > 0) { + failures.forEach((r) => { + const msg = r.status === 'rejected' ? r.reason : r.value?.data?.message; + showError(msg || t('更新失败')); + }); + if (failures.length < results.length) { + showSuccess(t('部分设置已更新')); + props.refresh?.(); + } + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 180 - 193, The current bulk update uses Promise.all with options.map and then filters results into errorResults and early returns, which can leave partial updates and skip props.refresh?.() and the success toast; change the implementation to use Promise.allSettled for the API.put calls (or perform them sequentially if you require atomicity), inspect settled results to separate fulfilled vs rejected (check result.status and result.value.data.success), call showError for each failure and show a single success toast if at least one update succeeded, then always call props.refresh?.() so the UI is consistent; update references to results, errorResults, API.put, showError, and props.refresh?.() in the function accordingly.
95-98: UseparseIntfor integer fieldWaffoPancakeMinTopUp.
WaffoPancakeMinTopUprepresents a count (minimum number of units), soparseFloatmay introduce floating-point precision issues. UseparseIntto match the field's semantics.Proposed fix
WaffoPancakeMinTopUp: props.options.WaffoPancakeMinTopUp !== undefined - ? parseFloat(props.options.WaffoPancakeMinTopUp) + ? parseInt(props.options.WaffoPancakeMinTopUp, 10) : 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx` around lines 95 - 98, The WaffoPancakeMinTopUp value is an integer count but currently uses parseFloat; change the parsing to parseInt with an explicit radix (10) so the field is interpreted as an integer and falls back to 1 when undefined or NaN (update the code around WaffoPancakeMinTopUp in SettingsPaymentGatewayWaffoPancake.jsx to use parseInt(props.options.WaffoPancakeMinTopUp, 10) and ensure a default of 1 if the parsed result is NaN).web/src/components/topup/RechargeCard.jsx (1)
426-426: Empty catch block silently swallows errors.Consider logging the error or removing the try-catch if the default
usdRate = 7fallback is intentional for all failure cases.Proposed fix
- } catch (e) {} + } catch (e) { + console.warn('Failed to parse status for USD rate:', e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/topup/RechargeCard.jsx` at line 426, The empty catch in RechargeCard.jsx silently swallows errors when computing usdRate (the try-catch around the usdRate assignment), so either remove the try-catch if you intend to always use the fallback usdRate = 7, or log the error inside the catch (e.g., console.error or your app logger) and then keep the fallback; update the catch block in the function/component that sets usdRate to include a meaningful log message (reference variable usdRate and the try block that computes it) rather than an empty catch.
🤖 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_waffo_pancake.go`:
- Around line 218-219: The current in-process LockOrder/UnlockOrder (used in
handlers like the Waffo webhook where LockOrder(tradeNo) and defer
UnlockOrder(tradeNo) are called) uses a process-local sync.Map and won’t prevent
races in multi-instance deployments; replace this with a distributed lock
(preferred: Redis-based lock using the existing Redis in the stack, or DB
row-level locking via a SELECT ... FOR UPDATE inside a transaction that scopes
the webhook processing) so that concurrent webhook requests for the same tradeNo
across instances are serialized; alternatively, if you intend to keep the
process-local LockOrder/UnlockOrder, add clear documentation of the
single-instance deployment constraint and change usages (e.g., in the Waffo
handler) to use the new distributed lock API instead of LockOrder/UnlockOrder.
In `@controller/topup.go`:
- Around line 82-86: The enableWaffoPancake boolean currently only checks
merchant/private/store/product IDs; include the webhook public-key readiness in
this gating by adding a non-empty trimmed check for the webhook public key (e.g.
strings.TrimSpace(setting.WaffoPancakeWebhookPublicKey) != "") into the same
expression that defines enableWaffoPancake so callback verification won't be
skipped when the key is missing; update the enableWaffoPancake assignment
accordingly (and optionally add a log/warning elsewhere if the webhook key is
missing).
In `@service/waffo_pancake.go`:
- Around line 334-343: The current verifyWaffoPancakeWebhookWithResolvedKeys
function falls back to test when opts.environment is empty, allowing test-signed
webhooks in prod; change it to require an explicit environment and remove the
prod->test fallback: have verifyWaffoPancakeWebhookWithResolvedKeys accept only
"prod" or "test" (return a clear error for any other/empty value), call
resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys) once and
pass that to verifyWaffoPancakeWebhookWithKey, and ensure callers (e.g.,
VerifyConfiguredWaffoPancakeWebhook) always provide opts.environment.
- Around line 173-181: VerifyConfiguredWaffoPancakeWebhook currently omits the
sandbox environment flag so verifyWaffoPancakeWebhook falls back to trying both
keys; update VerifyConfiguredWaffoPancakeWebhook to set opts.environment using
setting.WaffoPancakeSandbox (or equivalent) so the
waffoPancakeWebhookVerifyOptions.environment field is populated, ensuring
verifyWaffoPancakeWebhookWithResolvedKeys uses only the correct environment
(Test or Prod) when resolving publicKeys and prevents acceptance of the
hardcoded test key in production.
- Around line 154-158: The code is using http.DefaultClient.Do(req) which has no
timeout and can hang; replace it by creating a dedicated http.Client with a
sensible Timeout (e.g., 10s) and use client.Do(req) instead of
http.DefaultClient.Do; update the scope where resp and err are obtained (the
block using req, resp, err and defer resp.Body.Close()) so it uses this new
client (refer to the variables req, resp, err and the call currently to
http.DefaultClient.Do) and ensure any context-based cancellation is preserved if
the surrounding function accepts a context.
In `@web/src/components/settings/PaymentSetting.jsx`:
- Line 140: The current updater setInputs((prev) => ({ ...prev, ...newInputs }))
preserves prior values for keys that the backend omits, which can re-submit
stale Waffo Pancake credentials; change the update logic in the PaymentSetting
component so that when handling Waffo Pancake payloads you either replace the
credentials object entirely or explicitly clear sensitive fields omitted by
newInputs (e.g., merchantKey, privateKey) instead of merging with prev—locate
the call to setInputs and make the merge conditional: if newInputs contains a
Waffo Pancake credentials object, use that object as the source of truth (or set
omitted sensitive keys to null/empty) rather than shallow-merging with prev.
In `@web/src/i18n/locales/zh-CN.json`:
- Line 2345: Remove the duplicate translation entry for the key "未启用" so only a
single mapping for that key remains in the JSON; locate both occurrences of the
string "未启用" in the locale file, delete the later duplicate (or merge if
translations differ), and validate the JSON afterwards to ensure no trailing
commas or syntax errors.
---
Nitpick comments:
In `@controller/topup_waffo_pancake.go`:
- Around line 169-171: The call to topUp.Update() ignores errors causing stale
pending records; change the code around setting topUp.Status =
common.TopUpStatusFailed to capture the returned error from topUp.Update(),
handle it (e.g., log via your logger or c.Error/c.JSON with an internal error
message), and only proceed to respond after ensuring the update succeeded or
after logging the update error; reference the topUp variable and the
topUp.Update() method and ensure the failure path logs the error before sending
c.JSON(200, gin.H{...}).
- Around line 206-216: The handler currently logs and returns 200 OK when
tradeNo is empty (in the block using service.ExtractWaffoPancakeTradeNo and
event.NormalizedEventType), which suppresses retries and hides malformed webhook
metadata; change this to return a client/error status (e.g., c.String(400,
"missing trade no") or 422) instead of 200, enrich the log (replace log.Printf
call) to include event.ID and raw payload/metadata, and emit/record a monitoring
metric or alert (e.g., call metrics.IncWebhookMalformed or monitoring.Alert) so
missing trade numbers are visible and actionable rather than silently ignored.
In `@model/topup.go`:
- Around line 439-494: Both RechargeWaffo and RechargeWaffoPancake duplicate the
same idempotency + transaction + quota-credit logic; extract that common flow
into a single helper (e.g., processWaffoTopUp or applyTopUpTransaction) that
accepts the identifying tradeNo and any source-specific metadata, performs the
FOR UPDATE lookup on TopUp (mirroring the current refCol logic), validates
status, computes quotaToAdd, updates TopUp (CompleteTime/Status) and increments
User.quota inside one DB.Transaction, and returns the updated TopUp, quotaToAdd
and an error; then simplify RechargeWaffo and RechargeWaffoPancake to call this
helper and only handle post-success logging (RecordLog) and error mapping.
Ensure the helper preserves existing behaviors: PostgreSQL quoting, idempotent
success return (no-op if already success), same error messages, and calling
tx.Save / tx.Model updates as currently implemented.
In `@web/src/components/topup/RechargeCard.jsx`:
- Line 426: The empty catch in RechargeCard.jsx silently swallows errors when
computing usdRate (the try-catch around the usdRate assignment), so either
remove the try-catch if you intend to always use the fallback usdRate = 7, or
log the error inside the catch (e.g., console.error or your app logger) and then
keep the fallback; update the catch block in the function/component that sets
usdRate to include a meaningful log message (reference variable usdRate and the
try block that computes it) rather than an empty catch.
In `@web/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx`:
- Around line 300-307: The Form.InputNumber for WaffoPancakeUnitPrice currently
sets min={0} which contradicts the form submit validation that rejects values <=
0; update the InputNumber to use min={0.01} so the UI prevents zero input and
matches the submit-side check (locate the Form.InputNumber with
field='WaffoPancakeUnitPrice' in SettingsPaymentGatewayWaffoPancake.jsx and
align it with the submit validation logic that enforces > 0).
- Around line 180-193: The current bulk update uses Promise.all with options.map
and then filters results into errorResults and early returns, which can leave
partial updates and skip props.refresh?.() and the success toast; change the
implementation to use Promise.allSettled for the API.put calls (or perform them
sequentially if you require atomicity), inspect settled results to separate
fulfilled vs rejected (check result.status and result.value.data.success), call
showError for each failure and show a single success toast if at least one
update succeeded, then always call props.refresh?.() so the UI is consistent;
update references to results, errorResults, API.put, showError, and
props.refresh?.() in the function accordingly.
- Around line 95-98: The WaffoPancakeMinTopUp value is an integer count but
currently uses parseFloat; change the parsing to parseInt with an explicit radix
(10) so the field is interpreted as an integer and falls back to 1 when
undefined or NaN (update the code around WaffoPancakeMinTopUp in
SettingsPaymentGatewayWaffoPancake.jsx to use
parseInt(props.options.WaffoPancakeMinTopUp, 10) and ensure a default of 1 if
the parsed result is NaN).
🪄 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: b7f7f4f5-7cf3-41e1-8a73-b58cab233604
📒 Files selected for processing (12)
controller/topup.gocontroller/topup_waffo_pancake.gomodel/option.gomodel/topup.gorouter/api-router.goservice/waffo_pancake.gosetting/payment_waffo_pancake.goweb/src/components/settings/PaymentSetting.jsxweb/src/components/topup/RechargeCard.jsxweb/src/components/topup/index.jsxweb/src/i18n/locales/zh-CN.jsonweb/src/pages/Setting/Payment/SettingsPaymentGatewayWaffoPancake.jsx
Comment on lines
+218
to
+219
| LockOrder(tradeNo) | ||
| defer UnlockOrder(tradeNo) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other webhook handlers in the codebase handle locking
rg -n -A5 "LockOrder|UnlockOrder" --type=goRepository: QuantumNous/new-api
Length of output: 5240
🏁 Script executed:
# View the complete LockOrder/UnlockOrder implementation
sed -n '278,310p' controller/topup.goRepository: QuantumNous/new-api
Length of output: 678
🏁 Script executed:
# Check if there's documentation or deployment configuration
fd -e md -e yaml -e yml -e json -e toml | head -20Repository: QuantumNous/new-api
Length of output: 459
🏁 Script executed:
# Search for deployment or distributed deployment references
rg -i "(distributed|deployment|multi.instance|cluster)" --type=go -l | head -10Repository: QuantumNous/new-api
Length of output: 312
🏁 Script executed:
# Check if Redis is configured and used elsewhere in the codebase
rg -i "redis" --type=go -l | head -10Repository: QuantumNous/new-api
Length of output: 282
🏁 Script executed:
# Check docker-compose.yml for multi-instance setup
cat docker-compose.ymlRepository: QuantumNous/new-api
Length of output: 3338
🏁 Script executed:
# Check README for deployment guidance
head -100 README.md | grep -i -A5 "deploy\|instance\|distributed"Repository: QuantumNous/new-api
Length of output: 201
🏁 Script executed:
# Search for any comments about single-instance assumptions in topup.go
rg -B5 -A5 "LockOrder|orderLocks" controller/topup.go | head -50Repository: QuantumNous/new-api
Length of output: 953
In-process locking is insufficient for distributed deployments.
The LockOrder/UnlockOrder implementation uses a process-local sync.Map. The docker-compose.yml includes a comment indicating multi-node deployment is a supported scenario (SESSION_SECRET=random_string # multi-node deployment, set this to a random string). If multiple instances are deployed, concurrent webhook requests for the same tradeNo across instances can race.
This pattern is pervasive across all webhook handlers (Epay, Stripe, Creem, Waffo, etc.). For multi-instance deployments, switch to database-level locking (e.g., SELECT ... FOR UPDATE) or Redis-based locking (Redis is already in the stack). If single-instance deployment is the only supported mode, explicitly document this constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/topup_waffo_pancake.go` around lines 218 - 219, The current
in-process LockOrder/UnlockOrder (used in handlers like the Waffo webhook where
LockOrder(tradeNo) and defer UnlockOrder(tradeNo) are called) uses a
process-local sync.Map and won’t prevent races in multi-instance deployments;
replace this with a distributed lock (preferred: Redis-based lock using the
existing Redis in the stack, or DB row-level locking via a SELECT ... FOR UPDATE
inside a transaction that scopes the webhook processing) so that concurrent
webhook requests for the same tradeNo across instances are serialized;
alternatively, if you intend to keep the process-local LockOrder/UnlockOrder,
add clear documentation of the single-instance deployment constraint and change
usages (e.g., in the Waffo handler) to use the new distributed lock API instead
of LockOrder/UnlockOrder.
Comment on lines
+82
to
+86
| enableWaffoPancake := setting.WaffoPancakeEnabled && | ||
| strings.TrimSpace(setting.WaffoPancakeMerchantID) != "" && | ||
| strings.TrimSpace(setting.WaffoPancakePrivateKey) != "" && | ||
| strings.TrimSpace(setting.WaffoPancakeStoreID) != "" && | ||
| strings.TrimSpace(setting.WaffoPancakeProductID) != "" |
Contributor
There was a problem hiding this comment.
Include webhook public-key readiness in enableWaffoPancake gating.
Current checks only validate merchant/private/store/product IDs. If webhook verify key is missing, users can pay but callback verification may fail and quota won’t be credited.
🔐 Suggested gating update
enableWaffoPancake := setting.WaffoPancakeEnabled &&
strings.TrimSpace(setting.WaffoPancakeMerchantID) != "" &&
strings.TrimSpace(setting.WaffoPancakePrivateKey) != "" &&
strings.TrimSpace(setting.WaffoPancakeStoreID) != "" &&
- strings.TrimSpace(setting.WaffoPancakeProductID) != ""
+ strings.TrimSpace(setting.WaffoPancakeProductID) != "" &&
+ ((!setting.WaffoPancakeSandbox &&
+ strings.TrimSpace(setting.WaffoPancakeProdWebhookPublicKey) != "") ||
+ (setting.WaffoPancakeSandbox &&
+ strings.TrimSpace(setting.WaffoPancakeTestWebhookPublicKey) != ""))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| enableWaffoPancake := setting.WaffoPancakeEnabled && | |
| strings.TrimSpace(setting.WaffoPancakeMerchantID) != "" && | |
| strings.TrimSpace(setting.WaffoPancakePrivateKey) != "" && | |
| strings.TrimSpace(setting.WaffoPancakeStoreID) != "" && | |
| strings.TrimSpace(setting.WaffoPancakeProductID) != "" | |
| enableWaffoPancake := setting.WaffoPancakeEnabled && | |
| strings.TrimSpace(setting.WaffoPancakeMerchantID) != "" && | |
| strings.TrimSpace(setting.WaffoPancakePrivateKey) != "" && | |
| strings.TrimSpace(setting.WaffoPancakeStoreID) != "" && | |
| strings.TrimSpace(setting.WaffoPancakeProductID) != "" && | |
| ((!setting.WaffoPancakeSandbox && | |
| strings.TrimSpace(setting.WaffoPancakeProdWebhookPublicKey) != "") || | |
| (setting.WaffoPancakeSandbox && | |
| strings.TrimSpace(setting.WaffoPancakeTestWebhookPublicKey) != "")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/topup.go` around lines 82 - 86, The enableWaffoPancake boolean
currently only checks merchant/private/store/product IDs; include the webhook
public-key readiness in this gating by adding a non-empty trimmed check for the
webhook public key (e.g. strings.TrimSpace(setting.WaffoPancakeWebhookPublicKey)
!= "") into the same expression that defines enableWaffoPancake so callback
verification won't be skipped when the key is missing; update the
enableWaffoPancake assignment accordingly (and optionally add a log/warning
elsewhere if the webhook key is missing).
Comment on lines
+154
to
+158
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
Contributor
There was a problem hiding this comment.
HTTP client without timeout may hang indefinitely.
http.DefaultClient has no timeout configured. If the Waffo Pancake API is slow or unresponsive, the goroutine will block indefinitely, potentially exhausting resources.
Proposed fix: Use a client with timeout
+var waffoPancakeHTTPClient = &http.Client{
+ Timeout: 30 * time.Second,
+}
+
func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) {
// ... existing code ...
- resp, err := http.DefaultClient.Do(req)
+ resp, err := waffoPancakeHTTPClient.Do(req)
if err != nil {
return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resp, err := http.DefaultClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | |
| } | |
| defer resp.Body.Close() | |
| var waffoPancakeHTTPClient = &http.Client{ | |
| Timeout: 30 * time.Second, | |
| } | |
| func CreateWaffoPancakeCheckoutSession(ctx context.Context, params *WaffoPancakeCreateSessionParams) (*WaffoPancakeCheckoutSession, error) { | |
| // ... existing code ... | |
| resp, err := waffoPancakeHTTPClient.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("request Waffo Pancake checkout session: %w", err) | |
| } | |
| defer resp.Body.Close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/waffo_pancake.go` around lines 154 - 158, The code is using
http.DefaultClient.Do(req) which has no timeout and can hang; replace it by
creating a dedicated http.Client with a sensible Timeout (e.g., 10s) and use
client.Do(req) instead of http.DefaultClient.Do; update the scope where resp and
err are obtained (the block using req, resp, err and defer resp.Body.Close()) so
it uses this new client (refer to the variables req, resp, err and the call
currently to http.DefaultClient.Do) and ensure any context-based cancellation is
preserved if the surrounding function accepts a context.
Comment on lines
+173
to
+181
| func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) { | ||
| return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{ | ||
| toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(), | ||
| publicKeys: waffoPancakeWebhookPublicKeys{ | ||
| Test: setting.WaffoPancakeTestWebhookPublicKey, | ||
| Prod: setting.WaffoPancakeProdWebhookPublicKey, | ||
| }, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Sandbox mode setting is ignored in webhook verification.
VerifyConfiguredWaffoPancakeWebhook does not pass setting.WaffoPancakeSandbox to determine the environment. The opts.environment field is left empty, causing verifyWaffoPancakeWebhookWithResolvedKeys (lines 334-343) to fall back to trying both production and test keys.
This allows an attacker to forge webhooks using the hardcoded test public key (lines 25-33) even when the system is configured for production mode, potentially crediting accounts without actual payment.
🔒 Proposed fix: Pass sandbox setting to determine environment
func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) {
+ env := "prod"
+ if setting.WaffoPancakeSandbox {
+ env = "test"
+ }
return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{
+ environment: env,
toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(),
publicKeys: waffoPancakeWebhookPublicKeys{
Test: setting.WaffoPancakeTestWebhookPublicKey,
Prod: setting.WaffoPancakeProdWebhookPublicKey,
},
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) { | |
| return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{ | |
| toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(), | |
| publicKeys: waffoPancakeWebhookPublicKeys{ | |
| Test: setting.WaffoPancakeTestWebhookPublicKey, | |
| Prod: setting.WaffoPancakeProdWebhookPublicKey, | |
| }, | |
| }) | |
| } | |
| func VerifyConfiguredWaffoPancakeWebhook(payload string, signatureHeader string) (*waffoPancakeWebhookEvent, error) { | |
| env := "prod" | |
| if setting.WaffoPancakeSandbox { | |
| env = "test" | |
| } | |
| return verifyWaffoPancakeWebhook(payload, signatureHeader, waffoPancakeWebhookVerifyOptions{ | |
| environment: env, | |
| toleranceMs: waffoPancakeDefaultTolerance.Milliseconds(), | |
| publicKeys: waffoPancakeWebhookPublicKeys{ | |
| Test: setting.WaffoPancakeTestWebhookPublicKey, | |
| Prod: setting.WaffoPancakeProdWebhookPublicKey, | |
| }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/waffo_pancake.go` around lines 173 - 181,
VerifyConfiguredWaffoPancakeWebhook currently omits the sandbox environment flag
so verifyWaffoPancakeWebhook falls back to trying both keys; update
VerifyConfiguredWaffoPancakeWebhook to set opts.environment using
setting.WaffoPancakeSandbox (or equivalent) so the
waffoPancakeWebhookVerifyOptions.environment field is populated, ensuring
verifyWaffoPancakeWebhookWithResolvedKeys uses only the correct environment
(Test or Prod) when resolving publicKeys and prevents acceptance of the
hardcoded test key in production.
Comment on lines
+334
to
+343
| func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error { | ||
| if opts.environment == "prod" || opts.environment == "test" { | ||
| return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) | ||
| } | ||
|
|
||
| if err := verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("prod", opts.publicKeys)); err == nil { | ||
| return nil | ||
| } | ||
| return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("test", opts.publicKeys)) | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback to both keys weakens security guarantees.
When opts.environment is empty (which is always the case currently), the code tries the production key first, then falls back to the test key. This means a webhook signed with the test key will be accepted in production.
Even after fixing VerifyConfiguredWaffoPancakeWebhook to pass the environment, this fallback logic should be removed to enforce strict environment separation.
🔒 Proposed fix: Remove fallback, require explicit environment
func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error {
- if opts.environment == "prod" || opts.environment == "test" {
- return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys))
+ if opts.environment == "" {
+ return fmt.Errorf("webhook environment not specified")
}
-
- if err := verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("prod", opts.publicKeys)); err == nil {
- return nil
- }
- return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("test", opts.publicKeys))
+ return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error { | |
| if opts.environment == "prod" || opts.environment == "test" { | |
| return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) | |
| } | |
| if err := verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("prod", opts.publicKeys)); err == nil { | |
| return nil | |
| } | |
| return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey("test", opts.publicKeys)) | |
| } | |
| func verifyWaffoPancakeWebhookWithResolvedKeys(signatureInput string, signaturePart string, opts waffoPancakeWebhookVerifyOptions) error { | |
| if opts.environment == "" { | |
| return fmt.Errorf("webhook environment not specified") | |
| } | |
| return verifyWaffoPancakeWebhookWithKey(signatureInput, signaturePart, resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/waffo_pancake.go` around lines 334 - 343, The current
verifyWaffoPancakeWebhookWithResolvedKeys function falls back to test when
opts.environment is empty, allowing test-signed webhooks in prod; change it to
require an explicit environment and remove the prod->test fallback: have
verifyWaffoPancakeWebhookWithResolvedKeys accept only "prod" or "test" (return a
clear error for any other/empty value), call
resolveWaffoPancakeWebhookPublicKey(opts.environment, opts.publicKeys) once and
pass that to verifyWaffoPancakeWebhookWithKey, and ensure callers (e.g.,
VerifyConfiguredWaffoPancakeWebhook) always provide opts.environment.
| }); | ||
|
|
||
| setInputs(newInputs); | ||
| setInputs((prev) => ({ ...prev, ...newInputs })); |
Contributor
There was a problem hiding this comment.
Avoid preserving stale Waffo Pancake credentials when option payload is partial.
setInputs((prev) => ({ ...prev, ...newInputs })) keeps old values for keys omitted by the backend response, which can accidentally resubmit outdated merchant/private key settings.
🔧 Suggested safe merge for Waffo Pancake keys
- setInputs((prev) => ({ ...prev, ...newInputs }));
+ setInputs((prev) => ({
+ ...prev,
+ WaffoPancakeEnabled: false,
+ WaffoPancakeSandbox: false,
+ WaffoPancakeMerchantID: '',
+ WaffoPancakePrivateKey: '',
+ WaffoPancakeStoreID: '',
+ WaffoPancakeProductID: '',
+ WaffoPancakeReturnURL: '',
+ WaffoPancakeCurrency: 'USD',
+ WaffoPancakeUnitPrice: 1.0,
+ WaffoPancakeMinTopUp: 1,
+ WaffoPancakeProdWebhookPublicKey: '',
+ WaffoPancakeTestWebhookPublicKey: '',
+ ...newInputs,
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| setInputs((prev) => ({ ...prev, ...newInputs })); | |
| setInputs((prev) => ({ | |
| ...prev, | |
| WaffoPancakeEnabled: false, | |
| WaffoPancakeSandbox: false, | |
| WaffoPancakeMerchantID: '', | |
| WaffoPancakePrivateKey: '', | |
| WaffoPancakeStoreID: '', | |
| WaffoPancakeProductID: '', | |
| WaffoPancakeReturnURL: '', | |
| WaffoPancakeCurrency: 'USD', | |
| WaffoPancakeUnitPrice: 1.0, | |
| WaffoPancakeMinTopUp: 1, | |
| WaffoPancakeProdWebhookPublicKey: '', | |
| WaffoPancakeTestWebhookPublicKey: '', | |
| ...newInputs, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/PaymentSetting.jsx` at line 140, The current
updater setInputs((prev) => ({ ...prev, ...newInputs })) preserves prior values
for keys that the backend omits, which can re-submit stale Waffo Pancake
credentials; change the update logic in the PaymentSetting component so that
when handling Waffo Pancake payloads you either replace the credentials object
entirely or explicitly clear sensitive fields omitted by newInputs (e.g.,
merchantKey, privateKey) instead of merging with prev—locate the call to
setInputs and make the merge conditional: if newInputs contains a Waffo Pancake
credentials object, use that object as the source of truth (or set omitted
sensitive keys to null/empty) rather than shallow-merging with prev.
| "充值完成后跳回的页面": "充值完成后跳回的页面", | ||
| "启用后会按测试环境保存这组配置": "启用后会按测试环境保存这组配置", | ||
| "更新 Waffo Pancake 设置": "更新 Waffo Pancake 设置", | ||
| "未启用": "未启用", |
Contributor
There was a problem hiding this comment.
Remove duplicated translation key 未启用.
This key is already defined earlier in the same JSON. Duplicate keys make resolution order-dependent and can cause confusing i18n behavior/lint failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/i18n/locales/zh-CN.json` at line 2345, Remove the duplicate
translation entry for the key "未启用" so only a single mapping for that key
remains in the JSON; locate both occurrences of the string "未启用" in the locale
file, delete the later duplicate (or merge if translations differ), and validate
the JSON afterwards to ensure no trailing commas or syntax errors.
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
1 participant
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.
💡 沟通提示 / Pre-submission
📝 变更描述 / Description
新增Waffo Pancake集成
TODO：优化支付配置项
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
TODO