Optional Argument Validation Fails for non-latin characters about vies HOT 14 CLOSED

I just discovered, that parentheses need to be allowed too. Maybe it is better to remove this filter as @robmeek already suggested. I can see no need for it.
Thanks!

I'm not really a fan of allowing all sorts of entries to go in to prevent malicious code to be executed. Given the feedback up to this point, what I might do instead is allowing all characters except those I consider harmful by using htmlspecialchars.

from vies.

Thank you for responding so quickly!
I tested 7ef8962 and it works fine with our company names, but I have an address which is not working yet:

Speak & Fun España SL
C/ Asunción 80, 8ºB Valid
41011 SEVILLA
Espana
ESB91648113

from vies.

I wonder whether this library should be validating or filtering input to the API at all.

from vies.

Hello @robmeek,

I tested the hypothesis you provided here where I have the following data set for testing

'Greek Trader Name' => [
    [
        'countryCode'          => 'EL',
        'vatNumber'            => '999645865',
        'requesterCountryCode' => 'EL',
        'requesterVatNumber'   => '999645865',
        'traderName'           => 'ΤΡΑΙΝΟΣΕ',
        'traderCompanyType'    => 'AE',
        'traderStreet'         => 'ΚΑΡΟΛΟΥ 1-3',
        'traderPostcode'       => '10437',
        'traderCity'           => 'ΑΘΗΝΑ',
    ],
],

I've modified the filterArgument method in such a way it no longer strips out non-Latin characters by using FILTER_FLAG_STRIP_LOW.

private function filterArgument(string $argumentValue): string
{
    $argumentValue = str_replace(['"', '\''], '', $argumentValue);
    return filter_var($argumentValue, FILTER_SANITIZE_SPECIAL_CHARS, FILTER_FLAG_STRIP_LOW);
}

And modified the validation regex for validateArgument as you suggested, but a bit different so I can support more unicode languages using \pL and /u as is described at https://www.php.net/manual/en/regexp.reference.unicode.php.

    private function validateArgument(string $argumentValue): bool
    {
        if (false === filter_var($argumentValue, FILTER_VALIDATE_REGEXP, [
            'options' => ['regexp' => '/^[a-zA-Z0-9\s\.\-,\pL]+$/u']
        ])) {
            return false;
        }
        return true;
    }

Now the tests are running OK and we didn't break anything in the process, so that's always good.

Please have a look at my PR #100 and see if it solves your issue.

from vies.

@DragonBe Yes, that looks good to me.
Thanks for looking into this so promptly!

from vies.

Awaiting a code review from @krzaczek to make sure… once approved, it will be merged in.

from vies.

@DragonBe can you please add "+" to the white list too?

/^[a-zA-Z0-9\s\.\-,\pL\+]+$/U

Example of a valid company name in the VIES database: ACME GmbH + Co. KG

Thanks a lot!
Mark

from vies.

I just discovered, that parentheses need to be allowed too. Maybe it is better to remove this filter as @robmeek already suggested. I can see no need for it.
Thanks!

from vies.

Thanks for your feedback.
I'm afraid that is not enough either, because ampersand (definitely) and double quotation marks (maybe) are regular characters in company names.

from vies.

I was just reviewing the code bit again and there are two steps in the validation process:

1. filtering input
2. validating input

I could add the htmlspecialchars bit to the filtering process, but validation remains a critical step to consider. I'm running a couple of test scenarios now to figure out a good whitelist/blacklist approach as I don't want to forfeit on validation of input.

from vies.

Hmm, up to this point I have most of the issues cleared, except for the ampesant issue.

Test case: VAN AERDE & PARTNERS
Filter returns: VAN AERDE & PARTNERS
Input Validation succeeds
VIES Validation fails -> because & does not match & 😕

from vies.

Double checked the SoapCall result:

(object) array(
   'countryCode' => 'BE',
   'vatNumber' => '0458591947',
   'requestDate' => '2020-05-27+02:00',
   'valid' => false,
   'traderName' => '---',
   'traderCompanyType' => '---',
   'traderAddress' => '---',
   'requestIdentifier' => '',
)

Looks to me like the validation is ok, but the filtering should not filter the special characters

from vies.

OK, in 7ef8962 I was able to improve the filtering with additonal test cases and was even able to remove a potential security risk.

@fidelo-software can you have a look at it to see if your use cases are now covered? If not, please add some examples here in the thread or immediately to the test data provider.

from vies.

Correct, abbreviations in Spanish are often marked with a / which cannot be filtered out if we want to validate against VIES because Spain requires full trader details for validation purposes.

I know it's a bit annoying, but until I find a permanent solution I think I need to take it case by case. I've adjusted the validation rule in b971690.

from vies.