From d1fb4a2d5c30597c02ceffca71f98c030a83ede6 Mon Sep 17 00:00:00 2001 From: FC Stegerman Date: Fri, 16 Dec 2022 16:10:23 +0100 Subject: [PATCH 1/2] rework HTML escaping --- lib/fdroid/Package.rb | 129 ++++++++++++---------------- lib/fdroid/Repo.rb | 21 +++-- spec/lib/fdroid/FDroidIndex_spec.rb | 18 +++- 3 files changed, 88 insertions(+), 80 deletions(-) diff --git a/lib/fdroid/Package.rb b/lib/fdroid/Package.rb index 13a7034..b998d6c 100644 --- a/lib/fdroid/Package.rb +++ b/lib/fdroid/Package.rb @@ -77,7 +77,7 @@ module FDroid end def package_name - field 'packageName' + @package['packageName'] end def to_s @@ -88,53 +88,31 @@ module FDroid localized = Package.localized_graphic_path(@available_locales, @package['localized'], 'icon') if localized "#{package_name}/#{localized}" - elsif field('icon') - "icons-640/#{field('icon')}" + elsif @package['icon'] + "icons-640/#{@package['icon']}" end end # this must exist since all entries are sorted by name, # it uses tildes since they sort last def name - n = field('name') || Package.localized(@available_locales, @package['localized'], 'name') || '~missing name~' - - if n != nil - n = Loofah.scrub_fragment(n, :strip).to_text() - end - - return n + @package['name'] || Package.localized(@available_locales, @package['localized'], 'name') || '~missing name~' end def summary - s = field('summary') || Package.localized(@available_locales, @package['localized'], 'summary') - - if s != nil - s = Loofah.scrub_fragment(s, :strip).to_text() - end - - return s + @package['summary'] || Package.localized(@available_locales, @package['localized'], 'summary') end def description - desc = field('description') || Package.localized(@available_locales, @package['localized'], 'description') - - if desc != nil - desc = Package.process_package_description(desc) - end - - return desc + @package['description'] || Package.localized(@available_locales, @package['localized'], 'description') end def suggested_version_code - code = field('suggestedVersionCode') - if code != nil - code = Integer(code) - end - return code + Integer(@package['suggestedVersionCode']) rescue nil end def categories - return field('categories') + @package['categories'] end # Generates a hash of dumb strings to be used in templates. @@ -144,69 +122,67 @@ module FDroid # The 'versions' key is an array of Version.to_data hashes. # @return [Hash] def to_data - liberapay = field('liberapay') + liberapay = @package['liberapay'] if liberapay == nil - liberapayID = field('liberapayID') + liberapayID = @package['liberapayID'] if liberapayID != nil liberapay = "~#{liberapayID}" end end - { + data = { # These fields are taken as is from the metadata. If not present, they are 'package_name' => package_name, - 'author_email' => field('authorEmail'), - 'author_name' => field('authorName'), - 'author_website' => field('authorWebSite'), - 'translation' => field('translation'), - 'bitcoin' => field('bitcoin'), - 'litecoin' => field('litecoin'), - 'donate' => field('donate'), - 'flattrID' => field('flattrID'), + 'author_email' => @package['authorEmail'], + 'author_name' => @package['authorName'], + 'author_website' => @package['authorWebSite'], + 'translation' => @package['translation'], + 'bitcoin' => @package['bitcoin'], + 'litecoin' => @package['litecoin'], + 'donate' => @package['donate'], + 'flattrID' => @package['flattrID'], 'liberapay' => liberapay, - 'liberapayID' => field('liberapayID'), - 'openCollective' => field('openCollective'), - 'categories' => field('categories'), - 'anti_features' => field('antiFeatures'), + 'liberapayID' => @package['liberapayID'], + 'openCollective' => @package['openCollective'], + 'categories' => @package['categories'], + 'anti_features' => @package['antiFeatures'], 'suggested_version_code' => suggested_version_code, 'suggested_version_name' => @versions.detect { |p| p.version_code == suggested_version_code }&.version_name, - 'issue_tracker' => field('issueTracker'), - 'changelog' => field('changelog'), - 'license' => field('license'), - 'source_code' => field('sourceCode'), - 'website' => field('webSite'), - 'added' => field('added'), - 'last_updated' => field('lastUpdated'), + 'issue_tracker' => @package['issueTracker'], + 'changelog' => @package['changelog'], + 'license' => @package['license'], + 'source_code' => @package['sourceCode'], + 'website' => @package['webSite'], + 'added' => @package['added'], + 'last_updated' => @package['lastUpdated'], 'is_localized' => @is_localized, 'whats_new' => Package.process_package_description(Package.localized(@available_locales, @package['localized'], 'whatsNew')), - 'icon' => icon, 'title' => name, 'summary' => summary, - - 'description' => description, + 'description' => Package.process_package_description(description), 'feature_graphic' => Package.localized_graphic_path(@available_locales, @package['localized'], 'featureGraphic'), 'phone_screenshots' => Package.localized_graphic_list_paths(@available_locales, @package['localized'], 'phoneScreenshots'), 'seven_inch_screenshots' => Package.localized_graphic_list_paths(@available_locales, @package['localized'], 'sevenInchScreenshots'), 'ten_inch_screenshots' => Package.localized_graphic_list_paths(@available_locales, @package['localized'], 'tenInchScreenshots'), 'tv_screenshots' => Package.localized_graphic_list_paths(@available_locales, @package['localized'], 'tvScreenshots'), 'wear_screenshots' => Package.localized_graphic_list_paths(@available_locales, @package['localized'], 'wearScreenshots'), - 'versions' => @versions.sort.reverse.map { |p| p.to_data }, - 'beautiful_url' => "/packages/#{package_name}" } + + # recursively sanitise data before returning, except for description and + # whats_new, which have already passed through process_package_description + # (and were thus scrubbed by loofah via format_description_to_html) + return Package.sanitise(data, skip = ['description', 'whats_new']) end # Any transformations which are required to turn the "description" into something which is # displayable via HTML is done here (e.g. replacing "fdroid.app:" schemes, formatting new lines, # etc. def self.process_package_description(string) - if string == nil - return nil - end + return nil if string == nil - string = self.replace_fdroid_app_links(string) - self.format_description_to_html(string) + format_description_to_html(replace_fdroid_app_links(string)) end # Finds all https://f-droid.org links that end with an Application ID, and @@ -361,17 +337,26 @@ module FDroid private - def field(name) - if @package.key?(name) - value = @package[name] - case value - when Float then return value - when Integer then return value - when Array then return value.map { |i| Loofah.scrub_fragment(i, :strip).to_html(:save_with => 0) } - else - return Loofah.scrub_fragment(value, :strip).to_html(:save_with => 0) - end + # used to recursively sanitise the hash returned by to_data, except for any + # data already passed through process_package_description (and thus scrubbed + # by loofah) + def self.sanitise(value, skip = []) + case value + when String + value.gsub(/[<>"'&]/, ESCAPES) + when Hash + value.map { |k, v| skip.include?(k) ? [k, v] : [k, sanitise(v)] }.to_h + when Array + value.map { |x| sanitise(x) } + when Date, Float, Integer, nil + value + else + raise "cannot sanitise #{value.inspect}" end end + + ESCAPES = { + '<' => '<', '>' => '>', '"' => '"', "'" => ''', '&' => '&' + } end end diff --git a/lib/fdroid/Repo.rb b/lib/fdroid/Repo.rb index ebacabb..4065789 100644 --- a/lib/fdroid/Repo.rb +++ b/lib/fdroid/Repo.rb @@ -15,7 +15,6 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -require 'loofah' require 'uri' module FDroid @@ -25,21 +24,21 @@ module FDroid end def name - Loofah.scrub_fragment(@repo['name'], :strip).to_s + escape_html @repo['name'] end def address url = @repo['address'] - url =~ /\A#{URI::regexp}\z/ ? url : nil + url =~ /\A#{URI::regexp}\z/ ? escape_html(url) : nil end def icon_url url = "#{self.address}/icons/#{@repo['icon']}" - url =~ /\A#{URI::regexp}\z/ ? url : nil + url =~ /\A#{URI::regexp}\z/ ? escape_html(url) : nil end def description - Loofah.scrub_fragment(@repo['description'], :strip).to_s + escape_html @repo['description'] end def timestamp @@ -47,7 +46,17 @@ module FDroid end def date - added = Date.strptime("#{@repo['timestamp'] / 1000}", '%s') + Date.strptime("#{@repo['timestamp'] / 1000}", '%s') end + + private + + def escape_html(value) + value.gsub(/[<>"'&]/, ESCAPES) + end + + ESCAPES = { + '<' => '<', '>' => '>', '"' => '"', "'" => ''', '&' => '&' + } end end diff --git a/spec/lib/fdroid/FDroidIndex_spec.rb b/spec/lib/fdroid/FDroidIndex_spec.rb index f6dac36..0987ce0 100644 --- a/spec/lib/fdroid/FDroidIndex_spec.rb +++ b/spec/lib/fdroid/FDroidIndex_spec.rb @@ -155,6 +155,20 @@ here" scrubbed = Package.process_package_description(input) expect(scrubbed).to eql(output) end + + it 'sanitises' do + input = { + 'description' => 'bold/b>', + 'summary' => 'bold', + 'foo' => [''], + } + output = { + 'description' => 'bold/b>', + 'summary' => '<b>bold</b>', + 'foo' => ['<oops>'], + } + expect(Package.sanitise(input, skip = ['description'])).to eql(output) + end end RSpec.describe Permission do @@ -229,8 +243,8 @@ here" it 'Loofah runs on all text fields that can be rendered with HTML' do loofah_test = parse_loofah_test_from_gp().to_data expect(loofah_test['description']).to eq("This is just a test that alert('pwned!') loofah is stripping.") - expect(loofah_test['summary']).to eq("트리거 불안에 때 개인 정보를 보호하거나 상황을 패닉 앱alert('pwned!')") - expect(loofah_test['title']).to eq("alert('PWN!')") + expect(loofah_test['summary']).to eq("트리거 불안에 때 개인 정보를 보호하거나 상황을 패닉 앱<script>alert('pwned!')</script>") + expect(loofah_test['title']).to eq("<script>alert('PWN!')</script>") expect(loofah_test['whats_new']).to eq("Feature:
* Add support for packs (@Rudloff)
alert('pwned!')
Minor:
* Change name to Launcher
") end -- GitLab From 7a649b4e5f6322362479f37ef4b111d369c891d4 Mon Sep 17 00:00:00 2001 From: FC Stegerman Date: Mon, 19 Dec 2022 20:43:07 +0100 Subject: [PATCH 2/2] some more small improvements --- lib/fdroid/Package.rb | 60 +++++++++++----------- lib/fdroid/Version.rb | 6 --- lib/jekyll/FDroidPackageDetailGenerator.rb | 4 +- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/lib/fdroid/Package.rb b/lib/fdroid/Package.rb index b998d6c..9d1680c 100644 --- a/lib/fdroid/Package.rb +++ b/lib/fdroid/Package.rb @@ -76,6 +76,7 @@ module FDroid @is_localized = Package.is_localized(locale, @available_locales) end + # NB: safe (has strict checks on it and was the subject of a previous audit) def package_name @package['packageName'] end @@ -84,33 +85,7 @@ module FDroid package_name end - def icon - localized = Package.localized_graphic_path(@available_locales, @package['localized'], 'icon') - if localized - "#{package_name}/#{localized}" - elsif @package['icon'] - "icons-640/#{@package['icon']}" - end - end - - # this must exist since all entries are sorted by name, - # it uses tildes since they sort last - def name - @package['name'] || Package.localized(@available_locales, @package['localized'], 'name') || '~missing name~' - end - - def summary - @package['summary'] || Package.localized(@available_locales, @package['localized'], 'summary') - end - - def description - @package['description'] || Package.localized(@available_locales, @package['localized'], 'description') - end - - def suggested_version_code - Integer(@package['suggestedVersionCode']) rescue nil - end - + # NB: safe (can contain '&' but must be in site.config["app_categories"]) def categories @package['categories'] end @@ -335,8 +310,6 @@ module FDroid end end - private - # used to recursively sanitise the hash returned by to_data, except for any # data already passed through process_package_description (and thus scrubbed # by loofah) @@ -358,5 +331,34 @@ module FDroid ESCAPES = { '<' => '<', '>' => '>', '"' => '"', "'" => ''', '&' => '&' } + + private + + def icon + localized = Package.localized_graphic_path(@available_locales, @package['localized'], 'icon') + if localized + "#{package_name}/#{localized}" + elsif @package['icon'] + "icons-640/#{@package['icon']}" + end + end + + # this must exist since all entries are sorted by name, + # it uses tildes since they sort last + def name + @package['name'] || Package.localized(@available_locales, @package['localized'], 'name') || '~missing name~' + end + + def summary + @package['summary'] || Package.localized(@available_locales, @package['localized'], 'summary') + end + + def description + @package['description'] || Package.localized(@available_locales, @package['localized'], 'description') + end + + def suggested_version_code + Integer(@package['suggestedVersionCode']) rescue nil + end end end diff --git a/lib/fdroid/Version.rb b/lib/fdroid/Version.rb index 0119e36..216922e 100644 --- a/lib/fdroid/Version.rb +++ b/lib/fdroid/Version.rb @@ -69,11 +69,5 @@ module FDroid @version['uses-permission'].map { |perm| Permission.new(perm).to_data } end end - - private - - def field(name) - @package.key?(name) ? name : nil - end end end diff --git a/lib/jekyll/FDroidPackageDetailGenerator.rb b/lib/jekyll/FDroidPackageDetailGenerator.rb index afd192b..26358b1 100644 --- a/lib/jekyll/FDroidPackageDetailGenerator.rb +++ b/lib/jekyll/FDroidPackageDetailGenerator.rb @@ -64,9 +64,7 @@ module Jekyll site.collections["packages"].docs << FDroidPackageDetailPage.new(site, site.source, package) package.categories.each do |app_category| - app_category_id = app_category.dup - app_category_id.sub!('&', '&') - app_category_id = Utils.slugify(app_category_id) + app_category_id = Utils.slugify(app_category) if site.collections[app_category_id].nil? puts("Warning: Package '#{package.package_name}' has unknown category '#{app_category}', will be ignored") else -- GitLab