Added $to= option matching. - #716
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| None, | ||
| ) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[opengrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon
Please consider an alternative approach that avoids this security concern, or request a review from the sec-team on slack.
| ::flatbuffers::Vector<'a, ::flatbuffers::ForwardsUOffset<NetworkFilter>>, | ||
| >>(NetworkFilterList::VT_OPT_TO_DOMAINS_MAP_VALUES, None) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
reported by reviewdog 🐶
[opengrep] Detected 'unsafe' usage, please audit for secure usage
Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage
Cc @thypon
Please consider an alternative approach that avoids this security concern, or request a review from the sec-team on slack.
There was a problem hiding this comment.
Pull request overview
This PR adds full support for $to= option matching by bucketing filters on destination-hostname hashes (in addition to existing $domain=/$from= source-domain bucketing), and updates the serialized data format accordingly.
Changes:
- Implement
$to=token selection/bucketing and apply$to=constraints during request matching (including negated$to). - Extend the FlatBuffers schema and on-disk data format (version bump) to store
$to=-bucketed filter maps. - Add/adjust unit tests to cover
$to=tokenization and matching behavior, and update serialization hash expectations.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/filters/network.rs | Adds unit tests validating $to= tokenization and token-bucketing precedence. |
| tests/unit/filters/network_matchers.rs | Updates matcher tests to assert $to= is enforced against destination hostnames (incl. negation and $from+$to). |
| tests/unit/engine.rs | Updates expected serialization hashes due to schema/data changes. |
| src/request.rs | Adds destination-hostname hash list (hostname_hashes) and shared label-hash helper for source/destination. |
| src/optimizer.rs | Prevents pattern-based optimization for filters with $to= constraints. |
| src/network_filter_list.rs | Adds $to= token map lookup path before falling back to pattern buckets; adjusts match token plumbing. |
| src/flatbuffers/fb_network_filter.fbs | Extends NetworkFilterList with required $to= map index/values vectors. |
| src/flatbuffers/fb_network_filter_generated.rs | Regenerates FlatBuffers bindings to include $to= map fields (required). |
| src/filters/network.rs | Adds FilterTokens::OptToDomains and $to= participation in token selection and filter IDs. |
| src/filters/network_matchers.rs | Generalizes domain-inclusion/exclusion checks to accept hostname-hash slices and reuses them for $to=. |
| src/filters/fb_network.rs | Plumbs $to= include/exclude domain checks into FlatBuffer-backed filter matching. |
| src/filters/fb_network_builder.rs | Adds $to= bucket map builder and includes it in serialized NetworkFilterList. |
| src/data_format/mod.rs | Bumps data format version to 8 for the new schema/layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Rust Benchmark
Details
| Benchmark suite | Current: 5dda050 | Previous: 1aa280e | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
2148424065 ns/iter (± 11082447) |
2015539804 ns/iter (± 5714298) |
1.07 |
rule-match-first-request/brave-list |
1381950 ns/iter (± 7478) |
1390727 ns/iter (± 10519) |
0.99 |
blocker_new/brave-list |
113347094 ns/iter (± 888457) |
110250613 ns/iter (± 192940) |
1.03 |
blocker_new/brave-list-deserialize |
30099947 ns/iter (± 102974) |
30112355 ns/iter (± 105763) |
1.00 |
memory-usage-final/brave-list-initial |
9580730 B/iter (± 0) |
9561338 B/iter (± 0) |
1.00 |
memory-usage-final/brave-list-1000-requests |
3184590 B/iter (± 0) |
3183886 B/iter (± 0) |
1.00 |
memory-usage-max/brave-list-initial/max |
32089418 B/iter (± 0) |
32070026 B/iter (± 0) |
1.00 |
memory-usage-alloc-count/brave-list-initial/alloc-count |
284314 allocs/iter (± 0) |
284089 allocs/iter (± 0) |
1.00 |
memory-usage-alloc-count/brave-list-1000-requests/alloc-count |
81664 allocs/iter (± 0) |
80628 allocs/iter (± 0) |
1.01 |
url_cosmetic_resources/brave-list |
185300 ns/iter (± 711) |
184701 ns/iter (± 1096) |
1.00 |
cosmetic-class-id-match/brave-list |
3197595 ns/iter (± 850957) |
3236255 ns/iter (± 847078) |
0.99 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 20d01b3 | Previous: 1aa280e | Ratio |
|---|---|---|---|
rule-match-browserlike/brave-list |
2225661491 ns/iter (± 11029973) |
2015539804 ns/iter (± 5714298) |
1.10 |
blocker_new/brave-list-deserialize |
33555668 ns/iter (± 69021) |
30112355 ns/iter (± 105763) |
1.11 |
This comment was automatically generated by workflow using github-action-benchmark.
20d01b3 to
ebcc71c
Compare
| if filter.is_badfilter() { | ||
| // Note: `get_id()` doesn't include BAD_FILTER bit. | ||
| self.bad_filter_ids.insert(filter.get_id()); | ||
| // Exclude-only `$to=~` is rejected at parse; guard here for non-parse construction. |
There was a problem hiding this comment.
Let's clarify the comment: we skip the rule only if it has only excluded domains in $to=.
| // For now, filters with $to options are parsed but ignored | ||
| // to preserve existing matching behavior. | ||
| if filter.has_to_option() { | ||
| if filter.is_badfilter() { |
There was a problem hiding this comment.
Did we reorder this intentionally?
| } | ||
| if to_only_broad { | ||
| let mut tokens_buffer = TokensBuffer::default(); | ||
| match filter.get_tokens(&mut tokens_buffer) { |
There was a problem hiding this comment.
I believe something wrong here. We shouldn't rely on get_tokens there (and it's an expensive thing).
Fix #365