yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.9k forks source link

Regression: Cors::beforeAction() #15665

Closed alexraputa closed 6 years ago

alexraputa commented 6 years ago

There's a regression in 2.0.14-dev Cors::beforeAction() after merge: https://github.com/yiisoft/yii2/commit/399dbce0cadbd7631f726841ece3ecf6a830444d

This is my test in Codeception:

public function preflight(ApiTester $I): void
{
    $I->haveHttpHeader('Access-Control-Request-Headers', 'Content-Type');
    $I->haveHttpHeader('Access-Control-Request-Method', 'POST');
    $I->haveHttpHeader('Accept', '*/*');
    $I->sendOPTIONS('/users');

    $I->seeResponseCodeIs(200);
    $I->seeHttpHeader('Content-Type', 'application/vnd.api+json; charset=UTF-8');
    $I->seeHttpHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS');
    $I->seeHttpHeader('Access-Control-Allow-Headers', 'Content-Type');
}

Before merge https://github.com/yiisoft/yii2/commit/399dbce0cadbd7631f726841ece3ecf6a830444d:

See http header "Content-Type","application/vnd.api+json; charset=UTF-8"

- Expected | + Actual
@@ @@
-'application/vnd.api+json; charset=UTF-8'
+'application/vnd.api+json; charset=UTF-8'

After merge https://github.com/yiisoft/yii2/commit/399dbce0cadbd7631f726841ece3ecf6a830444d:

See http header "Content-Type","application/vnd.api+json; charset=UTF-8"

- Expected | + Actual
@@ @@
-'application/vnd.api+json; charset=UTF-8'
+'text/html; charset=UTF-8'

Additional info

Q A
Yii version 2.0.14-dev
PHP version 7.1.4
Operating system macOS High Sierra 10.13.3
samdark commented 6 years ago

I see. So there's no error per se and it's overall just confusing, right?

samdark commented 6 years ago

If so, moving to 2.0.15 to think if it can be done better.

alexraputa commented 6 years ago

I see. So there's no error per se and it's overall just confusing, right?

Yes, it's right.

alnidok commented 7 months ago

@samdark Old issue but I agree with https://github.com/yiisoft/yii2/issues/15665#issuecomment-367979881 about response headers for CORS-preflight request. Response headers must contain Access-Control-Allow-Methods.

For example if we try to send PATCH-request from domain.com to api.domain.com (cross origin), browser send preflight request with Access-Control-Request-Method: PATCH and expects to see PATCH in response header Access-Control-Allow-Methods, otherwise it will block further PATCH-request: ... has been blocked by CORS policy: Method PATCH is not allowed by Access-Control-Allow-Methods in preflight response.


OptionsAction added response headers https://github.com/yiisoft/yii2/blob/77ad6bc00847d4964a0b2a82d3b70dcd7cb5a1cf/framework/rest/OptionsAction.php#L43

but this action does not run because CorsFilter return false on beforeAction https://github.com/yiisoft/yii2/blob/4f80cda7130da5259ce93fe41eb681cbb70f30be/framework/filters/Cors.php#L109-L113

alnidok commented 6 months ago

Sorry, it is my mistake. I don't have Access-Control-Request-Method in behavior config.


Cors call prepareHeaders before return false https://github.com/yiisoft/yii2/blob/4f80cda7130da5259ce93fe41eb681cbb70f30be/framework/filters/Cors.php#L106

and add response header Access-Control-Allow-Methods with default config https://github.com/yiisoft/yii2/blob/4f80cda7130da5259ce93fe41eb681cbb70f30be/framework/filters/Cors.php#L171-L173