Bug 1280106 - Don't send push events to service workers without a principal. r=dragana

MozReview-Commit-ID: JwTU633hENl

--HG--
extra : rebase_source : 58d7df0903d433f9070941fd7d6853f2f2471b9c

dom/push/PushDB.jsm

@@ -125,10 +125,10 @@ this.PushDB.prototype = {
       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);
           };
         },

dom/push/PushNotifier.cpp

@@ -47,6 +47,7 @@ PushNotifier::NotifyPushWithData(const nsACString& aScope,
                                  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;
@@ -62,6 +63,7 @@ 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);
 }
@@ -70,6 +72,7 @@ NS_IMETHODIMP
 PushNotifier::NotifySubscriptionChange(const nsACString& aScope,
                                        nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionChangeDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
@@ -78,6 +81,7 @@ NS_IMETHODIMP
 PushNotifier::NotifySubscriptionModified(const nsACString& aScope,
                                          nsIPrincipal* aPrincipal)
 {
+  NS_ENSURE_ARG(aPrincipal);
   PushSubscriptionModifiedDispatcher dispatcher(aScope, aPrincipal);
   return Dispatch(dispatcher);
 }
@@ -86,6 +90,7 @@ 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);
 }
@@ -276,6 +281,9 @@ PushDispatcher::NotifyObserversAndWorkers()
 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.

dom/push/test/xpcshell/test_observer_remoting.js

@@ -11,10 +11,23 @@ add_task(function* test_observer_remoting() {
   }
 });
 
+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;
 
@@ -23,7 +36,10 @@ function* testInParent() {
 
   // 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;
@@ -32,16 +48,22 @@ function* testInParent() {
 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(
@@ -50,8 +72,7 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) {
     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);
@@ -68,7 +89,7 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) {
   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,
@@ -87,4 +108,4 @@ function* waitForNotifierObservers(expectedText, shouldNotify = false) {
     'Should fire subscription modified notifications with the correct scope');
   equal(subModifiedPrincipal, principal,
     'Should pass the principal as the subject of a modified notification');
-}
+});

dom/push/test/xpcshell/test_quota_observer.js

@@ -110,7 +110,8 @@ add_task(function* test_expiration_history_observer() {
     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',
@@ -121,6 +122,17 @@ add_task(function* test_expiration_history_observer() {
     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({
@@ -143,4 +155,29 @@ add_task(function* test_expiration_history_observer() {
 
   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;
 });