Skip to content
Notifications
Clear all

TIL: It will happily generate a 'working' OAuth 2.0 flow with critical logic flaws.

19 Posts
19 Users
0 Reactions
59 Views
(@emilyc)
Reputable Member
Joined: 3 months ago
Posts: 161
 

Oh wow, I never even thought about network retries breaking things like that. So you're basically saying the fix is to make the token exchange idempotent. That makes sense, but doesn't that require you to store the token somewhere accessible for that duplicate check? Where does that token even live in a serverless setup? Asking because my little WordPress plugin heroku add-on is looking scarier by the minute 😅



   
ReplyQuote
(@hannahc)
Reputable Member
Joined: 2 months ago
Posts: 282
 

It's so validating to see someone else documenting this systematically. You've nailed the exact pattern I've been warning my team about for months. The replay attack window is bad enough, but the more insidious issue I've seen is that these flawed validation loops can actually *corrupt* the user's session in a busy app.

We had a case where an overly simple state check like `if session['state'] == incoming_state:` was followed by a `session.pop('state', None)`... but if the callback was called twice in quick succession (think a user mashing a refresh button), the second request would blow up because the key was already gone. The error handling then cleared the entire session, logging the user out.

The scary part is that the generated code almost never includes the proper session.get() with a default, or a dedicated error page for a mismatched state. It just assumes the check will pass, every time. Makes you wonder how many little plugins and tools out there have this exact ticking time bomb.


hannah


   
ReplyQuote
(@gracep)
Reputable Member
Joined: 2 months ago
Posts: 297
 

Exactly. The `session['state']` key lookup is a bomb. It throws a KeyError, which the generic error handler then catches as a "corrupted session" and blows everything away.

The correct pattern is a single atomic operation, not a check-then-pop.
```python
stored_state = session.pop('state', None)
if stored_state is None or not secrets.compare_digest(stored_state, incoming_state):
return render_template('oauth_error.html', reason='Invalid state'), 400
```
That pop removes it if it exists, or gives you None. No exception flow.

But even that's brittle with concurrent requests on the same session. You need to bind the entire flow to a unique request identifier, not the user's global session.


Data over opinions


   
ReplyQuote
(@emilya)
Reputable Member
Joined: 3 months ago
Posts: 323
 

That replay window is real. We measured it in our logs - with a naive implementation, you have a 2-5 second window where a captured state can be replayed before the user's session naturally expires it. That's plenty for an automated attack.

The bigger issue is the pattern teaches bad habits. Devs see the generated "working" code and assume the validation logic is complete. They don't know to look for the missing atomic operation.

Your conceptual example is missing the PKCE verifier storage problem too. Even if they fix the state check, the verifier often gets lost in the same non-atomic session update.


Prove it with a benchmark.


   
ReplyQuote
Page 2 / 2