matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!
https://matomo.org/
GNU General Public License v3.0
19.7k stars 2.62k forks source link

Allow to disable all header() calls when Piwik API is used via internal method #2294

Open mattab opened 13 years ago

mattab commented 13 years ago

For example, if format=json and the API is called via the PHP internal method, it will prompt a "download" popup to download the JSON.

I'm not entirely sure why as the only header is: @header('Content-Type: application/json; charset=utf-8');

but in any case we should debug this and disable the headers / other things that prevent the Internal PHP method to work as expected.

See forum post: http://forum.piwik.org/read.php?2,74955

LeCoyote commented 11 years ago

I strongly agree with this, and I would like to see it considered as a bug, not a feature request.

The problem is that, under some circumstances, the header() issued by Piwik will mess with the calling page's output, although I am still unsure why or how.

Note that the problem is the same whether the format is json, php or xml.

LeCoyote commented 11 years ago

I would like to clarify. Using format=php, the calling page is rendered as text/plain. So far, using format=original is the only way I can get the result without it interfering with the calling context.

LeCoyote commented 11 years ago

Considering that the result of the calls to header() will seriously disrupt the calling code, I believe that this issue should be considered a bug, not a feature request. Besides, it is now over 2 years old. Can anything be done about it? Perhaps a quick mention in the documentation that "original" should be used when calling the API in PHP?

mattab commented 10 years ago

@LeCoyote

I made a change and now format=original will also set the text/plain header. This was to prevent potential sec issues with displaying raw content.

we'd like to fix this issue and allow to disable headers, unfortunately it's not trivial. There are dozens of calls to header() all over the codebase... (I just looked)

In your case maybe it is possible to call Piwik, before any output is sent to the client ?

LeCoyote commented 10 years ago

@matt

This issue is about Piwik unexpectedly adding headers using the internal method. The format=original workaround is the only way to use the internal method without breaking stuff. I don't understand how disabling this workaround is a security fix. Now the internal method is truly broken.

Your suggestion of calling Piwik before output is sent won't help. It leaves Piwik in control of the Content-type. This means every page that calls Piwik using the internal method is sentenced to serve text/plain. I cannot figure out how it can be a workaround.

Also, this is still a bug, not a new feature. Piwik's documentation of Piwik_API_Request::process() clearly mentions that using format=original should return a PHP object, not display anything, which is basically what sending the Content-type header will do. If this is broken, please at least make it obvious in the documentation.

I never said it would be trivial. I did say however that this should be addressed as a bug and I still stand by that statement. Unexpectedly disrupting content flow without warning or documentation to that effect is not a missing feature: it's a bug.

mattab commented 10 years ago

Thanks for feedback. It's definitely a bug....

I think the solution is to wrap all header() call into a refactored method, that will not print the headers if we are not dispatching. similar fix to https://github.com/piwik/piwik/blob/bc2f9371b7174f397b06df2d5f71ea5bb50c6cfb/core/SettingsPiwik.php#L173

Piwik's documentation of Piwik_API_Request::process() Btw maybe you can help with documentation. There is an "edit" link at the bottom of developer docs feel free to suggest improvements... Cheers!

LeCoyote commented 9 years ago

About a year ago, you mentioned that it was "definitely a bug". I can also confirm that it is still present. Why is it now considered an enhancement?

mattab commented 9 years ago

@LeCoyote changed it back to Bug