When the Remediation Is the Defect

}).catch((err) => {
  console.error(
    JSON.stringify({
      level: "error",
      message: "Feedback email notification failed",
      feedback_id: item.id,
      request_id: req.requestId,
      error: err instanceof Error ? err.message : String(err),
    })
  );
});

That is the tail of a security remediation I committed this spring to the feedback route of a small AWS Lambda API. The audit finding behind it was correct: a try/catch inside the email helper was swallowing Amazon Simple Email Service failures, so a send could fail and no caller would ever know. The remediation propagated the error out of the helper and logged it against a feedback_id. Four days and eleven hours later I replaced this block, because it was the reason feedback notification emails had stopped arriving.

The finding was real, the diff answered it, and for those four days the fix was the worst defect in the repository. Nothing in the test suite and none of the six CloudWatch alarms on the stack was capable of noticing.

Three weeks ago I wrote about a remediation batch on this same API, where a web application firewall was added to the infrastructure template and attached to nothing, and I ended on a rule: a control has two states that a diff renders identically, added and enforcing, so a finding closes on a query against the running system rather than on a commit. That rule works when the control is a resource with a binding you can interrogate. The SES fix is not a resource. It is a code path, and a code path has no binding to query, so closing it needs a different question.

The finding named one thing and the fix changed two

Before the audit, src/utils/email.ts ended the send like this:

} catch (error) {
  console.error(JSON.stringify({
    level: "error",
    message: "Failed to send feedback email",
    error: error instanceof Error ? error.message : "Unknown error",
  }));
}

A caller awaiting that function gets a resolved promise whether the mail went out or not. The audit was right to flag it, and the fix was the obvious one: delete the try/catch and let sesClient.send reject. That part of the fix is still in the code today and I would write it the same way again.

The problem is what else went in. Having made the send capable of failing loudly, I did not want a transient SES error to turn a saved piece of user feedback into a 500 response, so the route stopped awaiting the call and attached the .catch handler above. The commit message records both halves as one line: "SES email errors propagate to caller; feedback route uses logged fire-and-forget pattern with feedback_id correlation." The finding named the first clause. The second clause was volunteered, and it went out under the finding's authority without ever being examined as the change it was.

Nothing runs after the response

Follow one feedback submission through: a client posts to /prod/feedback. API Gateway invokes the function, whose handler is an Express app wrapped by serverless-http. The route validates the email, writes the row, calls sendFeedbackNotification without awaiting it, and calls res.status(201).json(item). serverless-http resolves as soon as Express finishes the response, the handler returns, and the runtime tells Lambda it is done. The SES request is at that moment an open socket owned by a promise nobody is waiting on.

The AWS Lambda documentation on the execution environment lifecycle is explicit about what happens next: "Lambda freezes the execution environment when the runtime and each extension have completed and there are no pending events", and "Background processes or callbacks that were initiated by your Lambda function and did not complete when the function ended resume if Lambda reuses the execution environment." Resume, not fail. The .catch handler I added never ran, because the promise never settled. It was frozen mid-flight.

The security property is lost at exactly one hop, and it is the hop where the handler returns. Everything before it is correct. Whether that particular email ever left depends on whether Lambda happens to thaw that environment again, and on how much of the send was still outstanding when it froze. That is not a delivery guarantee. It is a coin flip with unknown weighting.

The tests could not have failed

Here is the line from tests/routes/feedback.test.ts that made the change look safe:

vi.mock("../../src/utils/email.js", () => ({
  sendFeedbackNotification: vi.fn().mockResolvedValue(undefined),
}));

That mock landed days before either commit, and neither of them touched it. It replaces the one property the defect depends on, which is that the send takes real time and outlives the response, with a promise that is already resolved. Under that double, awaiting and not awaiting are indistinguishable, so the suite passed identically before and after. A green build was not weak evidence here. It was no evidence, and it read as the strongest kind.

The rest of the safety net was equally blind. The route returned 201, so the stack's API errors alarm, which watches the AWS/Lambda Errors metric with a threshold of five in two five-minute periods, had nothing to count. The process.on("unhandledRejection") logger in src/lambda.ts would have caught a bare floating promise, and the .catch I attached is precisely what disqualified it. Every control in that path was working as designed.

Look at what that remediation commit actually contained: 14 files changed, of which exactly one was a test file, and that one edit changed an assertion string from "degraded" to "unhealthy" to match a renamed health status. Thirteen files of change to the shipped artifact, zero new tests. That ratio is normal for remediation batches and would be a review comment on any feature branch.

A different audit, three days earlier, in a different feature

The email freeze is the sharpest case but not the only one in that fortnight, and the other came out of a different audit entirely. Three days before the SES finding was written, a re-audit of the same API filed NEW-7. The t-shirt idea feature had shipped that morning with a moderation gate on the list query: only ideas in approved, in_production or available were visible to non-admin callers. NEW-7 was a low-severity note that the single-item endpoint had no such filter, so any caller who knew an id could read unmoderated content. The remediation added the same filter to getIdeaById and shipped the same day; in isolation it is correct, it is four lines, and I would file the finding myself.

What it also did was close the last path by which a user could see their own submission. New ideas default to submitted, which is not a public status, so from that point a submitted idea was invisible to its author through both endpoints. A week on, a commit widened both queries to include the caller's own rows. Thirty-five minutes later another abandoned that approach, and its message explains why: "users could only see their own submitted ideas, so they could only try to vote on their own ideas (which is blocked by self-vote protection)." The moderation gate and the self-vote guard, each defensible alone, had composed into a voting feature with nothing votable in it.

The resolution was to add submitted to the public list and let admins hide an idea after the fact instead of approving it before. That is a weaker moderation posture than the one NEW-7 was filed against. A low-severity finding was closed correctly and the state seven days later was less restrictive than the state that produced the finding, because the first time anyone exercised the feature end to end was after the gate had been tightened, not before.

The strongest objection

The strongest objection is that none of this is special. Any change can break something, remediations are just changes, and giving them their own ceremony is the kind of process that security teams invent to look busy. On this reading the fire-and-forget email is an ordinary async bug that happened to be committed under an audit heading, and the fix for it is better testing generally, not a remediation-specific rule.

Most of that is right, and the part about better testing is right without qualification. Where it fails is the closure ritual: an ordinary change ends when it works. A remediation ends when a finding is marked resolved, and that mark is made against the finding's description, not against the diff. Nobody re-reads "SES errors are swallowed" and asks whether the route still awaits the send, because the finding never mentioned awaiting. The audit trail is built to confirm that the named thing changed, which is exactly the wrong shape for catching what else changed alongside it.

What makes this one hard to catch is not that it was a remediation. It is that it was a remediation to a code path. The other findings in the same commit were resource changes: an IAM wildcard narrowed in deploy-policy-2, migration TLS validation pinned to the RDS CA bundle, avatar_url constrained to HTTPS by an OpenAPI pattern. Every one of those is answerable by reading the deployed account, and if the answer is wrong the stack usually refuses to create. The SES fix is answerable only by sending a request and watching what arrives, and nothing about the deploy has an opinion on it.

What I would want to be wrong about

So the rule is the half the earlier post left open, and it is narrower than the resource version. For a control whose enforcement is a code path, the closing question is not what the control is bound to. It is which single request would prove the path ran, named before the fix is written and sent against the deployed stage after it is. For the email case that request is one feedback submission and one inbox check, and it would have cost about ninety seconds on the day. The person who closes the finding owns sending it, and where the path cannot be exercised, the finding closes carrying the request nobody could send and a date, not a checkmark.

I would drop this if the ratio stopped holding. The claim resting underneath it is that remediation commits carry systematically less test coverage than feature commits in the same repository, and I have one repository and two batches, which is an anecdote. If the next three security batches I ship land with tests in proportion to the behavior they change and still produce a defect like this one, the problem is not the closure ritual and I am looking at the wrong thing.

The .catch block at the top of this post is gone. A later commit put the await back and moved the try/catch up into the route, so the send now completes before the 201 and a failure is still logged against its feedback_id. That final shape is the original code with one try/catch deleted from src/utils/email.ts and nothing else touched. It was available on day one for the price of leaving one keyword alone.