Bug 1280106 - Don't send push events to service workers without a principal. r=dragana a=lizzard
authorKit Cambridge <kcambridge@mozilla.com>
Mon, 27 Jun 2016 08:22:59 -0700
changeset 339780 4bae0c26ac7116579c5a4ee0e4cd945c2094939a
parent 339779 9056c1ff97a6ffa10e787e52b64f34fae9151e73
child 339781 7a616bc015e5c458d0e86c47b853b256d348256d
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana, lizzard
bugs1280106
milestone49.0a2
Bug 1280106 - Don't send push events to service workers without a principal. r=dragana a=lizzard MozReview-Commit-ID: JwTU633hENl
dom/push/PushDB.jsm
dom/push/PushNotifier.cpp
dom/push/test/xpcshell/test_observer_remoting.js
dom/push/test/xpcshell/test_quota_observer.js
--- a/dom/push/PushDB.jsm
+++ b/dom/push/PushDB.jsm
@@ -120,20 +120,20 @@ this.PushDB.prototype = {
    */
   delete: function(aKeyID) {
     console.debug("delete()");
 
     return new Promise((resolve, reject) =>
       this.newTxn(
         "readwrite",
         this._dbStoreName,
-        function txnCb(aTxn, aStore) {
+        (aTxn, aStore) => {
           console.debug("delete: Removing record", aKeyID);
           aStore.get(aKeyID).onsuccess = event => {
-            aTxn.result = event.target.result;
+            aTxn.result = this.toPushRecord(event.target.result);
             aStore.delete(aKeyID);
           };
         },
         resolve,
         reject
       )
     );
   },
--- a/dom/push/PushNotifier.cpp
+++ b/dom/push/PushNotifier.cpp
@@ -42,55 +42,60 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(PushNoti
 NS_IMPL_CYCLE_COLLECTING_RELEASE(PushNotifier)
 
 NS_IMETHODIMP
 PushNotifier::NotifyPushWithData(const nsACString& aScope,
                                  nsIPrincipal* aPrincipal,
                                  const nsAString& aMessageId,
                                  uint32_t aDataLen, uint8_t* aData)
 {
+  NS_ENSURE_ARG(aPrincipal);
   nsTArray<uint8_t> data;
   if (!data.SetCapacity(aDataLen, fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   if (!data.InsertElementsAt(0, aData, aDataLen, fallible)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
   PushMessageDispatcher dispatcher(aScope, aPrincipal, aMessageId, Some(data));
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifyPush(const nsACString& aScope, nsIPrincipal* aPrincipal,
                          const nsAString& aMessageId)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushMessageDispatcher dispatcher(aScope, aPrincipal, aMessageId, Nothing());
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifySubscriptionChange(const nsACString& aScope,
                                        nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionChangeDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifySubscriptionModified(const nsACString& aScope,
                                          nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionModifiedDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
 
 NS_IMETHODIMP
 PushNotifier::NotifyError(const nsACString& aScope, nsIPrincipal* aPrincipal,
                           const nsAString& aMessage, uint32_t aFlags)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushErrorDispatcher dispatcher(aScope, aPrincipal, aMessage, aFlags);
   return Dispatch(dispatcher);
 }
 
 nsresult
 PushNotifier::Dispatch(PushDispatcher& aDispatcher)
 {
   if (XRE_IsParentProcess()) {
@@ -271,16 +276,19 @@ PushDispatcher::NotifyObserversAndWorker
 {
   Unused << NS_WARN_IF(NS_FAILED(NotifyObservers()));
   return NotifyWorkers();
 }
 
 bool
 PushDispatcher::ShouldNotifyWorkers()
 {
+  if (NS_WARN_IF(!mPrincipal)) {
+    return false;
+  }
   // System subscriptions use observer notifications instead of service worker
   // events. The `testing.notifyWorkers` pref disables worker events for
   // non-system subscriptions.
   return !nsContentUtils::IsSystemPrincipal(mPrincipal) &&
          Preferences::GetBool("dom.push.testing.notifyWorkers", true);
 }
 
 nsresult
--- a/dom/push/test/xpcshell/test_observer_remoting.js
+++ b/dom/push/test/xpcshell/test_observer_remoting.js
@@ -6,74 +6,95 @@ const pushNotifier = Cc['@mozilla.org/pu
 add_task(function* test_observer_remoting() {
   if (isParent) {
     yield testInParent();
   } else {
     yield testInChild();
   }
 });
 
+const childTests = [{
+  text: 'Hello from child!',
+  principal: Services.scriptSecurityManager.getSystemPrincipal(),
+}];
+
+const parentTests = [{
+  text: 'Hello from parent!',
+  principal: Services.scriptSecurityManager.getSystemPrincipal(),
+}];
+
 function* testInParent() {
   // Register observers for notifications from the child, then run the test in
   // the child and wait for the notifications.
-  let promiseNotifications = waitForNotifierObservers('Hello from child!');
+  let promiseNotifications = childTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ false)),
+    Promise.resolve()
+  );
   let promiseFinished = run_test_in_child('./test_observer_remoting.js');
   yield promiseNotifications;
 
   // Wait until the child is listening for notifications from the parent.
   yield do_await_remote_message('push_test_observer_remoting_child_ready');
 
   // Fire an observer notification in the parent that should be forwarded to
   // the child.
-  yield waitForNotifierObservers('Hello from parent!', true);
+  yield parentTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ true)),
+    Promise.resolve()
+  );
 
   // Wait for the child to exit.
   yield promiseFinished;
 }
 
 function* testInChild() {
   // Fire an observer notification in the child that should be forwarded to
   // the parent.
-  yield waitForNotifierObservers('Hello from child!', true);
+  yield childTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ true)),
+    Promise.resolve()
+  );
 
   // Register observers for notifications from the parent, let the parent know
   // we're ready, and wait for the notifications.
-  let promiseNotifierObservers = waitForNotifierObservers('Hello from parent!');
+  let promiseNotifierObservers = parentTests.reduce(
+    (p, test) => p.then(_ => waitForNotifierObservers(test, /* shouldNotify = */ false)),
+    Promise.resolve()
+  );
   do_send_remote_message('push_test_observer_remoting_child_ready');
   yield promiseNotifierObservers;
 }
 
-function* waitForNotifierObservers(expectedText, shouldNotify = false) {
+var waitForNotifierObservers = Task.async(function* ({ text, principal }, shouldNotify = false) {
   let notifyPromise = promiseObserverNotification(
     PushServiceComponent.pushTopic);
   let subChangePromise = promiseObserverNotification(
     PushServiceComponent.subscriptionChangeTopic);
   let subModifiedPromise = promiseObserverNotification(
     PushServiceComponent.subscriptionModifiedTopic);
 
   let scope = 'chrome://test-scope';
-  let principal = Services.scriptSecurityManager.getSystemPrincipal();
-  let data = new TextEncoder('utf-8').encode(expectedText);
+  let data = new TextEncoder('utf-8').encode(text);
 
   if (shouldNotify) {
     pushNotifier.notifyPushWithData(scope, principal, '', data.length, data);
     pushNotifier.notifySubscriptionChange(scope, principal);
     pushNotifier.notifySubscriptionModified(scope, principal);
   }
 
   let {
     data: notifyScope,
     subject: notifySubject,
   } = yield notifyPromise;
   equal(notifyScope, scope,
     'Should fire push notifications with the correct scope');
   let message = notifySubject.QueryInterface(Ci.nsIPushMessage);
   equal(message.principal, principal,
     'Should include the principal in the push message');
-  strictEqual(message.data.text(), expectedText, 'Should include data');
+  strictEqual(message.data.text(), text, 'Should include data');
 
   let {
     data: subChangeScope,
     subject: subChangePrincipal,
   } = yield subChangePromise;
   equal(subChangeScope, scope,
     'Should fire subscription change notifications with the correct scope');
   equal(subChangePrincipal, principal,
@@ -82,9 +103,9 @@ function* waitForNotifierObservers(expec
   let {
     data: subModifiedScope,
     subject: subModifiedPrincipal,
   } = yield subModifiedPromise;
   equal(subModifiedScope, scope,
     'Should fire subscription modified notifications with the correct scope');
   equal(subModifiedPrincipal, principal,
     'Should pass the principal as the subject of a modified notification');
-}
+});
--- a/dom/push/test/xpcshell/test_quota_observer.js
+++ b/dom/push/test/xpcshell/test_quota_observer.js
@@ -105,27 +105,39 @@ add_task(function* test_expiration_histo
   strictEqual(expiredRecord.quota, 0, 'Expired record not updated');
 
   let notifiedScopes = [];
   subChangePromise = promiseObserverNotification(PushServiceComponent.subscriptionChangeTopic, (subject, data) => {
     notifiedScopes.push(data);
     return notifiedScopes.length == 2;
   });
 
-  // Add an expired registration that we'll revive later.
+  // Add an expired registration that we'll revive later using the idle
+  // observer.
   yield putRecord('ALLOW_ACTION', {
     channelID: 'eb33fc90-c883-4267-b5cb-613969e8e349',
     pushEndpoint: 'https://example.org/push/2',
     scope: 'https://example.com/auctions',
     pushCount: 0,
     lastPush: 0,
     version: null,
     originAttributes: '',
     quota: 0,
   });
+  // ...And an expired registration that we'll revive on fetch.
+  yield putRecord('ALLOW_ACTION', {
+    channelID: '6b2d13fe-d848-4c5f-bdda-e9fc89727dca',
+    pushEndpoint: 'https://example.org/push/4',
+    scope: 'https://example.net/sales',
+    pushCount: 0,
+    lastPush: 0,
+    version: null,
+    originAttributes: '',
+    quota: 0,
+  });
 
   // Now visit the site...
   yield PlacesTestUtils.addVisits({
     uri: 'https://example.com/another-page',
     title: 'Infrequently-visited page',
     visitDate: Date.now() * 1000,
     transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
@@ -138,9 +150,34 @@ add_task(function* test_expiration_histo
     'https://example.com/deals'
   ], 'Wrong scopes for subscription changes');
 
   let aRecord = yield db.getByKeyID('379c0668-8323-44d2-a315-4ee83f1a9ee9');
   ok(!aRecord, 'Should drop expired record');
 
   let bRecord = yield db.getByKeyID('eb33fc90-c883-4267-b5cb-613969e8e349');
   ok(!bRecord, 'Should drop evicted record');
+
+  // Simulate a visit to a site with an expired registration, then fetch the
+  // record. This should drop the expired record and fire an observer
+  // notification.
+  yield PlacesTestUtils.addVisits({
+    uri: 'https://example.net/sales',
+    title: 'Firefox plushies, 99% off',
+    visitDate: Date.now() * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
+  });
+  subChangePromise = promiseObserverNotification(PushServiceComponent.subscriptionChangeTopic, (subject, data) => {
+    if (data == 'https://example.net/sales') {
+      ok(subject.isCodebasePrincipal,
+        'Should pass subscription principal as the subject');
+      return true;
+    }
+  });
+  let record = yield PushService.registration({
+    scope: 'https://example.net/sales',
+    originAttributes: '',
+  });
+  ok(!record, 'Should not return evicted record');
+  ok(!(yield db.getByKeyID('6b2d13fe-d848-4c5f-bdda-e9fc89727dca')),
+    'Should drop evicted record on fetch');
+  yield subChangePromise;
 });