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.
bentranter · Jan 2, 2018 · 4fd3aac · 4fd3aac
1 parent 36b6667
commit 4fd3aac
Show file tree

Hide file tree

Showing 2 changed files with 109 additions and 5 deletions.
diff --git 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:
+   * <body>
+   * <script>_gaq.push(['_trackPageview']);</script>
+   *  ...the rest of the body here...
+   * </body>
+   * 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
@@ -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", "/")