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

Guessing query strings should err on being conservative #164

Merged
merged 2 commits into from
May 15, 2017

Conversation

rokob
Copy link
Contributor

@rokob rokob commented May 12, 2017

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.

Any string with an equal sign should not be considered a query string, otherwise exceptions with
database statements can end up url encoded.
@@ -775,10 +775,10 @@ public function scrub(&$data, $replacement = '*')
$parsedValues = array_values($parsed);

/**
* If we have at least one key/value pair (i.e. a=b) then
* If we have at least two pairs (i.e. a=b&c=d) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds pretty good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yeah, okay. I think that's good 👍

@rokob rokob merged commit 5bd1dd7 into master May 15, 2017
@rokob rokob deleted the fix-query-string-guess branch May 16, 2017 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants