From 523980e2015baae3fc63721ded539270fba4678d Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sat, 25 Jul 2020 09:17:58 +1000 Subject: [PATCH] fix: correctly encode user entered data in attributes, Javascript, and HTML Fixes: https://github.com/pact-foundation/pact_broker/issues/325 --- .../api/renderers/html_pact_renderer.rb | 34 ++++++++++++++----- lib/pact_broker/badges/service.rb | 5 +-- lib/pact_broker/doc/views/layouts/main.haml | 2 +- lib/pact_broker/test/test_data_builder.rb | 8 ++--- lib/pact_broker/ui/controllers/clusters.rb | 2 +- lib/pact_broker/ui/controllers/groups.rb | 4 +-- lib/pact_broker/ui/controllers/index.rb | 2 +- lib/pact_broker/ui/controllers/matrix.rb | 4 +-- lib/pact_broker/ui/views/groups/show.html.erb | 6 ++-- .../ui/views/index/show-with-tags.haml | 20 +++++------ lib/pact_broker/ui/views/index/show.haml | 12 +++---- lib/pact_broker/ui/views/layouts/main.haml | 2 +- lib/pact_broker/ui/views/matrix/show.haml | 9 +++-- public/javascripts/pact.js | 8 +++-- script/seed.rb | 12 +++---- .../api/renderers/html_pact_renderer_spec.rb | 30 ++++++++++++---- spec/lib/pact_broker/badges/service_spec.rb | 12 +++---- 17 files changed, 105 insertions(+), 67 deletions(-) diff --git a/lib/pact_broker/api/renderers/html_pact_renderer.rb b/lib/pact_broker/api/renderers/html_pact_renderer.rb index 86ef66bb8..bdde3e3ad 100644 --- a/lib/pact_broker/api/renderers/html_pact_renderer.rb +++ b/lib/pact_broker/api/renderers/html_pact_renderer.rb @@ -4,6 +4,7 @@ require 'pact/doc/markdown/consumer_contract_renderer' require 'pact_broker/api/pact_broker_urls' require 'pact_broker/logging' +require 'rack' module PactBroker module Api @@ -58,8 +59,8 @@ def pact_metadata #{badge_list_item} #{badge_markdown_item}
  • - #{@pact.consumer.name} version: - #{@pact.consumer_version_number}#{tags} + #{consumer_name} version: + #{consumer_version_number}#{tags}
  • Date published: @@ -75,9 +76,9 @@ def pact_metadata Home
  • - @@ -88,7 +89,7 @@ def pact_metadata def badge_list_item "
  • - +
    • " end @@ -117,7 +118,19 @@ def base_url end def title - "Pact between #{@pact.consumer.name} and #{@pact.provider.name}" + "Pact between #{consumer_name} and #{provider_name}" + end + + def consumer_version_number + h(@pact.consumer_version_number) + end + + def consumer_name + h(@pact.consumer.name) + end + + def provider_name + h(@pact.provider.name) end def published_date @@ -154,7 +167,8 @@ def badge_url def tags if @pact.consumer_version_tag_names.any? - " (#{@pact.consumer_version_tag_names.join(", ")})" + tag_names = @pact.consumer_version_tag_names.collect{ |t| h(t) }.join(", ") + " (#{tag_names})" else "" end @@ -179,6 +193,10 @@ def consumer_contract logger.info "Could not parse the following content to a Pact due to #{e.class} #{e.message}, showing raw content instead: #{@json_content}" raise NotAPactError end + + def h string + Rack::Utils.escape_html(string) + end end end end diff --git a/lib/pact_broker/badges/service.rb b/lib/pact_broker/badges/service.rb index 88866ddaf..daf9a936d 100644 --- a/lib/pact_broker/badges/service.rb +++ b/lib/pact_broker/badges/service.rb @@ -4,6 +4,7 @@ require 'pact_broker/logging' require 'pact_broker/configuration' require 'pact_broker/build_http_options' +require 'erb' module PactBroker module Badges @@ -45,7 +46,7 @@ def badge_title pact, label, initials, metadata title = case (label || '').downcase when 'consumer' then consumer_name when 'provider' then provider_name - else "#{consumer_name}%2F#{provider_name}" + else "#{consumer_name}/#{provider_name}" end "#{title} pact".downcase end @@ -111,7 +112,7 @@ def build_shield_io_uri left_text, right_text, color end def escape_text text - text.gsub(" ", "%20").gsub("-", "--").gsub("_", "__") + ERB::Util.url_encode(text.gsub("-", "--").gsub("_", "__")) end def do_request(uri) diff --git a/lib/pact_broker/doc/views/layouts/main.haml b/lib/pact_broker/doc/views/layouts/main.haml index 820bbd6af..ffa647373 100644 --- a/lib/pact_broker/doc/views/layouts/main.haml +++ b/lib/pact_broker/doc/views/layouts/main.haml @@ -2,4 +2,4 @@ %head %link{rel: 'stylesheet', href: "#{base_url}/css/bootstrap.min.css"} %body{style: 'margin:20px'} - = yield \ No newline at end of file + != yield \ No newline at end of file diff --git a/lib/pact_broker/test/test_data_builder.rb b/lib/pact_broker/test/test_data_builder.rb index 600a7bde4..1f088923a 100644 --- a/lib/pact_broker/test/test_data_builder.rb +++ b/lib/pact_broker/test/test_data_builder.rb @@ -73,14 +73,14 @@ def create_condor self end - def create_pact_with_hierarchy consumer_name = "Consumer", consumer_version_number = "1.2.3", provider_name = "Provider", json_content = default_json_content + def create_pact_with_hierarchy consumer_name = "Consumer", consumer_version_number = "1.2.3", provider_name = "Provider", json_content = nil use_consumer(consumer_name) create_consumer(consumer_name) if !consumer use_provider(provider_name) create_provider provider_name if !provider use_consumer_version(consumer_version_number) create_consumer_version(consumer_version_number) if !consumer_version - create_pact json_content: json_content + create_pact json_content: json_content || default_json_content self end @@ -436,10 +436,10 @@ def set_created_at_if_set created_at, table_name, selector def default_json_content { "consumer" => { - "name" => "Condor" + "name" => consumer.name }, "provider" => { - "name" => "Pricing Service" + "name" => provider.name }, "interactions" => [], "random" => rand diff --git a/lib/pact_broker/ui/controllers/clusters.rb b/lib/pact_broker/ui/controllers/clusters.rb index 69f5cbfeb..d03b79a53 100644 --- a/lib/pact_broker/ui/controllers/clusters.rb +++ b/lib/pact_broker/ui/controllers/clusters.rb @@ -19,7 +19,7 @@ def initialize pacticipants, relationships get "/" do view_model = ViewDomain::IndexItems.new(pacticipant_service.find_index_items, base_url: base_url) - haml 'clusters/show', locals: {relationships: view_model, base_url: base_url} + haml 'clusters/show', locals: { relationships: view_model, base_url: base_url }, escape_html: true end end diff --git a/lib/pact_broker/ui/controllers/groups.rb b/lib/pact_broker/ui/controllers/groups.rb index 82f0d4e57..8adfe445c 100644 --- a/lib/pact_broker/ui/controllers/groups.rb +++ b/lib/pact_broker/ui/controllers/groups.rb @@ -13,13 +13,13 @@ class Groups < Base pacticipant = pacticipant_service.find_pacticipant_by_name(params[:name]) erb :'groups/show.html', { locals: { - csv_path: "#{base_url}/groups/#{params[:name]}.csv", + csv_path: "#{base_url}/groups/#{ERB::Util.url_encode(params[:name])}.csv", pacticipant_name: params[:name], repository_url: pacticipant&.repository_url, base_url: base_url } }, { - layout: 'layouts/main' + layout: 'layouts/main', } end diff --git a/lib/pact_broker/ui/controllers/index.rb b/lib/pact_broker/ui/controllers/index.rb index 32160d285..945c674dc 100644 --- a/lib/pact_broker/ui/controllers/index.rb +++ b/lib/pact_broker/ui/controllers/index.rb @@ -36,7 +36,7 @@ class Index < Base base_url: base_url } - haml page, {locals: locals, layout: :'layouts/main'} + haml page, { locals: locals, layout: :'layouts/main', escape_html: true } end def set_headers diff --git a/lib/pact_broker/ui/controllers/matrix.rb b/lib/pact_broker/ui/controllers/matrix.rb index 9e73edce9..23d280d90 100644 --- a/lib/pact_broker/ui/controllers/matrix.rb +++ b/lib/pact_broker/ui/controllers/matrix.rb @@ -42,7 +42,7 @@ class Matrix < Base Padrino.logger.exception(e) unless e.is_a?(PactBroker::Error) locals[:errors] = [e.message] end - haml :'matrix/show', {locals: locals, layout: :'layouts/main'} + haml :'matrix/show', { locals: locals, layout: :'layouts/main', escape_html: true } end get "/provider/:provider_name/consumer/:consumer_name" do @@ -60,7 +60,7 @@ class Matrix < Base badge_url: nil, base_url: base_url } - haml :'matrix/show', {locals: locals, layout: :'layouts/main'} + haml :'matrix/show', { locals: locals, layout: :'layouts/main', escape_html: true } end def create_selector_objects(selector_hashes) diff --git a/lib/pact_broker/ui/views/groups/show.html.erb b/lib/pact_broker/ui/views/groups/show.html.erb index 1341d7968..87dc93edc 100644 --- a/lib/pact_broker/ui/views/groups/show.html.erb +++ b/lib/pact_broker/ui/views/groups/show.html.erb @@ -31,13 +31,13 @@ body{ -

    Network graph of <%= pacticipant_name %> relationships

    +

    Network graph of <%= escape_html(pacticipant_name) %> relationships

    <% if repository_url %>

    Repository URL: <% -repository_link = "#{escape_html(repository_url)}" +repository_link = "#{repository_url}" %> <%= Sanitize.fragment(repository_link, Sanitize::Config::BASIC) %> @@ -105,7 +105,7 @@ var relationshipPath = function(relationship, nodeLocations, pacticipants) { var latestPactUrl = function (consumerName, providerName) { //TODO send this with the relationship data - return '<%= base_url %>/pacts/provider/' + providerName + '/consumer/' + consumerName + '/latest'; + return '<%= base_url %>' + '/pacts/provider/' + encodeURIComponent(providerName) + '/consumer/' + encodeURIComponent(consumerName) + '/latest'; }; var relationshipData = function(pacticipant, relationships) { diff --git a/lib/pact_broker/ui/views/index/show-with-tags.haml b/lib/pact_broker/ui/views/index/show-with-tags.haml index 45c060af4..88ea4a7aa 100644 --- a/lib/pact_broker/ui/views/index/show-with-tags.haml +++ b/lib/pact_broker/ui/views/index/show-with-tags.haml @@ -1,9 +1,9 @@ %body - = render :haml, :'index/_css_and_js', :layout => false + != render :haml, :'index/_css_and_js', :layout => false .container - = render :haml, :'index/_navbar', :layout => false, locals: {tag_toggle: false, base_url: base_url} + != render :haml, :'index/_navbar', :layout => false, locals: {tag_toggle: false, base_url: base_url} - if index_items.empty? - = render :haml, :'index/_getting-started', :layout => false + != render :haml, :'index/_getting-started', :layout => false %h1.page-header Pacts %table.table.table-bordered.table-striped{ id: 'relationships' } @@ -37,10 +37,10 @@ %tr{'data-pact-versions-url': index_item.pact_versions_url, 'data-consumer-name': index_item.consumer_name, 'data-provider-name': index_item.provider_name, 'data-integration-url': index_item.integration_url} %td.consumer %a{:href => index_item.consumer_group_url } - = escape_html(index_item.consumer_name) + = index_item.consumer_name %td.consumer-version-number{"data-text": index_item.consumer_version_order} %div.clippable - = escape_html(index_item.consumer_version_number) + = index_item.consumer_version_number - if index_item.consumer_version_number %button.clippy.invisible{ title: "Copy to clipboard" } %span.glyphicon.glyphicon-copy @@ -49,7 +49,7 @@ latest - index_item.consumer_version_latest_tag_names.each do | tag_name | .tag.label.label-primary - = escape_html(tag_name) + = tag_name %td.pact %span.pact %a{ href: index_item.pact_url, title: "View pact" } @@ -57,16 +57,16 @@ %a{ href: index_item.pact_matrix_url, title: "View pact matrix" } %td.provider %a{ href: index_item.provider_group_url } - = escape_html(index_item.provider_name) + = index_item.provider_name %td.provider-version-number %div.clippable - = escape_html(index_item.provider_version_number) + = index_item.provider_version_number - if index_item.provider_version_number %button.clippy.invisible{ title: "Copy to clipboard" } %span.glyphicon.glyphicon-copy - index_item.provider_version_latest_tag_names.each do | tag_name | .tag.label.label-primary - = escape_html(tag_name) + = tag_name %td{"data-text": index_item.publication_date_of_latest_pact_order} = index_item.publication_date_of_latest_pact.gsub("about ", "") %td{ class: index_item.webhook_status } @@ -86,7 +86,7 @@ %div.pagination - pagination_locals = { page_number: page_number, page_size: page_size, pagination_record_count: pagination_record_count, current_page_size: current_page_size } - = render :haml, :'index/_pagination', :layout => false, locals: pagination_locals + != render :haml, :'index/_pagination', :layout => false, locals: pagination_locals :javascript $(function(){ diff --git a/lib/pact_broker/ui/views/index/show.haml b/lib/pact_broker/ui/views/index/show.haml index 059a01208..9cd62e6b4 100644 --- a/lib/pact_broker/ui/views/index/show.haml +++ b/lib/pact_broker/ui/views/index/show.haml @@ -1,9 +1,9 @@ %body - = render :haml, :'index/_css_and_js', :layout => false + != render :haml, :'index/_css_and_js', :layout => false .container - = render :haml, :'index/_navbar', :layout => false, locals: {tag_toggle: true, base_url: base_url} + != render :haml, :'index/_navbar', :layout => false, locals: {tag_toggle: true, base_url: base_url} - if index_items.empty? - = render :haml, :'index/_getting-started', :layout => false + != render :haml, :'index/_getting-started', :layout => false %h1.page-header Pacts %table.table.table-bordered.table-striped{ id: 'relationships' } @@ -32,7 +32,7 @@ %td %td.consumer %a{ href: index_item.consumer_group_url } - = escape_html(index_item.consumer_name) + = index_item.consumer_name %td.pact %span.pact %a{ href: index_item.latest_pact_url, :title => "View pact" } @@ -40,7 +40,7 @@ %a{ href: index_item.pact_matrix_url, title: "View pact matrix" } %td.provider %a{ href: index_item.provider_group_url } - = escape_html(index_item.provider_name) + = index_item.provider_name %td %td{"data-text": index_item.publication_date_of_latest_pact_order} = index_item.publication_date_of_latest_pact @@ -58,7 +58,7 @@ %div.pagination - pagination_locals = { page_number: page_number, page_size: page_size, pagination_record_count: pagination_record_count, current_page_size: current_page_size } - = render :haml, :'index/_pagination', :layout => false, locals: pagination_locals + != render :haml, :'index/_pagination', :layout => false, locals: pagination_locals :javascript diff --git a/lib/pact_broker/ui/views/layouts/main.haml b/lib/pact_broker/ui/views/layouts/main.haml index d5f5e3fdc..748e0714f 100644 --- a/lib/pact_broker/ui/views/layouts/main.haml +++ b/lib/pact_broker/ui/views/layouts/main.haml @@ -6,4 +6,4 @@ %link{rel: 'stylesheet', href: "#{base_url}/css/bootstrap.min.css"} %script{type: 'text/javascript', src:"#{base_url}/javascripts/jquery-3.3.1.min.js"} %script{type: 'text/javascript', src:"#{base_url}/js/bootstrap.min.js"} - = yield + != yield diff --git a/lib/pact_broker/ui/views/matrix/show.haml b/lib/pact_broker/ui/views/matrix/show.haml index 92334bc8f..7475c3de3 100644 --- a/lib/pact_broker/ui/views/matrix/show.haml +++ b/lib/pact_broker/ui/views/matrix/show.haml @@ -20,15 +20,14 @@ - if defined?(errors) && errors.any? - errors.each do | error | %div.alert.alert-danger - = escape_html(error) - + = error %form{action: "#{base_url}/matrix", onsubmit:'return onSubmit()'} - selectors.each_with_index do | selector, index | .selector %label{for: "pacticipant#{index}"} Pacticipant name - %input{name: 'q[]pacticipant', id: "pacticipant1#{index}", class: 'pacticipant_name', value: escape_html(selector.pacticipant_name)} + %input{name: 'q[]pacticipant', id: "pacticipant1#{index}", class: 'pacticipant_name', value: selector.pacticipant_name} .input-group @@ -45,9 +44,9 @@ %option{ value: 'specify-all-tagged', selected: selector.specify_all_tagged } All versions with tag... - %input{name: 'q[]version', type: 'text', id: "pacticipant#{index}_version", class: 'version', value: escape_html(selector.pacticipant_version_number)} + %input{name: 'q[]version', type: 'text', id: "pacticipant#{index}_version", class: 'version', value: selector.pacticipant_version_number} - %input{name: 'q[]tag', type: 'text', id: "pacticipant#{index}_tag", class: 'tag', value: escape_html(selector.tag)} + %input{name: 'q[]tag', type: 'text', id: "pacticipant#{index}_tag", class: 'tag', value: selector.tag} %input{name: 'q[]latest', value: 'true', hidden: true, class: 'latest-flag'} diff --git a/public/javascripts/pact.js b/public/javascripts/pact.js index 68528242c..bcfcddccb 100644 --- a/public/javascripts/pact.js +++ b/public/javascripts/pact.js @@ -39,10 +39,14 @@ $(document).ready(function() { }); }); +function h(string) { + return jQuery('

    ').text(string).html(); +} + function createDeletionConfirmationText(data) { return `Do you wish to delete the pact for version ${ - data.consumerVersionNumber - } of ${data.consumerName}?`; + h(data.consumerVersionNumber) + } of ${h(data.consumerName)}?`; } function confirmDeleteResource( diff --git a/script/seed.rb b/script/seed.rb index ed4e0c288..6ad1fd139 100755 --- a/script/seed.rb +++ b/script/seed.rb @@ -58,13 +58,11 @@ url: "http://localhost:9292/verification-published-webhook", body: webhook_body.to_json) .set_now(Date.today - 101) - .tap{ | it | - 2.times do | i | - it.create_pact_with_verification("Foo", i.to_s, "Bar", i.to_s) - .create_pact_with_verification("Bar", i.to_s, "Foo", i.to_s) - .add_day - end - }.create_pact_with_hierarchy("Foo", "100", "Bar") + .create_pact_with_hierarchy("Foo/Foo", "100", "Bar/Bar") + .create_pact_with_hierarchy("Foo", "1", "Bar") + .create_pact_with_hierarchy("", "", "Bar/Bar") + .create_consumer_version_tag("prod") + .create_verification(provider_version: "1", tag_names: "prod") # .create_certificate(path: 'spec/fixtures/certificates/self-signed.badssl.com.pem') diff --git a/spec/lib/pact_broker/api/renderers/html_pact_renderer_spec.rb b/spec/lib/pact_broker/api/renderers/html_pact_renderer_spec.rb index 8895aa324..65120a4f5 100644 --- a/spec/lib/pact_broker/api/renderers/html_pact_renderer_spec.rb +++ b/spec/lib/pact_broker/api/renderers/html_pact_renderer_spec.rb @@ -23,22 +23,26 @@ module Renderers Timecop.return end - let(:consumer) { double('consumer', name: 'Consumer')} - let(:provider) { double('provider', name: 'Provider')} + let(:consumer_name) { 'Consumer' } + let(:provider_name) { 'Provider' } + let(:consumer_version_number) { '1.2.3' } + let(:consumer) { double('consumer', name: consumer_name) } + let(:provider) { double('provider', name: provider_name) } let(:consumer_version) { double('consumer version') } let(:created_at) { DateTime.new(2014, 02, 27) } let(:json_content) { load_fixture('renderer_pact.json') } let(:pact) do double('pact', json_content: json_content, - consumer_version_number: '1.2.3', + consumer_version_number: consumer_version_number, consumer: consumer, provider: provider, - consumer_version_tag_names: ['prod', 'master'], + consumer_version_tag_names: consumer_version_tag_names, created_at: created_at, consumer_version: consumer_version ) end + let(:consumer_version_tag_names) { ['prod', 'master'] } let(:pact_url) { '/pact/url' } let(:matrix_url) { '/matrix/url' } let(:options) do @@ -49,7 +53,7 @@ module Renderers end let(:logger) { double('logger').as_null_object } - subject { HtmlPactRenderer.call pact, options } + subject { HtmlPactRenderer.call(pact, options) } describe ".call" do it "renders the pact as HTML" do @@ -69,7 +73,7 @@ module Renderers end it "renders the badge image" do - expect(subject).to include "" + expect(subject).to include "" end it "renders a text area with the badge markdown" do @@ -81,6 +85,20 @@ module Renderers expect(subject).to include matrix_url end + context "with dodgey data" do + let(:consumer_name) { '' } + let(:provider_name) { '' } + let(:consumer_version_number) { '' } + let(:consumer_version_tag_names) { [''] } + + it "does not contain the literal / Pact Status](http://badge)]' + end + end + context "when enable_public_badge_access is false" do before do PactBroker.configuration.enable_public_badge_access = false diff --git a/spec/lib/pact_broker/badges/service_spec.rb b/spec/lib/pact_broker/badges/service_spec.rb index cb13befb8..fff9016d7 100644 --- a/spec/lib/pact_broker/badges/service_spec.rb +++ b/spec/lib/pact_broker/badges/service_spec.rb @@ -13,7 +13,7 @@ module Badges let(:expected_url) { "https://img.shields.io/badge/#{expected_left_text}-#{expected_right_text}-#{expected_color}.svg" } let(:expected_color) { "brightgreen" } let(:expected_right_text) { "verified" } - let(:expected_left_text) { "foo--bar%2fthing__blah%20pact" } + let(:expected_left_text) { "foo--bar%2Fthing__blah%20pact" } let(:response_status) { 200 } let!(:http_request) do stub_request(:get, expected_url).to_return(:status => response_status, :body => "svg") @@ -62,7 +62,7 @@ module Badges end context "when initials is true" do - let(:expected_left_text) { "fb%2ftb%20pact" } + let(:expected_left_text) { "fb%2Ftb%20pact" } let(:initials) { true } it "creates a badge with the consumer and provider initials" do @@ -73,7 +73,7 @@ module Badges end context "when initials is true but the consumer and provider names only contain one word" do - let(:expected_left_text) { "foo%2fbar%20pact" } + let(:expected_left_text) { "foo%2Fbar%20pact" } let(:initials) { true } let(:pact) { double("pact", consumer_name: "Foo", provider_name: "Bar") } @@ -85,7 +85,7 @@ module Badges end context "when initials is true but the consumer and provider names are one camel cased word" do - let(:expected_left_text) { "fa%2fbp%20pact" } + let(:expected_left_text) { "fa%2Fbp%20pact" } let(:initials) { true } let(:pact) { double("pact", consumer_name: "FooApp", provider_name: "barProvider") } @@ -97,7 +97,7 @@ module Badges end context "when initials is true but the consumer and provider names are one camel cased word" do - let(:expected_left_text) { "fa%2fdat%20pact" } + let(:expected_left_text) { "fa%2Fdat%20pact" } let(:initials) { true } let(:pact) { double("pact", consumer_name: "FooApp", provider_name: "doAThing") } @@ -111,7 +111,7 @@ module Badges context "when the tags are supplied" do let(:tags) { { consumer_tag: "prod", provider_tag: "master" } } - let(:expected_left_text) { "foo--bar%20(prod)%2fthing__blah%20(master)%20pact" } + let(:expected_left_text) { "foo--bar%20%28prod%29%2Fthing__blah%20%28master%29%20pact" } it "creates a badge with the consumer and provider names, not initials" do subject