mirror of
https://github.com/AstrBotDevs/AstrBot.git
synced 2026-10-16 15:06:04 +08:00
Fix/reject unresolvable image refs (#10434)
* fix: reject unresolvable image references instead of faking a local path A plugin can hand over a media reference such as `/api/v1/files/tokens/<token>`. Neither an existing file nor a valid payload, it was still returned as a local file, so `to_path()` resolved it into a path that does not exist on disk (on Windows `C:\api\v1\files\tokens\<token>`). That fabricated path reached `req.image_urls` and the prompt, and the real failure only surfaced later as a warning while preparing model images. `_materialize_media_ref()` now enforces its own contract: - A reference that exists but is not a regular file (directory, FIFO, socket, device) raises ValueError for every media family. - Image references must resolve to an existing file, URL or base64 payload. Images are read right after resolution, so a missing source is a failure rather than a file that may appear later. - Audio, video and generic file references keep returning a local path that does not exist yet: `ensure_wav()` and the STT retry loop rely on that for platform races such as NapCat handing over a voice path before it is readable. Supporting changes: - `to_base64_data(strict=False)` still degrades to `None` when an image ref cannot be materialized, which payload assembly relies on to skip bad refs. - `describe_media_ref()` reports unresolved refs as such instead of calling them a local media path. - Unavailable image warnings now include the original reference. Refs #10264 * fix: keep pathlib tolerance for over-long and non-regular media refs `_materialize_media_ref()` replaced `path.exists()` with a bare `path.stat()` and caught only OSError. pathlib's `exists()` also swallows ValueError, which Windows raises for paths past the 32767 character limit and for embedded NUL bytes. A bare base64 payload longer than that limit failed with `ValueError: stat: path too long for Windows` instead of being decoded, and audio/video/file references lost the pending-path behaviour for the same input. `to_base64_data()` had the matching gap: it caught only OSError around resolution, so a directory or empty reference escaped as ValueError even in non-strict mode. Payload assembly (openai/anthropic/gemini sources) relies on `None` there to skip one bad image instead of failing the whole request, and `is_recoverable_image_error()` already classifies ValueError as recoverable. Both sites now catch what pathlib and the module's own recovery predicate treat as a missing or unusable local file. Regression tests cover an oversized bare base64 image, an oversized pending audio path, and directory/empty references through the non-strict image path. * docs: describe the media reference error contract precisely Review feedback on the pull request: - `_materialize_media_ref()` only raises FileNotFoundError for a missing file URI when the reference is an image. Audio, video and file references return the pending path instead, which the `Raises:` section now states instead of reading as if it applied to every media family. - `Image.convert_to_file_path()` can raise ValueError as well: for an existing non-regular path, and already for an empty url/file before this branch. Docstrings only, no behaviour change.
This commit is contained in:
@@ -114,6 +114,7 @@ from astrbot.core.utils.file_extract import extract_file_moonshotai
|
||||
from astrbot.core.utils.image_input import prepare_request_images
|
||||
from astrbot.core.utils.llm_metadata import LLM_METADATAS
|
||||
from astrbot.core.utils.media_utils import (
|
||||
describe_media_ref,
|
||||
is_file_uri,
|
||||
is_recoverable_image_error,
|
||||
normalize_model_image_max_size,
|
||||
@@ -1503,7 +1504,9 @@ async def collect_initial_request(
|
||||
if not is_recoverable_image_error(exc):
|
||||
raise
|
||||
logger.warning(
|
||||
"Image attachment is unavailable (%s).", type(exc).__name__
|
||||
"Image attachment is unavailable (%s): %s",
|
||||
type(exc).__name__,
|
||||
describe_media_ref(comp.url or comp.file),
|
||||
)
|
||||
req.extra_user_content_parts.append(
|
||||
TextPart(text="[Image unavailable]")
|
||||
@@ -1567,8 +1570,11 @@ async def collect_initial_request(
|
||||
if not is_recoverable_image_error(exc):
|
||||
raise
|
||||
logger.warning(
|
||||
"Quoted image is unavailable (%s).",
|
||||
"Quoted image is unavailable (%s): %s",
|
||||
type(exc).__name__,
|
||||
describe_media_ref(
|
||||
reply_comp.url or reply_comp.file
|
||||
),
|
||||
)
|
||||
req.extra_user_content_parts.append(
|
||||
TextPart(text="[Image unavailable]")
|
||||
|
||||
@@ -538,6 +538,12 @@ class Image(BaseMessageComponent):
|
||||
Returns:
|
||||
str: 图片的本地路径,以绝对路径表示。
|
||||
|
||||
Raises:
|
||||
FileNotFoundError: 图片引用既不是存在的本地文件,也不是可下载的 URL
|
||||
或合法的 base64 数据。
|
||||
ValueError: 图片引用指向已存在但非普通文件的路径(例如目录),
|
||||
或者 url/file 均为空。
|
||||
|
||||
"""
|
||||
url = self.url or self.file
|
||||
if not url:
|
||||
|
||||
@@ -37,6 +37,7 @@ from astrbot.core.provider.entities import (
|
||||
)
|
||||
from astrbot.core.star.star_handler import EventType
|
||||
from astrbot.core.utils.config_number import coerce_int_config
|
||||
from astrbot.core.utils.media_utils import describe_media_ref
|
||||
from astrbot.core.utils.metrics import Metric
|
||||
|
||||
from .....astr_agent_context import AgentContextWrapper, AstrAgentContext
|
||||
@@ -299,8 +300,9 @@ class ThirdPartyAgentSubStage(Stage):
|
||||
image_path = await comp.convert_to_base64()
|
||||
except Exception as exc: # noqa: BLE001
|
||||
logger.warning(
|
||||
"Image attachment is unavailable (%s).",
|
||||
"Image attachment is unavailable (%s): %s",
|
||||
type(exc).__name__,
|
||||
describe_media_ref(comp.url or comp.file),
|
||||
)
|
||||
continue
|
||||
req.image_urls.append(image_path)
|
||||
|
||||
@@ -13,6 +13,7 @@ import math
|
||||
import mimetypes
|
||||
import os
|
||||
import shutil
|
||||
import stat
|
||||
import struct
|
||||
import subprocess
|
||||
import tempfile
|
||||
@@ -377,6 +378,9 @@ def describe_media_ref(media_ref: object | None) -> str:
|
||||
else:
|
||||
return f"bare base64 media payload_len={len(compact)}"
|
||||
|
||||
if not media_path_exists:
|
||||
return f"unresolved media ref name={Path(media_ref).name!r} len={ref_len}"
|
||||
|
||||
return f"local media path name={Path(media_ref).name!r} len={ref_len}"
|
||||
|
||||
|
||||
@@ -467,10 +471,23 @@ async def _materialize_media_ref(
|
||||
Supported references: local paths, file:// URIs, http(s) URLs, base64://,
|
||||
data:*;base64,... URIs, and legacy bare base64 payloads.
|
||||
|
||||
Image references are read as soon as they are resolved, so an image must
|
||||
resolve to a real file. Audio, video and generic file references may point to
|
||||
a local path that does not exist yet: platforms such as NapCat can hand over a
|
||||
path before the file is readable, and those callers convert or retry later.
|
||||
|
||||
Args:
|
||||
media_ref: Original media reference from a platform, plugin, or history.
|
||||
media_type: Logical media family used for temp filenames and defaults.
|
||||
default_suffix: Suffix to use when the reference does not carry one.
|
||||
|
||||
Raises:
|
||||
FileNotFoundError: If an image reference is neither an existing file nor a
|
||||
supported URL or base64 payload, or if an image file URI does not
|
||||
exist. Audio, video and file references do not raise for a missing
|
||||
path; they return it so the caller can retry later.
|
||||
ValueError: If the reference points to an existing path that is not a
|
||||
regular file.
|
||||
"""
|
||||
|
||||
cleanup_paths: list[Path] = []
|
||||
@@ -511,6 +528,20 @@ async def _materialize_media_ref(
|
||||
|
||||
if is_file_uri(media_ref):
|
||||
path = Path(file_uri_to_path(media_ref))
|
||||
try:
|
||||
file_stat = path.stat()
|
||||
except (OSError, ValueError):
|
||||
# Mirror pathlib's exists(): Windows raises ValueError for over-long
|
||||
# paths and embedded NUL bytes instead of an OSError.
|
||||
file_stat = None
|
||||
if file_stat is not None and not stat.S_ISREG(file_stat.st_mode):
|
||||
raise ValueError(f"media reference is not a regular file: {media_ref}")
|
||||
if file_stat is None and media_type == "image":
|
||||
raise FileNotFoundError(
|
||||
errno.ENOENT,
|
||||
"image file URI does not point to an existing file",
|
||||
media_ref,
|
||||
)
|
||||
return _LocalMediaFile(path=path, mime_type=_guess_mime_type(path))
|
||||
|
||||
if media_ref.startswith("data:"):
|
||||
@@ -564,12 +595,15 @@ async def _materialize_media_ref(
|
||||
)
|
||||
|
||||
path = Path(media_ref)
|
||||
path_exists = False
|
||||
try:
|
||||
path_exists = path.exists()
|
||||
except OSError:
|
||||
pass
|
||||
if path_exists:
|
||||
local_stat = path.stat()
|
||||
except (OSError, ValueError):
|
||||
# Mirror pathlib's exists(): Windows raises ValueError for over-long
|
||||
# paths and embedded NUL bytes instead of an OSError.
|
||||
local_stat = None
|
||||
if local_stat is not None:
|
||||
if not stat.S_ISREG(local_stat.st_mode):
|
||||
raise ValueError(f"media reference is not a regular file: {media_ref}")
|
||||
return _LocalMediaFile(path=path, mime_type=_guess_mime_type(path))
|
||||
|
||||
compact_media_ref = "".join(media_ref.split())
|
||||
@@ -604,6 +638,19 @@ async def _materialize_media_ref(
|
||||
cleanup_paths=cleanup_paths,
|
||||
)
|
||||
|
||||
if media_type == "image":
|
||||
# Images are read right after resolution, so a missing source is a
|
||||
# failure instead of a file that may appear later.
|
||||
raise FileNotFoundError(
|
||||
errno.ENOENT,
|
||||
"image reference is not an existing local file or a valid payload",
|
||||
media_ref,
|
||||
)
|
||||
|
||||
logger.debug(
|
||||
"Media reference does not exist locally yet; returning it for the caller: %s",
|
||||
describe_media_ref(media_ref),
|
||||
)
|
||||
return _LocalMediaFile(path=path, mime_type=_guess_mime_type(path))
|
||||
|
||||
|
||||
@@ -813,7 +860,18 @@ class MediaResolver:
|
||||
whose bytes cannot be identified.
|
||||
"""
|
||||
if self.media_type == "image":
|
||||
async with self.as_path(target_format=target_format) as resolved:
|
||||
# Resolve outside as_path(): an image reference that cannot be
|
||||
# materialized must degrade to None in non-strict mode, which payload
|
||||
# assembly relies on to skip bad image refs.
|
||||
try:
|
||||
resolved = await self._resolve_path(target_format=target_format)
|
||||
except (OSError, ValueError) as exc:
|
||||
# ValueError covers non-regular sources, which is_recoverable_image_error
|
||||
# already treats as skippable.
|
||||
if strict or not is_recoverable_image_error(exc):
|
||||
raise
|
||||
return None
|
||||
try:
|
||||
try:
|
||||
media_bytes = await asyncio.to_thread(resolved.read_bytes)
|
||||
except OSError as exc:
|
||||
@@ -859,6 +917,8 @@ class MediaResolver:
|
||||
base64_data=base64.b64encode(media_bytes).decode("utf-8"),
|
||||
mime_type=mime_type,
|
||||
)
|
||||
finally:
|
||||
resolved.cleanup()
|
||||
|
||||
async with self.as_path(
|
||||
target_format=target_format,
|
||||
|
||||
@@ -606,6 +606,154 @@ async def test_media_resolver_cleans_http_target_when_download_fails(
|
||||
assert not list(tmp_path.iterdir())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("media_type", ["image", "audio", "file"])
|
||||
async def test_media_resolver_rejects_directory_ref(tmp_path, media_type):
|
||||
with pytest.raises(ValueError, match="not a regular file"):
|
||||
await media_utils.MediaResolver(str(tmp_path), media_type=media_type).to_path()
|
||||
|
||||
with pytest.raises(ValueError, match="not a regular file"):
|
||||
await media_utils.MediaResolver(tmp_path.as_uri(), media_type=media_type).to_path()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_rejects_unresolved_image_url_path():
|
||||
ref = "/api/v1/files/tokens/ddbb1afd-84d9-4b1d-9b0e-5c7e6fe75684"
|
||||
|
||||
with pytest.raises(FileNotFoundError) as excinfo:
|
||||
await media_utils.MediaResolver(ref, media_type="image").to_path()
|
||||
|
||||
assert ref in str(excinfo.value)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_rejects_missing_image_file_uri(tmp_path):
|
||||
with pytest.raises(FileNotFoundError):
|
||||
await media_utils.MediaResolver(
|
||||
(tmp_path / "missing.png").as_uri(),
|
||||
media_type="image",
|
||||
).to_path()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("media_type", ["audio", "file"])
|
||||
async def test_media_resolver_keeps_pending_path_for_non_image(tmp_path, media_type):
|
||||
# Platforms such as NapCat can hand over a path before the file is readable.
|
||||
missing = str(tmp_path / "not-written-yet.amr")
|
||||
|
||||
resolved = await media_utils.MediaResolver(missing, media_type=media_type).to_path()
|
||||
|
||||
assert resolved == missing
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_record_keeps_pending_local_path_for_platform_races(tmp_path):
|
||||
# The STT stage retries this path until the platform finishes writing it.
|
||||
missing = tmp_path / "not-written-yet.amr"
|
||||
|
||||
resolved = await Record(file=str(missing)).convert_to_file_path()
|
||||
|
||||
assert resolved == str(missing)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_image_base64_data_degrades_for_unresolved_ref_when_not_strict():
|
||||
ref = "/api/v1/files/tokens/ddbb1afd-84d9-4b1d-9b0e-5c7e6fe75684"
|
||||
|
||||
assert await media_utils.resolve_image_ref_to_base64_data(ref) is None
|
||||
assert await media_utils.MediaResolver(ref, media_type="image").to_data_url() is None
|
||||
|
||||
with pytest.raises(FileNotFoundError):
|
||||
await media_utils.resolve_image_ref_to_base64_data(ref, strict=True)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("kind", ["directory", "empty"])
|
||||
async def test_image_base64_data_degrades_for_non_regular_ref_when_not_strict(
|
||||
tmp_path, kind
|
||||
):
|
||||
ref = str(tmp_path) if kind == "directory" else ""
|
||||
|
||||
assert await media_utils.resolve_image_ref_to_base64_data(ref) is None
|
||||
assert await media_utils.MediaResolver(ref, media_type="image").to_data_url() is None
|
||||
|
||||
with pytest.raises(ValueError):
|
||||
await media_utils.resolve_image_ref_to_base64_data(ref, strict=True)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_materializes_oversized_bare_base64_image(
|
||||
tmp_path, monkeypatch
|
||||
):
|
||||
monkeypatch.setattr(media_utils, "get_astrbot_temp_path", lambda: str(tmp_path))
|
||||
# Past the Windows path limit stat() raises ValueError while pathlib's
|
||||
# exists() reports False, so the payload must still be decoded and stored.
|
||||
payload = base64.b64encode(b"\x89PNG\r\n\x1a\n" + b"a" * (30 * 1024)).decode()
|
||||
assert len(payload) > 32767
|
||||
|
||||
resolved = await media_utils.MediaResolver(payload, media_type="image").to_path()
|
||||
|
||||
assert Path(resolved).read_bytes() == base64.b64decode(payload)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_keeps_oversized_pending_path_for_audio():
|
||||
# An over-long reference is not a usable path, so audio keeps retrying it.
|
||||
# to_path() additionally calls Path.resolve(), which rejects over-long
|
||||
# Windows paths on its own, so assert at the materialization layer.
|
||||
ref = "/api/v1/files/tokens/" + "a" * 40000
|
||||
|
||||
async with media_utils.MediaResolver(ref, media_type="audio").as_path() as resolved:
|
||||
assert resolved.path == Path(ref)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_prefers_existing_file_over_base64(tmp_path):
|
||||
# "abcd" is both a valid file name and a valid bare base64 payload.
|
||||
image_path = tmp_path / "abcd"
|
||||
image_path.write_bytes(b"not-really-an-image")
|
||||
|
||||
resolved = await media_utils.MediaResolver(
|
||||
str(image_path),
|
||||
media_type="image",
|
||||
).to_path()
|
||||
|
||||
assert resolved == str(image_path.resolve())
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_still_materializes_bare_base64_image(tmp_path, monkeypatch):
|
||||
monkeypatch.setattr(media_utils, "get_astrbot_temp_path", lambda: str(tmp_path))
|
||||
payload = base64.b64encode(b"abcd").decode()
|
||||
|
||||
resolved = await media_utils.MediaResolver(payload, media_type="image").to_path()
|
||||
|
||||
assert Path(resolved).read_bytes() == b"abcd"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_media_resolver_follows_symlink_to_regular_file(tmp_path):
|
||||
target = tmp_path / "target.png"
|
||||
target.write_bytes(b"png-bytes")
|
||||
link = tmp_path / "link.png"
|
||||
try:
|
||||
link.symlink_to(target)
|
||||
except (OSError, NotImplementedError):
|
||||
pytest.skip("symlinks are not available in this environment")
|
||||
|
||||
resolved = await media_utils.MediaResolver(str(link), media_type="image").to_path()
|
||||
|
||||
assert resolved == str(link.resolve())
|
||||
|
||||
|
||||
def test_describe_media_ref_marks_unresolved_ref():
|
||||
ref = "/api/v1/files/tokens/ddbb1afd-84d9-4b1d-9b0e-5c7e6fe75684"
|
||||
|
||||
assert media_utils.describe_media_ref(ref) == (
|
||||
f"unresolved media ref name={Path(ref).name!r} len={len(ref)}"
|
||||
)
|
||||
|
||||
|
||||
def test_describe_media_ref_does_not_include_payload_or_query():
|
||||
data_ref = "data:image/png;base64," + "A" * 128
|
||||
url_ref = "https://example.com/path/image.png?token=secret"
|
||||
|
||||
@@ -683,6 +683,36 @@ async def test_failed_images_keep_valid_input(harness, tmp_path, text, good):
|
||||
assert harness.captured[0].run_context.messages[-1].content
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("origin", ["component", "plugin"])
|
||||
async def test_unresolved_image_ref_is_reported_not_faked(harness, origin):
|
||||
ref = "/api/v1/files/tokens/ddbb1afd-84d9-4b1d-9b0e-5c7e6fe75684"
|
||||
event = make_event(text="")
|
||||
if origin == "component":
|
||||
event.message_obj.message = [Image(file=ref)]
|
||||
else:
|
||||
event.set_extra("provider_request", ProviderRequest(prompt="", image_urls=[ref]))
|
||||
|
||||
await process_event(harness, event, preprocess_first=True)
|
||||
|
||||
req = harness.captured[0].req
|
||||
payload = harness.provider.text_chat.await_args.kwargs["contexts"][-1]["content"]
|
||||
joined = "\n".join(
|
||||
part["text"] for part in payload if part["type"] == "text"
|
||||
)
|
||||
|
||||
assert req.image_urls == []
|
||||
assert str(Path(ref).resolve()) not in joined
|
||||
assert "local media path" not in joined
|
||||
if origin == "component":
|
||||
# The component keeps its own reference instead of a fabricated path.
|
||||
assert event.message_obj.message[0].file == ref
|
||||
assert "[Image unavailable]" in joined
|
||||
else:
|
||||
assert "unresolved media ref" in joined
|
||||
assert "skipped: image unavailable" in joined
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("origin", ["ordinary", "quote", "plugin", "hook"])
|
||||
@pytest.mark.parametrize("with_valid_image", [False, True])
|
||||
|
||||
Reference in New Issue
Block a user