fix: renew public grants and verify source identity and visibility

This commit is contained in:
Thomas Durieux committed 2026-09-13 15:05:43 +00:00
1 parent 8efb9bd506
commit 5e5cc6e0c2
6 files changed
+79 -27

No files matched your search

+5 -3
View File
@@ -98,9 +98,11 @@ after approval** on the Connections page when approval is delayed.
App-connected accounts default to the App for new repository/PR access. App-connected accounts default to the App for new repository/PR access.
Public repositories outside the selected installations use the App user grant, Public repositories outside the selected installations use the App user grant,
so users can paste a public URL without installing the App on its owner account. so users can paste a public URL without installing the App on its owner account.
The connection records the repository ID and checks that it is still public on The connection records the repository ID and verifies that the stored source
each source access. If it becomes private, reconnect through an installation name still resolves to that ID with public visibility on each source access.
with access. Existing installation bindings retain their installation checks. Private and Enterprise-internal repositories require an installation with access.
Long API traversals renew the App user token under the same user quota.
Existing installation bindings retain their installation checks.
This uses GitHub's documented [public resource access for App user tokens](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/choosing-permissions-for-a-github-app). This uses GitHub's documented [public resource access for App user tokens](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/choosing-permissions-for-a-github-app).
The explicit **Use existing OAuth access** choice handles repositories not yet The explicit **Use existing OAuth access** choice handles repositories not yet
+1 -1
View File
@@ -304,7 +304,7 @@ export async function getToken(repository: Repository) {
} }
} }
if (repository.model.githubAccess?.kind === "github-app") { if (repository.model.githubAccess?.kind === "github-app") {
return boundAppToken(repository.owner.id, repository.model.githubAccess); return boundAppToken(repository.owner.id, repository.model.githubAccess, repository.model.source.repositoryName);
} }
const credential = await getCredential(repository.owner.id); const credential = await getCredential(repository.owner.id);
const ownerAccessToken = credential?.token; const ownerAccessToken = credential?.token;
+1 -1
View File
@@ -26,7 +26,7 @@ export default class PullRequest {
} }
async getToken() { async getToken() {
if (this._model.githubAccess?.kind === "github-app") return boundAppToken(this.owner.id, this._model.githubAccess); if (this._model.githubAccess?.kind === "github-app") return boundAppToken(this.owner.id, this._model.githubAccess, this._model.source.repositoryFullName);
return (await getCredentialToken(this.owner.id, "github", { collection: "anonymizedpullrequests", id: this._model._id })) || config.GITHUB_TOKEN; return (await getCredentialToken(this.owner.id, "github", { collection: "anonymizedpullrequests", id: this._model._id })) || config.GITHUB_TOKEN;
} }
+18 -11
View File
@@ -144,7 +144,7 @@ export async function reconcileInstallation(installationId: number, revision: st
} }
export interface GitHubRepositoryInfo { export interface GitHubRepositoryInfo {
id: number; full_name: string; name: string; private: boolean; html_url: string; size: number; id: number; full_name: string; name: string; private: boolean; visibility?: string; html_url: string; size: number;
default_branch: string; owner: { id: number; login: string }; default_branch: string; owner: { id: number; login: string };
} }
export interface AppInstallation { export interface AppInstallation {
@@ -219,17 +219,24 @@ async function installationToken(binding: RepositoryAccess, ownerId: string): Pr
try { return await work; } finally { minting.delete(key); } try { return await work; } finally { minting.delete(key); }
} }
export async function boundAppToken(ownerId: string, binding: RepositoryAccess): Promise<string> { async function publicAppUserToken(ownerId: string): Promise<string> {
const token = await appUserToken(ownerId);
registerGitHubToken(token, { quotaKey: `app-user:${ownerId}`, renew: () => publicAppUserToken(ownerId) });
return token;
}
export async function boundAppToken(ownerId: string, binding: RepositoryAccess, sourceName?: string): Promise<string> {
if (!Number.isSafeInteger(binding.repositoryId)) throw appError(); if (!Number.isSafeInteger(binding.repositoryId)) throw appError();
if (binding.publicRead === true) { if (binding.publicRead === true) {
if (binding.installationId !== undefined) throw appError(); if (binding.installationId !== undefined || !sourceName || !/^[^/\s]+\/[^/\s]+$/.test(sourceName)) throw appError();
const userToken = await appUserToken(ownerId); const userToken = await publicAppUserToken(ownerId);
const repo = await githubRequest<GitHubRepositoryInfo>(`/repositories/${binding.repositoryId}`, userToken); // Source reads use owner/name, so validate that exact name against the
// bound ID. A replacement at a renamed repository's old URL must fail.
const repo = await githubRequest<GitHubRepositoryInfo>(
`/repos/${sourceName.split("/").map(encodeURIComponent).join("/")}`, userToken);
// A public binding must never gain private access, even if the user later // A public binding must never gain private access, even if the user later
// installs the App on this repository. Reconnect explicitly to do that. // installs the App on this repository. Reconnect explicitly to do that.
if (repo.id !== binding.repositoryId || repo.private !== false) throw appError("github_app_access_required"); if (repo.id !== binding.repositoryId || repo.private !== false || repo.visibility !== "public") throw appError("github_app_access_required");
// Resolve the current grant again on each source read. Do not register a
// repository-specific renewal callback against a user token shared by repos.
return userToken; return userToken;
} }
if (!Number.isSafeInteger(binding.installationId)) throw appError(); if (!Number.isSafeInteger(binding.installationId)) throw appError();
@@ -255,13 +262,13 @@ export async function selectRepositoryAccess(ownerId: string, fullName: string,
const userToken = await appUserToken(ownerId); const userToken = await appUserToken(ownerId);
const publicRepo = await githubRequest<GitHubRepositoryInfo>( const publicRepo = await githubRequest<GitHubRepositoryInfo>(
`/repos/${fullName.split("/").map(encodeURIComponent).join("/")}`, userToken); `/repos/${fullName.split("/").map(encodeURIComponent).join("/")}`, userToken);
if (publicRepo.private !== false || !Number.isSafeInteger(publicRepo.id)) throw appError("github_app_access_required"); if (publicRepo.private !== false || publicRepo.visibility !== "public" || !Number.isSafeInteger(publicRepo.id)) throw appError("github_app_access_required");
const binding: RepositoryAccess = { kind: "github-app", publicRead: true, const binding: RepositoryAccess = { kind: "github-app", publicRead: true,
repositoryId: publicRepo.id, revision: randomUUID() }; repositoryId: publicRepo.id, revision: randomUUID() };
return { binding, token: await boundAppToken(ownerId, binding) }; return { binding, token: await boundAppToken(ownerId, binding, fullName) };
} }
const binding: RepositoryAccess = { kind: "github-app", repositoryId: repo.id, installationId: repo.installationId, revision: randomUUID() }; const binding: RepositoryAccess = { kind: "github-app", repositoryId: repo.id, installationId: repo.installationId, revision: randomUUID() };
return { binding, token: await boundAppToken(ownerId, binding) }; return { binding, token: await boundAppToken(ownerId, binding, fullName) };
} }
const token = await getCredentialToken(ownerId); const token = await getCredentialToken(ownerId);
if (!token) throw appError("github_oauth_required"); if (!token) throw appError("github_oauth_required");
+1 -1
View File
@@ -495,7 +495,7 @@ router.post(
} }
if (repoUpdate.fullName !== repo.model.source.repositoryName && user.id !== repo.owner.id) throw appError("not_owner", 403); if (repoUpdate.fullName !== repo.model.source.repositoryName && user.id !== repo.owner.id) throw appError("not_owner", 403);
const sourceAccess = repo.model.githubAccess?.kind === "github-app" && repoUpdate.fullName === repo.model.source.repositoryName const sourceAccess = repo.model.githubAccess?.kind === "github-app" && repoUpdate.fullName === repo.model.source.repositoryName
? { token: await boundAppToken(repo.owner.id, repo.model.githubAccess), binding: repo.model.githubAccess } ? { token: await boundAppToken(repo.owner.id, repo.model.githubAccess, repo.model.source.repositoryName), binding: repo.model.githubAccess }
: await selectRepositoryAccess(repo.owner.id, `${parsedRepository.owner}/${parsedRepository.name}`, repo.model.githubAccess?.kind || "oauth"); : await selectRepositoryAccess(repo.owner.id, `${parsedRepository.owner}/${parsedRepository.name}`, repo.model.githubAccess?.kind || "oauth");
const repository = await getRepositoryFromGitHub({ const repository = await getRepositoryFromGitHub({
accessToken: sourceAccess.token, accessToken: sourceAccess.token,
+53 -10
View File
@@ -205,7 +205,7 @@ describeMongo("GitHub App credential and repository integration", function () {
if (url.endsWith("/branches")) return [{ name: "main", commit: { sha: "abc123" } }]; if (url.endsWith("/branches")) return [{ name: "main", commit: { sha: "abc123" } }];
if (url.endsWith("/readme")) return { status: 404 }; if (url.endsWith("/readme")) return { status: 404 };
if (url.endsWith("/pages")) return { status: 404 }; if (url.endsWith("/pages")) return { status: 404 };
return { id: 7, private: false, full_name: "other/public", name: "public", owner: { login: "other" }, default_branch: "main" }; return { id: 7, private: false, visibility: "public", full_name: "other/public", name: "public", owner: { login: "other" }, default_branch: "main" };
}); });
const selected = await app.selectRepositoryAccess(owner.id, "other/public", "github-app"); const selected = await app.selectRepositoryAccess(owner.id, "other/public", "github-app");
expect(selected.token).to.equal("ghu_access1"); expect(selected.token).to.equal("ghu_access1");
@@ -223,7 +223,7 @@ describeMongo("GitHub App credential and repository integration", function () {
}); });
it("requires explicit public visibility when selecting an uninstalled repository", async () => { it("requires explicit public visibility when selecting an uninstalled repository", async () => {
await app.saveAppGrant(owner.id, data()); await app.saveAppGrant(owner.id, data());
for (const metadata of [{ id: 7, private: true }, { id: 7 }, { private: false }]) { for (const metadata of [{ id: 7, private: true }, { id: 7 }, { id: 7, private: false }, { id: 7, private: false, visibility: "internal" }, { private: false, visibility: "public" }]) {
mock(url => url.includes("/user/installations") ? { installations: [] } : metadata); mock(url => url.includes("/user/installations") ? { installations: [] } : metadata);
await rejects(app.selectRepositoryAccess(owner.id, "other/private", "github-app"), "github_app_access_required"); await rejects(app.selectRepositoryAccess(owner.id, "other/private", "github-app"), "github_app_access_required");
} }
@@ -239,29 +239,72 @@ describeMongo("GitHub App credential and repository integration", function () {
it("stops public source reads after visibility changes, deletion, or grant revocation", async () => { it("stops public source reads after visibility changes, deletion, or grant revocation", async () => {
await app.saveAppGrant(owner.id, data()); await app.saveAppGrant(owner.id, data());
const binding = { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" }; const binding = { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" };
mock(() => ({ id: 7, private: false })); mock(() => ({ id: 7, private: false, visibility: "public" }));
expect(await app.boundAppToken(owner.id, binding)).to.equal("ghu_access1"); expect(await app.boundAppToken(owner.id, binding, "other/public")).to.equal("ghu_access1");
for (const metadata of [{ id: 7, private: true }, { status: 404 }, { id: 8, private: false }]) { for (const metadata of [{ id: 7, private: true }, { id: 7, private: false, visibility: "internal" }, { id: 7, private: false }, { status: 404 }, { id: 8, private: false, visibility: "public" }]) {
mock(() => metadata); mock(() => metadata);
await rejects(app.boundAppToken(owner.id, binding), "github_app_access_required"); await rejects(app.boundAppToken(owner.id, binding, "other/public"), "github_app_access_required");
} }
await Credentials.updateOne({ ownerId: owner.id }, { $set: { revoked: true } }); await Credentials.updateOne({ ownerId: owner.id }, { $set: { revoked: true } });
mock(() => { throw new Error("revoked grant must not reach GitHub"); }); mock(() => { throw new Error("revoked grant must not reach GitHub"); });
await rejects(app.boundAppToken(owner.id, binding), "github_app_reconnect_required"); await rejects(app.boundAppToken(owner.id, binding, "other/public"), "github_app_reconnect_required");
}); });
it("refreshes an expired App grant before reading a public repository", async () => { it("refreshes an expired App grant before reading a public repository", async () => {
await app.saveAppGrant(owner.id, data("old", -1)); await app.saveAppGrant(owner.id, data("old", -1));
mock(url => url.includes("/login/oauth/access_token") ? data("new") : { id: 7, private: false }); mock(url => url.includes("/login/oauth/access_token") ? data("new") : { id: 7, private: false, visibility: "public" });
expect(await app.boundAppToken(owner.id, { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" })) expect(await app.boundAppToken(owner.id, { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" }, "other/public"))
.to.equal("ghu_accessnew"); .to.equal("ghu_accessnew");
}); });
it("renews a public token inside an existing Octokit traversal without changing owner or quota", async () => {
await app.saveAppGrant(owner.id, data("old"));
const sent = [];
mock((url, options) => {
if (url.includes("/login/oauth/access_token")) return data("new");
if (url.includes("/git/trees/")) {
sent.push(new globalThis.Headers(options.headers).get("authorization"));
return { tree: [], truncated: false };
}
return { id: url.endsWith("/second") ? 8 : 7, private: false, visibility: "public" };
});
const binding = { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" };
const token = await app.boundAppToken(owner.id, binding, "other/public");
const oct = require("../src/core/GitHubUtils").octokit(token);
await app.boundAppToken(owner.id, { ...binding, repositoryId: 8 }, "other/second");
await oct.git.getTree({ owner: "other", repo: "public", tree_sha: "first" });
await Credentials.updateOne({ ownerId: owner.id, provider: app.APP_PROVIDER }, { $set: { expiresAt: new Date(0) } });
await oct.git.getTree({ owner: "other", repo: "public", tree_sha: "second" });
expect(sent).to.deep.equal(["token ghu_accessold", "token ghu_accessnew"]);
expect(githubQuotaKey(token)).to.equal(`app-user:${owner.id}`);
expect(githubQuotaKey("ghu_accessnew")).to.equal(githubQuotaKey(token));
});
it("rejects a replacement at the original name for repository and PR source reads", async () => {
await app.saveAppGrant(owner.id, data());
const binding = { kind: "github-app", publicRead: true, repositoryId: 7, revision: "public" };
const Repos = require("../src/core/model/anonymizedRepositories/anonymizedRepositories.model").default;
const PRs = require("../src/core/model/anonymizedPullRequests/anonymizedPullRequests.model").default;
const Repository = require("../src/core/Repository").default;
const PullRequest = require("../src/core/PullRequest").default;
const resources = [
new Repository(new Repos({ owner: owner.id, githubAccess: binding, source: { repositoryName: "other/original" } })),
new PullRequest(new PRs({ owner: owner.id, githubAccess: binding, source: { repositoryFullName: "other/original" } })),
];
mock(url => {
if (url.endsWith("/repositories/7")) return { id: 7, private: false, visibility: "public", full_name: "other/renamed" };
if (url.endsWith("/repos/other/original")) return { id: 8, private: false, visibility: "public" };
throw new Error("unexpected source lookup");
});
for (const resource of resources) await rejects(resource.getToken(), "github_app_access_required");
expect(calls.every(call => call.url.endsWith("/repos/other/original"))).to.equal(true);
mock(() => ({ id: 7, private: false, visibility: "public" }));
for (const resource of resources) expect(await resource.getToken()).to.equal("ghu_access1");
});
it("does not reinterpret missing or mixed installation bindings as public access", async () => { it("does not reinterpret missing or mixed installation bindings as public access", async () => {
await app.saveAppGrant(owner.id, data()); await app.saveAppGrant(owner.id, data());
mock(() => { throw new Error("invalid binding must not reach GitHub"); }); mock(() => { throw new Error("invalid binding must not reach GitHub"); });
for (const binding of [ for (const binding of [
{ kind: "github-app", repositoryId: 7, revision: "one" }, { kind: "github-app", repositoryId: 7, revision: "one" },
{ kind: "github-app", publicRead: true, repositoryId: 7, installationId: 4, revision: "one" }, { kind: "github-app", publicRead: true, repositoryId: 7, installationId: 4, revision: "one" },
]) await rejects(app.boundAppToken(owner.id, binding), "github_app_reconnect_required"); ]) await rejects(app.boundAppToken(owner.id, binding, "other/public"), "github_app_reconnect_required");
}); });
it("checks the user's access before minting a repository-restricted token", async () => { it("checks the user's access before minting a repository-restricted token", async () => {
await app.saveAppGrant(owner.id, data()); await app.saveAppGrant(owner.id, data());