Once the cors_domains is set apply cors_domains list to the prefight cors

[peterhashair]

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

• [ ] Functional review done
• [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• [ ] Security review done
• [ ] Wording review done
• [ ] Code review done
• [ ] Tests were added if useful/possible
• [ ] Reviewed for breaking changes
• [ ] Developer changelog updated if needed
• [ ] Documentation added if needed
• [ ] Existing documentation updated if needed

[justinvelluppillai]

Looks like you have some changes here from other merged PRs

[peterhashair]

@justinvelluppillai fixed :)

[sgiehl]

@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]

@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]

@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]

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]

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

[peterhashair]

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

[sgiehl]

@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]

@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]

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

$testingEnvironment->overrideConfig('General', 'cors_domains', ['https://example.com']);

[peterhashair]

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

[bx80]

@peterhashair There seem to be some other places where an array parameter is being used. https://github.com/matomo-org/matomo/blob/742071ed81919ebb745f7a735b03a84c26a83b03/plugins/CoreConsole/tests/System/ArchiveCronTest.php#L328

[peterhashair]

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

[bx80]

@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]

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

[github-actions[bot]]

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

[sgiehl]

@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]

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]

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

[github-actions[bot]]

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

[github-actions[bot]]

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

[sgiehl]

Closing this one as we won't continue working on it for now. We might consider doing that in a later step.