fix: guard against nil session in UserUpdate - #2666
Open
ridwanakf wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
Fixes #2665.
UserUpdatederefssessionat lines 106 and 172 without checking it, so a token whosesession_idclaim is blank or the nil UUID panics the handler and you get a 500 back. The issue has the full trail of how Auth ends up issuing a token like that.What is the new behavior?
Two
session == nil ||guards, so a missing session reads as "not good enough" rather than blowing up. Same thingrequirePasskeyManagementAALalready does for the passkey endpoints. You get 401insufficient_aalor 400current_password_requiredinstead of the 500.One thing I want to call out, since the lazier fix looks identical from the outside: these have to fail closed, not open. If you skip the check when there's no session the panic also goes away, but then
PUT /usercomes back 200 with the password changed, no AAL2 and no current password. I tried it to be sure. That's why the tests assert the exact 401 and 400 instead of just "not a 500".Tests are in
user_test.go: a hook-issued token with a blankedsession_id, the AAL2 line with a TOTP factor enrolled, and the current-password line. They panic on master and pass here.Additional context
I left the token schema alone. Constraining what
session_idcan be set to is probably worth doing, but it'd change things for anyone whose hook already writes that claim, so it felt like its own PR rather than something to sneak in here.Rest is green: vet, staticcheck, both gosec runs, and
go test ./... -p 1 -raceover all 36 packages with that one skipped.