Add Security tools : Strict Cookies, Headers, and PHPStan custom rule (…

…#101)

* Add SameSite Cookie attribute

* Add security headers

* Add PHP Stan rule for Route Security checking

.gitignore

@@ -1,3 +1,4 @@
 docker-compose.override.yml
 /.env
-.ssh/
+.ssh/
+.idea/

apps/back/composer.json

@@ -11,6 +11,7 @@
         "doctrine/doctrine-bundle": "^2.7",
         "doctrine/doctrine-migrations-bundle": "^3.2",
         "doctrine/orm": "^2.14",
+        "nelmio/security-bundle": "^3.0",
         "onelogin/php-saml": "^4.1",
         "phpdocumentor/reflection-docblock": "^5.3",
         "phpstan/phpdoc-parser": "^1.13",

apps/back/config/bundles.php

@@ -10,4 +10,5 @@
     Symfony\Bundle\TwigBundle\TwigBundle::class => ['all' => true],
     DAMA\DoctrineTestBundle\DAMADoctrineTestBundle::class => ['test' => true],
     Doctrine\Bundle\FixturesBundle\DoctrineFixturesBundle::class => ['dev' => true, 'test' => true],
+    Nelmio\SecurityBundle\NelmioSecurityBundle::class => ['all' => true],
 ];

apps/back/config/packages/nelmio_security.yaml

@@ -0,0 +1,82 @@
+# config/packages/nelmio_security.yaml
+nelmio_security:
+    # signs/verifies all cookies
+    # FIXME :
+    #   - PHPSESSID cookie is not correctly passed into the signing process, so we cannot use '*'. But additional cookies could be set a signed here
+    #   - see https://github.com/nelmio/NelmioSecurityBundle/issues/154
+    signed_cookie:
+        names: []
+    # prevents framing of the entire site
+    clickjacking:
+        paths:
+            '^/.*': DENY
+        hosts:
+#            - '^foo\.com$'
+#            - '\.example\.org$'
+
+    # prevents redirections outside the website's domain
+    external_redirects:
+        abort: true
+        log: true
+
+    # prevents inline scripts, unsafe eval, external scripts/images/styles/frames, etc
+    csp:
+        hosts: []
+        content_types: []
+        enforce:
+            level1_fallback: false
+            browser_adaptive:
+                enabled: false
+#            report-uri: '%router.request_context.base_url%/nelmio/csp/report'
+            default-src:
+                - 'none'
+            script-src:
+                - 'self'
+            block-all-mixed-content: true # defaults to false, blocks HTTP content over HTTPS transport
+            # upgrade-insecure-requests: true # defaults to false, upgrades HTTP requests to HTTPS transport
+
+    # disables content type sniffing for script resources
+    content_type:
+        nosniff: true
+
+    # forces Microsoft's XSS-Protection with
+    # its block mode
+    xss_protection:
+        enabled: true
+        mode_block: true
+#        report_uri: '%router.request_context.base_url%/nelmio/xss/report'
+
+    # Send a full URL in the ``Referer`` header when performing a same-origin request,
+    # only send the origin of the document to secure destination (HTTPS->HTTPS),
+    # and send no header to a less secure destination (HTTPS->HTTP).
+    # If ``strict-origin-when-cross-origin`` is not supported, use ``no-referrer`` policy,
+    # no referrer information is sent along with requests.
+    referrer_policy:
+        enabled: true
+        policies:
+            - 'no-referrer'
+            - 'strict-origin-when-cross-origin'
+
+    # forces HTTPS handling, don't combine with flexible mode
+    # and make sure you have SSL working on your site before enabling this
+    #    forced_ssl:
+    #        hsts_max_age: 2592000 # 30 days
+    #        hsts_subdomains: true
+    #        redirect_status_code: 302 # default, switch to 301 for permanent redirects
+
+    # flexible HTTPS handling, read the detailed config info
+    # and make sure you have SSL working on your site before enabling this
+
+
+when@prod:
+    nelmio_security:
+        # depends if you have a reverse proxy that will handle HTTPS, uncomment if you deploy with ansible for instance
+#        forced_ssl:
+#            hsts_max_age: 2592000 # 30 days
+#            hsts_subdomains: true
+#            redirect_status_code: 302 # default, switch to 301 for permanent redirects
+
+#    Seems unnecessary because we are'nt using any insecure page in prod
+#    flexible_ssl:
+#        cookie_name: auth
+#        unsecured_logout: false

apps/back/phpstan.neon

@@ -21,4 +21,7 @@ includes:
     - vendor/phpstan/phpstan-symfony/extension.neon
     - vendor/phpstan/phpstan-symfony/rules.neon
 
+rules:
+	- App\DevTools\PHPStan\RouteSecurityChecker
+
 ## May have to add static forbidden

apps/back/src/Controller/UserController.php

@@ -8,15 +8,19 @@
 use App\Dto\Request\UpdateUserDto;
 use App\Entity\User;
 use App\Repository\UserRepository;
+use App\Security\Enum\Right;
+use App\Security\Voter\UserVoter;
 use App\UseCase\User\CreateUser;
 use App\UseCase\User\UpdateUser;
 use Doctrine\ORM\EntityManagerInterface;
+use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
 use Symfony\Component\Routing\Annotation\Route;
+use Symfony\Component\Security\Core\Security;
 use Symfony\Component\Security\Http\Attribute\IsGranted;
 
-class UserController
+class UserController extends AbstractController
 {
     public function __construct(
         private readonly EntityManagerInterface $entityManager,
@@ -29,6 +33,7 @@ public function __construct(
     #[Route('/users', name: 'create_user', methods: ['POST'])]
     public function createUser(#[MapRequestPayload] CreateUserDto $userDto): JsonResponse
     {
+        $this->denyAccessUnlessGranted(UserVoter::CREATE_USER);
         $user = $this->createUser->createUser($userDto);
 
         $this->entityManager->flush();
@@ -37,16 +42,17 @@ public function createUser(#[MapRequestPayload] CreateUserDto $userDto): JsonRes
     }
 
     #[Route('/users', name: 'list_users', methods: ['GET'])]
-    public function listUsers(): JsonResponse
+    #[IsGranted(UserVoter::VIEW_ANY_USER)]
+    public function listUsers(\Symfony\Bundle\SecurityBundle\Security $security): JsonResponse
     {
         $users = $this->userRepository->findAll();
 
         return new JsonResponse($users);
     }
 
     #[Route('/users/{id}', name: 'get_user', methods: ['GET'])]
-    #[IsGranted('ROLE_RIGHT_USER_READ')]
-    public function getUser(User $user): JsonResponse
+    #[IsGranted(UserVoter::VIEW_ANY_USER)]
+    public function getUserEntity(User $user): JsonResponse
     {
         return new JsonResponse([
             'id' => $user->getId(),
@@ -55,7 +61,7 @@ public function getUser(User $user): JsonResponse
     }
 
     #[Route('/users/{id}', name: 'update_user', methods: ['PUT'])]
-    #[IsGranted('ROLE_RIGHT_USER_UPDATE')]
+    #[IsGranted(UserVoter::EDIT_ANY_USER, subject: 'user')]
     public function updateUser(User $user, #[MapRequestPayload] UpdateUserDto $userDto): JsonResponse
     {
         $user = $this->updateUser->updateUser($user, $userDto);
@@ -69,7 +75,7 @@ public function updateUser(User $user, #[MapRequestPayload] UpdateUserDto $userD
     }
 
     #[Route('/users/{id}', name: 'delete_user', methods: ['DELETE'])]
-    #[IsGranted('ROLE_RIGHT_USER_DELETE')]
+    #[IsGranted(UserVoter::DELETE_ANY_USER, subject: 'user')]
     public function deleteUser(User $user): JsonResponse
     {
         $this->entityManager->remove($user);

apps/back/src/DevTools/PHPStan/IKnowWhatImDoingThisIsAPublicRoute.php

@@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\DevTools\PHPStan;
+
+use Attribute;
+
+/**
+ * Add this attribute to Routes that can be accessed by ANY ONE
+ * This is generally the case for
+ *   - public website / content
+ *   - Login / Password recovery
+ *   - If you are securing your route elsewhere (firewall, security.yml, custom function, etc.)
+ */
+#[Attribute(Attribute::TARGET_METHOD)]
+class IKnowWhatImDoingThisIsAPublicRoute
+{
+}

apps/back/src/DevTools/PHPStan/RouteSecurityChecker.php

@@ -0,0 +1,178 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\DevTools\PHPStan;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitorAbstract;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\InClassMethodNode;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+use ReflectionAttribute;
+use ReflectionException;
+use ReflectionMethod;
+use Symfony\Component\Routing\Annotation\Route;
+use Symfony\Component\Security\Http\Attribute\IsGranted;
+
+use function array_filter;
+use function array_values;
+
+/** @implements  Rule<InClassMethodNode> */
+class RouteSecurityChecker implements Rule
+{
+    public function getNodeType(): string
+    {
+        return InClassMethodNode::class;
+    }
+
+    /** @inheritdoc */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        \assert($node instanceof InClassMethodNode);
+        $className                        = $scope->getClassReflection()?->getName();
+        $functionName                     = $scope->getFunctionName();
+        if (! $className || ! $functionName) {
+            return [];
+        }
+
+        try {
+            $reflection = new ReflectionMethod($className, $functionName);
+        } catch (ReflectionException) {
+            return [];
+        }
+
+        $attributes = $reflection->getAttributes();
+
+        // Parse only Routes
+        $routeAttribute =  $this->getAttribute(Route::class, $attributes);
+        if (! $routeAttribute) {
+            return [];
+        }
+
+        if ($this->functionAttributesContain(IKnowWhatImDoingThisIsAPublicRoute::class, $attributes)) {
+            return [];
+        }
+
+        $isGrantedAttribute =  $this->getAttribute(IsGranted::class, $attributes);
+
+        // LVL 1 NO PROTECTION AT ALL :: Missing vertical controls : no IsGranted and no denyAccessUnlessGranted (even without subject)
+        if ($isGrantedAttribute === null && ! $this->isDenyAccessUnlessGrantedCalledInRouteFunction($node, false)) {
+            return $this->buildError(
+                sprintf('🛑🔓 SECURITY: Route %s::%s is public !', $className, $functionName),
+                "Add an #[IsGranted] attribute or use the `denyAccessUnlessGranted` function.
+                If you are sure that this route should remain public, add the " . IKnowWhatImDoingThisIsAPublicRoute::class . ' attribute',
+            );
+        }
+
+        if ($this->functionAttributesContain(ThisRouteDoesntNeedAVoter::class, $attributes)){
+            return [];
+        }
+
+        // LVL 2 VERTICAL ACCESS ONLY :: IsGranted is present BUT no voter is called
+        if ($isGrantedAttribute === null && ! $this->isDenyAccessUnlessGrantedCalledInRouteFunction($node, true)) {
+            return $this->buildError(
+                sprintf('🛑🔓 SECURITY: Route %s::%s is insufficiently protected !', $className, $functionName),
+                "Pass the 'subject' argument to the \$this->denyAccessUnlessGranted() call.
+                    If you are sure that this route's protection should only on user's permissions, add a ".ThisRouteDoesntNeedAVoter::class." attribute.",
+            );
+        }
+        if ($isGrantedAttribute !== null){
+            $isGrantedAttributeInstance = $isGrantedAttribute->newInstance();
+            \assert($isGrantedAttributeInstance instanceof IsGranted);
+            if ($isGrantedAttributeInstance->subject === null) {
+                return $this->buildError(
+                    sprintf('🛑🔓 SECURITY: Route %s::%s is insufficiently protected !', $className, $functionName),
+                    "Pass the 'subject' argument to the 'IsGranted' attribute.
+                    If you are sure that this route's protection should only on user's permissions, add a ".ThisRouteDoesntNeedAVoter::class.' attribute.',
+                );
+            }
+        }
+        return [];
+    }
+
+    /** @param array<ReflectionAttribute<object>> $attributes */
+    private function functionAttributesContain(string $attributeClass, array $attributes): bool
+    {
+        return \count($this->getAttributes($attributeClass, $attributes)) > 0;
+    }
+
+    /**
+     * @param array<ReflectionAttribute<object>> $attributes
+     *
+     * @return ReflectionAttribute<object>|null
+     */
+    private function getAttribute(string $attributeClass, array $attributes): ReflectionAttribute|null
+    {
+        $attributes = $this->getAttributes($attributeClass, $attributes);
+
+        return \count($attributes) > 0 ? $attributes[0] : null;
+    }
+
+    /**
+     * @param array<ReflectionAttribute<object>> $attributes
+     *
+     * @return array<ReflectionAttribute<object>>
+     */
+    private function getAttributes(string $attributeClass, array $attributes): array
+    {
+        return array_values(array_filter(
+            $attributes,
+            static fn (ReflectionAttribute $attr) => $attr->getName() === $attributeClass
+        ));
+    }
+
+    /**
+     * @return array<RuleError>
+     */
+    private function buildError(string $message, string $tip): array
+    {
+        return [
+            RuleErrorBuilder::message($message . "\n")
+                ->tip($tip)
+                ->build(),
+        ];
+    }
+
+    private function isDenyAccessUnlessGrantedCalledInRouteFunction(InClassMethodNode $node, bool $requireSubject): bool
+    {
+        $visitor = new class ($requireSubject) extends NodeVisitorAbstract {
+            private bool $isSecurityCheckFunctionCalled = false;
+
+            public function __construct(private readonly bool $requireSubject)
+            {
+            }
+
+            public function enterNode(Node $node): int|null
+            {
+                if (
+                    $node instanceof MethodCall && $node->name instanceof Node\Identifier
+                    && $node->name->toString() === 'denyAccessUnlessGranted'
+                    && (!$this->requireSubject || isset($node->args[1]))
+                ) {
+                    $this->isSecurityCheckFunctionCalled = true;
+
+                    return NodeTraverser::STOP_TRAVERSAL;
+                }
+
+                return null;
+            }
+
+            public function isIsSecurityCheckFunctionCalled(): bool
+            {
+                return $this->isSecurityCheckFunctionCalled;
+            }
+        };
+
+        $traverser = new NodeTraverser();
+        $traverser->addVisitor($visitor);
+
+        $traverser->traverse($node->getOriginalNode()->stmts ?? []);
+
+        return $visitor->isIsSecurityCheckFunctionCalled();
+    }
+}

apps/back/src/DevTools/PHPStan/ThisRouteDoesntNeedAVoter.php

@@ -0,0 +1,17 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\DevTools\PHPStan;
+
+use Attribute;
+
+/**
+ * Add this attribute if the Route is limited to some users with specific permissions, but no horizontal check is needed
+ *  - no "ownership"
+ *  - no user context should allow / deny user from accessing the underlying resources
+ */
+#[Attribute(Attribute::TARGET_METHOD)]
+class ThisRouteDoesntNeedAVoter
+{
+}