Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop .textContent IE8 polyfill and rewrite escaping tests against public API #11331

Merged
7 changes: 0 additions & 7 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,6 @@ describe('ReactDOMServer', () => {
expect(response).toMatch(new RegExp('<img data-reactroot=""' + '/>'));
});

it('should generate simple markup for attribute with `>` symbol', () => {
var response = ReactDOMServer.renderToString(<img data-attr=">" />);
expect(response).toMatch(
new RegExp('<img data-attr="&gt;" data-reactroot=""' + '/>'),
);
});

it('should generate comment markup for component returns null', () => {
class NullComponent extends React.Component {
render() {
Expand Down

This file was deleted.

66 changes: 66 additions & 0 deletions packages/react-dom/src/__tests__/escapeTextForBrowser-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

var React;
var ReactDOMServer;

describe('escapeTextForBrowser', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOMServer = require('react-dom/server');
});

it('ampersand is escaped when passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{'&'}</span>);
expect(response).toMatch('<span data-reactroot="">&amp;</span>');
});

it('double quote is escaped when passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{'"'}</span>);
expect(response).toMatch('<span data-reactroot="">&quot;</span>');
});

it('single quote is escaped when passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{"'"}</span>);
expect(response).toMatch('<span data-reactroot="">&#x27;</span>');
});

it('greater than entity is escaped when passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{'>'}</span>);
expect(response).toMatch('<span data-reactroot="">&gt;</span>');
});

it('lower than entity is escaped when passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{'<'}</span>);
expect(response).toMatch('<span data-reactroot="">&lt;</span>');
});

it('number is correctly passed as text content', () => {
var response = ReactDOMServer.renderToString(<span>{42}</span>);
expect(response).toMatch('<span data-reactroot="">42</span>');
});

it('number is escaped to string when passed as text content', () => {
var response = ReactDOMServer.renderToString(<img data-attr={42} />);
expect(response).toMatch('<img data-attr="42" data-reactroot=""/>');
});

it('escape text content representing a script tag', () => {
var response = ReactDOMServer.renderToString(
<span>{'<script type=\'\' src=""></script>'}</span>,
);
expect(response).toMatch(
'<span data-reactroot="">&lt;script type=&#x27;&#x27; ' +
'src=&quot;&quot;&gt;&lt;/script&gt;</span>',
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,67 @@

'use strict';

var React;
var ReactDOMServer;

describe('quoteAttributeValueForBrowser', () => {
// TODO: can we express this test with only public API?
var quoteAttributeValueForBrowser = require('../shared/quoteAttributeValueForBrowser')
.default;
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOMServer = require('react-dom/server');
});

it('should escape boolean to string', () => {
expect(quoteAttributeValueForBrowser(true)).toBe('"true"');
expect(quoteAttributeValueForBrowser(false)).toBe('"false"');
it('ampersand is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr="&" />);
expect(response).toMatch('<img data-attr="&amp;" data-reactroot=""/>');
});

it('should escape object to string', () => {
var escaped = quoteAttributeValueForBrowser({
toString: function() {
return 'ponys';
},
});
it('double quote is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr={'"'} />);
expect(response).toMatch('<img data-attr="&quot;" data-reactroot=""/>');
});

it('single quote is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr="'" />);
expect(response).toMatch('<img data-attr="&#x27;" data-reactroot=""/>');
});

expect(escaped).toBe('"ponys"');
it('greater than entity is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr=">" />);
expect(response).toMatch('<img data-attr="&gt;" data-reactroot=""/>');
});

it('should escape number to string', () => {
expect(quoteAttributeValueForBrowser(42)).toBe('"42"');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question.

it('lower than entity is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr="<" />);
expect(response).toMatch('<img data-attr="&lt;" data-reactroot=""/>');
});

it('should escape string', () => {
var escaped = quoteAttributeValueForBrowser(
'<script type=\'\' src=""></script>',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test where this is literally the argument value.

Copy link
Contributor Author

@jeremenichelli jeremenichelli Nov 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon I'm thinking of creating tests for escapeText through the ReactDOMServer.renderToString API like these ones, do you think that it would make sense to write them? Also, if you do, shouldn't this test be placed there where the script would actually run?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of creating tests for escapeText thourgh the ReactDOMServer.renderToString API like these ones

Sounds goood.

Also, if you do, shouldn't this test be placed there where the script would actually run?

Not sure what you mean. We just want to verify <script> can't be injected as attribute value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, will add test on both escapeText and quoteAttribute tests.

it('number is escaped to string inside attributes', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon from here, tests for number, object and script tags on attributes have been added back. Above tests for text content in nodes contemplate these cases too, let me know if you ahve any more concerns or something that I might have missed.

var response = ReactDOMServer.renderToString(<img data-attr={42} />);
expect(response).toMatch('<img data-attr="42" data-reactroot=""/>');
});

it('object is passed to a string inside attributes', () => {
var sampleObject = {
toString: function() {
return 'ponys';
},
};

var response = ReactDOMServer.renderToString(
<img data-attr={sampleObject} />,
);
expect(escaped).not.toContain('<');
expect(escaped).not.toContain('>');
expect(escaped).not.toContain("'");
expect(escaped.substr(1, -1)).not.toContain('"');
expect(response).toMatch('<img data-attr="ponys" data-reactroot=""/>');
});

escaped = quoteAttributeValueForBrowser('&');
expect(escaped).toBe('"&amp;"');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the nice thing about these old tests is they didn't assert how we escape, only that we do escape. Which opens the door for changing that in the future.

Maybe it's not too important though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I didn't like form this test is that thy are just testing a switch JavaScript statement by doing it directly from the module. These internal modules pass thourgh other checks and algorithms when applied, so someone could modify that code, prevent the script to actually escape the characters and test would still pass, something that won't happen now 😃

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an ideal test would actually create a DOM node, put content into innerHTML and then assert that it only has a text node child with expected content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that could be one way. Though werid since this is used only on the server package, maybe adding cases from renderToStaticMarkup would be valuable.

Copy link
Collaborator

@gaearon gaearon Nov 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though werid since this is used only on the server package

Not sure what you mean. The end goal of rendering to a string is that the HTML shows up in somebody's browser :-) This is exactly what an integration test should verify: that what shows up in the browser would have been consistent with what we expect (as opposed to a dangerous script tag).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood it better after writing my response but didn't want to edit it.

Yeah I guess that would mimic the text string from server to node conversion better than just testing the string. Let me know if you are planning on migrating to that. I might think about revisiting these tests in the future if necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd like to send a PR for this, happy to review. I'm not 100% sure it's the right approach but it seems to make sense to me.

it('script tag is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(
<img data-attr={'<script type=\'\' src=""></script>'} />,
);
expect(response).toMatch(
'<img data-attr="&lt;script type=&#x27;&#x27; ' +
'src=&quot;&quot;&gt;&lt;/script&gt;" ' +
'data-reactroot=""/>',
);
});
});
16 changes: 0 additions & 16 deletions packages/react-dom/src/client/setTextContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
* LICENSE file in the root directory of this source tree.
*/

import ExecutionEnvironment from 'fbjs/lib/ExecutionEnvironment';

import setInnerHTML from './setInnerHTML';
import escapeTextContentForBrowser from '../shared/escapeTextContentForBrowser';
import {TEXT_NODE} from '../shared/HTMLNodeType';

/**
Expand Down Expand Up @@ -37,16 +33,4 @@ let setTextContent = function(node, text) {
node.textContent = text;
};

if (ExecutionEnvironment.canUseDOM) {
if (!('textContent' in document.documentElement)) {
setTextContent = function(node, text) {
if (node.nodeType === TEXT_NODE) {
node.nodeValue = text;
return;
}
setInnerHTML(node, escapeTextContentForBrowser(text));
};
}
}

export default setTextContent;
10 changes: 5 additions & 5 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';
import assertValidProps from '../shared/assertValidProps';
import dangerousStyleValue from '../shared/dangerousStyleValue';
import escapeTextContentForBrowser from '../shared/escapeTextContentForBrowser';
import escapeTextForBrowser from '../shared/escapeTextForBrowser';
import isCustomComponent from '../shared/isCustomComponent';
import omittedCloseTags from '../shared/omittedCloseTags';
import warnValidStyle from '../shared/warnValidStyle';
Expand Down Expand Up @@ -204,7 +204,7 @@ function getNonChildrenInnerMarkup(props) {
} else {
var content = props.children;
if (typeof content === 'string' || typeof content === 'number') {
return escapeTextContentForBrowser(content);
return escapeTextForBrowser(content);
}
}
return null;
Expand Down Expand Up @@ -572,13 +572,13 @@ class ReactDOMServerRenderer {
return '';
}
if (this.makeStaticMarkup) {
return escapeTextContentForBrowser(text);
return escapeTextForBrowser(text);
}
if (this.previousWasTextNode) {
return '<!-- -->' + escapeTextContentForBrowser(text);
return '<!-- -->' + escapeTextForBrowser(text);
}
this.previousWasTextNode = true;
return escapeTextContentForBrowser(text);
return escapeTextForBrowser(text);
} else {
var nextChild;
({child: nextChild, context} = resolve(child, context));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
var matchHtmlRegExp = /["'&<>]/;

/**
* Escape special characters in the given string of html.
* Escapes special characters and HTML entities in a given html string.
*
* @param {string} string The string to escape for inserting into HTML
* @param {string} string HTML string to escape for later insertion
* @return {string}
* @public
*/
Expand Down Expand Up @@ -98,7 +98,7 @@ function escapeHtml(string) {
* @param {*} text Text value to escape.
* @return {string} An escaped string.
*/
function escapeTextContentForBrowser(text) {
function escapeTextForBrowser(text) {
if (typeof text === 'boolean' || typeof text === 'number') {
// this shortcircuit helps perf for types that we know will never have
// special characters, especially given that this function is used often
Expand All @@ -108,4 +108,4 @@ function escapeTextContentForBrowser(text) {
return escapeHtml(text);
}

export default escapeTextContentForBrowser;
export default escapeTextForBrowser;
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import escapeTextContentForBrowser from './escapeTextContentForBrowser';
import escapeTextForBrowser from './escapeTextForBrowser';

/**
* Escapes attribute value to prevent scripting attacks.
Expand All @@ -14,7 +14,7 @@ import escapeTextContentForBrowser from './escapeTextContentForBrowser';
* @return {string} An escaped string.
*/
function quoteAttributeValueForBrowser(value) {
return '"' + escapeTextContentForBrowser(value) + '"';
return '"' + escapeTextForBrowser(value) + '"';
}

export default quoteAttributeValueForBrowser;