feat(render): image URL fetch + SSRF hardening + bg overlay config - #79
Closed
ajianaz wants to merge 8 commits into
Closed
feat(render): image URL fetch + SSRF hardening + bg overlay config#79ajianaz wants to merge 8 commits into
ajianaz wants to merge 8 commits into
Conversation
…ixes - image_to_data_uri now accepts http(s):// URLs: fetched with a 10s timeout and a 10 MB size cap; mime from Content-Type with URL extension fallback (ref: #74 item 1) - blog-hero: new brand field bg_image_overlay (0.0-1.0, default 0.7) controls the gradient overlay opacity on top of bg_image, replacing the hardcoded 0.7 (ref: #74 item 2) - server render handler wraps blocking render in spawn_blocking so remote fetches never block the async runtime - docs: correct --scale default (2.0, not 1.0), document the 2x output default and URL support, add bg_image_opacity dark-artwork guidance (ref: #74 item 3) Closes #74
CodeCora review on this PR flagged an SSRF vector: cosy serve renders attacker-controlled JSON, and image_to_data_uri would GET any http(s):// URL — including loopback, RFC1918, and cloud-metadata link-local targets — while default redirect following could bypass naive host filters, and fetch errors echoed internal details. - Resolve the URL host and require a globally routable address before connecting; re-validate scheme + host on every redirect hop (max 5) - IPv4-mapped IPv6, CGNAT, benchmark, documentation, multicast ranges are rejected too - deny by default: CLI render opts in locally; cosy serve only via the explicit --allow-private-images flag - fetch failures now log the cause and return a generic 'failed to load remote image' instead of echoing URLs and byte counts - 5 new tests: private-target block, redirect-to-metadata block, non-http scheme rejection, IP classification table, generic errors
Addresses the second CodeCora block round in full: - DNS rebinding: validate_url_host resolved independently of reqwest's connection resolution, so a hostile domain could rotate to a private IP between the two lookups. The fetch now pins the connection to the validated address via ClientBuilder::resolve — no second resolution. - Unbounded body: response.bytes() buffered everything before the size check; chunked encoding bypassed the Content-Length pre-check. The body is now streamed via take(MAX + 1) so memory is capped during transfer, not after. - Redirects are removed entirely (Policy::none + explicit 3xx error): each hop previously needed its own pin, which is not expressible; a redirect response is now an error in all modes. - https-only by default: plain http:// requires allow_private (local CLI or the server's --allow-private-images), per the 'use HTTPS for external connections' finding. - Tests updated: redirects not followed in either mode, https-only default, loopback hostname blocked, plus the existing SSRF table.
::a.b.c.d (IPv4-compatible, first 96 bits zero) now falls through to the embedded IPv4 check alongside ::ffff:a.b.c.d — ::127.0.0.1 and ::169.254.169.254 were previously treated as public IPv6.
reqwest honors HTTP(S)_PROXY/ALL_PROXY env vars by default; a proxy would perform its own DNS and connect to an unvalidated target, defeating the validated-address pin. Force direct connections.
CodeCora round-3 finding: in server mode an attacker-supplied
bg_image like '/etc/passwd' was read from disk and base64-embedded
into the rendered PNG — a pre-existing arbitrary-file-read vector
the URL fetch work made visible.
Introduce ImagePolicy { allow_private, allow_local } threaded
explicitly through render_template(_data) -> render_slide_to_png ->
process_template -> image_to_data_uri (and the |b64 filter):
- server default = ImagePolicy::SECURE: https-only public URLs, and
local filesystem paths are rejected outright
- new --allow-local-image-paths opt-in for the server (e.g. trusted
internal pipeline); --allow-private-images unchanged
- standalone CLI uses ImagePolicy::UNRESTRICTED (local, user-driven)
Replaces the process-global atomics with an explicit parameter so the
policy is visible in signatures and tests need no global mutation.
New test: secure policy rejects local paths; e2e verified server
default rejects '/etc/hostname' (warn logged, render proceeds without
the image) while the opt-in embeds it.
🔍 Cora AI Code ReviewReview powered by cora-code · BYOK · MIT |
Contributor
Author
|
Splitting per the Cora 10-minute review convention: the combined diff (feature + security hardening) no longer fits one review window even with a clean thread. Follow-ups: (1) small PR with the blog-hero overlay field + scale doc fixes, (2) the image-fetch + SSRF hardening core. Evidence trail from 3 completed review rounds with all findings fixed remains on #78. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #74
What
https://URLs —image_to_data_urifetches remote images (10 s timeout, 10 MB streamed cap viatake(), no redirects, DNS pinned to the validated address,no_proxy).ImagePolicy { allow_private, allow_local }threaded explicitly through the render chain:ImagePolicy::SECURE): https-only public URLs (loopback/RFC1918/link-local/CGNAT/IPv4-compatible-v6 rejected, connection pinned to the validated IP), local filesystem paths rejected outright--allow-private-images,--allow-local-image-pathsImagePolicy::UNRESTRICTED(local, user-driven)bg_image_overlay(0.0–1.0, default 0.7 = old hardcoded value) for the gradient overlay overbg_image.--scaledefault (2.0), documented remote-image security posture + flags, dark-artwork opacity guidance.Why
#74 (URL input, configurable overlay, doc accuracy) plus hardening of the new fetch surface: server renders attacker-controlled JSON, so remote fetch without guards is SSRF (metadata endpoints, internal services, DNS rebinding, proxy bypass, unbounded bodies, local file read/exfiltration).
Testing