Guessing query strings should err on being conservative

[rokob]

Any string with an equal sign should not be considered a query string, otherwise exceptions with
database statements can end up url encoded.

For example:

 18 class MyException extends Exception { }
 19
 20 $message = "Sql_Error_Data_truncated_for_column_'c_f'_at_row_1_SQL:_UPDATE_`a_b_c`_SET_`id`_='999',_`a_b_id`_='1234',_`c_r_id`_='888',_`position_id`_='44',_`user_id`_='32',_`date_created`_='2017-10-07_12:18:08_WHERE_`id`_= '1234'";
 21
 22 try {
 23     throw new \MyException($message);
 24 } catch (\Exception $e) {
 25     Rollbar::log(Level::error(), $e);
 26 }

Prior to this change, the message would get url encoded and be unreadable.

[ArturMoczulski]

What about cases where the query string only includes one argument i.e.: hostname.com/home?arg1=val1

I would assume that is the most common use case of the query string.

[rokob]

If it is part of a URL then it will be found above in the first time we try to identify a URL with a query string. This is just if it is a bare string. I'll change it to exactly one = or if multiple = then also has more than one & which should be good enough.

[ArturMoczulski]

Yeah, that sounds pretty good 👍

[rokob]

I actually think looking at this more closely that we should get rid of this code.

We use parse_url($data, PHP_URL_QUERY); to attempt to get a query string, which will get the proper query string for these cases:

http://whatever.com/?a=b
whatever.com/?a=b
?a=b

If we have a bare string a=b then assuming that is a query string is a bit of stretch. I think we should just rely on parse_url to do it's job. The incidence of bare query strings without being part of some larger url must be very small.

[ArturMoczulski]

Yeah, okay. I think that's good 👍