tracker-proxy
tracker-proxy copied to clipboard
matomo-org
added proxy feature for Matomo Tag Manager
Description:
This should fix #59, at least on my setup it is working in production.
I tried to get as much inspiration as possible from proxy.php and the other config values present in config.php.example while also only using older functions (no str_contains) to make it as compatible as possible.
Review
- [x] Functional review done
- It's working in production for me.
- [x] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
- Thought about what if the user calls index.php manually
- [x] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
- I added as much comments as possible and inserted fallbacks
- [ ] Security review done
- Exposing of data should not happen
- It could be possible to sneak something into file_get_contents here but I'm no security expert, can someone please have a thourough look?
- [x] Wording review done
- does not apply here I guess.
- [ ] Code review done
- [ ] Tests were added if useful/possible
- [x] Reviewed for breaking changes
- breaks nothing because it literally is a new folder and adds something that was missing
- [ ] Developer changelog updated if needed
- [ ] Documentation added if needed
- Did not update readme.md but added extensive comment to new config variable and added it to sample file
- [ ] Existing documentation updated if needed
Hello! Do you have any information when the solution will be ready? Thank you!
Hello! Do you have any information when the solution will be ready? Thank you!
The following tasks are as of 2023-02-07 open:
Security Review done
As already stated in my original message: I am no security expert, someone other than me needs to take a look at this. I know enough about security to know to leave tasks like this to experts.
Code review done
Of course I reviewed my own code before contributing but in my opinion this is also a task of e.g. a maintainer or a contributor.
Tests were added if useful/possible
Need to add these, sorry for the delay, just noticed https://github.com/matomo-org/tracker-proxy#running-the-tests
Developer changelog updated if needed
No update in main matomo needed, found no changelog in this repo.
Documentation added if needed
No update in main matomo needed, added documentation to be in line with other documentation found in this repo.
Existing documentation updated if needed
See above point.
I would appreciate help regarding the unit tests as I just noticed these here. But the changes are still in production on my side and no errors have been found.
@AltamashShaikh @snake14 Does anyone of you maybe has already worked with the tracker proxy and can check if this PR would be fine to merge?
@sgiehl I don't recognise or have any experience with this plugin. The changes look alright, but I don't have any context and could be missing something.
Hello, the problem I saw reading this code, is that it's based on a htaccess ... So, it will only works with apache (and with htaccess enabled)
If I correctly understand how Tag manager works, it will use the js file name like an id ? so why not calling the "js" file with this id like : js/index.php?i=mqdmlqdmlqksd ? and in your script changing this to the filename ?
Also, I will need to do tests, but what about headers that are returned from matomo ? And what about the headers sent to the proxy ?
About security, you do "pretty" nothing . So, I didn't see lot of problems . But maybe :
- add a way to pass the correct ip accessing the file ? ( using
x-forwarded-for? ) => else, on the point of the matomo server, the ip of the tracker will always be the ip of the client . ( so if the server want to block the client, it will block all clients )
A user has asked "when will this feature be ready?":
"I would like the Tag Manager script not to report the domain of the analytics server but that of the monitored site."
