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

Allow using unescaped $ symbol in the value of some special modifiers #131

Open
DandelionSprout opened this issue Mar 21, 2024 · 10 comments
Assignees
Labels
Priority: P4 T: syntax highlighter The issue is related to the syntax highlighting

Comments

@DandelionSprout
Copy link
Member

I noticed this when working on DandelionSprout/adfilt@7f50937 some minutes before I submitted this issue report, where the $ sign in ||www.amazon.$removeparam=/^[a-z_]{1,20}=[a-zA-Z0-9._-]{80,}$/ is shown by the VS syntax as invalid, despite such a setup being relatively important when handling RegEx in general.

image

So I can only presume that a fix of some sort has to be made, so that the $ sign would be shown as not-invalid.

@Alex-302
Copy link
Member

Shouldn't $ be escaped inside a regexp in the rule?

@DandelionSprout
Copy link
Member Author

No, because the $ marks the end of the RegEx line.

@Alex-302
Copy link
Member

Alex-302 commented Mar 21, 2024

I mean when you copy regexp to a rule.
https://github.com/gorhill/uBlock/wiki/Static-filter-syntax - I see \$ in some examples with regexps.

Btw it seems you forgot a directive here after removing the line 48
DandelionSprout/adfilt@ee3faac#diff-11d0fe9cc978acebe3ce9040b1f319079c575ad786eeae23659c15715b360116L88

@DandelionSprout
Copy link
Member Author

\$ is when there's a $ inside the matching text.

$ is for a textline end.

@ameshkov
Copy link
Member

The @Alex-302's point is that $ is generally considered a special character according to the filtering rule syntax and thus it needs to be escaped whenever you are using it in this context.

@scripthunter7 scripthunter7 added the T: syntax highlighter The issue is related to the syntax highlighting label Mar 25, 2024
@Alex-302
Copy link
Member

@ameshkov So there is an incompatibility with uBO here?

@DandelionSprout
Copy link
Member Author

Meshkov does raise a very important question there.

/^parameter=value$/ is not only fully supported in uBO, but my current personal impression is that it's strongly recommended in uBO.

The way you at Team AdGuard above describe it, would require /^parameter=value\$/, which'd be a little odd compared to the semi-standardised RegEx code standards, but also indeed does result in some sort of AG-uBO syntax differences.

DandelionSprout added a commit to DandelionSprout/adfilt that referenced this issue Mar 27, 2024
+ Testing out the still-odd-looking AdGuard syntax recommended in AdguardTeam/VscodeAdblockSyntax#131 + Adding yet another workaround to find actual list errors in uBO more easily.
@ameshkov
Copy link
Member

We should probably add support for this in AGTree/tsurlfilter just for compatibility reasons then.

@scripthunter7
Copy link
Member

@ameshkov If we add support, we need to modify:

  • AGTree for proper parsing in linter & browser extension
  • TMLanguage for proper syntax highlighting

@scripthunter7 scripthunter7 changed the title "$/" in RegEx-Removeparam is incorrectly shown as invalid. Allow using unescaped $ symbol in the value of some special modifiers Mar 27, 2024
@scripthunter7
Copy link
Member

scripthunter7 commented Apr 8, 2024

@ameshkov @Alex-302 There are two other points here to consider:

  1. The regular expression may contain unescaped commas. Normally, we would use such a comma to separate modifiers, this is the reason why AGLint gives the error that /^[a-z_]{1 is not a valid modifier for input $removeparam=/^[a-z_]{1,20}=[a-zA-Z0-9._-]{80,}$/, because AGTree splits it at the first unescaped comma within the regexp.
  2. There are some modifiers, like $domain, where the value can be completely regexp or just a list with some elements regexp. In other words both are $domain=/regex/ and $domain=example.org|/regex/|example.net valid cases.

Tokenization should work in these cases as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P4 T: syntax highlighter The issue is related to the syntax highlighting
Projects
None yet
Development

No branches or pull requests

6 participants