Closed Bug 614737 Opened 10 years ago Closed 10 years ago

Simplified crypto: ensure older clients detect outdated version


(Firefox :: Sync, defect)

Not set



Tracking Status
blocking2.0 --- beta8+


(Reporter: philikon, Assigned: rnewman)




(2 files, 1 obsolete file)

From bug 603489 comment 73:

The storageVersion detection code so far in Sync does not anticipate a change in the crypto structure. Upon upgrade we delete keys/pubkey and keys/privkey. This makes older Sync versions think that the passphrase is wrong instead of surfacing the "Your version is too old" error message on Sync. It doesn't even get that far because it fails on login.

This is very unfortunate. I believe we should do two things about this:

1. Do not delete keys/pubkey and keys/privkey upon migration. Just leave them around, they won't do harm and instead trick old clients into logging in properly first and then detecting their version is too old.

2. Make sure that we won't run into this problem in the future. This could be the case already since we now call remoteSetup() from verifyLogin() which definitely does this check somewhere, but it would good to have a test for this.
Blocks: 615021
Assignee: nobody → rnewman
blocking2.0: --- → beta8+
This patch adjusts wipeServer to leave the "keys" collection by default.

Tests exercise this particular functionality.

The scenario in question has been manually tested:

* Create two new 3.6/1.5 profiles with the same Sync credentials
* Sync successfully
* Update one to the current build, watch it wipe server and upgrade
* Attempt to sync other client: observe "upgrade needed" notification, rather than "wrong sync key".

For good measure, I then verified that upgrading the other client will allow syncing to resume. IT LIVES!

A subsequent patch will address the storage version change detection logic in an automated test.

(Two separate patches so we can keep things moving.)
Attachment #493772 - Flags: review?(philipp)
Attached patch Tests for detection of upgrade. (obsolete) — Splinter Review
This test verifies login, then verifies that sync fails for the correct reason if the server version is bumped. It verifies again after wiping the server, as would be done during an upgrade.

Note that the old logic for signin/passphrase/key verification is no longer in the tree, so this test doesn't exercise it -- in other words, if you take out the fix, this test still passes. This test is still valuable to avoid a future regression.
Attachment #493802 - Flags: review?(philipp)
Add a line to verify that login succeeded.
Attachment #493802 - Attachment is obsolete: true
Attachment #493805 - Flags: review?(philipp)
Attachment #493802 - Flags: review?(philipp)
Comment on attachment 493772 [details] [diff] [review]
Patch and tests for changes.

> // Some tests hang on OSX debug builds. See bug 604565.
> let DISABLE_TESTS_BUG_604565 = false;
> #ifdef XP_MACOSX
>-DISABLE_TESTS_BUG_604565 = true;
>+DISABLE_TESTS_BUG_604565 = false;

Please remove this :)
Attachment #493772 - Flags: review?(philipp) → review+
Attachment #493805 - Flags: review?(philipp) → review+
Pushed to fx-sync.
(In reply to comment #5)
> Pushed to fx-sync.
Closed: 10 years ago
Resolution: --- → FIXED
verified testing recently nightly build to force update against a 1.5.1 ext on 3.6
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.