-
Notifications
You must be signed in to change notification settings - Fork 120
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
Safer parse_str() usage #566
Safer parse_str() usage #566
Conversation
Hi @tanakahisateru, thank you for the contributions. bb0e839 looks good to me. I have some questions about 86b4483, though. I tried to reproduce the behavior you described in your comment. I was not able to get Rollbar to log the just the There are a couple things about using the Because of these two factors, I don't think merging 86b4483 would be wise or produce the results you are looking for. What are your thoughts? |
I just read #565, which adds a little more context about your thought process. Can you think of a scenario where we would have an error generated from |
Thanks @danielmorell . I actually experienced that no original errors reported while 100+ of function badTypeFunction(string $str): string
{
return preg_replace(..., $str, ...); // returns null when internal error occurred.
} We got a massive data violation to this function like below:
|
I think that PHP native functions don't have proper error handling. For example, |
My another idea is like below: final class Utilities
{
public static function saferParseStr($string)
{
try {
parse_str($string, $result);
} catch (\Throwable) {
@parse_str($string, $result);
$result = array_merge($result, ["...more than max_input_vars items"]);
}
return $result;
}
} To see if ($doCopy) {
// https://bugs.php.net/64634
if (!$source = self::box('fopen', $originFile, 'r')) {
throw new IOException(sprintf('Failed to copy "%s" to "%s" because source file could not be opened for reading: ', $originFile, $targetFile).self::$lastError, 0, null, $originFile);
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term it would be nice to avoid running every string through parse_str()
, but that would take some rethinking of how we handle inputs.
I think this is a good solution for our current problem. Thank you @tanakahisateru!
Description of the change
parse_str()
reports warning if parsed items exceeded over thanmax_input_vars
. Rollbar Logger logs this internal error instead of user errors.Type of change
Related issues
None
Checklists
Development
Code review