From 4fd3aacaed4f40306b61565b12806a6800658237 Mon Sep 17 00:00:00 2001 From: Ben Tranter Date: Tue, 2 Jan 2018 15:31:42 -0500 Subject: [PATCH] Add notes about security and frontend code Adds an explanation of how to safely redirect after a POST form submission, and documents how to use Turbolinks on the frontend outside of Rails by following the approach taken by rails-ujs and jquery-ujs. Also removes an outdated comment about a failing test. --- README.md | 113 ++++++++++++++++++++++++++++++++++++++-- spec/turbolinks_spec.cr | 1 - 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 0fe0410..37f0acd 100644 --- a/README.md +++ b/README.md @@ -14,10 +14,6 @@ dependencies: github: bentranter/turbolinks ``` -## Warning - -The `Location` header that is provided on a redirect after a form submit is used to set the value of `location` in `Turbolinks.visit(location)`. This value is current **not escaped** and opens you up to JS injection -- please escape this value manually, or wait until a fix is pushed to this repo. - ## Usage Turbolinks extends `HTTP::Handler`, so it can be used as HTTP middleware. You can use it with the standard library like so: @@ -47,6 +43,115 @@ end Kemal.run ``` +## A Note About Security + +A common pattern if you're coming from the Rails world (and using something like [rails-ujs](https://github.com/rails/rails/tree/master/actionview/app/assets/javascripts)) a common approach is to handle a `POST` request, and then redirect to another route. Turbolinks handles this case by intercepting the redirection after the `POST` request executes, and then responding with a JavaScript snippet to execute `Turbolinks.visit("#{location}")` for the location to redirect to. + +While this is typically safe, if your handler allows the user to input this `location`, you open yourself up to JavaScript injection. Lets look at two examples where a user is leaving a comment, submitted through a form `post` request. + +```crystal +require "kemal" +require "turbolinks" + +add_handler Turbolinks::Handler.new + +# Unsafe approach! +post "/unsafe-comment" do |env| + title = env.params.body["title"] + comment = env.params.body["comment"] + + # Imagine `new_comment` does something to handle a new commment, for + # example's sake. + new_comment(title, comment) + + # This is dangerous because the value of `title` here could be anything -- + # including malicious JavaScript. Since the frontend will excute JavaScript + # containing the value of this redirect here, any malicious JavaScript will + # execute. + env.redirect "/comments/#{title}" +end + +# Safe approach. +post "/safe-comment" do |env| + title = env.params.body["title"] + comment = env.params.body["comment"] + + # Imagine `new_comment` does something to handle a new commment, for + # example's sake. + comment_id = new_comment(title, comment) + + # This is safe because there's no way for the user submitted `title` or + # `comment` to be available in the returned JavaScript -- you generate an + # ID, and redirect to that route. + env.redirect "/comments/#{comment_id}" +end + +Kemal.run +``` + +If you _must_ do something like the unsafe approach, you'll need to sanitize the input yourself. + +## A Note For Non `rails-ujs` Users + +In order for form submissions _not_ to trigger a full page reload, you'll need to ensure that those submissions are submitted as AJAX requests. While a library like [Rails-UJS](https://github.com/rails/rails/tree/master/actionview/app/assets/javascripts) or [JQuery-UJS](https://github.com/rails/jquery-ujs) would handle this for you, it's straightforward to implement this yourself. The following JavaScript snippet adds bare-minimum support for AJAX form submissions that work with Turbolinks, but I encourage to look at what `rails-ujs` does as well. + +```js +(function() { + "use strict"; + /* + * For Google Analytics, you need this: + * + * + * ...the rest of the body here... + * + * See https://coderwall.com/p/ypzfdw/faster-page-loads-with-turbolinks for + * more info. + */ + + /* + * By default, Turbolinks submits forms normally. While this may feel + * frustrating as a consumer of the library, it makes sense: + * - No specialized logic on the backend + * - Cache is purged since the page refreshed. + */ + document.addEventListener("DOMContentLoaded", function() { + /** + * submit sends an HTTP request via XHR. + * @param {*Object} formEl - the form element to submit via XHR. + */ + function submit(formEl) { + var xhr = new XMLHttpRequest(); + xhr.open(formEl.method, formEl.action, true); + + /* See + * https://github.com/rails/rails/blob/master/actionview/app/assets/javascripts/rails-ujs/utils/ajax.coffee + * for a more in-depth usage. + */ + xhr.onreadystatechange = function() { + if (xhr.readyState === 4 && xhr.status === 200) { + var script = document.createElement("script"); + script.innerText = xhr.responseText; + document.head.appendChild(script).parentNode.removeChild(script); + } + } + /* Set relevant headers that some backends check for */ + xhr.setRequestHeader("Turbolinks-Referrer", window.location.href); + xhr.setRequestHeader("X-Requested-With", "xhr"); + xhr.send(new FormData(formEl)); + return false; + } + + /* Intercept **any** submit event to submit via XHR instead. */ + document.addEventListener("submit", function(e) { + e.preventDefault(); + if (e.srcElement) { + submit(e.srcElement); + } + }); + }); +})(); +``` + ## Development Turbolinks follows the typical Crystal project structure, so cloning the repo and making changes is all you need to do. However, you're encouraged to run this backend alongside the Turbolinks frontend to make sure it works as expected, especially when compared to the Rails backend. The Turbolinks frontend is available at [github.com/turbolinks/turbolinks](https://github.com/turbolinks/turbolinks), and the Rails gem is available at [github.com/turbolinks/turbolinks-rails](https://github.com/turbolinks/turbolinks-rails). diff --git a/spec/turbolinks_spec.cr b/spec/turbolinks_spec.cr index 7b18d15..f084dbf 100644 --- a/spec/turbolinks_spec.cr +++ b/spec/turbolinks_spec.cr @@ -28,7 +28,6 @@ describe Turbolinks::Handler do io.to_s.should eq("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nHello") end - # HEY this actually doesn't work, which is nice. it "should set a cookie for redirects occuring during a GET request" do io = IO::Memory.new request = HTTP::Request.new("Get", "/")