Commit 8ec3020c authored by Beth Rennie's avatar Beth Rennie
Browse files

Bug 1774022 - Validate features described with FML variables more strictly r=TravisLong 

Experimenter now performs the following additional validations:

- FML `int` field types are mapped to `integer` and not `number`; and
- `additionalProperties` is now false;


We now perform the same validation.

Differential Revision: https://phabricator.services.mozilla.com/D149146
parent e4be0147

toolkit/components/nimbus/FeatureManifest.yaml

+3 −0
Original line number Diff line number Diff line
@@ -13,6 +13,9 @@ testFeature: 
  hasExposure: false

  isEarlyStartup: true

  variables:

    enabled:

      type: boolean

      description: Whether or not this feature is enabled

    testInt:

      type: int

      fallbackPref: nimbus.testing.testInt

toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm

+5 −7
Original line number Diff line number Diff line
@@ -437,7 +437,7 @@ class _RemoteSettingsExperimentLoader { 
      description: manifest.description,

      type: "object",

      properties: {},

      additionalProperties: true,

      additionalProperties: false,

    };



    for (const [varName, desc] of Object.entries(manifest.variables)) {
@@ -449,21 +449,19 @@ class _RemoteSettingsExperimentLoader { 
          break;



        case "int":

          // NB: This is what Experimenter maps the int type to.

          prop.type = "number";

          prop.type = "integer";

          break;



        case "json":

          // NB: Experimenter presently ignores the json type, it will still be

          // allowed under additionalProperties.

          continue;

          // NB: Don't set a type of json fields, since they can be of any type.

          break;



        default:

          // NB: Experimenter doesn't outright reject invalid types either.

          Cu.reportError(

            `Feature ID ${featureId} has variable ${varName} with invalid FML type: ${prop.type}`

          );

          continue;

          break;

      }



      if (prop.type === "string" && !!desc.enum) {

toolkit/components/nimbus/test/NimbusTestUtils.jsm

+5 −0
Original line number Diff line number Diff line
@@ -152,6 +152,11 @@ const ExperimentTestUtils = { 
   */

  addTestFeatures(...features) {

    for (const feature of features) {

      if (Object.hasOwn(lazy.NimbusFeatures, feature.featureId)) {

        throw new Error(

          `Cannot add feature ${feature.featureId} -- a feature with this ID already exists!`

        );

      }

      lazy.NimbusFeatures[feature.featureId] = feature;

    }

    return () => {

toolkit/components/nimbus/test/browser/browser_remotesettingsexperimentloader_remote_defaults.js

+55 −19
Original line number Diff line number Diff line
@@ -73,7 +73,7 @@ const REMOTE_CONFIGURATION_FOO = ExperimentFakes.recipe("foo-rollout", { 
          featureId: "foo",

          enabled: true,

          isEarlyStartup: true,

          value: { remoteValue: 42 },

          value: { remoteValue: 42, enabled: true },

        },

      ],

    },
@@ -91,7 +91,7 @@ const REMOTE_CONFIGURATION_BAR = ExperimentFakes.recipe("bar-rollout", { 
          featureId: "bar",

          enabled: true,

          isEarlyStartup: true,

          value: { remoteValue: 3 },

          value: { remoteValue: 3, enabled: true },

        },

      ],

    },
@@ -112,17 +112,20 @@ async function setup(configuration) { 
    }

  );



  registerCleanupFunction(async () => {

    await client.db.clear();

  });



  return client;

  const cleanup = () => client.db.clear();

  return { client, cleanup };

}



add_task(async function test_remote_fetch_and_ready() {

  const sandbox = sinon.createSandbox();

  const fooInstance = new ExperimentFeature("foo", FOO_FAKE_FEATURE_MANIFEST);

  const barInstance = new ExperimentFeature("bar", BAR_FAKE_FEATURE_MANIFEST);



  const cleanupTestFeatures = ExperimentTestUtils.addTestFeatures(

    fooInstance,

    barInstance

  );



  const sandbox = sinon.createSandbox();

  const setExperimentActiveStub = sandbox.stub(

    TelemetryEnvironment,

    "setExperimentActive"
@@ -132,10 +135,6 @@ add_task(async function test_remote_fetch_and_ready() { 
    "setExperimentInactive"

  );



  registerCleanupFunction(

    ExperimentTestUtils.addTestFeatures(fooInstance, barInstance)

  );



  Assert.equal(

    fooInstance.getVariable("remoteValue"),

    undefined,
@@ -149,7 +148,7 @@ add_task(async function test_remote_fetch_and_ready() { 


  await ExperimentAPI.ready();



  let rsClient = await setup();

  let { client: rsClient, cleanup } = await setup();



  // Fake being initialized so we can update recipes

  // we don't need to start any timers
@@ -266,6 +265,9 @@ add_task(async function test_remote_fetch_and_ready() { 
  ExperimentAPI._store._deleteForTests(REMOTE_CONFIGURATION_FOO.slug);

  ExperimentAPI._store._deleteForTests(REMOTE_CONFIGURATION_BAR.slug);

  sandbox.restore();



  cleanupTestFeatures();

  await cleanup();

});



add_task(async function test_remote_fetch_on_updateRecipes() {
@@ -309,15 +311,20 @@ add_task(async function test_remote_fetch_on_updateRecipes() { 
add_task(async function test_finalizeRemoteConfigs_cleanup() {

  const featureFoo = new ExperimentFeature("foo", {

    description: "mochitests",

    variables: {},

    variables: {

      foo: { type: "boolean" },

    },

  });

  const featureBar = new ExperimentFeature("bar", {

    description: "mochitests",

    variables: {},

    variables: {

      bar: { type: "boolean" },

    },

  });



  registerCleanupFunction(

    ExperimentTestUtils.addTestFeatures(featureFoo, featureBar)

  const cleanupTestFeatures = ExperimentTestUtils.addTestFeatures(

    featureFoo,

    featureBar

  );



  let fooCleanup = await ExperimentFakes.enrollWithRollout(
@@ -357,7 +364,24 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() { 
    JSON.stringify({ bar: true, branch: { feature: { featureId: "bar" } } })

  );



  await setup([REMOTE_CONFIGURATION_FOO]);

  const remoteConfiguration = {

    ...REMOTE_CONFIGURATION_FOO,

    branches: [

      {

        ...REMOTE_CONFIGURATION_FOO.branches[0],

        features: [

          {

            ...REMOTE_CONFIGURATION_FOO.branches[0].features[0],

            value: {

              foo: true,

            },

          },

        ],

      },

    ],

  };



  const { cleanup } = await setup([remoteConfiguration]);

  RemoteSettingsExperimentLoader._initialized = true;

  await RemoteSettingsExperimentLoader.updateRecipes();

  await cleanupPromise;
@@ -382,6 +406,9 @@ add_task(async function test_finalizeRemoteConfigs_cleanup() { 
  // only sets the recipe as inactive

  ExperimentAPI._store._deleteForTests("bar-rollout");

  ExperimentAPI._store._deleteForTests("foo-rollout");



  cleanupTestFeatures();

  cleanup();

});



// If the remote config data returned from the store is not modified
@@ -420,6 +447,12 @@ add_task(async function remote_defaults_active_remote_defaults() { 
    description: "mochitest",

    variables: { enabled: { type: "boolean" } },

  });



  const cleanupTestFeatures = ExperimentTestUtils.addTestFeatures(

    barFeature,

    fooFeature

  );



  let rollout1 = ExperimentFakes.recipe("bar", {

    branches: [

      {
@@ -457,7 +490,7 @@ add_task(async function remote_defaults_active_remote_defaults() { 
  });



  // Order is important, rollout2 won't match at first

  await setup([rollout2, rollout1]);

  const { cleanup } = await setup([rollout2, rollout1]);

  let updatePromise = new Promise(resolve => barFeature.onUpdate(resolve));

  RemoteSettingsExperimentLoader._initialized = true;

  await RemoteSettingsExperimentLoader.updateRecipes("mochitest");
@@ -474,6 +507,9 @@ add_task(async function remote_defaults_active_remote_defaults() { 
  Assert.ok(fooFeature.isEnabled(), "Targeting should match");

  ExperimentAPI._store._deleteForTests("foo");

  ExperimentAPI._store._deleteForTests("bar");



  cleanup();

  cleanupTestFeatures();

});



add_task(async function remote_defaults_variables_storage() {

toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader_updateRecipes.js

+10 −5
Original line number Diff line number Diff line
@@ -354,10 +354,13 @@ add_task(async function test_updateRecipes_simpleFeatureInvalidAfterUpdate() { 
    type: "object",

    properties: {

      testInt: {

        type: "number",

        type: "integer",

      },

      enabled: {

        type: "boolean",

      },

    additionalProperties: true,

    },

    additionalProperties: false,

  };



  sinon.spy(loader, "updateRecipes");
@@ -390,9 +393,11 @@ add_task(async function test_updateRecipes_simpleFeatureInvalidAfterUpdate() { 
    loader._generateVariablesOnlySchema.calledOnce,

    "Should have generated a schema for testFeature"

  );

  ok(

    loader._generateVariablesOnlySchema.returned(EXPECTED_SCHEMA),

    "should have generated a schema with one field"



  Assert.deepEqual(

    loader._generateVariablesOnlySchema.returnValues[0],

    EXPECTED_SCHEMA,

    "should have generated a schema with two fields"

  );



  info("Replacing recipe with an invalid one");