Fix panic when rejecting a headless request#63189
Conversation
| // MFAResponse is required only when accepting a request. | ||
| var mfaResp *proto.MFAAuthenticateResponse | ||
| if req.MFAResponse != nil { |
There was a problem hiding this comment.
I think I'd rather see an if r == nil { return nil, nil } in (*lib/client.MFAChallengeResponse).GetOptionalMFAResponseProtoReq rather than nil checks here and there; there's a lot of callers to GetOptionalMFAResponseProtoReq and I'm not sure how many of them are susceptible to a client not passing the mfa response field (or passing an explicit null).
There was a problem hiding this comment.
Yeah, I think that makes sense. I was thinking of this too, but with widely used methods like this one I naturally start to look into introducing the least amount of possible behavior changes, so I ended up doing this in the handler.
There was a problem hiding this comment.
@Joerger Could you take a look again at the implementation after this change?
There was a problem hiding this comment.
The new check solves a very similar panic in (*Handler).deleteMFADeviceHandle, FWIW.
|
@ravicious See the table below for backport results.
|
* Fix panic * Add tests * Use `t.Context` instead of `context.Background` * Remove need for `headlessID` as separate field * Simplify err asserts when `expectedStatus` is 200 * Update copyright year * Close `userClient` * Make `getMFAResponse` a helper * Remove unnecessary empty lines * Move nil check to `MFAChallengeResponse`
5 participants
Closes #58864.
changelog: Fixed a server error when rejecting a headless authentication request in the Web UI
This might have been introduced by #53741 but I haven't checked because I wasn't able to build the binary due to Rust issues.
The cause of the issue appears to be that the client uses the same endpoint when accepting or rejecting a headless request. When rejecting, the MFA response isn't required and the client doesn't send it, but the handler assumed it's always present.
The fix was easy, but I noticed there are no tests that could've catched this regression. I used Claude Code to generate the bulk of them, asking it to look at other tests in
lib/web(primarilylib/web/apiserver_test.go). Of course I had to clean them up a fair amount, but it looks like the general pattern does resemble the patterns that can be seen inTestIntegrationsCRUDRolesAnywherefor example (setting up the server and so on).