@bx80 opened this Pull Request on November 18th 2021 Contributor

Description:

Fixes #17945

When Matomo is running behind a reverse proxy server that forwards traffic to a directory path (eg. proxy.com/matomo/) but Matomo itself is installed in a different path (eg. someotherhost.com/ or someotherhost.com/differentpath) then incorrect paths are used for links in the Multi Sites page, specifically: site selection, the evolution sparklines and add a new website.

This PR is a simple fix to remove the calculated piwik_url from these links and use relative paths instead.

Tested with three docker images: mariadb:latest, matomo:4.5.0 official docker and nginx:latest

nginx configured with

location /matomo/ {
        proxy_set_header Host $host;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Host $host;
        proxy_set_header X-Forwarded-Uri /matomo/;
        proxy_pass http://<matomo container ip>/;
        proxy_read_timeout 90;
    }

Matomo config.ini.php

[General]
assume_secure_protocol = 0
proxy_client_headers[] = "HTTP_X_FORWARDED_FOR"
proxy_host_headers[] = "HTTP_X_FORWARDED_HOST"
trusted_hosts[] = "<nginx container ip>"

Review

@sgiehl commented on November 18th 2021 Member

I'm not sure if removing the piwik_url from those urls is a good solution. Even though it might work, it would mean we need to adjust all twig templates that use the {{ piwikUrl }} variable for generating a url, as those urls would be incorrect as well. That means that likely the optout and other stuff might also not work.

@bx80 did you try if setting the config variable [General] proxy_uri_header = 1 solves the issue?

@bx80 commented on November 18th 2021 Contributor

@sgiehl I'd missed the proxy_uri_header option, yes that fixes the issue. This PR is therefore unnecessary. :man_facepalming:

@tsteur commented on November 18th 2021 Member

A lot of other links, even in twig templates, simply point to index.php. For example:

image

I think it be still an easy merge as it fixes it without having to configure something and AFAIK there's no downside to it? It doesn't mean we need to change all others now but the fix is there and it improves it quite a bit I would say.

@bx80 commented on November 18th 2021 Contributor

Ok, I can merge these fixes and also mention the config setting in the original issue so there is an immediate solution for those affected.

This Pull Request was closed on November 19th 2021
Powered by GitHub Issue Mirror