Skip to content
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

Jump to bottom

RUMM-2377 Add checks on intake request headers #1005

Merged
merged 1 commit into from
Aug 16, 2022
Merged

RUMM-2377 Add checks on intake request headers #1005

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 27 additions & 9 deletions dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/net/DataOkHttpUploaderV2.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import okhttp3.RequestBody

internal abstract class DataOkHttpUploaderV2(
internal var intakeUrl: String,
internal val clientToken: String,
internal val source: String,
internal val sdkVersion: String,
rawClientToken: String,
rawSource: String,
rawSdkVersion: String,
internal val callFactory: Call.Factory,
internal val contentType: String,
internal val androidInfoProvider: AndroidInfoProvider,
Expand All @@ -34,17 +34,18 @@ internal abstract class DataOkHttpUploaderV2(

private val uploaderName = javaClass.simpleName

internal val clientToken = if (isValidHeaderValue(rawClientToken)) rawClientToken else ""
internal val source: String = sanitizeHeaderValue(rawSource)
internal val sdkVersion: String = sanitizeHeaderValue(rawSdkVersion)

private val userAgent by lazy {
System.getProperty(SYSTEM_UA).let {
if (it.isNullOrBlank()) {
"Datadog/$sdkVersion " +
sanitizeHeaderValue(System.getProperty(SYSTEM_UA))
Copy link

Choose a reason for hiding this comment

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

Curiosity: Why don't you trust SYSTEM_UA property but you trust your androidInfoProvider, which relays on Build class, which in turns take information from system properties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mostly because the syntax for User Agent can become very sketchy and more likely to have weird characters.
Also the Build.xxx will be already fetched from the app zygote, whereas the UA is retrieved at Runtime, making it possible for the host app to override it.

Copy link

Choose a reason for hiding this comment

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

Got your point. It is a good trade-off.

.ifBlank {
"Datadog/${sanitizeHeaderValue(sdkVersion)} " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

not even sure if the sdkVersion should be sanitized at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sdkVersion will possibly be updatable from crossplatform SDKs, meaning it will be using public APIs. So in theory, it could have an invalid character yes

"(Linux; U; Android ${androidInfoProvider.osVersion}; " +
"${androidInfoProvider.deviceModel} " +
"Build/${androidInfoProvider.deviceBuildId})"
} else {
it
}
}
}

// region DataUploader
Expand Down Expand Up @@ -88,6 +89,9 @@ internal abstract class DataOkHttpUploaderV2(
data: ByteArray,
requestId: String
): UploadStatus {
if (clientToken.isBlank()) {
return UploadStatus.INVALID_TOKEN_ERROR
}
val request = buildRequest(data, requestId)
val call = callFactory.newCall(request)
val response = call.execute()
Expand Down Expand Up @@ -143,6 +147,20 @@ internal abstract class DataOkHttpUploaderV2(
}
}

private fun sanitizeHeaderValue(value: String?): String {
return value?.filter { isValidHeaderValueChar(it) }.orEmpty()
}

private fun isValidHeaderValue(value: String): Boolean {
return value.all { isValidHeaderValueChar(it) }
}

private fun isValidHeaderValueChar(c: Char): Boolean {
return c == '\t' || c in '\u0020' until '\u007F'
}

// endregion

companion object {

const val SYSTEM_UA = "http.agent"
Expand Down