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

Redirection issue #3590

Closed
okiehsch opened this issue Mar 9, 2018 · 5 comments
Closed

Redirection issue #3590

okiehsch opened this issue Mar 9, 2018 · 5 comments

Comments

@okiehsch
Copy link
Contributor

okiehsch commented Mar 9, 2018

Describe the issue

The filter

*/content/libs/comscore/comscore.min.js$script,first-party,redirect=noopjs

does not lead to a redirection of the request.

One or more specific URLs where the issue occurs

http://www.whas11.com/article/news/atf-investigating-after-congressional-candidate-cut-apart-ar-15/291-526898428

Steps for anyone to reproduce the issue

  • Disable uBO-unbreak

  • Go to

http://www.whas11.com/article/news/atf-investigating-after-congressional-candidate-cut-apart-ar-15/291-526898428

the video will not play because of the blocked content/libs/comscore/comscore.min.js request.

  • Add */content/libs/comscore/comscore.min.js$script,first-party,redirect=noopjs to your filter list and reload.
    The video will still not play and the logger shows that no redirection has occurred.

  • Add */content/libs/comscore/comscore.min.js$script,~third-party,redirect=noopjs to your filter list and reload.
    The video will play and the logger shows a successful redirection.

Your settings

  • OS/version: Windos 10
  • Browser/version: Chrome 65
  • uBlock Origin version: 1.15.2
Your filter lists

Default

@jspenguin2017
Copy link
Contributor

That is weird. With first-party the redirect= directive is discarded because there is no destination domain. Hold on, let me trace it.

image

@jspenguin2017
Copy link
Contributor

jspenguin2017 commented Mar 10, 2018

redirect-engine.js has a special case that handles first-party but nothing that handles ~third-party:
image

image
image

@okiehsch
Copy link
Contributor Author

Yes, to quote gorhill:

The current behavior is how I reworked redirect over time. Originally there had to be a domain option and a hostname part to the filter (||). Eventually I allowed first-party to be used in place of domain. Again later I allowed a redirect directive to not longer be forced to have a hostname. However the parsing needs to be reworked further it seems, I should not discard the directive if first-party is present without a hostname. A redirect directive is really two things at once: a static filter, and a redirect directive. I need to adjust the parser code for this.

@jspenguin2017
Copy link
Contributor

jspenguin2017 commented Mar 10, 2018

~third-party is ignored, so the second filter would be equivalent to:

*/content/libs/comscore/comscore.min.js$script,redirect=noopjs

Which is probably not intended.

The actual code that handles redirection is probably not aware of request party and needs to be adjusted. The parser is easy to fix (still need to figure out how to flag it as first-party though):

diff --git a/src/js/redirect-engine.js b/src/js/redirect-engine.js
index a7d539c..61ca5bf 100644
--- a/src/js/redirect-engine.js
+++ b/src/js/redirect-engine.js
@@ -318,8 +318,8 @@ RedirectEngine.prototype.compileRuleFromStaticFilter = function(line) {
             srcs = option.slice(7).split('|');
             continue;
         }
-        if ( option === 'first-party' ) {
-            srcs.push(µburi.domainFromHostname(des) || des);
+        if ( option === 'first-party' || option === '~third-party' ) {
+            srcs.push(µburi.domainFromHostname(des) || des || '<flag-first-party>');
             continue;
         }
         // One and only one type must be specified.

gorhill added a commit that referenced this issue Nov 3, 2020
This commit moves the parsing, compiling and enforcement
of the `redirect=` and `redirect-rule=` network filter
options into the static network filtering engine as
modifier options -- just like `csp=` and `queryprune=`.

This solves the two following issues:

- #3590
- uBlockOrigin/uBlock-issues#1008 (comment)

Additionally, `redirect=` option is not longer afflicted
by static network filtering syntax quirks, `redirect=`
filters can be used with any other static filtering
modifier options, can be excepted using `@@` and can be
badfilter-ed.

Since more than one `redirect=` directives could be found
to apply to a single network request, the concept of
redirect priority is introduced.

By default, `redirect=` directives have an implicit
priority of 0. Filter authors can declare an explicit
priority by appending `:[integer]` to the token of the
`redirect=` option, for example:

    ||example.com/*.js$1p,script,redirect=noopjs:100

The priority dictates which redirect token out of many
will be ultimately used. Cases of multiple `redirect=`
directives applying to a single blocked network request
are expected to be rather unlikely.

Explicit redirect priority should be used if and only if
there is a case of redirect ambiguity to solve.
@gorhill
Copy link
Owner

gorhill commented Nov 7, 2020

The redirect=/redirect-rule= options should no longer suffer syntax quirks in latest dev build -- the options are now parsed/compiled and enforced in the static network filtering engine. They can also be excepted just the same way we can except csp= and queryprune= filters -- all these filters are considered modifier filters.

@gorhill gorhill closed this as completed Nov 7, 2020
gorhill added a commit that referenced this issue Nov 28, 2020
Related issue:
- #3590

Since the `redirect=` option was refactored into a modifier
filter, presence of a type (`script`, `xhr`, etc.) is no
longer a requirement.
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

No branches or pull requests

4 participants
@gorhill @jspenguin2017 @okiehsch and others