Skip to content

fix: restrict future tasks to administrators#9224

Open
2409324124 wants to merge 2 commits into
AstrBotDevs:masterfrom
2409324124:fix/admin-only-future-tasks
Open

fix: restrict future tasks to administrators#9224
2409324124 wants to merge 2 commits into
AstrBotDevs:masterfrom
2409324124:fix/admin-only-future-tasks

Conversation

@2409324124

@2409324124 2409324124 commented Jul 12, 2026

Copy link
Copy Markdown

Related to #8517 and #8512. Complements the future-task ownership hardening in #8881, which closed #8859.

AstrBot's built-in future_task tool was not covered by the configurable permission wrapper for plugin and MCP tools, and unlike other stateful built-in tools, it had no execution-time administrator check. As a result, a regular group member could create persistent agent jobs that later invoke the configured LLM and consume provider tokens.

This can be abused to create many high-frequency recurring tasks. Each execution can trigger another LLM request, so allowing every group member to schedule tasks may cause sustained and potentially substantial token consumption and provider API costs until the jobs are discovered and removed.

Modifications / 改动点

  • Require an AstrBot administrator role for every future_task action through the event's standard is_admin() contract.

  • Reject unauthorized calls before accessing the cron manager, database, or scheduler.

  • Cover create, edit, delete, and list for regular members and missing roles to ensure authorization fails closed.

  • Verify administrators retain task creation behavior and persist the correct session and sender identity.

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

This is a backend-only authorization fix, so no UI screenshot is applicable.

  • Regression test before the fix: failed as expected because the member call reached add_active_job() and returned a successful scheduling response.
  • Admin-contract test before the follow-up: failed as expected because the implementation inspected a raw role string instead of event.is_admin().
  • pytest tests/unit/test_cron_tools.py tests/unit/test_cron_manager.py -q: 52 passed
  • ruff format --check .: 480 files already formatted
  • ruff check .: All checks passed
  • python -m py_compile astrbot/core/tools/cron_tools.py tests/unit/test_cron_tools.py: passed
  • git diff --check: passed

Checklist / 检查清单

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request restricts future task management to admin users by checking the user's role in FutureTaskTool.call. It also updates the unit tests to mock the admin role and adds a test case verifying that non-admin users are rejected. The review feedback suggests adding defensive checks to handle cases where context, context.context, or event might be None to prevent potential AttributeError runtime crashes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +101 to +106
event = context.context.event
if getattr(event, "role", None) != "admin":
return (
"error: Permission denied. Future task management is only allowed "
f"for admin users. User's ID is: {event.get_sender_id()}."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential AttributeError runtime crashes during testing or under unexpected execution contexts where context, context.context, or event might be None, it is safer to apply defensive programming checks before accessing their attributes.

Suggested change
event = context.context.event
if getattr(event, "role", None) != "admin":
return (
"error: Permission denied. Future task management is only allowed "
f"for admin users. User's ID is: {event.get_sender_id()}."
)
event = context.context.event if context and context.context else None
if event is None:
return "error: Event context is missing."
if getattr(event, "role", None) != "admin":
return (
"error: Permission denied. Future task management is only allowed "
f"for admin users. User's ID is: {event.get_sender_id()}."
)

@2409324124 2409324124 marked this pull request as ready for review July 12, 2026 16:43
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Jul 12, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The role check in FutureTaskTool.call relies on the magic string 'admin'; consider using a shared constant or role enum to avoid drift with other permission checks and make changes easier if role names evolve.
  • The new test only verifies that non-admin users are blocked for the create action; since the check is applied to all actions, it would be helpful to add at least one test that asserts non-admin behavior for edit/delete/list to guard against future refactors that might bypass the gate.
  • The permission check assumes event.get_sender_id is always present; if FutureTaskTool can be called from other contexts, consider a safer fallback (e.g., getattr(event, "get_sender_id", lambda: "<unknown>")()) to avoid potential attribute errors and maintain a consistent error response.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The role check in `FutureTaskTool.call` relies on the magic string `'admin'`; consider using a shared constant or role enum to avoid drift with other permission checks and make changes easier if role names evolve.
- The new test only verifies that non-admin users are blocked for the `create` action; since the check is applied to all actions, it would be helpful to add at least one test that asserts non-admin behavior for `edit`/`delete`/`list` to guard against future refactors that might bypass the gate.
- The permission check assumes `event.get_sender_id` is always present; if `FutureTaskTool` can be called from other contexts, consider a safer fallback (e.g., `getattr(event, "get_sender_id", lambda: "<unknown>")()`) to avoid potential attribute errors and maintain a consistent error response.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@2409324124

Copy link
Copy Markdown
Author

@sourcery-ai @gemini-code-assist

Follow-up commit 1d6f016b addresses the actionable review feedback:

  • Replaced the raw "admin" role comparison with the standard event.is_admin() contract.
  • Added fail-closed coverage for create, edit, delete, and list for both regular members and missing roles.
  • Added a positive admin creation test that verifies the persisted session and sender identity remain correct.
  • Re-ran the cron tool/manager suites: 52 passed. Full Ruff format and lint checks also pass.

I intentionally did not add partial None handling for context or event: FutureTaskTool receives an AstrAgentContext with an AstrMessageEvent by contract, consistent with neighboring built-in tools. Silently handling an invalid executor context here could mask an upstream lifecycle error.

Please re-review the latest commit.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an admin permission check to the FutureTaskTool class, restricting future task management actions to admin users only. It also updates the corresponding unit tests by refactoring test context creation and adding comprehensive test cases to verify that non-admin users are correctly rejected across all actions. No review comments were provided, and the changes look solid.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

1 participant