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