A couple thoughts from code review

Created on 4 April 2024, 3 months ago

Reviewing the module code, I had a couple thoughts:

Currently the directives which can be altered are filtered by checking the form structure with
if (!isset($form[$policy_type_key]['directives'][$child]['options']['sources']))

It's a small change, but by instead checking the schema of the directive, it wouldn't be added to frame-ancestors where it's not needed.
if (Csp::getDirectiveSchema($child) !== Csp::DIRECTIVE_SCHEMA_SOURCE_LIST)

However, the documentation for GA4 only indicates that the domains are required for img-src and connect-src when using the relevant features. If there aren't other Google libraries that require adding the domains to other directives (or in different configurations depending on the features used), it makes sense to me to have a single option that applies on the policy level that always adds the domains to both img-src and connect-src when enabled - rather than an option on every directive, many of which it will never be needed.
With this module being so targeted in its functionality, I think even just having the module enabled is enough indication that the policies should be altered, so enabling it per policy (or per directive) isn't even needed.

----

      $directive = $policy->getDirective($key);
      foreach ($supported_domains as $supported_domain) {
        $source = '*.' . $supported_domain;
        if (!in_array($source, $directive)) {
          $directive[] = $source;
          $directive[] = $supported_domain;
        }
        if (!in_array($supported_domain, $directive)) {
          $directive[] = $supported_domain;
        }
      }
      $policy->setDirective($key, $directive);

The code adds both the top level domain and a wildcard for its subdomain, but only the wildcard for subdomains should be added. When the policy is rendered, any duplicate values are removed, so it's also fine to just call appendDirective() with the new values.

  $policy->appendDirective(
    $key, 
    array_map(function ($domain) {
      return 'https://*.' . $domain;
    }, $supported_domains)
  );

The documentation indicates that some other functionality may require the google.com TLD be added without a wildcard, but not for the feature that requires country code TLDs.

πŸ“Œ Task
Status

Active

Version

1.2

Component

Code

Created by

πŸ‡¨πŸ‡¦Canada gapple

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Production build 0.69.0 2024