Conversation
|
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:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
21bc6b3 to
7e8f55b
Compare
There was a problem hiding this comment.
Effect service conventions: one finding on the new GitHub attachment proxy route. See the inline comment.
Secondary note: the new proxy branch in apps/server/src/http.ts changes backend behavior (token acquisition, upstream fetch, content-type gating) but is only covered indirectly by the new AssetAccess signing test. A focused test using test layers for ProcessRunner/HttpClient would lock in the 404/500 paths.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Reviewed the changed TypeScript against the Effect service conventions. Two findings in apps/server: a semantically distinct validation failure reused an unrelated error tag (caller-visible message no longer describes the failure), and the new asset-proxy backend behavior lacks focused tests. Import/namespace usage, Context.Service shape, dependency acquisition (yield* ProcessRunner.ProcessRunner), and the client-side changes look consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
b4165b5 to
594f2c9
Compare
There was a problem hiding this comment.
One finding: the ProcessRunner layer is now provided to browserApiCorsLayer, which never uses it, while assetRouteLayer (the actual consumer, via proxyGitHubUserAttachment) has no provider.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One finding: the ProcessRunner layer is now provided to browserApiCorsLayer, which never uses it, while assetRouteLayer (the actual consumer, via proxyGitHubUserAttachment) has no provider.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One new finding on ProcessRunner layer wiring in apps/server/src/http.ts. The per-request Effect.provide(ProcessRunner.layer) inside assetRouteLayer (line 268) is unchanged from the previous review and still applies.
Posted via Macroscope — Effect Service Conventions
594f2c9 to
e014682
Compare
ApprovabilityVerdict: Needs human review This PR introduces a new authenticated proxy mechanism for loading private GitHub repository images, including token retrieval via shell command and external HTTP requests with bearer authentication. The security implications and new capability scope warrant human review. You can customize Macroscope's approvability policy. Learn more. |
|
I found an additional private-repository image URL shape that this fix does not currently cover. PR comments can embed committed screenshots as: These render as broken images in the T3 PR Summary view for the same reason as #6600: the final image request is anonymous. The current Could this PR, or a focused follow-up, support repository image URLs belonging to the current pull request repository? The server should strictly validate the GitHub host, owner/repository, ref, and path; fetch through the existing authenticated integration; require an |
Closes #6600.
Images embedded in pull requests from private repositories are fetched anonymously by the renderer, so GitHub answers with
404even when the environment’s GitHub CLI is authenticated.This adds a signed asset resource for GitHub user attachments. Pull request markdown requests that URL from the environment server, which validates the attachment host and path, retrieves the existing
ghcredential, and returns the authenticated image bytes. This keeps credentials out of the client and works for remote environments as well as desktop.Before
Seeded from public PR #6594 with an unreferenced private test attachment substituted at render time. No private pull request content is shown.
After
Testing
pnpm --filter ./apps/server test --run src/assets/AssetAccess.test.tspnpm --filter @t3tools/web typecheckpnpm --filter @t3tools/contracts typecheckpnpm --filter ./apps/server typecheckBuilt with GPT-5.6-sol using the Codex harness.
Note
Proxy GitHub user attachment images in pull requests through authenticated server endpoint
github-user-attachmentasset resource kind to the contracts and asset access layer, with URL validation enforcing strictgithub.com/user-attachments/assets/<uuid>paths.proxyGitHubUserAttachmentin http.ts that fetches a token viagh auth token, performs an authenticated GET, validates the response is an image, and streams it with safe headers.PullRequestMarkdownto detect GitHub attachment URLs in markdown images and rewrite them through the authenticated asset proxy.environmentIdthrough pull request summary, timeline, review annotation, and editor components to enable proxying in all PR markdown contexts.gh); requests return 404 if no token is available.Macroscope summarized e014682.
Note
Medium Risk
Introduces authenticated outbound proxying tied to
ghCLI availability and token state; URL validation limits scope but misconfiguration yields silent 404s for images.Overview
Fixes private-repo PR images that 404 when the client loads
github.com/user-attachments/assets/…without credentials.Adds a
github-user-attachmentsigned asset in contracts and the server asset pipeline: only stricthttps://github.com/user-attachments/assets/{uuid}URLs are accepted; invalid hosts/paths fail withAssetGitHubAttachmentUrlValidationError. The asset route resolves signed tokens to a server-side proxy that reads a GitHub token viagh auth token, fetches the image withAuthorization: Bearer, streams image/ responses only, and applies SVG sandbox CSP via an optionalcontentTypeonassetResponseHeaders. Missing or bad auth/upstream responses return 404 so tokens never reach the renderer.Web:
PullRequestMarkdowntakesenvironmentId, swaps GitHub user-attachment<img>sources throughuseAssetUrl, and passes a custom renderer intoChatMarkdownvia newimageComponent. PR summary, timeline, review annotation, and markdown editor passenvironmentIdthrough. RPCassetsCreateUrlissues URLs for this resource without workspace context.Tests cover signing/validation in
AssetAccessand proxy behavior inhttp.test.ts.Reviewed by Cursor Bugbot for commit e014682. Bugbot is set up for automated code reviews on this repo. Configure here.