Looks like you have some changes here from other merged PRs

@justinvelluppillai fixed :)

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

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

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

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

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

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

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.

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

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

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

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