Improve auth_browseruid() #3268
Conversation
As discussed in https://forum.dokuwiki.org/d/18284-dont-store-ip-addresses/5 - remove the deprecated HTTP_ACCEPT_CHARSET - add HTTP_ACCEPT_LANGUAGE - add HTTP_ACCEPT_ENCODING - add HTTP_ACCEPT - use half of the IP address - add support for IPv6 - use SHA256 instead of MD5 Also: - remove `$uid = strtolower($uid)`, as it doesn't seem to help
The Accept header changes based on requested resource type, so it is not suited for auth_browseruid().
|
Some issues with this:
|
phy25
left a comment
phy25
left a comment
There was a problem hiding this comment.
For Accept-Encoding - "SDCH compression was removed from Google Chrome, and other Chromium products, in version 59 (2017-06-05)" https://en.wikipedia.org/wiki/SDCH. So I guess that's probably fine?
For the other two I am not quite sure. There's https://www.dokuwiki.org/browser which hasn't been updated for a long time.
| $uid .= $INPUT->server->str('HTTP_ACCEPT_LANGUAGE'); | ||
| $uid .= $INPUT->server->str('HTTP_ACCEPT_ENCODING'); | ||
| // convert IP string to packed binary representation | ||
| $pip = inet_pton($ip); |
There was a problem hiding this comment.
https://www.php.net/manual/en/function.inet-pton.php says this can also return false or throw error if IPv6 support is not enabled. $ip comes from clientIP which also reads from HTTP header so the IP address might be invalid. Could we make sure these are addressed?
There was a problem hiding this comment.
If clientIP() can't get a valid IP address, it returns '0.0.0.0'. But even if $ip is somehow invalid, inet_pton just returns false, which doesn't cause problems with the code as it is. false is treated as a 0-length string.
This leaves us with the issue of IPv6 support. There's a comment in the PHP documentation which says:
If you are receiving an "Unrecognized address" error for an IPv6 address, it's possible your version of PHP has not been compiled with IPv6 support.
I looked into it, this is actually a warning, after which inet_pton returns false and execution continues. However, this warning was deemed superfluous and removed from PHP in 2018: php/php-src@658a23a
So the worst case scenario is someone from an IPv6 address visiting a server with an outdated version of PHP compiled without IPv6 support. In this rare situation, if execution gets this far, the warning would be logged, but nothing would break (at least not here). So I would leave the pull request as it is, I don't think it's worth adding code to supress the warning. But I'll do it if you disagree.
As for the browser compatibility stuff, I'm waiting for your decision. https://www.dokuwiki.org/browser says IE9 or higher:
DokuWiki developers take care to make DokuWiki usable in Internet Explorer 9 or higher. But not all features may be available in IE. Certain design elements (eg. rounded corners) or advanced functionally (like drag'n'drop uploading multiple files) may be missing when using Internet Explorer.
(Last changed in 2018)
3 participants
As discussed in https://forum.dokuwiki.org/d/18284-dont-store-ip-addresses/5
add HTTP_ACCEPT- changes based on requested resource type, so not suited for auth_browseruid()Also:
$uid = strtolower($uid), as it doesn't seem to help