Merge pull request #239 from zendesk/keith/rewrite_validation_for_requests

[MPORT-502] Rewrite Requests validation class

Author: keitheous

lib/zendesk_apps_support/validations/requests.rb

@@ -1,57 +1,69 @@
 # frozen_string_literal: true
 
-require 'uri'
 require 'ipaddress_2'
+require 'uri'
 
 module ZendeskAppsSupport
   module Validations
     module Requests
       class << self
-        HTTP_REQUEST_METHODS = %w[GET HEAD POST PUT DELETE CONNECT OPTIONS TRACE].freeze
-
-        REQUEST_CALLS = [
-          /\w+\.request\(['"](.*)['"]/i, # ZAF request
-          /(?:\$|jQuery)\.(?:ajax|get|post|getJSON)\(['"](.*)['"]/i, # jQuery request
-          /\w+\.open\(['"](?:#{Regexp.union(HTTP_REQUEST_METHODS).source})['"],\s?['"](.*)['"]/i, # XMLHttpRequest
-          /fetch\(['"](.*)['"]/i # fetch
-        ].freeze
+        IP_ADDRESS = /\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/
 
         def call(package)
           errors = []
           files = package.js_files + package.html_files
 
           files.each do |file|
-            contents = file.read
-            REQUEST_CALLS.each do |request_pattern|
-              request = contents.match(request_pattern)
-              next unless request
-              uri = URI(request.captures[0])
-              if uri.scheme == 'http'
-                package.warnings << I18n.t('txt.apps.admin.warning.app_build.insecure_http_request',
-                                           uri: uri,
-                                           file: file.relative_path)
-              end
-
-              next unless IPAddress.valid? uri.host
-              ip = IPAddress.parse uri.host
-
-              blocked_ip_type = if ip.private?
-                                  I18n.t('txt.apps.admin.error.app_build.blocked_request_private')
-                                elsif ip.loopback?
-                                  I18n.t('txt.apps.admin.error.app_build.blocked_request_loopback')
-                                elsif ip.link_local?
-                                  I18n.t('txt.apps.admin.error.app_build.blocked_request_link_local')
-                                end
-
-              next unless blocked_ip_type
-              errors << I18n.t('txt.apps.admin.error.app_build.blocked_request',
-                               type: blocked_ip_type,
-                               uri: uri.host,
-                               file: file.relative_path)
+            file_content = file.read
+
+            http_protocol_urls = find_address_containing_http(file_content)
+            if http_protocol_urls.any?
+              package.warnings << I18n.t(
+                'txt.apps.admin.warning.app_build.insecure_http_request',
+                uri: http_protocol_urls.join(I18n.t('txt.apps.admin.error.app_build.listing_comma')),
+                file: file.relative_path
+              )
+            end
+
+            ip_addresses = file_content.scan(IP_ADDRESS)
+            if ip_addresses.any?
+              errors << blocked_ips_validation(file.relative_path, ip_addresses)
             end
           end
+
           errors
         end
+
+        private
+
+        def blocked_ips_validation(file_path, ip_addresses)
+          ip_addresses.each_with_object([]) do |ip_address, error_messages|
+            blocked_type = blocked_ip_type(ip_address)
+            next unless blocked_type
+
+            error_messages << I18n.t(
+              'txt.apps.admin.error.app_build.blocked_request',
+              type: blocked_type,
+              uri:  ip_address,
+              file: file_path
+            )
+          end
+        end
+
+        def blocked_ip_type(ip_address)
+          block_type =
+            case IPAddress.parse(ip_address)
+            when proc(&:private?) then 'private'
+            when proc(&:loopback?) then 'loopback'
+            when proc(&:link_local?) then 'link_local'
+            end
+
+          block_type && I18n.t("txt.apps.admin.error.app_build.blocked_request_#{block_type}")
+        end
+
+        def find_address_containing_http(file_content)
+          file_content.scan(URI.regexp(['http'])).map(&:compact).map(&:last)
+        end
       end
     end
   end

spec/validations/mime_spec.rb

@@ -3,7 +3,6 @@
 require 'spec_helper'
 
 describe ZendeskAppsSupport::Validations::Mime do
-  let(:subject) { ZendeskAppsSupport::Validations::Mime }
   let(:package) { double('Package', files: []) }
 
   def add_to_package(pathname)

spec/validations/requests_spec.rb

@@ -2,75 +2,52 @@
 
 require 'spec_helper'
 
-def request_function(style, address)
-  case style
-  when 'ZAF'
-    "function request() { client.request('#{address}') }"
-  when 'jQuery'
-    "function request() { jQuery.get('#{address}') }"
-  when 'jQuery$'
-    "function request() { $.ajax('#{address}') }"
-  when 'XMLHttpRequest'
-    "function request() { xhr.open('get', '#{address}') }"
-  when 'fetch'
-    "function request() { fetch('#{address}') }"
-  end
-end
-
-request_function_styles = %w[ZAF jQuery jQuery$ XMLHttpRequest fetch]
+describe ZendeskAppsSupport::Validations::Requests do
+  let(:package) { double('Package', warnings: [], html_files: []) }
+  let(:app_file) { double('AppFile', relative_path: 'app_file.js') }
 
-shared_examples 'an insecure request' do |file_path, function_style|
-  address = 'http://foo.com'
-  let(:markup) { request_function(function_style, address) }
+  before { allow(package).to receive(:js_files) { [app_file] } }
 
-  it "and raise a warning inside #{function_style} style requests" do
-    errors = subject.call(package)
-    expect(package.warnings[0]).to include('insecure HTTP request', address, file_path)
-    expect(errors).to be_empty
-  end
-end
+  context 'http protocols check' do
+    it 'returns no warnings for files that contain https urls' do
+      allow(app_file).to receive(:read) { "client.instance(\"https://foo-bar.com\");\r\n\t" }
 
-blocked_ips = {
-  private: {
-    range: '10.0.0.0/8, 172.16.0.0/12, or 192.168.0.0/16',
-    example: '192.168.0.1'
-  },
-  loopback: {
-    range: '127.0.0.0/8',
-    example: '127.0.0.1'
-  },
-  link_local: {
-    range: '169.254.0.0/16',
-    example: '169.254.0.1'
-  }
-}
+      subject.call(package)
+      expect(package.warnings).to be_empty
+    end
 
-shared_examples 'a blocked ip' do |file_path, function_style, ip_type, ip|
-  let(:markup) { request_function(function_style, "https://#{ip}") }
+    it 'returns warning with request information when files contain http url' do
+      allow(app_file).to receive(:read) { "client.instance(\"http://foo-bar.com\");\r\n\t" }
 
-  it "and throw a #{ip_type} ip error inside #{function_style} style request calls" do
-    errors = subject.call(package)
-    expect(package.warnings).to be_empty
-    expect(errors[0]).to include("request to a #{ip_type} ip", ip, file_path)
+      subject.call(package)
+      expect(package.warnings[0]).to include(
+        'Possible insecure HTTP request',
+        'foo-bar.com',
+        'in app_file.js',
+        'Consider using the HTTPS protocol instead.'
+      )
+    end
   end
-end
 
-describe ZendeskAppsSupport::Validations::Requests do
-  app_js_path = 'assets/app.js'
-  let(:app_js) { double('AppFile', relative_path: app_js_path, read: markup) }
-  let(:subject) { ZendeskAppsSupport::Validations::Requests }
-  let(:package) { double('Package', js_files: [app_js], html_files: [], warnings: []) }
+  context 'IPs check' do
+    it 'returns no validation error when scanning regular IP' do
+      allow(app_file).to receive(:read) { "client.instance(\"64.233.191.255\");\r\n\t" }
+      expect(subject.call(package).flatten).to be_empty
+    end
 
-  describe 'using the http protocol' do
-    request_function_styles.each { |function_style| it_behaves_like 'an insecure request', app_js_path, function_style }
-  end
+    it 'returns a validation error when scanning private IP' do
+      allow(app_file).to receive(:read) { "//var x = '192.168.0.1'\r\n \tclient.get(x)" }
+      expect(subject.call(package).flatten[0]).to include('request to a private ip 192.168.0.1')
+    end
+
+    it 'returns a validation error when scanning loopback IP' do
+      allow(app_file).to receive(:read) { "//var x = '127.0.0.1'\r\n \tclient.get(x)" }
+      expect(subject.call(package).flatten[0]).to include('request to a loopback ip 127.0.0.1')
+    end
 
-  blocked_ips.each do |type, ip|
-    describe "to #{ip[:range]} range ips" do
-      request_function_styles.each do |function_style|
-        type_localised = ZendeskAppsSupport::I18n.t("txt.apps.admin.error.app_build.blocked_request_#{type}")
-        it_behaves_like 'a blocked ip', app_js_path, function_style, type_localised, ip[:example]
-      end
+    it 'returns a validation error when scanning link_local IP' do
+      allow(app_file).to receive(:read) { "//var x = '169.254.0.1'\r\n \tclient.get(x)" }
+      expect(subject.call(package).flatten[0]).to include('request to a link-local ip 169.254.0.1')
     end
   end
 end