feat: proxy provider-display-config icons through the API#1096
Conversation
Add GET /admin/api/v1/provider-display-configs/{provider_key}/icon, a
server-side proxy that fetches the operator-set icon URL and streams the
bytes back to the browser under the same origin as the dashboard.
The motivation is the dashboard's Content-Security-Policy. `icon` URLs in
provider_display_configs are operator-pasted and span many external hosts
(npmmirror, simpleicons, Wikipedia, GitHub avatars, Webflow CDNs, ...) that
change as new providers are added. Enumerating each host in `img-src` is
a treadmill; the alternative — widening `img-src` to allow any HTTPS
origin — loosens CSP for every image load. Routing icons through this
endpoint means the browser only ever loads them from `'self'`, so `img-src`
can stay tight regardless of where operators source their logos.
Safety on the upstream fetch:
- only `https://` URLs are proxied; relative paths / registry-key shortcuts
return 404 and stay handled client-side
- 5 s total timeout
- 2 MiB body cap, enforced while streaming
- 3-redirect limit
- upstream `Content-Type` must start with `image/`; the bytes are returned
with that exact type and a 1 h `Cache-Control`
Follow-ups (separate PRs):
- app-doubleword-private: SPA renders icons via this endpoint
- internal: once both ship, tighten the CSP `img-src` back to
'self' data: blob:'
Deploying control-layer with
|
| Latest commit: |
d076673
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bbd98a6e.control-layer.pages.dev |
| Branch Preview URL: | https://feat-provider-icon-proxy.control-layer.pages.dev |
There was a problem hiding this comment.
Pull request overview
Adds a new Admin API endpoint to proxy operator-configured provider icon URLs through the dwctl server, so the dashboard can load provider logos same-origin and keep a tighter CSP without enumerating external image hosts.
Changes:
- Introduces
GET /admin/api/v1/provider-display-configs/{provider_key}/iconto fetch and return upstream icon bytes (https-only, timeout/redirect limits, size cap,image/*Content-Type guard, cache headers). - Wires the new route into the admin router.
- Registers the endpoint in the Admin OpenAPI spec.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| dwctl/src/openapi/admin.rs | Adds the new icon-proxy handler to the Admin OpenAPI paths. |
| dwctl/src/lib.rs | Routes GET /provider-display-configs/{provider_key}/icon to the new handler under the admin router. |
| dwctl/src/api/handlers/provider_display_configs.rs | Implements the icon proxy handler + capped-body reader helper and related constants. |
| let icon_value = { | ||
| let mut conn = state.db.read().acquire().await.map_err(|e| Error::Database(e.into()))?; | ||
| let mut repo = ProviderDisplayConfigs::new(&mut conn); | ||
| repo.get_by_key(&provider_key) | ||
| .await? | ||
| .ok_or_else(|| Error::NotFound { | ||
| resource: "provider display config".to_string(), | ||
| id: provider_key.clone(), | ||
| })? | ||
| .icon | ||
| .unwrap_or_default() | ||
| }; | ||
|
|
||
| // Only proxy absolute https:// URLs. Relative paths (`/brand/...`) and | ||
| // registry-key shortcuts (`anthropic`, `google`) are resolved by the SPA | ||
| // and don't need (or want) to round-trip through this endpoint. | ||
| let icon_url = match Url::parse(&icon_value) { |
| let client = reqwest::Client::builder() | ||
| .timeout(ICON_FETCH_TIMEOUT) | ||
| .redirect(reqwest::redirect::Policy::limited(3)) | ||
| .build() | ||
| .map_err(|e| Error::Other(anyhow::anyhow!("build http client: {e}")))?; |
| let resp = client.get(icon_url).send().await.map_err(|e| { | ||
| tracing::warn!(error = %e, "provider icon upstream fetch failed"); | ||
| Error::Other(anyhow::anyhow!("fetch provider icon: {e}")) | ||
| })?; |
| let body_bytes = read_capped_body(resp, MAX_ICON_SIZE_BYTES).await.map_err(|e| { | ||
| tracing::warn!(error = %e, "provider icon body read failed"); | ||
| Error::Other(anyhow::anyhow!("read provider icon body: {e}")) | ||
| })?; |
| pub async fn get_provider_display_config_icon<P: PoolProvider>( | ||
| State(state): State<AppState<P>>, | ||
| Path(provider_key): Path<String>, | ||
| _: RequiresPermission<resource::Models, operation::ReadOwn>, | ||
| ) -> Result<Response> { |
![doubleword-code[bot]](https://avatars.githubusercontent.com/in/3610877?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds a server-side icon proxy endpoint (GET /admin/api/v1/provider-display-configs/{provider_key}/icon) that fetches operator-configured icon URLs and returns them under the same origin. The motivation is sound: it allows the SPA to maintain a tight CSP (img-src 'self' data: blob:) without enumerating every external icon host.
The implementation includes several good security controls (5s timeout, 2 MiB body cap, 3-redirect limit, Content-Type validation), but lacks SSRF protections that are critical when fetching arbitrary admin-provided URLs. Per OWASP SSRF prevention guidance, URL proxies should validate that the resolved destination is not an internal/private resource.
Verdict: Needs changes before merge — add SSRF mitigations to prevent the endpoint from being abused to access internal network resources or cloud metadata endpoints.
Research notes
-
OWASP SSRF Prevention Cheat Sheet: Recommends allowlisting for Case 1 (known trusted targets) or blocklisting private IP ranges + DNS resolution checks for Case 2 (arbitrary external URLs). Since this endpoint accepts any
https://URL an admin pastes, it falls into Case 2. Key mitigations include:- Block private IP ranges (RFC1918:
10.0.0.0/8,172.16.0.0/12,192.168.0.0/16) - Block cloud metadata endpoints (
169.254.169.254,metadata.google.internal) - Disable redirects or validate redirect targets (partially addressed via 3-redirect limit)
- Use custom DNS resolver that blocks private IPs
- Block private IP ranges (RFC1918:
-
reqwest ClientBuilder docs: reqwest does not provide built-in SSRF protection. The
resolve()method can pin specific hostnames to IPs, but there's no native "block private IPs" feature — this must be implemented at the application layer or via a custom hyper connector. -
Codebase patterns: The webhook handler in
webhooks.rs:152-156validates webhook URLs are HTTPS (with localhost exception for dev), showing awareness of URL security concerns elsewhere in the codebase.
Suggested next steps
-
Blocking: Add SSRF protections to prevent fetching from internal/private networks:
- Option A (recommended): Use a custom DNS resolver (e.g.,
hickory-resolverwithreqwest::ClientBuilder::dns_resolver()) that rejects private IP resolutions - Option B: After DNS resolution, validate the IP address is public before connecting
- Block known cloud metadata endpoints explicitly
- Option A (recommended): Use a custom DNS resolver (e.g.,
-
Non-blocking: Consider adding tests for the new
get_provider_display_config_iconfunction covering:- Non-HTTPS URLs return 404
- Non-image Content-Type returns 500
- Body exceeding 2 MiB returns 500
- Upstream timeout behavior
- Redirect limit behavior
-
Nit: The doc comment path
/provider-display-configs/{provider_key}/icondoesn't match the actual route which is mounted at/admin/api/v1/provider-display-configs/{provider_key}/icon(seelib.rs:1192-1194). Update the#[utoipa::path]path to include the full prefix for accurate OpenAPI docs.
General findings
Missing SSRF Protections (Blocking)
The endpoint fetches arbitrary HTTPS URLs provided by admins without validating where those URLs resolve to. An attacker who compromises an admin account (or is a malicious admin) could:
- Set an icon URL pointing to
http://169.254.169.254/latest/meta-data/iam/security-credentials/(AWS IMDSv1) — though the code requireshttps://, some cloud providers support HTTPS metadata endpoints - Point to internal services:
https://internal-service.corp.local/secret-endpoint - Use DNS rebinding: Register a domain that initially resolves to a public IP (passing any initial check) but then rebinds to a private IP
The current https:// requirement helps but is insufficient — many internal services run on HTTPS, and cloud metadata endpoints increasingly support HTTPS.
Suggested fix: Implement one of these approaches:
Option A — Custom DNS resolver (preferred):
use hickory_resolver::config::{ResolverConfig, ResolverOpts};
use std::net::IpAddr;
fn is_public_ip(ip: IpAddr) -> bool {
ip.is_global() && !ip.is_loopback() && !ip.is_private()
}
// Configure reqwest client with DNS resolver that blocks private IPs
let resolver = /* custom resolver that checks is_public_ip */;
let client = reqwest::Client::builder()
.dns_resolver(Arc::new(resolver))
.timeout(ICON_FETCH_TIMEOUT)
.redirect(reqwest::redirect::Policy::limited(3))
.build()?;Option B — Explicit blocklist (defense in depth):
Add explicit validation after DNS resolution to reject connections to:
- RFC1918 private ranges
- Link-local addresses (
169.254.0.0/16) - Cloud metadata endpoints
- Loopback addresses
Note: reqwest doesn't expose post-DNS pre-connect hooks easily, so Option A with hickory-resolver is cleaner.
Missing Tests (Non-blocking)
No unit or integration tests exist for get_provider_display_config_icon. Given this is security-sensitive code handling external input, tests would help ensure the protections work as intended.
OpenAPI Path Mismatch (Nit)
The utoipa path annotation says path = "/provider-display-configs/{provider_key}/icon" but the actual route in lib.rs:1192-1194 mounts it at /admin/api/v1/provider-display-configs/{provider_key}/icon. This causes the generated OpenAPI spec to have incorrect paths.
| } | ||
| }; | ||
|
|
||
| let client = reqwest::Client::builder() |
There was a problem hiding this comment.
Blocking: This reqwest client has no SSRF protections. An attacker with admin access could set an icon URL that resolves to internal network resources or cloud metadata endpoints.
Why it matters: The OWASP SSRF Prevention Cheat Sheet explicitly warns that accepting arbitrary URLs requires validation that resolved destinations are not internal/private. While you correctly require https:// and limit redirects to 3, neither prevents:
- DNS rebinding attacks (domain resolves to public IP initially, then private IP)
- Internal services with valid HTTPS certificates
- Cloud metadata endpoints that support HTTPS (AWS IMDSv2, GCP, Azure all support HTTPS)
Per OWASP: "When the application can send requests to ANY external IP address or domain name... apply the block-list approach" including blocking RFC1918 private ranges, link-local addresses, and known metadata endpoints.
Suggested fix: Add a custom DNS resolver using hickory-resolver that validates resolved IPs are public before allowing the connection:
// Add to Cargo.toml: hickory-resolver = "0.26"
use hickory_resolver::config::{ResolverConfig, ResolverOpts};
use std::net::IpAddr;
struct SsrfSafeResolver { /* wraps hickory_resolver with IP validation */ }
fn is_safe_ip(ip: IpAddr) -> bool {
ip.is_global() // excludes private, loopback, link-local
&& !ip.is_loopback()
&& !ip.is_private()
&& !ip.is_link_local()
}
let client = reqwest::Client::builder()
.dns_resolver(Arc::new(SsrfSafeResolver::new()))
.timeout(ICON_FETCH_TIMEOUT)
.redirect(reqwest::redirect::Policy::limited(3))
.build()?;Alternatively, add explicit blocklist checks for known dangerous ranges as defense-in-depth.
| /// shortcuts are handled client-side and bypass this endpoint) | ||
| /// - **500** if the upstream fetch fails, times out, exceeds 2 MiB, returns | ||
| /// non-2xx, or serves a non-image Content-Type | ||
| #[utoipa::path( |
There was a problem hiding this comment.
Nit: The path in this utoipa annotation (/provider-display-configs/{provider_key}/icon) doesn't match the actual mounted route. In lib.rs:1192-1194, this handler is mounted at /provider-display-configs/{provider_key}/icon under the /admin/api/v1 prefix, making the full path /admin/api/v1/provider-display-configs/{provider_key}/icon.
Why it matters: The generated OpenAPI spec will have incorrect paths, which affects API documentation and any tooling that consumes the OpenAPI spec (client generators, API testers, etc.).
Suggested fix: Update the path to include the full prefix:
#[utoipa::path(
get,
path = "/admin/api/v1/provider-display-configs/{provider_key}/icon",
// ...
)]| Ok(buf.freeze()) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Non-blocking: No tests exist for the new get_provider_display_config_icon function or read_capped_body helper.
Why it matters: This is security-sensitive code that:
- Fetches arbitrary external URLs
- Validates Content-Type headers
- Enforces byte limits
- Handles various error conditions
Tests would help ensure these protections work correctly and catch regressions.
Suggested fix: Add tests covering:
- Non-HTTPS URLs return 404
- Non-image Content-Type returns 500 with appropriate error
- Response body exceeding 2 MiB returns 500
- Upstream timeout behavior
- 3-redirect limit behavior (4th redirect fails)
- Upstream returning 4xx/5xx status returns 500
- Valid icon returns 200 with correct Content-Type
Example structure:
#[sqlx::test]
async fn test_icon_proxy_rejects_non_https_url() {
// Setup provider with icon = "anthropic" (not a URL)
// Expect 404
}
#[sqlx::test]
async fn test_icon_proxy_body_limit_enforced() {
// Mock upstream returning >2MiB
// Expect 500 error
}Wikimedia (and likely other CDNs) reject requests with no User-Agent per
their published policy[1], so the icon proxy returned 500 with
"upstream icon host returned status 403" for any provider whose configured
icon URL pointed at upload.wikimedia.org. reqwest's default builder sets
no User-Agent.
Identify the proxy explicitly:
dwctl-icon-proxy/<CARGO_PKG_VERSION> (+https://github.com/doublewordai/control-layer)
Verified locally against all 13 production provider_display_configs.icon
URLs — every one now returns 200 instead of 500.
[1] https://meta.wikimedia.org/wiki/User-Agent_policy
There was a problem hiding this comment.
Summary
This PR adds GET /admin/api/v1/provider-display-configs/{provider_key}/icon, a server-side proxy for operator-set provider icon URLs. The motivation is sound: it allows the SPA to maintain a tight img-src 'self' data: blob: CSP regardless of where admin users source their icons.
The implementation includes several thoughtful safety measures:
- HTTPS-only URL validation
- 5-second timeout
- 2 MiB body cap enforced during streaming
- 3-redirect limit
- Content-Type must start with
image/ - Custom User-Agent to handle Wikimedia's policy
Verdict: Blocked — The icon proxy has a critical SSRF vulnerability that must be addressed before deployment. While admins set the icon URLs, this endpoint still allows arbitrary external fetches from your infrastructure without network-level protections.
Research Notes
I consulted OWASP's SSRF Prevention Cheat Sheet which identifies two cases:
- Case 1: Application should only contact identified/trusted applications → use allowlist
- Case 2: Application must contact ANY external resource → requires network-layer protections + deny-list at application layer
This endpoint falls into Case 2 (arbitrary external hosts). OWASP states:
"Do not accept complete URLs from the user because URL are difficult to validate and the parser can be abused"
"Deny-lists are bypass-prone. Prefer allow-lists. When unavoidable, block... private networks, localhost, cloud metadata endpoints"
The reqwest documentation shows that custom redirect policies can be used to block specific destinations, but the current implementation uses Policy::limited(3) which only limits count, not destination.
Suggested Next Steps
-
Add SSRF protection (Blocking): Implement IP address validation after DNS resolution to block private ranges, localhost, and cloud metadata endpoints. Consider using a crate like
ssrf_filteror implementing a custom TCP connector. -
Disable redirects or use custom policy (Blocking): Following redirects (even limited to 3) allows bypass of initial URL validation. Either disable redirects entirely (
Policy::none()) or implement a custom policy that validates each redirect destination. -
Add tests (Non-blocking): No tests exist for
get_provider_display_config_icon. Add tests covering successful fetch, non-HTTPS rejection, non-image content-type rejection, body cap enforcement, timeout, and redirect behavior. -
Document operational considerations (Nit): Consider documenting what happens when an upstream icon host goes down (cached response? error handling in SPA?), and whether rate limiting is needed if many clients request the same icon simultaneously.
General Findings
SSRF Vulnerability (Critical)
The icon proxy fetches arbitrary HTTPS URLs controlled by admins without validating the resolved IP address. An attacker with admin access (or ability to influence provider_display_configs.icon) could:
- Access internal services (e.g.,
http://169.254.169.254/latest/meta-data/on AWS, even over HTTPS via tunneling) - Scan internal network resources
- Exploit DNS rebinding to point a legitimate domain to an internal IP during request
The redirect policy compounds this: an attacker could configure a URL that initially appears valid but redirects to an internal resource.
Note on threat model: While only PlatformManager users can set icon URLs, SSRF from a compromised admin account or insider threat is still a valid concern. Additionally, if admin credentials are ever leaked, this becomes an immediate remote code execution vector when combined with other vulnerabilities.
Comparison to existing webhook code
Your webhook handler (webhooks.rs:152-158) validates URLs similarly but explicitly allows http://localhost and http://127.0.0.1 for development. The icon proxy doesn't have this exception, which is good, but neither has SSRF protection at the network layer.
General findings (auto-demoted from inline due to pre-validation)
- Blocking
dwctl/src/api/handlers/provider_display_configs.rs:379— Following redirects without validating redirect destinations enables SSRF bypass.- (demoted: code self-check failed at dwctl/src/api/handlers/provider_display_configs.rs:379: diff has
.timeout(ICON_FETCH_TIMEOUT), model claimed.redirect(reqwest::redirect::Policy::limited(3)))
- (demoted: code self-check failed at dwctl/src/api/handlers/provider_display_configs.rs:379: diff has
| // Only proxy absolute https:// URLs. Relative paths (`/brand/...`) and | ||
| // registry-key shortcuts (`anthropic`, `google`) are resolved by the SPA | ||
| // and don't need (or want) to round-trip through this endpoint. | ||
| let icon_url = match Url::parse(&icon_value) { |
There was a problem hiding this comment.
Blocking: SSRF vulnerability — URL validation only checks scheme is https, but doesn't prevent fetching from private IPs, localhost, or cloud metadata endpoints.
Why it matters: An attacker with admin access could set an icon URL that resolves to an internal resource (e.g., 169.254.169.254 for AWS metadata, or internal services). Per OWASP SSRF Prevention Cheat Sheet, accepting arbitrary URLs from users is dangerous because "the parser can be abused" and network-layer protections are essential when allowlists aren't feasible.
The url::Url type validates URL syntax but doesn't resolve DNS or check IP addresses. After parsing, you need to:
- Resolve the hostname
- Verify all resolved IPs are public (not RFC1918, loopback, link-local, or cloud metadata ranges)
- Protect against DNS rebinding
Suggested fix: Use a crate like ssrf_filter or implement a custom hyper::connect::Connect that validates IPs before establishing connections. Alternatively, restrict icon URLs to a curated allowlist of trusted CDNs.
Also consider: reqwest 0.13 supports custom connectors via .connect_with(...), which would let you enforce IP validation at the connection layer.
|
|
||
| // Several icon hosts (Wikimedia in particular, per its User-Agent policy) | ||
| // return 403 for requests with no User-Agent, so identify ourselves. | ||
| let client = reqwest::Client::builder() |
There was a problem hiding this comment.
Non-blocking: Creating a new reqwest::Client per request is inefficient.
Why it matters: Each call to Client::builder().build() creates a new connection pool. This wastes resources and prevents connection reuse across requests. The codebase already patterns around shared clients (see lib.rs:2699 where multi_step_reqwest_client is created once and cloned).
That said, for an icon endpoint that returns cached responses (1-hour Cache-Control), this may be acceptable since browsers will cache aggressively.
Suggested fix: Create a single reqwest::Client at application startup (perhaps alongside the webhook dispatcher client at webhooks/dispatcher.rs:92) and clone it here. Cloning a reqwest::Client is cheap (Arc-based, shares connection pool).
| /// Drain a `reqwest::Response` body, returning early with an error if the | ||
| /// accumulated bytes would exceed `max`. Avoids buffering an unbounded body | ||
| /// from an untrusted upstream. | ||
| async fn read_capped_body(mut resp: reqwest::Response, max: u64) -> anyhow::Result<bytes::Bytes> { |
There was a problem hiding this comment.
Non-blocking: Body cap implementation is correct but could be more defensive.
Why it matters: The current implementation correctly limits total bytes, but doesn't protect against chunk bombs (many tiny chunks causing allocation overhead) or slowloris-style attacks (very slow chunk delivery). The 5-second timeout helps with the latter.
One subtle issue: buf.extend_from_slice(&chunk) copies data. For a 2MB cap this is fine, but if you ever increase the cap, consider pre-allocating with BytesMut::with_capacity() based on Content-Length header when present.
Suggested fix: Consider adding a minimum throughput check or using tokio::time::timeout around individual chunk() calls if slow-read attacks become a concern. For now, the 5s total timeout is likely sufficient for icon fetching.
Address the latest code review on #1096: - **SSRF protection (blocking)**: install a custom `reqwest` DNS resolver that drops non-globally-routable addresses (RFC1918, loopback, link-local incl. cloud metadata 169.254.169.254, CGNAT, IPv6 unique-local / link-local, documentation / reserved). Without this, an operator-set icon URL whose host resolves into the cluster network (intentionally or via a hostile DNS response) lets an unprivileged caller pull data from the host. The filter runs at the DNS layer so CNAMEs and multi-record responses are also covered. - **Disable redirects**: the SSRF resolver only sees the initial host, so a 302 from upstream would bypass it. `Policy::none()` makes reqwest treat redirects as the final response, which our status-success check then rejects. - **Share the client via `OnceLock`**: building a `reqwest::Client` per request rebuilt the connection pool, TLS context, and SSRF resolver every call. - **Trim the icon value** before `Url::parse` (operator-pasted values often have trailing whitespace). - **Drop the `tracing::warn!` calls** at the upstream-fetch and body-read sites — `Error::Other` is already logged at error level by the central `IntoResponse` impl, so we were double-logging. - **Tests**: 11 new unit tests cover the SSRF predicate (IPv4 + IPv6, all the relevant private ranges, IPv4-mapped IPv6, CGNAT boundaries, public addresses still allowed).
|
Pushed Blocking (both rounds)
Non-blocking (Copilot)
TestsAdded 11 pure-Rust unit tests for the SSRF predicate covering all the relevant ranges, IPv4-mapped IPv6 ( Non-blocking I'm skipping
|
There was a problem hiding this comment.
Summary
This PR introduces an icon proxy endpoint (GET /admin/api/v1/provider-display-configs/{provider_key}/icon) that fetches operator-configured icon URLs server-side and streams them back to the browser. The primary motivation is CSP tightening—by routing icons through the API, the SPA can use img-src 'self' data: blob: regardless of where administrators paste icon URLs from.
The implementation is well-engineered with strong SSRF protections, appropriate safety limits (5s timeout, 2 MiB body cap, Content-Type validation), and sensible performance optimizations (shared reqwest::Client via OnceLock). The authorization model using ReadOwn for Models is appropriate since all authenticated users (StandardUser, PlatformManager, RequestViewer) have this permission.
Verdict: Approve—the SSRF mitigation is comprehensive and aligns with OWASP guidance, the code quality is high, and the tests cover the critical security predicates thoroughly.
Research notes
- OWASP SSRF Prevention Cheat Sheet: Confirms that filtering at the DNS resolver layer (as done here with
SsrfSafeResolver) is the recommended approach for Case 2 scenarios where arbitrary external URLs must be fetched. The cheat sheet explicitly recommends disabling redirects (Policy::none()) to prevent bypassing input validation—this is correctly implemented. The deny-list approach used here covers all ranges OWASP lists as minimum: RFC1918, loopback, link-local (including 169.254.169.254 metadata service), multicast, documentation ranges, and IPv6 unique-local. - reqwest docs:
Policy::none()creates a policy that "does not follow any redirect"—critical because the SSRF resolver only inspects the initial host, and a 302 could bypass it. - Codebase patterns: The
OnceLock<reqwest::Client>pattern matches how other long-lived clients are managed in the codebase (e.g.,multi_step_reqwest_clientinlib.rs:2699, webhook dispatcher inwebhooks/dispatcher.rs:92).
Suggested next steps
- Merge as-is—the implementation is production-ready.
- Follow up on CSP tightening (mentioned in commit message): Once the dashboard renders icons via this endpoint, tighten
img-srcback to'self' data: blob:inconfig.yaml. - Consider integration test: Add a Hurl or Playwright test that verifies the icon proxy returns 200 for a known public URL (e.g., a simpleicons.org icon) and 500 for a blocked internal IP attempt (though the latter may require mocking).
General findings
- The code comments are exemplary—each constant and function explains the why clearly (e.g., why 2 MiB cap, why 5s timeout, why redirects are disabled).
- The 11 unit tests for
is_globally_routablecover all major edge cases including IPv4-mapped IPv6 addresses—a common SSRF bypass vector that's easy to miss. - No resource leaks: the handler doesn't hold database connections longer than necessary (scoped block on line 485–496), and the shared client prevents connection pool exhaustion.
Note: No blocking issues found. The SSRF protection is comprehensive, the error handling follows existing patterns, and the authorization model is consistent with similar read-only endpoints.
While testing the SSRF resolver end-to-end I caught a gap and a regression from the previous commit: - **IP-literal URLs bypass the DNS resolver.** When a URL's host is already an IP (`https://10.0.0.1/foo`, `https://169.254.169.254/...`, `https://[::1]/...`), reqwest goes straight to connect without consulting `dns_resolver`, so `SsrfSafeResolver` never runs. Added a synchronous `url_host_is_globally_routable` check that runs in the handler before the request fires, applying `is_globally_routable` to any `url::Host::Ipv4` / `Ipv6` and deferring to the resolver for domain hosts. - **`Policy::none()` broke real upstreams.** Two of the provider icon hosts (npmmirror among them) serve a 302 to a CDN — disabling redirects outright returned 500 for them. Switched to `Policy::custom` capped at 3 hops, requiring https-only and re-running the IP-literal check on every redirect target. The DNS resolver still runs for hostname-based redirects on every hop, so redirecting into an internal host is still blocked at the resolver layer; the policy adds the synchronous IP / scheme checks reqwest can do without DNS. Tests: 3 more unit tests cover the new URL helper (hostnames unaffected, private IP literals blocked incl. `[::1]` / `[fc00::1]` / `[::ffff:10.0.0.1]`, public IP literals allowed). Total 14 SSRF tests now. Verified locally: - All 13 prod provider icons return 200 image bytes through the proxy. - `https://10.0.0.1/x` → 404 (handler URL-check rejects pre-flight). - `https://localhost/x` → 500 (resolver rejects post-DNS).
|
Pushed IP-literal hosts bypass the DNS resolverWhen the URL host is already an IP (
|
There was a problem hiding this comment.
Summary
This PR adds a server-side icon proxy endpoint (GET /admin/api/v1/provider-display-configs/{provider_key}/icon) that fetches operator-configured icon URLs and streams them back under the same origin, allowing the dashboard to maintain a tight CSP (img-src 'self' data: blob:). The implementation includes comprehensive SSRF protection through a custom DNS resolver, pre-flight IP literal checks, redirect policies, body size limits, and content-type validation.
Verdict: Needs minor changes before approval. The SSRF protection is well-implemented and follows OWASP best practices, but there's a permission model inconsistency and missing integration tests for the endpoint behavior.
Research notes
- OWASP SSRF Prevention Cheat Sheet: Recommends allowlisting schemes (✅ https-only), validating DNS responses (✅ custom resolver blocks private IPs), blocking redirects or validating redirect targets (✅ custom policy with 3-hop limit + validation), and enforcing response type validation (✅ image/* check). The implementation aligns with these recommendations.
- reqwest DNS resolver behavior: Confirmed that when a URL contains an IP literal (e.g.,
https://10.0.0.1/foo), reqwest bypasses the DNS resolver entirely and connects directly. The synchronousurl_host_is_globally_routablecheck correctly addresses this gap. - IPv4-mapped IPv6 addresses: The code correctly handles
::ffff:x.x.x.xby converting to IPv4 before checking, preventing bypass via IPv6 representation.
General findings
Permission Model Inconsistency (Non-blocking)
The endpoint uses RequiresPermission<resource::Models, operation::ReadOwn>, but unlike other ReadOwn endpoints, it doesn't filter results by user/group membership. Any user with Models::ReadOwn (StandardUser) can fetch icons for any provider key by enumeration:
// Line 511: Uses ReadOwn but no ownership filtering
_: RequiresPermission<resource::Models, operation::ReadOwn>,The list endpoint (list_provider_display_configs) filters providers based on group membership, but this endpoint accepts any provider_key without verifying the user has access to that specific provider. While provider keys aren't sensitive (they're identifiers like "anthropic", "openai") and icons are non-sensitive, this creates an inconsistency in the authorization model.
Suggested fix: Either document this exception (provider display configs are platform-wide resources visible to anyone who can read models), or add group-membership verification matching the list endpoint's logic.
Suggested next steps
-
Add integration tests for the icon proxy endpoint covering:
- Successful icon fetch (200 response with correct content-type)
- Provider not found (404)
- Non-HTTPS URL (404)
- Upstream returns non-2xx status (500)
- Upstream returns non-image content-type (500)
- Body exceeds 2 MiB cap (500)
- Redirect scenarios (follows valid, rejects invalid scheme/host)
-
Consider adding ETag/Last-Modified support for cache validation. Currently returns
Cache-Control: max-age=3600but no validators, meaning clients re-fetch unchanged icons after expiry even if bytes haven't changed. -
Document the permission model decision - clarify whether provider display configs are intended to be accessible by all users with
Models::ReadOwnregardless of group membership, or if this should be tightened.
| pub async fn get_provider_display_config_icon<P: PoolProvider>( | ||
| State(state): State<AppState<P>>, | ||
| Path(provider_key): Path<String>, | ||
| _: RequiresPermission<resource::Models, operation::ReadOwn>, |
There was a problem hiding this comment.
Non-blocking: Permission model inconsistency — this endpoint uses ReadOwn but doesn't filter by user/group membership like other ReadOwn endpoints do.
Why it matters: Any user with Models::ReadOwn (StandardUser) can fetch icons for ANY provider key by enumeration, bypassing the group-based filtering that list_provider_display_configs applies. While provider keys and icons aren't sensitive data, this creates an inconsistency in the authorization model that could confuse future maintainers.
Suggested fix: Either (a) add group-membership verification matching the list endpoint's logic, or (b) document explicitly that provider display configs are platform-wide resources accessible to anyone with Models::ReadOwn.
| // on each hop, so a redirect that resolves into the | ||
| // internal network is blocked there — this policy only adds | ||
| // the synchronous checks that don't need DNS. | ||
| .redirect(reqwest::redirect::Policy::custom(|attempt| { |
There was a problem hiding this comment.
Nit: Consider extracting the redirect policy into a named function for testability and clarity. This would make it easier to unit-test the redirect validation logic independently (e.g., testing that https:// → http:// redirects are rejected, or that 4+ redirects are blocked).
| .map_err(|e| Error::Other(anyhow::anyhow!("build icon response: {e}"))) | ||
| } | ||
|
|
||
| /// Drain a `reqwest::Response` body, returning early with an error if the |
There was a problem hiding this comment.
Nit: Consider using resp.bytes().await with a length check instead of manual chunked reading. However, the current streaming approach is correct for enforcing the cap mid-stream rather than after full download, so this is purely stylistic.
🤖 I have created a release *beep* *boop* --- ## [8.54.0](v8.53.0...v8.54.0) (2026-05-27) ### Features * proxy provider-display-config icons through the API ([#1096](#1096)) ([f460543](f460543)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Adds
GET /admin/api/v1/provider-display-configs/{provider_key}/icon— a server-side proxy that fetches the operator-set icon URL and streams the bytes back to the browser under the same origin as the dashboard.Why
provider_display_configs.iconURLs are operator-pasted in the admin surface and span many external hosts:…and that list grows each time an admin adds a provider with a new logo URL. Two CSP-side options are bad:
img-src→ treadmill, requires a CSP hotfix every time a new host appears.img-src https:→ allows any HTTPS image origin; widens the policy for every image load, not just provider icons.This PR fixes it at the source: icons are fetched server-side and returned same-origin, so the SPA's CSP can keep
img-src 'self' data: blob:and operators can configure any icon URL they like.Behaviour
https://icon, upstream serves an image200with upstream bytes +Content-Type+Cache-Control: public, max-age=3600404(relative paths and registry-key shortcuts are handled client-side and bypass the proxy)500Upstream-fetch safety:
https://onlyimage/(guards against an upstream returning an HTML error page that the browser would try to render in<img>)Auth:
RequiresPermission<Models, ReadOwn>— same gate asGET /provider-display-configs/{provider_key}. The endpoint never accepts a URL from the client, only theprovider_key, so SSRF is bounded to what an admin has already configured.Follow-ups (separate PRs)
<ProviderBadge>icons via this endpoint instead of the raw operator-set URL.img-srcback to'self' data: blob:.Test plan
cargo check -p dwctlcleanjust ci rust(clippy, fmt, tests)/admin/api/v1/provider-display-configs/openai/iconpost-deploy with a real session, confirm the PNG/SVG renders