fix(update): restrict to global admin, register commands, add error handling#345
Conversation
…andling - Change permission check from IsNormalAdmin to Env.AdminId (global admin only, matching docs) - Register /checkupdate and /update in bot command menu via SetMyCommands - Add try-catch with bot message reply for network/exceptions - Update tests to match new constructor and remove AdminService mocking
📝 WalkthroughWalkthroughThe PR refactors update command authorization in UpdateController from an injected AdminService dependency to a direct global-admin check via Env.AdminId. Exception handling is added to both command handlers, new bot commands are registered, and tests are updated to match the new authorization pattern. ChangesUpdate Command Authorization and Exception Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 docstrings
🧪 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 |
PR Check ReportSummary
Test Results
Code Quality
Test Artifacts
LinksThis report is auto-generated by GitHub Actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
TelegramSearchBot.Test/Controller/Manage/UpdateControllerTests.cs (2)
203-210: ⚡ Quick winUpdate outdated comment referencing removed AdminService.
The comment at lines 208-209 still references
AdminService.IsNormalAdmin, which was removed in this PR. The comment should be updated to reflect the newEnv.AdminIdauthorization model.Additionally, this test doesn't verify the permission check behavior. The test creates a user with ID
12345, but ifEnv.AdminIdhas a different value at test runtime, the command will be denied without the test detecting it.📝 Suggested comment update
[Fact] public async Task ExecuteAsync_CheckUpdateCommand_EntersCheckUpdateFlow() { var context = CreateContext("/checkupdate"); await _controller.ExecuteAsync(context); - // RED: AdminService.IsNormalAdmin throw → test fails. - // GREEN: mock IsNormalAdmin → verify status reply sent. + // Note: This test assumes Env.AdminId matches the default userId (12345). + // Consider adding explicit permission verification tests. }🤖 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 `@TelegramSearchBot.Test/Controller/Manage/UpdateControllerTests.cs` around lines 203 - 210, The test comment and assertions need updating to reflect the new Env.AdminId authorization model: replace the outdated reference to AdminService.IsNormalAdmin with Env.AdminId, ensure the test sets Env.AdminId to the test user ID (or otherwise arranges the environment so the created user ID 12345 is treated as admin) before calling UpdateController.ExecuteAsync, and add an assertion that the expected status reply was sent (verifying the permission check passed); locate this logic around the test method ExecuteAsync_CheckUpdateCommand_EntersCheckUpdateFlow and the CreateContext usage to make the change.
203-252: 🏗️ Heavy liftConsider adding explicit authorization tests.
The integration tests verify that command patterns are recognized and don't throw exceptions, but they don't verify the permission check behavior or response messages. Since this PR introduces a significant authorization change (restricting to
Env.AdminIdonly), consider adding tests that:
- Verify unauthorized users receive the permission denied message
- Verify authorized users (matching
Env.AdminId) can execute commands- Verify the bot's response content for different scenarios
💡 Example permission test structure
[Fact] public async Task ExecuteAsync_CheckUpdate_UnauthorizedUser_SendsPermissionDenied() { // Arrange: user ID != Env.AdminId long unauthorizedUserId = Env.AdminId + 1; var context = CreateContext("/checkupdate", userId: unauthorizedUserId); // Act await _controller.ExecuteAsync(context); // Assert: verify SendRequest was called with permission denied message _mockBotClient.Verify( x => x.SendRequest( It.Is<IRequest<Message>>(req => /* verify message contains "权限不足" */), It.IsAny<CancellationToken>()), Times.Once); } [Fact] public async Task ExecuteAsync_CheckUpdate_AuthorizedUser_SendsStatusMessage() { // Arrange: user ID == Env.AdminId var context = CreateContext("/checkupdate", userId: Env.AdminId); // Act & Assert await _controller.ExecuteAsync(context); // Verify SendRequest was called with status message _mockBotClient.Verify( x => x.SendRequest( It.IsAny<IRequest<Message>>(), It.IsAny<CancellationToken>()), Times.Once); }🤖 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 `@TelegramSearchBot.Test/Controller/Manage/UpdateControllerTests.cs` around lines 203 - 252, Add explicit authorization unit tests for the update commands: create one test that uses CreateContext("/checkupdate", userId: Env.AdminId + 1) to assert an unauthorized user receives the "权限不足" (permission denied) reply by verifying _mockBotClient.SendRequest was called with a request whose message text contains that phrase; create a second test that uses CreateContext("/checkupdate", userId: Env.AdminId) to assert the authorized flow executes by verifying _mockBotClient.SendRequest was called once with any IRequest<Message> (or with the expected status message) after calling _controller.ExecuteAsync(context). Ensure tests reference Env.AdminId, CreateContext, ExecuteAsync and verify via _mockBotClient.Verify.
🤖 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 `@TelegramSearchBot/Controller/Manage/UpdateController.cs`:
- Around line 39-43: The Env static constructor must stop silently defaulting
AdminId=0: validate that Config.json exists and parses as valid JSON, log any
exceptions (including parse errors and missing file), and enforce AdminId>0 (log
a clear error and throw/terminate startup if invalid) so controllers like
UpdateController, EditOCRConfController, EditVisionConfController,
EditLLMConfController, EditMcpConfController, RefreshController and
CheckBanGroupController cannot invert permission checks; update Env (static ctor
and any config-loading helper) to perform these checks and emit explicit error
logs before failing fast rather than leaving AdminId at 0.
---
Nitpick comments:
In `@TelegramSearchBot.Test/Controller/Manage/UpdateControllerTests.cs`:
- Around line 203-210: The test comment and assertions need updating to reflect
the new Env.AdminId authorization model: replace the outdated reference to
AdminService.IsNormalAdmin with Env.AdminId, ensure the test sets Env.AdminId to
the test user ID (or otherwise arranges the environment so the created user ID
12345 is treated as admin) before calling UpdateController.ExecuteAsync, and add
an assertion that the expected status reply was sent (verifying the permission
check passed); locate this logic around the test method
ExecuteAsync_CheckUpdateCommand_EntersCheckUpdateFlow and the CreateContext
usage to make the change.
- Around line 203-252: Add explicit authorization unit tests for the update
commands: create one test that uses CreateContext("/checkupdate", userId:
Env.AdminId + 1) to assert an unauthorized user receives the "权限不足" (permission
denied) reply by verifying _mockBotClient.SendRequest was called with a request
whose message text contains that phrase; create a second test that uses
CreateContext("/checkupdate", userId: Env.AdminId) to assert the authorized flow
executes by verifying _mockBotClient.SendRequest was called once with any
IRequest<Message> (or with the expected status message) after calling
_controller.ExecuteAsync(context). Ensure tests reference Env.AdminId,
CreateContext, ExecuteAsync and verify via _mockBotClient.Verify.
🪄 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
Run ID: fab96979-0d7b-491d-b183-d64844cf2a12
📒 Files selected for processing (3)
TelegramSearchBot.Test/Controller/Manage/UpdateControllerTests.csTelegramSearchBot/Controller/Manage/UpdateController.csTelegramSearchBot/Service/BotAPI/TelegramCommandRegistryService.cs
| // 仅允许全局管理员使用更新指令 | ||
| if (message.From.Id != Env.AdminId) { | ||
| await ReplyAsync(message, "❌ 权限不足：只有全局管理员才能使用更新指令。"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Env.AdminId initialization and usage patterns
# Check Env.AdminId declaration and initialization
ast-grep --pattern 'class Env {
$$$
AdminId
$$$
}'
# Check for validation or initialization of AdminId
rg -nP --type=cs 'AdminId\s*[=:]' -A 2 -B 2
# Check if there's startup validation
rg -nP --type=cs 'AdminId.*==\s*0|AdminId.*validation|ValidateConfig' -C 3Repository: ModerRAS/TelegramSearchBot
Length of output: 475
🏁 Script executed:
# Read full Env.cs to see AdminId property definition and type
cat -n TelegramSearchBot.Common/Env.cs | head -60Repository: ModerRAS/TelegramSearchBot
Length of output: 3507
🏁 Script executed:
# Find where config is loaded and how it's validated
rg -n --type=cs 'config\s*=|ConfigProvider|LoadConfig|BotConfig' -A 3 -B 1 | head -100Repository: ModerRAS/TelegramSearchBot
Length of output: 9677
🏁 Script executed:
# Check for BotConfig class definition to see AdminId field
fd -t f '*.cs' -x grep -l 'class.*BotConfig' {}Repository: ModerRAS/TelegramSearchBot
Length of output: 354
🏁 Script executed:
# Find Config class - likely in same directory as Env.cs
fd -t f -x grep -l 'class Config' {} \; | head -5Repository: ModerRAS/TelegramSearchBot
Length of output: 104
🏁 Script executed:
# Check for Config class in Common namespace
rg -n --type=cs 'class Config' -A 10 TelegramSearchBot.Common/Repository: ModerRAS/TelegramSearchBot
Length of output: 1103
🏁 Script executed:
# Also check what the default Config looks like and AdminId initialization
rg -n --type=cs 'AdminId' -B 5 -A 5 | head -80Repository: ModerRAS/TelegramSearchBot
Length of output: 7323
🏁 Script executed:
# Verify if there's any startup validation for AdminId
rg -n --type=cs 'AdminId.*0|AdminId.*validation|AdminId.*required' -B 2 -A 2Repository: ModerRAS/TelegramSearchBot
Length of output: 52
🏁 Script executed:
# Check if Config.json existence is validated at startup
rg -n --type=cs 'Config\.json|WorkDir.*Config' -B 2 -A 2 | head -50Repository: ModerRAS/TelegramSearchBot
Length of output: 523
Critical security issue: Env.AdminId defaults to zero when Config.json is missing, inverting permission checks across all management controllers.
The permission check if (message.From.Id != Env.AdminId) becomes if (message.From.Id != 0) when AdminId defaults to 0, allowing access to all users except user ID 0 instead of denying access to all except the configured admin. This occurs silently when Config.json is missing or contains invalid JSON (lines 14-21 of Env.cs catch exceptions without logging).
Add validation in the Env static constructor to: (1) require Config.json to exist and be valid JSON, (2) require AdminId > 0 with an explicit error message, and (3) log configuration failures. This vulnerability affects UpdateController, EditOCRConfController, EditVisionConfController, EditLLMConfController, EditMcpConfController, RefreshController, and CheckBanGroupController.
🤖 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 `@TelegramSearchBot/Controller/Manage/UpdateController.cs` around lines 39 -
43, The Env static constructor must stop silently defaulting AdminId=0: validate
that Config.json exists and parses as valid JSON, log any exceptions (including
parse errors and missing file), and enforce AdminId>0 (log a clear error and
throw/terminate startup if invalid) so controllers like UpdateController,
EditOCRConfController, EditVisionConfController, EditLLMConfController,
EditMcpConfController, RefreshController and CheckBanGroupController cannot
invert permission checks; update Env (static ctor and any config-loading helper)
to perform these checks and emit explicit error logs before failing fast rather
than leaving AdminId at 0.
1 participant
Summary
Changes
Testing
Summary by CodeRabbit
Release Notes
New Features
/checkupdatecommand enabling users to check system update availability and status./updatecommand allowing authorized users to initiate system updates.Bug Fixes