From 7110febecd89ea9981d41117bbd7412d32a97aaf Mon Sep 17 00:00:00 2001 From: Patrick Donelan Date: Fri, 10 Jul 2009 03:33:06 +0000 Subject: [PATCH] FilePump bug fixes Restricted file uris to uploads and extras dirs Validation messages for invalid file uris Updated i18n Added more tests --- lib/WebGUI/FilePump/Bundle.pm | 91 ++++++++++++++++++++++++++++- lib/WebGUI/Macro/FilePump.pm | 49 +++++++++++++--- lib/WebGUI/i18n/English/FilePump.pm | 4 +- t/FilePump/Bundle.t | 68 ++++++++++++++++----- 4 files changed, 188 insertions(+), 24 deletions(-) diff --git a/lib/WebGUI/FilePump/Bundle.pm b/lib/WebGUI/FilePump/Bundle.pm index 39a2800c8..8bebe129f 100644 --- a/lib/WebGUI/FilePump/Bundle.pm +++ b/lib/WebGUI/FilePump/Bundle.pm @@ -42,6 +42,11 @@ sub addFile { my $files = $self->get($collateralType); my $uriExists = $self->getCollateralDataIndex($files, 'uri', $uri) != -1 ? 1 : 0; return 0, 'Duplicate URI' if $uriExists; + + if (my $msg = $self->validate($uri)) { + return 0, $msg; + } + $self->setCollateral( $collateralType, 'fileId', @@ -54,6 +59,7 @@ sub addFile { $self->update({lastModified => time()}); return 1; } + #------------------------------------------------------------------- @@ -569,7 +575,7 @@ A URI object. sub fetchDir { my ($self, $uri ) = @_; - my $filepath = $uri->path; + my $filepath = $self->resolveFilePath($uri->path); return {} unless (-e $filepath && -r _ && -d _); my @stats = stat(_); my $dir = Path::Class::Dir->new($filepath); @@ -601,7 +607,7 @@ A URI object. sub fetchFile { my ($self, $uri ) = @_; - my $filepath = $uri->path; + my $filepath = $self->resolveFilePath($uri->path); return {} unless (-e $filepath && -r _); return $self->fetchDir($uri) if -d _; my @stats = stat(_); # recycle stat data from file tests. @@ -994,5 +1000,86 @@ sub setCollateral { return $keyValue; } +#------------------------------------------------------------------- + +=head2 validate ( $uri ) + +Check a uri for validation errors. Returns a validation error message (if problems found). + +=head3 $uri + +The URI to validate + +=cut + +sub validate { + my $self = shift; + my $uri = shift; + + my $urio = URI->new($uri) or return "Invalid URI: $uri"; + my $scheme = $urio->scheme; + my $path = $urio->path; + + # File validation + if ($scheme eq 'file') { + if ($path !~ m{^uploads/|^extras/}) { + return q{File uri must begin with file:uploads/.. or file:extras/..}; + } + + # N.B. Once we have Path::Class >= 0.17 we can use resolve() for a better solution to this canonicalisation problem + if ($path =~ m{\.\./}) { + return q{Directory traversal not permitted}; + } + + my $uploadsDir = Path::Class::Dir->new($self->session->config->get('uploadsPath')); + my $extrasDir = Path::Class::Dir->new($self->session->config->get('extrasPath')); + + my $file = $self->resolveFilePath($path); + + return q{File not found} unless -e $file; + + if (!$uploadsDir->contains($file) && !$extrasDir->contains($file)) { + return q{File uri must correspond to files inside your uploads dir or your extras dir}; + } + } + + return; +} + +#------------------------------------------------------------------- + +=head2 resolveFilePath ( $path ) + +Resolves a relative path into a L object. The path, +which must being with either C or C, is resolved into +an absolute path with C or C replaced with the value +of C or c from the site config file. + +For example, the following path + + file:extras/path/to/my/file + +Resolves to something like: + + /data/WebGUI/www/extras/path/to/my/file + +=head3 $path + +A relative file path that must begine with either C or C. + +=cut + +sub resolveFilePath { + my $self = shift; + my $path = shift; + + if ($path =~ s{^uploads/}{}) { + my $uploadsDir = Path::Class::Dir->new($self->session->config->get('uploadsPath')); + return Path::Class::File->new($uploadsDir, $path); + } elsif ($path =~ s{^extras/}{}) { + my $extrasDir = Path::Class::Dir->new($self->session->config->get('extrasPath')); + return Path::Class::File->new($extrasDir, $path); + } +} 1; diff --git a/lib/WebGUI/Macro/FilePump.pm b/lib/WebGUI/Macro/FilePump.pm index cbeb50d58..9ab4e50fb 100644 --- a/lib/WebGUI/Macro/FilePump.pm +++ b/lib/WebGUI/Macro/FilePump.pm @@ -13,6 +13,7 @@ package WebGUI::Macro::FilePump; use strict; use WebGUI::FilePump::Bundle; use Path::Class; +use WebGUI::International; =head1 NAME @@ -39,7 +40,8 @@ $bundleName, the name of a File Pump bundle. =item * -$type, the type of files from the Bundle that you are accessing. Either JS or javascript, or CSS or css. +$type, the type of files from the Bundle that you are accessing. +Either JS or javascript, or CSS or css (case-insensitive). =back @@ -62,15 +64,39 @@ sub process { my $bundle = WebGUI::FilePump::Bundle->new($session, $bundleId->[0]); return '' unless $bundle; + + my $bundleDir = $bundle->getPathClassDir; + + # Sometimes, when migrating sites, restoring from backup, etc., you can + # get into the situation where the bundle is defined in the db but doesn't + # exist on the filesystem. In this case, simply generate a warning and + # trigger a bundle rebuild. + if (!-e $bundleDir) { + $session->log->warn("Bundle is marked as built, but does not exist on filesystem. Attempting to rebuild: $bundleDir"); + my ($code, $error) = $bundle->build; + if ($error) { + my $i18n = WebGUI::International->new($session, 'FilePump'); + $error = sprintf $i18n->get('build error'), $error; + $session->log->error("Rebuild failed with error: $error"); + return $error; + } else { + $session->log->warn("Rebuild succeeded, continuing with macro processing"); + } + } + my $uploadsDir = Path::Class::Dir->new($session->config->get('uploadsPath')); + my $extrasDir = Path::Class::Dir->new($session->config->get('extrasPath')); my $uploadsUrl = Path::Class::Dir->new($session->config->get('uploadsURL')); + my $extrasUrl = Path::Class::Dir->new($session->config->get('extrasURL')); ##Normal mode if (! $session->var->isAdminOn) { + # Built files live at /path/to/uploads/filepump/bundle.timestamp/ which is + # a sub-dir of uploadsDir, so resolve the dir relative to uploads my $dir = $bundle->getPathClassDir->relative($uploadsDir); if ($type eq 'js' || $type eq 'javascript') { my $file = $uploadsUrl->file($dir, $bundle->bundleUrl . '.js'); - return sprintf qq|\n|, $file->stringify; } elsif ($type eq 'css') { my $file = $uploadsUrl->file($dir, $bundle->bundleUrl . '.css'); @@ -85,7 +111,7 @@ sub process { my $template; my $files; if ($type eq 'js' || $type eq 'javascript') { - $template = qq|\n|; $files = $bundle->get('jsFiles'); } elsif ($type eq 'css') { @@ -103,10 +129,19 @@ sub process { $url = $uri->opaque; } elsif ($scheme eq 'file') { - my $file = Path::Class::File->new($uri->path); - my $uploadsRelFile = $file->relative($uploadsDir); - $url = $uploadsUrl->file($uploadsRelFile)->stringify; - + my $file = $bundle->resolveFilePath($uri->path); + + # Un-built files live inside either uploads or extras + if ($uploadsDir->subsumes($file)) { + my $relFile = $file->relative($uploadsDir); + $url = $uploadsUrl->file($relFile)->stringify; + } elsif ($extrasDir->subsumes($file)) { + my $relFile = $file->relative($extrasDir); + $url = $extrasUrl->file($relFile)->stringify; + } else { + $session->log->warn("Invalid file: $file"); + next; + } } elsif ($scheme eq 'http' or $scheme eq 'https') { $url = $uri->as_string; diff --git a/lib/WebGUI/i18n/English/FilePump.pm b/lib/WebGUI/i18n/English/FilePump.pm index a5809c34e..a582a7ee6 100644 --- a/lib/WebGUI/i18n/English/FilePump.pm +++ b/lib/WebGUI/i18n/English/FilePump.pm @@ -71,8 +71,8 @@ our $I18N = { }, 'otherFiles' => { - message => q|CSS Images|, - lastUpdated => 1242681632, + message => q|Collateral|, + lastUpdated => 1247196636, context => q|Edit bundle label.| }, diff --git a/t/FilePump/Bundle.t b/t/FilePump/Bundle.t index 1dde9211f..ec01a287e 100644 --- a/t/FilePump/Bundle.t +++ b/t/FilePump/Bundle.t @@ -33,7 +33,7 @@ my $session = WebGUI::Test->session; #---------------------------------------------------------------------------- # Tests -my $tests = 54; # Increment this number for each test you create +my $tests = 62; # Increment this number for each test you create plan tests => 1 + $tests; # 1 for the use_ok #---------------------------------------------------------------------------- @@ -88,14 +88,37 @@ is( '... okay to add a duplicate to another type' ); +ok($bundle->addFile('JS', 'http://mysite.com/helloworld.js'), 'added a second http uri'); + cmp_deeply( [ $bundle->addFile('JS', 'http://mysite.com/script.js') ], [ 0, 'Duplicate URI' ], '... checking error message for duplicate URI' ); -$bundle->addFile('JS', 'http://mysite.com/helloworld.js'); -$bundle->addFile('JS', 'file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js'); +cmp_deeply( + [ $bundle->addFile('JS', 'file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js') ], + [ 0, q{File uri must begin with file:uploads/.. or file:extras/..} ], + '... checking error message for file outside of uploads' +); + +cmp_deeply( + [ $bundle->addFile('JS', 'file:extras/graviticEnergyDrive.js') ], + [ 0, q{File not found} ], + '... checking error message for missing file' +); + +cmp_deeply( + [ $bundle->addFile('JS', 'file:extras/../../etc/log.conf') ], + [ 0, q{Directory traversal not permitted} ], + '... checking error message for directory traversal' +); + +cmp_deeply( + $bundle->addFile('JS', 'file:extras/hoverhelp.js'), + 1, + 'added a valid file uri' +); my @fileUris = map { $_->{uri} } @{ $bundle->get('jsFiles') }; cmp_deeply( @@ -103,11 +126,24 @@ cmp_deeply( [qw{ http://mysite.com/script.js http://mysite.com/helloworld.js - file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js + file:extras/hoverhelp.js }], '... checking actual jsFiles data structure contents' ); +cmp_deeply( + $bundle->addFile('OTHER', 'file:extras/adminConsole'), + 1, + 'added a valid file folder' +); + +my @fileUris = map { $_->{uri} } @{ $bundle->get('otherFiles') }; +cmp_deeply( + [ @fileUris ], + [ 'file:extras/adminConsole' ], + '... checking actual otherFiles data structure contents' +); + ################################################################### # # moveFile{Up,Down} @@ -160,7 +196,7 @@ cmp_deeply( [qw{ http://mysite.com/helloworld.js http://mysite.com/script.js - file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js + file:extras/hoverhelp.js }], '... checking the actual order of js files' ); @@ -173,7 +209,7 @@ cmp_deeply( [ @fileUris ], [qw{ http://mysite.com/helloworld.js - file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js + file:extras/hoverhelp.js http://mysite.com/script.js }], '... checking the actual order of js files' @@ -270,9 +306,11 @@ cmp_deeply( 'fetchAsset: retrieved a file asset' ); -my $path = $fileAsset->getStorageLocation->getPath($fileAsset->get('filename')); -my $urilet = URI->new('file:'.$path); - +# Turn fileAsset into file:uploads/path/to/fileAsset (bc file uris must begin with either file:uploads/ or file:extras/) +my $path = Path::Class::File->new($fileAsset->getStorageLocation->getPath($fileAsset->get('filename'))); +my $uploadsDir = Path::Class::Dir->new($session->config->get('uploadsPath')); +$path = $path->relative($uploadsDir); +my $urilet = URI->new('file:uploads/'.$path); $guts = $bundle->fetchFile($urilet); cmp_deeply( $guts, @@ -284,7 +322,9 @@ cmp_deeply( 'fetchFile: retrieved a file from the filesystem' ); -my $uriDir = URI->new('file:'.$storage->getPath); +my $storageRelPath = 'uploads/' . Path::Class::Dir->new($storage->getPath)->relative($uploadsDir); + +my $uriDir = URI->new("file:$storageRelPath"); $guts = $bundle->fetchDir($uriDir); cmp_deeply( $guts, @@ -331,9 +371,11 @@ cmp_deeply( $bundle->deleteFiles('JS'); $bundle->deleteFiles('CSS'); +$bundle->deleteFiles('OTHER'); cmp_deeply($bundle->get('jsFiles'), [], ' deleteFiles deleted all JS URIs'); cmp_deeply($bundle->get('cssFiles'), [], ' ... deleted all CSS URIs'); +cmp_deeply($bundle->get('otherFiles'), [], ' ... deleted all OTHER URIs'); ################################################################### # @@ -355,8 +397,8 @@ $fileAsset->update({filename => 'pumpfile.css'}); $bundle->addFile('JS', 'asset://filePumpSnippet'); $bundle->addFile('CSS', 'asset://filePumpFileAsset'); -$bundle->addFile('OTHER', 'file:'.WebGUI::Test->getTestCollateralPath('gooey.jpg')); -$bundle->addFile('OTHER', 'file:'.$storage->getPath); +$bundle->addFile('OTHER', 'file:extras/plainblack.gif'); +$bundle->addFile('OTHER', "file:$storageRelPath"); my ($buildFlag, $error) = $bundle->build(); ok($buildFlag, 'build returns true when there are no errors'); diag $error unless $buildFlag; @@ -368,7 +410,7 @@ ok(-e $buildDir->stringify && -d _, '... new build directory created'); ok(!-e $oldBuildDir->stringify && !-d _, '... old build directory deleted'); my $jsFile = $buildDir->file($bundle->bundleUrl . '.js'); my $cssFile = $buildDir->file($bundle->bundleUrl . '.css'); -my $otherFile = $buildDir->file('gooey.jpg'); +my $otherFile = $buildDir->file('plainblack.gif'); my $otherDir = $buildDir->subdir($storage->getHexId); ok(-e $jsFile->stringify && -f _ && -s _, '... minified JS file built, not empty'); ok(-e $cssFile->stringify && -f _ && -s _, '... minified CSS file built, not empty');