Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate the test name when creating new jobs #6195

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/OpenQA/Jobs/Constants.pm
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ use Mojo::Base -base, -signatures;

use Exporter 'import';

# define regex for validating test names in accordance with `JobScenarios-01.yaml` and `JobTemplates-01.yaml`
# note: In contrast to the YAML schema a few more characters are allowed here as they are useful for manually
# triggered jobs, e.g. via `openqa-clone-custom-git-refspec`.
use constant TEST_NAME_REGEX => qr|^[A-Za-z 0-9_*.+,:/#@%"'-]+\z|;

# job states
use constant {
# initial job state; the job is supposed to be assigned to a worker by the scheduler
Expand Down Expand Up @@ -103,6 +108,7 @@ use constant DEFAULT_JOB_PRIORITY => 50;
use constant TAG_ID_COLUMN => "concat(VERSION, '-', BUILD)";

our @EXPORT = qw(
TEST_NAME_REGEX
ASSIGNED
CANCELLED
COMPLETE_RESULTS
Expand Down
5 changes: 3 additions & 2 deletions lib/OpenQA/Schema/ResultSet/Jobs.pm
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,10 @@ sub create_from_settings {
die 'The ' . join(',', @invalid_keys) . " cannot include / in value\n" if @invalid_keys;

# validate special settings
my %special_settings = (_PRIORITY => delete $settings{_PRIORITY});
my %special_settings = (TEST => delete $settings{TEST}, _PRIORITY => delete $settings{_PRIORITY});
my $validator = Mojolicious::Validator->new;
my $v = Mojolicious::Validator::Validation->new(validator => $validator, input => \%special_settings);
my $test = $v->required('TEST')->like(TEST_NAME_REGEX)->param;
my $prio = $v->optional('_PRIORITY')->num->param;
die 'The following settings are invalid: ' . join(', ', @{$v->failed}) . "\n" if $v->has_error;

Expand Down Expand Up @@ -176,9 +177,9 @@ sub create_from_settings {
my $value = $settings{$key};
$settings{$key} = decode_utf8 encode_json $value if (ref $value eq 'ARRAY' || ref $value eq 'HASH');
}
$new_job_args{TEST} = $settings{TEST};

# move important keys from the settings directly to the job
$new_job_args{TEST} = $test;
for my $key (OpenQA::Schema::Result::Jobs::MAIN_SETTINGS) {
if (my $value = delete $settings{$key}) { $new_job_args{$key} = $value }
}
Expand Down
2 changes: 1 addition & 1 deletion public/schema/JobScenarios-01.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ properties:
type: object
additionalProperties: false
patternProperties:
^[A-Za-z\s0-9_*.+-]+$:
^[A-Za-z 0-9_*.+-]+$:
type: object
description: The name of the job template
additionalProperties: false
Expand Down
2 changes: 1 addition & 1 deletion public/schema/JobTemplates-01.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ properties:
description: A test suite with machine and/or priority value specified, or a custom job template name if testsuite was specified
additionalProperties: false
patternProperties:
&testsuite-pattern ^[A-Za-z\s0-9_*.+-]+$:
&testsuite-pattern ^[A-Za-z 0-9_*.+-]+$:
type: object
additionalProperties: false
properties:
Expand Down
10 changes: 10 additions & 0 deletions t/api/04-jobs.t
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,16 @@ subtest 'get job status' => sub {
->json_is('/error_status' => 404, 'Status code correct')->json_is('/error' => 'Job does not exist');
};

subtest 'validation of test name' => sub {
my @disallowed = ('spam=eggs', "spam\teggs", "spam\neggs", "spam eggs\n");
my @allowed = ('spam.eggs', 'spam+eggs', 'spam:"eggs"', 'spam@eggs', "spam 'eggs'");
$t->post_ok('/api/v1/jobs', form => {TEST => $_})->status_is(400, "test name $_ disallowed") for @disallowed;
$t->json_is('/error' => 'The following settings are invalid: TEST', 'error for invalid test name returned');
$t->post_ok('/api/v1/jobs', form => {TEST => $_})->status_is(200, "test name $_ allowed") for @allowed;
is $jobs->search({TEST => {-in => \@disallowed}})->count, 0, 'no jobs with disallowed names created';
is $jobs->search({TEST => {-in => \@allowed}})->count, @allowed, 'all jobs with allowed names created';
};

subtest 'cancel job' => sub {
$t->post_ok('/api/v1/jobs/99963/cancel')->status_is(200);
is_deeply(
Expand Down
4 changes: 2 additions & 2 deletions t/api/08-jobtemplates.t
Original file line number Diff line number Diff line change
Expand Up @@ -496,12 +496,12 @@ $yaml->{products}{$product}{version} = '42.1';
# Add non-trivial test suites to exercise the validation
$yaml->{scenarios}{'x86_64'}{$product} = [
'spam',
"eg-G_S +133t*.\t",
"eg-G_S +133t*.",
{
'foo' => {},
},
{
"b-A_RRRR +133t*.\t" => {
"b-A_RRRR +133t*." => {
machine => 'x86_64',
priority => 33,
},
Expand Down
4 changes: 2 additions & 2 deletions t/data/job-templates/schema-invalid.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ properties:
description: A test suite with machine and/or priority value specified, or a custom job template name if testsuite was specified
additionalProperties: false
patternProperties:
^[A-Za-z\s0-9_*.+-]+$:
^[A-Za-z 0-9_*.+-]+$:
type: object
additionalProperties: false
properties:
Expand All @@ -49,7 +49,7 @@ properties:
type: string
testsuite:
type: string
pattern: ^[A-Za-z\s0-9_*.+-]+$
pattern: ^[A-Za-z 0-9_*.+-]+$
description: The test suite this scenario is based on if a custom job template name was used
description:
type: string
Expand Down