fix/typings: Remove Readonly<T> from AuthenticateHandler params #630
+5
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit removes the Readonly<T> wrappings around the `username` and `password` params to the `AuthenticateHandler` interface, which were added in moscajs#596. The motivation for this change is that when `password` is `Readonly<Buffer>`, the type is incompatible with `crypto.timingSafeEqual`, which is the function Aedes users should be using to compare raw, sensitive buffers with each other. Because `Readonly<Buffer>` is incompatible with `crypto.timingSafeEqual`, users end up having to cast with `password as Buffer`, which largely defeats the purpose of marking it `Readonly` in the first place and introduces casting in security-related areas of the code where it's not really needed in the first place. The error it gives is: No overload matches this call. Overload 1 of 2, '(a: ArrayBufferView, b: ArrayBufferView): boolean', gave the following error. Argument of type 'Readonly<Buffer>' is not assignable to parameter of type 'ArrayBufferView'. Type 'Readonly<Buffer>' is missing the following properties from type 'Float32Array': [Symbol.iterator], [Symbol.toStringTag] Overload 2 of 2, '(a: ArrayBufferView, b: ArrayBufferView): boolean', gave the following error. Argument of type 'Readonly<Buffer>' is not assignable to parameter of type 'ArrayBufferView'.ts(2769) Removing it from `username` has no effect, because strings are already immutable in JavaScript, and TypeScript will automatically treat it as if it were just `string`.
robertsLando
approved these changes
Apr 29, 2021
Pull Request Test Coverage Report for Build 793693562
💛 - Coveralls |
getlarge
approved these changes
Apr 29, 2021
|
I think @gnought introduced this type change few weeks ago, it would be nice to have his opinion before merging. |
|
@mcollina Thoughts? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit removes the Readonly wrappings around the
usernameandpasswordparams to theAuthenticateHandlerinterface, which were added in #596.The motivation for this change is that when
passwordisReadonly<Buffer>, the type is incompatible withcrypto.timingSafeEqual, which is the function Aedes users should be using to compare raw, sensitive buffers with each other.Because
Readonly<Buffer>is incompatible withcrypto.timingSafeEqual, users end up having to cast withpassword as Buffer, which largely defeats the purpose of marking itReadonlyin the first place and introduces casting in security-related areas of the code where it's not really needed in the first place.The error it gives is:
Removing it from
usernamehas no effect, because strings are already immutable in JavaScript, and TypeScript will automatically treat it as if it were juststring.