[native] Fix failing base64 decode for thumbhash
ClosedPublic
Actions

Authored by bartek on Dec 12 2023, 4:00 AM.

Details

Reviewers

atul
tomek

Commits

rCOMMd42495a7d3be: [native] Fix failing base64 decode for thumbhash

Summary

Fixes ENG-6031. It appears that ThumbhashModule sometimes returns base64 string containing newlines, which causes issues during encryption. More details in the linked issue description.

Even though still not sure if Android code is supposed to return base64 string with newlines, I decided to fix it by simply stripping them in JS. This is more future-proof than experimenting with native flags for base64 encoding.

Test Plan

I wasn't able to repro Android Base64.encodeToString() returning newline so I appended it manually for testing:

Confirmed that C++ is_valid_base64() returns false for this kind of thumbhash -> causes encryption failure
Confirmed that my fix makes is_valid_base64() return true -> encryption succeeds -> cannot reporduce anymore with my manually appended \n.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Dec 12 2023, 4:00 AM

bartek held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptDec 12 2023, 4:00 AM

bartek published this revision for review.Dec 12 2023, 4:12 AM

Harbormaster completed remote builds in B24989: Diff 34529.Dec 12 2023, 5:04 AM

It seems weird to me that the base64 ever ends with newline. Don't have much context into how everything works on native side, but are we always getting it back directly from the android.util.Base64 function? Are we maybe writing it to a file and reading it back at any point?

This revision is now accepted and ready to land.Dec 12 2023, 10:58 AM

Didn't do any original research or fact checking, so take it with a grain of salt, but it looks like inclusion of \n character might have to do with some original Base64 MIME spec:

Looks like the fix would involve the following flag:

So something like the following in ThumbHash.kt:

would presumably fix the problem.

It's surprising that the output is not deterministic. I wonder if calling Base64.encodeToString(...) without the Base64.DEFAULT flag results in different behavior than including it?

I think including the NO_WRAP flag would be a cleaner solution as it doesn't require us to "patch up" behavior of native modules on the JS side, but defer to you since you have more context.

Apologize for additional churn, but looks like changing that flag from Base64.DEFAULT to Base64.NO_WRAP does the trick.

Feel free to re-request changes if I'm missing anything

This revision now requires changes to proceed.Dec 12 2023, 11:36 AM

The NO_WRAP flag is the thing I mentioned in this Linear comment so yeah, your research confirms mine. I'm glad that you were able to confirm that this flag does the trick - I couldn't do it because, as I said before, I wasn't able to reproduce it myself. Also browsing the RFC 2045 mentioned in the Base64.encodeToString() docs didn't give me the answer about expected behavior.
This said, I'm going to use this flag in Kotlin because this is a root-cause solution.

However, I have a strong opinion that we should keep the JS regex-replace workaround I introduced. It won't hurt and it's future-proof.
And I still consider the native base64 encode behavior as random/undefined. Why wasn't it failing earlier? We've been using thumbhash with encrypted media for half a year now. I checked iOS too, hopefully, we don't set any of these flags.