From d6562832e70d07e5013001b3bed43c1a66bb4074 Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 14:56:59 +0200
Subject: [PATCH 1/6] adds rinku gem and auto_link for Comment.body - WIP

---
 Gemfile               | 3 ++-
 Gemfile.lock          | 2 ++
 app/models/comment.rb | 5 +++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Gemfile b/Gemfile
index 525d238fd..05d9ec2e9 100644
--- a/Gemfile
+++ b/Gemfile
@@ -29,7 +29,7 @@ gem 'omniauth-google-oauth2'
 gem 'kaminari'
 gem 'ancestry'
 gem 'acts-as-taggable-on'
-gem "responders"
+gem 'responders'
 gem 'foundation-rails'
 gem 'foundation_rails_helper'
 gem 'acts_as_votable'
@@ -40,6 +40,7 @@ gem 'social-share-button'
 gem 'initialjs-rails', '0.2.0'
 gem 'unicorn'
 gem 'paranoia'
+gem 'rinku', require: 'rails_rinku'
 gem 'savon'
 gem 'dalli'
 gem 'rollbar', '~> 2.2.1'
diff --git a/Gemfile.lock b/Gemfile.lock
index 7da5c9fe2..6dfcba2b0 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -294,6 +294,7 @@ GEM
       http-cookie (>= 1.0.2, < 2.0)
       mime-types (>= 1.16, < 3.0)
       netrc (~> 0.7)
+    rinku (1.7.3)
     rollbar (2.2.1)
     rspec (3.3.0)
       rspec-core (~> 3.3.0)
@@ -444,6 +445,7 @@ DEPENDENCIES
   quiet_assets
   rails (= 4.2.4)
   responders
+  rinku
   rollbar (~> 2.2.1)
   rspec-rails (~> 3.0)
   sass-rails (~> 5.0)
diff --git a/app/models/comment.rb b/app/models/comment.rb
index 233736824..586de729c 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -53,6 +53,11 @@ class Comment < ActiveRecord::Base
     self.user= author
   end
 
+  def body
+    unprocessed = super
+    unprocessed ? Rinku.auto_link(unprocessed).html_safe : unprocessed
+  end
+
   def total_votes
     cached_votes_total
   end

From 0888cb0bfa274ded891ab1415021cded35fa39e4 Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 15:06:51 +0200
Subject: [PATCH 2/6] adds target _blank and rel nofollow to Comment#body with
 rinku

---
 app/models/comment.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/comment.rb b/app/models/comment.rb
index 586de729c..5836fa103 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -55,7 +55,7 @@ class Comment < ActiveRecord::Base
 
   def body
     unprocessed = super
-    unprocessed ? Rinku.auto_link(unprocessed).html_safe : unprocessed
+    unprocessed ? Rinku.auto_link(unprocessed, :all, 'target="_blank" rel="nofollow"').html_safe : unprocessed
   end
 
   def total_votes

From f6246bf290d236938795cdd8455f2da55ef93656 Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 15:32:38 +0200
Subject: [PATCH 3/6] tests autolinking in a comment body

---
 spec/features/comments_spec.rb | 39 +++++++++++++---------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/spec/features/comments_spec.rb b/spec/features/comments_spec.rb
index 40e75a281..b880ae284 100644
--- a/spec/features/comments_spec.rb
+++ b/spec/features/comments_spec.rb
@@ -2,9 +2,10 @@ require 'rails_helper'
 include ActionView::Helpers::DateHelper
 
 feature 'Comments' do
+  let(:user)   { create :user }
+  let(:debate) { create :debate }
 
   scenario 'Index' do
-    debate = create(:debate)
     3.times { create(:comment, commentable: debate) }
 
     visit debate_path(debate)
@@ -19,8 +20,20 @@ feature 'Comments' do
     end
   end
 
+  scenario 'Autolinking' do
+    create :comment, commentable: debate, body: 'Built with http://rubyonrails.org/'
+
+    visit debate_path(debate)
+
+    within first('.comment') do
+      expect(page).to have_content 'Built with http://rubyonrails.org/'
+      expect(page).to have_link('http://rubyonrails.org/', href: 'http://rubyonrails.org/')
+      expect(find_link('http://rubyonrails.org/')[:rel]).to eq('nofollow')
+      expect(find_link('http://rubyonrails.org/')[:target]).to eq('_blank')
+    end
+  end
+
   scenario 'Paginated comments' do
-    debate = create(:debate)
     per_page = 10
     (per_page + 2).times { create(:comment, commentable: debate)}
 
@@ -39,7 +52,6 @@ feature 'Comments' do
 
   feature 'Not logged user' do
     scenario 'can not see comments forms' do
-      debate = create(:debate)
       create(:comment, commentable: debate)
       visit debate_path(debate)
 
@@ -53,9 +65,6 @@ feature 'Comments' do
   end
 
   scenario 'Create', :js do
-    user = create(:user)
-    debate = create(:debate)
-
     login_as(user)
     visit debate_path(debate)
 
@@ -68,9 +77,6 @@ feature 'Comments' do
   end
 
   scenario 'Errors on create', :js do
-    user = create(:user)
-    debate = create(:debate)
-
     login_as(user)
     visit debate_path(debate)
 
@@ -82,7 +88,6 @@ feature 'Comments' do
   scenario 'Reply', :js do
     citizen = create(:user, username: 'Ana')
     manuela = create(:user, username: 'Manuela')
-    debate  = create(:debate)
     comment = create(:comment, commentable: debate, user: citizen)
 
     login_as(manuela)
@@ -103,8 +108,6 @@ feature 'Comments' do
   end
 
   scenario 'Errors on reply', :js do
-    user = create(:user)
-    debate = create(:debate)
     comment = create(:comment, commentable: debate, user: user)
 
     login_as(user)
@@ -120,7 +123,6 @@ feature 'Comments' do
   end
 
   scenario "N replies", :js do
-    debate = create(:debate)
     parent = create(:comment, commentable: debate)
 
     7.times do
@@ -133,8 +135,6 @@ feature 'Comments' do
   end
 
   scenario "Flagging as inappropriate", :js do
-    user = create(:user)
-    debate = create(:debate)
     comment = create(:comment, commentable: debate)
 
     login_as(user)
@@ -151,8 +151,6 @@ feature 'Comments' do
   end
 
   scenario "Undoing flagging as inappropriate", :js do
-    user = create(:user)
-    debate = create(:debate)
     comment = create(:comment, commentable: debate)
     Flag.flag(user, comment)
 
@@ -170,7 +168,6 @@ feature 'Comments' do
   end
 
   scenario "Flagging turbolinks sanity check", :js do
-    user = create(:user)
     debate = create(:debate, title: "Should we change the world?")
     comment = create(:comment, commentable: debate)
 
@@ -187,7 +184,6 @@ feature 'Comments' do
   feature "Moderators" do
     scenario "can create comment as a moderator", :js do
       moderator = create(:moderator)
-      debate = create(:debate)
 
       login_as(moderator.user)
       visit debate_path(debate)
@@ -208,7 +204,6 @@ feature 'Comments' do
       citizen = create(:user, username: "Ana")
       manuela = create(:user, username: "Manuela")
       moderator = create(:moderator, user: manuela)
-      debate  = create(:debate)
       comment = create(:comment, commentable: debate, user: citizen)
 
       login_as(manuela)
@@ -234,7 +229,6 @@ feature 'Comments' do
 
     scenario "can not comment as an administrator" do
       moderator = create(:moderator)
-      debate = create(:debate)
 
       login_as(moderator.user)
       visit debate_path(debate)
@@ -246,7 +240,6 @@ feature 'Comments' do
   feature "Administrators" do
     scenario "can create comment as an administrator", :js do
       admin = create(:administrator)
-      debate = create(:debate)
 
       login_as(admin.user)
       visit debate_path(debate)
@@ -267,7 +260,6 @@ feature 'Comments' do
       citizen = create(:user, username: "Ana")
       manuela = create(:user, username: "Manuela")
       admin   = create(:administrator, user: manuela)
-      debate  = create(:debate)
       comment = create(:comment, commentable: debate, user: citizen)
 
       login_as(manuela)
@@ -293,7 +285,6 @@ feature 'Comments' do
 
     scenario "can not comment as a moderator" do
       admin  = create(:administrator)
-      debate = create(:debate)
 
       login_as(admin.user)
       visit debate_path(debate)

From 31cf51f07a756a80d4c0a8bf3a03a3c7cb090dd6 Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 18:28:10 +0200
Subject: [PATCH 4/6] adds text_with_links helper and use that in any
 comment.body in views, adds test to check for malicious injections in comment
 body

---
 app/helpers/comments_helper.rb               |  2 +-
 app/helpers/text_with_links_helper.rb        |  9 +++++++++
 app/mailers/mailer.rb                        |  1 +
 app/models/comment.rb                        |  5 -----
 app/views/admin/comments/index.html.erb      |  2 +-
 app/views/admin/users/show.html.erb          |  2 +-
 app/views/comments/_comment.html.erb         | 14 +++++++-------
 app/views/mailer/comment.html.erb            |  2 +-
 app/views/mailer/reply.html.erb              |  2 +-
 app/views/moderation/comments/index.html.erb |  2 +-
 spec/features/comments_spec.rb               | 13 ++++++++++++-
 11 files changed, 35 insertions(+), 19 deletions(-)
 create mode 100644 app/helpers/text_with_links_helper.rb

diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb
index 36952af7d..67c347ced 100644
--- a/app/helpers/comments_helper.rb
+++ b/app/helpers/comments_helper.rb
@@ -17,4 +17,4 @@ module CommentsHelper
     comments.select{|c| c.parent_id == parent.id}
   end
 
-end
\ No newline at end of file
+end
diff --git a/app/helpers/text_with_links_helper.rb b/app/helpers/text_with_links_helper.rb
new file mode 100644
index 000000000..32ada7c72
--- /dev/null
+++ b/app/helpers/text_with_links_helper.rb
@@ -0,0 +1,9 @@
+module TextWithLinksHelper
+
+  def text_with_links(text)
+    return unless text
+    sanitized = sanitize text, tags: %w(a), attributes: %w(href)
+    Rinku.auto_link(sanitized, :all, 'target="_blank" rel="nofollow"').html_safe
+  end
+
+end
diff --git a/app/mailers/mailer.rb b/app/mailers/mailer.rb
index a3db690a8..15a8eb54a 100644
--- a/app/mailers/mailer.rb
+++ b/app/mailers/mailer.rb
@@ -1,4 +1,5 @@
 class Mailer < ApplicationMailer
+  helper :text_with_links
 
   def comment(comment)
     @comment = comment
diff --git a/app/models/comment.rb b/app/models/comment.rb
index 5836fa103..233736824 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -53,11 +53,6 @@ class Comment < ActiveRecord::Base
     self.user= author
   end
 
-  def body
-    unprocessed = super
-    unprocessed ? Rinku.auto_link(unprocessed, :all, 'target="_blank" rel="nofollow"').html_safe : unprocessed
-  end
-
   def total_votes
     cached_votes_total
   end
diff --git a/app/views/admin/comments/index.html.erb b/app/views/admin/comments/index.html.erb
index ec0fddc1a..ce4bfa293 100644
--- a/app/views/admin/comments/index.html.erb
+++ b/app/views/admin/comments/index.html.erb
@@ -9,7 +9,7 @@
     <li id="<%= dom_id(comment) %>">
       <div class="row">
         <div class="small-12 medium-8 column">
-          <%= comment.body %>
+          <%= text_with_links comment.body %>
           <%= link_to comment.commentable.title, comment.commentable %>
         </div>
         <div class="small-6 medium-4 column text-right">
diff --git a/app/views/admin/users/show.html.erb b/app/views/admin/users/show.html.erb
index 79a4b223e..6d98d4e38 100644
--- a/app/views/admin/users/show.html.erb
+++ b/app/views/admin/users/show.html.erb
@@ -31,7 +31,7 @@
   <li id="<%= dom_id(comment) %>">
       <div class="row">
         <div class="small-12 medium-10 column">
-          <%= comment.body %>
+          <%= text_with_links comment.body %>
         </div>
       </div>
     </li>
diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb
index 3997efdaf..25d7585f3 100644
--- a/app/views/comments/_comment.html.erb
+++ b/app/views/comments/_comment.html.erb
@@ -63,17 +63,17 @@
           </div>
 
           <% if comment.as_administrator? %>
-            <p class="comment-user is-admin"><%= comment.body %></p>
+            <p class="comment-user is-admin"><%= text_with_links comment.body %></p>
           <% elsif comment.as_moderator? %>
-            <p class="comment-user is-moderator"><%= comment.body %></p>
+            <p class="comment-user is-moderator"><%= text_with_links comment.body %></p>
           <% elsif comment.user.official? && comment.user_id == @commentable.author_id %>
-            <p class="comment-user level-<%= comment.user.official_level %> is-author"><%= comment.body %></p>
+            <p class="comment-user level-<%= comment.user.official_level %> is-author"><%= text_with_links comment.body %></p>
           <% elsif comment.user.official? %>
-            <p class="comment-user level-<%= comment.user.official_level %>"><%= comment.body %></p>
+            <p class="comment-user level-<%= comment.user.official_level %>"><%= text_with_links comment.body %></p>
           <% elsif comment.user_id == @commentable.author_id %>
-            <p class="comment-user is-author"><%= comment.body %></p>
+            <p class="comment-user is-author"><%= text_with_links comment.body %></p>
           <% else %>
-            <p class="comment-user"><%= comment.body %></p>
+            <p class="comment-user"><%= text_with_links comment.body %></p>
           <% end %>
           <span id="<%= dom_id(comment) %>_votes" class="comment-votes right">
             <%= render 'comments/votes', comment: comment %>
@@ -101,4 +101,4 @@
       </div>
     </div>
   </div>
-<% end %>
\ No newline at end of file
+<% end %>
diff --git a/app/views/mailer/comment.html.erb b/app/views/mailer/comment.html.erb
index 959b8e0a0..16ace7990 100644
--- a/app/views/mailer/comment.html.erb
+++ b/app/views/mailer/comment.html.erb
@@ -13,6 +13,6 @@
   </p>
 
   <p style="border-left: 2px solid #DEE0E3;font-family: 'Open Sans','Helvetica Neue',arial,sans-serif;font-size: 14px;font-style: italic;font-weight: normal;line-height: 24px;margin-left: 20px;padding: 10px;">
-    <%= @comment.body %>
+    <%= text_with_links @comment.body %>
   </p>
 </td>
diff --git a/app/views/mailer/reply.html.erb b/app/views/mailer/reply.html.erb
index 924234f02..d006e205a 100644
--- a/app/views/mailer/reply.html.erb
+++ b/app/views/mailer/reply.html.erb
@@ -13,6 +13,6 @@
   </p>
 
   <p style="border-left: 2px solid #DEE0E3;font-family: 'Open Sans','Helvetica Neue',arial,sans-serif;font-size: 14px;font-style: italic;font-weight: normal;line-height: 24px;margin-left: 20px;padding: 10px;">
-    <%= @reply.body %>
+    <%= text_with_links @reply.body %>
   </p>
 </td>
diff --git a/app/views/moderation/comments/index.html.erb b/app/views/moderation/comments/index.html.erb
index ec642077b..d18dca8e8 100644
--- a/app/views/moderation/comments/index.html.erb
+++ b/app/views/moderation/comments/index.html.erb
@@ -23,7 +23,7 @@
         <%= comment.commentable_type.constantize.model_name.human %>
         <span class="date"><%= l comment.updated_at.to_date %></span>
       </td>
-      <td><%= comment.body %></td>
+      <td><%= text_with_links comment.body %></td>
       <td class="text-center"><%= comment.flags_count %></td>
       <td>
         <%= link_to t("moderation.comments.index.hide"), hide_in_moderation_screen_moderation_comment_path(comment, request.query_parameters), method: :put, class: "delete" %>
diff --git a/spec/features/comments_spec.rb b/spec/features/comments_spec.rb
index b880ae284..41c1a5f31 100644
--- a/spec/features/comments_spec.rb
+++ b/spec/features/comments_spec.rb
@@ -20,7 +20,7 @@ feature 'Comments' do
     end
   end
 
-  scenario 'Autolinking' do
+  scenario 'Turns links into html links' do
     create :comment, commentable: debate, body: 'Built with http://rubyonrails.org/'
 
     visit debate_path(debate)
@@ -33,6 +33,17 @@ feature 'Comments' do
     end
   end
 
+  scenario 'Sanitizes comment body for security' do
+    create :comment, commentable: debate, body: "<script>alert('hola')</script> http://madrid.es"
+
+    visit debate_path(debate)
+
+    within first('.comment') do
+      expect(page).to have_content "alert('hola') http://madrid.es"
+      expect(page).to have_link('http://madrid.es', href: 'http://madrid.es')
+    end
+  end
+
   scenario 'Paginated comments' do
     per_page = 10
     (per_page + 2).times { create(:comment, commentable: debate)}

From 5ecbe01d4731432c1f85a9bafc7ccd25c84db67e Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 19:05:34 +0200
Subject: [PATCH 5/6] prevents body comments to accept html a tags, sanitize
 them out instead as well

---
 app/helpers/text_with_links_helper.rb | 2 +-
 spec/features/comments_spec.rb        | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/app/helpers/text_with_links_helper.rb b/app/helpers/text_with_links_helper.rb
index 32ada7c72..a1fa6d935 100644
--- a/app/helpers/text_with_links_helper.rb
+++ b/app/helpers/text_with_links_helper.rb
@@ -2,7 +2,7 @@ module TextWithLinksHelper
 
   def text_with_links(text)
     return unless text
-    sanitized = sanitize text, tags: %w(a), attributes: %w(href)
+    sanitized = sanitize text
     Rinku.auto_link(sanitized, :all, 'target="_blank" rel="nofollow"').html_safe
   end
 
diff --git a/spec/features/comments_spec.rb b/spec/features/comments_spec.rb
index 41c1a5f31..437286bf7 100644
--- a/spec/features/comments_spec.rb
+++ b/spec/features/comments_spec.rb
@@ -34,13 +34,14 @@ feature 'Comments' do
   end
 
   scenario 'Sanitizes comment body for security' do
-    create :comment, commentable: debate, body: "<script>alert('hola')</script> http://madrid.es"
+    create :comment, commentable: debate, body: "<script>alert('hola')</script> <a href=\"javascript:alert('sorpresa!')\">click me<a/> http://madrid.es"
 
     visit debate_path(debate)
 
     within first('.comment') do
-      expect(page).to have_content "alert('hola') http://madrid.es"
+      expect(page).to have_content "click me http://madrid.es"
       expect(page).to have_link('http://madrid.es', href: 'http://madrid.es')
+      expect(page).not_to have_link('click me')
     end
   end
 

From ec4119582c59114759b90155bab8ba956ee6bd32 Mon Sep 17 00:00:00 2001
From: David Gil <dgilperez@gmail.com>
Date: Thu, 10 Sep 2015 20:42:57 +0200
Subject: [PATCH 6/6] accepts no html tags in text_with_links sanitize

---
 app/helpers/text_with_links_helper.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/helpers/text_with_links_helper.rb b/app/helpers/text_with_links_helper.rb
index a1fa6d935..55281e89e 100644
--- a/app/helpers/text_with_links_helper.rb
+++ b/app/helpers/text_with_links_helper.rb
@@ -2,7 +2,7 @@ module TextWithLinksHelper
 
   def text_with_links(text)
     return unless text
-    sanitized = sanitize text
+    sanitized = sanitize text, tags: [], attributes: []
     Rinku.auto_link(sanitized, :all, 'target="_blank" rel="nofollow"').html_safe
   end