PUT /user 500s when a customize_access_token hook blanks session_id

[ridwanakf]

Describe the bug

If a project runs a customize_access_token hook that writes session_id, Auth will issue a token that then 500s when you send it back to PUT /user.

MinimumViableTokenSchema requires session_id to be present, but only checks that it's a string. So "" passes validation, and so does "00000000-0000-0000-0000-000000000000". The hook docs describe it the same way, that Auth "will check for these claims after the hook is run and return an error if they are not present", so presence rather than value, and there's nothing there telling you to leave session_id alone.

Those two values happen to be the exact ones that make maybeLoadUserOrSession skip loading the session:

if claims.SessionId != "" && claims.SessionId != uuid.Nil.String() {

That's not an error path, it just leaves the session nil and carries on. Then UserUpdate does this in two places:

if user.HasMFAEnabled() && !session.IsAAL2() {   // user.go:106
...
if !session.IsRecovery() {                       // user.go:172

IsAAL2 (through GetAAL) and IsRecovery both deref the receiver, so both panic on a nil session. recoverer turns that into a 500.

What convinced me this is an oversight rather than an assumption I'm misreading: the same function already handles a nil session twice, at line 154 with session == nil || now.After(session.CreatedAt...) and again at 206 with if session != nil. So the code knows the session can be missing. Lines 106 and 172 just didn't get the same treatment. The 154 guard was added on purpose in #1164 and it treats a missing session as "not recent", so it fails closed.

The two lines need different things to reach them. Line 106 needs a verified MFA factor on the user, otherwise the && short circuits before the deref. Line 172 needs no MFA at all, just GOTRUE_SECURITY_UPDATE_PASSWORD_REQUIRE_CURRENT_PASSWORD and a user who has a password. So 172 is the easier one to trip.

One thing that'll make this confusing if you go looking in production logs: timeoutMiddleware runs the handler in a goroutine, recovers the panic and re-panics it from its own frame. The user.go frames are gone by the time it's logged, and the stack tops out in middleware.go. It ends up looking a lot like the trace in #2233, which is a different bug.

To Reproduce

Test in internal/api against master at fc654b05. No hand-crafted JWT anywhere, the server issues the token itself.

Hook that blanks the claim (the nil UUID behaves the same):

create or replace function probe_blank_session(input jsonb) returns jsonb as $$
declare result jsonb;
begin
  input := jsonb_set(input, '{claims,session_id}', '""'::jsonb, true);
  result := jsonb_build_object('claims', input->'claims');
  return result;
end; $$ language plpgsql;

Point GOTRUE_HOOK_CUSTOM_ACCESS_TOKEN_URI at it, then do a normal POST /token?grant_type=refresh_token. That comes back 200, and the token decodes to a real sub with session_id="".

Send that token to PUT /user with {"password": "..."} and you get:

{"code":500,"error_code":"unexpected_failure","msg":"Internal Server Error"}

plus request panicked / runtime error: invalid memory address or nil pointer dereference in the logs.

I ran both "" and the nil UUID, both panic. Line 106 does the same if you enroll a verified TOTP factor first.

Expected behavior

Fail closed. requirePasskeyManagementAAL already does exactly this for the passkey endpoints, and its doc comment says why: "The check fails closed, treating a missing session as not meeting AAL2."

So 401 insufficient_aal at line 106, and 400 current_password_required at 172.

I want to flag the direction, because the obvious fix is the wrong one. Skipping the check when there's no session also makes the panic go away, and I tried it: PUT /user then returns 200 with the password changed, no AAL2 and no current password. So it has to fail closed, not open.

The other thing worth considering separately is constraining session_id's value in the token schema so a hook can't neuter it in the first place. That felt like a bigger change with its own compat story, so I left it alone.

Screenshots

N/A, server side.

System information

• auth: master at fc654b05e7a02a6a8c375b3a7895bcd563b790ca, tag rc2.195.0-rc.8
• Postgres 15.18 from the repo's docker-compose-dev.yml
• macOS arm64

Additional context

On severity: I don't think this is a vulnerability and I'd rather say so than dress it up. It needs the project to be running a hook that writes session_id, which is an operator's choice, and the 500 lands on whoever holds that token rather than on somebody else. That's why it's here and not on HackerOne. Say the word if you read it differently.

For what it's worth I nearly didn't file it. My first read was that you'd need to hand-mint a JWT with the project secret to get a session-less token, which would have made it pretty academic. Then I found the hook path and Auth issues one itself, 200 and all.

Happy to send a PR, I've got the fix and tests ready. It's two nil checks.