Closed Bug 982965 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 - wontfix
firefox30 --- fixed

People

(Reporter: ckarlof, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

This represents a "never should happen" situation and it can lock the user out of her sync account if it happens.
Attached patch patch (obsolete) — Splinter Review
I can't get the mock object in the test to work properly - my getKeys is seemingly never called. Any thoughts?
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8390867 - Flags: feedback?(ckarlof)
You're missing a few setup steps. Mark will help get this working.
Attachment #8390867 - Flags: feedback?(ckarlof)
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.
Assignee: gavin.sharp → mhammond
Attachment #8390867 - Attachment is obsolete: true
Attachment #8391489 - Flags: review?(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?
Attachment #8391489 - Flags: review?(ckarlof) → review+
https://hg.mozilla.org/mozilla-central/rev/73c91ac7e8a9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Are we still planning on uplifting this?
(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.
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.
(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.
FYI server-side mitigations will empty client-state are discussed in Bug 985784.
> 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.
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.