Page MenuHomePhabricator

[lib] Add runtime check to `minimallyEncodeThreadCurrentUserInfo` ensure argument isn't already minimally encoded
ClosedPublic

Authored by atul on Dec 12 2023, 2:58 PM.
Tags
None
Referenced Files
F3157155: D10325.diff
Tue, Nov 5, 7:30 PM
Unknown Object (File)
Tue, Oct 15, 9:19 PM
Unknown Object (File)
Tue, Oct 15, 9:19 PM
Unknown Object (File)
Tue, Oct 15, 9:19 PM
Unknown Object (File)
Tue, Oct 15, 9:19 PM
Unknown Object (File)
Tue, Oct 15, 9:19 PM
Unknown Object (File)
Sep 9 2024, 9:13 PM
Unknown Object (File)
Sep 9 2024, 9:13 PM
Subscribers
None

Details

Summary

Ran into issue described here: https://phab.comm.dev/D10314#298619 where I was accidentally passing MinimallyEncodedThreadCurrentUserInfo to minimallyEncodeThreadCurrentUserInfo. Obviously this shouldn't have happened and the type system would have prevented it, but because I didn't validate and ascertain type of ClientDBThreadInfo (which is effectively any typed since it's being JSON.parsed from stringified JSON stored in SQLite) before passing to minimallyEncodeThreadCurrentUserInfo, it "re-minimally encoded it" silently and replaced permissions: "3" with permissions: "0" which led to eg ThreadList showing no entries and also no errors.

Again, updating convertClientDBThreadInfoToRawThreadInfo to properly validate all data that comes from the clientDB should prevent this invariant from being necessary, but I don't think it hurts to add runtime checks for now so any similar issues become immediately clear. Will also add these checks to all the other minimallyEncode* functions, though will pass on adding unit tests for now. Will also make sure to add tests to convertClientDBThreadInfoToRawThreadInfo to make sure it correctly handles ClientDBThreadInfo created from LegacyRawThreadInfos, MinimallyEncodedRawThreadInfos, and generally malformed ones.

Also added unit test that would've previously failed silently leading to incorrect permissions being stored in Redux to ensure that it now throws.


Depends on D10314

Test Plan

Unit tests, issue observed in D10314 is no longer occurring.

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D10325 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Dec 12 2023, 3:08 PM
This revision is now accepted and ready to land.Dec 12 2023, 4:52 PM

land what's easily landable to reduce size of stack

This revision was landed with ongoing or failed builds.Dec 12 2023, 7:11 PM
This revision was automatically updated to reflect the committed changes.