DEV Community

Cover image for The Smallest Fix With The Biggest Impact [Skips VS Technology Edition]
Joyce Foster
Joyce Foster

Posted on

The Smallest Fix With The Biggest Impact [Skips VS Technology Edition]

Summer Bug Smash: Smash Stories 🐛🛹

This is a submission for DEV's Summer Bug Smash: Smash Stories powered by Sentry.

Remember that one Regular Show episode where Skips tried to destroy the park's computer because it caught the Error 220 bug? He took one look at it, picked up a sledgehammer and said the line we’ve all felt as devs: “ There’s something evil in that computer. We gotta smash it”. In the cartoon, they literally smash the computer and this works to fix the bug. In real life? We don’t get sledgehammers. We get Github PRs.

Last week, I almost felt like Skips. I found a one-line bug in an open source repo that could’ve broken Instagram webhook security. No hammer, no explosion, just one misindented ‘if’ statement and a missing test. This is the story of how the smallest fix had the biggest impact.

-The Challenge
So what was my Error 220?
While contributing to the corsair open-source repo, I found a security breach in the Instagram webhook handler. Something about the verification flow felt off, so I started tracing it line by line. The code called timingSafeEqual but the result was indecisive. I took an extensive look at it and that's when I saw it- The if statement meant to guard the check was there, but timingSafeEqual was indented wrong. It was meant to return the result of timingSafeEqual to accept or reject the request, but it fell through instead. Although it was running, its return value wasn’t being used to control the flow.
This bug was tiny-one mis-indented line- but it had a great impact. In JS, it is not considered an error and so it’s easy to miss. Webhook security relies on a signature check to prove a request. If timingSafeEqual isn’t actually enforcing it, an attacker could forge a webhook and it would be accepted. The entire protection could fall apart over one tab.
View PR #759

-The Fix
In fixing it, I opened PR#759 to correct the indentation so crypto.timingSafeEqual would be inside the if block and its boolean result would decide whether to return true or false. Prior to the fix, the function was calling timingSafeEqual but then ignoring what it returned and continuing to execute anyway. The update makes it return instantly based on that result. If a signature doesn’t match, the request is rejected. In the same vein, it is accepted where it matches.

This change, small but significant, reconnects the verification to the return value. Consequently, the library actually enforces “only accepts requests signed by Instagram” instead of just running the check. This two-line difference restores the whole security guarantee and PR #759 has since been merged into the corsair repo, closing the gap for good.

-The Technical Breakdown
Here, Instagram signs every webhook with a secret key, and sends that signature in the request header. It is then the corsair's job to take the incoming payload, recreate the signature on our side, and compare it to the one Instagram sent. At this stage, crypto.timingSafeEqual comes in. It is a Node.js function made particularly for comparing secrets because it requires the same amount of time, whether they match or not. This prevents timing attacks.

In the process of generating the comparison, the bug broke the last step, and due to the indentation, the results never actually returned. So, the function would always continue, whether the signatures matched or not. After PR#759, I made the return value matter. If timingSafeEqual says false, it rejects. If it says true, it accepts. Now the check actually gates the request.

-The Lesson
What I am proud of: Recalling the Regular Show episode where the computer threw Error 220 and Skips immediately wanted to smash it. With this bug, it would’ve been just as easy to see the bad indentation, ignore it and let the webhook pass through anyway. I am proud that, like Skips, I decided to “smash” the bug by finding and fixing the indentation. This small choice positively restored the security.

What I learnt: I learnt that running a security check isn’t the same as enforcing it. My code was checking the signature but not using the result. Technology is only effective when you act on what it tells you. crypto.timingSafeEqual only mattered once I made its return value decide whether to accept or reject.

Challenges I came across: The challenge was that it failed silently. Just like Error 220 didn’t crash the computer in Regular Show, the bug didn’t show any errors. The request still went through whether the signature was valid or not. I had to slow down, read the flow and make one precise fix. This made me pay attention to the tiniest of details.

-Before and After Comparison
Before: The webhook validation ran timingSafeEqual but ignored the result, so all requests got through whether the signature matched or not.

After: PR#759 makes the return value gate the request, so only valid Instagram webhooks are processed.

Error 220 taught the Park crew that some problems just need to be smashed. PR #759 taught me that some problems just need to be seen. Not every fix has to be loud to matter. Sometimes the biggest impact comes from the smallest, most precise correction, one misindented line, caught and fixed, restoring an entire security guarantee. That's the smash bug story I'll remember.

Top comments (0)