Safer parse_str() usage

[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

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

None

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[danielmorell]

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 PHP Warning: parse_str(): Input variables exceeded... warning and not log any user generated error. I tried to do it a couple different ways, but each time Rollbar logged the user generated error. Both the error from parse_str() and the user generated error were logged to stdout. What is the context in which you saw this behavior?

There are a couple things about using the @ operator that are worth mentioning. First, I generally try to avoid it. I usually like to know about errors. Using @ can suppress more errors than intended and that should be reported. Additionally, @ does not change the behavior of a custom error handler set with set_error_handler(). This means using @ will not cause an error to be suppressed by Rollbar.

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?

[danielmorell]

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 parse_str() inside the Rollbar SDK that would not be generated elsewhere as well?

[tanakahisateru]

Thanks @danielmorell . I actually experienced that no original errors reported while 100+ of parse_str() warnings.

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:

ด็็็็็็็็็็็็็... (more than 1000 strange pattern)

[tanakahisateru]

I think that PHP native functions don't have proper error handling. For example, fopen() and fclose() should be used with @ and return value for unexpected IO error. Though ignoring items after 1001 is bad idea, parse_str() never do so correctly even if using without @. This is not general better practice but special case for this PHP behavior. (This is just my opinion. It's OK that someone else don't think same.)

[tanakahisateru]

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 \Symfony\Component\Filesystem\Filesystem::copy(), they are avoiding suddenly crash by PHP native function.

        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);
            }

[danielmorell]

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!