From a89fb6f83e17fe0afe9436227c50a7a78be8321e Mon Sep 17 00:00:00 2001 From: Don Armstrong Date: Wed, 10 Aug 2016 12:37:47 -0700 Subject: [PATCH] Use ETags; return timestamp not is_valid - Add test for libravatar.cgi - retrieve_libravatar and cache_location now return a timestamp, not is_valid which can then be used to calculate the Etag - Use HTTP::Server::Simple::CGI::Environment --- Debbugs/Libravatar.pm | 36 +++++++++++--------- cgi/libravatar.cgi | 50 ++++++++++++++++++--------- t/18_libravatar_cgi.t | 78 +++++++++++++++++++++++++++++++++++++++++++ t/lib/DebbugsTest.pm | 3 +- 4 files changed, 135 insertions(+), 32 deletions(-) create mode 100644 t/18_libravatar_cgi.t diff --git a/Debbugs/Libravatar.pm b/Debbugs/Libravatar.pm index 0dafaf2..0c84943 100644 --- a/Debbugs/Libravatar.pm +++ b/Debbugs/Libravatar.pm @@ -84,6 +84,7 @@ sub retrieve_libravatar{ ); my %param = @_; my $cache_location = $param{location}; + my $timestamp; $cache_location =~ s/\.[^\.\/]+$//; # take out a lock on the cache location so that if another request # is made while we are serving this one, we don't do double work @@ -94,9 +95,10 @@ sub retrieve_libravatar{ } else { # figure out if the cache is now valid; if it is, return the # cache location - my ($temp_location, $is_valid) = cache_location(email => $param{email}); - if ($is_valid) { - return $temp_location; + my $temp_location; + ($temp_location, $timestamp) = cache_location(email => $param{email}); + if ($timestamp) { + return ($temp_location,$timestamp); } } require LWP::UserAgent; @@ -166,7 +168,8 @@ sub retrieve_libravatar{ return undef; } simple_unlockfile($fh,$lockfile); - return $cache_location.'.'.$dest_type; + $timestamp = (stat($cache_location.'.'.$dest_type))[9]; + return ($cache_location.'.'.$dest_type,$timestamp); } sub blocked_libravatar { @@ -185,9 +188,9 @@ sub blocked_libravatar { return $blocked; } -# Returns ($path, $is_valid) +# Returns ($path, $timestamp) # - For blocked images, $path will be undef -# - If $is_valid is false (and $path is not undef), the image should +# - If $timestamp is 0 (and $path is not undef), the image should # be re-fetched. sub cache_location { my %param = @_; @@ -204,8 +207,8 @@ sub cache_location { for my $ext ('.png', '.jpg', '') { my $path = $stem.$ext; if (-e $path) { - my $is_valid = (time - (stat(_))[9] < 60*60) ? 1 : 0; - return ($path, $is_valid); + my $timestamp = (time - (stat(_))[9] < 60*60) ? (stat(_))[9] : 0; + return ($path, $timestamp); } } return ($stem, 0); @@ -258,21 +261,22 @@ sub handler { return Apache2::Const::DECLINED(); } # figure out what the md5sum of the e-mail is. - my ($cache_location, $is_valid) = cache_location(email => $email); + my ($cache_location, $timestamp) = cache_location(email => $email); # if we've got it, and it's less than one hour old, return it. - if ($is_valid) { + if ($timestamp) { serve_cache_mod_perl($cache_location,$r); return Apache2::Const::DECLINED(); } - $cache_location = retrieve_libravatar(location => $cache_location, - email => $email, - ); + ($cache_location,$timestamp) = + retrieve_libravatar(location => $cache_location, + email => $email, + ); if (not defined $cache_location) { # failure, serve the default image - serve_cache_mod_perl('',$r); + serve_cache_mod_perl('',$r,$timestamp); return Apache2::Const::DECLINED(); } else { - serve_cache_mod_perl($cache_location,$r); + serve_cache_mod_perl($cache_location,$r,$timestamp); return Apache2::Const::DECLINED(); } } @@ -282,7 +286,7 @@ sub handler { our $magic; sub serve_cache_mod_perl { - my ($cache_location,$r) = @_; + my ($cache_location,$r,$timestamp) = @_; if (not defined $cache_location or not length $cache_location) { # serve the default image $cache_location = $config{libravatar_default_image}; diff --git a/cgi/libravatar.cgi b/cgi/libravatar.cgi index 33aa90a..81f9c73 100755 --- a/cgi/libravatar.cgi +++ b/cgi/libravatar.cgi @@ -13,6 +13,7 @@ use Libravatar::URL; use CGI::Simple; use Cwd qw(abs_path); +use Digest::MD5 qw(md5_hex); my $q = CGI::Simple->new(); @@ -25,45 +26,64 @@ my %param = ); # if avatar is no, serve the empty png if ($param{avatar} ne 'yes' or not defined $param{email} or not length $param{email}) { - serve_cache('',$q); + serve_cache('',$q,0); exit 0; } -my ($cache_location, $is_valid) = cache_location(email => lc($param{email})); +my ($cache_location, $timestamp) = cache_location(email => lc($param{email})); # if we've got it, and it's less than one hour old, return it. -if ($is_valid) { - serve_cache($cache_location,$q); +if ($timestamp) { + serve_cache($cache_location,$q,$timestamp); exit 0; } # if we don't have it, get it, and store it in the cache -$cache_location = retrieve_libravatar(location => $cache_location, - email => lc($param{email}), - ); +($cache_location,$timestamp) = + retrieve_libravatar(location => $cache_location, + email => lc($param{email}), + ); if (not defined $cache_location) { # failure, serve the default image - serve_cache('',$q); + serve_cache('',$q,0); exit 0; } else { - serve_cache($cache_location,$q); + serve_cache($cache_location,$q,$timestamp); exit 0; } sub serve_cache { - my ($cache_location,$q) = @_; + my ($cache_location,$q,$timestamp) = @_; if (not defined $cache_location or not length $cache_location) { # serve the default image $cache_location = $config{libravatar_default_image}; + if (not defined $timestamp or not $timestamp) { + $timestamp = (stat($cache_location))[9]; + } + } + if (not defined $timestamp) { + # this probably means that the default image doesn't exist + print $q->header(status => 404); + print "404: Not found\n"; + return; + } + my $etag = md5_hex($cache_location.$timestamp); + if (defined $q->http('if-none-match') + and $etag eq $q->http('if-none-match')) { + print $q->header(-status => 304); + print "304: Not modified\n"; + return; } my $fh = IO::File->new($cache_location,'r') or - error($q,404, "Failed to open cached image $cache_location"); + error($q,404, "Failed to open cached image $cache_location"); my $m = File::LibMagic->new() or - error($q,500,'Unable to create File::LibMagic object'); + error($q,500,'Unable to create File::LibMagic object'); my $mime_string = $m->checktype_filename(abs_path($cache_location)) or - error($q,500,'Bad file; no mime known'); + error($q,500,'Bad file; no mime known'); print $q->header(-type => $mime_string, - -expires => '+1d', - ); + -expires => '+1d', + -status => 200, + -etag => $etag, + ); print <$fh>; close($fh); } diff --git a/t/18_libravatar_cgi.t b/t/18_libravatar_cgi.t new file mode 100644 index 0000000..4d20e47 --- /dev/null +++ b/t/18_libravatar_cgi.t @@ -0,0 +1,78 @@ +# -*- mode: cperl;-*- + +use warnings; +use strict; + +use Test::More; +# The test functions are placed here to make things easier +use lib qw(t/lib); +use DebbugsTest qw(:all); + +plan tests => 3; + +my $port = 11344; + +# HTTP::Server:::Simple defines a SIG{CHLD} handler that breaks system; undef it here. +$SIG{CHLD} = sub {}; +our %config; +eval { + %config = create_debbugs_configuration(debug => exists $ENV{DEBUG}?$ENV{DEBUG}:0); +}; +if ($@) { + BAIL_OUT($@); + } +$ENV{DEBBUGS_CONFIG_FILE} = "$config{config_dir}/debbugs_config"; +END{ + if ($ENV{DEBUG}) { + diag("spool_dir: $config{spool_dir}\n"); + diag("config_dir: $config{config_dir}\n"); + diag("sendmail_dir: $config{sendmail_dir}\n"); + } +} + +my $libravatar_cgi_handler = sub { + my $fh; + $ENV{DEBBUGS_CONFIG_FILE} = $config{config_dir}."/debbugs_config"; + open($fh,'-|',-e './cgi/libravatar.cgi'? './cgi/libravatar.cgi' + : '../cgi/libravatar.cgi'); + my $headers; + my $status = 200; + while (<$fh>) { + if (/^\s*$/ and $status) { + print "HTTP/1.1 $status OK\n"; + print $headers; + $status = 0; + print $_; + } elsif ($status) { + $headers .= $_; + if (/^Status:\s*(\d+)/i) { + $status = $1; + } + } else { + print $_; + } + } +}; + + +ok(DebbugsTest::HTTPServer::fork_and_create_webserver($libravatar_cgi_handler,$port), + 'forked HTTP::Server::Simple successfully'); + +use LWP::UserAgent; +my $ua = LWP::UserAgent->new; +$ua->agent("DebbugsTesting/0.1 "); + +# Create a request +my $req = HTTP::Request->new(GET => "http://localhost:$port/?avatar=no"); + +my $res = $ua->request($req); +ok($res->is_success(),'cgi/libravatar.cgi returns success'); +my $etag = $res->header('Etag'); + +$req = HTTP::Request->new(GET => "http://localhost:$port/?avatar=no", + ['If-None-Match',$etag]); +$res = $ua->request($req); +ok($res->code() eq '304','If-None-Match set gives us 304 not modified'); + + + diff --git a/t/lib/DebbugsTest.pm b/t/lib/DebbugsTest.pm index df28006..603b6ec 100644 --- a/t/lib/DebbugsTest.pm +++ b/t/lib/DebbugsTest.pm @@ -82,6 +82,7 @@ sub create_debbugs_configuration { \$gSpoolDir='$spool_dir'; \$gLibPath='@{[getcwd()]}/scripts'; \$gTemplateDir='@{[getcwd()]}/templates'; +\$gWebDir='@{[getcwd()]}/html'; \$gWebHost='localhost'; 1; END @@ -194,7 +195,7 @@ sub send_message{ { package DebbugsTest::HTTPServer; - use base qw(HTTP::Server::Simple::CGI); + use base qw(HTTP::Server::Simple::CGI HTTP::Server::Simple::CGI::Environment); our $child_pid = undef; our $webserver = undef; -- 2.39.2