Repository navigation
fix(sandbox): validate raster view_image content - #4450
sylvesterkaczmarek wants to merge 11 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6a6772f4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
changed the title
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Before we can merge it, please preserve the deliberately supported SVG path when valid SVG content begins with a UTF-8 BOM, XML comment, or DOCTYPE, and add focused regression coverage for those prolog forms. The current head rejects those files because removing the extension fallback leaves _detect_image_mime_type() recognizing only <svg or <?xml at the start. Please also remove the now-unused path parameter and its Path import.
The invalid-.png test covers the base failure. After these changes and green CI, this should be ready for another review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4269fc10e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the requested SVG compatibility cases and then simplified the implementation around the supported-format boundary. Raster formats with stable signatures now require byte evidence, while SVG/SVGZ retain the existing filename fallback so BOM/comment/DOCTYPE, UTF-16, and compressed SVG inputs stay compatible without adding an XML/decompression parser to media sniffing. Focused regression coverage includes the maintainer-requested prolog cases plus UTF-16 and SVGZ. The branch is mergeable; GitHub Actions remains gated before jobs are created. |
sylvesterkaczmarek
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@seratch The requested SVG compatibility coverage is addressed on the current head. I simplified the fix so signature-bearing raster formats require byte evidence, while SVG/SVGZ retain their established filename fallback for BOM/comment/DOCTYPE, UTF-16, and compressed inputs. Fresh Codex and security reviews on |
|
@sylvesterkaczmarek can you resolve the CI errors? |
|
Resolved the CI failures in 8bd5d1b: the regression test is now Ruff-formatted and explicitly narrows |
|
Fresh CI is now fully green on 8bd5d1b. The requested formatting/typecheck fixes are verified; ready for re-review. |
|
We'll merge #4462 instead; your contribution is included in its commit. |
Summary
Require
view_imageto identify signature-bearing raster image formats from the bytes it reads instead of trusting a raster-looking filename.The previous detector checked known image signatures first, then accepted any filename whose MIME type began with
image/. That meant arbitrary bytes stored asreport.png,photo.jpg, or another raster image extension could be returned as structured image data.This change narrows the filename fallback to SVG only. PNG, JPEG, GIF, WebP, BMP, and TIFF require their existing byte signatures. SVG and SVGZ retain the established filename fallback because SVG has no single binary magic value and SVGZ is compressed, so removing filename inference entirely would break valid BOM/prolog/alternate-encoding and compressed SVG inputs that were already supported.
Test plan
Added regression coverage showing that:
fake.pngare rejectedpayload.binare accepted as PNGDOCTYPEremain acceptedRepository CI is used for full verification because this environment has GitHub connector access but no materialized dependency checkout.
Issue number
None. Found during a sandbox content-validation audit.