-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Changes from all commits
a8785a5
4214c61
e6da941
4727856
52b3f90
095dd70
ca12b9e
895a01c
3a36d1a
058babe
f6f503d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
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="">&</span>'); | ||
}); | ||
|
||
it('double quote is escaped when passed as text content', () => { | ||
var response = ReactDOMServer.renderToString(<span>{'"'}</span>); | ||
expect(response).toMatch('<span data-reactroot="">"</span>'); | ||
}); | ||
|
||
it('single quote is escaped when passed as text content', () => { | ||
var response = ReactDOMServer.renderToString(<span>{"'"}</span>); | ||
expect(response).toMatch('<span data-reactroot="">'</span>'); | ||
}); | ||
|
||
it('greater than entity is escaped when passed as text content', () => { | ||
var response = ReactDOMServer.renderToString(<span>{'>'}</span>); | ||
expect(response).toMatch('<span data-reactroot="">></span>'); | ||
}); | ||
|
||
it('lower than entity is escaped when passed as text content', () => { | ||
var response = ReactDOMServer.renderToString(<span>{'<'}</span>); | ||
expect(response).toMatch('<span data-reactroot=""><</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=""><script type='' ' + | ||
'src=""></script></span>', | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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="&" 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=""" data-reactroot=""/>'); | ||
}); | ||
|
||
it('single quote is escaped inside attributes', () => { | ||
var response = ReactDOMServer.renderToString(<img data-attr="'" />); | ||
expect(response).toMatch('<img data-attr="'" 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=">" data-reactroot=""/>'); | ||
}); | ||
|
||
it('should escape number to string', () => { | ||
expect(quoteAttributeValueForBrowser(42)).toBe('"42"'); | ||
it('lower than entity is escaped inside attributes', () => { | ||
var response = ReactDOMServer.renderToString(<img data-attr="<" />); | ||
expect(response).toMatch('<img data-attr="<" data-reactroot=""/>'); | ||
}); | ||
|
||
it('should escape string', () => { | ||
var escaped = quoteAttributeValueForBrowser( | ||
'<script type=\'\' src=""></script>', | ||
it('number is escaped to string inside attributes', () => { | ||
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('"&"'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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="<script type='' ' + | ||
'src=""></script>" ' + | ||
'data-reactroot=""/>', | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds goood.
Not sure what you mean. We just want to verify
<script>
can't be injected as attribute value.There was a problem hiding this comment.
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.