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

Completes prior fix 366 for script sanitization #370

Merged
merged 1 commit into from
Apr 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ Lint/HandleExceptions:
- 'spec/dummy/bin/rake'

Metrics/AbcSize:
Max: 26
Max: 28

Metrics/CyclomaticComplexity:
Max: 7
Expand Down
9 changes: 8 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. Items under
Contributors: please follow the recommendations outlined at [keepachangelog.com](http://keepachangelog.com/). Please use the existing headings and styling as a guide, and add a link for the version diff at the bottom of the file. Also, please update the `Unreleased` link to compare to the latest release version.
## [Unreleased]

## [5.1.1] - 2016-04-04
##### Fixed
- [Security] Address failure to sanitize console messages when server rendering and displaying in the browser console. See [#366](https://github.com/shakacode/react_on_rails/pull/366) and [#370](https://github.com/shakacode/react_on_rails/pull/370) by [justin808](https://github.com/justin808)
##### Added
- railsContext includes the port number and a boolean if the code is being run on the server or client.

## [5.1.0] - 2016-04-03
##### Added
All 5.1.0 changes can be found in [#362](https://github.com/shakacode/react_on_rails/pull/362) by [justin808](https://github.com/justin808).
Expand Down Expand Up @@ -262,7 +268,8 @@ Best done with Object destructing:

##### Fixed
- Fix several generator related issues.
[Unreleased]: https://github.com/shakacode/react_on_rails/compare/5.1.0...master
[Unreleased]: https://github.com/shakacode/react_on_rails/compare/5.1.1...master
[5.1.1]: https://github.com/shakacode/react_on_rails/compare/5.0.0...5.1.1
[5.1.0]: https://github.com/shakacode/react_on_rails/compare/5.0.0...5.1.0
[5.0.0]: https://github.com/shakacode/react_on_rails/compare/4.0.3...5.0.0
[4.0.3]: https://github.com/shakacode/react_on_rails/compare/4.0.2...4.0.3
Expand Down
10 changes: 8 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -234,13 +234,19 @@ The `railsContext` has: (see implementation in file react_on_rails_helper.rb for
location: "#{uri.path}#{uri.query.present? ? "?#{uri.query}": ""}",
scheme: uri.scheme, # http
host: uri.host, # foo.com
port: uri.port,
pathname: uri.path, # /posts
search: uri.query, # id=30&limit=5

# Locale settings
i18nLocale: I18n.locale,
i18nDefaultLocale: I18n.default_locale,
httpAcceptLanguage: request.env["HTTP_ACCEPT_LANGUAGE"]
httpAcceptLanguage: request.env["HTTP_ACCEPT_LANGUAGE"],

# Other
serverSide: boolean # Are we being called on the server or client? NOTE, if you conditionally
# render something different on the server than the client, then React will only show the
# server version!
}
```

Expand Down Expand Up @@ -327,7 +333,7 @@ This is how you actually render the React components you exposed to `window` ins
+ **prerender:** enable server-side rendering of component. Set to false when debugging!
+ **id:** Id for the div. This will get assigned automatically if you do not provide an id.
+ **html_options:** Any other html options to get placed on the added div for the component.
+ **trace:** set to true to print additional debugging information in the browser. Defaults to true for development, off otherwise.
+ **trace:** set to true to print additional debugging information in the browser. Defaults to true for development, off otherwise. Note, on the client you will so both the railsContext and your props. On the server, you only see the railsContext being logged.
+ **replay_console:** Default is true. False will disable echoing server-rendering logs to the browser. While this can make troubleshooting server rendering difficult, so long as you have the default configuration of logging_on_server set to true, you'll still see the errors on the server.
+ **raise_on_prerender_error:** Default is false. True will throw an error on the server side rendering. Your controller will have to handle the error.

Expand Down
11 changes: 7 additions & 4 deletions app/helpers/react_on_rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def prepend_render_rails_context(render_value)
return render_value if @rendered_rails_context

data = {
rails_context: rails_context
rails_context: rails_context(server_side: false)
}

@rendered_rails_context = true
Expand Down Expand Up @@ -297,7 +297,7 @@ def server_rendered_react_component_html(props, react_component_name, dom_id,

wrapper_js = <<-JS
(function() {
var railsContext = #{rails_context.to_json};
var railsContext = #{rails_context(server_side: true).to_json};
#{initialize_redux_stores}
var props = #{props_string(props)};
return ReactOnRails.serverRenderReactComponent({
Expand Down Expand Up @@ -356,17 +356,18 @@ def initialize_redux_stores

# This is the definitive list of the default values used for the rails_context, which is the
# second parameter passed to both component and store generator functions.
def rails_context
def rails_context(server_side:)
@rails_context ||= begin
uri = URI.parse(request.original_url)
# uri = URI("http://foo.com/posts?id=30&limit=5#time=1305298413")
# uri = URI("http://foo.com:3000/posts?id=30&limit=5#time=1305298413")

result = {
# URL settings
href: request.original_url,
location: "#{uri.path}#{uri.query.present? ? "?#{uri.query}" : ''}",
scheme: uri.scheme, # http
host: uri.host, # foo.com
port: uri.port,
pathname: uri.path, # /posts
search: uri.query, # id=30&limit=5

Expand All @@ -382,6 +383,8 @@ def rails_context
end
result
end

@rails_context.merge(serverSide: server_side)
end

def raise_on_prerender_error_option(val)
Expand Down
9 changes: 7 additions & 2 deletions node_package/src/createReactElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@ export default function createReactElement({
location,
}) {
if (trace) {
console.log(`RENDERED ${name} to dom node with id: ${domNodeId} with props, railsContext:`,
props, railsContext);
if (railsContext && railsContext.serverSide) {
console.log(`RENDERED ${name} to dom node with id: ${domNodeId} with railsContext:`,
railsContext);
} else {
console.log(`RENDERED ${name} to dom node with id: ${domNodeId} with props, railsContext:`,
props, railsContext);
}
}

const componentObj = ReactOnRails.getComponent(name);
Expand Down
4 changes: 2 additions & 2 deletions node_package/src/scriptSanitizedVal.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
export default (val) => {
// Replace closing
const re = /<\/\W*script\W*>/gi;
return val.replace(re, '(/script)');
const re = /<\/\W*script/gi;
return val.replace(re, '(/script');
};
4 changes: 2 additions & 2 deletions node_package/tests/buildConsoleReplay.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ test('consoleReplay replays converts script tag inside of object string to be sa
];
const actual = consoleReplay();

const expected = `console.log.apply(console, ["some message (/script)<script>alert(\'WTF\')\
(/script)","{\\"a\\":\\"Wow(/script)<script>alert(\'WTF\')(/script)\\",\\"b\\":2}"]);
const expected = `console.log.apply(console, ["some message (/script><script>alert(\'WTF\')\
(/script>","{\\"a\\":\\"Wow(/script><script>alert(\'WTF\')(/script>\\",\\"b\\":2}"]);
console.warn.apply(console, ["other message","{\\"c\\":3,\\"d\\":4}"]);`;

assert.equals(actual, expected, 'Unexpected value for console replay history');
Expand Down
42 changes: 39 additions & 3 deletions node_package/tests/scriptSanitizedVal.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,47 @@
import test from 'tape';
import scriptSanitizedVal, { consoleReplay } from '../src/scriptSanitizedVal';

test('scriptSanitizedVal returns no </script>', (assert) => {
test('scriptSanitizedVal returns no </script if spaces, uppercase 1', (assert) => {
assert.plan(1);
const input = '[SERVER] This is a script:\"</div>\"</script> <script>alert(\'WTF\')</ SCRIPT >';
const actual = scriptSanitizedVal(input);
const expected = '[SERVER] This is a script:\"</div>\"(/script) <script>alert(\'WTF\')(/script)';;
const expected = '[SERVER] This is a script:\"</div>\"(/script> <script>alert(\'WTF\')(/script >';
assert.equals(actual, expected,
'consoleReplay should return an empty string if no console.history');
'scriptSanitizedVal replaces closing script tags');
});

test('scriptSanitizedVal returns no </script> 2', (assert) => {
assert.plan(1);
const input = 'Script2:"</div>"</script xx> <script>alert(\'WTF2\')</script xx>';
const actual = scriptSanitizedVal(input);
const expected = 'Script2:"</div>"(/script xx> <script>alert(\'WTF2\')(/script xx>';
assert.equals(actual, expected,
'scriptSanitizedVal replaces closing script tags');
});

test('scriptSanitizedVal returns no </script> 3', (assert) => {
assert.plan(1);
const input = 'Script3:"</div>"</ SCRIPT xx> <script>alert(\'WTF3\')</script xx>';
const actual = scriptSanitizedVal(input);
const expected = 'Script3:"</div>"(/script xx> <script>alert(\'WTF3\')(/script xx>';
assert.equals(actual, expected,
'scriptSanitizedVal replaces closing script tags');
});

test('scriptSanitizedVal returns no </script> 4', (assert) => {
assert.plan(1);
const input = 'Script4"</div>"</script <script>alert(\'WTF4\')</script>';
const actual = scriptSanitizedVal(input);
const expected = 'Script4"</div>"(/script <script>alert(\'WTF4\')(/script>';
assert.equals(actual, expected,
'scriptSanitizedVal replaces closing script tags');
});

test('scriptSanitizedVal returns no </script> 5', (assert) => {
assert.plan(1);
const input = 'Script5:"</div>"</ script> <script>alert(\'WTF5\')</script>';
const actual = scriptSanitizedVal(input);
const expected = 'Script5:"</div>"(/script> <script>alert(\'WTF5\')(/script>';
assert.equals(actual, expected,
'scriptSanitizedVal replaces closing script tags');
});
6 changes: 5 additions & 1 deletion spec/dummy/client/app/components/HelloWorldRedux.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ export default class HelloWorldRedux extends React.Component {

// If this creates an alert, we have a problem!
// see file node_package/src/scriptSanitizedVal.js for the fix to this prior issue.
console.log('This is a script:"</div>"</script> <script>alert(\'WTF\')</script>');
console.log('This is a script:"</div>"</script> <script>alert(\'WTF1\')</script>');
console.log('Script2:"</div>"</script xx> <script>alert(\'WTF2\')</script xx>');
console.log('Script3:"</div>"</ SCRIPT xx> <script>alert(\'WTF3\')</script xx>');
console.log('Script4"</div>"</script <script>alert(\'WTF4\')</script>');
console.log('Script5:"</div>"</ script> <script>alert(\'WTF5\')</script>');

return (
<div>
Expand Down
21 changes: 12 additions & 9 deletions spec/dummy/client/app/components/RailsContext.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@ import React, { PropTypes } from 'react';
import _ from 'lodash';

function renderContextRows(railsContext) {
console.log('railsContext.serverSide is ', railsContext.serverSide)
return _.transform(railsContext, (accum, value, key) => {
const className = `js-${key}`;
accum.push(
<tr key={className}>
<td><strong>
{key}:&nbsp;
</strong></td>
<td className={className}>{value}</td>
</tr>
);
if (key !== 'serverSide') {
const className = `js-${key}`;
accum.push(
<tr key={className}>
<td><strong>
{key}:&nbsp;
</strong></td>
<td className={className}>{value + ''}</td>
</tr>
);
}
}, []);
}

Expand Down
1 change: 1 addition & 0 deletions spec/dummy/spec/features/rails_context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
keys_to_vals = {
href: "http://#{host_port}/#{pathname}?ab=cd",
location: "/#{pathname}?ab=cd",
port: port,
scheme: "http",
host: host,
pathname: "/#{pathname}",
Expand Down
2 changes: 1 addition & 1 deletion spec/dummy/spec/requests/console_logging_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

expected = <<-JS
console.log.apply(console, ["[SERVER] RENDERED HelloWorldWithLogAndThrow to dom node \
with id: HelloWorldWithLogAndThrow-react-component-0 with props, railsContext:"
with id: HelloWorldWithLogAndThrow-react-component-0 with railsContext:"
console.log.apply(console, ["[SERVER] console.log in HelloWorld"]);
console.warn.apply(console, ["[SERVER] console.warn in HelloWorld"]);
console.error.apply(console, ["[SERVER] console.error in HelloWorld"]);
Expand Down