Skip to content

Commit

Permalink
Adding fix for the XSS issue
Browse files Browse the repository at this point in the history
  • Loading branch information
alan-at-work committed Oct 2, 2024
1 parent ccb8dc2 commit 676810e
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 92 deletions.
44 changes: 14 additions & 30 deletions app/decorators/investigation_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ def title
end

def unformatted_description
# Bypasses `FormattedDescription` for situations where we want the raw value of the field.
object.description
end

Expand Down Expand Up @@ -44,7 +43,7 @@ def case_title_key(viewing_user)
def case_summary_values
values = []

if investigation.is_private?
if object.is_private?
values << { text: "" }
values << { text: "" }
else
Expand All @@ -53,7 +52,6 @@ def case_summary_values
end

values << { text: status }

values
end

Expand All @@ -62,25 +60,21 @@ def source_details_summary_list(view_protected_details: false)
contact_details << h.tag.p(I18n.t("case.protected_details", data_type: "#{object.case_type} contact details"), class: "govuk-body-s govuk-!-margin-bottom-1 opss-secondary-text opss-text-align-right")

rows = [
should_display_date_received? ? { key: { text: "Received date" }, value: { text: date_received.to_formatted_s(:govuk) } } : nil,
should_display_received_by? ? { key: { text: "Received by" }, value: { text: received_type.upcase_first } } : nil,
{ key: { text: "Source type" }, value: { text: complainant.complainant_type } },
{ key: { text: "Contact details" }, value: { text: contact_details } }
]

rows.compact!

h.govuk_summary_list(rows:, borders: false, classes: "opss-summary-list-mixed opss-summary-list-mixed--narrow-dt")
end

def contact_details_list
h.tag.ul(class: "govuk-list govuk-list--bullet govuk-list--spaced") do
lis = []
lis << h.tag.li(complainant.name) if complainant.name.present?
lis << h.tag.li("Telephone: #{complainant.phone_number}") if complainant.phone_number.present?
lis << h.tag.li("Email: ".html_safe + h.mail_to(complainant.email_address, class: "govuk-link govuk-link--no-visited-state")) if complainant.email_address.present?
lis << h.tag.li(complainant.other_details) if complainant.other_details.present?
safe_join(lis)
lis << h.tag.li(h.sanitize(complainant.name)) if complainant.name.present?
lis << h.tag.li("Telephone: #{h.sanitize(complainant.phone_number)}") if complainant.phone_number.present?
lis << h.tag.li("Email: ".html_safe + h.mail_to(h.sanitize(complainant.email_address), class: "govuk-link govuk-link--no-visited-state")) if complainant.email_address.present?
lis << h.tag.li(h.sanitize(complainant.other_details)) if complainant.other_details.present?
h.safe_join(lis)
end
end

Expand All @@ -95,7 +89,7 @@ def created_by
end

def owner_display_name_for(viewer:)
return "No notification owner" unless investigation.owner
return "No notification owner" unless object.owner

owner.owner_short_name(viewer:)
end
Expand Down Expand Up @@ -125,22 +119,12 @@ def visibility_action
private

def category
@category ||= \
if categories.size == 1
h.simple_format(categories.first.downcase.upcase_first, class: "govuk-body")
else
h.tag.ul(class: "govuk-list") do
lis = categories.map { |cat| h.tag.li(cat.downcase.upcase_first) }
lis.join.html_safe
end
end
end

def should_display_date_received?
false
end

def should_display_received_by?
false
@category ||= if categories.size == 1
h.simple_format(categories.first.downcase.upcase_first, class: "govuk-body")
else
h.tag.ul(class: "govuk-list") do
h.safe_join(categories.map { |cat| h.tag.li(cat.downcase.upcase_first) })
end
end
end
end
57 changes: 32 additions & 25 deletions app/decorators/product_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,52 @@ class ProductDecorator < ApplicationDecorator
delegate_all
decorates_association :investigations

# Include the sanitize helper
include ActionView::Helpers::SanitizeHelper

# Safely combines the brand and name of the product
def name_with_brand
[brand, name].compact.join(" ")
[sanitize(brand), sanitize(name)].compact.join(" ")
end

def pretty_description
"Product: #{name}"
"Product: #{sanitize(name)}"
end

def unformatted_description
# Bypasses `FormattedDescription` for situations where we want the raw value of the field.
object.description
end

def details_list(date_case_closed: nil)
timestamp = date_case_closed.to_i if date_case_closed

psd_ref_key_html = '<abbr title="Product Safety Database">PSD</abbr> <span title="reference">ref</span>'.html_safe
psd_secondary_text_html = if date_case_closed.present?
" - The <abbr>PSD</abbr> reference number for this version of the product record - as recorded when the notification was closed: #{date_case_closed.to_formatted_s(:govuk)}."
" - The <abbr>PSD</abbr> reference number for this version of the product record - as recorded when the notification was closed: #{sanitize(date_case_closed.to_formatted_s(:govuk))}."
else
" - The <abbr>PSD</abbr> reference number for this product record"
end.html_safe
webpage_html = "<span class='govuk-!-font-size-16'>#{webpage}</span>".html_safe
when_placed_on_market_value = when_placed_on_market == "unknown_date" ? nil : when_placed_on_market

webpage_html = "<span class='govuk-!-font-size-16'>#{sanitize(webpage.presence || '')}</span>".html_safe

when_placed_on_market_value = when_placed_on_market == "unknown_date" ? nil : sanitize(when_placed_on_market.presence || "")

psd_ref_value_html = date_case_closed.present? ? psd_ref(timestamp:, investigation_was_closed: true) : psd_ref(timestamp:, investigation_was_closed: false)

rows = [
{ key: { text: psd_ref_key_html }, value: { text: "#{psd_ref_value_html}#{psd_secondary_text_html}".html_safe } },
{ key: { text: "Brand name" }, value: { text: object.brand } },
{ key: { text: "Product name" }, value: { text: object.name } },
{ key: { text: "Category" }, value: { text: category } },
{ key: { text: "Subcategory" }, value: { text: subcategory } },
{ key: { text: "Barcode" }, value: { text: barcode } },
{ key: { text: "Description" }, value: { text: description } },
{ key: { text: "Webpage" }, value: { text: webpage_html } },
{ key: { text: "Brand name" }, value: { text: sanitize(object.brand.presence || "") } },
{ key: { text: "Product name" }, value: { text: sanitize(object.name.presence || "") } },
{ key: { text: "Category" }, value: { text: sanitize(category.presence || "") } },
{ key: { text: "Subcategory" }, value: { text: sanitize(subcategory.presence || "") } },
{ key: { text: "Barcode" }, value: { text: sanitize(barcode.presence || "") } },
{ key: { text: "Description" }, value: { text: sanitize(description.presence || "") } },
{ key: { text: "Webpage" }, value: { text: webpage_html } }, # Already sanitized
{ key: { text: "Market date" }, value: { text: when_placed_on_market_value } },
{ key: { text: "Country of origin" }, value: { text: country_from_code(country_of_origin) } },
{ key: { text: "Counterfeit" }, value: counterfeit_row_value },
{ key: { text: "Product marking" }, value: { text: markings } },
{ key: { text: "Other product identifiers" }, value: { text: product_code } },
{ key: { text: "Country of origin" }, value: { text: sanitize(country_from_code(country_of_origin.presence || "")) } },
{ key: { text: "Counterfeit" }, value: counterfeit_row_value }, # Assuming this is already safe
{ key: { text: "Product marking" }, value: { text: sanitize(markings.presence || "") } },
{ key: { text: "Other product identifiers" }, value: { text: sanitize(product_code.presence || "") } },
]

h.govuk_summary_list(rows:)
Expand All @@ -65,7 +72,7 @@ def when_placed_on_market
end

def subcategory_and_category_label
product_and_category = [subcategory.presence, category.presence].compact
product_and_category = [sanitize(subcategory.presence), sanitize(category.presence)].compact

if product_and_category.length > 1
"#{product_and_category.first} (#{product_and_category.last.downcase})"
Expand All @@ -79,7 +86,7 @@ def markings
return I18n.t(".product.unknown") if object.markings_unknown?
return I18n.t(".product.none") if object.markings_no?

object.markings.join(", ")
sanitize(object.markings.join(", "))
end

def case_ids
Expand All @@ -88,11 +95,11 @@ def case_ids

def counterfeit_row_value
if product.counterfeit?
return { text: "<span class=\"opss-tag opss-tag--risk2 opss-tag--lrg\">Yes</span> - #{counterfeit_explanation}".html_safe }
return { text: "<span class=\"opss-tag opss-tag--risk2 opss-tag--lrg\">Yes</span> - #{sanitize(counterfeit_explanation)}".html_safe }
end

if product.genuine?
return { text: "No - #{counterfeit_explanation}" }
return { text: "No - #{sanitize(counterfeit_explanation)}" }
end

{ text: I18n.t(object.authenticity || :missing, scope: Product.model_name.i18n_key) }
Expand All @@ -107,14 +114,14 @@ def counterfeit_value
def counterfeit_explanation
return if product.unsure?

product.counterfeit? ? I18n.t("products.counterfeit") : I18n.t("products.genuine")
sanitize(product.counterfeit? ? I18n.t("products.counterfeit") : I18n.t("products.genuine"))
end

def owning_team_link
return "No owner" if owning_team.nil?
return "Your team is the product record owner" if owning_team == h.current_user.team

h.link_to owning_team.name, h.owner_product_path(object), class: "govuk-link govuk-link--no-visited-state"
h.link_to sanitize(owning_team.name), h.owner_product_path(object), class: "govuk-link govuk-link--no-visited-state"
end

def unique_cases_except(investigation)
Expand Down Expand Up @@ -149,15 +156,15 @@ def product_name_with_cases
rows = [
{
key: { text: "Product" },
value: { text: "#{name_with_brand} <span class='govuk-!-font-weight-regular govuk-!-font-size-16 govuk-!-padding-left-2 opss-no-wrap'>(psd-#{id})</span>".html_safe }
value: { text: "#{sanitize(name_with_brand)} <span class='govuk-!-font-weight-regular govuk-!-font-size-16 govuk-!-padding-left-2 opss-no-wrap'>(psd-#{sanitize(id.to_s)})</span>".html_safe }
}
]

if object.investigations.any?
object.investigations.each_with_index do |investigation, i|
rows << {
key: { text: i.zero? ? "Notification(s)" : "" },
value: { text: "#{investigation.user_title} <span class='govuk-!-font-weight-regular govuk-!-font-size-16 govuk-!-padding-left-2 opss-no-wrap'>(#{investigation.pretty_id})</span>".html_safe }
value: { text: "#{sanitize(investigation.user_title)} <span class='govuk-!-font-weight-regular govuk-!-font-size-16 govuk-!-padding-left-2 opss-no-wrap'>(#{sanitize(investigation.pretty_id)})</span>".html_safe }
}
end
end
Expand Down
Loading

0 comments on commit 676810e

Please sign in to comment.