Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added proxy feature for Matomo Tag Manager #79

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tobihille
Copy link

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

@Codov
Copy link

Codov commented Jan 4, 2023

Hello! Do you have any information when the solution will be ready?
Thank you!

@tobihille
Copy link
Author

tobihille commented Feb 7, 2023

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.

@sgiehl
Copy link
Member

sgiehl commented Mar 31, 2023

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

@snake14
Copy link

snake14 commented Apr 2, 2023

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

@thib3113
Copy link

thib3113 commented Apr 5, 2023

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 )

@atom-box
Copy link

atom-box commented Sep 1, 2023

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

@ShadowGaming100
Copy link

how do i use it??

any plan on merging it

@ShadowGaming100
Copy link

@tobihille can you please explain how to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Matomo Tag Manager
7 participants