LockKey doesn't get prefixed with prefix properly + patch

[hoshsadiq]

The RedisSessionHandler::$lockKey gets prefixed with just the prefix without the colon ($this->prefix.$lockKey), but every other session related sessions get prefixed with $this->prefix.':'.$sessionId). Lock key should also have a colon after the prefix ($this->prefix.':'.$this->lockKey). I've generated a patch to fix this. I'd give you a PR but I'm not sure where to point the PR to. You can apply this by saving it as diff.patch and running: patch -p0 < diff.patch.

This was asked about in #181 as well. There's already a PR to master #158 but this is for 1.1.

Below is the patch:

diff --git Session/Storage/Handler/RedisSessionHandler.php Session/Storage/Handler/RedisSessionHandler.php
index b6ad274..092ebfd 100644
--- Session/Storage/Handler/RedisSessionHandler.php
+++ Session/Storage/Handler/RedisSessionHandler.php
@@ -108,10 +108,10 @@ class RedisSessionHandler implements \SessionHandlerInterface

         $this->lockKey = $sessionId.'.lock';
         for ($i=0;$i<$attempts;$i++) {
-            $success = $this->redis->setnx($this->prefix.$this->lockKey, '1');
+            $success = $this->redis->setnx($this->getLockKey(), '1');
             if ($success) {
                 $this->locked = true;
-                $this->redis->expire($this->prefix.$this->lockKey, $this->lockMaxWait + 1);
+                $this->redis->expire($this->getLockKey(), $this->lockMaxWait + 1);
                 return true;
             }
             usleep($this->spinLockWait);
@@ -122,7 +122,7 @@ class RedisSessionHandler implements \SessionHandlerInterface

     private function unlockSession()
     {
-        $this->redis->del($this->prefix.$this->lockKey);
+        $this->redis->del($this->getLockKey());
         $this->locked = false;
     }

@@ -213,6 +213,11 @@ class RedisSessionHandler implements \SessionHandlerInterface
         return $this->prefix . ':' . $sessionId;
     }

+    public function getLockKey()
+    {
+        return $this->getRedisKey($this->lockKey);
+    }
+
     /**
      * Destructor
      */

[curry684]

@Seldaek I think this "issue" still exists after #404 right?

[Seldaek]

Yes I believe it does.

[curry684]

@hoshsadiq given that we released 3.0.0-RC2 and gearing towards 3.0.0 I think we should consider pushing this fix in there still. If you want to make the PR you'd only need to target master now as 2.x is heading for deprecation the day we get 3.x out there.

[B-Galati]

@curry684 @Seldaek I think we should deprecate this feature in v3 in favor of the Symfony redis handler and then update the doc to explain how to use symfony redis session handler. What do you think?

I'll do the proposed patch.

[curry684]

Makes sense.

[B-Galati]

What do yout think @dkarlovi?

[dkarlovi]

If you're asking about

I think we should deprecate this feature in v3 in favor of the Symfony redis handler

Sounds good to me, the handler there should work just as well.

Ironically, I'm not using it even though I wrote it since this bundle provides direct access to the Redis client which I needed explicitly so switched the session handler too. :)

If some issues come up, I pledge to fix them in Symfony. So, from me, 👍

[B-Galati]

Thanks a lot @dkarlovi!

I am now wondering what's the difference between ours and the one you implemented in Symfony. It looks like it's all about the lock behavior for write and read but I don't get why it's needed. Could be to mitigate some security issue?

[dkarlovi]

Session locks were added later IIRC, by @nicolas-grekas, it's a feature for all Symfony's session handlers, don't know the specifics.

Overall, the handler from Symfony is trivial, but works and is used: some people already fixed a few small bugs since it was added. You should be pretty safe to use it, IMO.

[B-Galati]

It looks like session locking was not implemented yet. See symfony/symfony#4976.
It has been recently implemented in phpredis extension but not compatible with cluster mode.
It looks like it's something really tricky :D.
I guess we could contribute our handler lock logic to Symfony before deprecating ours.

[curry684]

I think it's pretty pointless to do it on our end while it should be pretty much Symfony native behavior.

[B-Galati]

@curry684 Yes, what do you think about contributing a lock logic to the Symfony RedisSessionHandler?

[curry684]

Sounds like a lot of work :D But yeah I think we should eventually do it as it's likely the most used non-file session handler in Symfony.

[curry684]

Is fixed in 3.0.