Skip to content

Conversation

@urosg80
Copy link
Contributor

@urosg80 urosg80 commented Feb 17, 2025

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Two notes. Not sure how @yhirose is going to feel about changing public API.

Comment on lines 438 to 442
enum SSLVerifierResponse {
Verified, // connection certificate is verified and accepted
CheckAgain, // use the built-in certificate checker again
Declined // connection certificate was process but is declined
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Might I suggest different names:

enum SSLVerifierResponse {
  NoDecisionMade,        // no decision has been made, use built-in certificate verifier
  CertificateAccepted,   // connection certificate is verified and accepted
  CertificateRejected    // connection certificate was processed but is rejected
};

Also, order NoDecisionMade to the top. It makes more sense to be the default enum value and is certainly less dangerous than CertificateAccepted.

Just two cents from a random contributor...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proposed change I think is not bad, will update PR.

But since I first posted #2061 and asked if I should publish PR and got an "OK" from @yhirose I think it should be OK to be proposing public API change, although not a major one IMHO; final decision is still his.

httplib.h Outdated
Comment on lines 9635 to 9636
if (server_certificate_verifier_)
verificationStatus = server_certificate_verifier_(ssl2);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code base uses curlies even around single statements. Also, please run clang-format. This PR would fail #2062.

httplib.h Outdated
}

if (server_certificate_verification_) {
SSLVerifierResponse verification_status_ =
Copy link
Owner

Choose a reason for hiding this comment

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

Could you change it to auto verification_status?

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

Copy link
Owner

Choose a reason for hiding this comment

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

I meant two things, not only auto, but also verification_status (without the trailing _).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, missed that; fixed

@yhirose
Copy link
Owner

yhirose commented Feb 17, 2025

@urosg80 the code looks good. Thanks for your contribution!

@yhirose yhirose merged commit cdc2230 into yhirose:master Feb 17, 2025
5 of 6 checks passed
@urosg80
Copy link
Contributor Author

urosg80 commented Feb 17, 2025

@urosg80 the code looks good. Thanks for your contribution!

Great and thank you for the great library!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants