Closed Bug 1237546 Opened 6 years ago Closed 6 years ago

SapiService::Speak should encode to entities

Categories

(Core :: Web Speech, defect)

All
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- affected
firefox49 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

To support pitch controll, we use XML for Speak parameter on SAPI backend.  We should encode some character to entity
Comment on attachment 8705049 [details] [diff] [review]
Convert character to entity for SapiService::Speak

When using speechSynthesis.speak(new SpeechSynthesisUtterance("&") on SAPI backend, browser speaks as '&' only.  It should be "&', 'a', 'm', 'p' and ';'.

Our SAPI implementation use XML to support pitch value.  So some characters should be converted to entity.
Attachment #8705049 - Flags: review?(eitan)
Two questions before I review:
1. Don't we have an actual library for escaping XML? Seems like a very common and security sensitive thing to do.
2. Do Mac/Linux need followup patches as well? I think we may need to escape or sanitize SSML.
Flags: needinfo?(m_kato)
(In reply to Eitan Isaacson [:eeejay] from comment #3)
> Two questions before I review:
> 1. Don't we have an actual library for escaping XML? Seems like a very
> common and security sensitive thing to do.

ContentUtils has internal method into StringBuilder, but it doesn't export to others and requires nsINode and nsTextFragment.  If we should add this to ContentUtils, I will add it.

> 2. Do Mac/Linux need followup patches as well? I think we may need to escape
> or sanitize SSML.

OSX API doesn't recognize SSML.

speechd can use SSML mode, but to use SSML, we need call spd_set_data_mode(handle, SPD_DATA_SSML).
Flags: needinfo?(m_kato)
(In reply to Makoto Kato [:m_kato] from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > Two questions before I review:
> > 1. Don't we have an actual library for escaping XML? Seems like a very
> > common and security sensitive thing to do.
> 
> ContentUtils has internal method into StringBuilder, but it doesn't export
> to others and requires nsINode and nsTextFragment.  If we should add this to
> ContentUtils, I will add it.
> 

I'm not sure. Let's see what smaug thinks.
Flags: needinfo?(bugs)
Whatever StringBuilder does is very innerHTML specific and super performance hot code.
But, perhaps you could factor some of EncodeTextFragment to a macro which is then called twice in that method and used also for a method needed in this bug.
Or even better would be to not use macro, but some template method which should get inlined.

something like
void templace<T>EncodeText(const T aChar, nsAString& aOut) 
{
  switch (aChar) {
    case '<':
     aOut.AppendLiteral("&lt;");
     break;
    case '>':
      aOut.AppendLiteral("&gt;");
      break;
    case '&':
      aOut.AppendLiteral("&amp;");
      break;
    case 0x00A0:
      aOut.AppendLiteral("&nbsp;");
      break;
     default:
       aOut.Append(aChar);
       break;
  }
}
Flags: needinfo?(bugs)
Blocks: 1234654
Comment on attachment 8705049 [details] [diff] [review]
Convert character to entity for SapiService::Speak

Review of attachment 8705049 [details] [diff] [review]:
-----------------------------------------------------------------

This is good for now. We may need to figure out a way to share this across services or escape strings before they reach the service.
Attachment #8705049 - Flags: review?(eitan) → review+
mach try -b do -p win32,win64 -u all -t none
Do you plan to land this?

This might be last minute, but what do you think of using nsEscapeHTML2?

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsEscape.h#56
Flags: needinfo?(m_kato)
Blocks: 1268633
Attachment #8705049 - Attachment is obsolete: true
Flags: needinfo?(m_kato)
Attachment #8747648 - Flags: review?(eitan)
Attachment #8747648 - Flags: review?(eitan) → review+
https://hg.mozilla.org/mozilla-central/rev/8ed42b270f7e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1279842
You need to log in before you can comment on or make changes to this bug.