[ticket/11613] Cookies does not work for netbios domain #1975

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@karan10

karan10 commented Jan 31, 2014

If phpBB is installed on a local network, and his "domain" is only a
netbios name, cookies will not work.

http://tracker.phpbb.com/browse/PHPBB3-11613

PHPBB3-11613

karan
[ticket/11613] Cookies does not work for netbios domain
If phpBB is installed on a local network, and his "domain" is only a
netbios name, cookies will not work.

http://tracker.phpbb.com/browse/PHPBB3-11613

PHPBB3-11613
@@ -1052,7 +1052,7 @@ function set_cookie($name, $cookiedata, $cookietime)
$name_data = rawurlencode($config['cookie_name'] . '_' . $name) . '=' . rawurlencode($cookiedata);
$expire = gmdate('D, d-M-Y H:i:s \\G\\M\\T', $cookietime);
- $domain = (!$config['cookie_domain'] || $config['cookie_domain'] == 'localhost' || $config['cookie_domain'] == '127.0.0.1') ? '' : '; domain=' . $config['cookie_domain'];
+ $domain = (!$config['cookie_domain'] || $config['cookie_domain'] == 'localhost' || $config['cookie_domain'] == '127.0.0.1' || strpos($config['cookie_domain'], '.') === FALSE) ? '' : '; domain=' . $config['cookie_domain'];

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Jan 31, 2014

Contributor

this obsoletes the $config['cookie_domain'] == 'localhost' part?

Also false should be lowercase in phpBB

@nickvergessen

nickvergessen Jan 31, 2014

Contributor

this obsoletes the $config['cookie_domain'] == 'localhost' part?

Also false should be lowercase in phpBB

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Jan 31, 2014

Contributor

Maybe also check for : so ipv6 will work with "domain"

@nickvergessen

nickvergessen Jan 31, 2014

Contributor

Maybe also check for : so ipv6 will work with "domain"

This comment has been minimized.

Show comment Hide comment
@bantu

bantu Jan 31, 2014

Member

Should have a comment explaining the logic.

@bantu

bantu Jan 31, 2014

Member

Should have a comment explaining the logic.

@bantu

This comment has been minimized.

Show comment Hide comment
@bantu

bantu Jan 31, 2014

Member

Patch should possibly be against develop-olympus. Security implications must be considered.

Member

bantu commented Jan 31, 2014

Patch should possibly be against develop-olympus. Security implications must be considered.

@nickvergessen

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Mar 14, 2014

Contributor

Can we also unit test this?

Contributor

nickvergessen commented Mar 14, 2014

Can we also unit test this?

@nickvergessen nickvergessen added this to the 3.0.13-RC1 milestone Mar 14, 2014

@nickvergessen

This comment has been minimized.

Show comment Hide comment
@nickvergessen

nickvergessen Mar 14, 2014

Contributor

And as pre bantu, please make a new PR against develop-olympus

Contributor

nickvergessen commented Mar 14, 2014

And as pre bantu, please make a new PR against develop-olympus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment