Checking for localhost in cookie domain setting

[sgerlach]

Code change to resolve #546
Check for localhost set domain to false if so, else use the HOST_NAME variable

[digininja]

Looking at the definition for session_set_cookie_params, the default for the domain is null, wouldn't that be better than false for localhost?

session_set_cookie_params(
    int $lifetime_or_options,
    ?string $path = null,
    ?string $domain = null,
    ?bool $secure = null,
    ?bool $httponly = null
): bool

[sgerlach]

I'm unsure about the difference between null and false for this particular function. This example is explicit about false https://www.php.net/manual/en/function.setcookie.php#73107

In testing, setting the value to null does not unset the domain for the cookie again resulting in

Cookie “PHPSESSID” has been rejected for invalid domain.

So it appears that it in fact needs to be false to unset the domain name field. (Or make the default false for the definition) but either way still need the logic check somewhere

[sgerlach]

Actually, the logic check needs a bit more tweaking looking for a dot in the hostname and then falsing if not contained. 'localhost:8080' does not pass the HTTP_HOST == 'localhost' and the real problem is the lack of a dot in the hostname - Updated code committed

[digininja]

How does this work with 127.0.0.1?

[sgerlach]

hmm, more digging is neccesary here - might have to do more with a port number in the hostname if not on a standard port

[digininja]

parse_url https://www.php.net/manual/en/function.parse-url.php might help. You can use that to pull out just the host part.

[sgerlach]

Yes, the port number was the actual problem. Even though you technically should not set localhost as the domain, browsers do accept it. Browsers are not happy about port numbers in the domain name for the cookie and are generally not set for same origin policy. This update pulls the hostname part from the HTTP_HOST variable to set the session cookie. Really only comes into play when using non-standard ports to serve DVWA, like in docker scenarios or other non-privileged high port work. Happy to squash these commits if need be. Thanks for the pointer to parse_url @digininja

[digininja]

I was about to say that you'd removed the localhost test but if you don't need it and more then that's great. I'll give it a quick check later and then accept it.

…

On Tue, 21 Mar 2023, 19:03 Scott Gerlach, ***@***.***> wrote: Yes, the port number was the actual problem. Even though you technically should not set localhost as the domain, browsers do accept it. Browsers are not happy about port numbers in the domain name for the cookie and are generally not set for same origin policy. This update pulls the hostname part from the HTTP_HOST variable to set the session cookie. Really only comes into play when using non-standard ports to serve DVWA, like in docker scenarios or other non-privileged high port work. Happy to squash these commits if need be. Thanks for the pointer to parse_url @digininja <https://github.com/digininja> — Reply to this email directly, view it on GitHub <#547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA4SWJBXXOL7QHOXSLJ72DW5H3O3ANCNFSM6AAAAAAWBRQ7T4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[digininja]

Seems to work, thanks.