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

Use of a MessageParseStrategy and SMSMessageParser might lead to invalid messages being sent #43

Closed
giovannivelludo opened this issue Dec 31, 2019 · 1 comment

Comments

@giovannivelludo
Copy link
Contributor

giovannivelludo commented Dec 31, 2019

Method parseData() from DefaultSMSPartStrategy adds a character to the text of the message to be sent, but there are no checks to see if it still fits SMSMessage.MAX_MSG_TEXT_LEN.

I think SMSMessage.checkMessageText() should check if the (length of the SMS + length of DefaultSMSParseStrategy.HIDDEN_CHARACTER) fits SMSMessage.MAX_MSG_TEXT_LEN.
However, if whoever is using this library creates a new SMSMessage and then uses SMSMessageParser.getInstance().setMessageParseStrategy() to change how many characters are added in the beginning of the message, then the checks on the validity of SMSMessages already created might be wrong, for example if the hidden characters become longer in the new strategy.

Doing the check in getSMSContent() (or in general after the SMSMessage is created correctly) would not be a good idea, because then the user won't know why their message was created correctly but then failed a second check on its length.

A simple solution could be removing class SMSMessageParser and letting the app using this library decide if and what character to insert at the beginning of messages. That class was meant to make it easier for different groups to decide how they wanted to identify messages meant for their app, but in the end it's unnecessarily complicating everything.
A worse solution would be letting custom MessageParseStrategy classes only use one character at the beginning of messages, because at that point we might as well remove the possibility of creating those custom strategies, since it's faster to just have a method to change the character that is added before the message.
In both cases the best thing to do is letting developers of the app using this library choose if and how they want to add some ID or password in the beginning of each message, as it was done in our killer app.

@giovannivelludo
Copy link
Contributor Author

Duplicate of #19

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

1 participant