Skip to content

Bug: Cors filter can not be used with Shield's Token filter together #9431

Closed

Description

@yonggang-xiao

PHP Version

8.2

CodeIgniter4 Version

4.5.7

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

No response

What happened?

When using Cors and Token filter together, if token check failed and return in before filter, the Cors after filter can not running.

Image

Image

Image

Steps to Reproduce

  1. install Shield
  2. setup app/Config/Filters.php
public array $filters = [
        'cors' => [
            'before' => ['swagger', 'auth/token', 'api/*'],
            'after' => ['swagger', 'auth/token', 'api/*'],
        ],
        'tokens' => ['before' => ['api/*']],
    ];
  1. using Swagger UI (different url from the api url) to test api with wrong Authorization header

Expected Output

Cors filter should add all headers in before filter, not in after filter.

Anything else?

Cors.php

<?php

declare(strict_types=1);

/**
 * This file is part of CodeIgniter 4 framework.
 *
 * (c) CodeIgniter Foundation <admin@codeigniter.com>
 *
 * For the full copyright and license information, please view
 * the LICENSE file that was distributed with this source code.
 */

namespace CodeIgniter\Filters;

use CodeIgniter\HTTP\Cors as CorsService;
use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\RequestInterface;
use CodeIgniter\HTTP\ResponseInterface;

/**
 * @see \CodeIgniter\Filters\CorsTest
 */
class Cors implements FilterInterface
{
    private ?CorsService $cors = null;

    /**
     * @testTag $config is used for testing purposes only.
     *
     * @param array{
     *      allowedOrigins?: list<string>,
     *      allowedOriginsPatterns?: list<string>,
     *      supportsCredentials?: bool,
     *      allowedHeaders?: list<string>,
     *      exposedHeaders?: list<string>,
     *      allowedMethods?: list<string>,
     *      maxAge?: int,
     *  } $config
     */
    public function __construct(array $config = [])
    {
        if ($config !== []) {
            $this->cors = new CorsService($config);
        }
    }

    /**
     * @param list<string>|null $arguments
     *
     * @return ResponseInterface|string|void
     */
    public function before(RequestInterface $request, $arguments = null)
    {
        if (! $request instanceof IncomingRequest) {
            return;
        }

        $this->createCorsService($arguments);

        if (! $this->cors->isPreflightRequest($request)) {
            return;
        }

        /** @var ResponseInterface $response */
        $response = service('response');

        $response = $this->cors->handlePreflightRequest($request, $response);

        // Always adds `Vary: Access-Control-Request-Method` header for cacheability.
        // If there is an intermediate cache server such as a CDN, if a plain
        // OPTIONS request is sent, it may be cached. But valid preflight requests
        // have this header, so it will be cached separately.
        $response->appendHeader('Vary', 'Access-Control-Request-Method');

        return $response;
    }

    /**
     * @param list<string>|null $arguments
     */
    private function createCorsService(?array $arguments): void
    {
        $this->cors ??= ($arguments === null) ? CorsService::factory()
            : CorsService::factory($arguments[0]);
    }

    /**
     * @param list<string>|null $arguments
     *
     * @return ResponseInterface|void
     */
    public function after(RequestInterface $request, ResponseInterface $response, $arguments = null)
    {
        if (! $request instanceof IncomingRequest) {
            return;
        }

        $this->createCorsService($arguments);

        // Always adds `Vary: Access-Control-Request-Method` header for cacheability.
        // If there is an intermediate cache server such as a CDN, if a plain
        // OPTIONS request is sent, it may be cached. But valid preflight requests
        // have this header, so it will be cached separately.
        if ($request->is('OPTIONS')) {
            $response->appendHeader('Vary', 'Access-Control-Request-Method');
        }

        return $this->cors->addResponseHeaders($request, $response);
    }
}

TokenAuth.php

<?php

declare(strict_types=1);

/**
 * This file is part of CodeIgniter Shield.
 *
 * (c) CodeIgniter Foundation <admin@codeigniter.com>
 *
 * For the full copyright and license information, please view
 * the LICENSE file that was distributed with this source code.
 */

namespace CodeIgniter\Shield\Filters;

use CodeIgniter\Filters\FilterInterface;
use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\RedirectResponse;
use CodeIgniter\HTTP\RequestInterface;
use CodeIgniter\HTTP\Response;
use CodeIgniter\HTTP\ResponseInterface;
use CodeIgniter\Shield\Authentication\Authenticators\AccessTokens;

/**
 * Access Token Authentication Filter.
 *
 * Personal Access Token authentication for web applications.
 */
class TokenAuth implements FilterInterface
{
    /**
     * Do whatever processing this filter needs to do.
     * By default, it should not return anything during
     * normal execution. However, when an abnormal state
     * is found, it should return an instance of
     * CodeIgniter\HTTP\Response. If it does, script
     * execution will end and that Response will be
     * sent back to the client, allowing for error pages,
     * redirects, etc.
     *
     * @param array|null $arguments
     *
     * @return RedirectResponse|void
     */
    public function before(RequestInterface $request, $arguments = null)
    {
        if (! $request instanceof IncomingRequest) {
            return;
        }

        /** @var AccessTokens $authenticator */
        $authenticator = auth('tokens')->getAuthenticator();

        $result = $authenticator->attempt([
            'token' => $request->getHeaderLine(setting('Auth.authenticatorHeader')['tokens'] ?? 'Authorization'),
        ]);

        if (! $result->isOK() || (! empty($arguments) && $result->extraInfo()->tokenCant($arguments[0]))) {
            return service('response')
                ->setStatusCode(Response::HTTP_UNAUTHORIZED)
                ->setJson(['message' => lang('Auth.badToken')]);
        }

        if (setting('Auth.recordActiveDate')) {
            $authenticator->recordActiveDate();
        }

        // Block inactive users when Email Activation is enabled
        $user = $authenticator->getUser();
        if ($user !== null && ! $user->isActivated()) {
            $authenticator->logout();

            return service('response')
                ->setStatusCode(Response::HTTP_FORBIDDEN)
                ->setJson(['message' => lang('Auth.activationBlocked')]);
        }
    }

    /**
     * We don't have anything to do here.
     *
     * @param array|null $arguments
     */
    public function after(RequestInterface $request, ResponseInterface $response, $arguments = null): void
    {
    }
}

Activity

  1. added
    bugVerified issues on the current code behavior or pull requests that will fix them
    on Jan 24, 2025
  2. added
    missing featureReported issue which is not a bug but needs to be implemented
    and removed
    bugVerified issues on the current code behavior or pull requests that will fix them
    on Jan 24, 2025
  3. michalsn commented on Jan 24, 2025

    @michalsn
    Member

    As much as I understand what you want to achieve... this won't be very easy to "fix" as it's not really a bug.

    By design, when we return the ResponseInterface in the before filter the whole execution is finished and we immediately return the response. So the after filters are not called on purpose. If we change this, it could break many existing apps.

    There is a solution for your case. However, it will require you to create a new cors filter. You will have to put it in the $required filter array, and then inside the filter, you will have to check if it should be applied based on the current URL.

    Something like this:

    public function before(RequestInterface $request, $arguments = null)
    {
        if (! str_starts_with($request->getUri()->getPath(), '/api/')) {
            return;
        }
    
        return parent::before($request, $arguments);
    }

    and similar code for the after method. Filters from the required array are always executed.


    There is an option to add support in required filters, for URI path pattern recognition or for except, although we should gather opinions about this first. I will tag this as a "missing feature", although please (whoever is reading this) don't start implementing it yet.

  4. kenjis commented on Jan 25, 2025

    @kenjis
    Member

    There is an option to add support in required filters, for URI path pattern recognition or for except

    I oppose it. Required Filters are special filters that are applied to every request.
    If you need URI path pattern, you should use global filters.

  5. yonggang-xiao commented on Jan 25, 2025

    @yonggang-xiao
    Author

    My question is why in the before filter to process PreflightRequest only, and in the after filter to process normal requests?
    Can we merge the 2 process flows in the before filter?
    So the setup like this:

    public array $filters = [
            'cors' => [
                'before' => ['swagger', 'auth/token', 'api/*'],
            ],
            'tokens' => ['before' => ['api/*']],
        ];
  6. michalsn commented on Jan 25, 2025

    @michalsn
    Member

    @yonggang-xiao Sorry, I missed that part - it should work. As far as I can imagine, it wouldn't be a breaking change either - at least if developers don't rely on headers being added in the after filter. Does anyone see any drawbacks?

  7. michalsn commented on Jan 27, 2025

    @michalsn
    Member

    I'm changing the label back to "bug". Please check the PR: #9437

  8. added
    bugVerified issues on the current code behavior or pull requests that will fix them
    and removed
    missing featureReported issue which is not a bug but needs to be implemented
    on Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugVerified issues on the current code behavior or pull requests that will fix them

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions