Closed Bug 935917 Opened 12 years ago Closed 12 years ago

[Messages] When setting a message's body, we shouldn't try to parse it as HTML

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: lolimartinezcr, Assigned: julienw)

References

Details

(Keywords: sec-other)

Attachments

(1 file, 2 obsolete files)

patch v2
7.81 KB, patch
steveck
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release) Build ID: 20131025151332 Steps to reproduce: 1. Open Messaging app 2. Open the sent message WITH "<html>" content 3. Long press on it 4. Tap on Forward message option Actual results: Message field is empty Expected results: New message composer screen is shown with the message to be forwarded in the text field. The 'To' field is empty.
master 11/07 build: Gecko-336c939 Gaia-0c1587a
blocking-b2g: --- → 1.3?
Component: General → Gaia::SMS
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Sounds like a security bug, don't know how to change the flags.
Flags: needinfo?(stephouillon)
Flags: needinfo?(stephouillon)
Keywords: sec-other
Added as sec-other for investigation
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looks like the body just needs to be escaped.
Assignee: nobody → waldron.rick
Summary: [Messaging][Forward] Field "Message" is empty WHEN Forward is done AND content of message is "<html>" → [Messaging][Forward] Escape message body contents
Summary: [Messaging][Forward] Escape message body contents → [Messages][Forward] Escape message body contents
Comment on attachment 829543 [details] [review] https://github.com/mozilla-b2g/gaia/pull/13527 I wonder if we could not just make an "appendText" in compose.js, instead of using the same "append" that is used to append html content.
(In reply to Julien Wajsberg [:julienw] from comment #6) > Comment on attachment 829543 [details] [review] > https://github.com/mozilla-b2g/gaia/pull/13527 > > I wonder if we could not just make an "appendText" in compose.js, instead of > using the same "append" that is used to append html content. Is "html" really something that people send to each other, via messaging apps, and expect to be formatted for the recipient?
"append html content" is used when we get the text back from a contenteditable element. What I'm saying here is that (maybe) we should escape in compose.js when calling the exposed "append" (as opposed to the internal append). Or instead of escaping+deescaping, make the exposed "append" add a "addAsText: true" option when calling the internal "append". Not sure this makes sense?
Comment on attachment 829543 [details] [review] https://github.com/mozilla-b2g/gaia/pull/13527 I left a more precise comment on github.
(In reply to Julien Wajsberg [:julienw] from comment #8) > "append html content" is used when we get the text back from a > contenteditable element. > > What I'm saying here is that (maybe) we should escape in compose.js when > calling the exposed "append" (as opposed to the internal append). Or instead > of escaping+deescaping, make the exposed "append" add a "addAsText: true" > option when calling the internal "append". > > Not sure this makes sense? I'm having trouble following this and the comments on the PR. Do you want to take over the ticket?
is this a security concern from your team? thanks
Flags: needinfo?(stephouillon)
From the security perspective, current fix looks good. From what I've got from Julien's explanation, the discussion is about code design: I don't think it will have a security impact if one or the other solution is chosen (unless I miss something).
Flags: needinfo?(stephouillon)
Stéphanie, I was gonna ping you on this bug. We discussed with Rick yesterday and the fix will probably be heavier than expected. And it's a part of the code that deals with external input (the "sms" activity, and forwarding a sent or received messages). When the fix will land I'll flag you so that you know this is an area to check more precisely in your next audit.
Julien, sounds good.
Summary: [Messages][Forward] Escape message body contents → [Messages] When setting a message's body, we shouldn't try to parse it as HTML
Assignee: waldron.rick → felash
Attached patch patch v1 (obsolete) — — Splinter Review
I implemented a slightly different version of what I suggested in https://github.com/mozilla-b2g/gaia/pull/13527#discussion_r7661569 This was not working initially because of bug 939971, but fortunately I fixed this yesterday ;) Part of this code is from initial rwaldron's patch. See github PR at https://github.com/mozilla-b2g/gaia/pull/13878
Attachment #829543 - Attachment is obsolete: true
Attachment #829543 - Flags: review?(felash)
Attachment #8335379 - Flags: review?(schung)
Attachment #8335379 - Flags: feedback?(waldron.rick)
Comment on attachment 8335379 [details] [diff] [review] patch v1 Review of attachment 8335379 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/js/compose.js @@ +120,5 @@ > } else if (item.nodeName === 'IFRAME') { > // this iframe is generated by us > fragment.appendChild(item); > } else if (typeof item === 'string') { > + // note that the nbsp replacement is remove, per the above note. Oops, I removed this old comment in my current patch.
Triage: not blocking. Unless after Stephanie's review this really is a security risk, then please nominate again
blocking-b2g: 1.3? → ---
I don't really care whether it's a blocker or not, since it will land on master before the branching date. However, this is affecting a lot of uses cases: * forwarding * activity for messages with html-like tags (so I'd say a '<' character is enough to make things go wrong)
Blocks: 840527
Blocks: 919995
No longer blocks: 840527
Comment on attachment 8335379 [details] [diff] [review] patch v1 Review of attachment 8335379 [details] [diff] [review]: ----------------------------------------------------------------- This solution is quite straightforward and it works well except for some small concerns. Another question in https://github.com/mozilla-b2g/gaia/pull/13878#issuecomment-28976619 seems gecko issue but I'm not sure it's only reproducible on my device...I'll keep the flag since these are not major issues. ::: apps/sms/js/compose.js @@ +121,5 @@ > // this iframe is generated by us > fragment.appendChild(item); > } else if (typeof item === 'string') { > + // note that the nbsp replacement is remove, per the above note. > + item.replace(/ (?= )/g, '\u00A0').split('\n').forEach(function(line) { Maybe we don't need this step to preserve the white space since the container already have the style white-space: pre-wrap?
(In reply to Steve Chung [:steveck] from comment #19) > Comment on attachment 8335379 [details] [diff] [review] > patch v1 > > Review of attachment 8335379 [details] [diff] [review]: > ----------------------------------------------------------------- > > This solution is quite straightforward and it works well except for some > small concerns. Another question in > https://github.com/mozilla-b2g/gaia/pull/13878#issuecomment-28976619 seems > gecko issue but I'm not sure it's only reproducible on my device...I'll keep > the flag since these are not major issues. > > ::: apps/sms/js/compose.js > @@ +121,5 @@ > > // this iframe is generated by us > > fragment.appendChild(item); > > } else if (typeof item === 'string') { > > + // note that the nbsp replacement is remove, per the above note. > > + item.replace(/ (?= )/g, '\u00A0').split('\n').forEach(function(line) { > > Maybe we don't need this step to preserve the white space since the > container already have the style white-space: pre-wrap? Yep, this works perfectly without this. Too bad, I was so proud of this regexp ;)
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → ---
Attached patch patch v2 — — Splinter Review
Hey Steve, Here is the updated patch, I removed the adjacent spaces regular expression and updated the tests. https://github.com/mozilla-b2g/gaia/pull/13878 was updated with a separate commit containing only these changes to make it easier to review.
Attachment #8335379 - Attachment is obsolete: true
Attachment #8335379 - Flags: review?(schung)
Attachment #8335379 - Flags: feedback?(waldron.rick)
Attachment #8337623 - Flags: review?(schung)
Comment on attachment 8337623 [details] [diff] [review] patch v2 Review of attachment 8337623 [details] [diff] [review]: ----------------------------------------------------------------- This fixing looks good, only a concern in test case. Thanks for the patch!
Attachment #8337623 - Flags: review?(schung) → review+
msater: 2819f58c1215ae848a6c4772e0016783a5534946 Thanks!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Tested: 11/29 master gecko 9d4c5b2 gaia 77f58cb
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: