kintesh / containerise

Firefox extension to automatically open websites in a container
MIT License
411 stars 55 forks source link

Make regex and glob pattern matching stricter #130

Open bburky opened 4 years ago

bburky commented 4 years ago

I was trying to use glob patterns and ran into one bug and one quirk that made them impossible to use correctly:

Regex metacharacters (such as .) were not escaped in globs (which use regexes for parsing internally):

Patterns do not match the entire domain, even with "match domain only" enabled:

I believe it is more intuitive for patterns to always match the entire string when "match domain only" is enabled.

The first commit fixes the regex metacharacter bug and the second adds anchors when matching patterns with "match domain only" is enabled. The final commit updates the tests (not 100% if the logic is correct with the matchDomainOnly/should/should not testing).

The "match domain only" change is a breaking change. You can drop that commit if you want, but please document the current behavior. The glob example in the README matches many more domains more than just the specified domain.

A weaker change might be to add anchors only when matching glob patterns, not regexes. But note that this is a breaking change too: users might've been using example.com to match subdomains or www.example.com.

If you want the patterns to behave like prefixes it's not sufficient to only add a right $ anchor, because example\.com$ still matches evilexample.com

I'm not sure what the best solution here is.

LoveIsGrief commented 4 years ago

Hi,

Thanks for the contribution. I'll review it when I have time. Two comments I'd like to make beforehand are about

Patterns do not match the entire domain, even with "match domain only" enabled:

I think you're misunderstanding the feature. Normally, a pattern matches the whole URL e.g happy will match https://sad.com/happy. With "Match domain only", sad.com will be used as the target. Therefore @example.com matching evil.example.com.evil.com is expected.

@^example.com$ behaves as expected, but I don't want this to be necessary

That's how regexes work. Without the anchors, they match the whole string.

bburky commented 4 years ago

The main problem is that it is impossible to match the entire domain with a glob. Is that something you want to enable?

I agree it's not required to make regexes behave this way, but I was trying to find any way to use globs like I expected to.

LoveIsGrief commented 4 years ago

The main problem is that it is impossible to match the entire domain with a glob. Is that something you want to enable?

You might need to explain how you got to that conclusion :thinking:

*, *google* and *.com all match the entire google domains (and more). And just to make sure we're talking about the same thing: <protocol>://<domain>/<rest>

https://developer.mozilla.org/en-US/docs/Web/API/URL/hostname

const domain = url.hostname
url.hostname = domain

This is what you mean when you say "domain", right?

I agree it's not required to make regexes behave this way, but I was trying to find any way to use globs like I expected to.

Yes, I'm not disagreeing with your solution for globs.

benyaminl commented 2 years ago

Any update to this? Only need to be merged, one year already past.. yet no update :/

mikedlr commented 1 year ago

@benyaminl think that the maintainer has been busy. I opened an issue to discuss support. Are you able to help?

benyaminl commented 1 year ago

@mikedlr I already move to Tab Groups as it provide all function I need more than containerise, so I will pas this time.