CVE-2018-20851: Insufficient Information (NVD-CWE-noinfo)
value: Helpy before 2.2.0 allows agents to edit admins.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 8.8
Published: 2019-07-10T14:15Z
Repository: https://github.com/helpyio/helpy
Language: Ruby
Files changed: app/models/user.rb, app/views/admin/topics/_topic_options.html.erb, app/views/admin/users/_user.html.erb, app/views/admin/users/_user_context_menu.html.erb, app/views/admin/users/_user_info_with_tickets.html.erb, app/views/layouts/_admin_header.html.erb
Commit date: 2018-10-13 10:20:28 -0600
Commit message: Prevent agents from editing admins, other permission hardening

Diff: diff --git a/app/models/user.rb b/app/models/user.rb
index 6b4b8885..ea6f8d6a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -202,10 +202,16 @@ class User < ActiveRecord::Base
   def can_scrub_and_delete?
     return false if self.id == 2 || self.is_admin?
     true
   end
 
+  # Is this user editable by the current logged in agent?
+  def can_be_edited? current_user
+    return true if current_user.is_admin?
+    !self.is_agent?
+  end
+
   def self.notifiable_on_public
     agents.where(notify_on_public: true).reorder('id asc')
   end
 
   def self.notifiable_on_private
diff --git a/app/views/admin/topics/_topic_options.html.erb b/app/views/admin/topics/_topic_options.html.erb
index 47d79924..3f744bc8 100644
--- a/app/views/admin/topics/_topic_options.html.erb
+++ b/app/views/admin/topics/_topic_options.html.erb
@@ -9,11 +9,11 @@
       <li><%= link_to t('tag_with', default: 'Tag Ticket'), '#', class: 'add-tag-link' %></li>
       <hr/>
       <li class="dropdown-header"><%= @topic.user.name %></li>
       <li><%= link_to t(:tickets, default: 'Tickets'), remote: true %></li>
       <li><%= link_to t(:open_new_discussion, default: 'Open Discussion'), new_admin_topic_path(user_id: @topic.user.id), remote: true, class: '' %></li>
-      <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(@topic.user), remote: false %></li>
+      <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(@topic.user), remote: false if @topic.user.can_be_edited? current_user %></li>
     </ul>
   </span>
 </div>
 <span class="btn-group left-col-dropdown">
   <button class="status dropdown-toggle ticket-control" data-toggle="dropdown" aria-expanded="false">
diff --git a/app/views/admin/users/_user.html.erb b/app/views/admin/users/_user.html.erb
index e2c845f6..e52113b1 100644
--- a/app/views/admin/users/_user.html.erb
+++ b/app/views/admin/users/_user.html.erb
@@ -22,10 +22,10 @@
     <div class="btn-group">
       <span id="row-<%= user.id %>" data-toggle="dropdown" aria-expanded="false" class='btn dropdown-toggle fas fa-ellipsis-v'></span>
       <ul class="dropdown-menu dropdown-menu-right" role="menu">
         <li><%= link_to t(:open_new_discussion, default: 'Open Discussion'), new_admin_topic_path(user_id: user.id), remote: true, class: '' %></li>
         <li><%= link_to t(:discussions, default: 'Discussions'), admin_user_path(user), remote: true %></li>
-        <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user, mode: 'edit'), remote: false %></li>
+        <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user, mode: 'edit'), remote: false if user.can_be_edited? current_user%></li>
       </ul>
     </div>
   </td>
 </tr>
diff --git a/app/views/admin/users/_user_context_menu.html.erb b/app/views/admin/users/_user_context_menu.html.erb
index 9a6c72d4..7f90262f 100644
--- a/app/views/admin/users/_user_context_menu.html.erb
+++ b/app/views/admin/users/_user_context_menu.html.erb
@@ -1,10 +1,10 @@
 <span class="btn-group left-col-dropdown" style="margin-top:18px;">
   <%= link_to content_tag(:small, '', class: 'fas fa-ellipsis-v ticket-ellipsis btn'), '#', class: 'dropdown-toggle', data: { toggle: 'dropdown' }%>
   <ul class="dropdown-menu dropdown-menu-right" role="menu">
     <li><%= link_to t(:open_new_discussion, default: 'Open Discussion'), new_admin_topic_path(user_id: user.id), remote: true, class: '' %></li>
-    <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user), remote: false %></li>
+    <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user), remote: false if user.can_be_edited? current_user %></li>
     <li><%= link_to t(:scrub_user, default: 'Anonymize User'), admin_scrub_user_path(user), remote: true,
                     method: :post,
                     data: { confirm: "Are you sure you want to anonymize #{user.name}? This will anonymize the user and retain all of their public discussions." } if current_user.is_admin? && user.can_scrub_and_delete? %></li>
     <li><%= link_to t(:delete_user, default: 'Delete User'), admin_user_path(user), remote: true,
                     method: :delete,
diff --git a/app/views/admin/users/_user_info_with_tickets.html.erb b/app/views/admin/users/_user_info_with_tickets.html.erb
index dd42be51..e186621c 100644
--- a/app/views/admin/users/_user_info_with_tickets.html.erb
+++ b/app/views/admin/users/_user_info_with_tickets.html.erb
@@ -9,49 +9,49 @@
   <div class="row">
     <div class='col-md-12' id='user-info-info'>
 
       <div id="user-contact-info" class="user-info">
         <div class="tiny-header">
-          <%= best_in_place user, :name, as: :input, url: admin_user_path(user) %>
+          <%= best_in_place_if !user.is_agent?, user, :name, as: :input, url: admin_user_path(user) %>
         </div>
         <span class="small">
           <% if user.account_number.present? %>
             <%= t(:account_number, default: 'Account:') %>
-            <%= best_in_place user, :account_number, as: :input, url: admin_user_path(user),
+            <%= best_in_place_if !user.is_agent?, user, :account_number, as: :input, url: admin_user_path(user),
             place_holder: t('activerecord.attributes.user.account_number', default: 'Account Number') if user.account_number.present? %>
             <br/>
           <% end %>
-          <%= best_in_place user, :company, as: :input, url: admin_user_path(user),
+          <%= best_in_place_if !user.is_agent?, user, :company, as: :input, url: admin_user_path(user),
           place_holder: t('click_to_edit', what: t('activerecord.attributes.user.company'), default: 'click to edit %{what}') %><br/>
-          <%= best_in_place user, :email, as: :input, url: admin_user_path(user) %><br/>
+          <%= best_in_place_if !user.is_agent?, user, :email, as: :input, url: admin_user_path(user) %><br/>
 
         </span>
       </div>
       <div id="user-contact-info" class="user-info">
         <div class="tiny-header"><%= t(:contact_info, default: "Contact") %>:</div>
-        <%= best_in_place user, :street, as: :input, url: admin_user_path(user),
+        <%= best_in_place_if !user.is_agent?, user, :street, as: :input, url: admin_user_path(user),
         place_holder: t('click_to_edit', what: t('activerecord.attributes.user.street'), default: 'click to edit %{what}') %><br/>
-        <%= best_in_place user, :city, as: :input, url: admin_user_path(user),
+        <%= best_in_place_if !user.is_agent?, user, :city, as: :input, url: admin_user_path(user),
         place_holder: t('click_to_edit', what: t('activerecord.attributes.user.city'), default: 'click to edit %{what}') %>
         <br/>
-        <%= best_in_place user, :state, as: :input, url: admin_user_path(user),
+        <%= best_in_place_if !user.is_agent?, user, :state, as: :input, url: admin_user_path(user),
         place_holder: t('click_to_edit', what: t('activerecord.attributes.user.state'), default: 'click to edit %{what}') %>
-        <%= best_in_place user, :zip, as: :input, url: admin_user_path(user),
+        <%= best_in_place_if !user.is_agent?, user, :zip, as: :input, url: admin_user_path(user),
         place_holder: t('click_to_edit', what: t('activerecord.attributes.user.zip'), default: 'click to edit %{what}') %><br/>
       </div>
       <div id="user-contact-info" class="user-info">
         <div class="tiny-header"><%= t(:phone, default: "Phone") %>:</div>
-        <%= t(:work_phone, default: "work") %>: <%= best_in_place user, :work_phone,
+        <%= t(:work_phone, default: "work") %>: <%= best_in_place_if !user.is_agent?, user, :work_phone,
         as: :input, url: admin_user_path(user), place_holder: t('click_to_edit', what: '', default: 'click to edit %{what}') %><br/>
-        <%= t(:home_home, default: "home") %>: <%= best_in_place user, :home_phone,
+        <%= t(:home_home, default: "home") %>: <%= best_in_place_if !user.is_agent?, user, :home_phone,
         as: :input, url: admin_user_path(user), place_holder: t('click_to_edit', what: '', default: 'click to edit %{what}') %><br/>
-        <%= t(:cell_phone, default: "cell") %>: <%= best_in_place user, :cell_phone,
+        <%= t(:cell_phone, default: "cell") %>: <%= best_in_place_if !user.is_agent?, user, :cell_phone,
         as: :input, url: admin_user_path(user), place_holder: t('click_to_edit', what: '', default: 'click to edit %{what}') %><br/>
       </div>
       <div id="user-contact-info" class="user-info">
         <div class="tiny-header"><%= t(:notes, default: "Notes") %>:</div>
-        <%= best_in_place user, :notes, as: :textarea, url: admin_user_path(user),
+        <%= best_in_place_if !user.is_agent?, user, :notes, as: :textarea, url: admin_user_path(user),
         place_holder: t('click_to_edit', what: 'notes', default: 'click to edit %{what}'),
         confirm: true %><br/>
       </div>
     </div>
   </div>
diff --git a/app/views/layouts/_admin_header.html.erb b/app/views/layouts/_admin_header.html.erb
index 572c17cb..3ffe875e 100644
--- a/app/views/layouts/_admin_header.html.erb
+++ b/app/views/layouts/_admin_header.html.erb
@@ -84,11 +84,11 @@ a.navbar-brand {
                 <%= admin_avatar_menu %>
               </ul>
               <ul class="nav navbar-nav navbar-left" data-hook='admin-nav-left'>
                 <%= upper_nav_item(t(:inbox, default: "Inbox"), admin_topics_path,["topics"], ['index','show'], "fas fa-inbox") if (forums? || tickets?) && current_user.is_agent? %>
                 <%= upper_nav_item(t(:reports, default: 'Reports'), admin_reports_path(interval: 7), ["reports"], ['index', 'groups', 'team'], "fas fa-chart-bar") if tickets? && current_user.is_admin? %>
-                <%= upper_nav_item(t(:users, default: 'Customers'), admin_users_path(role: 'user'), ["users"], ["index","show","edit","update"], "fas fa-users") if current_user.is_admin? %>
+                <%= upper_nav_item(t(:users, default: 'Customers'), admin_users_path(role: 'user'), ["users"], ["index","show","edit","update"], "fas fa-users") if current_user.is_agent? %>
                 <%= helpcenter_menu if (knowledgebase? || forums?) && current_user.is_agent? %>
                 <%= content_tag(:li, link_to(t(:content, default: "Content"), admin_categories_path), class:'kblink') if knowledgebase? && current_user.role == 'editor' %>
                 <%#= content_tag(:li, link_to(t(:app_store, default: "App Store"), "http://helpy.io/store/"), class: "hidden-sm hidden-xs") if current_user.is_agent? %>
                 <%= content_tag(:li, link_to(t(:open_new_discussion, default: "New Ticket"), new_admin_topic_path), class: 'visible-xs hidden-lg hidden-md hidden-sm') if current_user.is_agent? %>
                 <%= content_tag(:li, link_to(t(:settings, default: "Settings"), admin_settings_path), class: 'visible-xs hidden-lg hidden-md hidden-sm') if current_user.is_admin? %>

Vulnerable code:       <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(@topic.user), remote: false %></li>
        <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user, mode: 'edit'), remote: false %></li>
    <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user), remote: false %></li>
          <%= best_in_place user, :name, as: :input, url: admin_user_path(user) %>
            <%= best_in_place user, :account_number, as: :input, url: admin_user_path(user),
          <%= best_in_place user, :company, as: :input, url: admin_user_path(user),
          <%= best_in_place user, :email, as: :input, url: admin_user_path(user) %><br/>
        <%= best_in_place user, :street, as: :input, url: admin_user_path(user),
        <%= best_in_place user, :city, as: :input, url: admin_user_path(user),
        <%= best_in_place user, :state, as: :input, url: admin_user_path(user),
        <%= best_in_place user, :zip, as: :input, url: admin_user_path(user),
        <%= t(:work_phone, default: "work") %>: <%= best_in_place user, :work_phone,
        <%= t(:home_home, default: "home") %>: <%= best_in_place user, :home_phone,
        <%= t(:cell_phone, default: "cell") %>: <%= best_in_place user, :cell_phone,
        <%= best_in_place user, :notes, as: :textarea, url: admin_user_path(user),
                <%= upper_nav_item(t(:users, default: 'Customers'), admin_users_path(role: 'user'), ["users"], ["index","show","edit","update"], "fas fa-users") if current_user.is_admin? %>
Fixed code:   # Is this user editable by the current logged in agent?
  def can_be_edited? current_user
    return true if current_user.is_admin?
    !self.is_agent?
  end

      <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(@topic.user), remote: false if @topic.user.can_be_edited? current_user %></li>
        <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user, mode: 'edit'), remote: false if user.can_be_edited? current_user%></li>
    <li><%= link_to t(:edit_user, default: 'Edit User'), edit_admin_user_path(user), remote: false if user.can_be_edited? current_user %></li>
          <%= best_in_place_if !user.is_agent?, user, :name, as: :input, url: admin_user_path(user) %>
            <%= best_in_place_if !user.is_agent?, user, :account_number, as: :input, url: admin_user_path(user),
          <%= best_in_place_if !user.is_agent?, user, :company, as: :input, url: admin_user_path(user),
          <%= best_in_place_if !user.is_agent?, user, :email, as: :input, url: admin_user_path(user) %><br/>
        <%= best_in_place_if !user.is_agent?, user, :street, as: :input, url: admin_user_path(user),
        <%= best_in_place_if !user.is_agent?, user, :city, as: :input, url: admin_user_path(user),
        <%= best_in_place_if !user.is_agent?, user, :state, as: :input, url: admin_user_path(user),
        <%= best_in_place_if !user.is_agent?, user, :zip, as: :input, url: admin_user_path(user),
        <%= t(:work_phone, default: "work") %>: <%= best_in_place_if !user.is_agent?, user, :work_phone,
        <%= t(:home_home, default: "home") %>: <%= best_in_place_if !user.is_agent?, user, :home_phone,
        <%= t(:cell_phone, default: "cell") %>: <%= best_in_place_if !user.is_agent?, user, :cell_phone,
        <%= best_in_place_if !user.is_agent?, user, :notes, as: :textarea, url: admin_user_path(user),
                <%= upper_nav_item(t(:users, default: 'Customers'), admin_users_path(role: 'user'), ["users"], ["index","show","edit","update"], "fas fa-users") if current_user.is_agent? %>