Skip to content

Conversation

@bmacnaughton
Copy link
Contributor

A number of files contain ranges of [A-z] which allows the characters [\]^_\ and the back-tick character. Those have been replaced by [A-Za-z].

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1625 (0db8241) into master (2331120) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1625   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1798      1846   +48     
=========================================
+ Hits          1798      1846   +48     
Impacted Files Coverage Δ
src/lib/isPostalCode.js 100.00% <ø> (ø)
src/lib/isBIC.js 100.00% <100.00%> (ø)
src/lib/isISO31661Alpha2.js 100.00% <100.00%> (ø)
src/lib/isLocale.js 100.00% <100.00%> (ø)
src/lib/isEAN.js 100.00% <0.00%> (ø)
src/lib/isTaxID.js 100.00% <0.00%> (ø)
src/lib/isAlphanumeric.js 100.00% <0.00%> (ø)
src/lib/isLicensePlate.js 100.00% <0.00%> (ø)
src/lib/isPassportNumber.js 100.00% <0.00%> (ø)
src/lib/isStrongPassword.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2331120...0db8241. Read the comment docs.

@bmacnaughton bmacnaughton changed the title Fix A-x ranges Fix A-z ranges Mar 10, 2021
tux-tn
tux-tn previously approved these changes Mar 10, 2021
Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch ! Thank you @bmacnaughton and congrats for your first contribution 🎉

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed 🎉 first-pr labels Mar 10, 2021
profnandaa
profnandaa previously approved these changes Mar 12, 2021
@profnandaa
Copy link
Member

Will recheck something before landing. Thanks for your contrib!

@bmacnaughton
Copy link
Contributor Author

can i help with any information for additional review? this seems like the validator passes some potentially troublesome characters.

@bmacnaughton
Copy link
Contributor Author

@tux-tn, @profnandaa - is there anything i can do to wrap this up? if you need to check something i'm happy to research it for you. i just want to close this out while i'm still fresh with it. and it's a pretty serious gap for a validator.

thanks.


const isBICReg = /^[A-z]{4}[A-z]{2}\w{2}(\w{3})?$/;
// https://en.wikipedia.org/wiki/ISO_9362
const isBICReg = /^[A-Za-z]{6}[A-Za-z0-9]{2}([A-Za-z0-9]{3})?$/;
Copy link
Member

@ezkemboi ezkemboi Mar 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: This is not part of the changes request. I am just offering suggestions and we can do it in a separate PR. I can also work on it after this PR get merged.

Great work here.
I like the simplicity, but I could offer an alternative based on fact that the second part is country code addition i.e:

  • Bank code (A-Z) : 4 letter code.

Country code (A-Z) : 2 letter code.

  • Location Code (0-9 or A-Z) : 2 digit code – either letters or numbers.
  • Branch Code (0-9 or A-Z) : optional 3 digit code – either letters or numbers*.

-> We could leverage the array in validISO31661Alpha2CountriesCodes for making this a robust validator.

NB: [Editing code based on comments]

// Making validISO31661Alpha2CountriesCodes exportable or just add to new util file
import { validISO31661Alpha2CountriesCodes } from './isISO31661Alpha2';
//... other code
export default function isBIC(str) {
  assertString(str);
  const countryCode = str.slice(4, 6);
  if(
    validISO31661Alpha2CountriesCodes.indexOf(countryCode) === -1 || 
    countryCode.toUpperCase !== 'XK' // for Republic of Kosovo
   ) {
    return false;
  }
  return isBICReg.test(str);
}

CC. @tux-tn and @profnandaa

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm happy to make a change doing this - i think it's a good idea. i would just return false without executing the isBICReg.test(str) if it's not a country code - no reason to execute the test if we already know the answer.

it's not clear to me why using includes is useful though; is there a compatibility issue somewhere that i'm not aware of? this seems like it should be a straight, simple use of indexOf() - some has to call a function each time but even then why replace some() with includes() to does the exact same thing but with an extra layer of a function call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is great. And I am happy that you are willing to make these changes.

First, I would like to say that, the code above was a sample and required more modifications.

  1. If it not country code, please do that return as you have mentioned
  2. Take the array of validISO31661Alpha2CountriesCodes from isISO31661Alpha2 and create a new file inside lib/util. Import for both isISO31661Alpha2 and BIC validators.
  3. You can make use of indexOf() instead of includes() and some()

I am happy to review that and give a go-ahead to merge this PR.
Thanks again for your contributions @bmacnaughton.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is ready to be merged. it made sense to check that the bank ID was valid.

@bmacnaughton bmacnaughton dismissed stale reviews from profnandaa and tux-tn via 0db8241 March 22, 2021 12:15
@bmacnaughton
Copy link
Contributor Author

@tux-tn @profnandaa - any chance of getting this reviewed and merged? the core problem that is fixed is pretty serious.

@tux-tn
Copy link
Member

tux-tn commented Mar 25, 2021

@bmacnaughton sorry can't help on this. Both ezkemboi and i already approved your PR but you still need to wait for profnandaa in order to merge it. He will probably do it when he has time

@ezkemboi
Copy link
Member

cc @profnandaa

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the good catch; my assumption had been that [A-z] == [A-Za-z]; now just learnt that A-z just meant all ASCII codes for 65 to 122, which include the in between none-alphas (91 - 96)!

@profnandaa profnandaa merged commit b65ddc5 into validatorjs:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎉 first-pr ready-to-land For PRs that are reviewed and ready to be landed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants