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

correct 2 small bugs #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

BertrandThelen
Copy link

@BertrandThelen BertrandThelen commented Aug 31, 2020

  • SSL tests were failing due to qCritical abort in case of certificate error because the for loop expectation was never met by the 2 errors at the same time
  • Http parse error disconnected the signal but did not closed the socket, leaving the client hanging (tested with curl to force a space in the query parameters).
    Returning a 400 Bad Request could be good but I do not know the full implication that this change could have to the code flow.

change the behavior in case of parser error to disconnectFromHost and provide an empty response to the client
Note: We could send a 400 BadRequest instead
Correct a bug that was still displaying error message and aborting in case of certificate error
@msvetkin
Copy link
Contributor

Hi @BertrandThelen,

Thank you for your contribution.
qt-labs/qthttpserver is official mirror of the qt-project.org.
Qt uses gerrit as development platform (https://wiki.qt.io/Setting_up_Gerrit)
Can you send a patch here https://codereview.qt-project.org/q/project:qt-extensions%252Fqthttpserver?
If you are not able send a patch please let me know and I will send a patch by myself.

@BertrandThelen
Copy link
Author

Hi @msvetkin

Finally managed to get everything set up correctly for gerrit. So the pull request has been sent there.
Don't know if I can/must delete this one?

update version
<< error << error.certificate();
if (error.error() == expectedError.error() ||
error.certificate() == expectedError.certificate()) {
expected = true;
Copy link

Choose a reason for hiding this comment

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

I happened to come across this pull request, and I wondered if the or in the logic should be changed into an and, since we're inverting the logic in the original code. Both there error type and the associated error certificate in the expected should match the actual error.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants