Page MenuHomePhabricator

[keyserver] Improve logging in checkInputValidator
ClosedPublic

Authored by ashoat on Mon, Jun 10, 9:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 27, 5:09 PM
Unknown Object (File)
Wed, Jun 26, 11:43 AM
Unknown Object (File)
Mon, Jun 24, 12:23 PM
Unknown Object (File)
Sat, Jun 22, 4:45 AM
Unknown Object (File)
Wed, Jun 19, 10:08 PM
Unknown Object (File)
Wed, Jun 19, 9:29 PM
Unknown Object (File)
Wed, Jun 19, 4:20 AM
Unknown Object (File)
Tue, Jun 18, 8:49 PM
Subscribers
None

Details

Summary

This diff does two things:

  1. Adds logging in checkInputValidator so we know what input validator is being failed.
  2. Makes sure that we don't throw a different error if sanitizeInput fails due to input validation. Instead, we'll just not include the sanitizedInput in the error.

(On the second point, I'm not entirely sure if it's ever possible for sanitizeInput to succeed input validation if it fails earlier...)

Depends on D12386

Test Plan

Prior to this, I was seeing an input validation failure when I tested an old client initiating a session recovery with refetchUserDataAfterAcknowledgment removed from the code.

The input validation failure was triggering a second input validation failure when trying to sanitize inputs, which was resulting in a weird error being returned to the client.

Following this change, I was seeing helpful logs of the keyserver side (indicating that the input validation failure was with keyserver_auth), and the keyserver started returning the expected invalid_parameters ServerError.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/utils/validation-utils.js
100 ↗(On Diff #41195)

I'm a little confused why we expect sanitizeInput to work in this case, given that input validation failed. sanitizeInput calls convertObject, which is fairly complicated to understand... but it seems like it's just recursively applying the same validation. Curious if reviewers have any perspective here

tomek added inline comments.
keyserver/src/utils/validation-utils.js
100 ↗(On Diff #41195)

Agree, it looks quite surprising. It seems like any time the validation fails, sanitization will replace the input with null.

This revision is now accepted and ready to land.Tue, Jun 11, 4:18 AM
kamil added inline comments.
keyserver/src/utils/validation-utils.js
100 ↗(On Diff #41195)

spent some time thinking about this and this looks really confusing - I think we could just remove this code as if I understand it correctly, the sanitizedInput will always be set to null

keyserver/src/utils/validation-utils.js
100 ↗(On Diff #41195)

I investigated more closely and found that before D7488, sanitizeInput was able to handle cases where validation fails.

I found a way to get convertObject to work even when validation fails. Will put up a separate diff for that shortly.

keyserver/src/utils/validation-utils.js
100 ↗(On Diff #41195)