But technically would it be possible to forget phpsessid in the first page view?
Maybe we could enforce loading a second page like login before logging in is accepted.
As for feeds, are we sure phpsessid is used in these? If yes we should certainly ensure it isn't included...
📎 Google Reader (133) - Mozilla Firefox_2012-04-15_08-56-26.png - 40.06 kB, 757x451, viewed 249 times.
📎 Google Reader (133) - Mozilla Firefox_2012-04-15_08-56-08.png - 41.58 kB, 768x457, viewed 246 times.
PHPSESSID is invoked when there isn't an existing Wedge cookie (like the start of every browsing session from a search engine) and that first page view, it will always shove PHPSESSID links everywhere. Remove your cookie, refresh, boom you'll see them.
We actually do, as it happens. You cannot go into login2 if you don't have a valid session. But that's not really the problem.Quote Maybe we could enforce loading a second page like login before logging in is accepted.
Yes, I am sure.Quote As for feeds, are we sure phpsessid is used in these? If yes we should certainly ensure it isn't included...
It's not possible to avoid them, because it still goes through ob_sessrewrite to handle pretty URLs, so the usual logic applies - namely that if no cookie was found, PHPSESSID is injected. And given the approach made by Google Reader etc., they wouldn't have a cookie, so they get PHPSESSIDs, which is what we're seeing here (and I separately validated that PHPSESSID was added)
However, Wedge does it this way... First of all, it determines whether user is a robot. If it is, it will skip adding PHPSESSID to links
So the reason why it broke in GR is that the URLs returned were not prettified.
Of course, there's still the problem of browsers that disable cookies entirely... i.e. if you're logged in and have cookies disabled like me right now, you absolutely need a phpsessid link in the URL.
I'm not sure why, but SMF and Wedge both add "&" at the end of the SID URL... Instead of simply using ";". I'm not sure SMF/Wedge would work *at all* if the installed PHP didn't support ";" as a separator.
(Jean Dujardin voice) Not so fast, Mr. Bond!
As pointed out above, SID isn't injected is $user_info['possibly_robot'] is true, which is always the case for Google Bot. (And probably Google Reader's bot as well.)
📎 Google Reader (11) - Mozilla Firefox_2012-04-15_23-09-36_cr.png - 44.18 kB, 555x465, viewed 272 times.
There is a (valid) argument about rejecting those cases and disallowing it entirely for security reasons.Quote Of course, there's still the problem of browsers that disable cookies entirely... i.e. if you're logged in and have cookies disabled like me right now, you absolutely need a phpsessid link in the URL.
Because & is the argument separator defined in PHP's configuration. Check the code in QueryString, if ; is arg-separator, it does one thing, but otherwise, it does something else to manually parse out the parameters. ; is just not the default. But we get cases, just for fun, of malformed entities being prepared occasionally too.
Seriously, it IS injecting PHPSESSID. It does it on the RSS validator. I have no idea what user agent Google Reader uses but I'm willing to bet it doesn't trip possibly_robot. It's actually irrelevant in this case. It does not matter whether it trips possibly_robot or not, it should NEVER be issuing PHPSESSID in feeds, ever, because some other feed readers will choke on non unique URLs, I know Thunderbird used to have issues with it, for example (because that didn't bother with cookies at one time)
I would be tempted to say that we should remove that compatibility code...
// Are we going to need to parse the ; out?
if (strpos(@ini_get('arg_separator.input'), ';') === false && !empty($_SERVER['QUERY_STRING']))
{
// Get rid of the old one! You don't know where it's been!
$_GET = array();
// Was this redirected? If so, get the REDIRECT_QUERY_STRING.
$_SERVER['QUERY_STRING'] = urldecode(substr($_SERVER['QUERY_STRING'], 0, 5) === 'url=/' ? $_SERVER['REDIRECT_QUERY_STRING'] : $_SERVER['QUERY_STRING']);
// Replace ';' with '&' and '&something&' with '&something=&'. (This is done for compatibility...)
parse_str(preg_replace('/&(\w+)(?=&|$)/', '&$1=', strtr($_SERVER['QUERY_STRING'], array(';?' => '&', ';' => '&', '%00' => '', "\0" => ''))), $_GET);
// Magic quotes still applies with parse_str - so clean it up.
if (function_exists('get_magic_quotes_gpc') && @get_magic_quotes_gpc() != 0 && empty($settings['integrate_magic_quotes']))
$_GET = $removeMagicQuoteFunction($_GET);
}
elseif (strpos(@ini_get('arg_separator.input'), ';') !== false)
{
if (function_exists('get_magic_quotes_gpc') && @get_magic_quotes_gpc() != 0 && empty($settings['integrate_magic_quotes']))
$_GET = $removeMagicQuoteFunction($_GET);
// Search engines will send action=profile%3Bu=1, which confuses PHP.
foreach ($_GET as $k => $v)
{
if (is_string($v) && strpos($k, ';') !== false)
{
$temp = explode(';', $v);
$_GET[$k] = $temp[0];
for ($i = 1, $n = count($temp); $i < $n; $i++)
{
@list ($key, $val) = @explode('=', $temp[$i], 2);
if (!isset($_GET[$key]))
$_GET[$key] = $val;
}
}
// This helps a lot with integration!
if (strpos($k, '?') === 0)
{
$_GET[substr($k, 1)] = $v;
unset($_GET[$k]);
}
}
}And we should have dealt with that long ago. Seriously, I'm surprised we had the problem at all, because if you'll look at noisen.com, the phpsessid is never injected *at all* into feeds...
Added a context variable in Feed.php to say we don't want to insert the session ID. This is tested against in Subs-Template.php. The reason why I was lazy for it, is that (1) just testing for Feedfetcher-Google (or whatever it's called) isn't going to do any good for other feed reader bots, (2) there is VERY little reason to have a session ID in a feed URL
If you have cookies disabled, then your session won't be active forever. Your feed reader will soon end up trying to access an incorrect session ID anyway.
This is (was) because the session ID injector wouldn't correctly identify static cache files as static, and would inject the SID into the names.
As they're not treated by purls, we always end up with a ";" at the end, the one that's injected by the SID test... (Currently '&' in SVN, which is even worse... Or maybe it isn't so? I remember removing a test against & which was probably done for that reason...?)
Then it WILL break.
The default in PHP itself is &, as per http://[url]http://php.net/manual/en/ini.core.php#ini.arg-separator.input[/url] and the choice to use ; is for avoiding all kinds of nightmares. QueryString.php, line 81 onwards.
So we have to leave that code in, unless you plan on fixing every URL to not use ; (and with all the other problems related to it).
SMF and Wedge using ; is a definite oddity though it does solve so many problems.
I can think of multiple reasons why it might be different between the two.
Either way, the bottom line is that it wasn't and while I have patched around it, I'm not happy with it (which is why I haven't yet committed it)
Note that SID containing a & is injected by SMF and Wedge, and I have no idea why ; wasn't used there.
Sure that's the case. Hence having feeds never contain a session id - and also why I was so adamant about fixing the canonical URL link as well, which also got SID added into it.
The whole thing about using SID in URLs is an interesting one and one I've been unwilling to make a move on because I'm inclined to think that having 'probably accurate' stats about the number of guests is probably slightly more important than having an SEO benefit to it (though having a canonical URL should fix most issues)
Actually, it's worse than that. When this topic kicked off and I threw it at the validator, that error it was giving relates to the fact that there were *malformed* entities in the data, namely amp without the trailing ; in it, where injected by this code.
It will break for servers that don't support both @ini_set and the specific setting that is arg_separator...
That code block is about providing compatibility with & (which it doesn't do entirely because it doesn't change links in the HTML afaik..??), and fixing other things.
Actually, as I said, I'd rather we don't provide support for & links... (Well, inside PURLs if anything!)
Committed what...? (I already feel a commit conflict coming...)
Sometimes, you need to re-break things to understand what they tried to fix in the first place.
But how come we can't simply determine that a new session with the same IP is simply the same session...? (Or at least the same person, when it comes to calculating stats...)
Which was due to my currently doing a rtrim() on the final URL and deleting any extra characters like ";"...
Previously, it was first removing & at the end and then removing ; or whatever.
You can't set it with ini_set. It's a PERDIR setting, not an ALL setting. You can only set it via .htaccess if your host allows you to do so (since it's a PHP setup thing, if you do it by ini_set, it's already too late as $_GET will by then already have been processed)
No, no, no it doesn't. Read what the code does. It detects whether arg_separator is ; or not, if it is, it sanitises $_GET based on it, if it's not, it re-parses the query string.
I did the same thing you did, a $context setting to have it never inject SID into URLs. But since you've committed it, I didn't bother (and I wasn't that happy with it, I felt there should have been something more elaborate)
For the same reason IP addresses are unreliable as sin anyway, things like NAT routing, where an IP address does not relate to a unique computer any more nor to a unique user.
Except that I saw cases where the trailing ; was present and cases where it wasn't - in the same page.Quote Which was due to my currently doing a rtrim() on the final URL and deleting any extra characters like ";"...
Previously, it was first removing & at the end and then removing ; or whatever.