Harden our HTTPS requirement

markup fix

A hard requirement on the php_openssl extension

This can be as simple as adding a requirement on ext-openssl in our composer.json.

re #5 I am not sure if Catch meant a "hard" requirement in that way.

If we added ext-openssl our Composer.json it would mean that if distro's composer.json had drupal/automatic_updates as a requirement then the distro could only be used in cases where ext-openssl was available even if some people(or most) would use that distro without wanting to use Automatic Updates or Package manager in any way

But more importantly if HTTPS becomes a requirement for package manager(which I think it will) in core we would NOT want to add it to core composer.json file. Since this would make a requirement for running package manager now a requirement for installing Drupal. We already have a few requirements for package manager that don't stop you from otherwise running Drupal, mostly a writeable codebase and access to the Composer executable.

By hard requirement I meant in hook_requirements() - could be an install error in contrib, then we'd need to figure out in the core https issue whether it stays like that, or becomes a warning etc.

We can't put an openssl requirement in core's composer.json since that would prevent sites installing core at all even if they don't want to use package_manager, so would have to be hook_requirements() there anyway.

So I did some investigating and manual testing and basically our current state is fine but these extra test will give better error messages

1. If a composer.json has "disable-tls": true, then composer config secure-http whether or not is set

   See https://getcomposer.org/doc/06-config.md#disable-tls

   Enabling this will implicitly disable the secure-http option.

2. I manually changed package_manager/tests/fixtures/fake_site/composer.json to have "disable-tls": true, and indeed our kernel tests will fail because of validation error in ComposerSettingsValidator because of 1). this happens in 8.x-2.x and 3.0.x
3. I manually tried making a composer project and use our 8.x-2.7 version and it also failed status checks because of the ComposerSettingsValidator

   If failed if set just
   "disable-tls": true,
   and

   "disable-tls": true,
         "secure-http": true,
   

   Even here secure-http reports as false because disable-tls overrides it as intended.

It was indeed the JsonProcessOutputCallback failing on warning messages.

I have a work around until 📌 Allow JsonProcessOutputCallback and other Composer runner callbacks to gracefully handle deprecated command options Fixed gets in (which i think should be merged first).

I think this looks pretty good generally. There are a few small things I'd like to clean up, but I think this is otherwise quite solid.

The one thing that really needs to be fixed is that we need to explain in a comment that disable-tls and secure-http are linked, because that it not at all obvious, and without that context, the logic is confusing.

Need to merge in 3.0.x with fixes to 📌 Allow JsonProcessOutputCallback and other Composer runner callbacks to gracefully handle deprecated command options Fixed

also this was discussed in the scrum.

Can you please summarize what was said in that meeting on the issue? Thanks! 🙏

#25:

make a follow-up to merge ComposerValidator and ComposerSettingsValidator into a single validator.

📌 Merge ComposerSettingsValidator into ComposerValidator Fixed 👍 — linking.

Only one real question left 🤓

Very little to complain about here, but we do need a few changes (one explanatory comment, one tiny revert, and one optimization).

🚀

This needs an issue summary update before commit. The summary mentions openssl and adding another validator, which was clearly not needed; can we remove all mention of that so that the issue describes what I'm about to commit?

🚢

The issue summary says that #6 and #8 are why the MR doesn't handle the openssl extension, but in #8 I said we should do that in hook_requirements() instead of composer.json, not that we shouldn't do it at all, and I think if we're going to require https, we should check it there. I've opened 📌 Flag a warning during status check if the OpenSSL extension is not enabled Fixed .

👍 Bumped it to critical and added it to the roadmap. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.