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

Function getPiwikUrlForOverlay returns wrong URL when it receives different URL than suffixed with piwik.php #9467

Closed
kaz231 opened this issue Jan 4, 2016 · 14 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@kaz231
Copy link

kaz231 commented Jan 4, 2016

If we are using e.g. "http://my.local/js/tracker.php" as piwikUrl in piwik.js, then Overlay tries to load following resource: https://my.local/js/tracker.phpplugins/Overlay/client/client.js?v=1 .

It's because of this line https://github.com/piwik/piwik/blob/2.15.0/js/piwik.js#L2632 that recognizes only URLs suffixed with "piwik.php".

Goal is to instead of removing previously defined suffix, create a function that will remove protocol://domain.tld[port]/ .

@kaz231
Copy link
Author

kaz231 commented Jan 4, 2016

Here's a proposed function:

function getOriginOfURL(url) {
    var domain = /https?:\/\/(www\.)?[-a-zA-Z0-9._]{2,256}(\.[a-z]{2,6})?(:[0-9]+)?\//i;
    return url.match(domain)[0];
}

@kaz231 kaz231 changed the title Function getPiwikUrlForOverlay returns wrong URL when it forwards to different file than piwik.php Function getPiwikUrlForOverlay returns wrong URL when it receives different URL than suffixed with piwik.php Jan 4, 2016
@Joey3000
Copy link
Contributor

Joey3000 commented Jan 4, 2016

@kaz231 There is a similar problem when one uses the /js directory as pkBaseURL, as described here: https://github.com/piwik/piwik/blob/2.16.0-b1/js/README.md. You would just need to call setAPIUrl() respectively in your tracker code on the tracked website. E.g. with the old synchronous tracker: After the piwikTracker definition:

var piwikTracker = Piwik.getTracker(pkBaseURL + "js/", 1);

add following line:

piwikTracker.setAPIUrl(pkBaseURL);

The closest one comes to in terms of documentation on this is following under https://piwik.org/docs/page-overlay/#page-overlay-troubleshooting:

Page Overlay tries to load scripts and data from the URL you pass to the Piwik tracker. If you have a restrictive mod_proxy setup or there’s another reason why this doesn’t work, use the method setAPIUrl(apiUrl) of the Piwik tracker to let it know from which URL it should load the scripts and the data. The parameter apiUrl has to point to the root directory of piwik, e.g. http://piwik.example.org/
or https://example.org/piwik/. The call to setAPIUrl has to be made before calling trackPageView.

See also: http://forum.piwik.org/t/piwik-1-10-page-overlay/8706?page=3

Please note that if using the /js directory as described in that README.md file, you won't need to do the "Deployment" part. It's already done. You would just need to:

change your tracking code to use "js/" instead of "piwik.js" and "piwik.php", respectively


@piwik-devs On a side note, the above information could be added to the above README.md file. Alternatively, the call of setAPIUrl(pkBaseURL) could be part of the generated tracker code by default.

@Joey3000
Copy link
Contributor

Joey3000 commented Jan 4, 2016

@piwik-devs On a side note, the above information could be added to the above README.md file.

PR: #9470

@quba
Copy link
Contributor

quba commented Jan 5, 2016

Unfortunately, this won't work as we can't update quickly tracking codes on hundreds of websites.

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

Maybe it would be easier and more fail proof to check whether it ends with .php and if so, remove everything after the last / slash.

@quba
Copy link
Contributor

quba commented Jan 10, 2016

In such case https://github.com/piwik/piwik/blob/master/js/tracker.php won't work.

@tsteur
Copy link
Member

tsteur commented Jan 10, 2016

We could on top check for an ending /js/ after removing the filename. I just want to avoid complicated regex like mentioned above that probably don't work in all cases (eg URLs can contain various characters etc.) . I'd keep it as simple as possible and if it doesn't work for someone they still can use as mentioned by @Joey3000 the method setAPIUrl was specifically built for this use case. From the description: Set the URL of the Piwik API. It is used for Page Overlay. This method should only be called when the API URL differs from the tracker URL.

@kaz231
Copy link
Author

kaz231 commented Jan 11, 2016

As I said, this is only to catch protocol://domain.tld[port]/ of given URL. We don't analyze rest of it, so that's why I didn't consider any other characters that could be available only in query path. This regex is really simple.

We goal is to have only domain with protocol and port at the end, because we found a case when Piwik tries to load resource from plugin, so removing of the filename won't give us anything. We want to fix it once and never come back to it. The solution that I proposed solves the problem.

@tsteur
Copy link
Member

tsteur commented Jan 11, 2016

What if Piwik is installed under sub.example.com/piwik/foo?

@mattab
Copy link
Member

mattab commented Jan 12, 2016

also: what if Piwik is installed under a unicode domain name? eg. http://ヒキワリ.ナットウ.ニホン

@kaz231
Copy link
Author

kaz231 commented Jan 12, 2016

@mattab I thought about it and it's possible to handle it with regex in JS.
@tsteur good point ! Looks like indeed removing of the suffix is the only solution, which will work in all cases.
Thanks for the feedback !

@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label Feb 13, 2016
@tsteur tsteur added this to the 2.16.1 milestone Feb 13, 2016
@tsteur tsteur self-assigned this Feb 13, 2016
@tsteur
Copy link
Member

tsteur commented Feb 13, 2016

Can you test this patch: #9775 ? I reckon it should fix it

@kaz231
Copy link
Author

kaz231 commented Feb 15, 2016

Sure, I will test it this week.

@kaz231
Copy link
Author

kaz231 commented Feb 29, 2016

@tsteur I tested and it works correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

5 participants