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 missing input validation for needle API endpoints #4985

Merged
merged 2 commits into from Jan 23, 2023
Merged

Fix missing input validation for needle API endpoints #4985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ced17de
Fix missing input validation for needle API endpoints
kraih Jan 23, 2023
68440d6
Initialize Mojolicious::Static only in one place
kraih Jan 23, 2023
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
4 changes: 2 additions & 2 deletions lib/OpenQA/WebAPI.pm
View file
Open in desktop
Expand Up @@ -184,8 +184,8 @@ sub startup ($self) {
$step_auth->post('/')->name('save_needle_ajax')->to('step#save_needle_ajax');
$step_r->get('/')->name('step')->to(action => 'view');

$r->get('/needles/:needle_id/image')->name('needle_image_by_id')->to('file#needle_image_by_id');
$r->get('/needles/:needle_id/json')->name('needle_json_by_id')->to('file#needle_json_by_id');
$r->get('/needles/<needle_id:num>/image')->name('needle_image_by_id')->to('file#needle_image_by_id');
$r->get('/needles/<needle_id:num>/json')->name('needle_json_by_id')->to('file#needle_json_by_id');
$r->get('/needles/:distri/#name')->name('needle_file')->to('file#needle');
# this route is used in the helper
$r->get('/image/:md5_dirname/.thumbs/#md5_basename')->name('thumb_image')->to('file#thumb_image');
Expand Down
60 changes: 30 additions & 30 deletions lib/OpenQA/WebAPI/Controller/File.pm
View file
Open in desktop
Expand Up @@ -12,6 +12,9 @@ use File::Spec;
use File::Spec::Functions 'catfile';
use Data::Dump 'pp';
use Mojo::File 'path';
use Scalar::Util qw(blessed);

has static => sub { Mojolicious::Static->new };

sub needle ($self) {
# do the format splitting ourselves instead of using mojo to restrict the suffixes
Expand Down Expand Up @@ -54,21 +57,21 @@ sub needle ($self) {
# we got something like subdir/needle.json, $path will be "subdir"
$needledir .= "/$path";
}
push(@{($self->{static} = Mojolicious::Static->new)->paths}, $needledir);
push @{$self->static->paths}, $needledir;

# name is an URL parameter and can't contain slashes, so it should be safe
return $self->serve_static_($name . $format);
return $self->_serve_static($name . $format);
}

sub _needle_by_id_and_extension ($self, $extension) {
my $needle_id = $self->param('needle_id') or return $self->reply->not_found;
my $needle = $self->schema->resultset('Needles')->find($needle_id) or return $self->reply->not_found;
return $self->reply->not_found unless my $needle_id = $self->param('needle_id');
return $self->reply->not_found unless my $needle = $self->schema->resultset('Needles')->find($needle_id);

my $needle_dir = $needle->directory->path;
my $needle_filename = $needle->name . $extension;

$self->{static} = Mojolicious::Static->new;
push(@{$self->{static}->paths}, $needle_dir);
return $self->serve_static_($needle_filename);
push @{$self->static->paths}, $needle_dir;
return $self->_serve_static($needle_filename);
}

sub needle_image_by_id ($self) {
Expand All @@ -85,16 +88,13 @@ sub _set_test ($self) {

$self->{testdirname} = $self->{job}->result_dir;
return unless $self->{testdirname};
$self->{static} = Mojolicious::Static->new;
push @{$self->{static}->paths}, $self->{testdirname};
push @{$self->{static}->paths}, $self->{testdirname} . "/ulogs";
push @{$self->static->paths}, $self->{testdirname}, "$self->{testdirname}/ulogs";
return 1;
}

sub test_file ($self) {
return $self->reply->not_found unless $self->_set_test;

return $self->serve_static_($self->param('filename'));
return $self->_serve_static($self->param('filename'));
}

sub download_asset ($self) {
Expand Down Expand Up @@ -139,51 +139,51 @@ sub test_asset ($self) {
return $self->redirect_to($path);
}

sub serve_static_ ($self, $asset) {
$self->app->log->debug("looking for " . pp($asset) . " in " . pp($self->{static}->paths));
$asset = $self->{static}->file($asset) if $asset && !ref($asset);
sub _serve_static ($self, $asset) {
my $static = $self->static;
my $log = $self->log;

$log->debug('looking for ' . pp($asset) . ' in ' . pp($static->paths));
$asset = $static->file($asset) if $asset && !ref($asset);
return $self->reply->not_found unless $asset;
$self->app->log->debug("found " . pp($asset));
$log->debug('found ' . pp($asset));

if (ref($asset) eq "Mojo::Asset::File") {
if (blessed $asset && $asset->isa('Mojo::Asset::File')) {
my $filename = basename($asset->path);
# guess content type from extension
my $headers = $self->res->headers;
if ($filename =~ m/\.([^\.]+)$/) {
my $ext = $1;
my $filetype = $self->app->types->type($ext);
if ($filetype) {
$self->res->headers->content_type($filetype);
}
if ($ext eq 'iso') {
# force saveAs
$self->res->headers->content_disposition("attachment; filename=$filename;");
}
$headers->content_type($filetype) if $filetype;

# force saveAs
$headers->content_disposition("attachment; filename=$filename;") if $ext eq 'iso';
}
else {
$self->res->headers->content_type("application/octet-stream");
}
}

$self->{static}->serve_asset($self, $asset);
$static->serve_asset($self, $asset);
return !!$self->rendered;
}

# images are served by test_file, only thumbnails are special
sub test_thumbnail ($self) {
return $self->reply->not_found unless $self->_set_test;

my $asset = $self->{static}->file(".thumbs/" . $self->param('filename'));
return $self->serve_static_($asset);
my $asset = $self->static->file(".thumbs/" . $self->param('filename'));
return $self->_serve_static($asset);
}

# this is the agnostic route to images - usually served by apache directly
sub thumb_image ($self) {
$self->{static} = Mojolicious::Static->new;
push @{$self->{static}->paths}, imagesdir();
push @{$self->static->paths}, imagesdir();

# name is an URL parameter and can't contain slashes, so it should be safe
my $dir = $self->param('md5_dirname') || ($self->param('md5_1') . '/' . $self->param('md5_2'));
return $self->serve_static_("$dir/.thumbs/" . $self->param('md5_basename'));
return $self->_serve_static("$dir/.thumbs/" . $self->param('md5_basename'));
}

1;
8 changes: 5 additions & 3 deletions t/ui/07-file.t
View file
Open in desktop
Expand Up @@ -72,9 +72,11 @@ subtest 'needle download' => sub {
$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')
# broken requests we saw in production that should not get past input validation
$t->get_ok('/needles/root-console-20200501/image')->status_is(404);
$t->get_ok('/needles/root-console-20200501/json')->status_is(404);

$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:
Expand Down