@peterhashair opened this Pull Request on March 30th 2022 Contributor

Description:

Fixes: #18812

Extend requests with headers and method do not record preflight cors in the database.

To reproduce the issue one needs to trigger prefight cors, eg add a request header more details can be found here

       headers:{'X-PINGOTHER':'pingpong'}

Review

@sgiehl commented on April 5th 2022 Member

I'm not really deep into such CORS stuff, but I was wondering if we shouldn't send a proper answer to a CORS request.
According to https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request it seems the correct answer would a 204 with some additional headers containing the allowed methods.
@peterhashair did you test what happens if we send a normal answer to the preflight request? Is the tracking request still sent in that case?

@peterhashair commented on April 5th 2022 Contributor

@sgiehl right that makes sense, will update that.

@peterhashair commented on April 7th 2022 Contributor

@sgiehl I am trying to figure out how to do a response as 204, but I notice there is a set of requests, not just one, if I respond 204 in the foreach, will that affect another request?

@peterhashair commented on April 7th 2022 Contributor

@sgiehl I am trying to figure out how to do a response as 204, but I notice there is a set of requests, not just one, if I respond 204 in the foreach, will that affect another request?

@sgiehl commented on April 7th 2022 Member

@peterhashair You may need to handle that at an earlier stage. As it's not relevant if the preflight request might contain one or more tracking requests, you should do that before the request is actually parsed. Guess here it might suite:
https://github.com/matomo-org/matomo/blob/078909ef807910a1f2a4d93d738306d0bf6a9e04/core/Tracker.php#L130-L136

@peterhashair commented on April 8th 2022 Contributor

@sgiehl it seems like the prefight works, just wondering should I use the site url store in the data for the Access-Control-Allow-Origin, otherwise there will be security issue I guess.

             header('Access-Control-Allow-Methods: GET, POST');
               header('Access-Control-Allow-Headers: *');
               header('Access-Control-Allow-Origin: *');
               Common::sendResponseCode(204);
@sgiehl commented on April 11th 2022 Member

I've now applied a couple of fixes and added a simple test to get this ready to merge...

This Pull Request was closed on April 11th 2022
Powered by GitHub Issue Mirror