Skip to content
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

redirect-url filter option #184

Merged
merged 6 commits into from
Sep 17, 2021
Merged

redirect-url filter option #184

merged 6 commits into from
Sep 17, 2021

Conversation

ShivanKaul
Copy link
Collaborator

@ShivanKaul ShivanKaul commented Sep 2, 2021

Add a new filter option to adblock called redirect-url (similar to redirect=) that allows loading of replacement resources via a URL instead of having to bundle all replacement resources in the client.

src/blocker.rs Outdated Show resolved Hide resolved
src/blocker.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom filter list subscriptions is merged in brave-core now, so there's no way this can make it to the browser unless there's some way to ensure redirect-url rule support can be disabled when parsing a list. I'd recommend adding some kind of ParseOptions struct in src/lists.rs, which can be passed as an argument to a new method on FilterSet, something like add_filter_list_with_opts and a similar add_filters_with_opts. The existing FilterFormat arg can be part of the options struct, as well as a new option to disable any redirect-url rules.

One last consideration that doesn't require any immediate action - I am working in parallel on redirect-rule support, which can provide a redirection for a matching request without causing it to be blocked (i.e. it will only be blocked+redirected if it also matches a separate blocking rule). For now it looks like redirect-url will have to have the same behavior as redirect, implementation-wise, but we should move forward with list-building as if it has the behavior of redirect-rule. redirect is kind of the old way of doing it; the separation of concerns is better with redirect-rule semantics and it also keeps us from having to support something like redirect-url-rule.

Edit: I've posted my redirect-rule implementation plan/progress here.

src/blocker.rs Outdated Show resolved Hide resolved
src/blocker.rs Outdated Show resolved Hide resolved
src/filters/network.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we are almost there.

I can take it from here; I'm going to rebase this on top of #186 and add a few minor tweaks to make the API a little less crufty (i.e. consolidating the buildup of existing constructors and options where possible) since it looks like we're going to have to make a minor version bump with breaking changes anyways.

src/blocker.rs Outdated Show resolved Hide resolved
src/data_format/legacy.rs Outdated Show resolved Hide resolved
src/data_format/legacy.rs Outdated Show resolved Hide resolved
src/data_format/v0.rs Outdated Show resolved Hide resolved
src/filters/network.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lists.rs Outdated Show resolved Hide resolved
src/lists.rs Outdated Show resolved Hide resolved
@ShivanKaul ShivanKaul force-pushed the feature-redirect-url branch 2 times, most recently from 2c3a8c6 to 8a18d61 Compare September 15, 2021 22:12
@ShivanKaul
Copy link
Collaborator Author

@antonok-edm addressed nits and removed mask setting in blocker.rs

@ShivanKaul
Copy link
Collaborator Author

I asked uBO folks what their thoughts were on redirect-url, seems like they understandably don't want to implement because of security concerns: https://www.reddit.com/r/uBlockOrigin/comments/poy9us/interest_in_redirecturl_filter_option/
@antonok-edm for us, we'd only be enabling the redirect-url option (via ParseOptions) when reading the sugarcoat filterlist, correct?

@antonok-edm
Copy link
Collaborator

don't want to implement because of security concerns

Yep, sounds about right. I imagine it'd also be harder for them to use such a thing without any server infrastructure.

we'd only be enabling the redirect-url option (via ParseOptions) when reading the sugarcoat filterlist, correct?

Correct, that's exactly why I wanted the option added!

@uBlock-user
Copy link

uBlock-user commented Sep 16, 2021

@antonok-edm No, because of security/performance issues(amoung others issue/s) raised here - uBlockOrigin/uBlock-issues#46 (comment)

ABP also implemented it and once these issues came to light in the public, they immediately moved forward to remove it.

@antonok-edm
Copy link
Collaborator

@uBlock-user I appreciate the context. Our $redirect-url implementation is a little bit different from $rewrite though - it's a redirect to a single fixed URL (i.e. no regexes or other runtime computations/modifications), and at least for now we're only allowing this to be used in Brave lists (any rules using it will get filtered out from 3rd party lists).

We plan on using this for very similar use-cases to redirect, except that the resources are autogenerated privacy-preserving alternatives to scripts observed on real websites. This should be a solid improvement in cases where bad scripts need exceptions to prevent sites from breaking. In practice we've already used this approach to stub out ~100MB of scripts, and they may need to be updated occasionally to keep up with website changes; it doesn't make sense to ship all of these resources to every user.

Move parse URL logic to network filter instead of blocker
@antonok-edm antonok-edm merged commit b7f29af into master Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants