From f9f705f4a736be8d50727970e216830780142d27 Mon Sep 17 00:00:00 2001 From: Niek Palm Date: Tue, 9 Nov 2021 08:16:21 +0100 Subject: [PATCH] fix: update return codes, no error code for job that are ignored (#1381) --- modules/webhook/lambdas/webhook/src/lambda.ts | 11 +++-- modules/webhook/lambdas/webhook/src/local.ts | 2 +- .../webhook/src/webhook/handler.test.ts | 38 ++++++++-------- .../lambdas/webhook/src/webhook/handler.ts | 43 ++++++++++++------- 4 files changed, 55 insertions(+), 39 deletions(-) diff --git a/modules/webhook/lambdas/webhook/src/lambda.ts b/modules/webhook/lambdas/webhook/src/lambda.ts index 94e10d8319..232f74000e 100644 --- a/modules/webhook/lambdas/webhook/src/lambda.ts +++ b/modules/webhook/lambdas/webhook/src/lambda.ts @@ -2,14 +2,17 @@ import { handle } from './webhook/handler'; import { APIGatewayEvent, Context, Callback } from 'aws-lambda'; import { logger } from './webhook/logger'; +export interface Response { + statusCode: number; + body?: string; +} + export const githubWebhook = async (event: APIGatewayEvent, context: Context, callback: Callback): Promise => { logger.setSettings({ requestId: context.awsRequestId }); logger.debug(JSON.stringify(event)); try { - const statusCode = await handle(event.headers, event.body as string); - callback(null, { - statusCode: statusCode, - }); + const response = await handle(event.headers, event.body as string); + callback(null, response); } catch (e) { callback(e as Error); } diff --git a/modules/webhook/lambdas/webhook/src/local.ts b/modules/webhook/lambdas/webhook/src/local.ts index 54de30f3b4..aa2839e43b 100644 --- a/modules/webhook/lambdas/webhook/src/local.ts +++ b/modules/webhook/lambdas/webhook/src/local.ts @@ -8,7 +8,7 @@ app.use(bodyParser.json()); app.post('/event_handler', (req, res) => { handle(req.headers, JSON.stringify(req.body)) - .then((c) => res.status(c).end()) + .then((c) => res.status(c.statusCode).end()) .catch((e) => { console.log(e); res.status(404); diff --git a/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts b/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts index 237a2da875..72e84219ec 100644 --- a/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts +++ b/modules/webhook/lambdas/webhook/src/webhook/handler.test.ts @@ -38,12 +38,12 @@ describe('handler', () => { it('returns 500 if no signature available', async () => { const resp = await handle({}, ''); - expect(resp).toBe(500); + expect(resp.statusCode).toBe(500); }); it('returns 401 if signature is invalid', async () => { const resp = await handle({ 'X-Hub-Signature': 'bbb' }, 'aaaa'); - expect(resp).toBe(401); + expect(resp.statusCode).toBe(401); }); describe('Test for workflowjob event: ', () => { @@ -56,14 +56,14 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); it('does not handle other events', async () => { const event = JSON.stringify(workflowjob_event); const resp = await handle({ 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'push' }, event); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(202); expect(sendActionRequest).not.toBeCalled(); }); @@ -73,7 +73,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).not.toBeCalled(); }); @@ -83,7 +83,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).not.toBeCalled(); }); @@ -94,7 +94,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(403); + expect(resp.statusCode).toBe(403); expect(sendActionRequest).not.toBeCalled(); }); @@ -105,7 +105,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -122,7 +122,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -139,7 +139,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -156,7 +156,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -173,7 +173,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -191,7 +191,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); it('Check not allowed runner label is declined', async () => { @@ -207,7 +207,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'workflow_job' }, event, ); - expect(resp).toBe(403); + expect(resp.statusCode).toBe(202); expect(sendActionRequest).not.toBeCalled(); }); }); @@ -219,7 +219,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); @@ -229,7 +229,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).not.toBeCalled(); }); @@ -239,7 +239,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).not.toBeCalled(); }); @@ -250,7 +250,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' }, event, ); - expect(resp).toBe(403); + expect(resp.statusCode).toBe(403); expect(sendActionRequest).not.toBeCalled(); }); @@ -261,7 +261,7 @@ describe('handler', () => { { 'X-Hub-Signature': await webhooks.sign(event), 'X-GitHub-Event': 'check_run' }, event, ); - expect(resp).toBe(200); + expect(resp.statusCode).toBe(201); expect(sendActionRequest).toBeCalled(); }); }); diff --git a/modules/webhook/lambdas/webhook/src/webhook/handler.ts b/modules/webhook/lambdas/webhook/src/webhook/handler.ts index f70aaca989..8d6c793d60 100644 --- a/modules/webhook/lambdas/webhook/src/webhook/handler.ts +++ b/modules/webhook/lambdas/webhook/src/webhook/handler.ts @@ -4,10 +4,11 @@ import { sendActionRequest } from '../sqs'; import { CheckRunEvent, WorkflowJobEvent } from '@octokit/webhooks-types'; import { getParameterValue } from '../ssm'; import { logger as rootLogger } from './logger'; +import { Response } from '../lambda'; const logger = rootLogger.getChildLogger(); -export async function handle(headers: IncomingHttpHeaders, body: string): Promise { +export async function handle(headers: IncomingHttpHeaders, body: string): Promise { // ensure header keys lower case since github headers can contain capitals. for (const key in headers) { headers[key.toLowerCase()] = headers[key]; @@ -15,27 +16,36 @@ export async function handle(headers: IncomingHttpHeaders, body: string): Promis const githubEvent = headers['x-github-event'] as string; - let status = await verifySignature(githubEvent, headers['x-hub-signature'] as string, body); - if (status != 200) { - return status; + let response: Response = { + statusCode: await verifySignature(githubEvent, headers['x-hub-signature'] as string, body), + }; + + if (response.statusCode != 200) { + return response; } const payload = JSON.parse(body); logger.info(`Received Github event ${githubEvent} from ${payload.repository.full_name}`); if (isRepoNotAllowed(payload.repository.full_name)) { - console.error(`Received event from unauthorized repository ${payload.repository.full_name}`); - return 403; + console.warn(`Received event from unauthorized repository ${payload.repository.full_name}`); + return { + statusCode: 403, + }; } if (githubEvent == 'workflow_job') { - status = await handleWorkflowJob(payload as WorkflowJobEvent, githubEvent); + response = await handleWorkflowJob(payload as WorkflowJobEvent, githubEvent); } else if (githubEvent == 'check_run') { - status = await handleCheckRun(payload as CheckRunEvent, githubEvent); + response = await handleCheckRun(payload as CheckRunEvent, githubEvent); } else { logger.warn(`Ignoring unsupported event ${githubEvent}`); + response = { + statusCode: 202, + body: `Ignoring unsupported event ${githubEvent}`, + }; } - return status; + return response; } async function verifySignature(githubEvent: string, signature: string, body: string): Promise { @@ -56,12 +66,15 @@ async function verifySignature(githubEvent: string, signature: string, body: str return 200; } -async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): Promise { +async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): Promise { const disableCheckWorkflowJobLabelsEnv = process.env.DISABLE_CHECK_WORKFLOW_JOB_LABELS || 'false'; const disableCheckWorkflowJobLabels = JSON.parse(disableCheckWorkflowJobLabelsEnv) as boolean; if (!disableCheckWorkflowJobLabels && !canRunJob(body)) { - logger.error(`Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`); - return 403; + logger.warn(`Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`); + return { + statusCode: 202, + body: `Received event contains runner labels '${body.workflow_job.labels}' that are not accepted.`, + }; } let installationId = body.installation?.id; @@ -78,10 +91,10 @@ async function handleWorkflowJob(body: WorkflowJobEvent, githubEvent: string): P }); } console.info(`Successfully queued job for ${body.repository.full_name}`); - return 200; + return { statusCode: 201 }; } -async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise { +async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise { let installationId = body.installation?.id; if (installationId == null) { installationId = 0; @@ -96,7 +109,7 @@ async function handleCheckRun(body: CheckRunEvent, githubEvent: string): Promise }); } console.info(`Successfully queued job for ${body.repository.full_name}`); - return 200; + return { statusCode: 201 }; } function isRepoNotAllowed(repo_full_name: string): boolean {