From 16133cc067ef4f296594016a6026d1916b83cabc Mon Sep 17 00:00:00 2001 From: Valentin Pertuisot Date: Thu, 23 Apr 2026 14:50:37 +0200 Subject: [PATCH] Enforce schema character set on vital.name in Feature Operation APIs The backend enforces [\w.@$-]* on vital.name via the vital-common-schema facet-path rule; until now the Android SDK only checked for blank names. This change validates the character set in featureOperationArgumentsValid: blank names are dropped with a WARN, names that fail the regex warn but are still emitted (backend is source of truth on the policy). operationKey has no schema character-set rule and is left untouched. Added VALID_OPERATION_NAME_REGEX and FO_ERROR_INVALID_NAME_CHARACTERS message. --- .../rum/internal/monitor/DatadogRumMonitor.kt | 49 +++++++-- .../internal/monitor/DatadogRumMonitorTest.kt | 101 ++++++++++++++++++ 2 files changed, 143 insertions(+), 7 deletions(-) diff --git a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt index 0b4423ee1a..5b3cc089b6 100644 --- a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt +++ b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt @@ -754,22 +754,38 @@ internal class DatadogRumMonitor( sdkCore.internalLogger.reportFeatureOperationApiUsage(ActionType.FAIL) } - private fun featureOperationArgumentsValid(name: String, operationKey: String?) = when { - name.isBlank() -> { + private fun featureOperationArgumentsValid(name: String, operationKey: String?): Boolean { + // Blank / empty names are rejected: the backend rejects them with + // its own non-empty precondition before evaluating the character-set + // regex, so drop client-side to match. + if (name.isBlank()) { sdkCore.internalLogger.logToUser(InternalLogger.Level.WARN) { FO_ERROR_INVALID_NAME.format(Locale.US, name) } - false + return false } - operationKey?.isBlank() == true -> { + // Names that fail the backend's `[\w.@$-]*` character-set regex + // trigger a warning but the event is still emitted — the backend is + // the source of truth, so client-side drop would force a customer + // SDK bump if the rule is ever relaxed. + @Suppress( + "UnsafeThirdPartyFunctionCall" + ) // Regex is a compile-time constant; matches() cannot throw on a non-null input + val nameMatchesBackendPattern = VALID_OPERATION_NAME_REGEX.matches(name) + if (!nameMatchesBackendPattern) { sdkCore.internalLogger.logToUser(InternalLogger.Level.WARN) { - FO_ERROR_INVALID_OPERATION_KEY.format(Locale.US, operationKey) + FO_ERROR_INVALID_NAME_CHARACTERS.format(Locale.US, name) } - false } - else -> true + val operationKeyIsBlank = operationKey?.isBlank() == true + if (operationKeyIsBlank) { + sdkCore.internalLogger.logToUser(InternalLogger.Level.WARN) { + FO_ERROR_INVALID_OPERATION_KEY.format(Locale.US, operationKey) + } + } + return !operationKeyIsBlank } // endregion @@ -952,9 +968,28 @@ internal class DatadogRumMonitor( internal const val FO_ERROR_INVALID_NAME = "Feature operation name cannot be an empty or blank string but was \"%s\". Vital event won't be sent." + internal const val FO_ERROR_INVALID_NAME_CHARACTERS = + "Feature operation name \"%s\" does not match the backend-accepted " + + "pattern [\\w.@\$-]* (letters, digits, _ . @ \$ -). The vital event " + + "will still be sent and may be rejected by the backend." + internal const val FO_ERROR_INVALID_OPERATION_KEY = "Feature operation key cannot be an empty or blank string but was \"%s\". Vital event won't be sent." + /** + * Mirrors the backend's server-side `vital.name` validation regex + * `[\w.@$-]*`. Names that do not match generate a developer warning + * but the event is still emitted — the backend is the single source + * of truth, so client-side drop would force a customer SDK bump if + * the policy is ever relaxed. `operationKey` is a separate parameter + * with its own validation. + * + * `\w` in `java.util.regex.Pattern` is ASCII-only (`[A-Za-z0-9_]`) + * unless `UNICODE_CHARACTER_CLASS` is set, which is what the backend + * regex assumes. + */ + internal val VALID_OPERATION_NAME_REGEX = Regex("^[\\w.@\$-]*$") + private fun InternalLogger.reportFeatureOperationApiUsage(actionType: ActionType) = logApiUsage { InternalTelemetryEvent.ApiUsage.AddOperationStepVital(actionType) } diff --git a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt index c7904b0c2e..22a9a7ddbe 100644 --- a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt +++ b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt @@ -53,6 +53,7 @@ import com.datadog.android.rum.internal.instrumentation.insights.InsightsCollect import com.datadog.android.rum.internal.metric.SessionMetricDispatcher import com.datadog.android.rum.internal.metric.slowframes.SlowFramesListener import com.datadog.android.rum.internal.monitor.DatadogRumMonitor.Companion.FO_ERROR_INVALID_NAME +import com.datadog.android.rum.internal.monitor.DatadogRumMonitor.Companion.FO_ERROR_INVALID_NAME_CHARACTERS import com.datadog.android.rum.internal.monitor.DatadogRumMonitor.Companion.FO_ERROR_INVALID_OPERATION_KEY import com.datadog.android.rum.internal.startup.RumAppStartupTelemetryReporter import com.datadog.android.rum.internal.vitals.VitalMonitor @@ -96,6 +97,7 @@ import org.mockito.kotlin.doAnswer import org.mockito.kotlin.doReturn import org.mockito.kotlin.eq import org.mockito.kotlin.inOrder +import org.mockito.kotlin.isNull import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.same @@ -2982,6 +2984,105 @@ internal class DatadogRumMonitorTest { verifyNoMoreInteractions(mockInternalLogger) } + @OptIn(ExperimentalRumApi::class) + @Test + fun `M warn but still emit W startFeatureOperation { operation name contains invalid character }`() { + // vital.name is documented in _vital-common-schema.json as restricted to + // letters, digits, and - _ . @ $. Names outside that set are warned + // about but still emitted — the backend is the source of truth on the + // policy, so client-side drop would force a customer SDK bump if the + // rule were ever relaxed. + val invalidName = "user login" + + assertMethodCallProducesValidEvent( + whenCalled = { + testedMonitor.startFeatureOperation(invalidName, null, fakeAttributes) + }, + then = { event -> + assertThat(event.name).isEqualTo(invalidName) + } + ) + + mockInternalLogger.verifyLog( + InternalLogger.Level.WARN, + InternalLogger.Target.USER, + FO_ERROR_INVALID_NAME_CHARACTERS.format(Locale.US, invalidName) + ) + } + + @OptIn(ExperimentalRumApi::class) + @Test + fun `M warn but still emit W startFeatureOperation { operation name contains non-ASCII character }`() { + // Pins the ASCII-only semantics of Java Pattern's `\w` (no + // UNICODE_CHARACTER_CLASS flag). Non-ASCII letters must fail the + // character-set regex. If this test ever starts failing because + // "ログイン" is accepted, the regex has gained Unicode semantics — that + // would be a silent behavior change and is caught here. + val invalidName = "ログイン" + + assertMethodCallProducesValidEvent( + whenCalled = { + testedMonitor.startFeatureOperation(invalidName, null, fakeAttributes) + }, + then = { event -> + assertThat(event.name).isEqualTo(invalidName) + } + ) + + mockInternalLogger.verifyLog( + InternalLogger.Level.WARN, + InternalLogger.Target.USER, + FO_ERROR_INVALID_NAME_CHARACTERS.format(Locale.US, invalidName) + ) + } + + @OptIn(ExperimentalRumApi::class) + @Test + fun `M accept name W startFeatureOperation { name only uses schema-allowed characters }`() { + val validNames = listOf( + "login", + "step42", + "login-v2", + "user_login", + "login.v2", + "login@prod", + "login\$1", + "LoginV2", + "login-v2@1.0.0_step\$1" + ) + + validNames.forEach { validName -> + // When + testedMonitor.startFeatureOperation(validName, null, fakeAttributes) + } + + // Then — no user-facing WARN was logged (character-set path never fired) + verify(mockInternalLogger, never()).log( + eq(InternalLogger.Level.WARN), + eq(InternalLogger.Target.USER), + any(), + isNull(), + any(), + isNull() + ) + } + + @OptIn(ExperimentalRumApi::class) + @Test + fun `M not restrict operationKey to name character set W startFeatureOperation`() { + // operation_key has no character-set restriction in the schema. + testedMonitor.startFeatureOperation("login", "session 42 / user foo", fakeAttributes) + + verify(mockInternalLogger, never()).log( + eq(InternalLogger.Level.WARN), + eq(InternalLogger.Target.USER), + any(), + isNull(), + any(), + isNull() + ) + } + @OptIn(ExperimentalRumApi::class) @Test fun `M log user message W startFeatureOperation { operation key is blank }`(