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

@@ -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<Path::Class::File> object. The path,
+which must being with either C<uploads> or C<extras>, is resolved into
+an absolute path with C<uploads> or C<extras> replaced with the value
+of C<uploadsPath> or c<extrasPath> 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<uploads> or C<extras>.
+
+=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;

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|<script type="text/javascript" src="%s">\n|, $file->stringify;
+            return sprintf qq|<script type="text/javascript" src="%s"></script>\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|<script type="text/javascript" src="%s">\n|;
+            $template = qq|<script type="text/javascript" src="%s"></script>\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 $file = $bundle->resolveFilePath($uri->path);
-                my $uploadsRelFile = $file->relative($uploadsDir);
+                
-                $url               = $uploadsUrl->file($uploadsRelFile)->stringify;
+                # 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;

lib/WebGUI/i18n/English/FilePump.pm

@@ -71,8 +71,8 @@ our $I18N = {
 	},
 
 	'otherFiles' => {
-		message => q|CSS Images|,
+		message => q|Collateral|,
-		lastUpdated => 1242681632,
+		lastUpdated => 1247196636,
 		context => q|Edit bundle label.|
 	},
 

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');
+cmp_deeply(
-$bundle->addFile('JS', 'file:/data/domains/mysite.com/www/uploads/XX/YY/XXYYZZ/graviticEnergyDrive.js');
+    [ $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'));
+# Turn fileAsset into file:uploads/path/to/fileAsset (bc file uris must begin with either file:uploads/ or file:extras/)
-my $urilet = URI->new('file:'.$path);
+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:extras/plainblack.gif');
-$bundle->addFile('OTHER', 'file:'.$storage->getPath);
+$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');