fix: sanitize unsupported image MIME types (e.g. GIF) in context cleaner - #9335
fix: sanitize unsupported image MIME types (e.g. GIF) in context cleaner#9335xiaoyuyu6420 wants to merge 3 commits into
Conversation
dosubot
Bot
added
size:M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_extract_image_mime, consider usingurllib.parse.urlsplit(or similar) instead of manualsplit('?', '#')to make URL path handling more robust for edge cases like encoded?/#in the path. - Given that
_UNSUPPORTED_IMAGE_MIMESis meant as a safety net for multiple gateways, you might want to expose a simple extension point (e.g., optional config injection or an overridable helper) so providers that truly support GIF can opt out without code changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_extract_image_mime`, consider using `urllib.parse.urlsplit` (or similar) instead of manual `split('?', '#')` to make URL path handling more robust for edge cases like encoded `?`/`#` in the path.
- Given that `_UNSUPPORTED_IMAGE_MIMES` is meant as a safety net for multiple gateways, you might want to expose a simple extension point (e.g., optional config injection or an overridable helper) so providers that truly support GIF can opt out without code changes.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/modalities.py" line_range="85-93" />
<code_context>
+ if path.endswith(ext):
+ return mime
+
+ source = part.get("source")
+ if isinstance(source, dict):
+ media_type = source.get("media_type")
+ if isinstance(media_type, str):
+ return media_type.lower()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Normalize `media_type` by stripping parameters (e.g. `image/gif; charset=binary`) before comparison.
In the current branch, `media_type` is lowercased but not stripped of parameters. If an API returns values like `image/gif; charset=binary` or `image/jpeg;quality=80`, `_is_unsupported_image_mime` will not match entries such as `image/gif` in `_UNSUPPORTED_IMAGE_MIMES`.
To align with the data-URL handling and make matching robust across providers, normalize by stripping parameters before lowercasing, e.g. `media_type.split(";", 1)[0].strip().lower()`.
```suggestion
source = part.get("source")
if isinstance(source, dict):
media_type = source.get("media_type")
if isinstance(media_type, str):
# Normalize media type by stripping parameters before comparison,
# e.g. "image/gif; charset=binary" -> "image/gif"
return media_type.split(";", 1)[0].strip().lower()
mime_type = part.get("mimeType") or part.get("mime_type")
if isinstance(mime_type, str):
# Normalize MIME type by stripping parameters before comparison
return mime_type.split(";", 1)[0].strip().lower()
```
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/modalities.py" line_range="91-93" />
<code_context>
+ if isinstance(media_type, str):
+ return media_type.lower()
+
+ mime_type = part.get("mimeType") or part.get("mime_type")
+ if isinstance(mime_type, str):
+ return mime_type.lower()
+
</code_context>
<issue_to_address>
**suggestion:** Consider stripping parameters from `mimeType`/`mime_type` as well to align with other MIME parsing.
Because `mimeType`/`mime_type` is returned as `mime_type.lower()`, a value like `image/gif;codec=xyz` won’t match `_UNSUPPORTED_IMAGE_MIMES` even though the base type is `image/gif`. Normalizing it with `split(";", 1)[0].strip().lower()` would align this path with data URLs and ensure unsupported image types are correctly detected.
```suggestion
mime_type = part.get("mimeType") or part.get("mime_type")
if isinstance(mime_type, str):
# Strip any parameters (e.g. "image/gif;codec=xyz" → "image/gif")
# to align with other MIME parsing and ensure unsupported types are detected.
return mime_type.split(";", 1)[0].strip().lower()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter out unsupported image MIME types (specifically image/gif) during context sanitization, preventing issues with gateways (such as Gemini-compatible proxies) that reject them even when the model claims image support. It includes robust extraction of MIME types from data URLs, path extensions, and various payload structures, accompanied by comprehensive unit tests. The review feedback suggests enhancing robustness by stripping leading and trailing whitespace from URLs, media types, and MIME types before processing.
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.
| if isinstance(url, str): | ||
| # data URLs look like "data:image/gif;base64,...." | ||
| if url.lower().startswith("data:"): |
There was a problem hiding this comment.
To make the MIME extraction more robust against leading or trailing whitespace in URLs (which can occasionally occur in raw or user-submitted contexts), consider stripping the url string before processing.
| if isinstance(url, str): | |
| # data URLs look like "data:image/gif;base64,...." | |
| if url.lower().startswith("data:"): | |
| if isinstance(url, str): | |
| url = url.strip() | |
| # data URLs look like "data:image/gif;base64,...." | |
| if url.lower().startswith("data:"): |
| source = part.get("source") | ||
| if isinstance(source, dict): | ||
| media_type = source.get("media_type") | ||
| if isinstance(media_type, str): | ||
| return media_type.lower() | ||
|
|
||
| mime_type = part.get("mimeType") or part.get("mime_type") | ||
| if isinstance(mime_type, str): | ||
| return mime_type.lower() |
There was a problem hiding this comment.
Similar to how the data URL MIME extraction strips whitespace (on line 75), it is safer to strip leading/trailing whitespace from media_type and mime_type to ensure robust matching against _UNSUPPORTED_IMAGE_MIMES.
| source = part.get("source") | |
| if isinstance(source, dict): | |
| media_type = source.get("media_type") | |
| if isinstance(media_type, str): | |
| return media_type.lower() | |
| mime_type = part.get("mimeType") or part.get("mime_type") | |
| if isinstance(mime_type, str): | |
| return mime_type.lower() | |
| source = part.get("source") | |
| if isinstance(source, dict): | |
| media_type = source.get("media_type") | |
| if isinstance(media_type, str): | |
| return media_type.strip().lower() | |
| mime_type = part.get("mimeType") or part.get("mime_type") | |
| if isinstance(mime_type, str): | |
| return mime_type.strip().lower() |
Address Sourcery/Gemini review feedback on AstrBotDevs#9335 without adding new abstractions. - Strip parameters (e.g. "image/gif; charset=binary") and whitespace from source.media_type and mimeType/mime_type before comparing against _UNSUPPORTED_IMAGE_MIMES, so parameterized GIF values are still detected. - Strip surrounding whitespace from image URLs defensively. - Switch the http(s) URL path extraction from a manual split('?', '#') to urllib.parse.urlsplit, which keeps percent-encoded delimiters inside the path and is more robust for edge cases. - Inline the normalization at both call sites instead of introducing a helper, keeping it consistent with the existing data-URL branch and honoring the repo's No-Unnecessary-Helpers rule. Adds 5 unit tests covering parameterized media_type/mimeType, the encoded path case, and a non-GIF-with-params regression guard.
|
Pushed an iteration addressing the review feedback. Summary of decisions: Adopted
Not adopted — extension point / config injection for Note on scope (issue #9295)
If maintainers want either, I can follow up in a separate PR rather than expand this one. Tests: 16 passed (11 existing + 5 new). |
|
这样实现会不会太粗暴了，直接拒绝了所有模型使用 GIF （虽然我也不知道哪个模型支持 GIF）。 This implementation might be too heavy-handed as it completely blocks GIFs for all models (unclear if any models even support GIFs). |
…e_mimes) Replaces the hardcoded GIF blocklist with a per-provider whitelist, addressing feedback that the global blocklist was too heavy-handed. Changes: - Add 'supported_image_mimes' config field (checkbox: JPEG/PNG/WebP/GIF) in provider settings. Unchecked = fallback blocklist (GIF filtered on known- problematic gateways). Checked = strict whitelist mode. - sanitize_contexts_by_modalities() now accepts supported_image_mimes param. - When whitelist is provided, only images with MIME types in the list are kept. - When whitelist is None/empty, existing GIF blocklist is used as fallback (backward compatible, preserves AstrBotDevs#9295 fix). - Schema-driven UI: WebUI auto-generates the checkbox group from config schema. - Update i18n (en-US, zh-CN) for the new field. - Add 4 tests: whitelist filtering, whitelist includes GIF, None fallback, empty-list fallback. Fixes suggestion from @sujoshua and Sourcery review on AstrBotDevs#9335. Tests: 20 passed (16 existing + 4 new whitelist tests).
dosubot
Bot
added
size:L
|
Hi @sujoshua, thanks for the feedback! I agree the hardcoded blocklist was too blunt. I just pushed an update that adds a configurable image MIME whitelist ( Behavior:
The WebUI auto-generates a checkbox group from the schema, so no Vue changes were needed. Would love your thoughts on this approach! |
dfb9c7a to
b0bf6f7
Compare
Address Sourcery/Gemini review feedback on AstrBotDevs#9335 without adding new abstractions. - Strip parameters (e.g. "image/gif; charset=binary") and whitespace from source.media_type and mimeType/mime_type before comparing against _UNSUPPORTED_IMAGE_MIMES, so parameterized GIF values are still detected. - Strip surrounding whitespace from image URLs defensively. - Switch the http(s) URL path extraction from a manual split('?', '#') to urllib.parse.urlsplit, which keeps percent-encoded delimiters inside the path and is more robust for edge cases. - Inline the normalization at both call sites instead of introducing a helper, keeping it consistent with the existing data-URL branch and honoring the repo's No-Unnecessary-Helpers rule. Adds 5 unit tests covering parameterized media_type/mimeType, the encoded path case, and a non-GIF-with-params regression guard.
…e_mimes) Replaces the hardcoded GIF blocklist with a per-provider whitelist, addressing feedback that the global blocklist was too heavy-handed. Changes: - Add 'supported_image_mimes' config field (checkbox: JPEG/PNG/WebP/GIF) in provider settings. Unchecked = fallback blocklist (GIF filtered on known- problematic gateways). Checked = strict whitelist mode. - sanitize_contexts_by_modalities() now accepts supported_image_mimes param. - When whitelist is provided, only images with MIME types in the list are kept. - When whitelist is None/empty, existing GIF blocklist is used as fallback (backward compatible, preserves AstrBotDevs#9295 fix). - Schema-driven UI: WebUI auto-generates the checkbox group from config schema. - Update i18n (en-US, zh-CN) for the new field. - Add 4 tests: whitelist filtering, whitelist includes GIF, None fallback, empty-list fallback. Fixes suggestion from @sujoshua and Sourcery review on AstrBotDevs#9335. Tests: 20 passed (16 existing + 4 new whitelist tests).
Fixes AstrBotDevs#9295. When a provider declares image modality support, the context sanitizer previously kept every image block as-is. Animated GIFs can then be persisted into session history and later cause continuous failures on OpenAI-compatible Gemini gateways that reject image/gif. - Drop image/gif data URLs and .gif http(s) URLs to a [Image] placeholder even when the provider supports image modality. - Keep the existing full image-strip path when image modality is absent. - Preserve JPEG/PNG/WebP and unknown-MIME images when image is supported. - Add unit coverage for the reporter scenario and related edge cases.
Address Sourcery/Gemini review feedback on AstrBotDevs#9335 without adding new abstractions. - Strip parameters (e.g. "image/gif; charset=binary") and whitespace from source.media_type and mimeType/mime_type before comparing against _UNSUPPORTED_IMAGE_MIMES, so parameterized GIF values are still detected. - Strip surrounding whitespace from image URLs defensively. - Switch the http(s) URL path extraction from a manual split('?', '#') to urllib.parse.urlsplit, which keeps percent-encoded delimiters inside the path and is more robust for edge cases. - Inline the normalization at both call sites instead of introducing a helper, keeping it consistent with the existing data-URL branch and honoring the repo's No-Unnecessary-Helpers rule. Adds 5 unit tests covering parameterized media_type/mimeType, the encoded path case, and a non-GIF-with-params regression guard.
…e_mimes) Replaces the hardcoded GIF blocklist with a per-provider whitelist, addressing feedback that the global blocklist was too heavy-handed. Changes: - Add 'supported_image_mimes' config field (checkbox: JPEG/PNG/WebP/GIF) in provider settings. Unchecked = fallback blocklist (GIF filtered on known- problematic gateways). Checked = strict whitelist mode. - sanitize_contexts_by_modalities() now accepts supported_image_mimes param. - When whitelist is provided, only images with MIME types in the list are kept. - When whitelist is None/empty, existing GIF blocklist is used as fallback (backward compatible, preserves AstrBotDevs#9295 fix). - Schema-driven UI: WebUI auto-generates the checkbox group from config schema. - Update i18n (en-US, zh-CN) for the new field. - Add 4 tests: whitelist filtering, whitelist includes GIF, None fallback, empty-list fallback. Fixes suggestion from @sujoshua and Sourcery review on AstrBotDevs#9335. Tests: 20 passed (16 existing + 4 new whitelist tests).
b0bf6f7 to
b7e898a
Compare
Rebased onto current masterThe CI checks on this PR had gone red on July 29 (the run logs have since expired, so the original failure is no longer inspectable), and the branch had fallen 51 commits behind Local verification after rebase
What changed in the rebase The feature commits replayed cleanly. The The new push should re-trigger the workflow runs for fresh CI results. |
|
Friendly ping @Soulter — rebased onto current |
2 participants
Issue / 问题
Fixes #9295.
When a quoted animated GIF is added to conversation history, it can be
persisted as
data:image/gif;base64,.... Some OpenAI-compatible Geminigateways declare
imagemodality support but rejectimage/gif, so everylater request fails with
mime type is not supported/convert_request_faileduntil the session history is cleared.sanitize_contexts_by_modalities()previously only checked coarse modalityflags (
image/audio/tool_use) and short-circuited when all three wereenabled, so GIF MIME cleanup never ran for the reporter's configuration.
Modifications / 改动点
Keep a narrow unsupported MIME list (
image/gif) inastrbot/core/provider/modalities.py.Extract image MIME best-effort from OpenAI-style
image_urldata URLs and.gifHTTP URLs.Replace unsupported GIF image blocks with
[Image]even when the providerclaims image support.
Preserve existing full image stripping when image modality is absent, and
preserve JPEG/PNG/WebP / unknown-MIME images when image is supported.
Add unit tests for the reporter scenario and related edge cases.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Results:
Trade-off note
This intentionally uses a narrow hard-coded MIME blacklist (
image/gif)instead of a new provider config switch, to keep the fix minimal and focused
on the reported failure mode. Providers that truly accept GIF will lose GIF
image blocks and receive a text placeholder instead. If maintainers prefer a
configurable MIME whitelist/blacklist, that can be layered on later without
reverting this safety net.
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new warnings or errors are introduced.
/ 我已确保没有引入新的警告或错误。
Summary by Sourcery
Sanitize unsupported image MIME types during context processing to prevent rejected GIFs from causing subsequent provider requests to fail.
New Features:
Bug Fixes:
Enhancements:
Tests: