Skip to content

Commit a09aef3

Browse files
juliusmarmingeclaude
juliusmarminge
and
claude
authored
fix(server): load PR uploads from SSO organizations (#21632)
Co-authored-by: Claude Opus 5.5 <[email protected]>
1 parent e4b98f9 commit a09aef3

5 files changed

Lines changed: 345 additions & 40 deletions

File tree

‎apps/server/src/assets/AssetAccess.test.ts‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import * as WorkspacePaths from "../workspace/WorkspacePaths.ts";
3535
import { assetFileResponse } from "../http.ts";
3636
import { ASSET_ROUTE_PREFIX, issueAssetUrl, readMcpAppHtml, resolveAsset } from "./AssetAccess.ts";
3737
import * as NativeAppIconResolver from "./NativeAppIconResolver.ts";
38+
import * as RepositoryIdentityResolver from "../project/RepositoryIdentityResolver.ts";
3839
import { symlinksSupported } from "@t3tools/shared/testing/symlinks";
3940
import { fakeSourceControlDriverInstance } from "@t3tools/source-control-testing/fakeSourceControlDriver";
4041
import * as BuiltInDrivers from "../sourceControl/builtInDrivers.ts";
@@ -1497,7 +1498,11 @@ describe("AssetAccess", () => {
14971498
);
14981499

14991500
it.effect("serves a host's credentialed media through that host's driver", () => {
1500-
const fetched: Array<{ url: string; headers: Readonly<Record<string, string>> }> = [];
1501+
const fetched: Array<{
1502+
url: string;
1503+
headers: Readonly<Record<string, string>>;
1504+
repository?: string;
1505+
}> = [];
15011506
const url = "https://github.com/owner/repo/blob/main/docs/shot.png";
15021507
const media = sourceControlDefinitions.mediaUrl(url)!;
15031508
const layerDrivers = Layer.succeed(BuiltInDrivers.SourceControlDrivers, [
@@ -1516,6 +1521,17 @@ describe("AssetAccess", () => {
15161521
}),
15171522
},
15181523
]);
1524+
// Clients that predate `repository` get the checkout's.
1525+
const layerIdentities = Layer.mock(RepositoryIdentityResolver.RepositoryIdentityResolver)({
1526+
resolve: () =>
1527+
Effect.succeed({
1528+
canonicalKey: "github.com/owner/repo",
1529+
locator: { source: "git-remote", remoteName: "origin", remoteUrl: "" },
1530+
provider: media.definition.kind,
1531+
owner: "owner",
1532+
name: "repo",
1533+
}),
1534+
});
15191535
return Effect.gen(function* () {
15201536
const decodeAssetCreateUrlInput = Schema.decodeEffect(AssetCreateUrlInput);
15211537

@@ -1539,12 +1555,21 @@ describe("AssetAccess", () => {
15391555
range: "bytes=0-4",
15401556
cookie: "session",
15411557
});
1542-
expect(fetched).toEqual([{ url: media.fetchUrl, headers: { range: "bytes=0-4" } }]);
1558+
expect(fetched).toEqual([
1559+
{ url: media.fetchUrl, headers: { range: "bytes=0-4" }, repository: "owner/repo" },
1560+
]);
15431561
expect(response.status).toBe(206);
15441562
// The upstream sent no type, so it comes from the file name the host gave the media.
15451563
expect(response.headers["content-type"]).toBe("image/png");
15461564
expect(response.headers["content-length"]).toBe("5");
1547-
}).pipe(Effect.provide(Layer.merge(layerTest, layerDrivers)), Effect.scoped);
1565+
1566+
// A pull request's own repository wins over the checkout it is shown in.
1567+
yield* sourceControlMediaResponse({ ...asset, repository: "enterprise/private" }, {});
1568+
expect(fetched.at(-1)?.repository).toBe("enterprise/private");
1569+
}).pipe(
1570+
Effect.provide(Layer.mergeAll(layerTest, layerDrivers, layerIdentities)),
1571+
Effect.scoped,
1572+
);
15481573
});
15491574

15501575
it.effect("refuses media no host keeps behind a credential, without echoing the URL", () =>

‎apps/server/src/assets/sourceControlMediaResponse.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import * as Effect from "effect/Effect";
44
import * as Option from "effect/Option";
55
import { HttpServerResponse } from "effect/http";
66

7+
import * as RepositoryIdentityResolver from "../project/RepositoryIdentityResolver.ts";
78
import * as BuiltInDrivers from "../sourceControl/builtInDrivers.ts";
89
import { sourceControlDefinitions } from "../sourceControl/sourceControlDefinitions.ts";
910

@@ -25,7 +26,12 @@ const SVG_CONTENT_SECURITY_POLICY = "default-src 'none'; style-src 'unsafe-inlin
2526
export const sourceControlMediaResponse = Effect.fn(
2627
"SourceControlMediaResponse.sourceControlMediaResponse",
2728
)(function* (
28-
asset: { readonly url: string; readonly cwd: string; readonly expiresAt: number },
29+
asset: {
30+
readonly url: string;
31+
readonly cwd: string;
32+
readonly repository?: string;
33+
readonly expiresAt: number;
34+
},
2935
requestHeaders: Record<string, string | undefined>,
3036
) {
3137
const remainingSeconds = Math.floor((asset.expiresAt - (yield* Clock.currentTimeMillis)) / 1000);
@@ -46,7 +52,20 @@ export const sourceControlMediaResponse = Effect.fn(
4652
const value = requestHeaders[name];
4753
if (value !== undefined) forwarded[name] = value;
4854
}
49-
const response = yield* fetchMedia({ url: asset.url, headers: forwarded });
55+
// Clients that predate `repository` fall back to the repository checked out at `cwd`.
56+
let repository = asset.repository;
57+
if (repository === undefined) {
58+
const identities = yield* RepositoryIdentityResolver.RepositoryIdentityResolver;
59+
const identity = yield* identities.resolve(asset.cwd);
60+
if (identity?.provider === media.definition.kind && identity.owner && identity.name) {
61+
repository = `${identity.owner}/${identity.name}`;
62+
}
63+
}
64+
const response = yield* fetchMedia({
65+
url: asset.url,
66+
headers: forwarded,
67+
...(repository === undefined ? {} : { repository }),
68+
});
5069
if (response === null) return HttpServerResponse.empty({ status: 502, headers });
5170
// It carries none of the upstream entity headers: a `content-length` with no body behind it
5271
// holds the connection open until the browser gives up on it.

‎packages/source-control-core/src/server/driver.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ export interface SourceControlMediaRequest {
2525
readonly url: string;
2626
/** Request headers to forward, such as `range`. */
2727
readonly headers: Readonly<Record<string, string>>;
28+
/** `owner/name` of the change request the media is in, else of the checkout it was shown in. */
29+
readonly repository?: string;
2830
}
2931

3032
/** The server side of a definition's `credentials.appSignIn`. */

‎packages/source-control-github/src/server/gitHubMediaFetch.test.ts‎

Lines changed: 158 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,78 @@ import { describe, expect, it } from "@effect/vitest";
22
import * as Effect from "effect/Effect";
33
import * as Layer from "effect/Layer";
44
import * as Redacted from "effect/Redacted";
5+
import * as TestClock from "effect/testing/TestClock";
56
import { HttpClient, HttpClientResponse } from "effect/http";
67

78
import * as GitHubCredentials from "./GitHubCredentials.ts";
89
import * as GitHubMediaFetch from "./gitHubMediaFetch.ts";
10+
import * as GitHubRest from "./GitHubRest.ts";
11+
12+
const PR_REPOSITORY = "enterprise/private";
13+
14+
/** An SSO organization: the upload URL answers with a sign-in page, a rendered signed URL with bytes. */
15+
const makeSsoFixture = (uploadUrl: string) => {
16+
const state = { account: "account-a", expired: new Set<string>() };
17+
const rendered: Array<GitHubRest.GitHubRestInput> = [];
18+
const signedUrls: Array<string> = [];
19+
const requests: Array<{ url: string; authorization?: string; range?: string }> = [];
20+
const credential = (host: string) =>
21+
Effect.sync(() => ({
22+
host,
23+
token: Redacted.make(state.account),
24+
fingerprint: state.account,
25+
source: "gh" as const,
26+
}));
27+
const layer = Layer.mergeAll(
28+
Layer.mock(GitHubCredentials.GitHubCredentials)({ get: credential }),
29+
Layer.mock(GitHubRest.GitHubRest)({
30+
rest: (input) =>
31+
Effect.sync(() => {
32+
rendered.push(input);
33+
const signed = `https://private-user-images.githubusercontent.com/1/upload.mp4?jwt=${state.account}-${rendered.length}&x=1`;
34+
const inRepository =
35+
(input.body as { readonly context?: string } | undefined)?.context === PR_REPOSITORY;
36+
if (inRepository) signedUrls.push(signed);
37+
return {
38+
status: 200,
39+
headers: {},
40+
body: `<p><img src="${inRepository ? signed.replaceAll("&", "&amp;") : uploadUrl}" alt=""></p>`,
41+
truncated: false,
42+
invalidUtf8: false,
43+
};
44+
}),
45+
}),
46+
Layer.succeed(
47+
HttpClient.HttpClient,
48+
HttpClient.make((request) =>
49+
Effect.sync(() => {
50+
requests.push({
51+
url: request.url,
52+
...(request.headers.authorization
53+
? { authorization: request.headers.authorization }
54+
: {}),
55+
...(request.headers.range ? { range: request.headers.range } : {}),
56+
});
57+
if (request.url === uploadUrl) {
58+
return HttpClientResponse.fromWeb(
59+
request,
60+
new Response("<html>Sign in with SSO</html>", {
61+
headers: { "content-type": "text/html; charset=utf-8" },
62+
}),
63+
);
64+
}
65+
expect(signedUrls).toContain(request.url);
66+
const status = state.expired.has(request.url) ? 403 : request.headers.range ? 206 : 200;
67+
return HttpClientResponse.fromWeb(
68+
request,
69+
new Response(null, { status, headers: { "content-type": "video/mp4" } }),
70+
);
71+
}),
72+
),
73+
),
74+
);
75+
return { layer, state, rendered, signedUrls, requests };
76+
};
977

1078
describe("GitHubMediaFetch", () => {
1179
it.effect("loads private media immediately after login with the GitHub credential", () => {
@@ -24,17 +92,20 @@ describe("GitHubMediaFetch", () => {
2492
expect(authorizations).toEqual([undefined, "Bearer signed-in", "Bearer signed-in"]);
2593
}).pipe(
2694
Effect.provide(
27-
Layer.mock(GitHubCredentials.GitHubCredentials)({
28-
get: (host) =>
29-
++lookups === 1
30-
? Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host }))
31-
: Effect.succeed({
32-
host,
33-
token: Redacted.make("signed-in"),
34-
source: "gh" as const,
35-
fingerprint: "fingerprint",
36-
}),
37-
}),
95+
Layer.merge(
96+
Layer.mock(GitHubRest.GitHubRest)({}),
97+
Layer.mock(GitHubCredentials.GitHubCredentials)({
98+
get: (host) =>
99+
++lookups === 1
100+
? Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host }))
101+
: Effect.succeed({
102+
host,
103+
token: Redacted.make("signed-in"),
104+
source: "gh" as const,
105+
fingerprint: "fingerprint",
106+
}),
107+
}),
108+
),
38109
),
39110
Effect.provideService(
40111
HttpClient.HttpClient,
@@ -70,15 +141,18 @@ describe("GitHubMediaFetch", () => {
70141
]);
71142
}).pipe(
72143
Effect.provide(
73-
Layer.mock(GitHubCredentials.GitHubCredentials)({
74-
get: (host) =>
75-
Effect.succeed({
76-
host,
77-
token: Redacted.make("signed-in"),
78-
source: "gh" as const,
79-
fingerprint: "fingerprint",
80-
}),
81-
}),
144+
Layer.merge(
145+
Layer.mock(GitHubRest.GitHubRest)({}),
146+
Layer.mock(GitHubCredentials.GitHubCredentials)({
147+
get: (host) =>
148+
Effect.succeed({
149+
host,
150+
token: Redacted.make("signed-in"),
151+
source: "gh" as const,
152+
fingerprint: "fingerprint",
153+
}),
154+
}),
155+
),
82156
),
83157
Effect.provideService(
84158
HttpClient.HttpClient,
@@ -101,4 +175,68 @@ describe("GitHubMediaFetch", () => {
101175
Effect.scoped,
102176
);
103177
});
178+
179+
it.effect(
180+
"renders SSO uploads in the PR repository and reuses the signed URL for a range",
181+
() => {
182+
const url = "https://github.com/user-attachments/assets/explicit-pr-context";
183+
const fixture = makeSsoFixture(url);
184+
return Effect.gen(function* () {
185+
const fetchMedia = yield* GitHubMediaFetch.make;
186+
const request = { url, headers: {}, repository: PR_REPOSITORY };
187+
expect((yield* fetchMedia(request))?.status).toBe(200);
188+
expect((yield* fetchMedia({ ...request, headers: { range: "bytes=2-5" } }))?.status).toBe(
189+
206,
190+
);
191+
expect(fixture.rendered).toHaveLength(1);
192+
expect(fixture.rendered[0]).toMatchObject({
193+
host: "github.com",
194+
method: "POST",
195+
path: "markdown",
196+
accept: "text/html",
197+
body: { text: `![](${url})`, mode: "gfm", context: PR_REPOSITORY },
198+
});
199+
expect(fixture.requests).toEqual([
200+
{ url, authorization: "Bearer account-a" },
201+
{ url: fixture.signedUrls[0] },
202+
{ url: fixture.signedUrls[0], range: "bytes=2-5" },
203+
]);
204+
}).pipe(Effect.provide(fixture.layer), Effect.scoped);
205+
},
206+
);
207+
208+
it.effect("renders a legacy upload in the repository its path names", () => {
209+
const url = `https://github.com/${PR_REPOSITORY}/assets/123/legacy`;
210+
const fixture = makeSsoFixture(url);
211+
return Effect.gen(function* () {
212+
const fetchMedia = yield* GitHubMediaFetch.make;
213+
const response = yield* fetchMedia({ url, headers: {}, repository: "personal/unrelated" });
214+
expect(response?.status).toBe(200);
215+
expect(fixture.rendered.map((input) => input.body)).toEqual([
216+
{ text: `![](${url})`, mode: "gfm", context: PR_REPOSITORY },
217+
]);
218+
}).pipe(Effect.provide(fixture.layer), Effect.scoped);
219+
});
220+
221+
it.effect("renders a fresh signed URL after an account switch, early expiry, or its TTL", () => {
222+
const url = "https://github.com/user-attachments/assets/cache-lifecycle";
223+
const fixture = makeSsoFixture(url);
224+
const request = { url, headers: {}, repository: PR_REPOSITORY };
225+
return Effect.gen(function* () {
226+
const fetchMedia = yield* GitHubMediaFetch.make;
227+
yield* fetchMedia(request);
228+
fixture.state.account = "account-b";
229+
yield* fetchMedia(request);
230+
expect(fixture.rendered).toHaveLength(2);
231+
232+
fixture.state.expired.add(fixture.signedUrls.at(-1)!);
233+
expect((yield* fetchMedia(request))?.status).toBe(200);
234+
expect(fixture.rendered).toHaveLength(3);
235+
236+
yield* TestClock.adjust("6 minutes");
237+
yield* fetchMedia(request);
238+
expect(fixture.rendered).toHaveLength(4);
239+
expect(fixture.requests.at(-1)?.url).toBe(fixture.signedUrls.at(-1));
240+
}).pipe(Effect.provide(fixture.layer), Effect.scoped);
241+
});
104242
});

0 commit comments

Comments
 (0)