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

feat(core): add option to ignore SSL errors #1521

Merged
merged 7 commits into from
Aug 6, 2023

Conversation

DeckerSU
Copy link
Contributor

@DeckerSU DeckerSU commented Aug 4, 2023

This pull request adds network settings to ignore SSL errors in order to handle the cases described here:

When the SSL certificate on the server is expired or invalid, users can still download from that server by choosing to ignore SSL errors.

If the project maintainers find this feature unacceptable, I apologize.

image

https://forum.qt.io/topic/125873/how-to-clear-qnetworkreply-ignoresslerrors

Otherwise, if you try to disable the ignoring of SSL errors,
the new ignore settings will not take immediate effect due
to the keep-alive connection with the server.
Copy link
Member

@trollixx trollixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing! Even ignoring the certificate expiration problem from the last week, I think this is a good workaround for all the SSL related issues reported over the years.

I have a few minor suggestions, please see inline.

src/libs/ui/settingsdialog.ui Outdated Show resolved Hide resolved
src/libs/ui/settingsdialog.ui Outdated Show resolved Hide resolved
Comment on lines 337 to 340
if (settings->isIgnoreSSLErrorsEnabled != ui->ignoreSSLErrorsCheckBox->isChecked()) {
// https://forum.qt.io/topic/125873/how-to-clear-qnetworkreply-ignoresslerrors/2
m_application->networkManager()->clearAccessCache();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already done in Core::Application::applySettings, but needs to be moved outside of proxy selection switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - c3be7f4 .

Comment on lines 679 to 685
if (m_application->settings()->isIgnoreSSLErrorsEnabled) {
// Connect to the reply's sslErrors signal to handle SSL errors
connect(reply, &QNetworkReply::sslErrors, [=](const QList<QSslError>& errors){
// Ignore all SSL errors
reply->ignoreSslErrors();
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signal should be handled on Core::NetworkAccessManager to cover all network activity performed by Zeal.

The easiest approach would probably be handling the signal in Core::Application, when network manager is created. Not the most elegant solution, but would avoid the need to pass settings into Core::NetworkAccessManager.

Copy link
Contributor Author

@DeckerSU DeckerSU Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You meant like this - b4e2c86 ? If so - done.

@trollixx trollixx linked an issue Aug 6, 2023 that may be closed by this pull request
@trollixx trollixx mentioned this pull request Aug 6, 2023
Copy link
Member

@trollixx trollixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround.

@trollixx trollixx changed the title Feature: ignore ssl errors network settings feat(core): add option to ignore SSL errors Aug 6, 2023
@trollixx trollixx enabled auto-merge (squash) August 6, 2023 17:59
@trollixx trollixx disabled auto-merge August 6, 2023 17:59
@trollixx trollixx merged commit 9eb6169 into zealdocs:main Aug 6, 2023
11 checks passed
@DeckerSU
Copy link
Contributor Author

DeckerSU commented Aug 6, 2023

Thanks for the quick turnaround.

Not at all, happy to assist! Thank you for providing those suggestions and explanations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

SSL Handshake Failed Downloading a docset gives "SSL handshake failed." Custom feeds won't work.
2 participants