ZF-9477 - Issues - Zend Framework

ZF-9477: Zend_Validate_CreditCard validation error

Issue Type:

Improvement

Created:

2010-03-18T21:52:20.000+0000

Last Updated:

2010-03-19T12:40:58.000+0000

Status:

Resolved

Fix version(s):

1.10.3 (01/Apr/10)

Reporter:

Chris Morrell (inxilpro)

Assignee:

Thomas Weidner (thomas)

Tags:

Zend_Validate

Related issues:

Attachments:

Validate.diff

Description

In some circumstances, Zend_Validate_CreditCard can return the wrong error message when validation fails. Even if validation is unable to determine a credit card type, it still uses the $type variable to determine what error to return. This is partially a side effect of the fact that Zend_Validate_CreditCard only validates against CCIs that have been set by addType() or setType(). This means that if a credit card is of a known type, but that type is not accepted, validation may behave in an less-than-ideal way.

For example, with the following code example:


$cardValidator = new Zend_Validate_CreditCard(array(
    'type' => array(
        Zend_Validate_CreditCard::VISA,
        Zend_Validate_CreditCard::AMERICAN_EXPRESS,
    ),
));
$cardValidator->isValid('6011956908810978');

Even though the card number '6011956908810978' is technically in a valid Discover card format, the error message produced by this code is "'6011956908810978' contains an invalid amount of digits".

This is because isValid() loops through the accepted types (in this case Visa and American_Express) and when it doesn't find a match, the $type variable is still set to "American_Express", which was the last type to loop. Then, when the following code runs:


if ($found == false) {
    if (!in_array($length, $this->_cardLength[$type])) {
        $this->_error(self::LENGTH, $value);
        return false;
    } else {
        $this->_error(self::PREFIX, $value);
        return false;
    }
}

It checks $this->_cardLength['American_Express'] which does not allow 16 digit card types.

If, instead the validation was performed in the following order:

Determine card type and error if invalid type
Determine if type is allowed and error if not allowed
Determine if length is correct for type and error if incorrect length

The validator would produce more useful errors and it would solve the above bug.

I'll try to attach a patch shortly with a proposed solution.

Comments

Posted by Chris Morrell (inxilpro) on 2010-03-18T22:37:25.000+0000

Here's a proposed solution. Essentially it does exactly what I described in the bug report:

It adds a new message constant "NOTACCEPTED"
It modifies the "PREFIX" message from "'%value%' is not from an allowed institute" to "'%value%' is not a valid credit card number"
It adds a message for the "NOTACCEPTED" constant "'%value%' is not an accepted credit card type"
It modifies the initial loop in isValid() so that it loops through ALL known types, not just those that have been marked as accepted
It performs 3 checks:
1. Was the type found? If not, error with the "PREFIX" error.
2. Is the found type an accepted type? If not, error with the "NOTACCEPTED" error.
3. Is the length correct for the found type? If not, error with the "LENGTH" error.

The rest of the isValid() method is the same.

Posted by Thomas Weidner (thomas) on 2010-03-19T07:30:23.000+0000

I agree that it would be better to return first if the institute is OK and then if the length is OK.

But your code is bogus. It adds some errorous behaviour.

Additionally NOTACCEPTED duplicates PREFIX in it's meaning. There is no difference between: "'%value%' is not from an allowed institute" and "'%value%' is not an accepted credit card type"

So: Agreed for the issue Not agreed for the patch

Posted by Chris Morrell (inxilpro) on 2010-03-19T07:50:40.000+0000

Fair enough, although I do think that there's a difference between:

The card has an unknown prefix
The card is not from an allowed institute

You're right, PREFIX should be left as-is, but it seems like like it should be provided when the card is from a known prefix but that prefix is not allowed, and a new error should be created for when the card is from an unknown prefix.

Aside from changing the meaning of one of the error constants, how does the code add erroneous behavior? I'm new at submitting code to ZF, so I want to make sure I do it properly.

Chris

Posted by Chris Morrell (inxilpro) on 2010-03-19T09:33:46.000+0000

Here's another potential solution. This time I just added a "NOPREFIX" message type with the message "'%value%' is not from a known institute". Then I made similar modifications to isValid(). I'm not sure what you're talking about erroneous behavior—it passed the unit tests.

Chris

Posted by Thomas Weidner (thomas) on 2010-03-19T12:10:28.000+0000

Changed from bug to improvement

Posted by Chris Morrell (inxilpro) on 2010-03-19T12:25:31.000+0000

Great, thanks! I guess I should have submitted a bug report and an improvement request. Looks like we're all set, though.

Chris

Posted by Thomas Weidner (thomas) on 2010-03-19T12:27:11.000+0000

Implemented with r21570

2 things to note on your not added patch: Noprefix was not added: "Not from an allowed institute" has the same meaning as "Unknown institute" for the customer. Its possible that some institues are just not known to Zend_Validate_CreditCard but still exist.

Regarding errorous behaviour: Your fix breaks the check itself. It makes it possible to have multiple types set (which can be done) but returns false when length and prefix are given for different types which are accepted but do not match.

Posted by Chris Morrell (inxilpro) on 2010-03-19T12:40:58.000+0000

I think that there is a slightly different meaning to the customer:

"Not allowed" means that they entered their card correctly but we do not accept the card type.
"Unknown" means that we don't recognize the type; it may be that it's valid and we do not accept it or it may be that it's invalid (due to a typo).

I also don't see how you'd get this erroneous behavior. It sounds like you're saying that my implementation would allow someone to pass in a card with a prefix & length that do not match as long as both are valid? I don't see how this is possible, as it:

First determines the type, and errors if the type is unknown
Next determine if the type is allowed, and errors if it is not allowed
Finally checks the length against the length definition for the determined type, and errors if it does not match

How could this produce the result you described? Or, it could be that I'm not understanding your description of the behavior.