Skip to content
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

maybe need to modify session garbage collector section. (FileHandler) #1565

Closed
eterv opened this issue Dec 1, 2018 · 10 comments
Closed

maybe need to modify session garbage collector section. (FileHandler) #1565

eterv opened this issue Dec 1, 2018 · 10 comments

Comments

@eterv
Copy link

eterv commented Dec 1, 2018

Actually, I saw similar another issue.
#942
I tested it.
I use php 7.1.

and below is some config value I use.
gc_maxlifetime = 7200 (2 hour)
gc_probability = 50 // (50%)
gc_divisor = 100
sid_bits_per_character = 5 (php auto config)


I think that maybe need to modify session garbage collector section.

Session/Handlers/FileHandler.php
public function gc();

original code is ...

$pattern = sprintf(
'/^%s[0-9a-f]{%d}$/', preg_quote($this->cookieName, '/'), ($this->matchIP === true ? 72 : 40)
);

but, why 40? I can't understand this.

for example, one of my session file name is a_sessionba2o0ekqqv8ogvqonrl2u0uqg86so8fm
a_session is cookiename.
so, session id is "ba2o0ekqqv8ogvqonrl2u0uqg86so8fm". the length of this string is 32.

So, actually, in my case 32 is correct value.
and I found this code part.

in Session/Session.php
protected function configureSidLength();

after be called this function, sidRegexp value is, "[0-9a-v]{32}" (in my case)

So, I tried to modify gc part code.

Here are some code. (Of course, this is incomplete code. I hope CI developer modify it properly)

PHP Code:

   $sid_regexp = '';
    $bits_per_character = (int) (ini_get('session.sid_bits_per_character') !== false ? ini_get('session.sid_bits_per_character') : 4);
    $sid_length = (int) (ini_get('session.sid_length') !== false ? ini_get('session.sid_length') : 40);
    switch ($bits_per_character) {
        case 4: $sid_regexp = '[0-9a-f]'; break;
        case 5: $sid_regexp = '[0-9a-v]'; break;
        case 6: $sid_regexp = '[0-9a-zA-Z,-]'; break;
    }
    $sid_regexp .= '{' . $sid_length . '}';
    $pattern = sprintf( '/^%s%s$/', preg_quote($this->cookieName, '/'), $sid_regexp );

    /* // Original Code
    $pattern = sprintf(
            '/^%s[0-9a-f]{%d}$/', preg_quote($this->cookieName, '/'), ($this->matchIP === true ? 72 : 40)
);*/ 

Anyway, after I modified it, Finally My php Garbage Collector could delete garbage session files.

Actually there is no problem in CI 3 FileHandle gc section.
I think CI3 already used code that similar to above code.

So, please consider it, check this point.

@lonnieezell
Copy link
Member

Your solution seems pretty thorough. I'm all for including this fix. Care to submit a PR for it?

@eterv
Copy link
Author

eterv commented Dec 19, 2018

@lonnieezell Thank you! but I'm just a beginner developer. Actually I don't know how to PR or test a part of code. besides I don't understand that matchIP part.
So, I'm so sorry to bother you. but could you please consider it?

@lonnieezell
Copy link
Member

Who said I understood the matchIP part? lol That's all code from CI3 that I didn't touch.

That's no problem, though, I'm not sure how long it will be until it gets fixed up. Will take a bit of a deep dive into your solution to ensure I understand all of the details, then figure out how to test it - which might be a bit tricky....

@eterv
Copy link
Author

eterv commented Dec 20, 2018

@lonnieezell I agree. it might be not so easy. Anyway I can wait for your work. So, Cheer up!!

@natanfelles
Copy link
Contributor

Please, @narfbg could you argue about it?

I think is was done to solve what today can be done implementing SessionUpdateTimestampHandlerInterface::validateId, that is useful when passing a session id like session_id('not-accepted-id'), but it is used only to sanitize the cookie input in:

if (isset($_COOKIE[$this->sessionCookieName]) && (
! is_string($_COOKIE[$this->sessionCookieName]) || ! preg_match('#\A' . $this->sidRegexp . '\z#', $_COOKIE[$this->sessionCookieName])
)
)
{
unset($_COOKIE[$this->sessionCookieName]);
}
$this->startSession();

Thanks!

@narfbg
Copy link
Collaborator

narfbg commented Jan 6, 2019

OK, I'll argue about it ... with you. :D

This issue is about garbage collection, not input validation.

@natanfelles
Copy link
Contributor

Oh, it is ever an honour argue overthinking things with you! 👍

Then, why is the cookie validated (oh, sorry if "validated" is not the better term)? Looks that, somebody is being logged out because of it.

Means that the issue is something about the number 40...

@narfbg
Copy link
Collaborator

narfbg commented Jan 7, 2019

GC is the mechanism that deletes expired session files.

It can even be turned off and cookies would still need to be validated as there's no connection between the two ... I don't understand why you're asking that.

@natanfelles
Copy link
Contributor

I asked you because the code has a comment authored by you, and I know that you know what to do.

I thought that you could explain some important doubts in the issue. Like, "why 40?". "is important to use the new interfaces or not?", "custom session id works?", "IP?", etc. But, OF COURSE, just if you want (and can) to do it. Otherwise, ignore it, whatever.

@narfbg
Copy link
Collaborator

narfbg commented Jan 8, 2019

OK, I'll answer the why 40 question - originially, all session IDs were SHA1 hashes and 40 characters is the length of a SHA1 hash in hexadecimal form. This is already hinted at in the comment block that you're referring to.

For all the rest of your questions though, I have no idea what you're trying to get at.

Just to save more time for everyone, this entire issue is likely the result of a missing patch from CI3.

lonnieezell added a commit that referenced this issue Jan 23, 2019
Updating session id cleanup for filehandler. Fixes #1681 Fixes #1565
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants