@peterhashair opened this Pull Request on April 19th 2022 Contributor

Description:

Fixes: #19116
Once the cors_domains are set apply the cors_domains list to the prefight cors. By default, it allows all. Maybe we should use the getAllKnownUrlsForAllSites() array + cors_domains as the whitelist array.

Review

@justinvelluppillai commented on April 19th 2022 Contributor

Looks like you have some changes here from other merged PRs

@peterhashair commented on April 19th 2022 Contributor
@sgiehl commented on April 25th 2022 Member

@peterhashair I'm not very familiar with the CORS stuff, but based on the documentation your PR can't work, as the origin only allows one record to be returned. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin

<origin>
Specifies an origin. Only a single origin can be specified. If the server supports clients from multiple origins, it must return the origin for the specific client making the request.

@peterhashair commented on April 27th 2022 Contributor

@sgiehl I got an idea not sure if I am correct, I move the Corshandler to the Tracker main, works on my local, but I update Corshandler a little, will that cause any issues?

@sgiehl commented on April 27th 2022 Member

@peterhashair I actually wasn't aware of the CORSHandler. Maybe it would make more sense to move the preflight handling completely to the CORSHandler.

@MichaelRoosz commented on April 27th 2022 Contributor

We are also setting cors headers here: https://github.com/matomo-org/matomo/blob/4.x-dev/core/Tracker/Response.php#L111, it might be a good idea to merge this also with CORSHandler

@peterhashair commented on April 28th 2022 Contributor

@sgiehl @MichaelRoosz rewrite CORSHandlera little bit hopefully that makes more sense.

@peterhashair commented on May 4th 2022 Contributor

@sgiehl trying to think of a test, is there a function I can use to do a fake cors request?

@sgiehl commented on May 4th 2022 Member

@peterhashair I guess you could use Http::sendHttpRequestBy to send a curl request with the according request headers. There should be flag you can set so the method also returns the returned status and headers so you are able to check if they are expected.

@peterhashair commented on May 19th 2022 Contributor

@bx80 Trying to add below in the setUp to test the config, but it seems like the test config won't update config. any ideas?

       $testingEnvironment->overrideConfig('General', 'cors_domains', 'https://example.com', true);
@bx80 commented on May 19th 2022 Contributor

@peterhashair It might need to be an array value instead of a string

$testingEnvironment->overrideConfig('General', 'cors_domains', ['https://example.com']);
@peterhashair commented on May 20th 2022 Contributor

@bx80 the old function doesn't accept array, I extend the function, the last param true accept arrary

@bx80 commented on May 20th 2022 Contributor
@peterhashair commented on May 20th 2022 Contributor

@bx80 tried all kinds of different ways, but the config still won't set, any suggestions?

@bx80 commented on May 20th 2022 Contributor

@peterhashair I think the parent IntegrationTestCase class for the test already has a fixture defined which also it's own test environment initialized, instead of overriding the config setting on new test environment object you could try overriding the config on the parent fixture's test environment with:

self::$fixture->getTestEnvironment()->overrideConfig('General', 'cors_domains', ['https://example.com']);
@peterhashair commented on May 23rd 2022 Contributor

@bx80 figure out why, if all the tests are passed will request a review

@github-actions[bot] commented on June 1st 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@sgiehl commented on June 3rd 2022 Member

@justinvelluppillai This PR would now be ready to merge. But it actually changes more than the issue that should be solved. All the CORS handling is now also done for API requests. That means if the cors_domains config is set incorrect, the Matomo UI will no longer work correctly. The page will load, but all API requests will fail, causing only an error to show up.
Personally I think that behavior is kind of correct. But we might need to consider to either mention that in the dev change log and maybe also update or add an FAQ. This might be kind of a breaking change as it restricts API requests being sent from other sites...

@tsteur commented on June 3rd 2022 Member

That means if the cors_domains config is set incorrect, the Matomo UI will no longer work correctly. The page will load, but all API requests will fail, causing only an error to show up. ... This might be kind of a breaking change as it restricts API requests being sent from other sites...

FYI if there's any kind of breaking change then we should wait for Matomo 5 before merging this. Or maybe we can prevent the breaking change in some way. We wouldn't want to break the UI.

Can we also confirm there's no security issue by this new implementation which is quite different (for all kind of web servers)? I've tested this locally and before I was not allowed to send a reporting API request from a different domain but now it allows me to send a request from that domain. Meaning this would be also a security regression.

@tsteur commented on June 4th 2022 Member

By sending a 401 or 403 response code can we also make sure this won't cause any duplicate requests to be sent in the JS tracker eg because of https://github.com/matomo-org/matomo/blob/4.10.1/js/piwik.js#L2843-L2850 where we send the same request again if there's eg a 4XX response. If there's any way this could happen then this will likely result in duplicate tracked data and increased server load.

I'm thinking at least for tracking API it's potentially on purpose maybe to send a 2XX response code in some cases instead of 4XX as the response may not be needed and the 2XX prevents users from thinking there is an error and then they email support or create bug reports when there's actually no problem. This may be different for the reporting API where the response is usually needed.

Also note that HTTP response codes of tracking and reporting API are API see https://developer.matomo.org/guides/apis meaning changing the response code is also in itself a breaking change.

Maybe the behaviour for tracking API should be unchanged, and / or generally the behaviour on the response code may need to be different depending on if any domains are configured or if the standard applies.

From what I can see generally even if the response code is a 2XX then it doesn't mean the response can be read, the browser would still show a CORS error so not sure how valid the context of the original issue is

image
@github-actions[bot] commented on June 22nd 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

Powered by GitHub Issue Mirror