Fix fallback for empty dd.appsec.enabled/DD_APPSEC_ENABLED (#4075)

Default is `inactive`, but on empty string it was falling back to `false`.
DataDog · Oct 24, 2022 · 79df296 · 79df296
1 parent ad47d5c
commit 79df296
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 3 deletions.
diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java
@@ -894,9 +894,10 @@ private static boolean isFeatureEnabled(AgentFeature feature) {
   private static boolean isAppSecFullyDisabled() {
     // must be kept in sync with logic from Config!
     final String featureEnabledSysprop = AgentFeature.APPSEC.systemProp;
-    String settingValue = System.getProperty(featureEnabledSysprop);
+    String settingValue = getNullIfEmpty(System.getProperty(featureEnabledSysprop));
     if (settingValue == null) {
-      settingValue = ddGetEnv(featureEnabledSysprop);
+      settingValue = getNullIfEmpty(ddGetEnv(featureEnabledSysprop));
+      settingValue = settingValue != null && settingValue.isEmpty() ? null : settingValue;
     }
 
     // defaults to inactive
@@ -906,6 +907,13 @@ private static boolean isAppSecFullyDisabled() {
         || settingValue.equalsIgnoreCase("inactive"));
   }
 
+  private static String getNullIfEmpty(final String value) {
+    if (value == null || value.isEmpty()) {
+      return null;
+    }
+    return value;
+  }
+
   /** @return configured JMX start delay in seconds */
   private static int getJmxStartDelay() {
     String startDelay = ddGetProperty("dd.dogstatsd.start-delay");

diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java
@@ -1098,7 +1098,17 @@ && isJavaVersionAtLeast(8)
     }
     telemetryHeartbeatInterval = telemetryInterval;
 
-    String appSecEnabled = configProvider.getString(APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED);
+    // ConfigProvider.getString currently doesn't fallback to default for empty strings. So we have
+    // special handling here until we have a general solution for empty string value fallback.
+    String appSecEnabled = configProvider.getString(APPSEC_ENABLED);
+    if (appSecEnabled == null || appSecEnabled.isEmpty()) {
+      appSecEnabled =
+          configProvider.getStringExcludingSource(
+              APPSEC_ENABLED, DEFAULT_APPSEC_ENABLED, SystemPropertiesConfigSource.class);
+      if (appSecEnabled.isEmpty()) {
+        appSecEnabled = DEFAULT_APPSEC_ENABLED;
+      }
+    }
     this.appSecEnabled = ProductActivationConfig.fromString(appSecEnabled);
     appSecReportingInband =
         configProvider.getBoolean(APPSEC_REPORTING_INBAND, DEFAULT_APPSEC_REPORTING_INBAND);

diff --git a/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy
@@ -5,6 +5,7 @@ import datadog.trace.bootstrap.config.provider.ConfigConverter
 import datadog.trace.bootstrap.config.provider.ConfigProvider
 import datadog.trace.test.util.DDSpecification
 import org.junit.Rule
+import spock.lang.Unroll
 
 import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_CLIENT_ERROR_STATUSES
 import static datadog.trace.api.ConfigDefaults.DEFAULT_HTTP_SERVER_ERROR_STATUSES
@@ -2035,6 +2036,68 @@ class ConfigTest extends DDSpecification {
     config.getMetricsIgnoredResources() == ["GET /healthcheck", "SELECT foo from bar"].toSet()
   }
 
+  @Unroll
+  def "appsec state with sys = #sys env = #env"() {
+    setup:
+    if (sys != null) {
+      System.setProperty("dd.appsec.enabled", sys)
+    }
+    if (env != null) {
+      environmentVariables.set("DD_APPSEC_ENABLED", env)
+    }
+
+    when:
+    def config = new Config()
+
+    then:
+    config.getAppSecEnabledConfig() == res
+
+    where:
+    sys        | env        | res
+    null       | null       | ProductActivationConfig.ENABLED_INACTIVE
+    null       | ""         | ProductActivationConfig.ENABLED_INACTIVE
+    null       | "inactive" | ProductActivationConfig.ENABLED_INACTIVE
+    null       | "false"    | ProductActivationConfig.FULLY_DISABLED
+    null       | "0"        | ProductActivationConfig.FULLY_DISABLED
+    null       | "invalid"  | ProductActivationConfig.FULLY_DISABLED
+    null       | "true"     | ProductActivationConfig.FULLY_ENABLED
+    null       | "1"        | ProductActivationConfig.FULLY_ENABLED
+    ""         | null       | ProductActivationConfig.ENABLED_INACTIVE
+    ""         | ""         | ProductActivationConfig.ENABLED_INACTIVE
+    ""         | "inactive" | ProductActivationConfig.ENABLED_INACTIVE
+    ""         | "false"    | ProductActivationConfig.FULLY_DISABLED
+    ""         | "0"        | ProductActivationConfig.FULLY_DISABLED
+    ""         | "invalid"  | ProductActivationConfig.FULLY_DISABLED
+    ""         | "true"     | ProductActivationConfig.FULLY_ENABLED
+    ""         | "1"        | ProductActivationConfig.FULLY_ENABLED
+    "inactive" | null       | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | ""         | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "inactive" | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "false"    | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "0"        | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "invalid"  | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "true"     | ProductActivationConfig.ENABLED_INACTIVE
+    "inactive" | "1"        | ProductActivationConfig.ENABLED_INACTIVE
+    "false"    | null       | ProductActivationConfig.FULLY_DISABLED
+    "false"    | ""         | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "inactive" | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "false"    | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "0"        | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "invalid"  | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "true"     | ProductActivationConfig.FULLY_DISABLED
+    "false"    | "1"        | ProductActivationConfig.FULLY_DISABLED
+    "0"        | null       | ProductActivationConfig.FULLY_DISABLED
+    "true"     | null       | ProductActivationConfig.FULLY_ENABLED
+    "true"     | ""         | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "inactive" | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "false"    | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "0"        | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "invalid"  | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "true"     | ProductActivationConfig.FULLY_ENABLED
+    "true"     | "1"        | ProductActivationConfig.FULLY_ENABLED
+    "1"        | null       | ProductActivationConfig.FULLY_ENABLED
+  }
+
   static class ClassThrowsExceptionForValueOfMethod {
     static ClassThrowsExceptionForValueOfMethod valueOf(String ignored) {
       throw new Throwable()