fix: resolve duplicate connections to the same GitHub account (#13022)
## Thinking Path > - Paperclip lets people share agents while keeping GitHub access personal. > - Each managed Git or GitHub operation selects an eligible connection grant. > - Connecting the same GitHub account twice creates two grants. > - The old resolver counted grants and rejected them as competing identities. > - Managed commands then ran anonymously and reported a misleading login failure. > - This change compares GitHub account IDs and selects one eligible grant for the same account. > - The benefit is reliable access after reconnecting, with clear diagnostics for real failures. ## Linked Issues or Issue Description Refs #13005. **What happened?** Two active connections owned by one Paperclip user pointed to the same GitHub account. Managed Git refused both as ambiguous. The agent could not push, although the account was connected and had repository access. **Expected behavior** Multiple grants for the same GitHub account resolve to one eligible authorization. Different accounts remain ambiguous. Unavailable access explains its cause without blocking unrelated work. **Steps to reproduce** 1. Connect the same GitHub account twice for one Paperclip user and allow the shared agent through both connection audiences. 2. Start an instruction as that user. 3. Run managed gh or git push. Before this fix, no credential is provided. ## What Changed - Compare stable GitHub account IDs when more than one eligible grant exists. Never deduplicate by login alone. - Prefer an available grant, then the newest authorization with a stable ID tie-breaker. Refresh and webhook timestamps do not change the selection. - Keep the selected credential and connection policy together. Do not combine permissions or fall back from a dedicated account to a personal account. - Print the redacted unavailable reason in managed command output. Unrelated local operations still work anonymously. - Add database and executable launcher regressions, and document selection behavior. ## Verification - Final `pnpm -r typecheck` and `pnpm build` passed. - Fourteen operation credential integration tests passed, covering duplicate personal/dedicated grants, incomplete credentials, distinct accounts with the same login, missing identity metadata, revocation, membership, connection audiences, and A → B → A steering. Existing Git credential and gateway suites and both executable launcher tests also passed. - The local broad test run encountered three embedded-Postgres lifecycle timeouts and stale modules from edits made during that run. A fresh process rerun of all four affected suites passed all 35 tests. The full Node 24 CI test matrix passed on the final commit. - CI passed all 31 checks on `797973b30beb16ba5fa69ed281835e1ab812b449` (Storybook visual regression was correctly skipped). An unrelated Company Settings UI test failed once; the focused local reproduction and rerun of its CI shard both passed without code changes. - Fresh Greptile review of the final commit: 5/5, with no open findings. Security checks passed. - Live acceptance passed with both duplicate connections enabled: managed `gh api user` returned the expected account, managed `git push` succeeded, and the agent created #13023 and pushed its review fixes. No host login or credential changes were used. - Applied the final source/compiled patch to the affected instance with backups, after confirming no runs were active. Restarted service health and the final resolver selection were verified. The patch is an overlay on the existing deployment; this PR supplies the upstream fix. ## Risks The resolver selects one authorization for an already permitted GitHub account. It does not combine repository permissions across connections. If the selected authorization has narrower access, that operation can still be denied by GitHub. Different provider account IDs and unknown duplicate identities continue to fail closed. No schema, host credential, or connection permission changes are included. ## Model Used OpenAI GPT-6 through Codex assisted implementation and verification with shell, database, and browser tools. The exact model variant and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
ff24578765
commit
297d8741f5
|
|
@ -28,6 +28,17 @@ An explicit dedicated-agent grant overrides personal selection. Revoked, disable
|
|||
|
||||
Connection setup and permissions display: “This agent uses this GitHub account for everyone's work, instead of the person giving instructions.”
|
||||
|
||||
Multiple eligible connections for the same GitHub account are treated as one
|
||||
identity, using GitHub's stable account ID rather than its login. The resolver
|
||||
selects an available grant, preferring the newest authorization with a stable
|
||||
ID tie-breaker. Duplicate eligibility includes an active credential record with
|
||||
the correct owner, the OAuth access-token reference, and repository access
|
||||
metadata. It keeps that grant's credential and connection policy together;
|
||||
it does not combine repository access or bypass connection audiences. Distinct
|
||||
accounts or unidentifiable duplicate grants remain ambiguous. Managed commands
|
||||
print the redacted reason when GitHub access is unavailable, while unrelated
|
||||
local operations can still proceed without credentials.
|
||||
|
||||
Run details show identity revisions and redacted GitHub results: responsible person, selected login when available, personal/dedicated source, and an unavailable reason. Tasks do not receive an additional identity indicator or takeover action.
|
||||
|
||||
## Deployment and verification
|
||||
|
|
|
|||
|
|
@ -11,6 +11,25 @@ const cleanups: Array<() => Promise<unknown>> = [];
|
|||
afterEach(async () => { for (const cleanup of cleanups.splice(0).reverse()) await cleanup(); });
|
||||
|
||||
describe("managed GitHub launchers", () => {
|
||||
it("explains unavailable access while allowing local work without credentials", async () => {
|
||||
const root = await mkdtemp(path.join(os.tmpdir(), "paperclip-github-diagnostic-"));
|
||||
cleanups.push(() => rm(root, {recursive:true,force:true}));
|
||||
const bin = path.join(root,"managed"), realBin = path.join(root,"real");
|
||||
await mkdir(bin); await mkdir(realBin);
|
||||
await writeFile(path.join(bin,"gh"), githubLauncherSource(), {mode:0o700});
|
||||
await writeFile(path.join(realBin,"gh"), '#!/usr/bin/env node\nprocess.stdout.write(JSON.stringify({token:process.env.GH_TOKEN ?? null}));', {mode:0o700});
|
||||
const server = createServer((_req,res) => {
|
||||
res.setHeader("content-type","application/json");
|
||||
res.end(JSON.stringify({status:"unavailable",reason:"More than one managed GitHub identity matches this run",env:{GH_TOKEN:"must-not-be-used"}}));
|
||||
});
|
||||
await new Promise<void>(resolve => server.listen(0,"127.0.0.1",resolve));
|
||||
cleanups.push(() => new Promise<void>((resolve,reject) => server.close(error => error ? reject(error) : resolve())));
|
||||
const {port} = server.address() as {port:number};
|
||||
const result = await exec(path.join(bin,"gh"), [], {env:{...process.env,...githubBrokerEnvironment({GH_TOKEN:"host-token"},{url:`http://127.0.0.1:${port}`,token:"run-capability"}),PATH:`${bin}:${realBin}:${process.env.PATH}`}});
|
||||
expect(JSON.parse(result.stdout)).toEqual({token:null});
|
||||
expect(result.stderr).toContain("More than one managed GitHub identity matches this run");
|
||||
expect(result.stderr).not.toMatch(/host-token|must-not-be-used|run-capability/);
|
||||
});
|
||||
it("captures each command's identity and clears host credentials when the next person has none", async () => {
|
||||
const root = await mkdtemp(path.join(os.tmpdir(), "paperclip-github-launcher-test-"));
|
||||
cleanups.push(() => rm(root, { recursive: true, force: true }));
|
||||
|
|
|
|||
|
|
@ -56,6 +56,12 @@ async function main() {
|
|||
}
|
||||
if (!response.ok) throw new Error('GitHub credential context unavailable; retry this operation');
|
||||
const result = await response.json();
|
||||
if (result.status === 'unavailable') {
|
||||
const reason = typeof result.reason === 'string'
|
||||
? result.reason.replace(/[\x00-\x1f\x7f]/g, ' ').slice(0, 500)
|
||||
: 'Check the GitHub connection in Paperclip';
|
||||
process.stderr.write('Paperclip: GitHub access unavailable: ' + reason + '. Continuing without GitHub credentials.\n');
|
||||
}
|
||||
if (result.status === 'available') {
|
||||
for (const [key, value] of Object.entries(result.env || {})) {
|
||||
if (/^(GH_TOKEN|GITHUB_TOKEN|PAPERCLIP_GIT_TOKEN|GIT_TERMINAL_PROMPT|GIT_AUTHOR_(NAME|EMAIL)|GIT_COMMITTER_(NAME|EMAIL)|GIT_CONFIG_COUNT|GIT_CONFIG_(KEY|VALUE)_\d+)$/.test(key) && typeof value === 'string') env[key] = value;
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import { agents, companies, companyMemberships, companySecrets, connectionGrants
|
|||
import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js";
|
||||
import { initializeRunIdentity, reserveSteeredIdentity, acceptSteeredIdentity } from "../services/run-identity.js";
|
||||
import { resolveGitHubOperationCredentials } from "../services/github-operation-credentials.js";
|
||||
import { filterResolvedGitHubConnectionsForRun, resolveManagedGitHubIdentitySelection } from "../services/git-credentials.js";
|
||||
|
||||
const vault = vi.hoisted(() => ({
|
||||
resolveUserSecretValue: vi.fn(async (_company: string, input: { responsibleUserId: string }) => ({ value: `test-token-${input.responsibleUserId}` })),
|
||||
|
|
@ -37,9 +38,9 @@ const support = await getEmbeddedPostgresTestSupport();
|
|||
await db.insert(toolConnections).values({id:connectionId,companyId:input.companyId,applicationId,name:connectionId,uid:connectionId,transport:"mcp_remote",status:"active",enabled:true,credentialPolicy:dedicated?"per_agent":"per_user",config:{sourceTemplateKey:"github"}});
|
||||
await db.insert(toolConnectionInstalls).values({companyId:input.companyId,connectionId,targetType:"agent",targetId:input.agentId});
|
||||
if (!dedicated) await db.insert(userSecretDefinitions).values({id:definitionId,companyId:input.companyId,key:definitionId,name:"Test GitHub"});
|
||||
await db.insert(companySecrets).values({id:secretId,companyId:input.companyId,key:secretId,name:"Test token",scope:dedicated?"company":"user",ownerUserId:dedicated?null:user,userSecretDefinitionId:dedicated?null:definitionId});
|
||||
await db.insert(companySecrets).values({id:secretId,companyId:input.companyId,key:secretId,name:`Test token ${secretId}`,scope:dedicated?"company":"user",ownerUserId:dedicated?null:user,userSecretDefinitionId:dedicated?null:definitionId});
|
||||
await db.insert(connectionGrants).values({id,companyId:input.companyId,connectionId,kind:dedicated?"agent":"user",subjectUserId:dedicated?null:user,subjectAgentId:dedicated?input.agentId:null,status:"active",credentialSecretRefs:[{secretId,configPath:"oauth.access_token",versionSelector:"latest"}],providerTenant:{github:{userId:user,login:user,installationCount:1,repositoryCount:1,repositorySelection:"selected",installationIds:["1"],installationOwnerLogins:[user]}}});
|
||||
return {id,connectionId};
|
||||
return {id,connectionId,secretId,definitionId};
|
||||
}
|
||||
async function switchTo(input: Awaited<ReturnType<typeof seed>>, user:string) {
|
||||
const id=randomUUID();
|
||||
|
|
@ -67,10 +68,71 @@ const support = await getEmbeddedPostgresTestSupport();
|
|||
await db.update(companyMemberships).set({status:"inactive"}).where(eq(companyMemberships.companyId,input.companyId));
|
||||
expect((await resolveGitHubOperationCredentials(db,input)).status).toBe("unavailable");
|
||||
await db.update(companyMemberships).set({status:"active"}).where(eq(companyMemberships.companyId,input.companyId));
|
||||
await grant(input,"A");
|
||||
const differentAccount = await grant(input,"A");
|
||||
await db.update(connectionGrants).set({providerTenant:{github:{userId:"other-github-id",login:"A",installationCount:1,repositoryCount:1,repositorySelection:"selected",installationIds:["1"],installationOwnerLogins:["A"]}}}).where(eq(connectionGrants.id,differentAccount.id));
|
||||
expect((await resolveGitHubOperationCredentials(db,input)).reason).toMatch(/More than one/);
|
||||
await expect(resolveGitHubOperationCredentials(db,{...input,companyId:randomUUID()})).rejects.toThrow();
|
||||
});
|
||||
it("uses one stable grant when the same person connects the same GitHub account twice", async () => {
|
||||
const input = await seed();
|
||||
const first = await grant(input, "A");
|
||||
const second = await grant(input, "A");
|
||||
await db.update(connectionGrants).set({createdAt:new Date("2026-01-01"),updatedAt:new Date("2027-01-01")}).where(eq(connectionGrants.id,first.id));
|
||||
await db.update(connectionGrants).set({createdAt:new Date("2026-02-01")}).where(eq(connectionGrants.id,second.id));
|
||||
const context = {...input,responsibleUserId:"A"};
|
||||
expect((await resolveManagedGitHubIdentitySelection(db,input.companyId,context)).grant?.id).toBe(second.id);
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"available",login:"A",source:"personal"});
|
||||
const connections = [first,second].map(row => ({id:row.connectionId,config:{sourceTemplateKey:"github"}}));
|
||||
expect(await filterResolvedGitHubConnectionsForRun({db,...context,connections})).toEqual([connections[1]]);
|
||||
// A newer webhook on the old connection must not change the selected policy.
|
||||
await db.update(connectionGrants).set({updatedAt:new Date("2028-01-01")}).where(eq(connectionGrants.id,first.id));
|
||||
expect((await resolveManagedGitHubIdentitySelection(db,input.companyId,context)).grant?.id).toBe(second.id);
|
||||
await db.update(connectionGrants).set({status:"revoked"}).where(eq(connectionGrants.id,second.id));
|
||||
expect((await resolveManagedGitHubIdentitySelection(db,input.companyId,context)).grant?.id).toBe(first.id);
|
||||
await db.update(toolConnections).set({enabled:false}).where(eq(toolConnections.id,first.connectionId));
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"unavailable",env:{}});
|
||||
});
|
||||
it("does not conflate missing GitHub account IDs or another agent's connection audience", async () => {
|
||||
const input = await seed();
|
||||
const first = await grant(input,"A");
|
||||
const duplicate = await grant(input,"A");
|
||||
await db.update(connectionGrants).set({providerTenant:null}).where(eq(connectionGrants.id,duplicate.id));
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"unavailable",env:{}});
|
||||
await db.update(toolConnectionInstalls).set({targetId:randomUUID()}).where(eq(toolConnectionInstalls.connectionId,duplicate.connectionId));
|
||||
expect((await resolveManagedGitHubIdentitySelection(db,input.companyId,{...input,responsibleUserId:"A"})).grant?.id).toBe(first.id);
|
||||
await switchTo(input,"B");
|
||||
expect((await resolveGitHubOperationCredentials(db,input)).env).toEqual({});
|
||||
});
|
||||
it.each(["missing-ref", "disabled-secret", "missing-secret", "wrong-owner", "disabled-definition", "no-repositories"])(
|
||||
"ignores an incomplete newer duplicate when the same account has an eligible grant (%s)", async (problem) => {
|
||||
const input = await seed();
|
||||
const first = await grant(input,"A");
|
||||
const second = await grant(input,"A");
|
||||
await db.update(connectionGrants).set({createdAt:new Date("2026-01-01")}).where(eq(connectionGrants.id,first.id));
|
||||
await db.update(connectionGrants).set({createdAt:new Date("2026-02-01")}).where(eq(connectionGrants.id,second.id));
|
||||
if (problem === "missing-ref") await db.update(connectionGrants).set({credentialSecretRefs:[]}).where(eq(connectionGrants.id,second.id));
|
||||
if (problem === "disabled-secret") await db.update(companySecrets).set({status:"disabled"}).where(eq(companySecrets.id,second.secretId));
|
||||
if (problem === "disabled-definition") await db.update(userSecretDefinitions).set({status:"disabled"}).where(eq(userSecretDefinitions.id,second.definitionId));
|
||||
if (problem === "missing-secret") await db.delete(companySecrets).where(eq(companySecrets.id,second.secretId));
|
||||
if (problem === "wrong-owner") await db.update(companySecrets).set({ownerUserId:"B"}).where(eq(companySecrets.id,second.secretId));
|
||||
if (problem === "no-repositories") await db.update(connectionGrants).set({providerTenant:{github:{userId:"A",login:"A",installationCount:0,repositoryCount:0,repositorySelection:"none",installationIds:[],installationOwnerLogins:[]}}}).where(eq(connectionGrants.id,second.id));
|
||||
expect((await resolveManagedGitHubIdentitySelection(db,input.companyId,{...input,responsibleUserId:"A"})).grant?.id).toBe(first.id);
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"available",login:"A"});
|
||||
const connections = [first,second].map(row => ({id:row.connectionId,config:{sourceTemplateKey:"github"}}));
|
||||
expect(await filterResolvedGitHubConnectionsForRun({db,...input,responsibleUserId:"A",connections})).toEqual([connections[0]]);
|
||||
},
|
||||
);
|
||||
it("retains dedicated override semantics when the dedicated account has duplicate grants", async () => {
|
||||
const input = await seed();
|
||||
await grant(input,"A");
|
||||
const first = await grant(input,"robot",true);
|
||||
const second = await grant(input,"robot",true);
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"available",source:"dedicated",login:"robot"});
|
||||
await db.update(connectionGrants).set({status:"revoked"}).where(eq(connectionGrants.id,first.id));
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"available",source:"dedicated",login:"robot"});
|
||||
await db.update(connectionGrants).set({status:"revoked"}).where(eq(connectionGrants.id,second.id));
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"unavailable",source:"dedicated",env:{}});
|
||||
});
|
||||
it("honors dedicated overrides and never substitutes personal credentials when revoked or disabled", async () => {
|
||||
const input=await seed(); await grant(input,"A"); const dedicated=await grant(input,"robot",true);
|
||||
expect(await resolveGitHubOperationCredentials(db,input)).toMatchObject({status:"available",source:"dedicated",login:"robot"});
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import {
|
|||
connectionGrants,
|
||||
toolConnectionInstalls,
|
||||
toolConnections,
|
||||
userSecretDefinitions,
|
||||
type Db,
|
||||
} from "@paperclipai/db";
|
||||
import { and, eq, inArray, or } from "drizzle-orm";
|
||||
|
|
@ -354,7 +355,14 @@ export async function resolveManagedGitHubIdentitySelection(
|
|||
: [];
|
||||
const candidates = dedicated.length > 0 ? dedicated : personal.length > 0 ? personal : delegated;
|
||||
const identitySource = dedicated.length > 0 ? "dedicated" as const : "personal" as const;
|
||||
if (candidates.length !== 1) {
|
||||
// Reconnecting can create another connection/grant for the same GitHub
|
||||
// account. Ambiguity is about provider identities, not the number of rows.
|
||||
// Only trust GitHub's stable account ID; equal logins or missing metadata
|
||||
// cannot establish that two grants belong to the same person.
|
||||
const githubUserIds = candidates.map((candidate) => candidate.providerTenant?.github?.userId?.trim());
|
||||
if (candidates.length === 0 || (candidates.length > 1 && (
|
||||
githubUserIds.some((id) => !id) || new Set(githubUserIds).size !== 1
|
||||
))) {
|
||||
return {
|
||||
configured: true, identitySource,
|
||||
error: candidates.length === 0
|
||||
|
|
@ -362,7 +370,44 @@ export async function resolveManagedGitHubIdentitySelection(
|
|||
: "More than one managed GitHub identity matches this run",
|
||||
};
|
||||
}
|
||||
const grant = candidates[0]!;
|
||||
const credentialIds = candidates.flatMap((grant) => grant.credentialSecretRefs
|
||||
.filter((ref) => ref.configPath === "oauth.access_token").map((ref) => ref.secretId));
|
||||
const credentialRecords = candidates.length > 1 && credentialIds.length > 0
|
||||
? await db.select({
|
||||
id: companySecrets.id, status: companySecrets.status, deletedAt: companySecrets.deletedAt,
|
||||
scope: companySecrets.scope, ownerUserId: companySecrets.ownerUserId,
|
||||
definitionStatus: userSecretDefinitions.status, definitionDeletedAt: userSecretDefinitions.deletedAt,
|
||||
}).from(companySecrets).leftJoin(userSecretDefinitions, and(
|
||||
eq(userSecretDefinitions.id, companySecrets.userSecretDefinitionId),
|
||||
eq(userSecretDefinitions.companyId, companyId),
|
||||
)).where(and(
|
||||
eq(companySecrets.companyId, companyId), inArray(companySecrets.id, credentialIds),
|
||||
))
|
||||
: [];
|
||||
const hasCredentialRecord = (grant: typeof connectionGrants.$inferSelect) => {
|
||||
if (candidates.length === 1) return true;
|
||||
const github = grant.providerTenant?.github;
|
||||
const ref = grant.credentialSecretRefs.find((ref) => ref.configPath === "oauth.access_token");
|
||||
return Boolean(github && github.installationCount > 0 && github.repositoryCount > 0 && ref
|
||||
&& credentialRecords.some((secret) => secret.id === ref.secretId
|
||||
&& secret.status === "active" && !secret.deletedAt
|
||||
&& (grant.kind === "user"
|
||||
? secret.scope === "user" && secret.ownerUserId === grant.subjectUserId
|
||||
&& secret.definitionStatus === "active" && !secret.definitionDeletedAt
|
||||
: secret.scope === "company")));
|
||||
};
|
||||
const isAvailable = (grant: typeof connectionGrants.$inferSelect) =>
|
||||
grant.status === "active" && hasCredentialRecord(grant) && githubConnections.some((connection) =>
|
||||
connection.id === grant.connectionId && connection.enabled && connection.status === "active",
|
||||
);
|
||||
// Prefer an available authorization for this same account, then the newest
|
||||
// connection grant. Do not rank by updatedAt: refreshes/webhooks change it.
|
||||
// Select one grant, preserving its credential and connection policy intact.
|
||||
const grant = [...candidates].sort((a, b) =>
|
||||
Number(isAvailable(b)) - Number(isAvailable(a))
|
||||
|| b.createdAt.getTime() - a.createdAt.getTime()
|
||||
|| a.id.localeCompare(b.id),
|
||||
)[0]!;
|
||||
const connection = githubConnections.find((candidate) => candidate.id === grant.connectionId);
|
||||
if (!connection?.enabled || connection.status !== "active") {
|
||||
return { configured: true, identitySource, error: "The managed GitHub connection is unavailable" };
|
||||
|
|
|
|||
Loading…
Reference in New Issue