Skip to content

Commit 963b176

Browse files
authored
fix(github): keep OAuth codes, state and tokens out of logs (#11066)
* fix(github): keep OAuth codes, state and tokens out of logs Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com> * fix(github): sanitize OAuth errors before analytics and logs Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com> --------- Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com> Co-authored-by: Michael Uray <michaeluray@users.noreply.github.com>
1 parent 1870da7 commit 963b176

4 files changed

Lines changed: 233 additions & 74 deletions

File tree

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
import type { PersonId, WorkspaceUuid } from '@hcengineering/core'
2+
3+
jest.mock('@hcengineering/analytics', () => ({
4+
Analytics: {
5+
handleError: jest.fn()
6+
}
7+
}))
8+
9+
import { Analytics } from '@hcengineering/analytics'
10+
import {
11+
getInstallationLogContext,
12+
getOAuthCallbackLogContext,
13+
githubLogErrorCategory,
14+
reportGithubCredentialError
15+
} from '../logContext'
16+
17+
const workspace = '00000000-0000-0000-0000-000000000001' as WorkspaceUuid
18+
const accountId = '00000000-0000-0000-0000-000000000002' as PersonId
19+
20+
describe('log contexts', () => {
21+
it('uses stable categories for credential-related request failures', () => {
22+
expect(githubLogErrorCategory).toEqual({
23+
installationMapping: 'installation-mapping-failed',
24+
installationRemoval: 'installation-removal-failed',
25+
oauthCallback: 'oauth-callback-failed',
26+
oauthExchange: 'oauth-exchange-failed',
27+
oauthTokenRefresh: 'oauth-token-refresh-failed'
28+
})
29+
})
30+
31+
it('contains only non-secret installation identifiers', () => {
32+
const context = getInstallationLogContext(workspace, accountId, 123)
33+
34+
expect(context).toEqual({ workspace, accountId, installationId: 123 })
35+
for (const secretField of ['token', 'code', 'state', 'secret']) {
36+
expect(context).not.toHaveProperty(secretField)
37+
}
38+
})
39+
40+
it('contains only non-secret OAuth callback identifiers', () => {
41+
const context = getOAuthCallbackLogContext(workspace, accountId)
42+
43+
expect(context).toEqual({ workspace, accountId })
44+
for (const secretField of ['token', 'code', 'state', 'secret']) {
45+
expect(context).not.toHaveProperty(secretField)
46+
}
47+
})
48+
49+
it('reports credential-bearing request failures with fresh, credential-free errors', () => {
50+
const clientSecret = 'client-secret-value'
51+
const code = 'oauth-code-value'
52+
const state = 'oauth-state-value'
53+
const token = 'installation-token-value'
54+
const original = Object.assign(
55+
new Error(
56+
`https://github.com/login/oauth/access_token?client_secret=${clientSecret}&code=${code}&state=${state}`
57+
),
58+
{ cause: new Error(`token=${token}`) }
59+
)
60+
const logger = { error: jest.fn() }
61+
62+
try {
63+
throw original
64+
} catch {
65+
reportGithubCredentialError(
66+
logger,
67+
'failed to request github access token',
68+
githubLogErrorCategory.oauthCallback
69+
)
70+
}
71+
72+
const analyticsError = (Analytics.handleError as jest.Mock).mock.calls[0][0] as Error
73+
const loggerArguments = logger.error.mock.calls[0]
74+
const serializedSinks = serializeWithErrorDetails([analyticsError, loggerArguments])
75+
76+
expect(Analytics.handleError).toHaveBeenCalledWith(analyticsError)
77+
expect(logger.error).toHaveBeenCalledWith('failed to request github access token', {
78+
error: githubLogErrorCategory.oauthCallback
79+
})
80+
expect(analyticsError).not.toBe(original)
81+
expect(analyticsError.message).toBe(githubLogErrorCategory.oauthCallback)
82+
expect(analyticsError).not.toHaveProperty('cause')
83+
expect(analyticsError.stack).not.toContain(clientSecret)
84+
expect(analyticsError.stack).not.toContain(code)
85+
expect(analyticsError.stack).not.toContain(state)
86+
expect(analyticsError.stack).not.toContain(token)
87+
expect(serializedSinks).not.toContain(clientSecret)
88+
expect(serializedSinks).not.toContain(code)
89+
expect(serializedSinks).not.toContain(state)
90+
expect(serializedSinks).not.toContain(token)
91+
})
92+
})
93+
94+
function serializeWithErrorDetails (value: unknown): string {
95+
return JSON.stringify(value, (_key, item) => {
96+
if (item instanceof Error) {
97+
return Object.fromEntries(Object.getOwnPropertyNames(item).map((name) => [name, item[name as keyof Error]]))
98+
}
99+
return item
100+
})
101+
}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
import { Analytics } from '@hcengineering/analytics'
2+
import type { AccountUuid, MeasureContext, PersonId, WorkspaceUuid } from '@hcengineering/core'
3+
4+
export const githubLogErrorCategory = {
5+
installationMapping: 'installation-mapping-failed',
6+
installationRemoval: 'installation-removal-failed',
7+
oauthCallback: 'oauth-callback-failed',
8+
oauthExchange: 'oauth-exchange-failed',
9+
oauthTokenRefresh: 'oauth-token-refresh-failed'
10+
} as const
11+
12+
export type GithubLogErrorCategory = (typeof githubLogErrorCategory)[keyof typeof githubLogErrorCategory]
13+
14+
/**
15+
* Deliberately discards the caught error. OAuth and request errors can contain
16+
* request URLs and credentials in their message, stack, or cause.
17+
*/
18+
export function createGithubCredentialSafeError (category: GithubLogErrorCategory): Error {
19+
return new Error(category)
20+
}
21+
22+
export function reportGithubCredentialError (
23+
ctx: Pick<MeasureContext, 'error'>,
24+
message: string,
25+
category: GithubLogErrorCategory,
26+
context: Record<string, unknown> = {}
27+
): Error {
28+
const error = createGithubCredentialSafeError(category)
29+
Analytics.handleError(error)
30+
ctx.error(message, { ...context, error: category })
31+
return error
32+
}
33+
34+
export function getInstallationLogContext<T extends PersonId | AccountUuid> (
35+
workspace: WorkspaceUuid,
36+
accountId: T,
37+
installationId: number
38+
): { workspace: WorkspaceUuid, accountId: T, installationId: number } {
39+
return { workspace, accountId, installationId }
40+
}
41+
42+
export function getOAuthCallbackLogContext (
43+
workspace: WorkspaceUuid,
44+
accountId: PersonId
45+
): { workspace: WorkspaceUuid, accountId: PersonId } {
46+
return { workspace, accountId }
47+
}

‎services/github/pod-github/src/platform.ts‎

Lines changed: 61 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,11 @@ import { type StorageAdapter } from '@hcengineering/server-core'
4444
import { join } from 'path'
4545
import { createPlatformClient } from './client'
4646
import config from './config'
47+
import {
48+
createGithubCredentialSafeError,
49+
githubLogErrorCategory,
50+
reportGithubCredentialError
51+
} from './logContext'
4752
import { GithubWorkerWorkspaceState, getGithubWorkerState } from './workspaceUtils'
4853
import { registerLoaders } from './loaders'
4954
import { createNotification } from './notifications'
@@ -403,7 +408,7 @@ export class PlatformWorker {
403408
.catch((err) => {
404409
if (err.status !== 404) {
405410
// Already deleted.
406-
ctx.error('error from github api', { error: err })
411+
ctx.error('error from github api', { error: githubLogErrorCategory.installationRemoval })
407412
}
408413
})
409414

@@ -511,9 +516,19 @@ export class PlatformWorker {
511516
false
512517
)
513518
}
514-
} catch (err: any) {
515-
Analytics.handleError(err)
516-
await this.updateAccountAuthRecord(ctx, payload, { error: errorToObj(err) }, undefined, false)
519+
} catch {
520+
reportGithubCredentialError(
521+
ctx,
522+
'failed to exchange github oauth token',
523+
githubLogErrorCategory.oauthExchange
524+
)
525+
await this.updateAccountAuthRecord(
526+
ctx,
527+
payload,
528+
{ error: githubLogErrorCategory.oauthExchange },
529+
undefined,
530+
false
531+
)
517532
}
518533
}
519534

@@ -740,48 +755,52 @@ export class PlatformWorker {
740755
async checkRefreshToken (ctx: MeasureContext, auth: GithubUserRecord, force: boolean = false): Promise<boolean> {
741756
const expired = auth.expiresIn != null && auth.expiresIn < Date.now() / 1000
742757
if (auth.refreshToken != null && (force || expired)) {
743-
const uri =
744-
'https://github.com/login/oauth/access_token?' +
745-
makeQuery({
746-
client_id: config.ClientID,
747-
client_secret: config.ClientSecret,
748-
grant_type: 'refresh_token',
749-
refresh_token: auth.refreshToken
758+
try {
759+
const uri =
760+
'https://github.com/login/oauth/access_token?' +
761+
makeQuery({
762+
client_id: config.ClientID,
763+
client_secret: config.ClientSecret,
764+
grant_type: 'refresh_token',
765+
refresh_token: auth.refreshToken
766+
})
767+
768+
const result = await fetch(uri, {
769+
method: 'POST',
770+
headers: {
771+
Accept: 'application/json'
772+
}
750773
})
774+
const resultJson = await result.json()
751775

752-
const result = await fetch(uri, {
753-
method: 'POST',
754-
headers: {
755-
Accept: 'application/json'
756-
}
757-
})
758-
const resultJson = await result.json()
776+
if (resultJson.error !== undefined) {
777+
// We need to clear github integration info.
778+
await this.revokeUserAuth(ctx, auth)
779+
return false
780+
} else {
781+
// Update okit
782+
const nowTime = Date.now() / 1000
783+
const dta: GithubUserRecord = {
784+
...auth,
785+
token: resultJson.access_token,
786+
code: null,
787+
expiresIn: nowTime + (resultJson.expires_in as number),
788+
refreshToken: resultJson.refresh_token,
789+
refreshTokenExpiresIn: nowTime + (resultJson.refresh_token_expires_in as number),
790+
scope: resultJson.scope
791+
}
792+
auth.token = resultJson.access_token
793+
auth.code = null
794+
auth.expiresIn = dta.expiresIn
795+
auth.refreshToken = dta.refreshToken
796+
auth.refreshTokenExpiresIn = dta.refreshTokenExpiresIn
797+
auth.scope = dta.scope
759798

760-
if (resultJson.error !== undefined) {
761-
// We need to clear github integration info.
762-
await this.revokeUserAuth(ctx, auth)
763-
return false
764-
} else {
765-
// Update okit
766-
const nowTime = Date.now() / 1000
767-
const dta: GithubUserRecord = {
768-
...auth,
769-
token: resultJson.access_token,
770-
code: null,
771-
expiresIn: nowTime + (resultJson.expires_in as number),
772-
refreshToken: resultJson.refresh_token,
773-
refreshTokenExpiresIn: nowTime + (resultJson.refresh_token_expires_in as number),
774-
scope: resultJson.scope
799+
await this.userManager.updateUser(dta)
800+
return true
775801
}
776-
auth.token = resultJson.access_token
777-
auth.code = null
778-
auth.expiresIn = dta.expiresIn
779-
auth.refreshToken = dta.refreshToken
780-
auth.refreshTokenExpiresIn = dta.refreshTokenExpiresIn
781-
auth.scope = dta.scope
782-
783-
await this.userManager.updateUser(dta)
784-
return true
802+
} catch {
803+
throw createGithubCredentialSafeError(githubLogErrorCategory.oauthTokenRefresh)
785804
}
786805
}
787806
return true

‎services/github/pod-github/src/server.ts‎

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@ import { createNodeMiddleware } from '@octokit/webhooks'
22
import { App } from 'octokit'
33

44
import config from './config'
5+
import {
6+
getInstallationLogContext,
7+
getOAuthCallbackLogContext,
8+
githubLogErrorCategory,
9+
reportGithubCredentialError
10+
} from './logContext'
511
import { PlatformWorker } from './platform'
612

713
import bp from 'body-parser'
@@ -80,28 +86,20 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro
8086
} = req.body
8187
try {
8288
const decodedToken = decodeToken(payloadData.token)
83-
ctx.info('/api/v1/installation', {
84-
email: decodedToken.account,
85-
workspaceName: decodedToken.workspace,
86-
body: req.body
87-
})
89+
ctx.info(
90+
'/api/v1/installation',
91+
getInstallationLogContext(decodedToken.workspace, payloadData.accountId, payloadData.installationId)
92+
)
8893

8994
await ctx.with('map-installation', {}, (ctx) =>
9095
worker.mapInstallation(ctx, decodedToken.workspace, payloadData.installationId, payloadData.accountId)
9196
)
9297
res.status(200)
9398
res.json({})
94-
} catch (err: any) {
95-
Analytics.handleError(err)
96-
const tok = decodeToken(payloadData.token, false)
97-
ctx.error('failed to map-installation', {
98-
workspace: tok.workspace,
99-
installationid: payloadData.installationId,
100-
email: tok?.account,
101-
error: err.message
102-
})
99+
} catch {
100+
reportGithubCredentialError(ctx, 'failed to map-installation', githubLogErrorCategory.installationMapping)
103101
res.status(401)
104-
res.json({ error: err.message })
102+
res.json({ error: githubLogErrorCategory.installationMapping })
105103
}
106104
})
107105

@@ -122,12 +120,7 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro
122120
} = JSON.parse(atob(payloadData.state))
123121

124122
const decodedToken = decodeToken(decodedData.token)
125-
ctx.info('request github access-token', {
126-
workspace: decodedToken.workspace,
127-
accountId: payloadData.accountId,
128-
code: payloadData.code,
129-
state: payloadData.state
130-
})
123+
ctx.info('request github access-token', getOAuthCallbackLogContext(decodedToken.workspace, decodedData.accountId))
131124
await ctx.with('request-github-access-token', {}, async (ctx) => {
132125
await worker.requestGithubAccessToken(ctx, {
133126
workspace: decodedToken.workspace,
@@ -138,10 +131,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro
138131
})
139132
res.status(200)
140133
res.json({})
141-
} catch (err: any) {
142-
Analytics.handleError(err)
134+
} catch {
135+
reportGithubCredentialError(ctx, 'failed to request github access token', githubLogErrorCategory.oauthCallback)
143136
res.status(401)
144-
res.json({ error: err.message })
137+
res.json({ error: githubLogErrorCategory.oauthCallback })
145138
}
146139
})
147140

@@ -154,11 +147,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro
154147
} = req.body
155148

156149
const decodedToken = decodeToken(payloadData.token)
157-
ctx.info('/api/v1/installation-remove', {
158-
email: decodedToken.account,
159-
workspaceName: decodedToken.workspace,
160-
body: req.body
161-
})
150+
ctx.info(
151+
'/api/v1/installation-remove',
152+
getInstallationLogContext(decodedToken.workspace, decodedToken.account, payloadData.installationId)
153+
)
162154

163155
ctx.info('remove-installation', {
164156
workspace: decodedToken.workspace,
@@ -169,10 +161,10 @@ export async function start (ctx: MeasureContext, brandingMap: BrandingMap): Pro
169161
)
170162
res.status(200)
171163
res.json({})
172-
} catch (err: any) {
173-
Analytics.handleError(err)
164+
} catch {
165+
reportGithubCredentialError(ctx, 'failed to remove installation', githubLogErrorCategory.installationRemoval)
174166
res.status(401)
175-
res.json({ error: err.message })
167+
res.json({ error: githubLogErrorCategory.installationRemoval })
176168
}
177169
})
178170

0 commit comments

Comments
 (0)