fix(pull-requests): render private GitHub images in PR bodies - #8446
fix(pull-requests): render private GitHub images in PR bodies#8446Xanacas wants to merge 5 commits into
Conversation
GitHub serves user-attachments uploads only to an authenticated viewer, so images in PR bodies rendered as broken placeholders in the app. Clients now route those URLs through the signed asset route; the server resolves GitHub's redirect with the gh CLI token and answers 302, so bytes flow from signed storage straight to the browser and the token never leaves the server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect Service Conventions: one finding — the new GitHubAttachmentProxy service adds backend behavior with no focused test. Service definition order, make/layer exports, Foo["Service"] typing, environment-based dependency acquisition, and the new Schema.TaggedErrorClass all match the conventions.
Posted via Macroscope — Effect Service Conventions
| const resolveAttachmentLocation: GitHubAttachmentProxy["Service"]["resolveAttachmentLocation"] = ( | ||
| url, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| const token = yield* readToken; | ||
| const request = HttpClientRequest.get(url, { | ||
| headers: token === null ? {} : { authorization: `token ${token}` }, | ||
| }); | ||
| const response = yield* httpClient | ||
| .execute(request) | ||
| .pipe(Effect.provideService(FetchHttpClient.RequestInit, { redirect: "manual" })); | ||
| if (response.status < 300 || response.status >= 400) return null; | ||
| const location = response.headers["location"]; | ||
| return location !== undefined && location.startsWith("https://") ? location : null; | ||
| }).pipe( | ||
| Effect.tapError((cause) => | ||
| Effect.logWarning("Failed to resolve a GitHub attachment.", { url, cause }), | ||
| ), | ||
| Effect.orElseSucceed(() => null), | ||
| ); |
There was a problem hiding this comment.
New backend behavior lands without a focused test: nothing exercises resolveAttachmentLocation, including the security-relevant guards (only 3xx responses are followed, and only an https:// location is returned) or the no-token path. Consider adding GitHubAttachmentProxy.test.ts with an HttpClient test layer (as AnalyticsService.test.ts / CliTokenManager.test.ts do) plus a stub GitHubCli layer, covering: 302 with an https location resolves, non-3xx status yields null, a non-https location yields null, and a failing gh auth token still issues the request unauthenticated.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Focused tests added in bd78fb8 (GitHubAttachmentProxy.test.ts): stub GitHubCli + HttpClient layers cover the token header and cached read, the anonymous no-token fallback, and null answers for non-3xx responses and non-https locations.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1da4829ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return isGitHubUserAttachmentUrl(source) | ||
| ? { _tag: "GitHubAttachment", url: source } | ||
| : { _tag: "Direct", uri: source }; |
There was a problem hiding this comment.
When app.t3.codes or the mobile app updates before a connected environment, this reclassifies every GitHub user-attachment URL and sends the new github-attachment resource to assets.createUrl; an older server's AssetResource decoder rejects that unknown tag. Consequently, even public attachments that previously rendered directly become unavailable during version skew. Gate the proxy on a server capability or fall back to the original direct URL when URL creation fails.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in fbba68e: both clients now fall back to loading the GitHub URL directly when signed-URL creation fails (older server rejecting the tag) or when no environment is available — public uploads keep rendering, private ones degrade to the unavailable chip they showed before this PR.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This adds a credentialed GitHub redirect proxy and a new asset contract across server, WebSocket, web, and mobile paths, with security-sensitive token and redirect handling. The mobile fallback path also has an unresolved load-error concern. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
Route handlers resolve services at serve time, so providing the proxy to makeRoutesLayer never reached the request fiber and every proxied image answered 404. Caught in an integrated web pass; unit tests could not see it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Before/after from an integrated web pass against a private-repo PR (Vantisgo, PR body with three Before (images 404 as broken placeholders):
After (rendered through the signed asset proxy; all three images load with real dimensions):
The integrated pass also caught a layer-wiring bug the unit tests could not see: route handlers resolve services at serve time, so the proxy layer had to be provided on the server runtime context rather than on |
Stub GitHubCli and HttpClient layers cover the token header and its cached read, the anonymous fallback when gh has no token, and the null answers for non-redirect responses and non-https locations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…available An older server rejects the github-attachment resource during version skew, and a markdown render without an environment has no proxy at all. Both now fall back to loading the GitHub URL directly — public uploads keep working, private ones degrade to the unavailable chip instead of regressing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| : { _tag: "workspace-file", threadId: props.threadId, path: imageSource.path } | ||
| } | ||
| sourceKey={imageSource._tag === "GitHubAttachment" ? imageSource.url : imageSource.path} | ||
| {...(imageSource._tag === "GitHubAttachment" ? { fallbackUri: imageSource.url } : {})} |
There was a problem hiding this comment.
🟡 Medium threads/ThreadFeed.tsx:1635
When the signed URL issued for a GitHub attachment returns a load error, the image is rendered as Image unavailable even though fallbackUri is loadable. fallbackUri is only selected when useAssetUrlState fails before loading; update ThreadMarkdownImageView to retry fallbackUri from onError before marking the image unavailable, matching the web renderer.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/threads/ThreadFeed.tsx around line 1635:
When the signed URL issued for a GitHub attachment returns a load error, the image is rendered as `Image unavailable` even though `fallbackUri` is loadable. `fallbackUri` is only selected when `useAssetUrlState` fails before loading; update `ThreadMarkdownImageView` to retry `fallbackUri` from `onError` before marking the image unavailable, matching the web renderer.
There was a problem hiding this comment.
Reviewed the web-scoped changes (ChatMarkdown.tsx, pullRequest/pullRequestMarkdown.logic.ts) for shared-primitive use, Tailwind/CSS ownership, and environment routing.
Environment routing is correct: ChatMarkdownAssetImage requires an explicit environmentId, ChatMarkdown resolves it from threadRef?.environmentId ?? explicitEnvironmentId ?? null with no active-environment fallback, and every PullRequestMarkdown / PullRequestMarkdownEditor call site threads the owning environment explicitly.
One consistency finding on the new no-environment GitHub attachment branch (inline).
Posted via Macroscope — UI Consistency
The no-environment branch rendered a raw img with inline geometry and the browser's broken-image glyph on failure; it now shares the block layout and the unavailable chip of the proxied path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Images embedded in pull request bodies rendered as broken placeholders: GitHub serves
github.com/user-attachments/assets/…uploads only to an authenticated viewer, and the browser sends no GitHub cookies from the app's origin (verified: unauthenticated fetch → 404, with aghtoken → 200).Fixed by routing those URLs through the existing signed asset flow. The markdown image classifier in
client-runtimereturns a newGitHubAttachmentsource, clients request a signed/api/assetsURL for it over the authenticated WebSocket, and the server resolves GitHub's redirect with theghCLI token (cached, single-flight) and answers 302 to the short-lived signed storage URL. Image bytes flow from storage straight to the browser; the token never leaves the server. Claim expiry is bucketed so repeat issuances stay byte-identical and browser caches hit. Web and mobile both render through the shared classifier; public-repo uploads resolve anonymously ifghis not authenticated.Verified end to end in a local web run against a private-repo PR — before/after screenshots in this comment.
Changes made by Claude Fable 5 (Claude Code).
🤖 Generated with Claude Code
Note
Add
github-attachmentasset type to render private GitHub images in PR bodiesgithub-attachmentvariant inAssetResource, asset claims, andResolvedAsset, with strict validation viaisGitHubUserAttachmentUrlin assets.tsGitHubAttachmentProxyuses cachedgh auth tokento follow GitHub's redirect and respond with a 302 to the storage URL (privatemax-age=240), or 404 on failureclassifyMarkdownImageSourcein markdownImages.ts andattachmentFromLinein pullRequestMarkdown.logic.ts now use the shared validator for GitHub attachment link detectionissueAssetUrlrejects non-allowlisted remote URLs withAssetRemoteUrlValidationError; any caller passing an unvalidated URL that previously fell through to other handling will now receive this errorMacroscope summarized 42a2295.