1364991 - WebAuthnTransactionParent.cpp Cast away const here since NSS wants to be able to use non-const functions

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P2)

Description

[J.C. Jones [:jcj] (he/they)]

Reported by Axel:

In https://dxr.mozilla.org/mozilla-central/source/dom/webauthn/WebAuthnTransactionParent.cpp#17

We cast away constness unnecessarily - this is only needed for the soft-token's interactions with NSS, so we should remain const here and move the cast down.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

:jc, feel free to adjust the priority best fitting your plan. :)

Comment 2

[J.C. Jones [:jcj] (he/they)]

I'll move it up to P2 - this is also a good first bug, but I imagine we'll just fix it while doing Bug 1332681.

Comment 3

[Axel Nennker]

Attachment: const_cast.patch

./mach is happy with this patch

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8869831 [details] [diff] [review]
const_cast.patch

Review of attachment 8869831 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Axel. The mochitests were happy with it as well.

The only thing I'm changing is the const_cast<unsigned char*> instances to const_cast<uint8_t*> to match the others. I've gone ahead with that and pushed the patch to Try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d2bfac5e2377e6f8d9134f6d9165fd69c4d6c2

I'm also going to re-open this review in MozReview and give it to qdot for his take.

::: dom/webauthn/U2FSoftTokenManager.cpp
@@ +777,4 @@
>  
>    // Decode the key handle
>    UniqueSECKEYPrivateKey privKey = PrivateKeyFromKeyHandle(slot, mWrappingKey,
> +                                                           const_cast<unsigned char*>(aKeyHandle.Elements()),

These .Elements() calls should all be casting to uint8_t.

Comment 5

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1364991 - Make U2FTokenManager use const where possible

The U2F Soft Token, due to its usage of NSS, has to have const values be
marked non-const - but no such limitation should exist for other implementations
of U2F, so this patch moves the const_cast-ing from the U2FTokenManager-level
down to the U2FSoftTokenManager, where it is actually necessary.

Credit to Axel Nennker for this patch.

Review commit: https://reviewboard.mozilla.org/r/141674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141674/

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8870219 [details]
Bug 1364991 - Make U2FTokenManager use const where possible

https://reviewboard.mozilla.org/r/141674/#review145314

Comment 7

[J.C. Jones [:jcj] (he/they)]

Thanks for the review, :qdot, and thanks for the patch, Axel.

Try run came back clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d2bfac5e2377e6f8d9134f6d9165fd69c4d6c2

Marking checkin-needed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0300ea496f8b
Make U2FTokenManager use const where possible r=qdot

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0300ea496f8b