diff --git a/services/github/pod-github/src/__tests__/logContext.test.ts b/services/github/pod-github/src/__tests__/logContext.test.ts new file mode 100644 index 0000000000..33d877ea7b --- /dev/null +++ b/services/github/pod-github/src/__tests__/logContext.test.ts @@ -0,0 +1,101 @@ +import type { PersonId, WorkspaceUuid } from '@hcengineering/core' + +jest.mock('@hcengineering/analytics', () => ({ + Analytics: { + handleError: jest.fn() + } +})) + +import { Analytics } from '@hcengineering/analytics' +import { + getInstallationLogContext, + getOAuthCallbackLogContext, + githubLogErrorCategory, + reportGithubCredentialError +} from '../logContext' + +const workspace = '00000000-0000-0000-0000-000000000001' as WorkspaceUuid +const accountId = '00000000-0000-0000-0000-000000000002' as PersonId + +describe('log contexts', () => { + it('uses stable categories for credential-related request failures', () => { + expect(githubLogErrorCategory).toEqual({ + installationMapping: 'installation-mapping-failed', + installationRemoval: 'installation-removal-failed', + oauthCallback: 'oauth-callback-failed', + oauthExchange: 'oauth-exchange-failed', + oauthTokenRefresh: 'oauth-token-refresh-failed' + }) + }) + + it('contains only non-secret installation identifiers', () => { + const context = getInstallationLogContext(workspace, accountId, 123) + + expect(context).toEqual({ workspace, accountId, installationId: 123 }) + for (const secretField of ['token', 'code', 'state', 'secret']) { + expect(context).not.toHaveProperty(secretField) + } + }) + + it('contains only non-secret OAuth callback identifiers', () => { + const context = getOAuthCallbackLogContext(workspace, accountId) + + expect(context).toEqual({ workspace, accountId }) + for (const secretField of ['token', 'code', 'state', 'secret']) { + expect(context).not.toHaveProperty(secretField) + } + }) + + it('reports credential-bearing request failures with fresh, credential-free errors', () => { + const clientSecret = 'client-secret-value' + const code = 'oauth-code-value' + const state = 'oauth-state-value' + const token = 'installation-token-value' + const original = Object.assign( + new Error( + `https://github.com/login/oauth/access_token?client_secret=${clientSecret}&code=${code}&state=${state}` + ), + { cause: new Error(`token=${token}`) } + ) + const logger = { error: jest.fn() } + + try { + throw original + } catch { + reportGithubCredentialError( + logger, + 'failed to request github access token', + githubLogErrorCategory.oauthCallback + ) + } + + const analyticsError = (Analytics.handleError as jest.Mock).mock.calls[0][0] as Error + const loggerArguments = logger.error.mock.calls[0] + const serializedSinks = serializeWithErrorDetails([analyticsError, loggerArguments]) + + expect(Analytics.handleError).toHaveBeenCalledWith(analyticsError) + expect(logger.error).toHaveBeenCalledWith('failed to request github access token', { + error: githubLogErrorCategory.oauthCallback + }) + expect(analyticsError).not.toBe(original) + expect(analyticsError.message).toBe(githubLogErrorCategory.oauthCallback) + expect(analyticsError).not.toHaveProperty('cause') + expect(analyticsError.stack).not.toContain(clientSecret) + expect(analyticsError.stack).not.toContain(code) + expect(analyticsError.stack).not.toContain(state) + expect(analyticsError.stack).not.toContain(token) + expect(serializedSinks).not.toContain(clientSecret) + expect(serializedSinks).not.toContain(code) + expect(serializedSinks).not.toContain(state) + expect(serializedSinks).not.toContain(token) + }) +}) + +function serializeWithErrorDetails (value: unknown): string { + return JSON.stringify(value, (_key, item) => { + if (item instanceof Error) { + return Object.fromEntries(Object.getOwnPropertyNames(item).map((name) => [name, item[name as keyof Error]])) + } + return item + }) +} diff --git a/services/github/pod-github/src/logContext.ts b/services/github/pod-github/src/logContext.ts new file mode 100644 index 0000000000..c4d7425faa --- /dev/null +++ b/services/github/pod-github/src/logContext.ts @@ -0,0 +1,47 @@ +import { Analytics } from '@hcengineering/analytics' +import type { AccountUuid, MeasureContext, PersonId, WorkspaceUuid } from '@hcengineering/core' + +export const githubLogErrorCategory = { + installationMapping: 'installation-mapping-failed', + installationRemoval: 'installation-removal-failed', + oauthCallback: 'oauth-callback-failed', + oauthExchange: 'oauth-exchange-failed', + oauthTokenRefresh: 'oauth-token-refresh-failed' +} as const + +export type GithubLogErrorCategory = (typeof githubLogErrorCategory)[keyof typeof githubLogErrorCategory] + +/** + * Deliberately discards the caught error. OAuth and request errors can contain + * request URLs and credentials in their message, stack, or cause. + */ +export function createGithubCredentialSafeError (category: GithubLogErrorCategory): Error { + return new Error(category) +} + +export function reportGithubCredentialError ( + ctx: Pick, + message: string, + category: GithubLogErrorCategory, + context: Record = {} +): Error { + const error = createGithubCredentialSafeError(category) + Analytics.handleError(error) + ctx.error(message, { ...context, error: category }) + return error +} + +export function getInstallationLogContext ( + workspace: WorkspaceUuid, + accountId: T, + installationId: number +): { workspace: WorkspaceUuid, accountId: T, installationId: number } { + return { workspace, accountId, installationId } +} + +export function getOAuthCallbackLogContext ( + workspace: WorkspaceUuid, + accountId: PersonId +): { workspace: WorkspaceUuid, accountId: PersonId } { + return { workspace, accountId } +} diff --git a/services/github/pod-github/src/platform.ts b/services/github/pod-github/src/platform.ts index 97455c5eb0..70d4b32322 100644 --- a/services/github/pod-github/src/platform.ts +++ b/services/github/pod-github/src/platform.ts @@ -44,6 +44,11 @@ import { type StorageAdapter } from '@hcengineering/server-core' import { join } from 'path' import { createPlatformClient } from './client' import config from './config' +import { + createGithubCredentialSafeError, + githubLogErrorCategory, + reportGithubCredentialError +} from './logContext' import { GithubWorkerWorkspaceState, getGithubWorkerState } from './workspaceUtils' import { registerLoaders } from './loaders' import { createNotification } from './notifications' @@ -403,7 +408,7 @@ export class PlatformWorker { .catch((err) => { if (err.status !== 404) { // Already deleted. - ctx.error('error from github api', { error: err }) + ctx.error('error from github api', { error: githubLogErrorCategory.installationRemoval }) } }) @@ -511,9 +516,19 @@ export class PlatformWorker { false ) } - } catch (err: any) { - Analytics.handleError(err) - await this.updateAccountAuthRecord(ctx, payload, { error: errorToObj(err) }, undefined, false) + } catch { + reportGithubCredentialError( + ctx, + 'failed to exchange github oauth token', + githubLogErrorCategory.oauthExchange + ) + await this.updateAccountAuthRecord( + ctx, + payload, + { error: githubLogErrorCategory.oauthExchange }, + undefined, + false + ) } } @@ -740,48 +755,52 @@ export class PlatformWorker { async checkRefreshToken (ctx: MeasureContext, auth: GithubUserRecord, force: boolean = false): Promise { const expired = auth.expiresIn != null && auth.expiresIn < Date.now() / 1000 if (auth.refreshToken != null && (force || expired)) { - const uri = - 'https://github.com/login/oauth/access_token?' + - makeQuery({ - client_id: config.ClientID, - client_secret: config.ClientSecret, - grant_type: 'refresh_token', - refresh_token: auth.refreshToken + try { + const uri = + 'https://github.com/login/oauth/access_token?' + + makeQuery({ + client_id: config.ClientID, + client_secret: config.ClientSecret, + grant_type: 'refresh_token', + refresh_token: auth.refreshToken + }) + + const result = await fetch(uri, { + method: 'POST', + headers: { + Accept: 'application/json' + } }) + const resultJson = await result.json() - const result = await fetch(uri, { - method: 'POST', - headers: { - Accept: 'application/json' + if (resultJson.error !== undefined) { + // We need to clear github integration info. + await this.revokeUserAuth(ctx, auth) + return false + } else { + // Update okit + const nowTime = Date.now() / 1000 + const dta: GithubUserRecord = { + ...auth, + token: resultJson.access_token, + code: null, + expiresIn: nowTime + (resultJson.expires_in as number), + refreshToken: resultJson.refresh_token, + refreshTokenExpiresIn: nowTime + (resultJson.refresh_token_expires_in as number), + scope: resultJson.scope + } + auth.token = resultJson.access_token + auth.code = null + auth.expiresIn = dta.expiresIn + auth.refreshToken = dta.refreshToken + auth.refreshTokenExpiresIn = dta.refreshTokenExpiresIn + auth.scope = dta.scope + + await this.userManager.updateUser(dta) + return true } - }) - const resultJson = await result.json() - - if (resultJson.error !== undefined) { - // We need to clear github integration info. - await this.revokeUserAuth(ctx, auth) - return false - } else { - // Update okit - const nowTime = Date.now() / 1000 - const dta: GithubUserRecord = { - ...auth, - token: resultJson.access_token, - code: null, - expiresIn: nowTime + (resultJson.expires_in as number), - refreshToken: resultJson.refresh_token, - refreshTokenExpiresIn: nowTime + (resultJson.refresh_token_expires_in as number), - scope: resultJson.scope - } - auth.token = resultJson.access_token - auth.code = null - auth.expiresIn = dta.expiresIn - auth.refreshToken = dta.refreshToken - auth.refreshTokenExpiresIn = dta.refreshTokenExpiresIn - auth.scope = dta.scope - - await this.userManager.updateUser(dta) - return true + } catch { + throw createGithubCredentialSafeError(githubLogErrorCategory.oauthTokenRefresh) } } return true diff --git a/services/github/pod-github/src/server.ts b/services/github/pod-github/src/server.ts index 05a0b123bb..489c8120ce 100644 --- a/services/github/pod-github/src/server.ts +++ b/services/github/pod-github/src/server.ts @@ -2,6 +2,12 @@ import { createNodeMiddleware } from '@octokit/webhooks' import { App } from 'octokit' import config from './config' +import { + getInstallationLogContext, + getOAuthCallbackLogContext, + githubLogErrorCategory, + reportGithubCredentialError +} from './logContext' import { PlatformWorker } from './platform' import bp from 'body-parser' @@ -80,28 +86,20 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro } = req.body try { const decodedToken = decodeToken(payloadData.token) - ctx.info('/api/v1/installation', { - email: decodedToken.account, - workspaceName: decodedToken.workspace, - body: req.body - }) + ctx.info( + '/api/v1/installation', + getInstallationLogContext(decodedToken.workspace, payloadData.accountId, payloadData.installationId) + ) await ctx.with('map-installation', {}, (ctx) => worker.mapInstallation(ctx, decodedToken.workspace, payloadData.installationId, payloadData.accountId) ) res.status(200) res.json({}) - } catch (err: any) { - Analytics.handleError(err) - const tok = decodeToken(payloadData.token, false) - ctx.error('failed to map-installation', { - workspace: tok.workspace, - installationid: payloadData.installationId, - email: tok?.account, - error: err.message - }) + } catch { + reportGithubCredentialError(ctx, 'failed to map-installation', githubLogErrorCategory.installationMapping) res.status(401) - res.json({ error: err.message }) + res.json({ error: githubLogErrorCategory.installationMapping }) } }) @@ -122,12 +120,7 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro } = JSON.parse(atob(payloadData.state)) const decodedToken = decodeToken(decodedData.token) - ctx.info('request github access-token', { - workspace: decodedToken.workspace, - accountId: payloadData.accountId, - code: payloadData.code, - state: payloadData.state - }) + ctx.info('request github access-token', getOAuthCallbackLogContext(decodedToken.workspace, decodedData.accountId)) await ctx.with('request-github-access-token', {}, async (ctx) => { await worker.requestGithubAccessToken(ctx, { workspace: decodedToken.workspace, @@ -138,10 +131,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro }) res.status(200) res.json({}) - } catch (err: any) { - Analytics.handleError(err) + } catch { + reportGithubCredentialError(ctx, 'failed to request github access token', githubLogErrorCategory.oauthCallback) res.status(401) - res.json({ error: err.message }) + res.json({ error: githubLogErrorCategory.oauthCallback }) } }) @@ -154,11 +147,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro } = req.body const decodedToken = decodeToken(payloadData.token) - ctx.info('/api/v1/installation-remove', { - email: decodedToken.account, - workspaceName: decodedToken.workspace, - body: req.body - }) + ctx.info( + '/api/v1/installation-remove', + getInstallationLogContext(decodedToken.workspace, decodedToken.account, payloadData.installationId) + ) ctx.info('remove-installation', { workspace: decodedToken.workspace, @@ -169,10 +161,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro ) res.status(200) res.json({}) - } catch (err: any) { - Analytics.handleError(err) + } catch { + reportGithubCredentialError(ctx, 'failed to remove installation', githubLogErrorCategory.installationRemoval) res.status(401) - res.json({ error: err.message }) + res.json({ error: githubLogErrorCategory.installationRemoval }) } })