Skip to content

Commit 52daeda

Browse files
juliusmarmingejuliusmarminge
committed
fix(github): normalize subdomain host fallbacks consistently
1 parent d8be4fa commit 52daeda

6 files changed

Lines changed: 158 additions & 88 deletions

File tree

‎packages/contracts/src/pullRequest.test.ts‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
PullRequestListResult,
1111
PullRequestReviewerRequestInput,
1212
PullRequestSetFilesViewedInput,
13-
gitHubApiHostOf,
1413
pullRequestHostOf,
1514
resolvePullRequestAuthorFilter,
1615
} from "./pullRequest.ts";
@@ -90,26 +89,19 @@ describe("PullRequestListResult", () => {
9089
),
9190
).toBe("forge.example");
9291
});
93-
it("names a GitHub checkout behind a github.com subdomain by github.com", () => {
94-
const identity = {
95-
canonicalKey: "work.github.com/acme/web",
96-
locator: { remoteUrl: "[email protected]:acme/web.git" },
97-
};
98-
expect(pullRequestHostOf(identity, SourceControlProviderKind.make("github"))).toBe(
99-
"github.com",
100-
);
101-
expect(
102-
pullRequestHostOf(
103-
{ canonicalKey: "github.example.com/acme/web" },
104-
SourceControlProviderKind.make("github"),
105-
),
106-
).toBe("github.example.com");
107-
});
10892

109-
it("reads a github.com subdomain with a port through github.com", () => {
110-
expect(gitHubApiHostOf("work.github.com:8443")).toBe("github.com");
111-
expect(gitHubApiHostOf("github.example.com:8443")).toBe("github.example.com:8443");
93+
it("names GitHub subdomain checkouts by their API host and preserves enterprise ports", () => {
94+
for (const [host, expected] of [
95+
["ssh.github.com", "github.com"],
96+
["work.github.com:8443", "github.com"],
97+
["github.example.com:8443", "github.example.com:8443"],
98+
]) {
99+
const identity = { canonicalKey: `${host}/acme/web` };
100+
expect(pullRequestHostOf(identity, SourceControlProviderKind.make("github"))).toBe(expected);
101+
expect(pullRequestHostOf(identity, SourceControlProviderKind.make("gitlab"))).toBe(host);
102+
}
112103
});
104+
113105
/**
114106
* The RPC builds this codec at call time, so a shape it cannot lower — an open-keyed record
115107
* with an optional value, for one — fails as an interrupted request rather than as a schema

‎packages/contracts/src/pullRequest.ts‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -912,11 +912,7 @@ export function pullRequestHostOf(
912912
return kind === "github" ? gitHubApiHostOf(host) : host.toLowerCase();
913913
}
914914

915-
/**
916-
* The host a GitHub checkout is read through. github.com serves no API under its subdomains, so
917-
* a remote there (`ssh.github.com`, or an SSH alias like `work.github.com`) is read through
918-
* github.com; a self-hosted GitHub lives on its own domain.
919-
*/
915+
/** SSH remotes under github.com share github.com's API host and credentials. */
920916
export function gitHubApiHostOf(host: string): string {
921917
const normalized = host.toLowerCase();
922918
return normalized.replace(/:\d+$/, "").endsWith(".github.com") ? "github.com" : normalized;

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

Lines changed: 138 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,54 @@ describe("GitHubSourceControlProvider repository resolution", () => {
318318
}).pipe(Effect.provide(layer));
319319
});
320320

321+
it.effect(
322+
"reads GitHub subdomain remotes through github.com and preserves enterprise ports",
323+
() =>
324+
Effect.gen(function* () {
325+
for (const [remote, host, apiHost] of [
326+
["ssh://[email protected]:443/acme/web.git", "ssh.github.com", "github.com"],
327+
["[email protected]:acme/web.git", "work.github.com", "github.com"],
328+
[
329+
"https://github.example.com:8443/acme/web.git",
330+
"github.example.com:8443",
331+
"github.example.com:8443",
332+
],
333+
] as const) {
334+
const requests: string[] = [];
335+
const { layer } = harness({
336+
remotes: remotesOutput(["origin", remote]),
337+
api: {
338+
rest: (input) =>
339+
Effect.sync(() => {
340+
requests.push(`${input.host} ${input.path}`);
341+
return restResponse({
342+
full_name: "acme/web",
343+
html_url: `https://${apiHost}/acme/web`,
344+
ssh_url: `git@${apiHost}:acme/web.git`,
345+
default_branch: "trunk",
346+
});
347+
}),
348+
},
349+
});
350+
yield* Effect.gen(function* () {
351+
const gh = yield* GitHubSourceControlProvider.make;
352+
assert.strictEqual(yield* gh.getDefaultBranch({ cwd: "/repo" }), "trunk");
353+
assert.strictEqual(
354+
yield* gh.getDefaultBranch({
355+
cwd: "/repo",
356+
context: { ...githubContext(host), remoteUrl: remote },
357+
}),
358+
"trunk",
359+
);
360+
assert.deepStrictEqual(requests, [
361+
`${apiHost} repos/acme/web`,
362+
`${apiHost} repos/acme/web`,
363+
]);
364+
}).pipe(Effect.provide(layer), Effect.provideService(HostProcess.Environment, {}));
365+
}
366+
}),
367+
);
368+
321369
it.effect("reads a GH_REPO owner/name behind a github.com subdomain through github.com", () => {
322370
const hosts: string[] = [];
323371
const { layer } = harness({
@@ -337,11 +385,54 @@ describe("GitHubSourceControlProvider repository resolution", () => {
337385
});
338386
return Effect.gen(function* () {
339387
const gh = yield* GitHubSourceControlProvider.make;
340-
yield* gh.getDefaultBranch({ cwd: "/repo", context: githubContext("work.github.com") });
341-
assert.deepStrictEqual(hosts, ["github.com repos/chosen/repo"]);
388+
assert.strictEqual(
389+
yield* gh.getDefaultBranch({ cwd: "/repo", context: githubContext("work.github.com") }),
390+
"main",
391+
);
392+
assert.strictEqual(yield* gh.getDefaultBranch({ cwd: "/repo" }), "main");
393+
assert.deepStrictEqual(hosts, [
394+
"github.com repos/chosen/repo",
395+
"github.com repos/chosen/repo",
396+
]);
342397
}).pipe(
343398
Effect.provide(layer),
344-
Effect.provideService(HostProcess.Environment, { GH_REPO: "chosen/repo" }),
399+
Effect.provideService(HostProcess.Environment, {
400+
GH_REPO: "chosen/repo",
401+
GH_HOST: "ssh.github.com",
402+
}),
403+
);
404+
});
405+
406+
it.effect("uses github.com for a GH_HOST alias when cloning without a checkout remote", () => {
407+
const requests: string[] = [];
408+
const { layer } = harness({
409+
remotes: "",
410+
api: {
411+
rest: (input) =>
412+
Effect.sync(() => {
413+
requests.push(`${input.host} ${input.path}`);
414+
return restResponse({
415+
full_name: "chosen/repo",
416+
html_url: "https://github.com/chosen/repo",
417+
ssh_url: "[email protected]:chosen/repo.git",
418+
});
419+
}),
420+
},
421+
});
422+
return Effect.gen(function* () {
423+
const gh = yield* GitHubSourceControlProvider.make;
424+
assert.deepStrictEqual(
425+
yield* gh.getRepositoryCloneUrls({ cwd: "/repo", repository: "chosen/repo" }),
426+
{
427+
nameWithOwner: "chosen/repo",
428+
url: "https://github.com/chosen/repo",
429+
sshUrl: "[email protected]:chosen/repo.git",
430+
},
431+
);
432+
assert.deepStrictEqual(requests, ["github.com repos/chosen/repo"]);
433+
}).pipe(
434+
Effect.provide(layer),
435+
Effect.provideService(HostProcess.Environment, { GH_HOST: "work.github.com" }),
345436
);
346437
});
347438

@@ -683,6 +774,50 @@ describe("GitHubSourceControlProvider writes", () => {
683774
}).pipe(Effect.provide(layer));
684775
});
685776

777+
it.effect("creates repositories on the API host named by GH_HOST", () =>
778+
Effect.gen(function* () {
779+
for (const [host, apiHost] of [
780+
["work.github.com", "github.com"],
781+
["github.example.com:8443", "github.example.com:8443"],
782+
]) {
783+
const requests: string[] = [];
784+
const { layer } = harness({
785+
remotes: "",
786+
api: {
787+
rest: (input) =>
788+
Effect.sync(() => {
789+
requests.push(`${input.host} ${input.method ?? "GET"} ${input.path}`);
790+
return input.path === "user"
791+
? restResponse({ login: "me" })
792+
: restResponse({
793+
full_name: "acme/new",
794+
html_url: `https://${apiHost}/acme/new`,
795+
ssh_url: `git@${apiHost}:acme/new.git`,
796+
});
797+
}),
798+
},
799+
});
800+
yield* Effect.gen(function* () {
801+
const gh = yield* GitHubSourceControlProvider.make;
802+
yield* gh.createRepository({
803+
cwd: "/repo",
804+
repository: "acme/new",
805+
visibility: "private",
806+
});
807+
yield* gh.createRepository({ cwd: "/repo", repository: "new", visibility: "private" });
808+
assert.deepStrictEqual(requests, [
809+
`${apiHost} GET user`,
810+
`${apiHost} POST orgs/acme/repos`,
811+
`${apiHost} POST user/repos`,
812+
]);
813+
}).pipe(
814+
Effect.provide(layer),
815+
Effect.provideService(HostProcess.Environment, { GH_HOST: host }),
816+
);
817+
}
818+
}),
819+
);
820+
686821
it.effect("looks up a bare repository name under the signed-in account on that host", () => {
687822
const requests: Array<string> = [];
688823
return Effect.gen(function* () {

‎packages/source-control-github/src/server/GitHubSourceControlProvider.ts‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,7 @@ export const make = Effect.gen(function* () {
454454
const sourceControlHost = yield* SourceControlHost.SourceControlHost;
455455
const process = sourceControlHost.process;
456456
const environment = yield* HostProcess.Environment;
457+
const environmentHost = apiHostForHint(environment.GH_HOST?.trim() || "github.com");
457458
const git = sourceControlHost.git;
458459
const fileSystem = yield* FileSystem.FileSystem;
459460

@@ -475,7 +476,7 @@ export const make = Effect.gen(function* () {
475476
}) {
476477
const envRepository = environment.GH_REPO?.trim();
477478
const hostHint = input.host ?? contextHost(input.context);
478-
const defaultHost = apiHostForHint(hostHint ?? environment.GH_HOST ?? "github.com");
479+
const defaultHost = apiHostForHint(hostHint ?? environmentHost);
479480
if (envRepository) {
480481
const locator = parseGitHubRepositorySelector(envRepository, defaultHost);
481482
if (locator !== null) return locator;
@@ -961,10 +962,10 @@ export const make = Effect.gen(function* () {
961962
),
962963
getRepositoryCloneUrls: (input) =>
963964
Effect.gen(function* () {
964-
const fallbackHost = (yield* resolveRepository(input).pipe(
965+
const fallbackHost = yield* resolveRepository(input).pipe(
965966
Effect.map((locator) => locator.host),
966-
Effect.orElseSucceed(() => environment.GH_HOST ?? "github.com"),
967-
)).toLowerCase();
967+
Effect.orElseSucceed(() => environmentHost),
968+
);
968969
const bareName = input.repository.trim().replace(/\.git$/i, "");
969970
const locator =
970971
parseGitHubRepositorySelector(input.repository, fallbackHost) ??
@@ -980,14 +981,11 @@ export const make = Effect.gen(function* () {
980981
),
981982
createRepository: (input) =>
982983
Effect.gen(function* () {
983-
const locator = parseGitHubRepositorySelector(
984-
input.repository,
985-
(environment.GH_HOST ?? "github.com").toLowerCase(),
986-
);
984+
const locator = parseGitHubRepositorySelector(input.repository, environmentHost);
987985
const viewer = locator === null ? null : yield* readViewerLogin(locator.host);
988986
const owner = locator?.owner ?? viewer;
989987
const name = locator?.name ?? input.repository.trim();
990-
const host = locator?.host ?? (environment.GH_HOST?.trim().toLowerCase() || "github.com");
988+
const host = locator?.host ?? environmentHost;
991989
const isViewer = viewer !== null && owner?.toLowerCase() === viewer.toLowerCase();
992990
const response = yield* rest(
993991
{

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

Lines changed: 0 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import { assert, describe, it } from "@effect/vitest";
22

33
import {
44
gitHubApiHostForRemote,
5-
resolveGitHubRepository,
65
selectGitHubBaseRepository,
76
} from "./gitHubRepositoryResolution.ts";
87

@@ -84,48 +83,4 @@ describe("gitHubApiHostForRemote", () => {
8483
);
8584
assert.strictEqual(gitHubApiHostForRemote("[email protected]:a/b.git"), null);
8685
});
87-
88-
it("reads a subdomain of github.com through github.com", () => {
89-
assert.strictEqual(
90-
gitHubApiHostForRemote("ssh://[email protected]:443/acme/web.git"),
91-
"github.com",
92-
);
93-
assert.strictEqual(gitHubApiHostForRemote("[email protected]:acme/web.git"), "github.com");
94-
assert.strictEqual(
95-
gitHubApiHostForRemote("https://work.github.com:8443/acme/web.git"),
96-
"github.com",
97-
);
98-
});
99-
});
100-
101-
describe("resolveGitHubRepository", () => {
102-
it("resolves a checkout behind a github.com subdomain alias to github.com", () => {
103-
const remotes = [
104-
"origin\[email protected]:acme/web.git (fetch)",
105-
"origin\[email protected]:acme/web.git (push)",
106-
].join("\n");
107-
const expected = {
108-
host: "github.com",
109-
locator: { host: "github.com", owner: "acme", name: "web" },
110-
};
111-
assert.deepStrictEqual(
112-
resolveGitHubRepository({
113-
remotes,
114-
resolved: "",
115-
hostHint: undefined,
116-
defaultHost: "github.com",
117-
}),
118-
expected,
119-
);
120-
// The provider context names the alias as the checkout's host.
121-
assert.deepStrictEqual(
122-
resolveGitHubRepository({
123-
remotes,
124-
resolved: "",
125-
hostHint: "work.github.com",
126-
defaultHost: "github.com",
127-
}),
128-
expected,
129-
);
130-
});
13186
});

‎packages/source-control-github/src/server/gitHubRepositoryResolution.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,12 +157,7 @@ export function parsePullRequestReference(
157157
return { kind: "branch", headSelector: trimmed };
158158
}
159159

160-
/**
161-
* The GitHub host a remote URL is served from, or null for a remote that is not GitHub. An SSH
162-
* alias (`git@github-work:owner/repo`) names no API host of its own; it is read through
163-
* `github.com`, which is what such an alias almost always stands for (issue #6198). So is a
164-
* subdomain of github.com, such as `ssh.github.com` or an alias like `work.github.com`.
165-
*/
160+
/** Dotless GitHub SSH aliases and github.com subdomains use github.com for API requests. */
166161
export function gitHubApiHostForRemote(remoteUrl: string): string | null {
167162
const provider = detectSourceControlProviderFromRemoteUrl(remoteUrl);
168163
if (provider === null) return null;
@@ -175,7 +170,6 @@ export function gitHubApiHostForRemote(remoteUrl: string): string | null {
175170
return provider.kind === "github" ? host : null;
176171
}
177172

178-
/** A caller's host hint, read the way a remote's host is: an alias is not an API host. */
179173
export function apiHostForHint(host: string): string {
180174
const normalized = host.toLowerCase();
181175
return !normalized.includes(".") && normalized.includes("github")

0 commit comments

Comments
 (0)