982965 - Throw an error if we try to calculate the X-Client-State header based on an empty kB

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Chris Karlof [:ckarlof]]

This represents a "never should happen" situation and it can lock the user out of her sync account if it happens.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

I can't get the mock object in the test to work properly - my getKeys is seemingly never called. Any thoughts?

Comment 2

[Chris Karlof [:ckarlof]]

You're missing a few setup steps. Mark will help get this working.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-982965-ensure-we-fail-if-getKeys-can-t-get-kA-or.patch

The test infrastructure is way more difficult to use than it should be, but now isn't the time to fix that.

Also, as discussed, we decided getKeys() itself should throw/reject if no kA or kB, so this patch also does that.

Comment 4

[Chris Karlof [:ckarlof]]

Comment on attachment 8391489 [details] [diff] [review]
0001-Bug-982965-ensure-we-fail-if-getKeys-can-t-get-kA-or.patch

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

::: services/sync/tests/unit/test_browserid_identity.js
@@ +432,5 @@
> +
> +  // Mock a fxAccounts object that returns no keys
> +  let fxa = new FxAccounts({
> +    fetchAndUnwrapKeys: function () {
> +      //throw "LSDKFSLDFKSD:F";

Delete this?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/73c91ac7e8a9

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/73c91ac7e8a9

Comment 7

[Chris Karlof [:ckarlof]]

Are we still planning on uplifting this?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Chris Karlof [:ckarlof] from comment #7)
> Are we still planning on uplifting this?

IIUC, this basically turns an obscure failure into a less-obscure one?  So really one of Chris or Gavin should probably make that call based on how obscure it actually was ;)  It seems very low risk.

Comment 9

[Chris Karlof [:ckarlof]]

We might want to get this uplifted. We saw an empty "client state" sent to token server today: Bug 985504. However, the real check we might want to do is to make sure we don't ever send an empty client state header to the token server.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Chris Karlof [:ckarlof] from comment #9)
> We might want to get this uplifted. We saw an empty "client state" sent to
> token server today: Bug 985504. However, the real check we might want to do
> is to make sure we don't ever send an empty client state header to the token
> server.

I must have understood this bug - I didn't realize that sending the empty client state effectively broke the account for the user.  But I'd say the real check should be made by the server.

So while I'd be fine with uplifting this patch, rfkelly tells me he should be able to handle this case on the server without much difficulty, and having the server reject it seems the more robust option anyway.

Ryan says it may take a few days to get into prod, but an uplift request here would probably also take a few days to make it to beta users.

Chris, given the above and your concern that this patch doesn't do the correct check anyway, do you think it is worthwhile requesting uplift?  I've no real objection, but also don't see it as particularly valuable if the server can reliably reject this case.

Comment 11

[Ryan Kelly [:rfkelly]]

FYI server-side mitigations will empty client-state are discussed in Bug 985784.

Comment 12

[Chris Karlof [:ckarlof]]

> Chris, given the above and your concern that this patch doesn't do the correct check anyway, do you think it is worthwhile requesting uplift?  I've no real objection, but also don't see it as particularly valuable if the server can reliably reject this case.

I think having the server reject empty client-state is a good idea. :)

Let's save our uplift karma for something else.