handle AADSTS50173 as explicit revocation signal for azure refresh tokens#4842
handle AADSTS50173 as explicit revocation signal for azure refresh tokens#4842jordanTunstill wants to merge 1 commit intomainfrom
Conversation
| } else if errors.Is(verificationErr, ErrTokenExpired) { | ||
| continue TokenLoop | ||
| } else if errors.Is(verificationErr, ErrTokenRevoked) { | ||
| // Token was explicitly revoked; emit a clean unverified result. | ||
| r = createResult(token, clientId, tenantId, false, nil, nil) | ||
| break ClientLoop |
There was a problem hiding this comment.
This change here is correct, but I see asymmetry with ErrTokenExpired:
ErrTokenExpired: continue TokenLoop -> no result emitted
ErrTokenRevoked: break ClientLoop -> unverified result emitted
Is this intentional?
There was a problem hiding this comment.
@shahzadhaider1 I was operating under the idea that the rest of this detector was working as intended, so I wanted to implement my change without affecting the rest of the functionality. LMK your thoughts, happy to revisit it if the other aspects of the detector should also be changed.
There was a problem hiding this comment.
Yeah, totally understand wanting to keep the scope tight. I was just flagging it since we're already updating this detector and the asymmetry stood out during review.
The concern is about multi-scan behavior: if a token was previously reported as verified and then expires before the next scan, ErrTokenExpired -> continue TokenLoop means no result is emitted. Downstream systems receive silence, and the token stays open with stale verified state indefinitely.
This PR actually handles revocation correctly from that perspective; re-emitting as unverified lets consumers update their state, which makes the expired case look worse by comparison. Would it be worth giving ErrTokenExpired the same treatment?
Happy to discuss further or address it in a separate PR if you'd prefer to keep this one focused.
There was a problem hiding this comment.
@shahzadhaider1 sounds good, while I wait for a product-eng review, I'll look into adding this.
2 participants
AADSTS50173 error messages were falling through to the default error branch, surfacing
revoked tokens as verification errors (unknown) rather than clean
unverified results.
Added ErrTokenRevoked sentinel and handled it
deterministically in processMatches.
The error message is: "unexpected error ‘invalid_grant': AADSTS50173: The provided grant has expired due to it being revoked, a fresh auth token is needed"
Description:
Explain the purpose of the PR.
Checklist:
make test-community)?make lintthis requires golangci-lint)?Note
Low Risk
Low risk, localized change to Azure refresh-token verification error handling that only affects how revoked tokens are classified (unverified vs unexpected verification error).
Overview
Azure Entra refresh-token verification now recognizes
AADSTS50173as an explicit revocation signal.This adds a new sentinel
ErrTokenRevoked, maps theAADSTS50173error message to it inverifyMatch, and updatesprocessMatchesto return a clean unverified result (no verification error) when encountered instead of surfacing it as an unexpected error.Written by Cursor Bugbot for commit 8ac7bbe. This will update automatically on new commits. Configure here.