From 0340774523d7b11bd54c59780e1504d12d697f3d Mon Sep 17 00:00:00 2001

From: Adam Williamson <awilliam@redhat.com>

Date: Wed, 18 Nov 2020 10:02:08 -0800

Subject: [PATCH 4/4] parse_url_settings: don't parse value for non-asset types

If a test sets NOT_ASSET_URL: http://some/parseable.url , we don't

actually want to parse that to NOT_ASSET: parseable.url , because

we aren't going to download it later. We had this bug back in 2016

and fixed it specifically in f54a18f , then regressed it again in

391f95e3 because we needed to pass the parsed file name to the

asset type check.

When we split up `parse_url_settings` and `create_downloads_list`

the asset type check wound up in `create_downloads_list`. To fix

the bug we can't avoid duplicating it here - we do have to check

it in both places, here to avoid setting $settings->{$short} if

it's not an asset type, and there to avoid doing the download if

somehow both NOT_ASSET_URL and NOT_ASSET wind up being set by

templates or the scheduler. It's not worth splitting the check to

a shared utility function as you still need flow control. I don't

think the log message will ever be duplicated as we should never

manage to reach and fail the check *both* times (only one or the

other).

Signed-off-by: Adam Williamson <awilliam@redhat.com>

---

 lib/OpenQA/JobSettings.pm | 13 +++++++++----

 t/api/02-iso-download.t   |  1 +

 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/OpenQA/JobSettings.pm b/lib/OpenQA/JobSettings.pm

index c3c7d97c9..335c8d1ef 100644

--- a/lib/OpenQA/JobSettings.pm

+++ b/lib/OpenQA/JobSettings.pm

@@ -21,7 +21,7 @@ use File::Basename;

 use Mojo::URL;

 use Mojo::Util 'url_unescape';

 use OpenQA::Log 'log_debug';

-use OpenQA::Utils 'get_url_short';

+use OpenQA::Utils qw(asset_type_from_setting get_url_short);

 sub generate_settings {

     my ($params) = @_;

@@ -113,9 +113,9 @@ sub handle_plus_in_settings {

 # Given a hashref of settings, parse any whose names end in _URL

 # to the short name, then if there is not already a setting with

-# the short name, set it to the filename from the URL (with the

-# compression extension removed in the case of _DECOMPRESS_URL).

-# This has to happen *before* generate_jobs

+# the short name and the setting is an asset type, set it to the

+# filename from the URL (with the compression extension removed

+# in the case of _DECOMPRESS_URL).

 sub parse_url_settings {

     my ($settings) = @_;

     for my $setting (keys %$settings) {

@@ -137,6 +137,11 @@ sub parse_url_settings {

             log_debug("Unable to get filename from $url. Ignoring $setting");

             next;

         }

+        # We shouldn't set the short setting for non-asset types

+        unless (asset_type_from_setting($short, $filename)) {

+            log_debug("_URL downloading only allowed for asset types! $short is not an asset type");

+            next;

+        }

         $settings->{$short} = $filename;

     }

     return undef;

diff --git a/t/api/02-iso-download.t b/t/api/02-iso-download.t

index 914d30552..a718ef154 100644

--- a/t/api/02-iso-download.t

+++ b/t/api/02-iso-download.t

@@ -158,6 +158,7 @@ check_job_setting($t, $rsp, 'KERNEL', 'callitvmlinuz',

 $rsp = schedule_iso({%params, NO_ASSET_URL => 'http://localhost/nonexistent.iso'});

 is($rsp->json->{count}, $expected_job_count, 'a regular ISO post creates the expected number of jobs');

 check_download_asset('non-asset _URL');

+check_job_setting($t, $rsp, 'NO_ASSET', undef, 'NO_ASSET is not parsed from NO_ASSET_URL');

 # Using asset _URL but without filename extractable from URL create warning in log file, jobs, but no gru job

 $rsp = schedule_iso({%iso, ISO_URL => 'http://localhost'});

-- 

2.29.2