neomerx / cors-psr7 Goto Github PK

• Improve performance and caching support;
• Improve support in IE #31;
• Refactor;
• Newer PHPUnit;
• Add strong typing, min PHP 7.1.

When requests are sent from locally stored files their Origin header would be file://

Currently CORS analyzer throws exception that such URL cannot be parsed.

Analyzer should better write to logs that such headers cannot be parsed and return AnalysisResultInterface::TYPE_REQUEST_OUT_OF_CORS_SCOPE

• Origin not allowed
• Header/method not allowed
• etc

Call global function such as array_merge as \array_merge which improves performance a bit.

Default parameters include a few small arrays. It's a small but possible performance tuning.

Trying to setup package in PHP-DI container and seems like I've found small mistake in init method.

Second setAllowedOrigins([]) overwrites areAllOriginsAllowed to false value, then enableAllOriginsAllowed() makes no sense one line before.

I am trying to implement this as a middleware on Silex Framework (version 2). Silex by default does not support PSR 7 request and responses, so I use DiactorosFactory Library to convert the Symfony Foundation Request/Response to & from PSR 7 Request and Response.

In the CORS Settings, I have something like this

->setServerOrigin([
    'scheme' => 'http',
    'host'   => 'www.something.test',
    'port'   => 80,
])

And the check fails even when the call is made to the host that is configured above i.e. www.something.test.

I am making a preflight request to http://www.something.test and when it comes to Analyzer.php's isSameHost method $host get's value of www.something.test and $hostURL which should be parsed object, has host property as null.

This is the because internally it uses php's function parse_url to parse url if it is string.

and after parsing www.something.test returns array which looks like

array(..) {
  ["path"]=>
  string(5) "www.something.test"
}

and thus does not have enough information to match the host.

I am not sure why

$host where which returns www.something.test passed to createParsedURL as it does not have enough data to properly parse all the request.

Would it be possible to tag an 1.1 release?

This is because versions before 1.0.4 throw an error with PHP 7.2 because of assert('is_string($url) || is_array($url)'); in ParsedUrl.php. Currently only way of ignoring everything below 1.0.4 would be to manually list all the minor releases.

"neomerx/cors-psr7": "1.0.4|1.0.5|1.0.6|1.0.7|1.0.8|1.0.9|1.0.11|1.0.12|1.0.13",

This not optimal and requires manually updating composer.json on my side always when neomerx/cors-psr7 has a new release.

Related to neomerx/cors-illuminate#10

The idea is when handling same origin (aka non-CORS) request to have in logs something like

Request is not CORS (request origin is empty or equals to server one). {"request":null,"server":"[object] (Neomerx\\Cors\\Http\\ParsedUrl: http://localhost:8888)"}

As configuration of allowed headers might be frustrating process (send request => fail => change conf => send request => next header is not allowed => change conf, frustration => ...) it might be useful to have option for all headers allowed '*'. As it's insecure it should not be enabled by default. But it could be an option for development.

The idea in influenced by neomerx/cors-illuminate#11

• Travis config
• License notices
• etc

Add logs and ability to turn it on/off with

• Psr\Log\LoggerInterface
• LoggerAwareInterface

Debug for overall app logic and info for important decisions such as request deny.

Related to neomerx/cors-illuminate#8

Hi

Thanks for an awesome library

Due to the use of static:: in

Because the class constants are private they can't be accessed from the child class. And when you extend the analyzer it will automatically try to use the child scope when using static:: so it would work if you used self:: in the Analyzer class.

Would you be open to a pull-request to fix this?

They have changed something and logo has become broken. Also, 2017 could be changed to 2018 as well.

PHP minimum to 5.6 as 5.5 no longer supported
PHP Unit also move to 5.7 as 4.x branch is also not supported

Not sure how it plays out with HHVM as this platform is not supported by PHP Unit.

Are you planning to create a factory for Settings?

For example:

(new SettingsFactory)->createFromArray([
    'foo' => 'bar',
    'bar' => 'baz',
    'baz' => 'qux',
]);

I would like not to write this:

https://github.com/sunrise-php/awesome-skeleton/blob/master/src/Middleware/CorsMiddleware.php#L80

What are your thoughts on this?

When the request header 'Host' value is in form of Host: example.com (without port) the parse_url method isn't capable of parsing the url correctly.

Feeding a url without a prefixed '' to parse_url will result in a non discovered domain name if the port is not present.

But I think a Host header in this form should parseable as url - or am I mistaking?

Host: example.com

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23
https://github.com/neomerx/cors-psr7/blob/master/src/Analyzer.php#L256
https://github.com/neomerx/cors-psr7/blob/master/tests/Http/ParsedUrlTest.php#L59

If using the exposed header keys they are set as an array into the http response header. As a result, there are several header lines with the same name in the response.

The Internet Explorer cannot handle the headers, even if it would be correct according to w3c specification.

In "/src/Strategies/Settings.php" the following function should look like this.

BEFORE:

public function getResponseExposedHeaders(RequestInterface $request)
    {
        return $this->getEnabledItems($this->settings[self::KEY_EXPOSED_HEADERS]);
    }

AFTER:

public function getResponseExposedHeaders(RequestInterface $request)
    {
        return implode(', ', $this->getEnabledItems($this->settings[self::KEY_EXPOSED_HEADERS]));
    }

It would be nice if you could recognize this issue and update the project.
Other browsers are not be affected by changing the returned value to a comma-separated string.

Scrutinizer lowers scoring for supplementary code such as default configuration sample.

What kind of test does the following code does?

     $this->settings->setServerOrigin([
        'scheme' => 'http',
        'host'   => 'example.com',
        'port'   => 123,
    ])

To pass the checks, should the request be coming from http://example.com port 123 or to http://example.com:123?

I am trying to implement the library on Silex Framework and I am trying to match the request is coming to the one I am specifying, and it is not working.

If config value is absent use reasonable default value where possible

Related to this issue neomerx/cors-illuminate#18

The problem is it requires restarting the PHP server in order to settings changes take effect. It's less a problem for production but for development it's inconvenient.

Regarding these lines labeled as check 'Host' request. What is the purpose of this part of the code? When should I use it?

I understand what it does but I cannot find where it belongs in the CORS recommendation.

Or does anyone know what happened to @neomerx? There has been no account activity since early 2020.

Currently when a request without origin or origin not matching the server's one incomes the component logs at debug level

Request is not CORS (request origin is empty or equals to server one).  Check config settings for Server Origin.

There are two issues here:

• From the log it's not clear is it a problem with the request (no origin) or there is no match for the origin. It should be 2 distinct messages depending on the situation.
• debug level should be bumped to info because if a developer activates logging it more likely to expect the request to be CORS so this issue should be more visible.

That's the corresponding code.

Change private $settings to protected.

Related to neomerx/cors-illuminate#16

Currently there is a config option Settings::VALUE_ALLOW_ALL_HEADERS which should allow all headers pass through CORS. It works fine for internal lib logic. No problem here. The problem is that this value * is actually sent to client in Access-Control-Allow-Headers and browser don't understand this value.

It looks the only possible way is listing all allowed headers and special * should be removed.

It was added mostly to make development easier. However since logging has been added to the lib this feature is not so important.

It is recommended avoid using Settings::VALUE_ALLOW_ALL_HEADERS and just list all allowed headers in Settings::KEY_ALLOWED_HEADERS

Users should be aware it is not secure to use CORS as access management system.
Add explanation why.

Plus 'null' value

Currently Settings could be configured only with invoking methods and settings cannot be effectively cached and restored back from cache.

Add to Settings abilities to configure it with config in array format with methods setSettings and getSettings.

Document how to integrate the package with systems (e.g. access control) that want to send allowed methods/headers per request individually.

Is there a reason for only supporting psr/log:^3.0? This library doesn't seem to utilize the changes that went into 3.0 specifically and should have no trouble supporting ^2.0 || ^3.0, right?

I only did some shallow testing by installing 2.0 and running the unit tests. But maybe I'm missing something?

Definitely as a separate project. Possible integration

• Laravel
• Lumen

• Scrutinizer
• Travis CI
• Packagist