From 05fa731768332349fe4cc6be38f886c7d9d928c7 Mon Sep 17 00:00:00 2001 From: Tim Biggs Date: Sun, 1 Nov 2026 19:45:55 +0000 Subject: [PATCH 1/4] fix(server): private repository attachments load in pull requests A private repository's `github.com/user-attachments/assets/` URL is a 404 to any token, so the media fetch came back empty and clients drew blank tiles. When that URL is a 404, ask GitHub to render it in the change request's repository, which yields a short-lived signed link, and fetch that instead. Mobile and web now send the repository with the media resource. Co-Authored-By: Claude Opus 5.5 --- .../pull-requests/PullRequestMarkdown.tsx | 4 +- .../pull-requests/PullRequestMedia.tsx | 26 +-- .../pullRequestMediaSource.test.ts | 3 +- .../pull-requests/pullRequestMediaSource.ts | 8 +- .../src/lib/useMediaVideoPlayback.test.tsx | 4 +- apps/server/src/assets/AssetAccess.test.ts | 10 +- apps/server/src/assets/AssetAccess.ts | 6 + .../src/assets/sourceControlMediaResponse.ts | 13 +- apps/web/src/components/ChatMarkdown.tsx | 13 +- .../pullRequest/PullRequestDetailPanel.tsx | 4 +- .../pullRequest/PullRequestMarkdown.tsx | 13 +- packages/contracts/src/assets.ts | 4 +- .../source-control-core/src/server/driver.ts | 2 + .../src/server/gitHubMediaFetch.test.ts | 191 ++++++++++++++++-- .../src/server/gitHubMediaFetch.ts | 72 ++++++- .../src/shared/gitHubMedia.ts | 7 +- 16 files changed, 322 insertions(+), 58 deletions(-) diff --git a/apps/mobile/src/features/pull-requests/PullRequestMarkdown.tsx b/apps/mobile/src/features/pull-requests/PullRequestMarkdown.tsx index e09963c2bf32a..fb524764987c8 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestMarkdown.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestMarkdown.tsx @@ -55,7 +55,9 @@ export function PullRequestMarkdownRepository(props: { projectId: target.input.projectId, }); const cwd = project?.workspaceRoot; - const media = cwd ? { environmentId: target.environmentId, cwd } : null; + const media = cwd + ? { environmentId: target.environmentId, cwd, repository: target.input.repository } + : null; return ( diff --git a/apps/mobile/src/features/pull-requests/PullRequestMedia.tsx b/apps/mobile/src/features/pull-requests/PullRequestMedia.tsx index 54d3a82227731..95a062af36088 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestMedia.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestMedia.tsx @@ -1,12 +1,10 @@ import { createContext, useContext, useState } from "react"; -import { pullRequestMediaSource, pullRequestImageSource } from "./pullRequestMediaSource"; -import { useFilePreviewGallery } from "../../components/FilePreviewGallery"; -import { sourceControlClients } from "@t3tools/client-runtime/source-control-clients"; import { - SOURCE_CONTROL_MEDIA_ASSET, - type AssetResource, - type EnvironmentId, -} from "@t3tools/contracts"; + pullRequestMediaSource, + pullRequestImageSource, + type PullRequestMediaLocation, +} from "./pullRequestMediaSource"; +import { useFilePreviewGallery } from "../../components/FilePreviewGallery"; import { videoMimeType } from "@t3tools/shared/video"; import { Pressable, View } from "react-native"; import { Image } from "expo-image"; @@ -25,25 +23,19 @@ import { export const PullRequestMediaVisibleContext = createContext(true); -export const PullRequestMediaContext = createContext<{ - readonly environmentId: EnvironmentId; - readonly cwd: string; -} | null>(null); +export const PullRequestMediaContext = createContext(null); export function usePullRequestMediaUrl(url: string) { const context = useContext(PullRequestMediaContext); - const fetchUrl = sourceControlClients.mediaUrl(url)?.fetchUrl; - const resource: AssetResource | null = - context && fetchUrl - ? { _tag: SOURCE_CONTROL_MEDIA_ASSET, cwd: context.cwd, url: fetchUrl } - : null; + const source = pullRequestMediaSource(url, context); + const resource = source.resource ?? null; const asset = useAssetUrlState(context?.environmentId ?? null, resource); const refresh = useRefreshAssetUrl(context?.environmentId ?? null, resource); return { uri: asset._tag === "Success" ? asset.url : resource ? null : url, cacheKey: context && resource ? assetImageCacheKey(context.environmentId, resource) : undefined, unavailable: resource !== null && asset._tag === "Failure", - source: pullRequestMediaSource(url, context), + source, resolvePlaybackUri: resource ? async () => (await refresh()) ?? null : undefined, }; } diff --git a/apps/mobile/src/features/pull-requests/pullRequestMediaSource.test.ts b/apps/mobile/src/features/pull-requests/pullRequestMediaSource.test.ts index 3651b28402328..9c8c4e9414b2f 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestMediaSource.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestMediaSource.test.ts @@ -3,7 +3,7 @@ import { EnvironmentId } from "@t3tools/contracts"; import { filePreviewGallery } from "../../lib/filePreviewGallery"; import { pullRequestImageSource } from "./pullRequestMediaSource"; -const location = { environmentId: EnvironmentId.make("test"), cwd: "/repo" }; +const location = { environmentId: EnvironmentId.make("test"), cwd: "/repo", repository: "o/r" }; const url = "https://github.com/user-attachments/assets/abc-123"; describe("PR media previews", () => { @@ -14,5 +14,6 @@ describe("PR media previews", () => { const gallery = filePreviewGallery(selected, [first, second]); expect(gallery).toEqual([first, selected]); expect(gallery.every((item) => "resource" in item)).toBe(true); + expect(first).toMatchObject({ resource: { repository: "o/r" } }); }); }); diff --git a/apps/mobile/src/features/pull-requests/pullRequestMediaSource.ts b/apps/mobile/src/features/pull-requests/pullRequestMediaSource.ts index 075eccfc38d9b..8eb4bee5a11c9 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestMediaSource.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestMediaSource.ts @@ -5,6 +5,7 @@ import type { FilePreviewItem } from "../../components/FilePreviewModal.types"; export interface PullRequestMediaLocation { readonly environmentId: EnvironmentId; readonly cwd: string; + readonly repository: string; } /** Keep the original resource identity so previewers can resolve and renew its capability. */ @@ -13,7 +14,12 @@ export function pullRequestMediaSource(url: string, location: PullRequestMediaLo return location && fetchUrl ? { environmentId: location.environmentId, - resource: { _tag: SOURCE_CONTROL_MEDIA_ASSET, cwd: location.cwd, url: fetchUrl }, + resource: { + _tag: SOURCE_CONTROL_MEDIA_ASSET, + cwd: location.cwd, + repository: location.repository, + url: fetchUrl, + }, } : { uri: url }; } diff --git a/apps/mobile/src/lib/useMediaVideoPlayback.test.tsx b/apps/mobile/src/lib/useMediaVideoPlayback.test.tsx index 261017c49b06e..db8b2e605e243 100644 --- a/apps/mobile/src/lib/useMediaVideoPlayback.test.tsx +++ b/apps/mobile/src/lib/useMediaVideoPlayback.test.tsx @@ -197,7 +197,9 @@ function setup(kind: "thread" | "PR" | "files", compact = false) { /> ) : ( - + { ); it.effect("serves a host's credentialed media through that host's driver", () => { - const fetched: Array<{ url: string; headers: Readonly> }> = []; + const fetched: Array = []; const url = "https://github.com/owner/repo/blob/main/docs/shot.png"; const media = sourceControlDefinitions.mediaUrl(url)!; const layerDrivers = Layer.succeed(BuiltInDrivers.SourceControlDrivers, [ @@ -1492,7 +1493,7 @@ describe("AssetAccess", () => { // Every client and server release spells the resource this way on the wire. const { resource } = yield* decodeAssetCreateUrlInput({ - resource: { _tag: "github-media", cwd: "/repo", url }, + resource: { _tag: "github-media", cwd: "/repo", url, repository: "owner/repo" }, }); const issued = yield* issueAssetUrl({ resource }); const suffix = issued.relativeUrl.slice(`${ASSET_ROUTE_PREFIX}/`.length); @@ -1502,6 +1503,7 @@ describe("AssetAccess", () => { kind: "source-control-media", url: media.fetchUrl, cwd: "/repo", + repository: "owner/repo", expiresAt: issued.expiresAt, }); if (asset?.kind !== "source-control-media") return; @@ -1510,7 +1512,9 @@ describe("AssetAccess", () => { range: "bytes=0-4", cookie: "session", }); - expect(fetched).toEqual([{ url: media.fetchUrl, headers: { range: "bytes=0-4" } }]); + expect(fetched).toEqual([ + { url: media.fetchUrl, repository: "owner/repo", headers: { range: "bytes=0-4" } }, + ]); expect(response.status).toBe(206); // The upstream sent no type, so it comes from the file name the host gave the media. expect(response.headers["content-type"]).toBe("image/png"); diff --git a/apps/server/src/assets/AssetAccess.ts b/apps/server/src/assets/AssetAccess.ts index 41f0d8e3a7e2b..f8383562b8bc5 100644 --- a/apps/server/src/assets/AssetAccess.ts +++ b/apps/server/src/assets/AssetAccess.ts @@ -175,6 +175,7 @@ const AssetClaimsSchema = Schema.Union([ /** A host's `fetchUrl`, narrowed at mint time; the signature is what keeps it there. */ url: Schema.String, cwd: Schema.String, + repository: Schema.optionalKey(Schema.String), expiresAt: Schema.Number, }), ]); @@ -203,6 +204,7 @@ export type ResolvedAsset = readonly kind: "source-control-media"; readonly url: string; readonly cwd: string; + readonly repository?: string; readonly expiresAt: number; }; @@ -841,6 +843,9 @@ export const issueAssetUrl = Effect.fn("AssetAccess.issueAssetUrl")(function* (i kind: SOURCE_CONTROL_MEDIA_ASSET, url: media.fetchUrl, cwd: input.resource.cwd, + ...(input.resource.repository === undefined + ? {} + : { repository: input.resource.repository }), expiresAt, }; fileName = media.fileName; @@ -959,6 +964,7 @@ export const resolveAsset = Effect.fn("AssetAccess.resolveAsset")(function* ( kind: "source-control-media", url: claims.url, cwd: claims.cwd, + ...(claims.repository === undefined ? {} : { repository: claims.repository }), expiresAt: claims.expiresAt, } satisfies ResolvedAsset; } diff --git a/apps/server/src/assets/sourceControlMediaResponse.ts b/apps/server/src/assets/sourceControlMediaResponse.ts index 96e96f32a178a..f036094d8c3d6 100644 --- a/apps/server/src/assets/sourceControlMediaResponse.ts +++ b/apps/server/src/assets/sourceControlMediaResponse.ts @@ -25,7 +25,12 @@ const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inlin export const sourceControlMediaResponse = Effect.fn( "SourceControlMediaResponse.sourceControlMediaResponse", )(function* ( - asset: { readonly url: string; readonly cwd: string; readonly expiresAt: number }, + asset: { + readonly url: string; + readonly cwd: string; + readonly repository?: string; + readonly expiresAt: number; + }, requestHeaders: Record, ) { const remainingSeconds = Math.floor((asset.expiresAt - (yield* Clock.currentTimeMillis)) / 1000); @@ -46,7 +51,11 @@ export const sourceControlMediaResponse = Effect.fn( const value = requestHeaders[name]; if (value !== undefined) forwarded[name] = value; } - const response = yield* fetchMedia({ url: asset.url, headers: forwarded }); + const response = yield* fetchMedia({ + url: asset.url, + ...(asset.repository === undefined ? {} : { repository: asset.repository }), + headers: forwarded, + }); if (response === null) return HttpServerResponse.empty({ status: 502, headers }); // It carries none of the upstream entity headers: a `content-length` with no body behind it // holds the connection open until the browser gives up on it. diff --git a/apps/web/src/components/ChatMarkdown.tsx b/apps/web/src/components/ChatMarkdown.tsx index 928d21fc46b97..0004120eac9cc 100644 --- a/apps/web/src/components/ChatMarkdown.tsx +++ b/apps/web/src/components/ChatMarkdown.tsx @@ -247,6 +247,7 @@ interface ChatMarkdownProps { /** Loads media a source control host keeps behind a credential through the server, which holds `cwd`'s; without it a private repository's uploads load unauthenticated and 404. */ sourceControlMedia?: boolean | undefined; + sourceControlMediaRepository?: string | undefined; headingLevelOffset?: number | undefined; } @@ -504,6 +505,7 @@ function useChatMarkdownState({ renderContextReference, headingLevelOffset = 0, sourceControlMedia = false, + sourceControlMediaRepository, }: ChatMarkdownProps) { const resolvedTheme = useRenderedAppearance(); const markdownRef = useRef(null); @@ -901,6 +903,7 @@ function useChatMarkdownState({ expandMedia, fileLinkChip, sourceControlMedia, + sourceControlMediaRepository, renderContextReference, headingLevelOffset, imageBaseDir, @@ -1375,6 +1378,7 @@ const CHAT_MARKDOWN_COMPONENTS = { cwd, environmentId, sourceControlMedia, + sourceControlMediaRepository, imageBaseDir, threadRef, workspaceMutationId, @@ -1457,7 +1461,14 @@ const CHAT_MARKDOWN_COMPONENTS = { return ( | null, threadRef: ScopedThreadRef | null, + repository: string, ) { return { remarkPlugins: pullRequestAutolinkPlugins(detail), lineBreaks: sourceControlClients.find(detail?.provider ?? "")?.hardLineBreaks ?? false, threadRef, + repository, }; } @@ -511,7 +513,7 @@ export function PullRequestDetailPanel({ : (sourceControlClients.parseChangeRequestUrl(detail.url)?.link.repositoryUrl ?? null); const hostClient = sourceControlClients.get(detail?.provider); const openOnHostText = openOnHostLabel(hostClient); - const markdownContext = pullRequestMarkdownContext(detail, threadRef); + const markdownContext = pullRequestMarkdownContext(detail, threadRef, reference.repository); const authorProfileUrl = detail?.author && repositoryUrl !== null ? hostClient.authorProfileUrl(detail.author.login, repositoryUrl) diff --git a/apps/web/src/components/pullRequest/PullRequestMarkdown.tsx b/apps/web/src/components/pullRequest/PullRequestMarkdown.tsx index 566d34910d446..74b88e681774a 100644 --- a/apps/web/src/components/pullRequest/PullRequestMarkdown.tsx +++ b/apps/web/src/components/pullRequest/PullRequestMarkdown.tsx @@ -23,6 +23,8 @@ export const PullRequestMarkdownContext = createContext<{ remarkPlugins: PluggableList; lineBreaks: boolean; threadRef: ScopedThreadRef | null; + /** `owner/name` of the change request, which the host needs to resolve its private uploads. */ + repository?: string; } | null>(null); function PullRequestHostVideo({ @@ -30,13 +32,20 @@ function PullRequestHostVideo({ cwd, url, fetchUrl, + repository, }: { environmentId: EnvironmentId; cwd: string; url: string; fetchUrl: string; + repository: string | undefined; }) { - const resource: AssetResource = { _tag: SOURCE_CONTROL_MEDIA_ASSET, cwd, url: fetchUrl }; + const resource: AssetResource = { + _tag: SOURCE_CONTROL_MEDIA_ASSET, + cwd, + url: fetchUrl, + ...(repository === undefined ? {} : { repository }), + }; const assetUrl = useAssetUrlState(environmentId, resource); const refreshAssetUrl = useAssetUrlRefresh(environmentId, resource); const src = @@ -92,6 +101,7 @@ export function PullRequestMarkdown({ {...(context ? { extraRemarkPlugins: context.remarkPlugins } : {})} lineBreaks={lineBreaks ?? context?.lineBreaks ?? false} sourceControlMedia + sourceControlMediaRepository={context?.repository} /> ); } @@ -104,6 +114,7 @@ export function PullRequestMarkdown({ cwd={cwd} url={segment.url} fetchUrl={hostMedia.fetchUrl} + repository={context?.repository} /> ); } diff --git a/packages/contracts/src/assets.ts b/packages/contracts/src/assets.ts index da3eb0e42062f..36b8cbe7ba017 100644 --- a/packages/contracts/src/assets.ts +++ b/packages/contracts/src/assets.ts @@ -54,10 +54,12 @@ export const AssetResource = Schema.Union([ }), // Media a change request body points at on its host. A private repository serves these only // to a request that carries a credential, which the client has none of, so the server fetches - // them with the credential the host's driver holds for the repository at `cwd`. + // them with the credential the host's driver holds for the repository at `cwd`. `repository` is + // the change request's own `owner/name`: GitHub resolves a private upload only within one. Schema.TaggedStruct(SOURCE_CONTROL_MEDIA_ASSET, { cwd: TrimmedNonEmptyString.check(Schema.isMaxLength(ASSET_PATH_MAX_LENGTH)), url: TrimmedNonEmptyString.check(Schema.isMaxLength(2048)), + repository: Schema.optionalKey(TrimmedNonEmptyString.check(Schema.isMaxLength(256))), }), ]); export type AssetResource = typeof AssetResource.Type; diff --git a/packages/source-control-core/src/server/driver.ts b/packages/source-control-core/src/server/driver.ts index 936742b0659d0..605acca4acb19 100644 --- a/packages/source-control-core/src/server/driver.ts +++ b/packages/source-control-core/src/server/driver.ts @@ -23,6 +23,8 @@ import type * as SourceControlRateLimit from "./SourceControlRateLimit.ts"; export interface SourceControlMediaRequest { /** A `fetchUrl` this host's definition returned from `mediaUrl`. */ readonly url: string; + /** `owner/name` of the change request the media belongs to, when the client knows it. */ + readonly repository?: string; /** Request headers to forward, such as `range`. */ readonly headers: Readonly>; } diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts index 07e2bbd8f724b..4b2b0c582f7c7 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts @@ -6,6 +6,35 @@ import { HttpClient, HttpClientResponse } from "effect/http"; import * as GitHubCredentials from "./GitHubCredentials.ts"; import * as GitHubMediaFetch from "./gitHubMediaFetch.ts"; +import * as GitHubRequestPolicy from "./GitHubRequestPolicy.ts"; +import * as GitHubRest from "./GitHubRest.ts"; + +const signedIn = Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host) => + Effect.succeed({ + host, + token: Redacted.make("signed-in"), + source: "gh" as const, + fingerprint: "fingerprint", + }), +}); + +const renderedBy = (rest: GitHubRest.GitHubRest["Service"]["rest"]) => + Layer.mock(GitHubRest.GitHubRest)({ rest }); + +const neverRendered = renderedBy(() => Effect.die("rendered markdown for media it did not need")); + +const renderedHtml = (body: string) => ({ + status: 200, + headers: {}, + body, + truncated: false, + invalidUtf8: false, +}); + +const ATTACHMENT_ID = "47b6c0b9-cc20-4b2f-a3f4-60a7150ad3b8"; +const ATTACHMENT_URL = `https://github.com/user-attachments/assets/${ATTACHMENT_ID}`; +const SIGNED_LINK = `https://private-user-images.githubusercontent.com/1/2-${ATTACHMENT_ID}.png?jwt=signed`; describe("GitHubMediaFetch", () => { it.effect("loads private media immediately after login with the GitHub credential", () => { @@ -24,17 +53,20 @@ describe("GitHubMediaFetch", () => { expect(authorizations).toEqual([undefined, "Bearer signed-in", "Bearer signed-in"]); }).pipe( Effect.provide( - Layer.mock(GitHubCredentials.GitHubCredentials)({ - get: (host) => - ++lookups === 1 - ? Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host })) - : Effect.succeed({ - host, - token: Redacted.make("signed-in"), - source: "gh" as const, - fingerprint: "fingerprint", - }), - }), + Layer.merge( + Layer.mock(GitHubCredentials.GitHubCredentials)({ + get: (host) => + ++lookups === 1 + ? Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host })) + : Effect.succeed({ + host, + token: Redacted.make("signed-in"), + source: "gh" as const, + fingerprint: "fingerprint", + }), + }), + neverRendered, + ), ), Effect.provideService( HttpClient.HttpClient, @@ -59,8 +91,10 @@ describe("GitHubMediaFetch", () => { const authorizations: Array<[string, string | undefined]> = []; return Effect.gen(function* () { const fetchMedia = yield* GitHubMediaFetch.make; + // A public attachment redirects, so it never spends quota on a signed link. const response = yield* fetchMedia({ url: "https://github.com/user-attachments/assets/1a1842fb", + repository: "owner/repo", headers: { range: "bytes=0-1" }, }); expect(response?.status).toBe(200); @@ -69,17 +103,7 @@ describe("GitHubMediaFetch", () => { ["objects.example.com", undefined], ]); }).pipe( - Effect.provide( - Layer.mock(GitHubCredentials.GitHubCredentials)({ - get: (host) => - Effect.succeed({ - host, - token: Redacted.make("signed-in"), - source: "gh" as const, - fingerprint: "fingerprint", - }), - }), - ), + Effect.provide(Layer.merge(signedIn, neverRendered)), Effect.provideService( HttpClient.HttpClient, HttpClient.make((request) => { @@ -101,4 +125,127 @@ describe("GitHubMediaFetch", () => { Effect.scoped, ); }); + + it.effect("loads a private attachment through the signed link GitHub renders for it", () => { + const requests: Array<[string, string | undefined, string | undefined]> = []; + const rendered: Array = []; + return Effect.gen(function* () { + const fetchMedia = yield* GitHubMediaFetch.make; + const request = { + url: ATTACHMENT_URL, + repository: "owner/private", + headers: { range: "bytes=0-1" }, + }; + expect((yield* fetchMedia(request))?.status).toBe(206); + // A video's range requests must not each spend quota on a new link. + expect((yield* fetchMedia(request))?.status).toBe(206); + expect(rendered).toEqual([ + { + host: "github.com", + operation: "renderAttachmentLink", + method: "POST", + path: "markdown", + body: { text: `![](${ATTACHMENT_URL})`, mode: "gfm", context: "owner/private" }, + allowReserve: true, + }, + ]); + expect(requests.slice(0, 2)).toEqual([ + ["github.com", "Bearer signed-in", "bytes=0-1"], + ["private-user-images.githubusercontent.com", undefined, "bytes=0-1"], + ]); + }).pipe( + Effect.provide( + Layer.merge( + signedIn, + renderedBy((input) => { + rendered.push(input); + return Effect.succeed( + renderedHtml(`

`), + ); + }), + ), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + const url = new URL(request.url); + requests.push([url.hostname, request.headers.authorization, request.headers.range]); + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response(null, { status: url.hostname === "github.com" ? 404 : 206 }), + ), + ); + }), + ), + Effect.scoped, + ); + }); + + it.effect("answers 404 when GitHub gives no signed link for the attachment", () => + Effect.gen(function* () { + for (const [repository, rendering] of [ + // GitHub leaves the URL unsigned for some repositories, such as ones the account only reads. + ["owner/other", Effect.succeed(renderedHtml(``))], + // A link it did not sign, or to another file, must not be fetched. + [ + "owner/other", + Effect.succeed(renderedHtml(``)), + ], + [ + "owner/other", + Effect.succeed( + renderedHtml( + '', + ), + ), + ], + [ + "owner/other", + Effect.fail( + new GitHubRequestPolicy.GitHubApiNotFoundError({ + host: "github.com", + operation: "renderAttachmentLink", + }), + ), + ], + ] as const) { + const hosts: Array = []; + const response = yield* Effect.gen(function* () { + const fetchMedia = yield* GitHubMediaFetch.make; + return yield* fetchMedia({ url: ATTACHMENT_URL, repository, headers: {} }); + }).pipe( + Effect.provide(renderedBy(() => rendering)), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + hosts.push(new URL(request.url).hostname); + return Effect.succeed( + HttpClientResponse.fromWeb(request, new Response(null, { status: 404 })), + ); + }), + ), + ); + expect(response?.status).toBe(404); + expect(hosts).toEqual(["github.com"]); + } + }).pipe(Effect.provide(signedIn), Effect.scoped), + ); + + it.effect("spends nothing on a signed link without a repository to render it in", () => + Effect.gen(function* () { + const fetchMedia = yield* GitHubMediaFetch.make; + const response = yield* fetchMedia({ url: ATTACHMENT_URL, headers: {} }); + expect(response?.status).toBe(404); + }).pipe( + Effect.provide(Layer.merge(signedIn, neverRendered)), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.succeed(HttpClientResponse.fromWeb(request, new Response(null, { status: 404 }))), + ), + ), + Effect.scoped, + ), + ); }); diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.ts b/packages/source-control-github/src/server/gitHubMediaFetch.ts index 4dfb2a23e6341..c1c24a7037c87 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.ts @@ -1,5 +1,7 @@ import type { SourceControlMediaRequest } from "@t3tools/source-control-core/server/driver"; +import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Redacted from "effect/Redacted"; import { FetchHttpClient, @@ -8,9 +10,10 @@ import { type HttpClientResponse, } from "effect/http"; -import { GITHUB_CREDENTIALED_MEDIA_HOSTS } from "../shared/gitHubMedia.ts"; +import { GITHUB_CREDENTIALED_MEDIA_HOSTS, gitHubAttachmentId } from "../shared/gitHubMedia.ts"; import * as GitHubCredentials from "./GitHubCredentials.ts"; +import * as GitHubRest from "./GitHubRest.ts"; /** * Everything a redirect leads to — the presigned object stores GitHub hands assets off to — @@ -27,10 +30,26 @@ const isCredentialedHost = (url: string) => { const MAX_REDIRECTS = 3; const MANUAL_REDIRECT: RequestInit = { redirect: "manual" }; +const SIGNED_ATTACHMENT_HOST = "private-user-images.githubusercontent.com"; +const IMAGE_SOURCE_PATTERN = /]*?\bsrc="([^"]+)"/u; +// GitHub signs these links for five minutes. +const SIGNED_LINK_CACHE_LIFETIME = "1 minute"; + +const signedAttachmentLink = (html: string, attachmentId: string) => { + const source = IMAGE_SOURCE_PATTERN.exec(html)?.[1]?.replaceAll("&", "&"); + if (source === undefined || !URL.canParse(source)) return null; + const { protocol, hostname, pathname } = new URL(source); + return protocol === "https:" && + hostname === SIGNED_ATTACHMENT_HOST && + pathname.includes(attachmentId) + ? source + : null; +}; export const make = Effect.gen(function* () { const credentials = yield* GitHubCredentials.GitHubCredentials; const httpClient = HttpClient.withScope(yield* HttpClient.HttpClient); + const api = yield* GitHubRest.GitHubRest; // Both media hosts are served by github.com's account, which is the host `gh` stores it under. // Read on every fetch, so media loads as soon as the user signs in. @@ -39,9 +58,12 @@ export const make = Effect.gen(function* () { Effect.orElseSucceed(() => null), ); - return Effect.fn("GitHubMediaFetch.fetchMedia")(function* (request: SourceControlMediaRequest) { - const bearer = yield* token; - let target = request.url; + const follow = Effect.fnUntraced(function* ( + url: string, + headers: SourceControlMediaRequest["headers"], + bearer: Redacted.Redacted | null, + ) { + let target = url; for (let hop = 0; ; hop += 1) { const authorization = bearer !== null && isCredentialedHost(target) ? `Bearer ${Redacted.value(bearer)}` : null; @@ -49,7 +71,7 @@ export const make = Effect.gen(function* () { .execute( HttpClientRequest.get(target).pipe( HttpClientRequest.setHeaders({ - ...request.headers, + ...headers, // The bytes are streamed straight through, so never let an encoding layer in. "accept-encoding": "identity", ...(authorization === null ? {} : { authorization }), @@ -65,4 +87,44 @@ export const make = Effect.gen(function* () { target = next.toString(); } }); + + // A private repository's attachment URL is a 404 to any token. Only GitHub's own rendering of it + // says where the file is, as a short-lived signed link that needs no credential. + const signedLinks = yield* Cache.makeWith( + Effect.fnUntraced(function* (key: { + readonly url: string; + readonly attachmentId: string; + readonly repository: string; + }) { + const rendered = yield* api.rest({ + host: "github.com", + operation: "renderAttachmentLink", + method: "POST", + path: "markdown", + body: { text: `![](${key.url})`, mode: "gfm", context: key.repository }, + allowReserve: true, + }); + return signedAttachmentLink(rendered.body, key.attachmentId); + }), + { + capacity: 256, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value !== null ? SIGNED_LINK_CACHE_LIFETIME : 0, + }, + ); + + return Effect.fn("GitHubMediaFetch.fetchMedia")(function* (request: SourceControlMediaRequest) { + const bearer = yield* token; + const response = yield* follow(request.url, request.headers, bearer); + const attachmentId = gitHubAttachmentId(request.url); + if (response?.status !== 404 || request.repository === undefined || attachmentId === null) { + return response; + } + const link = yield* Cache.get(signedLinks, { + url: request.url, + attachmentId, + repository: request.repository, + }).pipe(Effect.orElseSucceed(() => null)); + return link === null ? response : yield* follow(link, request.headers, bearer); + }); }); diff --git a/packages/source-control-github/src/shared/gitHubMedia.ts b/packages/source-control-github/src/shared/gitHubMedia.ts index eec80c071fd1b..c59a976b1e4ce 100644 --- a/packages/source-control-github/src/shared/gitHubMedia.ts +++ b/packages/source-control-github/src/shared/gitHubMedia.ts @@ -14,13 +14,18 @@ export const GITHUB_CREDENTIALED_MEDIA_HOSTS: ReadonlySet = new Set([ LFS_HOST, ]); -const ATTACHMENT_PATH_PATTERN = /^\/user-attachments\/assets\/[\w-]+$/u; +const ATTACHMENT_PATH_PATTERN = /^\/user-attachments\/assets\/([\w-]+)$/u; const LEGACY_ATTACHMENT_PATH_PATTERN = /^\/[^/]+\/[^/]+\/assets\/\d+\/[\w-]+$/u; /** `blob` and `raw` both address file bytes; `raw` is the one the token is honoured on. */ const REPOSITORY_FILE_PATTERN = /^\/([^/]+)\/([^/]+)\/(?:raw|blob)\/(.*[^/])$/u; /** GitHub rejects 40-hex branch and tag names, and serves no SHA-256 repositories. */ const COMMIT_SHA_PATTERN = /^[0-9a-f]{40}$/u; +/** The id of an uploaded attachment, which the file name of its signed link also carries. */ +export function gitHubAttachmentId(fetchUrl: string): string | null { + return ATTACHMENT_PATH_PATTERN.exec(new URL(fetchUrl).pathname)?.[1] ?? null; +} + function fileName(fetchUrl: string): string { const segment = new URL(fetchUrl).pathname.split("/").pop() ?? ""; let decoded: string; From 23c7fdfd231a53085fefdd9a14f1a8c8d0342656 Mon Sep 17 00:00:00 2001 From: Tim Biggs Date: Thu, 5 Nov 2026 08:16:37 +0000 Subject: [PATCH 2/4] fix(github): signed attachment links stay with the account that got them The signed-link cache was keyed only by attachment and repository, so after switching GitHub accounts or signing out, a link signed for the previous account was still served for up to a minute. Key it by the credential's fingerprint and render with that credential pinned, the way the PR API keys its caches, and render nothing without a credential. Co-Authored-By: Claude Opus 5.5 --- .../src/server/gitHubMediaFetch.test.ts | 58 +++++++++++++------ .../src/server/gitHubMediaFetch.ts | 19 +++++- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts index 4b2b0c582f7c7..538c2e4570874 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts @@ -19,8 +19,11 @@ const signedIn = Layer.mock(GitHubCredentials.GitHubCredentials)({ }), }); -const renderedBy = (rest: GitHubRest.GitHubRest["Service"]["rest"]) => - Layer.mock(GitHubRest.GitHubRest)({ rest }); +const renderedBy = ( + rest: GitHubRest.GitHubRest["Service"]["rest"], + credential: GitHubRest.GitHubRest["Service"]["credential"] = () => + Effect.succeed({ token: Redacted.make("signed-in"), fingerprint: "fingerprint" }), +) => Layer.mock(GitHubRest.GitHubRest)({ rest, credential }); const neverRendered = renderedBy(() => Effect.die("rendered markdown for media it did not need")); @@ -128,7 +131,8 @@ describe("GitHubMediaFetch", () => { it.effect("loads a private attachment through the signed link GitHub renders for it", () => { const requests: Array<[string, string | undefined, string | undefined]> = []; - const rendered: Array = []; + const rendered: Array<[string | undefined, GitHubRest.GitHubRestInput]> = []; + let account: string | null = "first"; return Effect.gen(function* () { const fetchMedia = yield* GitHubMediaFetch.make; const request = { @@ -139,15 +143,22 @@ describe("GitHubMediaFetch", () => { expect((yield* fetchMedia(request))?.status).toBe(206); // A video's range requests must not each spend quota on a new link. expect((yield* fetchMedia(request))?.status).toBe(206); + // Nor may another account, or no account, reuse a link GitHub signed for this one. + account = "second"; + expect((yield* fetchMedia(request))?.status).toBe(206); + account = null; + expect((yield* fetchMedia(request))?.status).toBe(404); + const rendering = { + host: "github.com", + operation: "renderAttachmentLink", + method: "POST", + path: "markdown", + body: { text: `![](${ATTACHMENT_URL})`, mode: "gfm", context: "owner/private" }, + allowReserve: true, + }; expect(rendered).toEqual([ - { - host: "github.com", - operation: "renderAttachmentLink", - method: "POST", - path: "markdown", - body: { text: `![](${ATTACHMENT_URL})`, mode: "gfm", context: "owner/private" }, - allowReserve: true, - }, + ["first", rendering], + ["second", rendering], ]); expect(requests.slice(0, 2)).toEqual([ ["github.com", "Bearer signed-in", "bytes=0-1"], @@ -157,12 +168,25 @@ describe("GitHubMediaFetch", () => { Effect.provide( Layer.merge( signedIn, - renderedBy((input) => { - rendered.push(input); - return Effect.succeed( - renderedHtml(`

`), - ); - }), + renderedBy( + (input) => + Effect.gen(function* () { + const pinned = yield* GitHubRequestPolicy.PinnedGitHubCredential; + rendered.push([pinned?.credentialFingerprint, input]); + return renderedHtml( + `

`, + ); + }), + (host) => + account === null + ? Effect.fail( + new GitHubRequestPolicy.GitHubApiAuthenticationError({ + host, + operation: "credential", + }), + ) + : Effect.succeed({ token: Redacted.make(account), fingerprint: account }), + ), ), ), Effect.provideService( diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.ts b/packages/source-control-github/src/server/gitHubMediaFetch.ts index c1c24a7037c87..2289b76ccfc38 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.ts @@ -2,6 +2,7 @@ import type { SourceControlMediaRequest } from "@t3tools/source-control-core/ser import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; +import * as Option from "effect/Option"; import * as Redacted from "effect/Redacted"; import { FetchHttpClient, @@ -13,6 +14,7 @@ import { import { GITHUB_CREDENTIALED_MEDIA_HOSTS, gitHubAttachmentId } from "../shared/gitHubMedia.ts"; import * as GitHubCredentials from "./GitHubCredentials.ts"; +import * as GitHubRequestPolicy from "./GitHubRequestPolicy.ts"; import * as GitHubRest from "./GitHubRest.ts"; /** @@ -89,9 +91,11 @@ export const make = Effect.gen(function* () { }); // A private repository's attachment URL is a 404 to any token. Only GitHub's own rendering of it - // says where the file is, as a short-lived signed link that needs no credential. + // says where the file is, as a short-lived signed link that needs no credential, so a link is + // reused only under the credential GitHub signed it for. const signedLinks = yield* Cache.makeWith( Effect.fnUntraced(function* (key: { + readonly fingerprint: string; readonly url: string; readonly attachmentId: string; readonly repository: string; @@ -120,11 +124,22 @@ export const make = Effect.gen(function* () { if (response?.status !== 404 || request.repository === undefined || attachmentId === null) { return response; } + const signer = yield* api.credential("github.com").pipe(Effect.option); + if (Option.isNone(signer)) return response; + const { token: signerToken, fingerprint } = signer.value; const link = yield* Cache.get(signedLinks, { + fingerprint, url: request.url, attachmentId, repository: request.repository, - }).pipe(Effect.orElseSucceed(() => null)); + }).pipe( + Effect.provideService(GitHubRequestPolicy.PinnedGitHubCredential, { + host: "github.com", + token: signerToken, + credentialFingerprint: fingerprint, + }), + Effect.orElseSucceed(() => null), + ); return link === null ? response : yield* follow(link, request.headers, bearer); }); }); From 146181c5964b8d6c74aa2016219dbcbc6104803a Mon Sep 17 00:00:00 2001 From: Tim Biggs Date: Sun, 8 Nov 2026 02:51:18 +0000 Subject: [PATCH 3/4] fix(github): load PR uploads from private repositories GitHub answers a token's request for an upload in a private repository with a plain 404, so only an SSO sign-in page led to a rendered signed URL and private repositories showed blank attachments. After a 404, look up the repository's visibility (cached for an hour) and render the signed URL only when it is private. Public repositories, ones the account cannot see, and requests without a repository keep their 404. Co-Authored-By: Claude Opus 5.5 --- .../src/server/gitHubMediaFetch.test.ts | 95 ++++++++++++++++--- .../src/server/gitHubMediaFetch.ts | 35 ++++++- 2 files changed, 117 insertions(+), 13 deletions(-) diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts index 8248a76e4b04d..4f67a71794670 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts @@ -7,14 +7,22 @@ import { HttpClient, HttpClientResponse } from "effect/http"; import * as GitHubCredentials from "./GitHubCredentials.ts"; import * as GitHubMediaFetch from "./gitHubMediaFetch.ts"; +import * as GitHubRequestPolicy from "./GitHubRequestPolicy.ts"; import * as GitHubRest from "./GitHubRest.ts"; const PR_REPOSITORY = "enterprise/private"; -/** An SSO organization: the upload URL answers with a sign-in page, a rendered signed URL with bytes. */ -const makeSsoFixture = (uploadUrl: string) => { +/** + * The upload URL answers with an SSO organization's sign-in page, or with the plain 404 a token gets + * in a private repository, a public one, or one the account cannot see. A signed URL has bytes. + */ +const makeUploadFixture = ( + uploadUrl: string, + repository: "sso" | "private" | "public" | "hidden" = "sso", +) => { const state = { account: "account-a", expired: new Set() }; const rendered: Array = []; + const lookups: Array = []; const signedUrls: Array = []; const requests: Array<{ url: string; authorization?: string; range?: string }> = []; const credential = (host: string) => @@ -27,8 +35,28 @@ const makeSsoFixture = (uploadUrl: string) => { const layer = Layer.mergeAll( Layer.mock(GitHubCredentials.GitHubCredentials)({ get: credential }), Layer.mock(GitHubRest.GitHubRest)({ - rest: (input) => - Effect.sync(() => { + rest: (input) => { + if (input.path !== "markdown") { + lookups.push(input.path); + return repository === "hidden" + ? Effect.fail( + new GitHubRequestPolicy.GitHubApiNotFoundError({ + host: input.host, + operation: input.operation, + }), + ) + : Effect.succeed({ + status: 200, + headers: {}, + body: JSON.stringify({ + full_name: PR_REPOSITORY, + private: repository !== "public", + }), + truncated: false, + invalidUtf8: false, + }); + } + return Effect.sync(() => { rendered.push(input); const signed = `https://private-user-images.githubusercontent.com/1/upload.mp4?jwt=${state.account}-${rendered.length}&x=1`; const inRepository = @@ -41,7 +69,8 @@ const makeSsoFixture = (uploadUrl: string) => { truncated: false, invalidUtf8: false, }; - }), + }); + }, }), Layer.succeed( HttpClient.HttpClient, @@ -57,9 +86,14 @@ const makeSsoFixture = (uploadUrl: string) => { if (request.url === uploadUrl) { return HttpClientResponse.fromWeb( request, - new Response("Sign in with SSO", { - headers: { "content-type": "text/html; charset=utf-8" }, - }), + repository === "sso" + ? new Response("Sign in with SSO", { + headers: { "content-type": "text/html; charset=utf-8" }, + }) + : new Response("Not Found", { + status: 404, + headers: { "content-type": "text/plain; charset=utf-8" }, + }), ); } expect(signedUrls).toContain(request.url); @@ -72,7 +106,7 @@ const makeSsoFixture = (uploadUrl: string) => { ), ), ); - return { layer, state, rendered, signedUrls, requests }; + return { layer, state, rendered, lookups, signedUrls, requests }; }; describe("GitHubMediaFetch", () => { @@ -180,7 +214,7 @@ describe("GitHubMediaFetch", () => { "renders SSO uploads in the PR repository and reuses the signed URL for a range", () => { const url = "https://github.com/user-attachments/assets/explicit-pr-context"; - const fixture = makeSsoFixture(url); + const fixture = makeUploadFixture(url); return Effect.gen(function* () { const fetchMedia = yield* GitHubMediaFetch.make; const request = { url, headers: {}, repository: PR_REPOSITORY }; @@ -205,9 +239,46 @@ describe("GitHubMediaFetch", () => { }, ); + it.effect( + "renders an upload in a private repository, where it is a plain 404 to any token", + () => { + const url = "https://github.com/user-attachments/assets/private-repository"; + const fixture = makeUploadFixture(url, "private"); + const request = { url, headers: {}, repository: PR_REPOSITORY }; + return Effect.gen(function* () { + const fetchMedia = yield* GitHubMediaFetch.make; + expect((yield* fetchMedia(request))?.status).toBe(200); + yield* TestClock.adjust("6 minutes"); + expect((yield* fetchMedia(request))?.status).toBe(200); + expect(fixture.rendered).toHaveLength(2); + // The repository's visibility outlives a signed URL. + expect(fixture.lookups).toEqual([`repos/${PR_REPOSITORY}`]); + }).pipe(Effect.provide(fixture.layer), Effect.scoped); + }, + ); + + it.effect("leaves a 404 alone unless the repository is known to be private", () => + Effect.gen(function* () { + for (const [repository, request] of [ + ["public", { repository: PR_REPOSITORY }], + ["hidden", { repository: PR_REPOSITORY }], + ["private", {}], + ] as const) { + const url = "https://github.com/user-attachments/assets/missing"; + const fixture = makeUploadFixture(url, repository); + const response = yield* Effect.gen(function* () { + const fetchMedia = yield* GitHubMediaFetch.make; + return yield* fetchMedia({ url, headers: {}, ...request }); + }).pipe(Effect.provide(fixture.layer)); + expect(response?.status).toBe(404); + expect(fixture.rendered).toEqual([]); + } + }).pipe(Effect.scoped), + ); + it.effect("renders a legacy upload in the repository its path names", () => { const url = `https://github.com/${PR_REPOSITORY}/assets/123/legacy`; - const fixture = makeSsoFixture(url); + const fixture = makeUploadFixture(url); return Effect.gen(function* () { const fetchMedia = yield* GitHubMediaFetch.make; const response = yield* fetchMedia({ url, headers: {}, repository: "personal/unrelated" }); @@ -220,7 +291,7 @@ describe("GitHubMediaFetch", () => { it.effect("renders a fresh signed URL after an account switch, early expiry, or its TTL", () => { const url = "https://github.com/user-attachments/assets/cache-lifecycle"; - const fixture = makeSsoFixture(url); + const fixture = makeUploadFixture(url); const request = { url, headers: {}, repository: PR_REPOSITORY }; return Effect.gen(function* () { const fetchMedia = yield* GitHubMediaFetch.make; diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.ts b/packages/source-control-github/src/server/gitHubMediaFetch.ts index b03c0d327a6ce..0391a022fe049 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.ts @@ -1,7 +1,12 @@ import type { SourceControlMediaRequest } from "@t3tools/source-control-core/server/driver"; +import { decodeJsonResult } from "@t3tools/shared/schemaJson"; +import * as Cache from "effect/Cache"; import * as Clock from "effect/Clock"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Redacted from "effect/Redacted"; +import * as Result from "effect/Result"; +import * as Schema from "effect/Schema"; import { FetchHttpClient, HttpClient, @@ -39,6 +44,7 @@ const REPOSITORY_PATTERN = /^[\w.-]+\/[\w.-]+$/u; // GitHub's signed upload links last about five minutes. const SIGNED_URL_CACHE_TTL_MS = 4 * 60_000; const SIGNED_URL_CACHE_MAX_ENTRIES = 64; +const decodeVisibility = decodeJsonResult(Schema.Struct({ private: Schema.Boolean })); const contentTypeOf = (response: HttpClientResponse.HttpClientResponse) => response.headers["content-type"]?.split(";", 1)[0]?.trim().toLowerCase() ?? ""; @@ -112,6 +118,25 @@ export const make = Effect.gen(function* () { } }); + // Only a private repository's uploads are worth rendering after a 404; in a public one the file + // is gone. + const privateRepositories = yield* Cache.makeWith( + Effect.fnUntraced(function* (repository: string) { + const [owner = "", name = ""] = repository.split("/"); + const response = yield* rest.rest({ + host: "github.com", + operation: "getRepository", + path: `repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}`, + }); + const decoded = decodeVisibility(response.body); + return Result.isSuccess(decoded) && decoded.success.private; + }), + { + capacity: SIGNED_URL_CACHE_MAX_ENTRIES, + timeToLive: (exit) => (Exit.isSuccess(exit) ? "1 hour" : 0), + }, + ); + // An organization with SAML SSO answers a token-authenticated upload request with its sign-in // page. Rendering the link as markdown in the repository's context hands back a signed URL. const renderSignedUrl = Effect.fnUntraced(function* (url: string, repository: string) { @@ -164,7 +189,15 @@ export const make = Effect.gen(function* () { request.headers, signedIn?.token ?? null, ); - if (response === null || contentTypeOf(response) !== "text/html") return response; + if (response === null) return response; + if (contentTypeOf(response) !== "text/html") { + // A private repository's upload is a plain 404 to any token. + if (response.status !== 404 || repository === undefined) return response; + const isPrivate = yield* Cache.get(privateRepositories, repository).pipe( + Effect.orElseSucceed(() => false), + ); + if (!isPrivate) return response; + } } if (cacheKey === null || repository === undefined || signedIn === null) return response; const signed = yield* renderSignedUrl(request.url, repository); From 381f9aa97aca8b8de8e436a15671b22edd2b0140 Mon Sep 17 00:00:00 2001 From: Tim Biggs Date: Sun, 8 Nov 2026 03:00:07 +0000 Subject: [PATCH 4/4] fix(github): a repository's visibility is cached per account An account that cannot read a private repository reused another account's cached "private" answer and rendered its uploads anyway. Key the visibility cache by the credential fingerprint as well, so that account looks the repository up itself and keeps its 404. Co-Authored-By: Claude Opus 5.5 --- .../src/server/gitHubMediaFetch.test.ts | 15 ++++++++++++-- .../src/server/gitHubMediaFetch.ts | 20 ++++++++++++------- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts index 4f67a71794670..058ed642a47c9 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.test.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.test.ts @@ -20,7 +20,11 @@ const makeUploadFixture = ( uploadUrl: string, repository: "sso" | "private" | "public" | "hidden" = "sso", ) => { - const state = { account: "account-a", expired: new Set() }; + const state = { + account: "account-a", + expired: new Set(), + unreadableBy: new Set(), + }; const rendered: Array = []; const lookups: Array = []; const signedUrls: Array = []; @@ -38,7 +42,7 @@ const makeUploadFixture = ( rest: (input) => { if (input.path !== "markdown") { lookups.push(input.path); - return repository === "hidden" + return repository === "hidden" || state.unreadableBy.has(state.account) ? Effect.fail( new GitHubRequestPolicy.GitHubApiNotFoundError({ host: input.host, @@ -253,6 +257,13 @@ describe("GitHubMediaFetch", () => { expect(fixture.rendered).toHaveLength(2); // The repository's visibility outlives a signed URL. expect(fixture.lookups).toEqual([`repos/${PR_REPOSITORY}`]); + + // It is not another account's to reuse: one that cannot read the repository renders nothing. + fixture.state.account = "account-b"; + fixture.state.unreadableBy.add("account-b"); + expect((yield* fetchMedia(request))?.status).toBe(404); + expect(fixture.rendered).toHaveLength(2); + expect(fixture.lookups).toHaveLength(2); }).pipe(Effect.provide(fixture.layer), Effect.scoped); }, ); diff --git a/packages/source-control-github/src/server/gitHubMediaFetch.ts b/packages/source-control-github/src/server/gitHubMediaFetch.ts index 0391a022fe049..b845d79a7511f 100644 --- a/packages/source-control-github/src/server/gitHubMediaFetch.ts +++ b/packages/source-control-github/src/server/gitHubMediaFetch.ts @@ -119,10 +119,13 @@ export const make = Effect.gen(function* () { }); // Only a private repository's uploads are worth rendering after a 404; in a public one the file - // is gone. + // is gone. Whether an account can read the repository is part of the answer. const privateRepositories = yield* Cache.makeWith( - Effect.fnUntraced(function* (repository: string) { - const [owner = "", name = ""] = repository.split("/"); + Effect.fnUntraced(function* (key: { + readonly fingerprint: string; + readonly repository: string; + }) { + const [owner = "", name = ""] = key.repository.split("/"); const response = yield* rest.rest({ host: "github.com", operation: "getRepository", @@ -192,10 +195,13 @@ export const make = Effect.gen(function* () { if (response === null) return response; if (contentTypeOf(response) !== "text/html") { // A private repository's upload is a plain 404 to any token. - if (response.status !== 404 || repository === undefined) return response; - const isPrivate = yield* Cache.get(privateRepositories, repository).pipe( - Effect.orElseSucceed(() => false), - ); + if (response.status !== 404 || repository === undefined || signedIn === null) { + return response; + } + const isPrivate = yield* Cache.get(privateRepositories, { + fingerprint: signedIn.fingerprint, + repository, + }).pipe(Effect.orElseSucceed(() => false)); if (!isPrivate) return response; } }