Skip to content

Enforce schema character set on vital.name in Feature Operation APIs #3393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Valpertui wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Enforce schema character set on vital.name in Feature Operation APIs #3393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions ...android-rum/src/main/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Contributor

@aleksandr-gringauz aleksandr-gringauz Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could instead do:

val nameMatchesBackendPattern = name.matches(VALID_OPERATION_NAME_REGEX)

This way you don't have to suppress the warning, because String.matches is already in detekt_custom_safe_calls.yml.

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
Expand Down Expand Up @@ -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)
}
Expand Down
101 changes: 101 additions & 0 deletions ...oid-rum/src/test/kotlin/com/datadog/android/rum/internal/monitor/DatadogRumMonitorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<RumRawEvent.StartFeatureOperation>(
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<RumRawEvent.StartFeatureOperation>(
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 }`(
Expand Down
Loading