Fix Bug 1457233 - Allow AS router message content to include subset of HTML #4162

piatra · 2018-05-22T20:19:51Z

I've tried including fluent as external modules that resulted in some issues where the build step would complain about the syntax and fail. Alternatively using the compat version requires the regenerator-runtime and in order to build you would have to include this module before fluent, this would result in too many external modules.

Similar problems occur when bundling fluent with AS:

syntax errors force to use compat which also implies a dependency on regenerator-runtime
- maybe a different preprocessing step?
the spread syntax results in an error for me even if I rebased on top of the PR which added the plugin support
at build step it expects document to be globally available

But it works if you get through all of that, although I haven't figured out how to render links yet.

The extra logic involved (LocalizationProvider) is only temporary until projectfluent/fluent.js#202 is fixed.

k88hudson

The reason the spread syntax isn't working is that node_modules is excluded in the rule in webpack.config.js. If you change /node_modules/ to /node_modules\/(?!(fluent|fluent-react)\/).*/, it should work. That said, using externals will side-step this problem.

k88hudson · 2018-05-23T03:51:33Z

system-addon/content-src/activity-stream-prerender.jsx

@@ -1,3 +1,4 @@
+import "regenerator-runtime/runtime";


Hm, I see the issue when I tried building this. The syntax that seems to be causing the issue isfor await (const x of iterable), which seems weird to me – we could try to figure this out, however I think the easiest way to deal with this is to use the external (i.e. manually copy node_modules/fluent/fluent.js and node_modules/fluent-react/fluent-react.js). Have you tried that?

After some more investigation async iterator support seemed to have landed in acorn 5.4.0 (see https://github.com/acornjs/acorn/releases/tag/5.4.0), so the same bug that is causing our object rest spread bug is also causing this :(

k88hudson · 2018-05-23T03:51:59Z

system-addon/content-src/asrouter/asrouter-content.jsx

@@ -1,6 +1,8 @@
 import {actionCreators as ac, ASRouterActions as ra} from "common/Actions.jsm";
 import {OUTGOING_MESSAGE_NAME as AS_GENERAL_OUTGOING_MESSAGE_NAME} from "content-src/lib/init-store";
 import {ImpressionsWrapper} from "./components/ImpressionsWrapper/ImpressionsWrapper";
+import {LocalizationProvider} from "fluent-react";
+import {MessageContext} from "fluent/compat";


if you're using fluent/compat you'll need to npm install fluent

piatra · 2018-05-25T10:46:16Z

~~I've only whitelisted <b> tags.~~ If any other tag is found, it will be stripped, its text content will be used.

Right now anchors are rendered using ASRouterAnchor but snippets can contain links in arbitrary positions (not just at the end of the message). This requires some changes on the snippets side, anchors will be wrapped in <cta> tags and the URL will be provided separately in the payload like the endpoint demo we currently have.

k88hudson

This works great! Can you maybe add a schema and/or some documentation to content-src/asrouter/schemas that defines the allowed tags?

k88hudson · 2018-06-05T15:36:36Z

system-addon/content-src/asrouter/asrouter-content.jsx

+function convertLinks(links) {
+  if (links) {
+    return Object.keys(links).reduce((acc, linkTag) => {
+      acc[linkTag] = <a href={links[linkTag].url} />;


Can you whitelist allowed protocols here, so at least javascript: isn't allowed?

actually you can use safeURI here for that

k88hudson · 2018-06-05T15:38:18Z

webpack.prerender.config.js

@@ -24,6 +24,7 @@ module.exports = Object.assign({}, webpackConfig, {
    "react-dom": "commonjs react-dom"
  },
  plugins: [
-    new webpack.BannerPlugin(banner)
+    new webpack.BannerPlugin(banner),
+    new webpack.DefinePlugin({document: {createElement: () => {}}})


looool hax :P Can you add a comment about why this is needed?

k88hudson · 2018-06-05T15:43:08Z

system-addon/content-src/asrouter/asrouter-content.jsx

+          <LocalizationProvider messages={generateMessages(this.state.message.content.text)}>
+            <SimpleSnippet
+              {...this.state.message}
+              richText={<RichText text={this.state.message.content} />}


I would do text={this.state.message.content.text} links={this.state.message.content.links} or just {...this.state.message.content}, passing message.content as text is kind of confusing

k88hudson · 2018-06-05T15:47:14Z

package-lock.json

@@ -392,14 +392,14 @@
      "dev": true
    },
    "asn1.js": {
-      "version": "4.9.2",


this seems like it might be a lot of changes for just adding a couple of dependencies, what version of npm are you using out of curiosity?

k88hudson · 2018-06-05T15:47:37Z

system-addon/content-src/asrouter/asrouter-content.jsx

@@ -53,6 +55,45 @@ function shouldSendImpressionOnUpdate(nextProps, prevProps) {
  return (nextProps.message.id && (!prevProps.message || prevProps.message.id !== nextProps.message.id));
 }

+function* generateMessages(content) {


Any particular reason this is a generator and not an async function?

k88hudson · 2018-06-05T15:47:42Z

system-addon/content-src/asrouter/asrouter-content.jsx

+  return null;
+}
+
+function RichText(props) {


Can you add a small comment about what this is for

piatra · 2018-06-06T16:38:14Z

Documentation here https://github.com/mozilla/activity-stream/blob/7e544945fbe0cba46eba9387e4c593e98d8f9f55/content-src/asrouter/schemas/message-format.md
And updated the schema as well.

piatra · 2018-06-07T10:10:55Z

Not sure what to do about the package-lock.json diff.

npm --version
5.6.0

k88hudson

Great work! I tested with this https://gist.github.com/k88hudson/2d627d17ebd3e622e5b05e060d98b182 and it's working great, plus my XSS attack was blocked 😜

k88hudson reviewed May 23, 2018

View reviewed changes

piatra force-pushed the bug1457233 branch from 77e97ac to 8e72772 Compare May 23, 2018 20:13

k88hudson assigned piatra May 23, 2018

k88hudson added the PR / In progress don't review yet label May 23, 2018

piatra added PR / Needs review and removed PR / In progress don't review yet labels May 25, 2018

piatra assigned k88hudson and unassigned piatra and k88hudson May 25, 2018

piatra added PR / In progress don't review yet and removed PR / Needs review labels May 25, 2018

piatra added PR / Needs review and removed PR / In progress don't review yet labels Jun 4, 2018

piatra force-pushed the bug1457233 branch from 6764b8e to ce66188 Compare June 4, 2018 15:44

piatra assigned k88hudson Jun 4, 2018

k88hudson suggested changes Jun 5, 2018

View reviewed changes

k88hudson added PR / Needs work and removed PR / Needs review labels Jun 5, 2018

k88hudson assigned piatra and unassigned k88hudson Jun 5, 2018

piatra force-pushed the bug1457233 branch from 7b5f26d to 6e3220c Compare June 6, 2018 18:17

piatra mentioned this pull request Jun 7, 2018

Fix Bug 1457233 - Use custom metric attributes for links #4196

Merged

piatra added 6 commits June 7, 2018 12:06

Fix bug 1457233 - Allow a subset of HTML in snippet content

4269704

Convert links in snippet messages

82e52cf

Add comment about fluent-react build step

c0d4688

Use safeURI

9f5ec90

Remove generator usage

433dd69

Bad path

d3ad612

Update schema with allowed HTML

51bbb28

piatra force-pushed the bug1457233 branch from 00b4c1f to 51bbb28 Compare June 7, 2018 10:06

piatra added PR / Needs review and removed PR / Needs work labels Jun 7, 2018

piatra assigned k88hudson and unassigned piatra Jun 7, 2018

k88hudson approved these changes Jun 7, 2018

View reviewed changes

k88hudson added PR / R+ and removed PR / Needs review labels Jun 7, 2018

k88hudson assigned piatra and unassigned k88hudson Jun 7, 2018

piatra merged commit 75050a9 into mozilla:master Jun 8, 2018

Fix Bug 1457233 - Allow AS router message content to include subset of HTML #4162

Fix Bug 1457233 - Allow AS router message content to include subset of HTML #4162

Uh oh!

Conversation

piatra commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k88hudson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piatra commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k88hudson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piatra commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piatra commented Jun 7, 2018

Uh oh!

k88hudson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

piatra commented May 22, 2018 •

edited

Loading

piatra commented May 25, 2018 •

edited

Loading

piatra commented Jun 6, 2018 •

edited

Loading