[PM.24380] fix: Correct and redact flight recorder hostname on logs#6633
[PM.24380] fix: Correct and redact flight recorder hostname on logs#6633
Conversation
Replacing api url with selfhosted one on toFailure at NetworkResultCall
github-actions
bot
added
app:password-manager
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6633 +/- ##
==========================================
+ Coverage 85.79% 86.42% +0.62%
==========================================
Files 804 789 -15
Lines 57205 56813 -392
Branches 8306 8311 +5
==========================================
+ Hits 49081 49100 +19
+ Misses 5242 4832 -410
+ Partials 2882 2881 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
aj-rosado
changed the title
SaintPatrck
left a comment
SaintPatrck
left a comment
There was a problem hiding this comment.
❓ I recently added logging of hostname to CookieIntercepter. Do those need to be checked/redacted also?
data/src/main/kotlin/com/bitwarden/data/manager/flightrecorder/FlightRecorderWriterImpl.kt
Outdated
Show resolved
Hide resolved
|
|
||
| // 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) { |
| // Match against the defaults from RetrofitsImpl.kt line 111 and EnvironmentUrlDataJson | ||
| val actualHost = if (baseUrlsProvider != null) { | ||
| when (originalUrl.host) { | ||
| "api.bitwarden.com" -> baseUrlsProvider.getBaseApiUrl().toHttpUrlOrNull()?.host |
There was a problem hiding this comment.
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")
}
}
}
}There was a problem hiding this comment.
🤔 The kind of logs that are showing the wrong url are not being catched by the interceptors.
The UnknownHostException is only being returned by the enqueue's callback from Retrofit
There was a problem hiding this comment.
I see, that is pretty annoying. I just don't like the idea of having the baseUrlsProvider in this class, as it breaks our separation of concerns.
I'd be curious if @SaintPatrck has any thoughts about this?
There was a problem hiding this comment.
Apologies for not seeing this earlier.
🤔 I agree that injecting baseUrlsProvider here feels like the wrong layer. Since we're already comparing against hardcoded cloud hostnames, we could skip the provider entirely and just categorize them. If we map *.bitwarden.com → "US", *.bitwarden.eu → "EU", anything else → "SH", self-hosted hostnames never touch the log, the wrong-URL bug disappears, and the class stays clean.
Thoughts?
There was a problem hiding this comment.
That would resolve my worries. Unless there is a desire to get specific base urls?
There was a problem hiding this comment.
I re-read comments on the Jira ticket and it seems like there's a desire to keep the cloud hostnames. If that's still the case, we can just check and map the self-hosted hostnames to [REDACTED] and leave the known ones alone.
There was a problem hiding this comment.
Initial approach was to have a regex that would identify URL patterns and [REDACT] them.
This had one issue, sometimes we are throwing logs with the hostname that is similar to a namespace.
Like on UnknownHostException
java.net.UnknownHostException: Unable to resolve host "vault.qa.bitwarden.pw": No address associated with hostname
and NetworkCookieManager
2026-03-02 13:05:31:871 – DEBUG – NetworkCookieManagerImpl – getCookies(vault.qa.bitwarden.pw): resolved=vault.qa.bitwarden.pw, count=0
We can have a more specific approach to these scenarios, we might have unidentified scenarios that are leaking the hostname and we need to be aware of this on future implementations.
I am not sure if we feel that need to not redact the hostname on cloud or if it was simply a nice to have @vvolkgang
There was a problem hiding this comment.
Knowing if the actual log was a call to our servers or not will help us troubleshoot issues.
we can just check and map the self-hosted hostnames to [REDACTED] and leave the known ones alone.
works although @aj-rosado will know better.
There was a problem hiding this comment.
If we want to keep the cloud domains without the BaseUrlProvide we end up with the wrong URL on the UnknownHostException, as it will always return https://api.bitwarden.com/, I am not sure if we have an alternative to get the correct url on this scenario 🤔
2026-03-04 17:39:47:781 – WARNING – NetworkResultCall – Network Error: https://api.bitwarden.com/accounts/prelogin
java.net.UnknownHostException: Unable to resolve host "vault.qa.bitwarden.pw": No address associated with hostname
There was a problem hiding this comment.
We have a concept of EnvironmentRegion in the app already.
enum class EnvironmentRegion {
UNITED_STATES,
EUROPEAN_UNION,
INTERNAL,
SELF_HOSTED,
}What if we replaced the hostname with [UNITED_STATES] or [SELF_HOST]?
This change is redacting them, so we are fine |
# Conflicts: # data/src/main/kotlin/com/bitwarden/data/manager/di/DataManagerModule.kt # data/src/main/kotlin/com/bitwarden/data/manager/flightrecorder/FlightRecorderWriterImpl.kt
Adding regex validations to HostnameRedactionUtil in order to redact names Changing the NetworkResultCall to get the correct hostname from the Throwable's message
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24380
📔 Objective
This aims two address different scenarios.
BaseUrlInterceptorhaving updated the url. So now we are checking on theNetworkResultCallif we should update the BaseUrl using the existingBaseUrlsProvider