Repository navigation
Python: Support structured MIME subtypes in data URIs - #14304
Zhewen Tan (tandede) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Expands the Data URI parsing logic to accept MIME subtypes containing structured syntax suffixes and vendor-tree subtypes (e.g., image/svg+xml, application/vnd.api+json).
Changes:
- Added unit tests covering MIME subtypes with
+suffixand vendor subtypes. - Relaxed the MIME subtype regex to allow additional valid characters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/tests/unit/contents/test_data_uri.py | Adds regression tests for structured-suffix and vendor-subtype Data URIs. |
| python/semantic_kernel/contents/utils/data_uri.py | Updates the MIME type regex to accept richer subtype formats. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pattern = "(((?P<mime_type>[a-zA-Z]+/[a-zA-Z0-9!#$&^_.+-]+)(?P<parameters>(;[a-zA-Z0-9]+=+[a-zA-Z0-9]+)*))?(;+(?P<data_format>.*)))?(,(?P<data_str>.*))" # noqa: E501 | ||
| match = re.match(pattern, data) |
| pattern = "(((?P<mime_type>[a-zA-Z]+/[a-zA-Z0-9!#$&^_.+-]+)(?P<parameters>(;[a-zA-Z0-9]+=+[a-zA-Z0-9]+)*))?(;+(?P<data_format>.*)))?(,(?P<data_str>.*))" # noqa: E501 | ||
| match = re.match(pattern, data) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 861112f49281
Model: claude-opus-4.8
Overview
This PR makes a single-line change at python/semantic_kernel/contents/utils/data_uri.py:138, widening the
MIME subtype regex character class from [a-zA-Z-]+ to [a-zA-Z0-9!#$&^_.+-]+ so registered structured
subtypes like image/svg+xml and application/vnd.api+json parse, plus two matching regression tests. The
change is strictly additive: the new class excludes the structural delimiters / ; , =, so no previously
accepted input parses differently and no new backtracking ambiguity is created. All nine existing negative
tests still raise, the full test_data_uri.py suite passes, and downstream consumers (BinaryContent,
ImageContent, AudioContent) receive mime_type through the unchanged to_string()/to_dict() paths.
The residual ReDoS and RFC-token-completeness concerns are pre-existing, not worsened by this PR, and are
already captured in the open reviewer feedback threads; no publishable defect remains.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
|
I’ve synced this branch with the latest |
GitHub Actions
Failure
GitHub Actions
Failure
…oding is set (microsoft#14464) ### What this fixes `ChatMessageContent.to_prompt()` is annotated `-> str` but returns `bytes` for any message that carries an `encoding`. ```python root = self.to_element() return ElementTree.tostring(root, encoding=self.encoding or "unicode", short_empty_elements=False) ``` `encoding` is a real field on the content, so the `or` never protects it: setting `encoding="utf-8"` makes `tostring` take its bytes branch. Measured on `ca40aa72`: ``` encoding='utf-8' -> type=bytes value=b'<message role="user" encoding="utf-8">...' encoding='ascii' -> b'<?xml ...?>\n<message ...' # XML declaration too ``` This is user-visible rather than merely untidy: `jinja2_system_helpers._message_to_prompt` and its handlebars twin render a context variable via `str(context.to_prompt())`, so the prompt the model receives literally contains `b'<message role="user" ...>'` — bytes repr plus quote noise — whenever the message had an encoding set. Every other `tostring` call site in the same package already hardcodes `encoding="unicode"`: `chat_history.py:285`, `:317`, `:362`, `chat_message_content.py:275`, `handlebars_system_helpers.py:37`, `:55` and `jinja2_system_helpers.py:29`. This one line was the outlier, so the change aligns it rather than introducing a new convention. The message's own encoding is still preserved: it is an attribute on the element and round-trips through `to_element`/`from_element`; it describes the text content, not how we serialize a prompt string. ### Change One line, plus a docstring note, and a parametrized regression test over `utf-8`, `ascii` and `latin-1` asserting the prompt is a `str`, has no XML declaration, keeps the non-ASCII text, and re-parses to the same content. ### How this was verified `cd /private/tmp/sk/tree/python`, `PYTHONPATH=.`, the repo's own test file `tests/unit/contents/test_chat_message_content.py`. Base control — `semantic_kernel/contents/chat_message_content.py` restored from `ca40aa7226531d28a721d0ca0e451d0aaf86dafc` with `git diff -- <that file>` printing nothing (so the run measures upstream code with the new tests kept), sha re-echoed: ``` FAILED tests/unit/contents/test_chat_message_content.py::test_cmc_to_prompt_with_encoding[utf8] FAILED tests/unit/contents/test_chat_message_content.py::test_cmc_to_prompt_with_encoding[ascii] FAILED tests/unit/contents/test_chat_message_content.py::test_cmc_to_prompt_with_encoding[latin1] 3 failed, 43 passed ``` With the patch applied: `46 passed`. Enclosing directory, both revisions, same command (`pytest tests/unit/contents`): the `test_chat_history.py::test_to_from_file` / `::test_chat_history_serialize` collection errors appear **identically at base and at head** — they come from optional extras missing in my environment, not from this diff. I did not run the integration or dotnet suites. `ruff check` on the two files reports only a pre-existing `D` finding at `chat_message_content.py:201`, present at base too; `ruff format --check` is clean. (`mypy` was not installed in this environment, so the CI type gate is unverified here.) Dedup: searched issues and PRs (all states) for `to_prompt`, `message_to_prompt` and `encoding unicode prompt`; the only hits are merged history (microsoft#13738, microsoft#5550). The nearby `data_uri` MIME-subtype defect I found while investigating is already claimed by open microsoft#14304, so it is deliberately not part of this PR.
Motivation and Context
DataUri.from_data_uri()rejects valid MIME subtypes containing structured syntax suffixes or vendor facets because its parser only accepts letters and hyphens. This prevents common values such asimage/svg+xmlandapplication/vnd.api+jsonfrom being used in data URIs.Description
Expand the MIME subtype character class to accept digits and the punctuation allowed in registered subtype names. Add regression cases covering SVG and vendor JSON data URIs, including MIME type preservation and payload decoding.
Validation:
Contribution Checklist