-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Extend sync crypto to support byte arrays #7375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/craig/sync_deletion_of_duck_ai_chats
Are you sure you want to change the base?
Extend sync crypto to support byte arrays #7375
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
08feebf to
d36ea31
Compare
6833195 to
38e016a
Compare
malmstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHIP IT
d36ea31 to
32ff896
Compare
38e016a to
259e824
Compare
0c92c2f to
76cd1e5
Compare
7f14801 to
c7269ca
Compare
76cd1e5 to
e268906
Compare
c7269ca to
3b1274f
Compare
e268906 to
3d1db92
Compare
3b1274f to
b537d28
Compare
b8f6d64 to
c46ac61
Compare
9f55739 to
7076d7e
Compare
c46ac61 to
b706df5
Compare
7076d7e to
e779640
Compare
b2e25e9 to
8fad7c6
Compare
c3d194b to
4becbdb
Compare
8fad7c6 to
68609b8
Compare
| syncCrypto.encrypt("something".toByteArray()) | ||
|
|
||
| verify(recorder).record(DATA_ENCRYPT) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test verification never executes due to preceding exception
The verify(recorder).record(...) statements in whenEncryptByteArrayFailsThenExceptionThrown and whenDecryptByteArrayFailsThenExceptionThrown are placed after the line that throws an exception. Since syncCrypto.encrypt()/decrypt() throws before reaching the verification, these assertions never run. The tests will pass even if the recorder is never called in production, potentially masking missing error recording behavior.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
fa156a9 to
6b6dc6d
Compare
4becbdb to
29fc78f
Compare
6b6dc6d to
ee92e81
Compare
30b7555 to
f3e5f30
Compare
4957e06 to
4f4a176
Compare
e0b3b07 to
c651d8e
Compare
5eacdaa to
05596f0
Compare
c651d8e to
7636a72
Compare
05596f0 to
610d556
Compare
7636a72 to
3dfdcb3
Compare

Task/Issue URL: https://app.asana.com/1/137249556945/project/72649045549333/task/1212290226255081?focus=true
Description
Adds sync crypto support for byte arrays, adding the ability to encrypt and decrypt them similar to the existing functions which support strings only.
There's a few extra changes required because of needing to extend the test suite, as well as a few
FakeCryptoimplementations for testing, each needing tweaked.But the core change is:
String -> Byte Arrayas they are doing now already, then invoke the other function so that the logic is shared between the two impls.Steps to test this PR
Note
Adds ByteArray encrypt/decrypt APIs to sync crypto, implements them in native and real crypto, and updates tests and fakes accordingly.
SyncCryptowithencrypt(data: ByteArray)anddecrypt(data: ByteArray); clarify string encrypt returns Base64.RealSyncCrypto: implement new ByteArray methods with error recording; keep string methods; early-return on empty inputs.SyncLib/SyncNativeLib: add ByteArray overloads forencryptData/decryptDataand wire throughSyncNativeLibImpl.EncryptBytesResultandDecryptBytesResult; adapt string methods to reuse ByteArray flow.SyncCryptoTest: add ByteArray encrypt/decrypt tests and adjust string tests.FakeCryptoin tests (app,autofill,saved-sites,sync-settings) to implement new ByteArray methods.Written by Cursor Bugbot for commit 610d556. This will update automatically on new commits. Configure here.