From 23be2cee9cb1b32448badaef9c4e72f05cb6ba93 Mon Sep 17 00:00:00 2001 From: Sankalp Thakur Date: Fri, 4 Sep 2026 02:23:38 +0400 Subject: [PATCH] fix(proxy): restore pusher email from the token cache checkUserPushPermission keys on action.userEmail. The token cache only stored the username, so a cache hit left the last committer's email in place and the permission check ran against the wrong user. Store email with the cached identity, and run resolveUserFromToken on tag pushes as well as branch pushes. Related to #1400 (cache-hit and tag-chain slices; GHES / unmatched gitAccount paths are unchanged). Signed-off-by: Sankalp Thakur --- src/proxy/chain.ts | 1 + .../push-action/resolveUserFromToken.ts | 9 ++-- .../processors/push-action/tokenIdentity.ts | 17 ++++--- test/chain.test.ts | 8 ++++ test/processors/resolveUserFromToken.test.ts | 44 ++++++++++++++++--- 5 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index cab32f5c4..18fa0db47 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -43,6 +43,7 @@ const branchPushChain: ProcessorExec[] = [ ]; const tagPushChain: ProcessorExec[] = [ + proc.push.resolveUserFromToken, proc.push.checkRepoInAuthorisedList, proc.push.checkUserPushPermission, proc.push.checkIfWaitingAuth, diff --git a/src/proxy/processors/push-action/resolveUserFromToken.ts b/src/proxy/processors/push-action/resolveUserFromToken.ts index 54c8e2099..2476fe1b1 100644 --- a/src/proxy/processors/push-action/resolveUserFromToken.ts +++ b/src/proxy/processors/push-action/resolveUserFromToken.ts @@ -73,8 +73,11 @@ async function exec(req: Request, action: Action): Promise { const cached = scmTokenCache.lookup(provider.name, token); if (cached) { - step.log(`${provider.name}: resolved push identity from cache: ${cached}`); - action.user = cached; + step.log(`${provider.name}: resolved push identity from cache: ${cached.username}`); + action.user = cached.username; + if (cached.email) { + action.userEmail = cached.email; + } action.addStep(step); return action; } @@ -97,7 +100,7 @@ async function exec(req: Request, action: Action): Promise { ); action.user = user.username; action.userEmail = user.email; - scmTokenCache.store(provider.name, token, user.username); + scmTokenCache.store(provider.name, token, user.username, user.email); } else { step.log( `No git-proxy user has gitAccount '${identity.login}' — ` + diff --git a/src/proxy/processors/push-action/tokenIdentity.ts b/src/proxy/processors/push-action/tokenIdentity.ts index 993ee6287..7690fbc9e 100644 --- a/src/proxy/processors/push-action/tokenIdentity.ts +++ b/src/proxy/processors/push-action/tokenIdentity.ts @@ -20,7 +20,9 @@ export type ScmUserInfo = { login: string; }; -type CacheEntry = { username: string; provider: string; cachedAt: number }; +export type CachedIdentity = { username: string; email: string | null }; + +type CacheEntry = CachedIdentity & { provider: string; cachedAt: number }; // 7 days — PATs are rarely rotated more frequently than this in practice; the cache is a // rate-limit optimization only (keys are one-way SHA-512 hashes, not recoverable tokens). const DEFAULT_TTL_MS = 7 * 24 * 60 * 60 * 1000; @@ -37,7 +39,7 @@ export class ScmTokenCache { return crypto.createHash('sha512').update(`${provider}:${token}`).digest('hex'); } - lookup(provider: string, token: string): string | null { + lookup(provider: string, token: string): CachedIdentity | null { const k = this.key(provider, token); const entry = this.cache.get(k); if (!entry) return null; @@ -46,11 +48,16 @@ export class ScmTokenCache { return null; } entry.cachedAt = Date.now(); - return entry.username; + return { username: entry.username, email: entry.email ?? null }; } - store(provider: string, token: string, username: string): void { - this.cache.set(this.key(provider, token), { username, provider, cachedAt: Date.now() }); + store(provider: string, token: string, username: string, email: string | null = null): void { + this.cache.set(this.key(provider, token), { + username, + email, + provider, + cachedAt: Date.now(), + }); } evictByUsername(provider: string, username: string): void { diff --git a/test/chain.test.ts b/test/chain.test.ts index afc6d7e1b..bdb4880e5 100644 --- a/test/chain.test.ts +++ b/test/chain.test.ts @@ -564,6 +564,14 @@ describe('proxy chain', function () { expect(pullChain).toEqual(chain.pullActionChain); }); + it('runs resolveUserFromToken before checkUserPushPermission on tag pushes', () => { + const tag = chain.tagPushChain; + const resolveIdx = tag.indexOf(processors.push.resolveUserFromToken); + const permIdx = tag.indexOf(processors.push.checkUserPushPermission); + expect(resolveIdx).toBeGreaterThanOrEqual(0); + expect(permIdx).toBeGreaterThan(resolveIdx); + }); + it('returns tagPushChain when action.type is push and action.actionType is TAG', async () => { const action = new Action( '2', diff --git a/test/processors/resolveUserFromToken.test.ts b/test/processors/resolveUserFromToken.test.ts index 14eec0c72..015d34bac 100644 --- a/test/processors/resolveUserFromToken.test.ts +++ b/test/processors/resolveUserFromToken.test.ts @@ -131,7 +131,16 @@ describe('ScmTokenCache', () => { it('should return username on cache hit', () => { const cache = new ScmTokenCache(); cache.store('github', 'sometoken', 'octocat'); - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); + }); + + it('should return stored email on cache hit', () => { + const cache = new ScmTokenCache(); + cache.store('github', 'sometoken', 'octocat', 'octocat@github.com'); + expect(cache.lookup('github', 'sometoken')).toEqual({ + username: 'octocat', + email: 'octocat@github.com', + }); }); it('should return null after TTL expires', () => { @@ -153,10 +162,10 @@ describe('ScmTokenCache', () => { provider: 'github', cachedAt: Date.now() - 90, }); - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); // hit resets cachedAt + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); // hit resets cachedAt // backdate again to 90ms — if TTL had not been reset, this would be 180ms total (expired) (cache as any).cache.get(key).cachedAt = Date.now() - 90; - expect(cache.lookup('github', 'sometoken')).toBe('octocat'); // still valid because TTL was reset + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'octocat', email: null }); // still valid because TTL was reset }); it('should not share entries across providers', () => { @@ -173,14 +182,14 @@ describe('ScmTokenCache', () => { cache.evictByUsername('github', 'alice'); expect(cache.lookup('github', 'token1')).toBeNull(); expect(cache.lookup('github', 'token2')).toBeNull(); - expect(cache.lookup('github', 'token3')).toBe('bob'); + expect(cache.lookup('github', 'token3')).toEqual({ username: 'bob', email: null }); }); it('should not evict across providers', () => { const cache = new ScmTokenCache(); cache.store('github', 'sometoken', 'alice'); cache.evictByUsername('gitlab', 'alice'); - expect(cache.lookup('github', 'sometoken')).toBe('alice'); + expect(cache.lookup('github', 'sometoken')).toEqual({ username: 'alice', email: null }); }); }); @@ -399,4 +408,29 @@ describe('resolveUserFromToken cache integration', () => { expect(result.user).toBe('cached-user'); expect(fetchSpy).not.toHaveBeenCalled(); }); + + it('should set userEmail from cache so permission checks the pusher not the last committer', async () => { + vi.doMock('../../src/db', () => ({ + findUserByGitAccount: vi.fn(), + })); + vi.doMock('../../src/proxy/processors/push-action/tokenIdentity', async () => { + const real = await vi.importActual< + typeof import('../../src/proxy/processors/push-action/tokenIdentity') + >('../../src/proxy/processors/push-action/tokenIdentity'); + const cache = new real.ScmTokenCache(); + cache.store('github', 'ghp_testtoken123', 'bob', 'bob@corp.example'); + return { ...real, scmTokenCache: cache }; + }); + const mod = await import('../../src/proxy/processors/push-action/resolveUserFromToken'); + const req = makeRequest(); + const action = makeAction('https://github.com/finos/git-proxy.git'); + action.user = 'eve'; + action.userEmail = 'eve@corp.example'; + + const result = await mod.exec(req, action); + + expect(result.user).toBe('bob'); + expect(result.userEmail).toBe('bob@corp.example'); + expect(fetchSpy).not.toHaveBeenCalled(); + }); });