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
Next Next commit
Rename escapeText util. Test quoteAttributeValueForBrowser through Re…
…actDOMServer API
  • Loading branch information
jeremenichelli committed Nov 8, 2017
commit a8785a5358f48902d3dba0d741a578dc4760d5dd
9 changes: 0 additions & 9 deletions packages/react-dom/src/__tests__/ReactServerRendering-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,6 @@ describe('ReactDOMServer', () => {
);
});

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;" ' + ROOT_ATTRIBUTE_NAME + '=""' + '/>',
),
);
});

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

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,40 +9,69 @@

'use strict';

var ExecutionEnvironment;
var React;
var ReactDOMServer;

var ROOT_ATTRIBUTE_NAME;

describe('quoteAttributeValueForBrowser', () => {
// TODO: can we express this test with only public API?
var quoteAttributeValueForBrowser = require('../shared/quoteAttributeValueForBrowser')
.default;

it('should escape boolean to string', () => {
expect(quoteAttributeValueForBrowser(true)).toBe('"true"');
expect(quoteAttributeValueForBrowser(false)).toBe('"false"');
beforeEach(() => {
jest.resetModules();
React = require('react');

ExecutionEnvironment = require('fbjs/lib/ExecutionEnvironment');
ExecutionEnvironment.canUseDOM = false;
ReactDOMServer = require('react-dom/server');

// TODO: can we express this test with only public API?
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can. Just embed it here :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, forgot to take it out.

var DOMProperty = require('../shared/DOMProperty');
ROOT_ATTRIBUTE_NAME = DOMProperty.ROOT_ATTRIBUTE_NAME;
});

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

expect(escaped).toBe('"ponys"');
it('double quote is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr='"' />);
expect(response).toMatch(
new RegExp(
'<img data-attr="&quot;" ' + ROOT_ATTRIBUTE_NAME + '=""' + '/>',
),
);
});

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('single quote is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr="'" />);
expect(response).toMatch(
new RegExp(
'<img data-attr="&#x27;" ' + ROOT_ATTRIBUTE_NAME + '=""' + '/>',
),
);
});

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('greater than entity is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr=">" />);
expect(response).toMatch(
new RegExp(
'<img data-attr="&gt;" ' + ROOT_ATTRIBUTE_NAME + '=""' + '/>',
),
);
expect(escaped).not.toContain('<');
expect(escaped).not.toContain('>');
expect(escaped).not.toContain("'");
expect(escaped.substr(1, -1)).not.toContain('"');
});

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('lower than entity is escaped inside attributes', () => {
var response = ReactDOMServer.renderToString(<img data-attr="<" />);
expect(response).toMatch(
new RegExp(
'<img data-attr="&lt;" ' + ROOT_ATTRIBUTE_NAME + '=""' + '/>',
),
);
});
});
13 changes: 0 additions & 13 deletions packages/react-dom/src/client/setTextContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
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 +36,4 @@ var 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 @@ -33,7 +33,7 @@ import {
import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';
import assertValidProps from '../shared/assertValidProps';
import dangerousStyleValue from '../shared/dangerousStyleValue';
import escapeTextContentForBrowser from '../shared/escapeTextContentForBrowser';
import escapeText from '../shared/escapeText';
import isCustomComponent from '../shared/isCustomComponent';
import omittedCloseTags from '../shared/omittedCloseTags';
import warnValidStyle from '../shared/warnValidStyle';
Expand Down Expand Up @@ -209,7 +209,7 @@ function getNonChildrenInnerMarkup(props) {
} else {
var content = props.children;
if (typeof content === 'string' || typeof content === 'number') {
return escapeTextContentForBrowser(content);
return escapeText(content);
}
}
return null;
Expand Down Expand Up @@ -574,13 +574,13 @@ class ReactDOMServerRenderer {
return '';
}
if (this.makeStaticMarkup) {
return escapeTextContentForBrowser(text);
return escapeText(text);
}
if (this.previousWasTextNode) {
return '<!-- -->' + escapeTextContentForBrowser(text);
return '<!-- -->' + escapeText(text);
}
this.previousWasTextNode = true;
return escapeTextContentForBrowser(text);
return escapeText(text);
} else {
var nextChild;
({child: nextChild, context} = resolve(child, context));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ function escapeHtml(string) {
* @param {*} text Text value to escape.
* @return {string} An escaped string.
*/
function escapeTextContentForBrowser(text) {
function escapeTextContent(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 escapeTextContent;
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 escapeText from './escapeText';

/**
* 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 '"' + escapeText(value) + '"';
}

export default quoteAttributeValueForBrowser;