Skip to content

Commit

Permalink
verify csrf for remote forms
Browse files Browse the repository at this point in the history
  • Loading branch information
ryanwoldatwork committed Dec 13, 2022
1 parent 98d6f01 commit e96fb4e
Show file tree
Hide file tree
Showing 12 changed files with 111 additions and 30 deletions.
1 change: 1 addition & 0 deletions app/controllers/admin/forms_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ def form_params
:department,
:bureau,
:load_css,
:verify_csrf,
:ui_truncate_text_responses,
:question_text_01,
:question_text_02,
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ def paginate(scope, default_per_page = 20)
}, collection]
end

# customized response for `#verify_authenticity_token`
def handle_unverified_request
render json: { messages: { submission: ["invalid CSRF authenticity token"] } }, status: :unprocessable_entity
end


private

# For Devise
Expand Down
6 changes: 5 additions & 1 deletion app/controllers/submissions_controller.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# frozen_string_literal: true

class SubmissionsController < ApplicationController
protect_from_forgery only: []
before_action :set_form, only: %i[new create]
append_before_action :verify_authenticity_token, if: :form_requires_verification

layout 'public', only: :new

Expand Down Expand Up @@ -141,4 +141,8 @@ def submission_params
permitted_fields << %i[language location_code referer page fba_directive]
params.require(:submission).permit(permitted_fields)
end

def form_requires_verification
@form.verify_csrf?
end
end
8 changes: 8 additions & 0 deletions app/views/admin/forms/delivery.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@
</div>
<br>
<%= render 'components/whitelist_options', f: f %>
<div class="grid-row">
<div class="grid-col-6">
<div class="field">
<%= f.label :verify_csrf, class: "usa-label" %>
<%= f.check_box :verify_csrf, class: "usa-checkbox" %>
</div>
</div>
</div>
<p>
<%= f.submit (@form.persisted? ? "Update Form" : "Create Form"), class: "usa-button" %>
</p>
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/forms/example.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,6 @@
</div>
<% end %>
<!--- End demo content -->
<script src="<%= touchpoint_url(@form.short_uuid, format: :js) %>" integrity="sha256-<%= form_integrity_checksum(form: @form) %>" async></script>
<script src="<%= touchpoint_url(@form.short_uuid, format: :js) %>" async></script>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<%= form_with(model: omb_cx_reporting_collection, url: (omb_cx_reporting_collection.persisted? ? admin_omb_cx_reporting_collection_path(omb_cx_reporting_collection) : admin_omb_cx_reporting_collections_path), data: { turbo: false }) do |form| %>
<%= form_with(model: omb_cx_reporting_collection, url: (omb_cx_reporting_collection.persisted? ? admin_omb_cx_reporting_collection_path(omb_cx_reporting_collection) : admin_omb_cx_reporting_collections_path), local: true, data: { turbo: false }) do |form| %>
<% if omb_cx_reporting_collection.errors.any? %>
<div id="error_explanation">
<h2><%= pluralize(omb_cx_reporting_collection.errors.count, "error") %> prohibited this omb_cx_reporting_collection from being saved:</h2>
Expand Down
23 changes: 12 additions & 11 deletions app/views/components/forms/_custom.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,27 @@

<form action="<%= root_url %>touchpoints/<%= form.present? ? form.short_uuid : nil %>/submissions.json" method="POST">
<div class="touchpoints-form-body">
<%= hidden_field_tag :authenticity_token, form_authenticity_token if form.verify_csrf? -%>
<%= hidden_field_tag :fba_location_code, params[:location_code] %>
<input type="text" name="fba_directive" id="fba_directive"
style="display:none !important"
tabindex="-1"
style="display:none !important"
tabindex="-1"
autocomplete="off">
<% form.form_sections.each_with_index do |section, index| %>
<div class="section <%= 'visible' if section == form.form_sections.first -%>">
<% if (section != form.form_sections.first) || (section != form.form_sections.last) %>
<% if section != form.form_sections.first %>
<%- if (section != form.form_sections.first) || (section != form.form_sections.last) %>
<%- if section != form.form_sections.first %>
<div class="pagination-buttons">
<%= link_to (t 'form.back'), "#previous-page", class: "usa-button previous-section", 'data-form-section-target' => "" %>
</div>
<br>
<% end %>
<% end %>

<% if form.form_sections.size > 1 %>
<%- if form.form_sections.size > 1 %>
<div class="section-header">
<h4 class="section-title-view">
<% if section.title.present? -%>
<%- if section.title.present? -%>
<%= section.title %> -
<% end %>
<small>
Expand All @@ -40,9 +41,9 @@
end
%>

<% if question.question_type != "hidden_field" %>
<%- if question.question_type != "hidden_field" %>
<div class="question white-bg">
<% if question.question_type == "text_field" %>
<%- if question.question_type == "text_field" %>
<%= render 'components/forms/question_types/text_field', form: form, question: question, question_number: multi_section_question_number %>
<% elsif question.question_type == "text_email_field" %>
<%= render 'components/forms/question_types/text_email_field', form: form, question: question, question_number: multi_section_question_number %>
Expand Down Expand Up @@ -78,9 +79,9 @@
<% end %>
</div>

<% if (section != form.form_sections.first) || (section != form.form_sections.last) %>
<%- if (section != form.form_sections.first) || (section != form.form_sections.last) %>
<div class="pagination-buttons">
<% if (section == form.form_sections.first) && form.early_submission? %>
<%- if (section == form.form_sections.first) && form.early_submission? %>
<div>
<h4>
<%= t 'form.answer_more_questions' %>
Expand All @@ -95,7 +96,7 @@
<% end %>
</div>
<% end %>
<% if section == form.form_sections.last && !form.suppress_submit_button %>
<%- if section == form.form_sections.last && !form.suppress_submit_button %>
<p class="submit-button">
<button type="submit" class="usa-button submit_form_button"><%= t 'form.submit' %></button>
</p>
Expand Down
33 changes: 19 additions & 14 deletions app/views/components/widget/_fba.js.erb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

function FBAform(d, N) {
return {
<% if form.load_css %>
<%- if form.load_css %>
css: '<%= escape_javascript(render partial: "components/widget/widget", formats: :css, locals: { form: form }) %>',
<% end %>
formId: "<%= form.short_uuid %>",
Expand All @@ -29,12 +29,12 @@ function FBAform(d, N) {
init: function(options) {
this.javscriptIsEnabled();
this.options = options;
<% if form.load_css %>
<%- if form.load_css %>
this.loadCss();
<% end %>
this.loadHtml();
<% unless (local_assigns.has_key?(:suppress_ui) && suppress_ui) -%>
<% if form.delivery_method == "modal" -%>
<%- unless (local_assigns.has_key?(:suppress_ui) && suppress_ui) -%>
<%- if form.delivery_method == "modal" -%>
this.loadButton();
<% end %>
<% end %>
Expand All @@ -56,7 +56,7 @@ function FBAform(d, N) {
self.handleClick(event);
});
},
<% if form.load_css %>
<%- if form.load_css %>
loadCss: function()
{
d.head.innerHTML += '<style>' + this.css + '</style>';
Expand All @@ -65,23 +65,23 @@ function FBAform(d, N) {
loadHtml: function()
{
<%# inject the form interface for inline with no wrapper %>
<% if form.delivery_method == "inline" && form.suppress_submit_button %>
<% if form.element_selector? %>
<%- if form.delivery_method == "inline" && form.suppress_submit_button %>
<%- if form.element_selector? %>
if(d.getElementById('<%= form.element_selector %>') != null) {
d.getElementById('<%= form.element_selector %>').innerHTML = "<%= escape_javascript render(partial: 'components/widget/no_modal', locals: { form: form }) %>";
}
<% end %>
<%# inject the form interface for inline with the modal wrapper %>
<% elsif form.delivery_method == "inline" %>
<% if form.element_selector? %>
<%- if form.element_selector? %>
if(d.getElementById('<%= form.element_selector %>') != null) {
d.getElementById('<%= form.element_selector %>').innerHTML = "<%= escape_javascript render(partial: 'components/widget/modal', locals: { form: form }) %>";
}
<% end %>
<% end %>

<%# inject the form interface for modal and custom-button-modal %>
<% if form.delivery_method == "modal" || form.delivery_method == "custom-button-modal" %>
<%- if form.delivery_method == "modal" || form.delivery_method == "custom-button-modal" %>
this.dialogEl = document.createElement('div');
this.dialogEl.setAttribute("hidden", true);
this.dialogEl.setAttribute('class', 'fba-modal');
Expand All @@ -100,8 +100,8 @@ function FBAform(d, N) {
phoneElements[i].addEventListener('keyup', this.handlePhoneInput.bind(this), false);
}
<%# add button behaviors for custom-button-modal %>
<% if form.delivery_method == "custom-button-modal" %>
<% if form.element_selector? %>
<%- if form.delivery_method == "custom-button-modal" %>
<%- if form.element_selector? %>
if(d.getElementById('<%= form.element_selector %>') != null) {
d.getElementById('<%= form.element_selector %>').addEventListener('click', this.handleButtonClick.bind(this), false);
}
Expand Down Expand Up @@ -142,7 +142,7 @@ function FBAform(d, N) {
alertErrorElementBody.innerHTML = "";
},
handleClick: function(e) {
<% if form.delivery_method == "modal" %>
<%- if form.delivery_method == "modal" %>
if (this.dialogOpen && !e.target.closest('#fba-button') && !e.target.closest('.fba-modal-dialog')) {
this.closeDialog();
}
Expand Down Expand Up @@ -546,8 +546,13 @@ function FBAform(d, N) {
xhr.open("POST", url);
xhr.setRequestHeader("Content-Type", "application/json; charset=UTF-8;");
xhr.onload = callback.bind(this);
debugger
xhr.send(JSON.stringify({
"submission": params
"submission": params,
<%- if @form.verify_csrf? %>
"authenticity_token": form.querySelector("#authenticity_token") ?
form.querySelector("#authenticity_token").value : null
<% end %>
}));
},
pagination: function() {
Expand All @@ -562,7 +567,7 @@ function FBAform(d, N) {
if (!this.validateForm(currentPage)) return false;
currentPage.classList.remove("visible");
currentPage.previousElementSibling.classList.add("visible");

// if in a modal, scroll to the top of the modal on previous button click
if(document.getElementsByClassName("fba-modal")[0]) {
document.getElementsByClassName("fba-modal")[0].scrollTo(0,0);
Expand Down
2 changes: 1 addition & 1 deletion config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
config.action_dispatch.show_exceptions = false

# Disable request forgery protection in test environment.
config.action_controller.allow_forgery_protection = false
config.action_controller.allow_forgery_protection = true

# Store uploaded files on the local file system in a temporary directory
config.active_storage.service = :test
Expand Down
7 changes: 7 additions & 0 deletions db/migrate/20221213041152_add_form_csrf_flag.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AddFormCsrfFlag < ActiveRecord::Migration[7.0]
def change
add_column :forms, :verify_csrf, :boolean, default: true

Form.update_all(verify_csrf: false)
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2022_12_07_055927) do
ActiveRecord::Schema[7.0].define(version: 2022_12_13_041152) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

Expand Down Expand Up @@ -235,6 +235,7 @@
t.string "notification_frequency", default: "instant"
t.integer "service_id"
t.integer "questions_count", default: 0
t.boolean "verify_csrf", default: true
t.index ["legacy_touchpoint_id"], name: "index_forms_on_legacy_touchpoint_id"
t.index ["legacy_touchpoint_uuid"], name: "index_forms_on_legacy_touchpoint_uuid"
t.index ["organization_id"], name: "index_forms_on_organization_id"
Expand Down
48 changes: 48 additions & 0 deletions spec/features/touchpoints_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,54 @@
end
end

context 'form.verify_csrf=true, but with invalid authenticity_token' do
before do
expect(form.verify_csrf).to be true
visit touchpoint_path(form)
page.execute_script("document.getElementById('authenticity_token').value = 'an invalid csrf token'")
click_button 'Submit'
end

it 'fails the submission' do
expect(page).to have_content('Error')
expect(page).to have_content('submission invalid CSRF authenticity token')
expect(page.current_path).to eq("/touchpoints/#{form.short_uuid}/submit") # stays on same page
end
end

context 'form.verify_csrf=true, but without authenticity_token' do
before do
expect(form.verify_csrf).to be true
visit touchpoint_path(form)
page.execute_script("document.getElementById('authenticity_token').remove()")
expect(page).to_not have_css("#authenticity_token", visible: false)
click_button 'Submit'
end

it 'fails the submission' do
expect(page).to have_content('Error')
expect(page).to have_content('submission invalid CSRF authenticity token')
expect(page.current_path).to eq("/touchpoints/#{form.short_uuid}/submit") # stays on same page
end
end

context 'form.verify_csrf=false' do
before do
form.update!(verify_csrf: false)
expect(form.verify_csrf).to be false

visit touchpoint_path(form)
expect(page).to_not have_css("#authenticity_token", visible: false)
click_button 'Submit'
end

it 'successful submission' do
expect(page).to have_content('Success')
expect(page).to have_content('Thank you. Your feedback has been received.')
expect(page.current_path).to eq("/touchpoints/#{form.short_uuid}/submit") # stays on same page
end
end

context 'custom success text' do
before do
form.update(success_text: "Much success. \n With a second line.")
Expand Down

0 comments on commit e96fb4e

Please sign in to comment.