accept sendgrid webhook posts and logs them#1256
Conversation
✅ Deploy Preview for bettervoting ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| 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}`); | ||
| Logger.info(req, `SendGridWebhook body: ${rawBody.toString('utf8')}`); |
There was a problem hiding this comment.
Does this contain the email address or other sensitive info?
There was a problem hiding this comment.
Almost certainly! I fixed it.
JonBlauvelt
left a comment
JonBlauvelt
left a comment
There was a problem hiding this comment.
Overall looks like a great improvement to clean all of this up! Just had a couple questions/concerns/clarifications
| }) | ||
| if (duplicateRolls.length > 0) { | ||
| throw new BadRequest(`Some submitted voters already exist: ${duplicateRolls.map( (roll: ElectionRoll) => `${roll.voter_id ? roll.voter_id : ''} ${roll.email ? roll.email : ''}`).join(',')}`) | ||
| throw new BadRequest(`Some submitted voters already exist (${duplicateRolls.length} duplicates found)`) |
There was a problem hiding this comment.
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.
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.
| // 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)}`); |
There was a problem hiding this comment.
this seems good - do we still have some way to understand who this was later?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| debug(context?:ILoggingContext, message?: any, ...optionalParams: any[]):void{ | ||
| if (process.env.NODE_ENV === 'production') return; |
There was a problem hiding this comment.
Do we not have a way to just configure the logging level on the environment?
There was a problem hiding this comment.
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?
2 participants
This is the first step in addressing #713.
Next steps. Once it is merged and live, I will make sendgrid actually send to the endpoint, and look at the logs. If they look sensible, I will then submit a second PR that (1) checks the sendgrid signature and (2) if it is right uses the sendgrid information to update the voter roll history appropriately with "dropped" or "bounced" events.
This PR also goes through a Large Number of Cases where we were logging PII and instead logs pseudoanonymized versions. That is, instead of email, you'll see some [abc123], and if two different logs are about the same email and the logs are happening on the same pod that hasn't been restarted then both logs will report [abc123]. This strikes I think an acceptable balance for now between
This PR also supresses debug logs in production.