Repository navigation
Python: Decode percent-encoded base64 data URI payloads - #9664
Zhang Bowen (MuonChaser) wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
GitHub Actions
Error
agent-framework-automation
Bot
added
the
python
GitHub Actions
Inactive
|
@microsoft-github-policy-service agree |
GitHub Actions
Inactive
GitHub Actions
Inactive
BlueRaddish
left a comment
BlueRaddish
left a comment
There was a problem hiding this comment.
Independent offline verification of 532e37bc2932764c303f0ac335d1ec1744f99151 adds serialized-message coverage beyond the PNG cases: Linux / CPython 3.11 job.
I exercised public Content.from_uri() → Message.to_dict() → ordinary JSON encode/decode → Message.from_dict(), then public media detection and the existing binary extraction helpers. Across PNG, JPEG, GIF and PDF, both fully percent-escaped and mixed literal/escaped payloads preserve the original URI, media type and decoded bytes. Literal + remains literal in the mixed cases; metadata containing %2F stays unchanged in the stored URI.
- Restoring only
_types.pyto the PR's original merge base9c8a3514a15a702f81bfda456d80c7ccaa4d2712produces 8 genuine invalid-base64 failures and 4 passing literal-URI controls. The same 12 cases all pass on this head. - The complete
packages/core/tests/core/test_types.pypasses: 508 tests, zero skips. - The normal changed-file
prekchecks pass, with 4 nonapplicable hook skips and no tracked source or configuration changes afterward.
This is a focused factual COMMENT, not whole-PR approval. No live provider/model calls, other OS/Python lanes, complete workspace tests or distribution build were run by this review. The contributor's source/branch and PR description remain unchanged, and no competing fix was opened. AI assistance was used for the independent probes and this report.
|
BlueRaddish Thanks for the additional serialized-message coverage and baseline comparison. I checked the linked |
|
/review |
![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: Findings reported
Scope: full PR (1 commit(s)): 532e37bc2932
Model: gpt-5.6-sol
Overview
The change centralizes one-time payload percent-decoding in the existing data-URI parser and adds strong regressions for escaped base64 characters, nested escapes, metadata isolation, URI preservation, and decoder strictness. Those guards establish the intended decoding semantics, but eager decoding on validation-only paths creates meaningful memory amplification, and retaining the escaped URI leaves providers and transports that bypass the shared helpers inconsistent with the newly supported representation.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_types.py
| media_type=parts[0][5:] or "text/plain", # Remove 'data:' | ||
| is_base64=bool(parameters) and parameters[-1] == "base64", | ||
| payload=payload, | ||
| payload=unquote(payload), |
There was a problem hiding this comment.
Calling unquote in the shared parser also runs during Content.from_uri() validation, even though _validate_uri discards payload. A percent-heavy URI can make unquote peak at roughly 18x the input size (for example, 3 MB produced about 56 MB of transient allocation), and the Responses hosting parser accepts request-owned input_image values without a length guard, so concurrent requests can exhaust process memory before any provider handles the image. Please avoid decoding on validation-only paths and enforce a payload limit before percent-decoding in consumers that need the payload.
There was a problem hiding this comment.
Confirmed offline: Content.from_uri() performs payload decoding on the validation-only path even though that path only needs metadata. Decoding should be deferred to consumers that need the payload; a new size-limit policy needs maintainer agreement. Implementation was handed over to @ktz03, so I am leaving this finding open for the replacement PR rather than expanding #9664 in parallel.
| media_type=parts[0][5:] or "text/plain", # Remove 'data:' | ||
| is_base64=bool(parameters) and parameters[-1] == "base64", | ||
| payload=payload, | ||
| payload=unquote(payload), |
There was a problem hiding this comment.
This normalizes only _parse_data_uri's returned payload while Content.from_uri() intentionally retains the escaped URI. Consumers that use the shared helpers now accept escaped base64, but Ollama, A2A, and the hosting MCP/A2A adapters parse content.uri themselves and still strict-decode or reject %, so the same valid Content succeeds or fails by provider or transport. Please preserve URI storage while routing every data-content consumer through a canonical one-time-decoding accessor, and add parity coverage for these paths.
There was a problem hiding this comment.
Confirmed with the same literal/escaped PNG pair: core detection and byte extraction accept both, while Ollama's image decoder, the A2A URI accessor, and hosting MCP/A2A conversion reject the escaped form. This PR's helper fix does not establish adapter-wide parity. Since implementation was handed over to LI (@ktz03), these consumer paths and parity tests should be coordinated in the replacement scope with the core team. No provider or transport code has been changed here; this thread remains open.
Motivation & Context
Equivalent literal and percent-encoded base64 data URI payloads currently produce different results: the literal PNG is detected and extracted, while its escaped form fails. Percent-decoding the payload before base64 processing restores their equivalence, as required by the Fetch data URL processor.
Description & Review Guide
unquote, which preserves literal+. The extraction helper continues to return a base64 string. Add offline regressions through public media detection and Content construction, checking decoded bytes, escaped/,+,=, lowercase escapes, raw inputs, nested escapes, metadata and errors.uv run poe test -A -m "not integration" -n 4) also passed on Linux / Python 3.12.3: 19,158 passed, 551 skipped, 2 xfailed, with 790 warnings and no failures or errors. This follows the repository default selection, excluding integration, DevUI and Lab. Live provider calls and the other Python/OS CI matrix lanes were not run locally.+survives while%252Fremains%2Fafter one decode.Related Issue
Fixes #9653.
Trivial-change exception: this is an obvious correction to the existing base64 data URI processing order, restoring equality for two representations of the same payload. It introduces no API, URI mode, decoder permissiveness, or provider-specific transformation. No maintainer agreement is claimed. Pre-publication all-state searches found no competing PR; implementation is coordinated in #9653.
AI Assistance
AI assistance details: Reproduction, implementation, regression tests, local validation and PR description.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.