diff --git a/src/amo/components/AddonDetail.js b/src/amo/components/AddonDetail.js index 18f0ef96f75..2fd2df427f9 100644 --- a/src/amo/components/AddonDetail.js +++ b/src/amo/components/AddonDetail.js @@ -1,23 +1,56 @@ import React, { PropTypes } from 'react'; -import translate from 'core/i18n/translate'; - import AddonMeta from 'amo/components/AddonMeta'; import InstallButton from 'disco/components/InstallButton'; import LikeButton from 'amo/components/LikeButton'; import ScreenShots from 'amo/components/ScreenShots'; import SearchBox from 'amo/components/SearchBox'; +import translate from 'core/i18n/translate'; +import { nl2br, sanitizeHTML } from 'core/utils'; import 'amo/css/AddonDetail.scss'; -export class AddonDetail extends React.Component { +export const allowedDescriptionTags = [ + 'a', + 'abbr', + 'acronym', + 'b', + 'blockquote', + 'br', + 'code', + 'em', + 'i', + 'li', + 'ol', + 'strong', + 'ul', +]; + +class AddonDetail extends React.Component { static propTypes = { i18n: PropTypes.object, + addon: PropTypes.shape({ + name: PropTypes.string.isRequired, + authors: PropTypes.array.isRequired, + slug: PropTypes.string.isRequired, + }), } render() { - const { i18n } = this.props; + const { i18n, addon } = this.props; + + const authorList = addon.authors.map( + (author) => `${author.name}`); + + const title = i18n.sprintf( + // L10n: Example: The Add-On by The Author + i18n.gettext('%(addonName)s %(startSpan)sby %(authorList)s%(endSpan)s'), { + addonName: addon.name, + authorList: authorList.join(', '), + startSpan: '', + endSpan: '', + }); return (
@@ -28,13 +61,11 @@ export class AddonDetail extends React.Component {
-

Placeholder Add-on Title - by AwesomeAddons

- +

+
-

Lorem ipsum dolor sit amet, dicat graece partiendo cu usu. - Vis recusabo accusamus et.

+

@@ -54,17 +85,9 @@ export class AddonDetail extends React.Component {

{i18n.gettext('About this extension')}

-

Lorem ipsum dolor sit amet, dicat graece partiendo cu usu. Vis - recusabo accusamus et, vitae scriptorem in vel. Sed ei eleifend - molestiae deseruisse, sit mucius noster mentitum ex. Eu pro illum - iusto nemore, te legere antiopam sit. Suas simul ad usu, ex putent - timeam fierent eum. Dicam equidem cum cu. Vel ea vidit timeam.

- -

Eu nam dicant oportere, et per habeo euismod denique, te appetere - temporibus mea. Ad solum reprehendunt vis, sea eros accusata senserit - an, eam utinam theophrastus in. Debet consul vis ex. Mei an iusto - delicatissimi, ut timeam electram maiestatis nam, te petentium - intellegebat ius. Ei legere everti.

+
+
); diff --git a/src/amo/containers/DetailPage.js b/src/amo/containers/DetailPage.js index 239c349ba7f..2a958a4def1 100644 --- a/src/amo/containers/DetailPage.js +++ b/src/amo/containers/DetailPage.js @@ -1,13 +1,39 @@ -import React from 'react'; +import React, { PropTypes } from 'react'; +import { compose } from 'redux'; +import { asyncConnect } from 'redux-async-connect'; +import { connect } from 'react-redux'; import AddonDetail from 'amo/components/AddonDetail'; +import translate from 'core/i18n/translate'; +import { loadAddonIfNeeded } from 'core/utils'; + +export class DetailPage extends React.Component { + static propTypes = { + addon: PropTypes.object, + } -export default class DetailPage extends React.Component { render() { return (
- +
); } } + +function mapStateToProps(state, ownProps) { + const { slug } = ownProps.params; + return { + addon: state.addons[slug], + slug, + }; +} + +export default compose( + asyncConnect([{ + deferred: true, + promise: loadAddonIfNeeded, + }]), + connect(mapStateToProps), + translate({ withRef: true }), +)(DetailPage); diff --git a/src/core/purify.js b/src/core/purify.js index 32272f99082..3c4f56b6f44 100644 --- a/src/core/purify.js +++ b/src/core/purify.js @@ -1,4 +1,14 @@ import createDOMPurify from 'dompurify'; import universalWindow from 'core/window'; -export default createDOMPurify(universalWindow); +const purify = createDOMPurify(universalWindow); +export default purify; + +purify.addHook('afterSanitizeAttributes', (node) => { + // Set all elements owning target to target=_blank + // and add rel="noreferrer". + if ('target' in node) { + node.setAttribute('target', '_blank'); + node.setAttribute('rel', 'noreferrer'); + } +}); diff --git a/src/core/utils.js b/src/core/utils.js index a7083d4428d..e41a3068d5c 100644 --- a/src/core/utils.js +++ b/src/core/utils.js @@ -1,6 +1,11 @@ import camelCase from 'camelcase'; import config from 'config'; +import { loadEntities } from 'core/actions'; +import { fetchAddon } from 'core/api'; +import log from 'core/logger'; +import purify from 'core/purify'; + export function gettext(str) { return str; } @@ -61,3 +66,39 @@ export function getClientApp(userAgentString) { export function isValidClientApp(value, { _config = config } = {}) { return _config.get('validClientApplications').includes(value); } + +export function sanitizeHTML(text, allowTags = []) { + // TODO: Accept tags to allow and run through dom-purify. + return { + __html: purify.sanitize(text, { ALLOWED_TAGS: allowTags }), + }; +} + +// Convert new lines to HTML breaks. +export function nl2br(text) { + return text.replace(/(?:\r\n|\r|\n)/g, '
'); +} + +export function findAddon(state, slug) { + return state.addons[slug]; +} + +// asyncConnect() helper for loading an add-on by slug. +// +// This accepts component properties and returns a promise +// that resolves when the requested add-on has been dispatched. +// If the add-on has already been fetched, the add-on value is returned. +// +export function loadAddonIfNeeded( + { store: { dispatch, getState }, params: { slug } } +) { + const state = getState(); + const addon = findAddon(state, slug); + if (addon) { + log.info(`Found addon ${addon.id} in state`); + return addon; + } + log.info(`Fetching addon ${slug} from API`); + return fetchAddon({ slug, api: state.api }) + .then(({ entities }) => dispatch(loadEntities(entities))); +} diff --git a/src/disco/components/Addon.js b/src/disco/components/Addon.js index 5aae876db37..3a6b871a5fc 100644 --- a/src/disco/components/Addon.js +++ b/src/disco/components/Addon.js @@ -4,8 +4,8 @@ import React, { PropTypes } from 'react'; import ReactCSSTransitionGroup from 'react-addons-css-transition-group'; import { connect } from 'react-redux'; import translate from 'core/i18n/translate'; -import purify from 'core/purify'; +import { sanitizeHTML } from 'core/utils'; import config from 'config'; import themeAction, { getThemeData } from 'disco/themePreview'; import tracking from 'core/tracking'; @@ -44,22 +44,6 @@ import { import 'disco/css/Addon.scss'; -purify.addHook('afterSanitizeAttributes', (node) => { - // Set all elements owning target to target=_blank - // and add rel="noreferrer". - if ('target' in node) { - node.setAttribute('target', '_blank'); - node.setAttribute('rel', 'noreferrer'); - } -}); - -function sanitizeHTML(text, allowTags = []) { - // TODO: Accept tags to allow and run through dom-purify. - return { - __html: purify.sanitize(text, { ALLOWED_TAGS: allowTags }), - }; -} - export class Addon extends React.Component { static propTypes = { accentcolor: PropTypes.string, diff --git a/src/search/containers/AddonPage/index.js b/src/search/containers/AddonPage/index.js index f9c0ac4cd45..da21305132b 100644 --- a/src/search/containers/AddonPage/index.js +++ b/src/search/containers/AddonPage/index.js @@ -1,9 +1,7 @@ import React, { PropTypes } from 'react'; import { connect } from 'react-redux'; import { asyncConnect } from 'redux-async-connect'; -import { fetchAddon } from 'core/api'; -import { loadEntities } from 'core/actions'; -import { gettext as _ } from 'core/utils'; +import { gettext as _, loadAddonIfNeeded } from 'core/utils'; import NotFound from 'core/components/NotFound'; import JsonData from 'search/components/JsonData'; @@ -127,20 +125,6 @@ function mapStateToProps(state, ownProps) { }; } -export function findAddon(state, slug) { - return state.addons[slug]; -} - -export function loadAddonIfNeeded({ store: { dispatch, getState }, params: { slug } }) { - const state = getState(); - const addon = findAddon(state, slug); - if (addon) { - return addon; - } - return fetchAddon({ slug, api: state.api }) - .then(({ entities }) => dispatch(loadEntities(entities))); -} - const CurrentAddonPage = asyncConnect([{ deferred: true, promise: loadAddonIfNeeded, diff --git a/tests/client/amo/components/TestAddonDetail.js b/tests/client/amo/components/TestAddonDetail.js new file mode 100644 index 00000000000..68c2b41d424 --- /dev/null +++ b/tests/client/amo/components/TestAddonDetail.js @@ -0,0 +1,202 @@ +import React from 'react'; +import { findDOMNode } from 'react-dom'; +import { + findRenderedComponentWithType, + renderIntoDocument, +} from 'react-addons-test-utils'; + +import AddonDetail, { allowedDescriptionTags } + from 'amo/components/AddonDetail'; +import I18nProvider from 'core/i18n/Provider'; +import InstallButton from 'disco/components/InstallButton'; + +import { getFakeI18nInst } from 'tests/client/helpers'; + + +export const fakeAddon = { + name: 'Chill Out', + slug: 'chill-out', + authors: [{ + name: 'Krupa', + url: 'http://olympia.dev/en-US/firefox/user/krupa/', + }], + summary: 'This is a summary of the chill out add-on', + description: 'This is a longer description of the chill out add-on', +}; + +function render({ addon = fakeAddon, ...customProps } = {}) { + const i18n = getFakeI18nInst(); + const props = { i18n, addon, ...customProps }; + + return findRenderedComponentWithType(renderIntoDocument( + + + + ), AddonDetail); +} + +function renderAsDOMNode(...args) { + const root = render(...args); + return findDOMNode(root); +} + +describe('AddonDetail', () => { + it('renders a name', () => { + const rootNode = renderAsDOMNode(); + assert.include(rootNode.querySelector('h1').textContent, + 'Chill Out'); + }); + + it('renders a single author', () => { + const authorUrl = 'http://olympia.dev/en-US/firefox/user/krupa/'; + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + authors: [{ + name: 'Krupa', + url: authorUrl, + }], + }, + }); + assert.equal(rootNode.querySelector('h1').textContent, + 'Chill Out by Krupa'); + assert.equal(rootNode.querySelector('h1 a').attributes.href.value, + authorUrl); + }); + + it('renders multiple authors', () => { + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + authors: [{ + name: 'Krupa', + url: 'http://olympia.dev/en-US/firefox/user/krupa/', + }, { + name: 'Fligtar', + url: 'http://olympia.dev/en-US/firefox/user/fligtar/', + }], + }, + }); + assert.equal(rootNode.querySelector('h1').textContent, + 'Chill Out by Krupa, Fligtar'); + }); + + it('sanitizes a title', () => { + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + name: '', + }, + }); + // Make sure an actual script tag was not created. + assert.equal(rootNode.querySelector('h1 script'), null); + // Make sure the script HTML has been escaped and removed. + assert.notInclude(rootNode.querySelector('h1').textContent, 'script'); + }); + + it('allows certain HTML tags in the title', () => { + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + authors: [{ + name: 'Krupa', + url: 'http://olympia.dev/en-US/firefox/user/krupa/', + }], + }, + }); + // Make sure these tags were whitelisted. + assert.equal(rootNode.querySelector('h1 span a').textContent, 'Krupa'); + // Make sure the santizer didn't strip the class attribute: + const byLine = rootNode.querySelector('h1 span'); + assert.ok(byLine.attributes.class, 'the class attribute is not empty'); + }); + + it('configures the install button', () => { + const root = findRenderedComponentWithType(render(), InstallButton); + assert.equal(root.props.slug, fakeAddon.slug); + }); + + it('renders a summary', () => { + const rootNode = renderAsDOMNode(); + assert.include(rootNode.querySelector('div.description').textContent, + fakeAddon.summary); + }); + + it('sanitizes a summary', () => { + const scriptHTML = ''; + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + summary: scriptHTML, + }, + }); + // Make sure an actual script tag was not created. + assert.equal(rootNode.querySelector('div.description script'), null); + // Make sure the script HTML has been escaped and removed. + assert.notInclude(rootNode.querySelector('div.description').textContent, + scriptHTML); + }); + + it('renders a description', () => { + const rootNode = renderAsDOMNode(); + assert.include(rootNode.querySelector('section.about').textContent, + fakeAddon.description); + }); + + it('sanitizes bad description HTML', () => { + const scriptHTML = ''; + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + description: scriptHTML, + }, + }); + // Make sure an actual script tag was not created. + assert.equal(rootNode.querySelector('section.about script'), null); + // Make sure the script HTML has been escaped and removed. + assert.notInclude(rootNode.querySelector('section.about').textContent, + scriptHTML); + }); + + it('converts new lines in the description to breaks', () => { + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + description: '\n\n\n', + }, + }); + assert.equal(rootNode.querySelectorAll('section.about br').length, 3); + }); + + it('preserves certain HTML tags in the description', () => { + let description = ''; + const allowedTags = [...allowedDescriptionTags]; + // Ignore
since it's checked elsewhere. + allowedTags.splice(allowedTags.indexOf('br'), 1); + + for (const tag of allowedTags) { + description = `${description} <${tag}>placeholder`; + } + const rootNode = renderAsDOMNode({ + addon: { ...fakeAddon, description }, + }); + for (const tagToCheck of allowedTags) { + assert.equal( + rootNode.querySelectorAll(`section.about ${tagToCheck}`).length, 1, + `${tagToCheck} tag was not whitelisted`); + } + }); + + it('strips dangerous HTML tag attributes from description', () => { + const rootNode = renderAsDOMNode({ + addon: { + ...fakeAddon, + description: + 'placeholder', + }, + }); + const anchor = rootNode.querySelector('section.about a'); + assert.equal(anchor.attributes.onclick, null); + assert.equal(anchor.attributes.href, null); + }); +}); diff --git a/tests/client/amo/containers/TestDetail.js b/tests/client/amo/containers/TestDetail.js index 837b9311a91..5cc3eb1ed2b 100644 --- a/tests/client/amo/containers/TestDetail.js +++ b/tests/client/amo/containers/TestDetail.js @@ -4,27 +4,42 @@ import { findRenderedComponentWithType, renderIntoDocument, } from 'react-addons-test-utils'; +import { Provider } from 'react-redux'; +import createStore from 'amo/store'; import { getFakeI18nInst } from 'tests/client/helpers'; import DetailPage from 'amo/containers/DetailPage'; import I18nProvider from 'core/i18n/Provider'; -import translate from 'core/i18n/translate'; -function renderDetailPage({ ...props }) { - const MyDetailPage = translate({ withRef: true })(DetailPage); +import { fakeAddon } from '../components/TestAddonDetail'; + +function render( + { + props = { params: { slug: fakeAddon.slug } }, + state = { + addons: { + [fakeAddon.slug]: fakeAddon, + }, + }, + } = {} +) { + const store = createStore(state); return findRenderedComponentWithType(renderIntoDocument( - + + + - ), MyDetailPage).getWrappedInstance(); + ), DetailPage); } describe('DetailPage', () => { - it('renders a heading', () => { - const root = renderDetailPage(); + it('renders an add-on', () => { + const root = render(); const rootNode = findDOMNode(root); - assert.include(rootNode.querySelector('h1').textContent, 'Placeholder Add-on'); + assert.include(rootNode.querySelector('h1').textContent, + `${fakeAddon.name} by ${fakeAddon.authors[0].name}`); }); }); diff --git a/tests/client/core/test_utils.js b/tests/client/core/test_utils.js index 6a228061e87..ae0ca20190f 100644 --- a/tests/client/core/test_utils.js +++ b/tests/client/core/test_utils.js @@ -1,11 +1,18 @@ +import * as actions from 'core/actions'; +import * as api from 'core/api'; import { camelCaseProps, convertBoolean, + findAddon, getClientApp, getClientConfig, isValidClientApp, + loadAddonIfNeeded, + nl2br, } from 'core/utils'; +import { unexpectedSuccess } from 'tests/client/helpers'; + describe('camelCaseProps', () => { const input = { underscore_delimited: 'underscore', @@ -207,3 +214,120 @@ describe('isValidClientApp', () => { assert.equal(isValidClientApp('whatever', { _config }), false); }); }); + +describe('findAddon', () => { + const addon = sinon.stub(); + const state = { + addons: { + 'the-addon': addon, + }, + }; + + it('finds the add-on in the state', () => { + assert.strictEqual(findAddon(state, 'the-addon'), addon); + }); + + it('does not find the add-on in the state', () => { + assert.strictEqual(findAddon(state, 'different-addon'), undefined); + }); +}); + +describe('loadAddonIfNeeded', () => { + const apiState = { token: 'my.jwt.token' }; + const loadedSlug = 'my-addon'; + let loadedAddon; + let dispatch; + + beforeEach(() => { + loadedAddon = sinon.stub(); + dispatch = sinon.spy(); + }); + + function makeProps(slug) { + return { + store: { + getState: () => ({ + addons: { + [loadedSlug]: loadedAddon, + }, + api: apiState, + }), + dispatch, + }, + params: { slug }, + }; + } + + it('returns the add-on if loaded', () => { + assert.strictEqual(loadAddonIfNeeded(makeProps(loadedSlug)), loadedAddon); + }); + + it('loads the add-on if it is not loaded', () => { + const slug = 'other-addon'; + const props = makeProps(slug, apiState); + const addon = sinon.stub(); + const entities = { [slug]: addon }; + const mockApi = sinon.mock(api); + mockApi + .expects('fetchAddon') + .once() + .withArgs({ slug, api: apiState }) + .returns(Promise.resolve({ entities })); + const action = sinon.stub(); + const mockActions = sinon.mock(actions); + mockActions + .expects('loadEntities') + .once() + .withArgs(entities) + .returns(action); + return loadAddonIfNeeded(props).then(() => { + assert(dispatch.calledWith(action), 'dispatch not called'); + mockApi.verify(); + mockActions.verify(); + }); + }); + + it('handles 404s when loading the add-on', () => { + const slug = 'other-addon'; + const props = makeProps(slug, apiState); + const mockApi = sinon.mock(api); + mockApi + .expects('fetchAddon') + .once() + .withArgs({ slug, api: apiState }) + .returns(Promise.reject(new Error('Error accessing API'))); + const mockActions = sinon.mock(actions); + mockActions + .expects('loadEntities') + .never(); + return loadAddonIfNeeded(props) + .then(unexpectedSuccess, + () => { + assert(!dispatch.called, 'dispatch called'); + mockApi.verify(); + mockActions.verify(); + }); + }); +}); + +describe('nl2br', () => { + it('converts \n to
', () => { + assert.equal(nl2br('\n'), '
'); + }); + + it('converts \r to
', () => { + assert.equal(nl2br('\r'), '
'); + }); + + it('converts \r\n to
', () => { + assert.equal(nl2br('\r\n'), '
'); + }); + + it('converts multiple new lines to multiple breaks', () => { + assert.equal(nl2br('\n\n'), '

'); + }); + + it('converts multiple new lines (Windows) to multiple breaks', () => { + assert.equal(nl2br('\r\n\r\n'), '

'); + }); +}); diff --git a/tests/client/search/containers/TestAddonPage.js b/tests/client/search/containers/TestAddonPage.js index f2341d89359..fac1f5609ba 100644 --- a/tests/client/search/containers/TestAddonPage.js +++ b/tests/client/search/containers/TestAddonPage.js @@ -2,11 +2,8 @@ import React from 'react'; import { renderIntoDocument } from 'react-addons-test-utils'; import { findDOMNode } from 'react-dom'; import { Provider } from 'react-redux'; -import AddonPage, { findAddon, loadAddonIfNeeded } from 'search/containers/AddonPage'; +import AddonPage from 'search/containers/AddonPage'; import createStore from 'search/store'; -import * as actions from 'core/actions'; -import * as api from 'core/api'; -import { unexpectedSuccess } from 'tests/client/helpers'; describe('AddonPage', () => { const basicAddon = { @@ -198,99 +195,4 @@ describe('AddonPage', () => { const root = render({ state: initialState, props: { params: { slug: 'other-addon' } } }); assert(root.querySelector('h1').textContent.includes("we can't find what you're looking for")); }); - - describe('findAddon', () => { - const addon = sinon.stub(); - const state = { - addons: { - 'the-addon': addon, - }, - }; - - it('finds the add-on in the state', () => { - assert.strictEqual(findAddon(state, 'the-addon'), addon); - }); - - it('does not find the add-on in the state', () => { - assert.strictEqual(findAddon(state, 'different-addon'), undefined); - }); - }); - - describe('loadAddonIfNeeded', () => { - const apiState = { token: 'my.jwt.token' }; - const loadedSlug = 'my-addon'; - let loadedAddon; - let dispatch; - - beforeEach(() => { - loadedAddon = sinon.stub(); - dispatch = sinon.spy(); - }); - - function makeProps(slug) { - return { - store: { - getState: () => ({ - addons: { - [loadedSlug]: loadedAddon, - }, - api: apiState, - }), - dispatch, - }, - params: { slug }, - }; - } - - it('returns the add-on if loaded', () => { - assert.strictEqual(loadAddonIfNeeded(makeProps(loadedSlug)), loadedAddon); - }); - - it('loads the add-on if it is not loaded', () => { - const slug = 'other-addon'; - const props = makeProps(slug, apiState); - const addon = sinon.stub(); - const entities = { [slug]: addon }; - const mockApi = sinon.mock(api); - mockApi - .expects('fetchAddon') - .once() - .withArgs({ slug, api: apiState }) - .returns(Promise.resolve({ entities })); - const action = sinon.stub(); - const mockActions = sinon.mock(actions); - mockActions - .expects('loadEntities') - .once() - .withArgs(entities) - .returns(action); - return loadAddonIfNeeded(props).then(() => { - assert(dispatch.calledWith(action), 'dispatch not called'); - mockApi.verify(); - mockActions.verify(); - }); - }); - - it('handles 404s when loading the add-on', () => { - const slug = 'other-addon'; - const props = makeProps(slug, apiState); - const mockApi = sinon.mock(api); - mockApi - .expects('fetchAddon') - .once() - .withArgs({ slug, api: apiState }) - .returns(Promise.reject(new Error('Error accessing API'))); - const mockActions = sinon.mock(actions); - mockActions - .expects('loadEntities') - .never(); - return loadAddonIfNeeded(props) - .then(unexpectedSuccess, - () => { - assert(!dispatch.called, 'dispatch called'); - mockApi.verify(); - mockActions.verify(); - }); - }); - }); });