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

UI: use POST request instead of get to avoid parameters logging #3359

Closed
mattab opened this issue Sep 5, 2012 · 44 comments
Closed

UI: use POST request instead of get to avoid parameters logging #3359

mattab opened this issue Sep 5, 2012 · 44 comments
Assignees
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Sep 5, 2012

Currently, many admin features use ajax requests with GET parameters. Parameters can include the token_auth and therefore is sensitive information. The requests to piwik are usually logged in the apache/nginx/iis access logs.

Let's do a quick audit of the piwik source code, and change all these requests to POST, post params are not logged.

@pebosi
Copy link
Contributor

pebosi commented Oct 5, 2012

Created a patch changing some ajaxRequest.type's to POST

@pebosi
Copy link
Contributor

pebosi commented Oct 5, 2012

Attachment:
get-to-post.patch

@pebosi
Copy link
Contributor

pebosi commented Oct 5, 2012

Attachment: rm debug
get-to-post.2.patch

@sgiehl
Copy link
Member

sgiehl commented Oct 25, 2012

(In [7309]) refs #3359 use POST instead of GET requests / do not send token_auth within query strings

@sgiehl
Copy link
Member

sgiehl commented Oct 27, 2012

(In [7317]) fixes #3483, refs #3359 use GET to query for widgets as query string is used to build urls for sparklines

@sgiehl
Copy link
Member

sgiehl commented Oct 29, 2012

(In [7322]) fixes #3487, refs #3359 refactoring ajax requests to use a global method to query for module actions

@timo-bes
Copy link
Member

If I remember correctly, I used GET in the helper method because something didn't work with POST. This change should be very well tested (i.e. every ajax request that is impacted by it).

I think there was a problem with multi row evolution but maybe also somewhere else.

@timo-bes
Copy link
Member

Also, I think I remember a problem with UTF8 urls and page titles in Transitions when using POST.

Will do some testing...

@timo-bes
Copy link
Member

(In [7338]) refs #3359: if the user passes a date to piwikHelper.ajaxCall, don't manipulate it in case of period=range

@timo-bes
Copy link
Member

I ended up not doing much testing because piwikHelper.ajaxCall looks a little weird to me... Most parameters are sent as URL and POST parameters which will most likely cause confusion/problems at some point.

How does PHP handle that anyway? Are the URL parameters in $_GET and the POST parameters in $_POST? That would indeed be confusing.

I would suggest to pass everything as URL parameters and only the token as a POST parameter. Having the rest in the logs can be useful if you want to analyze how Piwik is used.

@timo-bes
Copy link
Member

Note to the tester (or myself): two critical parts are (Multi) Row Evolution and Transitions because they rely on the encoding being intact after passing the value back and forth. Test with very long urls, utf8 urls, encoded urls, double encoded and urls with unencoded special characters. Do the same for page titles in Transitions. Also, check what other parts were affected by the change and test those AJAX requests.

@sgiehl
Copy link
Member

sgiehl commented Oct 31, 2012

Ok. It should be the easiest way just to send the token_auth as POST and the rest as GET parameters. I'll change that, so there shouldn't be anymore problems caused by that.

@mattab
Copy link
Member Author

mattab commented Oct 31, 2012

thanks for your tests on this, it really helps to know about the test cases etc.

Steve, it looks good to only put token_auth as POST!
Are all the other changes safe?

Note to testers: Check if fixing this also fixes the following bug:

  • In dashboard and in Visitors>Locations report, clicking Goal Icon displays zero everywhere. However data displays correctly in Goals>Overview report by Country. EDIT: is being tracked in: Conversions not showing up in Goals metrics #3492 but possibly will be fixed here

@sgiehl
Copy link
Member

sgiehl commented Oct 31, 2012

(In [7343]) refs #3359 only send token_auth as POST param

@mattab
Copy link
Member Author

mattab commented Nov 1, 2012

(In [7351]) Ping will deploy to demo to test? Refs #3359

@sgiehl
Copy link
Member

sgiehl commented Nov 1, 2012

(In [7355]) fixes #3265 pass disableLink param to the widgets
refs #3359 use global ajax method to fetch available widgets

@sgiehl
Copy link
Member

sgiehl commented Nov 3, 2012

(In [7365]) refs #3359 use global ajax method

@sgiehl
Copy link
Member

sgiehl commented Nov 3, 2012

(In [7367]) refs #3359 use global ajax method

@sgiehl
Copy link
Member

sgiehl commented Nov 3, 2012

(In [7368]) refs #3359 use global ajax method for datatables

@sgiehl
Copy link
Member

sgiehl commented Nov 4, 2012

(In [7370]) refs #3359 use global ajax method for goal management

@sgiehl
Copy link
Member

sgiehl commented Nov 4, 2012

(In [7371]) refs #3359 do not send empty params in datatable requests (as they should not be needed)

@sgiehl
Copy link
Member

sgiehl commented Nov 4, 2012

(In [7372]) refs #3359 use global ajax method for SEO widget

@sgiehl
Copy link
Member

sgiehl commented Nov 5, 2012

(In [7385]) refs #3359 use global ajax method for privacy settings

@sgiehl
Copy link
Member

sgiehl commented Nov 19, 2012

(In [7489]) refs #3359 moving ajax requests to a new ajax helper with more functionality as the old functions

@timo-bes
Copy link
Member

I think this is related to this ticket: When using a date range on the dashboard, some widgets don't work. The problem is that only the end date is sent and not "start,end".

Steve, could you take a look at this?

To reproduce

@sgiehl
Copy link
Member

sgiehl commented Nov 28, 2012

I guess that should already be fixed in [7539]. But I'll take a closer look at that later.

@timo-bes
Copy link
Member

That change was after the latest beta. So you're probably right, Steve.

Matt, could you release another one? Is there anything else in trunk that is unstable at the moment?

@mattab
Copy link
Member Author

mattab commented Dec 4, 2012

(In [7575]) Will release a new beta (sorry for delay @timo) Refs #3359

@mattab
Copy link
Member Author

mattab commented Dec 4, 2012

Beta is available at: http://piwik.org/piwik-1.9.3-b9.zip

Note: different path than usual while we're finalizing security improvements.

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2012

(In [7582]) refs #3359 use new ajaxHelper for PDFReport management

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2012

(In [7584]) refs #3359 use new ajaxHelper for general settings

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2012

(In [7586]) refs #3359 use new ajaxHelper for sitesmanager

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2012

(In [7587]) refs #3359 use new ajaxHelper for usersettings

@sgiehl
Copy link
Member

sgiehl commented Dec 7, 2012

(In [7588]) refs #3359 use global ajax helper for usersmanager

@sgiehl
Copy link
Member

sgiehl commented Dec 8, 2012

(In [7589]) refs #3359 use global ajax helper for mobilemessaging

@sgiehl
Copy link
Member

sgiehl commented Dec 9, 2012

(In [7590]) refs #3359 marked some (now unused) methods as deprecated; small improvements

@sgiehl
Copy link
Member

sgiehl commented Dec 9, 2012

(In [7591]) refs #3359 small fix

@sgiehl
Copy link
Member

sgiehl commented Dec 9, 2012

(In [7592]) refs #3359 no need to set token_auth as get param as it will be sent as post

@mattab
Copy link
Member Author

mattab commented Jan 14, 2013

Note: just saw that the real time live queyr every 10 seconds contain the token auth. Would be awesome to finish the refactoring or apply the change to the Live plugin.

@mattab
Copy link
Member Author

mattab commented Jan 14, 2013

Btw SteveG awesome work& code on this one! Nice series of commits in this ticket ;-)

@sgiehl
Copy link
Member

sgiehl commented Jan 14, 2013

Live plugin was fixed in [7750]

@mattab
Copy link
Member Author

mattab commented Jan 14, 2013

Awesome!

PS: post commit hooks are temporarily broken (under investigation)

@sgiehl
Copy link
Member

sgiehl commented Jan 17, 2013

I'm colsing this ticket for now. There shouldn't be anymore requests done by javascript using token_auth as get param. The only requests done, not using the new ajax helper, are within the feedback, login & install module. If anything is still left, please reopen.

@mattab
Copy link
Member Author

mattab commented Jan 17, 2013

Nice refactoring & security improvement!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Security For issues that make Matomo more secure. Please report issues through HackerOne and not in Github. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

4 participants