mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 09:13:14 +00:00
fix: render GitHub-hosted images in PR descriptions on reviews page (#1589)
* fix: render GitHub-hosted images in PR descriptions on reviews page PR description images hosted on GitHub (user-attachment uploads and *.githubusercontent.com) render broken on the reviews page because private-repo attachments require GitHub auth the browser session lacks. Add an authenticated backend image proxy (host-allowlisted to guard against SSRF) and route those image URLs through it from the reviews UI. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: harden PR image proxy (IDOR, SVG XSS, unbounded buffering) Address review findings on the review-page image proxy: - IDOR: the proxy fetched any *.githubusercontent.com URL with the App installation token, gated only by route-param repo access, so a user authorized for one repo could read images from another private repo the App can see. Bind the URL to the authorized PR — only proxy URLs that appear in that PR's body. - SVG XSS: served any image/* inline from the API origin, including image/svg+xml which can run script. Restrict to safe raster types and add X-Content-Type-Options: nosniff + a locked-down CSP. - DoS: enforced the size cap only after buffering the full response. Stream and abort once the cap is exceeded. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
a8c7af4192
commit
0bff510aae
7 changed files with 307 additions and 5 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 "}
|
||||
|
||||
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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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 = {
|
|||
</div>
|
||||
),
|
||||
hr: () => <hr className="border-[var(--ui-border-subtle)] my-3" />,
|
||||
img: ({ src, alt }: ComponentProps<"img">) => (
|
||||
<img
|
||||
src={typeof src === "string" ? src : undefined}
|
||||
alt={alt ?? ""}
|
||||
loading="lazy"
|
||||
className="my-2 h-auto max-w-full rounded-md border border-[var(--ui-border-subtle)]"
|
||||
/>
|
||||
),
|
||||
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<BoundaryProps, BoundaryState> {
|
|||
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<typeof defaultUrlTransform>[2]
|
||||
) =>
|
||||
key === "src" ? transformImageUrl(url) : defaultUrlTransform(url, key, node);
|
||||
}, [transformImageUrl]);
|
||||
|
||||
return (
|
||||
<div className="min-w-0 max-w-full text-[13px] leading-6 break-words [overflow-wrap:anywhere] [&_.streamdown]:text-[color:var(--ui-text)]">
|
||||
<MarkdownErrorBoundary content={content}>
|
||||
|
|
@ -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}
|
||||
</Streamdown>
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
44
ui/src/lib/reviewImage.test.ts
Normal file
44
ui/src/lib/reviewImage.test.ts
Normal file
|
|
@ -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"
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
@ -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<ReviewDiffFile> | 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<SideTab>("info")
|
||||
const [selectedFile, setSelectedFile] = useState<string | null>(null)
|
||||
const fileRefs = useRef<Record<string, HTMLDivElement | null>>({})
|
||||
|
|
@ -932,7 +937,10 @@ function ReviewBodyInner({
|
|||
<PrHeader detail={detail} />
|
||||
<div className="mt-4 rounded-lg border border-border bg-card p-4">
|
||||
{detail.pr.body ? (
|
||||
<Markdown content={detail.pr.body} />
|
||||
<Markdown
|
||||
content={detail.pr.body}
|
||||
transformImageUrl={transformPrImage}
|
||||
/>
|
||||
) : (
|
||||
<p className="text-xs text-muted-foreground">
|
||||
This PR has no description.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue