Skip to content
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

[ECO-4737, ECO-4756] Fix auth token encoding in React Native #1750

Merged
merged 5 commits into from
May 3, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Apr 24, 2024

Resolves #1730 , resolves #1749

Fixes incorrect usage of the underlying ArrayBuffer buffer in web BufferUtils, where we didn't account for the byteOffset and byteLength of the original array buffer view. This resulted in us incorrectly encoding auth tokens in the React Native bundle, where we use a polyfill for the TextEncoder class, which returned Uint8Array as a subarray of the underlying ArrayBuffer (see polyfill's code).

@github-actions github-actions bot temporarily deployed to staging/pull/1750/features April 24, 2024 14:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/bundle-report April 24, 2024 14:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/typedoc April 24, 2024 14:21 Inactive
@VeskeR VeskeR force-pushed the 1749/fix-invalid-auth-token-react-native branch from 921a176 to 5d25531 Compare April 24, 2024 14:38
@github-actions github-actions bot temporarily deployed to staging/pull/1750/features April 24, 2024 14:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/bundle-report April 24, 2024 14:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/typedoc April 24, 2024 14:39 Inactive
@VeskeR VeskeR force-pushed the 1749/fix-invalid-auth-token-react-native branch from 5d25531 to b747d9d Compare April 24, 2024 17:12
@github-actions github-actions bot temporarily deployed to staging/pull/1750/features April 24, 2024 17:12 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/typedoc April 24, 2024 17:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/bundle-report April 24, 2024 17:13 Inactive
@VeskeR VeskeR force-pushed the 1749/fix-invalid-auth-token-react-native branch from b747d9d to 21d04f5 Compare April 30, 2024 10:47
@github-actions github-actions bot temporarily deployed to staging/pull/1750/features April 30, 2024 10:47 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/bundle-report April 30, 2024 10:48 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/typedoc April 30, 2024 10:48 Inactive
@VeskeR VeskeR changed the title WIP test buffer in utf8Encode [ECO-4737, ECO-4756] Fix auth token encoding in React Native Apr 30, 2024
@VeskeR VeskeR marked this pull request as ready for review April 30, 2024 11:05
@VeskeR VeskeR force-pushed the 1749/fix-invalid-auth-token-react-native branch from 21d04f5 to 7745d0f Compare April 30, 2024 11:06
@github-actions github-actions bot temporarily deployed to staging/pull/1750/features April 30, 2024 11:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/bundle-report April 30, 2024 11:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1750/typedoc April 30, 2024 11:07 Inactive
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lawrence-forooghian
Copy link
Collaborator

Continuing our conversation on Slack where I asked why you'd switched web’s output type from ArrayBuffer to Uint8Array.

You said that going back and forth between Uint8Array and ArrayBuffer is error-prone because you have to make sure to remember to use byteOffset and byteLength when accessing a Uint8Array’s underlying storage. Fair enough, but I guess that could be solved by just having a method in BufferUtils for converting a Uint8Array to an ArrayBuffer and making sure that we always use it?

In terms of what's the more "correct" type to use, you said that using Uint8Array makes it more consistent with Node’s Buffer, which is a subclass of Uint8Array. Fair enough, although it does seem like that, out of ArrayBuffer and Uint8Array, the web platform usually uses ArrayBuffer as its binary data type — see, e.g. WebSocket’s binaryType property or SubtleCrypto’s exportKey. MDN’s documentation for ArrayBuffer also says "The ArrayBuffer object is used to represent a generic raw binary data buffer", which sounds like what we want.

Just some thoughts, will leave it to you to decide what to do 🙂

@VeskeR
Copy link
Contributor Author

VeskeR commented May 1, 2024

I guess that could be solved by just having a method in BufferUtils for converting a Uint8Array to an ArrayBuffer and making sure that we always use it?

Agree, I thought about this yesterday after your comments and started leaning towards this approach as well.
It will achieve the same goal and additionally ensure that if the consumer of the BufferUtils gets an array, they won't be able to access its .buffer property incorrectly (without using byteOffset and byteLength), as there is simply no such property on ArrayBuffer (and this could happen if we return Uint8Array from BufferUtils).

I'll go ahead and change it so we always convert to ArrayBuffer using the dedicated method which knows about byteOffset and byteLength.

VeskeR added 3 commits May 2, 2024 12:57
… when converting to ArrayBuffer

This commit also ensures that `toArrayBuffer` method is using
`.byteOffset` and `.byteLength` properties to retrieve correct section
of the underlying ArrayBuffer used by the provided TypedArray.

Resolves #1730, Resolves #1749
Also removes comment about old browsers support as it's not needed after
#1633
@VeskeR
Copy link
Contributor Author

VeskeR commented May 2, 2024

@lawrence-forooghian Updated PR to always convert to ArrayBuffer using dedicated method in a20f20b

@VeskeR VeskeR merged commit 961daa5 into main May 3, 2024
12 checks passed
@VeskeR VeskeR deleted the 1749/fix-invalid-auth-token-react-native branch May 3, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants