urldecode parameters when getting from $_SERVER['QUERY_STRING'] #8943
Conversation
diosmosis
added
not-in-changelog
| @@ -412,6 +412,9 @@ public static function getRequestParametersGET() | |||
| return array(); | |||
| } | |||
| $GET = UrlHelper::getArrayFromQueryString($_SERVER['QUERY_STRING']); | |||
| foreach ($GET as $name => $value) { | |||
| $GET[$name] = urldecode($value); | |||
| } | |||
There was a problem hiding this comment.
can this have various side effects re security?
There was a problem hiding this comment.
API requests use output sanitization, so shouldn't be an issue there.
Unless I am mistaken controllers don't use rendered API responses.
So if a visualization doesn't do output sanitization, there will be an issue, though that would affect more than URL decoding.
Could do this only for getRawSegmentFromRequest, though this weird no URL decoding needs to be fixed eventually.
There was a problem hiding this comment.
We should make this method private, to make sure nobody else can call it but it is an API method. So we will break API here, maybe it's not used though. Alternatively we could leave this method as it is, and add a new private method like getUrlDecodedRequestParametersGET. As long as it is only used for segments it might not introduce a security regression.
There was a problem hiding this comment.
A new private method sounds like a good idea.
… to urldecode since it is not done by PHP.
diosmosis
force-pushed
the
8813_query_string_decode
branch
from
d2b97dd
to
29cd331
Compare
|
I'm not confident about merging this one late in the beta cycle, moving to 2.15.1 where we will have bit more time to make sure this won't regress |
|
@mattab please remember to test on demo before merging since it can't seem to be reproduced otherwise. |
diosmosis
added
Pull Request WIP
|
There was actually another easier way to fix this issue see: https://github.com/piwik/piwik/pull/9495/files I assume this change is not needed as such, so closing. Please reopen if i'm mistaken |
As title. When getting query parameters from QUERY_STRING server var, make sure to urldecode since it is not done by PHP.
Should fix the issue when the segment param is obtained directly from QUERY_STRING. However, I can't test the fix since I cannot reproduce it locally. @mattab Perhaps you could apply this to demo & check if it works?
Fixes #8813