Configurable CORS for Api.php

[sasin91]

Noticed there was a question on the help_bar regarding CORS support for clients consuming the API endpoints.

This feature adds support for defining domains including wildcard matching and restricting allowed headers & http methods in the config/config.php file.

[DaFa66]

Thank you sasin91 for your solution. However, I'm a little concerned about the security can of worms you may have opened here.

1. Allowing All Origins ('*'): This can be risky, especially if the API handles sensitive data or operations. It enables any external domain to access the API, which can lead to Cross-Site Request Forgery (CSRF) and other attacks.

2. Wildcard Handling: The code attempts to handle wildcard origins, but this can be complex and error-prone. Incorrect handling can unintentionally expose the API to unintended origins.

3. Credentials (CORS_ALLOWED_CREDENTIALS): Allowing credentials with '' for origins is problematic. If Access-Control-Allow-Origin is set to '', Access-Control-Allow-Credentials cannot be true as per the CORS specification. This setting can lead to security vulnerabilities by exposing user credentials to all origins.

What do you think about:

config.php
define('CORS_ENABLED', true);
define('CORS_ALLOWED_ORIGINS', 'https://trusteddomain.com, https://subdomain.trusteddomain.com');
define('CORS_ALLOWED_METHODS', 'GET, POST, PUT, DELETE, OPTIONS');
define('CORS_ALLOWED_HEADERS', 'X-Requested-With, Content-Type, Origin, Authorization, Accept, Client-Security-Token, Accept-Encoding');
define('CORS_ALLOWED_CREDENTIALS', true);

Use of Environment Variable:

// Example of using environment variables for CORS configuration
define('CORS_ENABLED', getenv('CORS_ENABLED') === 'true' ? true : false);
define('CORS_ALLOWED_ORIGINS', getenv('CORS_ALLOWED_ORIGINS'));
define('CORS_ALLOWED_METHODS', getenv('CORS_ALLOWED_METHODS'));
define('CORS_ALLOWED_HEADERS', getenv('CORS_ALLOWED_HEADERS'));
define('CORS_ALLOWED_CREDENTIALS', getenv('CORS_ALLOWED_CREDENTIALS') === 'true' ? true : false);

In this example, you would set environment variables (CORS_ENABLED, CORS_ALLOWED_ORIGINS, CORS_ALLOWED_METHODS, CORS_ALLOWED_HEADERS, CORS_ALLOWED_CREDENTIALS) in your server's configuration or .env file for each environment.

There would also be a need to work on wildcard handling to make it more robust and thoroughly tested or even remove wildcards altogether. This is an untested sample block of code from GPT4-o

// Example of improved wildcard handling logic
if (!empty($_SERVER['HTTP_ORIGIN'])) {
    $origin = $_SERVER['HTTP_ORIGIN'];
    $allowedOrigins = explode(',', CORS_ALLOWED_ORIGINS);

    $isOriginAllowed = false;
    foreach ($allowedOrigins as $allowedOrigin) {
        if (strpos($allowedOrigin, '*') === false) {
            // Exact match
            if ($origin === $allowedOrigin) {
                $isOriginAllowed = true;
                break;
            }
        } else {
            // Wildcard match
            $allowedOriginPattern = str_replace('*', '.*', preg_quote($allowedOrigin));
            if (preg_match('/^' . $allowedOriginPattern . '$/', $origin)) {
                $isOriginAllowed = true;
                break;
            }
        }
    }

    if ($isOriginAllowed) {
        header('Access-Control-Allow-Origin: ' . $origin);
    } else {
        header('Access-Control-Allow-Origin: ' . $allowedOrigins[0]);
    }
}

And a suggestion to separate configuration for credentials to ensure that Access-Control-Allow-Credentials is only enabled for trusted origins.

// Example of handling CORS credentials separately
if (defined('CORS_ALLOWED_CREDENTIALS') && CORS_ALLOWED_CREDENTIALS === true) {
    if (defined('CORS_ALLOWED_ORIGINS') && CORS_ALLOWED_ORIGINS !== '*') {
        // Only set credentials header for specific trusted origins
        if (in_array($origin, explode(',', CORS_ALLOWED_ORIGINS))) {
            header('Access-Control-Allow-Credentials: true');
        } else {
            header('Access-Control-Allow-Credentials: false');
        }
    } else {
        header('Access-Control-Allow-Credentials: true');
    }
}

In this example, Access-Control-Allow-Credentials is only set to true when CORS_ALLOWED_CREDENTIALS is enabled and the origin is explicitly listed in CORS_ALLOWED_ORIGINS (if not set to '*'). This ensures credentials are only sent to trusted origins, enhancing security.

[sasin91]

Hi @DaFa66

Thanks for the feedback, i appreciate it. I understand your concern, i had thought of it myself but also noticed that Access-Control-Allow-Origin: * should disable Access-Control-Allow-Credentials. As a remedy, i was tinkering with working around it by setting Access-Control-Allow-Origin: ${$_SERVER['HTTP_ORIGIN']} when given a * but decided not to, as it feels more like a dirty hack.

If specifying a list of domains, ie. http://localhost, http://trongate-framework.test is acceptable i would actually prefer that. Thanks for the suggestion. :)

Regards usage of getenv(...) i think that would be a good idea, however in some cases $_ENV does not receive the defined vars which seems to be in $_SERVER in those cases, it is likely due to variables_order = "EGPCS" in php.ini missing the preceding E but i'm not entirely sure on that matter. Should we address this with a framework helper or document it in the framework docs? :)

Honestly, i have not had a specific usecase for wildcards myself, so i vote it to be removed unless specifically desired. The reason i included it in the first place, was i noticed it in other frameworks ootb. Personally, i'd rather explicitly define the subdomains :)

[sasin91]

@DaFa66 please take a look after i refactored it into a class :)

[DaFa66]

Cheers Jonas, I'll take a look at what you have done in the next few days. It's a bit crazy here at the moment with work and finding it hard to set aside some time to go over your refactor. Please keep in mind too, that DC will have the final say on whether he accepts this PR. And please be patient, as he is rather busy creating a wonderful addition to the Trongate Framework.

[sasin91]

All good, take your time and likewise to DC. 😊

[trongate]

Thanks for this. This is not a topic that I'm an expert on, so forgive me if I'm slightly less clued up on this topic than I should be.

Anyway... here are my initial thoughts:

The upshot of the pull request appears to be giving users the ability to define how CORS shenanigans behave, by default, within the config file. Even though I understand that this is a feature that some other PHP frameworks have, I don't like the implementation because it puts an IF statement in front of every request that comes in via the API Manager.

This puts the request in the category of a great idea that I'd like to say 'no' to. We have the fastest PHP framework in the world. This is only made possible by the fact that we've said 'no' to lots of perfectly good ideas.

Personally speaking, if I had a particular endpoint or series of endpoints that required the kind of specialized CORS functionality that is alluded to, I'd handle that at the controller level and - specifically - by using the 'before hook' feature that comes with the API manager. Putting an IF statement in front of everything is just what other frameworks do. Trongate is different.

That being said, I think your knowledge of this topic is way above mine. I can see that and I recognise that! I'd really love to take some of the ideas you've come up with here and make them part of a tutorial series - perhaps one that is produced by you or somebody else. Everything that you're suggesting is certainly cool and of value. I like it. I just don't agree with having IF statements in front of everything unless it's an absolutely essential feature.

QUICK HEADS UP: In the not-too-distant future, we're hoping to add localization functionality into the Trongate framework. This will make Trongate a multi-lingual framework. It's something that gets requested constantly and I think it will open up Trongate to a much larger audience. I think building that feature (when the time comes) probably will require having an IF statement put in front of everything. However, it's something that has been getting discussed for about two years and it's an example of a rare situation where it might be acceptable to do such a thing. I mention this only to make you aware of how seriously we protect the framework from a wall of brilliantly written IF statements.

Again, your knowledge is outstanding and you're precisely the kind of person we need on board. I wish there was some mechanism whereby we could all get together (perhaps once a month) to share thoughts on how to move the framework forward.

Take care, my friend, and thank you.

Hail Sasin91!

[sasin91]

I understand your reasoning and see why this wouldn't be desired.

I actually like your idea of adding it on the controllers where it's needed, I think packing Cors as a module would make the most sense. :)

[sasin91]

@trongate thanks for the kind words. 😊 I'd gladly help when it fits in. on topic, getting together to discuss the framework.. something like Discord, Slack, Teams or maybe Zoom or Tuple? 🙂

Do you have something in mind?