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.

Sign up for GitHub

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

Jump to bottom

Fix URLs for needles in subdirectories (POO #58959) #2456

Merged
merged 1 commit into from
Nov 5, 2019
Merged

Fix URLs for needles in subdirectories (POO #58959) #2456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 28 additions & 4 deletions lib/OpenQA/WebAPI/Controller/File.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,39 @@ sub needle {
my $version = $self->param('version') || '';
my $jsonfile = $self->param('jsonfile') || '';

# locate the needle in the needle directory for the given distri and version
my $needledir = needledir($distri, $version);

# make sure the directory of the file parameter is a real subdir of testcasedir before
# applying it as needledir to prevent access outside of the zoo
# using it to find needle subdirectory, to prevent access outside of the zoo
if ($jsonfile && !is_in_tests($jsonfile)) {
warn "$jsonfile is not in a subdir of $prjdir/share/tests or $prjdir/tests";
return $self->render(text => 'Forbidden', status => 403);
}

# locate the needle in the needle directory for the given distri and version
push(@{($self->{static} = Mojolicious::Static->new)->paths}, needledir($distri, $version));
# Reject directory traversal breakouts here...
if (index($jsonfile, '..') != -1) {
warn "jsonfile value $jsonfile is invalid, cannot contain ..";
return $self->render(text => 'Forbidden', status => 403);
}
# we need to handle the needle being in a subdirectory - we cannot assume it is always just
# '$needledir/$name.$format'. figure out subdirectory elements from the JSON file path
# Note this means you cannot just browse to /needles/distri/subdir/needle.png;
# you can only find needles in subdirectories by passing the jsonfile parameter
my ($dummy1, $path, $dummy2) = fileparse($jsonfile);
# drop the trailing / from $path
$path = substr($path, 0, -1);
if (index($path, '/needles') != -1) {
# we got something like /var/lib/openqa/share/tests/distri/needles/(subdir)/needle.json
my @elems = split('/needles', $path, 2);
if (defined $elems[1]) {
$needledir .= $elems[1];
}
}
elsif ($path ne '.') {
# we got something like subdir/needle.json, $path will be "subdir"
$needledir .= "/$path";
}
push(@{($self->{static} = Mojolicious::Static->new)->paths}, $needledir);

# name is an URL parameter and can't contain slashes, so it should be safe
return $self->serve_static_($name . $format);
Expand Down
31 changes: 29 additions & 2 deletions t/ui/07-file.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Mojo::Base -strict;

use FindBin;
use File::Spec;
use lib "$FindBin::Bin/../lib";
use Test::More;
use Test::Mojo;
Expand Down Expand Up @@ -63,25 +64,51 @@ sub write_file {

subtest 'needle download' => sub {
# clean leftovers from previous run
my $needle_dir = Mojo::File->new('t/data/openqa/share/tests/opensuse/needles');
my $needle_path = 't/data/openqa/share/tests/opensuse/needles';
my $abs_needle_path = File::Spec->rel2abs($needle_path);
my $needle_dir = Mojo::File->new($needle_path);
$needle_dir->remove_tree();

$t->get_ok('/needles/opensuse/inst-timezone-text.png')->status_is(404, '404 if image not present');
$t->get_ok('/needles/1/image')->status_is(404, '404 if image not present');
$t->get_ok('/needles/1/json')->status_is(404, '404 if json not present');

# create fake json file and image

$needle_dir->make_path();
my $json
= '{"area" : [{"height": 217, "type": "match", "width": 384, "xpos": 0, "ypos": 0},{"height": 60, "type": "exclude", "width": 160, "xpos": 175, "ypos": 45}], "tags": ["inst-timezone"]}';
write_file("$needle_dir/inst-timezone-text.png", "png\n");
write_file("$needle_dir/inst-timezone-text.json", $json);

# and another, in a subdirectory, to test that
my $needle_subdir = Mojo::File->new('t/data/openqa/share/tests/opensuse/needles/subdirectory');
$needle_subdir->make_path();
my $json2
= '{"area" : [{"height": 217, "type": "match", "width": 384, "xpos": 0, "ypos": 0},{"height": 60, "type": "exclude", "width": 160, "xpos": 175, "ypos": 45}], "tags": ["inst-subdirectory"]}';
write_file("$needle_subdir/inst-subdirectory.png", "png\n");
write_file("$needle_subdir/inst-subdirectory.json", $json2);

$t->get_ok('/needles/opensuse/inst-timezone-text.png')->status_is(200)->content_type_is('image/png')
->content_is("png\n");
$t->get_ok('/needles/1/image')->status_is(200)->content_type_is('image/png')->content_is("png\n");
$t->get_ok('/needles/1/json')->status_is(200)->content_type_is('application/json;charset=UTF-8')->content_is($json);

# arguably this should work and be tested, but does not work now because
# of how we do routing:
#$t->get_ok('/needles/opensuse/subdirectory/inst-subdirectory.png')

# currently you can only find a needle in a subdirectory by passing the
# jsonfile query parameter like this:
$t->get_ok("/needles/opensuse/inst-subdirectory.png?jsonfile=$needle_path/subdirectory/inst-subdirectory.json")
->status_is(200)->content_type_is('image/png')->content_is("png\n");
# also test with jsonfile as absolute path (as usual in production)
$t->get_ok("/needles/opensuse/inst-subdirectory.png?jsonfile=$abs_needle_path/subdirectory/inst-subdirectory.json")
->status_is(200)->content_type_is('image/png')->content_is("png\n");

# getting needle image and json by ID also does not work for needles
# in subdirectories, but arguably should do and should be tested:
#$t->get_ok('/needles/2/image')->status_is(200)->content_type_is('image/png')->content_is("png\n");
#$t->get_ok('/needles/2/json')->status_is(200)->content_type_is('application/json;charset=UTF-8')->content_is($json2);
};


Expand Down
8 changes: 7 additions & 1 deletion t/ui/12-needle-edit.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,9 +614,15 @@ subtest '(created) needles can be accessed over API' => sub {
->status_is(200, 'needle accessible')->content_type_is('image/png');
@warnings = warnings {
$t->get_ok('/needles/opensuse/test-newneedle.png?jsonfile=/try/to/break_out.json')
->status_is(403, 'access to files outside the test directory not granted');
->status_is(403, 'access to files outside the test directory not granted (absolute)');
};
map { like($_, qr/is not in a subdir of/, 'expected warning') } @warnings;
@warnings = warnings {
$t->get_ok(
'/needles/opensuse/test-newneedle.png?jsonfile=t/data/openqa/share/tests/opensuse/needles/../../../../try/to/break_out.json'
)->status_is(403, 'access to files outside the test directory not granted (relative)');
};
map { like($_, qr/cannot contain ../, 'expected warning') } @warnings;

my $tmp_dir = 't/tmp_needles';
File::Path::rmtree($tmp_dir);
Expand Down