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

Validator ignores extra characters #35

Open
Arkkimaagi opened this issue Mar 29, 2017 · 9 comments
Open

Validator ignores extra characters #35

Arkkimaagi opened this issue Mar 29, 2017 · 9 comments
Milestone

Comments

@Arkkimaagi
Copy link

For me it seems that the validator is too flexible. It says that this is valid:
FI@ +425*000*151=0 == #000$ 0° 23€€€!!!!!

And altho technically it does contain valid numbers for an fictional IBAN number, that gibberish in my mind should not count as a valid IBAN.

@Arkkimaagi
Copy link
Author

At least provide a strict mode and/or warn people that they have to sanitize themself and how to do it.

@LaurentVB
Copy link
Collaborator

I'll have a look, but you're right, this should obviously be marked as invalid.
I think whitespace should be the only type of characters ignored.

@ostrolucky
Copy link

ostrolucky commented Jun 19, 2017

It shouldn't even convert input to uppercase. We had a problem when Sk2683300000002700890226 passed the validation of this library and then bank transfer failed because of "Sk" instead of "SK"

@mpkorstanje
Copy link

Right now the correct way to use isValid seems to be:

if(IBAN.isValid(input){
  model = IBAN.electronicFormat(input);
  view = IBAN.printFormat(input);
}

This makes sense when dealing with user input as they are entering it but less so when merely validating if the input adheres to a format.

So I think the validation should be

    exports.isValid = function(input){
        if (!isString(input)){
            return false;
        }
        var iban = electronicFormat(input);
        if(iban !==input ){
            return false;
        }
        var countryStructure = countries[iban.slice(0,2)];
        return !!countryStructure && countryStructure.isValid(iban);
    };

And then we might add a parse function to facilitate the other use case

    exports.parse = function(input){
         var iban = electronicFormat(input);
         if(isValid(iban)){
              return iban;
         }
        return undefined;
    };

So we can do

var iban = IBAN.parse(input);
if(iban){
  model = iban;
  view = IBAN.printFormat(input);
}

@LaurentVB
Copy link
Collaborator

Hey guys, thanks for your feedback.
I think we do not have the same use case in mind for this library. Fundamentally, if the user misplaces a space, or types lowercase letters, it can still be validated and accepted as a valid IBAN. The internal representation, though, is subject to specifications (captured in the electronicFormat and printFormat functions).
So I think what we have here is a problem of documentation.
Yes, you should always use the electronicFormat when you store or send the IBAN. It's the canonical representation of the IBAN. Yes, isValid will return true for anything that can be trivially converted to a valid IBAN in canonical format.
This library aims to adhere to the principle of robustness. But you're right that it deserves to be documented 👍

For your use case, I think this lib can also help, though:

var strictlyValid = (input === IBAN.electronicFormat(input)) && IBAN.isValid(input);

That said, a bank that drops a payment because the IBAN was not all uppercase is 😱

@Arkkimaagi
Copy link
Author

This one caught us by surprise as the naming is confusing. It's not just documentation that should be improved.

Instead of ‘isValid‘ it would be clearer if ‘containsIBAN‘ or similar would be used. Generally I feel that a isValid should be strict by default and lax alternatives should be named differently.

@Moejoe90
Copy link

Whats the status of this issue?

@mpkorstanje
Copy link

The only thing on which there is a consensus is that the documentation could stand some improvement but so far no one has bothered to do so yet.

I submitted mmjmanders/ng-iban#19 a while ago and that has been accepted and merged. As far as I am concerned the problem has been resolved.

@stefanve
Copy link

I think maybe a IBAN.clean(iban) could be helpful because although spaces and some other strengess are allowed in the specs, most banks does not validate them. clean() should make uppercase remove all that is not A-Z 0-9
I had a IBAN with ^ in it, it came out valid....

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

No branches or pull requests

6 participants