Implement scriptlet download and storage#8557
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
231d816 to
d08fbc3
Compare
5e042be to
476d1a9
Compare
476d1a9 to
f113371
Compare
7213cde to
3b6160c
Compare
3b6160c to
f23bf2f
Compare
cmonfortep
left a comment
There was a problem hiding this comment.
Great work @CrisBarreiro
Just some comments, but overall this is super well done and solid!
| @Toggle.DefaultValue(DefaultFeatureValue.FALSE) | ||
| /** | ||
| * @return `true` when the feature is operational. | ||
| * When false, feature is still accessible, but not working as expected. |
There was a problem hiding this comment.
Can we clarify what means "not working as expected"?
There was a problem hiding this comment.
Sure, will do
For reference, this is the contingency path, so we'd want to still show the settings entry, but show some contingency messaging to the user
|
|
||
| @Toggle.DefaultValue(DefaultFeatureValue.FALSE) | ||
| /** | ||
| * Kill-switch. When false, the feature is completely inaccessible. |
There was a problem hiding this comment.
is it a kill switch? I assume this is the one we will use for phase rollout? It seems the real kill switch is self(), but this one serve us more for FF and rollout. Just to confirm this.
There was a problem hiding this comment.
It's a kill-switch. There are 3 different relevant flags here:
self-> If enabled, we can inject the scripts (provided the user settings allow us to). If disabled, we won't inject the scripts, and we'll show contingency messaging insteadisDiscoverable-> This is mainly meant to be used during development, so we can hide the entry from settings. I decided to use a sub-feature because then it's easier to remove it laterenabledByDefault-> This is the one we'll use for the rollout
Phase 1, we'll enable both self and isDiscoverable. We can also perform a gradual rollout on isDiscoverable for extra safety
Then, if all looks good, we'll enable enabledByDefault as a gradual rollout
|
|
||
| @ContributesBinding(AppScope::class) | ||
| class RealScriptletDownloader @Inject constructor( | ||
| @Named("nonCaching") private val okHttpClient: Lazy<OkHttpClient>, |
There was a problem hiding this comment.
Can we double check this is actually expected? Mostly to be sure the BE can hold the load on requests here. Do they offer cache policy in their response?
There was a problem hiding this comment.
We'll only try to refresh the scripts when privacy config has changed, the feature status allows for it, and there's a new version, so we won't be calling the endpoint that frequently. I disabled caching here because we're already effectively caching the response in the database, so this would be redundant.
The only edge case I can think of is #8557 (comment), but other than that, I don't see any other issues
f23bf2f to
5142d91
Compare
5142d91 to
8a3a2a0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87c9397. Configure here.
| private fun buildJsonAdapter(): JsonAdapter<AdBlockingExtensionSettings> { | ||
| val moshi = Moshi.Builder().add(KotlinJsonAdapterFactory()).build() | ||
| return moshi.adapter(AdBlockingExtensionSettings::class.java) | ||
| } |
There was a problem hiding this comment.
Identical buildJsonAdapter() method duplicated three times
Low Severity
The buildJsonAdapter() method — creating a Moshi instance with KotlinJsonAdapterFactory() and building an AdBlockingExtensionSettings adapter — is identically duplicated in ScriptletDownloadWorker, ScriptletDownloadWorkerScheduler, and RealAdBlockingExtensionConfigProvider. Each copy allocates its own Moshi instance. Extracting this into a shared provider or companion function would reduce maintenance burden and risk of inconsistent future changes.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 87c9397. Configure here.



Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1214486375443736?focus=true
Description
Steps to test this PR
Tip
Set logcat filter
package:mine tag~:"RealScriptletUpdater|ScriptletDownloadWorker|AdBlockingExtensionProvider|RealPrivacyConfigDownloader|ScriptletSignatureValidator|RealAdBlockingExtensionConfigProvider"Feature 1 - RC flag disabled
onPrivacyConfigDownloadedad_blocking_extension.dbdoesn't existFeature 2 - RC flag disabled with settings
onPrivacyConfigDownloadedad_blocking_extension.dbdoesn't existFeature 3 - Kill-switch on, operational flag off (feature is discoverable but dormant)
Feature 4 - Both flags enabled (full operational path)
Feature 5 - RC flag disabled with settings
onPrivacyConfigDownloadedad_blocking_extension.dbdoesn't existPatches
Patch 1 - update privacy-config
UI changes
n/a
Note
Medium Risk
Adds new background download + signature verification and persists content to a new Room database, so failures could impact update reliability and introduces new network/storage paths. Feature is gated by remote toggles (kill-switch/contingency) which reduces blast radius but the crypto/WorkManager logic is still moderately risk-prone.
Overview
Implements end-to-end remote scriptlet update for the ad-blocking extension: parses settings from remote config, schedules a
WorkManagerjob when the feature is discoverable (and not in contingency mode), downloads scriptlets, validates them via ECDSA signature + UTF-8 checks, and stores the resulting versioned scriptlets.Adds a new Room-backed store (
ad_blocking_extension.db) with DAO/repository plumbing and DI wiring, plus new module dependencies and comprehensive unit tests for config parsing, worker scheduling/execution, updating logic, and signature validation. Also wires:ad-blocking-implinto the mainappmodule build.Reviewed by Cursor Bugbot for commit 87c9397. Bugbot is set up for automated code reviews on this repo. Configure here.