Closed Bug 1015194 Opened 6 years ago Closed 3 years ago

[Messages] Invalid recipients are sometimes still considered as valid recipients

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog, b2g18 affected, b2g-v1.2 affected, b2g-v1.3 affected, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 affected)

RESOLVED WONTFIX
tracking-b2g backlog
Tracking Status
b2g18 --- affected
b2g-v1.2 --- affected
b2g-v1.3 --- affected
b2g-v1.3T --- affected
b2g-v1.4 --- affected
b2g-v2.0 --- affected

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [sms-most-wanted])

Attachments

(1 file)

STR:
1. enter 3 invalid recipients (type: "a;" then "b;" then "c;" => this will enter 3 invalid recipients a, b, c)
2. tap recipient "a" => it will move to "editable" mode
3. tap recipient "b" => recipient "a" will move back to "invalid" and "b" will move to "editable"

=> the header reads "1 recipient" but it should read "new message"

4. enter some text

=> "send" button is enabled whereas it should be disabled

5. press send

=> we're moved to a "a" thread and a message is being sent; but it should not.
=> eventually we have an error.

We found the issue while working on bug 1011573.
This is probably happening since v1.1.
(In reply to Julien Wajsberg [:julienw] from comment #0)
> => eventually we have an error.

Given there is a letter-to-number conversion, it could happen to have a real number and have a real SMS sent. That said, someone is really trying to find bugs if that happens ;)




The issue comes from the fact we don't update the list if there is something currently typed (see [1]). This check is necessary because we don't really know the right index to change, we only guess it should be the last one, which is right in most cases, but not here.
As a result, the "to be invalid" recipient is never marked as invalid in the JS object, but it is _visually_ invalid because the class is correctly set, because it was invalid before.

It's not _that_ easy to fix it, it's also not _that_ difficult.
I've nominated it to 1.4? but feel free to block to 1.3t if necessary.
Blocks: 952519
Just noticed that it's also possible to send SMS to invalid recipient using the following STR:

1. Enter any message text;
2. Enter valid recipient and press enter (eg. "123");
3. Observe that send button is enabled;
4. Enter some invalid number (eg. "..."), but don't confirm it via Enter;
5. Tap on Send button.

Expected result: message is sent to "123" only;
Actual result: message is sent to both "123" and "...".
Been around since 1.1, hence backlog
blocking-b2g: 1.4? → backlog
Paul,

Would this be a security issue? Are we opening a loop hole here?
Flags: needinfo?(ptheriault)
IMO this is no security issue, only a buggy behavior.
(adding a whiteboard so that the sms team can find it back when planning sprints)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Whiteboard: [sms-most-wanted]
(In reply to Julien Wajsberg [:julienw] (away until 2nd June) from comment #6)
> IMO this is no security issue, only a buggy behavior.
> (adding a whiteboard so that the sms team can find it back when planning
> sprints)

Agreed.
Flags: needinfo?(ptheriault)
This was fixed in the v1.3t branch only in bug 989600 and bug 1011573. Should not be difficult to retrofit the fix here.
Taking to try to do what I suggested in comment 8.
Assignee: nobody → felash
blocking-b2g: backlog → ---
Duplicate of this bug: 1141463
Comment on attachment 8592135 [details] [review]
[gaia] julienw:1015194-fix-recipients-handling > mozilla-b2g:master

just a WIP
Comment on attachment 8592135 [details] [review]
[gaia] julienw:1015194-fix-recipients-handling > mozilla-b2g:master

Hey Oleg,

what do you think of this approach ?

Known issue:
* the focus is not retained after I enter a recipient. I didn't find a good way to make it work yet, so if you have an idea I'd be glad to know it.
* we send several 'add' and render several times when we recover a draft; we could wait until all "add" operations have been handled, but I'm not sure the added complexity is worth it?
* I think we could remove some useless render/focus calls that happen in the event handler (in the view). What do you think?
Attachment #8592135 - Flags: feedback?(azasypkin)
Depends on: 1142540
mmm and currently the STR in comment 0 is not working properly (one of the invalid recipients disappear :/).

Just let me know what you think of the approach !
Priority: -- → P1
Comment on attachment 8592135 [details] [review]
[gaia] julienw:1015194-fix-recipients-handling > mozilla-b2g:master

I think the approach is good, left some thoughts at GitHub as well.

Thanks!
Attachment #8592135 - Flags: feedback?(azasypkin) → feedback+
See Also: → 1234129
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.