I know this was accepted on the proposal, but looking at the implementation details, and some notes on performance I added below, I'm wondering whether the function should use HttpUrl instead. That way we could get pretty much the same performance benefits as with Uri, while still keeping this as a Kotlin module

HttpUrl is from the okhttp module, which the API module is not using. We cannot use it unless we add the dependency there. It’s the same issue that was with Uri, which required Android dependencies in a pure Kotlin module.

Just thinking out loud, but if in the future we want to better orchestrate all types of request interception, we might want to avoid each implementation performing the same URL manipulations, as a way to improve performance

I agree with that. But we need to take into account that we will need to add other dependencies to the API module. As in this case we’re using strings for the urls, but we’re converting them here because we cannot pass them as HttpUrl/ Uri becase the API module doesn’t have Android/ OkHttp dependencies.

Why do we need this toString again? If the URL is malformed, requestUrl.toHttpUrlOrNull() should've returned null, which already short-circuits

The requestUrl.toHttpUrlOrNull() is used to normalise the request URL. The requirement is that the request URL should be normalised first, so protocol and domain parts are always considered to be lowercase. That function is used to handle that. However, the regex matchins requires a string, that’s why I am calling that again.

Why do we need this?

Entries must be for eTLD+1 domains, other entries will be ignored. I need to make sure that the entry is in that format, because we don’t support subdomains in the entries. example.com is a valid entry, while some.subdomain.example.com should be skipped.

At this point, we have already validated the domain, so could we use com.duckduckgo.app.browser.Domain? That will allow us to use a more performant implementation of sameOrSubdomain later on. Otherwise, we would convert this back to Uri, so we can get the domain from it

I won’t really help us, becasue the domain in this case if the one from the request. We take it like this:

val httpUrl = requestUrl.toHttpUrlOrNull() ?: return false
val requestDomain = httpUrl.topPrivateDomain() ?: return false

sameOrSubdomain is used to check if the document url is the same or a subdomain of the domans that the rule needs to apply for.

Non-atomic map update creates brief blocklist gap

Medium Severity

The blockedRequests.clear() followed by blockedRequests.putAll(newBlockedRequests) is not atomic. Between these two calls, any concurrent containedInBlocklist invocation will see an empty map and return false, allowing requests that are in the blocklist to pass through. Using a volatile reference swap (e.g., AtomicReference<Map<...>>) instead of mutating a ConcurrentHashMap in place would avoid this window.

The blocklist is processed at app launch or afer the privacy config is downloaded, this shouldn’t be a real issue.

Shouldn't this be a String (or a Regex)?

Rules have the following format, so we cannot use String or Regex, because the parsing would fail.

"rules": [
                    {
                        "rule": “testing/*.jpg",
                        "domains": ["example.com"],
                        "reason": ["EXAMPLE"]
                    }
                ]

This List<Map<String, @JvmSuppressWildcards Any>>? is used here because this is a Moshi/JSON deserialization model. It represents the raw JSON structure before it's parsed into typed objects.

Each rule in the JSON is an object with varying keys (rule, domains, reason), so Moshi deserializes each one as a Map<String, Any>. The typed conversion happens later in BlocklistRuleEntity.fromJson(map), which validates the keys and casts values to their expected types.

What do we need this manual parsing for?

Also, I think failing on unknown properties is too restrictive and will break older versions if we add a new field. If we really need this, I think we should at least fire a pixel to alert on the situation, but I think we can just ignore unknown fields

The requirement was to skip rules with unknown properties or missing required ones. I discussed with Dave as well and this is how he suggested to handle this. I can add a pixel there so that we know about this situation. But the reason for this was to avoid situations where the rule is accepted but behaves differently on older versions (I guess in scenarios where we would add a new property which should be considered as well when checking if the request is in the blocklist).

Unchecked generic cast on deserialized list elements

Low Severity

The cast as? List<String> on the domains value only checks is List at runtime due to JVM type erasure — it does not verify that elements are actually String. If the config JSON contains non-string values in the domains array, the cast silently succeeds but a ClassCastException will be thrown later when domainMatches iterates the list, bypassing the runCatching in loadToMemory since the error occurs at match time in containedInBlocklist.

_{Please tell me if this was useful or not with a 👍 or 👎.}

This is not a real concern, the same would happen if we would use a data model for parsing instead of using the fromJson method.