-
Notifications
You must be signed in to change notification settings - Fork 53
accept sendgrid webhook posts and logs them #1256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4a32cf7
7ff6748
d8a1846
421dfdb
0bf51b9
f1e2903
5d6998d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { expectPermission } from "../controllerUtils"; | |
| import { BadRequest } from "@curveball/http-errors"; | ||
| import { IElectionRequest } from "../../IRequest"; | ||
| import { Response, NextFunction } from 'express'; | ||
| import { logSafeHash } from "../../Services/Logging/logSafeHash"; | ||
|
|
||
| const ElectionRollModel = ServiceLocator.electionRollDb(); | ||
|
|
||
|
|
@@ -40,8 +41,7 @@ const revealVoterIdByEmail = async (req: IElectionRequest, res: Response, next: | |
| const actor = req.user?.email || 'unknown'; | ||
|
|
||
| // PROMINENT LOGGING - This action should be highly visible in logs | ||
| Logger.error(req, `🚨 BREAK GLASS ACTION 🚨 ${className}.revealVoterIdByEmail - Election: ${electionId}, Email: ${email}, Actor: ${actor}`); | ||
| console.error(`🚨🚨🚨 BREAK GLASS: Voter ID revealed for ${email} in election ${electionId} by user ${actor} 🚨🚨🚨`); | ||
| Logger.error(req, `BREAK GLASS ACTION - ${className}.revealVoterIdByEmail - Election: ${electionId}, Email: ${logSafeHash(email)}, Actor: ${logSafeHash(actor)}`); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems good - do we still have some way to understand who this was later?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a voter id is revealed, that fact gets added to their history in the rolls. The people who can read that history are system_admin, owner, admin, auditor, credentialer. |
||
|
|
||
| const electionRoll = await ElectionRollModel.getRollsByElectionID(electionId, req); | ||
| if (!electionRoll) { | ||
|
|
@@ -53,7 +53,7 @@ const revealVoterIdByEmail = async (req: IElectionRequest, res: Response, next: | |
| // Find the roll entry by email | ||
| const rollEntry = electionRoll.find(roll => roll.email?.toLowerCase() === email.toLowerCase()); | ||
| if (!rollEntry) { | ||
| const msg = `No voter found with email ${email}`; | ||
| const msg = `No voter found with email ${logSafeHash(email)}`; | ||
| Logger.info(req, msg); | ||
| throw new BadRequest(msg); | ||
| } | ||
|
|
@@ -68,7 +68,7 @@ const revealVoterIdByEmail = async (req: IElectionRequest, res: Response, next: | |
|
|
||
| await ElectionRollModel.update(rollEntry, req, '🚨 VOTER_ID_REVEALED'); | ||
|
|
||
| Logger.error(req, `🚨 BREAK GLASS COMPLETED 🚨 Voter ID ${rollEntry.voter_id} revealed for ${email}`); | ||
| Logger.error(req, `BREAK GLASS COMPLETED - ${className}.revealVoterIdByEmail - Election: ${electionId}, VoterID: ${logSafeHash(rollEntry.voter_id)}, Email: ${logSafeHash(email)}`); | ||
|
|
||
| res.json({ | ||
| voter_id: rollEntry.voter_id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { Response } from 'express'; | ||
| import { IRequest } from '../IRequest'; | ||
| import Logger from '../Services/Logging/Logger'; | ||
| import { logSafeHash } from '../Services/Logging/logSafeHash'; | ||
|
|
||
| interface SendGridEvent { | ||
| email?: string; | ||
| event?: string; | ||
| sg_message_id?: string; | ||
| timestamp?: number; | ||
| [key: string]: unknown; | ||
| } | ||
|
|
||
| export const sendGridWebhookController = (req: IRequest, res: Response) => { | ||
| const rawBody = req.body as Buffer; | ||
| const signature = String(req.headers['x-twilio-email-event-webhook-signature'] ?? 'missing'); | ||
| const timestamp = String(req.headers['x-twilio-email-event-webhook-timestamp'] ?? 'missing'); | ||
| Logger.info(req, `SendGridWebhook signature=${signature} timestamp=${timestamp}`); | ||
|
|
||
| try { | ||
| const events: SendGridEvent[] = JSON.parse(rawBody.toString('utf8')); | ||
| const summary = events.map(e => `event=${e.event} email=${logSafeHash(e.email)}`).join('; '); | ||
| Logger.info(req, `SendGridWebhook events: ${summary}`); | ||
| } catch { | ||
| Logger.warn(req, `SendGridWebhook: could not parse body`); | ||
| } | ||
|
|
||
| // TODO: verify signature using SENDGRID_WEBHOOK_VERIFICATION_KEY env var | ||
| // verification checks: ECDSA signature over (timestamp + raw body) matches public key | ||
| res.status(200).send('OK'); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ export class LoggerImpl { | |
| } | ||
|
|
||
| debug(context?:ILoggingContext, message?: any, ...optionalParams: any[]):void{ | ||
| if (process.env.NODE_ENV === 'production') return; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not have a way to just configure the logging level on the environment?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haven't heard from Arend, but I don't think so. I can't find anything in the argocd or the helm chart. So... I guess this works?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😭 |
||
| this.log(context, "", message, ...optionalParams); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { createHmac, randomBytes } from "crypto"; | ||
|
|
||
| // Per-process random secret. Not stable across pods or restarts, | ||
| // but impossible to recover from outside the running container. | ||
| const secret = randomBytes(32); | ||
|
|
||
| function getWeeklyKey(): Buffer { | ||
| const msPerWeek = 7 * 24 * 60 * 60 * 1000; | ||
| const weekIndex = Math.floor(Date.now() / msPerWeek); | ||
| return createHmac("sha256", secret).update(String(weekIndex)).digest(); | ||
| } | ||
|
|
||
| /** | ||
| * HMAC PII for log output. Produces a short, consistent hash that allows | ||
| * correlating repeated events from the same identity within the same | ||
| * process and ~7-day window, without exposing the raw value. | ||
| */ | ||
| export function logSafeHash(value: string | undefined | null): string { | ||
| if (!value) return "[empty]"; | ||
| const key = getWeeklyKey(); | ||
| const hash = createHmac("sha256", key).update(value).digest("hex").slice(0, 12); | ||
| return `[h:${hash}]`; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, this seems odd to me - the client is providing either an id or an email as input. It seems unusual to redact that in the error response and not inform the client where the problem is. Am I missing it? Is there some vulnerability here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression is that (1) this kind of exception gets logged and (2) user is not going to be well-poised to make use of the backend error anyway. In theory the frontend should not let you submit duplicates, so its really only a race condition I guess (or something weird and nefarious)?
We could just status(400).json reply to them I guess, but I'm not sure if there's a Clever Way to use BadRequest that (1) gives the info but (2) doesn't let it get logged.
So that's my ill-formed thinking around it.