-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(isFQDN): add allow_wildcard
option
#1647
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1647 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 100 100
Lines 1843 1845 +2
=========================================
+ Hits 1843 1845 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure about the validity of this one; will we violating FQDN in principle? Can we leave this to the user to clean out the *.
before sending in the rest to isFQDN
?
@profnandaa I’m using validator.js on backend (NestJs with class-validator that hunder the hood use validator.js) to validate DTO on Rest API. So in my case it’s perfectly valid to let client send domain with wildcard. Alternatively I should use some ugly regex. Even more wildcard FQDN is commonly used e.g. on SSL certificate and Firewall, and i think it can be helpful for such frontend application. I hope this PR will be merged soon. |
@tux-tn -- please have a look and advise. |
@tux-tn @profnandaa any updates here? Would love to see this! |
@profnandaa and @ezkemboi please ignore the reviews requests, were made by mistake. Sorry |
@tux-tn -- could you please have a look at this? |
Why is it difficult to remove this specific pattern? "*.example.com".replace(/^\*\./, "") I hope this doesn't introduce any potential exploits 🤞 |
My intent here is not to sanitize the input, but to validate the input client side. Sanitization eventually comes as a backend stuff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Sorry for the late review 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, will be getting this in. I'm staging the release this weekend for final check off by the maintainers on Monday/Tuesday and we make the release.
Co-authored-by: Andrea Fassina <andrea.fassina@nativery.com>
Co-authored-by: Andrea Fassina <andrea.fassina@nativery.com>
Co-authored-by: Andrea Fassina <andrea.fassina@nativery.com>
Added the
allow_wildcard
option forisFQDN
. Now the validator can allow domain starting with*.
(e.g.*.example.com
or*.shop.example.com
).Fixes #1646
Checklist