fix: критические баги и дыры безопасности#55
Conversation
- SQLite WAL mode для корректной работы concurrent writes - recover() во все горутины для предотвращения краша бота - Graceful shutdown: bot.Stop() при SIGTERM - subtle.ConstantTimeCompare для секретов callback (timing attack) - Пагинация GetAllUsers (тикающая бомба при >1000 юзеров) - Бан при chargeback (предотвращение мошенничества) - io.LimitReader на ответы внешних API (OOM защита) - IdleTimeout на HTTP-сервере (slowloris защита) - Проверка isModerator в mod state handlers - HTML escape firstName в уведомлении админу Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b56b69407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Банём пользователя — chargeback = мошенничество, повторная регистрация запрещена | ||
| if err := h.bot.db.BanUser(payment.TelegramID, 0); err != nil { | ||
| slog.Warn("Chargeback: не удалось забанить пользователя", "error", err, "telegram_id", payment.TelegramID) | ||
| } |
There was a problem hiding this comment.
Calling BanUser here does not mirror the existing admin-ban flow in processBanUser, which also deletes the Remnawave account and the local users row. Because the only general ban gate is in handleStart (internal/bot/handlers.go:155), a chargebacked user who already has the chat open can still use regular handlers such as handlePayButton (internal/bot/payment_handler.go:19) against the still-present account. If chargeback is meant to be a permanent ban, this path needs the same cleanup as a normal ban.
Useful? React with 👍 / 👎.
| if err := h.bot.db.BanUser(payment.TelegramID, 0); err != nil { | ||
| slog.Warn("Chargeback: не удалось забанить пользователя", "error", err, "telegram_id", payment.TelegramID) |
There was a problem hiding this comment.
If BanUser fails here (for example because SQLite is temporarily locked), the handler only logs a warning and still returns success to Platega. That means the callback will not be retried even though the security action failed, so the user stays unbanned while the admin alert says they were banned. This should fail the callback the same way the preceding UpdatePaymentStatus call does.
Useful? React with 👍 / 👎.
| go func() { | ||
| defer recoverGoroutine("scheduler") | ||
| telegramBot.StartScheduler(ctx) |
There was a problem hiding this comment.
The new recoverGoroutine wrapper only logs and returns. If StartScheduler panics once because of a malformed DB/API record, this goroutine exits permanently while the bot keeps serving Telegram requests, so subscription expiry handling, auto-kicks, and confirmed_not_activated retries stop until someone manually restarts the process. The same pattern is also used for the sync loop and alerter.
Useful? React with 👍 / 👎.
|
@codex review last commit |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Да иди нахуй |
…, temp file, partial failure, chargeback, mutex scope)
- shutdownCh в тестовых хелперах setupTestBot/setupSchedulerTestBot (утечка горутин) - done-канал в rate limiter'ах для корректного shutdown cleanup-горутин - исключить chargebacked из CountFirstPaymentsByMonth (консистентность с остальными revenue-запросами) - idempotency guard в handleChargeback (защита от повторной обработки при retry) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review last commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ef93febc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if payment.Status == "chargebacked" { | ||
| return nil | ||
| } | ||
|
|
||
| if err := h.bot.db.UpdatePaymentStatus(payment.ID, "chargebacked"); err != nil { |
There was a problem hiding this comment.
This guard makes retries ineffective after partial failures in the same handler. The code marks the payment as chargebacked and later returns an error if BanUser fails, but the next callback retry will reload the payment and immediately exit at payment.Status == "chargebacked", so ban/removal side effects are never retried. In that scenario, the payment remains chargebacked while the user may stay unbanned and undeleted.
Useful? React with 👍 / 👎.
| // НЕ делаем return — продолжаем удаление из БД, чтобы избежать partial failure | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Removing the early return here causes local user cleanup to proceed even when DeleteUser in Remnawave failed with a non-404 error. Since scheduler processing only continues for users still present in the bot DB (runSubscriptionSchedulerPass maps DB users first), this makes the failed panel deletion non-recoverable by future scheduler runs and can leave orphaned accounts in Remnawave indefinitely.
Useful? React with 👍 / 👎.
2 participants
Summary
Полный аудит кодовой базы выявил критические проблемы безопасности и надёжности. Все исправления покрыты существующими тестами.
CRITICAL
database is lockedrecover()во все горутины — один panic убивал весь процесс ботаbot.Stop()никогда не вызывался при SIGTERM, процесс зависал!=, заменено наsubtle.ConstantTimeCompareHIGH
GetAllUsers— был хардкод 1000, пользователи 1001+ были невидимы для scheduler/broadcastio.ReadAll— OOM при злонамеренном ответе внешних APIIdleTimeoutна HTTP-сервере — защита от slowlorisMEDIUM
isModerator()— снятый модератор мог завершить операциюfirstName— инъекция форматирования в уведомлении админуTest plan
make fmt— без ошибокmake tests— все тесты зелёные🤖 Generated with Claude Code