Merge pull request #497 from brizental/1712922-ping-body-limit

Beatriz Rizental · web-flow · commit c0b139be3cbe · 2021-07-06T12:13:56.000+02:00
Bug 1712922 - Implement ping body size limit
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 * [#431](https://github.com/mozilla/glean.js/pull/431): BUGFIX: Record the timestamp for events before dispatching to the internal task queue.
 * [#462](https://github.com/mozilla/glean.js/pull/462): Implement Storage module for Qt/QML platform.
 * [#466](https://github.com/mozilla/glean.js/pull/466): Expose `ErrorType` enum, for using with the `testGetNumRecordedErrors` API.
+* [#497](https://github.com/mozilla/glean.js/pull/497): Implement limit of 1MB for ping request payload. Limit is calculated after gzip compression.
 
 # v0.15.0 (2021-06-03)
 
diff --git a/glean/src/core/glean.ts b/glean/src/core/glean.ts
@@ -249,7 +249,6 @@ class Glean {
       // This is fine, we are inside a dispatched task that is guaranteed to run before any
       // other task. No external API call will be executed before we leave this task.
       Context.initialized = true;
-      Glean.pingUploader.setInitialized(true);
 
       // The upload enabled flag may have changed since the last run, for
       // example by the changing of a config file.
diff --git a/glean/src/core/upload/index.ts b/glean/src/core/upload/index.ts
@@ -7,16 +7,31 @@ import { gzipSync, strToU8 } from "fflate";
 import type Platform from "../../platform/index.js";
 import type { Configuration } from "../config.js";
 import { GLEAN_VERSION } from "../constants.js";
+import { Context } from "../context.js";
 import log, { LoggingLevel } from "../log.js";
 import type { Observer as PingsDatabaseObserver, PingInternalRepresentation } from "../pings/database.js";
 import type PingsDatabase from "../pings/database.js";
 import type PlatformInfo from "../platform_info.js";
-import type { UploadResult} from "./uploader.js";
+import type { UploadResult } from "./uploader.js";
 import type Uploader from "./uploader.js";
 import { UploadResultStatus } from "./uploader.js";
 
 const LOG_TAG = "core.Upload";
 
+/**
+ * Policies for ping storage, uploading and requests.
+ */
+export class Policy {
+  constructor (
+    // The maximum recoverable failures allowed per uploading window.
+    //
+    // Limiting this is necessary to avoid infinite loops on requesting upload tasks.
+    readonly maxRecoverableFailures: number = 3,
+    // The maximum size in bytes a ping body may have to be eligible for upload.
+    readonly maxPingBodySize: number = 1024 * 1024 // 1MB
+  ) {}
+}
+
 interface QueuedPing extends PingInternalRepresentation {
   identifier: string
 }
@@ -33,6 +48,14 @@ const enum PingUploaderStatus {
   Cancelling,
 }
 
+// Error to be thrown in case the final ping body is larger than MAX_PING_BODY_SIZE.
+class PingBodyOverflowError extends Error {
+  constructor(message?: string) {
+    super(message);
+    this.name = "PingBodyOverflow";
+  }
+}
+
 /**
  * A ping uploader. Manages a queue of pending pings to upload.
  *
@@ -57,13 +80,13 @@ class PingUploader implements PingsDatabaseObserver {
 
   private readonly platformInfo: PlatformInfo;
   private readonly serverEndpoint: string;
-  private readonly pingsDatabase: PingsDatabase;
-
-  // Whether or not Glean was initialized, as reported by the
-  // singleton object.
-  private initialized = false;
 
-  constructor(config: Configuration, platform: Platform, pingsDatabase: PingsDatabase) {
+  constructor(
+    config: Configuration,
+    platform: Platform,
+    private readonly pingsDatabase: PingsDatabase,
+    private readonly policy = new Policy
+  ) {
     this.queue = [];
     this.status = PingUploaderStatus.Idle;
     // Initialize the ping uploader with either the platform defaults or a custom
@@ -74,17 +97,6 @@ class PingUploader implements PingsDatabaseObserver {
     this.pingsDatabase = pingsDatabase;
   }
 
-  /**
-   * Signals that initialization of Glean was completed.
-   *
-   * This is required in order to not depend on the Glean object.
-   *
-   * @param state An optional state to set the initialization status to.
-   */
-  setInitialized(state?: boolean): void {
-    this.initialized = state ?? true;
-  }
-
   /**
    * Enqueues a new ping at the end of the line.
    *
@@ -139,21 +151,33 @@ class PingUploader implements PingsDatabaseObserver {
     };
 
     const stringifiedBody = JSON.stringify(ping.payload);
+    // We prefer using `strToU8` instead of TextEncoder directly,
+    // because it will polyfill TextEncoder if it's not present in the environment.
+    // Environments that don't provide TextEncoder are IE and most importantly QML.
+    const encodedBody = strToU8(stringifiedBody);
+
+    let finalBody: string | Uint8Array;
+    let bodySizeInBytes: number;
     try {
-      const compressedBody = gzipSync(strToU8(stringifiedBody));
+      finalBody = gzipSync(encodedBody);
+      bodySizeInBytes = finalBody.length;
       headers["Content-Encoding"] = "gzip";
-      headers["Content-Length"] = compressedBody.length.toString();
-      return {
-        headers,
-        payload: compressedBody
-      };
     } catch {
-      headers["Content-Length"] = stringifiedBody.length.toString();
-      return {
-        headers,
-        payload: stringifiedBody
-      };
+      finalBody = stringifiedBody;
+      bodySizeInBytes = encodedBody.length;
+    }
+
+    if (bodySizeInBytes > this.policy.maxPingBodySize) {
+      throw new PingBodyOverflowError(
+        `Body for ping ${ping.identifier} exceeds ${this.policy.maxPingBodySize}bytes. Discarding.`
+      );
     }
+
+    headers["Content-Length"] = bodySizeInBytes.toString();
+    return {
+      headers,
+      payload: finalBody
+    };
   }
 
   /**
@@ -163,7 +187,7 @@ class PingUploader implements PingsDatabaseObserver {
    * @returns The status number of the response or `undefined` if unable to attempt upload.
    */
   private async attemptPingUpload(ping: QueuedPing): Promise<UploadResult> {
-    if (!this.initialized) {
+    if (!Context.initialized) {
       log(
         LOG_TAG,
         "Attempted to upload a ping, but Glean is not initialized yet. Ignoring.",
@@ -172,17 +196,24 @@ class PingUploader implements PingsDatabaseObserver {
       return { result: UploadResultStatus.RecoverableFailure };
     }
 
-    const finalPing = await this.preparePingForUpload(ping);
-    const result = await this.uploader.post(
-      // We are sure that the applicationId is not `undefined` at this point,
-      // this function is only called when submitting a ping
-      // and that function return early when Glean is not initialized.
-      `${this.serverEndpoint}${ping.path}`,
-      finalPing.payload,
-      finalPing.headers
-    );
-
-    return result;
+    try {
+      const finalPing = await this.preparePingForUpload(ping);
+      return await this.uploader.post(
+        // We are sure that the applicationId is not `undefined` at this point,
+        // this function is only called when submitting a ping
+        // and that function return early when Glean is not initialized.
+        `${this.serverEndpoint}${ping.path}`,
+        finalPing.payload,
+        finalPing.headers
+      );
+    } catch(e) {
+      log(LOG_TAG, ["Error trying to build ping request:", e], LoggingLevel.Warn);
+      // An unrecoverable failure will make sure the offending ping is removed from the queue and
+      // deleted from the database, which is what we want here.
+      return {
+        result: UploadResultStatus.UnrecoverableFailure
+      };
+    }
   }
 
   /**
@@ -232,7 +263,7 @@ class PingUploader implements PingsDatabaseObserver {
     if (result === UploadResultStatus.UnrecoverableFailure || (status && status >= 400 && status < 500)) {
       log(
         LOG_TAG,
-        `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status ?? "no status"}.`,
+        `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was: ${status ?? "no status"}.`,
         LoggingLevel.Warn
       );
       await this.pingsDatabase.deletePing(identifier);
@@ -261,7 +292,7 @@ class PingUploader implements PingsDatabaseObserver {
         this.enqueuePing(nextPing);
       }
 
-      if (retries >= 3) {
+      if (retries >= this.policy.maxRecoverableFailures) {
         log(
           LOG_TAG,
           "Reached maximum recoverable failures for the current uploading window. You are done.",
diff --git a/glean/tests/unit/core/upload/index.spec.ts b/glean/tests/unit/core/upload/index.spec.ts
@@ -11,7 +11,7 @@ import { Context } from "../../../../src/core/context";
 import Glean from "../../../../src/core/glean";
 import PingType from "../../../../src/core/pings/ping_type";
 import { collectAndStorePing } from "../../../../src/core/pings/maker";
-import PingUploader from "../../../../src/core/upload";
+import PingUploader, { Policy } from "../../../../src/core/upload";
 import { UploadResultStatus } from "../../../../src/core/upload/uploader";
 
 const sandbox = sinon.createSandbox();
@@ -38,12 +38,16 @@ async function fillUpPingsDatabase(numPings: number): Promise<string[]> {
 }
 
 /**
- * Waits for the uploader on the Glean singleton to end its uploading job,
+ * Waits for a PingUploader to end its uploading job,
  * if it's actualy doing any.
+ *
+ * By default will wait for the uploader on the Glean singleton.
+ *
+ * @param uploader The uploader to wait for.
  */
-async function waitForGleanUploader(): Promise<void> {
-  if (Glean["pingUploader"]["currentJob"]) {
-    await Glean["pingUploader"]["currentJob"];
+async function waitForUploader(uploader = Glean["pingUploader"]): Promise<void> {
+  if (uploader["currentJob"]) {
+    await uploader["currentJob"];
   }
 }
 
@@ -79,7 +83,6 @@ describe("PingUploader", function() {
 
     // Create a new uploader and attach it to the existing storage.
     const uploader = new PingUploader(new Configuration(), Glean.platform, Context.pingsDatabase);
-    uploader.setInitialized();
     Context.pingsDatabase.attachObserver(uploader);
 
     // Mock the 'triggerUpload' function so that 'scanPendingPings' does not
@@ -104,7 +107,7 @@ describe("PingUploader", function() {
   it("if multiple pings are enqueued subsequently, we don't attempt to upload each ping more than once", async function () {
     const spy = sandbox.spy(Glean.platform.uploader, "post");
     await fillUpPingsDatabase(100);
-    await waitForGleanUploader();
+    await waitForUploader();
     assert.strictEqual(spy.callCount, 100);
   });
 
@@ -113,7 +116,6 @@ describe("PingUploader", function() {
     await fillUpPingsDatabase(10);
 
     const uploader = new PingUploader(new Configuration(), Glean.platform, Context.pingsDatabase);
-    uploader.setInitialized();
     Context.pingsDatabase.attachObserver(uploader);
     await Context.pingsDatabase.scanPendingPings();
 
@@ -132,7 +134,7 @@ describe("PingUploader", function() {
     // as the default exported mock already returns a success response always.
     await fillUpPingsDatabase(10);
 
-    await waitForGleanUploader();
+    await waitForUploader();
     assert.deepStrictEqual(await Context.pingsDatabase.getAllPings(), {});
     assert.strictEqual(Glean["pingUploader"]["queue"].length, 0);
   });
@@ -145,7 +147,7 @@ describe("PingUploader", function() {
     }));
     await fillUpPingsDatabase(10);
 
-    await waitForGleanUploader();
+    await waitForUploader();
     assert.deepStrictEqual(await Context.pingsDatabase.getAllPings(), {});
     assert.strictEqual(Glean["pingUploader"]["queue"].length, 0);
   });
@@ -158,7 +160,7 @@ describe("PingUploader", function() {
     }));
     await fillUpPingsDatabase(1);
 
-    await waitForGleanUploader();
+    await waitForUploader();
     // Ping should still be there.
     const allPings = await Context.pingsDatabase.getAllPings();
     assert.deepStrictEqual(Object.keys(allPings).length, 1);
@@ -193,17 +195,56 @@ describe("PingUploader", function() {
       status: 500,
       result: UploadResultStatus.Success
     }));
+
+    // Create a new ping uploader with a fixed max recoverable failures limit.
+    const uploader = new PingUploader(
+      new Configuration(),
+      Glean.platform,
+      Context.pingsDatabase,
+      new Policy(
+        3, // maxRecoverableFailures
+      )
+    );
+
+    // Overwrite the Glean ping uploader with the test one.
+    Context.pingsDatabase.attachObserver(uploader);
+
     await fillUpPingsDatabase(1);
+    await waitForUploader(uploader);
 
-    await waitForGleanUploader();
     assert.strictEqual(stub.callCount, 3);
   });
 
+  it("pings which exceed max ping body size are not sent", async function () {
+    // Create a new ping uploader with a very low max ping body size,
+    // so that virtually any ping body will throw an error.
+    const uploader = new PingUploader(
+      new Configuration(),
+      Glean.platform,
+      Context.pingsDatabase,
+      new Policy(
+        3, // maxRecoverableFailures
+        1 // maxPingBodySize
+      )
+    );
+
+    // Overwrite the Glean ping uploader with the test one.
+    Context.pingsDatabase.attachObserver(uploader);
+
+    const spy = sandbox.spy(Glean.platform.uploader, "post");
+    // Add a bunch of pings to the database, in order to trigger upload attempts on the uploader.
+    await fillUpPingsDatabase(10);
+    await waitForUploader(uploader);
+
+    // Check that none of those pings were actually sent.
+    assert.strictEqual(spy.callCount, 0);
+  });
+
   it("correctly build ping request", async function () {
     const postSpy = sandbox.spy(Glean.platform.uploader, "post");
 
     const expectedDocumentId = (await fillUpPingsDatabase(1))[0];
-    await waitForGleanUploader();
+    await waitForUploader();
 
     const url = postSpy.firstCall.args[0].split("/");
     const appId = url[url.length - 4];
