-
Notifications
You must be signed in to change notification settings - Fork 942
[PM.24380] fix: Correct and redact flight recorder hostname on logs #6633
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
81fc72d
45c195a
b35e020
78b2985
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package com.bitwarden.network.core | ||
|
|
||
| import com.bitwarden.network.interceptor.BaseUrlsProvider | ||
| import com.bitwarden.network.model.NetworkResult | ||
| import okhttp3.HttpUrl.Companion.toHttpUrlOrNull | ||
| import okhttp3.Request | ||
| import okio.IOException | ||
| import okio.Timeout | ||
|
|
@@ -23,10 +25,12 @@ private const val NO_CONTENT_RESPONSE_CODE: Int = 204 | |
| internal class NetworkResultCall<T>( | ||
| private val backingCall: Call<T>, | ||
| private val successType: Type, | ||
| private val baseUrlsProvider: BaseUrlsProvider? = null, | ||
| ) : Call<NetworkResult<T>> { | ||
| override fun cancel(): Unit = backingCall.cancel() | ||
|
|
||
| override fun clone(): Call<NetworkResult<T>> = NetworkResultCall(backingCall, successType) | ||
| override fun clone(): Call<NetworkResult<T>> = | ||
| NetworkResultCall(backingCall, successType, baseUrlsProvider) | ||
|
|
||
| override fun enqueue(callback: Callback<NetworkResult<T>>): Unit = backingCall.enqueue( | ||
| object : Callback<T> { | ||
|
|
@@ -67,8 +71,32 @@ internal class NetworkResultCall<T>( | |
| fun executeForResult(): NetworkResult<T> = requireNotNull(execute().body()) | ||
|
|
||
| private fun Throwable.toFailure(): NetworkResult<T> { | ||
| // We rebuild the URL without query params, we do not want to log those | ||
| val url = backingCall.request().url.toUrl().run { "$protocol://$authority$path" } | ||
| val originalUrl = backingCall.request().url.toUrl() | ||
|
|
||
| // Check if this is a hardcoded default URL that will be replaced by BaseUrlInterceptor | ||
| // Match against the defaults from RetrofitsImpl.kt line 111 and EnvironmentUrlDataJson | ||
| val actualHost = if (baseUrlsProvider != null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use a |
||
| when (originalUrl.host) { | ||
| "api.bitwarden.com" -> baseUrlsProvider.getBaseApiUrl().toHttpUrlOrNull()?.host | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can avoid all this complexity with an Interceptor that handles the error logging. class NetworkErrorLogInterceptor : Interceptor {
override fun intercept(chain: Interceptor.Chain): Response {
return chain
.proceed(chain.request())
.also {
if (!it.isSuccessful) {
val url = it.request.url.toUrl().run { "$protocol://$authority$path" }
Timber.e("Network Error: $url")
}
}
}
} |
||
| "identity.bitwarden.com" -> baseUrlsProvider.getBaseIdentityUrl() | ||
| .toHttpUrlOrNull()?.host | ||
|
|
||
| "events.bitwarden.com" -> baseUrlsProvider.getBaseEventsUrl() | ||
| .toHttpUrlOrNull()?.host | ||
|
|
||
| else -> null | ||
| } | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| // Rebuild the URL without query params, using actual host if available | ||
| val url = if (actualHost != null) { | ||
| "${originalUrl.protocol}://$actualHost${originalUrl.path}" | ||
| } else { | ||
| "${originalUrl.protocol}://${originalUrl.authority}${originalUrl.path}" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to rebuild here, can't we return |
||
| } | ||
|
|
||
| Timber.w(this, "Network Error: $url") | ||
| return NetworkResult.Failure(this) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package com.bitwarden.network.util | ||
|
|
||
| /** | ||
| * List of official Bitwarden cloud hostnames that are safe to log. | ||
| */ | ||
| private val BITWARDEN_HOSTS = listOf("bitwarden.com", "bitwarden.eu", "bitwarden.pw") | ||
|
|
||
| /** | ||
| * Redacts hostnames in a log message by replacing bare hostnames with [REDACTED_SELF_HOST]. | ||
| * | ||
| * Only redacts hostnames that match [configuredHosts] AND are not official Bitwarden domains. | ||
| * Preserves all Bitwarden domains (including QA/dev environments). | ||
| * | ||
| * @param configuredHosts Set of hostnames from BaseUrlsProvider | ||
| * @return Message with hostnames redacted as [REDACTED_SELF_HOST] | ||
| */ | ||
| fun String.redactHostnamesInMessage(configuredHosts: Set<String>): String = | ||
| configuredHosts.fold(this) { result, hostname -> | ||
| val escapedHostname = Regex.escape(hostname) | ||
| val bareHostnamePattern = Regex("""\b$escapedHostname\b""") | ||
| bareHostnamePattern.replace(result) { hostname.redactIfSelfHosted() } | ||
| } | ||
|
|
||
| private fun String.redactIfSelfHosted(): String { | ||
| val isBitwardenHost = BITWARDEN_HOSTS.any { this.endsWith(it) } | ||
| return if (isBitwardenHost) this else "[REDACTED_SELF_HOST]" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we merge these changes first?
#6616