From 8f462463b6293d6840ca577101db00922697cec1 Mon Sep 17 00:00:00 2001 From: sutterj Date: Tue, 11 Jun 2024 20:48:41 -0400 Subject: [PATCH] feat: update tslog json transport - update tslog json transport to spit out proper json - sanitize log message in some places - add additional secret mask regex values --- src/app/api/auth/lib/nextauth-options.ts | 8 +-- src/app/context/AuthProvider.tsx | 7 +- src/bot/config.ts | 9 ++- src/bot/rules.ts | 82 +++++++++++++++--------- src/server/octokit/controller.ts | 2 +- src/server/repos/controller.ts | 20 +++--- src/utils/logger.ts | 58 +++++++++-------- 7 files changed, 107 insertions(+), 79 deletions(-) diff --git a/src/app/api/auth/lib/nextauth-options.ts b/src/app/api/auth/lib/nextauth-options.ts index 9a84771..a0040b5 100644 --- a/src/app/api/auth/lib/nextauth-options.ts +++ b/src/app/api/auth/lib/nextauth-options.ts @@ -118,16 +118,16 @@ export const nextAuthOptions: AuthOptions = { if (!(metadata instanceof Error) && metadata.provider) { // redact the provider secret here delete metadata.provider - authLogger.error({ code, metadata }) + authLogger.error('Auth error', { code, metadata }) } else { - authLogger.error({ code, metadata }) + authLogger.error('Auth error', { code, metadata }) } }, warn(code) { - authLogger.warn({ code }) + authLogger.warn('Auth warn', { code }) }, debug(code, metadata) { - authLogger.debug({ code, metadata }) + authLogger.debug('Auth debug', { code, metadata }) }, }, callbacks: { diff --git a/src/app/context/AuthProvider.tsx b/src/app/context/AuthProvider.tsx index 1058417..a4d1957 100644 --- a/src/app/context/AuthProvider.tsx +++ b/src/app/context/AuthProvider.tsx @@ -5,6 +5,9 @@ import { Session } from 'next-auth' import { SessionProvider, signOut, useSession } from 'next-auth/react' import { ReactNode, useEffect } from 'react' +import { logger } from 'utils/logger' + +const authProviderLogger = logger.getSubLogger({ name: 'auth-provider' }) const VerifiedAuthProvider = ({ children }: { children: ReactNode }) => { const session = useSession() @@ -16,12 +19,12 @@ const VerifiedAuthProvider = ({ children }: { children: ReactNode }) => { } if (session.data?.error === 'RefreshAccessTokenError') { - console.error('Could not refresh access token - signing out') + authProviderLogger.error('Could not refresh access token - signing out') signOut() } if (session.data && new Date(session.data.expires) < new Date()) { - console.log('session expired - signing out') + authProviderLogger.info('session expired - signing out') signOut() } }, [ diff --git a/src/bot/config.ts b/src/bot/config.ts index 2a8c3ca..d5b815f 100644 --- a/src/bot/config.ts +++ b/src/bot/config.ts @@ -55,9 +55,8 @@ export const getEnvConfig = () => { export const validateConfig = (config: InternalContributionForksConfig) => { try { internalContributionForksConfig.parse(config) - } catch (e) { - configLogger.error('Invalid config found!') - configLogger.error(e) + } catch (error) { + configLogger.error('Invalid config found!', { error }) throw new Error( 'Invalid config found! Please check the config and error log for more details.', ) @@ -82,7 +81,7 @@ export const getConfig = async (orgId?: string) => { // Lastly check github for a config if (!orgId) { - logger.error( + configLogger.error( 'No orgId present, Organization ID is required to set a config when not using environment variables', ) throw new Error('Organization ID is required to set a config!') @@ -90,7 +89,7 @@ export const getConfig = async (orgId?: string) => { config = await getGitHubConfig(orgId) - logger.info(`Using following config values`, { + configLogger.info(`Using following config values`, { config, }) diff --git a/src/bot/rules.ts b/src/bot/rules.ts index b2c480d..9e8dd4d 100644 --- a/src/bot/rules.ts +++ b/src/bot/rules.ts @@ -25,13 +25,13 @@ export const createAllPushProtection = async ( repositoryNodeId: string, actorNodeId: string, ) => { - rulesLogger.debug('Creating branch protection ruleset for fork', { + rulesLogger.info('Creating branch protection for all branches', { repositoryOwner: context.payload.repository.owner.login, repositoryName: context.payload.repository.name, }) try { - // Add branch protection via rulesets to the all branches + // Add branch protection via rulesets to all branches await createBranchProtectionRuleset( context, actorNodeId, @@ -40,22 +40,35 @@ export const createAllPushProtection = async ( ) } catch (error) { rulesLogger.error( - 'Failed to create branch protection for fork, falling back to branch protections', + 'Failed to create branch protection via rulesets, falling back to branch protections', { error, }, ) - const createBranchProtectionRes = await createBranchProtection( - context, - repositoryNodeId, - '*', - actorNodeId, - ) + try { + // Add branch protection via GQL to all branches + await createBranchProtection(context, repositoryNodeId, '*', actorNodeId) + } catch (error) { + rulesLogger.error( + 'Failed to create branch protection via GQL, falling back to REST', + { + error, + }, + ) - rulesLogger.info('Branch protection created', { - createBranchProtectionRes, - }) + try { + // Add branch protection via REST to all branches + await createBranchProtectionREST(context, '*') + } catch (error) { + rulesLogger.error( + 'Failed to create branch protection for all branches', + { + error, + }, + ) + } + } } } @@ -73,13 +86,13 @@ export const createDefaultBranchProtection = async ( actorNodeId: string, defaultBranch: string, ) => { - rulesLogger.debug('Creating branch protection ruleset for mirror', { + rulesLogger.info('Creating branch protection for default branch', { repositoryOwner: context.payload.repository.owner.login, repositoryName: context.payload.repository.name, }) try { - // Add branch protections via ruleset to the default branch + // Add branch protection via ruleset to the default branch await createBranchProtectionRuleset( context, actorNodeId, @@ -96,17 +109,14 @@ export const createDefaultBranchProtection = async ( ) try { - const createBranchProtectionRes = await createBranchProtection( + // Add branch protection via GQL to the default branch + await createBranchProtection( context, repositoryNodeId, defaultBranch, actorNodeId, true, ) - - rulesLogger.info('Branch protection created', { - createBranchProtectionRes, - }) } catch (error) { rulesLogger.error( 'Failed to create branch protection from BP GQL, trying REST instead', @@ -115,13 +125,17 @@ export const createDefaultBranchProtection = async ( }, ) - const createBranchProtectionRes = await createBranchProtectionREST( - context, - defaultBranch, - ) - rulesLogger.info('Branch protection created', { - createBranchProtectionRes, - }) + try { + // Add branch protection via REST to the default branch + await createBranchProtectionREST(context, defaultBranch) + } catch (error) { + rulesLogger.error( + 'Failed to create branch protection for default branch', + { + error, + }, + ) + } } } } @@ -140,6 +154,10 @@ const createBranchProtectionRuleset = async ( includeRefs: string[], isMirror = false, ) => { + rulesLogger.info('Creating branch protection via rulesets', { + isMirror, + }) + // Get the current branch protection rulesets const getBranchProtectionRuleset = await context.octokit.graphql<{ repository: Repository @@ -153,7 +171,7 @@ const createBranchProtectionRuleset = async ( (ruleset) => ruleset?.name === ruleName, ) ) { - rulesLogger.info('Branch protection rule already exists', { + rulesLogger.info('Branch protection ruleset already exists', { getBranchProtectionRuleset, }) @@ -172,7 +190,7 @@ const createBranchProtectionRuleset = async ( includeRefs, }) - rulesLogger.info('Created branch protection rule', { + rulesLogger.info('Created branch protection via rulesets', { branchProtectionRuleset, }) } @@ -191,7 +209,7 @@ const createBranchProtection = async ( actorId: string, isMirror = false, ) => { - rulesLogger.info('Creating branch protection', { + rulesLogger.info('Creating branch protection via GQL', { isMirror, }) @@ -203,7 +221,7 @@ const createBranchProtection = async ( actorId, }) - rulesLogger.info('Created branch protection', { + rulesLogger.info('Created branch protection via GQL', { forkBranchProtection, }) } @@ -217,6 +235,8 @@ const createBranchProtectionREST = async ( context: ContextEvent, pattern: string, ) => { + rulesLogger.info('Creating branch protection via REST') + const res = await context.octokit.repos.updateBranchProtection({ branch: pattern, enforce_admins: true, @@ -235,7 +255,7 @@ const createBranchProtectionREST = async ( restrictions: null, }) - rulesLogger.info('Created branch protection rule to default branch', { + rulesLogger.info('Created branch protection via REST', { res, repositoryOwner: context.payload.repository.owner.login, repositoryName: context.payload.repository.name, diff --git a/src/server/octokit/controller.ts b/src/server/octokit/controller.ts index 2c25fb5..e8448fb 100644 --- a/src/server/octokit/controller.ts +++ b/src/server/octokit/controller.ts @@ -23,7 +23,7 @@ export const checkInstallationHandler = async ({ return { installed: false } } catch (error) { - octokitApiLogger.info('Failed to check installation', { input, error }) + octokitApiLogger.error('Failed to check app installation', { input, error }) return { installed: false } } diff --git a/src/server/repos/controller.ts b/src/server/repos/controller.ts index 660f96d..e898ae8 100644 --- a/src/server/repos/controller.ts +++ b/src/server/repos/controller.ts @@ -23,7 +23,7 @@ export const createMirrorHandler = async ({ const config = await getConfig(input.orgId) - reposApiLogger.debug('Fetched config', config) + reposApiLogger.debug('Fetched config', { config }) const { publicOrg, privateOrg } = config @@ -52,15 +52,15 @@ export const createMirrorHandler = async ({ `Repo ${orgData.data.login}/${input.newRepoName} already exists`, ) } - } catch (e) { + } catch (error) { // We just threw this error, so we know it's safe to rethrow - if ((e as Error).message.includes('already exists')) { - throw e + if ((error as Error).message.includes('already exists')) { + throw error } - if (!(e as Error).message.includes('Not Found')) { - logger.error({ error: e }) - throw e + if (!(error as Error).message.includes('Not Found')) { + reposApiLogger.error('Not found', { error }) + throw error } } @@ -140,16 +140,16 @@ export const createMirrorHandler = async ({ success: true, data: newRepo.data, } - } catch (e) { + } catch (error) { // Clean up the private mirror repo made await privateOctokit.rest.repos.delete({ owner: privateOrg, repo: input.newRepoName, }) - logger.error({ error: e }) + reposApiLogger.error('Error creating mirror', { error }) - throw e + throw error } } catch (error) { reposApiLogger.error('Error creating mirror', { error }) diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 3389301..3de07cb 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ import { Logger } from 'tslog' // If you need logs during tests you can set the env var TEST_LOGGING=true @@ -17,55 +18,60 @@ const getLoggerType = () => { export const logger = new Logger({ type: getLoggerType(), maskValuesRegEx: [ - /"access_token":"[^"]+"/g, + /"access[-._]?token":"[^"]+"/g, + /"api[-._]?key":"[^"]+"/g, + /"client[-._]?secret":"[^"]+"/g, + /"cookie":"[^"]+"/g, + /"password":"[^"]+"/g, + /"refresh[-._]?token":"[^"]+"/g, + /"secret":"[^"]+"/g, + /"token":"[^"]+"/g, /(?<=:\/\/)([^:]+):([^@]+)(?=@)/g, ], overwrite: { transportJSON: (log) => { - let logObjWithMeta = log as { + const logObjWithMeta = log as { + [key: string]: any _meta?: Record + } + + const output: { meta?: Record message?: string + info?: Record data?: Record - [key: string]: any - } - - const meta = logObjWithMeta._meta + } = {} - delete logObjWithMeta._meta + // set meta + output.meta = logObjWithMeta._meta - // If the log is only a string, then set "message" + // set message if it's a string or set it as info if ( Object.prototype.hasOwnProperty.call(logObjWithMeta, '0') && typeof logObjWithMeta['0'] === 'string' ) { - const message = logObjWithMeta['0'] - delete logObjWithMeta['0'] - logObjWithMeta = { - message, - data: logObjWithMeta, - meta, - } + output.message = logObjWithMeta['0'] } else { - logObjWithMeta = { - data: logObjWithMeta, - meta, - } + output.info = logObjWithMeta['0'] } - if (Object.keys(logObjWithMeta.data ?? {}).length === 0) { - delete logObjWithMeta.data + // set data + if (Object.prototype.hasOwnProperty.call(logObjWithMeta, '1')) { + output.data = logObjWithMeta['1'] } - const output = JSON.stringify(logObjWithMeta) - console.log(output) }, }, }) -logger.info('Initialized logger') +logger.getSubLogger({ name: 'default' }).info('Initialized logger') // Redirect next logs to our logger >:( -console.warn = logger.info.bind(logger) -console.error = logger.info.bind(logger) +console.warn = logger + .getSubLogger({ name: 'console' }) + .warn.bind(logger.getSubLogger({ name: 'console' })) +// Currently set to warn because of warning issued by undici showing as error +console.error = logger + .getSubLogger({ name: 'console' }) + .warn.bind(logger.getSubLogger({ name: 'console' }))