Page MenuHomePhabricator

[keyserver] Make sanitizeInput work even if validation fails
ClosedPublic

Authored by ashoat on Jun 11 2024, 6:45 AM.
Tags
None
Referenced Files
F3536323: D12391.id41207.diff
Wed, Dec 25, 6:22 PM
F3531763: D12391.diff
Wed, Dec 25, 8:03 AM
F3529688: D12391.id41208.diff
Tue, Dec 24, 8:42 PM
F3529687: D12391.id41207.diff
Tue, Dec 24, 8:42 PM
F3529686: D12391.id41206.diff
Tue, Dec 24, 8:42 PM
F3529667: D12391.id.diff
Tue, Dec 24, 8:42 PM
F3529659: D12391.diff
Tue, Dec 24, 8:41 PM
Unknown Object (File)
Sat, Dec 7, 11:15 PM
Subscribers
None

Details

Summary

We actually only use sanitizeInput when validation fails, so it's important that it works in this case.

See context here.

Test Plan

I added a test case to validation-utils.test.js that was failing before the changes in this diff

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I think this solution has an issue: some sensitive data could be placed in a prop that fails the validation and we probably won't be able to sanitize it properly. I don't think this problem can be solved in general, but maybe we should traverse the whole tree after the conversion and redact by hand all the fields that should usually be sanitized e.g. password.

This revision is now accepted and ready to land.Jun 11 2024, 8:42 AM

I think this solution has an issue: some sensitive data could be placed in a prop that fails the validation and we probably won't be able to sanitize it properly.

Hmm... perhaps I'm not understanding you correctly. I added a couple test cases with nested validators, and these did reveal an issue: I wasn't propagating options during recursion. But a second traversal (as you suggested) didn't appear to be necessary.

Perhaps you are suggesting that we should redact properties based on their keys? That's probably not a bad idea, but I'd rather keep it out of the scope here for now.

The way it's written now, if an unrelated part of the input fails validation, passwords in an expected position should still be redacted. It's only if the password itself is in an unexpected position that we would fail to redact it.

This revision was landed with ongoing or failed builds.Jun 11 2024, 9:25 AM
This revision was automatically updated to reflect the committed changes.

Hmm... perhaps I'm not understanding you correctly. I added a couple test cases with nested validators, and these did reveal an issue: I wasn't propagating options during recursion. But a second traversal (as you suggested) didn't appear to be necessary.

Perhaps you are suggesting that we should redact properties based on their keys? That's probably not a bad idea, but I'd rather keep it out of the scope here for now.

Yeah, that's my suggestion. And agree, we don't have to include this in the scope right now.

The way it's written now, if an unrelated part of the input fails validation, passwords in an expected position should still be redacted. It's only if the password itself is in an unexpected position that we would fail to redact it.

I was thinking also about cases like this:

const validator = tShape<{
  +nested1: { +password: string },
}>({
  nested1: tShape<{ +password: string }>({
    password: tPassword,
  }),
});
const object = { nested2: { password: 'password' } };

Where we won't redact the password. So basically, any misplacement between the sensitive prop and the root would block us from sanitizing it.