1286287 - Add utility method to Bugzilla::CGI for configuring CSP headers

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

There should be a utility method that sets up the CSP headers (prefix and unprefixed) to be sent by $cgi->headers().

$cgi->content_security_policy(key => [values...])

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286287_3.patch

Alright, this even passes doc tests but I consider this a first draft. The whole reason for doing objects (as discussed earlier in IRC) it to catch typos,
which is why this optional feature pulls in a Type::Tiny and MooX::StrictConstructor. 

In testing this, I have twice misspelled one of the CSP directives...

This is a very lax policy and the intention is that it shouldn't break anything.

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 8773015 [details] [diff] [review]
1286287_3.patch

Review of attachment 8773015 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/CGI.pm
@@ +116,5 @@
> +            my $csp = Bugzilla::CGI::ContentSecurityPolicy->new(@_);
> +            $self->{Bugzilla_csp} = $csp;
> +        }
> +
> +        return $self->{Bugzilla_csp} //= Bugzilla::CGI::ContentSecurityPolicy->new(DEFAULT_CSP);

This is different than how I envisioned it. I was thinking we would have the default csp params and then individual scripts can override specific directives. The way it is written, if a script wanted to add/modify a directive, it would also need to provide all other values as well. How about?:

sub content_security_policy {
    my $self = shift;
    if (Bugzilla->has_feature('csp')) {
        my %params = DEFAULT_CSP;
        if (@_) {
            my %add_params = @_;
            foreach my $key (keys %add_params) {
                $params{$key} = $add_params{$key};
            }
        }
        require Bugzilla::CGI::ContentSecurityPolicy;
        return $self->{cgi_csp} //= Bugzilla::CGI::ContentSecurityPolicy->new(%params);
    }
    return undef;
}

::: Bugzilla/CGI/ContentSecurityPolicy.pm
@@ +8,5 @@
> +package Bugzilla::CGI::ContentSecurityPolicy;
> +
> +use 5.14.0;
> +use strict;
> +use warnings;

nit: add newline

@@ +22,5 @@
> +            ^(?: https?:// )?  # optional http:// or https://
> +            [*A-Za-z0-9.-]+    # hostname including wildcards. Possibly too permissive.
> +            (?: :[0-9]     )?  # optional port !x
> +};
> +my $SRC      = $SRC_KEYWORD | $SRC_URI;

nit: fix whitespace

@@ +31,5 @@
> +)];
> +
> +my @ALL_SRC = qw(
> +    default_src child_src connect_src font_src img_src media_src object_src
> +    script_src style_src 

nit: whitespace, also balance the values:

my @ALL_SRC = qw(
    default_src child_src connect_src
    font_src img_src media_src 
    object_src script_src style_src
);

@@ +40,5 @@
> +has 'base_uri'   => ( is => 'ro', isa => Str, predicate => 1 );
> +has 'report_only' => ( is => 'ro', isa => Bool );
> +has 'referrer' => ( is => 'ro', isa => $REFERRER_KEYWORD, predicate => 1 );
> +has 'sandbox' => ( is => 'ro', isa => Bool, default => 0 );
> +has 'upgrade_insecure_requests' => ( is => 'ro', isa => Bool, default => 0 );

nit: line up =>

@@ +44,5 @@
> +has 'upgrade_insecure_requests' => ( is => 'ro', isa => Bool, default => 0 );
> +
> +has 'header_value' => (is => 'lazy');
> +has 'nonce' => ( is => 'lazy', init_arg => undef, predicate => 1 );
> +has 'disable' => (is => 'ro', isa => Bool, default => 0);

same

@@ +300,5 @@
> +This returns a list of header names. This will typically be C<Content-Security-Policy>, C<X-Content-Security-Policy>, and C<X-WebKit-CSP>.
> +
> +=head2 header_value()
> +
> +This returns the value or left-of-colon part of the header.

right-of-colon

@@ +310,5 @@
> +=head2 nonce() / has_nonce()
> +
> +This is unique value that can used if the 'nonce' is used as a source for style_src or script_src.
> +
> +=head1 UNDOCUMENTED THINGS

To be consistent with all other pod: 

=head1 B<Methods in need of POD>

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1286287_4.patch

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 8774970 [details] [diff] [review]
1286287_4.patch

Review of attachment 8774970 [details] [diff] [review]:
-----------------------------------------------------------------

Otherwise checks out. Fix on commit. r=dkl

::: Bugzilla/CGI.pm
@@ +108,5 @@
>      return $self;
>  }
>  
> +sub content_security_policy {
> +    my ($self) = @_;

Change back to 'my $self = shift;' otherwise the script dies with unknown attribute to new().

@@ +114,5 @@
> +        require Bugzilla::CGI::ContentSecurityPolicy;
> +        return $self->{Bugzilla_csp} if $self->{Bugzilla_csp};
> +        my %params = DEFAULT_CSP;
> +        if (@_) {
> +            my %add_params = @_;

# Accept either hash ref or hash as a list
my %add_params = ref $_[0] ? %{ $_[0] } : @_;

@@ +116,5 @@
> +        my %params = DEFAULT_CSP;
> +        if (@_) {
> +            my %add_params = @_;
> +            foreach my $key (keys %add_params) {
> +                if (defined $add_params) {

if (defined $add_params{$key}) {

::: Bugzilla/CGI/ContentSecurityPolicy.pm
@@ +54,5 @@
> +    my $method = 'has_' . $directive;
> +    return $self->$method;
> +}
> +
> +sub headers {

Nit: I liked header_names better as this way makes me think will return a hash of all headers and their values instead of just the names.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:bugzilla/bugzilla.git
   66a6a5c..742fab1  master -> master