Browse files

Completely remove file upload support

Previously, we'd just deleted the form elements. This
wouldn't prevent a malicious request from uploading a
malicious file attachment. So let's remove all upload/
attachment support so that it can't (plus it was code
that was "dead" from an intended execution perspective.
  • Loading branch information...
1 parent eb8cc6e commit 3e21d3c592e1a0fbdbad8892d4de19159e4fb964 @jamiecobbett jamiecobbett committed Oct 19, 2012
Showing with 4 additions and 170 deletions.
  1. +2 −47 lib/guard.rb
  2. +1 −22 lib/zendesk_request.rb
  3. +1 −37 lib/zendesk_ticket.rb
  4. +0 −15 public/stylesheets/gov_feedback.css
  5. +0 −49 test/unit/file_type_test.rb
View
49 lib/guard.rb
@@ -2,13 +2,6 @@
class Guard
- MAX_UPLOAD_FILE_SIZE_IN_BYTE = 20971520 #20MB
-
- @valid_file_type = {
- "text" => /.*/,
- "application" => /-officedocument|pdf/
- }
-
#Content validations
def self.validationsForAmendContent(form_data)
@@errors = {}
@@ -22,48 +15,29 @@ def self.validationsForAmendContent(form_data)
self.validate_date_is_equal_or_greater_than_today("Need by", need_by, "Changes can only be made after today.")
self.validate_not_before_date_is_equal_or_greater_than_need_by(not_before, need_by, "Not before date should be the same or later than the date which the changes are required to be made on.")
- if form_data[:uploaded_data] && self.doesFieldHaveValue(form_data[:uploaded_data][:filename])
- validate_upload_file("uploaded_data", form_data[:uploaded_data])
- end
@@errors
end
#User validations
def self.validationsForCreateUser(form_data)
@@errors = {}
- if form_data[:uploaded_data] && self.doesFieldHaveValue(form_data[:uploaded_data][:filename])
- required = ["name", "email", "job"]
- else
- required = ["name", "email", "job", "user_name", "user_email"]
- end
+ required = ["name", "email", "job", "user_name", "user_email"]
validate(form_data, required, {"phone" => form_data["phone"]}, {"email" => form_data["email"]})
- if form_data[:uploaded_data] && self.doesFieldHaveValue(form_data[:uploaded_data][:filename])
- validate_upload_file("uploaded_data", form_data[:uploaded_data])
- end
-
@@errors
end
def self.validationsForDeleteUser(form_data)
@@errors = {}
- if form_data[:uploaded_data] && self.doesFieldHaveValue(form_data[:uploaded_data][:filename])
- required = ["name", "email", "job"]
- else
- required = ["name", "email", "job", "user_name", "user_email"]
- end
+ required = ["name", "email", "job", "user_name", "user_email"]
validate(form_data, required, {"phone" => form_data["phone"]}, {"email" => form_data["email"]})
self.checkOptionalDateFieldsAreComplete(form_data, [["Not before", "not_before_day", "not_before_month", "not_before_year"]])
not_before = validate_date_in_valid_range("Not_before", "not_before_day", "not_before_month", "not_before_year", form_data)
self.validate_date_is_equal_or_greater_than_today("Not before", not_before, "Not before date should be the same or later than today.")
- if form_data[:uploaded_data] && self.doesFieldHaveValue(form_data[:uploaded_data][:filename])
- validate_upload_file("uploaded_data", form_data[:uploaded_data])
- end
-
@@errors
end
@@ -163,12 +137,6 @@ def self.doesFieldHaveValue(field_value)
field_value && !field_value.strip.empty?
end
- def self.validate_upload_file(field_name, upload_file)
- if validate_file_type(field_name, upload_file[:type]) && upload_file[:tempfile].size > MAX_UPLOAD_FILE_SIZE_IN_BYTE
- @@errors[field_name] = "The attached file, #{upload_file[:filename]}, is bigger than 20MB size limitation."
- end
- end
-
def self.validate_date_in_valid_range(date_field_name, day, month, year, form_data)
if !form_data[day].empty? && !form_data[month].empty? && !form_data[year].empty?
date_to_validate = form_data[day] + "-" + form_data[month] + "-" + form_data[year]
@@ -205,17 +173,4 @@ def self.validate_organisation_or_other_is_entered(form_data)
form_data[:other_organisation] = ""
end
end
-
- def self.validate_file_type(field_name, file_type)
- @@errors ||= {}
- valid = false
- type = file_type.split("/")
-
- if @valid_file_type[type[0]] && (type[1] =~ @valid_file_type[type[0]])
- valid = true
- else
- @@errors[field_name] = "Only text, word and pdf file allowed."
- end
- valid
- end
end
View
23 lib/zendesk_request.rb
@@ -12,26 +12,6 @@ def self.get_organisations(client)
def self.raise_zendesk_request(client, params, from_route)
ticket_to_raise = ZendeskTicket.new(client, params, from_route)
-
- if ticket_to_raise.has_attachments
- create_ticket_with_attachment(client, ticket_to_raise)
- else
- client.ticket.create(
- :subject => ticket_to_raise.subject,
- :description => "Created via Govt API",
- :priority => "normal",
- :requester => {"locale_id" => 1, "name" => ticket_to_raise.name, "email" => ticket_to_raise.email},
- :fields => [{"id" => "21494928", "value" => ticket_to_raise.organisation},
- {"id" => "21487987", "value" => ticket_to_raise.job},
- {"id" => "21471291", "value" => ticket_to_raise.phone},
- {"id" => "21485833", "value" => ticket_to_raise.need_by_date},
- {"id" => "21502036", "value" => ticket_to_raise.not_before_date}],
- :tags => [ticket_to_raise.tag],
- :comment => {:value => ticket_to_raise.comment})
- end
- end
-
- def self.create_ticket_with_attachment(client, ticket_to_raise)
client.ticket.create(
:subject => ticket_to_raise.subject,
:description => "Created via Govt API",
@@ -43,7 +23,6 @@ def self.create_ticket_with_attachment(client, ticket_to_raise)
{"id" => "21485833", "value" => ticket_to_raise.need_by_date},
{"id" => "21502036", "value" => ticket_to_raise.not_before_date}],
:tags => [ticket_to_raise.tag],
- :comment => {:value => ticket_to_raise.comment, :uploads => ticket_to_raise.file_token})
+ :comment => {:value => ticket_to_raise.comment})
end
-
end
View
38 lib/zendesk_ticket.rb
@@ -1,6 +1,6 @@
class ZendeskTicket
- attr_reader :name, :email, :organisation, :job, :phone, :comment, :subject, :tag, :need_by_date, :not_before_date, :file_token
+ attr_reader :name, :email, :organisation, :job, :phone, :comment, :subject, :tag, :need_by_date, :not_before_date
@@in_comments = {"amend-content" => [:other_organisation, :url1, :url2, :url3],
"create-user" => [:other_organisation, :user_name, :user_email, :additional],
"remove-user" => [:other_organisation, :user_name, :user_email, :additional],
@@ -52,16 +52,8 @@ def initialize(client, params, from_route)
if has_value(params[:not_before_day])
@not_before_date = params[:not_before_day] + "/" + params[:not_before_month] + "/" + params[:not_before_year]
end
-
- check_for_attachments(client, params)
- end
-
-
- def has_attachments
- @file_token.length > 0
end
-
private
def has_value(param)
@@ -149,34 +141,6 @@ def build_full_url_path(partial_path)
end
end
-# attachments
- def upload_file_to_create_file_token(client, tempfile, filename)
- directory = "./tmp"
- path = File.join(directory, filename)
- File.open(path, "wb") { |f| f.write(tempfile.read) }
- file_token = upload_file(client, path)
- File.delete(path)
- file_token
- end
-
- def upload_file(client, path)
- upload = ZendeskAPI::Upload.create(client, :file => File.open(path))
- upload.token
- end
-
- def check_for_attachments(client, params)
- @file_token = []
- if params[:uploaded_data] && doesFieldHaveValue(params[:uploaded_data][:filename])
- tempfile = params[:uploaded_data][:tempfile]
- filename = params[:uploaded_data][:filename]
- @file_token << upload_file_to_create_file_token(client, tempfile, filename)
- end
-
- if !@file_token.empty? && @comment.empty?
- @comment = "[Attachment(s)]"
- end
- end
-
def doesFieldHaveValue(field_value)
field_value && !field_value.strip.empty?
end
View
15 public/stylesheets/gov_feedback.css
@@ -106,10 +106,6 @@ label, legend.date {
width: 20%;
}
-div.upload label, div.comments label {
- width: 100%;
-}
-
label.long, legend.long {
width: auto;
}
@@ -177,17 +173,6 @@ input[type="text"], input[type="email"], input[type="tel"] {
border: 1px solid #BBB;
}
-#file-input, #file-added {
- min-width: 350px;
- margin-left: 0.3em;
- border: 1px, solid, #BBB;
-}
-
-div.upload {
- padding-top: 0.3em;
- padding-bottom: 0.3em;
-}
-
div.line {
border-bottom: 1px solid #BBB;
}
View
49 test/unit/file_type_test.rb
@@ -1,49 +0,0 @@
-require "test_helper"
-
-class ValidationTest < Test::Unit::TestCase
- include Rack::Test::Methods
-
- def test_should_return_true_when_file_type_is_valid
- #Given
- file_type_text = "text/csv"
- file_type_word = "application/vnd.openxmlformats-officedocument.wordprocessingml.document"
- file_type_pdf = "application/pdf"
- field_name = "Uploaded data"
-
- #When
- is_valid_text = Guard.validate_file_type(field_name,file_type_text)
- is_valid_word = Guard.validate_file_type(field_name, file_type_word)
- is_valid_pdf = Guard.validate_file_type(field_name, file_type_pdf)
-
- #Then
- assert(is_valid_text)
- assert(is_valid_word)
- assert(is_valid_pdf)
- end
-
- def test_should_return_false_with_wrong_category
- #Given
- expected_error_message = "Only text, word and pdf file allowed."
- file_type_image = "image/png"
- field_name = "Uploaded data"
-
- #When
- is_valid = Guard.validate_file_type(field_name, file_type_image)
-
- #Then
- assert(!is_valid)
- end
-
- def test_should_return_false_unsupported_type_in_application_category
- #Given
- expected_error_message = "Only text, word and pdf file allowed."
- file_type_image = "application/atom+xml"
- field_name = "Uploaded data"
-
- #When
- is_valid = Guard.validate_file_type(field_name, file_type_image)
-
- #Then
- assert(!is_valid)
- end
-end

0 comments on commit 3e21d3c

Please sign in to comment.