diff --git a/agent/dashboard/review_api.py b/agent/dashboard/review_api.py index 22d593ba..8aa8a0bf 100644 --- a/agent/dashboard/review_api.py +++ b/agent/dashboard/review_api.py @@ -8,12 +8,15 @@ with the App installation token. from __future__ import annotations +import ipaddress import logging +import socket from collections.abc import Awaitable, Callable from typing import Any, Literal +from urllib.parse import urljoin, urlparse import httpx -from fastapi import HTTPException +from fastapi import HTTPException, Response from ..reviewer_findings import REVIEWER_THREAD_KIND from ..utils.github_app import get_github_app_installation_token @@ -407,6 +410,129 @@ async def get_review_diff(owner: str, repo: str, pr_number: int) -> dict[str, An } +# --- PR description image proxy ---------------------------------------------- +# PR bodies can embed images hosted on GitHub (user-attachment uploads, +# *.githubusercontent.com). For private repos those URLs require GitHub auth the +# browser doesn't have, so they render broken. We proxy them through the App +# installation token. The host allowlist + per-redirect public-IP check guard +# against SSRF (only GitHub-owned hosts are ever contacted). + +_ALLOWED_IMAGE_HOST_SUFFIXES = (".githubusercontent.com",) +_MAX_IMAGE_REDIRECTS = 5 +_MAX_IMAGE_BYTES = 25 * 1024 * 1024 +# Only safe raster formats — SVG (image/svg+xml) can execute script in our +# origin, so it is never served. +_ALLOWED_IMAGE_CONTENT_TYPES = frozenset( + {"image/png", "image/jpeg", "image/gif", "image/webp", "image/avif"} +) + + +def _is_allowed_image_url(url: str) -> bool: + parsed = urlparse(url) + if parsed.scheme != "https": + return False + host = (parsed.hostname or "").lower() + if not host: + return False + if host == "github.com" or host == "www.github.com": + # On github.com only user-attachment assets are images worth proxying. + return parsed.path.startswith("/user-attachments/") + return any(host.endswith(suffix) for suffix in _ALLOWED_IMAGE_HOST_SUFFIXES) + + +def _host_resolves_public(hostname: str) -> bool: + try: + addr_infos = socket.getaddrinfo(hostname, None) + except socket.gaierror: + return False + if not addr_infos: + return False + for addr_info in addr_infos: + try: + ip = ipaddress.ip_address(addr_info[4][0]) + except ValueError: + return False + if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved: + return False + return True + + +def _validate_image_url(url: str) -> None: + if not _is_allowed_image_url(url): + raise HTTPException(400, "image host not allowed") + hostname = urlparse(url).hostname or "" + if not _host_resolves_public(hostname): + raise HTTPException(400, "image host not allowed") + + +async def _require_image_in_pr(owner: str, repo: str, pr_number: int, url: str, token: str) -> None: + """Bind the requested image to the authorized PR. + + The proxy fetches with the App installation token, which can read every repo + the App is installed on. Without this check a caller authorized for one repo + could proxy an image URL from another private repo (IDOR). Only URLs that + actually appear in this PR's body are allowed. + """ + pr_payload = await _github_get(f"/repos/{owner}/{repo}/pulls/{pr_number}", token) + body = pr_payload.get("body") or "" + if url not in body: + raise HTTPException(403, "image not referenced by this PR") + + +async def proxy_pr_image(owner: str, repo: str, pr_number: int, url: str) -> Response: + """Stream a GitHub-hosted PR image through the App token. + + The URL must appear in the target PR's body (bound to the authorized + resource), and every URL (including redirect targets) is validated against + the GitHub host allowlist and a public-IP check before it is contacted. + """ + _validate_image_url(url) + token = await _require_app_token() + await _require_image_in_pr(owner, repo, pr_number, url, token) + headers = {"Authorization": f"Bearer {token}", "Accept": "image/*"} + + current_url = url + async with httpx.AsyncClient(timeout=_GITHUB_TIMEOUT, follow_redirects=False) as client: + for _ in range(_MAX_IMAGE_REDIRECTS + 1): + async with client.stream("GET", current_url, headers=headers) as response: + if response.is_redirect: + location = response.headers.get("Location") + if not location: + raise HTTPException(502, "image fetch failed (redirect without target)") + current_url = urljoin(str(response.url), location) + _validate_image_url(current_url) + continue + + if response.status_code >= 400: + raise HTTPException(502, f"image fetch failed ({response.status_code})") + + content_type = ( + response.headers.get("Content-Type", "").lower().split(";", 1)[0].strip() + ) + if content_type not in _ALLOWED_IMAGE_CONTENT_TYPES: + raise HTTPException(415, "unsupported image type") + + # Stream and abort once over the cap so a large (or lying) upstream + # can't make the worker buffer the whole file. + content = bytearray() + async for chunk in response.aiter_bytes(): + content.extend(chunk) + if len(content) > _MAX_IMAGE_BYTES: + raise HTTPException(413, "image too large") + + return Response( + content=bytes(content), + media_type=content_type, + headers={ + "Cache-Control": "private, max-age=300", + "X-Content-Type-Options": "nosniff", + "Content-Security-Policy": "default-src 'none'; sandbox", + }, + ) + + raise HTTPException(502, "too many redirects fetching image") + + async def trigger_re_review(owner: str, repo: str, pr_number: int, login: str) -> dict[str, Any]: from ..utils.slack import GitHubPrRef from ..webapp import trigger_pr_review_from_ref diff --git a/agent/dashboard/routes.py b/agent/dashboard/routes.py index f022017a..ffbd81b9 100644 --- a/agent/dashboard/routes.py +++ b/agent/dashboard/routes.py @@ -65,6 +65,7 @@ from .review_api import ( get_review, get_review_diff, list_reviews, + proxy_pr_image, trigger_re_review, ) from .review_chat_api import ( @@ -838,6 +839,18 @@ async def api_get_review_diff( return await get_review_diff(owner, repo, pr_number) +@router.get("/reviews/{owner}/{repo}/{pr_number}/image") +async def api_get_review_image( + owner: str, + repo: str, + pr_number: int, + url: str, + session: dict[str, Any] = _SESSION_DEP, +) -> Response: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + return await proxy_pr_image(owner, repo, pr_number, url) + + @router.post("/reviews/{owner}/{repo}/{pr_number}/re-review") async def api_re_review( owner: str, diff --git a/tests/test_review_api.py b/tests/test_review_api.py index dd917188..257541a3 100644 --- a/tests/test_review_api.py +++ b/tests/test_review_api.py @@ -1,5 +1,11 @@ +import pytest +from fastapi import HTTPException + from agent.dashboard.review_api import ( + _ALLOWED_IMAGE_CONTENT_TYPES, _finding_counts, + _is_allowed_image_url, + _require_image_in_pr, _serialize_diff_groups, _serialize_finding, _thread_review_summary, @@ -70,6 +76,48 @@ def test_thread_review_summary_requires_pr_meta(): assert _thread_review_summary({"metadata": {"kind": "reviewer"}}) is None +def test_is_allowed_image_url_accepts_github_hosts(): + assert _is_allowed_image_url("https://github.com/user-attachments/assets/abc-123") + assert _is_allowed_image_url("https://private-user-images.githubusercontent.com/1/x.png?jwt=y") + assert _is_allowed_image_url("https://user-images.githubusercontent.com/1/x.png") + + +def test_is_allowed_image_url_rejects_unsafe_urls(): + # Non-https scheme. + assert not _is_allowed_image_url("http://github.com/user-attachments/assets/x") + # github.com but not a user-attachment path. + assert not _is_allowed_image_url("https://github.com/langchain-ai/open-swe") + # Arbitrary external host (SSRF guard). + assert not _is_allowed_image_url("https://evil.example.com/x.png") + # Lookalike host that merely contains the suffix substring. + assert not _is_allowed_image_url("https://githubusercontent.com.evil.com/x.png") + # Internal address. + assert not _is_allowed_image_url("https://169.254.169.254/latest/meta-data") + + +def test_image_content_type_allowlist_excludes_svg(): + # SVG can execute script in our origin, so it must never be served. + assert "image/svg+xml" not in _ALLOWED_IMAGE_CONTENT_TYPES + assert "image/png" in _ALLOWED_IMAGE_CONTENT_TYPES + + +async def test_require_image_in_pr_rejects_unreferenced_url(monkeypatch): + async def fake_github_get(path, token, **kwargs): + return {"body": "see ![diagram](https://x.githubusercontent.com/a.png)"} + + monkeypatch.setattr("agent.dashboard.review_api._github_get", fake_github_get) + + # A URL not present in the PR body (cross-repo IDOR attempt) is rejected. + with pytest.raises(HTTPException) as exc: + await _require_image_in_pr( + "acme", "repo", 7, "https://x.githubusercontent.com/other-repo.png", "tok" + ) + assert exc.value.status_code == 403 + + # A URL actually embedded in the PR body is allowed. + await _require_image_in_pr("acme", "repo", 7, "https://x.githubusercontent.com/a.png", "tok") + + def test_reviewer_thread_id_matches_webapp(): assert reviewer_thread_id("acme", "repo", 7) == generate_reviewer_thread_id("acme", "repo", 7) diff --git a/ui/src/components/agents/ported/Markdown.tsx b/ui/src/components/agents/ported/Markdown.tsx index cdcb0864..b5ff47de 100644 --- a/ui/src/components/agents/ported/Markdown.tsx +++ b/ui/src/components/agents/ported/Markdown.tsx @@ -1,12 +1,18 @@ import { Component, memo, useMemo } from "react"; -import { Streamdown } from "streamdown"; -import type { ReactNode } from "react"; +import { Streamdown, defaultUrlTransform } from "streamdown"; +import type { ComponentProps, ReactNode } from "react"; import "streamdown/styles.css"; interface MarkdownProps { content: string; /** When true, keep Streamdown in streaming mode for the duration of the run. */ isLive?: boolean; + /** + * Rewrite image `src` URLs before rendering (e.g. route private GitHub + * attachments through an authenticated proxy). Return the URL unchanged to + * leave it as-is. + */ + transformImageUrl?: (src: string) => string; } /** @@ -61,6 +67,14 @@ const STREAMDOWN_COMPONENTS = { ), hr: () =>
, + img: ({ src, alt }: ComponentProps<"img">) => ( + {alt + ), code: ({ className, children }: { className?: string; children?: ReactNode }) => { const text = String(children); const match = /language-([^\s]+)/.exec(className || ""); @@ -119,6 +133,7 @@ class MarkdownErrorBoundary extends Component { export const Markdown = memo(function Markdown({ content, isLive = false, + transformImageUrl, }: MarkdownProps) { const components = useMemo( () => ({ @@ -137,6 +152,16 @@ export const Markdown = memo(function Markdown({ [] ); + const urlTransform = useMemo(() => { + if (!transformImageUrl) return undefined; + return ( + url: string, + key: string, + node: Parameters[2] + ) => + key === "src" ? transformImageUrl(url) : defaultUrlTransform(url, key, node); + }, [transformImageUrl]); + return (
@@ -148,6 +173,7 @@ export const Markdown = memo(function Markdown({ shikiTheme={SHIKI_THEME} className="streamdown-agent min-w-0 max-w-full" components={components} + urlTransform={urlTransform} > {content} diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index c3c8d960..8fbfd865 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -14,6 +14,43 @@ if (!API_BASE && typeof window !== "undefined") { console.warn("VITE_DASHBOARD_API_BASE_URL is not set") } +const GITHUB_IMAGE_HOST_RE = + /^(?:www\.)?github\.com$|\.githubusercontent\.com$/i + +/** + * Build an authenticated proxy URL for GitHub-hosted PR images. Private-repo + * attachments can't be loaded directly by the browser, so they're routed + * through the dashboard backend which holds the App token. Non-GitHub image + * URLs are returned unchanged. + */ +export function reviewImageProxyUrl( + owner: string, + repo: string, + number: number, + src: string +): string { + let parsed: URL + try { + parsed = new URL(src) + } catch { + return src + } + if ( + parsed.protocol !== "https:" || + !GITHUB_IMAGE_HOST_RE.test(parsed.hostname) + ) { + return src + } + if ( + /^(?:www\.)?github\.com$/i.test(parsed.hostname) && + !parsed.pathname.startsWith("/user-attachments/") + ) { + return src + } + const path = `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/image` + return `${API_BASE}/dashboard/api${path}?url=${encodeURIComponent(src)}` +} + export class ApiError extends Error { constructor( public readonly status: number, diff --git a/ui/src/lib/reviewImage.test.ts b/ui/src/lib/reviewImage.test.ts new file mode 100644 index 00000000..507734b4 --- /dev/null +++ b/ui/src/lib/reviewImage.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "vitest" + +import { reviewImageProxyUrl } from "./api" + +describe("reviewImageProxyUrl", () => { + it("proxies github user-attachment images", () => { + const out = reviewImageProxyUrl( + "acme", + "repo", + 7, + "https://github.com/user-attachments/assets/abc-123" + ) + expect(out).toContain("/dashboard/api/reviews/acme/repo/7/image?url=") + expect(out).toContain( + encodeURIComponent("https://github.com/user-attachments/assets/abc-123") + ) + }) + + it("proxies githubusercontent images", () => { + const out = reviewImageProxyUrl( + "acme", + "repo", + 7, + "https://private-user-images.githubusercontent.com/1/x.png?jwt=y" + ) + expect(out).toContain("/dashboard/api/reviews/acme/repo/7/image?url=") + }) + + it("leaves non-github image hosts untouched", () => { + const src = "https://cdn.example.com/x.png" + expect(reviewImageProxyUrl("acme", "repo", 7, src)).toBe(src) + }) + + it("leaves non-attachment github.com urls untouched", () => { + const src = "https://github.com/acme/repo/blob/main/x.png" + expect(reviewImageProxyUrl("acme", "repo", 7, src)).toBe(src) + }) + + it("leaves unparseable urls untouched", () => { + expect(reviewImageProxyUrl("acme", "repo", 7, "not a url")).toBe( + "not a url" + ) + }) +}) diff --git a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx index ab01fba1..fa6bab79 100644 --- a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx +++ b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx @@ -71,7 +71,7 @@ import { warmDiffHighlighter, } from "@/components/agents/utils/diffUtils" import { Skeleton } from "@/components/ui/skeleton" -import { api } from "@/lib/api" +import { api, reviewImageProxyUrl } from "@/lib/api" import { useSession } from "@/lib/session" import { cn } from "@/lib/utils" @@ -460,6 +460,11 @@ function ReviewBodyInner({ diffFiles: Array | null }) { const composer = useReviewChatComposer() + const transformPrImage = useCallback( + (src: string) => + reviewImageProxyUrl(detail.owner, detail.repo, detail.number, src), + [detail.owner, detail.repo, detail.number] + ) const [sideTab, setSideTab] = useState("info") const [selectedFile, setSelectedFile] = useState(null) const fileRefs = useRef>({}) @@ -932,7 +937,10 @@ function ReviewBodyInner({
{detail.pr.body ? ( - + ) : (

This PR has no description.