-
Notifications
You must be signed in to change notification settings - Fork 11.8k
Fix Str::isUrl() returning false for single-char domain names (#58538) #58686
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
Fix Str::isUrl() returning false for single-char domain names (#58538) #58686
Conversation
|
The test failing appears unrelated to the code change, must be a flaky test. |
|
Maybe, this would be enough: public static function isUrl($value, array $protocols = [])
{
if (empty($protocols)) {
try {
return Uri::new($value) !== null;
} catch () {
return false;
}
}
// …
}But maybe, this is more like something to be included in #58132 🤔 |
|
@shaedrich what do you think about this approach? All tests pass with it and it hands off validation to League URI instead of doing our massive regex. I believe #58132 will be a larger refactor and land on 13.x branch. public static function isUrl($value, array $protocols = [])
{
if (! is_string($value)) {
return false;
}
try {
$uri = \League\Uri\Uri::new($value);
} catch (\Throwable) {
return false;
}
$scheme = $uri->getScheme();
$host = $uri->getHost();
if ($scheme === null || $host === null || $host === '' || preg_match('/^\.+$/', $host)) {
return false;
}
$protocolList = empty($protocols)
? 'aaa|aaas|about|acap|acct|acd|acr|adiumxtra|adt|afp|afs|aim|amss|android|appdata|apt|ark|attachment|aw|barion|beshare|bitcoin|bitcoincash|blob|bolo|browserext|calculator|callto|cap|cast|casts|chrome|chrome-extension|cid|coap|coap\+tcp|coap\+ws|coaps|coaps\+tcp|coaps\+ws|com-eventbrite-attendee|content|conti|crid|cvs|dab|data|dav|diaspora|dict|did|dis|dlna-playcontainer|dlna-playsingle|dns|dntp|dpp|drm|drop|dtn|dvb|ed2k|elsi|example|facetime|fax|feed|feedready|file|filesystem|finger|first-run-pen-experience|fish|fm|ftp|fuchsia-pkg|geo|gg|git|gizmoproject|go|gopher|graph|gtalk|h323|ham|hcap|hcp|http|https|hxxp|hxxps|hydrazone|iax|icap|icon|im|imap|info|iotdisco|ipn|ipp|ipps|irc|irc6|ircs|iris|iris\.beep|iris\.lwz|iris\.xpc|iris\.xpcs|isostore|itms|jabber|jar|jms|keyparc|lastfm|ldap|ldaps|leaptofrogans|lorawan|lvlt|magnet|mailserver|mailto|maps|market|message|mid|mms|modem|mongodb|moz|ms-access|ms-browser-extension|ms-calculator|ms-drive-to|ms-enrollment|ms-excel|ms-eyecontrolspeech|ms-gamebarservices|ms-gamingoverlay|ms-getoffice|ms-help|ms-infopath|ms-inputapp|ms-lockscreencomponent-config|ms-media-stream-id|ms-mixedrealitycapture|ms-mobileplans|ms-officeapp|ms-people|ms-project|ms-powerpoint|ms-publisher|ms-restoretabcompanion|ms-screenclip|ms-screensketch|ms-search|ms-search-repair|ms-secondary-screen-controller|ms-secondary-screen-setup|ms-settings|ms-settings-airplanemode|ms-settings-bluetooth|ms-settings-camera|ms-settings-cellular|ms-settings-cloudstorage|ms-settings-connectabledevices|ms-settings-displays-topology|ms-settings-emailandaccounts|ms-settings-language|ms-settings-location|ms-settings-lock|ms-settings-nfctransactions|ms-settings-notifications|ms-settings-power|ms-settings-privacy|ms-settings-proximity|ms-settings-screenrotation|ms-settings-wifi|ms-settings-workplace|ms-spd|ms-sttoverlay|ms-transit-to|ms-useractivityset|ms-virtualtouchpad|ms-visio|ms-walk-to|ms-whiteboard|ms-whiteboard-cmd|ms-word|msnim|msrp|msrps|mss|mtqp|mumble|mupdate|mvn|news|nfs|ni|nih|nntp|notes|ocf|oid|onenote|onenote-cmd|opaquelocktoken|openpgp4fpr|pack|palm|paparazzi|payto|pkcs11|platform|pop|pres|prospero|proxy|pwid|psyc|pttp|qb|query|redis|rediss|reload|res|resource|rmi|rsync|rtmfp|rtmp|rtsp|rtsps|rtspu|s3|secondlife|service|session|sftp|sgn|shttp|sieve|simpleledger|sip|sips|skype|smb|sms|smtp|snews|snmp|soap\.beep|soap\.beeps|soldat|spiffe|spotify|ssh|steam|stun|stuns|submit|svn|tag|teamspeak|tel|teliaeid|telnet|tftp|tg|things|thismessage|tip|tn3270|tool|ts3server|turn|turns|tv|udp|unreal|urn|ut2004|v-event|vemmi|ventrilo|videotex|vnc|view-source|wais|webcal|wpid|ws|wss|wtai|wyciwyg|xcon|xcon-userid|xfire|xmlrpc\.beep|xmlrpc\.beeps|xmpp|xri|ymsgr|z39\.50|z39\.50r|z39\.50s'
: implode('|', $protocols);
return preg_match('/^(' . $protocolList . ')$/i', $scheme) > 0;
} |
|
I would use |
|
Alright @shaedrich, I thin we ran this as far as we can using We could go through the trouble of removing the tests that should fail in theory, however this constitutes a breaking change and would have down stream implications for anyone using the existing code. I think we have to just go the regex route for this PR and solve the greater issue in the #58132 PR. I am going to revert the changes for now and we can focus on the regex implementation unless you have any other further ideas. |
This reverts commit 94c1e9d.
|
Yeah, not changing this entirely should be better on a patch release for now |
Fixes #58538
The regex in
Str::isUrl()required a TLD-like suffix after the hostname, causing single-character domain names (e.g.http://l:8000) to fail validation. This cuased errors when using single letter/etc/hostsaliases for local development.This updates the domain regex to match Symfony's
UrlValidator(symfony/symfony#43876), this splits domain matching into three types: punycode domains, multi-level domains, and single-level domains.