From fee60fdcc32ceadfba92736b518dd223de3f18b6 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Mon, 16 Nov 2020 15:09:28 -0800
Subject: [PATCH 1/4] Handle placeholders after parsing _URL settings

b3bc8ebf caused some problems by moving the parsing of `_URL`
settings to after the call to `JobSettings::generate_settings`,
which is where handling of placeholders (%FOO%), usually from the
templates, is done. This broke the case where e.g. a template
uses `%ISO%` in a value, and the job is posted with `ISO_URL` but
no explicit `ISO` set. In that case the derived `ISO` setting
should be used in the placeholder expansion, but this was broken.

This fixes the problems by splitting up the "parse _URL settings"
and "create download list" functions of `create_downloads_list`,
and moving the parsing part into `generate_settings`, just before
placeholder expansion happens. This is after we pull in settings
from the templates, so the bug b3bc8ebf was trying to fix (POO
62159) should still be fixed. Download list creation should still
happen the same as currently, so POO #70687 should still be fixed.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 lib/OpenQA/JobSettings.pm | 39 ++++++++++++++++++++++++
 lib/OpenQA/Utils.pm       | 63 +++++++++++++++++----------------------
 t/api/02-iso-download.t   | 13 ++++++++
 3 files changed, 79 insertions(+), 36 deletions(-)

diff --git a/lib/OpenQA/JobSettings.pm b/lib/OpenQA/JobSettings.pm
index 0d6c11811..b0e364c8c 100644
--- a/lib/OpenQA/JobSettings.pm
+++ b/lib/OpenQA/JobSettings.pm
@@ -17,6 +17,12 @@ package OpenQA::JobSettings;
 use strict;
 use warnings;
 
+use File::Basename;
+use Mojo::URL;
+use Mojo::Util 'url_unescape';
+use OpenQA::Log 'log_debug';
+use OpenQA::Utils 'get_url_short';
+
 sub generate_settings {
     my ($params) = @_;
     my $settings = $params->{settings};
@@ -53,6 +59,7 @@ sub generate_settings {
         $settings->{JOB_DESCRIPTION} = $job_template->description if length $job_template->description;
     }
 
+    parse_url_settings($settings);
     handle_plus_in_settings($settings);
     return expand_placeholders($settings);
 }
@@ -104,4 +111,36 @@ 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
+sub parse_url_settings {
+    my ($settings) = @_;
+    for my $setting (keys %$settings) {
+        my ($short, $do_extract) = get_url_short($setting);
+        next unless ($short);
+        next if ($settings->{$short});
+        # As this comes in from an API call, URL will be URI-encoded
+        # This obviously creates a vuln if untrusted users can POST
+        $settings->{$setting} = url_unescape($settings->{$setting});
+        my $url = $settings->{$setting};
+        my $filename;
+        $filename = Mojo::URL->new($url)->path->parts->[-1];
+        if ($do_extract) {
+            # if user wants to extract downloaded file, final filename
+            # will have last extension removed
+            $filename = fileparse($filename, qr/\.[^.]*/);
+        }
+        $settings->{$short} = $filename;
+        if (!$settings->{$short}) {
+            log_debug("Unable to get filename from $url. Ignoring $setting");
+            delete $settings->{$short} unless $settings->{$short};
+            next;
+        }
+    }
+    return undef;
+}
+
 1;
diff --git a/lib/OpenQA/Utils.pm b/lib/OpenQA/Utils.pm
index 4db65eaeb..6dc3d0880 100644
--- a/lib/OpenQA/Utils.pm
+++ b/lib/OpenQA/Utils.pm
@@ -21,7 +21,6 @@ use Carp;
 use Cwd 'abs_path';
 use IPC::Run();
 use Mojo::URL;
-use Mojo::Util 'url_unescape';
 use Regexp::Common 'URI';
 use Try::Tiny;
 use Mojo::File 'path';
@@ -62,6 +61,7 @@ our @EXPORT  = qw(
   asset_type_from_setting
   check_download_url
   check_download_passlist
+  get_url_short
   create_downloads_list
   human_readable_size
   locate_asset
@@ -499,45 +499,36 @@ sub check_download_passlist {
     return ();
 }
 
+sub get_url_short {
+    # Given a setting name, if it ends with _URL or _DECOMPRESS_URL
+    # return the name with that string stripped, and a flag indicating
+    # whether decompression will be needed. If it doesn't, returns
+    # empty string and 0.
+    my ($arg) = @_;
+    return ('', 0) unless ($arg =~ /_URL$/);
+    my $short;
+    my $do_extract = 0;
+    if ($arg =~ /_DECOMPRESS_URL$/) {
+        $short      = substr($arg, 0, -15);
+        $do_extract = 1;
+    }
+    else {
+        $short = substr($arg, 0, -4);
+    }
+    return ($short, $do_extract);
+}
+
 sub create_downloads_list {
     my ($args) = @_;
     my %downloads = ();
     for my $arg (keys %$args) {
-        next unless ($arg =~ /_URL$/);
-        # As this comes in from an API call, URL will be URI-encoded
-        # This obviously creates a vuln if untrusted users can POST
-        $args->{$arg} = url_unescape($args->{$arg});
-        my $url        = $args->{$arg};
-        my $do_extract = 0;
-        my $short;
-        my $filename;
-        # if $args{FOO_URL} or $args{FOO_DECOMPRESS_URL} is set but $args{FOO}
-        # is not, we will set $args{FOO} (the filename of the downloaded asset)
-        # to the URL filename. This has to happen *before*
-        # generate_jobs so the jobs have FOO set
-        if ($arg =~ /_DECOMPRESS_URL$/) {
-            $do_extract = 1;
-            $short      = substr($arg, 0, -15);    # remove whole _DECOMPRESS_URL substring
-        }
-        else {
-            $short = substr($arg, 0, -4);          # remove _URL substring
-        }
-        if (!$args->{$short}) {
-            $filename = Mojo::URL->new($url)->path->parts->[-1];
-            if ($do_extract) {
-                # if user wants to extract downloaded file, final filename
-                # will have last extension removed
-                $filename = fileparse($filename, qr/\.[^.]*/);
-            }
-            $args->{$short} = $filename;
-            if (!$args->{$short}) {
-                log_debug("Unable to get filename from $url. Ignoring $arg");
-                delete $args->{$short} unless $args->{$short};
-                next;
-            }
-        }
-        else {
-            $filename = $args->{$short};
+        my $url = $args->{$arg};
+        my ($short, $do_extract) = get_url_short($arg);
+        next unless ($short);
+        my $filename = $args->{$short};
+        unless ($filename) {
+            log_debug("No target filename set for $url. Ignoring $arg");
+            next;
         }
         # We're only going to allow downloading of asset types. We also
         # need this to determine the download location later
diff --git a/t/api/02-iso-download.t b/t/api/02-iso-download.t
index fcb3744f0..53b67ebf5 100644
--- a/t/api/02-iso-download.t
+++ b/t/api/02-iso-download.t
@@ -298,4 +298,17 @@ subtest 'download task only blocks the related job when test suites have differe
     is scalar(@{$gru_dep_tasks->{$gru_task_ids[0]}}), 3, 'one download task was created and it blocked 3 jobs';
 };
 
+subtest 'placeholder expansions work with _URL-derived settings' => sub {
+    $test_suites->find({name => 'kde'})->settings->create({key => 'FOOBAR', value => '%ISO%'});
+    my $new_params = {%params, ISO_URL => 'http://localhost/openSUSE-13.1-DVD-i586-Build0091-Media.iso', TEST => 'kde'};
+    $rsp = schedule_iso($new_params, 200);
+    is $rsp->json->{count}, 1, 'one job was scheduled';
+    my $expanderjob = get_job($rsp->json->{ids}->[0]);
+    is(
+        $expanderjob->{settings}->{FOOBAR},
+        'openSUSE-13.1-DVD-i586-Build0091-Media.iso',
+        '%ISO% in template is expanded by posted ISO_URL'
+    );
+};
+
 done_testing();
-- 
2.29.2