Merge pull request #754 from OneSignal/fix/base_html_tag_error

Fix SW scope handling of base html tag
OneSignal · Feb 26, 2021 · 62ed102 · 62ed102
2 parents bf7d47d + 597508b
commit 62ed102
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 20 deletions.
diff --git a/express_webpack/index.html b/express_webpack/index.html
@@ -1,5 +1,8 @@
 <!DOCTYPE html>
 <html lang="en">
+<!-- Some sites use this HTML base tag to change the origin for all relative links.
+    Ensure OneSignal SDK correctly handles this. -->
+<base href="https://www.example.com/">
 <script src="/sdks/Dev-OneSignalSDK.js" async=""></script>
 <script>
     const SERVICE_WORKER_PATH = "push/onesignal/";

diff --git a/src/helpers/page/ServiceWorkerUtilHelper.ts b/src/helpers/page/ServiceWorkerUtilHelper.ts
@@ -1,10 +1,13 @@
 import Log from "../../libraries/Log";
 
 export default class ServiceWorkerUtilHelper {
-  // Get the service worker based on a scope as a domain can have none to many service workers.
+  // Get the service worker based on a relative scope.
+  // A  relative scope is required as a domain can have none to many service workers installed.
   static async getRegistration(scope: string): Promise<ServiceWorkerRegistration | null | undefined> {
     try {
-      return await navigator.serviceWorker.getRegistration(scope);
+      // Adding location.origin to negate the effects of a possible <base> html tag the page may have.
+      const url = location.origin + scope;
+      return await navigator.serviceWorker.getRegistration(url);
     } catch (e) {
       // This could be null in an HTTP context or error if the user doesn't accept cookies
       Log.warn("[Service Worker Status] Error Checking service worker registration", scope, e);

diff --git a/test/support/mocks/service-workers/models/MockServiceWorkerContainer.ts b/test/support/mocks/service-workers/models/MockServiceWorkerContainer.ts
@@ -40,8 +40,11 @@ export abstract class MockServiceWorkerContainer implements ServiceWorkerContain
     return this.dispatchEventUtil.dispatchEvent(evt);
   }
 
+  // clientURL can be relative or a full URL
   async getRegistration(clientURL?: string): Promise<ServiceWorkerRegistration | undefined> {
-    const scope = clientURL || "/";
+    const scope = clientURL ?
+      clientURL.replace(location.origin, "") : // Turn a possible full URL into a scope
+      "/";
 
     // 1. If we find an exact path match
     let registration = this.serviceWorkerRegistrations.get(scope);

diff --git a/test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts b/test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts
@@ -31,6 +31,11 @@ export class MockServiceWorkerContainerWithAPIBan extends MockServiceWorkerConta
     if (!clientURL) {
       throw new Error("Must include clientURL to get the SW of the scope we registered, not the current page being viewed.")
     }
+
+    if (!clientURL.startsWith(location.origin)) {
+      throw new Error("Must always use full URL as the HTML <base> tag can change the relative path.");
+    }
+
     return super.getRegistration(clientURL);
   }
 

diff --git a/test/unit/managers/ServiceWorkerManager.ts b/test/unit/managers/ServiceWorkerManager.ts
@@ -299,7 +299,7 @@ test('installWorker() installs Worker new scope when it changes', async t => {
   // 4. Ensure we kept the original ServiceWorker.
   //   A. Original could contain more than just OneSignal code
   //   B. New ServiceWorker instance will have it's own pushToken, this may have not been sent onesignal.com yet.
-  const orgRegistration = await navigator.serviceWorker.getRegistration("/");
+  const orgRegistration = await navigator.serviceWorker.getRegistration(`${location.origin}/`);
   t.is(new URL(orgRegistration!.scope).pathname, "/");
 });
 

diff --git a/test/unit/meta/mockPushManager.ts b/test/unit/meta/mockPushManager.ts
@@ -8,7 +8,7 @@ import { MockPushManager } from "../../support/mocks/service-workers/models/Mock
 import { MockPushSubscription } from "../../support/mocks/service-workers/models/MockPushSubscription";
 
 async function getServiceWorkerRegistration(): Promise<ServiceWorkerRegistration | undefined> {
-  return await navigator.serviceWorker.getRegistration("/");
+  return await navigator.serviceWorker.getRegistration(`${location.origin}/`);
 }
 
 test.beforeEach(async t => {

diff --git a/test/unit/meta/mockServiceWorker.ts b/test/unit/meta/mockServiceWorker.ts
@@ -48,6 +48,20 @@ test('mock service worker registration should return the registered worker', asy
   t.deepEqual(registrations, [registration]);
 });
 
+test('mock service worker registration should return registered worker with relative scope', async t => {
+  const scope = '/';
+  await navigator.serviceWorker.register('/worker.js', { scope });
+  const registration = await navigator.serviceWorker.getRegistration(scope);
+  t.truthy(registration);
+});
+
+test('mock service worker registration should return registered worker with full url scope', async t => {
+  const scope = `${location.origin}/`;
+  await navigator.serviceWorker.register('/worker.js', { scope });
+  const registration = await navigator.serviceWorker.getRegistration(scope);
+  t.truthy(registration);
+});
+
 test('mock service worker getRegistrations should return multiple registered workers', async t => {
   const expectedRegistrations = [] as ServiceWorkerRegistration[];
   expectedRegistrations.push(await navigator.serviceWorker.register('/workerA.js', { scope: '/' }));

diff --git a/test/unit/meta/mockServiceWorkerContainerWithAPIBan.ts b/test/unit/meta/mockServiceWorkerContainerWithAPIBan.ts
@@ -15,7 +15,7 @@ test('mock service worker browser API properties should exist', async t => {
 });
 
 test('mock service worker should not return an existing registration for a clean run', async t => {
-  const registration = await navigator.serviceWorker.getRegistration("/");
+  const registration = await navigator.serviceWorker.getRegistration(`${location.origin}/`);
   t.is(registration, undefined);
 
   const registrations = await navigator.serviceWorker.getRegistrations();
@@ -25,67 +25,76 @@ test('mock service worker should not return an existing registration for a clean
 test('mock service worker unregistration should return no registered workers', async t => {
   await navigator.serviceWorker.register('/worker.js', { scope: '/' });
 
-  const initialRegistration = await navigator.serviceWorker.getRegistration("/");
+  const initialRegistration = await navigator.serviceWorker.getRegistration(`${location.origin}/`);
   await initialRegistration!.unregister();
 
-  const postUnsubscribeRegistration = await navigator.serviceWorker.getRegistration("/");
+  const postUnsubscribeRegistration = await navigator.serviceWorker.getRegistration(`${location.origin}/`);
   t.is(postUnsubscribeRegistration, undefined);
 
   const registrations = await navigator.serviceWorker.getRegistrations();
   t.deepEqual(registrations, []);
 });
 
-test('Should thow when calling navigator.serviceWorker.controller', async t => {
+test('Should throw when calling navigator.serviceWorker.controller', async t => {
   try {
     navigator.serviceWorker!.controller;
-    t.fail("Should have thrown!")
+    t.fail("Should have thrown!");
   } catch (e) {
     t.deepEqual(e, new Error("Don't use, assumes page control!"));
   }
 });
 
-test('Should thow when calling navigator.serviceWorker.ready', async t => {
+test('Should throw when calling navigator.serviceWorker.ready', async t => {
   try {
     await navigator.serviceWorker!.ready;
-    t.fail("Should have thrown!")
+    t.fail("Should have thrown!");
   } catch (e) {
     t.deepEqual(e, new Error("Don't use, assumes page control!"));
   }
 });
 
-test('Should thow when calling navigator.serviceWorker.getRegistration without a url', async t => {
+test('Should throw when calling navigator.serviceWorker.getRegistration without a url', async t => {
   try {
     await navigator.serviceWorker!.getRegistration();
-    t.fail("Should have thrown!")
+    t.fail("Should have thrown!");
   } catch (e) {
     t.deepEqual(e, new Error("Must include clientURL to get the SW of the scope we registered, not the current page being viewed."));
   }
 });
 
-test('Should thow when setting navigator.serviceWorker.oncontrollerchange', async t => {
+test('Should throw when calling navigator.serviceWorker.getRegistration with a relative URL', async t => {
+  try {
+    await navigator.serviceWorker!.getRegistration("/");
+    t.fail("Should have thrown!");
+  } catch (e) {
+    t.deepEqual(e, new Error("Must always use full URL as the HTML <base> tag can change the relative path."));
+  }
+});
+
+test('Should throw when setting navigator.serviceWorker.oncontrollerchange', async t => {
   try {
     navigator.serviceWorker.oncontrollerchange = function(){};
-    t.fail("Should have thrown!")
+    t.fail("Should have thrown!");
   } catch (e) {
     t.deepEqual(e, new Error("Don't use, assumes page control!"));
   }
 });
 
-test('Should thow when setting navigator.serviceWorker.addEventListener(controllerchange, ...)', async t => {
+test('Should throw when setting navigator.serviceWorker.addEventListener(controllerchange, ...)', async t => {
   try {
     navigator.serviceWorker.addEventListener('controllerchange', function(){});
-    t.fail("Should have thrown!")
+    t.fail("Should have thrown!");
   } catch (e) {
     t.deepEqual(e, new Error("Don't use, assumes page control!"));
   }
 });
 
-test('Should not thow when setting navigator.serviceWorker.addEventListener(message, ...)', async t => {
+test('Should not throw when setting navigator.serviceWorker.addEventListener(message, ...)', async t => {
   navigator.serviceWorker.addEventListener('message', function(){});
   t.pass();
 });
 
-test('Should not thow when setting navigator.serviceWorker.addEventListener(messageerror, ...)', async t => {
+test('Should not throw when setting navigator.serviceWorker.addEventListener(messageerror, ...)', async t => {
   navigator.serviceWorker.addEventListener('messageerror', function(){});
   t.pass();
 });