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

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

Open
mattab opened this issue Apr 11, 2011 · 8 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc.

Comments

@mattab
Copy link
Member

mattab commented Apr 11, 2011

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
Copy link

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
Copy link

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
Copy link

LeCoyote commented May 6, 2013

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
Copy link
Member Author

mattab commented Mar 16, 2014

@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
Copy link

@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
Copy link
Member Author

mattab commented Mar 24, 2014

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!

@mattab mattab added this to the 2.x - The Great Piwik 2.x Backlog milestone Jul 8, 2014
@mattab mattab removed P: low labels Aug 3, 2014
@mattab mattab added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. and removed Bug For errors / faults / flaws / inconsistencies etc. labels Oct 11, 2014
@LeCoyote
Copy link

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 mattab added Bug For errors / faults / flaws / inconsistencies etc. and removed Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Aug 12, 2015
@mattab
Copy link
Member Author

mattab commented Aug 12, 2015

@LeCoyote changed it back to Bug

@mattab mattab modified the milestones: Long term, Mid term Dec 5, 2016
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

2 participants