search

guillaumebriday / laravel-blog

Laravel 11.0 blog application with Hotwire, Horizon, Telescope, Sanctum, Breeze and Pusher
https://laravel-blog.guillaumebriday.fr
MIT License
1.75k stars 572 forks source link

Remove alpha_dash validation restriction on User->name #37

Closed tompenzer closed 6 years ago

tompenzer commented 6 years ago

I can't figure out why there's an alpha-dash restriction on user names. Having full names with spaces seems fine. I've removed the alpha_dash rules in both the following places without incident:

https://github.com/guillaumebriday/laravel-blog/blob/master/app/Http/Requests/Admin/UsersRequest.php#L25

https://github.com/guillaumebriday/laravel-blog/blob/master/app/Http/Requests/UsersRequest.php#L25

Is there a reason not to allow spaces?

guillaumebriday commented 6 years ago

Hey !

I set alpha_dash because I think it's fine to prevent specials chars in the name. Spaces are not allowed with alpha_dash but it's not that bad because most of the time there is dashes in name instead of spaces.

tompenzer commented 6 years ago

But is there a problem caused by not having this rule?

guillaumebriday commented 6 years ago

There is no technical problem, I just want to prevent specials chars from a name 👍

tompenzer commented 6 years ago

I'm personally fine with that aspect, since my name is blessedly special-character-free, but that seems a bit mean to those unfortunates who do have them, and have to figure this out for themselves. Personally, my main issue was not being able to put full names with spaces, and it's my impression that blog authors often use their full names.

tompenzer commented 6 years ago

I guess my view might be biased since I use the project mainly with self-registration disabled, and this is a kind of mitigation for XSS attacks if you make use of those usernames anywhere without proper {{}}/e() escaping. To me, that would seem like a steep usability price to pay, and I'd tend to just treat it like untrusted user input and not attempt to sanitize it to this extent.

tompenzer commented 6 years ago

Oh, one thing I just realized is that in my use case, I really only care about admin UserRequest rules allowing special characters (since the admin interface is the only way to create users with self-registration disabled), and it could stay in the non-admin UserRequest rules, and still address the concern from this issue:

https://github.com/guillaumebriday/laravel-blog/issues/19

guillaumebriday commented 6 years ago

Ok I see your point. I think a good compromise could be to extend Validation with an alpha_dash_spaces maybe ?

tompenzer commented 6 years ago

Oh, yeah! That would be a good improvement. I wasn't aware of that option.

You might even consider having that in the normal UserRequest rules, and absent from Admin/UserRequest rules, since admins can be trusted to not enter malicious strings, if you're comfortable with the normal edit profile form throwing an error if those users try to use it. I would assume there are many languages where common names will fail alpha_dash_spaces.

tompenzer commented 6 years ago

I'm not seeing alpha_dash_spaces listed as a standard Laravel validation rule ( https://laravel.com/docs/5.6/validation#rule-alpha-dash ). For something as personal and important as names, if you want to be internationalization-friendly, it might be best to use a blacklist of undesired characters rather than a whitelist of acceptable ones. That said, for English and most French names, /[a-z\- ]+/i is probably a decent shortcut (et Jérôme alors).

Edit: Just tested; accents work fine with alpha/alpha-dash validation. The same can't be said for the regex.

Edit2: Inspired by this hint ( https://stackoverflow.com/a/6068190 ), I believe the ideal regex would be /[\pL\pN\-_ ]+/ (allowing numbers and underscores too, which I think would be fine).

Edit3: Looks like the Laravel/Illuminate devs had a similar idea: https://github.com/illuminate/validation/blob/master/Concerns/ValidatesAttributes.php#L290 (\pM being Unicode characters with a 'mark' property http://php.net/manual/en/regexp.reference.unicode.php ). I'd just make a rule with a space (or \pZ to keep in the theme of Unicode-compliance) added to their regex (I think either before the dash, or you need to add a backslash before the dash) and call it a wrap. Demo: https://regex101.com/r/UPqUAq/1 (and with allowance for apostrophe for names that contain them: https://regex101.com/r/UPqUAq/3 or even with tight control over the spaces: https://regex101.com/r/UPqUAq/10 )

tompenzer commented 6 years ago

If you're interested, I've successfully tested adding some custom rules you might find more appropriate for the purpose of user names in my project fork, and I'd be happy to submit a PR to merge them back upstream:

https://github.com/tompenzer/penzone/commit/b721f78f4e3dfc479c6a171086ef4e1651798c88

I've looked into how best to address this scenario within the Laravel ecosystem, and it would appear unlikely to me for such a feature PR to get accepted into Illuminate/Validation without an extensive set of tests and justifications. I'll maybe explore creating a small composer package for these rules, but pending that, this seems like a decent way to go about it.

guillaumebriday commented 6 years ago

yeah we need to create a custom validator for this use case.

Your AlphaName one looks like what I wanted to do.

I will propose this PR if I find free time !

If you want to submit a PR for AlphaName laravel-blog, it could be awesome !

Thanks for your contribution

guillaumebriday commented 6 years ago

Done here : https://github.com/guillaumebriday/laravel-blog/pull/39