feat: add automatic link sanitization using AdGuard filters#38
feat: add automatic link sanitization using AdGuard filters#38screwys wants to merge 2 commits intoalyraffauf:masterfrom
Conversation
There was a problem hiding this comment.
Hey, thanks for the PR! I have a couple concerns about this approach and have left some comments in my review.
The biggest issue, though, is that the primary method of distribution here is Flatpak, and we don't actually have network access in our permissions. We could add it, though I'm not really keen on it. A vendored approach might make some sense. Either way, it doesn't seem like this was tested adequately in the Flatpak sandbox, which is how most users use the app.
| go func() { | ||
| path := filepath.Join(configDir(), "adguard_filter.txt") | ||
| if info, err := os.Stat(path); err != nil || time.Since(info.ModTime()) > 24*time.Hour { | ||
| if resp, err := http.Get(adguardURL); err == nil && resp.StatusCode == 200 { |
There was a problem hiding this comment.
http.Get doesn't enforce a timeout by default, so there's some risk of this hanging forever. Not a huge deal and probably won't surface but we should set up our own with a timeout, like this:
var httpClient = &http.Client{
Timeout: 10 * time.Second,
}But I have to be honest, I'm not sure about this overall flow, even a hit every 24 hours is pretty aggressive.
There was a problem hiding this comment.
...right, the flatpak doesn't have network permissions by default. I forgot about this even though that was something I found nice with the flatpak, and probably it would be better to respect that. Either way, 24 hours was quite agressive, even a stale version should do the job, at least for my use case. I tried testing the flatpak with bundled adguard filter list , but I am not sure if this looks clean enough, we can go with a simpler filter list too.
| } | ||
|
|
||
| if cfg.SanitizeLinks { | ||
| sanitized = applyTrackingProtection(sanitized) |
There was a problem hiding this comment.
On cold start (so, default behavior) there is a race condition if we need to hit the network for the adguard rules, because it runs in a goroutine and everything else is synchronous. The result is that no rules are applied at all.
| monitor := gio.BaseFileMonitor(monitorIface) | ||
| if monitor != nil { | ||
| monitor.ConnectChanged(func(file, otherFile gio.Filer, eventType gio.FileMonitorEvent) { | ||
| configMonitor = gio.BaseFileMonitor(monitorIface) |
There was a problem hiding this comment.
I'd prefer this be a separate PR, but that's not a blocker.
There was a problem hiding this comment.
I could not reproduce the crash that led me to change that after switching buttons on/off for 2 minutes, so hopefully it should be fine now.
| ShowAppNames bool `toml:"show_app_names"` | ||
| ForceDarkMode bool `toml:"force_dark_mode"` | ||
| StayAlive bool `toml:"stay_alive"` | ||
| SanitizeLinks bool `toml:"sanitize_links"` |
There was a problem hiding this comment.
We use sanitize internally in the code for a variety of things (redirects, now adguard). It might make sense to name the config and user-facing elements something more obviously related to stripping params with adguard
|
Updated. |
2 participants
This PR aims to provide a link sanitization feature by using the Adguard tracker protection list. It is useful for browsers that do not support this natively, in case the user does not want to add/enable extensions.
I want to be transparent that AI was used for the creation of this feature.
monitorwas changed toconfigMonitorto prevent a garbage-collection crash while updating the configuration.