zemuldo / iso_8583

:credit_card::moneybag: JavaScript library for iso 8583 messaging. Handles message validation & conversion between interfaces using iso 8583 standard. Contributors are welcome.
https://zemuldo.github.io/iso_8583/
MIT License
85 stars 50 forks source link

BUG: Suspecting a bug in checking binary fields length #141

Open kugesh-Rajasekaran opened 1 month ago

kugesh-Rajasekaran commented 1 month ago

When we check the length of a binary field, we compare the binary length directly with the length of the incoming value, which is in hex.

   if (this_format.ContentType === 'b') {
              if (this_format.MaxLen  === this.Msg[field].length) {    // this.Msg[field] is in hex format and this_format.MaxLen is in bytes
                const size = this_format.MaxLen / 2;
                const thisBuff = Buffer.alloc(size, this.Msg[field], 'hex');
                buff = Buffer.concat([buff, thisBuff]);
    ...

This should be

   if (this_format.ContentType === 'b') {
              if (this_format.MaxLen * 2  === this.Msg[field].length) {    // multiplied with 2 to make a byte with couple of hex characters 
                const size = this_format.MaxLen / 2;
                const thisBuff = Buffer.alloc(size, this.Msg[field], 'hex');
                buff = Buffer.concat([buff, thisBuff]);
    ...

Let me know if any further details necessary

zemuldo commented 1 month ago

Please explain the difference in the code here for context :)

kugesh-Rajasekaran commented 1 month ago
// assemble0_127_Fields.ts, line 56
if (this_format.ContentType === 'b') {
  if (this_format.MaxLen === this.Msg[field].length) {
    ...
  }
}

this.Msg[field] - string in hex format this_format.MaxLen - length in bytes

In this code, each byte is formed by combining two characters of hex. However, I believe the comparison this_format.MaxLen === this.Msg[field].length is incorrect. For example, if this_format.MaxLen is 8 (representing 8 bytes), the input hex value should be 16 characters long (2 characters per byte), but the current comparison expects it to be 8 characters long. I propose the following modification to the code:

if (this_format.ContentType === 'b') {
  if (this_format.MaxLen * 2 === this.Msg[field].length) {     // multiplied by 2 to make a byte with a pair of hex characters
    ...
  }
}

Could you please check this and let me know if it is correct? If it looks good, I will proceed with raising a pull request.

kugesh-Rajasekaran commented 1 month ago

Any updates on this?

zemuldo commented 3 weeks ago

I see your point. While I investigate, Happy to review a PR, Lets see how it works with the tests. I will be looking in to this next week :)