search

govCMS / scaffold-tooling

6 stars 17 forks source link

[GOVCMS-6928] Ban outgoing HTTP requests in theme #271

Closed barbun closed 5 months ago

barbun commented 1 year ago

Issue

Currently, outgoing HTTP requests can be made utilising either Drupal::httpClient() or GuzzleHttp\Client. These methods introduce security risks and should be banned.

Proposed solution

Expand on phpstan ruleset (used by shipshape) to disallow GuzzleHttp\Client namespace and Drupal::httpClient() static method.

barbun commented 10 months ago

thanks @steveworley , pushed an update that adds phpstan-extra.neon config file to separate the checks with lower severity.

simesy commented 10 months ago

What is the security risk out of interest, and how would it differ to using Feeds?

barbun commented 10 months ago

What is the security risk out of interest, and how would it differ to using Feeds?

Thanks @simesy

The rationale is that a theme should not be making outgoing HTTP requests.

Using Migrate would adhere to Drupal safeguards and also limit data parsing abilities to avoid e.g. malicious executables from being intentionally or unintentionally pulled from a remote at runtime.

simesy commented 10 months ago

The rationale is that a theme should not be making outgoing HTTP requests.

Welcome to GovCMS lets do weird stuff in the theme!

simesy commented 10 months ago

I don't have any skin in this game but it seems like a sane/safe thing to do as long as no existing SaaS sites are using this method.

barbun commented 10 months ago

I don't have any skin in this game but it seems like a sane/safe thing to do as long as no existing SaaS sites are using this method.

Good pick up :-) Yeah, we are setting severity to low for now, which shouldn't block deployments. I am also doing some testing with the SaaS projects before we merge this.

murraywoodman commented 2 months ago

There is at least one very large GovCMS clients with skin in the game on this one. In many cases Migrate is not a suitable option for data import, and in these cases running a GET request to pull the data is the only way to get the data in. If this avenue is removed, important functionality will break. This is therefore not a trivial proposal. The security threat should be better explained and clients who are relying on this should be notified. Alternative options for import should be provided.