U2FTokenTransport promises should resolve to U2F data buffers

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [webauthn])

Attachments

(1 attachment)

Currently, we call U2FTokenTransport::Register() and Sign() and pass stack references that we expect to be filled when the promise resolves. The then() callback itself copies the nsTArrays, so this probably works by accident?

Well, this is bad. We should have proper Register/Sign promises that return a result specific to the request type. That result then holds the u8 buffers returned by the token.
(Assignee)

Updated

a year ago
Summary: U2FTokenTransport promises should return U2F data → U2FTokenTransport promises should resolve to U2F data buffers
(Assignee)

Comment 1

a year ago
Created attachment 8884769 [details] [diff] [review]
0001-Bug-1379580-U2FTokenTransport-promises-should-resolv.patch
Attachment #8884769 - Flags: review?(kyle)
Comment on attachment 8884769 [details] [diff] [review]
0001-Bug-1379580-U2FTokenTransport-promises-should-resolv.patch

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

r+, though I do have one question.

::: dom/webauthn/U2FTokenManager.cpp
@@ +322,5 @@
>  }
>  
>  void
>  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> +                                  U2FSignResult&& aResult)

Just curious, why the move semantics here and in register? I think the call to SendConfirmSign/Register is just going to copy these elements into a message block for IPC, so afaik there's nothing after this that really needs the ownership? Doesn't really hurt anything and I suppose if you're following the Promise API, it's fine. Just want to make sure I'm not missing something.
Attachment #8884769 - Flags: review?(kyle) → review+
(Assignee)

Comment 3

a year ago
(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #2)
> >  void
> >  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> > +                                  U2FSignResult&& aResult)
> 
> Just curious, why the move semantics here and in register? I think the call
> to SendConfirmSign/Register is just going to copy these elements into a
> message block for IPC, so afaik there's nothing after this that really needs
> the ownership? Doesn't really hurt anything and I suppose if you're
> following the Promise API, it's fine. Just want to make sure I'm not missing
> something.

You're right, I went a little overboard with the move semantics here. We actually just need to pass by reference from the promise->Then() callback.

Thanks!

Comment 4

a year ago
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/695e774df6c6
U2FTokenTransport promises should resolve to U2F data buffers r=qDot

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/695e774df6c6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.