Skip to content

fix(qqbot): authorize approval clicks for c2c dm sessions - #40926

Open
longXboy wants to merge 1 commit into
NousResearch:mainfrom
longXboy:fix/qqbot-dm-approval-authorization
Open

fix(qqbot): authorize approval clicks for c2c dm sessions#40926
longXboy wants to merge 1 commit into
NousResearch:mainfrom
longXboy:fix/qqbot-dm-approval-authorization

Conversation

@longXboy

@longXboy longXboy commented Jun 7, 2026

Copy link
Copy Markdown

Problem

QQBot approval button clicks from private (c2c) chats were rejected with:

WARNING gateway.platforms.qqbot.adapter: [QQBot:...] Rejected unauthorized approval click for session agent:main:qqbot:dm:<user_openid> (operator=<user_openid>)

The clicking user is the legitimate session owner, yet their own approval was denied.

Root cause

QQ c2c private chats build their session source with chat_type="dm" and chat_id == user_openid (see _handle_c2c_message), so build_session_key() produces agent:main:qqbot:dm:<user_openid>.

But _is_authorized_interaction_for_session() only handled the c2c and group/guild chat types — it fell through to return False for every dm session, rejecting the user's own click.

Fix

Add a dm branch that authorizes when the clicking operator matches the session chat_id (== user_openid), mirroring c2c semantics.

Known limitation (tracked separately)

Guild DMs (_handle_dm_message) instead key chat_id == guild_id, which build_session_key() does not pair with the user id, so guild-DM approval clicks remain unauthorized. This is documented inline and pinned by a test; fixing it requires threading the user id into the session key and is out of scope for this focused fix.

Tests

  • test_approval_click_dm_session_authorized — c2c dm session authorizes the matching user
  • test_approval_click_guild_dm_currently_rejected — pins the guild-DM gap

Full tests/gateway/test_qqbot.py passes (163 passed); ruff check clean on both files.

QQ c2c private chats build their session source with chat_type="dm"
and chat_id == user_openid (see _handle_c2c_message), so
build_session_key() yields "agent:main:qqbot:dm:<user_openid>".
_is_authorized_interaction_for_session only handled the "c2c" and
"group"/"guild" chat types, so it fell through to return False for
every "dm" session and rejected the legitimate user's own approval
click with "Rejected unauthorized approval click".

Add a "dm" branch that authorizes when the clicking operator matches
the session chat_id (== user_openid), mirroring c2c semantics.

Guild DMs (_handle_dm_message) instead key chat_id == guild_id, which
build_session_key() does not pair with the user id, so their approval
clicks remain unauthorized. That is documented inline and pinned by a
test; fixing it requires threading the user id into the session key and
is tracked separately.

Tests:
- test_approval_click_dm_session_authorized: c2c dm session authorizes
- test_approval_click_guild_dm_currently_rejected: pins guild-DM gap
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/qqbot QQ Bot adapter P2 Medium — degraded but workaround exists labels Jun 7, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verified — clean authorization fix with thorough test coverage.

What I checked:

  1. Logic correctness: The chat_type == "dm" branch correctly mirrors the existing chat_type == "c2c" pattern. QQ c2c private chats build their session key as dm:<user_openid> via _handle_c2c_message, and the clicking operator is that same user, so operator == chat_id is the right authorization check.

  2. Guild-DM exclusion: The test test_approval_click_guild_dm_currently_rejected correctly pins the known limitation — guild DMs key by guild_id rather than user_openid, so authorization fails as expected until the session-key scheme is updated. Explicitly documenting this as a known gap is better than silently failing.

  3. No auth bypass: The new dm branch is placed before the group/guild branch, so it correctly handles the dm case without falling through to broader checks.

  4. Test coverage: Three test cases cover the authorized c2c DM case, the unauthorized operator case (pre-existing), and the guild-DM rejection case — matching the three behavioral outcomes.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has the reported mismatch: QQ C2C intake creates a chat_type="dm" source at gateway/platforms/qqbot/adapter.py:1291-1297, while build_session_key() emits ...:dm:<chat_id> for DMs at gateway/session.py:907-915. The authorization branch accepts only c2c at gateway/platforms/qqbot/adapter.py:1103-1104, so a valid owner click falls through to rejection at gateway/platforms/qqbot/adapter.py:1148-1154.

The added dm branch retains the existing exact operator == chat_id authorization condition, and the added dispatch-level test exercises the previously rejected session-key shape. git apply --check confirms the patch applies cleanly to current main.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants