feat(tools): image reference processing and native provider support - #1251
Conversation
… support - Support OpenAI image edits via both Multipart form-data and JSON payloads - Automatically append reference image descriptions to prompt under [Reference Image Roles] - Support downloading image URLs for Gemini native image generation - Deduplicate reference images to optimize API request size - Add unit tests for Codex, DashScope, MiniMax, BytePlus and local/remote path resolution
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: This is a valuable image-reference direction, but it is not safe to merge as-is. The new reference-image URL path adds server-side HTTP downloads without the repo's SSRF and size-limit protections.
Risk level: High
Mandatory gates:
- Duplicate/prior implementation: clear. I found prior native image-generation work (#1002/#1006/#1021) and read_image URL support (#1191), but no duplicate PR for create_image reference-image handling.
- Project standards: issue found — new outbound URL fetches should follow the existing SSRF-safe client / bounded-download patterns used elsewhere in the repo.
- Strategic necessity: clear value. Reference images for create_image are a useful product capability across native and API-key-backed image providers.
- CI/checks: currently incomplete/in progress at review time (
gocheck still running; release-versioning/web green).
Findings:
- Critical:
internal/tools/create_image.goaddsdownloadImageBytes, which fetches user/agent-suppliedref_images[].urlwith a plainhttp.Client{Timeout: 30 * time.Second}andio.ReadAllwith no SSRF validation, no redirect policy, and no response size cap. That path is used by the OpenAI/DALL-E multipart edit branch when the reference is a URL. This bypasses the existing outbound-fetch hardening ininternal/security/ssrf.go/internal/tools/web_shared.goand can dial loopback, metadata, private networks, or arbitrarily large responses from the gateway process. Please validate URLs with the shared SSRF guard orinternal/security.NewSafeClient/pinned-IP workflow, refuse unsafe redirects, and use a bounded read likelimitedReadAllwith an image-specific max. Add regression tests for blocked loopback/private/metadata URLs and oversized responses. - Important: provider-specific reference URL handling is inconsistent. OpenAI multipart downloads the URL server-side, while JSON/OpenRouter/Codex paths forward external URLs downstream. That may be acceptable, but the trust boundary should be explicit in code/tests/docs: local gateway downloads must be SSRF-safe; provider-forwarded URLs must remain HTTP(S) only and should not silently become gateway-side fetches later.
Verdict: REQUEST_CHANGES
Next step: harden the gateway-side URL download path first, then re-run the provider/tool tests and wait for normal CI to finish before merge.
downloadImageBytes fetched caller-supplied ref_images[].url with a plain http.Client and unbounded io.ReadAll — no SSRF validation, redirect policy, or size cap, letting the gateway dial loopback/private/metadata hosts or read arbitrarily large responses. - Validate the URL via security.Validate and pin the resolved IP, then download through security.NewSafeClient (pinned dial, no redirects). - Cap the response with a bounded read (refImageMaxBytes, 20 MB). - Reject non-HTTP(S) reference URLs up front (file://, data:, gopher://) so provider-forwarded URLs stay HTTP(S)-only; document the trust boundary between gateway-side fetch and provider-forwarded URLs. - Add regression tests: blocked loopback/private/metadata, unfollowed redirect, oversized response, and non-http(s) scheme rejection.
|
@mrgoonie Thanks for the SSRF catch — hardened in Critical (SSRF in
Important (trust boundary) — made explicit:
Regression tests ( (Local note: the |
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: Re-reviewed 0711196e after the SSRF hardening update. The previous Critical finding is addressed: gateway-side reference image downloads now validate through the shared SSRF guard, pin the resolved IP, use security.NewSafeClient with redirects disabled, and read with a bounded refImageMaxBytes cap.
Risk level: Medium
Mandatory gates:
- Duplicate/prior implementation: clear. Prior native image-generation and
read_imageURL hardening work exists, but no duplicate PR forcreate_imagereference-image handling. - Project standards: passed after the SSRF fix. The gateway-side fetch path now follows the repo's safe outbound-fetch pattern.
- Strategic necessity: clear value. Reference images are a useful capability for image edits/generation across native and API-key-backed providers.
- CI/checks: green (
go,web,release-versioning).
Verification:
go test ./internal/tools ./internal/providers- Checked
internal/tools/create_image.gofordownloadImageBytes, HTTP(S)-only URL validation, SSRF guard usage, redirect rejection, and bounded reads. - Checked
internal/tools/create_image_ssrf_test.gofor loopback/private/link-local metadata blocking, redirect rejection, oversized response rejection, happy path, and non-http(s) rejection.
Findings:
- No Critical or Important findings remain from the previous SSRF/trust-boundary review.
Verdict: APPROVE
Summary
Adds image-reference processing for image-generation tools and wires native provider support so reference images can drive edits/generation across OpenAI, Gemini, Codex, DashScope, MiniMax and BytePlus.
Type
main)Target Branch
dev(feature).What changed
[Reference Image Roles]sectionChecklist
go build ./...passesgo build -tags sqliteonly ./...passes (if Go changes)go vet ./...passesgo test -race ./...cd ui/web && pnpm build(if UI changes) — n/a, no UI changes$1, $2(no string concat) — n/a, no SQLinternal/upgrade/version.go(if new migration) — n/a, no migrationTest Plan
go build ./...andgo build -tags sqliteonly ./...pass.go test ./internal/providerspasses (includes the newcodex_native_image_test.go).create_image_native_path_test.go,codex_native_image_test.go).go test ./internal/toolscannot build locally on Windows due to a pre-existing, unrelated issue ondev—internal/tools/document_parser_test.goreferences helpers (waitForRecordedPIDs,findLivePIDs) defined only inshell_abort_test.gowhich is gated//go:build !windows. This is not introduced by this PR and passes on the Linux CI runners.🤖 Generated with Claude Code