-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(aiocqhttp): resolve bare record file references #9219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,13 @@ | ||||||||||||||||||||||||||
| import asyncio | ||||||||||||||||||||||||||
| import base64 | ||||||||||||||||||||||||||
| import binascii | ||||||||||||||||||||||||||
| import inspect | ||||||||||||||||||||||||||
| import itertools | ||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||
| import uuid | ||||||||||||||||||||||||||
| from collections.abc import Awaitable | ||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||
| from typing import Any, cast | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from aiocqhttp import CQHttp, Event | ||||||||||||||||||||||||||
|
|
@@ -21,6 +24,7 @@ | |||||||||||||||||||||||||
| PlatformMetadata, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| from astrbot.core.platform.astr_message_event import MessageSesion | ||||||||||||||||||||||||||
| from astrbot.core.utils.media_utils import file_uri_to_path, is_file_uri | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from ...register import register_platform_adapter | ||||||||||||||||||||||||||
| from .aiocqhttp_message_event import * | ||||||||||||||||||||||||||
|
|
@@ -140,6 +144,113 @@ async def convert_message(self, event: Event) -> AstrBotMessage | None: | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return abm | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| async def _resolve_record_data( | ||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||
| data: dict[str, Any], | ||||||||||||||||||||||||||
| routing_params: dict[str, Any], | ||||||||||||||||||||||||||
| ) -> dict[str, Any]: | ||||||||||||||||||||||||||
| """Resolve a file-only OneBot record segment through ``get_record``. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||
| data: Raw record segment data from the OneBot event. | ||||||||||||||||||||||||||
| routing_params: Account routing parameters for the OneBot action. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||
| A copy of the segment data with an accessible media source when the | ||||||||||||||||||||||||||
| OneBot implementation can provide one. Otherwise, the original data. | ||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||
| normalized = dict(data) | ||||||||||||||||||||||||||
| for key in ("file", "url", "path"): | ||||||||||||||||||||||||||
| if isinstance(normalized.get(key), str): | ||||||||||||||||||||||||||
| normalized[key] = normalized[key].strip() | ||||||||||||||||||||||||||
| file_ref = normalized.get("file") | ||||||||||||||||||||||||||
| if not isinstance(file_ref, str) or not file_ref: | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| has_direct_source = bool( | ||||||||||||||||||||||||||
| normalized.get("url") or normalized.get("path") | ||||||||||||||||||||||||||
| ) or is_file_uri(file_ref) | ||||||||||||||||||||||||||
| has_direct_source = has_direct_source or file_ref.startswith( | ||||||||||||||||||||||||||
| ("http://", "https://", "base64://", "data:") | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| has_direct_source = has_direct_source or Path(file_ref).is_file() | ||||||||||||||||||||||||||
| except (OSError, ValueError): | ||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||
|
Comment on lines
+176
to
+179
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On Windows, passing a string with invalid path characters (such as colons in URLs like
Suggested change
|
||||||||||||||||||||||||||
| if not has_direct_source: | ||||||||||||||||||||||||||
| compact = "".join(file_ref.split()) | ||||||||||||||||||||||||||
| padding = len(compact) % 4 | ||||||||||||||||||||||||||
| if padding: | ||||||||||||||||||||||||||
| compact += "=" * (4 - padding) | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| decoded = base64.b64decode(compact, validate=True) | ||||||||||||||||||||||||||
| has_direct_source = ( | ||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||
| len(decoded) >= 12 | ||||||||||||||||||||||||||
| and decoded[:4] == b"RIFF" | ||||||||||||||||||||||||||
| and decoded[8:12] == b"WAVE" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| or (len(decoded) >= 12 and decoded[4:8] == b"ftyp") | ||||||||||||||||||||||||||
| or decoded.startswith( | ||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||
| b"#!AMR\n", | ||||||||||||||||||||||||||
| b"#!AMR-WB\n", | ||||||||||||||||||||||||||
| b"OggS", | ||||||||||||||||||||||||||
| b"fLaC", | ||||||||||||||||||||||||||
| b"ID3", | ||||||||||||||||||||||||||
| b"\xff\xfb", | ||||||||||||||||||||||||||
| b"\xff\xfa", | ||||||||||||||||||||||||||
| b"\xff\xf3", | ||||||||||||||||||||||||||
| b"\xff\xf2", | ||||||||||||||||||||||||||
| b"\xff\xe3", | ||||||||||||||||||||||||||
| b"\xff\xe2", | ||||||||||||||||||||||||||
| b"#!SILK_V3", | ||||||||||||||||||||||||||
| b"\x02#!SILK_V3", | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| except (binascii.Error, ValueError): | ||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||
|
Comment on lines
+180
to
+213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Detecting legacy bare base64 payloads solely by attempting to decode them can lead to false positives. For example, a bare filename that is a hex string (such as an MD5 hash like To prevent this, we should verify that the decoded bytes actually start with a known audio magic header (matching the formats supported by if not has_direct_source:
compact = "".join(file_ref.split())
padding = len(compact) % 4
if padding:
compact += "=" * (4 - padding)
try:
decoded = base64.b64decode(compact, validate=True)
has_direct_source = (
decoded.startswith(b"RIFF")
or decoded.startswith(b"#!AM")
or decoded.startswith(b"OggS")
or decoded.startswith(b"fLa")
or decoded.startswith(b"ID3")
or decoded.startswith(bytes([255, 251]))
or decoded.startswith(bytes([255, 243]))
or decoded.startswith(bytes([255, 242]))
or decoded.startswith(b"ftyp")
or decoded.startswith(b"#!SILK_V3")
or decoded.startswith(bytes([2]) + b"#!SILK_V3")
)
except (binascii.Error, ValueError):
pass |
||||||||||||||||||||||||||
| if has_direct_source: | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| result = await self.bot.call_action( | ||||||||||||||||||||||||||
| action="get_record", | ||||||||||||||||||||||||||
| file=file_ref, | ||||||||||||||||||||||||||
| out_format="wav", | ||||||||||||||||||||||||||
| **routing_params, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| except Exception as exc: | ||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||
| "OneBot get_record failed; preserving the original record segment: %s", | ||||||||||||||||||||||||||
| exc, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if not isinstance(result, dict): | ||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||
| "OneBot get_record returned no file path; preserving the original " | ||||||||||||||||||||||||||
| "record segment." | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| converted_file = result.get("file") | ||||||||||||||||||||||||||
| if isinstance(converted_file, str) and converted_file.strip(): | ||||||||||||||||||||||||||
| converted_file = converted_file.strip() | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| if Path(file_uri_to_path(converted_file)).is_file(): | ||||||||||||||||||||||||||
| normalized["file"] = converted_file | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
| except (OSError, ValueError): | ||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||
|
Comment on lines
+241
to
+246
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, catching
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||
| "OneBot get_record returned no accessible media source; " | ||||||||||||||||||||||||||
| "preserving the original record segment." | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| return normalized | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| async def _convert_handle_request_event(self, event: Event) -> AstrBotMessage: | ||||||||||||||||||||||||||
| """OneBot V11 请求类事件""" | ||||||||||||||||||||||||||
| abm = AstrBotMessage() | ||||||||||||||||||||||||||
|
|
@@ -410,7 +521,12 @@ async def _convert_handle_message_event( | |||||||||||||||||||||||||
| f"不支持的消息段类型,已忽略: {t}, data={m['data']}" | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||
| a = ComponentTypes[t](**m["data"]) | ||||||||||||||||||||||||||
| component_data = m["data"] | ||||||||||||||||||||||||||
| if t == "record": | ||||||||||||||||||||||||||
| component_data = await self._resolve_record_data( | ||||||||||||||||||||||||||
| component_data, routing_params | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| a = ComponentTypes[t](**component_data) | ||||||||||||||||||||||||||
| abm.message.append(a) | ||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||
| logger.exception( | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,219 @@ | ||
| import base64 | ||
| import io | ||
| import wave | ||
| from pathlib import Path | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| import pytest | ||
| from aiocqhttp import Event | ||
|
|
||
| from astrbot.core.message.components import Record | ||
| from astrbot.core.platform.sources.aiocqhttp.aiocqhttp_platform_adapter import ( | ||
| AiocqhttpAdapter, | ||
| ) | ||
|
|
||
|
|
||
| def _record_event(data: dict) -> Event: | ||
| event = Event.from_payload( | ||
| { | ||
| "post_type": "message", | ||
| "message_type": "private", | ||
| "sub_type": "friend", | ||
| "message_id": 100, | ||
| "user_id": 200, | ||
| "self_id": 300, | ||
| "sender": {"user_id": 200, "nickname": "tester"}, | ||
| "message": [{"type": "record", "data": data}], | ||
| "raw_message": "", | ||
| "time": 1, | ||
| "font": 0, | ||
| } | ||
| ) | ||
| assert event is not None | ||
| return event | ||
|
|
||
|
|
||
| def _adapter(bot: AsyncMock) -> AiocqhttpAdapter: | ||
| adapter = AiocqhttpAdapter.__new__(AiocqhttpAdapter) | ||
| adapter.bot = bot | ||
| return adapter | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_bare_record_file_is_resolved_through_onebot_get_record(tmp_path): | ||
| converted_file = tmp_path / "voice.wav" | ||
| with wave.open(str(converted_file), "wb") as wav_file: | ||
| wav_file.setnchannels(1) | ||
| wav_file.setsampwidth(2) | ||
| wav_file.setframerate(8000) | ||
| wav_file.writeframes(b"\x00\x00") | ||
| bot = AsyncMock() | ||
| bot.call_action.return_value = {"file": str(converted_file)} | ||
| event = _record_event({"file": "voice.amr"}) | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event(event) | ||
|
|
||
| assert len(message.message) == 1 | ||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == str(converted_file) | ||
| assert await record.convert_to_file_path() == str(converted_file.resolve()) | ||
| assert event.message[0]["data"]["file"] == "voice.amr" | ||
| bot.call_action.assert_awaited_once_with( | ||
| action="get_record", | ||
| file="voice.amr", | ||
| out_format="wav", | ||
| self_id=300, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_record_with_legacy_bare_base64_does_not_call_get_record(): | ||
| audio_buffer = io.BytesIO() | ||
| with wave.open(audio_buffer, "wb") as wav_file: | ||
| wav_file.setnchannels(1) | ||
| wav_file.setsampwidth(2) | ||
| wav_file.setframerate(8000) | ||
| wav_file.writeframes(b"\x00\x00") | ||
| encoded_audio = base64.b64encode(audio_buffer.getvalue()).decode() | ||
| bot = AsyncMock() | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event({"file": encoded_audio}) | ||
| ) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == encoded_audio | ||
| resolved_path = Path(await record.convert_to_file_path()) | ||
| try: | ||
| resolved_bytes = resolved_path.read_bytes() | ||
| assert resolved_bytes[:4] == b"RIFF" | ||
| assert resolved_bytes[8:12] == b"WAVE" | ||
| finally: | ||
| resolved_path.unlink(missing_ok=True) | ||
| bot.call_action.assert_not_awaited() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| "audio_bytes", | ||
| [ | ||
| b"\x00\x00\x00\x18ftypM4A \x00\x00\x00\x00M4A isom", | ||
| b"\xff\xf3\x90\x00", | ||
| b"\xff\xf2\x90\x00", | ||
| ], | ||
| ) | ||
| async def test_record_with_supported_bare_base64_signature_skips_get_record( | ||
| audio_bytes, | ||
| ): | ||
| encoded_audio = base64.b64encode(audio_bytes).decode() | ||
| bot = AsyncMock() | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event({"file": encoded_audio}) | ||
| ) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == encoded_audio | ||
| bot.call_action.assert_not_awaited() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| "file_ref", | ||
| [ | ||
| "0d2bb1468a87d64414f8e563cc61c33c", | ||
| "550e8400e29b41d4a716446655440000", | ||
| ], | ||
| ) | ||
| async def test_hash_or_uuid_record_file_is_resolved_through_onebot_get_record( | ||
| file_ref, | ||
| ): | ||
| bot = AsyncMock() | ||
| bot.call_action.return_value = {} | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event({"file": file_ref}) | ||
| ) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == file_ref | ||
| bot.call_action.assert_awaited_once_with( | ||
| action="get_record", | ||
| file=file_ref, | ||
| out_format="wav", | ||
| self_id=300, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_record_with_url_does_not_call_get_record(): | ||
| bot = AsyncMock() | ||
| data = { | ||
| "file": " voice.amr ", | ||
| "url": " https://example.test/voice.amr ", | ||
| } | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event(_record_event(data)) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == "voice.amr" | ||
| assert record.url == "https://example.test/voice.amr" | ||
| assert data["file"] == " voice.amr " | ||
| assert data["url"] == " https://example.test/voice.amr " | ||
| bot.call_action.assert_not_awaited() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_record_failure_preserves_original_record_segment(): | ||
| bot = AsyncMock() | ||
| bot.call_action.side_effect = RuntimeError("get_record unavailable") | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event({"file": "voice.amr"}) | ||
| ) | ||
|
|
||
| assert len(message.message) == 1 | ||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == "voice.amr" | ||
| bot.call_action.assert_awaited_once() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_inaccessible_get_record_path_preserves_original_record_segment(): | ||
| bot = AsyncMock() | ||
| bot.call_action.return_value = {"file": "/remote/voice.wav"} | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event({"file": "voice.amr"}) | ||
| ) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == "voice.amr" | ||
| bot.call_action.assert_awaited_once() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("source_field", ["path", "url"]) | ||
| async def test_record_with_existing_source_field_does_not_call_get_record(source_field): | ||
| bot = AsyncMock() | ||
|
|
||
| message = await _adapter(bot)._convert_handle_message_event( | ||
| _record_event( | ||
| { | ||
| "file": "voice.amr", | ||
| source_field: "/remote/voice.amr", | ||
| } | ||
| ) | ||
| ) | ||
|
|
||
| record = message.message[0] | ||
| assert isinstance(record, Record) | ||
| assert record.file == "voice.amr" | ||
| bot.call_action.assert_not_awaited() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be beneficial to add a test case to verify that a bare filename consisting of valid base64 characters (such as a hex hash or UUID) is correctly resolved through bot.call_action.assert_not_awaited()
@pytest.mark.asyncio
async def test_record_with_hex_filename_is_resolved_through_get_record(tmp_path):
converted_file = tmp_path / "voice.wav"
with wave.open(str(converted_file), "wb") as wav_file:
wav_file.setnchannels(1)
wav_file.setsampwidth(2)
wav_file.setframerate(8000)
wav_file.writeframes(bytes([0, 0]))
bot = AsyncMock()
bot.call_action.return_value = {"file": str(converted_file)}
event = _record_event({"file": "0d2bb1468a87d64414f8e563cc61c33c"})
message = await _adapter(bot)._convert_handle_message_event(event)
assert len(message.message) == 1
record = message.message[0]
assert isinstance(record, Record)
assert record.file == str(converted_file)
bot.call_action.assert_awaited_once_with(
action="get_record",
file="0d2bb1468a87d64414f8e563cc61c33c",
out_format="wav",
self_id=300,
)References
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider splitting
_resolve_record_datainto smaller helpers for normalization, media-source detection, base64-audio sniffing, andget_recordhandling to keep the main flow linear and readable.You can keep all current behavior but reduce complexity by extracting the clearly independent concerns into small helpers. This will also make it easier to test each piece.
1. Extract normalization and direct-source detection
Move the normalization and “has direct source” logic into helpers to clarify the high‑level flow:
Then
_resolve_record_databecomes more linear and easier to read:2. Extract base64 audio detection
The inline “mini format sniffer” can be encapsulated into a single helper. That keeps
_has_direct_media_sourcereadable and prevents further format heuristics from bloating_resolve_record_data:If you already have or can add
media_utils, this helper can be moved there, and_has_direct_media_sourcecan call something likeis_base64_audio(file_ref)instead.3. Extract the
get_recordcall / post-processingThe call to
self.bot.call_actionand its result handling can be factored out, isolating routing/bot coupling:This preserves all existing behavior (including logging and fallbacks) while making
_resolve_record_dataa thin orchestrator, which addresses the complexity concerns without reverting your feature.