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

Logger incorrectly reporting header= filters #1932

Closed
8 tasks done
otksm opened this issue Jan 18, 2022 · 6 comments
Closed
8 tasks done

Logger incorrectly reporting header= filters #1932

otksm opened this issue Jan 18, 2022 · 6 comments
Labels
bug Something isn't working fixed issue has been addressed

Comments

@otksm
Copy link

otksm commented Jan 18, 2022

Prerequisites

I tried to reproduce the issue when...

  • uBO is the only extension
  • uBO with default lists/settings
  • using a new, unmodified browser profile

Description

I understand this is still experimental but after upgrading to 1.40.8, filters with a header= option is incorrectly reported as header=[object Object] (literally) in the uBO logger. I also do not see any warning not to open issues about this in the Static filter syntax wiki page.

A specific URL where the issue occurs

Everywhere but https://news.ycombinator.com/ may be a good example.

Steps to Reproduce

  1. Pick a header and add *$script,header={header_name}:{header_value} to My filters.
  2. Open DevTools and the uBO logger and visit the site.
  3. Use DevTools and the logger to confirm that the response body is blocked.
  4. Click on the static filter in the logger to find out where it originates from.

Expected behavior

Logger correctly reports the filter as *$script,header={header_name}:{header_value} and originating from My filters. This worked as expected in 1.39.2.

Actual behavior

Logger reports the filter as *$script,header=[object Object] and returns Static filter could not be found in any of the currently enabled filter lists.

Some other header=-related quirks that I noticed but don't know if you want them filed:

  1. Header name is case-sensitive. This means $header=header_name will only match header_name but not Header_Name. This is problematic because some sites (e.g. https://news.ycombinator.com) capitalize the first letter of each word in their headers.
  2. Regex can only be applied to the header_value field but not header_name or header_name:header_value. My current workaround is to add two filters, one capitalized and one non-capitalized.
  3. Logger also returns Static filter could not be found in any of the currently enabled filter lists when there are two regex in one filter, for example /^regex$/$script,header=header_name:/^header_value$/,_.
  4. Again I understand the header= filter option is still experimental, but should this ever move to stable status, it would be nice to have some sort of indication (a different shade/color?) in the uBO logger that only the response is blocked. Currently it looks like (understandably) that a network request is not blocked and then very quickly blocked in the logger.

I can provide a potential use case if you need one to proceed.

uBlock Origin version

1.40.8

Browser name and version

Firefox 96.0.1

Operating System and version

N/A

gorhill added a commit to gorhill/uBlock that referenced this issue Jan 18, 2022
@gorhill
Copy link
Member

gorhill commented Jan 18, 2022

Thanks for reporting. Regression from refactoring work in 1.40.0.

@gwarser gwarser added bug Something isn't working fixed issue has been addressed labels Jan 18, 2022
@gwarser gwarser closed this as completed Jan 18, 2022
@gwarser gwarser reopened this Jan 18, 2022
@gwarser gwarser added something to address something to address and removed fixed issue has been addressed labels Jan 18, 2022
@gwarser
Copy link

gwarser commented Jan 18, 2022

Point 3:

  • this is fine /news\.ycombinator\.com/$header=Server:/nginx/,domain=ycombinator.com
  • this too /news\.ycombinator\.com/$domain=ycombinator.com,header=Server:/nginx/,_
  • this one cannot be found /news\.ycombinator\.com/$header=Server:/nginx/,_

@gorhill
Copy link
Member

gorhill commented Jan 28, 2022

this one cannot be found /news\.ycombinator\.com/$header=Server:/nginx/,_

This is not specific to header option, this also occurs for removeparam:

/news\.ycombinator\.com/$removeparam=/server=nginx/,_

This happens because the _ noop option is dropped, it has no internal representation, and when the filter is "decompiled" from its internal representation at logging time, there is no ,_ added to the canonical representation.

For now I will improve the parser to have it better guess that /news\.ycombinator\.com/$removeparam=/server=nginx/ as a whole is not to be treated as the pattern for a regex-based filter -- we can assume that when $ is followed by ~?[\w]+[^\$]*$, surely the whole thing is not a regex-based pattern. Maybe this will allow to fully drop the need to use _ to solve ambiguity.

gorhill added a commit to gorhill/uBlock that referenced this issue Jan 28, 2022
@otksm
Copy link
Author

otksm commented Feb 19, 2022

More regarding point 1 (and by extension, point 2):

https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2

Just as in HTTP/1.x, header field names are strings of ASCII characters that are compared in a case-insensitive fashion. However, header field names MUST be converted to lowercase prior to their encoding in HTTP/2. A request or response containing uppercase header field names MUST be treated as malformed (Section 8.1.2.6).

So it would seem like only websites served through HTTP <= 1.x are affected by point 1. Maybe this in itself is a useful narrowing option (e.g. targeted scripts were only served
through HTTP/2 websites) and therefore not worth fixing?

gorhill added a commit to gorhill/uBlock that referenced this issue Jul 14, 2023
Also completed fix for reverse lookup issues related to `header=`
filter option:
uBlockOrigin/uBlock-issues#1932
@gorhill
Copy link
Member

gorhill commented Dec 4, 2023

this one cannot be found /news\.ycombinator\.com/$header=Server:/nginx/,_

I can't reproduce this with latest dev build, probably fixed at some point while improving static filtering parser.

@gwarser
Copy link

gwarser commented Dec 4, 2023

Fixed in last commit linked gorhill/uBlock@aa6baf9

@gwarser gwarser added fixed issue has been addressed and removed something to address something to address labels Dec 4, 2023
@gwarser gwarser closed this as completed Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

3 participants