Issues in SessionSecurityListener.php - Fixed Issues - Inspection of "Merge pull request #2638 from acrobat/replace-old-..." - Kunstmaan/KunstmaanBundlesCMS - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — 5.3 ( 958546...1cc96e )

by Jeroen

created 2020-03-04 14:43 UTC

EventListener/SessionSecurityListener.php (4 issues)

Labels

Severity

Upgrade to new PHP Analysis Engine

These results are based on our legacy PHP analysis, consider migrating to our new PHP analysis engine instead. Learn more

<?php

namespace Kunstmaan\AdminBundle\EventListener;

use Psr\Log\LoggerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Session\SessionInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\HttpKernelInterface;

class SessionSecurityListener
{
    /**
     * @var LoggerInterface
     */
    private $logger;

    /**
     * @var bool
     */
    private $ipCheck;

    /**
     * @var bool
     */
    private $userAgentCheck;

    /**
     * @var string
     */
    private $ip;

    /**
     * @var string
     */
    private $userAgent;

    /**
     * @param bool            $ipCheck
     * @param bool            $userAgentCheck
     * @param LoggerInterface $logger
     */
    public function __construct($ipCheck, $userAgentCheck, LoggerInterface $logger)
    {
        $this->ipCheck = $ipCheck;
        $this->userAgentCheck = $userAgentCheck;
        $this->logger = $logger;
    }

    /**
     * @param FilterResponseEvent $event
     */
    public function onKernelResponse(FilterResponseEvent $event)
    {
        if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) {
            return;
        }

        // Make sure the ip and user agent is stored in the session
        $request = $event->getRequest();
        if ($request->hasSession() && $request->getSession()->isStarted()) {
            $session = $request->getSession();
            if ($this->ipCheck && !$session->has('kuma_ip')) {
                $session->set('kuma_ip', $this->getIp($request));
            }
            if ($this->userAgentCheck && !$session->has('kuma_ua')) {
                $session->set('kuma_ua', $this->getUserAgent($request));
            }
        }
    }

    /**
     * @param GetResponseEvent $event
     */
    public function onKernelRequest(GetResponseEvent $event)
    {
        if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) {
            return;
        }

        $request = $event->getRequest();
        if ($request->hasSession() && $request->getSession()->isStarted()) {
            $session = $request->getSession();

            // Check that the ip matches
            if ($this->ipCheck && $session->has('kuma_ip') && $session->get('kuma_ip') != $this->getIp($request)) {
                $this->logger->error(sprintf(
                    "Session ip '%s' does not match with request ip '%s', invalidating the current session",
                    $session->get('kuma_ip'),
                    $this->getIp($request)
                ));
                $this->invalidateSession($session, $request);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
            }

            // Check that the user agent matches
            if ($this->userAgentCheck && $session->has('kuma_ua') && $session->get('kuma_ua') != $this->getUserAgent($request)) {
                $this->logger->error(sprintf(
                    "Session user agent '%s' does not match with request user agent '%s', invalidating the current session",
                    $session->get('kuma_ua'),
                    $this->getUserAgent($request)
                ));
                $this->invalidateSession($session, $request);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
            }
        }
    }

    /**
     * @param SessionInterface $session
     * @param Request          $request
     */
    private function invalidateSession(SessionInterface $session, Request $request)
    {
        $session->invalidate();
        $session->set('kuma_ip', $this->getIp($request));
        $session->set('kuma_ua', $this->getUserAgent($request));
    }

    /**
     * @param Request $request
     *
     * @return string
     */
    private function getIp(Request $request)
    {
        if (!$this->ip) {
            $forwarded = $request->server->get('HTTP_X_FORWARDED_FOR');
            if (strlen($forwarded) > 0) {
                $parts = explode(',', $forwarded);
                $parts = array_map('trim', $parts);
                $parts = array_filter($parts);
                if (count($parts) > 0) {
                    $ip = $parts[0];
                }
            }
            if (empty($ip)) {
                $ip = $request->getClientIp();
            }
            $this->ip = $ip;
        }

        return $this->ip;
    }

    /**
     * @param Request $request
     *
     * @return array|string

     */
    private function getUserAgent(Request $request)
    {
        if (!$this->userAgent) {
            $this->userAgent = $request->headers->get('User-Agent');
class Id
{
    public $id;

    public function __construct($id)
    {
        $this->id = $id;
    }

}

class Account
{
    /** @var  Id $id */
    public $id;
}

$account_id = false;

if (starsAreRight()) {
    $account_id = new Id(42);
}

$account = new Account();
if ($account instanceof Id)
{
    $account->id = $account_id;
}
        }

        return $this->userAgent;
    }
}


1			<?php
2
3			namespace Kunstmaan\AdminBundle\EventListener;
4
5			use Psr\Log\LoggerInterface;
6			use Symfony\Component\HttpFoundation\Request;
7			use Symfony\Component\HttpFoundation\Session\SessionInterface;
8			use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
9			use Symfony\Component\HttpKernel\Event\GetResponseEvent;
10			use Symfony\Component\HttpKernel\HttpKernelInterface;
11
12			class SessionSecurityListener
13			{
14			/**
15			* @var LoggerInterface
16			*/
17			private $logger;
18
19			/**
20			* @var bool
21			*/
22			private $ipCheck;
23
24			/**
25			* @var bool
26			*/
27			private $userAgentCheck;
28
29			/**
30			* @var string
31			*/
32			private $ip;
33
34			/**
35			* @var string
36			*/
37			private $userAgent;
38
39			/**
40			* @param bool $ipCheck
41			* @param bool $userAgentCheck
42			* @param LoggerInterface $logger
43			*/
44	3		public function __construct($ipCheck, $userAgentCheck, LoggerInterface $logger)
45			{
46	3		$this->ipCheck = $ipCheck;
47	3		$this->userAgentCheck = $userAgentCheck;
48	3		$this->logger = $logger;
49	3		}
50
51			/**
52			* @param FilterResponseEvent $event
53			*/
54	2		public function onKernelResponse(FilterResponseEvent $event)
55			{
56	2		if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) {
57	1		return;
58			}
59
60			// Make sure the ip and user agent is stored in the session
61	2		$request = $event->getRequest();
62	2		if ($request->hasSession() && $request->getSession()->isStarted()) {
63	2		$session = $request->getSession();
64	2		if ($this->ipCheck && !$session->has('kuma_ip')) {
65	2		$session->set('kuma_ip', $this->getIp($request));
66			}
67	2		if ($this->userAgentCheck && !$session->has('kuma_ua')) {
68	2		$session->set('kuma_ua', $this->getUserAgent($request));
69			}
70			}
71	2		}
72
73			/**
74			* @param GetResponseEvent $event
75			*/
76	1		public function onKernelRequest(GetResponseEvent $event)
77			{
78	1		if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) {
79	1		return;
80			}
81
82	1		$request = $event->getRequest();
83	1		if ($request->hasSession() && $request->getSession()->isStarted()) {
84	1		$session = $request->getSession();
85
86			// Check that the ip matches
87	1	View Code Duplication	if ($this->ipCheck && $session->has('kuma_ip') && $session->get('kuma_ip') != $this->getIp($request)) {
88	1		$this->logger->error(sprintf(
89	1		"Session ip '%s' does not match with request ip '%s', invalidating the current session",
90	1		$session->get('kuma_ip'),
91	1		$this->getIp($request)
92			));
93	1		$this->invalidateSession($session, $request);
			0 ignored issues – show Bug introduced 2017-11-21 06:15 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$session` defined by `$request->getSession()` on line `84` can be `null`; however, `Kunstmaan\AdminBundle\Ev...er::invalidateSession()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
94			}
95
96			// Check that the user agent matches
97	1	View Code Duplication	if ($this->userAgentCheck && $session->has('kuma_ua') && $session->get('kuma_ua') != $this->getUserAgent($request)) {
98	1		$this->logger->error(sprintf(
99	1		"Session user agent '%s' does not match with request user agent '%s', invalidating the current session",
100	1		$session->get('kuma_ua'),
101	1		$this->getUserAgent($request)
102			));
103	1		$this->invalidateSession($session, $request);
			0 ignored issues – show Bug introduced 2017-11-21 06:15 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$session` defined by `$request->getSession()` on line `84` can be `null`; however, `Kunstmaan\AdminBundle\Ev...er::invalidateSession()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
104			}
105			}
106	1		}
107
108			/**
109			* @param SessionInterface $session
110			* @param Request $request
111			*/
112	1		private function invalidateSession(SessionInterface $session, Request $request)
113			{
114	1		$session->invalidate();
115	1		$session->set('kuma_ip', $this->getIp($request));
116	1		$session->set('kuma_ua', $this->getUserAgent($request));
117	1		}
118
119			/**
120			* @param Request $request
121			*
122			* @return string
123			*/
124	3		private function getIp(Request $request)
125			{
126	3		if (!$this->ip) {
127	3		$forwarded = $request->server->get('HTTP_X_FORWARDED_FOR');
128	3		if (strlen($forwarded) > 0) {
129	2		$parts = explode(',', $forwarded);
130	2		$parts = array_map('trim', $parts);
131	2		$parts = array_filter($parts);
132	2		if (count($parts) > 0) {
133	2		$ip = $parts[0];
134			}
135			}
136	3		if (empty($ip)) {
137	1		$ip = $request->getClientIp();
138			}
139	3		$this->ip = $ip;
140			}
141
142	3		return $this->ip;
143			}
144
145			/**
146			* @param Request $request
147			*
148			* @return array\|string
			0 ignored issues – show Documentation introduced 2018-09-26 07:17 UTC by Report Bug Copy Issue Report Show Similar Issues like this Consider making the return type a bit more specific; maybe use `string\|string[]`. This check looks for the generic type `array` as a return type and suggests a more specific type. This type is inferred from the actual code. Loading history...
149			*/
150	3		private function getUserAgent(Request $request)
151			{
152	3		if (!$this->userAgent) {
153	3		$this->userAgent = $request->headers->get('User-Agent');
			0 ignored issues – show Documentation Bug introduced 2018-04-18 13:05 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$request->headers->get('User-Agent')` can also be of type `array<integer,string>`. However, the property `$userAgent` is declared as type `string`. Maybe add an additional type check? Our type inference engine has found a suspicous assignment of a value to a property. This check raises an issue when a value that can be of a mixed type is assigned to a property that is type hinted more strictly. For example, imagine you have a variable `$accountId` that can either hold an Id object or false (if there is no account id yet). Your code now assigns that value to the `id` property of an instance of the `Account` class. This class holds a proper account, so the id value must no longer be false. Either this assignment is in error or a type check should be added for that assignment. class Id { public $id; public function __construct($id) { $this->id = $id; } } class Account { /** @var Id $id */ public $id; } $account_id = false; if (starsAreRight()) { $account_id = new Id(42); } $account = new Account(); if ($account instanceof Id) { $account->id = $account_id; } Loading history...
154			}
155
156	3		return $this->userAgent;
157			}
158			}
159

Kunstmaan / KunstmaanBundlesCMS

Push — 5.3 ( 958546...1cc96e )

EventListener/SessionSecurityListener.php (4 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

Duplication Side-by-Side

Filter issues like