[pve-devel] [PATCH 1/9] HTTPServer.pm: add cookie handling methods

[Fabian Grünbichler]

I am not sure if those should really be promoted to class methods?

IMHO they are more of the "static" helper kind, and should get the
cookie name as additional parameter instead. that way we would not need
to pass the whole server reference to stuff like the Formatters later
on, but just the cookie name..

we could even move them out of HTTPServer to somewhere like Tools.pm,
since all they do is escape and format text (but they might be a bit too
specific to warrant such a move..)

some extra input from Wolfgang inline ;)

On Tue, Jan 10, 2017 at 11:55:18AM +0100, Dietmar Maurer wrote:
> Copied from PVE::REST (I want to get rid of this PVE::REST class).
> 
> Signed-off-by: Dietmar Maurer <dietmar at proxmox.com>
> ---
>  PVE/HTTPServer.pm | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
> index afc152c..168020c 100755
> --- a/PVE/HTTPServer.pm
> +++ b/PVE/HTTPServer.pm
> @@ -145,6 +145,32 @@ sub log_aborted_request {
>      $self->log_request($reqstate);
>  }
>  
> +sub extract_auth_cookie {
> +    my ($self, $cookie) = @_;
> +
> +    return undef if !$cookie;
> +
> +    my $cookie_name = $self->{cookie_name};
> +
> +    my $ticket = ($cookie =~ /(?:^|\s)$cookie_name=([^;]*)/)[0];

although this is just copied and is currently safe, we could use the
opportunity and wrap the variable with \Q and \E to escape it as
additional safeguard to prevent future accidents.

> +
> +    if ($ticket && $ticket =~ m/^PVE%3A/) {
> +	$ticket = uri_unescape($ticket);
> +    }
> +
> +    return $ticket;
> +}
> +
> +sub create_auth_cookie {
> +    my ($self, $ticket) = @_;
> +
> +    my $cookie_name = $self->{cookie_name};
> +
> +    my $encticket = uri_escape($ticket);
> +
> +    return "${cookie_name}=$encticket; path=/; secure;";
> +}
> +
>  sub cleanup_reqstate {
>      my ($reqstate) = @_;
>  
> @@ -586,7 +612,7 @@ sub proxy_request {
>  	    PVEClientIP => $clientip,
>  	};
>  
> -	$headers->{'cookie'} = PVE::REST::create_auth_cookie($ticket) if $ticket;
> +	$headers->{'cookie'} = $self->create_auth_cookie($ticket) if $ticket;
>  	$headers->{'CSRFPreventionToken'} = $token if $token;
>  	$headers->{'Accept-Encoding'} = 'gzip' if $reqstate->{accept_gzip};
>  
> @@ -1234,7 +1260,7 @@ sub unshift_read_header {
>  		} elsif ($path =~ m!$baseuri!) {
>  		    my $token = $r->header('CSRFPreventionToken');
>  		    my $cookie = $r->header('Cookie');
> -		    my $ticket = PVE::REST::extract_auth_cookie($cookie);
> +		    my $ticket = $self->extract_auth_cookie($cookie);
>  
>  		    my ($rel_uri, $format) = split_abs_uri($path);
>  		    if (!$format) {
> @@ -1620,6 +1646,8 @@ sub new {
>  
>      my $self = bless { %args }, $class;
>  
> +    $self->{cookie_name} //= 'PVEAuthCookie';
> +
>      PVE::REST::set_base_handler_class($self->{base_handler_class});
>  
>      # init inotify
> -- 
> 2.1.4
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel