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

Match whole strings instead of partial strings when using patterns #13

Closed
aravindanve opened this issue Sep 24, 2021 · 4 comments
Closed

Comments

@aravindanve
Copy link

I feel it would be better if patterns matched only whole strings instead of partial strings. For example^m[0-9]+$ instead of m[0-9]+. Because the following harmless names end up failing validation:

momentum1
kalashnikov1
bmx1
ghost1
humans1

This could be remedied by changing the pattern list to:

[
    "^db[0-9]+$",
    "^dc[0-9]+$",
    "^dev[0-9]+$",
    "^dns[0-9]+$",
    "^ftp[0-9]+$",
    "^host[0-9]+$",
    "^m[0-9]+$",
    "^mail[0-9]+$",
    "^mx[0-9]+$",
    "^ns[0-9]+$",
    "^server-[0-9]+$",
    "^server[0-9]+$",
    "^smtp[0-9]+$",
    "^static[0-9]+$",
    "^test[0-9]+$",
    "^v[0-9]+$",
    "^vpn[0-9]+$",
    "^web[0-9]+$",
    "^ww[0-9]+$"
]

Should I send a PR?

@jedireza
Copy link
Owner

jedireza commented Sep 24, 2021

I see what you mean and agree that the names you mentioned seem harmless.

I'm sure part of the original motivation for some of these is to prevent spoofing. Let's say these sub-domains are used on a blog platform, where the site can be fully customized. A user could create a subdomain like ftp-downloads.prodvider.ext and create an official looking website as the primary domain and go phishing.

Maybe instead of matching the whole string we matched on boundaries. So the mx rule for example... maybe the pattern shouldn't start with or be prefixed by an underscore or dash?

/(^|-|_)mx[0-9]+/.test('www-mx1') // true
/(^|-|_)mx[0-9]+/.test('bmx1') // false
/(^|-|_)mx[0-9]+/.test('mx1') // true

Now I also see that the rules are using + instead of * for the number ranges. This should probably be updated:

/(^|-|_)ftp[0-9]+/.test('ftp') // false -- this should be true
/(^|-|_)ftp[0-9]*/.test('ftp') // true -- uses * instead

@aravindanve
Copy link
Author

Okay, that seems acceptable. Maybe the same boundary rule can be applied to the end too?

/(^|-|_)ftp[0-9]*($|-|_)/

Because with something like /(^|-|_)m[0-9]*/ any domain starting with m would fail validation

@jedireza
Copy link
Owner

Sounds reasonable. Ideally a PR addressing these things would also introduce tests so expected behavior is kept in check. The changes we've discussed would technically be a breaking change (major version bump) even if the API doesn't change. But with that said, this might be a good time to review the API ergonomics/idioms, considering TypeScript, etc...

Thanks for getting involved in the project.

@aravindanve
Copy link
Author

Yes, I think a way to add names and patterns to the bundled list would be nice. Apart from that, I also noticed that auto-import did not work for me on vscode. I'm not sure why, maybe it doesn't work for certain kinds of exports? This needs to be investigated further.

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

2 participants