Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix setup ssl check #15147

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Fix setup ssl check #15147

merged 3 commits into from
Jul 23, 2024

Conversation

uberbrady
Copy link
Collaborator

We have a customer who is doing a setup in a complex environment, and while their TLS certificates are valid relative to their internal web browsers, they are not valid relative to the Snipe-IT instance, itself - it doesn't know about the certificates. And it really shouldn't need to, either.

And the one (that I know of) place where we actually try to fetch pages from our own instance is when we check for the .env file to see if it's exposed. And instead of failing 'nicely' when the certificate is 'bad', it actually 500's.

This just turns off the TLS certificate checking during that check. I also added a test stub but I wasn't able to figure out how to fill it out, so I marked it as Incomplete. As another precaution, I also decided to widen the exception handler to get all Exceptions, and not just HttpClientException - which, according to PHPStorm, never gets thrown anywhere anyways.

For tests, as per @jerm 's recommendation, I used BadSSL's "self-signed" URL - https://self-signed.badssl.com. I had to do some nasty hard-coding to get the test to work, but when I did that, on develop, that certificate check did cause the 500. On my branch, it did not. So I'm pretty sure this fixes the problem.

Copy link

what-the-diff bot commented Jul 23, 2024

PR Summary

  • Simplification of Code in SettingsController.php
    Some irrelevant code was removed and an update has been made to enhance security and performance. dotEnvFileIsExposed() method now uses a more reliable approach for making HTTP requests.

  • Addition and Update in ShowSetUpPageTest.php
    A new test case testInvalidTLSCertsOkWhenCheckingForEnvFile() was added to ensure our software behaves correctly even with invalid TLS certificates. This new test case is currently in development and will soon be fully functional.

@snipe snipe merged commit 09e2c0b into snipe:develop Jul 23, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants