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

Return error when the lenght of the field value does not match the spec fixed lenght #179

Closed
alovak opened this issue Jun 9, 2022 · 3 comments · Fixed by #180
Closed

Comments

@alovak
Copy link
Contributor

alovak commented Jun 9, 2022

Currently, if we set an empty value to the field with the fixed and variable length prefix, there will be no error when we pack such a message. Receiving side will get malformed data and will try to parse it...

It looks like for empty values we skip the length check, but we should not:

https://github.com/moov-io/iso8583/blob/master/field/string.go#L67:

	if len(packed) == 0 {
		return []byte{}, nil
	}

this is not a simple fix, as this if/return was added to fix other issue: #149

Example of the error when field 48 - Acceptor name is empty and receiving party is trying to read LLL prefix (which was not put into the message as we skip adding prefix because of ^^) and reads the value of the next field (49, currency code) instead:

failed to unpack field 48 (Institution/Merchant Name): failed to decode length: data length: 840 is larger than maximum 25
@mfdeveloper508
Copy link
Contributor

Right, The code is very dangerous code
main parser that included MTI(bitmap) don't run correctly
it seems that we should update composite field only for #149

@mfdeveloper508 mfdeveloper508 linked a pull request Jun 27, 2022 that will close this issue
@mfdeveloper508 mfdeveloper508 linked a pull request Jul 1, 2022 that will close this issue
@alovak
Copy link
Contributor Author

alovak commented Jul 8, 2022

Hey @Dakinola892 as you were the author of #149 I want to ask for your feedback and maybe help us with finding the best solution to the issue.

@Dakinola892
Copy link
Contributor

I believe I added #149 as a band-aid fix to a problem we were having locally with parsing, but with the updatest to moov its no longer a problem, so i think mfdevelopers's PR to remove it is a decent enough solution

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

Successfully merging a pull request may close this issue.

3 participants